[PATCH] thermal/drivers/hisi: Switch to interrupt mode

[PATCH] thermal/drivers/hisi: Switch to interrupt mode

[Daniel Lezcano]

At this moment, we have both the interrupt setup and the polling enabled. The
interrupt does nothing more than forcing an update while the temperature is
polled every second.

We can do much better than that, threshold is set to 65C in the DT and the
passive ooling device enters in the dance when 75C is reached. We need to
sample the temperature at 65C in order to let the IPA gather enough values for
the PID computation. If the SoC is running at a temperature below 65C, we will
be constantly polling for nothing.

This patch disables the sensor when the temperature is below the temperature
and enables it when passing the threshold. It resuls the sensor thermal sensor
driver will have no activity most of the time.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index 39f4627..74ea70d 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -218,6 +218,15 @@ static int hisi_thermal_get_temp(void *__data, int *temp)
 	return 0;
 }
 
+static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
+				       bool on)
+{
+	struct thermal_zone_device *tzd = sensor->tzd;
+
+	tzd->ops->set_mode(tzd,
+		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
+}
+
 static const struct thermal_zone_of_device_ops hisi_of_thermal_ops = {
 	.get_temp = hisi_thermal_get_temp,
 };
@@ -236,12 +245,16 @@ static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
 		dev_crit(&data->pdev->dev, "THERMAL ALARM: %d > %d\n",
 			 temp, sensor->thres_temp);
 
+		hisi_thermal_toggle_sensor(&data->sensor, true);
+
 		thermal_zone_device_update(data->sensor.tzd,
 					   THERMAL_EVENT_UNSPECIFIED);
 
 	} else if (temp < sensor->thres_temp) {
 		dev_crit(&data->pdev->dev, "THERMAL ALARM stopped: %d < %d\n",
 			 temp, sensor->thres_temp);
+
+		hisi_thermal_toggle_sensor(&data->sensor, false);
 	}
 
 	return IRQ_HANDLED;
@@ -286,15 +299,6 @@ static const struct of_device_id of_hisi_thermal_match[] = {
 };
 MODULE_DEVICE_TABLE(of, of_hisi_thermal_match);
 
-static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
-				       bool on)
-{
-	struct thermal_zone_device *tzd = sensor->tzd;
-
-	tzd->ops->set_mode(tzd,
-		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
-}
-
 static int hisi_thermal_setup(struct hisi_thermal_data *data)
 {
 	struct hisi_thermal_sensor *sensor;
@@ -393,7 +397,7 @@ static int hisi_thermal_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	hisi_thermal_toggle_sensor(&data->sensor, true);
+	hisi_thermal_toggle_sensor(&data->sensor, false);
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH] thermal/drivers/hisi: Switch to interrupt mode

[Keerthy]

On Wednesday 27 September 2017 05:24 AM, Daniel Lezcano wrote:
> At this moment, we have both the interrupt setup and the polling enabled. The
> interrupt does nothing more than forcing an update while the temperature is
> polled every second.
> 
> We can do much better than that, threshold is set to 65C in the DT and the
> passive ooling device enters in the dance when 75C is reached. We need to

/s/ooling/cooling

> sample the temperature at 65C in order to let the IPA gather enough values for
> the PID computation. If the SoC is running at a temperature below 65C, we will
> be constantly polling for nothing.
> 
> This patch disables the sensor when the temperature is below the temperature

temperature below 65C you mean?

> and enables it when passing the threshold. It resuls the sensor thermal sensor

/s/resuls/results

> driver will have no activity most of the time.

By chance you miss the interrupt you will never have cooling on again?

> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/hisi_thermal.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
> index 39f4627..74ea70d 100644
> --- a/drivers/thermal/hisi_thermal.c
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -218,6 +218,15 @@ static int hisi_thermal_get_temp(void *__data, int *temp)
>  	return 0;
>  }
>  
> +static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
> +				       bool on)
> +{
> +	struct thermal_zone_device *tzd = sensor->tzd;
> +
> +	tzd->ops->set_mode(tzd,
> +		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
> +}
> +
>  static const struct thermal_zone_of_device_ops hisi_of_thermal_ops = {
>  	.get_temp = hisi_thermal_get_temp,
>  };
> @@ -236,12 +245,16 @@ static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
>  		dev_crit(&data->pdev->dev, "THERMAL ALARM: %d > %d\n",
>  			 temp, sensor->thres_temp);
>  
> +		hisi_thermal_toggle_sensor(&data->sensor, true);
> +
>  		thermal_zone_device_update(data->sensor.tzd,
>  					   THERMAL_EVENT_UNSPECIFIED);
>  
>  	} else if (temp < sensor->thres_temp) {
>  		dev_crit(&data->pdev->dev, "THERMAL ALARM stopped: %d < %d\n",
>  			 temp, sensor->thres_temp);
> +
> +		hisi_thermal_toggle_sensor(&data->sensor, false);
>  	}
>  
>  	return IRQ_HANDLED;
> @@ -286,15 +299,6 @@ static const struct of_device_id of_hisi_thermal_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, of_hisi_thermal_match);
>  
> -static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
> -				       bool on)
> -{
> -	struct thermal_zone_device *tzd = sensor->tzd;
> -
> -	tzd->ops->set_mode(tzd,
> -		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
> -}
> -
>  static int hisi_thermal_setup(struct hisi_thermal_data *data)
>  {
>  	struct hisi_thermal_sensor *sensor;
> @@ -393,7 +397,7 @@ static int hisi_thermal_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	hisi_thermal_toggle_sensor(&data->sensor, true);
> +	hisi_thermal_toggle_sensor(&data->sensor, false);
>  
>  	return 0;
>  }
> 

Re: [PATCH] thermal/drivers/hisi: Switch to interrupt mode

[Daniel Lezcano]

On Wed, Sep 27, 2017 at 01:56:20PM +0530, Keerthy wrote:
> 
> 
> On Wednesday 27 September 2017 05:24 AM, Daniel Lezcano wrote:
> > At this moment, we have both the interrupt setup and the polling enabled. The
> > interrupt does nothing more than forcing an update while the temperature is
> > polled every second.
> > 
> > We can do much better than that, threshold is set to 65C in the DT and the
> > passive ooling device enters in the dance when 75C is reached. We need to
> 
> /s/ooling/cooling
> 
> > sample the temperature at 65C in order to let the IPA gather enough values for
> > the PID computation. If the SoC is running at a temperature below 65C, we will
> > be constantly polling for nothing.
> > 
> > This patch disables the sensor when the temperature is below the temperature
> 
> temperature below 65C you mean?

Yeah ... :/
 
> > and enables it when passing the threshold. It resuls the sensor thermal sensor
> 
> /s/resuls/results
> 
> > driver will have no activity most of the time.
> 
> By chance you miss the interrupt you will never have cooling on again?

Mmh, I'm not sure it is possible to miss an interrupt with this driver but if
it is the case, there are different scenario.

 - The temperature is increasing and reaches the reset value: then the board
   reboots
 - The temperature increases above 65C and then decreases, and increases
   again, the interrupt will fire and the driver will work as expected

 

 <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

[PATCH V2] thermal/drivers/hisi: Switch to interrupt mode

[Daniel Lezcano]

At this moment, we have both the interrupt setup and the polling enabled. The
interrupt does nothing more than forcing an update while the temperature is
polled every second.

We can do much better than that, threshold is set to 65C in the DT and the
passive cooling device enters in the dance when 75C is reached. We need to
sample the temperature at 65C in order to let the IPA gather enough values for
the PID computation. If the SoC is running at a temperature below 65C, we will
be constantly polling for nothing.

This patch disables the sensor when the temperature is below 65C and enables it
when passing the threshold. It results the thermal sensor driver will have no
activity most of the time.

Cc: Keerthy <j-keerthy@ti.com>
Cc: Leo Yang <leo.yan@linaro.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index 39f4627..74ea70d 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -218,6 +218,15 @@ static int hisi_thermal_get_temp(void *__data, int *temp)
 	return 0;
 }
 
+static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
+				       bool on)
+{
+	struct thermal_zone_device *tzd = sensor->tzd;
+
+	tzd->ops->set_mode(tzd,
+		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
+}
+
 static const struct thermal_zone_of_device_ops hisi_of_thermal_ops = {
 	.get_temp = hisi_thermal_get_temp,
 };
@@ -236,12 +245,16 @@ static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
 		dev_crit(&data->pdev->dev, "THERMAL ALARM: %d > %d\n",
 			 temp, sensor->thres_temp);
 
+		hisi_thermal_toggle_sensor(&data->sensor, true);
+
 		thermal_zone_device_update(data->sensor.tzd,
 					   THERMAL_EVENT_UNSPECIFIED);
 
 	} else if (temp < sensor->thres_temp) {
 		dev_crit(&data->pdev->dev, "THERMAL ALARM stopped: %d < %d\n",
 			 temp, sensor->thres_temp);
+
+		hisi_thermal_toggle_sensor(&data->sensor, false);
 	}
 
 	return IRQ_HANDLED;
@@ -286,15 +299,6 @@ static const struct of_device_id of_hisi_thermal_match[] = {
 };
 MODULE_DEVICE_TABLE(of, of_hisi_thermal_match);
 
-static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
-				       bool on)
-{
-	struct thermal_zone_device *tzd = sensor->tzd;
-
-	tzd->ops->set_mode(tzd,
-		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
-}
-
 static int hisi_thermal_setup(struct hisi_thermal_data *data)
 {
 	struct hisi_thermal_sensor *sensor;
@@ -393,7 +397,7 @@ static int hisi_thermal_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	hisi_thermal_toggle_sensor(&data->sensor, true);
+	hisi_thermal_toggle_sensor(&data->sensor, false);
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH V2] thermal/drivers/hisi: Switch to interrupt mode

[Leo Yan]

Hi Daniel,

On Thu, Sep 28, 2017 at 07:13:44AM +0200, Daniel Lezcano wrote:
> At this moment, we have both the interrupt setup and the polling enabled. The
> interrupt does nothing more than forcing an update while the temperature is
> polled every second.
> 
> We can do much better than that, threshold is set to 65C in the DT and the
> passive cooling device enters in the dance when 75C is reached. We need to
> sample the temperature at 65C in order to let the IPA gather enough values for
> the PID computation. If the SoC is running at a temperature below 65C, we will
> be constantly polling for nothing.
> 
> This patch disables the sensor when the temperature is below 65C and enables it
> when passing the threshold. It results the thermal sensor driver will have no
> activity most of the time.
> 
> Cc: Keerthy <j-keerthy@ti.com>
> Cc: Leo Yang <leo.yan@linaro.org>

s/Yang/Yan :) Have tested this patch on Hikey at my side:

Reviewed-by: Leo Yan <leo.yan@linaro.org>
Tested-by: Leo Yan <leo.yan@linaro.org>

> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/hisi_thermal.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
> index 39f4627..74ea70d 100644
> --- a/drivers/thermal/hisi_thermal.c
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -218,6 +218,15 @@ static int hisi_thermal_get_temp(void *__data, int *temp)
>  	return 0;
>  }
>  
> +static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
> +				       bool on)
> +{
> +	struct thermal_zone_device *tzd = sensor->tzd;
> +
> +	tzd->ops->set_mode(tzd,
> +		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
> +}
> +
>  static const struct thermal_zone_of_device_ops hisi_of_thermal_ops = {
>  	.get_temp = hisi_thermal_get_temp,
>  };
> @@ -236,12 +245,16 @@ static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
>  		dev_crit(&data->pdev->dev, "THERMAL ALARM: %d > %d\n",
>  			 temp, sensor->thres_temp);
>  
> +		hisi_thermal_toggle_sensor(&data->sensor, true);
> +
>  		thermal_zone_device_update(data->sensor.tzd,
>  					   THERMAL_EVENT_UNSPECIFIED);
>  
>  	} else if (temp < sensor->thres_temp) {
>  		dev_crit(&data->pdev->dev, "THERMAL ALARM stopped: %d < %d\n",
>  			 temp, sensor->thres_temp);
> +
> +		hisi_thermal_toggle_sensor(&data->sensor, false);
>  	}
>  
>  	return IRQ_HANDLED;
> @@ -286,15 +299,6 @@ static const struct of_device_id of_hisi_thermal_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, of_hisi_thermal_match);
>  
> -static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
> -				       bool on)
> -{
> -	struct thermal_zone_device *tzd = sensor->tzd;
> -
> -	tzd->ops->set_mode(tzd,
> -		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
> -}
> -
>  static int hisi_thermal_setup(struct hisi_thermal_data *data)
>  {
>  	struct hisi_thermal_sensor *sensor;
> @@ -393,7 +397,7 @@ static int hisi_thermal_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	hisi_thermal_toggle_sensor(&data->sensor, true);
> +	hisi_thermal_toggle_sensor(&data->sensor, false);
>  
>  	return 0;
>  }
> -- 
> 2.7.4
> 

Re: [PATCH V2] thermal/drivers/hisi: Switch to interrupt mode

[Daniel Lezcano]

On Thu, Sep 28, 2017 at 02:57:52PM +0800, Leo Yan wrote:
> Hi Daniel,
> 
> On Thu, Sep 28, 2017 at 07:13:44AM +0200, Daniel Lezcano wrote:
> > At this moment, we have both the interrupt setup and the polling enabled. The
> > interrupt does nothing more than forcing an update while the temperature is
> > polled every second.
> > 
> > We can do much better than that, threshold is set to 65C in the DT and the
> > passive cooling device enters in the dance when 75C is reached. We need to
> > sample the temperature at 65C in order to let the IPA gather enough values for
> > the PID computation. If the SoC is running at a temperature below 65C, we will
> > be constantly polling for nothing.
> > 
> > This patch disables the sensor when the temperature is below 65C and enables it
> > when passing the threshold. It results the thermal sensor driver will have no
> > activity most of the time.
> > 
> > Cc: Keerthy <j-keerthy@ti.com>
> > Cc: Leo Yang <leo.yan@linaro.org>
> 
> s/Yang/Yan :) Have tested this patch on Hikey at my side:

Oops sorry :)

> Reviewed-by: Leo Yan <leo.yan@linaro.org>
> Tested-by: Leo Yan <leo.yan@linaro.org>


Great! Thanks for testing.

  -- Daniel

Re: [PATCH V2] thermal/drivers/hisi: Switch to interrupt mode

[Eduardo Valentin]

Hello,

On Thu, Sep 28, 2017 at 09:32:20AM +0200, Daniel Lezcano wrote:
> On Thu, Sep 28, 2017 at 02:57:52PM +0800, Leo Yan wrote:
> > Hi Daniel,
> > 
> > On Thu, Sep 28, 2017 at 07:13:44AM +0200, Daniel Lezcano wrote:
> > > At this moment, we have both the interrupt setup and the polling enabled. The
> > > interrupt does nothing more than forcing an update while the temperature is
> > > polled every second.
> > > 
> > > We can do much better than that, threshold is set to 65C in the DT and the
> > > passive cooling device enters in the dance when 75C is reached. We need to
> > > sample the temperature at 65C in order to let the IPA gather enough values for
> > > the PID computation. If the SoC is running at a temperature below 65C, we will
> > > be constantly polling for nothing.
> > > 
> > > This patch disables the sensor when the temperature is below 65C and enables it
> > > when passing the threshold. It results the thermal sensor driver will have no
> > > activity most of the time.
> > > 
> > > Cc: Keerthy <j-keerthy@ti.com>
> > > Cc: Leo Yang <leo.yan@linaro.org>
> > 
> > s/Yang/Yan :) Have tested this patch on Hikey at my side:
> 
> Oops sorry :)
> 
> > Reviewed-by: Leo Yan <leo.yan@linaro.org>
> > Tested-by: Leo Yan <leo.yan@linaro.org>
> 

Is this still needed after the latest rework done?

> 
> Great! Thanks for testing.
> 
>   -- Daniel

Re: [PATCH V2] thermal/drivers/hisi: Switch to interrupt mode

[Daniel Lezcano]

On 05/12/2017 03:00, Eduardo Valentin wrote:
> Hello,
> 
> On Thu, Sep 28, 2017 at 09:32:20AM +0200, Daniel Lezcano wrote:
>> On Thu, Sep 28, 2017 at 02:57:52PM +0800, Leo Yan wrote:
>>> Hi Daniel,
>>>
>>> On Thu, Sep 28, 2017 at 07:13:44AM +0200, Daniel Lezcano wrote:
>>>> At this moment, we have both the interrupt setup and the polling enabled. The
>>>> interrupt does nothing more than forcing an update while the temperature is
>>>> polled every second.
>>>>
>>>> We can do much better than that, threshold is set to 65C in the DT and the
>>>> passive cooling device enters in the dance when 75C is reached. We need to
>>>> sample the temperature at 65C in order to let the IPA gather enough values for
>>>> the PID computation. If the SoC is running at a temperature below 65C, we will
>>>> be constantly polling for nothing.
>>>>
>>>> This patch disables the sensor when the temperature is below 65C and enables it
>>>> when passing the threshold. It results the thermal sensor driver will have no
>>>> activity most of the time.
>>>>
>>>> Cc: Keerthy <j-keerthy@ti.com>
>>>> Cc: Leo Yang <leo.yan@linaro.org>
>>>
>>> s/Yang/Yan :) Have tested this patch on Hikey at my side:
>>
>> Oops sorry :)
>>
>>> Reviewed-by: Leo Yan <leo.yan@linaro.org>
>>> Tested-by: Leo Yan <leo.yan@linaro.org>
>>
> 
> Is this still needed after the latest rework done?

No longer needed.



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