| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<ZQ2PR01MB13073EF3BD7A64F2C098AA8FE6882@ZQ2PR01MB1307.CHNPR01.prod.partner.outlook.cn>
Date: Fri, 23 Aug 2024 03:41:40 +0000
From: Hal Feng <hal.feng@...rfivetech.com>
To: Conor Dooley <conor@...nel.org>, Xingyu Wu <xingyu.wu@...rfivetech.com>
CC: Emil Renner Berthing <emil.renner.berthing@...onical.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>, Michael Turquette
<mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>
Subject: RE: [PATCH v6] clk: starfive: jh7110-sys: Fix lower rate of CPUfreq
by setting PLL0 rate to 1.5GHz
> On 18.08.24 21:44, Conor Dooley wrote:
> On Thu, Aug 08, 2024 at 03:44:49AM +0000, Xingyu Wu wrote:
> > On 06/08/2024 17:58, Emil Renner Berthing wrote:
> > >
> > > 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
> > > > 250/333/500/1000MHz in fact. The PLL0 rate should be default set
> > > > to 1.5GHz and set the divider of cpu_core clock to 2 in safe.
> > > >
> > > > To keeo the cpu frequency stable when setting PLL0, the parent
> > > > clock of the cpu_root clock needs to be switched from PLL0 to
> > > > another parent clock and add notifier function to do this 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.
> > > >
> > > > To keep the DTS same in Linux and U-Boot and the PLL0 rate is 1GHz
> > > > in U-Boot, the PLL0 rate should be set to 1.5GHz in the driver
> > > > instead of DTS.
> > > >
> > > > Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for
> > > > JH7110
> > > > SoC")
> > > > Signed-off-by: Xingyu Wu <xingyu.wu@...rfivetech.com>
> > > > ---
> > > >
> > > > Hi Stephen and Emil,
> > > >
> > > > This patch is to fix the lower rate of CPUfreq by adding the
> > > > notifier for PLL0 clock and changing the PLL0 rate to 1.5GHz.
> > > >
> > > > To keep the DTS same in Linux and U-Boot as Conor wants[1] and the
> > > > PLL0 rate is 1GHz in U-Boot, the PLL0 rate should be set to 1.5GHz
> > > > in the driver instead of DTS.
> > > >
> > > > [1]:
> > > > https://lore.kernel.org/all/20240515-reorder-even-8b9eebd91b45@spu
> > > > d/
> > > >
> > > > Thanks,
> > > > Xingyu Wu
> > > >
> > > > ---
> > > > .../clk/starfive/clk-starfive-jh7110-sys.c | 54 ++++++++++++++++++-
> > > > drivers/clk/starfive/clk-starfive-jh71x0.h | 2 +
> > > > 2 files changed, 54 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/starfive/clk-starfive-jh7110-sys.c
> > > > b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
> > > > index 8f5e5abfa178..7469981fb405 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,11 @@ 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;
> > > > }
> > > >
> > > > @@ -501,7 +531,27 @@ static int __init jh7110_syscrg_probe(struct
> > > platform_device *pdev)
> > > > if (ret)
> > > > return ret;
> > > >
> > > > - return jh7110_reset_controller_register(priv, "rst-sys", 0);
> > > > + ret = jh7110_reset_controller_register(priv, "rst-sys", 0);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + /* Set the divider cpu_core to 2 and set the PLL0 rate to 1.5G. */
> > > > + pllclk = clk_get(priv->dev, "pll0_out");
> > > > + if (!IS_ERR(pllclk)) {
> > > > + struct clk *cpu_core = priv-
> > > >reg[JH7110_SYSCLK_CPU_CORE].hw.clk;
> > > > +
> > > > + ret = clk_set_rate(cpu_core, clk_get_rate(cpu_core) / 2);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = clk_set_rate(pllclk, 1500000000);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + clk_put(pllclk);
> > > > + }
> > > > +
> > > > + return 0;
> > >
> > > I'm still not a fan of hardcoding cpu frequencies in the driver.
> > > You've added the notifiers exactly so that we can use the standard device
> tree settings for this.
> > >
> > > In other words I much prefer v5 of this patchset.
> > >
> > > /Emil
> > >
> >
> > Thanks, Emil.
> >
> > Hi Conor, what do you think about this issue?
>
> Apologies for the delay replying, I didn't realise there was a question here
> directed at me. My only real thought on the patchset is that what is done
> should not cause problems when the same devicetree is used for both U-Boot
> and for the kernel. As long as that's satisfied, I don't mind how you choose to
> implement it.
Actually VF2 U-Boot can run at 1.5GHz. It will work if the PMIC sets the CPU power
supply voltage to 1.04V. The reason why we run VF2 U-Boot at 1.0GHz is that the
default voltage supplied by the PMIC is 0.9V which only supports JH7110 cores
running at 1.0GHz.
So v5 of this patchset won't cause problems if the VF2 U-Boot makes some changes
to support running at 1.5GHz. And I will make these changes when I implement
OF_UPSTREAM for VF2 U-Boot.
BTW, if v5 is applied, the patch 2 of v5 should be rebased on the new mainline since
jh7110-common.dtsi has been created.
Best regards,
Hal
Powered by blists - more mailing lists