2019-04-23 04:19:12

by Andy Tang

[permalink] [raw]
Subject: [PATCH v6] arm64: dts: ls1088a: add one more thermal zone node

Ls1088a has 2 thermal sensors, core cluster and SoC platform. Core cluster
sensor is used to monitor the temperature of core and SoC platform is for
platform. The current dts only support the first sensor.
This patch adds the second sensor node to dts to enable it.

Signed-off-by: Yuantian Tang <[email protected]>
---
v6:
- add cooling device map to cpu0-7 in platform node.
arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi | 43 +++++++++++++++++++++--
1 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
index 661137f..a697a82 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
@@ -129,19 +129,19 @@
};

thermal-zones {
- cpu_thermal: cpu-thermal {
+ core-cluster {
polling-delay-passive = <1000>;
polling-delay = <5000>;
thermal-sensors = <&tmu 0>;

trips {
- cpu_alert: cpu-alert {
+ core_cluster_alert: core-cluster-alert {
temperature = <85000>;
hysteresis = <2000>;
type = "passive";
};

- cpu_crit: cpu-crit {
+ core_cluster_crit: core-cluster-crit {
temperature = <95000>;
hysteresis = <2000>;
type = "critical";
@@ -150,7 +150,42 @@

cooling-maps {
map0 {
- trip = <&cpu_alert>;
+ trip = <&core_cluster_alert>;
+ cooling-device =
+ <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+ };
+ };
+ };
+
+ platform {
+ polling-delay-passive = <1000>;
+ polling-delay = <5000>;
+ thermal-sensors = <&tmu 1>;
+
+ trips {
+ platform_alert: platform-alert {
+ temperature = <85000>;
+ hysteresis = <2000>;
+ type = "passive";
+ };
+
+ platform_crit: platform-crit {
+ temperature = <95000>;
+ hysteresis = <2000>;
+ type = "critical";
+ };
+ };
+
+ cooling-maps {
+ map0 {
+ trip = <&platform_alert>;
cooling-device =
<&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
<&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
--
1.7.1


2019-05-10 03:17:03

by Shawn Guo

[permalink] [raw]
Subject: Re: [PATCH v6] arm64: dts: ls1088a: add one more thermal zone node

On Tue, Apr 23, 2019 at 10:25:07AM +0800, Yuantian Tang wrote:
> Ls1088a has 2 thermal sensors, core cluster and SoC platform. Core cluster
> sensor is used to monitor the temperature of core and SoC platform is for
> platform. The current dts only support the first sensor.
> This patch adds the second sensor node to dts to enable it.
>
> Signed-off-by: Yuantian Tang <[email protected]>
> ---
> v6:
> - add cooling device map to cpu0-7 in platform node.

@Daniel, are you fine with this version?

Shawn

2019-05-10 03:44:49

by Andy Tang

[permalink] [raw]
Subject: RE: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more thermal zone node

> -----Original Message-----
> From: Shawn Guo <[email protected]>
> Sent: 2019??5??10?? 11:14
> To: Andy Tang <[email protected]>
> Cc: Leo Li <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more thermal
> zone node
>
> Caution: EXT Email
>
> On Tue, Apr 23, 2019 at 10:25:07AM +0800, Yuantian Tang wrote:
> > Ls1088a has 2 thermal sensors, core cluster and SoC platform. Core
> > cluster sensor is used to monitor the temperature of core and SoC
> > platform is for platform. The current dts only support the first sensor.
> > This patch adds the second sensor node to dts to enable it.
> >
> > Signed-off-by: Yuantian Tang <[email protected]>
> > ---
> > v6:
> > - add cooling device map to cpu0-7 in platform node.
I like to explain a little. I think it makes sense that multiple thermal zone map to same cooling device.
In this way, no matter which thermal zone raises a temp alarm, it can call cooling device to chill out.
I also asked cpufreq maintainer about the cooling map issue, he think it would be fine.
I have tested and no issue found.

Daniel, what's your thought?

Thanks,
Andy
>
> @Daniel, are you fine with this version?
>
> Shawn


Attachments:
(No filename) (8.85 kB)

2019-05-10 07:20:07

by Daniel Lezcano

[permalink] [raw]
Subject: Re: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more thermal zone node

On 10/05/2019 05:40, Andy Tang wrote:
>> -----Original Message-----
>> From: Shawn Guo <[email protected]>
>> Sent: 2019??5??10?? 11:14
>> To: Andy Tang <[email protected]>
>> Cc: Leo Li <[email protected]>; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]
>> Subject: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more thermal
>> zone node
>>
>> Caution: EXT Email
>>
>> On Tue, Apr 23, 2019 at 10:25:07AM +0800, Yuantian Tang wrote:
>>> Ls1088a has 2 thermal sensors, core cluster and SoC platform. Core
>>> cluster sensor is used to monitor the temperature of core and SoC
>>> platform is for platform. The current dts only support the first sensor.
>>> This patch adds the second sensor node to dts to enable it.
>>>
>>> Signed-off-by: Yuantian Tang <[email protected]>
>>> ---
>>> v6:
>>> - add cooling device map to cpu0-7 in platform node.
> I like to explain a little. I think it makes sense that multiple thermal zone map to same cooling device.
> In this way, no matter which thermal zone raises a temp alarm, it can call cooling device to chill out.
> I also asked cpufreq maintainer about the cooling map issue, he think it would be fine.
> I have tested and no issue found.
>
> Daniel, what's your thought?

If there are multiple thermal zones, they will be managed by different
instances of a thermal governor. Each instances will act on the shared
cooling device and will collide in their decisions:

- If the sensors are closed, their behavior will be similar regarding
the temperature. The governors may take the same decision for the
cooling device. But in such case having just one thermal zone managed is
enough.

- If the sensors are not closed, their behavior will be different
regarding the temperature. The governors will take different decision
regarding the cooling device (one will decrease the freq, other will
increase the freq).

As the thermal governors are not able to manage several thermal zones
and there is one cooling device (the cpu cooling device), this setup
won't work as expected IMO.

The setup making sense is having a thermal zone per 'cluster' and a
cooling device per 'cluster'. That means the platform has one clock line
per 'cluster'. The thermal management happens in a self-contained
thermal zone (one cooling device - one governor - one thermal zone).

In the case of HMP, other combinations are possible to be optimal.



--
<http://www.linaro.org/> Linaro.org ?? Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

2019-05-10 08:52:20

by Andy Tang

[permalink] [raw]
Subject: RE: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more thermal zone node

+ Viresh for help.

> -----Original Message-----
> From: Daniel Lezcano <[email protected]>
> Sent: 2019??5??10?? 15:17
> To: Andy Tang <[email protected]>; Shawn Guo <[email protected]>
> Cc: Leo Li <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: Re: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more thermal
> zone node
>
> Caution: EXT Email
>
> On 10/05/2019 05:40, Andy Tang wrote:
> >> -----Original Message-----
> >> From: Shawn Guo <[email protected]>
> >> Sent: 2019??5??10?? 11:14
> >> To: Andy Tang <[email protected]>
> >> Cc: Leo Li <[email protected]>; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected]
> >> Subject: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more
> >> thermal zone node
> >>
> >> Caution: EXT Email
> >>
> >> On Tue, Apr 23, 2019 at 10:25:07AM +0800, Yuantian Tang wrote:
> >>> Ls1088a has 2 thermal sensors, core cluster and SoC platform. Core
> >>> cluster sensor is used to monitor the temperature of core and SoC
> >>> platform is for platform. The current dts only support the first sensor.
> >>> This patch adds the second sensor node to dts to enable it.
> >>>
> >>> Signed-off-by: Yuantian Tang <[email protected]>
> >>> ---
> >>> v6:
> >>> - add cooling device map to cpu0-7 in platform node.
> > I like to explain a little. I think it makes sense that multiple thermal zone
> map to same cooling device.
> > In this way, no matter which thermal zone raises a temp alarm, it can call
> cooling device to chill out.
> > I also asked cpufreq maintainer about the cooling map issue, he think it
> would be fine.
> > I have tested and no issue found.
> >
> > Daniel, what's your thought?
>
> If there are multiple thermal zones, they will be managed by different
> instances of a thermal governor. Each instances will act on the shared cooling
> device and will collide in their decisions:
>
> - If the sensors are closed, their behavior will be similar regarding the
> temperature. The governors may take the same decision for the cooling
> device. But in such case having just one thermal zone managed is enough.
>
> - If the sensors are not closed, their behavior will be different regarding the
> temperature. The governors will take different decision regarding the cooling
> device (one will decrease the freq, other will increase the freq).
>
> As the thermal governors are not able to manage several thermal zones and
> there is one cooling device (the cpu cooling device), this setup won't work as
> expected IMO.
>
> The setup making sense is having a thermal zone per 'cluster' and a cooling
> device per 'cluster'. That means the platform has one clock line per 'cluster'.
> The thermal management happens in a self-contained thermal zone (one
> cooling device - one governor - one thermal zone).
>
> In the case of HMP, other combinations are possible to be optimal.
Hi Viresh,

I want to map multiple thermal zones to the same cooling device. The above is the discussion about it.
It seems reasonable. But I am not expert on this. Could you please provide some thoughts? Thanks.

BR,
Andy
>
>
>
> --
>
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> linaro.org%2F&amp;data=02%7C01%7Candy.tang%40nxp.com%7C935b7a08
> 31cc466da40808d6d5176f8e%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
> %7C0%7C636930693965540189&amp;sdata=WK9NDBVwzhCC9WMkmjLObJ
> OBQwRG%2Fboed%2FKx18xiBNQ%3D&amp;reserved=0> Linaro.org ?? Open
> source software for ARM SoCs
>
> Follow Linaro:
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> facebook.com%2Fpages%2FLinaro&amp;data=02%7C01%7Candy.tang%40nx
> p.com%7C935b7a0831cc466da40808d6d5176f8e%7C686ea1d3bc2b4c6fa92c
> d99c5c301635%7C0%7C0%7C636930693965550202&amp;sdata=O0mWO76
> 9sK2ZMGX9AxgLGYNVfkFHBD4ZIGCclttvyPI%3D&amp;reserved=0>
> Facebook |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ftwitte
> r.com%2F%23!%2Flinaroorg&amp;data=02%7C01%7Candy.tang%40nxp.com
> %7C935b7a0831cc466da40808d6d5176f8e%7C686ea1d3bc2b4c6fa92cd99c5
> c301635%7C0%7C0%7C636930693965550202&amp;sdata=tV%2Bi8Bk3OqO
> h%2FZHpBr2NQDACVvtGi8KNGQt5dZaTeyg%3D&amp;reserved=0> Twitter |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> linaro.org%2Flinaro-blog%2F&amp;data=02%7C01%7Candy.tang%40nxp.co
> m%7C935b7a0831cc466da40808d6d5176f8e%7C686ea1d3bc2b4c6fa92cd99
> c5c301635%7C0%7C0%7C636930693965550202&amp;sdata=0E8a938xEt7l2
> MBLnMETsCQKfhJMgmzFNtuCKPXf5SY%3D&amp;reserved=0> Blog

2019-05-10 10:15:08

by Viresh Kumar

[permalink] [raw]
Subject: Re: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more thermal zone node

On 10-05-19, 08:47, Andy Tang wrote:
> + Viresh for help.
>
> > -----Original Message-----
> > From: Daniel Lezcano <[email protected]>
> > Sent: 2019年5月10日 15:17
> > To: Andy Tang <[email protected]>; Shawn Guo <[email protected]>
> > Cc: Leo Li <[email protected]>; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected]
> > Subject: Re: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more thermal
> > zone node
> >
> > Caution: EXT Email
> >
> > On 10/05/2019 05:40, Andy Tang wrote:
> > >> -----Original Message-----
> > >> From: Shawn Guo <[email protected]>
> > >> Sent: 2019年5月10日 11:14
> > >> To: Andy Tang <[email protected]>
> > >> Cc: Leo Li <[email protected]>; [email protected];
> > >> [email protected]; [email protected];
> > >> [email protected]; [email protected];
> > >> [email protected]; [email protected];
> > >> [email protected]; [email protected]
> > >> Subject: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more
> > >> thermal zone node
> > >>
> > >> Caution: EXT Email
> > >>
> > >> On Tue, Apr 23, 2019 at 10:25:07AM +0800, Yuantian Tang wrote:
> > >>> Ls1088a has 2 thermal sensors, core cluster and SoC platform. Core
> > >>> cluster sensor is used to monitor the temperature of core and SoC
> > >>> platform is for platform. The current dts only support the first sensor.
> > >>> This patch adds the second sensor node to dts to enable it.
> > >>>
> > >>> Signed-off-by: Yuantian Tang <[email protected]>
> > >>> ---
> > >>> v6:
> > >>> - add cooling device map to cpu0-7 in platform node.
> > > I like to explain a little. I think it makes sense that multiple thermal zone
> > map to same cooling device.
> > > In this way, no matter which thermal zone raises a temp alarm, it can call
> > cooling device to chill out.
> > > I also asked cpufreq maintainer about the cooling map issue, he think it
> > would be fine.

Yes, you asked me and I said it should be okay.

> > > I have tested and no issue found.
> > >
> > > Daniel, what's your thought?
> >
> > If there are multiple thermal zones, they will be managed by different
> > instances of a thermal governor. Each instances will act on the shared cooling
> > device and will collide in their decisions:
> >
> > - If the sensors are closed, their behavior will be similar regarding the
> > temperature. The governors may take the same decision for the cooling
> > device. But in such case having just one thermal zone managed is enough.
> >
> > - If the sensors are not closed, their behavior will be different regarding the
> > temperature. The governors will take different decision regarding the cooling
> > device (one will decrease the freq, other will increase the freq).
> >
> > As the thermal governors are not able to manage several thermal zones and
> > there is one cooling device (the cpu cooling device), this setup won't work as
> > expected IMO.
> >
> > The setup making sense is having a thermal zone per 'cluster' and a cooling
> > device per 'cluster'. That means the platform has one clock line per 'cluster'.
> > The thermal management happens in a self-contained thermal zone (one
> > cooling device - one governor - one thermal zone).
> >
> > In the case of HMP, other combinations are possible to be optimal.

But not sure how I missed the obvious, though I do remember thinking about this.

So the problem is that the cpu_cooling driver will get requests in parallel to
set different max frequencies and the last call will always win and may result
in undesired outcome.

Sorry about creating the confusion.

--
viresh

2019-05-11 04:06:01

by Andy Tang

[permalink] [raw]
Subject: RE: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more thermal zone node

Thanks Viresh for your explanation.

BR,
Andy
> -----Original Message-----
> From: Viresh Kumar <[email protected]>
> Sent: 2019年5月10日 18:12
> To: Andy Tang <[email protected]>
> Cc: Daniel Lezcano <[email protected]>; Shawn Guo
> <[email protected]>; Leo Li <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: Re: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more thermal
> zone node
>
> Caution: EXT Email
>
> On 10-05-19, 08:47, Andy Tang wrote:
> > + Viresh for help.
> >
> > > -----Original Message-----
> > > From: Daniel Lezcano <[email protected]>
> > > Sent: 2019年5月10日 15:17
> > > To: Andy Tang <[email protected]>; Shawn Guo
> <[email protected]>
> > > Cc: Leo Li <[email protected]>; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected]; [email protected]
> > > Subject: Re: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more
> > > thermal zone node
> > >
> > > Caution: EXT Email
> > >
> > > On 10/05/2019 05:40, Andy Tang wrote:
> > > >> -----Original Message-----
> > > >> From: Shawn Guo <[email protected]>
> > > >> Sent: 2019年5月10日 11:14
> > > >> To: Andy Tang <[email protected]>
> > > >> Cc: Leo Li <[email protected]>; [email protected];
> > > >> [email protected]; [email protected];
> > > >> [email protected]; [email protected];
> > > >> [email protected]; [email protected];
> > > >> [email protected]; [email protected]
> > > >> Subject: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more
> > > >> thermal zone node
> > > >>
> > > >> Caution: EXT Email
> > > >>
> > > >> On Tue, Apr 23, 2019 at 10:25:07AM +0800, Yuantian Tang wrote:
> > > >>> Ls1088a has 2 thermal sensors, core cluster and SoC platform.
> > > >>> Core cluster sensor is used to monitor the temperature of core
> > > >>> and SoC platform is for platform. The current dts only support the first
> sensor.
> > > >>> This patch adds the second sensor node to dts to enable it.
> > > >>>
> > > >>> Signed-off-by: Yuantian Tang <[email protected]>
> > > >>> ---
> > > >>> v6:
> > > >>> - add cooling device map to cpu0-7 in platform node.
> > > > I like to explain a little. I think it makes sense that multiple
> > > > thermal zone
> > > map to same cooling device.
> > > > In this way, no matter which thermal zone raises a temp alarm, it
> > > > can call
> > > cooling device to chill out.
> > > > I also asked cpufreq maintainer about the cooling map issue, he
> > > > think it
> > > would be fine.
>
> Yes, you asked me and I said it should be okay.
>
> > > > I have tested and no issue found.
> > > >
> > > > Daniel, what's your thought?
> > >
> > > If there are multiple thermal zones, they will be managed by
> > > different instances of a thermal governor. Each instances will act
> > > on the shared cooling device and will collide in their decisions:
> > >
> > > - If the sensors are closed, their behavior will be similar
> > > regarding the temperature. The governors may take the same decision
> > > for the cooling device. But in such case having just one thermal zone
> managed is enough.
> > >
> > > - If the sensors are not closed, their behavior will be different
> > > regarding the temperature. The governors will take different
> > > decision regarding the cooling device (one will decrease the freq, other
> will increase the freq).
> > >
> > > As the thermal governors are not able to manage several thermal
> > > zones and there is one cooling device (the cpu cooling device), this
> > > setup won't work as expected IMO.
> > >
> > > The setup making sense is having a thermal zone per 'cluster' and a
> > > cooling device per 'cluster'. That means the platform has one clock line
> per 'cluster'.
> > > The thermal management happens in a self-contained thermal zone (one
> > > cooling device - one governor - one thermal zone).
> > >
> > > In the case of HMP, other combinations are possible to be optimal.
>
> But not sure how I missed the obvious, though I do remember thinking about
> this.
>
> So the problem is that the cpu_cooling driver will get requests in parallel to
> set different max frequencies and the last call will always win and may result
> in undesired outcome.
>
> Sorry about creating the confusion.
>
> --
> viresh