Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] thermal: imx8mm: Add get_trend ops
@ 2020-05-13  2:58 Anson Huang
  2020-05-22 17:33 ` Daniel Lezcano
  0 siblings, 1 reply; 8+ messages in thread
From: Anson Huang @ 2020-05-13  2:58 UTC (permalink / raw)
  To: rui.zhang, daniel.lezcano, amit.kucheria, shawnguo, s.hauer,
	kernel, festevam, linux-pm, linux-arm-kernel, linux-kernel
  Cc: Linux-imx

Add get_trend ops for i.MX8MM thermal to apply fast cooling
mechanism, when temperature exceeds passive trip point, the
highest cooling action will be applied, and when temperature
drops to lower than the margin below passive trip point, the
lowest cooling action will be applied.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 drivers/thermal/imx8mm_thermal.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/thermal/imx8mm_thermal.c b/drivers/thermal/imx8mm_thermal.c
index e6061e2..8f6a0b8 100644
--- a/drivers/thermal/imx8mm_thermal.c
+++ b/drivers/thermal/imx8mm_thermal.c
@@ -38,6 +38,8 @@
 #define TMU_VER1		0x1
 #define TMU_VER2		0x2
 
+#define IMX_TEMP_COOL_MARGIN	10000
+
 struct thermal_soc_data {
 	u32 num_sensors;
 	u32 version;
@@ -103,8 +105,33 @@ static int tmu_get_temp(void *data, int *temp)
 	return tmu->socdata->get_temp(data, temp);
 }
 
+static int tmu_get_trend(void *p, int trip, enum thermal_trend *trend)
+{
+	struct tmu_sensor *sensor = p;
+	int trip_temp, temp, ret;
+
+	if (!sensor->tzd)
+		return -EINVAL;
+
+	ret = sensor->tzd->ops->get_trip_temp(sensor->tzd, trip, &trip_temp);
+	if (ret)
+		return ret;
+
+	temp = READ_ONCE(sensor->tzd->temperature);
+
+	if (temp > trip_temp)
+		*trend = THERMAL_TREND_RAISE_FULL;
+	else if (temp < (trip_temp - IMX_TEMP_COOL_MARGIN))
+		*trend = THERMAL_TREND_DROP_FULL;
+	else
+		*trend = THERMAL_TREND_STABLE;
+
+	return 0;
+}
+
 static struct thermal_zone_of_device_ops tmu_tz_ops = {
 	.get_temp = tmu_get_temp,
+	.get_trend = tmu_get_trend,
 };
 
 static void imx8mm_tmu_enable(struct imx8mm_tmu *tmu, bool enable)
-- 
2.7.4


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] thermal: imx8mm: Add get_trend ops
  2020-05-13  2:58 [PATCH] thermal: imx8mm: Add get_trend ops Anson Huang
@ 2020-05-22 17:33 ` Daniel Lezcano
  2020-05-23  0:35   ` Anson Huang
  0 siblings, 1 reply; 8+ messages in thread
From: Daniel Lezcano @ 2020-05-22 17:33 UTC (permalink / raw)
  To: Anson Huang, rui.zhang, amit.kucheria, shawnguo, s.hauer, kernel,
	festevam, linux-pm, linux-arm-kernel, linux-kernel
  Cc: Linux-imx

On 13/05/2020 04:58, Anson Huang wrote:
> Add get_trend ops for i.MX8MM thermal to apply fast cooling
> mechanism, when temperature exceeds passive trip point, the
> highest cooling action will be applied, and when temperature
> drops to lower than the margin below passive trip point, the
> lowest cooling action will be applied.

You are not describing what is the goal of this change.

IIUC, the resulting change will be an on/off action. The thermal zone is
mitigated with the highest cooling effect, so the lowest OPP, then the
temperature trend is stable until it goes below the trip - margin where
the mitigation is stopped.

Except, I'm missing something, setting a trip point with a 10000
hysteresis and a cooling map min/max set to the highest opp will result
on the same.


> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  drivers/thermal/imx8mm_thermal.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/thermal/imx8mm_thermal.c b/drivers/thermal/imx8mm_thermal.c
> index e6061e2..8f6a0b8 100644
> --- a/drivers/thermal/imx8mm_thermal.c
> +++ b/drivers/thermal/imx8mm_thermal.c
> @@ -38,6 +38,8 @@
>  #define TMU_VER1		0x1
>  #define TMU_VER2		0x2
>  
> +#define IMX_TEMP_COOL_MARGIN	10000
> +
>  struct thermal_soc_data {
>  	u32 num_sensors;
>  	u32 version;
> @@ -103,8 +105,33 @@ static int tmu_get_temp(void *data, int *temp)
>  	return tmu->socdata->get_temp(data, temp);
>  }
>  
> +static int tmu_get_trend(void *p, int trip, enum thermal_trend *trend)
> +{
> +	struct tmu_sensor *sensor = p;
> +	int trip_temp, temp, ret;
> +
> +	if (!sensor->tzd)
> +		return -EINVAL;
> +
> +	ret = sensor->tzd->ops->get_trip_temp(sensor->tzd, trip, &trip_temp);
> +	if (ret)
> +		return ret;
> +
> +	temp = READ_ONCE(sensor->tzd->temperature);
> +
> +	if (temp > trip_temp)
> +		*trend = THERMAL_TREND_RAISE_FULL;
> +	else if (temp < (trip_temp - IMX_TEMP_COOL_MARGIN))
> +		*trend = THERMAL_TREND_DROP_FULL;
> +	else
> +		*trend = THERMAL_TREND_STABLE;
> +
> +	return 0;
> +}
> +
>  static struct thermal_zone_of_device_ops tmu_tz_ops = {
>  	.get_temp = tmu_get_temp,
> +	.get_trend = tmu_get_trend,
>  };
>  
>  static void imx8mm_tmu_enable(struct imx8mm_tmu *tmu, bool enable)
> 


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] thermal: imx8mm: Add get_trend ops
  2020-05-22 17:33 ` Daniel Lezcano
@ 2020-05-23  0:35   ` Anson Huang
  2020-05-23 12:33     ` Daniel Lezcano
  0 siblings, 1 reply; 8+ messages in thread
From: Anson Huang @ 2020-05-23  0:35 UTC (permalink / raw)
  To: Daniel Lezcano, rui.zhang, amit.kucheria, shawnguo, s.hauer,
	kernel, festevam, linux-pm, linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

Hi, Daniel


> Subject: Re: [PATCH] thermal: imx8mm: Add get_trend ops
> 
> On 13/05/2020 04:58, Anson Huang wrote:
> > Add get_trend ops for i.MX8MM thermal to apply fast cooling mechanism,
> > when temperature exceeds passive trip point, the highest cooling
> > action will be applied, and when temperature drops to lower than the
> > margin below passive trip point, the lowest cooling action will be
> > applied.
> 
> You are not describing what is the goal of this change.

The goal of this change is to make sure whenever temperature exceeds passive trip point,
the highest cooling action will be applied immediately, e.g., if there are many cpufreq OPP,
the default cooling will be step by step, it will take some more rounds to make cpufreq drop
to lowest OPP, while on i.MX, we expect the cpufreq drop to lowest OPP immediately.

> 
> IIUC, the resulting change will be an on/off action. The thermal zone is
> mitigated with the highest cooling effect, so the lowest OPP, then the
> temperature trend is stable until it goes below the trip - margin where the
> mitigation is stopped.

Yes, your understanding is correctly, once the temperature exceeds passive trip point,
the highest cooling action will be applied immediately and then it will be stable there
until temperature drop to trip - margin, then the cooling action will be cancelled, the
margin is to avoid the back and forth near the passive trip point.

> 
> Except, I'm missing something, setting a trip point with a 10000 hysteresis and
> a cooling map min/max set to the highest opp will result on the same.

Yes setting cooling map min/max cooling state to highest OPP will make the highest
cooling action applied immediately, and to have the function of cooling action being
cancelled when temperature drops to trip - margin, I have to define another trip point,
say passive trip point is 85000, and cooling map min/max set to highest OPP in passive
trip point then add another trip point named "active" with 75000, and without any
cooling map in it, right?

If yes, then I think I can try to make the changes in DT instead of thermal driver. 

Thanks,
Anson

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] thermal: imx8mm: Add get_trend ops
  2020-05-23  0:35   ` Anson Huang
@ 2020-05-23 12:33     ` Daniel Lezcano
  2020-05-24  0:50       ` Anson Huang
  0 siblings, 1 reply; 8+ messages in thread
From: Daniel Lezcano @ 2020-05-23 12:33 UTC (permalink / raw)
  To: Anson Huang, rui.zhang, amit.kucheria, shawnguo, s.hauer, kernel,
	festevam, linux-pm, linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

On 23/05/2020 02:35, Anson Huang wrote:
> Hi, Daniel
> 
> 
>> Subject: Re: [PATCH] thermal: imx8mm: Add get_trend ops
>>
>> On 13/05/2020 04:58, Anson Huang wrote:
>>> Add get_trend ops for i.MX8MM thermal to apply fast cooling mechanism,
>>> when temperature exceeds passive trip point, the highest cooling
>>> action will be applied, and when temperature drops to lower than the
>>> margin below passive trip point, the lowest cooling action will be
>>> applied.
>>
>> You are not describing what is the goal of this change.
> 
> The goal of this change is to make sure whenever temperature exceeds passive trip point,
> the highest cooling action will be applied immediately, e.g., if there are many cpufreq OPP,
> the default cooling will be step by step, it will take some more rounds to make cpufreq drop
> to lowest OPP, while on i.MX, we expect the cpufreq drop to lowest OPP immediately.

Whatever the slope of the temperature increase?

>> IIUC, the resulting change will be an on/off action. The thermal zone is
>> mitigated with the highest cooling effect, so the lowest OPP, then the
>> temperature trend is stable until it goes below the trip - margin where the
>> mitigation is stopped.
> 
> Yes, your understanding is correctly, once the temperature exceeds passive trip point,
> the highest cooling action will be applied immediately and then it will be stable there
> until temperature drop to trip - margin, then the cooling action will be cancelled, the
> margin is to avoid the back and forth near the passive trip point.
> 
>>
>> Except, I'm missing something, setting a trip point with a 10000 hysteresis and
>> a cooling map min/max set to the highest opp will result on the same.
> 
> Yes setting cooling map min/max cooling state to highest OPP will make the highest
> cooling action applied immediately, and to have the function of cooling action being
> cancelled when temperature drops to trip - margin, I have to define another trip point,
> say passive trip point is 85000, and cooling map min/max set to highest OPP in passive
> trip point then add another trip point named "active" with 75000, and without any
> cooling map in it, right?

May be I misunderstood but only the change as below is needed. No need
to add a trip point, especially an 'active' trip which is a for an
active cooling device like a fan.

diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index cc7152ecedd9..bea263bd06b4 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -231,10 +231,10 @@ cooling-maps {
 				map0 {
 					trip = <&cpu_alert0>;
 					cooling-device =
-						<&A53_0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-						<&A53_1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-						<&A53_2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-						<&A53_3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+						<&A53_0 2 2>,
+						<&A53_1 2 2>,
+						<&A53_2 2 2>,
+						<&A53_3 2 2>
 				};
 			};
 		};


> If yes, then I think I can try to make the changes in DT instead of thermal driver. 


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] thermal: imx8mm: Add get_trend ops
  2020-05-23 12:33     ` Daniel Lezcano
@ 2020-05-24  0:50       ` Anson Huang
  2020-05-25  2:46         ` Anson Huang
  0 siblings, 1 reply; 8+ messages in thread
From: Anson Huang @ 2020-05-24  0:50 UTC (permalink / raw)
  To: Daniel Lezcano, rui.zhang, amit.kucheria, shawnguo, s.hauer,
	kernel, festevam, linux-pm, linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

Hi, Daniel

> Subject: Re: [PATCH] thermal: imx8mm: Add get_trend ops
> 
> On 23/05/2020 02:35, Anson Huang wrote:
> > Hi, Daniel
> >
> >
> >> Subject: Re: [PATCH] thermal: imx8mm: Add get_trend ops
> >>
> >> On 13/05/2020 04:58, Anson Huang wrote:
> >>> Add get_trend ops for i.MX8MM thermal to apply fast cooling
> >>> mechanism, when temperature exceeds passive trip point, the highest
> >>> cooling action will be applied, and when temperature drops to lower
> >>> than the margin below passive trip point, the lowest cooling action
> >>> will be applied.
> >>
> >> You are not describing what is the goal of this change.
> >
> > The goal of this change is to make sure whenever temperature exceeds
> > passive trip point, the highest cooling action will be applied
> > immediately, e.g., if there are many cpufreq OPP, the default cooling
> > will be step by step, it will take some more rounds to make cpufreq drop to
> lowest OPP, while on i.MX, we expect the cpufreq drop to lowest OPP
> immediately.
> 
> Whatever the slope of the temperature increase?

Yes.

> 
> >> IIUC, the resulting change will be an on/off action. The thermal zone
> >> is mitigated with the highest cooling effect, so the lowest OPP, then
> >> the temperature trend is stable until it goes below the trip - margin
> >> where the mitigation is stopped.
> >
> > Yes, your understanding is correctly, once the temperature exceeds
> > passive trip point, the highest cooling action will be applied
> > immediately and then it will be stable there until temperature drop to
> > trip - margin, then the cooling action will be cancelled, the margin is to avoid
> the back and forth near the passive trip point.
> >
> >>
> >> Except, I'm missing something, setting a trip point with a 10000
> >> hysteresis and a cooling map min/max set to the highest opp will result on
> the same.
> >
> > Yes setting cooling map min/max cooling state to highest OPP will make
> > the highest cooling action applied immediately, and to have the
> > function of cooling action being cancelled when temperature drops to
> > trip - margin, I have to define another trip point, say passive trip
> > point is 85000, and cooling map min/max set to highest OPP in passive
> > trip point then add another trip point named "active" with 75000, and
> without any cooling map in it, right?
> 
> May be I misunderstood but only the change as below is needed. No need to
> add a trip point, especially an 'active' trip which is a for an active cooling
> device like a fan.
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> index cc7152ecedd9..bea263bd06b4 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> @@ -231,10 +231,10 @@ cooling-maps {
>  				map0 {
>  					trip = <&cpu_alert0>;
>  					cooling-device =
> -						<&A53_0 THERMAL_NO_LIMIT
> THERMAL_NO_LIMIT>,
> -						<&A53_1 THERMAL_NO_LIMIT
> THERMAL_NO_LIMIT>,
> -						<&A53_2 THERMAL_NO_LIMIT
> THERMAL_NO_LIMIT>,
> -						<&A53_3 THERMAL_NO_LIMIT
> THERMAL_NO_LIMIT>;
> +						<&A53_0 2 2>,
> +						<&A53_1 2 2>,
> +						<&A53_2 2 2>,
> +						<&A53_3 2 2>
>  				};
>  			};
>  		};
> 
> 

Thanks, I will have a try to see if it meets our expectation.

Anson

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] thermal: imx8mm: Add get_trend ops
  2020-05-24  0:50       ` Anson Huang
@ 2020-05-25  2:46         ` Anson Huang
  2020-05-25 11:04           ` Daniel Lezcano
  0 siblings, 1 reply; 8+ messages in thread
From: Anson Huang @ 2020-05-25  2:46 UTC (permalink / raw)
  To: Daniel Lezcano, rui.zhang, amit.kucheria, shawnguo, s.hauer,
	kernel, festevam, linux-pm, linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

Hi, Daniel


> Subject: RE: [PATCH] thermal: imx8mm: Add get_trend ops
> 
> Hi, Daniel
> 
> > Subject: Re: [PATCH] thermal: imx8mm: Add get_trend ops
> >
> > On 23/05/2020 02:35, Anson Huang wrote:
> > > Hi, Daniel
> > >
> > >
> > >> Subject: Re: [PATCH] thermal: imx8mm: Add get_trend ops
> > >>
> > >> On 13/05/2020 04:58, Anson Huang wrote:
> > >>> Add get_trend ops for i.MX8MM thermal to apply fast cooling
> > >>> mechanism, when temperature exceeds passive trip point, the
> > >>> highest cooling action will be applied, and when temperature drops
> > >>> to lower than the margin below passive trip point, the lowest
> > >>> cooling action will be applied.
> > >>
> > >> You are not describing what is the goal of this change.
> > >
> > > The goal of this change is to make sure whenever temperature exceeds
> > > passive trip point, the highest cooling action will be applied
> > > immediately, e.g., if there are many cpufreq OPP, the default
> > > cooling will be step by step, it will take some more rounds to make
> > > cpufreq drop to
> > lowest OPP, while on i.MX, we expect the cpufreq drop to lowest OPP
> > immediately.
> >
> > Whatever the slope of the temperature increase?
> 
> Yes.
> 
> >
> > >> IIUC, the resulting change will be an on/off action. The thermal
> > >> zone is mitigated with the highest cooling effect, so the lowest
> > >> OPP, then the temperature trend is stable until it goes below the
> > >> trip - margin where the mitigation is stopped.
> > >
> > > Yes, your understanding is correctly, once the temperature exceeds
> > > passive trip point, the highest cooling action will be applied
> > > immediately and then it will be stable there until temperature drop
> > > to trip - margin, then the cooling action will be cancelled, the
> > > margin is to avoid
> > the back and forth near the passive trip point.
> > >
> > >>
> > >> Except, I'm missing something, setting a trip point with a 10000
> > >> hysteresis and a cooling map min/max set to the highest opp will
> > >> result on
> > the same.
> > >
> > > Yes setting cooling map min/max cooling state to highest OPP will
> > > make the highest cooling action applied immediately, and to have the
> > > function of cooling action being cancelled when temperature drops to
> > > trip - margin, I have to define another trip point, say passive trip
> > > point is 85000, and cooling map min/max set to highest OPP in
> > > passive trip point then add another trip point named "active" with
> > > 75000, and
> > without any cooling map in it, right?
> >
> > May be I misunderstood but only the change as below is needed. No need
> > to add a trip point, especially an 'active' trip which is a for an
> > active cooling device like a fan.
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > index cc7152ecedd9..bea263bd06b4 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > @@ -231,10 +231,10 @@ cooling-maps {
> >  				map0 {
> >  					trip = <&cpu_alert0>;
> >  					cooling-device =
> > -						<&A53_0 THERMAL_NO_LIMIT
> > THERMAL_NO_LIMIT>,
> > -						<&A53_1 THERMAL_NO_LIMIT
> > THERMAL_NO_LIMIT>,
> > -						<&A53_2 THERMAL_NO_LIMIT
> > THERMAL_NO_LIMIT>,
> > -						<&A53_3 THERMAL_NO_LIMIT
> > THERMAL_NO_LIMIT>;
> > +						<&A53_0 2 2>,
> > +						<&A53_1 2 2>,
> > +						<&A53_2 2 2>,
> > +						<&A53_3 2 2>
> >  				};
> >  			};
> >  		};
> >
> >
> 
> Thanks, I will have a try to see if it meets our expectation.

I tried modifying the min/max to '2' in cooling map, it works that whenever cooling
action is needed, the max cooling action will be applied. But I also noticed some behaviors
which NOT as expected:

1. to easy the test, I enable the " CONFIG_THERMAL_WRITABLE_TRIPS", and just modify the
passive trip threshold to trigger the cooling action, this is much more easy then putting the board
into an oven to increase the SoC temperature or running many high loading test to increase the temperature,
but when I modify the passive trip threshold to be lower than current temperature, the cooling action is NOT
triggered immediately, it is because the default step_wise governor will NOT trigger the cooling action when
the trend is THERMAL_TREND_STABLE.
But what expected is, when the temperature is exceed the passive trip threshold, the cooling action can be
triggered immediately no matter the trend is stable or raising. That means we have to implement our own
.get_trend callback?

2. No margin for releasing the cooling action, for example, if cooling action is triggered, when the temperature
drops below the passive trip threshold, the cooling action will be cancelled immediately, if SoC keeps running
at full performance, the temperature will increase very soon, which may cause the SoC keep triggering/cancelling
the cooling action around the passive trip threshold. If there is a margin, the situation will be much better.

Do you have any idea/comment about them?

Thanks,
Anson

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] thermal: imx8mm: Add get_trend ops
  2020-05-25  2:46         ` Anson Huang
@ 2020-05-25 11:04           ` Daniel Lezcano
  2020-05-25 15:05             ` Anson Huang
  0 siblings, 1 reply; 8+ messages in thread
From: Daniel Lezcano @ 2020-05-25 11:04 UTC (permalink / raw)
  To: Anson Huang, rui.zhang, amit.kucheria, shawnguo, s.hauer, kernel,
	festevam, linux-pm, linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

On 25/05/2020 04:46, Anson Huang wrote:
> Hi, Daniel

[ ... ]

> I tried modifying the min/max to '2' in cooling map, it works that
> whenever cooling action is needed, the max cooling action will be
> applied. But I also noticed some behaviors which NOT as expected:
> 
> 1. to easy the test, I enable the " CONFIG_THERMAL_WRITABLE_TRIPS",
> and just modify the passive trip threshold to trigger the cooling
> action, this is much more easy then putting the board into an oven to
> increase the SoC temperature or running many high loading test to
> increase the temperature, but when I modify the passive trip
> threshold to be lower than current temperature, the cooling action is
> NOT triggered immediately, it is because the default step_wise
> governor will NOT trigger the cooling action when the trend is
> THERMAL_TREND_STABLE. But what expected is, when the temperature is
> exceed the passive trip threshold, the cooling action can be 
> triggered immediately no matter the trend is stable or raising.

You are right, what is expected is, when the temperature exceeds the
passive trip threshold, a cooling action happens, the trend is raising
in this case.

But in your test, it is not what is happening: the trip point is
changing, not the temperature.

Probably, the cpufreq driver is at its lowest OPP, so there is no room
for more cooling effect when changing the trip point.

IMO, the test is not right as the trip point is decreased to a
temperature where actually the SoC is not hot.

If you want to test it easily, I recommend to use dhrystone, something like:

 dhrystone -t 6 -l 10000

That will make your board to heat immediately.

> That
> means we have to implement our own .get_trend callback?

From my POV it must disappear, because it has little meaning. The
governor is the one which should be dealing with that and call the
corresponding cooling index.

> 2. No margin for releasing the cooling action, for example, if
> cooling action is triggered, when the temperature drops below the
> passive trip threshold, the cooling action will be cancelled
> immediately, if SoC keeps running at full performance, the
> temperature will increase very soon, which may cause the SoC keep
> triggering/cancelling the cooling action around the passive trip
> threshold. If there is a margin, the situation will be much better.
> 
> Do you have any idea/comment about them?

Yes, that is a good point. The hysteresis is supposed to do that. There
is a work done by Andrzej Pietrasiewicz to disable / enable the thermal
zones [1]. I think we should be able to fix that after the changes are done.

  -- Daniel

[1] https://www.spinics.net/lists/netdev/msg644762.html


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] thermal: imx8mm: Add get_trend ops
  2020-05-25 11:04           ` Daniel Lezcano
@ 2020-05-25 15:05             ` Anson Huang
  0 siblings, 0 replies; 8+ messages in thread
From: Anson Huang @ 2020-05-25 15:05 UTC (permalink / raw)
  To: Daniel Lezcano, rui.zhang, amit.kucheria, shawnguo, s.hauer,
	kernel, festevam, linux-pm, linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

Hi, Daniel

> Subject: Re: [PATCH] thermal: imx8mm: Add get_trend ops
> 
> On 25/05/2020 04:46, Anson Huang wrote:
> > Hi, Daniel
> 
> [ ... ]
> 
> > I tried modifying the min/max to '2' in cooling map, it works that
> > whenever cooling action is needed, the max cooling action will be
> > applied. But I also noticed some behaviors which NOT as expected:
> >
> > 1. to easy the test, I enable the " CONFIG_THERMAL_WRITABLE_TRIPS",
> > and just modify the passive trip threshold to trigger the cooling
> > action, this is much more easy then putting the board into an oven to
> > increase the SoC temperature or running many high loading test to
> > increase the temperature, but when I modify the passive trip threshold
> > to be lower than current temperature, the cooling action is NOT
> > triggered immediately, it is because the default step_wise governor
> > will NOT trigger the cooling action when the trend is
> > THERMAL_TREND_STABLE. But what expected is, when the temperature is
> > exceed the passive trip threshold, the cooling action can be triggered
> > immediately no matter the trend is stable or raising.
> 
> You are right, what is expected is, when the temperature exceeds the passive
> trip threshold, a cooling action happens, the trend is raising in this case.
> 
> But in your test, it is not what is happening: the trip point is changing, not the
> temperature.
> 
> Probably, the cpufreq driver is at its lowest OPP, so there is no room for more
> cooling effect when changing the trip point.
> 
> IMO, the test is not right as the trip point is decreased to a temperature where
> actually the SoC is not hot.
> 
> If you want to test it easily, I recommend to use dhrystone, something like:
> 
>  dhrystone -t 6 -l 10000
> 
> That will make your board to heat immediately.

Thanks, I understand. To aligned with the formal test method, I will inform our test
team to update the test case to meet the requirement.

> 
> > That
> > means we have to implement our own .get_trend callback?
> 
> From my POV it must disappear, because it has little meaning. The governor is
> the one which should be dealing with that and call the corresponding cooling
> index.

OK, I will use common .get_trend() implementation.

> 
> > 2. No margin for releasing the cooling action, for example, if cooling
> > action is triggered, when the temperature drops below the passive trip
> > threshold, the cooling action will be cancelled immediately, if SoC
> > keeps running at full performance, the temperature will increase very
> > soon, which may cause the SoC keep triggering/cancelling the cooling
> > action around the passive trip threshold. If there is a margin, the
> > situation will be much better.
> >
> > Do you have any idea/comment about them?
> 
> Yes, that is a good point. The hysteresis is supposed to do that. There is a work
> done by Andrzej Pietrasiewicz to disable / enable the thermal zones [1]. I think
> we should be able to fix that after the changes are done.

OK, then I will wait for this change. So to apply MAX cooling action immediately,
all expected changes for i.MX platforms are to assign min/max cooling index in
DT cooling map, I will summit a patch set then.

Thanks,
Anson.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-13  2:58 [PATCH] thermal: imx8mm: Add get_trend ops Anson Huang
2020-05-22 17:33 ` Daniel Lezcano
2020-05-23  0:35   ` Anson Huang
2020-05-23 12:33     ` Daniel Lezcano
2020-05-24  0:50       ` Anson Huang
2020-05-25  2:46         ` Anson Huang
2020-05-25 11:04           ` Daniel Lezcano
2020-05-25 15:05             ` Anson Huang

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org
	public-inbox-index linux-pm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git