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

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

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
>