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 <[email protected]>
Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for JH7110 SoC")
Signed-off-by: Xingyu Wu <[email protected]>
---
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/[email protected]/
v1: https://lore.kernel.org/all/[email protected]/
Thanks,
Xingyu Wu
---
.../jh7110-starfive-visionfive-2.dtsi | 5 +
.../clk/starfive/clk-starfive-jh7110-pll.c | 102 ++++++++++++++++++
2 files changed, 107 insertions(+)
diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
index 45b58b6f3df8..0c57d833fb29 100644
--- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
@@ -336,6 +336,11 @@ &pwmdac {
status = "okay";
};
+&pllclk {
+ assigned-clocks = <&pllclk JH7110_PLLCLK_PLL0_OUT>;
+ assigned-clock-rates = <1500000000>;
+};
+
&qspi {
#address-cells = <1>;
#size-cells = <0>;
diff --git a/drivers/clk/starfive/clk-starfive-jh7110-pll.c b/drivers/clk/starfive/clk-starfive-jh7110-pll.c
index 3598390e8fd0..7a53ded8d526 100644
--- a/drivers/clk/starfive/clk-starfive-jh7110-pll.c
+++ b/drivers/clk/starfive/clk-starfive-jh7110-pll.c
@@ -24,11 +24,14 @@
#include <linux/device.h>
#include <linux/kernel.h>
#include <linux/mfd/syscon.h>
+#include <linux/of_address.h>
#include <linux/platform_device.h>
#include <linux/regmap.h>
#include <dt-bindings/clock/starfive,jh7110-crg.h>
+#include "clk-starfive-jh7110.h"
+
/* this driver expects a 24MHz input frequency from the oscillator */
#define JH7110_PLL_OSC_RATE 24000000UL
@@ -72,6 +75,9 @@
#define JH7110_PLL_PREDIV_SHIFT 0
#define JH7110_PLL_PREDIV_MASK GENMASK(5, 0)
+#define JH7110_CPU_ROOT_MUX_OSC 0
+#define JH7110_CPU_ROOT_MUX_PLL0 1
+
enum jh7110_pll_mode {
JH7110_PLL_MODE_FRACTION,
JH7110_PLL_MODE_INTEGER,
@@ -140,6 +146,8 @@ struct jh7110_pll_data {
struct jh7110_pll_priv {
struct device *dev;
struct regmap *regmap;
+ void __iomem *syscrg_base;
+ bool is_first_set;
struct jh7110_pll_data pll[JH7110_PLLCLK_END];
};
@@ -275,6 +283,25 @@ static struct jh7110_pll_priv *jh7110_pll_priv_from(struct jh7110_pll_data *pll)
return container_of(pll, struct jh7110_pll_priv, pll[pll->idx]);
}
+static void jh7110_pll_syscrg_update_div(void __iomem *base,
+ unsigned int id,
+ unsigned int div)
+{
+ unsigned int reg = readl(base + id * 4);
+
+ writel((reg & ~JH71X0_CLK_DIV_MASK) | div, (base + id * 4));
+}
+
+static void jh7110_pll_syscrg_update_mux(void __iomem *base,
+ unsigned int id,
+ unsigned int mux)
+{
+ unsigned int reg = readl(base + id * 4);
+
+ writel((reg & ~JH71X0_CLK_MUX_MASK) | (mux << JH71X0_CLK_MUX_SHIFT),
+ (base + id * 4));
+}
+
static void jh7110_pll_regvals_get(struct regmap *regmap,
const struct jh7110_pll_info *info,
struct jh7110_pll_regvals *ret)
@@ -352,6 +379,47 @@ static int jh7110_pll_determine_rate(struct clk_hw *hw, struct clk_rate_request
return 0;
}
+static bool jh7110_pll0_is_assigned_clock(struct device_node *node)
+{
+ struct of_phandle_args clkspec;
+ int ret;
+
+ ret = of_parse_phandle_with_args(node, "assigned-clocks",
+ "#clock-cells", 0, &clkspec);
+ if (ret < 0 || clkspec.np != node)
+ return false;
+
+ if (clkspec.args[0] == JH7110_PLLCLK_PLL0_OUT)
+ return true;
+
+ return false;
+}
+
+/*
+ * 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.
+ *
+ * If cpu rate rather than 1.25GHz, PMIC need to be set higher voltage.
+ * But the PMIC is controlled by CPUfreq and I2C, which boot later than
+ * PLL driver when using assigned_clock to set PLL0 rate. So set the
+ * CPU_CORE divider to 2(default 1) first and make sure the cpu rate is
+ * lower than 1.25G when pll0 rate will be set more than 1.25G.
+ */
+static void jh7110_pll0_rate_preset(struct jh7110_pll_priv *priv,
+ unsigned long rate)
+{
+ if (rate > 1250000000 && priv->is_first_set &&
+ jh7110_pll0_is_assigned_clock(priv->dev->of_node))
+ jh7110_pll_syscrg_update_div(priv->syscrg_base,
+ JH7110_SYSCLK_CPU_CORE, 2);
+
+ jh7110_pll_syscrg_update_mux(priv->syscrg_base,
+ JH7110_SYSCLK_CPU_ROOT,
+ JH7110_CPU_ROOT_MUX_OSC);
+ priv->is_first_set = false;
+}
+
static int jh7110_pll_set_rate(struct clk_hw *hw, unsigned long rate,
unsigned long parent_rate)
{
@@ -372,6 +440,9 @@ static int jh7110_pll_set_rate(struct clk_hw *hw, unsigned long rate,
return -EINVAL;
found:
+ if (pll->idx == JH7110_PLLCLK_PLL0_OUT)
+ jh7110_pll0_rate_preset(priv, rate);
+
if (val->mode == JH7110_PLL_MODE_FRACTION)
regmap_update_bits(priv->regmap, info->offsets.frac, JH7110_PLL_FRAC_MASK,
val->frac << JH7110_PLL_FRAC_SHIFT);
@@ -387,6 +458,12 @@ static int jh7110_pll_set_rate(struct clk_hw *hw, unsigned long rate,
regmap_update_bits(priv->regmap, info->offsets.frac, JH7110_PLL_POSTDIV1_MASK,
(u32)val->postdiv1 << JH7110_PLL_POSTDIV1_SHIFT);
+ /* Set parent of cpu_root back to PLL0 */
+ if (pll->idx == JH7110_PLLCLK_PLL0_OUT)
+ jh7110_pll_syscrg_update_mux(priv->syscrg_base,
+ JH7110_SYSCLK_CPU_ROOT,
+ JH7110_CPU_ROOT_MUX_PLL0);
+
return 0;
}
@@ -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");
+ 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;
+
+np_put:
+ of_node_put(np);
+ if (ret)
+ return ret;
+
return devm_of_clk_add_hw_provider(&pdev->dev, jh7110_pll_get, priv);
}
--
2.25.1
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 <[email protected]>
> Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for JH7110 SoC")
> Signed-off-by: Xingyu Wu <[email protected]>
> ---
>
> 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/[email protected]/
> v1: https://lore.kernel.org/all/[email protected]/
>
> 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
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 <[email protected]>
>> Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for JH7110 SoC")
>> Signed-off-by: Xingyu Wu <[email protected]>
>> ---
>>
>> 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/[email protected]/
>> v1: https://lore.kernel.org/all/[email protected]/
>>
>> 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
> [email protected]
> 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
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 <[email protected]>
> > Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for JH7110
> > SoC")
> > Signed-off-by: Xingyu Wu <[email protected]>
> > ---
> >
> > 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
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 <[email protected]>
>>> Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for JH7110
>>> SoC")
>>> Signed-off-by: Xingyu Wu <[email protected]>
>>> ---
>>>
>>> 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.
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
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 <[email protected]>
> >>> Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for
> >>> JH7110
> >>> SoC")
> >>> Signed-off-by: Xingyu Wu <[email protected]>
> >>> ---
> >>>
> >>> 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/[email protected]/
Thanks,
Xingyu Wu
> -----Original Message-----
> From: Bo Gan <[email protected]>
> Sent: 2024年4月3日 8:27
> To: Krzysztof Kozlowski <[email protected]>; Xingyu Wu
> <[email protected]>; Michael Turquette
> <[email protected]>; Stephen Boyd <[email protected]>; Conor Dooley
> <[email protected]>; Emil Renner Berthing
> <[email protected]>; Rob Herring <[email protected]>;
> Krzysztof Kozlowski <[email protected]>
> Cc: Paul Walmsley <[email protected]>; Palmer Dabbelt
> <[email protected]>; Albert Ou <[email protected]>; Hal Feng
> <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; [email protected]
> 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 <[email protected]>
> >> Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for
> >> JH7110 SoC")
> >> Signed-off-by: Xingyu Wu <[email protected]>
> >> ---
> >>
> >> 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
> > [email protected]
> > 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@mail.gmail.com/
Best regards,
Xingyu Wu
Hi Xingyu,
On 2024-04-03 2:44 AM, Xingyu Wu wrote:
> 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 <[email protected]>
>>>>> Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for
>>>>> JH7110
>>>>> SoC")
>>>>> Signed-off-by: Xingyu Wu <[email protected]>
>>>>> ---
>>>>>
>>>>> 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/[email protected]/
Both reparenting cpu_root and enforcing the maximum cpu_core frequency can be
accomplished with clk notifiers. See for example ccu_mux_notifier_register() in
drivers/clk/sunxi-ng/ccu_mux.c.
Regards,
Samuel
On 2024-04-05 5:28 AM, Samuel Holland wrote:
>
> Hi Xingyu,
>
> On 2024-04-03 2:44 AM, Xingyu Wu wrote:
> > 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 <[email protected]>
> >>>>> Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for
> >>>>> JH7110
> >>>>> SoC")
> >>>>> Signed-off-by: Xingyu Wu <[email protected]>
> >>>>> ---
> >>>>>
> >>>>> 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@star
> >>>>> fi
> >>>>> ve
> >>>>> tech.com/
> >>>>> v1:
> >>>>> https://lore.kernel.org/all/20230811033631.160912-1-xingyu.wu@star
> >>>>> fi
> >>>>> 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@starfive
> > tech.com/
>
> Both reparenting cpu_root and enforcing the maximum cpu_core frequency can
> be accomplished with clk notifiers. See for example ccu_mux_notifier_register()
> in drivers/clk/sunxi-ng/ccu_mux.c.
>
This seems like a good idea. I'll try it.
Thanks,
Xingyu Wu