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

[Stefan Agner]

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

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