Subject: [PATCH] thermal: of: Introduce governor selection in dts

As there could be more thermal zones on a system and
more variety in thermal governors provided in kernel,
this patch provides flexibility of governor selection
for a thermal zone declared in device tree.

Change-Id: Ie4a75d762709cbbe9f1806dae325d13f71982e78
Signed-off-by: Chung-yih Wang <[email protected]>
---
Documentation/devicetree/bindings/thermal/thermal.txt | 9 +++++++++
drivers/thermal/of-thermal.c | 10 ++++++++++
2 files changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt
b/Documentation/devicetree/bindings/thermal/thermal.txt
index 8a49362..30a5d41 100644
--- a/Documentation/devicetree/bindings/thermal/thermal.txt
+++ b/Documentation/devicetree/bindings/thermal/thermal.txt
@@ -174,6 +174,13 @@ Optional property:
2000mW, while on a 10'' tablet is around
4500mW.

+- thermal-governor-name: The name of governor used to control the
+ thermal zone instead of the default one
+ specified in kernel config. For reference, if
+ default governor is step_wise, one could
+ select power_allocator for cpu_thermal zone
+ in dts.
+
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
@@ -555,6 +562,8 @@ thermal-zones {

sustainable-power = <2500>;

+ thermal-governor-name = "power_allocator";
+
trips {
/* Trips are based on resulting linear equation */
cpu_trip: cpu-trip {
diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index b295b2b..45570ac 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -882,6 +882,7 @@ int __init of_parse_thermal_zones(void)
}

for_each_child_of_node(np, child) {
+ const char *governor_name;
struct thermal_zone_device *zone;
struct thermal_zone_params *tzp;
int i, mask = 0;
@@ -909,6 +910,15 @@ int __init of_parse_thermal_zones(void)
goto exit_free;
}

+ /* Select a preferred governor if declared */
+ if (!of_property_read_string(child,
+ "thermal-governor-name",
+ &governor_name)) {
+ strncpy(tzp->governor_name,
+ governor_name,
+ sizeof(tzp->governor_name) - 1);
+ }
+
/* No hwmon because there might be hwmon drivers registering */
tzp->no_hwmon = true;

--
2.1.2


2015-08-07 07:18:45

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH] thermal: of: Introduce governor selection in dts

2015-08-07 16:09 GMT+09:00 Chung-Yih Wang (王崇懿) <[email protected]>:
> As there could be more thermal zones on a system and
> more variety in thermal governors provided in kernel,
> this patch provides flexibility of governor selection
> for a thermal zone declared in device tree.

How is this a property of a hardware, of a board?

>
> Change-Id: Ie4a75d762709cbbe9f1806dae325d13f71982e78

This tag should not go to upstream.

Best regards,
Krzysztof


> Signed-off-by: Chung-yih Wang <[email protected]>
> ---
> Documentation/devicetree/bindings/thermal/thermal.txt | 9 +++++++++
> drivers/thermal/of-thermal.c | 10 ++++++++++
> 2 files changed, 19 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt
> b/Documentation/devicetree/bindings/thermal/thermal.txt
> index 8a49362..30a5d41 100644
> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> @@ -174,6 +174,13 @@ Optional property:
> 2000mW, while on a 10'' tablet is around
> 4500mW.
>
> +- thermal-governor-name: The name of governor used to control the
> + thermal zone instead of the default one
> + specified in kernel config. For reference, if
> + default governor is step_wise, one could
> + select power_allocator for cpu_thermal zone
> + in dts.
> +
> 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
> @@ -555,6 +562,8 @@ thermal-zones {
>
> sustainable-power = <2500>;
>
> + thermal-governor-name = "power_allocator";
> +
> trips {
> /* Trips are based on resulting linear equation */
> cpu_trip: cpu-trip {
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index b295b2b..45570ac 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -882,6 +882,7 @@ int __init of_parse_thermal_zones(void)
> }
>
> for_each_child_of_node(np, child) {
> + const char *governor_name;
> struct thermal_zone_device *zone;
> struct thermal_zone_params *tzp;
> int i, mask = 0;
> @@ -909,6 +910,15 @@ int __init of_parse_thermal_zones(void)
> goto exit_free;
> }
>
> + /* Select a preferred governor if declared */
> + if (!of_property_read_string(child,
> + "thermal-governor-name",
> + &governor_name)) {
> + strncpy(tzp->governor_name,
> + governor_name,
> + sizeof(tzp->governor_name) - 1);
> + }
> +
> /* No hwmon because there might be hwmon drivers registering */
> tzp->no_hwmon = true;
>
> --
> 2.1.2
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2015-08-07 10:31:56

by Mark Rutland

[permalink] [raw]
Subject: Re: [PATCH] thermal: of: Introduce governor selection in dts

On Fri, Aug 07, 2015 at 08:09:39AM +0100, Chung-Yih Wang (王崇懿) wrote:
> As there could be more thermal zones on a system and
> more variety in thermal governors provided in kernel,
> this patch provides flexibility of governor selection
> for a thermal zone declared in device tree.
>
> Change-Id: Ie4a75d762709cbbe9f1806dae325d13f71982e78
> Signed-off-by: Chung-yih Wang <[email protected]>
> ---
> Documentation/devicetree/bindings/thermal/thermal.txt | 9 +++++++++
> drivers/thermal/of-thermal.c | 10 ++++++++++
> 2 files changed, 19 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt
> b/Documentation/devicetree/bindings/thermal/thermal.txt
> index 8a49362..30a5d41 100644
> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> @@ -174,6 +174,13 @@ Optional property:
> 2000mW, while on a 10'' tablet is around
> 4500mW.
>
> +- thermal-governor-name: The name of governor used to control the
> + thermal zone instead of the default one
> + specified in kernel config. For reference, if
> + default governor is step_wise, one could
> + select power_allocator for cpu_thermal zone
> + in dts.


This is not a hardware or system property, but rather a Linux
implementation detail.

This really shouldn't go in the DT.

Mark.

> +
> 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
> @@ -555,6 +562,8 @@ thermal-zones {
>
> sustainable-power = <2500>;
>
> + thermal-governor-name = "power_allocator";
> +
> trips {
> /* Trips are based on resulting linear equation */
> cpu_trip: cpu-trip {
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index b295b2b..45570ac 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -882,6 +882,7 @@ int __init of_parse_thermal_zones(void)
> }
>
> for_each_child_of_node(np, child) {
> + const char *governor_name;
> struct thermal_zone_device *zone;
> struct thermal_zone_params *tzp;
> int i, mask = 0;
> @@ -909,6 +910,15 @@ int __init of_parse_thermal_zones(void)
> goto exit_free;
> }
>
> + /* Select a preferred governor if declared */
> + if (!of_property_read_string(child,
> + "thermal-governor-name",
> + &governor_name)) {
> + strncpy(tzp->governor_name,
> + governor_name,
> + sizeof(tzp->governor_name) - 1);
> + }
> +
> /* No hwmon because there might be hwmon drivers registering */
> tzp->no_hwmon = true;
>
> --
> 2.1.2
>

Subject: Re: [PATCH] thermal: of: Introduce governor selection in dts

This patch was originally introduced when we made power_allocator the
default governor where we had issues in binding a thermal zone w/o
parameters to. Then we came out this facility for binding a specific
governor to a thermal zone in dts instead of the default governor.
Javi seems like this idea much.

On Fri, Aug 7, 2015 at 6:31 PM, Mark Rutland <[email protected]> wrote:
> On Fri, Aug 07, 2015 at 08:09:39AM +0100, Chung-Yih Wang (王崇懿) wrote:
>> As there could be more thermal zones on a system and
>> more variety in thermal governors provided in kernel,
>> this patch provides flexibility of governor selection
>> for a thermal zone declared in device tree.
>>
>> Change-Id: Ie4a75d762709cbbe9f1806dae325d13f71982e78
>> Signed-off-by: Chung-yih Wang <[email protected]>
>> ---
>> Documentation/devicetree/bindings/thermal/thermal.txt | 9 +++++++++
>> drivers/thermal/of-thermal.c | 10 ++++++++++
>> 2 files changed, 19 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt
>> b/Documentation/devicetree/bindings/thermal/thermal.txt
>> index 8a49362..30a5d41 100644
>> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
>> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
>> @@ -174,6 +174,13 @@ Optional property:
>> 2000mW, while on a 10'' tablet is around
>> 4500mW.
>>
>> +- thermal-governor-name: The name of governor used to control the
>> + thermal zone instead of the default one
>> + specified in kernel config. For reference, if
>> + default governor is step_wise, one could
>> + select power_allocator for cpu_thermal zone
>> + in dts.
>
>
> This is not a hardware or system property, but rather a Linux
> implementation detail.
>
> This really shouldn't go in the DT.
>
> Mark.
>
>> +
>> 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
>> @@ -555,6 +562,8 @@ thermal-zones {
>>
>> sustainable-power = <2500>;
>>
>> + thermal-governor-name = "power_allocator";
>> +
>> trips {
>> /* Trips are based on resulting linear equation */
>> cpu_trip: cpu-trip {
>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>> index b295b2b..45570ac 100644
>> --- a/drivers/thermal/of-thermal.c
>> +++ b/drivers/thermal/of-thermal.c
>> @@ -882,6 +882,7 @@ int __init of_parse_thermal_zones(void)
>> }
>>
>> for_each_child_of_node(np, child) {
>> + const char *governor_name;
>> struct thermal_zone_device *zone;
>> struct thermal_zone_params *tzp;
>> int i, mask = 0;
>> @@ -909,6 +910,15 @@ int __init of_parse_thermal_zones(void)
>> goto exit_free;
>> }
>>
>> + /* Select a preferred governor if declared */
>> + if (!of_property_read_string(child,
>> + "thermal-governor-name",
>> + &governor_name)) {
>> + strncpy(tzp->governor_name,
>> + governor_name,
>> + sizeof(tzp->governor_name) - 1);
>> + }
>> +
>> /* No hwmon because there might be hwmon drivers registering */
>> tzp->no_hwmon = true;
>>
>> --
>> 2.1.2
>>

2015-08-10 17:47:43

by Javi Merino

[permalink] [raw]
Subject: Re: [PATCH] thermal: of: Introduce governor selection in dts

On Mon, Aug 10, 2015 at 09:00:49AM +0100, Chung-Yih Wang (王崇懿) wrote:
> This patch was originally introduced when we made power_allocator the
> default governor where we had issues in binding a thermal zone w/o
> parameters to. Then we came out this facility for binding a specific
> governor to a thermal zone in dts instead of the default governor.
> Javi seems like this idea much.

While I can understand why this is not suitable for devicetree, we
should have a way in the kernel to configure different governors for
different thermal zones defined in device tree. Thermal zones defined
from platform code can choose the thermal governor when they are
registered.

If this information can't go in device tree, where can we put it? As
an additional parameter to thermal_zone_of_sensor_register()?

Cheers,
Javi

> On Fri, Aug 7, 2015 at 6:31 PM, Mark Rutland <[email protected]> wrote:
> > On Fri, Aug 07, 2015 at 08:09:39AM +0100, Chung-Yih Wang (王崇懿) wrote:
> >> As there could be more thermal zones on a system and
> >> more variety in thermal governors provided in kernel,
> >> this patch provides flexibility of governor selection
> >> for a thermal zone declared in device tree.
> >>
> >> Change-Id: Ie4a75d762709cbbe9f1806dae325d13f71982e78
> >> Signed-off-by: Chung-yih Wang <[email protected]>
> >> ---
> >> Documentation/devicetree/bindings/thermal/thermal.txt | 9 +++++++++
> >> drivers/thermal/of-thermal.c | 10 ++++++++++
> >> 2 files changed, 19 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt
> >> b/Documentation/devicetree/bindings/thermal/thermal.txt
> >> index 8a49362..30a5d41 100644
> >> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> >> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> >> @@ -174,6 +174,13 @@ Optional property:
> >> 2000mW, while on a 10'' tablet is around
> >> 4500mW.
> >>
> >> +- thermal-governor-name: The name of governor used to control the
> >> + thermal zone instead of the default one
> >> + specified in kernel config. For reference, if
> >> + default governor is step_wise, one could
> >> + select power_allocator for cpu_thermal zone
> >> + in dts.
> >
> >
> > This is not a hardware or system property, but rather a Linux
> > implementation detail.
> >
> > This really shouldn't go in the DT.
> >
> > Mark.
> >
> >> +
> >> 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
> >> @@ -555,6 +562,8 @@ thermal-zones {
> >>
> >> sustainable-power = <2500>;
> >>
> >> + thermal-governor-name = "power_allocator";
> >> +
> >> trips {
> >> /* Trips are based on resulting linear equation */
> >> cpu_trip: cpu-trip {
> >> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> >> index b295b2b..45570ac 100644
> >> --- a/drivers/thermal/of-thermal.c
> >> +++ b/drivers/thermal/of-thermal.c
> >> @@ -882,6 +882,7 @@ int __init of_parse_thermal_zones(void)
> >> }
> >>
> >> for_each_child_of_node(np, child) {
> >> + const char *governor_name;
> >> struct thermal_zone_device *zone;
> >> struct thermal_zone_params *tzp;
> >> int i, mask = 0;
> >> @@ -909,6 +910,15 @@ int __init of_parse_thermal_zones(void)
> >> goto exit_free;
> >> }
> >>
> >> + /* Select a preferred governor if declared */
> >> + if (!of_property_read_string(child,
> >> + "thermal-governor-name",
> >> + &governor_name)) {
> >> + strncpy(tzp->governor_name,
> >> + governor_name,
> >> + sizeof(tzp->governor_name) - 1);
> >> + }
> >> +
> >> /* No hwmon because there might be hwmon drivers registering */
> >> tzp->no_hwmon = true;
> >>
> >> --
> >> 2.1.2
> >>
>

2015-08-11 09:07:44

by Sudeep Holla

[permalink] [raw]
Subject: Re: [PATCH] thermal: of: Introduce governor selection in dts



On 10/08/15 18:47, Javi Merino wrote:
> On Mon, Aug 10, 2015 at 09:00:49AM +0100, Chung-Yih Wang (王崇懿) wrote:
>> This patch was originally introduced when we made power_allocator the
>> default governor where we had issues in binding a thermal zone w/o
>> parameters to. Then we came out this facility for binding a specific
>> governor to a thermal zone in dts instead of the default governor.
>> Javi seems like this idea much.
>
> While I can understand why this is not suitable for devicetree, we
> should have a way in the kernel to configure different governors for
> different thermal zones defined in device tree. Thermal zones defined
> from platform code can choose the thermal governor when they are
> registered.
>
> If this information can't go in device tree, where can we put it? As
> an additional parameter to thermal_zone_of_sensor_register()?
>

Why can't it be via sysfs allowing users to select their choice of
governor ? (like cpuidle/freq or even devfreq I assume)

Regards,
Sudeep

2015-11-02 21:24:29

by Eduardo Valentin

[permalink] [raw]
Subject: Re: [PATCH] thermal: of: Introduce governor selection in dts

On Tue, Aug 11, 2015 at 10:07:31AM +0100, Sudeep Holla wrote:
>
>
> On 10/08/15 18:47, Javi Merino wrote:
> >On Mon, Aug 10, 2015 at 09:00:49AM +0100, Chung-Yih Wang (王崇懿) wrote:
> >>This patch was originally introduced when we made power_allocator the
> >>default governor where we had issues in binding a thermal zone w/o
> >>parameters to. Then we came out this facility for binding a specific
> >>governor to a thermal zone in dts instead of the default governor.
> >>Javi seems like this idea much.
> >
> >While I can understand why this is not suitable for devicetree, we
> >should have a way in the kernel to configure different governors for
> >different thermal zones defined in device tree. Thermal zones defined
> >from platform code can choose the thermal governor when they are
> >registered.
> >
> >If this information can't go in device tree, where can we put it? As
> >an additional parameter to thermal_zone_of_sensor_register()?
> >
>
> Why can't it be via sysfs allowing users to select their choice of
> governor ? (like cpuidle/freq or even devfreq I assume)
>

Yes, sysfs configuration is the current recommended way.

Although it is tempting to have it in device tree, its
design decisions are not meant for OS implementation/specifics.

BR,

> Regards,
> Sudeep