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/[email protected]/
Changes since v3:
- Added the notifier for PLL0 clock.
- Set cpu_core rate in DTS
v3: https://lore.kernel.org/all/[email protected]/
Changes since v2:
- Made the steps into the process into the process of setting PLL0 rate
v2: https://lore.kernel.org/all/[email protected]/
Changes since v1:
- Added the fixes tag in the commit.
v1: https://lore.kernel.org/all/[email protected]/
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
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 <[email protected]>
---
.../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
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 <[email protected]>
---
.../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
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 <[email protected]>
This looks much better, thanks!
Reviewed-by: Emil Renner Berthing <[email protected]>
> ---
> .../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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
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 <[email protected]>
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 <[email protected]>
> ---
> .../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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
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?
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
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.
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 <[email protected]>
> ---
> .../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 {
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 <[email protected]>
> > ---
> > .../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 {
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
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.
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
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.
On Wed, May 15, 2024 at 7:31 PM Conor Dooley <[email protected]> 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
On Tue, Jun 04, 2024 at 12:45:48PM +0300, David Abdurachmanov wrote:
> On Wed, May 15, 2024 at 7:31 PM Conor Dooley <[email protected]> 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/[email protected]/
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.