linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm/mxsfb: fix pixel clock polarity
@ 2016-12-08  0:27 Stefan Agner
  2016-12-08  0:49 ` Marek Vasut
  0 siblings, 1 reply; 11+ messages in thread
From: Stefan Agner @ 2016-12-08  0:27 UTC (permalink / raw)
  To: marex; +Cc: airlied, dri-devel, linux-kernel, Stefan Agner

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,

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

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/mxsfb: fix pixel clock polarity
  2016-12-08  0:27 [PATCH] drm/mxsfb: fix pixel clock polarity Stefan Agner
@ 2016-12-08  0:49 ` Marek Vasut
  2016-12-08  0:59   ` Stefan Agner
  0 siblings, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2016-12-08  0:49 UTC (permalink / raw)
  To: Stefan Agner; +Cc: airlied, dri-devel, linux-kernel

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.

> 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 ?

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


-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/mxsfb: fix pixel clock polarity
  2016-12-08  0:49 ` Marek Vasut
@ 2016-12-08  0:59   ` Stefan Agner
  2016-12-08  1:26     ` Stefan Agner
  0 siblings, 1 reply; 11+ messages in thread
From: Stefan Agner @ 2016-12-08  0:59 UTC (permalink / raw)
  To: Marek Vasut; +Cc: airlied, dri-devel, linux-kernel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/mxsfb: fix pixel clock polarity
  2016-12-08  0:59   ` Stefan Agner
@ 2016-12-08  1:26     ` Stefan Agner
  2016-12-08  2:37       ` Marek Vasut
  0 siblings, 1 reply; 11+ messages in thread
From: Stefan Agner @ 2016-12-08  1:26 UTC (permalink / raw)
  To: Marek Vasut; +Cc: airlied, dri-devel, linux-kernel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/mxsfb: fix pixel clock polarity
  2016-12-08  1:26     ` Stefan Agner
@ 2016-12-08  2:37       ` Marek Vasut
  2016-12-08 20:46         ` Stefan Agner
  0 siblings, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2016-12-08  2:37 UTC (permalink / raw)
  To: Stefan Agner; +Cc: airlied, dri-devel, linux-kernel

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

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 ?

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/mxsfb: fix pixel clock polarity
  2016-12-08  2:37       ` Marek Vasut
@ 2016-12-08 20:46         ` Stefan Agner
  2016-12-08 23:38           ` Marek Vasut
  0 siblings, 1 reply; 11+ messages in thread
From: Stefan Agner @ 2016-12-08 20:46 UTC (permalink / raw)
  To: Marek Vasut; +Cc: airlied, dri-devel, linux-kernel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/mxsfb: fix pixel clock polarity
  2016-12-08 20:46         ` Stefan Agner
@ 2016-12-08 23:38           ` Marek Vasut
  2016-12-14  0:01             ` Stefan Agner
  0 siblings, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2016-12-08 23:38 UTC (permalink / raw)
  To: Stefan Agner; +Cc: airlied, dri-devel, linux-kernel, Philipp Zabel

On 12/08/2016 09:46 PM, Stefan Agner wrote:
> 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@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.
>>
>> 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.

Good

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

This is probably where the problem crept in [1], droping PIXDATA_POSEDGE
actually makes this patch work for me. CCing Philipp.

[1] https://patchwork.kernel.org/patch/9301517/

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/mxsfb: fix pixel clock polarity
  2016-12-08 23:38           ` Marek Vasut
@ 2016-12-14  0:01             ` Stefan Agner
  2016-12-14  8:04               ` Marek Vasut
  0 siblings, 1 reply; 11+ messages in thread
From: Stefan Agner @ 2016-12-14  0:01 UTC (permalink / raw)
  To: Marek Vasut; +Cc: airlied, dri-devel, linux-kernel, Philipp Zabel

On 2016-12-08 15:38, Marek Vasut wrote:
> On 12/08/2016 09:46 PM, Stefan Agner wrote:
>> 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@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.
>>>
>>> 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.
> 
> Good
> 
>> 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.
> 
> This is probably where the problem crept in [1], droping PIXDATA_POSEDGE
> actually makes this patch work for me. CCing Philipp.
> 
> [1] https://patchwork.kernel.org/patch/9301517/

I looked at a old data sheet of that display and it seemed that
PIXDATA_POSEDGE is the right thing. Panelook.cn lists newer data sheets,
but I couldn't find them on the open internet, do you  have access to a
newer one?

http://www.panelook.cn/COM43H4M85ULC_ORTUSTECH_4.3_LCM_overview_17296.html

I guess in the end it doesn't matter: Given that it is verified that the
controllers data sheet is right, I vote for merging that patch and fix
the displays polarity...

--
Stefan

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/mxsfb: fix pixel clock polarity
  2016-12-14  0:01             ` Stefan Agner
@ 2016-12-14  8:04               ` Marek Vasut
  2016-12-14 20:29                 ` Stefan Agner
  0 siblings, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2016-12-14  8:04 UTC (permalink / raw)
  To: Stefan Agner; +Cc: airlied, dri-devel, linux-kernel, Philipp Zabel

On 12/14/2016 01:01 AM, Stefan Agner wrote:
> On 2016-12-08 15:38, Marek Vasut wrote:
>> On 12/08/2016 09:46 PM, Stefan Agner wrote:
>>> 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@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.
>>>>
>>>> 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.
>>
>> Good
>>
>>> 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.
>>
>> This is probably where the problem crept in [1], droping PIXDATA_POSEDGE
>> actually makes this patch work for me. CCing Philipp.
>>
>> [1] https://patchwork.kernel.org/patch/9301517/
> 
> I looked at a old data sheet of that display and it seemed that
> PIXDATA_POSEDGE is the right thing. Panelook.cn lists newer data sheets,
> but I couldn't find them on the open internet, do you  have access to a
> newer one?

Which "version" do you have ? Probably not though.

> http://www.panelook.cn/COM43H4M85ULC_ORTUSTECH_4.3_LCM_overview_17296.html
> 
> I guess in the end it doesn't matter: Given that it is verified that the
> controllers data sheet is right, I vote for merging that patch and fix
> the displays polarity...

Merging which patch ?

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/mxsfb: fix pixel clock polarity
  2016-12-14  8:04               ` Marek Vasut
@ 2016-12-14 20:29                 ` Stefan Agner
  2016-12-14 20:38                   ` Marek Vasut
  0 siblings, 1 reply; 11+ messages in thread
From: Stefan Agner @ 2016-12-14 20:29 UTC (permalink / raw)
  To: Marek Vasut; +Cc: airlied, dri-devel, linux-kernel, Philipp Zabel

On 2016-12-14 00:04, Marek Vasut wrote:
> On 12/14/2016 01:01 AM, Stefan Agner wrote:
>> On 2016-12-08 15:38, Marek Vasut wrote:
>>> On 12/08/2016 09:46 PM, Stefan Agner wrote:
>>>> 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@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.
>>>>>
>>>>> 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.
>>>
>>> Good
>>>
>>>> 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.
>>>
>>> This is probably where the problem crept in [1], droping PIXDATA_POSEDGE
>>> actually makes this patch work for me. CCing Philipp.
>>>
>>> [1] https://patchwork.kernel.org/patch/9301517/
>>
>> I looked at a old data sheet of that display and it seemed that
>> PIXDATA_POSEDGE is the right thing. Panelook.cn lists newer data sheets,
>> but I couldn't find them on the open internet, do you  have access to a
>> newer one?
> 
> Which "version" do you have ? Probably not though.
> 

I couldn't find it anymore, but I think it said Rev. 0

>> http://www.panelook.cn/COM43H4M85ULC_ORTUSTECH_4.3_LCM_overview_17296.html
>>
>> I guess in the end it doesn't matter: Given that it is verified that the
>> controllers data sheet is right, I vote for merging that patch and fix
>> the displays polarity...
> 
> Merging which patch ?

This patch.

And I guess, to keep your display working, a patch which changes the
Ortustech pixel clock flag...

--
Stefan

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/mxsfb: fix pixel clock polarity
  2016-12-14 20:29                 ` Stefan Agner
@ 2016-12-14 20:38                   ` Marek Vasut
  0 siblings, 0 replies; 11+ messages in thread
From: Marek Vasut @ 2016-12-14 20:38 UTC (permalink / raw)
  To: Stefan Agner; +Cc: airlied, dri-devel, linux-kernel, Philipp Zabel

On 12/14/2016 09:29 PM, Stefan Agner wrote:
> On 2016-12-14 00:04, Marek Vasut wrote:
>> On 12/14/2016 01:01 AM, Stefan Agner wrote:
>>> On 2016-12-08 15:38, Marek Vasut wrote:
>>>> On 12/08/2016 09:46 PM, Stefan Agner wrote:
>>>>> 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@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.
>>>>>>
>>>>>> 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.
>>>>
>>>> Good
>>>>
>>>>> 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.
>>>>
>>>> This is probably where the problem crept in [1], droping PIXDATA_POSEDGE
>>>> actually makes this patch work for me. CCing Philipp.
>>>>
>>>> [1] https://patchwork.kernel.org/patch/9301517/
>>>
>>> I looked at a old data sheet of that display and it seemed that
>>> PIXDATA_POSEDGE is the right thing. Panelook.cn lists newer data sheets,
>>> but I couldn't find them on the open internet, do you  have access to a
>>> newer one?
>>
>> Which "version" do you have ? Probably not though.
>>
> 
> I couldn't find it anymore, but I think it said Rev. 0
> 
>>> http://www.panelook.cn/COM43H4M85ULC_ORTUSTECH_4.3_LCM_overview_17296.html
>>>
>>> I guess in the end it doesn't matter: Given that it is verified that the
>>> controllers data sheet is right, I vote for merging that patch and fix
>>> the displays polarity...
>>
>> Merging which patch ?
> 
> This patch.
> 
> And I guess, to keep your display working, a patch which changes the
> Ortustech pixel clock flag...

Well, let's do that. Btw can you send a V2 with s/edige/edge/ ? ;-)

Thanks!

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2016-12-14 20:38 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-08  0:27 [PATCH] drm/mxsfb: fix pixel clock polarity Stefan Agner
2016-12-08  0:49 ` Marek Vasut
2016-12-08  0:59   ` Stefan Agner
2016-12-08  1:26     ` Stefan Agner
2016-12-08  2:37       ` Marek Vasut
2016-12-08 20:46         ` Stefan Agner
2016-12-08 23:38           ` Marek Vasut
2016-12-14  0:01             ` Stefan Agner
2016-12-14  8:04               ` Marek Vasut
2016-12-14 20:29                 ` Stefan Agner
2016-12-14 20:38                   ` Marek Vasut

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).