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

[Jonathan Liu <net147@gmail.com>]

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