lkml.org 
[lkml]   [2016]   [Dec]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] drm/mxsfb: fix pixel clock polarity
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...

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

\
 
 \ /
  Last update: 2016-12-08 02:06    [W:0.055 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site