Re: [PATCH v2] clk: starfive: jh7110-sys: Fix lower rate of CPUfreq by setting PLL0 rate to 1.5GHz

[Xingyu Wu]

On 2023/8/22 16:56, Emil Renner Berthing wrote:
> On Tue, 22 Aug 2023 at 05:33, Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx> wrote:
>> On 2023/8/21 23:38, Emil Renner Berthing wrote:
>> > On Mon, 21 Aug 2023 at 17:29, Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx> 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 set to 1.5GHz. Change the parent of cpu_root clock
>> >> and the divider of cpu_core before the setting.
>> >>
>> >> 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]
>> >> and sets PLL0 rate to 1.5GHz. In order not to affect the cpu
>> >> operation, 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.
>> >
>> > Hi Xingyu,
>> >
>> > It sounds like this is something the driver should handle
>> > automatically whenever clk_set_rate() is called on the PLL0 clock.
>> > Then we should be able to use regular assigned-clocks /
>> > assigned-clock-rates stanzas in the device tree instead of having this
>> > 1.5GHz rate hard-coded in the driver.
>> >
>> > /Emil
>>
>> Hi Emil,
>>
>> The frequency of PLL0 is set according to this process to avoid crash:
>> 1. The divider of the cpu_core clock should be set to 2 if PLL0 is set to 1.5GHz.
>> 2. The parent of cpu_root is changed from pll0 to osc.
>> 3. The PLL0 is set to 1.5GHz.
>> 4. The parent of cpu_root is changed from osc to pll0 back.
>> I don't think assigned-clock-rates/assigned-clock-parents can do such a complicated job.
> 
> Right, but what I'm saying is that if calling clk_set_rate() on the
> PLL0 clock causes a crash, that sounds like a bug in the driver. If
> you fix that bug, and make clk_set_rate() on the PLL0 clock do the
> procedure above when changing the rate, then the PLL0 clock can work
> just like any other clock and assigned-clock-rates would work.
> 
> /Emil

Yeah, if fix this bug, I should add there steps when setting rate in the PLL clock driver. But how to get and use the clocks of cpu_root and cpu_core in the PLL driver? It seem to be only two ways, clk_get() or ioremap(). If use clk_get(), the pll node should add syscrg clocks. It shows that the PLL driver depends on the SYSCRG driver, which seems confusing because normally the syscrg depends on the pll. Do you have a better suggestion?

Thanks.
Xingyu Wu

> 
>> >
>> >> [1]: https://github.com/starfive-tech/VisionFive2/issues/55
>> >>
>> >> This patch is based on linux-next(20230818) which has merge PLL driver
>> >> on the StarFive JH7110 SoC.
>> >>
>> >> Thanks,
>> >> Xingyu Wu
>> >>
>> >> ---
>> >>  .../clk/starfive/clk-starfive-jh7110-sys.c    | 47 ++++++++++++++++++-
>> >>  1 file changed, 46 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/clk/starfive/clk-starfive-jh7110-sys.c b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
>> >> index 3884eff9fe93..b6b9e967dfc7 100644
>> >> --- a/drivers/clk/starfive/clk-starfive-jh7110-sys.c
>> >> +++ b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
>> >> @@ -501,7 +501,52 @@ 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 PLL0 rate to 1.5GHz
>> >> +        * In order to not affect the cpu when the PLL0 rate is changing,
>> >> +        * we need to switch the parent of cpu_root clock to osc clock first,
>> >> +        * and then switch back after setting the PLL0 rate.
>> >> +        */
>> >> +       pllclk = clk_get(priv->dev, "pll0_out");
>> >> +       if (!IS_ERR(pllclk)) {
>> >> +               struct clk *osc = clk_get(&pdev->dev, "osc");
>> >> +               struct clk *cpu_root = priv->reg[JH7110_SYSCLK_CPU_ROOT].hw.clk;
>> >> +               struct clk *cpu_core = priv->reg[JH7110_SYSCLK_CPU_CORE].hw.clk;
>> >> +
>> >> +               if (IS_ERR(osc)) {
>> >> +                       clk_put(pllclk);
>> >> +                       return PTR_ERR(osc);
>> >> +               }
>> >> +
>> >> +               /*
>> >> +                * CPU need voltage regulation by CPUfreq if set 1.5GHz.
>> >> +                * So in this driver, cpu_core need to be set the divider to be 2 first
>> >> +                * and will be 750M after setting parent.
>> >> +                */
>> >> +               ret = clk_set_rate(cpu_core, clk_get_rate(cpu_core) / 2);
>> >> +               if (ret)
>> >> +                       goto failed_set;
>> >> +
>> >> +               ret = clk_set_parent(cpu_root, osc);
>> >> +               if (ret)
>> >> +                       goto failed_set;
>> >> +
>> >> +               ret = clk_set_rate(pllclk, 1500000000);
>> >> +               if (ret)
>> >> +                       goto failed_set;
>> >> +
>> >> +               ret = clk_set_parent(cpu_root, pllclk);
>> >> +
>> >> +failed_set:
>> >> +               clk_put(pllclk);
>> >> +               clk_put(osc);
>> >> +       }
>> >> +
>> >> +       return ret;
>> >>  }
>> >>
>> >>  static const struct of_device_id jh7110_syscrg_match[] = {
>> >> --
>> >> 2.25.1
>> >>
>>