| Message ID | 1507209984-17123-3-git-send-email-horms+renesas@verge.net.au (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Simon Horman |
| Headers | show |
On 05/10/17 14:26, Simon Horman wrote: > From: Dien Pham <dien.pham.ry@rvc.renesas.com> > > 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 <dien.pham.ry@rvc.renesas.com> > Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > --- > 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 = <&sysc R8A7796_PD_CA57_CPU0>; > next-level-cache = <&L2_CA57>; > enable-method = "psci"; > + clocks =<&cpg CPG_CORE R8A7796_CLK_Z>; > + operating-points-v2 = <&cluster0_opp>; > }; > > a57_1: cpu@1 { > @@ -55,6 +57,7 @@ > power-domains = <&sysc R8A7796_PD_CA57_CPU1>; > next-level-cache = <&L2_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
Hi Simon, On Thu, Oct 5, 2017 at 3:26 PM, Simon Horman <horms+renesas@verge.net.au> wrote: > From: Dien Pham <dien.pham.ry@rvc.renesas.com> > > 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@500000000 { 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@500000000 has a unit name, but no reg property etc. opp-500000000 Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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
Hi Sudeep, On Thu, Oct 5, 2017 at 5:04 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > On 05/10/17 14:26, Simon Horman wrote: >> From: Dien Pham <dien.pham.ry@rvc.renesas.com> >> >> 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 = <&sysc R8A7796_PD_CA57_CPU0>; >> next-level-cache = <&L2_CA57>; >> enable-method = "psci"; >> + clocks =<&cpg CPG_CORE R8A7796_CLK_Z>; >> + operating-points-v2 = <&cluster0_opp>; >> }; >> >> a57_1: cpu@1 { >> @@ -55,6 +57,7 @@ >> power-domains = <&sysc R8A7796_PD_CA57_CPU1>; >> next-level-cache = <&L2_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 -- geert@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
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 <dien.pham.ry@rvc.renesas.com> > > > > 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 <dien.pham.ry@rvc.renesas.com> > > Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> > > Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > > --- > > 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 = <&sysc R8A7796_PD_CA57_CPU0>; > > next-level-cache = <&L2_CA57>; > > enable-method = "psci"; > > + clocks =<&cpg CPG_CORE R8A7796_CLK_Z>; > > + operating-points-v2 = <&cluster0_opp>; > > }; > > > > a57_1: cpu@1 { > > @@ -55,6 +57,7 @@ > > power-domains = <&sysc R8A7796_PD_CA57_CPU1>; > > next-level-cache = <&L2_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.
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 <horms+renesas@verge.net.au> wrote: > > From: Dien Pham <dien.pham.ry@rvc.renesas.com> > > > > 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@500000000 { > > 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@500000000 has a unit name, > but no reg property > > etc. > > opp-500000000 Thanks, will fix.
On 09/10/17 12:57, Geert Uytterhoeven wrote: > Hi Sudeep, > > On Thu, Oct 5, 2017 at 5:04 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> On 05/10/17 14:26, Simon Horman wrote: >>> From: Dien Pham <dien.pham.ry@rvc.renesas.com> >>> >>> 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 = <&sysc R8A7796_PD_CA57_CPU0>; >>> next-level-cache = <&L2_CA57>; >>> enable-method = "psci"; >>> + clocks =<&cpg CPG_CORE R8A7796_CLK_Z>; >>> + operating-points-v2 = <&cluster0_opp>; >>> }; >>> >>> a57_1: cpu@1 { >>> @@ -55,6 +57,7 @@ >>> power-domains = <&sysc R8A7796_PD_CA57_CPU1>; >>> next-level-cache = <&L2_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.
Hi Sudeep, On Tue, Oct 10, 2017 at 4:33 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > On 09/10/17 12:57, Geert Uytterhoeven wrote: >> On Thu, Oct 5, 2017 at 5:04 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>> On 05/10/17 14:26, Simon Horman wrote: >>>> From: Dien Pham <dien.pham.ry@rvc.renesas.com> >>>> >>>> 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 = <&sysc R8A7796_PD_CA57_CPU0>; >>>> next-level-cache = <&L2_CA57>; >>>> enable-method = "psci"; >>>> + clocks =<&cpg CPG_CORE R8A7796_CLK_Z>; >>>> + operating-points-v2 = <&cluster0_opp>; >>>> }; >>>> >>>> a57_1: cpu@1 { >>>> @@ -55,6 +57,7 @@ >>>> power-domains = <&sysc R8A7796_PD_CA57_CPU1>; >>>> next-level-cache = <&L2_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 -- geert@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
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 = <&sysc R8A7796_PD_CA57_CPU0>; next-level-cache = <&L2_CA57>; enable-method = "psci"; + clocks =<&cpg CPG_CORE R8A7796_CLK_Z>; + operating-points-v2 = <&cluster0_opp>; }; a57_1: cpu@1 { @@ -55,6 +57,7 @@ power-domains = <&sysc R8A7796_PD_CA57_CPU1>; next-level-cache = <&L2_CA57>; enable-method = "psci"; + operating-points-v2 = <&cluster0_opp>; }; a53_0: cpu@100 { @@ -64,6 +67,8 @@ power-domains = <&sysc R8A7796_PD_CA53_CPU0>; next-level-cache = <&L2_CA53>; enable-method = "psci"; + clocks =<&cpg CPG_CORE R8A7796_CLK_Z2>; + operating-points-v2 = <&cluster1_opp>; }; a53_1: cpu@101 { @@ -73,6 +78,7 @@ power-domains = <&sysc R8A7796_PD_CA53_CPU1>; next-level-cache = <&L2_CA53>; enable-method = "psci"; + operating-points-v2 = <&cluster1_opp>; }; a53_2: cpu@102 { @@ -82,6 +88,7 @@ power-domains = <&sysc R8A7796_PD_CA53_CPU2>; next-level-cache = <&L2_CA53>; enable-method = "psci"; + operating-points-v2 = <&cluster1_opp>; }; a53_3: cpu@103 { @@ -91,6 +98,7 @@ power-domains = <&sysc R8A7796_PD_CA53_CPU3>; next-level-cache = <&L2_CA53>; enable-method = "psci"; + operating-points-v2 = <&cluster1_opp>; }; L2_CA57: cache-controller-0 { @@ -108,6 +116,56 @@ }; }; + cluster0_opp: opp_table0 { + compatible = "operating-points-v2"; + opp-shared; + + opp@500000000 { + opp-hz = /bits/ 64 <500000000>; + opp-microvolt = <820000>; + clock-latency-ns = <300000>; + }; + opp@1000000000 { + opp-hz = /bits/ 64 <1000000000>; + opp-microvolt = <820000>; + clock-latency-ns = <300000>; + }; + opp@1500000000 { + opp-hz = /bits/ 64 <1500000000>; + opp-microvolt = <820000>; + clock-latency-ns = <300000>; + }; + opp@1600000000 { + opp-hz = /bits/ 64 <1600000000>; + opp-microvolt = <900000>; + clock-latency-ns = <300000>; + turbo-mode; + }; + opp@1700000000 { + opp-hz = /bits/ 64 <1700000000>; + opp-microvolt = <900000>; + clock-latency-ns = <300000>; + turbo-mode; + }; + opp@1800000000 { + opp-hz = /bits/ 64 <1800000000>; + opp-microvolt = <960000>; + clock-latency-ns = <300000>; + turbo-mode; + }; + }; + + cluster1_opp: opp_table1 { + compatible = "operating-points-v2"; + opp-shared; + + opp@1200000000 { + opp-hz = /bits/ 64 <1200000000>; + opp-microvolt = <820000>; + clock-latency-ns = <300000>; + }; + }; + extal_clk: extal { compatible = "fixed-clock"; #clock-cells = <0>;