Re: [PATCH v2 2/2] arm64: dts: r8a7796: Add OPPs table for cpu devices

[Geert Uytterhoeven]

Hi Sudeep,

On Tue, Oct 10, 2017 at 4:33 PM, Sudeep Holla  wrote:
> On 09/10/17 12:57, Geert Uytterhoeven wrote:
>> On Thu, Oct 5, 2017 at 5:04 PM, Sudeep Holla  wrote:
>>> On 05/10/17 14:26, Simon Horman wrote:
 From: Dien Pham 

 Current, OPP tables are defined temporary,
 they are being evaluated and adjust in future.
>>
 --- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi
 +++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
 @@ -46,6 +46,8 @@
   power-domains = < R8A7796_PD_CA57_CPU0>;
   next-level-cache = <_CA57>;
   enable-method = "psci";
 + clocks =< CPG_CORE R8A7796_CLK_Z>;
 + operating-points-v2 = <_opp>;
   };

   a57_1: cpu@1 {
 @@ -55,6 +57,7 @@
   power-domains = < R8A7796_PD_CA57_CPU1>;
   next-level-cache = <_CA57>;
   enable-method = "psci";
>>>
>>> Just curious why clocks are not specified in secondaries ?
>>
>> Thanks for noticing, it would indeed be good to describe the clock dependency
>> for all CPU cores.
>>
>>> Does this continue work if I hotplug out CPUs in ascending order and
>>> then hotplug back in descending order ? Also the current driver or OS
>>> may deal with that but not a good assumption when write DT
>>
>> Yes that works, as Linux doesn't handle CPU hotplug details.
>> CPU hotplug is controlled by PSCI, i.e. out of control of the Linux kernel.
>> So it doesn't matter at all what is described here ;-)
>>
>
> Ah no, sorry for not being clear earlier. I was referring to below code
> snippet in drivers/cpufreq/cpufreq-dt.c
>
> 162 cpu_clk = clk_get(cpu_dev, NULL);
>
> 163 if (IS_ERR(cpu_clk)) {
>
> 164 ret = PTR_ERR(cpu_clk);
>
> 165 dev_err(cpu_dev, "%s: failed to get clk: %d\n",
> __func__, ret);
> 166 return ret;
>
> 167 }
>
> Now on systems using r8a7796.dtsi, if you hotplug out all A53s and
> hotplug back in A53_3 first, that should trigger cpufreq_driver->init
> from cpufreq_online which should result in execution above code.
>
> If that takes patch of__of_clk_get_by_name, then that may be problem.
> I was originally point at this when I referred hotplug and was not PSCI
> related.

IC.

That's something we cannot try, as the secure firmware doesn't enable the A53
cores, only the A57 cores.

And disabling all A57 cores is also not possible, as at least one of them
has to be kept running.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH v2 2/2] arm64: dts: r8a7796: Add OPPs table for cpu devices

[Sudeep Holla]

On 09/10/17 12:57, Geert Uytterhoeven wrote:
> Hi Sudeep,
> 
> On Thu, Oct 5, 2017 at 5:04 PM, Sudeep Holla  wrote:
>> On 05/10/17 14:26, Simon Horman wrote:
>>> From: Dien Pham 
>>>
>>> Current, OPP tables are defined temporary,
>>> they are being evaluated and adjust in future.
> 
>>> --- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi
>>> +++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
>>> @@ -46,6 +46,8 @@
>>>   power-domains = < R8A7796_PD_CA57_CPU0>;
>>>   next-level-cache = <_CA57>;
>>>   enable-method = "psci";
>>> + clocks =< CPG_CORE R8A7796_CLK_Z>;
>>> + operating-points-v2 = <_opp>;
>>>   };
>>>
>>>   a57_1: cpu@1 {
>>> @@ -55,6 +57,7 @@
>>>   power-domains = < R8A7796_PD_CA57_CPU1>;
>>>   next-level-cache = <_CA57>;
>>>   enable-method = "psci";
>>
>> Just curious why clocks are not specified in secondaries ?
> 
> Thanks for noticing, it would indeed be good to describe the clock dependency
> for all CPU cores.
> 
>> Does this continue work if I hotplug out CPUs in ascending order and
>> then hotplug back in descending order ? Also the current driver or OS
>> may deal with that but not a good assumption when write DT
> 
> Yes that works, as Linux doesn't handle CPU hotplug details.
> CPU hotplug is controlled by PSCI, i.e. out of control of the Linux kernel.
> So it doesn't matter at all what is described here ;-)
> 

Ah no, sorry for not being clear earlier. I was referring to below code
snippet in drivers/cpufreq/cpufreq-dt.c

162 cpu_clk = clk_get(cpu_dev, NULL);

163 if (IS_ERR(cpu_clk)) {

164 ret = PTR_ERR(cpu_clk);

165 dev_err(cpu_dev, "%s: failed to get clk: %d\n",
__func__, ret);
166 return ret;

167 }

Now on systems using r8a7796.dtsi, if you hotplug out all A53s and
hotplug back in A53_3 first, that should trigger cpufreq_driver->init
from cpufreq_online which should result in execution above code.

If that takes patch of__of_clk_get_by_name, then that may be problem.
I was originally point at this when I referred hotplug and was not PSCI
related.

-- 
Regards,
Sudeep

Re: [PATCH v2 2/2] arm64: dts: r8a7796: Add OPPs table for cpu devices

[Simon Horman]

On Mon, Oct 09, 2017 at 01:56:17PM +0200, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Thu, Oct 5, 2017 at 3:26 PM, Simon Horman  
> wrote:
> > From: Dien Pham 
> >
> > Current, OPP tables are defined temporary,
> > they are being evaluated and adjust in future.
> >
> > Based in part on work by Hien Dang.
> 
> > --- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> > +++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> 
> > @@ -108,6 +116,56 @@
> > };
> > };
> >
> > +   cluster0_opp: opp_table0 {
> > +   compatible = "operating-points-v2";
> > +   opp-shared;
> > +
> > +   opp@5 {
> 
> As reported before, with "make dtbs W=1":
> 
> arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dtb: Warning
> (unit_address_vs_reg): Node /opp_table0/opp@5 has a unit name,
> but no reg property
> 
> etc.
> 
> opp-5

Thanks, will fix.

Re: [PATCH v2 2/2] arm64: dts: r8a7796: Add OPPs table for cpu devices

[Simon Horman]

On Thu, Oct 05, 2017 at 04:04:47PM +0100, Sudeep Holla wrote:
> 
> 
> On 05/10/17 14:26, Simon Horman wrote:
> > From: Dien Pham 
> > 
> > Current, OPP tables are defined temporary,
> > they are being evaluated and adjust in future.
> > 
> 
> I assume these OPPs will continue to work in future but just not optimal.

Yes, that is my understanding.

> > Based in part on work by Hien Dang.
> > 
> > Signed-off-by: Dien Pham 
> > Signed-off-by: Takeshi Kihara 
> > Signed-off-by: Simon Horman 
> > ---
> > v2 [Simon Horman]
> > - Only provide one operating points node for each operating-points-v2 node
> >   as per the binding; other nodes were unused and have been removed
> > 
> > v1 [Simon Horman]
> > - consolidated several patches into one
> > 
> > v0 [Dien Pham]
> > ---
> >  arch/arm64/boot/dts/renesas/r8a7796.dtsi | 58 
> > 
> >  1 file changed, 58 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi 
> > b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> > index 57ac5ca6ed98..2d9edc61437c 100644
> > --- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> > +++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> > @@ -46,6 +46,8 @@
> > power-domains = < R8A7796_PD_CA57_CPU0>;
> > next-level-cache = <_CA57>;
> > enable-method = "psci";
> > +   clocks =< CPG_CORE R8A7796_CLK_Z>;
> > +   operating-points-v2 = <_opp>;
> > };
> >  
> > a57_1: cpu@1 {
> > @@ -55,6 +57,7 @@
> > power-domains = < R8A7796_PD_CA57_CPU1>;
> > next-level-cache = <_CA57>;
> > enable-method = "psci";
> 
> Just curious why clocks are not specified in secondaries ?
> Does this continue work if I hotplug out CPUs in ascending order and
> then hotplug back in descending order ? Also the current driver or OS
> may deal with that but not a good assumption when write DT

Good point. I expect that this is an oversight.

Re: [PATCH v2 2/2] arm64: dts: r8a7796: Add OPPs table for cpu devices

[Geert Uytterhoeven]

Hi Sudeep,

On Thu, Oct 5, 2017 at 5:04 PM, Sudeep Holla  wrote:
> On 05/10/17 14:26, Simon Horman wrote:
>> From: Dien Pham 
>>
>> Current, OPP tables are defined temporary,
>> they are being evaluated and adjust in future.

>> --- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi
>> +++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
>> @@ -46,6 +46,8 @@
>>   power-domains = < R8A7796_PD_CA57_CPU0>;
>>   next-level-cache = <_CA57>;
>>   enable-method = "psci";
>> + clocks =< CPG_CORE R8A7796_CLK_Z>;
>> + operating-points-v2 = <_opp>;
>>   };
>>
>>   a57_1: cpu@1 {
>> @@ -55,6 +57,7 @@
>>   power-domains = < R8A7796_PD_CA57_CPU1>;
>>   next-level-cache = <_CA57>;
>>   enable-method = "psci";
>
> Just curious why clocks are not specified in secondaries ?

Thanks for noticing, it would indeed be good to describe the clock dependency
for all CPU cores.

> Does this continue work if I hotplug out CPUs in ascending order and
> then hotplug back in descending order ? Also the current driver or OS
> may deal with that but not a good assumption when write DT

Yes that works, as Linux doesn't handle CPU hotplug details.
CPU hotplug is controlled by PSCI, i.e. out of control of the Linux kernel.
So it doesn't matter at all what is described here ;-)

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH v2 2/2] arm64: dts: r8a7796: Add OPPs table for cpu devices

[Geert Uytterhoeven]

Hi Simon,

On Thu, Oct 5, 2017 at 3:26 PM, Simon Horman  wrote:
> From: Dien Pham 
>
> Current, OPP tables are defined temporary,
> they are being evaluated and adjust in future.
>
> Based in part on work by Hien Dang.

> --- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi

> @@ -108,6 +116,56 @@
> };
> };
>
> +   cluster0_opp: opp_table0 {
> +   compatible = "operating-points-v2";
> +   opp-shared;
> +
> +   opp@5 {

As reported before, with "make dtbs W=1":

arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dtb: Warning
(unit_address_vs_reg): Node /opp_table0/opp@5 has a unit name,
but no reg property

etc.

opp-5

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH v2 2/2] arm64: dts: r8a7796: Add OPPs table for cpu devices

[Sudeep Holla]

On 05/10/17 14:26, Simon Horman wrote:
> From: Dien Pham 
> 
> Current, OPP tables are defined temporary,
> they are being evaluated and adjust in future.
> 

I assume these OPPs will continue to work in future but just not optimal.

> Based in part on work by Hien Dang.
> 
> Signed-off-by: Dien Pham 
> Signed-off-by: Takeshi Kihara 
> Signed-off-by: Simon Horman 
> ---
> v2 [Simon Horman]
> - Only provide one operating points node for each operating-points-v2 node
>   as per the binding; other nodes were unused and have been removed
> 
> v1 [Simon Horman]
> - consolidated several patches into one
> 
> v0 [Dien Pham]
> ---
>  arch/arm64/boot/dts/renesas/r8a7796.dtsi | 58 
> 
>  1 file changed, 58 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi 
> b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> index 57ac5ca6ed98..2d9edc61437c 100644
> --- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> @@ -46,6 +46,8 @@
>   power-domains = < R8A7796_PD_CA57_CPU0>;
>   next-level-cache = <_CA57>;
>   enable-method = "psci";
> + clocks =< CPG_CORE R8A7796_CLK_Z>;
> + operating-points-v2 = <_opp>;
>   };
>  
>   a57_1: cpu@1 {
> @@ -55,6 +57,7 @@
>   power-domains = < R8A7796_PD_CA57_CPU1>;
>   next-level-cache = <_CA57>;
>   enable-method = "psci";

Just curious why clocks are not specified in secondaries ?
Does this continue work if I hotplug out CPUs in ascending order and
then hotplug back in descending order ? Also the current driver or OS
may deal with that but not a good assumption when write DT

-- 
Regards,
Sudeep

[PATCH v2 2/2] arm64: dts: r8a7796: Add OPPs table for cpu devices

[Simon Horman]

From: Dien Pham 

Current, OPP tables are defined temporary,
they are being evaluated and adjust in future.

Based in part on work by Hien Dang.

Signed-off-by: Dien Pham 
Signed-off-by: Takeshi Kihara 
Signed-off-by: Simon Horman 
---
v2 [Simon Horman]
- Only provide one operating points node for each operating-points-v2 node
  as per the binding; other nodes were unused and have been removed

v1 [Simon Horman]
- consolidated several patches into one

v0 [Dien Pham]
---
 arch/arm64/boot/dts/renesas/r8a7796.dtsi | 58 
 1 file changed, 58 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi 
b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
index 57ac5ca6ed98..2d9edc61437c 100644
--- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
@@ -46,6 +46,8 @@
power-domains = < R8A7796_PD_CA57_CPU0>;
next-level-cache = <_CA57>;
enable-method = "psci";
+   clocks =< CPG_CORE R8A7796_CLK_Z>;
+   operating-points-v2 = <_opp>;
};
 
a57_1: cpu@1 {
@@ -55,6 +57,7 @@
power-domains = < R8A7796_PD_CA57_CPU1>;
next-level-cache = <_CA57>;
enable-method = "psci";
+   operating-points-v2 = <_opp>;
};
 
a53_0: cpu@100 {
@@ -64,6 +67,8 @@
power-domains = < R8A7796_PD_CA53_CPU0>;
next-level-cache = <_CA53>;
enable-method = "psci";
+   clocks =< CPG_CORE R8A7796_CLK_Z2>;
+   operating-points-v2 = <_opp>;
};
 
a53_1: cpu@101 {
@@ -73,6 +78,7 @@
power-domains = < R8A7796_PD_CA53_CPU1>;
next-level-cache = <_CA53>;
enable-method = "psci";
+   operating-points-v2 = <_opp>;
};
 
a53_2: cpu@102 {
@@ -82,6 +88,7 @@
power-domains = < R8A7796_PD_CA53_CPU2>;
next-level-cache = <_CA53>;
enable-method = "psci";
+   operating-points-v2 = <_opp>;
};
 
a53_3: cpu@103 {
@@ -91,6 +98,7 @@
power-domains = < R8A7796_PD_CA53_CPU3>;
next-level-cache = <_CA53>;
enable-method = "psci";
+   operating-points-v2 = <_opp>;
};
 
L2_CA57: cache-controller-0 {
@@ -108,6 +116,56 @@
};
};
 
+   cluster0_opp: opp_table0 {
+   compatible = "operating-points-v2";
+   opp-shared;
+
+   opp@5 {
+   opp-hz = /bits/ 64 <5>;
+   opp-microvolt = <82>;
+   clock-latency-ns = <30>;
+   };
+   opp@10 {
+   opp-hz = /bits/ 64 <10>;
+   opp-microvolt = <82>;
+   clock-latency-ns = <30>;
+   };
+   opp@15 {
+   opp-hz = /bits/ 64 <15>;
+   opp-microvolt = <82>;
+   clock-latency-ns = <30>;
+   };
+   opp@16 {
+   opp-hz = /bits/ 64 <16>;
+   opp-microvolt = <90>;
+   clock-latency-ns = <30>;
+   turbo-mode;
+   };
+   opp@17 {
+   opp-hz = /bits/ 64 <17>;
+   opp-microvolt = <90>;
+   clock-latency-ns = <30>;
+   turbo-mode;
+   };
+   opp@18 {
+   opp-hz = /bits/ 64 <18>;
+   opp-microvolt = <96>;
+   clock-latency-ns = <30>;
+   turbo-mode;
+   };
+   };
+
+   cluster1_opp: opp_table1 {
+   compatible = "operating-points-v2";
+   opp-shared;
+
+   opp@12 {
+   opp-hz = /bits/ 64 <12>;
+   opp-microvolt = <82>;
+   clock-latency-ns = <30>;
+   };
+   };
+
extal_clk: extal {
compatible = "fixed-clock";
#clock-cells = <0>;
-- 
2.1.4