Re: [PATCH v2 2/2] drm/sun4i: tcon: improve DCLK polarity handling
On 1/11/21 6:20 PM, Maxime Ripard wrote: On Fri, Jan 08, 2021 at 03:34:52PM +0100, Giulio Benetti wrote: Hi, On 1/8/21 10:23 AM, Maxime Ripard wrote: Hi, Thanks for those patches On Thu, Jan 07, 2021 at 03:30:32AM +0100, Giulio Benetti wrote: From: Giulio Benetti It turned out(Maxime suggestion) that bit 26 of SUN4I_TCON0_IO_POL_REG is dedicated to invert DCLK polarity and this makes thing really easier than before. So let's handle DCLK polarity by adding SUN4I_TCON0_IO_POL_DCLK_POSITIVE as bit 26 and activating according to bus_flags the same way is done for all the other signals. Cc: Maxime Ripard Suggested-by would be nice here :) Ok, didn't know about this tag Signed-off-by: Giulio Benetti --- drivers/gpu/drm/sun4i/sun4i_tcon.c | 20 +--- drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 + 2 files changed, 2 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index 52598bb0fb0b..30171ccd87e5 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -569,26 +569,8 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon, if (info->bus_flags & DRM_BUS_FLAG_DE_LOW) val |= SUN4I_TCON0_IO_POL_DE_NEGATIVE; - /* -* On A20 and similar SoCs, the only way to achieve Positive Edge -* (Rising Edge), is setting dclk clock phase to 2/3(240°). -* By default TCON works in Negative Edge(Falling Edge), -* this is why phase is set to 0 in that case. -* Unfortunately there's no way to logically invert dclk through -* IO_POL register. -* The only acceptable way to work, triple checked with scope, -* is using clock phase set to 0° for Negative Edge and set to 240° -* for Positive Edge. -* On A33 and similar SoCs there would be a 90° phase option, -* but it divides also dclk by 2. -* Following code is a way to avoid quirks all around TCON -* and DOTCLOCK drivers. -*/ if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE) - clk_set_phase(tcon->dclk, 0); - - if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) - clk_set_phase(tcon->dclk, 240); + val |= SUN4I_TCON0_IO_POL_DCLK_POSITIVE; I'm not really sure why we need the first patch of this series here? The idea was to have 2 for testing, 1st one is already applicable, while the other must be tested, but I can send only one with no problem. That patch only seem to undo what you did in patch 1 No, it doesn't, the 2nd one change the way it achieve the same thing, because the 1st swap DCLK phase, while the 2nd uses the IO_POL bit to set IO polarity according to bus_flags. It makes sense for testing, but I'm not sure we want to carry it into the history. Can you squash them both into the same patch? Sure, I'm going to send V3 then. Thank you Best regards -- Giulio Benetti Benetti Engineering sas ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 2/2] drm/sun4i: tcon: improve DCLK polarity handling
On Fri, Jan 08, 2021 at 03:34:52PM +0100, Giulio Benetti wrote: > Hi, > > On 1/8/21 10:23 AM, Maxime Ripard wrote: > > Hi, > > > > Thanks for those patches > > > > On Thu, Jan 07, 2021 at 03:30:32AM +0100, Giulio Benetti wrote: > > > From: Giulio Benetti > > > > > > It turned out(Maxime suggestion) that bit 26 of SUN4I_TCON0_IO_POL_REG is > > > dedicated to invert DCLK polarity and this makes thing really easier than > > > before. So let's handle DCLK polarity by adding > > > SUN4I_TCON0_IO_POL_DCLK_POSITIVE as bit 26 and activating according to > > > bus_flags the same way is done for all the other signals. > > > > > > Cc: Maxime Ripard > > > > Suggested-by would be nice here :) > > Ok, didn't know about this tag > > > > Signed-off-by: Giulio Benetti > > > --- > > > drivers/gpu/drm/sun4i/sun4i_tcon.c | 20 +--- > > > drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 + > > > 2 files changed, 2 insertions(+), 19 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c > > > b/drivers/gpu/drm/sun4i/sun4i_tcon.c > > > index 52598bb0fb0b..30171ccd87e5 100644 > > > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c > > > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c > > > @@ -569,26 +569,8 @@ static void sun4i_tcon0_mode_set_rgb(struct > > > sun4i_tcon *tcon, > > > if (info->bus_flags & DRM_BUS_FLAG_DE_LOW) > > > val |= SUN4I_TCON0_IO_POL_DE_NEGATIVE; > > > - /* > > > - * On A20 and similar SoCs, the only way to achieve Positive Edge > > > - * (Rising Edge), is setting dclk clock phase to 2/3(240°). > > > - * By default TCON works in Negative Edge(Falling Edge), > > > - * this is why phase is set to 0 in that case. > > > - * Unfortunately there's no way to logically invert dclk through > > > - * IO_POL register. > > > - * The only acceptable way to work, triple checked with scope, > > > - * is using clock phase set to 0° for Negative Edge and set to 240° > > > - * for Positive Edge. > > > - * On A33 and similar SoCs there would be a 90° phase option, > > > - * but it divides also dclk by 2. > > > - * Following code is a way to avoid quirks all around TCON > > > - * and DOTCLOCK drivers. > > > - */ > > > if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE) > > > - clk_set_phase(tcon->dclk, 0); > > > - > > > - if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) > > > - clk_set_phase(tcon->dclk, 240); > > > + val |= SUN4I_TCON0_IO_POL_DCLK_POSITIVE; > > > > I'm not really sure why we need the first patch of this series here? > > The idea was to have 2 for testing, 1st one is already applicable, while the > other must be tested, but I can send only one with no problem. > > > That patch only seem to undo what you did in patch 1 > > No, it doesn't, the 2nd one change the way it achieve the same thing, > because the 1st swap DCLK phase, while the 2nd uses the IO_POL bit to set IO > polarity according to bus_flags. It makes sense for testing, but I'm not sure we want to carry it into the history. Can you squash them both into the same patch? Thanks! Maxime signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 2/2] drm/sun4i: tcon: improve DCLK polarity handling
Hi, On 1/8/21 10:23 AM, Maxime Ripard wrote: Hi, Thanks for those patches On Thu, Jan 07, 2021 at 03:30:32AM +0100, Giulio Benetti wrote: From: Giulio Benetti It turned out(Maxime suggestion) that bit 26 of SUN4I_TCON0_IO_POL_REG is dedicated to invert DCLK polarity and this makes thing really easier than before. So let's handle DCLK polarity by adding SUN4I_TCON0_IO_POL_DCLK_POSITIVE as bit 26 and activating according to bus_flags the same way is done for all the other signals. Cc: Maxime Ripard Suggested-by would be nice here :) Ok, didn't know about this tag Signed-off-by: Giulio Benetti --- drivers/gpu/drm/sun4i/sun4i_tcon.c | 20 +--- drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 + 2 files changed, 2 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index 52598bb0fb0b..30171ccd87e5 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -569,26 +569,8 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon, if (info->bus_flags & DRM_BUS_FLAG_DE_LOW) val |= SUN4I_TCON0_IO_POL_DE_NEGATIVE; - /* -* On A20 and similar SoCs, the only way to achieve Positive Edge -* (Rising Edge), is setting dclk clock phase to 2/3(240°). -* By default TCON works in Negative Edge(Falling Edge), -* this is why phase is set to 0 in that case. -* Unfortunately there's no way to logically invert dclk through -* IO_POL register. -* The only acceptable way to work, triple checked with scope, -* is using clock phase set to 0° for Negative Edge and set to 240° -* for Positive Edge. -* On A33 and similar SoCs there would be a 90° phase option, -* but it divides also dclk by 2. -* Following code is a way to avoid quirks all around TCON -* and DOTCLOCK drivers. -*/ if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE) - clk_set_phase(tcon->dclk, 0); - - if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) - clk_set_phase(tcon->dclk, 240); + val |= SUN4I_TCON0_IO_POL_DCLK_POSITIVE; I'm not really sure why we need the first patch of this series here? The idea was to have 2 for testing, 1st one is already applicable, while the other must be tested, but I can send only one with no problem. That patch only seem to undo what you did in patch 1 No, it doesn't, the 2nd one change the way it achieve the same thing, because the 1st swap DCLK phase, while the 2nd uses the IO_POL bit to set IO polarity according to bus_flags. Best Regards -- Giulio Benetti Benetti Engineering sas ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 2/2] drm/sun4i: tcon: improve DCLK polarity handling
Hi Marjan, On 1/8/21 10:32 AM, Marjan Pascolo wrote: Hi, please don't top post, answer in-line as we do, and please use plain-test instead of HTML. These are the standards in Mailing Lists(ML). Quote " I'm not really sure why we need the first patch of this series here? That patch only seem to undo what you did in patch 1 " Already answered to Maxime And another question (probably could be a stupid one): in "/[PATCH v2 2/2] drm/sun4i: tcon: improve DCLK polarity handling/" I see you deleted: - clk_set_phase(tcon->dclk, 0); Is safe to assume that phase register will be always set to 0? We always assumed it is set to 0 by default. Or maybe will be safer manually set it to 0 in every condition to avoid surprises (dirt values due to previous condition)? That would be a good idea, so something like this: ''' int phase = 0; if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) phase = 240; clk_set_phase(tcon->dclk, phase); ''' because now DRM_BUS_FLAG_PIXDATA_SAMPLE_ and DRM_BUS_FLAG_PIXDATA_DRIVE_ are exclusive, while before not. But then if bit 26 solution works everything gets easier. Best Regards Giulio Marjan Il 08/01/2021 10:23, Maxime Ripard ha scritto: Hi, Thanks for those patches On Thu, Jan 07, 2021 at 03:30:32AM +0100, Giulio Benetti wrote: From: Giulio Benetti It turned out(Maxime suggestion) that bit 26 of SUN4I_TCON0_IO_POL_REG is dedicated to invert DCLK polarity and this makes thing really easier than before. So let's handle DCLK polarity by adding SUN4I_TCON0_IO_POL_DCLK_POSITIVE as bit 26 and activating according to bus_flags the same way is done for all the other signals. Cc: Maxime Ripard Suggested-by would be nice here :) Signed-off-by: Giulio Benetti --- drivers/gpu/drm/sun4i/sun4i_tcon.c | 20 +--- drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 + 2 files changed, 2 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index 52598bb0fb0b..30171ccd87e5 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -569,26 +569,8 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon, if (info->bus_flags & DRM_BUS_FLAG_DE_LOW) val |= SUN4I_TCON0_IO_POL_DE_NEGATIVE; - /* -* On A20 and similar SoCs, the only way to achieve Positive Edge -* (Rising Edge), is setting dclk clock phase to 2/3(240°). -* By default TCON works in Negative Edge(Falling Edge), -* this is why phase is set to 0 in that case. -* Unfortunately there's no way to logically invert dclk through -* IO_POL register. -* The only acceptable way to work, triple checked with scope, -* is using clock phase set to 0° for Negative Edge and set to 240° -* for Positive Edge. -* On A33 and similar SoCs there would be a 90° phase option, -* but it divides also dclk by 2. -* Following code is a way to avoid quirks all around TCON -* and DOTCLOCK drivers. -*/ if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE) - clk_set_phase(tcon->dclk, 0); - - if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) - clk_set_phase(tcon->dclk, 240); + val |= SUN4I_TCON0_IO_POL_DCLK_POSITIVE; I'm not really sure why we need the first patch of this series here? That patch only seem to undo what you did in patch 1 Maxime ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 2/2] drm/sun4i: tcon: improve DCLK polarity handling
Hi, Quote " I'm not really sure why we need the first patch of this series here? That patch only seem to undo what you did in patch 1 " And another question (probably could be a stupid one): in "/[PATCH v2 2/2] drm/sun4i: tcon: improve DCLK polarity handling/" I see you deleted: - clk_set_phase(tcon->dclk, 0); Is safe to assume that phase register will be always set to 0? Or maybe will be safer manually set it to 0 in every condition to avoid surprises (dirt values due to previous condition)? Marjan Il 08/01/2021 10:23, Maxime Ripard ha scritto: Hi, Thanks for those patches On Thu, Jan 07, 2021 at 03:30:32AM +0100, Giulio Benetti wrote: From: Giulio Benetti It turned out(Maxime suggestion) that bit 26 of SUN4I_TCON0_IO_POL_REG is dedicated to invert DCLK polarity and this makes thing really easier than before. So let's handle DCLK polarity by adding SUN4I_TCON0_IO_POL_DCLK_POSITIVE as bit 26 and activating according to bus_flags the same way is done for all the other signals. Cc: Maxime Ripard Suggested-by would be nice here :) Signed-off-by: Giulio Benetti --- drivers/gpu/drm/sun4i/sun4i_tcon.c | 20 +--- drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 + 2 files changed, 2 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index 52598bb0fb0b..30171ccd87e5 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -569,26 +569,8 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon, if (info->bus_flags & DRM_BUS_FLAG_DE_LOW) val |= SUN4I_TCON0_IO_POL_DE_NEGATIVE; - /* -* On A20 and similar SoCs, the only way to achieve Positive Edge -* (Rising Edge), is setting dclk clock phase to 2/3(240°). -* By default TCON works in Negative Edge(Falling Edge), -* this is why phase is set to 0 in that case. -* Unfortunately there's no way to logically invert dclk through -* IO_POL register. -* The only acceptable way to work, triple checked with scope, -* is using clock phase set to 0° for Negative Edge and set to 240° -* for Positive Edge. -* On A33 and similar SoCs there would be a 90° phase option, -* but it divides also dclk by 2. -* Following code is a way to avoid quirks all around TCON -* and DOTCLOCK drivers. -*/ if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE) - clk_set_phase(tcon->dclk, 0); - - if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) - clk_set_phase(tcon->dclk, 240); + val |= SUN4I_TCON0_IO_POL_DCLK_POSITIVE; I'm not really sure why we need the first patch of this series here? That patch only seem to undo what you did in patch 1 Maxime ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 2/2] drm/sun4i: tcon: improve DCLK polarity handling
Hi, Thanks for those patches On Thu, Jan 07, 2021 at 03:30:32AM +0100, Giulio Benetti wrote: > From: Giulio Benetti > > It turned out(Maxime suggestion) that bit 26 of SUN4I_TCON0_IO_POL_REG is > dedicated to invert DCLK polarity and this makes thing really easier than > before. So let's handle DCLK polarity by adding > SUN4I_TCON0_IO_POL_DCLK_POSITIVE as bit 26 and activating according to > bus_flags the same way is done for all the other signals. > > Cc: Maxime Ripard Suggested-by would be nice here :) > Signed-off-by: Giulio Benetti > --- > drivers/gpu/drm/sun4i/sun4i_tcon.c | 20 +--- > drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 + > 2 files changed, 2 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c > b/drivers/gpu/drm/sun4i/sun4i_tcon.c > index 52598bb0fb0b..30171ccd87e5 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c > @@ -569,26 +569,8 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon > *tcon, > if (info->bus_flags & DRM_BUS_FLAG_DE_LOW) > val |= SUN4I_TCON0_IO_POL_DE_NEGATIVE; > > - /* > - * On A20 and similar SoCs, the only way to achieve Positive Edge > - * (Rising Edge), is setting dclk clock phase to 2/3(240°). > - * By default TCON works in Negative Edge(Falling Edge), > - * this is why phase is set to 0 in that case. > - * Unfortunately there's no way to logically invert dclk through > - * IO_POL register. > - * The only acceptable way to work, triple checked with scope, > - * is using clock phase set to 0° for Negative Edge and set to 240° > - * for Positive Edge. > - * On A33 and similar SoCs there would be a 90° phase option, > - * but it divides also dclk by 2. > - * Following code is a way to avoid quirks all around TCON > - * and DOTCLOCK drivers. > - */ > if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE) > - clk_set_phase(tcon->dclk, 0); > - > - if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) > - clk_set_phase(tcon->dclk, 240); > + val |= SUN4I_TCON0_IO_POL_DCLK_POSITIVE; I'm not really sure why we need the first patch of this series here? That patch only seem to undo what you did in patch 1 Maxime ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
