From: Giulio Benetti <giulio.benetti@micronovasrl.com> To: Maxime Ripard <maxime.ripard@free-electrons.com> Cc: airlied@linux.ie, wens@csie.org, dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly Date: Wed, 24 Jan 2018 18:38:17 +0100 [thread overview] Message-ID: <59f7b542-3b1d-ff62-e290-37c47f4075ff@micronovasrl.com> (raw) In-Reply-To: <aa724bd5-cfb1-128c-af32-db29253f32aa@micronovasrl.com> 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 >> > >
WARNING: multiple messages have this Message-ID (diff)
From: giulio.benetti@micronovasrl.com (Giulio Benetti) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly Date: Wed, 24 Jan 2018 18:38:17 +0100 [thread overview] Message-ID: <59f7b542-3b1d-ff62-e290-37c47f4075ff@micronovasrl.com> (raw) In-Reply-To: <aa724bd5-cfb1-128c-af32-db29253f32aa@micronovasrl.com> 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 >> > >
next prev parent reply other threads:[~2018-01-24 17:38 UTC|newest] Thread overview: 63+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-01-20 18:50 [PATCH 1/2] drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE Giulio Benetti 2018-01-20 18:50 ` Giulio Benetti 2018-01-20 18:50 ` Giulio Benetti 2018-01-20 18:50 ` [PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly Giulio Benetti 2018-01-20 18:50 ` Giulio Benetti 2018-01-20 18:50 ` Giulio Benetti 2018-01-22 8:51 ` Maxime Ripard 2018-01-22 8:51 ` Maxime Ripard 2018-01-22 8:51 ` Maxime Ripard 2018-01-22 20:27 ` Giulio Benetti 2018-01-22 20:27 ` Giulio Benetti 2018-01-22 20:27 ` Giulio Benetti 2018-01-24 17:38 ` Giulio Benetti [this message] 2018-01-24 17:38 ` Giulio Benetti 2018-01-24 19:37 ` Giulio Benetti 2018-01-24 19:37 ` Giulio Benetti 2018-01-24 19:37 ` Giulio Benetti 2018-01-25 15:21 ` Maxime Ripard 2018-01-25 15:21 ` Maxime Ripard 2018-01-25 15:21 ` Maxime Ripard 2018-01-25 16:50 ` Giulio Benetti 2018-01-25 16:50 ` Giulio Benetti 2018-01-26 14:56 ` Maxime Ripard 2018-01-26 14:56 ` Maxime Ripard 2018-01-26 15:55 ` Giulio Benetti 2018-01-26 15:55 ` Giulio Benetti 2018-01-27 19:06 ` Giulio Benetti 2018-01-27 22:07 ` Giulio Benetti 2018-01-27 22:07 ` Giulio Benetti 2018-01-27 22:07 ` Giulio Benetti 2018-02-01 10:14 ` Maxime Ripard 2018-02-01 10:14 ` Maxime Ripard 2018-02-01 10:14 ` Maxime Ripard 2018-02-01 16:09 ` Giulio Benetti 2018-02-01 16:09 ` Giulio Benetti 2018-02-01 16:09 ` Giulio Benetti 2018-02-05 14:21 ` Maxime Ripard 2018-02-05 14:21 ` Maxime Ripard 2018-02-05 14:21 ` Maxime Ripard 2018-01-29 12:53 ` Maxime Ripard 2018-01-29 12:53 ` Maxime Ripard 2018-01-29 12:53 ` Maxime Ripard 2018-02-07 10:39 ` Maxime Ripard 2018-02-07 10:39 ` Maxime Ripard 2018-02-07 10:39 ` Maxime Ripard 2018-02-07 12:49 ` Giulio Benetti 2018-02-07 12:49 ` Giulio Benetti 2018-02-08 20:40 ` Maxime Ripard 2018-02-08 20:40 ` Maxime Ripard 2018-02-08 20:40 ` Maxime Ripard 2018-02-09 10:13 ` Chen-Yu Tsai 2018-02-09 10:13 ` Chen-Yu Tsai 2018-02-09 10:13 ` Chen-Yu Tsai 2018-02-15 18:05 ` Giulio Benetti 2018-02-15 18:05 ` Giulio Benetti 2018-02-16 15:50 ` Maxime Ripard 2018-02-16 15:50 ` Maxime Ripard 2018-02-16 15:50 ` Maxime Ripard 2018-02-28 15:56 ` Giulio Benetti 2018-02-28 15:56 ` Giulio Benetti 2018-02-28 15:56 ` Giulio Benetti 2018-01-22 8:45 ` [PATCH 1/2] drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE Maxime Ripard 2018-01-22 8:45 ` Maxime Ripard
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=59f7b542-3b1d-ff62-e290-37c47f4075ff@micronovasrl.com \ --to=giulio.benetti@micronovasrl.com \ --cc=airlied@linux.ie \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=maxime.ripard@free-electrons.com \ --cc=wens@csie.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.