[PATCH] drm/sun4i: fix HSYNC and VSYNC polarity

[PATCH] drm/sun4i: fix HSYNC and VSYNC polarity

[Giulio Benetti]

Differently from other Lcd signals, HSYNC and VSYNC signals
result inverted if their bits are cleared to 0.

Invert their settings of IO_POL register.

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 3c15cf2..aaf911a 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -389,10 +389,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;
 
 	regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
-- 
2.7.4

Re: [PATCH] drm/sun4i: fix HSYNC and VSYNC polarity

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 443 bytes --]

On Thu, Feb 15, 2018 at 06:54:48PM +0100, Giulio Benetti wrote:
> Differently from other Lcd signals, HSYNC and VSYNC signals
> result inverted if their bits are cleared to 0.
> 
> Invert their settings of IO_POL register.
> 
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>

Applied, thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm/sun4i: fix HSYNC and VSYNC polarity

[Jonathan Liu]

Hi Giulio,

On Thu, 15 Feb 2018 at 17:54, Giulio Benetti
<giulio.benetti@micronovasrl.com> wrote:
>
> Differently from other Lcd signals, HSYNC and VSYNC signals
> result inverted if their bits are cleared to 0.
>
> Invert their settings of IO_POL register.
>
> 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 3c15cf2..aaf911a 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -389,10 +389,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;
>
>         regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,

I am running Linux 4.19.6 and noticed with Olimex LCD-OLinuXino-7TS 7"
LCD touchscreen (Innolux AT070TN92) connected to Olimex
A20-OLinuXino-MICRO that the image does not display correctly after
this change.
The image is shifted to the right.

Reverting the change results in the image being displayed correctly on
the screen.

I have in the device tree a "panel" node with compatible =
"innolux,at070tn92" which uses the timings in
drivers/gpu/drm/panel/panel-simple.c.

Any ideas?

Thanks.

Regards,
Jonathan

Re: [PATCH] drm/sun4i: fix HSYNC and VSYNC polarity

[Giulio Benetti]

Hi Jonathan,

Il 06/12/2018 08:29, Jonathan Liu ha scritto:
> Hi Giulio,
> 
> On Thu, 15 Feb 2018 at 17:54, Giulio Benetti
> <giulio.benetti@micronovasrl.com> wrote:
>>
>> Differently from other Lcd signals, HSYNC and VSYNC signals
>> result inverted if their bits are cleared to 0.
>>
>> Invert their settings of IO_POL register.
>>
>> 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 3c15cf2..aaf911a 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> @@ -389,10 +389,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;
>>
>>          regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
> 
> I am running Linux 4.19.6 and noticed with Olimex LCD-OLinuXino-7TS 7"
> LCD touchscreen (Innolux AT070TN92) connected to Olimex
> A20-OLinuXino-MICRO that the image does not display correctly after
> this change.
> The image is shifted to the right.
> 
> Reverting the change results in the image being displayed correctly on
> the screen.
> 
> I have in the device tree a "panel" node with compatible =
> "innolux,at070tn92" which uses the timings in
> drivers/gpu/drm/panel/panel-simple.c.
> 
> Any ideas?

Checking Display Datasheet:
https://www.olimex.com/Products/Retired/A13-LCD7-TS/resources/S700-AT070TN92.pdf

Page 13 section 3.3.2 you can see it needs active low VS and HS.

You can refer to this Thread and check scope captures about VS and HS 
versus TCON0_IOPOL register:
https://lists.freedesktop.org/archives/dri-devel/2018-January/163874.html

There should be something that wrongly sets one of these or both:
mode->flags |= DRM_MODE_FLAG_PHSYNC;
and/or
mode->flags |= DRM_MODE_FLAG_PVSYNC;

Checked in panel-simple.c but it's not there.

At the moment I don't have a board to check it with me, I'll try to do 
it soon, but I'm full with other work at the moment.

If you have the chance to debug you could try to find in which point 
mode->flags gets changed with those 2 flags.

When the problem came out I've noticed the same thing for u-boot too but 
it's been decided to not patch it because in that case every single 
sunxi defconfig had to be changed from:
CONFIG_VIDEO_LCD_MODE="...,sync:3,..."
to:
CONFIG_VIDEO_LCD_MODE="...,sync:0,..."
and it would have been a great mess, probably not worth it:
https://lists.denx.de/pipermail/u-boot/2018-February/321405.html

So keep in mind that TCON driver works differently for HS and 
VS(inverted) polarity in u-boot and linux.

Hope to work this out soon.
Best regards
-- 
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

Re: [PATCH] drm/sun4i: fix HSYNC and VSYNC polarity

[Jonathan Liu]

Hi Giulio,

On Thu, 6 Dec 2018 at 22:00, Giulio Benetti
<giulio.benetti@micronovasrl.com> wrote:
>
> Hi Jonathan,
>
> Il 06/12/2018 08:29, Jonathan Liu ha scritto:
> > Hi Giulio,
> >
> > On Thu, 15 Feb 2018 at 17:54, Giulio Benetti
> > <giulio.benetti@micronovasrl.com> wrote:
> >>
> >> Differently from other Lcd signals, HSYNC and VSYNC signals
> >> result inverted if their bits are cleared to 0.
> >>
> >> Invert their settings of IO_POL register.
> >>
> >> 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 3c15cf2..aaf911a 100644
> >> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >> @@ -389,10 +389,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;
> >>
> >>          regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
> >
> > I am running Linux 4.19.6 and noticed with Olimex LCD-OLinuXino-7TS 7"
> > LCD touchscreen (Innolux AT070TN92) connected to Olimex
> > A20-OLinuXino-MICRO that the image does not display correctly after
> > this change.
> > The image is shifted to the right.
> >
> > Reverting the change results in the image being displayed correctly on
> > the screen.
> >
> > I have in the device tree a "panel" node with compatible =
> > "innolux,at070tn92" which uses the timings in
> > drivers/gpu/drm/panel/panel-simple.c.
> >
> > Any ideas?

>
> Checking Display Datasheet:
> https://www.olimex.com/Products/Retired/A13-LCD7-TS/resources/S700-AT070TN92.pdf
>
> Page 13 section 3.3.2 you can see it needs active low VS and HS.
>
> You can refer to this Thread and check scope captures about VS and HS
> versus TCON0_IOPOL register:
> https://lists.freedesktop.org/archives/dri-devel/2018-January/163874.html
>
> There should be something that wrongly sets one of these or both:
> mode->flags |= DRM_MODE_FLAG_PHSYNC;
> and/or
> mode->flags |= DRM_MODE_FLAG_PVSYNC;
>
> Checked in panel-simple.c but it's not there.

flags is 0 because it is not assigned in the struct definition for the panel.
Before this change, TCON0_IO_POL_REG would be 0x03000000 (both bits
set to 1) and image displays correctly.
After this change, TCON0_IO_POL_REG is 0x00000000 (both bits set to 0)
and image doesn't display correctly.

Checked using "devmem2 0x01c0c088" on A20-OLinuXino-MICRO Rev J.

>
> At the moment I don't have a board to check it with me, I'll try to do
> it soon, but I'm full with other work at the moment.
>
> If you have the chance to debug you could try to find in which point
> mode->flags gets changed with those 2 flags.
>
> When the problem came out I've noticed the same thing for u-boot too but
> it's been decided to not patch it because in that case every single
> sunxi defconfig had to be changed from:
> CONFIG_VIDEO_LCD_MODE="...,sync:3,..."
> to:
> CONFIG_VIDEO_LCD_MODE="...,sync:0,..."
> and it would have been a great mess, probably not worth it:
> https://lists.denx.de/pipermail/u-boot/2018-February/321405.html
>
> So keep in mind that TCON driver works differently for HS and
> VS(inverted) polarity in u-boot and linux.
>
> Hope to work this out soon.

Regards,
Jonathan

Re: [PATCH] drm/sun4i: fix HSYNC and VSYNC polarity

[Giulio Benetti]

Hi Jonathan,

Il 11/12/2018 11:49, Jonathan Liu ha scritto:
> Hi Giulio,
> 
> On Thu, 6 Dec 2018 at 22:00, Giulio Benetti
> <giulio.benetti@micronovasrl.com> wrote:
>>
>> Hi Jonathan,
>>
>> Il 06/12/2018 08:29, Jonathan Liu ha scritto:
>>> Hi Giulio,
>>>
>>> On Thu, 15 Feb 2018 at 17:54, Giulio Benetti
>>> <giulio.benetti@micronovasrl.com> wrote:
>>>>
>>>> Differently from other Lcd signals, HSYNC and VSYNC signals
>>>> result inverted if their bits are cleared to 0.
>>>>
>>>> Invert their settings of IO_POL register.
>>>>
>>>> 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 3c15cf2..aaf911a 100644
>>>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>> @@ -389,10 +389,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;
>>>>
>>>>           regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
>>>
>>> I am running Linux 4.19.6 and noticed with Olimex LCD-OLinuXino-7TS 7"
>>> LCD touchscreen (Innolux AT070TN92) connected to Olimex
>>> A20-OLinuXino-MICRO that the image does not display correctly after
>>> this change.
>>> The image is shifted to the right.
>>>
>>> Reverting the change results in the image being displayed correctly on
>>> the screen.
>>>
>>> I have in the device tree a "panel" node with compatible =
>>> "innolux,at070tn92" which uses the timings in
>>> drivers/gpu/drm/panel/panel-simple.c.
>>>
>>> Any ideas?
> 
>>
>> Checking Display Datasheet:
>> https://www.olimex.com/Products/Retired/A13-LCD7-TS/resources/S700-AT070TN92.pdf
>>
>> Page 13 section 3.3.2 you can see it needs active low VS and HS.
>>
>> You can refer to this Thread and check scope captures about VS and HS
>> versus TCON0_IOPOL register:
>> https://lists.freedesktop.org/archives/dri-devel/2018-January/163874.html
>>
>> There should be something that wrongly sets one of these or both:
>> mode->flags |= DRM_MODE_FLAG_PHSYNC;
>> and/or
>> mode->flags |= DRM_MODE_FLAG_PVSYNC;
>>
>> Checked in panel-simple.c but it's not there.
> 
> flags is 0 because it is not assigned in the struct definition for the panel.

I don't think it is 0, because otherwise IO_POL_REG wouldn't be set to 
0x03000000 but to 0.
What is checked is exactly mode->flags, so the problem seems to be upstream.

This is my doubt, it seems mode->flags is not initialized or overriden 
at a certain point, this is why I want to debug it with Jtag tomorrow.

> Before this change, TCON0_IO_POL_REG would be 0x03000000 (both bits
> set to 1) and image displays correctly > After this change, TCON0_IO_POL_REG is 0x00000000 (both bits set to 0)
> and image doesn't display correctly.
> 
> Checked using "devmem2 0x01c0c088" on A20-OLinuXino-MICRO Rev J.

0x03000000 as I've triple checked with scope means Positive H/Vsync,
and 0x00000000 Negative H/VSync.

Please check on the Thread I've pointed you above where there are all 
the links to the scope captures.

Are you completely sure you're using the correct panel?
This is because if with 0x03000000 it works correctly, it means that 
you're using Positive VS and HS but on datasheet on Figure 3.2 it shows 
that they must be negative.

Do you have any chance to measure those signals with a scope?

Tomorrow, while debugging, I'll re-check H/Vsync signals again.

Kind regards
-- 
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

Re: [PATCH] drm/sun4i: fix HSYNC and VSYNC polarity

[Giulio Benetti]

Forgot to ask you,

Il 11/12/2018 18:20, Giulio Benetti ha scritto:
> Hi Jonathan,
> 
> Il 11/12/2018 11:49, Jonathan Liu ha scritto:
>> Hi Giulio,
>>
>> On Thu, 6 Dec 2018 at 22:00, Giulio Benetti
>> <giulio.benetti@micronovasrl.com> wrote:
>>>
>>> Hi Jonathan,
>>>
>>> Il 06/12/2018 08:29, Jonathan Liu ha scritto:
>>>> Hi Giulio,
>>>>
>>>> On Thu, 15 Feb 2018 at 17:54, Giulio Benetti
>>>> <giulio.benetti@micronovasrl.com> wrote:
>>>>>
>>>>> Differently from other Lcd signals, HSYNC and VSYNC signals
>>>>> result inverted if their bits are cleared to 0.
>>>>>
>>>>> Invert their settings of IO_POL register.
>>>>>
>>>>> 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 3c15cf2..aaf911a 100644
>>>>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>>> @@ -389,10 +389,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;
>>>>>
>>>>>           regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
>>>>
>>>> I am running Linux 4.19.6 and noticed with Olimex LCD-OLinuXino-7TS 7"
>>>> LCD touchscreen (Innolux AT070TN92) connected to Olimex
>>>> A20-OLinuXino-MICRO that the image does not display correctly after
>>>> this change.
>>>> The image is shifted to the right.
>>>>
>>>> Reverting the change results in the image being displayed correctly on
>>>> the screen.
>>>>
>>>> I have in the device tree a "panel" node with compatible =
>>>> "innolux,at070tn92" which uses the timings in
>>>> drivers/gpu/drm/panel/panel-simple.c.
>>>>
>>>> Any ideas?
>>
>>>
>>> Checking Display Datasheet:
>>> https://www.olimex.com/Products/Retired/A13-LCD7-TS/resources/S700-AT070TN92.pdf 
>>>
>>>
>>> Page 13 section 3.3.2 you can see it needs active low VS and HS.
>>>
>>> You can refer to this Thread and check scope captures about VS and HS
>>> versus TCON0_IOPOL register:
>>> https://lists.freedesktop.org/archives/dri-devel/2018-January/163874.html 
>>>
>>>
>>> There should be something that wrongly sets one of these or both:
>>> mode->flags |= DRM_MODE_FLAG_PHSYNC;
>>> and/or
>>> mode->flags |= DRM_MODE_FLAG_PVSYNC;
>>>
>>> Checked in panel-simple.c but it's not there.
>>
>> flags is 0 because it is not assigned in the struct definition for the 
>> panel.
> 
> I don't think it is 0, because otherwise IO_POL_REG wouldn't be set to 
> 0x03000000 but to 0.
> What is checked is exactly mode->flags, so the problem seems to be 
> upstream.
> 
> This is my doubt, it seems mode->flags is not initialized or overriden 
> at a certain point, this is why I want to debug it with Jtag tomorrow.
> 
>> Before this change, TCON0_IO_POL_REG would be 0x03000000 (both bits
>> set to 1) and image displays correctly > After this change, 
>> TCON0_IO_POL_REG is 0x00000000 (both bits set to 0)
>> and image doesn't display correctly.
>>
>> Checked using "devmem2 0x01c0c088" on A20-OLinuXino-MICRO Rev J.
> 
> 0x03000000 as I've triple checked with scope means Positive H/Vsync,
> and 0x00000000 Negative H/VSync.
> 
> Please check on the Thread I've pointed you above where there are all 
> the links to the scope captures.
> 
> Are you completely sure you're using the correct panel?
> This is because if with 0x03000000 it works correctly, it means that 
> you're using Positive VS and HS but on datasheet on Figure 3.2 it shows 
> that they must be negative.
> 
> Do you have any chance to measure those signals with a scope?
> 
> Tomorrow, while debugging, I'll re-check H/Vsync signals again.
> 
> Kind regards

Can you precisely point me the sources of:
- u-boot
- kernel
- dts

you're using?

Thanks

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

Re: [PATCH] drm/sun4i: fix HSYNC and VSYNC polarity

[Jonathan Liu]

Hi Giulio,

On Wed, 12 Dec 2018 at 04:39, Giulio Benetti
<giulio.benetti@micronovasrl.com> wrote:
>
> Forgot to ask you,
>
> Il 11/12/2018 18:20, Giulio Benetti ha scritto:
> > Hi Jonathan,
> >
> > Il 11/12/2018 11:49, Jonathan Liu ha scritto:
> >> Hi Giulio,
> >>
> >> On Thu, 6 Dec 2018 at 22:00, Giulio Benetti
> >> <giulio.benetti@micronovasrl.com> wrote:
> >>>
> >>> Hi Jonathan,
> >>>
> >>> Il 06/12/2018 08:29, Jonathan Liu ha scritto:
> >>>> Hi Giulio,
> >>>>
> >>>> On Thu, 15 Feb 2018 at 17:54, Giulio Benetti
> >>>> <giulio.benetti@micronovasrl.com> wrote:
> >>>>>
> >>>>> Differently from other Lcd signals, HSYNC and VSYNC signals
> >>>>> result inverted if their bits are cleared to 0.
> >>>>>
> >>>>> Invert their settings of IO_POL register.
> >>>>>
> >>>>> 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 3c15cf2..aaf911a 100644
> >>>>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >>>>> @@ -389,10 +389,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;
> >>>>>
> >>>>>           regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
> >>>>
> >>>> I am running Linux 4.19.6 and noticed with Olimex LCD-OLinuXino-7TS 7"
> >>>> LCD touchscreen (Innolux AT070TN92) connected to Olimex
> >>>> A20-OLinuXino-MICRO that the image does not display correctly after
> >>>> this change.
> >>>> The image is shifted to the right.
> >>>>
> >>>> Reverting the change results in the image being displayed correctly on
> >>>> the screen.
> >>>>
> >>>> I have in the device tree a "panel" node with compatible =
> >>>> "innolux,at070tn92" which uses the timings in
> >>>> drivers/gpu/drm/panel/panel-simple.c.
> >>>>
> >>>> Any ideas?
> >>
> >>>
> >>> Checking Display Datasheet:
> >>> https://www.olimex.com/Products/Retired/A13-LCD7-TS/resources/S700-AT070TN92.pdf
> >>>
> >>>
> >>> Page 13 section 3.3.2 you can see it needs active low VS and HS.
> >>>
> >>> You can refer to this Thread and check scope captures about VS and HS
> >>> versus TCON0_IOPOL register:
> >>> https://lists.freedesktop.org/archives/dri-devel/2018-January/163874.html
> >>>
> >>>
> >>> There should be something that wrongly sets one of these or both:
> >>> mode->flags |= DRM_MODE_FLAG_PHSYNC;
> >>> and/or
> >>> mode->flags |= DRM_MODE_FLAG_PVSYNC;
> >>>
> >>> Checked in panel-simple.c but it's not there.
> >>
> >> flags is 0 because it is not assigned in the struct definition for the
> >> panel.
> >
> > I don't think it is 0, because otherwise IO_POL_REG wouldn't be set to
> > 0x03000000 but to 0.
> > What is checked is exactly mode->flags, so the problem seems to be
> > upstream.
> >
> > This is my doubt, it seems mode->flags is not initialized or overriden
> > at a certain point, this is why I want to debug it with Jtag tomorrow.
> >
> >> Before this change, TCON0_IO_POL_REG would be 0x03000000 (both bits
> >> set to 1) and image displays correctly > After this change,
> >> TCON0_IO_POL_REG is 0x00000000 (both bits set to 0)
> >> and image doesn't display correctly.
> >>
> >> Checked using "devmem2 0x01c0c088" on A20-OLinuXino-MICRO Rev J.
> >
> > 0x03000000 as I've triple checked with scope means Positive H/Vsync,
> > and 0x00000000 Negative H/VSync.
> >
> > Please check on the Thread I've pointed you above where there are all
> > the links to the scope captures.
> >
> > Are you completely sure you're using the correct panel?
> > This is because if with 0x03000000 it works correctly, it means that
> > you're using Positive VS and HS but on datasheet on Figure 3.2 it shows
> > that they must be negative.
> >
> > Do you have any chance to measure those signals with a scope?
> >
> > Tomorrow, while debugging, I'll re-check H/Vsync signals again.
> >
> > Kind regards

>
> Can you precisely point me the sources of:
> - u-boot
> - kernel
> - dts
>
> you're using?
>
> Thanks

U-Boot - git tag v2017.03
Linux - git tag v4.19.6
DTS - see device tree changes in
https://github.com/net147/linux/tree/sun7i-drm-wip but change the
compatible to "innolux,at070tn92"

Thanks.

Regards,
Jonathan

Re: [PATCH] drm/sun4i: fix HSYNC and VSYNC polarity

[Jonathan Liu]

Hi Giulio,

On Wed, 12 Dec 2018 at 04:20, Giulio Benetti
<giulio.benetti@micronovasrl.com> wrote:
>
> Hi Jonathan,
>
> Il 11/12/2018 11:49, Jonathan Liu ha scritto:
> > Hi Giulio,
> >
> > On Thu, 6 Dec 2018 at 22:00, Giulio Benetti
> > <giulio.benetti@micronovasrl.com> wrote:
> >>
> >> Hi Jonathan,
> >>
> >> Il 06/12/2018 08:29, Jonathan Liu ha scritto:
> >>> Hi Giulio,
> >>>
> >>> On Thu, 15 Feb 2018 at 17:54, Giulio Benetti
> >>> <giulio.benetti@micronovasrl.com> wrote:
> >>>>
> >>>> Differently from other Lcd signals, HSYNC and VSYNC signals
> >>>> result inverted if their bits are cleared to 0.
> >>>>
> >>>> Invert their settings of IO_POL register.
> >>>>
> >>>> 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 3c15cf2..aaf911a 100644
> >>>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >>>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >>>> @@ -389,10 +389,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;
> >>>>
> >>>>           regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
> >>>
> >>> I am running Linux 4.19.6 and noticed with Olimex LCD-OLinuXino-7TS 7"
> >>> LCD touchscreen (Innolux AT070TN92) connected to Olimex
> >>> A20-OLinuXino-MICRO that the image does not display correctly after
> >>> this change.
> >>> The image is shifted to the right.
> >>>
> >>> Reverting the change results in the image being displayed correctly on
> >>> the screen.
> >>>
> >>> I have in the device tree a "panel" node with compatible =
> >>> "innolux,at070tn92" which uses the timings in
> >>> drivers/gpu/drm/panel/panel-simple.c.
> >>>
> >>> Any ideas?
> >
> >>
> >> Checking Display Datasheet:
> >> https://www.olimex.com/Products/Retired/A13-LCD7-TS/resources/S700-AT070TN92.pdf
> >>
> >> Page 13 section 3.3.2 you can see it needs active low VS and HS.
> >>
> >> You can refer to this Thread and check scope captures about VS and HS
> >> versus TCON0_IOPOL register:
> >> https://lists.freedesktop.org/archives/dri-devel/2018-January/163874.html
> >>
> >> There should be something that wrongly sets one of these or both:
> >> mode->flags |= DRM_MODE_FLAG_PHSYNC;
> >> and/or
> >> mode->flags |= DRM_MODE_FLAG_PVSYNC;
> >>
> >> Checked in panel-simple.c but it's not there.

> >
> > flags is 0 because it is not assigned in the struct definition for the panel.
>
> I don't think it is 0, because otherwise IO_POL_REG wouldn't be set to
> 0x03000000 but to 0.
> What is checked is exactly mode->flags, so the problem seems to be upstream.
>
> This is my doubt, it seems mode->flags is not initialized or overriden
> at a certain point, this is why I want to debug it with Jtag tomorrow.
>

If you look at the change made by your patch:
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -389,10 +389,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;

        regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,

If mode->flags is 0, then before your change it would set
SUN4I_TCON0_IO_POL_HSYNC_POSITIVE and
SUN4I_TCON0_IO_POL_VSYNC_POSITIVE bits to 1 (resulting in 0x03000000).
If mode->flags is not 0, then after your change it would not set
SUN4I_TCON0_IO_POL_HSYNC_POSITIVE and
SUN4I_TCON0_IO_POL_VSYNC_POSITIVE bits to 1 (resulting in 0x00000000).

> > Before this change, TCON0_IO_POL_REG would be 0x03000000 (both bits
> > set to 1) and image displays correctly > After this change, TCON0_IO_POL_REG is 0x00000000 (both bits set to 0)
> > and image doesn't display correctly.
> >
> > Checked using "devmem2 0x01c0c088" on A20-OLinuXino-MICRO Rev J.
>
> 0x03000000 as I've triple checked with scope means Positive H/Vsync,
> and 0x00000000 Negative H/VSync.
>
> Please check on the Thread I've pointed you above where there are all
> the links to the scope captures.
>
> Are you completely sure you're using the correct panel?

Yes, I am sure I am using the correct panel. It has the following
marking on sticker:
AT070TN92V.X 89A070ZZ-0K1

> This is because if with 0x03000000 it works correctly, it means that
> you're using Positive VS and HS but on datasheet on Figure 3.2 it shows
> that they must be negative.
>
> Do you have any chance to measure those signals with a scope?

No.

>
> Tomorrow, while debugging, I'll re-check H/Vsync signals again.
>

Thanks.

Regards,
Jonathan

Re: [PATCH] drm/sun4i: fix HSYNC and VSYNC polarity

[Giulio Benetti]

Hi Jonathan,

Il 13/12/2018 03:55, Jonathan Liu ha scritto:
>> Can you precisely point me the sources of:
>> - u-boot
>> - kernel
>> - dts
>>
>> you're using?
>>
>> Thanks
> 
> U-Boot - git tag v2017.03
> Linux - git tag v4.19.6
> DTS - see device tree changes in
> https://github.com/net147/linux/tree/sun7i-drm-wip but change the
> compatible to "innolux,at070tn92"

With Linux v4.19.6 "innolux,at070tn92" doesn't even work and I suspect 
it is because of clock frequency tolerance check for pll setting, so it 
gets discarded from drm resulting in not having a crtc.

Anyway I've just re-checked the signals using another display:
"innolux,at043tn24" that has a more rounded clock frequency, so it's 
accepted by drm and **signals are correct**.
Unfortunately I don't have your same display here to test, so I can't 
help you about that precise panel.

Sorry.
Best regards
-- 
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

Re: [PATCH] drm/sun4i: fix HSYNC and VSYNC polarity

[Giulio Benetti]

Il 13/12/2018 04:08, Jonathan Liu ha scritto:
> Hi Giulio,
> 
> On Wed, 12 Dec 2018 at 04:20, Giulio Benetti
> <giulio.benetti@micronovasrl.com> wrote:
>>
>> Hi Jonathan,
>>
>> Il 11/12/2018 11:49, Jonathan Liu ha scritto:
>>> Hi Giulio,
>>>
>>> On Thu, 6 Dec 2018 at 22:00, Giulio Benetti
>>> <giulio.benetti@micronovasrl.com> wrote:
>>>>
>>>> Hi Jonathan,
>>>>
>>>> Il 06/12/2018 08:29, Jonathan Liu ha scritto:
>>>>> Hi Giulio,
>>>>>
>>>>> On Thu, 15 Feb 2018 at 17:54, Giulio Benetti
>>>>> <giulio.benetti@micronovasrl.com> wrote:
>>>>>>
>>>>>> Differently from other Lcd signals, HSYNC and VSYNC signals
>>>>>> result inverted if their bits are cleared to 0.
>>>>>>
>>>>>> Invert their settings of IO_POL register.
>>>>>>
>>>>>> 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 3c15cf2..aaf911a 100644
>>>>>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>>>> @@ -389,10 +389,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;
>>>>>>
>>>>>>            regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
>>>>>
>>>>> I am running Linux 4.19.6 and noticed with Olimex LCD-OLinuXino-7TS 7"
>>>>> LCD touchscreen (Innolux AT070TN92) connected to Olimex
>>>>> A20-OLinuXino-MICRO that the image does not display correctly after
>>>>> this change.
>>>>> The image is shifted to the right.
>>>>>
>>>>> Reverting the change results in the image being displayed correctly on
>>>>> the screen.
>>>>>
>>>>> I have in the device tree a "panel" node with compatible =
>>>>> "innolux,at070tn92" which uses the timings in
>>>>> drivers/gpu/drm/panel/panel-simple.c.
>>>>>
>>>>> Any ideas?
>>>
>>>>
>>>> Checking Display Datasheet:
>>>> https://www.olimex.com/Products/Retired/A13-LCD7-TS/resources/S700-AT070TN92.pdf
>>>>
>>>> Page 13 section 3.3.2 you can see it needs active low VS and HS.
>>>>
>>>> You can refer to this Thread and check scope captures about VS and HS
>>>> versus TCON0_IOPOL register:
>>>> https://lists.freedesktop.org/archives/dri-devel/2018-January/163874.html
>>>>
>>>> There should be something that wrongly sets one of these or both:
>>>> mode->flags |= DRM_MODE_FLAG_PHSYNC;
>>>> and/or
>>>> mode->flags |= DRM_MODE_FLAG_PVSYNC;
>>>>
>>>> Checked in panel-simple.c but it's not there.
> 
>>>
>>> flags is 0 because it is not assigned in the struct definition for the panel.
>>
>> I don't think it is 0, because otherwise IO_POL_REG wouldn't be set to
>> 0x03000000 but to 0.
>> What is checked is exactly mode->flags, so the problem seems to be upstream.
>>
>> This is my doubt, it seems mode->flags is not initialized or overriden
>> at a certain point, this is why I want to debug it with Jtag tomorrow.

mode->flags is correct, just checked with debugger.

>>
> 
> If you look at the change made by your patch:
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -389,10 +389,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;
> 
>          regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
> 
> If mode->flags is 0, 

mode->flags is 0xa(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC) by 
default if not specified different in panel-simple.c so the signals must 
be active LOW(IO_POL_REG = 0x00000000).

> then before your change it would set
> SUN4I_TCON0_IO_POL_HSYNC_POSITIVE and
> SUN4I_TCON0_IO_POL_VSYNC_POSITIVE bits to 1 (resulting in 0x03000000).

And it was wrong, because as I've pointed you above, especially in the 
Thread with all the scope captures, 0x03000000 means active HIGH 
H/Vsync, while 0x00000000 means active LOW H/Vsync.

> If mode->flags is not 0, then after your change it would not set
> SUN4I_TCON0_IO_POL_HSYNC_POSITIVE and
> SUN4I_TCON0_IO_POL_VSYNC_POSITIVE bits to 1 (resulting in 0x00000000).

If mode->flags is 0x5(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC) i.e. 
specified in panel-simple.c the signals must be active HIGH(IO_POL_REG = 
0x03000000).

IO_POL_REG = 0x03000000 is active HIGH
IO_POL_REG = 0x00000000 is active LOW

as I've checked many times and rechecked today lot of times too.

>>> Before this change, TCON0_IO_POL_REG would be 0x03000000 (both bits
>>> set to 1) and image displays correctly > After this change, TCON0_IO_POL_REG is 0x00000000 (both bits set to 0)
>>> and image doesn't display correctly.
>>>
>>> Checked using "devmem2 0x01c0c088" on A20-OLinuXino-MICRO Rev J.
>>
>> 0x03000000 as I've triple checked with scope means Positive H/Vsync,
>> and 0x00000000 Negative H/VSync.
>>
>> Please check on the Thread I've pointed you above where there are all
>> the links to the scope captures.
>>
>> Are you completely sure you're using the correct panel?
> 
> Yes, I am sure I am using the correct panel. It has the following
> marking on sticker:
> AT070TN92V.X 89A070ZZ-0K1

At this point the only way to make it working is to specify 
DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC in panel-simple.c and check.
Can you do a check and tell me if this solves the problem?
Also, it's not sufficient to submit a patch for that display in 
panel-simple.c (IMHO) because I would check with scope if it is the way 
wou're describing.

I don't know how to describe it better than this and IMHO the scope can 
give us the final answer.

Hope I've been helpful.

Best regards
-- 
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

Re: [PATCH] drm/sun4i: fix HSYNC and VSYNC polarity

[Giulio Benetti]

Il 13/12/2018 12:58, Giulio Benetti ha scritto:
> 
> 
> Il 13/12/2018 04:08, Jonathan Liu ha scritto:
>> Hi Giulio,
>>
>> On Wed, 12 Dec 2018 at 04:20, Giulio Benetti
>> <giulio.benetti@micronovasrl.com> wrote:
>>>
>>> Hi Jonathan,
>>>
>>> Il 11/12/2018 11:49, Jonathan Liu ha scritto:
>>>> Hi Giulio,
>>>>
>>>> On Thu, 6 Dec 2018 at 22:00, Giulio Benetti
>>>> <giulio.benetti@micronovasrl.com> wrote:
>>>>>
>>>>> Hi Jonathan,
>>>>>
>>>>> Il 06/12/2018 08:29, Jonathan Liu ha scritto:
>>>>>> Hi Giulio,
>>>>>>
>>>>>> On Thu, 15 Feb 2018 at 17:54, Giulio Benetti
>>>>>> <giulio.benetti@micronovasrl.com> wrote:
>>>>>>>
>>>>>>> Differently from other Lcd signals, HSYNC and VSYNC signals
>>>>>>> result inverted if their bits are cleared to 0.
>>>>>>>
>>>>>>> Invert their settings of IO_POL register.
>>>>>>>
>>>>>>> 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 3c15cf2..aaf911a 100644
>>>>>>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>>>>> @@ -389,10 +389,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;
>>>>>>>
>>>>>>>             regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
>>>>>>
>>>>>> I am running Linux 4.19.6 and noticed with Olimex LCD-OLinuXino-7TS 7"
>>>>>> LCD touchscreen (Innolux AT070TN92) connected to Olimex
>>>>>> A20-OLinuXino-MICRO that the image does not display correctly after
>>>>>> this change.
>>>>>> The image is shifted to the right.
>>>>>>
>>>>>> Reverting the change results in the image being displayed correctly on
>>>>>> the screen.
>>>>>>
>>>>>> I have in the device tree a "panel" node with compatible =
>>>>>> "innolux,at070tn92" which uses the timings in
>>>>>> drivers/gpu/drm/panel/panel-simple.c.
>>>>>>
>>>>>> Any ideas?
>>>>
>>>>>
>>>>> Checking Display Datasheet:
>>>>> https://www.olimex.com/Products/Retired/A13-LCD7-TS/resources/S700-AT070TN92.pdf
>>>>>
>>>>> Page 13 section 3.3.2 you can see it needs active low VS and HS.
>>>>>
>>>>> You can refer to this Thread and check scope captures about VS and HS
>>>>> versus TCON0_IOPOL register:
>>>>> https://lists.freedesktop.org/archives/dri-devel/2018-January/163874.html
>>>>>
>>>>> There should be something that wrongly sets one of these or both:
>>>>> mode->flags |= DRM_MODE_FLAG_PHSYNC;
>>>>> and/or
>>>>> mode->flags |= DRM_MODE_FLAG_PVSYNC;
>>>>>
>>>>> Checked in panel-simple.c but it's not there.
>>
>>>>
>>>> flags is 0 because it is not assigned in the struct definition for the panel.
>>>
>>> I don't think it is 0, because otherwise IO_POL_REG wouldn't be set to
>>> 0x03000000 but to 0.
>>> What is checked is exactly mode->flags, so the problem seems to be upstream.
>>>
>>> This is my doubt, it seems mode->flags is not initialized or overriden
>>> at a certain point, this is why I want to debug it with Jtag tomorrow.
> 
> mode->flags is correct, just checked with debugger.
> 
>>>
>>
>> If you look at the change made by your patch:
>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> @@ -389,10 +389,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;
>>
>>           regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
>>
>> If mode->flags is 0,
> 
> mode->flags is 0xa(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC) by
> default if not specified different in panel-simple.c so the signals must
> be active LOW(IO_POL_REG = 0x00000000).

I was wrong, mode->flags is 0 by default if not specified, anyway this 
doesn't change the problem.
It seems that that display works with H/Vsync active HIGH(Positive).

Taken from drm_modes.h:
  *  - DRM_MODE_FLAG_PHSYNC: horizontal sync is active high.
  *  - DRM_MODE_FLAG_NHSYNC: horizontal sync is active low.
  *  - DRM_MODE_FLAG_PVSYNC: vertical sync is active high.
  *  - DRM_MODE_FLAG_NVSYNC: vertical sync is active low.

Maybe Datasheet is wrong at this point, since it should work correctly with:
mode->flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC

I've found other 2 Datasheet but they are equal about that:
https://datasheetspdf.com/pdf-file/775300/Innolux/AT070TN92/1
https://datasheetspdf.com/pdf-file/1256472/Innolux/AT070TN92-V1/1

Can you give a try changing those flags in panel-simple.c too?
I mean a try with:
mode->flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC
and one with:
mode->flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC

I expect it to work correctly with the last one.

Thank you

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

> 
>> then before your change it would set
>> SUN4I_TCON0_IO_POL_HSYNC_POSITIVE and
>> SUN4I_TCON0_IO_POL_VSYNC_POSITIVE bits to 1 (resulting in 0x03000000).
> 
> And it was wrong, because as I've pointed you above, especially in the
> Thread with all the scope captures, 0x03000000 means active HIGH
> H/Vsync, while 0x00000000 means active LOW H/Vsync.
> 
>> If mode->flags is not 0, then after your change it would not set
>> SUN4I_TCON0_IO_POL_HSYNC_POSITIVE and
>> SUN4I_TCON0_IO_POL_VSYNC_POSITIVE bits to 1 (resulting in 0x00000000).
> 
> If mode->flags is 0x5(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC) i.e.
> specified in panel-simple.c the signals must be active HIGH(IO_POL_REG =
> 0x03000000).
> 
> IO_POL_REG = 0x03000000 is active HIGH
> IO_POL_REG = 0x00000000 is active LOW
> 
> as I've checked many times and rechecked today lot of times too.
> 
>>>> Before this change, TCON0_IO_POL_REG would be 0x03000000 (both bits
>>>> set to 1) and image displays correctly > After this change, TCON0_IO_POL_REG is 0x00000000 (both bits set to 0)
>>>> and image doesn't display correctly.
>>>>
>>>> Checked using "devmem2 0x01c0c088" on A20-OLinuXino-MICRO Rev J.
>>>
>>> 0x03000000 as I've triple checked with scope means Positive H/Vsync,
>>> and 0x00000000 Negative H/VSync.
>>>
>>> Please check on the Thread I've pointed you above where there are all
>>> the links to the scope captures.
>>>
>>> Are you completely sure you're using the correct panel?
>>
>> Yes, I am sure I am using the correct panel. It has the following
>> marking on sticker:
>> AT070TN92V.X 89A070ZZ-0K1
> 
> At this point the only way to make it working is to specify
> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC in panel-simple.c and check.
> Can you do a check and tell me if this solves the problem?
> Also, it's not sufficient to submit a patch for that display in
> panel-simple.c (IMHO) because I would check with scope if it is the way
> wou're describing.
> 
> I don't know how to describe it better than this and IMHO the scope can
> give us the final answer.
> 
> Hope I've been helpful.
> 
> Best regards
>