[PATCH v5 0/2] Add notifier for PLL0 clock and set it 1.5GHz on

[PATCH v5 0/2] Add notifier for PLL0 clock and set it 1.5GHz on

[Xingyu Wu]

This patch is to add the notifier for PLL0 clock and set the PLL0 rate
to 1.5GHz to fix the lower rate of CPUfreq on the JH7110 SoC.

The first patch is to add the notifier for PLL0 clock. Setting the PLL0
rate need the son clock (cpu_root) to switch its parent clock to OSC 
clock and switch it back after setting PLL0 rate. It need to use the 
cpu_root clock from SYSCRG and register the notifier in the SYSCRG
driver.

The second patch is to set cpu_core rate to 500MHz and PLL0 rate to
1.5GHz to fix the problem about the lower rate of CPUfreq on the 
visionfive board. The cpu_core clock rate is set to 500MHz first to
ensure that the cpu frequency will not suddenly become high and the cpu 
voltage is not enough to cause a crash when the PLL0 is set to 1.5GHz.
The cpu voltage and frequency are then adjusted together by CPUfreq.

Changes since v4:
- Fixed the wrong words.
- Added the Fixes tag in first patch.

v4: https://lore.kernel.org/all/20240410033148.213991-1-xingyu.wu@starfivetech.com/

Changes since v3: 
- Added the notifier for PLL0 clock.
- Set cpu_core rate in DTS 

v3: https://lore.kernel.org/all/20240402090920.11627-1-xingyu.wu@starfivetech.com/

Changes since v2: 
- Made the steps into the process into the process of setting PLL0 rate

v2: https://lore.kernel.org/all/20230821152915.208366-1-xingyu.wu@starfivetech.com/

Changes since v1: 
- Added the fixes tag in the commit.

v1: https://lore.kernel.org/all/20230811033631.160912-1-xingyu.wu@starfivetech.com/

Xingyu Wu (2):
  clk: starfive: jh7110-sys: Add notifier for PLL0 clock
  riscv: dts: starfive: visionfive-2: Fix lower rate of CPUfreq by
    setting PLL0 rate to 1.5GHz

 .../jh7110-starfive-visionfive-2.dtsi         |  6 ++++
 .../clk/starfive/clk-starfive-jh7110-sys.c    | 31 ++++++++++++++++++-
 drivers/clk/starfive/clk-starfive-jh71x0.h    |  2 ++
 3 files changed, 38 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH v5 1/2] clk: starfive: jh7110-sys: Add notifier for PLL0 clock

[Xingyu Wu]

Add notifier function for PLL0 clock. In the function, the cpu_root clock
should be operated by saving its current parent and setting a new safe
parent (osc clock) before setting the PLL0 clock rate. After setting PLL0
rate, it should be switched back to the original parent clock.

Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for JH7110 SoC")
Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
---
 .../clk/starfive/clk-starfive-jh7110-sys.c    | 31 ++++++++++++++++++-
 drivers/clk/starfive/clk-starfive-jh71x0.h    |  2 ++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/starfive/clk-starfive-jh7110-sys.c b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
index 8f5e5abfa178..dafa3ae71751 100644
--- a/drivers/clk/starfive/clk-starfive-jh7110-sys.c
+++ b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
@@ -385,6 +385,32 @@ int jh7110_reset_controller_register(struct jh71x0_clk_priv *priv,
 }
 EXPORT_SYMBOL_GPL(jh7110_reset_controller_register);
 
+/*
+ * This clock notifier is called when the rate of PLL0 clock is to be changed.
+ * The cpu_root clock should save the curent parent clock and swicth its parent
+ * clock to osc before PLL0 rate will be changed. Then swicth its parent clock
+ * back after the PLL0 rate is completed.
+ */
+static int jh7110_pll0_clk_notifier_cb(struct notifier_block *nb,
+				       unsigned long action, void *data)
+{
+	struct jh71x0_clk_priv *priv = container_of(nb, struct jh71x0_clk_priv, pll_clk_nb);
+	struct clk *cpu_root = priv->reg[JH7110_SYSCLK_CPU_ROOT].hw.clk;
+	int ret = 0;
+
+	if (action == PRE_RATE_CHANGE) {
+		struct clk *osc = clk_get(priv->dev, "osc");
+
+		priv->original_clk = clk_get_parent(cpu_root);
+		ret = clk_set_parent(cpu_root, osc);
+		clk_put(osc);
+	} else if (action == POST_RATE_CHANGE) {
+		ret = clk_set_parent(cpu_root, priv->original_clk);
+	}
+
+	return notifier_from_errno(ret);
+}
+
 static int __init jh7110_syscrg_probe(struct platform_device *pdev)
 {
 	struct jh71x0_clk_priv *priv;
@@ -413,7 +439,10 @@ static int __init jh7110_syscrg_probe(struct platform_device *pdev)
 		if (IS_ERR(priv->pll[0]))
 			return PTR_ERR(priv->pll[0]);
 	} else {
-		clk_put(pllclk);
+		priv->pll_clk_nb.notifier_call = jh7110_pll0_clk_notifier_cb;
+		ret = clk_notifier_register(pllclk, &priv->pll_clk_nb);
+		if (ret)
+			return ret;
 		priv->pll[0] = NULL;
 	}
 
diff --git a/drivers/clk/starfive/clk-starfive-jh71x0.h b/drivers/clk/starfive/clk-starfive-jh71x0.h
index 23e052fc1549..e3f441393e48 100644
--- a/drivers/clk/starfive/clk-starfive-jh71x0.h
+++ b/drivers/clk/starfive/clk-starfive-jh71x0.h
@@ -114,6 +114,8 @@ struct jh71x0_clk_priv {
 	spinlock_t rmw_lock;
 	struct device *dev;
 	void __iomem *base;
+	struct clk *original_clk;
+	struct notifier_block pll_clk_nb;
 	struct clk_hw *pll[3];
 	struct jh71x0_clk reg[];
 };
-- 
2.25.1

Re: [PATCH v5 1/2] clk: starfive: jh7110-sys: Add notifier for PLL0 clock

[Emil Renner Berthing]

Xingyu Wu wrote:
> Add notifier function for PLL0 clock. In the function, the cpu_root clock
> should be operated by saving its current parent and setting a new safe
> parent (osc clock) before setting the PLL0 clock rate. After setting PLL0
> rate, it should be switched back to the original parent clock.
>
> Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for JH7110 SoC")
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>

This looks much better, thanks!

Reviewed-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>

> ---
>  .../clk/starfive/clk-starfive-jh7110-sys.c    | 31 ++++++++++++++++++-
>  drivers/clk/starfive/clk-starfive-jh71x0.h    |  2 ++
>  2 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/starfive/clk-starfive-jh7110-sys.c b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
> index 8f5e5abfa178..dafa3ae71751 100644
> --- a/drivers/clk/starfive/clk-starfive-jh7110-sys.c
> +++ b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
> @@ -385,6 +385,32 @@ int jh7110_reset_controller_register(struct jh71x0_clk_priv *priv,
>  }
>  EXPORT_SYMBOL_GPL(jh7110_reset_controller_register);
>
> +/*
> + * This clock notifier is called when the rate of PLL0 clock is to be changed.
> + * The cpu_root clock should save the curent parent clock and swicth its parent
> + * clock to osc before PLL0 rate will be changed. Then swicth its parent clock
> + * back after the PLL0 rate is completed.
> + */
> +static int jh7110_pll0_clk_notifier_cb(struct notifier_block *nb,
> +				       unsigned long action, void *data)
> +{
> +	struct jh71x0_clk_priv *priv = container_of(nb, struct jh71x0_clk_priv, pll_clk_nb);
> +	struct clk *cpu_root = priv->reg[JH7110_SYSCLK_CPU_ROOT].hw.clk;
> +	int ret = 0;
> +
> +	if (action == PRE_RATE_CHANGE) {
> +		struct clk *osc = clk_get(priv->dev, "osc");
> +
> +		priv->original_clk = clk_get_parent(cpu_root);
> +		ret = clk_set_parent(cpu_root, osc);
> +		clk_put(osc);
> +	} else if (action == POST_RATE_CHANGE) {
> +		ret = clk_set_parent(cpu_root, priv->original_clk);
> +	}
> +
> +	return notifier_from_errno(ret);
> +}
> +
>  static int __init jh7110_syscrg_probe(struct platform_device *pdev)
>  {
>  	struct jh71x0_clk_priv *priv;
> @@ -413,7 +439,10 @@ static int __init jh7110_syscrg_probe(struct platform_device *pdev)
>  		if (IS_ERR(priv->pll[0]))
>  			return PTR_ERR(priv->pll[0]);
>  	} else {
> -		clk_put(pllclk);
> +		priv->pll_clk_nb.notifier_call = jh7110_pll0_clk_notifier_cb;
> +		ret = clk_notifier_register(pllclk, &priv->pll_clk_nb);
> +		if (ret)
> +			return ret;
>  		priv->pll[0] = NULL;
>  	}
>
> diff --git a/drivers/clk/starfive/clk-starfive-jh71x0.h b/drivers/clk/starfive/clk-starfive-jh71x0.h
> index 23e052fc1549..e3f441393e48 100644
> --- a/drivers/clk/starfive/clk-starfive-jh71x0.h
> +++ b/drivers/clk/starfive/clk-starfive-jh71x0.h
> @@ -114,6 +114,8 @@ struct jh71x0_clk_priv {
>  	spinlock_t rmw_lock;
>  	struct device *dev;
>  	void __iomem *base;
> +	struct clk *original_clk;
> +	struct notifier_block pll_clk_nb;
>  	struct clk_hw *pll[3];
>  	struct jh71x0_clk reg[];
>  };
> --
> 2.25.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v5 2/2] riscv: dts: starfive: visionfive-2: Fix lower rate of CPUfreq by setting PLL0 rate to 1.5GHz

[Xingyu Wu]

CPUfreq supports 4 cpu frequency loads on 375/500/750/1500MHz.
But now PLL0 rate is 1GHz and the cpu frequency loads become
333/500/500/1000MHz in fact.

The PLL0 rate should be default set to 1.5GHz and set the
cpu_core rate to 500MHz in safe.

Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for JH7110 SoC")
Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
---
 .../boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi     | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
index 45b58b6f3df8..28981b267de4 100644
--- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
@@ -390,6 +390,12 @@ spi_dev0: spi@0 {
 	};
 };
 
+&syscrg {
+	assigned-clocks = <&syscrg JH7110_SYSCLK_CPU_CORE>,
+			  <&pllclk JH7110_PLLCLK_PLL0_OUT>;
+	assigned-clock-rates = <500000000>, <1500000000>;
+};
+
 &sysgpio {
 	i2c0_pins: i2c0-0 {
 		i2c-pins {
-- 
2.25.1

Re: [PATCH v5 2/2] riscv: dts: starfive: visionfive-2: Fix lower rate of CPUfreq by setting PLL0 rate to 1.5GHz

[Emil Renner Berthing]

Xingyu Wu wrote:
> CPUfreq supports 4 cpu frequency loads on 375/500/750/1500MHz.
> But now PLL0 rate is 1GHz and the cpu frequency loads become
> 333/500/500/1000MHz in fact.
>
> The PLL0 rate should be default set to 1.5GHz and set the
> cpu_core rate to 500MHz in safe.
>
> Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for JH7110 SoC")
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>

This should really be based on Conor's riscv-dt-for-next branch, eg. the change
should be to the new jh7110-common.dtsi instead since the Milk-V Mars board
would most likely also benefit from this change.

In any case:
Reviewed-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>

> ---
>  .../boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi     | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> index 45b58b6f3df8..28981b267de4 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> @@ -390,6 +390,12 @@ spi_dev0: spi@0 {
>  	};
>  };
>
> +&syscrg {
> +	assigned-clocks = <&syscrg JH7110_SYSCLK_CPU_CORE>,
> +			  <&pllclk JH7110_PLLCLK_PLL0_OUT>;
> +	assigned-clock-rates = <500000000>, <1500000000>;
> +};
> +
>  &sysgpio {
>  	i2c0_pins: i2c0-0 {
>  		i2c-pins {
> --
> 2.25.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v5 2/2] riscv: dts: starfive: visionfive-2: Fix lower rate of CPUfreq by setting PLL0 rate to 1.5GHz

[Samuel Holland]

On 2024-05-07 1:53 AM, Xingyu Wu wrote:
> CPUfreq supports 4 cpu frequency loads on 375/500/750/1500MHz.
> But now PLL0 rate is 1GHz and the cpu frequency loads become
> 333/500/500/1000MHz in fact.
> 
> The PLL0 rate should be default set to 1.5GHz and set the
> cpu_core rate to 500MHz in safe.

Can this be accomplished by instead setting the CLK_SET_RATE_PARENT flag on the
CPU_CORE clock? That way PLL0 is automatically set when cpufreq tries to change
the CPU core frequency. Then there is no DT change and no compatibility issue.

Regards,
Samuel

> Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for JH7110 SoC")
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> ---
>  .../boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi     | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> index 45b58b6f3df8..28981b267de4 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> @@ -390,6 +390,12 @@ spi_dev0: spi@0 {
>  	};
>  };
>  
> +&syscrg {
> +	assigned-clocks = <&syscrg JH7110_SYSCLK_CPU_CORE>,
> +			  <&pllclk JH7110_PLLCLK_PLL0_OUT>;
> +	assigned-clock-rates = <500000000>, <1500000000>;
> +};
> +
>  &sysgpio {
>  	i2c0_pins: i2c0-0 {
>  		i2c-pins {

RE: [PATCH v5 2/2] riscv: dts: starfive: visionfive-2: Fix lower rate of CPUfreq by setting PLL0 rate to 1.5GHz

[Xingyu Wu]

On 12/05/2024 02:47, Samuel Holland wrote:
> 
> On 2024-05-07 1:53 AM, Xingyu Wu wrote:
> > CPUfreq supports 4 cpu frequency loads on 375/500/750/1500MHz.
> > But now PLL0 rate is 1GHz and the cpu frequency loads become
> > 333/500/500/1000MHz in fact.
> >
> > The PLL0 rate should be default set to 1.5GHz and set the cpu_core
> > rate to 500MHz in safe.
> 	
> Can this be accomplished by instead setting the CLK_SET_RATE_PARENT flag on
> the CPU_CORE clock? That way PLL0 is automatically set when cpufreq tries to
> change the CPU core frequency. Then there is no DT change and no compatibility
> issue.
> 
> Regards,
> Samuel

Thanks for your advice. But cpufreq tries to change the CPU core rate and also
the PLL0 rate with the flag of CLK_SET_RATE_PARENT and the PLL0 will be
changed frequently. I think it goes against our intention and the PLL0 rate should
be fixed or rarely changed. This helps to stabilize the system.

Best regards,
Xingyu Wu

> 
> > Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for JH7110
> > SoC")
> > Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> > ---
> >  .../boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi     | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git
> > a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> > b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> > index 45b58b6f3df8..28981b267de4 100644
> > --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> > +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> > @@ -390,6 +390,12 @@ spi_dev0: spi@0 {
> >  	};
> >  };
> >
> > +&syscrg {
> > +	assigned-clocks = <&syscrg JH7110_SYSCLK_CPU_CORE>,
> > +			  <&pllclk JH7110_PLLCLK_PLL0_OUT>;
> > +	assigned-clock-rates = <500000000>, <1500000000>; };
> > +
> >  &sysgpio {
> >  	i2c0_pins: i2c0-0 {
> >  		i2c-pins {

Re: [PATCH v5 0/2] Add notifier for PLL0 clock and set it 1.5GHz on

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

On Tue, May 07, 2024 at 02:53:17PM +0800, Xingyu Wu wrote:
> This patch is to add the notifier for PLL0 clock and set the PLL0 rate
> to 1.5GHz to fix the lower rate of CPUfreq on the JH7110 SoC.
> 
> The first patch is to add the notifier for PLL0 clock. Setting the PLL0
> rate need the son clock (cpu_root) to switch its parent clock to OSC 
> clock and switch it back after setting PLL0 rate. It need to use the 
> cpu_root clock from SYSCRG and register the notifier in the SYSCRG
> driver.
> 
> The second patch is to set cpu_core rate to 500MHz and PLL0 rate to
> 1.5GHz to fix the problem about the lower rate of CPUfreq on the 
> visionfive board. The cpu_core clock rate is set to 500MHz first to
> ensure that the cpu frequency will not suddenly become high and the cpu 
> voltage is not enough to cause a crash when the PLL0 is set to 1.5GHz.
> The cpu voltage and frequency are then adjusted together by CPUfreq.

Hmm, how does sequencing work here? If we split the patches between
trees it sounds like without the dts patch, the clock tree would (or
could) crash, or mainline if the clock changes there before the dts ones
do. Am I misunderstanding that?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH v5 0/2] Add notifier for PLL0 clock and set it 1.5GHz on

[Xingyu Wu]

On 11/05/2024 05:05, Conor Dooley wrote:
> 
> On Tue, May 07, 2024 at 02:53:17PM +0800, Xingyu Wu wrote:
> > This patch is to add the notifier for PLL0 clock and set the PLL0 rate
> > to 1.5GHz to fix the lower rate of CPUfreq on the JH7110 SoC.
> >
> > The first patch is to add the notifier for PLL0 clock. Setting the
> > PLL0 rate need the son clock (cpu_root) to switch its parent clock to
> > OSC clock and switch it back after setting PLL0 rate. It need to use
> > the cpu_root clock from SYSCRG and register the notifier in the SYSCRG
> > driver.
> >
> > The second patch is to set cpu_core rate to 500MHz and PLL0 rate to
> > 1.5GHz to fix the problem about the lower rate of CPUfreq on the
> > visionfive board. The cpu_core clock rate is set to 500MHz first to
> > ensure that the cpu frequency will not suddenly become high and the
> > cpu voltage is not enough to cause a crash when the PLL0 is set to 1.5GHz.
> > The cpu voltage and frequency are then adjusted together by CPUfreq.
> 
> Hmm, how does sequencing work here? If we split the patches between trees it
> sounds like without the dts patch, the clock tree would (or
> could) crash, or mainline if the clock changes there before the dts ones do. Am I
> misunderstanding that?

Oh, I think you misunderstood it. Patch 1 (clock driver patch) does not cause the
clock tree crash without the patch 2 (dts patch), and it just provides the correct
flow of how to change the PLL0 rate. The patch 2 is to set the clock rate of
cpu_core and PLL0 rate, which causes the crash without patch 1. Setting cpu_core
rate is to avoid crashes by insufficient cpu voltage when setting PLL0 rate.

Best regards,
Xingyu Wu

Re: [PATCH v5 0/2] Add notifier for PLL0 clock and set it 1.5GHz on

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2064 bytes --]

On Sat, May 11, 2024 at 03:02:56AM +0000, Xingyu Wu wrote:
> On 11/05/2024 05:05, Conor Dooley wrote:
> > 
> > On Tue, May 07, 2024 at 02:53:17PM +0800, Xingyu Wu wrote:
> > > This patch is to add the notifier for PLL0 clock and set the PLL0 rate
> > > to 1.5GHz to fix the lower rate of CPUfreq on the JH7110 SoC.
> > >
> > > The first patch is to add the notifier for PLL0 clock. Setting the
> > > PLL0 rate need the son clock (cpu_root) to switch its parent clock to
> > > OSC clock and switch it back after setting PLL0 rate. It need to use
> > > the cpu_root clock from SYSCRG and register the notifier in the SYSCRG
> > > driver.
> > >
> > > The second patch is to set cpu_core rate to 500MHz and PLL0 rate to
> > > 1.5GHz to fix the problem about the lower rate of CPUfreq on the
> > > visionfive board. The cpu_core clock rate is set to 500MHz first to
> > > ensure that the cpu frequency will not suddenly become high and the
> > > cpu voltage is not enough to cause a crash when the PLL0 is set to 1.5GHz.
> > > The cpu voltage and frequency are then adjusted together by CPUfreq.
> > 
> > Hmm, how does sequencing work here? If we split the patches between trees it
> > sounds like without the dts patch, the clock tree would (or
> > could) crash, or mainline if the clock changes there before the dts ones do. Am I
> > misunderstanding that?
> 
> Oh, I think you misunderstood it. Patch 1 (clock driver patch) does not cause the
> clock tree crash without the patch 2 (dts patch), and it just provides the correct
> flow of how to change the PLL0 rate. The patch 2 is to set the clock rate of
> cpu_core and PLL0 rate, which causes the crash without patch 1. Setting cpu_core
> rate is to avoid crashes by insufficient cpu voltage when setting PLL0 rate.

So is the problem in the other direction then? My dts tree will crash if
I apply the dts change without the clock patch?
Additionally, what about U-Boot? Will it have problems if the dts is
imported there without changes to its clock driver?

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH v5 0/2] Add notifier for PLL0 clock and set it 1.5GHz on

[Xingyu Wu]

On 11/05/2024 20:19, Conor Dooley wrote:
> 
> On Sat, May 11, 2024 at 03:02:56AM +0000, Xingyu Wu wrote:
> > On 11/05/2024 05:05, Conor Dooley wrote:
> > >
> > > On Tue, May 07, 2024 at 02:53:17PM +0800, Xingyu Wu wrote:
> > > > This patch is to add the notifier for PLL0 clock and set the PLL0
> > > > rate to 1.5GHz to fix the lower rate of CPUfreq on the JH7110 SoC.
> > > >
> > > > The first patch is to add the notifier for PLL0 clock. Setting the
> > > > PLL0 rate need the son clock (cpu_root) to switch its parent clock
> > > > to OSC clock and switch it back after setting PLL0 rate. It need
> > > > to use the cpu_root clock from SYSCRG and register the notifier in
> > > > the SYSCRG driver.
> > > >
> > > > The second patch is to set cpu_core rate to 500MHz and PLL0 rate
> > > > to 1.5GHz to fix the problem about the lower rate of CPUfreq on
> > > > the visionfive board. The cpu_core clock rate is set to 500MHz
> > > > first to ensure that the cpu frequency will not suddenly become
> > > > high and the cpu voltage is not enough to cause a crash when the PLL0 is set
> to 1.5GHz.
> > > > The cpu voltage and frequency are then adjusted together by CPUfreq.
> > >
> > > Hmm, how does sequencing work here? If we split the patches between
> > > trees it sounds like without the dts patch, the clock tree would (or
> > > could) crash, or mainline if the clock changes there before the dts
> > > ones do. Am I misunderstanding that?
> >
> > Oh, I think you misunderstood it. Patch 1 (clock driver patch) does
> > not cause the clock tree crash without the patch 2 (dts patch), and it
> > just provides the correct flow of how to change the PLL0 rate. The
> > patch 2 is to set the clock rate of cpu_core and PLL0 rate, which
> > causes the crash without patch 1. Setting cpu_core rate is to avoid crashes by
> insufficient cpu voltage when setting PLL0 rate.
> 
> So is the problem in the other direction then? My dts tree will crash if I apply the
> dts change without the clock patch?

Sorry, I tested it and it could not crash using only dts patch. It can separate the
patches and use it individually.

> Additionally, what about U-Boot? Will it have problems if the dts is imported
> there without changes to its clock driver?
> 

It is not apply to U-Boot. In the U-Boot, the PLL0 rate should be 1GHz to for GMAC
and PMIC to work. But now the PLL0 rate should be 1.5GHz in the Linux.

Best regards,
Xingyu Wu

Re: [PATCH v5 0/2] Add notifier for PLL0 clock and set it 1.5GHz on

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2901 bytes --]

On Tue, May 14, 2024 at 07:40:02AM +0000, Xingyu Wu wrote:
> On 11/05/2024 20:19, Conor Dooley wrote:
> > 
> > On Sat, May 11, 2024 at 03:02:56AM +0000, Xingyu Wu wrote:
> > > On 11/05/2024 05:05, Conor Dooley wrote:
> > > >
> > > > On Tue, May 07, 2024 at 02:53:17PM +0800, Xingyu Wu wrote:
> > > > > This patch is to add the notifier for PLL0 clock and set the PLL0
> > > > > rate to 1.5GHz to fix the lower rate of CPUfreq on the JH7110 SoC.
> > > > >
> > > > > The first patch is to add the notifier for PLL0 clock. Setting the
> > > > > PLL0 rate need the son clock (cpu_root) to switch its parent clock
> > > > > to OSC clock and switch it back after setting PLL0 rate. It need
> > > > > to use the cpu_root clock from SYSCRG and register the notifier in
> > > > > the SYSCRG driver.
> > > > >
> > > > > The second patch is to set cpu_core rate to 500MHz and PLL0 rate
> > > > > to 1.5GHz to fix the problem about the lower rate of CPUfreq on
> > > > > the visionfive board. The cpu_core clock rate is set to 500MHz
> > > > > first to ensure that the cpu frequency will not suddenly become
> > > > > high and the cpu voltage is not enough to cause a crash when the PLL0 is set
> > to 1.5GHz.
> > > > > The cpu voltage and frequency are then adjusted together by CPUfreq.
> > > >
> > > > Hmm, how does sequencing work here? If we split the patches between
> > > > trees it sounds like without the dts patch, the clock tree would (or
> > > > could) crash, or mainline if the clock changes there before the dts
> > > > ones do. Am I misunderstanding that?
> > >
> > > Oh, I think you misunderstood it. Patch 1 (clock driver patch) does
> > > not cause the clock tree crash without the patch 2 (dts patch), and it
> > > just provides the correct flow of how to change the PLL0 rate. The
> > > patch 2 is to set the clock rate of cpu_core and PLL0 rate, which
> > > causes the crash without patch 1. Setting cpu_core rate is to avoid crashes by
> > insufficient cpu voltage when setting PLL0 rate.
> > 
> > So is the problem in the other direction then? My dts tree will crash if I apply the
> > dts change without the clock patch?
> 
> Sorry, I tested it and it could not crash using only dts patch. It can separate the
> patches and use it individually.
> 
> > Additionally, what about U-Boot? Will it have problems if the dts is imported
> > there without changes to its clock driver?
> > 
> 
> It is not apply to U-Boot. In the U-Boot, the PLL0 rate should be 1GHz to for GMAC
> and PMIC to work. But now the PLL0 rate should be 1.5GHz in the Linux.

There's a push in U-Boot to move devicestrees to use "OF_UPSTREAM",
which means importing devicetrees directly from Linux and using them in
U-Boot. I don't really want to merge a patch that would present U-Boot
with a problem if the VisionFive 2 moved to that model there.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH v5 0/2] Add notifier for PLL0 clock and set it 1.5GHz on

[Xingyu Wu]

On 15/05/2024 02:08, Conor Dooley wrote:
> 
> On Tue, May 14, 2024 at 07:40:02AM +0000, Xingyu Wu wrote:
> > On 11/05/2024 20:19, Conor Dooley wrote:
> > >
> > > On Sat, May 11, 2024 at 03:02:56AM +0000, Xingyu Wu wrote:
> > > > On 11/05/2024 05:05, Conor Dooley wrote:
> > > > >
> > > > > On Tue, May 07, 2024 at 02:53:17PM +0800, Xingyu Wu wrote:
> > > > > > This patch is to add the notifier for PLL0 clock and set the
> > > > > > PLL0 rate to 1.5GHz to fix the lower rate of CPUfreq on the JH7110 SoC.
> > > > > >
> > > > > > The first patch is to add the notifier for PLL0 clock. Setting
> > > > > > the
> > > > > > PLL0 rate need the son clock (cpu_root) to switch its parent
> > > > > > clock to OSC clock and switch it back after setting PLL0 rate.
> > > > > > It need to use the cpu_root clock from SYSCRG and register the
> > > > > > notifier in the SYSCRG driver.
> > > > > >
> > > > > > The second patch is to set cpu_core rate to 500MHz and PLL0
> > > > > > rate to 1.5GHz to fix the problem about the lower rate of
> > > > > > CPUfreq on the visionfive board. The cpu_core clock rate is
> > > > > > set to 500MHz first to ensure that the cpu frequency will not
> > > > > > suddenly become high and the cpu voltage is not enough to
> > > > > > cause a crash when the PLL0 is set
> > > to 1.5GHz.
> > > > > > The cpu voltage and frequency are then adjusted together by CPUfreq.
> > > > >
> > > > > Hmm, how does sequencing work here? If we split the patches
> > > > > between trees it sounds like without the dts patch, the clock
> > > > > tree would (or
> > > > > could) crash, or mainline if the clock changes there before the
> > > > > dts ones do. Am I misunderstanding that?
> > > >
> > > > Oh, I think you misunderstood it. Patch 1 (clock driver patch)
> > > > does not cause the clock tree crash without the patch 2 (dts
> > > > patch), and it just provides the correct flow of how to change the
> > > > PLL0 rate. The patch 2 is to set the clock rate of cpu_core and
> > > > PLL0 rate, which causes the crash without patch 1. Setting
> > > > cpu_core rate is to avoid crashes by
> > > insufficient cpu voltage when setting PLL0 rate.
> > >
> > > So is the problem in the other direction then? My dts tree will
> > > crash if I apply the dts change without the clock patch?
> >
> > Sorry, I tested it and it could not crash using only dts patch. It can
> > separate the patches and use it individually.
> >
> > > Additionally, what about U-Boot? Will it have problems if the dts is
> > > imported there without changes to its clock driver?
> > >
> >
> > It is not apply to U-Boot. In the U-Boot, the PLL0 rate should be 1GHz
> > to for GMAC and PMIC to work. But now the PLL0 rate should be 1.5GHz in the
> Linux.
> 
> There's a push in U-Boot to move devicestrees to use "OF_UPSTREAM", which
> means importing devicetrees directly from Linux and using them in U-Boot. I
> don't really want to merge a patch that would present U-Boot with a problem if
> the VisionFive 2 moved to that model there.
> 
> Cheers,
> Conor.

Would it be better  if I  change the rates of PLL0 and CPU core in the driver not dts,
and avoid the dts of Linux and U-Boot being different?

Thanks,
Xingyu Wu 

Re: [PATCH v5 0/2] Add notifier for PLL0 clock and set it 1.5GHz on

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 670 bytes --]

On Wed, May 15, 2024 at 02:23:47AM +0000, Xingyu Wu wrote:
> On 15/05/2024 02:08, Conor Dooley wrote:

> > There's a push in U-Boot to move devicestrees to use "OF_UPSTREAM", which
> > means importing devicetrees directly from Linux and using them in U-Boot. I
> > don't really want to merge a patch that would present U-Boot with a problem if
> > the VisionFive 2 moved to that model there.

> Would it be better  if I  change the rates of PLL0 and CPU core in the driver not dts,
> and avoid the dts of Linux and U-Boot being different?

I'd definitely prefer if we don't include stuff in the kernel tree that
would cause problems for U-Boot if imported there, yeah.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v5 0/2] Add notifier for PLL0 clock and set it 1.5GHz on

[David Abdurachmanov]

On Wed, May 15, 2024 at 7:31 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Wed, May 15, 2024 at 02:23:47AM +0000, Xingyu Wu wrote:
> > On 15/05/2024 02:08, Conor Dooley wrote:
>
> > > There's a push in U-Boot to move devicestrees to use "OF_UPSTREAM", which
> > > means importing devicetrees directly from Linux and using them in U-Boot. I
> > > don't really want to merge a patch that would present U-Boot with a problem if
> > > the VisionFive 2 moved to that model there.
>
> > Would it be better  if I  change the rates of PLL0 and CPU core in the driver not dts,
> > and avoid the dts of Linux and U-Boot being different?
>
> I'd definitely prefer if we don't include stuff in the kernel tree that
> would cause problems for U-Boot if imported there, yeah.
>

What is the current state of this patchset?

I noticed this patchset on the U-Boot side from Hal Feng:
[PATCH v1 0/4] Sync StarFive JH7110 clock and reset dt-bindings with Linux

It seems to indicate that there is WIP for OF_UPSTREAM support.

Cheers,
david

Re: [PATCH v5 0/2] Add notifier for PLL0 clock and set it 1.5GHz on

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1628 bytes --]

On Tue, Jun 04, 2024 at 12:45:48PM +0300, David Abdurachmanov wrote:
> On Wed, May 15, 2024 at 7:31 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Wed, May 15, 2024 at 02:23:47AM +0000, Xingyu Wu wrote:
> > > On 15/05/2024 02:08, Conor Dooley wrote:
> >
> > > > There's a push in U-Boot to move devicestrees to use "OF_UPSTREAM", which
> > > > means importing devicetrees directly from Linux and using them in U-Boot. I
> > > > don't really want to merge a patch that would present U-Boot with a problem if
> > > > the VisionFive 2 moved to that model there.
> >
> > > Would it be better  if I  change the rates of PLL0 and CPU core in the driver not dts,
> > > and avoid the dts of Linux and U-Boot being different?
> >
> > I'd definitely prefer if we don't include stuff in the kernel tree that
> > would cause problems for U-Boot if imported there, yeah.
> >
> 
> What is the current state of this patchset?

v6: https://lore.kernel.org/all/20240603020607.25122-1-xingyu.wu@starfivetech.com/

Guess it didn't go to the riscv ml because the dts patch was dropped.

> I noticed this patchset on the U-Boot side from Hal Feng:
> [PATCH v1 0/4] Sync StarFive JH7110 clock and reset dt-bindings with Linux
> 
> It seems to indicate that there is WIP for OF_UPSTREAM support.

And as a commenter on that patchset you reference said, they should
actually use OF_UPSTREAM directly rather than manual syncs like that
series. I'm not sure how the U-Boot folks want to address that w.r.t.
bisection though, in cases like this where the defines do not have
identical names.

Thanks,
Conor.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]