[PATCH 0/7] clk: renesas: rcar-gen3: Add support for CPU core clock boost modes

[PATCH 0/7] clk: renesas: rcar-gen3: Add support for CPU core clock boost modes

[Geert Uytterhoeven]

	Hi Mike, Stephen,

This patch series improves the accuracy of the CPU clock rate on R-Car
Gen3 SoCs, and adds support for CPU boost modes (marked "turbo-mode" in
the operating points table in DT).
As usual, it is preceded by a few small cleanups.

The easiest way to test this is by enabling
CONFIG_CPU_FREQ_DEFAULT_GOV_USERSPACE, and writing the requested clock
rate to /sys/devices/system/cpu/cpufreq/policy$n/scaling_setspeed
(n=0 for the first cluster, n=2 or 4 for the second cluster).
Note that you need to do

    echo 1 > /sys/devices/system/cpu/cpufreq/boost

before you can enable boost modes.

The PLL rates and CPU clock rates and voltages can be monitored in
/sys/kernel/debug/{clk/{z,z2,.pll[024]}/clk_rate,reg*/dvfs/cpu*/*uV}.
The DVFS voltage measured by the on-board max9611 can be monitored
remotely using iio-monitor.

This series has been tested on Salvator-(X)S (with R-Car H3 ES1.0, H3
ES2.0, M3-W, and M3-N) and Ebisu-4D (R-Car E3).

As boost modes may be unstable without increasing the CPU core voltage,
this series depends on "[PATCH 0/2] arm64: dts: renesas: Add cpu-supply
properties for DVFS"[1].  Hence I think it is better to postpone this to
v5.14.

Thanks for your comments!

[1] https://lore.kernel.org/r/20210326105009.1574424-1-geert+renesas@glider.be

Geert Uytterhoeven (7):
  clk: renesas: rcar-gen3: Update Z clock rate formula in comments
  clk: renesas: rcar-gen3: Make cpg_z_clk.mask u32
  clk: renesas: rcar-gen3: Remove superfluous masking in
    cpg_z_clk_set_rate()
  clk: renesas: rcar-gen3: Grammar s/dependent of/dependent on/
  clk: renesas: rcar-gen3: Increase Z clock accuracy
  clk: renesas: rcar-gen3: Add custom clock for PLLs
  clk: renesas: rcar-gen3: Add boost support to Z clocks

 drivers/clk/renesas/rcar-gen3-cpg.c | 183 +++++++++++++++++++++++-----
 1 file changed, 154 insertions(+), 29 deletions(-)

-- 
2.25.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 1/7] clk: renesas: rcar-gen3: Update Z clock rate formula in comments

[Geert Uytterhoeven]

The fixed divider in the calculation of the Z and Z2 clock rates was
generalized from a hardcoded value of two to a parameterized value, but
the comments were not updated accordingly.

Fixes: 20cc05ba04a93f05 ("clk: renesas: rcar-gen3: Parameterise Z and Z2 clock fixed divisor")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/clk/renesas/rcar-gen3-cpg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
index caa0f9414e45fe73..5edc85ab04b4430f 100644
--- a/drivers/clk/renesas/rcar-gen3-cpg.c
+++ b/drivers/clk/renesas/rcar-gen3-cpg.c
@@ -38,7 +38,8 @@
  * Traits of this clock:
  * prepare - clk_prepare only ensures that parents are prepared
  * enable - clk_enable only ensures that parents are enabled
- * rate - rate is adjustable.  clk->rate = (parent->rate * mult / 32 ) / 2
+ * rate - rate is adjustable.
+ *        clk->rate = (parent->rate * mult / 32 ) / fixed_div
  * parent - fixed parent.  No clk_set_parent support
  */
 #define CPG_FRQCRB			0x00000004
-- 
2.25.1

[PATCH 2/7] clk: renesas: rcar-gen3: Make cpg_z_clk.mask u32

[Geert Uytterhoeven]

cpg_z_clk.mask contains a mask for a 32-bit register.
Hence its size can be reduced from unsigned long to u32.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/clk/renesas/rcar-gen3-cpg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
index 5edc85ab04b4430f..e5edf1b908411643 100644
--- a/drivers/clk/renesas/rcar-gen3-cpg.c
+++ b/drivers/clk/renesas/rcar-gen3-cpg.c
@@ -50,8 +50,8 @@ struct cpg_z_clk {
 	struct clk_hw hw;
 	void __iomem *reg;
 	void __iomem *kick_reg;
-	unsigned long mask;
 	unsigned int fixed_div;
+	u32 mask;
 };
 
 #define to_z_clk(_hw)	container_of(_hw, struct cpg_z_clk, hw)
-- 
2.25.1

[PATCH 3/7] clk: renesas: rcar-gen3: Remove superfluous masking in cpg_z_clk_set_rate()

[Geert Uytterhoeven]

Due to the clamping of mult, "(32 - mult) << __ffs(zclk->mask)" can
never exceed the mask.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/clk/renesas/rcar-gen3-cpg.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
index e5edf1b908411643..cf0d049aa8c78115 100644
--- a/drivers/clk/renesas/rcar-gen3-cpg.c
+++ b/drivers/clk/renesas/rcar-gen3-cpg.c
@@ -104,8 +104,7 @@ static int cpg_z_clk_set_rate(struct clk_hw *hw, unsigned long rate,
 	if (readl(zclk->kick_reg) & CPG_FRQCRB_KICK)
 		return -EBUSY;
 
-	cpg_reg_modify(zclk->reg, zclk->mask,
-		       ((32 - mult) << __ffs(zclk->mask)) & zclk->mask);
+	cpg_reg_modify(zclk->reg, zclk->mask, (32 - mult) << __ffs(zclk->mask));
 
 	/*
 	 * Set KICK bit in FRQCRB to update hardware setting and wait for
-- 
2.25.1

[PATCH 4/7] clk: renesas: rcar-gen3: Grammar s/dependent of/dependent on/

[Geert Uytterhoeven]

Fix grammar in comments for cpg_z_clk_set_rate().

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/clk/renesas/rcar-gen3-cpg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
index cf0d049aa8c78115..a241bf6e904f2f66 100644
--- a/drivers/clk/renesas/rcar-gen3-cpg.c
+++ b/drivers/clk/renesas/rcar-gen3-cpg.c
@@ -117,7 +117,7 @@ static int cpg_z_clk_set_rate(struct clk_hw *hw, unsigned long rate,
 	 *
 	 * Using experimental measurements, it seems that no more than
 	 * ~10 iterations are needed, independently of the CPU rate.
-	 * Since this value might be dependent of external xtal rate, pll1
+	 * Since this value might be dependent on external xtal rate, pll1
 	 * rate or even the other emulation clocks rate, use 1000 as a
 	 * "super" safe value.
 	 */
-- 
2.25.1

[PATCH 5/7] clk: renesas: rcar-gen3: Increase Z clock accuracy

[Geert Uytterhoeven]

Improve accuracy in the .determine_rate() callback for Z and Z2 clocks
by using rounded divisions.  This is similar to the calculation of rates
and multipliers in the .recalc_rate() resp. set_rate() callbacks.

Sample impact for a few requested clock rates:
  - R-Car H3:
      - Z 500 MHz:	468 MHz => 515 MHz
      - Z2 1000 MHz:	973 MHz => 1011 MHz
  - R-Car M3-W:
      - Z 500 MHz:	422 MHz => 516 MHz
      - Z2 800 MHz:	750 MHz => 788 MHz

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/clk/renesas/rcar-gen3-cpg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
index a241bf6e904f2f66..6b389c1caca76f07 100644
--- a/drivers/clk/renesas/rcar-gen3-cpg.c
+++ b/drivers/clk/renesas/rcar-gen3-cpg.c
@@ -83,10 +83,10 @@ static int cpg_z_clk_determine_rate(struct clk_hw *hw,
 	if (max_mult < min_mult)
 		return -EINVAL;
 
-	mult = div64_ul(req->rate * 32ULL, prate);
+	mult = DIV_ROUND_CLOSEST_ULL(req->rate * 32ULL, prate);
 	mult = clamp(mult, min_mult, max_mult);
 
-	req->rate = div_u64((u64)prate * mult, 32);
+	req->rate = DIV_ROUND_CLOSEST_ULL((u64)prate * mult, 32);
 	return 0;
 }
 
-- 
2.25.1

[PATCH 6/7] clk: renesas: rcar-gen3: Add custom clock for PLLs

[Geert Uytterhoeven]

Currently the PLLs are modeled as fixed factor clocks, based on initial
settings.  However, enabling CPU boost clock rates requires increasing
the PLL clock rates.

Add a custom clock driver to model the PLL clocks.  This will allow the
Z (CPU) clock driver to request changing the PLL clock rate.

Based on a patch in the BSP by Takeshi Kihara
<takeshi.kihara.df@renesas.com>.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/clk/renesas/rcar-gen3-cpg.c | 147 ++++++++++++++++++++++++----
 1 file changed, 128 insertions(+), 19 deletions(-)

diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
index 6b389c1caca76f07..cc9a116d7d093ce7 100644
--- a/drivers/clk/renesas/rcar-gen3-cpg.c
+++ b/drivers/clk/renesas/rcar-gen3-cpg.c
@@ -26,12 +26,127 @@
 #include "rcar-cpg-lib.h"
 #include "rcar-gen3-cpg.h"
 
-#define CPG_PLL0CR		0x00d8
+#define CPG_PLLECR		0x00d0	/* PLL Enable Control Register */
+
+#define CPG_PLLECR_PLLST(n)	BIT(8 + (n))	/* PLLn Circuit Status */
+
+#define CPG_PLL0CR		0x00d8	/* PLLn Control Registers */
 #define CPG_PLL2CR		0x002c
 #define CPG_PLL4CR		0x01f4
 
+#define CPG_PLLnCR_STC_MASK	GENMASK(30, 24)	/* PLL Circuit Mult. Ratio */
+
 #define CPG_RCKCR_CKSEL	BIT(15)	/* RCLK Clock Source Select */
 
+/* PLL Clocks */
+struct cpg_pll_clk {
+	struct clk_hw hw;
+	void __iomem *pllcr_reg;
+	void __iomem *pllecr_reg;
+	unsigned int fixed_mult;
+	u32 pllecr_pllst_mask;
+};
+
+#define to_pll_clk(_hw)   container_of(_hw, struct cpg_pll_clk, hw)
+
+static unsigned long cpg_pll_clk_recalc_rate(struct clk_hw *hw,
+					     unsigned long parent_rate)
+{
+	struct cpg_pll_clk *pll_clk = to_pll_clk(hw);
+	unsigned int mult;
+	u32 val;
+
+	val = readl(pll_clk->pllcr_reg) & CPG_PLLnCR_STC_MASK;
+	mult = (val >> __ffs(CPG_PLLnCR_STC_MASK)) + 1;
+
+	return parent_rate * mult * pll_clk->fixed_mult;
+}
+
+static int cpg_pll_clk_determine_rate(struct clk_hw *hw,
+				      struct clk_rate_request *req)
+{
+	struct cpg_pll_clk *pll_clk = to_pll_clk(hw);
+	unsigned int min_mult, max_mult, mult;
+	unsigned long prate;
+
+	prate = req->best_parent_rate * pll_clk->fixed_mult;
+	min_mult = max(div64_ul(req->min_rate, prate), 1ULL);
+	max_mult = min(div64_ul(req->max_rate, prate), 128ULL);
+	if (max_mult < min_mult)
+		return -EINVAL;
+
+	mult = DIV_ROUND_CLOSEST_ULL(req->rate, prate);
+	mult = clamp(mult, min_mult, max_mult);
+
+	req->rate = prate * mult;
+	return 0;
+}
+
+static int cpg_pll_clk_set_rate(struct clk_hw *hw, unsigned long rate,
+				unsigned long parent_rate)
+{
+	struct cpg_pll_clk *pll_clk = to_pll_clk(hw);
+	unsigned int mult, i;
+	u32 val;
+
+	mult = DIV_ROUND_CLOSEST_ULL(rate, parent_rate * pll_clk->fixed_mult);
+	mult = clamp(mult, 1U, 128U);
+
+	val = readl(pll_clk->pllcr_reg);
+	val &= ~CPG_PLLnCR_STC_MASK;
+	val |= (mult - 1) << __ffs(CPG_PLLnCR_STC_MASK);
+	writel(val, pll_clk->pllcr_reg);
+
+	for (i = 1000; i; i--) {
+		if (readl(pll_clk->pllecr_reg) & pll_clk->pllecr_pllst_mask)
+			return 0;
+
+		cpu_relax();
+	}
+
+	return -ETIMEDOUT;
+}
+
+static const struct clk_ops cpg_pll_clk_ops = {
+	.recalc_rate = cpg_pll_clk_recalc_rate,
+	.determine_rate = cpg_pll_clk_determine_rate,
+	.set_rate = cpg_pll_clk_set_rate,
+};
+
+static struct clk * __init cpg_pll_clk_register(const char *name,
+						const char *parent_name,
+						void __iomem *base,
+						unsigned int mult,
+						unsigned int offset,
+						unsigned int index)
+
+{
+	struct cpg_pll_clk *pll_clk;
+	struct clk_init_data init = {};
+	struct clk *clk;
+
+	pll_clk = kzalloc(sizeof(*pll_clk), GFP_KERNEL);
+	if (!pll_clk)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &cpg_pll_clk_ops;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+
+	pll_clk->hw.init = &init;
+	pll_clk->pllcr_reg = base + offset;
+	pll_clk->pllecr_reg = base + CPG_PLLECR;
+	pll_clk->fixed_mult = mult;	/* PLL refclk x (setting + 1) x mult */
+	pll_clk->pllecr_pllst_mask = CPG_PLLECR_PLLST(index);
+
+	clk = clk_register(NULL, &pll_clk->hw);
+	if (IS_ERR(clk))
+		kfree(pll_clk);
+
+	return clk;
+}
+
 /*
  * Z Clock & Z2 Clock
  *
@@ -314,16 +429,13 @@ struct clk * __init rcar_gen3_cpg_clk_register(struct device *dev,
 
 	case CLK_TYPE_GEN3_PLL0:
 		/*
-		 * PLL0 is a configurable multiplier clock. Register it as a
-		 * fixed factor clock for now as there's no generic multiplier
-		 * clock implementation and we currently have no need to change
-		 * the multiplier value.
+		 * PLL0 is implemented as a custom clock, to change the
+		 * multiplier when cpufreq changes between normal and boost
+		 * modes.
 		 */
-		value = readl(base + CPG_PLL0CR);
-		mult = (((value >> 24) & 0x7f) + 1) * 2;
-		if (cpg_quirks & PLL_ERRATA)
-			mult *= 2;
-		break;
+		mult = (cpg_quirks & PLL_ERRATA) ? 4 : 2;
+		return cpg_pll_clk_register(core->name, __clk_get_name(parent),
+					    base, mult, CPG_PLL0CR, 0);
 
 	case CLK_TYPE_GEN3_PLL1:
 		mult = cpg_pll_config->pll1_mult;
@@ -332,16 +444,13 @@ struct clk * __init rcar_gen3_cpg_clk_register(struct device *dev,
 
 	case CLK_TYPE_GEN3_PLL2:
 		/*
-		 * PLL2 is a configurable multiplier clock. Register it as a
-		 * fixed factor clock for now as there's no generic multiplier
-		 * clock implementation and we currently have no need to change
-		 * the multiplier value.
+		 * PLL2 is implemented as a custom clock, to change the
+		 * multiplier when cpufreq changes between normal and boost
+		 * modes.
 		 */
-		value = readl(base + CPG_PLL2CR);
-		mult = (((value >> 24) & 0x7f) + 1) * 2;
-		if (cpg_quirks & PLL_ERRATA)
-			mult *= 2;
-		break;
+		mult = (cpg_quirks & PLL_ERRATA) ? 4 : 2;
+		return cpg_pll_clk_register(core->name, __clk_get_name(parent),
+					    base, mult, CPG_PLL2CR, 2);
 
 	case CLK_TYPE_GEN3_PLL3:
 		mult = cpg_pll_config->pll3_mult;
-- 
2.25.1

[PATCH 7/7] clk: renesas: rcar-gen3: Add boost support to Z clocks

[Geert Uytterhoeven]

Add support for switching the Z and Z2 clocks between normal and boost
modes, by requesting clock rate changes to parent PLLs.

Inspired by a patch in the BSP by Takeshi Kihara
<takeshi.kihara.df@renesas.com>.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/clk/renesas/rcar-gen3-cpg.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
index cc9a116d7d093ce7..558191c99b48d1f1 100644
--- a/drivers/clk/renesas/rcar-gen3-cpg.c
+++ b/drivers/clk/renesas/rcar-gen3-cpg.c
@@ -165,6 +165,7 @@ struct cpg_z_clk {
 	struct clk_hw hw;
 	void __iomem *reg;
 	void __iomem *kick_reg;
+	unsigned long max_rate;		/* Maximum rate for normal mode */
 	unsigned int fixed_div;
 	u32 mask;
 };
@@ -190,7 +191,18 @@ static int cpg_z_clk_determine_rate(struct clk_hw *hw,
 {
 	struct cpg_z_clk *zclk = to_z_clk(hw);
 	unsigned int min_mult, max_mult, mult;
-	unsigned long prate;
+	unsigned long rate, prate;
+
+	rate = min(req->rate, req->max_rate);
+	if (rate <= zclk->max_rate) {
+		/* Set parent rate to initial value for normal modes */
+		prate = zclk->max_rate;
+	} else {
+		/* Set increased parent rate for boost modes */
+		prate = rate;
+	}
+	req->best_parent_rate = clk_hw_round_rate(clk_hw_get_parent(hw),
+						  prate * zclk->fixed_div);
 
 	prate = req->best_parent_rate / zclk->fixed_div;
 	min_mult = max(div64_ul(req->min_rate * 32ULL, prate), 1ULL);
@@ -198,7 +210,7 @@ static int cpg_z_clk_determine_rate(struct clk_hw *hw,
 	if (max_mult < min_mult)
 		return -EINVAL;
 
-	mult = DIV_ROUND_CLOSEST_ULL(req->rate * 32ULL, prate);
+	mult = DIV_ROUND_CLOSEST_ULL(rate * 32ULL, prate);
 	mult = clamp(mult, min_mult, max_mult);
 
 	req->rate = DIV_ROUND_CLOSEST_ULL((u64)prate * mult, 32);
@@ -268,7 +280,7 @@ static struct clk * __init cpg_z_clk_register(const char *name,
 
 	init.name = name;
 	init.ops = &cpg_z_clk_ops;
-	init.flags = 0;
+	init.flags = CLK_SET_RATE_PARENT;
 	init.parent_names = &parent_name;
 	init.num_parents = 1;
 
@@ -279,9 +291,13 @@ static struct clk * __init cpg_z_clk_register(const char *name,
 	zclk->fixed_div = div; /* PLLVCO x 1/div x SYS-CPU divider */
 
 	clk = clk_register(NULL, &zclk->hw);
-	if (IS_ERR(clk))
+	if (IS_ERR(clk)) {
 		kfree(zclk);
+		return clk;
+	}
 
+	zclk->max_rate = clk_hw_get_rate(clk_hw_get_parent(&zclk->hw)) /
+			 zclk->fixed_div;
 	return clk;
 }
 
-- 
2.25.1

Re: [PATCH 0/7] clk: renesas: rcar-gen3: Add support for CPU core clock boost modes

[Stephen Boyd]

Quoting Geert Uytterhoeven (2021-03-26 05:00:53)
>         Hi Mike, Stephen,
> 
> This patch series improves the accuracy of the CPU clock rate on R-Car
> Gen3 SoCs, and adds support for CPU boost modes (marked "turbo-mode" in
> the operating points table in DT).
> As usual, it is preceded by a few small cleanups.
> 
> The easiest way to test this is by enabling
> CONFIG_CPU_FREQ_DEFAULT_GOV_USERSPACE, and writing the requested clock
> rate to /sys/devices/system/cpu/cpufreq/policy$n/scaling_setspeed
> (n=0 for the first cluster, n=2 or 4 for the second cluster).
> Note that you need to do
> 
>     echo 1 > /sys/devices/system/cpu/cpufreq/boost
> 
> before you can enable boost modes.
> 
> The PLL rates and CPU clock rates and voltages can be monitored in
> /sys/kernel/debug/{clk/{z,z2,.pll[024]}/clk_rate,reg*/dvfs/cpu*/*uV}.
> The DVFS voltage measured by the on-board max9611 can be monitored
> remotely using iio-monitor.
> 
> This series has been tested on Salvator-(X)S (with R-Car H3 ES1.0, H3
> ES2.0, M3-W, and M3-N) and Ebisu-4D (R-Car E3).
> 
> As boost modes may be unstable without increasing the CPU core voltage,
> this series depends on "[PATCH 0/2] arm64: dts: renesas: Add cpu-supply
> properties for DVFS"[1].  Hence I think it is better to postpone this to
> v5.14.
> 
> Thanks for your comments!
> 

Acked-by: Stephen Boyd <sboyd@kernel.org>

Re: [PATCH 6/7] clk: renesas: rcar-gen3: Add custom clock for PLLs

[Stephen Boyd]

Quoting Geert Uytterhoeven (2021-03-26 05:00:59)
> +}
> +
> +static int cpg_pll_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> +                               unsigned long parent_rate)
> +{
> +       struct cpg_pll_clk *pll_clk = to_pll_clk(hw);
> +       unsigned int mult, i;
> +       u32 val;
> +
> +       mult = DIV_ROUND_CLOSEST_ULL(rate, parent_rate * pll_clk->fixed_mult);
> +       mult = clamp(mult, 1U, 128U);
> +
> +       val = readl(pll_clk->pllcr_reg);
> +       val &= ~CPG_PLLnCR_STC_MASK;
> +       val |= (mult - 1) << __ffs(CPG_PLLnCR_STC_MASK);
> +       writel(val, pll_clk->pllcr_reg);
> +
> +       for (i = 1000; i; i--) {
> +               if (readl(pll_clk->pllecr_reg) & pll_clk->pllecr_pllst_mask)
> +                       return 0;
> +
> +               cpu_relax();
> +       }

Why not readl_poll_timeout()?

> +
> +       return -ETIMEDOUT;
> +}
> +

Re: [PATCH 6/7] clk: renesas: rcar-gen3: Add custom clock for PLLs

[Geert Uytterhoeven]

Hi Stephen,

On Tue, Mar 30, 2021 at 4:22 AM Stephen Boyd <sboyd@kernel.org> wrote:
> Quoting Geert Uytterhoeven (2021-03-26 05:00:59)
> > +}
> > +
> > +static int cpg_pll_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> > +                               unsigned long parent_rate)
> > +{
> > +       struct cpg_pll_clk *pll_clk = to_pll_clk(hw);
> > +       unsigned int mult, i;
> > +       u32 val;
> > +
> > +       mult = DIV_ROUND_CLOSEST_ULL(rate, parent_rate * pll_clk->fixed_mult);
> > +       mult = clamp(mult, 1U, 128U);
> > +
> > +       val = readl(pll_clk->pllcr_reg);
> > +       val &= ~CPG_PLLnCR_STC_MASK;
> > +       val |= (mult - 1) << __ffs(CPG_PLLnCR_STC_MASK);
> > +       writel(val, pll_clk->pllcr_reg);
> > +
> > +       for (i = 1000; i; i--) {
> > +               if (readl(pll_clk->pllecr_reg) & pll_clk->pllecr_pllst_mask)
> > +                       return 0;
> > +
> > +               cpu_relax();
> > +       }
>
> Why not readl_poll_timeout()?

I had considered that.  But then I noticed readl_poll_timeout() does not
call cpu_relax() if sleep_us == 0.  Perhaps such a call should be added
(at the risk of introducing a subtle regression elsewhere)?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 0/7] clk: renesas: rcar-gen3: Add support for CPU core clock boost modes

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Friday, March 26, 2021 9:01 PM
> 
> 	Hi Mike, Stephen,
> 
> This patch series improves the accuracy of the CPU clock rate on R-Car
> Gen3 SoCs, and adds support for CPU boost modes (marked "turbo-mode" in
> the operating points table in DT).
> As usual, it is preceded by a few small cleanups.
> 
> The easiest way to test this is by enabling
> CONFIG_CPU_FREQ_DEFAULT_GOV_USERSPACE, and writing the requested clock
> rate to /sys/devices/system/cpu/cpufreq/policy$n/scaling_setspeed
> (n=0 for the first cluster, n=2 or 4 for the second cluster).
> Note that you need to do
> 
>     echo 1 > /sys/devices/system/cpu/cpufreq/boost
> 
> before you can enable boost modes.
> 
> The PLL rates and CPU clock rates and voltages can be monitored in
> /sys/kernel/debug/{clk/{z,z2,.pll[024]}/clk_rate,reg*/dvfs/cpu*/*uV}.
> The DVFS voltage measured by the on-board max9611 can be monitored
> remotely using iio-monitor.
> 
> This series has been tested on Salvator-(X)S (with R-Car H3 ES1.0, H3
> ES2.0, M3-W, and M3-N) and Ebisu-4D (R-Car E3).

Thank you for the patch!

Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

And, I tested on R-Car H3 ES3.0 and it worked. So,

Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
# I'm not sure adding Tested-by tag is suitable for patch 1 and 4 though...

> As boost modes may be unstable without increasing the CPU core voltage,
> this series depends on "[PATCH 0/2] arm64: dts: renesas: Add cpu-supply
> properties for DVFS"[1].  Hence I think it is better to postpone this to
> v5.14.

I think so.

Best regards,
Yoshihiro Shimoda

Re: [PATCH 6/7] clk: renesas: rcar-gen3: Add custom clock for PLLs

[Stephen Boyd]

Quoting Geert Uytterhoeven (2021-03-30 00:02:03)
> Hi Stephen,
> 
> On Tue, Mar 30, 2021 at 4:22 AM Stephen Boyd <sboyd@kernel.org> wrote:
> > Quoting Geert Uytterhoeven (2021-03-26 05:00:59)
> > > +}
> > > +
> > > +static int cpg_pll_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> > > +                               unsigned long parent_rate)
> > > +{
> > > +       struct cpg_pll_clk *pll_clk = to_pll_clk(hw);
> > > +       unsigned int mult, i;
> > > +       u32 val;
> > > +
> > > +       mult = DIV_ROUND_CLOSEST_ULL(rate, parent_rate * pll_clk->fixed_mult);
> > > +       mult = clamp(mult, 1U, 128U);
> > > +
> > > +       val = readl(pll_clk->pllcr_reg);
> > > +       val &= ~CPG_PLLnCR_STC_MASK;
> > > +       val |= (mult - 1) << __ffs(CPG_PLLnCR_STC_MASK);
> > > +       writel(val, pll_clk->pllcr_reg);
> > > +
> > > +       for (i = 1000; i; i--) {
> > > +               if (readl(pll_clk->pllecr_reg) & pll_clk->pllecr_pllst_mask)
> > > +                       return 0;
> > > +
> > > +               cpu_relax();
> > > +       }
> >
> > Why not readl_poll_timeout()?
> 
> I had considered that.  But then I noticed readl_poll_timeout() does not
> call cpu_relax() if sleep_us == 0.  Perhaps such a call should be added
> (at the risk of introducing a subtle regression elsewhere)?
> 

Risk seems low. Maybe introduce a readl_poll_timeout_relaxed() if it's a
concern. At the least, a patch to document why it doesn't use
cpu_relax() right now would be good to have.