[PATCH 1/2] drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE

[PATCH 1/2] drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE

[Giulio Benetti]

Can't set dclk polarity on sun4i.

Handle both positive and negative dclk polarity,
according to bus_flags.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index f4284b5..6121210 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -17,6 +17,7 @@
 #include <drm/drm_encoder.h>
 #include <drm/drm_modes.h>
 #include <drm/drm_of.h>
+#include <drm/drm_panel.h>
 
 #include <uapi/drm/drm_mode.h>
 
@@ -173,6 +174,9 @@ static void sun4i_tcon0_mode_set_common(struct sun4i_tcon *tcon,
 static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 				     const struct drm_display_mode *mode)
 {
+	struct drm_panel *panel = tcon->panel;
+	struct drm_connector *connector = panel->connector;
+	struct drm_display_info display_info = connector->display_info;
 	unsigned int bp, hsync, vsync;
 	u8 clk_delay;
 	u32 val = 0;
@@ -226,8 +230,13 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
 		val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
 
+	if(display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_POSEDGE)
+		val |= SUN4I_TCON0_IO_POL_DCLK_PHASE(1);
+
 	regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
-			   SUN4I_TCON0_IO_POL_HSYNC_POSITIVE | SUN4I_TCON0_IO_POL_VSYNC_POSITIVE,
+			   SUN4I_TCON0_IO_POL_HSYNC_POSITIVE |
+			   SUN4I_TCON0_IO_POL_VSYNC_POSITIVE |
+			   SUN4I_TCON0_IO_POL_DCLK_PHASE(3),
 			   val);
 
 	/* Map output pins to channel 0 */
-- 
2.7.4

[PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly

[Giulio Benetti]

On previous handling, if specified DRM_MODE_FLAG_N*SYNC,
it was ignored,
because only PHSYNC and PVSYNC were taken into account.
DRM_MODE_FLAG_P*SYNC and DRM_MODE_FLAG_N*SYNC are not exclusive.

If flags contains PVSYNC, it doesn't mean it is NVSYNC.
And it's true also the contrary.
Also, as I've checked with scope on A20,
if (flags & PVSYNC) then SUN4I_TCON0_IO_POL_VSYNC_POSITIVE
must be set, as name suggests.
It seems all display io polarities starts inverted if 0.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>

PVSYNC and PHSYNC only

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 6121210..e873a37 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -224,10 +224,10 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 		     SUN4I_TCON0_BASIC3_H_SYNC(hsync));
 
 	/* Setup the polarity of the various signals */
-	if (!(mode->flags & DRM_MODE_FLAG_PHSYNC))
+	if (mode->flags & DRM_MODE_FLAG_PHSYNC)
 		val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
 
-	if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
+	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
 		val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
 
 	if(display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_POSEDGE)
-- 
2.7.4

[PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly

[Maxime Ripard]

On Sat, Jan 20, 2018 at 07:50:21PM +0100, Giulio Benetti wrote:
> On previous handling, if specified DRM_MODE_FLAG_N*SYNC,
> it was ignored,
> because only PHSYNC and PVSYNC were taken into account.
> DRM_MODE_FLAG_P*SYNC and DRM_MODE_FLAG_N*SYNC are not exclusive.
> 
> If flags contains PVSYNC, it doesn't mean it is NVSYNC.
> And it's true also the contrary.
> Also, as I've checked with scope on A20,
> if (flags & PVSYNC) then SUN4I_TCON0_IO_POL_VSYNC_POSITIVE
> must be set, as name suggests.
> It seems all display io polarities starts inverted if 0.
> 
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> 
> PVSYNC and PHSYNC only
> 
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>

Checkpatch:
WARNING: Duplicate signature

> ---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index 6121210..e873a37 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -224,10 +224,10 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
>  		     SUN4I_TCON0_BASIC3_H_SYNC(hsync));
>  
>  	/* Setup the polarity of the various signals */
> -	if (!(mode->flags & DRM_MODE_FLAG_PHSYNC))
> +	if (mode->flags & DRM_MODE_FLAG_PHSYNC)
>  		val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
>  
> -	if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
> +	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
>  		val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;

I'm not sure why you were talking of the differences between NVSYNC
and PVSYNC if you're not making use of any of it here?

Also, how was it tested? This seems quite weird that we haven't caught
that one sooner, and I'm a bit worried about the possible regressions
here.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180122/2391f52b/attachment-0001.sig>

[PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly

[Giulio Benetti]

Hi,

Il 22/01/2018 09:51, Maxime Ripard ha scritto:
> On Sat, Jan 20, 2018 at 07:50:21PM +0100, Giulio Benetti wrote:
>> On previous handling, if specified DRM_MODE_FLAG_N*SYNC,
>> it was ignored,
>> because only PHSYNC and PVSYNC were taken into account.
>> DRM_MODE_FLAG_P*SYNC and DRM_MODE_FLAG_N*SYNC are not exclusive.
>>
>> If flags contains PVSYNC, it doesn't mean it is NVSYNC.
>> And it's true also the contrary.
>> Also, as I've checked with scope on A20,
>> if (flags & PVSYNC) then SUN4I_TCON0_IO_POL_VSYNC_POSITIVE
>> must be set, as name suggests.
>> It seems all display io polarities starts inverted if 0.
>>
>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
>>
>> PVSYNC and PHSYNC only
>>
>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> 
> Checkpatch:
> WARNING: Duplicate signature

Sorry I didn't use ./scripts/checkpatch.pl

> 
>> ---
>>   drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> index 6121210..e873a37 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> @@ -224,10 +224,10 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
>>   		     SUN4I_TCON0_BASIC3_H_SYNC(hsync));
>>   
>>   	/* Setup the polarity of the various signals */
>> -	if (!(mode->flags & DRM_MODE_FLAG_PHSYNC))
>> +	if (mode->flags & DRM_MODE_FLAG_PHSYNC)
>>   		val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
>>   
>> -	if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
>> +	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
>>   		val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
> 
> I'm not sure why you were talking of the differences between NVSYNC
> and PVSYNC if you're not making use of any of it here?

Thinking about it more now, the point is that all Lcd IOs seem to be 
inverted by default(at least on A20).
With inverted, I mean that if for example PVSYNC,
I should see vsync line low and when asserted to give VSync,
it goes high.
This is what I've checked with oscilloscope on A20.
Can someone give a try on A33? Otherwise I will,
but I will take some time.
On uboot, everything is treated equal to kernel,
but to have my falling edge dclk and low h/vsync I had to specify:
CONFIG_VIDEO_LCD_DCLK_PHASE=0 (giving me falling edge on dclk)
and
CONFIG_VIDEO_LCD_MODE="....,sync:3,..."
but digging into code, I see "sync:3" means H/VSYNC HIGH,
but I experience both LOW during their pulse.

> 
> Also, how was it tested? This seems quite weird that we haven't caught
> that one sooner, and I'm a bit worried about the possible regressions
> here.

It sounds really strange to me too,
because everybody under uboot use "sync:3"(HIGH).
I will retry to measure,
unfortunately at home I don't have a scope,
but I think I'm going to have one soon, because of this. :)

> 
> Maxime
> 


-- 
Giulio Benetti
R&D Manager &
Advanced Research

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale ? 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

[PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly

[Giulio Benetti]

Hi,

Il 22/01/2018 21:27, Giulio Benetti ha scritto:
> Hi,
> 
> Il 22/01/2018 09:51, Maxime Ripard ha scritto:
>> On Sat, Jan 20, 2018 at 07:50:21PM +0100, Giulio Benetti wrote:
>>> On previous handling, if specified DRM_MODE_FLAG_N*SYNC,
>>> it was ignored,
>>> because only PHSYNC and PVSYNC were taken into account.
>>> DRM_MODE_FLAG_P*SYNC and DRM_MODE_FLAG_N*SYNC are not exclusive.
>>>
>>> If flags contains PVSYNC, it doesn't mean it is NVSYNC.
>>> And it's true also the contrary.
>>> Also, as I've checked with scope on A20,
>>> if (flags & PVSYNC) then SUN4I_TCON0_IO_POL_VSYNC_POSITIVE
>>> must be set, as name suggests.
>>> It seems all display io polarities starts inverted if 0.
>>>
>>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
>>>
>>> PVSYNC and PHSYNC only
>>>
>>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
>>
>> Checkpatch:
>> WARNING: Duplicate signature
> 
> Sorry I didn't use ./scripts/checkpatch.pl
> 
>>
>>> ---
>>> ? drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++--
>>> ? 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c 
>>> b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>> index 6121210..e873a37 100644
>>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>> @@ -224,10 +224,10 @@ static void sun4i_tcon0_mode_set_rgb(struct 
>>> sun4i_tcon *tcon,
>>> ?????????????? SUN4I_TCON0_BASIC3_H_SYNC(hsync));
>>> ????? /* Setup the polarity of the various signals */
>>> -??? if (!(mode->flags & DRM_MODE_FLAG_PHSYNC))
>>> +??? if (mode->flags & DRM_MODE_FLAG_PHSYNC)
>>> ????????? val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
>>> -??? if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
>>> +??? if (mode->flags & DRM_MODE_FLAG_PVSYNC)
>>> ????????? val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
>>
>> I'm not sure why you were talking of the differences between NVSYNC
>> and PVSYNC if you're not making use of any of it here?
> 
> Thinking about it more now, the point is that all Lcd IOs seem to be 
> inverted by default(at least on A20).
> With inverted, I mean that if for example PVSYNC,
> I should see vsync line low and when asserted to give VSync,
> it goes high.
> This is what I've checked with oscilloscope on A20.
> Can someone give a try on A33? Otherwise I will,
> but I will take some time.
> On uboot, everything is treated equal to kernel,
> but to have my falling edge dclk and low h/vsync I had to specify:
> CONFIG_VIDEO_LCD_DCLK_PHASE=0 (giving me falling edge on dclk)
> and
> CONFIG_VIDEO_LCD_MODE="....,sync:3,..."
> but digging into code, I see "sync:3" means H/VSYNC HIGH,
> but I experience both LOW during their pulse.
> 
>>
>> Also, how was it tested? This seems quite weird that we haven't caught
>> that one sooner, and I'm a bit worried about the possible regressions
>> here.
> 
> It sounds really strange to me too,
> because everybody under uboot use "sync:3"(HIGH).
> I will retry to measure,
> unfortunately at home I don't have a scope,
> but I think I'm going to have one soon, because of this. :)

Here I am with scope captures and tcon0 registers dump:
tcon0_regs => https://pasteboard.co/H4r8Zcs.png
dclk_d0 => https://pasteboard.co/H4r8QRe.png
dclk_de => https://pasteboard.co/H4r8zh4R.png
dclk_vsnc => https://pasteboard.co/H4r8Hye.png

As you can see circled in reg on registers,
TCON0_IO_POL_REG = 0x00000000.
But on all the waveforms you can see:
- dclk_d0: clock phase is 0, but it starts with falling edge, otherwise 
the rising front overlaps dclk rising edge(not good), so to me this is 
falling, then I mean it Negative.
- dclk_de: de pulse is clearly negative, even if register is 0 and its' 
polarity bit is 0.
- dclk_vsnc: same as dclk_de
- dclk_hsync: I didn't take scope screenshot but I can assure you it's 
negative.

You can also check all the other registers about TCON0.

Now I proceed testing it on A33, maybe the peripheral is slightly 
different between Axx SoCs, if I find it that way,
it should be only a check about SoC or peripheral ID,
and treat polarity as it should be done.

Kind regards
-- 
Giulio Benetti
R&D Manager &
Advanced Research

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale ? 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

> 
>>
>> Maxime
>>
> 
> 

[PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly

[Giulio Benetti]

Hi,

Il 24/01/2018 18:38, Giulio Benetti ha scritto:
> Hi,
> 
> Il 22/01/2018 21:27, Giulio Benetti ha scritto:
>> Hi,
>>
>> Il 22/01/2018 09:51, Maxime Ripard ha scritto:
>>> On Sat, Jan 20, 2018 at 07:50:21PM +0100, Giulio Benetti wrote:
>>>> On previous handling, if specified DRM_MODE_FLAG_N*SYNC,
>>>> it was ignored,
>>>> because only PHSYNC and PVSYNC were taken into account.
>>>> DRM_MODE_FLAG_P*SYNC and DRM_MODE_FLAG_N*SYNC are not exclusive.
>>>>
>>>> If flags contains PVSYNC, it doesn't mean it is NVSYNC.
>>>> And it's true also the contrary.
>>>> Also, as I've checked with scope on A20,
>>>> if (flags & PVSYNC) then SUN4I_TCON0_IO_POL_VSYNC_POSITIVE
>>>> must be set, as name suggests.
>>>> It seems all display io polarities starts inverted if 0.
>>>>
>>>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
>>>>
>>>> PVSYNC and PHSYNC only
>>>>
>>>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
>>>
>>> Checkpatch:
>>> WARNING: Duplicate signature
>>
>> Sorry I didn't use ./scripts/checkpatch.pl
>>
>>>
>>>> ---
>>>> ? drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++--
>>>> ? 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c 
>>>> b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>> index 6121210..e873a37 100644
>>>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>> @@ -224,10 +224,10 @@ static void sun4i_tcon0_mode_set_rgb(struct 
>>>> sun4i_tcon *tcon,
>>>> ?????????????? SUN4I_TCON0_BASIC3_H_SYNC(hsync));
>>>> ????? /* Setup the polarity of the various signals */
>>>> -??? if (!(mode->flags & DRM_MODE_FLAG_PHSYNC))
>>>> +??? if (mode->flags & DRM_MODE_FLAG_PHSYNC)
>>>> ????????? val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
>>>> -??? if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
>>>> +??? if (mode->flags & DRM_MODE_FLAG_PVSYNC)
>>>> ????????? val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
>>>
>>> I'm not sure why you were talking of the differences between NVSYNC
>>> and PVSYNC if you're not making use of any of it here?
>>
>> Thinking about it more now, the point is that all Lcd IOs seem to be 
>> inverted by default(at least on A20).
>> With inverted, I mean that if for example PVSYNC,
>> I should see vsync line low and when asserted to give VSync,
>> it goes high.
>> This is what I've checked with oscilloscope on A20.
>> Can someone give a try on A33? Otherwise I will,
>> but I will take some time.
>> On uboot, everything is treated equal to kernel,
>> but to have my falling edge dclk and low h/vsync I had to specify:
>> CONFIG_VIDEO_LCD_DCLK_PHASE=0 (giving me falling edge on dclk)
>> and
>> CONFIG_VIDEO_LCD_MODE="....,sync:3,..."
>> but digging into code, I see "sync:3" means H/VSYNC HIGH,
>> but I experience both LOW during their pulse.
>>
>>>
>>> Also, how was it tested? This seems quite weird that we haven't caught
>>> that one sooner, and I'm a bit worried about the possible regressions
>>> here.
>>
>> It sounds really strange to me too,
>> because everybody under uboot use "sync:3"(HIGH).
>> I will retry to measure,
>> unfortunately at home I don't have a scope,
>> but I think I'm going to have one soon, because of this. :)
> 
> Here I am with scope captures and tcon0 registers dump:
> tcon0_regs => https://pasteboard.co/H4r8Zcs.png
> dclk_d0 => https://pasteboard.co/H4r8QRe.png
> dclk_de => https://pasteboard.co/H4r8zh4R.png
> dclk_vsnc => https://pasteboard.co/H4r8Hye.png
> 
> As you can see circled in reg on registers,
> TCON0_IO_POL_REG = 0x00000000.
> But on all the waveforms you can see:
> - dclk_d0: clock phase is 0, but it starts with falling edge, otherwise 
> the rising front overlaps dclk rising edge(not good), so to me this is 
> falling, then I mean it Negative.
> - dclk_de: de pulse is clearly negative, even if register is 0 and its' 
> polarity bit is 0.
> - dclk_vsnc: same as dclk_de
> - dclk_hsync: I didn't take scope screenshot but I can assure you it's 
> negative.
> 
> You can also check all the other registers about TCON0.
> 
> Now I proceed testing it on A33, maybe the peripheral is slightly 
> different between Axx SoCs, if I find it that way,
> it should be only a check about SoC or peripheral ID,
> and treat polarity as it should be done.

Here I am with A33 waveforms:
tcon0_regs => https://pasteboard.co/H4rXfN0M.png
dclk_d0 => https://pasteboard.co/H4rVXwy.png
dclk_de => https://pasteboard.co/H4rWDt8.png
dclk_vsnc => https://pasteboard.co/H4rWRACu.png
dclk_hsync => https://pasteboard.co/H4rWK6I.png

It behaves the same way as A20, so as I mean IO polarity,
all signals(except D0-D23), are inverted.
For A33 I've used A33-OLinuXino.
For A20 our LiNova1.

Do you agree with me?

> 
> Kind regards


-- 
Giulio Benetti
R&D Manager &
Advanced Research

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale ? 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

[PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly

[Maxime Ripard]

Hi,

On Wed, Jan 24, 2018 at 08:37:28PM +0100, Giulio Benetti wrote:
> Hi,
> 
> Il 24/01/2018 18:38, Giulio Benetti ha scritto:
> > Hi,
> > 
> > Il 22/01/2018 21:27, Giulio Benetti ha scritto:
> > > Hi,
> > > 
> > > Il 22/01/2018 09:51, Maxime Ripard ha scritto:
> > > > On Sat, Jan 20, 2018 at 07:50:21PM +0100, Giulio Benetti wrote:
> > > > > On previous handling, if specified DRM_MODE_FLAG_N*SYNC,
> > > > > it was ignored,
> > > > > because only PHSYNC and PVSYNC were taken into account.
> > > > > DRM_MODE_FLAG_P*SYNC and DRM_MODE_FLAG_N*SYNC are not exclusive.
> > > > > 
> > > > > If flags contains PVSYNC, it doesn't mean it is NVSYNC.
> > > > > And it's true also the contrary.
> > > > > Also, as I've checked with scope on A20,
> > > > > if (flags & PVSYNC) then SUN4I_TCON0_IO_POL_VSYNC_POSITIVE
> > > > > must be set, as name suggests.
> > > > > It seems all display io polarities starts inverted if 0.
> > > > > 
> > > > > Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> > > > > 
> > > > > PVSYNC and PHSYNC only
> > > > > 
> > > > > Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> > > > 
> > > > Checkpatch:
> > > > WARNING: Duplicate signature
> > > 
> > > Sorry I didn't use ./scripts/checkpatch.pl
> > > 
> > > > 
> > > > > ---
> > > > > ? drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++--
> > > > > ? 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > > b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > > index 6121210..e873a37 100644
> > > > > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > > @@ -224,10 +224,10 @@ static void
> > > > > sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
> > > > > ?????????????? SUN4I_TCON0_BASIC3_H_SYNC(hsync));
> > > > > ????? /* Setup the polarity of the various signals */
> > > > > -??? if (!(mode->flags & DRM_MODE_FLAG_PHSYNC))
> > > > > +??? if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> > > > > ????????? val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
> > > > > -??? if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
> > > > > +??? if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> > > > > ????????? val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
> > > > 
> > > > I'm not sure why you were talking of the differences between NVSYNC
> > > > and PVSYNC if you're not making use of any of it here?
> > > 
> > > Thinking about it more now, the point is that all Lcd IOs seem to be
> > > inverted by default(at least on A20).
> > > With inverted, I mean that if for example PVSYNC,
> > > I should see vsync line low and when asserted to give VSync,
> > > it goes high.
> > > This is what I've checked with oscilloscope on A20.
> > > Can someone give a try on A33? Otherwise I will,
> > > but I will take some time.
> > > On uboot, everything is treated equal to kernel,
> > > but to have my falling edge dclk and low h/vsync I had to specify:
> > > CONFIG_VIDEO_LCD_DCLK_PHASE=0 (giving me falling edge on dclk)
> > > and
> > > CONFIG_VIDEO_LCD_MODE="....,sync:3,..."
> > > but digging into code, I see "sync:3" means H/VSYNC HIGH,
> > > but I experience both LOW during their pulse.
> > > 
> > > > 
> > > > Also, how was it tested? This seems quite weird that we haven't caught
> > > > that one sooner, and I'm a bit worried about the possible regressions
> > > > here.
> > > 
> > > It sounds really strange to me too,
> > > because everybody under uboot use "sync:3"(HIGH).
> > > I will retry to measure,
> > > unfortunately at home I don't have a scope,
> > > but I think I'm going to have one soon, because of this. :)
> > 
> > Here I am with scope captures and tcon0 registers dump:
> > tcon0_regs => https://pasteboard.co/H4r8Zcs.png
> > dclk_d0 => https://pasteboard.co/H4r8QRe.png
> > dclk_de => https://pasteboard.co/H4r8zh4R.png
> > dclk_vsnc => https://pasteboard.co/H4r8Hye.png
> > 
> > As you can see circled in reg on registers,
> > TCON0_IO_POL_REG = 0x00000000.
> > But on all the waveforms you can see:
> > - dclk_d0: clock phase is 0, but it starts with falling edge, otherwise
> > the rising front overlaps dclk rising edge(not good), so to me this is
> > falling, then I mean it Negative.
> > - dclk_de: de pulse is clearly negative, even if register is 0 and its'
> > polarity bit is 0.
> > - dclk_vsnc: same as dclk_de
> > - dclk_hsync: I didn't take scope screenshot but I can assure you it's
> > negative.
> > 
> > You can also check all the other registers about TCON0.
> > 
> > Now I proceed testing it on A33, maybe the peripheral is slightly
> > different between Axx SoCs, if I find it that way,
> > it should be only a check about SoC or peripheral ID,
> > and treat polarity as it should be done.
> 
> Here I am with A33 waveforms:
> tcon0_regs => https://pasteboard.co/H4rXfN0M.png
> dclk_d0 => https://pasteboard.co/H4rVXwy.png
> dclk_de => https://pasteboard.co/H4rWDt8.png
> dclk_vsnc => https://pasteboard.co/H4rWRACu.png
> dclk_hsync => https://pasteboard.co/H4rWK6I.png

Thanks, that's really helpful.

> It behaves the same way as A20, so as I mean IO polarity,
> all signals(except D0-D23), are inverted.
> For A33 I've used A33-OLinuXino.
> For A20 our LiNova1.

Indeed, HSYNC and VSYNC look inverted. I don't really know what the
polarity of D0 would be just by judging at that capture, but we would
have noticed if the colors were inverted for quite some time now.

DE seems to be active high though, since it's only going to be at a
logical low level when data are not transmitted, so during the blank
periods.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180125/b8de47ff/attachment-0001.sig>

[PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly

[Giulio Benetti]

Hi,

Il 25/01/2018 16:21, Maxime Ripard ha scritto:
> Hi,
> 
> On Wed, Jan 24, 2018 at 08:37:28PM +0100, Giulio Benetti wrote:
>> Hi,
>>
>> Il 24/01/2018 18:38, Giulio Benetti ha scritto:
>>> Hi,
>>>
>>> Il 22/01/2018 21:27, Giulio Benetti ha scritto:
>>>> Hi,
>>>>
>>>> Il 22/01/2018 09:51, Maxime Ripard ha scritto:
>>>>> On Sat, Jan 20, 2018 at 07:50:21PM +0100, Giulio Benetti wrote:
>>>>>> On previous handling, if specified DRM_MODE_FLAG_N*SYNC,
>>>>>> it was ignored,
>>>>>> because only PHSYNC and PVSYNC were taken into account.
>>>>>> DRM_MODE_FLAG_P*SYNC and DRM_MODE_FLAG_N*SYNC are not exclusive.
>>>>>>
>>>>>> If flags contains PVSYNC, it doesn't mean it is NVSYNC.
>>>>>> And it's true also the contrary.
>>>>>> Also, as I've checked with scope on A20,
>>>>>> if (flags & PVSYNC) then SUN4I_TCON0_IO_POL_VSYNC_POSITIVE
>>>>>> must be set, as name suggests.
>>>>>> It seems all display io polarities starts inverted if 0.
>>>>>>
>>>>>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
>>>>>>
>>>>>> PVSYNC and PHSYNC only
>>>>>>
>>>>>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
>>>>>
>>>>> Checkpatch:
>>>>> WARNING: Duplicate signature
>>>>
>>>> Sorry I didn't use ./scripts/checkpatch.pl
>>>>
>>>>>
>>>>>> ---
>>>>>>  ? drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++--
>>>>>>  ? 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>>>> b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>>>> index 6121210..e873a37 100644
>>>>>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>>>> @@ -224,10 +224,10 @@ static void
>>>>>> sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
>>>>>>  ?????????????? SUN4I_TCON0_BASIC3_H_SYNC(hsync));
>>>>>>  ????? /* Setup the polarity of the various signals */
>>>>>> -??? if (!(mode->flags & DRM_MODE_FLAG_PHSYNC))
>>>>>> +??? if (mode->flags & DRM_MODE_FLAG_PHSYNC)
>>>>>>  ????????? val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
>>>>>> -??? if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
>>>>>> +??? if (mode->flags & DRM_MODE_FLAG_PVSYNC)
>>>>>>  ????????? val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
>>>>>
>>>>> I'm not sure why you were talking of the differences between NVSYNC
>>>>> and PVSYNC if you're not making use of any of it here?
>>>>
>>>> Thinking about it more now, the point is that all Lcd IOs seem to be
>>>> inverted by default(at least on A20).
>>>> With inverted, I mean that if for example PVSYNC,
>>>> I should see vsync line low and when asserted to give VSync,
>>>> it goes high.
>>>> This is what I've checked with oscilloscope on A20.
>>>> Can someone give a try on A33? Otherwise I will,
>>>> but I will take some time.
>>>> On uboot, everything is treated equal to kernel,
>>>> but to have my falling edge dclk and low h/vsync I had to specify:
>>>> CONFIG_VIDEO_LCD_DCLK_PHASE=0 (giving me falling edge on dclk)
>>>> and
>>>> CONFIG_VIDEO_LCD_MODE="....,sync:3,..."
>>>> but digging into code, I see "sync:3" means H/VSYNC HIGH,
>>>> but I experience both LOW during their pulse.
>>>>
>>>>>
>>>>> Also, how was it tested? This seems quite weird that we haven't caught
>>>>> that one sooner, and I'm a bit worried about the possible regressions
>>>>> here.
>>>>
>>>> It sounds really strange to me too,
>>>> because everybody under uboot use "sync:3"(HIGH).
>>>> I will retry to measure,
>>>> unfortunately at home I don't have a scope,
>>>> but I think I'm going to have one soon, because of this. :)
>>>
>>> Here I am with scope captures and tcon0 registers dump:
>>> tcon0_regs => https://pasteboard.co/H4r8Zcs.png
>>> dclk_d0 => https://pasteboard.co/H4r8QRe.png
>>> dclk_de => https://pasteboard.co/H4r8zh4R.png
>>> dclk_vsnc => https://pasteboard.co/H4r8Hye.png
>>>
>>> As you can see circled in reg on registers,
>>> TCON0_IO_POL_REG = 0x00000000.
>>> But on all the waveforms you can see:
>>> - dclk_d0: clock phase is 0, but it starts with falling edge, otherwise
>>> the rising front overlaps dclk rising edge(not good), so to me this is
>>> falling, then I mean it Negative.
>>> - dclk_de: de pulse is clearly negative, even if register is 0 and its'
>>> polarity bit is 0.
>>> - dclk_vsnc: same as dclk_de
>>> - dclk_hsync: I didn't take scope screenshot but I can assure you it's
>>> negative.
>>>
>>> You can also check all the other registers about TCON0.
>>>
>>> Now I proceed testing it on A33, maybe the peripheral is slightly
>>> different between Axx SoCs, if I find it that way,
>>> it should be only a check about SoC or peripheral ID,
>>> and treat polarity as it should be done.
>>
>> Here I am with A33 waveforms:
>> tcon0_regs => https://pasteboard.co/H4rXfN0M.png
>> dclk_d0 => https://pasteboard.co/H4rVXwy.png
>> dclk_de => https://pasteboard.co/H4rWDt8.png
>> dclk_vsnc => https://pasteboard.co/H4rWRACu.png
>> dclk_hsync => https://pasteboard.co/H4rWK6I.png
> 
> Thanks, that's really helpful.
> 
>> It behaves the same way as A20, so as I mean IO polarity,
>> all signals(except D0-D23), are inverted.
>> For A33 I've used A33-OLinuXino.
>> For A20 our LiNova1.
> 
> Indeed, HSYNC and VSYNC look inverted. 

Yes, so they should be inverted inside the driver.

> I don't really know what the
> polarity of D0 would be just by judging at that capture, but we would
> have noticed if the colors were inverted for quite some time now.

D0-D23 are correct.
With that capture,
I mean to show you instead dclk is inverted,
as dclk samples D0 on falling edge.
So 0 is NEGEDGE and 1 is POSEDGE(1/3 of clock phase).
1/3 clock phase seems enough to me to be considered POSEDGE,
2/3 instead risks to go too much to the right of D0(even if it could work).

> 
> DE seems to be active high though, since it's only going to be at a
> logical low level when data are not transmitted, so during the blank
> periods.

Yes, you're right, DE is data enable, and is asserted high as 0.
But it must be added.
I'm planning to send a new patchset with all these things corrected for 
kernel.

A little out of thread but:
I'd like to send one for u-boot too,
but this means also to modify every sunxi "sync:3" to "sync:0" and 
vice-versa.

What do you think?

> 
> Maxime
> 


-- 
Giulio Benetti
R&D Manager &
Advanced Research

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale ? 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

[PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly

[Maxime Ripard]

On Thu, Jan 25, 2018 at 05:50:18PM +0100, Giulio Benetti wrote:
> > > > > > On Sat, Jan 20, 2018 at 07:50:21PM +0100, Giulio Benetti wrote:
> > > > > > > On previous handling, if specified DRM_MODE_FLAG_N*SYNC,
> > > > > > > it was ignored,
> > > > > > > because only PHSYNC and PVSYNC were taken into account.
> > > > > > > DRM_MODE_FLAG_P*SYNC and DRM_MODE_FLAG_N*SYNC are not exclusive.
> > > > > > > 
> > > > > > > If flags contains PVSYNC, it doesn't mean it is NVSYNC.
> > > > > > > And it's true also the contrary.
> > > > > > > Also, as I've checked with scope on A20,
> > > > > > > if (flags & PVSYNC) then SUN4I_TCON0_IO_POL_VSYNC_POSITIVE
> > > > > > > must be set, as name suggests.
> > > > > > > It seems all display io polarities starts inverted if 0.
> > > > > > > 
> > > > > > > Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> > > > > > > 
> > > > > > > PVSYNC and PHSYNC only
> > > > > > > 
> > > > > > > Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> > > > > > 
> > > > > > Checkpatch:
> > > > > > WARNING: Duplicate signature
> > > > > 
> > > > > Sorry I didn't use ./scripts/checkpatch.pl
> > > > > 
> > > > > > 
> > > > > > > ---
> > > > > > >  ? drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++--
> > > > > > >  ? 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > > > > b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > > > > index 6121210..e873a37 100644
> > > > > > > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > > > > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > > > > @@ -224,10 +224,10 @@ static void
> > > > > > > sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
> > > > > > >  ?????????????? SUN4I_TCON0_BASIC3_H_SYNC(hsync));
> > > > > > >  ????? /* Setup the polarity of the various signals */
> > > > > > > -??? if (!(mode->flags & DRM_MODE_FLAG_PHSYNC))
> > > > > > > +??? if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> > > > > > >  ????????? val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
> > > > > > > -??? if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
> > > > > > > +??? if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> > > > > > >  ????????? val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
> > > > > > 
> > > > > > I'm not sure why you were talking of the differences between NVSYNC
> > > > > > and PVSYNC if you're not making use of any of it here?
> > > > > 
> > > > > Thinking about it more now, the point is that all Lcd IOs seem to be
> > > > > inverted by default(at least on A20).
> > > > > With inverted, I mean that if for example PVSYNC,
> > > > > I should see vsync line low and when asserted to give VSync,
> > > > > it goes high.
> > > > > This is what I've checked with oscilloscope on A20.
> > > > > Can someone give a try on A33? Otherwise I will,
> > > > > but I will take some time.
> > > > > On uboot, everything is treated equal to kernel,
> > > > > but to have my falling edge dclk and low h/vsync I had to specify:
> > > > > CONFIG_VIDEO_LCD_DCLK_PHASE=0 (giving me falling edge on dclk)
> > > > > and
> > > > > CONFIG_VIDEO_LCD_MODE="....,sync:3,..."
> > > > > but digging into code, I see "sync:3" means H/VSYNC HIGH,
> > > > > but I experience both LOW during their pulse.
> > > > > 
> > > > > > 
> > > > > > Also, how was it tested? This seems quite weird that we haven't caught
> > > > > > that one sooner, and I'm a bit worried about the possible regressions
> > > > > > here.
> > > > > 
> > > > > It sounds really strange to me too,
> > > > > because everybody under uboot use "sync:3"(HIGH).
> > > > > I will retry to measure,
> > > > > unfortunately at home I don't have a scope,
> > > > > but I think I'm going to have one soon, because of this. :)
> > > > 
> > > > Here I am with scope captures and tcon0 registers dump:
> > > > tcon0_regs => https://pasteboard.co/H4r8Zcs.png
> > > > dclk_d0 => https://pasteboard.co/H4r8QRe.png
> > > > dclk_de => https://pasteboard.co/H4r8zh4R.png
> > > > dclk_vsnc => https://pasteboard.co/H4r8Hye.png
> > > > 
> > > > As you can see circled in reg on registers,
> > > > TCON0_IO_POL_REG = 0x00000000.
> > > > But on all the waveforms you can see:
> > > > - dclk_d0: clock phase is 0, but it starts with falling edge, otherwise
> > > > the rising front overlaps dclk rising edge(not good), so to me this is
> > > > falling, then I mean it Negative.
> > > > - dclk_de: de pulse is clearly negative, even if register is 0 and its'
> > > > polarity bit is 0.
> > > > - dclk_vsnc: same as dclk_de
> > > > - dclk_hsync: I didn't take scope screenshot but I can assure you it's
> > > > negative.
> > > > 
> > > > You can also check all the other registers about TCON0.
> > > > 
> > > > Now I proceed testing it on A33, maybe the peripheral is slightly
> > > > different between Axx SoCs, if I find it that way,
> > > > it should be only a check about SoC or peripheral ID,
> > > > and treat polarity as it should be done.
> > > 
> > > Here I am with A33 waveforms:
> > > tcon0_regs => https://pasteboard.co/H4rXfN0M.png
> > > dclk_d0 => https://pasteboard.co/H4rVXwy.png
> > > dclk_de => https://pasteboard.co/H4rWDt8.png
> > > dclk_vsnc => https://pasteboard.co/H4rWRACu.png
> > > dclk_hsync => https://pasteboard.co/H4rWK6I.png
> > 
> > Thanks, that's really helpful.
> > 
> > > It behaves the same way as A20, so as I mean IO polarity,
> > > all signals(except D0-D23), are inverted.
> > > For A33 I've used A33-OLinuXino.
> > > For A20 our LiNova1.
> > 
> > Indeed, HSYNC and VSYNC look inverted.
> 
> Yes, so they should be inverted inside the driver.

Yep. And the LCD panels used on our boards as well in order to avoid
any breakages.

> > I don't really know what the polarity of D0 would be just by
> > judging at that capture, but we would have noticed if the colors
> > were inverted for quite some time now.
> 
> D0-D23 are correct.
>
> With that capture, I mean to show you instead dclk is inverted, as
> dclk samples D0 on falling edge.

Ah right, DCLK being the first channel?

> So 0 is NEGEDGE and 1 is POSEDGE(1/3 of clock phase).
> 1/3 clock phase seems enough to me to be considered POSEDGE,
> 2/3 instead risks to go too much to the right of D0(even if it could work).

Do you have captures with both settings?

> > DE seems to be active high though, since it's only going to be at
> > a logical low level when data are not transmitted, so during the
> > blank periods.
> 
> Yes, you're right, DE is data enable, and is asserted high as 0.

No, it is asserted high as 1

> But it must be added.
> I'm planning to send a new patchset with all these things corrected for
> kernel.

Ok.

> A little out of thread but:
> I'd like to send one for u-boot too,
> but this means also to modify every sunxi "sync:3" to "sync:0" and
> vice-versa.
> 
> What do you think?

That it's going to be a nightmare... We've advertised since the very
beginning something, and we're about to break it. I'm not sure we want
to do that.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180126/62bccf5c/attachment.sig>

[PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly

[Giulio Benetti]

Hi,

Il 26/01/2018 15:56, Maxime Ripard ha scritto:
> On Thu, Jan 25, 2018 at 05:50:18PM +0100, Giulio Benetti wrote:
>>>>>>> On Sat, Jan 20, 2018 at 07:50:21PM +0100, Giulio Benetti wrote:
>>>>>>>> On previous handling, if specified DRM_MODE_FLAG_N*SYNC,
>>>>>>>> it was ignored,
>>>>>>>> because only PHSYNC and PVSYNC were taken into account.
>>>>>>>> DRM_MODE_FLAG_P*SYNC and DRM_MODE_FLAG_N*SYNC are not exclusive.
>>>>>>>>
>>>>>>>> If flags contains PVSYNC, it doesn't mean it is NVSYNC.
>>>>>>>> And it's true also the contrary.
>>>>>>>> Also, as I've checked with scope on A20,
>>>>>>>> if (flags & PVSYNC) then SUN4I_TCON0_IO_POL_VSYNC_POSITIVE
>>>>>>>> must be set, as name suggests.
>>>>>>>> It seems all display io polarities starts inverted if 0.
>>>>>>>>
>>>>>>>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
>>>>>>>>
>>>>>>>> PVSYNC and PHSYNC only
>>>>>>>>
>>>>>>>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
>>>>>>>
>>>>>>> Checkpatch:
>>>>>>> WARNING: Duplicate signature
>>>>>>
>>>>>> Sorry I didn't use ./scripts/checkpatch.pl
>>>>>>
>>>>>>>
>>>>>>>> ---
>>>>>>>>   ? drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++--
>>>>>>>>   ? 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>>>>>> b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>>>>>> index 6121210..e873a37 100644
>>>>>>>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>>>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>>>>>> @@ -224,10 +224,10 @@ static void
>>>>>>>> sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
>>>>>>>>   ?????????????? SUN4I_TCON0_BASIC3_H_SYNC(hsync));
>>>>>>>>   ????? /* Setup the polarity of the various signals */
>>>>>>>> -??? if (!(mode->flags & DRM_MODE_FLAG_PHSYNC))
>>>>>>>> +??? if (mode->flags & DRM_MODE_FLAG_PHSYNC)
>>>>>>>>   ????????? val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
>>>>>>>> -??? if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
>>>>>>>> +??? if (mode->flags & DRM_MODE_FLAG_PVSYNC)
>>>>>>>>   ????????? val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
>>>>>>>
>>>>>>> I'm not sure why you were talking of the differences between NVSYNC
>>>>>>> and PVSYNC if you're not making use of any of it here?
>>>>>>
>>>>>> Thinking about it more now, the point is that all Lcd IOs seem to be
>>>>>> inverted by default(at least on A20).
>>>>>> With inverted, I mean that if for example PVSYNC,
>>>>>> I should see vsync line low and when asserted to give VSync,
>>>>>> it goes high.
>>>>>> This is what I've checked with oscilloscope on A20.
>>>>>> Can someone give a try on A33? Otherwise I will,
>>>>>> but I will take some time.
>>>>>> On uboot, everything is treated equal to kernel,
>>>>>> but to have my falling edge dclk and low h/vsync I had to specify:
>>>>>> CONFIG_VIDEO_LCD_DCLK_PHASE=0 (giving me falling edge on dclk)
>>>>>> and
>>>>>> CONFIG_VIDEO_LCD_MODE="....,sync:3,..."
>>>>>> but digging into code, I see "sync:3" means H/VSYNC HIGH,
>>>>>> but I experience both LOW during their pulse.
>>>>>>
>>>>>>>
>>>>>>> Also, how was it tested? This seems quite weird that we haven't caught
>>>>>>> that one sooner, and I'm a bit worried about the possible regressions
>>>>>>> here.
>>>>>>
>>>>>> It sounds really strange to me too,
>>>>>> because everybody under uboot use "sync:3"(HIGH).
>>>>>> I will retry to measure,
>>>>>> unfortunately at home I don't have a scope,
>>>>>> but I think I'm going to have one soon, because of this. :)
>>>>>
>>>>> Here I am with scope captures and tcon0 registers dump:
>>>>> tcon0_regs => https://pasteboard.co/H4r8Zcs.png
>>>>> dclk_d0 => https://pasteboard.co/H4r8QRe.png
>>>>> dclk_de => https://pasteboard.co/H4r8zh4R.png
>>>>> dclk_vsnc => https://pasteboard.co/H4r8Hye.png
>>>>>
>>>>> As you can see circled in reg on registers,
>>>>> TCON0_IO_POL_REG = 0x00000000.
>>>>> But on all the waveforms you can see:
>>>>> - dclk_d0: clock phase is 0, but it starts with falling edge, otherwise
>>>>> the rising front overlaps dclk rising edge(not good), so to me this is
>>>>> falling, then I mean it Negative.
>>>>> - dclk_de: de pulse is clearly negative, even if register is 0 and its'
>>>>> polarity bit is 0.
>>>>> - dclk_vsnc: same as dclk_de
>>>>> - dclk_hsync: I didn't take scope screenshot but I can assure you it's
>>>>> negative.
>>>>>
>>>>> You can also check all the other registers about TCON0.
>>>>>
>>>>> Now I proceed testing it on A33, maybe the peripheral is slightly
>>>>> different between Axx SoCs, if I find it that way,
>>>>> it should be only a check about SoC or peripheral ID,
>>>>> and treat polarity as it should be done.
>>>>
>>>> Here I am with A33 waveforms:
>>>> tcon0_regs => https://pasteboard.co/H4rXfN0M.png
>>>> dclk_d0 => https://pasteboard.co/H4rVXwy.png
>>>> dclk_de => https://pasteboard.co/H4rWDt8.png
>>>> dclk_vsnc => https://pasteboard.co/H4rWRACu.png
>>>> dclk_hsync => https://pasteboard.co/H4rWK6I.png
>>>
>>> Thanks, that's really helpful.
>>>
>>>> It behaves the same way as A20, so as I mean IO polarity,
>>>> all signals(except D0-D23), are inverted.
>>>> For A33 I've used A33-OLinuXino.
>>>> For A20 our LiNova1.
>>>
>>> Indeed, HSYNC and VSYNC look inverted.
>>
>> Yes, so they should be inverted inside the driver.
> 
> Yep. And the LCD panels used on our boards as well in order to avoid
> any breakages.

Can you provide a list?
Or is there a way I can find it on my own?
I can create a whole patch-set providing this too on panel-simple.c
Ok?

> 
>>> I don't really know what the polarity of D0 would be just by
>>> judging at that capture, but we would have noticed if the colors
>>> were inverted for quite some time now.
>>
>> D0-D23 are correct.
>>
>> With that capture, I mean to show you instead dclk is inverted, as
>> dclk samples D0 on falling edge.
> 
> Ah right, DCLK being the first channel?

Yes, sorry I didn't place a label on channels

> 
>> So 0 is NEGEDGE and 1 is POSEDGE(1/3 of clock phase).
>> 1/3 clock phase seems enough to me to be considered POSEDGE,
>> 2/3 instead risks to go too much to the right of D0(even if it could work).
> 
> Do you have captures with both settings?

Not now, but asap I'm going to take.

> 
>>> DE seems to be active high though, since it's only going to be at
>>> a logical low level when data are not transmitted, so during the
>>> blank periods.
>>
>> Yes, you're right, DE is data enable, and is asserted high as 0.
> 
> No, it is asserted high as 1

Sorry, I wanted to tell it is asserted high with IO_POL register bit 
cleared to 0. So we're saying same thing now.

> 
>> But it must be added.
>> I'm planning to send a new patchset with all these things corrected for
>> kernel.
> 
> Ok.
> 
>> A little out of thread but:
>> I'd like to send one for u-boot too,
>> but this means also to modify every sunxi "sync:3" to "sync:0" and
>> vice-versa.
>>
>> What do you think?
> 
> That it's going to be a nightmare... We've advertised since the very
> beginning something, and we're about to break it. I'm not sure we want
> to do that.

I can take care about that.
But I also think that a lot of displays work because they use only 
DE-mode, almost ignoring HSync and VSync signals(HV-mode).

In any case I have to produce these patches because of my company's 
board based on A20 and A33, and modify defconfig according to it.
The only technical nightmare I see is to produce a commit for every 
defconfig to be modified and copy-paste che commit-log substituing board 
name(1-2 days of work).
Problem is testing, but we're speaking about something that probably was 
badly working, but you couldn't see it on display.
So I think this is only an improvement at the end.

I'm sorry I've taken bad news.
Drink 1 glass of Spritz to go over! :)

> 
> Thanks!
> Maxime
> 


-- 
Giulio Benetti
R&D Manager &
Advanced Research

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale ? 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

[PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly

[Giulio Benetti]

Il 26/01/2018 16:55, Giulio Benetti ha scritto:
> Hi,
> 
> Il 26/01/2018 15:56, Maxime Ripard ha scritto:
>> On Thu, Jan 25, 2018 at 05:50:18PM +0100, Giulio Benetti wrote:
>>>>>>>> On Sat, Jan 20, 2018 at 07:50:21PM +0100, Giulio Benetti wrote:
>>>>>>>>> On previous handling, if specified DRM_MODE_FLAG_N*SYNC,
>>>>>>>>> it was ignored,
>>>>>>>>> because only PHSYNC and PVSYNC were taken into account.
>>>>>>>>> DRM_MODE_FLAG_P*SYNC and DRM_MODE_FLAG_N*SYNC are not exclusive.
>>>>>>>>>
>>>>>>>>> If flags contains PVSYNC, it doesn't mean it is NVSYNC.
>>>>>>>>> And it's true also the contrary.
>>>>>>>>> Also, as I've checked with scope on A20,
>>>>>>>>> if (flags & PVSYNC) then SUN4I_TCON0_IO_POL_VSYNC_POSITIVE
>>>>>>>>> must be set, as name suggests.
>>>>>>>>> It seems all display io polarities starts inverted if 0.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
>>>>>>>>>
>>>>>>>>> PVSYNC and PHSYNC only
>>>>>>>>>
>>>>>>>>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
>>>>>>>>
>>>>>>>> Checkpatch:
>>>>>>>> WARNING: Duplicate signature
>>>>>>>
>>>>>>> Sorry I didn't use ./scripts/checkpatch.pl
>>>>>>>
>>>>>>>>
>>>>>>>>> ---
>>>>>>>>> ? ? drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++--
>>>>>>>>> ? ? 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>>>>>>> b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>>>>>>> index 6121210..e873a37 100644
>>>>>>>>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>>>>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>>>>>>> @@ -224,10 +224,10 @@ static void
>>>>>>>>> sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
>>>>>>>>> ? ?????????????? SUN4I_TCON0_BASIC3_H_SYNC(hsync));
>>>>>>>>> ? ????? /* Setup the polarity of the various signals */
>>>>>>>>> -??? if (!(mode->flags & DRM_MODE_FLAG_PHSYNC))
>>>>>>>>> +??? if (mode->flags & DRM_MODE_FLAG_PHSYNC)
>>>>>>>>> ? ????????? val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
>>>>>>>>> -??? if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
>>>>>>>>> +??? if (mode->flags & DRM_MODE_FLAG_PVSYNC)
>>>>>>>>> ? ????????? val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
>>>>>>>>
>>>>>>>> I'm not sure why you were talking of the differences between NVSYNC
>>>>>>>> and PVSYNC if you're not making use of any of it here?
>>>>>>>
>>>>>>> Thinking about it more now, the point is that all Lcd IOs seem to be
>>>>>>> inverted by default(at least on A20).
>>>>>>> With inverted, I mean that if for example PVSYNC,
>>>>>>> I should see vsync line low and when asserted to give VSync,
>>>>>>> it goes high.
>>>>>>> This is what I've checked with oscilloscope on A20.
>>>>>>> Can someone give a try on A33? Otherwise I will,
>>>>>>> but I will take some time.
>>>>>>> On uboot, everything is treated equal to kernel,
>>>>>>> but to have my falling edge dclk and low h/vsync I had to specify:
>>>>>>> CONFIG_VIDEO_LCD_DCLK_PHASE=0 (giving me falling edge on dclk)
>>>>>>> and
>>>>>>> CONFIG_VIDEO_LCD_MODE="....,sync:3,..."
>>>>>>> but digging into code, I see "sync:3" means H/VSYNC HIGH,
>>>>>>> but I experience both LOW during their pulse.
>>>>>>>
>>>>>>>>
>>>>>>>> Also, how was it tested? This seems quite weird that we haven't 
>>>>>>>> caught
>>>>>>>> that one sooner, and I'm a bit worried about the possible 
>>>>>>>> regressions
>>>>>>>> here.
>>>>>>>
>>>>>>> It sounds really strange to me too,
>>>>>>> because everybody under uboot use "sync:3"(HIGH).
>>>>>>> I will retry to measure,
>>>>>>> unfortunately at home I don't have a scope,
>>>>>>> but I think I'm going to have one soon, because of this. :)
>>>>>>
>>>>>> Here I am with scope captures and tcon0 registers dump:
>>>>>> tcon0_regs => https://pasteboard.co/H4r8Zcs.png
>>>>>> dclk_d0 => https://pasteboard.co/H4r8QRe.png
>>>>>> dclk_de => https://pasteboard.co/H4r8zh4R.png
>>>>>> dclk_vsnc => https://pasteboard.co/H4r8Hye.png
>>>>>>
>>>>>> As you can see circled in reg on registers,
>>>>>> TCON0_IO_POL_REG = 0x00000000.
>>>>>> But on all the waveforms you can see:
>>>>>> - dclk_d0: clock phase is 0, but it starts with falling edge, 
>>>>>> otherwise
>>>>>> the rising front overlaps dclk rising edge(not good), so to me 
>>>>>> this is
>>>>>> falling, then I mean it Negative.
>>>>>> - dclk_de: de pulse is clearly negative, even if register is 0 and 
>>>>>> its'
>>>>>> polarity bit is 0.
>>>>>> - dclk_vsnc: same as dclk_de
>>>>>> - dclk_hsync: I didn't take scope screenshot but I can assure you 
>>>>>> it's
>>>>>> negative.
>>>>>>
>>>>>> You can also check all the other registers about TCON0.
>>>>>>
>>>>>> Now I proceed testing it on A33, maybe the peripheral is slightly
>>>>>> different between Axx SoCs, if I find it that way,
>>>>>> it should be only a check about SoC or peripheral ID,
>>>>>> and treat polarity as it should be done.
>>>>>
>>>>> Here I am with A33 waveforms:
>>>>> tcon0_regs => https://pasteboard.co/H4rXfN0M.png
>>>>> dclk_d0 => https://pasteboard.co/H4rVXwy.png
>>>>> dclk_de => https://pasteboard.co/H4rWDt8.png
>>>>> dclk_vsnc => https://pasteboard.co/H4rWRACu.png
>>>>> dclk_hsync => https://pasteboard.co/H4rWK6I.png
>>>>
>>>> Thanks, that's really helpful.
>>>>
>>>>> It behaves the same way as A20, so as I mean IO polarity,
>>>>> all signals(except D0-D23), are inverted.
>>>>> For A33 I've used A33-OLinuXino.
>>>>> For A20 our LiNova1.
>>>>
>>>> Indeed, HSYNC and VSYNC look inverted.
>>>
>>> Yes, so they should be inverted inside the driver.
>>
>> Yep. And the LCD panels used on our boards as well in order to avoid
>> any breakages.
> 
> Can you provide a list?
> Or is there a way I can find it on my own?
> I can create a whole patch-set providing this too on panel-simple.c
> Ok?

Maybe not, or only a few part of them.
This is because of what I write below.

> 
>>
>>>> I don't really know what the polarity of D0 would be just by
>>>> judging at that capture, but we would have noticed if the colors
>>>> were inverted for quite some time now.
>>>
>>> D0-D23 are correct.
>>>
>>> With that capture, I mean to show you instead dclk is inverted, as
>>> dclk samples D0 on falling edge.
>>
>> Ah right, DCLK being the first channel?
> 
> Yes, sorry I didn't place a label on channels
> 
>>
>>> So 0 is NEGEDGE and 1 is POSEDGE(1/3 of clock phase).
>>> 1/3 clock phase seems enough to me to be considered POSEDGE,
>>> 2/3 instead risks to go too much to the right of D0(even if it could 
>>> work).
>>
>> Do you have captures with both settings?
> 
> Not now, but asap I'm going to take.

Here we are:
1/3 phase: https://pasteboard.co/H4VehON.png
2/3 phase: https://pasteboard.co/H4Veq8a.png

Yellow: D0
Blue: DCLK

As you can see:
1/3 phase has DCLK rising edge almost in the middle of D0
2/3 phase has DCLK rising edge that comes too late

I would go for "1/3 phase" for Rising edge
and "normal phase" for Falling edge.

What do you think?

> 
>>
>>>> DE seems to be active high though, since it's only going to be at
>>>> a logical low level when data are not transmitted, so during the
>>>> blank periods.
>>>
>>> Yes, you're right, DE is data enable, and is asserted high as 0.
>>
>> No, it is asserted high as 1
> 
> Sorry, I wanted to tell it is asserted high with IO_POL register bit 
> cleared to 0. So we're saying same thing now.
> 
>>
>>> But it must be added.
>>> I'm planning to send a new patchset with all these things corrected for
>>> kernel.
>>
>> Ok.
>>
>>> A little out of thread but:
>>> I'd like to send one for u-boot too,
>>> but this means also to modify every sunxi "sync:3" to "sync:0" and
>>> vice-versa.
>>>
>>> What do you think?
>>
>> That it's going to be a nightmare... We've advertised since the very
>> beginning something, and we're about to break it. I'm not sure we want
>> to do that.
> 
> I can take care about that.
> But I also think that a lot of displays work because they use only 
> DE-mode, almost ignoring HSync and VSync signals(HV-mode).
> 
> In any case I have to produce these patches because of my company's 
> board based on A20 and A33, and modify defconfig according to it.
> The only technical nightmare I see is to produce a commit for every 
> defconfig to be modified and copy-paste che commit-log substituing board 
> name(1-2 days of work).
> Problem is testing, but we're speaking about something that probably was 
> badly working, but you couldn't see it on display.
> So I think this is only an improvement at the end.
> 
> I'm sorry I've taken bad news.
> Drink 1 glass of Spritz to go over! :)

IMHO I think that we have only to take care about displays that don't 
have DE signal.
If DE signal exists, then displays are driven through DE only for back 
and front porch as I know, and on most displays I've used, Hsync and 
VSync are ignored.
DE is used not only for Data Enable, but also for sync the very 
beginning of frame, the rest of syncing is done by pause between every 
line sent.
This is should be why nobody noticed it before,
I think almost every display is used in DE mode only.
So, if we fix bug for HSync and VSync, risk should be very low.
Indeed, everybody or almost, use sync:3 because display ignore those 2 
signals (HSync and VSYnc) in favour of DE.
And I don't know how many people checked with oscilloscope signals after 
getting display working in a few.

So maybe we can keep all defconfig files untouched.
The same in Linux, prior displays have DE signal of course,
but nowadays it's difficult to me finding out a display without DE-mode.
I only remember I.MX31 not supporting DE-signal,
because at that time the most used sync system was HV-Mode,
principally done by Sharp as I recall right.

Maybe the nightmare is less important than we thought.

What about it?

Thanks

Giulio

> 
>>
>> Thanks!
>> Maxime
>>
> 
> 


-- 
Giulio Benetti
R&D Manager &
Advanced Research

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale ? 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

[PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly

[Maxime Ripard]

On Sat, Jan 27, 2018 at 11:07:09PM +0100, Giulio Benetti wrote:
> > > > > I don't really know what the polarity of D0 would be just by
> > > > > judging at that capture, but we would have noticed if the colors
> > > > > were inverted for quite some time now.
> > > > 
> > > > D0-D23 are correct.
> > > > 
> > > > With that capture, I mean to show you instead dclk is inverted, as
> > > > dclk samples D0 on falling edge.
> > > 
> > > Ah right, DCLK being the first channel?
> > 
> > Yes, sorry I didn't place a label on channels
> > 
> > > 
> > > > So 0 is NEGEDGE and 1 is POSEDGE(1/3 of clock phase).
> > > > 1/3 clock phase seems enough to me to be considered POSEDGE,
> > > > 2/3 instead risks to go too much to the right of D0(even if it
> > > > could work).
> > > 
> > > Do you have captures with both settings?
> > 
> > Not now, but asap I'm going to take.
> 
> Here we are:
> 1/3 phase: https://pasteboard.co/H4VehON.png
> 2/3 phase: https://pasteboard.co/H4Veq8a.png
> 
> Yellow: D0
> Blue: DCLK
> 
> As you can see:
> 1/3 phase has DCLK rising edge almost in the middle of D0
> 2/3 phase has DCLK rising edge that comes too late
> 
> I would go for "1/3 phase" for Rising edge and "normal phase" for
> Falling edge.
> 
> What do you think?

It seems fair. This need a whole lot of comments though :)

> > > That it's going to be a nightmare... We've advertised since the very
> > > beginning something, and we're about to break it. I'm not sure we want
> > > to do that.
> > 
> > I can take care about that.
> > But I also think that a lot of displays work because they use only
> > DE-mode, almost ignoring HSync and VSync signals(HV-mode).
> > 
> > In any case I have to produce these patches because of my company's
> > board based on A20 and A33, and modify defconfig according to it.
> > The only technical nightmare I see is to produce a commit for every
> > defconfig to be modified and copy-paste che commit-log substituing board
> > name(1-2 days of work).
> > Problem is testing, but we're speaking about something that probably was
> > badly working, but you couldn't see it on display.
> > So I think this is only an improvement at the end.
> > 
> > I'm sorry I've taken bad news.
> > Drink 1 glass of Spritz to go over! :)
> 
> IMHO I think that we have only to take care about displays that don't have
> DE signal.
>
> If DE signal exists, then displays are driven through DE only for back and
> front porch as I know, and on most displays I've used, Hsync and VSync are
> ignored.
> DE is used not only for Data Enable, but also for sync the very beginning of
> frame, the rest of syncing is done by pause between every line sent.
> This is should be why nobody noticed it before,
> I think almost every display is used in DE mode only.
> So, if we fix bug for HSync and VSync, risk should be very low.
> Indeed, everybody or almost, use sync:3 because display ignore those 2
> signals (HSync and VSYnc) in favour of DE.
> And I don't know how many people checked with oscilloscope signals after
> getting display working in a few.

I know I did, but I apparently didn't pay attention to that and was
more focused on getting the timings right :)

But clearly this is a separate discussion that needs to be held on the
U-Boot ML.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180201/1af6e84f/attachment.sig>

[PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly

[Giulio Benetti]

Il 01/02/2018 11:14, Maxime Ripard ha scritto:
> On Sat, Jan 27, 2018 at 11:07:09PM +0100, Giulio Benetti wrote:
>>>>>> I don't really know what the polarity of D0 would be just by
>>>>>> judging at that capture, but we would have noticed if the colors
>>>>>> were inverted for quite some time now.
>>>>>
>>>>> D0-D23 are correct.
>>>>>
>>>>> With that capture, I mean to show you instead dclk is inverted, as
>>>>> dclk samples D0 on falling edge.
>>>>
>>>> Ah right, DCLK being the first channel?
>>>
>>> Yes, sorry I didn't place a label on channels
>>>
>>>>
>>>>> So 0 is NEGEDGE and 1 is POSEDGE(1/3 of clock phase).
>>>>> 1/3 clock phase seems enough to me to be considered POSEDGE,
>>>>> 2/3 instead risks to go too much to the right of D0(even if it
>>>>> could work).
>>>>
>>>> Do you have captures with both settings?
>>>
>>> Not now, but asap I'm going to take.
>>
>> Here we are:
>> 1/3 phase: https://pasteboard.co/H4VehON.png
>> 2/3 phase: https://pasteboard.co/H4Veq8a.png
>>
>> Yellow: D0
>> Blue: DCLK
>>
>> As you can see:
>> 1/3 phase has DCLK rising edge almost in the middle of D0
>> 2/3 phase has DCLK rising edge that comes too late
>>
>> I would go for "1/3 phase" for Rising edge and "normal phase" for
>> Falling edge.
>>
>> What do you think?
> 
> It seems fair. This need a whole lot of comments though :)

Yes, then, do I proceed resubmitting both corrected patches with 
corrected commit logs?

> 
>>>> That it's going to be a nightmare... We've advertised since the very
>>>> beginning something, and we're about to break it. I'm not sure we want
>>>> to do that.
>>>
>>> I can take care about that.
>>> But I also think that a lot of displays work because they use only
>>> DE-mode, almost ignoring HSync and VSync signals(HV-mode).
>>>
>>> In any case I have to produce these patches because of my company's
>>> board based on A20 and A33, and modify defconfig according to it.
>>> The only technical nightmare I see is to produce a commit for every
>>> defconfig to be modified and copy-paste che commit-log substituing board
>>> name(1-2 days of work).
>>> Problem is testing, but we're speaking about something that probably was
>>> badly working, but you couldn't see it on display.
>>> So I think this is only an improvement at the end.
>>>
>>> I'm sorry I've taken bad news.
>>> Drink 1 glass of Spritz to go over! :)
>>
>> IMHO I think that we have only to take care about displays that don't have
>> DE signal.
>>
>> If DE signal exists, then displays are driven through DE only for back and
>> front porch as I know, and on most displays I've used, Hsync and VSync are
>> ignored.
>> DE is used not only for Data Enable, but also for sync the very beginning of
>> frame, the rest of syncing is done by pause between every line sent.
>> This is should be why nobody noticed it before,
>> I think almost every display is used in DE mode only.
>> So, if we fix bug for HSync and VSync, risk should be very low.
>> Indeed, everybody or almost, use sync:3 because display ignore those 2
>> signals (HSync and VSYnc) in favour of DE.
>> And I don't know how many people checked with oscilloscope signals after
>> getting display working in a few.
> 
> I know I did, but I apparently didn't pay attention to that and was
> more focused on getting the timings right :)
> 
> But clearly this is a separate discussion that needs to be held on the
> U-Boot ML.

Ok, so I'd create a patch regarding HSync and VSync polarity and send it 
to uboot ML.

> 
> Maxime
> 


-- 
Giulio Benetti
R&D Manager &
Advanced Research

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale ? 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

[PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly

[Maxime Ripard]

On Thu, Feb 01, 2018 at 05:09:33PM +0100, Giulio Benetti wrote:
> Il 01/02/2018 11:14, Maxime Ripard ha scritto:
> > On Sat, Jan 27, 2018 at 11:07:09PM +0100, Giulio Benetti wrote:
> > > > > > > I don't really know what the polarity of D0 would be just by
> > > > > > > judging at that capture, but we would have noticed if the colors
> > > > > > > were inverted for quite some time now.
> > > > > > 
> > > > > > D0-D23 are correct.
> > > > > > 
> > > > > > With that capture, I mean to show you instead dclk is inverted, as
> > > > > > dclk samples D0 on falling edge.
> > > > > 
> > > > > Ah right, DCLK being the first channel?
> > > > 
> > > > Yes, sorry I didn't place a label on channels
> > > > 
> > > > > 
> > > > > > So 0 is NEGEDGE and 1 is POSEDGE(1/3 of clock phase).
> > > > > > 1/3 clock phase seems enough to me to be considered POSEDGE,
> > > > > > 2/3 instead risks to go too much to the right of D0(even if it
> > > > > > could work).
> > > > > 
> > > > > Do you have captures with both settings?
> > > > 
> > > > Not now, but asap I'm going to take.
> > > 
> > > Here we are:
> > > 1/3 phase: https://pasteboard.co/H4VehON.png
> > > 2/3 phase: https://pasteboard.co/H4Veq8a.png
> > > 
> > > Yellow: D0
> > > Blue: DCLK
> > > 
> > > As you can see:
> > > 1/3 phase has DCLK rising edge almost in the middle of D0
> > > 2/3 phase has DCLK rising edge that comes too late
> > > 
> > > I would go for "1/3 phase" for Rising edge and "normal phase" for
> > > Falling edge.
> > > 
> > > What do you think?
> > 
> > It seems fair. This need a whole lot of comments though :)
> 
> Yes, then, do I proceed resubmitting both corrected patches with corrected
> commit logs?

Yes, please.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180205/0f492f4e/attachment.sig>

[PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly

[Maxime Ripard]

Hi,

On Fri, Jan 26, 2018 at 04:55:54PM +0100, Giulio Benetti wrote:
> > > > > It behaves the same way as A20, so as I mean IO polarity,
> > > > > all signals(except D0-D23), are inverted.
> > > > > For A33 I've used A33-OLinuXino.
> > > > > For A20 our LiNova1.
> > > > 
> > > > Indeed, HSYNC and VSYNC look inverted.
> > > 
> > > Yes, so they should be inverted inside the driver.
> > 
> > Yep. And the LCD panels used on our boards as well in order to avoid
> > any breakages.
> 
> Can you provide a list?
> Or is there a way I can find it on my own?
> I can create a whole patch-set providing this too on panel-simple.c
> Ok?

Grepping through the DT, it seems like there's only the A13 q8 tablet
and the SinA33. The former uses simple-panel, the latter a custom
panel driver.

> > > A little out of thread but:
> > > I'd like to send one for u-boot too,
> > > but this means also to modify every sunxi "sync:3" to "sync:0" and
> > > vice-versa.
> > > 
> > > What do you think?
> > 
> > That it's going to be a nightmare... We've advertised since the very
> > beginning something, and we're about to break it. I'm not sure we want
> > to do that.
> 
> I can take care about that.
> But I also think that a lot of displays work because they use only DE-mode,
> almost ignoring HSync and VSync signals(HV-mode).
> 
> In any case I have to produce these patches because of my company's board
> based on A20 and A33, and modify defconfig according to it.
> The only technical nightmare I see is to produce a commit for every
> defconfig to be modified and copy-paste che commit-log substituing board
> name(1-2 days of work).
> Problem is testing, but we're speaking about something that probably was
> badly working, but you couldn't see it on display.
> So I think this is only an improvement at the end.

It really is a separate discussion, but you would also need to fix all
the environments everywhere, which is simply not feasible.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180129/884737a3/attachment-0001.sig>

[PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly

[Maxime Ripard]

On Wed, Jan 24, 2018 at 08:37:28PM +0100, Giulio Benetti wrote:
> > > > Also, how was it tested? This seems quite weird that we haven't caught
> > > > that one sooner, and I'm a bit worried about the possible regressions
> > > > here.
> > > 
> > > It sounds really strange to me too,
> > > because everybody under uboot use "sync:3"(HIGH).
> > > I will retry to measure,
> > > unfortunately at home I don't have a scope,
> > > but I think I'm going to have one soon, because of this. :)
> > 
> > Here I am with scope captures and tcon0 registers dump:
> > tcon0_regs => https://pasteboard.co/H4r8Zcs.png
> > dclk_d0 => https://pasteboard.co/H4r8QRe.png
> > dclk_de => https://pasteboard.co/H4r8zh4R.png
> > dclk_vsnc => https://pasteboard.co/H4r8Hye.png
> > 
> > As you can see circled in reg on registers,
> > TCON0_IO_POL_REG = 0x00000000.
> > But on all the waveforms you can see:
> > - dclk_d0: clock phase is 0, but it starts with falling edge, otherwise
> > the rising front overlaps dclk rising edge(not good), so to me this is
> > falling, then I mean it Negative.
> > - dclk_de: de pulse is clearly negative, even if register is 0 and its'
> > polarity bit is 0.
> > - dclk_vsnc: same as dclk_de
> > - dclk_hsync: I didn't take scope screenshot but I can assure you it's
> > negative.
> > 
> > You can also check all the other registers about TCON0.
> > 
> > Now I proceed testing it on A33, maybe the peripheral is slightly
> > different between Axx SoCs, if I find it that way,
> > it should be only a check about SoC or peripheral ID,
> > and treat polarity as it should be done.
> 
> Here I am with A33 waveforms:
> tcon0_regs => https://pasteboard.co/H4rXfN0M.png
> dclk_d0 => https://pasteboard.co/H4rVXwy.png
> dclk_de => https://pasteboard.co/H4rWDt8.png
> dclk_vsnc => https://pasteboard.co/H4rWRACu.png
> dclk_hsync => https://pasteboard.co/H4rWK6I.png
> 
> It behaves the same way as A20, so as I mean IO polarity,
> all signals(except D0-D23), are inverted.
> For A33 I've used A33-OLinuXino.
> For A20 our LiNova1.

If you have an A33 handy, you probably want to read that mail:
https://lists.freedesktop.org/archives/dri-devel/2017-July/147951.html

Especially the 90-phase part.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180207/069af4a7/attachment.sig>

[PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly

[Giulio Benetti]

Hi,

Il 07/02/2018 11:39, Maxime Ripard ha scritto:
> On Wed, Jan 24, 2018 at 08:37:28PM +0100, Giulio Benetti wrote:
>>>>> Also, how was it tested? This seems quite weird that we haven't caught
>>>>> that one sooner, and I'm a bit worried about the possible regressions
>>>>> here.
>>>>
>>>> It sounds really strange to me too,
>>>> because everybody under uboot use "sync:3"(HIGH).
>>>> I will retry to measure,
>>>> unfortunately at home I don't have a scope,
>>>> but I think I'm going to have one soon, because of this. :)
>>>
>>> Here I am with scope captures and tcon0 registers dump:
>>> tcon0_regs => https://pasteboard.co/H4r8Zcs.png
>>> dclk_d0 => https://pasteboard.co/H4r8QRe.png
>>> dclk_de => https://pasteboard.co/H4r8zh4R.png
>>> dclk_vsnc => https://pasteboard.co/H4r8Hye.png
>>>
>>> As you can see circled in reg on registers,
>>> TCON0_IO_POL_REG = 0x00000000.
>>> But on all the waveforms you can see:
>>> - dclk_d0: clock phase is 0, but it starts with falling edge, otherwise
>>> the rising front overlaps dclk rising edge(not good), so to me this is
>>> falling, then I mean it Negative.
>>> - dclk_de: de pulse is clearly negative, even if register is 0 and its'
>>> polarity bit is 0.
>>> - dclk_vsnc: same as dclk_de
>>> - dclk_hsync: I didn't take scope screenshot but I can assure you it's
>>> negative.
>>>
>>> You can also check all the other registers about TCON0.
>>>
>>> Now I proceed testing it on A33, maybe the peripheral is slightly
>>> different between Axx SoCs, if I find it that way,
>>> it should be only a check about SoC or peripheral ID,
>>> and treat polarity as it should be done.
>>
>> Here I am with A33 waveforms:
>> tcon0_regs => https://pasteboard.co/H4rXfN0M.png
>> dclk_d0 => https://pasteboard.co/H4rVXwy.png
>> dclk_de => https://pasteboard.co/H4rWDt8.png
>> dclk_vsnc => https://pasteboard.co/H4rWRACu.png
>> dclk_hsync => https://pasteboard.co/H4rWK6I.png
>>
>> It behaves the same way as A20, so as I mean IO polarity,
>> all signals(except D0-D23), are inverted.
>> For A33 I've used A33-OLinuXino.
>> For A20 our LiNova1.
> 
> If you have an A33 handy, you probably want to read that mail:
> https://lists.freedesktop.org/archives/dri-devel/2017-July/147951.html
> 
> Especially the 90-phase part.

Here is a summary of different SoCs TCON:
With DCLK_Sel:
0x0 => normal phase
0x1 => 1/3 phase
0x2 => 2/3 phase

A10, A20

With DCLK_Sel:
0x0 => normal phase
0x1 => 1/3 phase
0x2 => 2/3 phase
0x5 => DCLK/2 phase 0
0x4 => DCLK/2 phase 90

A31, A31s, A33, A80, A83T

Also I've found that TCON1 has not this feature,
nor first, nor second case(at least is not described on user manuals).

So I could handle differently according to SoC.
Unfortunately there is not TCON register keeping IP version,
so the only way I see is to create a long if-or statement to understand
which kind of TCON we're using.

But what sounds not the best to me, is that DCLK is divided by 2 if
using phase 90. So we need to reconsider also bitclock driver according
to this.
I don't know if it make sense.

IMHO, I would keep only:
- As normal => "0x1 => 1/3 phase"
- As inverted => "0x0 => normal phase"

According to scope captures above on both A20 and A33.
Unfortunately I don't have other boards for the other SoCs to take captures.

What do you think?

> 
> Maxime
> 

-- 
Giulio Benetti
R&D Manager &
Advanced Research

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale ? 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

[PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly

[Maxime Ripard]

On Wed, Feb 07, 2018 at 01:49:59PM +0100, Giulio Benetti wrote:
> Hi,
> 
> Il 07/02/2018 11:39, Maxime Ripard ha scritto:
> > On Wed, Jan 24, 2018 at 08:37:28PM +0100, Giulio Benetti wrote:
> >>>>> Also, how was it tested? This seems quite weird that we haven't caught
> >>>>> that one sooner, and I'm a bit worried about the possible regressions
> >>>>> here.
> >>>>
> >>>> It sounds really strange to me too,
> >>>> because everybody under uboot use "sync:3"(HIGH).
> >>>> I will retry to measure,
> >>>> unfortunately at home I don't have a scope,
> >>>> but I think I'm going to have one soon, because of this. :)
> >>>
> >>> Here I am with scope captures and tcon0 registers dump:
> >>> tcon0_regs => https://pasteboard.co/H4r8Zcs.png
> >>> dclk_d0 => https://pasteboard.co/H4r8QRe.png
> >>> dclk_de => https://pasteboard.co/H4r8zh4R.png
> >>> dclk_vsnc => https://pasteboard.co/H4r8Hye.png
> >>>
> >>> As you can see circled in reg on registers,
> >>> TCON0_IO_POL_REG = 0x00000000.
> >>> But on all the waveforms you can see:
> >>> - dclk_d0: clock phase is 0, but it starts with falling edge, otherwise
> >>> the rising front overlaps dclk rising edge(not good), so to me this is
> >>> falling, then I mean it Negative.
> >>> - dclk_de: de pulse is clearly negative, even if register is 0 and its'
> >>> polarity bit is 0.
> >>> - dclk_vsnc: same as dclk_de
> >>> - dclk_hsync: I didn't take scope screenshot but I can assure you it's
> >>> negative.
> >>>
> >>> You can also check all the other registers about TCON0.
> >>>
> >>> Now I proceed testing it on A33, maybe the peripheral is slightly
> >>> different between Axx SoCs, if I find it that way,
> >>> it should be only a check about SoC or peripheral ID,
> >>> and treat polarity as it should be done.
> >>
> >> Here I am with A33 waveforms:
> >> tcon0_regs => https://pasteboard.co/H4rXfN0M.png
> >> dclk_d0 => https://pasteboard.co/H4rVXwy.png
> >> dclk_de => https://pasteboard.co/H4rWDt8.png
> >> dclk_vsnc => https://pasteboard.co/H4rWRACu.png
> >> dclk_hsync => https://pasteboard.co/H4rWK6I.png
> >>
> >> It behaves the same way as A20, so as I mean IO polarity,
> >> all signals(except D0-D23), are inverted.
> >> For A33 I've used A33-OLinuXino.
> >> For A20 our LiNova1.
> > 
> > If you have an A33 handy, you probably want to read that mail:
> > https://lists.freedesktop.org/archives/dri-devel/2017-July/147951.html
> > 
> > Especially the 90-phase part.
> 
> Here is a summary of different SoCs TCON:
> With DCLK_Sel:
> 0x0 => normal phase
> 0x1 => 1/3 phase
> 0x2 => 2/3 phase
> 
> A10, A20

Have you tested the option 4 and 5 there too?

> With DCLK_Sel:
> 0x0 => normal phase
> 0x1 => 1/3 phase
> 0x2 => 2/3 phase
> 0x5 => DCLK/2 phase 0
> 0x4 => DCLK/2 phase 90
> 
> A31, A31s, A33, A80, A83T

Ok, great, so Chen-Yu was right :)

I guess the option 5 (DCLK/2 phase 0) is still to early, just like
you've shown in the previous captures?

> Also I've found that TCON1 has not this feature,
> nor first, nor second case(at least is not described on user manuals).

At lot of things are not described, unfortunately...

> So I could handle differently according to SoC.
> Unfortunately there is not TCON register keeping IP version,
> so the only way I see is to create a long if-or statement to understand
> which kind of TCON we're using.

You can base that on the compatible, and add a field in the
sun4i_tcon_quirks structure, that will avoid the if statements you
mentionned.

> But what sounds not the best to me, is that DCLK is divided by 2 if
> using phase 90. So we need to reconsider also bitclock driver according
> to this.
> I don't know if it make sense.
> 
> IMHO, I would keep only:
> - As normal => "0x1 => 1/3 phase"

So that would mean sampling at raising edge on this one??

> - As inverted => "0x0 => normal phase"

And falling edge?

If so, and if remember the captures properly, the sampling would occur
right before the rise, and not really around the fall.

Would 2/3 be better here?

> According to scope captures above on both A20 and A33.
> Unfortunately I don't have other boards for the other SoCs to take captures.
> 
> What do you think?

I guess we can make that part applicable to all SoCs, we haven't seen
any significant differences on those part.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180208/5d5d9e7c/attachment.sig>

[PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly

[Chen-Yu Tsai]

On Fri, Feb 9, 2018 at 4:40 AM, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> On Wed, Feb 07, 2018 at 01:49:59PM +0100, Giulio Benetti wrote:
>> Hi,
>>
>> Il 07/02/2018 11:39, Maxime Ripard ha scritto:
>> > On Wed, Jan 24, 2018 at 08:37:28PM +0100, Giulio Benetti wrote:
>> >>>>> Also, how was it tested? This seems quite weird that we haven't caught
>> >>>>> that one sooner, and I'm a bit worried about the possible regressions
>> >>>>> here.
>> >>>>
>> >>>> It sounds really strange to me too,
>> >>>> because everybody under uboot use "sync:3"(HIGH).
>> >>>> I will retry to measure,
>> >>>> unfortunately at home I don't have a scope,
>> >>>> but I think I'm going to have one soon, because of this. :)
>> >>>
>> >>> Here I am with scope captures and tcon0 registers dump:
>> >>> tcon0_regs => https://pasteboard.co/H4r8Zcs.png
>> >>> dclk_d0 => https://pasteboard.co/H4r8QRe.png
>> >>> dclk_de => https://pasteboard.co/H4r8zh4R.png
>> >>> dclk_vsnc => https://pasteboard.co/H4r8Hye.png
>> >>>
>> >>> As you can see circled in reg on registers,
>> >>> TCON0_IO_POL_REG = 0x00000000.
>> >>> But on all the waveforms you can see:
>> >>> - dclk_d0: clock phase is 0, but it starts with falling edge, otherwise
>> >>> the rising front overlaps dclk rising edge(not good), so to me this is
>> >>> falling, then I mean it Negative.
>> >>> - dclk_de: de pulse is clearly negative, even if register is 0 and its'
>> >>> polarity bit is 0.
>> >>> - dclk_vsnc: same as dclk_de
>> >>> - dclk_hsync: I didn't take scope screenshot but I can assure you it's
>> >>> negative.
>> >>>
>> >>> You can also check all the other registers about TCON0.
>> >>>
>> >>> Now I proceed testing it on A33, maybe the peripheral is slightly
>> >>> different between Axx SoCs, if I find it that way,
>> >>> it should be only a check about SoC or peripheral ID,
>> >>> and treat polarity as it should be done.
>> >>
>> >> Here I am with A33 waveforms:
>> >> tcon0_regs => https://pasteboard.co/H4rXfN0M.png
>> >> dclk_d0 => https://pasteboard.co/H4rVXwy.png
>> >> dclk_de => https://pasteboard.co/H4rWDt8.png
>> >> dclk_vsnc => https://pasteboard.co/H4rWRACu.png
>> >> dclk_hsync => https://pasteboard.co/H4rWK6I.png
>> >>
>> >> It behaves the same way as A20, so as I mean IO polarity,
>> >> all signals(except D0-D23), are inverted.
>> >> For A33 I've used A33-OLinuXino.
>> >> For A20 our LiNova1.
>> >
>> > If you have an A33 handy, you probably want to read that mail:
>> > https://lists.freedesktop.org/archives/dri-devel/2017-July/147951.html
>> >
>> > Especially the 90-phase part.
>>
>> Here is a summary of different SoCs TCON:
>> With DCLK_Sel:
>> 0x0 => normal phase
>> 0x1 => 1/3 phase
>> 0x2 => 2/3 phase
>>
>> A10, A20
>
> Have you tested the option 4 and 5 there too?
>
>> With DCLK_Sel:
>> 0x0 => normal phase
>> 0x1 => 1/3 phase
>> 0x2 => 2/3 phase
>> 0x5 => DCLK/2 phase 0
>> 0x4 => DCLK/2 phase 90
>>
>> A31, A31s, A33, A80, A83T
>
> Ok, great, so Chen-Yu was right :)
>
> I guess the option 5 (DCLK/2 phase 0) is still to early, just like
> you've shown in the previous captures?
>
>> Also I've found that TCON1 has not this feature,
>> nor first, nor second case(at least is not described on user manuals).
>
> At lot of things are not described, unfortunately...

On some SoCs, TCON1 does not have channel 0 (LCD), so it does
not have a configurable dot clock, so no settings.

ChenYu

>> So I could handle differently according to SoC.
>> Unfortunately there is not TCON register keeping IP version,
>> so the only way I see is to create a long if-or statement to understand
>> which kind of TCON we're using.
>
> You can base that on the compatible, and add a field in the
> sun4i_tcon_quirks structure, that will avoid the if statements you
> mentionned.
>
>> But what sounds not the best to me, is that DCLK is divided by 2 if
>> using phase 90. So we need to reconsider also bitclock driver according
>> to this.
>> I don't know if it make sense.
>>
>> IMHO, I would keep only:
>> - As normal => "0x1 => 1/3 phase"
>
> So that would mean sampling at raising edge on this one??
>
>> - As inverted => "0x0 => normal phase"
>
> And falling edge?
>
> If so, and if remember the captures properly, the sampling would occur
> right before the rise, and not really around the fall.
>
> Would 2/3 be better here?
>
>> According to scope captures above on both A20 and A33.
>> Unfortunately I don't have other boards for the other SoCs to take captures.
>>
>> What do you think?
>
> I guess we can make that part applicable to all SoCs, we haven't seen
> any significant differences on those part.
>
> Maxime
>
> --
> Maxime Ripard, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> http://bootlin.com
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

[PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly

[Giulio Benetti]

Hi,

Il 08/02/2018 21:40, Maxime Ripard ha scritto:
> On Wed, Feb 07, 2018 at 01:49:59PM +0100, Giulio Benetti wrote:
>> Hi,
>>
>> Il 07/02/2018 11:39, Maxime Ripard ha scritto:
>>> On Wed, Jan 24, 2018 at 08:37:28PM +0100, Giulio Benetti wrote:
>>>>>>> Also, how was it tested? This seems quite weird that we haven't caught
>>>>>>> that one sooner, and I'm a bit worried about the possible regressions
>>>>>>> here.
>>>>>>
>>>>>> It sounds really strange to me too,
>>>>>> because everybody under uboot use "sync:3"(HIGH).
>>>>>> I will retry to measure,
>>>>>> unfortunately at home I don't have a scope,
>>>>>> but I think I'm going to have one soon, because of this. :)
>>>>>
>>>>> Here I am with scope captures and tcon0 registers dump:
>>>>> tcon0_regs => https://pasteboard.co/H4r8Zcs.png
>>>>> dclk_d0 => https://pasteboard.co/H4r8QRe.png
>>>>> dclk_de => https://pasteboard.co/H4r8zh4R.png
>>>>> dclk_vsnc => https://pasteboard.co/H4r8Hye.png
>>>>>
>>>>> As you can see circled in reg on registers,
>>>>> TCON0_IO_POL_REG = 0x00000000.
>>>>> But on all the waveforms you can see:
>>>>> - dclk_d0: clock phase is 0, but it starts with falling edge, otherwise
>>>>> the rising front overlaps dclk rising edge(not good), so to me this is
>>>>> falling, then I mean it Negative.
>>>>> - dclk_de: de pulse is clearly negative, even if register is 0 and its'
>>>>> polarity bit is 0.
>>>>> - dclk_vsnc: same as dclk_de
>>>>> - dclk_hsync: I didn't take scope screenshot but I can assure you it's
>>>>> negative.
>>>>>
>>>>> You can also check all the other registers about TCON0.
>>>>>
>>>>> Now I proceed testing it on A33, maybe the peripheral is slightly
>>>>> different between Axx SoCs, if I find it that way,
>>>>> it should be only a check about SoC or peripheral ID,
>>>>> and treat polarity as it should be done.
>>>>
>>>> Here I am with A33 waveforms:
>>>> tcon0_regs => https://pasteboard.co/H4rXfN0M.png
>>>> dclk_d0 => https://pasteboard.co/H4rVXwy.png
>>>> dclk_de => https://pasteboard.co/H4rWDt8.png
>>>> dclk_vsnc => https://pasteboard.co/H4rWRACu.png
>>>> dclk_hsync => https://pasteboard.co/H4rWK6I.png
>>>>
>>>> It behaves the same way as A20, so as I mean IO polarity,
>>>> all signals(except D0-D23), are inverted.
>>>> For A33 I've used A33-OLinuXino.
>>>> For A20 our LiNova1.
>>>
>>> If you have an A33 handy, you probably want to read that mail:
>>> https://lists.freedesktop.org/archives/dri-devel/2017-July/147951.html
>>>
>>> Especially the 90-phase part.
>>
>> Here is a summary of different SoCs TCON:
>> With DCLK_Sel:
>> 0x0 => normal phase
>> 0x1 => 1/3 phase
>> 0x2 => 2/3 phase
>>
>> A10, A20
> 
> Have you tested the option 4 and 5 there too?
> 
>> With DCLK_Sel:
>> 0x0 => normal phase
>> 0x1 => 1/3 phase
>> 0x2 => 2/3 phase
>> 0x5 => DCLK/2 phase 0
>> 0x4 => DCLK/2 phase 90
>>
>> A31, A31s, A33, A80, A83T
> 
> Ok, great, so Chen-Yu was right :)
> 
> I guess the option 5 (DCLK/2 phase 0) is still to early, just like
> you've shown in the previous captures?

Exactly, it is like this:
https://pasteboard.co/H4r8QRe.png but with clock divided by 2.

> 
>> Also I've found that TCON1 has not this feature,
>> nor first, nor second case(at least is not described on user manuals).
> 
> At lot of things are not described, unfortunately...
> 
>> So I could handle differently according to SoC.
>> Unfortunately there is not TCON register keeping IP version,
>> so the only way I see is to create a long if-or statement to understand
>> which kind of TCON we're using.
> 
> You can base that on the compatible, and add a field in the
> sun4i_tcon_quirks structure, that will avoid the if statements you
> mentionned.
> 
>> But what sounds not the best to me, is that DCLK is divided by 2 if
>> using phase 90. So we need to reconsider also bitclock driver according
>> to this.
>> I don't know if it make sense.
>>
>> IMHO, I would keep only:
>> - As normal => "0x1 => 1/3 phase"
> 
> So that would mean sampling at raising edge on this one??

Exactly rising edge.

> 
>> - As inverted => "0x0 => normal phase"
> 
> And falling edge?

Yes.

> 
> If so, and if remember the captures properly, the sampling would occur
> right before the rise, and not really around the fall.
> 
> Would 2/3 be better here?

Yes, you're right, 2/3 phase is better:

1/3 phase: https://pasteboard.co/H4VehON.png
2/3 phase: https://pasteboard.co/H4Veq8a.png

Take a look at the bit in middle(yellow) sampled by clock(blue).

Rising edge is almost in the middle of D0 bit.

> 
>> According to scope captures above on both A20 and A33.
>> Unfortunately I don't have other boards for the other SoCs to take captures.
>>
>> What do you think?
> 
> I guess we can make that part applicable to all SoCs, we haven't seen
> any significant differences on those part.

So let's keep:
- As normal(rising edge) => IO_POL_REG "0x2 => 2/3 phase"
- As inverted(falling edge) => IO_POL_REG "0x0 => normal phase"

Ok?

> 
> Maxime
> 


-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale ? 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

[PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly

[Maxime Ripard]

On Thu, Feb 15, 2018 at 07:05:56PM +0100, Giulio Benetti wrote:
> > If so, and if remember the captures properly, the sampling would occur
> > right before the rise, and not really around the fall.
> > 
> > Would 2/3 be better here?
> 
> Yes, you're right, 2/3 phase is better:
> 
> 1/3 phase: https://pasteboard.co/H4VehON.png
> 2/3 phase: https://pasteboard.co/H4Veq8a.png
> 
> Take a look at the bit in middle(yellow) sampled by clock(blue).
> 
> Rising edge is almost in the middle of D0 bit.
> 
> > 
> > > According to scope captures above on both A20 and A33.
> > > Unfortunately I don't have other boards for the other SoCs to take captures.
> > > 
> > > What do you think?
> > 
> > I guess we can make that part applicable to all SoCs, we haven't seen
> > any significant differences on those part.
> 
> So let's keep:
> - As normal(rising edge) => IO_POL_REG "0x2 => 2/3 phase"
> - As inverted(falling edge) => IO_POL_REG "0x0 => normal phase"

I was actually thinking 1/3 for rising, 2/3 for falling.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180216/af2ccffd/attachment.sig>

[PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly

[Giulio Benetti]

Hi,

Il 16/02/2018 16:50, Maxime Ripard ha scritto:
> On Thu, Feb 15, 2018 at 07:05:56PM +0100, Giulio Benetti wrote:
>>> If so, and if remember the captures properly, the sampling would occur
>>> right before the rise, and not really around the fall.
>>>
>>> Would 2/3 be better here?
>>
>> Yes, you're right, 2/3 phase is better:
>>
>> 1/3 phase: https://pasteboard.co/H4VehON.png
>> 2/3 phase: https://pasteboard.co/H4Veq8a.png
>>
>> Take a look at the bit in middle(yellow) sampled by clock(blue).
>>
>> Rising edge is almost in the middle of D0 bit.
>>
>>>
>>>> According to scope captures above on both A20 and A33.
>>>> Unfortunately I don't have other boards for the other SoCs to take captures.
>>>>
>>>> What do you think?
>>>
>>> I guess we can make that part applicable to all SoCs, we haven't seen
>>> any significant differences on those part.
>>
>> So let's keep:
>> - As normal(rising edge) => IO_POL_REG "0x2 => 2/3 phase"
>> - As inverted(falling edge) => IO_POL_REG "0x0 => normal phase"
> 
> I was actually thinking 1/3 for rising, 2/3 for falling.

1/3 is almost the same waveform as D0,
having rising an falling edges almost in sync with D0.
It's not so clear because DCLK has a bad figure,
but it is that way.

2/3 instead is almost in the middle of D0 as rising.

Summarizing:
- use 0/3 as falling, then DRM_BUS_FLAG_PIXDATA_NEGEDGE
- use 2/3 as rising, then DRM_BUS_FLAG_PIXDATA_POSEDGE

I follow with a new patch using clk_set_phase function.

> 
> Maxime
> 


-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale ? 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

[PATCH 1/2] drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE

[Maxime Ripard]

Hi,

On Sat, Jan 20, 2018 at 07:50:20PM +0100, Giulio Benetti wrote:
> Can't set dclk polarity on sun4i.
> 
> Handle both positive and negative dclk polarity,
> according to bus_flags.

It's not really that we can't set it, it's that it's been ignored.

> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> ---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index f4284b5..6121210 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -17,6 +17,7 @@
>  #include <drm/drm_encoder.h>
>  #include <drm/drm_modes.h>
>  #include <drm/drm_of.h>
> +#include <drm/drm_panel.h>
>  
>  #include <uapi/drm/drm_mode.h>
>  
> @@ -173,6 +174,9 @@ static void sun4i_tcon0_mode_set_common(struct sun4i_tcon *tcon,
>  static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
>  				     const struct drm_display_mode *mode)
>  {
> +	struct drm_panel *panel = tcon->panel;
> +	struct drm_connector *connector = panel->connector;
> +	struct drm_display_info display_info = connector->display_info;
>  	unsigned int bp, hsync, vsync;
>  	u8 clk_delay;
>  	u32 val = 0;
> @@ -226,8 +230,13 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
>  	if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
>  		val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
>  
> +	if(display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_POSEDGE)
> +		val |= SUN4I_TCON0_IO_POL_DCLK_PHASE(1);
> +

Checkpatch here returns:
ERROR: space required before the open parenthesis '('

And, where did you find that info? How does it interact with the
set_phase callback of the dotclock driver?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180122/fb1e86d8/attachment.sig>