diff mbox

[v2,13/14] drm: rcar-du: Restrict DPLL duty cycle workaround to H3 ES1.x

Message ID 20170626181226.29575-14-laurent.pinchart+renesas@ideasonboard.com (mailing list archive)
State New, archived
Headers show

Commit Message

Laurent Pinchart June 26, 2017, 6:12 p.m. UTC
The H3 ES1.x exhibits dot clock duty cycle stability issues. We can work
around them by configuring the DPLL to twice the desired frequency,
coupled with a /2 post-divider. This isn't needed on other SoCs and
breaks HDMI output on M3-W for a currently unknown reason, so restrict
the workaround to H3 ES1.x.

From an implementation point of view, move work around handling outside
of the rcar_du_dpll_divider() function by requesting a x2 DPLL output
frequency explicitly. The existing post-divider calculation mechanism
will then take care of dividing the clock by two automatically.

While at it, print a more useful debugging message to ease debugging
clock rate issues.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 37 +++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 10 deletions(-)

Comments

Kieran Bingham Aug. 1, 2017, 2:06 p.m. UTC | #1
Hi Laurent,

On 26/06/17 19:12, Laurent Pinchart wrote:
> The H3 ES1.x exhibits dot clock duty cycle stability issues. We can work
> around them by configuring the DPLL to twice the desired frequency,
> coupled with a /2 post-divider. This isn't needed on other SoCs and
> breaks HDMI output on M3-W for a currently unknown reason, so restrict
> the workaround to H3 ES1.x.
> 
> From an implementation point of view, move work around handling outside
> of the rcar_du_dpll_divider() function by requesting a x2 DPLL output
> frequency explicitly. The existing post-divider calculation mechanism
> will then take care of dividing the clock by two automatically.
> 
> While at it, print a more useful debugging message to ease debugging
> clock rate issues.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 37 +++++++++++++++++++++++++---------
>  1 file changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 8f942ebdd0c6..6c29981377c0 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -13,6 +13,7 @@
>  
>  #include <linux/clk.h>
>  #include <linux/mutex.h>
> +#include <linux/sys_soc.h>
>  
>  #include <drm/drmP.h>
>  #include <drm/drm_atomic.h>
> @@ -129,10 +130,8 @@ static void rcar_du_dpll_divider(struct rcar_du_crtc *rcrtc,
>  			for (fdpll = 1; fdpll < 32; fdpll++) {
>  				unsigned long output;
>  
> -				/* 1/2 (FRQSEL=1) for duty rate 50% */
>  				output = input * (n + 1) / (m + 1)
> -				       / (fdpll + 1) / 2;
> -
> +				       / (fdpll + 1);

I'm finding this hard to interpret vs the commit-message.

Here we remove the /2 (which affects all targets... is this a problem?)

>  				if (output >= 400000000)
>  					continue;
>  
> @@ -158,6 +157,11 @@ static void rcar_du_dpll_divider(struct rcar_du_crtc *rcrtc,
>  		 best_diff);
>  }
>  
> +static const struct soc_device_attribute rcar_du_r8a7795_es1[] = {
> +	{ .soc_id = "r8a7795", .revision = "ES1.*" },
> +	{ /* sentinel */ }
> +};
> +
>  static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>  {
>  	const struct drm_display_mode *mode = &rcrtc->crtc.state->adjusted_mode;
> @@ -185,7 +189,20 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>  
>  		extclk = clk_get_rate(rcrtc->extclock);
>  		if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {
> -			rcar_du_dpll_divider(rcrtc, &dpll, extclk, mode_clock);
> +			unsigned long target = mode_clock;
> +
> +			/*
> +			 * The H3 ES1.x exhibits dot clock duty cycle stability
> +			 * issues. We can work around them by configuring the
> +			 * DPLL to twice the desired frequency, coupled with a
> +			 * /2 post-divider. This isn't needed on other SoCs and

But here we discuss 'coupling' it with a /2 post - divider.

My inference here then is that by setting a target that is 'twice' the value -
code later will provide the /2 post-divide?

> +			 * breaks HDMI output on M3-W for a currently unknown
> +			 * reason, so restrict the workaround to H3 ES1.x.
> +			 */
> +			if (soc_device_match(rcar_du_r8a7795_es1))
> +				target *= 2;
> +
> +			rcar_du_dpll_divider(rcrtc, &dpll, extclk, target);
>  			extclk = dpll.output;
>  		}
>  
> @@ -197,8 +214,6 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>  
>  		if (abs((long)extrate - (long)mode_clock) <
>  		    abs((long)rate - (long)mode_clock)) {
> -			dev_dbg(rcrtc->group->dev->dev,
> -				"crtc%u: using external clock\n", rcrtc->index);
>  
>  			if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {
>  				u32 dpllcr = DPLLCR_CODE | DPLLCR_CLKE
> @@ -215,12 +230,14 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>  
>  				rcar_du_group_write(rcrtc->group, DPLLCR,
>  						    dpllcr);
> -
> -				escr = ESCR_DCLKSEL_DCLKIN | 1;
> -			} else {
> -				escr = ESCR_DCLKSEL_DCLKIN | extdiv;
>  			}
> +
> +			escr = ESCR_DCLKSEL_DCLKIN | extdiv;

Therefore - is this the post-divider?

If my inferences are correct - then OK:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

--
KB

>  		}
> +
> +		dev_dbg(rcrtc->group->dev->dev,
> +			"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
> +			mode_clock, extrate, rate, escr);
>  	}
>  
>  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
>
Laurent Pinchart Aug. 1, 2017, 6:39 p.m. UTC | #2
Hi Kieran,

On Tuesday 01 Aug 2017 15:06:20 Kieran Bingham wrote:
> On 26/06/17 19:12, Laurent Pinchart wrote:
> > The H3 ES1.x exhibits dot clock duty cycle stability issues. We can work
> > around them by configuring the DPLL to twice the desired frequency,
> > coupled with a /2 post-divider. This isn't needed on other SoCs and
> > breaks HDMI output on M3-W for a currently unknown reason, so restrict
> > the workaround to H3 ES1.x.
> > 
> > From an implementation point of view, move work around handling outside
> > of the rcar_du_dpll_divider() function by requesting a x2 DPLL output
> > frequency explicitly. The existing post-divider calculation mechanism
> > will then take care of dividing the clock by two automatically.
> > 
> > While at it, print a more useful debugging message to ease debugging
> > clock rate issues.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 37 +++++++++++++++++++++-------
> >  1 file changed, 27 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 8f942ebdd0c6..6c29981377c0
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > @@ -13,6 +13,7 @@
> > 
> >  #include <linux/clk.h>
> >  #include <linux/mutex.h>
> > +#include <linux/sys_soc.h>
> > 
> >  #include <drm/drmP.h>
> >  #include <drm/drm_atomic.h>
> > @@ -129,10 +130,8 @@ static void rcar_du_dpll_divider(struct rcar_du_crtc
> > *rcrtc,> 
> >  			for (fdpll = 1; fdpll < 32; fdpll++) {
> >  				unsigned long output;
> > 
> > -				/* 1/2 (FRQSEL=1) for duty rate 50% */
> >  				output = input * (n + 1) / (m + 1)
> > -				       / (fdpll + 1) / 2;
> > -
> > +				       / (fdpll + 1);
> 
> I'm finding this hard to interpret vs the commit-message.
> 
> Here we remove the /2 (which affects all targets... is this a problem?)

The purpose of this function is to compute DPLL parameters for given input and 
output frequencies. However, the current implementation computes parameters 
that result in twice the requested frequency, assuming that the caller will 
configure a /2 post-divider.

I found this confusing, so the patch modifies the function to use the 
requested output frequency, and updates the caller accordingly. The function 
now performs the operation described by its name.

This indeed affects all targets, but there's no DPLL on Gen2, so in practice 
only H3 and M3-W are affected.

> >  				if (output >= 400000000)
> >  					continue;

[snip]

> > @@ -185,7 +189,20 @@ static void rcar_du_crtc_set_display_timing(struct
> > rcar_du_crtc *rcrtc)> 
> >  		extclk = clk_get_rate(rcrtc->extclock);
> >  		if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {
> > 
> > -			rcar_du_dpll_divider(rcrtc, &dpll, extclk,
> > mode_clock);
> > +			unsigned long target = mode_clock;
> > +
> > +			/*
> > +			 * The H3 ES1.x exhibits dot clock duty cycle
> > stability
> > +			 * issues. We can work around them by configuring the
> > +			 * DPLL to twice the desired frequency, coupled with a
> > +			 * /2 post-divider. This isn't needed on other SoCs
> > and
> 
> But here we discuss 'coupling' it with a /2 post - divider.
> 
> My inference here then is that by setting a target that is 'twice' the value
> - code later will provide the /2 post-divide?

Correct. The /2 post-divider is required on H3 ES1.x to obtain a stable output 
clock, so the output of the DPLL has to be twice the pixel clock frequency. 
Based on my understanding it shouldn't hurt to do the same on H3 ES2.0 and M3-
W, but in practice that doesn't work. I've thus restricted the post-divider to 
H3 ES1.x.

> > +			 * breaks HDMI output on M3-W for a currently unknown
> > +			 * reason, so restrict the workaround to H3 ES1.x.
> > +			 */
> > +			if (soc_device_match(rcar_du_r8a7795_es1))
> > +				target *= 2;
> > +
> > +			rcar_du_dpll_divider(rcrtc, &dpll, extclk, target);
> >  			extclk = dpll.output;
> >  		}
> > 
> > @@ -197,8 +214,6 @@ static void rcar_du_crtc_set_display_timing(struct
> > rcar_du_crtc *rcrtc)
> >  		if (abs((long)extrate - (long)mode_clock) <
> >  		    abs((long)rate - (long)mode_clock)) {
> > -			dev_dbg(rcrtc->group->dev->dev,
> > -				"crtc%u: using external clock\n", rcrtc-
>index);
> > 
> >  			if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {
> >  				u32 dpllcr = DPLLCR_CODE | DPLLCR_CLKE
> > 
> > @@ -215,12 +230,14 @@ static void rcar_du_crtc_set_display_timing(struct
> > rcar_du_crtc *rcrtc)
> >  				rcar_du_group_write(rcrtc->group, DPLLCR,
> >  						    dpllcr);
> > -
> > -				escr = ESCR_DCLKSEL_DCLKIN | 1;
> > -			} else {
> > -				escr = ESCR_DCLKSEL_DCLKIN | extdiv;
> >  			}
> > +
> > +			escr = ESCR_DCLKSEL_DCLKIN | extdiv;
> 
> Therefore - is this the post-divider?

Correct again. extdiv is computed as DPLL output frequency / pixel clock 
frequency, so there's no need for any SoC-specific code here.

> If my inferences are correct - then OK:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> >  		}
> > +
> > +		dev_dbg(rcrtc->group->dev->dev,
> > +			"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
> > +			mode_clock, extrate, rate, escr);
> >  	}
> >  	
> >  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
Laurent Pinchart Dec. 11, 2017, 8:58 p.m. UTC | #3
Hi Kieran,

I found this e-mail already written and sitting in my outbox, so even if it's 
quite outdated I decided to still send it.

On Tuesday 01 Aug 2017 15:06:20 Kieran Bingham wrote:
> On 26/06/17 19:12, Laurent Pinchart wrote:
> > The H3 ES1.x exhibits dot clock duty cycle stability issues. We can work
> > around them by configuring the DPLL to twice the desired frequency,
> > coupled with a /2 post-divider. This isn't needed on other SoCs and
> > breaks HDMI output on M3-W for a currently unknown reason, so restrict
> > the workaround to H3 ES1.x.
> > 
> > From an implementation point of view, move work around handling outside
> > of the rcar_du_dpll_divider() function by requesting a x2 DPLL output
> > frequency explicitly. The existing post-divider calculation mechanism
> > will then take care of dividing the clock by two automatically.
> > 
> > While at it, print a more useful debugging message to ease debugging
> > clock rate issues.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 37 ++++++++++++++++++++---------
> >  1 file changed, 27 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 8f942ebdd0c6..6c29981377c0
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > @@ -13,6 +13,7 @@
> > 
> >  #include <linux/clk.h>
> >  #include <linux/mutex.h>
> > +#include <linux/sys_soc.h>
> > 
> >  #include <drm/drmP.h>
> >  #include <drm/drm_atomic.h>
> > @@ -129,10 +130,8 @@ static void rcar_du_dpll_divider(struct rcar_du_crtc
> > *rcrtc,
> >  			for (fdpll = 1; fdpll < 32; fdpll++) {
> >  				unsigned long output;
> > 
> > -				/* 1/2 (FRQSEL=1) for duty rate 50% */
> >  				output = input * (n + 1) / (m + 1)
> > -				       / (fdpll + 1) / 2;
> > -
> > +				       / (fdpll + 1);
> 
> I'm finding this hard to interpret vs the commit-message.
> 
> Here we remove the /2 (which affects all targets... is this a problem?)

The purpose of the rcar_du_dpll_divider() function is to compute the DPLL 
settings for a target output frequency. However, at the moment, it computes 
settings that will generate twice the output frequency, assuming that the 
caller will configure an additional /2 post-divider.

I found that confusing, so I've modified this function to generate the target 
output frequency. The caller is modified below to request twice the desired 
display clock frequency when adding the post-divider.

Note that DPLLs are available on Gen3 only, so this affects H3 and M3-W only.

> >  				if (output >= 400000000)
> >  					continue;

[snip]

> > @@ -185,7 +189,20 @@ static void rcar_du_crtc_set_display_timing(struct
> > rcar_du_crtc *rcrtc)> 
> >  		extclk = clk_get_rate(rcrtc->extclock);
> >  		if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {
> > 
> > -			rcar_du_dpll_divider(rcrtc, &dpll, extclk,
> > mode_clock);
> > +			unsigned long target = mode_clock;
> > +
> > +			/*
> > +			 * The H3 ES1.x exhibits dot clock duty cycle
> > stability
> > +			 * issues. We can work around them by configuring the
> > +			 * DPLL to twice the desired frequency, coupled with a
> > +			 * /2 post-divider. This isn't needed on other SoCs
> > and
> 
> But here we discuss 'coupling' it with a /2 post - divider.
> 
> My inference here then is that by setting a target that is 'twice' the value
> - code later will provide the /2 post-divide?

Correct. On H3 ES1.x the behaviour of the code is unchanged, while on H3 ES2.0 
and M3-W we now configure the DPLL without the post-divider.

> > +			 * breaks HDMI output on M3-W for a currently unknown
> > +			 * reason, so restrict the workaround to H3 ES1.x.
> > +			 */
> > +			if (soc_device_match(rcar_du_r8a7795_es1))
> > +				target *= 2;
> > +
> > +			rcar_du_dpll_divider(rcrtc, &dpll, extclk, target);
> >  			extclk = dpll.output;
> >  		}
> > 
> > @@ -197,8 +214,6 @@ static void rcar_du_crtc_set_display_timing(struct
> > rcar_du_crtc *rcrtc)
> >  		if (abs((long)extrate - (long)mode_clock) <
> >  		    abs((long)rate - (long)mode_clock)) {
> > -			dev_dbg(rcrtc->group->dev->dev,
> > -				"crtc%u: using external clock\n",
> > rcrtc->index);
> > 
> >  			if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {
> >  				u32 dpllcr = DPLLCR_CODE | DPLLCR_CLKE
> > @@ -215,12 +230,14 @@ static void rcar_du_crtc_set_display_timing(struct
> > rcar_du_crtc *rcrtc)
> >  				rcar_du_group_write(rcrtc->group, DPLLCR,
> >  						    dpllcr);
> > -
> > -				escr = ESCR_DCLKSEL_DCLKIN | 1;
> > -			} else {
> > -				escr = ESCR_DCLKSEL_DCLKIN | extdiv;
> >  			}
> > +
> > +			escr = ESCR_DCLKSEL_DCLKIN | extdiv;
> 
> Therefore - is this the post-divider?

Correct again. There's no need for any SoC-specific processing here, as extdiv 
is simply computed as DPLL output frequency / pixel clock frequency.

> If my inferences are correct - then OK:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> >  		}
> > +
> > +		dev_dbg(rcrtc->group->dev->dev,
> > +			"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
> > +			mode_clock, extrate, rate, escr);
> >  	}
> >  	
> >  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
diff mbox

Patch

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 8f942ebdd0c6..6c29981377c0 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -13,6 +13,7 @@ 
 
 #include <linux/clk.h>
 #include <linux/mutex.h>
+#include <linux/sys_soc.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_atomic.h>
@@ -129,10 +130,8 @@  static void rcar_du_dpll_divider(struct rcar_du_crtc *rcrtc,
 			for (fdpll = 1; fdpll < 32; fdpll++) {
 				unsigned long output;
 
-				/* 1/2 (FRQSEL=1) for duty rate 50% */
 				output = input * (n + 1) / (m + 1)
-				       / (fdpll + 1) / 2;
-
+				       / (fdpll + 1);
 				if (output >= 400000000)
 					continue;
 
@@ -158,6 +157,11 @@  static void rcar_du_dpll_divider(struct rcar_du_crtc *rcrtc,
 		 best_diff);
 }
 
+static const struct soc_device_attribute rcar_du_r8a7795_es1[] = {
+	{ .soc_id = "r8a7795", .revision = "ES1.*" },
+	{ /* sentinel */ }
+};
+
 static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 {
 	const struct drm_display_mode *mode = &rcrtc->crtc.state->adjusted_mode;
@@ -185,7 +189,20 @@  static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 
 		extclk = clk_get_rate(rcrtc->extclock);
 		if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {
-			rcar_du_dpll_divider(rcrtc, &dpll, extclk, mode_clock);
+			unsigned long target = mode_clock;
+
+			/*
+			 * The H3 ES1.x exhibits dot clock duty cycle stability
+			 * issues. We can work around them by configuring the
+			 * DPLL to twice the desired frequency, coupled with a
+			 * /2 post-divider. This isn't needed on other SoCs and
+			 * breaks HDMI output on M3-W for a currently unknown
+			 * reason, so restrict the workaround to H3 ES1.x.
+			 */
+			if (soc_device_match(rcar_du_r8a7795_es1))
+				target *= 2;
+
+			rcar_du_dpll_divider(rcrtc, &dpll, extclk, target);
 			extclk = dpll.output;
 		}
 
@@ -197,8 +214,6 @@  static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 
 		if (abs((long)extrate - (long)mode_clock) <
 		    abs((long)rate - (long)mode_clock)) {
-			dev_dbg(rcrtc->group->dev->dev,
-				"crtc%u: using external clock\n", rcrtc->index);
 
 			if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {
 				u32 dpllcr = DPLLCR_CODE | DPLLCR_CLKE
@@ -215,12 +230,14 @@  static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 
 				rcar_du_group_write(rcrtc->group, DPLLCR,
 						    dpllcr);
-
-				escr = ESCR_DCLKSEL_DCLKIN | 1;
-			} else {
-				escr = ESCR_DCLKSEL_DCLKIN | extdiv;
 			}
+
+			escr = ESCR_DCLKSEL_DCLKIN | extdiv;
 		}
+
+		dev_dbg(rcrtc->group->dev->dev,
+			"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
+			mode_clock, extrate, rate, escr);
 	}
 
 	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,