[PATCH] drm/mxsfb: fix pixel clock polarity
stefan at agner.ch
Thu Dec 8 01:26:40 UTC 2016
On 2016-12-07 16:59, Stefan Agner wrote:
> On 2016-12-07 16:49, Marek Vasut wrote:
>> On 12/08/2016 01:27 AM, Stefan Agner wrote:
>>> The DRM subsystem specifies the pixel clock polarity from a
>>> controllers perspective: DRM_BUS_FLAG_PIXDATA_NEGEDGE means
>>> the controller drives the data on pixel clocks falling edge.
>>> That is the controllers DOTCLK_POL=0 (Default is data launched
>>> at negative edge).
>>> Also change the data enable logic to be high active by default
>>> and only change if explicitly requested via bus_flags. With
>>> that defaults are:
>>> - Data enable: high active
>>> - Pixel clock polarity: controller drives data on negative edge
>>> Signed-off-by: Stefan Agner <stefan at agner.ch>
>>> Hi Marek,
>> Hi, that was quick, thanks for checking this.
> Yeah, I couldn't wait seeing it flying :-)
>>> I discovered this while testing on a i.MX 7 eLCDIF IP. Particularly the
>>> non-standard DE polarity was causing issues. I was using a EDT display
>>> which is part of simple panel driver since a while now and does not
>>> specify any bus_flags currently... Of course I could (and probably should)
>>> add the proper bus_flags there too, but there are several displays
>>> which do not specify any polarity and likely rely on sensible driver
>>> standards (which is afact high active for the DE signal).
>> I actually use a panel which requires correct settings of the flags, see
>> e0932f9d7ba9a16f99a84943b720f109de8e3e06 in mainline , so this patch
>> would break things for me. So I wonder whether you should fix the panel
>> driver or whether the mxsfb should be fixed ?
> If you ask me, mxsfb.
> Ok, there are actually two things, one is a bug, one is a default
> The bug: Pixel clock polarity is clearly defined to be controller
> centric (see comments around DRM_BUS_FLAG_PIXDATA_*EDGE in
> include/drm/drm_connector.h). The driver does it wrong currently.
> This might affect your display, and if it does, it is actually wrong
> also in your display... However, since it is a bug, I think it is not
> really a debate, it should be fixed...
FWIW, it seems that Ortustech com43h4m85ulc samples on falling edge, so
DRM_BUS_FLAG_PIXDATA_POSEDGE seems right. And it means that DOTCLK_POL
should be 1 (inverted), so with this patch the polarity should actually
be correct for that panel.
> The default change: Data enable should be high by default because that
> is what most displays require, including yours. This won't break your
> display, since you request DRM_BUS_FLAG_DE_HIGH anyway...
> We could debate whether that default change is necessary, but since it
> also won't affect your display, I think it is a worthwhile change.
>>> drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 11 +++++++++--
>>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
>>> index 0818903..4bcc8a3 100644
>>> --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
>>> +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
>>> @@ -168,9 +168,16 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb)
>>> vdctrl0 |= VDCTRL0_HSYNC_ACT_HIGH;
>>> if (m->flags & DRM_MODE_FLAG_PVSYNC)
>>> vdctrl0 |= VDCTRL0_VSYNC_ACT_HIGH;
>>> - if (bus_flags & DRM_BUS_FLAG_DE_HIGH)
>>> + /* Data Enable should be high active by default */
>>> + if (!(bus_flags & DRM_BUS_FLAG_DE_LOW))
>>> vdctrl0 |= VDCTRL0_ENABLE_ACT_HIGH;
>>> - if (bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE)
>>> + /*
>>> + * Note: DRM_BUS_FLAG_PIXDATA defines are controller centric,
>>> + * controllers VDCTRL0_DOTCLK is display centric.
>>> + * Drive on positive edige -> display samples on falling edge
>>> + * DRM_BUS_FLAG_PIXDATA_POSEDGE -> VDCTRL0_DOTCLK_ACT_FALLING
>>> + */
>>> + if (bus_flags & DRM_BUS_FLAG_PIXDATA_POSEDGE)
>>> vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING;
>>> writel(vdctrl0, mxsfb->base + LCDC_VDCTRL0);
More information about the dri-devel