2019-05-30 06:29:15

by Hsin-Yi Wang

[permalink] [raw]
Subject: Re: [PATCH v2 1/8] arm64: dts: mt8183: add thermal zone node

On Fri, May 10, 2019 at 9:27 PM michael.kao <[email protected]> wrote:

> +
> + tzts1: tzts1 {
> + polling-delay-passive = <0>;
> + polling-delay = <0>;
> + thermal-sensors = <&thermal 1>;
> + sustainable-power = <0>;
> + trips {};
> + cooling-maps {};
> + };
> +
Is 0 a valid initial sustainable-power setting? Since we'll still get
warning[1] about this, though it might not be harmful.

If 0 is a valid setting, maybe we should consider showing the warning
of not setting this property in [2]?

[1] https://elixir.bootlin.com/linux/latest/source/drivers/thermal/power_allocator.c#L570
[2] https://elixir.bootlin.com/linux/latest/source/drivers/thermal/of-thermal.c#L1049


2019-05-30 16:10:00

by Matthias Kaehlcke

[permalink] [raw]
Subject: Re: [PATCH v2 1/8] arm64: dts: mt8183: add thermal zone node

On Thu, May 30, 2019 at 02:27:28PM +0800, Hsin-Yi Wang wrote:
> On Fri, May 10, 2019 at 9:27 PM michael.kao <[email protected]> wrote:
>
> > +
> > + tzts1: tzts1 {
> > + polling-delay-passive = <0>;
> > + polling-delay = <0>;
> > + thermal-sensors = <&thermal 1>;
> > + sustainable-power = <0>;
> > + trips {};
> > + cooling-maps {};
> > + };
> > +
> Is 0 a valid initial sustainable-power setting? Since we'll still get
> warning[1] about this, though it might not be harmful.
>
> If 0 is a valid setting, maybe we should consider showing the warning
> of not setting this property in [2]?
>
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/thermal/power_allocator.c#L570
> [2] https://elixir.bootlin.com/linux/latest/source/drivers/thermal/of-thermal.c#L1049

IIUC a value of 0 is pointless, the thermal framework will still use
an estimated value:

https://elixir.bootlin.com/linux/v5.1.5/source/drivers/thermal/power_allocator.c#L203

As commented on v1 (https://patchwork.kernel.org/patch/10926519/#22620905)
the value of the property may depend on the thermal characteristics of
the device, there is not one correct value per SoC/core. If it is
specified at SoC level device makers should be aware that they might
have to override it for 'optimal' behavior on their device.

2019-12-06 09:48:09

by Michael Kao

[permalink] [raw]
Subject: RE: [PATCH v2 1/8] arm64: dts: mt8183: add thermal zone node



-----Original Message-----
From: Matthias Kaehlcke [mailto:[email protected]]
Sent: Friday, May 31, 2019 12:08 AM
To: Hsin-Yi Wang <[email protected]>
Cc: Michael Kao (高振翔) <[email protected]>; Zhang Rui <[email protected]>; Eduardo Valentin <[email protected]>; Daniel Lezcano <[email protected]>; Rob Herring <[email protected]>; Mark Rutland <[email protected]>; Matthias Brugger <[email protected]>; [email protected]; [email protected]; lkml <[email protected]>; moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE <[email protected]>; [email protected]
Subject: Re: [PATCH v2 1/8] arm64: dts: mt8183: add thermal zone node

On Thu, May 30, 2019 at 02:27:28PM +0800, Hsin-Yi Wang wrote:
> On Fri, May 10, 2019 at 9:27 PM michael.kao <[email protected]> wrote:
>
> > +
> > + tzts1: tzts1 {
> > + polling-delay-passive = <0>;
> > + polling-delay = <0>;
> > + thermal-sensors = <&thermal 1>;
> > + sustainable-power = <0>;
> > + trips {};
> > + cooling-maps {};
> > + };
> > +
> Is 0 a valid initial sustainable-power setting? Since we'll still get
> warning[1] about this, though it might not be harmful.
>
> If 0 is a valid setting, maybe we should consider showing the warning
> of not setting this property in [2]?
>
> [1]
> https://elixir.bootlin.com/linux/latest/source/drivers/thermal/power_a
> llocator.c#L570 [2]
> https://elixir.bootlin.com/linux/latest/source/drivers/thermal/of-ther
> mal.c#L1049

IIUC a value of 0 is pointless, the thermal framework will still use an estimated value:

https://elixir.bootlin.com/linux/v5.1.5/source/drivers/thermal/power_allocator.c#L203

As commented on v1 (https://patchwork.kernel.org/patch/10926519/#22620905)
the value of the property may depend on the thermal characteristics of the device, there is not one correct value per SoC/core. If it is specified at SoC level device makers should be aware that they might have to override it for 'optimal' behavior on their device.

I think there is no need to set sustainable-power for tzts1~6.
They don't bind thermal instance and will not thermal throttle.
So it will be also zero if we don't set sustainable-power and let it use estimate value.
For the purpose of preventing warning, I think I can set sustainable-power of tzts1~6 the same to cpu_thermal.