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

[Giulio Benetti <giulio.benetti@micronovasrl.com>]

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