> -----Original Message----- > From: Bo Gan <ganboing@xxxxxxxxx> > Sent: 2024年4月3日 8:27 > To: Krzysztof Kozlowski <krzk@xxxxxxxxxx>; Xingyu Wu > <xingyu.wu@xxxxxxxxxxxxxxxx>; Michael Turquette > <mturquette@xxxxxxxxxxxx>; Stephen Boyd <sboyd@xxxxxxxxxx>; Conor Dooley > <conor@xxxxxxxxxx>; Emil Renner Berthing > <emil.renner.berthing@xxxxxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; > Krzysztof Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx> > Cc: Paul Walmsley <paul.walmsley@xxxxxxxxxx>; Palmer Dabbelt > <palmer@xxxxxxxxxxx>; Albert Ou <aou@xxxxxxxxxxxxxxxxx>; Hal Feng > <hal.feng@xxxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; linux- > clk@xxxxxxxxxxxxxxx; linux-riscv@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v3] clk: starfive: pll: Fix lower rate of CPUfreq by setting PLL0 > rate to 1.5GHz > > On 4/2/24 9:18 AM, 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@starfiv > >> etech.com/ > >> v1: > >> https://lore.kernel.org/all/20230811033631.160912-1-xingyu.wu@starfiv > >> etech.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. > > > > ... > > > >> > >> @@ -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? > > > > > > > > Best regards, > > Krzysztof > > > > > > _______________________________________________ > > linux-riscv mailing list > > linux-riscv@xxxxxxxxxxxxxxxxxxx > > http://lists.infradead.org/mailman/listinfo/linux-riscv > > > > Hi Xingyu, > > Echoing Krzysztof's point. This piece code seems wrong to me. This logic belongs > to syscrg, rather than pll. Why don't you do the pll0->osc->pll0 switching from > syscrg side during probing? > > Bo Yes, That's what I thought and I did it in previous patches. But Emil seemed to like to put the steps into the clk_set_rate() when setting PLL0 rate[1]. So I tried to use this way in this patch. [1]: https://lore.kernel.org/all/CAJM55Z-gYpn_FjG2Zb__Nt=rbrNQN8QDNB=KEFdeVkz9Ptv72Q@xxxxxxxxxxxxxx/ Best regards, Xingyu Wu