From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751765AbeAVU2D (ORCPT ); Mon, 22 Jan 2018 15:28:03 -0500 Received: from mail.micronovasrl.com ([212.103.203.10]:55302 "EHLO mail.micronovasrl.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751244AbeAVU17 (ORCPT ); Mon, 22 Jan 2018 15:27:59 -0500 Authentication-Results: mail.micronovasrl.com (amavisd-new); dkim=pass reason="pass (just generated, assumed good)" header.d=micronovasrl.com X-Spam-Flag: NO X-Spam-Score: -2.9 Subject: Re: [PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly To: Maxime Ripard Cc: airlied@linux.ie, wens@csie.org, dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org References: <1516474221-114596-1-git-send-email-giulio.benetti@micronovasrl.com> <1516474221-114596-2-git-send-email-giulio.benetti@micronovasrl.com> <20180122085112.7xo2t3x5ag4k2kpl@flea.lan> From: Giulio Benetti Message-ID: Date: Mon, 22 Jan 2018 21:27:58 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <20180122085112.7xo2t3x5ag4k2kpl@flea.lan> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: it Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> >> PVSYNC and PHSYNC only >> >> Signed-off-by: Giulio Benetti > > 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. :) > > Maxime > -- 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: giulio.benetti@micronovasrl.com (Giulio Benetti) Date: Mon, 22 Jan 2018 21:27:58 +0100 Subject: [PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly In-Reply-To: <20180122085112.7xo2t3x5ag4k2kpl@flea.lan> References: <1516474221-114596-1-git-send-email-giulio.benetti@micronovasrl.com> <1516474221-114596-2-git-send-email-giulio.benetti@micronovasrl.com> <20180122085112.7xo2t3x5ag4k2kpl@flea.lan> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 >> >> PVSYNC and PHSYNC only >> >> Signed-off-by: Giulio Benetti > > 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. :) > > Maxime > -- 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Giulio Benetti Subject: Re: [PATCH 2/2] drm/sun4i: Handle DRM_MODE_FLAG_**SYNC_POSITIVE correctly Date: Mon, 22 Jan 2018 21:27:58 +0100 Message-ID: References: <1516474221-114596-1-git-send-email-giulio.benetti@micronovasrl.com> <1516474221-114596-2-git-send-email-giulio.benetti@micronovasrl.com> <20180122085112.7xo2t3x5ag4k2kpl@flea.lan> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <20180122085112.7xo2t3x5ag4k2kpl@flea.lan> Content-Language: it List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Maxime Ripard Cc: airlied@linux.ie, wens@csie.org, linux-arm-kernel@lists.infradead.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org 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 >> >> PVSYNC and PHSYNC only >> >> Signed-off-by: Giulio Benetti > = > 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 |=3D SUN4I_TCON0_IO_POL_HSYNC_POSITIVE; >> = >> - if (!(mode->flags & DRM_MODE_FLAG_PVSYNC)) >> + if (mode->flags & DRM_MODE_FLAG_PVSYNC) >> val |=3D 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=3D0 (giving me falling edge on dclk) and CONFIG_VIDEO_LCD_MODE=3D"....,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. :) > = > Maxime > = -- = 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 =80 26.000 i.v. Iscritta al Reg. Imprese di Padova N. 02663420285 Numero R.E.A. 258642