| Message ID | 1545338982-19466-1-git-send-email-ykaneko0929@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Simon Horman |
| Headers | show |
| Series | [PATCH/RFT] arm64: dts: renesas: r8a7795: Create thermal zone to support IPA | expand |
Hi Keneko-san, Thanks for your work. Comments to this patch applies to all patches in this series. On 2018-12-21 05:49:39 +0900, Yoshihiro Kaneko wrote: > From: Dien Pham <dien.pham.ry@renesas.com> > > [dien.pham.ry: arm64: dts: r8a7795: Add support IPA for CA53 core] > > Setup a thermal zone driven by SoC temperature sensor. > Create passive trip points and bind them to CPUFreq cooling > device that supports power extension. > > In R-Car Gen3, IPA is supportted for only one channel > (on H3/M3/M3N board, it is channel THS3). Reason: > Currently, IPA controls base on only CPU temperature. > And only one thermal channel is assembled closest > CPU cores is selected as target of IPA. > If other channels are used, IPA controlling is not properly. > > Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com> > [gaku.inami.xw: fix the trip temperature for cooling-device] > [gaku.inami.xw: fix the power coefficient] > Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com> > Signed-off-by: Hien Dang <hien.dang.eb@rvc.renesas.com> > Signed-off-by: An Huynh <an.huynh.uj@rvc.renesas.com> > [takeshi.kihara.df: fix W=1 dtc unit_address_vs_reg warnings] > Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com> > --- > > This patch is based on the devel branch of Simon Horman's renesas tree. > > arch/arm64/boot/dts/renesas/r8a7795.dtsi | 64 ++++++++++++++------------------ > 1 file changed, 27 insertions(+), 37 deletions(-) > > diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi > index af9605d..dd52b50 100644 > --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi > +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi > @@ -155,6 +155,9 @@ > power-domains = <&sysc R8A7795_PD_CA57_CPU0>; > next-level-cache = <&L2_CA57>; > enable-method = "psci"; > + dynamic-power-coefficient = <854>; > + cooling-min-level = <0>; > + cooling-max-level = <2>; I can't find any documentation or code which makes use of the cooling-min-level and cooling-max-level properties on v5.0-rc1. What is the intended usage for these properties? > clocks = <&cpg CPG_CORE R8A7795_CLK_Z>; > operating-points-v2 = <&cluster0_opp>; > capacity-dmips-mhz = <1024>; > @@ -207,6 +210,10 @@ > power-domains = <&sysc R8A7795_PD_CA53_CPU0>; > next-level-cache = <&L2_CA53>; > enable-method = "psci"; > + #cooling-cells = <2>; > + dynamic-power-coefficient = <277>; > + cooling-min-level = <0>; > + cooling-max-level = <2>; > clocks = <&cpg CPG_CORE R8A7795_CLK_Z2>; > operating-points-v2 = <&cluster1_opp>; > capacity-dmips-mhz = <535>; > @@ -3098,58 +3105,30 @@ > polling-delay-passive = <250>; > polling-delay = <1000>; > thermal-sensors = <&tsc 0>; > + sustainable-power = <6313>; > > trips { > - sensor1_passive: sensor1-passive { > - temperature = <95000>; > - hysteresis = <1000>; > - type = "passive"; > - }; > sensor1_crit: sensor1-crit { > temperature = <120000>; > hysteresis = <1000>; > type = "critical"; > }; > }; > - > - cooling-maps { > - map0 { > - trip = <&sensor1_passive>; > - cooling-device = <&a57_0 4 4>, > - <&a57_1 4 4>, > - <&a57_2 4 4>, > - <&a57_3 4 4>; > - }; > - }; > }; > > sensor_thermal2: sensor-thermal2 { > polling-delay-passive = <250>; > polling-delay = <1000>; > thermal-sensors = <&tsc 1>; > + sustainable-power = <6313>; > > trips { > - sensor2_passive: sensor2-passive { > - temperature = <95000>; > - hysteresis = <1000>; > - type = "passive"; > - }; > sensor2_crit: sensor2-crit { > temperature = <120000>; > hysteresis = <1000>; > type = "critical"; > }; > }; > - > - cooling-maps { > - map0 { > - trip = <&sensor2_passive>; > - cooling-device = <&a57_0 4 4>, > - <&a57_1 4 4>, > - <&a57_2 4 4>, > - <&a57_3 4 4>; > - }; > - }; > }; > > sensor_thermal3: sensor-thermal3 { > @@ -3158,11 +3137,18 @@ > thermal-sensors = <&tsc 2>; > > trips { > - sensor3_passive: sensor3-passive { > - temperature = <95000>; > + threshold: trip-point0 { > + temperature = <90000>; > + hysteresis = <1000>; > + type = "passive"; > + }; What is the usage of trip-point0? The label threshold is never referenced anywhere, or am I missing something? > + > + target: trip-point1 { > + temperature = <100000>; > hysteresis = <1000>; > type = "passive"; > }; > + > sensor3_crit: sensor3-crit { > temperature = <120000>; > hysteresis = <1000>; > @@ -3172,11 +3158,15 @@ > > cooling-maps { > map0 { > - trip = <&sensor3_passive>; > - cooling-device = <&a57_0 4 4>, > - <&a57_1 4 4>, > - <&a57_2 4 4>, > - <&a57_3 4 4>; > + trip = <&target>; > + cooling-device = <&a57_0 0 2>; We have 5 (0-4) cooling states for the A57s on this SoC. Out of curiosity why allow states 0-2 here and not force it do more cooling or keep the to max cooling (4) as before this change as this is set to a trip point with a rather large temperature? Not saying this is wrong only curious :-) > + contribution = <1024>; > + }; > + > + map1 { > + trip = <&target>; > + cooling-device = <&a53_0 0 2>; > + contribution = <1024>; > }; > }; > }; > -- > 1.9.1 >
On Thu, Jan 10, 2019 at 05:33:57PM +0100, Niklas Söderlund wrote: > Hi Keneko-san, > > Thanks for your work. > > Comments to this patch applies to all patches in this series. > > On 2018-12-21 05:49:39 +0900, Yoshihiro Kaneko wrote: > > From: Dien Pham <dien.pham.ry@renesas.com> > > > > [dien.pham.ry: arm64: dts: r8a7795: Add support IPA for CA53 core] > > > > Setup a thermal zone driven by SoC temperature sensor. > > Create passive trip points and bind them to CPUFreq cooling > > device that supports power extension. > > > > In R-Car Gen3, IPA is supportted for only one channel > > (on H3/M3/M3N board, it is channel THS3). Reason: > > Currently, IPA controls base on only CPU temperature. > > And only one thermal channel is assembled closest > > CPU cores is selected as target of IPA. > > If other channels are used, IPA controlling is not properly. > > > > Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com> > > [gaku.inami.xw: fix the trip temperature for cooling-device] > > [gaku.inami.xw: fix the power coefficient] > > Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com> > > Signed-off-by: Hien Dang <hien.dang.eb@rvc.renesas.com> > > Signed-off-by: An Huynh <an.huynh.uj@rvc.renesas.com> > > [takeshi.kihara.df: fix W=1 dtc unit_address_vs_reg warnings] > > Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> > > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com> > > --- > > > > This patch is based on the devel branch of Simon Horman's renesas tree. > > > > arch/arm64/boot/dts/renesas/r8a7795.dtsi | 64 ++++++++++++++------------------ > > 1 file changed, 27 insertions(+), 37 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi > > index af9605d..dd52b50 100644 > > --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi > > +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi > > @@ -155,6 +155,9 @@ > > power-domains = <&sysc R8A7795_PD_CA57_CPU0>; > > next-level-cache = <&L2_CA57>; > > enable-method = "psci"; > > + dynamic-power-coefficient = <854>; > > + cooling-min-level = <0>; > > + cooling-max-level = <2>; > > I can't find any documentation or code which makes use of the > cooling-min-level and cooling-max-level properties on v5.0-rc1. What is > the intended usage for these properties? It looks like these properties were removed in v4.17-rc1 by bbcf071969b2 ("cpufreq: cpu0: rename driver and internals to 'cpufreq_dt'") and should be dropped from this and similar patches. > > > clocks = <&cpg CPG_CORE R8A7795_CLK_Z>; > > operating-points-v2 = <&cluster0_opp>; > > capacity-dmips-mhz = <1024>; > > @@ -207,6 +210,10 @@ > > power-domains = <&sysc R8A7795_PD_CA53_CPU0>; > > next-level-cache = <&L2_CA53>; > > enable-method = "psci"; > > + #cooling-cells = <2>; > > + dynamic-power-coefficient = <277>; > > + cooling-min-level = <0>; > > + cooling-max-level = <2>; > > clocks = <&cpg CPG_CORE R8A7795_CLK_Z2>; > > operating-points-v2 = <&cluster1_opp>; > > capacity-dmips-mhz = <535>; > > @@ -3098,58 +3105,30 @@ > > polling-delay-passive = <250>; > > polling-delay = <1000>; > > thermal-sensors = <&tsc 0>; > > + sustainable-power = <6313>; > > > > trips { > > - sensor1_passive: sensor1-passive { > > - temperature = <95000>; > > - hysteresis = <1000>; > > - type = "passive"; > > - }; > > sensor1_crit: sensor1-crit { > > temperature = <120000>; > > hysteresis = <1000>; > > type = "critical"; > > }; > > }; > > - > > - cooling-maps { > > - map0 { > > - trip = <&sensor1_passive>; > > - cooling-device = <&a57_0 4 4>, > > - <&a57_1 4 4>, > > - <&a57_2 4 4>, > > - <&a57_3 4 4>; > > - }; > > - }; > > }; > > > > sensor_thermal2: sensor-thermal2 { > > polling-delay-passive = <250>; > > polling-delay = <1000>; > > thermal-sensors = <&tsc 1>; > > + sustainable-power = <6313>; > > > > trips { > > - sensor2_passive: sensor2-passive { > > - temperature = <95000>; > > - hysteresis = <1000>; > > - type = "passive"; > > - }; > > sensor2_crit: sensor2-crit { > > temperature = <120000>; > > hysteresis = <1000>; > > type = "critical"; > > }; > > }; > > - > > - cooling-maps { > > - map0 { > > - trip = <&sensor2_passive>; > > - cooling-device = <&a57_0 4 4>, > > - <&a57_1 4 4>, > > - <&a57_2 4 4>, > > - <&a57_3 4 4>; > > - }; > > - }; > > }; > > > > sensor_thermal3: sensor-thermal3 { > > @@ -3158,11 +3137,18 @@ > > thermal-sensors = <&tsc 2>; > > > > trips { > > - sensor3_passive: sensor3-passive { > > - temperature = <95000>; > > + threshold: trip-point0 { > > + temperature = <90000>; > > + hysteresis = <1000>; > > + type = "passive"; > > + }; > > What is the usage of trip-point0? The label threshold is never > referenced anywhere, or am I missing something? Good point. Given this patch in its current form I think trip-point0 should be dropped. > > > + > > + target: trip-point1 { > > + temperature = <100000>; > > hysteresis = <1000>; > > type = "passive"; > > }; > > + > > sensor3_crit: sensor3-crit { > > temperature = <120000>; > > hysteresis = <1000>; > > @@ -3172,11 +3158,15 @@ > > > > cooling-maps { > > map0 { > > - trip = <&sensor3_passive>; > > - cooling-device = <&a57_0 4 4>, > > - <&a57_1 4 4>, > > - <&a57_2 4 4>, > > - <&a57_3 4 4>; > > + trip = <&target>; > > + cooling-device = <&a57_0 0 2>; > > We have 5 (0-4) cooling states for the A57s on this SoC. Out of > curiosity why allow states 0-2 here and not force it do more cooling or > keep the to max cooling (4) as before this change as this is set to a > trip point with a rather large temperature? Not saying this is wrong > only curious :-) This I do not know. Do you think it would be worth following-up on internally? > > + contribution = <1024>; > > + }; > > + > > + map1 { > > + trip = <&target>; > > + cooling-device = <&a53_0 0 2>; > > + contribution = <1024>; > > }; > > }; > > }; > > -- > > 1.9.1 > > > > -- > Regards, > Niklas Söderlund >
Hi Simon, On 2019-04-08 14:29:41 +0200, Simon Horman wrote: > > > @@ -3172,11 +3158,15 @@ > > > > > > cooling-maps { > > > map0 { > > > - trip = <&sensor3_passive>; > > > - cooling-device = <&a57_0 4 4>, > > > - <&a57_1 4 4>, > > > - <&a57_2 4 4>, > > > - <&a57_3 4 4>; > > > + trip = <&target>; > > > + cooling-device = <&a57_0 0 2>; > > > > We have 5 (0-4) cooling states for the A57s on this SoC. Out of > > curiosity why allow states 0-2 here and not force it do more cooling or > > keep the to max cooling (4) as before this change as this is set to a > > trip point with a rather large temperature? Not saying this is wrong > > only curious :-) > > This I do not know. Do you think it would be worth following-up on > internally? I'm no expert on this but I think it could be educational to learn why not the full range is used. Still this might be correct so I don't want to block this patch based on my ignorance.
Hi Simon, On Mon, Apr 8, 2019 at 2:29 PM Simon Horman <horms@verge.net.au> wrote: > On Thu, Jan 10, 2019 at 05:33:57PM +0100, Niklas Söderlund wrote: > > On 2018-12-21 05:49:39 +0900, Yoshihiro Kaneko wrote: > > > From: Dien Pham <dien.pham.ry@renesas.com> > > > > > > [dien.pham.ry: arm64: dts: r8a7795: Add support IPA for CA53 core] > > > > > > Setup a thermal zone driven by SoC temperature sensor. > > > Create passive trip points and bind them to CPUFreq cooling > > > device that supports power extension. > > > > > > In R-Car Gen3, IPA is supportted for only one channel > > > (on H3/M3/M3N board, it is channel THS3). Reason: > > > Currently, IPA controls base on only CPU temperature. > > > And only one thermal channel is assembled closest > > > CPU cores is selected as target of IPA. > > > If other channels are used, IPA controlling is not properly. > > > > > > Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com> > > > [gaku.inami.xw: fix the trip temperature for cooling-device] > > > [gaku.inami.xw: fix the power coefficient] > > > Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com> > > > Signed-off-by: Hien Dang <hien.dang.eb@rvc.renesas.com> > > > Signed-off-by: An Huynh <an.huynh.uj@rvc.renesas.com> > > > [takeshi.kihara.df: fix W=1 dtc unit_address_vs_reg warnings] > > > Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> > > > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com> > > > --- > > > > > > This patch is based on the devel branch of Simon Horman's renesas tree. > > > > > > arch/arm64/boot/dts/renesas/r8a7795.dtsi | 64 ++++++++++++++------------------ > > > 1 file changed, 27 insertions(+), 37 deletions(-) > > > > > > diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi > > > index af9605d..dd52b50 100644 > > > --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi > > > +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi > > > @@ -155,6 +155,9 @@ > > > power-domains = <&sysc R8A7795_PD_CA57_CPU0>; > > > next-level-cache = <&L2_CA57>; > > > enable-method = "psci"; > > > + dynamic-power-coefficient = <854>; > > > + cooling-min-level = <0>; > > > + cooling-max-level = <2>; > > > > I can't find any documentation or code which makes use of the > > cooling-min-level and cooling-max-level properties on v5.0-rc1. What is > > the intended usage for these properties? > > It looks like these properties were removed in v4.17-rc1 by > bbcf071969b2 ("cpufreq: cpu0: rename driver and internals to 'cpufreq_dt'") > and should be dropped from this and similar patches. See commit e04907dbc25930b8 ("dt-bindings: thermal: Remove "cooling-{min|max}-level" properties"). Gr{oetje,eeting}s, Geert
diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi index af9605d..dd52b50 100644 --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi @@ -155,6 +155,9 @@ power-domains = <&sysc R8A7795_PD_CA57_CPU0>; next-level-cache = <&L2_CA57>; enable-method = "psci"; + dynamic-power-coefficient = <854>; + cooling-min-level = <0>; + cooling-max-level = <2>; clocks = <&cpg CPG_CORE R8A7795_CLK_Z>; operating-points-v2 = <&cluster0_opp>; capacity-dmips-mhz = <1024>; @@ -207,6 +210,10 @@ power-domains = <&sysc R8A7795_PD_CA53_CPU0>; next-level-cache = <&L2_CA53>; enable-method = "psci"; + #cooling-cells = <2>; + dynamic-power-coefficient = <277>; + cooling-min-level = <0>; + cooling-max-level = <2>; clocks = <&cpg CPG_CORE R8A7795_CLK_Z2>; operating-points-v2 = <&cluster1_opp>; capacity-dmips-mhz = <535>; @@ -3098,58 +3105,30 @@ polling-delay-passive = <250>; polling-delay = <1000>; thermal-sensors = <&tsc 0>; + sustainable-power = <6313>; trips { - sensor1_passive: sensor1-passive { - temperature = <95000>; - hysteresis = <1000>; - type = "passive"; - }; sensor1_crit: sensor1-crit { temperature = <120000>; hysteresis = <1000>; type = "critical"; }; }; - - cooling-maps { - map0 { - trip = <&sensor1_passive>; - cooling-device = <&a57_0 4 4>, - <&a57_1 4 4>, - <&a57_2 4 4>, - <&a57_3 4 4>; - }; - }; }; sensor_thermal2: sensor-thermal2 { polling-delay-passive = <250>; polling-delay = <1000>; thermal-sensors = <&tsc 1>; + sustainable-power = <6313>; trips { - sensor2_passive: sensor2-passive { - temperature = <95000>; - hysteresis = <1000>; - type = "passive"; - }; sensor2_crit: sensor2-crit { temperature = <120000>; hysteresis = <1000>; type = "critical"; }; }; - - cooling-maps { - map0 { - trip = <&sensor2_passive>; - cooling-device = <&a57_0 4 4>, - <&a57_1 4 4>, - <&a57_2 4 4>, - <&a57_3 4 4>; - }; - }; }; sensor_thermal3: sensor-thermal3 { @@ -3158,11 +3137,18 @@ thermal-sensors = <&tsc 2>; trips { - sensor3_passive: sensor3-passive { - temperature = <95000>; + threshold: trip-point0 { + temperature = <90000>; + hysteresis = <1000>; + type = "passive"; + }; + + target: trip-point1 { + temperature = <100000>; hysteresis = <1000>; type = "passive"; }; + sensor3_crit: sensor3-crit { temperature = <120000>; hysteresis = <1000>; @@ -3172,11 +3158,15 @@ cooling-maps { map0 { - trip = <&sensor3_passive>; - cooling-device = <&a57_0 4 4>, - <&a57_1 4 4>, - <&a57_2 4 4>, - <&a57_3 4 4>; + trip = <&target>; + cooling-device = <&a57_0 0 2>; + contribution = <1024>; + }; + + map1 { + trip = <&target>; + cooling-device = <&a53_0 0 2>; + contribution = <1024>; }; }; };