Re: [PATCH] drm/mxsfb: fix pixel clock polarity

[Stefan Agner <stefan@agner.ch>]

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@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
> change.
> 
> 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.

--
Stefan

> 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.
> 
> --
> Stefan
> 
> 
>>
>>> --
>>> Stefan
>>>
>>>  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);
>>>