[PATCH] drivers: thermal: step_wise: add support for hysteresis

[PATCH] drivers: thermal: step_wise: add support for hysteresis

[Amit Kucheria]

From: Ram Chandrasekar <rkumbako@codeaurora.org>

Currently, step wise governor increases the mitigation when the
temperature goes above a threshold and decreases the mitigation when the
temperature goes below the threshold. If there is a case where the
temperature is wavering around the threshold, the mitigation will be
applied and removed every iteration, which is not very efficient.

The use of hysteresis temperature could avoid this ping-pong of
mitigation by relaxing the mitigation to happen only when the
temperature goes below this lower hysteresis value.

Signed-off-by: Ram Chandrasekar <rkumbako@codeaurora.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
[Rebased patch from downstream]
Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 drivers/thermal/step_wise.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
index 6e051cbd824ff..2c8a34a7cf959 100644
--- a/drivers/thermal/step_wise.c
+++ b/drivers/thermal/step_wise.c
@@ -24,7 +24,7 @@
  *       for this trip point
  *    d. if the trend is THERMAL_TREND_DROP_FULL, use lower limit
  *       for this trip point
- * If the temperature is lower than a trip point,
+ * If the temperature is lower than a hysteresis temperature,
  *    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
@@ -115,30 +115,31 @@ static void update_passive_instance(struct thermal_zone_device *tz,
 
 static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
 {
-	int trip_temp;
+	int trip_temp, hyst_temp;
 	enum thermal_trip_type trip_type;
 	enum thermal_trend trend;
 	struct thermal_instance *instance;
-	bool throttle = false;
+	bool throttle;
 	int old_target;
 
 	if (trip == THERMAL_TRIPS_NONE) {
-		trip_temp = tz->forced_passive;
+		hyst_temp = trip_temp = tz->forced_passive;
 		trip_type = THERMAL_TRIPS_NONE;
 	} else {
 		tz->ops->get_trip_temp(tz, trip, &trip_temp);
+		hyst_temp = trip_temp;
+		if (tz->ops->get_trip_hyst) {
+			tz->ops->get_trip_hyst(tz, trip, &hyst_temp);
+			hyst_temp = trip_temp - hyst_temp;
+		}
 		tz->ops->get_trip_type(tz, trip, &trip_type);
 	}
 
 	trend = get_tz_trend(tz, trip);
 
-	if (tz->temperature >= trip_temp) {
-		throttle = true;
-		trace_thermal_zone_trip(tz, trip, trip_type);
-	}
-
-	dev_dbg(&tz->device, "Trip%d[type=%d,temp=%d]:trend=%d,throttle=%d\n",
-				trip, trip_type, trip_temp, trend, throttle);
+	dev_dbg(&tz->device,
+		"Trip%d[type=%d,temp=%d,hyst=%d]:trend=%d,throttle=%d\n",
+		trip, trip_type, trip_temp, hyst_temp, trend, throttle);
 
 	mutex_lock(&tz->lock);
 
@@ -147,6 +148,18 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
 			continue;
 
 		old_target = instance->target;
+		throttle = false;
+		/*
+		 * Lower the mitigation only if the temperature
+		 * goes below the hysteresis temperature.
+		 */
+		if (tz->temperature >= trip_temp ||
+		    (tz->temperature >= hyst_temp &&
+		     old_target != THERMAL_NO_TARGET)) {
+			throttle = true;
+			trace_thermal_zone_trip(tz, trip, trip_type);
+		}
+
 		instance->target = get_target_state(instance, trend, throttle);
 		dev_dbg(&instance->cdev->device, "old_target=%d, target=%d\n",
 					old_target, (int)instance->target);
-- 
2.17.1

Re: [PATCH] drivers: thermal: step_wise: add support for hysteresis

[Thara Gopinath]

On 11/21/2019 12:50 AM, Amit Kucheria wrote:
> From: Ram Chandrasekar <rkumbako@codeaurora.org>
> 
> Currently, step wise governor increases the mitigation when the
> temperature goes above a threshold and decreases the mitigation when the
> temperature goes below the threshold. If there is a case where the
> temperature is wavering around the threshold, the mitigation will be
> applied and removed every iteration, which is not very efficient.
> 
> The use of hysteresis temperature could avoid this ping-pong of
> mitigation by relaxing the mitigation to happen only when the
> temperature goes below this lower hysteresis value.
Hi Amit,

Can this not lead to ping-pong around the hysteresis temperature?
If the idea is to minimize ping-pong isn't average a better method?

Warm Regards
Thara

> 
> Signed-off-by: Ram Chandrasekar <rkumbako@codeaurora.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> [Rebased patch from downstream]
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
>  drivers/thermal/step_wise.c | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
> index 6e051cbd824ff..2c8a34a7cf959 100644
> --- a/drivers/thermal/step_wise.c
> +++ b/drivers/thermal/step_wise.c
> @@ -24,7 +24,7 @@
>   *       for this trip point
>   *    d. if the trend is THERMAL_TREND_DROP_FULL, use lower limit
>   *       for this trip point
> - * If the temperature is lower than a trip point,
> + * If the temperature is lower than a hysteresis temperature,
>   *    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
> @@ -115,30 +115,31 @@ static void update_passive_instance(struct thermal_zone_device *tz,
>  
>  static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
>  {
> -	int trip_temp;
> +	int trip_temp, hyst_temp;
>  	enum thermal_trip_type trip_type;
>  	enum thermal_trend trend;
>  	struct thermal_instance *instance;
> -	bool throttle = false;
> +	bool throttle;
>  	int old_target;
>  
>  	if (trip == THERMAL_TRIPS_NONE) {
> -		trip_temp = tz->forced_passive;
> +		hyst_temp = trip_temp = tz->forced_passive;
>  		trip_type = THERMAL_TRIPS_NONE;
>  	} else {
>  		tz->ops->get_trip_temp(tz, trip, &trip_temp);
> +		hyst_temp = trip_temp;
> +		if (tz->ops->get_trip_hyst) {
> +			tz->ops->get_trip_hyst(tz, trip, &hyst_temp);
> +			hyst_temp = trip_temp - hyst_temp;
> +		}
>  		tz->ops->get_trip_type(tz, trip, &trip_type);
>  	}
>  
>  	trend = get_tz_trend(tz, trip);
>  
> -	if (tz->temperature >= trip_temp) {
> -		throttle = true;
> -		trace_thermal_zone_trip(tz, trip, trip_type);
> -	}
> -
> -	dev_dbg(&tz->device, "Trip%d[type=%d,temp=%d]:trend=%d,throttle=%d\n",
> -				trip, trip_type, trip_temp, trend, throttle);
> +	dev_dbg(&tz->device,
> +		"Trip%d[type=%d,temp=%d,hyst=%d]:trend=%d,throttle=%d\n",
> +		trip, trip_type, trip_temp, hyst_temp, trend, throttle);
>  
>  	mutex_lock(&tz->lock);
>  
> @@ -147,6 +148,18 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
>  			continue;
>  
>  		old_target = instance->target;
> +		throttle = false;
> +		/*
> +		 * Lower the mitigation only if the temperature
> +		 * goes below the hysteresis temperature.
> +		 */
> +		if (tz->temperature >= trip_temp ||
> +		    (tz->temperature >= hyst_temp &&
> +		     old_target != THERMAL_NO_TARGET)) {
> +			throttle = true;
> +			trace_thermal_zone_trip(tz, trip, trip_type);
> +		}
> +
>  		instance->target = get_target_state(instance, trend, throttle);
>  		dev_dbg(&instance->cdev->device, "old_target=%d, target=%d\n",
>  					old_target, (int)instance->target);
> 

Re: [PATCH] drivers: thermal: step_wise: add support for hysteresis

[Amit Kucheria]

On Thu, Nov 21, 2019 at 7:40 PM Thara Gopinath
<thara.gopinath@linaro.org> wrote:
>
> On 11/21/2019 12:50 AM, Amit Kucheria wrote:
> > From: Ram Chandrasekar <rkumbako@codeaurora.org>
> >
> > Currently, step wise governor increases the mitigation when the
> > temperature goes above a threshold and decreases the mitigation when the
> > temperature goes below the threshold. If there is a case where the
> > temperature is wavering around the threshold, the mitigation will be
> > applied and removed every iteration, which is not very efficient.
> >
> > The use of hysteresis temperature could avoid this ping-pong of
> > mitigation by relaxing the mitigation to happen only when the
> > temperature goes below this lower hysteresis value.
> Hi Amit,
>
> Can this not lead to ping-pong around the hysteresis temperature?

That isn't how hysteresis is supposed to work if there is a sufficient
delta between your trip point and your hysteresis value.

e.g. if you have a trip at 80C and a hysteresis of 10C, it means that
you will start throttling at 80C, but you won't STOP throttling until
you cool down to 70C. At that point, you will wait again to get to 80C
before throttling again.
IOW, on the downward slope (80 -> 70) you still have throttling active
and on the upward slope (70 -> 80), you have no[1] throttling, so
different behaviour is the same temperature range depending on
direction.

If your hysteresis value was very low .e.g. 1C, it would certainly be useless.

> If the idea is to minimize ping-pong isn't average a better method?

We shouldn't ping-pong with the asymmetric behaviour described above.

Regards,
Amit
[1] This is a simple example with a single trip point.

Re: [PATCH] drivers: thermal: step_wise: add support for hysteresis

[Thara Gopinath]

On 11/21/2019 09:38 AM, Amit Kucheria wrote:
> On Thu, Nov 21, 2019 at 7:40 PM Thara Gopinath
> <thara.gopinath@linaro.org> wrote:
>>
>> On 11/21/2019 12:50 AM, Amit Kucheria wrote:
>>> From: Ram Chandrasekar <rkumbako@codeaurora.org>
>>>
>>> Currently, step wise governor increases the mitigation when the
>>> temperature goes above a threshold and decreases the mitigation when the
>>> temperature goes below the threshold. If there is a case where the
>>> temperature is wavering around the threshold, the mitigation will be
>>> applied and removed every iteration, which is not very efficient.
>>>
>>> The use of hysteresis temperature could avoid this ping-pong of
>>> mitigation by relaxing the mitigation to happen only when the
>>> temperature goes below this lower hysteresis value.
>> Hi Amit,
>>
>> Can this not lead to ping-pong around the hysteresis temperature?
> 
> That isn't how hysteresis is supposed to work if there is a sufficient
> delta between your trip point and your hysteresis value.
> 
> e.g. if you have a trip at 80C and a hysteresis of 10C, it means that
> you will start throttling at 80C, but you won't STOP throttling until
> you cool down to 70C. At that point, you will wait again to get to 80C
> before throttling again.
> IOW, on the downward slope (80 -> 70) you still have throttling active
> and on the upward slope (70 -> 80), you have no[1] throttling, so
> different behaviour is the same temperature range depending on
> direction.
> 
> If your hysteresis value was very low .e.g. 1C, it would certainly be useless.

Thanks for the explanation. I think averaging can still give a smoother
experience/transition. But then it has to be implemented and tested
against this solution. Other reason for this solution is hysteresis can
be a higher value if needed where as averaging might not give that
flexibility. I have some other comments on the patch which I have posted
separately.
> 
>> If the idea is to minimize ping-pong isn't average a better method?
> 
> We shouldn't ping-pong with the asymmetric behaviour described above.
> 
> Regards,
> Amit
> [1] This is a simple example with a single trip point.
> 


-- 
Warm Regards
Thara

Re: [PATCH] drivers: thermal: step_wise: add support for hysteresis

[Thara Gopinath]

Hi Amit,

On 11/21/2019 12:50 AM, Amit Kucheria wrote:
> From: Ram Chandrasekar <rkumbako@codeaurora.org>
> 
> Currently, step wise governor increases the mitigation when the
> temperature goes above a threshold and decreases the mitigation when the
> temperature goes below the threshold. If there is a case where the
> temperature is wavering around the threshold, the mitigation will be
> applied and removed every iteration, which is not very efficient.
> 
> The use of hysteresis temperature could avoid this ping-pong of
> mitigation by relaxing the mitigation to happen only when the
> temperature goes below this lower hysteresis value.
> 
> Signed-off-by: Ram Chandrasekar <rkumbako@codeaurora.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> [Rebased patch from downstream]
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
>  drivers/thermal/step_wise.c | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
> index 6e051cbd824ff..2c8a34a7cf959 100644
> --- a/drivers/thermal/step_wise.c
> +++ b/drivers/thermal/step_wise.c
> @@ -24,7 +24,7 @@
>   *       for this trip point
>   *    d. if the trend is THERMAL_TREND_DROP_FULL, use lower limit
>   *       for this trip point
> - * If the temperature is lower than a trip point,
> + * If the temperature is lower than a hysteresis temperature,
>   *    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
> @@ -115,30 +115,31 @@ static void update_passive_instance(struct thermal_zone_device *tz,
>  
>  static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
>  {
> -	int trip_temp;
> +	int trip_temp, hyst_temp;
>  	enum thermal_trip_type trip_type;
>  	enum thermal_trend trend;
>  	struct thermal_instance *instance;
> -	bool throttle = false;
> +	bool throttle;

There is no need to remove throttle = false here. You are setting it to
false later down.

>  	int old_target;
>  
>  	if (trip == THERMAL_TRIPS_NONE) {
> -		trip_temp = tz->forced_passive;
> +		hyst_temp = trip_temp = tz->forced_passive;
>  		trip_type = THERMAL_TRIPS_NONE;
>  	} else {
>  		tz->ops->get_trip_temp(tz, trip, &trip_temp);
> +		hyst_temp = trip_temp;
> +		if (tz->ops->get_trip_hyst) {
> +			tz->ops->get_trip_hyst(tz, trip, &hyst_temp);
> +			hyst_temp = trip_temp - hyst_temp;
> +		}
>  		tz->ops->get_trip_type(tz, trip, &trip_type);
>  	}
>  
>  	trend = get_tz_trend(tz, trip);
>  
> -	if (tz->temperature >= trip_temp) {
> -		throttle = true;
> -		trace_thermal_zone_trip(tz, trip, trip_type);
> -	}
> -
> -	dev_dbg(&tz->device, "Trip%d[type=%d,temp=%d]:trend=%d,throttle=%d\n",
> -				trip, trip_type, trip_temp, trend, throttle);
> +	dev_dbg(&tz->device,
> +		"Trip%d[type=%d,temp=%d,hyst=%d]:trend=%d,throttle=%d\n",
> +		trip, trip_type, trip_temp, hyst_temp, trend, throttle);

throttle value is not final here. Why is the debug print and the setting
of throttle reversed ? Idea is to print the final value of
throttle.

>  
>  	mutex_lock(&tz->lock);
>  
> @@ -147,6 +148,18 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
>  			continue;
>  
>  		old_target = instance->target;
> +		throttle = false;
> +		/*
> +		 * Lower the mitigation only if the temperature
> +		 * goes below the hysteresis temperature.
> +		 */

I think this requires more comment here on why there is a check for
old_target != THERMAL_NO_TARGET. Basically to ensure that the hysteresis
is considered only when temperature is dropping.

> +		if (tz->temperature >= trip_temp ||
> +		    (tz->temperature >= hyst_temp &&
> +		     old_target != THERMAL_NO_TARGET)) {
> +			throttle = true;
> +			trace_thermal_zone_trip(tz, trip, trip_type);
> +		}
> +
>  		instance->target = get_target_state(instance, trend, throttle);
>  		dev_dbg(&instance->cdev->device, "old_target=%d, target=%d\n",
>  					old_target, (int)instance->target);
> 


-- 
Warm Regards
Thara

Re: [PATCH] drivers: thermal: step_wise: add support for hysteresis

[Amit Kucheria]

On Thu, Nov 21, 2019 at 11:21 AM Amit Kucheria <amit.kucheria@linaro.org> wrote:
>
> From: Ram Chandrasekar <rkumbako@codeaurora.org>
>
> Currently, step wise governor increases the mitigation when the
> temperature goes above a threshold and decreases the mitigation when the
> temperature goes below the threshold. If there is a case where the
> temperature is wavering around the threshold, the mitigation will be
> applied and removed every iteration, which is not very efficient.
>
> The use of hysteresis temperature could avoid this ping-pong of
> mitigation by relaxing the mitigation to happen only when the
> temperature goes below this lower hysteresis value.
>
> Signed-off-by: Ram Chandrasekar <rkumbako@codeaurora.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> [Rebased patch from downstream]
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>

Daniel, Rui: ping.

Keerthy: This works for you, right?

> ---
>  drivers/thermal/step_wise.c | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
> index 6e051cbd824ff..2c8a34a7cf959 100644
> --- a/drivers/thermal/step_wise.c
> +++ b/drivers/thermal/step_wise.c
> @@ -24,7 +24,7 @@
>   *       for this trip point
>   *    d. if the trend is THERMAL_TREND_DROP_FULL, use lower limit
>   *       for this trip point
> - * If the temperature is lower than a trip point,
> + * If the temperature is lower than a hysteresis temperature,
>   *    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
> @@ -115,30 +115,31 @@ static void update_passive_instance(struct thermal_zone_device *tz,
>
>  static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
>  {
> -       int trip_temp;
> +       int trip_temp, hyst_temp;
>         enum thermal_trip_type trip_type;
>         enum thermal_trend trend;
>         struct thermal_instance *instance;
> -       bool throttle = false;
> +       bool throttle;
>         int old_target;
>
>         if (trip == THERMAL_TRIPS_NONE) {
> -               trip_temp = tz->forced_passive;
> +               hyst_temp = trip_temp = tz->forced_passive;
>                 trip_type = THERMAL_TRIPS_NONE;
>         } else {
>                 tz->ops->get_trip_temp(tz, trip, &trip_temp);
> +               hyst_temp = trip_temp;
> +               if (tz->ops->get_trip_hyst) {
> +                       tz->ops->get_trip_hyst(tz, trip, &hyst_temp);
> +                       hyst_temp = trip_temp - hyst_temp;
> +               }
>                 tz->ops->get_trip_type(tz, trip, &trip_type);
>         }
>
>         trend = get_tz_trend(tz, trip);
>
> -       if (tz->temperature >= trip_temp) {
> -               throttle = true;
> -               trace_thermal_zone_trip(tz, trip, trip_type);
> -       }
> -
> -       dev_dbg(&tz->device, "Trip%d[type=%d,temp=%d]:trend=%d,throttle=%d\n",
> -                               trip, trip_type, trip_temp, trend, throttle);
> +       dev_dbg(&tz->device,
> +               "Trip%d[type=%d,temp=%d,hyst=%d]:trend=%d,throttle=%d\n",
> +               trip, trip_type, trip_temp, hyst_temp, trend, throttle);
>
>         mutex_lock(&tz->lock);
>
> @@ -147,6 +148,18 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
>                         continue;
>
>                 old_target = instance->target;
> +               throttle = false;
> +               /*
> +                * Lower the mitigation only if the temperature
> +                * goes below the hysteresis temperature.
> +                */
> +               if (tz->temperature >= trip_temp ||
> +                   (tz->temperature >= hyst_temp &&
> +                    old_target != THERMAL_NO_TARGET)) {
> +                       throttle = true;
> +                       trace_thermal_zone_trip(tz, trip, trip_type);
> +               }
> +
>                 instance->target = get_target_state(instance, trend, throttle);
>                 dev_dbg(&instance->cdev->device, "old_target=%d, target=%d\n",
>                                         old_target, (int)instance->target);
> --
> 2.17.1
>

Re: [PATCH] drivers: thermal: step_wise: add support for hysteresis

[Daniel Lezcano]

On 21/11/2019 06:50, Amit Kucheria wrote:
> From: Ram Chandrasekar <rkumbako@codeaurora.org>
> 
> Currently, step wise governor increases the mitigation when the
> temperature goes above a threshold and decreases the mitigation when the
> temperature goes below the threshold. 
>
> If there is a case where the
> temperature is wavering around the threshold, the mitigation will be
> applied and removed every iteration, which is not very efficient.
>
> The use of hysteresis temperature could avoid this ping-pong of
> mitigation by relaxing the mitigation to happen only when the
> temperature goes below this lower hysteresis value.

What I'm worried about is how the hysteresis is used in the current
code, where the destination of this data is to set the value in the
sensor hardware if it is supported.

Using the hysteresis in the governor seems like abusing the initial
purpose of this information.

Moreover, the hysteresis creates a gray area where the above algorithm
(DROPPING && !throttle) => state-- or (RAISING && throttle) => state++
may drop the performances because we will continue mitigating even below
the threshold.

As the governor is an open-loop controller, I'm not sure if we can do
something except adding some kind of low pass filter to prevent
mitigation bounces.

> Signed-off-by: Ram Chandrasekar <rkumbako@codeaurora.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> [Rebased patch from downstream]
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
>  drivers/thermal/step_wise.c | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
> index 6e051cbd824ff..2c8a34a7cf959 100644
> --- a/drivers/thermal/step_wise.c
> +++ b/drivers/thermal/step_wise.c
> @@ -24,7 +24,7 @@
>   *       for this trip point
>   *    d. if the trend is THERMAL_TREND_DROP_FULL, use lower limit
>   *       for this trip point
> - * If the temperature is lower than a trip point,
> + * If the temperature is lower than a hysteresis temperature,
>   *    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
> @@ -115,30 +115,31 @@ static void update_passive_instance(struct thermal_zone_device *tz,
>  
>  static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
>  {
> -	int trip_temp;
> +	int trip_temp, hyst_temp;
>  	enum thermal_trip_type trip_type;
>  	enum thermal_trend trend;
>  	struct thermal_instance *instance;
> -	bool throttle = false;
> +	bool throttle;
>  	int old_target;
>  
>  	if (trip == THERMAL_TRIPS_NONE) {
> -		trip_temp = tz->forced_passive;
> +		hyst_temp = trip_temp = tz->forced_passive;
>  		trip_type = THERMAL_TRIPS_NONE;
>  	} else {
>  		tz->ops->get_trip_temp(tz, trip, &trip_temp);
> +		hyst_temp = trip_temp;
> +		if (tz->ops->get_trip_hyst) {
> +			tz->ops->get_trip_hyst(tz, trip, &hyst_temp);
> +			hyst_temp = trip_temp - hyst_temp;
> +		}
>  		tz->ops->get_trip_type(tz, trip, &trip_type);
>  	}
>  
>  	trend = get_tz_trend(tz, trip);
>  
> -	if (tz->temperature >= trip_temp) {
> -		throttle = true;
> -		trace_thermal_zone_trip(tz, trip, trip_type);
> -	}
> -
> -	dev_dbg(&tz->device, "Trip%d[type=%d,temp=%d]:trend=%d,throttle=%d\n",
> -				trip, trip_type, trip_temp, trend, throttle);
> +	dev_dbg(&tz->device,
> +		"Trip%d[type=%d,temp=%d,hyst=%d]:trend=%d,throttle=%d\n",
> +		trip, trip_type, trip_temp, hyst_temp, trend, throttle);
>  
>  	mutex_lock(&tz->lock);
>  
> @@ -147,6 +148,18 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
>  			continue;
>  
>  		old_target = instance->target;
> +		throttle = false;
> +		/*
> +		 * Lower the mitigation only if the temperature
> +		 * goes below the hysteresis temperature.
> +		 */
> +		if (tz->temperature >= trip_temp ||
> +		    (tz->temperature >= hyst_temp &&
> +		     old_target != THERMAL_NO_TARGET)) {
> +			throttle = true;
> +			trace_thermal_zone_trip(tz, trip, trip_type);
> +		}
> +
>  		instance->target = get_target_state(instance, trend, throttle);
>  		dev_dbg(&instance->cdev->device, "old_target=%d, target=%d\n",
>  					old_target, (int)instance->target);
> 


-- 
 <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] drivers: thermal: step_wise: add support for hysteresis

[Ram Chandrasekar]

On 12/11/2019 6:35 AM, Daniel Lezcano wrote:
> On 21/11/2019 06:50, Amit Kucheria wrote:
>> From: Ram Chandrasekar <rkumbako@codeaurora.org>
>>
>> Currently, step wise governor increases the mitigation when the
>> temperature goes above a threshold and decreases the mitigation when the
>> temperature goes below the threshold.
>>
>> If there is a case where the
>> temperature is wavering around the threshold, the mitigation will be
>> applied and removed every iteration, which is not very efficient.
>>
>> The use of hysteresis temperature could avoid this ping-pong of
>> mitigation by relaxing the mitigation to happen only when the
>> temperature goes below this lower hysteresis value.
> 
> What I'm worried about is how the hysteresis is used in the current
> code, where the destination of this data is to set the value in the
> sensor hardware if it is supported.
> 
> Using the hysteresis in the governor seems like abusing the initial
> purpose of this information.
> 
> Moreover, the hysteresis creates a gray area where the above algorithm
> (DROPPING && !throttle) => state-- or (RAISING && throttle) => state++
> may drop the performances because we will continue mitigating even below
> the threshold.
> 
> As the governor is an open-loop controller, I'm not sure if we can do
> something except adding some kind of low pass filter to prevent
> mitigation bounces.
> 

We have two different use cases for the step wise algorithm, and the 
hysteresis makes sense only in one.

For example, say we are controlling CPU junction temperature at 95C. 
When using step wise, mitigation is applied iteratively and there is a 
possibility that temperature can shoot up before the algorithm can reach 
an optimal mitigation level to keep the temperature near threshold.

In order to help this state machine, we use a second back stop rule in 
the same thermal zone at a higher temperature (say 105C) with a 
hysteresis of 10C to mitigate CPU to a fixed value, by specifying 
upper/lower limit to be the same. The idea is that the second rule will 
place a hard hammer to bring the temperature down close to 95C and then 
it will remove the mitigation. Once mitigation is removed, the junction 
temperature rule state machine will re-adjust from that point to an 
optimal mitigation level. The junction temperature rule doesn’t use 
hysteresis.

Another example is skin temperature mitigation for mobile devices, where 
the step wise algorithm with hysteresis just reduces the operating max 
frequency to a fixed value, when the threshold is reached. And the 
junction temperature rule starts mitigating from this operating max.

That is the reason we have not generalized or mandated the hysteresis 
usage in this patch. The idea is to use it selectively based on use case.


>> Signed-off-by: Ram Chandrasekar <rkumbako@codeaurora.org>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> [Rebased patch from downstream]
>> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
>> ---
>>   drivers/thermal/step_wise.c | 35 ++++++++++++++++++++++++-----------
>>   1 file changed, 24 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
>> index 6e051cbd824ff..2c8a34a7cf959 100644
>> --- a/drivers/thermal/step_wise.c
>> +++ b/drivers/thermal/step_wise.c
>> @@ -24,7 +24,7 @@
>>    *       for this trip point
>>    *    d. if the trend is THERMAL_TREND_DROP_FULL, use lower limit
>>    *       for this trip point
>> - * If the temperature is lower than a trip point,
>> + * If the temperature is lower than a hysteresis temperature,
>>    *    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
>> @@ -115,30 +115,31 @@ static void update_passive_instance(struct thermal_zone_device *tz,
>>   
>>   static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
>>   {
>> -	int trip_temp;
>> +	int trip_temp, hyst_temp;
>>   	enum thermal_trip_type trip_type;
>>   	enum thermal_trend trend;
>>   	struct thermal_instance *instance;
>> -	bool throttle = false;
>> +	bool throttle;
>>   	int old_target;
>>   
>>   	if (trip == THERMAL_TRIPS_NONE) {
>> -		trip_temp = tz->forced_passive;
>> +		hyst_temp = trip_temp = tz->forced_passive;
>>   		trip_type = THERMAL_TRIPS_NONE;
>>   	} else {
>>   		tz->ops->get_trip_temp(tz, trip, &trip_temp);
>> +		hyst_temp = trip_temp;
>> +		if (tz->ops->get_trip_hyst) {
>> +			tz->ops->get_trip_hyst(tz, trip, &hyst_temp);
>> +			hyst_temp = trip_temp - hyst_temp;
>> +		}
>>   		tz->ops->get_trip_type(tz, trip, &trip_type);
>>   	}
>>   
>>   	trend = get_tz_trend(tz, trip);
>>   
>> -	if (tz->temperature >= trip_temp) {
>> -		throttle = true;
>> -		trace_thermal_zone_trip(tz, trip, trip_type);
>> -	}
>> -
>> -	dev_dbg(&tz->device, "Trip%d[type=%d,temp=%d]:trend=%d,throttle=%d\n",
>> -				trip, trip_type, trip_temp, trend, throttle);
>> +	dev_dbg(&tz->device,
>> +		"Trip%d[type=%d,temp=%d,hyst=%d]:trend=%d,throttle=%d\n",
>> +		trip, trip_type, trip_temp, hyst_temp, trend, throttle);
>>   
>>   	mutex_lock(&tz->lock);
>>   
>> @@ -147,6 +148,18 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
>>   			continue;
>>   
>>   		old_target = instance->target;
>> +		throttle = false;
>> +		/*
>> +		 * Lower the mitigation only if the temperature
>> +		 * goes below the hysteresis temperature.
>> +		 */
>> +		if (tz->temperature >= trip_temp ||
>> +		    (tz->temperature >= hyst_temp &&
>> +		     old_target != THERMAL_NO_TARGET)) {
>> +			throttle = true;
>> +			trace_thermal_zone_trip(tz, trip, trip_type);
>> +		}
>> +
>>   		instance->target = get_target_state(instance, trend, throttle);
>>   		dev_dbg(&instance->cdev->device, "old_target=%d, target=%d\n",
>>   					old_target, (int)instance->target);
>>
> 
> 

Re: [PATCH] drivers: thermal: step_wise: add support for hysteresis

[Daniel Lezcano]

On 08/01/2020 01:31, Ram Chandrasekar wrote:
> 
> 
> On 12/11/2019 6:35 AM, Daniel Lezcano wrote:
>> On 21/11/2019 06:50, Amit Kucheria wrote:
>>> From: Ram Chandrasekar <rkumbako@codeaurora.org>
>>>
>>> Currently, step wise governor increases the mitigation when the
>>> temperature goes above a threshold and decreases the mitigation when the
>>> temperature goes below the threshold.
>>>
>>> If there is a case where the
>>> temperature is wavering around the threshold, the mitigation will be
>>> applied and removed every iteration, which is not very efficient.
>>>
>>> The use of hysteresis temperature could avoid this ping-pong of
>>> mitigation by relaxing the mitigation to happen only when the
>>> temperature goes below this lower hysteresis value.
>>
>> What I'm worried about is how the hysteresis is used in the current
>> code, where the destination of this data is to set the value in the
>> sensor hardware if it is supported.
>>
>> Using the hysteresis in the governor seems like abusing the initial
>> purpose of this information.
>>
>> Moreover, the hysteresis creates a gray area where the above algorithm
>> (DROPPING && !throttle) => state-- or (RAISING && throttle) => state++
>> may drop the performances because we will continue mitigating even below
>> the threshold.
>>
>> As the governor is an open-loop controller, I'm not sure if we can do
>> something except adding some kind of low pass filter to prevent
>> mitigation bounces.
>>
> 
> We have two different use cases for the step wise algorithm, and the
> hysteresis makes sense only in one.
> 
> For example, say we are controlling CPU junction temperature at 95C.
> When using step wise, mitigation is applied iteratively and there is a
> possibility that temperature can shoot up before the algorithm can reach
> an optimal mitigation level to keep the temperature near threshold.
> 
> In order to help this state machine, we use a second back stop rule in
> the same thermal zone at a higher temperature (say 105C) with a
> hysteresis of 10C to mitigate CPU to a fixed value, by specifying
> upper/lower limit to be the same. The idea is that the second rule will
> place a hard hammer to bring the temperature down close to 95C and then
> it will remove the mitigation. Once mitigation is removed, the junction
> temperature rule state machine will re-adjust from that point to an
> optimal mitigation level. The junction temperature rule doesn’t use
> hysteresis.
> 
> Another example is skin temperature mitigation for mobile devices, where
> the step wise algorithm with hysteresis just reduces the operating max
> frequency to a fixed value, when the threshold is reached. And the
> junction temperature rule starts mitigating from this operating max.
> 
> That is the reason we have not generalized or mandated the hysteresis
> usage in this patch. The idea is to use it selectively based on use case.

Did you ever try the IPA governor?


-- 
 <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] drivers: thermal: step_wise: add support for hysteresis

[Ram Chandrasekar]

On 1/9/2020 3:46 PM, Daniel Lezcano wrote:
> On 08/01/2020 01:31, Ram Chandrasekar wrote:
>>
>>
>> On 12/11/2019 6:35 AM, Daniel Lezcano wrote:
>>> On 21/11/2019 06:50, Amit Kucheria wrote:
>>>> From: Ram Chandrasekar <rkumbako@codeaurora.org>
>>>>
>>>> Currently, step wise governor increases the mitigation when the
>>>> temperature goes above a threshold and decreases the mitigation when the
>>>> temperature goes below the threshold.
>>>>
>>>> If there is a case where the
>>>> temperature is wavering around the threshold, the mitigation will be
>>>> applied and removed every iteration, which is not very efficient.
>>>>
>>>> The use of hysteresis temperature could avoid this ping-pong of
>>>> mitigation by relaxing the mitigation to happen only when the
>>>> temperature goes below this lower hysteresis value.
>>>
>>> What I'm worried about is how the hysteresis is used in the current
>>> code, where the destination of this data is to set the value in the
>>> sensor hardware if it is supported.
>>>
>>> Using the hysteresis in the governor seems like abusing the initial
>>> purpose of this information.
>>>
>>> Moreover, the hysteresis creates a gray area where the above algorithm
>>> (DROPPING && !throttle) => state-- or (RAISING && throttle) => state++
>>> may drop the performances because we will continue mitigating even below
>>> the threshold.
>>>
>>> As the governor is an open-loop controller, I'm not sure if we can do
>>> something except adding some kind of low pass filter to prevent
>>> mitigation bounces.
>>>
>>
>> We have two different use cases for the step wise algorithm, and the
>> hysteresis makes sense only in one.
>>
>> For example, say we are controlling CPU junction temperature at 95C.
>> When using step wise, mitigation is applied iteratively and there is a
>> possibility that temperature can shoot up before the algorithm can reach
>> an optimal mitigation level to keep the temperature near threshold.
>>
>> In order to help this state machine, we use a second back stop rule in
>> the same thermal zone at a higher temperature (say 105C) with a
>> hysteresis of 10C to mitigate CPU to a fixed value, by specifying
>> upper/lower limit to be the same. The idea is that the second rule will
>> place a hard hammer to bring the temperature down close to 95C and then
>> it will remove the mitigation. Once mitigation is removed, the junction
>> temperature rule state machine will re-adjust from that point to an
>> optimal mitigation level. The junction temperature rule doesn’t use
>> hysteresis.
>>
>> Another example is skin temperature mitigation for mobile devices, where
>> the step wise algorithm with hysteresis just reduces the operating max
>> frequency to a fixed value, when the threshold is reached. And the
>> junction temperature rule starts mitigating from this operating max.
>>
>> That is the reason we have not generalized or mandated the hysteresis
>> usage in this patch. The idea is to use it selectively based on use case.
> 
> Did you ever try the IPA governor?
> 
> 
Yes. We understand IPA could bring down the power/frequency cap multiple 
levels compared to step wise which is doing iteratively. With cooling 
device weights this makes IPA good for skin temperature control. But 
when we evaluated IPA, we found step wise is giving a better performance 
compared to IPA for localized junction temperature rules. Additionally, 
step wise for the junction temperature rule is easier to tune with fewer 
parameters. With these additional enhancements, step wise turned out to 
be a better choice for local junction temperature rules.

[PATCH] drivers: thermal: step_wise: add support for hysteresis

[Lina Iyer]

From: Ram Chandrasekar <rkumbako@codeaurora.org>

From: Ram Chandrasekar <rkumbako@codeaurora.org>

Step wise governor increases the mitigation level when the temperature
goes above a threshold and will decrease the mitigation when the
temperature falls below the threshold. If it were a case, where the
temperature hovers around a threshold, the mitigation will be applied
and removed at every iteration. This reaction to the temperature is
inefficient for performance.

The use of hysteresis temperature could avoid this ping-pong of
mitigation by relaxing the mitigation to happen only when the
temperature goes below this lower hysteresis value.

Signed-off-by: Ram Chandrasekar <rkumbako@codeaurora.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 drivers/thermal/step_wise.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
index ee047ca43084..cf07e2269291 100644
--- a/drivers/thermal/step_wise.c
+++ b/drivers/thermal/step_wise.c
@@ -36,7 +36,7 @@
  *       for this trip point
  *    d. if the trend is THERMAL_TREND_DROP_FULL, use lower limit
  *       for this trip point
- * If the temperature is lower than a trip point,
+ * If the temperature is lower than a hysteresis temperature,
  *    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
@@ -127,7 +127,7 @@ static void update_passive_instance(struct thermal_zone_device *tz,
 
 static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
 {
-	int trip_temp;
+	int trip_temp, hyst_temp;
 	enum thermal_trip_type trip_type;
 	enum thermal_trend trend;
 	struct thermal_instance *instance;
@@ -135,22 +135,23 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
 	int old_target;
 
 	if (trip == THERMAL_TRIPS_NONE) {
-		trip_temp = tz->forced_passive;
+		hyst_temp = trip_temp = tz->forced_passive;
 		trip_type = THERMAL_TRIPS_NONE;
 	} else {
 		tz->ops->get_trip_temp(tz, trip, &trip_temp);
+		hyst_temp = trip_temp;
+		if (tz->ops->get_trip_hyst) {
+			tz->ops->get_trip_hyst(tz, trip, &hyst_temp);
+			hyst_temp = trip_temp - hyst_temp;
+		}
 		tz->ops->get_trip_type(tz, trip, &trip_type);
 	}
 
 	trend = get_tz_trend(tz, trip);
 
-	if (tz->temperature >= trip_temp) {
-		throttle = true;
-		trace_thermal_zone_trip(tz, trip, trip_type);
-	}
-
-	dev_dbg(&tz->device, "Trip%d[type=%d,temp=%d]:trend=%d,throttle=%d\n",
-				trip, trip_type, trip_temp, trend, throttle);
+	dev_dbg(&tz->device,
+		"Trip%d[type=%d,temp=%d,hyst=%d]:trend=%d,throttle=%d\n",
+		trip, trip_type, trip_temp, hyst_temp, trend, throttle);
 
 	mutex_lock(&tz->lock);
 
@@ -159,6 +160,18 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
 			continue;
 
 		old_target = instance->target;
+		throttle = false;
+		/*
+		 * Lower the mitigation only if the temperature
+		 * goes below the hysteresis temperature.
+		 */
+		if (tz->temperature >= trip_temp ||
+		   (tz->temperature >= hyst_temp &&
+		   old_target != THERMAL_NO_TARGET)) {
+			throttle = true;
+			trace_thermal_zone_trip(tz, trip, trip_type);
+		}
+
 		instance->target = get_target_state(instance, trend, throttle);
 		dev_dbg(&instance->cdev->device, "old_target=%d, target=%d\n",
 					old_target, (int)instance->target);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] drivers: thermal: step_wise: add support for hysteresis

[Daniel Lezcano]

On Mon, May 07, 2018 at 11:54:08AM -0600, Lina Iyer wrote:
> From: Ram Chandrasekar <rkumbako@codeaurora.org>
> 
> From: Ram Chandrasekar <rkumbako@codeaurora.org>
> 
> Step wise governor increases the mitigation level when the temperature
> goes above a threshold and will decrease the mitigation when the
> temperature falls below the threshold. If it were a case, where the
> temperature hovers around a threshold, the mitigation will be applied
> and removed at every iteration. This reaction to the temperature is
> inefficient for performance.
> 
> The use of hysteresis temperature could avoid this ping-pong of
> mitigation by relaxing the mitigation to happen only when the
> temperature goes below this lower hysteresis value.

I don't disagree with this but the ping-pong around a temperature is usually
avoided with a P-I-D computation which is implemented with the IPA governor.
Wouldn't be more interesting to add the power numbers like some other
platforms, so the IPA could be used?

You will probably have better results with the IPA than changing the step-wise
governor behavior (which may potentially impact other users).


-- 

 <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] drivers: thermal: step_wise: add support for hysteresis

[Lina Iyer]

Hi Daniel,

On Tue, May 08 2018 at 20:04 -0600, Daniel Lezcano wrote:
>On Mon, May 07, 2018 at 11:54:08AM -0600, Lina Iyer wrote:
>> From: Ram Chandrasekar <rkumbako@codeaurora.org>
>>
>> From: Ram Chandrasekar <rkumbako@codeaurora.org>
>>
>> Step wise governor increases the mitigation level when the temperature
>> goes above a threshold and will decrease the mitigation when the
>> temperature falls below the threshold. If it were a case, where the
>> temperature hovers around a threshold, the mitigation will be applied
>> and removed at every iteration. This reaction to the temperature is
>> inefficient for performance.
>>
>> The use of hysteresis temperature could avoid this ping-pong of
>> mitigation by relaxing the mitigation to happen only when the
>> temperature goes below this lower hysteresis value.
>
>I don't disagree with this but the ping-pong around a temperature is usually
>avoided with a P-I-D computation which is implemented with the IPA governor.
>Wouldn't be more interesting to add the power numbers like some other
>platforms, so the IPA could be used?
>
Possibly. But we have had better thermal performance for our hardware, with
stepwise and custom governor. Much of the mitigation happens through the
firmware and hardware. The stepwise governor works well for us.

>You will probably have better results with the IPA than changing the step-wise
>governor behavior (which may potentially impact other users).
>
This should not impact others who have not implemented the
->get_trip_hyst method.

Thanks,
Lina

Re: [PATCH] drivers: thermal: step_wise: add support for hysteresis

[Zhang Rui]

Hi, Lina,

On 一, 2018-05-07 at 11:54 -0600, Lina Iyer wrote:
> From: Ram Chandrasekar <rkumbako@codeaurora.org>
> 
> From: Ram Chandrasekar <rkumbako@codeaurora.org>
> 
> Step wise governor increases the mitigation level when the
> temperature
> goes above a threshold and will decrease the mitigation when the
> temperature falls below the threshold. If it were a case, where the
> temperature hovers around a threshold, the mitigation will be applied
> and removed at every iteration. This reaction to the temperature is
> inefficient for performance.
> 
> The use of hysteresis temperature could avoid this ping-pong of
> mitigation by relaxing the mitigation to happen only when the
> temperature goes below this lower hysteresis value.
> 
the idea looks okay to me, just some minor comments.

> Signed-off-by: Ram Chandrasekar <rkumbako@codeaurora.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
>  drivers/thermal/step_wise.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/thermal/step_wise.c
> b/drivers/thermal/step_wise.c
> index ee047ca43084..cf07e2269291 100644
> --- a/drivers/thermal/step_wise.c
> +++ b/drivers/thermal/step_wise.c
> @@ -36,7 +36,7 @@
>   *       for this trip point
>   *    d. if the trend is THERMAL_TREND_DROP_FULL, use lower limit
>   *       for this trip point
> - * If the temperature is lower than a trip point,
> + * If the temperature is lower than a hysteresis temperature,

1. if you update this, you should update "if the temperature is higher
than ..." as well.

2. the updated comment does not fully match the code change you made
below.

>   *    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
> @@ -127,7 +127,7 @@ static void update_passive_instance(struct
> thermal_zone_device *tz,
>  
>  static void thermal_zone_trip_update(struct thermal_zone_device *tz,
> int trip)
>  {
> -	int trip_temp;
> +	int trip_temp, hyst_temp;
>  	enum thermal_trip_type trip_type;
>  	enum thermal_trend trend;
>  	struct thermal_instance *instance;
> @@ -135,22 +135,23 @@ static void thermal_zone_trip_update(struct
> thermal_zone_device *tz, int trip)
>  	int old_target;
>  
>  	if (trip == THERMAL_TRIPS_NONE) {
> -		trip_temp = tz->forced_passive;
> +		hyst_temp = trip_temp = tz->forced_passive;
>  		trip_type = THERMAL_TRIPS_NONE;
>  	} else {
>  		tz->ops->get_trip_temp(tz, trip, &trip_temp);
> +		hyst_temp = trip_temp;
> +		if (tz->ops->get_trip_hyst) {
> +			tz->ops->get_trip_hyst(tz, trip,
> &hyst_temp);
> +			hyst_temp = trip_temp - hyst_temp;
> +		}
>  		tz->ops->get_trip_type(tz, trip, &trip_type);
>  	}
>  
>  	trend = get_tz_trend(tz, trip);
>  
> -	if (tz->temperature >= trip_temp) {
> -		throttle = true;
> -		trace_thermal_zone_trip(tz, trip, trip_type);
> -	}
> -
> -	dev_dbg(&tz->device,
> "Trip%d[type=%d,temp=%d]:trend=%d,throttle=%d\n",
> -				trip, trip_type, trip_temp, trend,
> throttle);
> +	dev_dbg(&tz->device,
> +		"Trip%d[type=%d,temp=%d,hyst=%d]:trend=%d,throttle=%
> d\n",
> +		trip, trip_type, trip_temp, hyst_temp, trend,
> throttle);
>  
throttle is not set properly here, so this debug message does not make
sense.

thanks,
rui
>  	mutex_lock(&tz->lock);
>  
> @@ -159,6 +160,18 @@ static void thermal_zone_trip_update(struct
> thermal_zone_device *tz, int trip)
>  			continue;
>  
>  		old_target = instance->target;
> +		throttle = false;
> +		/*
> +		 * Lower the mitigation only if the temperature
> +		 * goes below the hysteresis temperature.
> +		 */
> +		if (tz->temperature >= trip_temp ||
> +		   (tz->temperature >= hyst_temp &&
> +		   old_target != THERMAL_NO_TARGET)) {
> +			throttle = true;
> +			trace_thermal_zone_trip(tz, trip,
> trip_type);
> +		}
> +

>  		instance->target = get_target_state(instance, trend,
> throttle);
>  		dev_dbg(&instance->cdev->device, "old_target=%d,
> target=%d\n",
>  					old_target, (int)instance-
> >target);