| Message ID | 20170626181226.29575-14-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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, >
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,
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 --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,
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(-)