| Message ID | 1561555938-21595-1-git-send-email-robert.chiras@nxp.com |
|---|---|
| Headers | show |
| Series | Improvements and fixes for mxsfb DRM driver | expand |
Hi Robert, On Wed, Jun 26, 2019 at 04:32:08PM +0300, Robert Chiras wrote: > This patch-set improves the use of eLCDIF block on iMX 8 SoCs (like 8MQ, 8MM > and 8QXP). Following, are the new features added and fixes from this > patch-set: > > 1. Add support for drm_bridge > On 8MQ and 8MM, the LCDIF block is not directly connected to a parallel > display connector, where an LCD panel can be attached, but instead it is > connected to DSI controller. Since this DSI stands between the display > controller (eLCDIF) and the physical connector, the DSI can be implemented > as a DRM bridge. So, in order to be able to connect the mxsfb driver to > the DSI driver, the support for a drm_bridge was needed in mxsfb DRM > driver (the actual driver for the eLCDIF block). So I wanted to test this but with both my somewhat cleaned up nwl driver¹ and the nwl driver forward ported from the nxp vendor tree I'm looking at a black screen with current mainline - while my dcss forward port gives me nice output on mipi dsi. Do you have a tree that uses mipi dsi on imx8mq where I could look at to check for differences? Cheers, -- Guido > > 2. Add support for additional pixel formats > Some of the pixel formats needed by Android were not implemented in this > driver, but they were actually supported. So, add support for them. > > 3. Add support for horizontal stride > Having support for horizontal stride allows the use of eLCDIF with a GPU > (for example) that can only output resolution sizes multiple of a power of > 8. For example, 1080 is not a power of 16, so in order to support 1920x1080 > output from GPUs that can produce linear buffers only in sizes multiple to 16, > this feature is needed. > > 3. Few minor features and bug-fixing > The addition of max-res DT property was actually needed in order to limit > the bandwidth usage of the eLCDIF block. This is need on systems where > multiple display controllers are presend and the memory bandwidth is not > enough to handle all of them at maximum capacity (like it is the case on > 8MQ, where there are two display controllers: DCSS and eLCDIF). > The rest of the patches are bug-fixes. > > Mirela Rabulea (1): > drm/mxsfb: Signal mode changed when bpp changed > > Robert Chiras (9): > drm/mxsfb: Update mxsfb to support a bridge > drm/mxsfb: Update mxsfb with additional pixel formats > drm/mxsfb: Fix the vblank events > dt-bindings: display: Add max-res property for mxsfb > drm/mxsfb: Add max-res property for MXSFB > drm/mxsfb: Update mxsfb to support LCD reset > drm/mxsfb: Improve the axi clock usage > drm/mxsfb: Clear OUTSTANDING_REQS bits > drm/mxsfb: Add support for horizontal stride > > .../devicetree/bindings/display/mxsfb.txt | 6 + > drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 290 ++++++++++++++++++--- > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 189 +++++++++++--- > drivers/gpu/drm/mxsfb/mxsfb_drv.h | 10 +- > drivers/gpu/drm/mxsfb/mxsfb_out.c | 26 +- > drivers/gpu/drm/mxsfb/mxsfb_regs.h | 128 ++++++--- > 6 files changed, 531 insertions(+), 118 deletions(-) > > -- > 2.7.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ¹ https://lists.freedesktop.org/archives/dri-devel/2019-March/209685.html
Hi Guido, On Jo, 2019-07-11 at 17:04 +0200, Guido Günther wrote: > Hi Robert, > On Wed, Jun 26, 2019 at 04:32:08PM +0300, Robert Chiras wrote: > > > > This patch-set improves the use of eLCDIF block on iMX 8 SoCs (like > > 8MQ, 8MM > > and 8QXP). Following, are the new features added and fixes from > > this > > patch-set: > > > > 1. Add support for drm_bridge > > On 8MQ and 8MM, the LCDIF block is not directly connected to a > > parallel > > display connector, where an LCD panel can be attached, but instead > > it is > > connected to DSI controller. Since this DSI stands between the > > display > > controller (eLCDIF) and the physical connector, the DSI can be > > implemented > > as a DRM bridge. So, in order to be able to connect the mxsfb > > driver to > > the DSI driver, the support for a drm_bridge was needed in mxsfb > > DRM > > driver (the actual driver for the eLCDIF block). > So I wanted to test this but with both my somewhat cleaned up nwl > driver¹ and the nwl driver forward ported from the nxp vendor tree > I'm > looking at a black screen with current mainline - while my dcss > forward > port gives me nice output on mipi dsi. Do you have a tree that uses > mipi > dsi on imx8mq where I could look at to check for differences? Somewhere on the pixel path (between the display controller and the DSI) there is a block that inverts the polarity. I can't remember exactly what was the role of this block, but the polarity is inverted when eLCDIF is used in combination with the DSI. If you take a look at my DSI driver from NXP releases (I guess you have them), you will see there is a hack in mode_fixup: unsigned int *flags = &mode->flags; if (dsi->sync_pol { *flags |= DRM_MODE_FLAG_PHSYNC; *flags |= DRM_MODE_FLAG_PVSYNC; *flags &= ~DRM_MODE_FLAG_NHSYNC; *flags &= ~DRM_MODE_FLAG_NVSYNC; } else { *flags &= ~DRM_MODE_FLAG_PHSYNC; *flags &= ~DRM_MODE_FLAG_PVSYNC; *flags |= DRM_MODE_FLAG_NHSYNC; *flags |= DRM_MODE_FLAG_NVSYNC; } I know it's not clean, but it works for now. You can try this in your driver and see if it helps. These days I will also take your nwl-dsi driver and test it, and also add support for bridge and eLCDIF to see if I can make it work. Best regards, Robert > > Cheers, > -- Guido > > > > > > > 2. Add support for additional pixel formats > > Some of the pixel formats needed by Android were not implemented in > > this > > driver, but they were actually supported. So, add support for them. > > > > 3. Add support for horizontal stride > > Having support for horizontal stride allows the use of eLCDIF with > > a GPU > > (for example) that can only output resolution sizes multiple of a > > power of > > 8. For example, 1080 is not a power of 16, so in order to support > > 1920x1080 > > output from GPUs that can produce linear buffers only in sizes > > multiple to 16, > > this feature is needed. > > > > 3. Few minor features and bug-fixing > > The addition of max-res DT property was actually needed in order to > > limit > > the bandwidth usage of the eLCDIF block. This is need on systems > > where > > multiple display controllers are presend and the memory bandwidth > > is not > > enough to handle all of them at maximum capacity (like it is the > > case on > > 8MQ, where there are two display controllers: DCSS and eLCDIF). > > The rest of the patches are bug-fixes. > > > > Mirela Rabulea (1): > > drm/mxsfb: Signal mode changed when bpp changed > > > > Robert Chiras (9): > > drm/mxsfb: Update mxsfb to support a bridge > > drm/mxsfb: Update mxsfb with additional pixel formats > > drm/mxsfb: Fix the vblank events > > dt-bindings: display: Add max-res property for mxsfb > > drm/mxsfb: Add max-res property for MXSFB > > drm/mxsfb: Update mxsfb to support LCD reset > > drm/mxsfb: Improve the axi clock usage > > drm/mxsfb: Clear OUTSTANDING_REQS bits > > drm/mxsfb: Add support for horizontal stride > > > > .../devicetree/bindings/display/mxsfb.txt | 6 + > > drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 290 > > ++++++++++++++++++--- > > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 189 > > +++++++++++--- > > drivers/gpu/drm/mxsfb/mxsfb_drv.h | 10 +- > > drivers/gpu/drm/mxsfb/mxsfb_out.c | 26 +- > > drivers/gpu/drm/mxsfb/mxsfb_regs.h | 128 ++++++--- > > 6 files changed, 531 insertions(+), 118 deletions(-) > > > > -- > > 2.7.4 > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fli > > sts.infradead.org%2Fmailman%2Flistinfo%2Flinux-arm- > > kernel&data=02%7C01%7Crobert.chiras%40nxp.com%7C7dc01a0bdf9245b > > 8d87008d70611055b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6369 > > 84542481903425&sdata=ySInO6H1B4kJtJUwRs2uTIUve0SSNZF0s%2Bv%2FDU > > 0Vy1E%3D&reserved=0 > > > ¹ https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fl > ists.freedesktop.org%2Farchives%2Fdri-devel%2F2019- > March%2F209685.html&data=02%7C01%7Crobert.chiras%40nxp.com%7C7dc0 > 1a0bdf9245b8d87008d70611055b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7 > C0%7C636984542481913416&sdata=ucYDQLiK7RalRF%2B5MeB3%2F76cFLGWa7C > mxCFLEg4Wvqc%3D&reserved=0
Hi Robert, On Fri, Jul 12, 2019 at 08:15:32AM +0000, Robert Chiras wrote: > Hi Guido, > > On Jo, 2019-07-11 at 17:04 +0200, Guido Günther wrote: > > Hi Robert, > > On Wed, Jun 26, 2019 at 04:32:08PM +0300, Robert Chiras wrote: > > > > > > This patch-set improves the use of eLCDIF block on iMX 8 SoCs (like > > > 8MQ, 8MM > > > and 8QXP). Following, are the new features added and fixes from > > > this > > > patch-set: > > > > > > 1. Add support for drm_bridge > > > On 8MQ and 8MM, the LCDIF block is not directly connected to a > > > parallel > > > display connector, where an LCD panel can be attached, but instead > > > it is > > > connected to DSI controller. Since this DSI stands between the > > > display > > > controller (eLCDIF) and the physical connector, the DSI can be > > > implemented > > > as a DRM bridge. So, in order to be able to connect the mxsfb > > > driver to > > > the DSI driver, the support for a drm_bridge was needed in mxsfb > > > DRM > > > driver (the actual driver for the eLCDIF block). > > So I wanted to test this but with both my somewhat cleaned up nwl > > driver¹ and the nwl driver forward ported from the nxp vendor tree > > I'm > > looking at a black screen with current mainline - while my dcss > > forward > > port gives me nice output on mipi dsi. Do you have a tree that uses > > mipi > > dsi on imx8mq where I could look at to check for differences? > Somewhere on the pixel path (between the display controller and the > DSI) there is a block that inverts the polarity. I can't remember > exactly what was the role of this block, but the polarity is inverted > when eLCDIF is used in combination with the DSI. > If you take a look at my DSI driver from NXP releases (I guess you have > them), you will see there is a hack in mode_fixup: > > unsigned int *flags = &mode->flags; > if (dsi->sync_pol { > *flags |= DRM_MODE_FLAG_PHSYNC; > *flags |= DRM_MODE_FLAG_PVSYNC; > *flags &= ~DRM_MODE_FLAG_NHSYNC; > *flags &= ~DRM_MODE_FLAG_NVSYNC; > } else { > *flags &= ~DRM_MODE_FLAG_PHSYNC; > *flags &= ~DRM_MODE_FLAG_PVSYNC; > *flags |= DRM_MODE_FLAG_NHSYNC; > *flags |= DRM_MODE_FLAG_NVSYNC; > } Thanks for the suggestion! I'll try that. > > I know it's not clean, but it works for now. You can try this in your > driver and see if it helps. > These days I will also take your nwl-dsi driver and test it, and also > add support for bridge and eLCDIF to see if I can make it work. I have hacky bridge support over here already. Give me some days to clean it up and it might safe you some work. Cheers, -- Guido
Hi Robert, On Tue, Jul 16, 2019 at 04:54:50PM +0200, Guido Günther wrote: > Hi Robert, > On Fri, Jul 12, 2019 at 08:15:32AM +0000, Robert Chiras wrote: > > Hi Guido, > > > > On Jo, 2019-07-11 at 17:04 +0200, Guido Günther wrote: > > > Hi Robert, > > > On Wed, Jun 26, 2019 at 04:32:08PM +0300, Robert Chiras wrote: > > > > > > > > This patch-set improves the use of eLCDIF block on iMX 8 SoCs (like > > > > 8MQ, 8MM > > > > and 8QXP). Following, are the new features added and fixes from > > > > this > > > > patch-set: > > > > > > > > 1. Add support for drm_bridge > > > > On 8MQ and 8MM, the LCDIF block is not directly connected to a > > > > parallel > > > > display connector, where an LCD panel can be attached, but instead > > > > it is > > > > connected to DSI controller. Since this DSI stands between the > > > > display > > > > controller (eLCDIF) and the physical connector, the DSI can be > > > > implemented > > > > as a DRM bridge. So, in order to be able to connect the mxsfb > > > > driver to > > > > the DSI driver, the support for a drm_bridge was needed in mxsfb > > > > DRM > > > > driver (the actual driver for the eLCDIF block). > > > So I wanted to test this but with both my somewhat cleaned up nwl > > > driver¹ and the nwl driver forward ported from the nxp vendor tree > > > I'm > > > looking at a black screen with current mainline - while my dcss > > > forward > > > port gives me nice output on mipi dsi. Do you have a tree that uses > > > mipi > > > dsi on imx8mq where I could look at to check for differences? > > Somewhere on the pixel path (between the display controller and the > > DSI) there is a block that inverts the polarity. I can't remember > > exactly what was the role of this block, but the polarity is inverted > > when eLCDIF is used in combination with the DSI. > > If you take a look at my DSI driver from NXP releases (I guess you have > > them), you will see there is a hack in mode_fixup: > > > > unsigned int *flags = &mode->flags; > > if (dsi->sync_pol { > > *flags |= DRM_MODE_FLAG_PHSYNC; > > *flags |= DRM_MODE_FLAG_PVSYNC; > > *flags &= ~DRM_MODE_FLAG_NHSYNC; > > *flags &= ~DRM_MODE_FLAG_NVSYNC; > > } else { > > *flags &= ~DRM_MODE_FLAG_PHSYNC; > > *flags &= ~DRM_MODE_FLAG_PVSYNC; > > *flags |= DRM_MODE_FLAG_NHSYNC; > > *flags |= DRM_MODE_FLAG_NVSYNC; > > } > > Thanks for the suggestion! I'll try that. > > > > > I know it's not clean, but it works for now. You can try this in your > > driver and see if it helps. > > These days I will also take your nwl-dsi driver and test it, and also > > add support for bridge and eLCDIF to see if I can make it work. > > I have hacky bridge support over here already. Give me some days to > clean it up and it might safe you some work. Your suggestion above (plus some other fixes) worked and mxsfb+nwl+mixel-dphy works over here. I'll try to send a v1 of the nwl driver out during the week. Cheers, -- Guido > Cheers, > -- Guido > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
Hi Robert, On Wed, Jun 26, 2019 at 04:32:09PM +0300, Robert Chiras wrote: > Currently, the MXSFB DRM driver only supports a panel. But, its output > display signal can also be redirected to another encoder, like a DSI > controller. In this case, that DSI controller may act like a drm_bridge. > In order support this use-case too, this patch adds support for > drm_bridge in mxsfb. > > Signed-off-by: Robert Chiras <robert.chiras@nxp.com> > --- > drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 46 +++++++++++++++++++++++++++++++++++--- > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 46 +++++++++++++++++++++++++++++++++----- > drivers/gpu/drm/mxsfb/mxsfb_drv.h | 4 +++- > drivers/gpu/drm/mxsfb/mxsfb_out.c | 26 +++++++++++---------- > drivers/gpu/drm/mxsfb/mxsfb_regs.h | 15 +++++++++++++ > 5 files changed, 116 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > index 93f4133..14bde024 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > @@ -93,8 +93,11 @@ static void mxsfb_set_bus_fmt(struct mxsfb_drm_private *mxsfb) > > reg = readl(mxsfb->base + LCDC_CTRL); > > - if (mxsfb->connector.display_info.num_bus_formats) > - bus_format = mxsfb->connector.display_info.bus_formats[0]; > + if (mxsfb->connector->display_info.num_bus_formats) > + bus_format = mxsfb->connector->display_info.bus_formats[0]; > + > + DRM_DEV_DEBUG_DRIVER(drm->dev, "Using bus_format: 0x%08X\n", > + bus_format); > > reg &= ~CTRL_BUS_WIDTH_MASK; > switch (bus_format) { > @@ -122,6 +125,9 @@ static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) > clk_prepare_enable(mxsfb->clk_disp_axi); > clk_prepare_enable(mxsfb->clk); > > + writel(CTRL2_OUTSTANDING_REQS__REQ_16, > + mxsfb->base + LCDC_V4_CTRL2 + REG_SET); > + > /* If it was disabled, re-enable the mode again */ > writel(CTRL_DOTCLK_MODE, mxsfb->base + LCDC_CTRL + REG_SET); > > @@ -131,12 +137,15 @@ static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) > writel(reg, mxsfb->base + LCDC_VDCTRL4); > > writel(CTRL_RUN, mxsfb->base + LCDC_CTRL + REG_SET); > + writel(CTRL1_RECOVERY_ON_UNDERFLOW, mxsfb->base + LCDC_CTRL1 + REG_SET); > } > > static void mxsfb_disable_controller(struct mxsfb_drm_private *mxsfb) > { > u32 reg; > > + writel(CTRL_RUN, mxsfb->base + LCDC_CTRL + REG_CLR); > + > /* > * Even if we disable the controller here, it will still continue > * until its FIFOs are running out of data > @@ -202,8 +211,9 @@ static dma_addr_t mxsfb_get_fb_paddr(struct mxsfb_drm_private *mxsfb) > > static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb) > { > + struct drm_device *drm = mxsfb->pipe.crtc.dev; > struct drm_display_mode *m = &mxsfb->pipe.crtc.state->adjusted_mode; > - const u32 bus_flags = mxsfb->connector.display_info.bus_flags; > + const u32 bus_flags = mxsfb->connector->display_info.bus_flags; > u32 vdctrl0, vsync_pulse_len, hsync_pulse_len; > int err; > > @@ -227,6 +237,13 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb) > > clk_set_rate(mxsfb->clk, m->crtc_clock * 1000); > > + DRM_DEV_DEBUG_DRIVER(drm->dev, "Pixel clock: %dkHz (actual: %dkHz)\n", > + m->crtc_clock, > + (int)(clk_get_rate(mxsfb->clk) / 1000)); > + DRM_DEV_DEBUG_DRIVER(drm->dev, "Connector bus_flags: 0x%08X\n", > + bus_flags); > + DRM_DEV_DEBUG_DRIVER(drm->dev, "Mode flags: 0x%08X\n", m->flags); > + > writel(TRANSFER_COUNT_SET_VCOUNT(m->crtc_vdisplay) | > TRANSFER_COUNT_SET_HCOUNT(m->crtc_hdisplay), > mxsfb->base + mxsfb->devdata->transfer_count); > @@ -279,6 +296,7 @@ void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb) > dma_addr_t paddr; > > mxsfb_enable_axi_clk(mxsfb); > + writel(0, mxsfb->base + LCDC_CTRL); > mxsfb_crtc_mode_set_nofb(mxsfb); > > /* Write cur_buf as well to avoid an initial corrupt frame */ > @@ -302,6 +320,8 @@ void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb, > { > struct drm_simple_display_pipe *pipe = &mxsfb->pipe; > struct drm_crtc *crtc = &pipe->crtc; > + struct drm_framebuffer *fb = pipe->plane.state->fb; > + struct drm_framebuffer *old_fb = old_state->fb; > struct drm_pending_vblank_event *event; > dma_addr_t paddr; > > @@ -324,4 +344,24 @@ void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb, > writel(paddr, mxsfb->base + mxsfb->devdata->next_buf); > mxsfb_disable_axi_clk(mxsfb); > } > + > + if (!fb || !old_fb) > + return; > + > + /* > + * TODO: Currently, we only support pixel format change, but we need > + * also to care about size changes too > + */ > + if (old_fb->format->format != fb->format->format) { > + struct drm_format_name_buf old_fmt_buf; > + struct drm_format_name_buf new_fmt_buf; > + > + DRM_DEV_DEBUG_DRIVER(crtc->dev->dev, > + "Switching pixel format: %s -> %s\n", > + drm_get_format_name(old_fb->format->format, > + &old_fmt_buf), > + drm_get_format_name(fb->format->format, > + &new_fmt_buf)); > + mxsfb_set_pixel_fmt(mxsfb, true); This assumes mxsfb_set_pixel_fmt has two arguments which is being introduced in the following commit. With that fixed: Tested-by: Guido Günther <agx@sigxcpu.org> Cheers, -- Guido > + } > } > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > index 6fafc90..0d171e9 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > @@ -98,9 +98,25 @@ static void mxsfb_pipe_enable(struct drm_simple_display_pipe *pipe, > struct drm_crtc_state *crtc_state, > struct drm_plane_state *plane_state) > { > + struct drm_connector *connector; > struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe); > struct drm_device *drm = pipe->plane.dev; > > + if (!mxsfb->connector) { > + list_for_each_entry(connector, > + &drm->mode_config.connector_list, > + head) > + if (connector->encoder == &mxsfb->pipe.encoder) { > + mxsfb->connector = connector; > + break; > + } > + } > + > + if (!mxsfb->connector) { > + dev_warn(drm->dev, "No connector attached, using default\n"); > + mxsfb->connector = &mxsfb->panel_connector; > + } > + > pm_runtime_get_sync(drm->dev); > drm_panel_prepare(mxsfb->panel); > mxsfb_crtc_enable(mxsfb); > @@ -126,6 +142,9 @@ static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) > drm_crtc_send_vblank_event(crtc, event); > } > spin_unlock_irq(&drm->event_lock); > + > + if (mxsfb->connector != &mxsfb->panel_connector) > + mxsfb->connector = NULL; > } > > static void mxsfb_pipe_update(struct drm_simple_display_pipe *pipe, > @@ -223,16 +242,33 @@ static int mxsfb_load(struct drm_device *drm, unsigned long flags) > > ret = drm_simple_display_pipe_init(drm, &mxsfb->pipe, &mxsfb_funcs, > mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, > - &mxsfb->connector); > + mxsfb->connector); > if (ret < 0) { > dev_err(drm->dev, "Cannot setup simple display pipe\n"); > goto err_vblank; > } > > - ret = drm_panel_attach(mxsfb->panel, &mxsfb->connector); > - if (ret) { > - dev_err(drm->dev, "Cannot connect panel\n"); > - goto err_vblank; > + /* > + * Attach panel only if there is one. > + * If there is no panel attach, it must be a bridge. In this case, we > + * need a reference to its connector for a proper initialization. > + * We will do this check in pipe->enable(), since the connector won't > + * be attached to an encoder until then. > + */ > + > + if (mxsfb->panel) { > + ret = drm_panel_attach(mxsfb->panel, mxsfb->connector); > + if (ret) { > + dev_err(drm->dev, "Cannot connect panel\n"); > + goto err_vblank; > + } > + } else if (mxsfb->bridge) { > + ret = drm_simple_display_pipe_attach_bridge(&mxsfb->pipe, > + mxsfb->bridge); > + if (ret) { > + dev_err(drm->dev, "Cannot connect bridge\n"); > + goto err_vblank; > + } > } > > drm->mode_config.min_width = MXSFB_MIN_XRES; > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > index d975300..0b65b51 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > @@ -27,8 +27,10 @@ struct mxsfb_drm_private { > struct clk *clk_disp_axi; > > struct drm_simple_display_pipe pipe; > - struct drm_connector connector; > + struct drm_connector panel_connector; > + struct drm_connector *connector; > struct drm_panel *panel; > + struct drm_bridge *bridge; > }; > > int mxsfb_setup_crtc(struct drm_device *dev); > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_out.c b/drivers/gpu/drm/mxsfb/mxsfb_out.c > index 91e76f9..b9acf2b 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_out.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_out.c > @@ -22,7 +22,8 @@ > static struct mxsfb_drm_private * > drm_connector_to_mxsfb_drm_private(struct drm_connector *connector) > { > - return container_of(connector, struct mxsfb_drm_private, connector); > + return container_of(connector, struct mxsfb_drm_private, > + panel_connector); > } > > static int mxsfb_panel_get_modes(struct drm_connector *connector) > @@ -77,22 +78,23 @@ static const struct drm_connector_funcs mxsfb_panel_connector_funcs = { > int mxsfb_create_output(struct drm_device *drm) > { > struct mxsfb_drm_private *mxsfb = drm->dev_private; > - struct drm_panel *panel; > int ret; > > - ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, &panel, NULL); > + ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, > + &mxsfb->panel, &mxsfb->bridge); > if (ret) > return ret; > > - mxsfb->connector.dpms = DRM_MODE_DPMS_OFF; > - mxsfb->connector.polled = 0; > - drm_connector_helper_add(&mxsfb->connector, > - &mxsfb_panel_connector_helper_funcs); > - ret = drm_connector_init(drm, &mxsfb->connector, > - &mxsfb_panel_connector_funcs, > - DRM_MODE_CONNECTOR_Unknown); > - if (!ret) > - mxsfb->panel = panel; > + if (mxsfb->panel) { > + mxsfb->connector = &mxsfb->panel_connector; > + mxsfb->connector->dpms = DRM_MODE_DPMS_OFF; > + mxsfb->connector->polled = 0; > + drm_connector_helper_add(mxsfb->connector, > + &mxsfb_panel_connector_helper_funcs); > + ret = drm_connector_init(drm, mxsfb->connector, > + &mxsfb_panel_connector_funcs, > + DRM_MODE_CONNECTOR_Unknown); > + } > > return ret; > } > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_regs.h b/drivers/gpu/drm/mxsfb/mxsfb_regs.h > index 932d7ea..71426aa 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_regs.h > +++ b/drivers/gpu/drm/mxsfb/mxsfb_regs.h > @@ -14,19 +14,31 @@ > > #define LCDC_CTRL 0x00 > #define LCDC_CTRL1 0x10 > +#define LCDC_V4_CTRL2 0x20 > #define LCDC_V3_TRANSFER_COUNT 0x20 > #define LCDC_V4_TRANSFER_COUNT 0x30 > #define LCDC_V4_CUR_BUF 0x40 > #define LCDC_V4_NEXT_BUF 0x50 > #define LCDC_V3_CUR_BUF 0x30 > #define LCDC_V3_NEXT_BUF 0x40 > +#define LCDC_TIMING 0x60 > #define LCDC_VDCTRL0 0x70 > #define LCDC_VDCTRL1 0x80 > #define LCDC_VDCTRL2 0x90 > #define LCDC_VDCTRL3 0xa0 > #define LCDC_VDCTRL4 0xb0 > +#define LCDC_DVICTRL0 0xc0 > +#define LCDC_DVICTRL1 0xd0 > +#define LCDC_DVICTRL2 0xe0 > +#define LCDC_DVICTRL3 0xf0 > +#define LCDC_DVICTRL4 0x100 > +#define LCDC_V4_DATA 0x180 > +#define LCDC_V3_DATA 0x1b0 > #define LCDC_V4_DEBUG0 0x1d0 > #define LCDC_V3_DEBUG0 0x1f0 > +#define LCDC_AS_CTRL 0x210 > +#define LCDC_AS_BUF 0x220 > +#define LCDC_AS_NEXT_BUF 0x230 > > #define CTRL_SFTRST (1 << 31) > #define CTRL_CLKGATE (1 << 30) > @@ -45,12 +57,15 @@ > #define CTRL_DF24 (1 << 1) > #define CTRL_RUN (1 << 0) > > +#define CTRL1_RECOVERY_ON_UNDERFLOW (1 << 24) > #define CTRL1_FIFO_CLEAR (1 << 21) > #define CTRL1_SET_BYTE_PACKAGING(x) (((x) & 0xf) << 16) > #define CTRL1_GET_BYTE_PACKAGING(x) (((x) >> 16) & 0xf) > #define CTRL1_CUR_FRAME_DONE_IRQ_EN (1 << 13) > #define CTRL1_CUR_FRAME_DONE_IRQ (1 << 9) > > +#define CTRL2_OUTSTANDING_REQS__REQ_16 (4 << 21) > + > #define TRANSFER_COUNT_SET_VCOUNT(x) (((x) & 0xffff) << 16) > #define TRANSFER_COUNT_GET_VCOUNT(x) (((x) >> 16) & 0xffff) > #define TRANSFER_COUNT_SET_HCOUNT(x) ((x) & 0xffff) > -- > 2.7.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
Hi, I'm not very familiar with mxsfb, just some things that stood out while looking at the code: On Wed, Jun 26, 2019 at 04:32:10PM +0300, Robert Chiras wrote: > Since version 4 of eLCDIF, there are some registers that can do > transformations on the input data, like re-arranging the pixel > components. By doing that, we can support more pixel formats. > This patch adds support for X/ABGR and RGBX/A. Although, the local alpha > is not supported by eLCDIF, the alpha pixel formats were added to the > supported pixel formats but it will be ignored. This was necessary since > there are systems (like Android) that requires such pixel formats. > > Also, add support for the following pixel formats: > 16 bpp: RG16 ,BG16, XR15, XB15, AR15, AB15 > Set the bus format based on input from the user and panel capabilities. > Save the bus format in crtc->mode.private_flags, so the bridge can use > it. > > Signed-off-by: Robert Chiras <robert.chiras@nxp.com> > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > --- > drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 158 ++++++++++++++++++++++++++++++------- > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 30 +++++-- > drivers/gpu/drm/mxsfb/mxsfb_drv.h | 3 +- > drivers/gpu/drm/mxsfb/mxsfb_regs.h | 100 ++++++++++++++--------- > 4 files changed, 217 insertions(+), 74 deletions(-) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > index 14bde024..e48396d 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > @@ -41,14 +41,17 @@ static u32 set_hsync_pulse_width(struct mxsfb_drm_private *mxsfb, u32 val) > } > > /* Setup the MXSFB registers for decoding the pixels out of the framebuffer */ > -static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb) > +static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb, bool update) > { > struct drm_crtc *crtc = &mxsfb->pipe.crtc; > struct drm_device *drm = crtc->dev; > const u32 format = crtc->primary->state->fb->format->format; > - u32 ctrl, ctrl1; > + u32 ctrl = 0, ctrl1 = 0; > + bool bgr_format = true; > + struct drm_format_name_buf format_name_buf; > > - ctrl = CTRL_BYPASS_COUNT | CTRL_MASTER; > + if (!update) > + ctrl = CTRL_BYPASS_COUNT | CTRL_MASTER; > > /* > * WARNING: The bus width, CTRL_SET_BUS_WIDTH(), is configured to > @@ -57,64 +60,158 @@ static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb) > * to arbitrary value. This limitation should not pose an issue. > */ > > - /* CTRL1 contains IRQ config and status bits, preserve those. */ > - ctrl1 = readl(mxsfb->base + LCDC_CTRL1); > - ctrl1 &= CTRL1_CUR_FRAME_DONE_IRQ_EN | CTRL1_CUR_FRAME_DONE_IRQ; > + if (!update) { > + /* CTRL1 contains IRQ config and status bits, preserve those. */ > + ctrl1 = readl(mxsfb->base + LCDC_CTRL1); > + ctrl1 &= CTRL1_CUR_FRAME_DONE_IRQ_EN | CTRL1_CUR_FRAME_DONE_IRQ; > + } > + > + DRM_DEV_DEBUG_DRIVER(drm->dev, "Setting up %s mode\n", > + drm_get_format_name(format, &format_name_buf)); > + > + /* Do some clean-up that we might have from a previous mode */ > + ctrl &= ~CTRL_SHIFT_DIR(1); > + ctrl &= ~CTRL_SHIFT_NUM(0x3f); > + if (mxsfb->devdata->ipversion >= 4) > + writel(CTRL2_ODD_LINE_PATTERN(0x7) | > + CTRL2_EVEN_LINE_PATTERN(0x7), Would it make sense to not use magic constants here but rather '#define' the different line pattern values as well? > + mxsfb->base + LCDC_V4_CTRL2 + REG_CLR); > > switch (format) { > - case DRM_FORMAT_RGB565: > - dev_dbg(drm->dev, "Setting up RGB565 mode\n"); > + case DRM_FORMAT_BGR565: /* BG16 */ > + if (mxsfb->devdata->ipversion < 4) > + goto err; > + writel(CTRL2_ODD_LINE_PATTERN(0x5) | > + CTRL2_EVEN_LINE_PATTERN(0x5), > + mxsfb->base + LCDC_V4_CTRL2 + REG_SET); > + /* Fall through */ > + case DRM_FORMAT_RGB565: /* RG16 */ > + ctrl |= CTRL_SET_WORD_LENGTH(0); > + ctrl &= ~CTRL_DF16; > + ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0xf); > + break; > + case DRM_FORMAT_XBGR1555: /* XB15 */ > + case DRM_FORMAT_ABGR1555: /* AB15 */ > + if (mxsfb->devdata->ipversion < 4) > + goto err; > + writel(CTRL2_ODD_LINE_PATTERN(0x5) | > + CTRL2_EVEN_LINE_PATTERN(0x5), > + mxsfb->base + LCDC_V4_CTRL2 + REG_SET); > + /* Fall through */ > + case DRM_FORMAT_XRGB1555: /* XR15 */ > + case DRM_FORMAT_ARGB1555: /* AR15 */ > ctrl |= CTRL_SET_WORD_LENGTH(0); > + ctrl |= CTRL_DF16; > ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0xf); > break; > - case DRM_FORMAT_XRGB8888: > - dev_dbg(drm->dev, "Setting up XRGB8888 mode\n"); > + case DRM_FORMAT_RGBX8888: /* RX24 */ > + case DRM_FORMAT_RGBA8888: /* RA24 */ > + /* RGBX - > 0RGB */ > + ctrl |= CTRL_SHIFT_DIR(1); > + ctrl |= CTRL_SHIFT_NUM(8); > + bgr_format = false; > + /* Fall through */ > + case DRM_FORMAT_XBGR8888: /* XB24 */ > + case DRM_FORMAT_ABGR8888: /* AB24 */ > + if (bgr_format) { > + if (mxsfb->devdata->ipversion < 4) > + goto err; > + writel(CTRL2_ODD_LINE_PATTERN(0x5) | > + CTRL2_EVEN_LINE_PATTERN(0x5), > + mxsfb->base + LCDC_V4_CTRL2 + REG_SET); > + } > + /* Fall through */ > + case DRM_FORMAT_XRGB8888: /* XR24 */ > + case DRM_FORMAT_ARGB8888: /* AR24 */ > ctrl |= CTRL_SET_WORD_LENGTH(3); > /* Do not use packed pixels = one pixel per word instead. */ > ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0x7); > break; > default: > - dev_err(drm->dev, "Unhandled pixel format %08x\n", format); > - return -EINVAL; > + goto err; > } > > - writel(ctrl1, mxsfb->base + LCDC_CTRL1); > - writel(ctrl, mxsfb->base + LCDC_CTRL); > + if (update) { > + writel(ctrl, mxsfb->base + LCDC_CTRL + REG_SET); > + writel(ctrl1, mxsfb->base + LCDC_CTRL1 + REG_SET); > + } else { > + writel(ctrl, mxsfb->base + LCDC_CTRL); > + writel(ctrl1, mxsfb->base + LCDC_CTRL1); > + } > > return 0; > + > +err: > + DRM_DEV_ERROR(drm->dev, "Unhandled pixel format: %s\n", > + drm_get_format_name(format, &format_name_buf)); > + > + return -EINVAL; > +} > + > +static u32 get_bus_format_from_bpp(u32 bpp) > +{ > + switch (bpp) { > + case 16: > + return MEDIA_BUS_FMT_RGB565_1X16; > + case 18: > + return MEDIA_BUS_FMT_RGB666_1X18; > + case 24: > + return MEDIA_BUS_FMT_RGB888_1X24; > + default: > + return MEDIA_BUS_FMT_RGB888_1X24; > + } > } > > static void mxsfb_set_bus_fmt(struct mxsfb_drm_private *mxsfb) > { > struct drm_crtc *crtc = &mxsfb->pipe.crtc; > + unsigned int bits_per_pixel = crtc->primary->state->fb->format->depth; > struct drm_device *drm = crtc->dev; > u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; > - u32 reg; > - > - reg = readl(mxsfb->base + LCDC_CTRL); > + int num_bus_formats = mxsfb->connector->display_info.num_bus_formats; > + const u32 *bus_formats = mxsfb->connector->display_info.bus_formats; > + u32 reg = 0; > + int i = 0; > + > + /* match the user requested bus_format to one supported by the panel */ > + if (num_bus_formats) { > + u32 user_bus_format = get_bus_format_from_bpp(bits_per_pixel); > + > + bus_format = bus_formats[0]; > + for (i = 0; i < num_bus_formats; i++) { > + if (user_bus_format == bus_formats[i]) { > + bus_format = user_bus_format; > + break; > + } > + } > + } > > - if (mxsfb->connector->display_info.num_bus_formats) > - bus_format = mxsfb->connector->display_info.bus_formats[0]; > + /* > + * CRTC will dictate the bus format via private_flags[16:1] > + * and private_flags[0] will signal a bus format change > + */ > + crtc->mode.private_flags &= ~0x1FFFF; /* clear bus format */ > + crtc->mode.private_flags |= (bus_format << 1); /* set bus format */ > + crtc->mode.private_flags |= 0x1; /* bus format change indication*/ > > DRM_DEV_DEBUG_DRIVER(drm->dev, "Using bus_format: 0x%08X\n", > bus_format); > > - reg &= ~CTRL_BUS_WIDTH_MASK; > switch (bus_format) { > case MEDIA_BUS_FMT_RGB565_1X16: > - reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT); > + reg = CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT); > break; > case MEDIA_BUS_FMT_RGB666_1X18: > - reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_18BIT); > + reg = CTRL_SET_BUS_WIDTH(STMLCDIF_18BIT); > break; > case MEDIA_BUS_FMT_RGB888_1X24: > - reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_24BIT); > + reg = CTRL_SET_BUS_WIDTH(STMLCDIF_24BIT); > break; > default: > dev_err(drm->dev, "Unknown media bus format %d\n", bus_format); > break; > } > - writel(reg, mxsfb->base + LCDC_CTRL); > + writel(reg, mxsfb->base + LCDC_CTRL + REG_SET); > } > > static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) > @@ -125,8 +222,9 @@ static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) > clk_prepare_enable(mxsfb->clk_disp_axi); > clk_prepare_enable(mxsfb->clk); > > - writel(CTRL2_OUTSTANDING_REQS__REQ_16, > - mxsfb->base + LCDC_V4_CTRL2 + REG_SET); > + if (mxsfb->devdata->ipversion >= 4) > + writel(CTRL2_OUTSTANDING_REQS__REQ_16, > + mxsfb->base + LCDC_V4_CTRL2 + REG_SET); > > /* If it was disabled, re-enable the mode again */ > writel(CTRL_DOTCLK_MODE, mxsfb->base + LCDC_CTRL + REG_SET); > @@ -144,6 +242,10 @@ static void mxsfb_disable_controller(struct mxsfb_drm_private *mxsfb) > { > u32 reg; > > + if (mxsfb->devdata->ipversion >= 4) > + writel(CTRL2_OUTSTANDING_REQS(0x7), > + mxsfb->base + LCDC_V4_CTRL2 + REG_CLR); > + > writel(CTRL_RUN, mxsfb->base + LCDC_CTRL + REG_CLR); > > /* > @@ -231,7 +333,7 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb) > /* Clear the FIFOs */ > writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_SET); > > - err = mxsfb_set_pixel_fmt(mxsfb); > + err = mxsfb_set_pixel_fmt(mxsfb, false); > if (err) > return; > > @@ -316,7 +418,7 @@ void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb) > } > > void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb, > - struct drm_plane_state *state) > + struct drm_plane_state *old_state) This hunk belongs to the previous commit. > { > struct drm_simple_display_pipe *pipe = &mxsfb->pipe; > struct drm_crtc *crtc = &pipe->crtc; > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > index 0d171e9..d9fb734 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > @@ -40,6 +40,27 @@ enum mxsfb_devtype { > MXSFB_V4, > }; > > +/* > + * When adding new formats, make sure to update the num_formats from > + * mxsfb_devdata below. > + */ > +static const u32 mxsfb_formats[] = { > + /* MXSFB_V3 */ > + DRM_FORMAT_XRGB8888, > + DRM_FORMAT_ARGB8888, > + DRM_FORMAT_RGB565, > + /* MXSFB_V4 */ > + DRM_FORMAT_XBGR8888, > + DRM_FORMAT_ABGR8888, > + DRM_FORMAT_RGBX8888, > + DRM_FORMAT_RGBA8888, > + DRM_FORMAT_ARGB1555, > + DRM_FORMAT_XRGB1555, > + DRM_FORMAT_ABGR1555, > + DRM_FORMAT_XBGR1555, > + DRM_FORMAT_BGR565 > +}; > + > static const struct mxsfb_devdata mxsfb_devdata[] = { > [MXSFB_V3] = { > .transfer_count = LCDC_V3_TRANSFER_COUNT, > @@ -49,6 +70,7 @@ static const struct mxsfb_devdata mxsfb_devdata[] = { > .hs_wdth_mask = 0xff, > .hs_wdth_shift = 24, > .ipversion = 3, > + .num_formats = 3, > }, > [MXSFB_V4] = { > .transfer_count = LCDC_V4_TRANSFER_COUNT, > @@ -58,14 +80,10 @@ static const struct mxsfb_devdata mxsfb_devdata[] = { > .hs_wdth_mask = 0x3fff, > .hs_wdth_shift = 18, > .ipversion = 4, > + .num_formats = ARRAY_SIZE(mxsfb_formats), > }, > }; > > -static const uint32_t mxsfb_formats[] = { > - DRM_FORMAT_XRGB8888, > - DRM_FORMAT_RGB565 > -}; > - > static struct mxsfb_drm_private * > drm_pipe_to_mxsfb_drm_private(struct drm_simple_display_pipe *pipe) > { > @@ -241,7 +259,7 @@ static int mxsfb_load(struct drm_device *drm, unsigned long flags) > } > > ret = drm_simple_display_pipe_init(drm, &mxsfb->pipe, &mxsfb_funcs, > - mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, > + mxsfb_formats, mxsfb->devdata->num_formats, NULL, > mxsfb->connector); > if (ret < 0) { > dev_err(drm->dev, "Cannot setup simple display pipe\n"); > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > index 0b65b51..8fb65d3 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > @@ -16,6 +16,7 @@ struct mxsfb_devdata { > unsigned int hs_wdth_mask; > unsigned int hs_wdth_shift; > unsigned int ipversion; > + unsigned int num_formats; > }; > > struct mxsfb_drm_private { > @@ -42,6 +43,6 @@ void mxsfb_disable_axi_clk(struct mxsfb_drm_private *mxsfb); > void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb); > void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb); > void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb, > - struct drm_plane_state *state); > + struct drm_plane_state *old_state); > > #endif /* __MXSFB_DRV_H__ */ > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_regs.h b/drivers/gpu/drm/mxsfb/mxsfb_regs.h > index 71426aa..9ee0d3c7 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_regs.h > +++ b/drivers/gpu/drm/mxsfb/mxsfb_regs.h > @@ -40,54 +40,76 @@ > #define LCDC_AS_BUF 0x220 > #define LCDC_AS_NEXT_BUF 0x230 > > -#define CTRL_SFTRST (1 << 31) > -#define CTRL_CLKGATE (1 << 30) > -#define CTRL_BYPASS_COUNT (1 << 19) > -#define CTRL_VSYNC_MODE (1 << 18) > -#define CTRL_DOTCLK_MODE (1 << 17) > -#define CTRL_DATA_SELECT (1 << 16) > -#define CTRL_SET_BUS_WIDTH(x) (((x) & 0x3) << 10) > -#define CTRL_GET_BUS_WIDTH(x) (((x) >> 10) & 0x3) > -#define CTRL_BUS_WIDTH_MASK (0x3 << 10) > -#define CTRL_SET_WORD_LENGTH(x) (((x) & 0x3) << 8) > -#define CTRL_GET_WORD_LENGTH(x) (((x) >> 8) & 0x3) > -#define CTRL_MASTER (1 << 5) > -#define CTRL_DF16 (1 << 3) > -#define CTRL_DF18 (1 << 2) > -#define CTRL_DF24 (1 << 1) > -#define CTRL_RUN (1 << 0) > - > -#define CTRL1_RECOVERY_ON_UNDERFLOW (1 << 24) > -#define CTRL1_FIFO_CLEAR (1 << 21) > -#define CTRL1_SET_BYTE_PACKAGING(x) (((x) & 0xf) << 16) > -#define CTRL1_GET_BYTE_PACKAGING(x) (((x) >> 16) & 0xf) > -#define CTRL1_CUR_FRAME_DONE_IRQ_EN (1 << 13) > -#define CTRL1_CUR_FRAME_DONE_IRQ (1 << 9) > - > -#define CTRL2_OUTSTANDING_REQS__REQ_16 (4 << 21) > +/* reg bit manipulation */ > +#define REG_MASK(e, s) (((1 << ((e) - (s) + 1)) - 1) << (s)) > +#define REG_PUT(x, e, s) (((x) << (s)) & REG_MASK(e, s)) > +#define REG_GET(x, e, s) (((x) & REG_MASK(e, s)) >> (s)) > + > +#define SWIZZLE_LE 0 /* Little-Endian or No swap */ > +#define SWIZZLE_BE 1 /* Big-Endian or swap all */ > +#define SWIZZLE_HWD 2 /* Swap half-words */ > +#define SWIZZLE_HWD_BYTE 3 /* Swap bytes within each half-word */ > + > +#define CTRL_SFTRST BIT(31) > +#define CTRL_CLKGATE BIT(30) > +#define CTRL_SHIFT_DIR(x) REG_PUT((x), 26, 26) > +#define CTRL_SHIFT_NUM(x) REG_PUT((x), 25, 21) > +#define CTRL_BYPASS_COUNT BIT(19) > +#define CTRL_VSYNC_MODE BIT(18) > +#define CTRL_DOTCLK_MODE BIT(17) > +#define CTRL_DATA_SELECT BIT(16) > +#define CTRL_INPUT_SWIZZLE(x) REG_PUT((x), 15, 14) > +#define CTRL_CSC_SWIZZLE(x) REG_PUT((x), 13, 12) > +#define CTRL_SET_BUS_WIDTH(x) REG_PUT((x), 11, 10) > +#define CTRL_GET_BUS_WIDTH(x) REG_GET((x), 11, 10) > +#define CTRL_BUS_WIDTH_MASK REG_PUT((0x3), 11, 10) > +#define CTRL_SET_WORD_LENGTH(x) REG_PUT((x), 9, 8) > +#define CTRL_GET_WORD_LENGTH(x) REG_GET((x), 9, 8) > +#define CTRL_MASTER BIT(5) > +#define CTRL_DF16 BIT(3) > +#define CTRL_DF18 BIT(2) > +#define CTRL_DF24 BIT(1) > +#define CTRL_RUN BIT(0) > + > +#define CTRL1_RECOVERY_ON_UNDERFLOW BIT(24) > +#define CTRL1_FIFO_CLEAR BIT(21) > +#define CTRL1_SET_BYTE_PACKAGING(x) REG_PUT((x), 19, 16) > +#define CTRL1_GET_BYTE_PACKAGING(x) REG_GET((x), 19, 16) > +#define CTRL1_CUR_FRAME_DONE_IRQ_EN BIT(13) > +#define CTRL1_CUR_FRAME_DONE_IRQ BIT(9) Splitting the cleanups (introduction of BIT(x) usage) from new defines would ease reviewing. Cheers, -- Guido > + > +#define REQ_1 0 > +#define REQ_2 1 > +#define REQ_4 2 > +#define REQ_8 3 > +#define REQ_16 4 > + > +#define CTRL2_OUTSTANDING_REQS(x) REG_PUT((x), 23, 21) > +#define CTRL2_ODD_LINE_PATTERN(x) REG_PUT((x), 18, 16) > +#define CTRL2_EVEN_LINE_PATTERN(x) REG_PUT((x), 14, 12) > > #define TRANSFER_COUNT_SET_VCOUNT(x) (((x) & 0xffff) << 16) > #define TRANSFER_COUNT_GET_VCOUNT(x) (((x) >> 16) & 0xffff) > #define TRANSFER_COUNT_SET_HCOUNT(x) ((x) & 0xffff) > #define TRANSFER_COUNT_GET_HCOUNT(x) ((x) & 0xffff) > > -#define VDCTRL0_ENABLE_PRESENT (1 << 28) > -#define VDCTRL0_VSYNC_ACT_HIGH (1 << 27) > -#define VDCTRL0_HSYNC_ACT_HIGH (1 << 26) > -#define VDCTRL0_DOTCLK_ACT_FALLING (1 << 25) > -#define VDCTRL0_ENABLE_ACT_HIGH (1 << 24) > -#define VDCTRL0_VSYNC_PERIOD_UNIT (1 << 21) > -#define VDCTRL0_VSYNC_PULSE_WIDTH_UNIT (1 << 20) > -#define VDCTRL0_HALF_LINE (1 << 19) > -#define VDCTRL0_HALF_LINE_MODE (1 << 18) > +#define VDCTRL0_ENABLE_PRESENT BIT(28) > +#define VDCTRL0_VSYNC_ACT_HIGH BIT(27) > +#define VDCTRL0_HSYNC_ACT_HIGH BIT(26) > +#define VDCTRL0_DOTCLK_ACT_FALLING BIT(25) > +#define VDCTRL0_ENABLE_ACT_HIGH BIT(24) > +#define VDCTRL0_VSYNC_PERIOD_UNIT BIT(21) > +#define VDCTRL0_VSYNC_PULSE_WIDTH_UNIT BIT(20) > +#define VDCTRL0_HALF_LINE BIT(19) > +#define VDCTRL0_HALF_LINE_MODE BIT(18) > #define VDCTRL0_SET_VSYNC_PULSE_WIDTH(x) ((x) & 0x3ffff) > #define VDCTRL0_GET_VSYNC_PULSE_WIDTH(x) ((x) & 0x3ffff) > > #define VDCTRL2_SET_HSYNC_PERIOD(x) ((x) & 0x3ffff) > #define VDCTRL2_GET_HSYNC_PERIOD(x) ((x) & 0x3ffff) > > -#define VDCTRL3_MUX_SYNC_SIGNALS (1 << 29) > -#define VDCTRL3_VSYNC_ONLY (1 << 28) > +#define VDCTRL3_MUX_SYNC_SIGNALS BIT(29) > +#define VDCTRL3_VSYNC_ONLY BIT(28) > #define SET_HOR_WAIT_CNT(x) (((x) & 0xfff) << 16) > #define GET_HOR_WAIT_CNT(x) (((x) >> 16) & 0xfff) > #define SET_VERT_WAIT_CNT(x) ((x) & 0xffff) > @@ -95,7 +117,7 @@ > > #define VDCTRL4_SET_DOTCLK_DLY(x) (((x) & 0x7) << 29) /* v4 only */ > #define VDCTRL4_GET_DOTCLK_DLY(x) (((x) >> 29) & 0x7) /* v4 only */ > -#define VDCTRL4_SYNC_SIGNALS_ON (1 << 18) > +#define VDCTRL4_SYNC_SIGNALS_ON BIT(18) > #define SET_DOTCLK_H_VALID_DATA_CNT(x) ((x) & 0x3ffff) > > #define DEBUG0_HSYNC (1 < 26) > @@ -116,7 +138,7 @@ > #define STMLCDIF_18BIT 2 /* pixel data bus to the display is of 18 bit width */ > #define STMLCDIF_24BIT 3 /* pixel data bus to the display is of 24 bit width */ > > -#define MXSFB_SYNC_DATA_ENABLE_HIGH_ACT (1 << 6) > -#define MXSFB_SYNC_DOTCLK_FALLING_ACT (1 << 7) /* negative edge sampling */ > +#define MXSFB_SYNC_DATA_ENABLE_HIGH_ACT BIT(6) > +#define MXSFB_SYNC_DOTCLK_FALLING_ACT BIT(7) /* negative edge sampling */ > > #endif /* __MXSFB_REGS_H__ */ > -- > 2.7.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
Hi Robert, On Wed, Jun 26, 2019 at 04:32:08PM +0300, Robert Chiras wrote: > This patch-set improves the use of eLCDIF block on iMX 8 SoCs (like 8MQ, 8MM > and 8QXP). Following, are the new features added and fixes from this > patch-set: There was some feedback on various patches, do you intend to pick that up again? That would be cool since there's some overlapping work popping up already e.g. in https://patchwork.freedesktop.org/series/64595/ showing up and it's the base for the tiny https://patchwork.freedesktop.org/series/64300/ Cheers, -- Guido
On Ma, 2019-08-13 at 12:23 +0200, Guido Günther wrote: > Hi Robert, > On Wed, Jun 26, 2019 at 04:32:08PM +0300, Robert Chiras wrote: > > > > This patch-set improves the use of eLCDIF block on iMX 8 SoCs (like > > 8MQ, 8MM > > and 8QXP). Following, are the new features added and fixes from > > this > > patch-set: > There was some feedback on various patches, do you intend to pick > that > up again? That would be cool since there's some overlapping work > popping > up already e.g. in > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2 > Fpatchwork.freedesktop.org%2Fseries%2F64595%2F&data=02%7C01%7Crob > ert.chiras%40nxp.com%7C6ca6724a656b41912f0408d71fd83da0%7C686ea1d3bc2 > b4c6fa92cd99c5c301635%7C0%7C0%7C637012885918631196&sdata=b3CrbNu% > 2FcsWBOA%2BcaQLX%2BrlrK7%2Fhf2%2F1vZS3eQGN7aM%3D&reserved=0 > > showing up and it's the base for the tiny > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2 > Fpatchwork.freedesktop.org%2Fseries%2F64300%2F&data=02%7C01%7Crob > ert.chiras%40nxp.com%7C6ca6724a656b41912f0408d71fd83da0%7C686ea1d3bc2 > b4c6fa92cd99c5c301635%7C0%7C0%7C637012885918641196&sdata=h6KLVnSx > xBwvK%2FvPF9zt4DQR6WnF1pyQSwKBTO4rQTg%3D&reserved=0 > > Cheers, > -- Guido Hi Guido, Yes, I plan to submit a next revision, but first I wanted to try it with your patch-set for the nwl-dsi driver. Thanks for the heads-up. Best regards, Robert