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

Commit Message

Xingyu Wu April 2, 2024, 9:09 a.m. UTC

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 <[e-mail]>
Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for JH7110 SoC")
Signed-off-by: Xingyu Wu <[e-mail]>
---

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/[e-mail]/
v1: https://lore.kernel.org/all/[e-mail]/

Thanks,
Xingyu Wu
---
 .../jh7110-starfive-visionfive-2.dtsi         |   5 +
 .../clk/starfive/clk-starfive-jh7110-pll.c    | 102 ++++++++++++++++++
 2 files changed, 107 insertions(+)

Comments

Krzysztof Kozlowski April 2, 2024, 4:18 p.m. UTC | #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 <[e-mail]>
> Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for JH7110 SoC")
> Signed-off-by: Xingyu Wu <[e-mail]>
> ---
> 
> 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/[e-mail]/
> v1: https://lore.kernel.org/all/[e-mail]/
> 
> 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

Bo Gan April 3, 2024, 12:26 a.m. UTC | #2

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 <[e-mail]>
>> Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for JH7110 SoC")
>> Signed-off-by: Xingyu Wu <[e-mail]>
>> ---
>>
>> 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/[e-mail]/
>> v1: https://lore.kernel.org/all/[e-mail]/
>>
>> 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
> [e-mail]
> 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

Xingyu Wu April 3, 2024, 7:19 a.m. UTC | #3

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 <[e-mail]>
> > Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for JH7110
> > SoC")
> > Signed-off-by: Xingyu Wu <[e-mail]>
> > ---
> >
> > 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

Krzysztof Kozlowski April 3, 2024, 7:23 a.m. UTC | #4

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 <[e-mail]>
>>> Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for JH7110
>>> SoC")
>>> Signed-off-by: Xingyu Wu <[e-mail]>
>>> ---
>>>
>>> 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

Xingyu Wu April 3, 2024, 7:24 a.m. UTC | #5

> -----Original Message-----
> From: Bo Gan <[e-mail]>
> Sent: 2024年4月3日 8:27
> To: Krzysztof Kozlowski <[e-mail]>; Xingyu Wu
> <[e-mail]>; Michael Turquette
> <[e-mail]>; Stephen Boyd <[e-mail]>; Conor Dooley
> <[e-mail]>; Emil Renner Berthing
> <[e-mail]>; Rob Herring <[e-mail]>;
> Krzysztof Kozlowski <[e-mail]>
> Cc: Paul Walmsley <[e-mail]>; Palmer Dabbelt
> <[e-mail]>; Albert Ou <[e-mail]>; Hal Feng
> <[e-mail]>; [e-mail]; linux-
> [e-mail]; [e-mail]; [e-mail]
> 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 <[e-mail]>
> >> Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for
> >> JH7110 SoC")
> >> Signed-off-by: Xingyu Wu <[e-mail]>
> >> ---
> >>
> >> 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
> > [e-mail]
> > 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=[e-mail]/

Best regards,
Xingyu Wu

Xingyu Wu April 3, 2024, 7:44 a.m. UTC | #6

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 <[e-mail]>
> >>> Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for
> >>> JH7110
> >>> SoC")
> >>> Signed-off-by: Xingyu Wu <[e-mail]>
> >>> ---
> >>>
> >>> 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/[e-mail]/

Thanks,
Xingyu Wu

Samuel Holland April 4, 2024, 9:27 p.m. UTC | #7

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 <[e-mail]>
>>>>> Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for
>>>>> JH7110
>>>>> SoC")
>>>>> Signed-off-by: Xingyu Wu <[e-mail]>
>>>>> ---
>>>>>
>>>>> 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/[e-mail]/

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

Xingyu Wu April 7, 2024, 1:14 p.m. UTC | #8

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 <[e-mail]>
> >>>>> Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for
> >>>>> JH7110
> >>>>> SoC")
> >>>>> Signed-off-by: Xingyu Wu <[e-mail]>
> >>>>> ---
> >>>>>
> >>>>> 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

Patch

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);
 }