[PATCH v1] thermal/of: Introduce k-po, k-pu and k-i for a thermal zone

[PATCH v1] thermal/of: Introduce k-po, k-pu and k-i for a thermal zone

[Finley Xiao]

The default value for k_pu is:
    2 * sustainable_power / (desired_temperature - switch_on_temp)
The default value for k_po is:
    sustainable_power / (desired_temperature - switch_on_temp)
The default value for k_i is 10.

Even though these parameters of the PID controller can be changed
by the following sysfs files:
    /sys/class/thermal/thermal_zoneX/k_pu
    /sys/class/thermal/thermal_zoneX/k_po
    /sys/class/thermal/thermal_zoneX/k_i

But it's still more convenient to change the default values by devicetree,
so introduce these three optional properties. If provided these properties,
they will be parsed and associated with the thermal zone via the thermal
zone parameters.

Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
---
 Documentation/devicetree/bindings/thermal/thermal.txt | 14 ++++++++++++++
 drivers/thermal/thermal_of.c                          |  7 +++++++
 2 files changed, 21 insertions(+)

diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
index f78bec19ca35..ebe936b57ded 100644
--- a/Documentation/devicetree/bindings/thermal/thermal.txt
+++ b/Documentation/devicetree/bindings/thermal/thermal.txt
@@ -165,6 +165,20 @@ Optional property:
 			2000mW, while on a 10'' tablet is around
 			4500mW.
 
+- k-po:			Proportional parameter of the PID controller when
+			current temperature is above the target.
+  Type: signed
+  Size: one cell
+
+- k-pu:			Proportional parameter of the PID controller when
+			current temperature is below the target.
+  Type: signed
+  Size: one cell
+
+- k-i:			Integral parameter of the PID controller.
+  Type: signed
+  Size: one cell
+
 Note: The delay properties are bound to the maximum dT/dt (temperature
 derivative over time) in two situations for a thermal zone:
 (i)  - when passive cooling is activated (polling-delay-passive); and
diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
index ddf88dbe7ba2..b2a9f92cd8d2 100644
--- a/drivers/thermal/thermal_of.c
+++ b/drivers/thermal/thermal_of.c
@@ -1089,6 +1089,7 @@ int __init of_parse_thermal_zones(void)
 		struct thermal_zone_params *tzp;
 		int i, mask = 0;
 		u32 prop;
+		s32 sval;
 
 		tz = thermal_of_build_thermal_zone(child);
 		if (IS_ERR(tz)) {
@@ -1113,6 +1114,12 @@ int __init of_parse_thermal_zones(void)
 
 		if (!of_property_read_u32(child, "sustainable-power", &prop))
 			tzp->sustainable_power = prop;
+		if (!of_property_read_s32(child, "k-po", &sval))
+			tzp->k_po = sval;
+		if (!of_property_read_s32(child, "k-pu", &sval))
+			tzp->k_pu = sval;
+		if (!of_property_read_s32(child, "k-i", &sval))
+			tzp->k_i = sval;
 
 		for (i = 0; i < tz->ntrips; i++)
 			mask |= 1 << i;
-- 
2.11.0

Re: [PATCH v1] thermal/of: Introduce k-po, k-pu and k-i for a thermal zone

[Rob Herring]

On Tue, Aug 11, 2020 at 08:31:15PM +0800, Finley Xiao wrote:
> The default value for k_pu is:
>     2 * sustainable_power / (desired_temperature - switch_on_temp)
> The default value for k_po is:
>     sustainable_power / (desired_temperature - switch_on_temp)
> The default value for k_i is 10.
> 
> Even though these parameters of the PID controller can be changed
> by the following sysfs files:
>     /sys/class/thermal/thermal_zoneX/k_pu
>     /sys/class/thermal/thermal_zoneX/k_po
>     /sys/class/thermal/thermal_zoneX/k_i
> 
> But it's still more convenient to change the default values by devicetree,
> so introduce these three optional properties. If provided these properties,
> they will be parsed and associated with the thermal zone via the thermal
> zone parameters.
> 
> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
> ---
>  Documentation/devicetree/bindings/thermal/thermal.txt | 14 ++++++++++++++

Bindings should be a separate file and this one is a DT schema now.

>  drivers/thermal/thermal_of.c                          |  7 +++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
> index f78bec19ca35..ebe936b57ded 100644
> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> @@ -165,6 +165,20 @@ Optional property:
>  			2000mW, while on a 10'' tablet is around
>  			4500mW.
>  
> +- k-po:			Proportional parameter of the PID controller when
> +			current temperature is above the target.
> +  Type: signed
> +  Size: one cell
> +
> +- k-pu:			Proportional parameter of the PID controller when
> +			current temperature is below the target.
> +  Type: signed
> +  Size: one cell
> +
> +- k-i:			Integral parameter of the PID controller.
> +  Type: signed
> +  Size: one cell

What's PID?

I know nothing about the sysfs params, but the binding needs to stand on 
it's own and needs enough detail to educate me.

Rob

Re: [PATCH v1] thermal/of: Introduce k-po, k-pu and k-i for a thermal zone

[Lukasz Luba]

Hi Rob,

On 8/25/20 12:09 AM, Rob Herring wrote:
> On Tue, Aug 11, 2020 at 08:31:15PM +0800, Finley Xiao wrote:
>> The default value for k_pu is:
>>      2 * sustainable_power / (desired_temperature - switch_on_temp)
>> The default value for k_po is:
>>      sustainable_power / (desired_temperature - switch_on_temp)
>> The default value for k_i is 10.
>>
>> Even though these parameters of the PID controller can be changed
>> by the following sysfs files:
>>      /sys/class/thermal/thermal_zoneX/k_pu
>>      /sys/class/thermal/thermal_zoneX/k_po
>>      /sys/class/thermal/thermal_zoneX/k_i
>>
>> But it's still more convenient to change the default values by devicetree,
>> so introduce these three optional properties. If provided these properties,
>> they will be parsed and associated with the thermal zone via the thermal
>> zone parameters.
>>
>> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
>> ---
>>   Documentation/devicetree/bindings/thermal/thermal.txt | 14 ++++++++++++++
> 
> Bindings should be a separate file and this one is a DT schema now.
> 
>>   drivers/thermal/thermal_of.c                          |  7 +++++++
>>   2 files changed, 21 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
>> index f78bec19ca35..ebe936b57ded 100644
>> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
>> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
>> @@ -165,6 +165,20 @@ Optional property:
>>   			2000mW, while on a 10'' tablet is around
>>   			4500mW.
>>   
>> +- k-po:			Proportional parameter of the PID controller when
>> +			current temperature is above the target.
>> +  Type: signed
>> +  Size: one cell
>> +
>> +- k-pu:			Proportional parameter of the PID controller when
>> +			current temperature is below the target.
>> +  Type: signed
>> +  Size: one cell
>> +
>> +- k-i:			Integral parameter of the PID controller.
>> +  Type: signed
>> +  Size: one cell
> 
> What's PID?
> 
> I know nothing about the sysfs params, but the binding needs to stand on
> it's own and needs enough detail to educate me.
> 

Sorry for the delay, I missed that patch.
These parameters are the coefficients for the
Proportional-Integral-Derivative (PID) controller [1], which is the
core of the Intelligent Power Allocation (IPA) thermal governor.

Only IPA uses them, thus I don't think the governors parameters:
k-po, k-pu, k-i
should be part of the DeviceTree. I haven't seen such governors
tunnables in the DT, please point me if they exist somewhere.

Do you think Rob they might be specified in the DT?

Regards,
Lukasz

[1] https://en.wikipedia.org/wiki/PID_controller

Re: [PATCH v1] thermal/of: Introduce k-po, k-pu and k-i for a thermal zone

[Daniel Lezcano]

On 25/08/2020 10:25, Lukasz Luba wrote:
> Hi Rob,
> 
> On 8/25/20 12:09 AM, Rob Herring wrote:
>> On Tue, Aug 11, 2020 at 08:31:15PM +0800, Finley Xiao wrote:
>>> The default value for k_pu is:
>>>      2 * sustainable_power / (desired_temperature - switch_on_temp)
>>> The default value for k_po is:
>>>      sustainable_power / (desired_temperature - switch_on_temp)
>>> The default value for k_i is 10.
>>>
>>> Even though these parameters of the PID controller can be changed
>>> by the following sysfs files:
>>>      /sys/class/thermal/thermal_zoneX/k_pu
>>>      /sys/class/thermal/thermal_zoneX/k_po
>>>      /sys/class/thermal/thermal_zoneX/k_i
>>>
>>> But it's still more convenient to change the default values by
>>> devicetree,
>>> so introduce these three optional properties. If provided these
>>> properties,
>>> they will be parsed and associated with the thermal zone via the thermal
>>> zone parameters.
>>>
>>> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
>>> ---
>>>   Documentation/devicetree/bindings/thermal/thermal.txt | 14
>>> ++++++++++++++
>>
>> Bindings should be a separate file and this one is a DT schema now.
>>
>>>   drivers/thermal/thermal_of.c                          |  7 +++++++
>>>   2 files changed, 21 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt
>>> b/Documentation/devicetree/bindings/thermal/thermal.txt
>>> index f78bec19ca35..ebe936b57ded 100644
>>> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
>>> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
>>> @@ -165,6 +165,20 @@ Optional property:
>>>               2000mW, while on a 10'' tablet is around
>>>               4500mW.
>>>   +- k-po:            Proportional parameter of the PID controller when
>>> +            current temperature is above the target.
>>> +  Type: signed
>>> +  Size: one cell
>>> +
>>> +- k-pu:            Proportional parameter of the PID controller when
>>> +            current temperature is below the target.
>>> +  Type: signed
>>> +  Size: one cell
>>> +
>>> +- k-i:            Integral parameter of the PID controller.
>>> +  Type: signed
>>> +  Size: one cell
>>
>> What's PID?
>>
>> I know nothing about the sysfs params, but the binding needs to stand on
>> it's own and needs enough detail to educate me.
> Sorry for the delay, I missed that patch.
> These parameters are the coefficients for the
> Proportional-Integral-Derivative (PID) controller [1], which is the
> core of the Intelligent Power Allocation (IPA) thermal governor.

Just a few words to elaborate a bit for Rob who may not have time to
digest the whole concept from Wikipedia :)

The PID is an regulation loop where the input is compared to the output.

For example when driving a car and you aim a speed cruise of 90km/h. You
press the accelerator and watch the current speed. The smaller the
current speed is, the stronger you will push the accelerator. And the
closer to the cruise speed the car is, the lesser you push the
accelerator until the car stabilize to the cruise to speed.

The k-* describes how strong you push the accelerator and release it.

In the thermal framework, that has an impact on how brutal the
mitigation acts and depending on them it results in a flat temperature
curve or a sawtooth aspect.

These coefficient depends on the ambient temperature (casing, room
temperature), the heat sink and the load. Depending on the use cases,
you may want to change their values at runtime.

From my POV, setting these values in the DT does not really make sense.

It would make much more sense to have the thermal specifications of the
board (heat sink capacity, resistivity, max temperature or TDP), so from
there we should be able to compute anything related to the thermal
profile and configure from the kernel.


> Only IPA uses them, thus I don't think the governors parameters:
> k-po, k-pu, k-i
> should be part of the DeviceTree. I haven't seen such governors
> tunnables in the DT, please point me if they exist somewhere.
> 
> Do you think Rob they might be specified in the DT?
> 
> Regards,
> Lukasz
> 
> [1] https://en.wikipedia.org/wiki/PID_controller


-- 
<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

Re: [PATCH v1] thermal/of: Introduce k-po, k-pu and k-i for a thermal zone

[Rob Herring]

On Tue, Aug 25, 2020 at 3:44 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 25/08/2020 10:25, Lukasz Luba wrote:
> > Hi Rob,
> >
> > On 8/25/20 12:09 AM, Rob Herring wrote:
> >> On Tue, Aug 11, 2020 at 08:31:15PM +0800, Finley Xiao wrote:
> >>> The default value for k_pu is:
> >>>      2 * sustainable_power / (desired_temperature - switch_on_temp)
> >>> The default value for k_po is:
> >>>      sustainable_power / (desired_temperature - switch_on_temp)
> >>> The default value for k_i is 10.
> >>>
> >>> Even though these parameters of the PID controller can be changed
> >>> by the following sysfs files:
> >>>      /sys/class/thermal/thermal_zoneX/k_pu
> >>>      /sys/class/thermal/thermal_zoneX/k_po
> >>>      /sys/class/thermal/thermal_zoneX/k_i
> >>>
> >>> But it's still more convenient to change the default values by
> >>> devicetree,
> >>> so introduce these three optional properties. If provided these
> >>> properties,
> >>> they will be parsed and associated with the thermal zone via the thermal
> >>> zone parameters.
> >>>
> >>> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
> >>> ---
> >>>   Documentation/devicetree/bindings/thermal/thermal.txt | 14
> >>> ++++++++++++++
> >>
> >> Bindings should be a separate file and this one is a DT schema now.
> >>
> >>>   drivers/thermal/thermal_of.c                          |  7 +++++++
> >>>   2 files changed, 21 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt
> >>> b/Documentation/devicetree/bindings/thermal/thermal.txt
> >>> index f78bec19ca35..ebe936b57ded 100644
> >>> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> >>> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> >>> @@ -165,6 +165,20 @@ Optional property:
> >>>               2000mW, while on a 10'' tablet is around
> >>>               4500mW.
> >>>   +- k-po:            Proportional parameter of the PID controller when
> >>> +            current temperature is above the target.
> >>> +  Type: signed
> >>> +  Size: one cell
> >>> +
> >>> +- k-pu:            Proportional parameter of the PID controller when
> >>> +            current temperature is below the target.
> >>> +  Type: signed
> >>> +  Size: one cell
> >>> +
> >>> +- k-i:            Integral parameter of the PID controller.
> >>> +  Type: signed
> >>> +  Size: one cell
> >>
> >> What's PID?
> >>
> >> I know nothing about the sysfs params, but the binding needs to stand on
> >> it's own and needs enough detail to educate me.
> > Sorry for the delay, I missed that patch.
> > These parameters are the coefficients for the
> > Proportional-Integral-Derivative (PID) controller [1], which is the
> > core of the Intelligent Power Allocation (IPA) thermal governor.
>
> Just a few words to elaborate a bit for Rob who may not have time to
> digest the whole concept from Wikipedia :)
>
> The PID is an regulation loop where the input is compared to the output.
>
> For example when driving a car and you aim a speed cruise of 90km/h. You
> press the accelerator and watch the current speed. The smaller the
> current speed is, the stronger you will push the accelerator. And the
> closer to the cruise speed the car is, the lesser you push the
> accelerator until the car stabilize to the cruise to speed.
>
> The k-* describes how strong you push the accelerator and release it.
>
> In the thermal framework, that has an impact on how brutal the
> mitigation acts and depending on them it results in a flat temperature
> curve or a sawtooth aspect.
>
> These coefficient depends on the ambient temperature (casing, room
> temperature), the heat sink and the load. Depending on the use cases,
> you may want to change their values at runtime.
>
> From my POV, setting these values in the DT does not really make sense.

I pretty much always agree with not putting things in DT. :)

Rob