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

[Stefan Agner]

On 2016-12-07 18:37, Marek Vasut wrote:
> On 12/08/2016 02:26 AM, Stefan Agner wrote:
>> 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@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...
>>
>> 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.
> 
> Well, if I apply this patch, my image is shifted by 1 px to the left and
> there is a 1px white bar on the right side, so I think I have some
> polarity problem now ?

Ok, lets create facts here:
1. SoloX Refrence Manual, Figure 37-13. shows DOTCLK_POL=0, and it shows
that the controller drives signals on falling edge of the pixel clock.
The i.MX 7 has the same figure.
2. Just to verify, I hooked up an oscilloscope on my i.MX 7: It shows
that with DOTCLK_POL=0 the controller drives on falling edge:
http://imgur.com/a/2f2Xt 

So my measurements verify what is in the i.MX data sheets.

The current code sets the bit when negative edge (falling edge) is
requested, which is wrong:
#define VDCTRL0_DOTCLK_ACT_FALLING	(1 << 25)
...
if (bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE)
     vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING;

Not sure what is going on with your display, maybe the datasheet is just
wrong (it requires DRM_BUS_FLAG_PIXDATA_NEGEDGE in fact) or it is some
other artifact.

--
Stefan