RE: [PATCH v3] clk: starfive: pll: Fix lower rate of CPUfreq by setting PLL0 rate to 1.5GHz

[Xingyu Wu]

On 03/04/2024 0:18, Krzysztof Kozlowski wrote:
> 
> On 02/04/2024 11:09, 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.
> >
> > So PLL0 rate should be default set to 1.5GHz. But setting the
> > PLL0 rate need certain steps:
> >
> > 1. Change the parent of cpu_root clock to OSC clock.
> > 2. Change the divider of cpu_core if PLL0 rate is higher than
> >    1.25GHz before CPUfreq boot.
> > 3. Change the parent of cpu_root clock back to PLL0 clock.
> >
> > Reviewed-by: Hal Feng <hal.feng@xxxxxxxxxxxxxxxx>
> > Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for JH7110
> > SoC")
> > Signed-off-by: Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx>
> > ---
> >
> > Hi Stephen and Emil,
> >
> > This patch fixes the issue about lower rate of CPUfreq[1] by setting
> > PLL0 rate to 1.5GHz.
> >
> > In order not to affect the cpu operation, setting the PLL0 rate need
> > certain steps. The cpu_root's parent clock should be changed first.
> > And the divider of the cpu_core clock should be set to 2 so they won't
> > crash when setting 1.5GHz without voltage regulation. Due to PLL
> > driver boot earlier than SYSCRG driver, cpu_core and cpu_root clocks
> > are using by ioremap().
> >
> > [1]: https://github.com/starfive-tech/VisionFive2/issues/55
> >
> > Previous patch link:
> > v2:
> > https://lore.kernel.org/all/20230821152915.208366-1-xingyu.wu@starfive
> > tech.com/
> > v1:
> > https://lore.kernel.org/all/20230811033631.160912-1-xingyu.wu@starfive
> > tech.com/
> >
> > Thanks,
> > Xingyu Wu
> > ---
> >  .../jh7110-starfive-visionfive-2.dtsi         |   5 +
> >  .../clk/starfive/clk-starfive-jh7110-pll.c    | 102 ++++++++++++++++++
> 
> Please do not mix DTS and driver code. That's not really portable. DTS is being
> exported and used in other projects.

OK, I will submit that in two patches.

> 
> ...
> 
> >
> > @@ -458,6 +535,8 @@ static int jh7110_pll_probe(struct platform_device
> *pdev)
> >  	struct jh7110_pll_priv *priv;
> >  	unsigned int idx;
> >  	int ret;
> > +	struct device_node *np;
> > +	struct resource res;
> >
> >  	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> >  	if (!priv)
> > @@ -489,6 +568,29 @@ static int jh7110_pll_probe(struct platform_device
> *pdev)
> >  			return ret;
> >  	}
> >
> > +	priv->is_first_set = true;
> > +	np = of_find_compatible_node(NULL, NULL, "starfive,jh7110-syscrg");
> 
> Your drivers should not do it. It's fragile, hides true link/dependency.
> Please use phandles.
> 
> 
> > +	if (!np) {
> > +		ret = PTR_ERR(np);
> > +		dev_err(priv->dev, "failed to get syscrg node\n");
> > +		goto np_put;
> > +	}
> > +
> > +	ret = of_address_to_resource(np, 0, &res);
> > +	if (ret) {
> > +		dev_err(priv->dev, "failed to get syscrg resource\n");
> > +		goto np_put;
> > +	}
> > +
> > +	priv->syscrg_base = ioremap(res.start, resource_size(&res));
> > +	if (!priv->syscrg_base)
> > +		ret = -ENOMEM;
> 
> Why are you mapping other device's IO? How are you going to ensure synced
> access to registers?

Because setting PLL0 rate need specific steps and use the clocks of SYSCRG.
But SYSCRG driver also need PLL clock to be clock source when adding clock
providers. I tried to add SYSCRG clocks in 'clocks' property in DT and use
clk_get() to get the clocks. But it could not run and crash. So I use ioremap()
instead.

Best regards,
Xingyu Wu