lkml.org 
[lkml]   [2024]   [Apr]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH v3] clk: starfive: pll: Fix lower rate of CPUfreq by setting PLL0 rate to 1.5GHz
    Date
    On 03/04/2024 15:24, Krzysztof Kozlowski wrote:
    >
    > On 03/04/2024 09:19, Xingyu Wu wrote:
    > > 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@starfivetech.com>
    > >>> Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for
    > >>> JH7110
    > >>> SoC")
    > >>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
    > >>> ---
    > >>>
    > >>> 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@starfi
    > >>> ve
    > >>> tech.com/
    > >>> v1:
    > >>> https://lore.kernel.org/all/20230811033631.160912-1-xingyu.wu@starfi
    > >>> ve
    > >>> 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.
    >
    > That's not a reason to map other device's IO. That could be a reason for having
    > syscon or some other sort of relationship, like clock or reset.
    >
    > > 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.
    >
    > So instead of properly model the relationship, you entangle the drivers even
    > more.
    >
    > Please come with a proper design for this. I have no clue about your hardware,
    > but that looks like you are asynchronously configuring the same hardware in two
    > different places.
    >
    > Sorry, that's poor code.
    >
    > Best regards,
    > Krzysztof

    Hi Krzysztof,

    If I use the old patch[1] like v2 and set the PLL0 default rate in the SYSCRG driver,
    will it be better?

    [1]: https://lore.kernel.org/all/20230821152915.208366-1-xingyu.wu@starfivetech.com/

    Thanks,
    Xingyu Wu

    \
     
     \ /
      Last update: 2024-05-27 16:20    [W:4.088 / U:0.736 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site