[PATCH v2 0/3] thermal: gov_step_wise: Two cleanups and small adjustment

[PATCH v2 0/3] thermal: gov_step_wise: Two cleanups and small adjustment

[Rafael J. Wysocki]

Hi All,

This series makes a straightforward code cleanup in the Step-wise thermal
governor, clarifies a slightly ambiguous comment in it, and adjusts its
behavior to be consistent with another throttling logic description
comment and possibly reduce undesirable performance oscillations between
high and low levels due to cooling in some cases.

Thanks!

[PATCH v2 1/3] thermal: gov_step_wise: Clean up local variable initialization

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Make the initialization of local variable throttle in
thermal_zone_trip_update() more straightforward.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_step_wise.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

--- a/drivers/thermal/gov_step_wise.c
+++ b/drivers/thermal/gov_step_wise.c
@@ -69,16 +69,14 @@
 				     const struct thermal_trip_desc *td,
 				     int trip_threshold)
 {
+	bool throttle = tz->temperature >= trip_threshold;
 	const struct thermal_trip *trip = &td->trip;
 	enum thermal_trend trend = get_tz_trend(tz, trip);
 	int trip_id = thermal_zone_trip_id(tz, trip);
 	struct thermal_instance *instance;
-	bool throttle = false;
 
-	if (tz->temperature >= trip_threshold) {
-		throttle = true;
+	if (throttle)
 		trace_thermal_zone_trip(tz, trip_id, trip->type);
-	}
 
 	dev_dbg(&tz->device, "Trip%d[type=%d,temp=%d]:trend=%d,throttle=%d\n",
 		trip_id, trip->type, trip_threshold, trend, throttle);

[PATCH v2 2/3] thermal: gov_step_wise: Clarify cooling logic description comment

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The cooling logic description comment next to the get_target_state()
definition is slightly ambiguous in what it means by "lower cooling
state", so clarify that by replacing the ambuguous phrase with "the
minimum applicable cooling state".

No functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_step_wise.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/drivers/thermal/gov_step_wise.c
+++ b/drivers/thermal/gov_step_wise.c
@@ -23,8 +23,8 @@
  *    b. if the trend is THERMAL_TREND_DROPPING, do nothing
  * If the temperature is lower than a trip point,
  *    a. if the trend is THERMAL_TREND_RAISING, do nothing
- *    b. if the trend is THERMAL_TREND_DROPPING, use lower cooling
- *       state for this trip point, if the cooling state already
+ *    b. if the trend is THERMAL_TREND_DROPPING, use the minimum applicable
+ *       cooling state for this trip point, or if the cooling state already
  *       equals lower limit, deactivate the thermal instance
  */
 static unsigned long get_target_state(struct thermal_instance *instance,

[PATCH v2 3/3] thermal: gov_step_wise: Allow cooling level to be reduced earlier

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The current behavior of the Step-wise thermal governor is to increase
the cooling level one step at a time after trip point threshold passing
by thermal zone temperature until the temperature stops to rise and then
do nothing until it falls down below the (possibly new) trip point
threshold, at which point the cooling level is reduced straight to the
applicable minimum.

While this generally works, it is not in agreement with the throttling
logic description comment in step_wise_manage() any more after some
relatively recent changes, and in the case of passive cooling, it may
lead to undesirable performance oscillations between high and low
levels.

For this reason, modify the governor's cooling device state selection
function, get_target_state(), to reduce cooling by one level even if
the temperature is still above the thermal zone threshold, but the
temperature has started to fall down.  However, ensure that the cooling
level will remain above the applicable minimum in that case to pull
the zone temperature further down, possibly until it falls below the
trip threshold (which may now be equal to the low temperature of the
trip).

Doing so should help higher performance to be restored earlier in some
cases which is desirable especially for passive trip points with
relatively high hysteresis values.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_step_wise.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

--- a/drivers/thermal/gov_step_wise.c
+++ b/drivers/thermal/gov_step_wise.c
@@ -20,7 +20,9 @@
  * If the temperature is higher than a trip point,
  *    a. if the trend is THERMAL_TREND_RAISING, use higher cooling
  *       state for this trip point
- *    b. if the trend is THERMAL_TREND_DROPPING, do nothing
+ *    b. if the trend is THERMAL_TREND_DROPPING, use a lower cooling state
+ *       for this trip point, but keep the cooling state above the applicable
+ *       minimum
  * If the temperature is lower than a trip point,
  *    a. if the trend is THERMAL_TREND_RAISING, do nothing
  *    b. if the trend is THERMAL_TREND_DROPPING, use the minimum applicable
@@ -51,6 +53,17 @@
 	if (throttle) {
 		if (trend == THERMAL_TREND_RAISING)
 			return clamp(cur_state + 1, instance->lower, instance->upper);
+
+		/*
+		 * If the zone temperature is falling, the cooling level can
+		 * be reduced, but it should still be above the lower state of
+		 * the given thermal instance to pull the temperature further
+		 * down.
+		 */
+		if (trend == THERMAL_TREND_DROPPING)
+			return clamp(cur_state - 1,
+				     min(instance->lower + 1, instance->upper),
+				     instance->upper);
 	} else if (trend == THERMAL_TREND_DROPPING) {
 		if (cur_state <= instance->lower)
 			return THERMAL_NO_TARGET;

Re: [PATCH v2 1/3] thermal: gov_step_wise: Clean up local variable initialization

[Lukasz Luba]

On 8/25/25 14:27, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Make the initialization of local variable throttle in
> thermal_zone_trip_update() more straightforward.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/gov_step_wise.c |    6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> --- a/drivers/thermal/gov_step_wise.c
> +++ b/drivers/thermal/gov_step_wise.c
> @@ -69,16 +69,14 @@
>   				     const struct thermal_trip_desc *td,
>   				     int trip_threshold)
>   {
> +	bool throttle = tz->temperature >= trip_threshold;
>   	const struct thermal_trip *trip = &td->trip;
>   	enum thermal_trend trend = get_tz_trend(tz, trip);
>   	int trip_id = thermal_zone_trip_id(tz, trip);
>   	struct thermal_instance *instance;
> -	bool throttle = false;
>   
> -	if (tz->temperature >= trip_threshold) {
> -		throttle = true;
> +	if (throttle)
>   		trace_thermal_zone_trip(tz, trip_id, trip->type);
> -	}
>   
>   	dev_dbg(&tz->device, "Trip%d[type=%d,temp=%d]:trend=%d,throttle=%d\n",
>   		trip_id, trip->type, trip_threshold, trend, throttle);
> 
> 
> 


LGTM,

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v2 2/3] thermal: gov_step_wise: Clarify cooling logic description comment

[Lukasz Luba]

On 8/25/25 14:28, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The cooling logic description comment next to the get_target_state()
> definition is slightly ambiguous in what it means by "lower cooling
> state", so clarify that by replacing the ambuguous phrase with "the
> minimum applicable cooling state".
> 
> No functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/gov_step_wise.c |    4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --- a/drivers/thermal/gov_step_wise.c
> +++ b/drivers/thermal/gov_step_wise.c
> @@ -23,8 +23,8 @@
>    *    b. if the trend is THERMAL_TREND_DROPPING, do nothing
>    * If the temperature is lower than a trip point,
>    *    a. if the trend is THERMAL_TREND_RAISING, do nothing
> - *    b. if the trend is THERMAL_TREND_DROPPING, use lower cooling
> - *       state for this trip point, if the cooling state already
> + *    b. if the trend is THERMAL_TREND_DROPPING, use the minimum applicable
> + *       cooling state for this trip point, or if the cooling state already
>    *       equals lower limit, deactivate the thermal instance
>    */
>   static unsigned long get_target_state(struct thermal_instance *instance,
> 
> 
> 

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v2 3/3] thermal: gov_step_wise: Allow cooling level to be reduced earlier

[Lukasz Luba]

On 8/25/25 14:31, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The current behavior of the Step-wise thermal governor is to increase
> the cooling level one step at a time after trip point threshold passing
> by thermal zone temperature until the temperature stops to rise and then
> do nothing until it falls down below the (possibly new) trip point
> threshold, at which point the cooling level is reduced straight to the
> applicable minimum.

Quite long single sentence to describe these stuff...

> 
> While this generally works, it is not in agreement with the throttling
> logic description comment in step_wise_manage() any more after some
> relatively recent changes, and in the case of passive cooling, it may
> lead to undesirable performance oscillations between high and low
> levels.
> 
> For this reason, modify the governor's cooling device state selection
> function, get_target_state(), to reduce cooling by one level even if
> the temperature is still above the thermal zone threshold, but the
> temperature has started to fall down.  However, ensure that the cooling
> level will remain above the applicable minimum in that case to pull
> the zone temperature further down, possibly until it falls below the
> trip threshold (which may now be equal to the low temperature of the
> trip).
> 
> Doing so should help higher performance to be restored earlier in some
> cases which is desirable especially for passive trip points with
> relatively high hysteresis values.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/gov_step_wise.c |   15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> --- a/drivers/thermal/gov_step_wise.c
> +++ b/drivers/thermal/gov_step_wise.c
> @@ -20,7 +20,9 @@
>    * If the temperature is higher than a trip point,
>    *    a. if the trend is THERMAL_TREND_RAISING, use higher cooling
>    *       state for this trip point
> - *    b. if the trend is THERMAL_TREND_DROPPING, do nothing
> + *    b. if the trend is THERMAL_TREND_DROPPING, use a lower cooling state
> + *       for this trip point, but keep the cooling state above the applicable
> + *       minimum
>    * If the temperature is lower than a trip point,
>    *    a. if the trend is THERMAL_TREND_RAISING, do nothing
>    *    b. if the trend is THERMAL_TREND_DROPPING, use the minimum applicable
> @@ -51,6 +53,17 @@
>   	if (throttle) {
>   		if (trend == THERMAL_TREND_RAISING)
>   			return clamp(cur_state + 1, instance->lower, instance->upper);
> +
> +		/*
> +		 * If the zone temperature is falling, the cooling level can
> +		 * be reduced, but it should still be above the lower state of
> +		 * the given thermal instance to pull the temperature further
> +		 * down.
> +		 */
> +		if (trend == THERMAL_TREND_DROPPING)
> +			return clamp(cur_state - 1,
> +				     min(instance->lower + 1, instance->upper),
> +				     instance->upper);
>   	} else if (trend == THERMAL_TREND_DROPPING) {
>   		if (cur_state <= instance->lower)
>   			return THERMAL_NO_TARGET;
> 
> 
> 

That make sense

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>