[PATCH 0/2] SoC specific trip points and alarm interrupt for imx_thermal

[PATCH 0/2] SoC specific trip points and alarm interrupt for imx_thermal

[Philipp Zabel]

Hi,

After trying Siarhei Siamashka's cpuburn[1] on a consumer rated i.MX6Q
without any cooling implements, I feel like the 2 second polling interval
is not fast enough for sudden outbursts of high CPU activity.

Here is a first shot at using the alarm value comparator interrupt to react
to trip points more quickly (in this version hardware triggers measurements
at 10 Hz). For safer testing I've also made the passive trip point user
configurable.

regards
Philipp

[1] http://ssvb.github.io/2012/04/10/cpuburn-arm-cortex-a9.html

[PATCH 1/2] thermal: imx: dynamic passive and SoC specific critical trip points

[Philipp Zabel]

Set passive and critical trip point values depending on the maximum die
temperature stored in the OCOTP fuses. This allows higher trip points
for industrial and automotive rated i.MX6 SoCs.
Also allow to configure the passive trip point from userspace.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/thermal/imx_thermal.c | 50 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 40 insertions(+), 10 deletions(-)

diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index d16c33c..9387e47 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -55,12 +55,6 @@ enum imx_thermal_trip {
  */
 #define IMX_TEMP_PASSIVE		85000
 
-/*
- * The maximum die temperature on imx parts is 105C, let's give some cushion
- * for noise and possible temperature rise between measurements.
- */
-#define IMX_TEMP_CRITICAL		100000
-
 #define IMX_POLLING_DELAY		2000 /* millisecond */
 #define IMX_PASSIVE_DELAY		1000
 
@@ -70,6 +64,8 @@ struct imx_thermal_data {
 	enum thermal_device_mode mode;
 	struct regmap *tempmon;
 	int c1, c2; /* See formula in imx_get_sensor_data() */
+	unsigned long temp_passive;
+	unsigned long temp_critical;
 };
 
 static int imx_get_temp(struct thermal_zone_device *tz, unsigned long *temp)
@@ -156,15 +152,35 @@ static int imx_get_trip_type(struct thermal_zone_device *tz, int trip,
 static int imx_get_crit_temp(struct thermal_zone_device *tz,
 			     unsigned long *temp)
 {
-	*temp = IMX_TEMP_CRITICAL;
+	struct imx_thermal_data *data = tz->devdata;
+
+	*temp = data->temp_critical;
 	return 0;
 }
 
 static int imx_get_trip_temp(struct thermal_zone_device *tz, int trip,
 			     unsigned long *temp)
 {
-	*temp = (trip == IMX_TRIP_PASSIVE) ? IMX_TEMP_PASSIVE :
-					     IMX_TEMP_CRITICAL;
+	struct imx_thermal_data *data = tz->devdata;
+
+	*temp = (trip == IMX_TRIP_PASSIVE) ? data->temp_passive :
+					     data->temp_critical;
+	return 0;
+}
+
+static int imx_set_trip_temp(struct thermal_zone_device *tz, int trip,
+			     unsigned long temp)
+{
+	struct imx_thermal_data *data = tz->devdata;
+
+	if (trip == IMX_TRIP_CRITICAL)
+		return -EPERM;
+
+	if (temp > IMX_TEMP_PASSIVE)
+		return -EINVAL;
+
+	data->temp_passive = temp;
+
 	return 0;
 }
 
@@ -211,6 +227,7 @@ static const struct thermal_zone_device_ops imx_tz_ops = {
 	.get_trip_type = imx_get_trip_type,
 	.get_trip_temp = imx_get_trip_temp,
 	.get_crit_temp = imx_get_crit_temp,
+	.set_trip_temp = imx_set_trip_temp,
 };
 
 static int imx_get_sensor_data(struct platform_device *pdev)
@@ -267,6 +284,18 @@ static int imx_get_sensor_data(struct platform_device *pdev)
 	data->c1 = 1000 * (t1 - t2) / (n1 - n2);
 	data->c2 = 1000 * t2 - data->c1 * n2;
 
+	/*
+	 * Set the default passive cooling trip point to 20 °C below the
+	 * maximum die temperature. Can be changed from userspace.
+	 */
+	data->temp_passive = 1000 * (t2 - 20);
+
+	/*
+	 * The maximum die temperature is t2, let's give 5 °C cushion
+	 * for noise and possible temperature rise between measurements.
+	 */
+	data->temp_critical = 1000 * (t2 - 5);
+
 	return 0;
 }
 
@@ -314,7 +343,8 @@ static int imx_thermal_probe(struct platform_device *pdev)
 	}
 
 	data->tz = thermal_zone_device_register("imx_thermal_zone",
-						IMX_TRIP_NUM, 0, data,
+						IMX_TRIP_NUM,
+						BIT(IMX_TRIP_PASSIVE), data,
 						&imx_tz_ops, NULL,
 						IMX_PASSIVE_DELAY,
 						IMX_POLLING_DELAY);
-- 
1.8.4.rc0

Re: [PATCH 1/2] thermal: imx: dynamic passive and SoC specific critical trip points

[Shawn Guo]

On Thu, Aug 01, 2013 at 06:33:11PM +0200, Philipp Zabel wrote:
> Set passive and critical trip point values depending on the maximum die
> temperature stored in the OCOTP fuses. This allows higher trip points
> for industrial and automotive rated i.MX6 SoCs.
> Also allow to configure the passive trip point from userspace.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Acked-by: Shawn Guo <shawn.guo@linaro.org>

[PATCH 2/2] thermal: imx: implement thermal alarm interrupt handling

[Philipp Zabel]

Enable automatic measurements at 10 Hz and use the alarm interrupt to react
more quickly to sudden temperature changes above the passive or critical
temperature trip points.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/thermal/imx_thermal.c | 140 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 127 insertions(+), 13 deletions(-)

diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index 9387e47..411be0a 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -12,6 +12,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/mfd/syscon.h>
@@ -31,6 +32,8 @@
 #define MISC0_REFTOP_SELBIASOFF		(1 << 3)
 
 #define TEMPSENSE0			0x0180
+#define TEMPSENSE0_ALARM_VALUE_SHIFT	20
+#define TEMPSENSE0_ALARM_VALUE_MASK	(0xfff << TEMPSENSE0_ALARM_VALUE_SHIFT)
 #define TEMPSENSE0_TEMP_CNT_SHIFT	8
 #define TEMPSENSE0_TEMP_CNT_MASK	(0xfff << TEMPSENSE0_TEMP_CNT_SHIFT)
 #define TEMPSENSE0_FINISHED		(1 << 2)
@@ -66,33 +69,62 @@ struct imx_thermal_data {
 	int c1, c2; /* See formula in imx_get_sensor_data() */
 	unsigned long temp_passive;
 	unsigned long temp_critical;
+	unsigned long alarm_temp;
+	unsigned long last_temp;
+	bool irq_enabled;
+	int irq;
 };
 
+static void imx_set_alarm_temp(struct imx_thermal_data *data,
+			       signed long alarm_temp)
+{
+	struct regmap *map = data->tempmon;
+	int alarm_value;
+
+	data->alarm_temp = alarm_temp;
+	alarm_value = (alarm_temp - data->c2) / data->c1;
+	regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_ALARM_VALUE_MASK);
+	regmap_write(map, TEMPSENSE0 + REG_SET, alarm_value <<
+			TEMPSENSE0_ALARM_VALUE_SHIFT);
+}
+
 static int imx_get_temp(struct thermal_zone_device *tz, unsigned long *temp)
 {
 	struct imx_thermal_data *data = tz->devdata;
 	struct regmap *map = data->tempmon;
-	static unsigned long last_temp;
 	unsigned int n_meas;
+	bool wait;
 	u32 val;
 
-	/*
-	 * Every time we measure the temperature, we will power on the
-	 * temperature sensor, enable measurements, take a reading,
-	 * disable measurements, power off the temperature sensor.
-	 */
-	regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_POWER_DOWN);
-	regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_MEASURE_TEMP);
+	if (data->mode == THERMAL_DEVICE_ENABLED) {
+		/* Check if a measurement is currently in progress */
+		regmap_read(map, TEMPSENSE0, &val);
+		wait = !(val & TEMPSENSE0_FINISHED);
+	} else {
+		/*
+		 * Every time we measure the temperature, we will power on the
+		 * temperature sensor, enable measurements, take a reading,
+		 * disable measurements, power off the temperature sensor.
+		 */
+		regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_POWER_DOWN);
+		regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_MEASURE_TEMP);
+
+		wait = true;
+	}
 
 	/*
 	 * According to the temp sensor designers, it may require up to ~17us
 	 * to complete a measurement.
 	 */
-	usleep_range(20, 50);
+	if (wait)
+		usleep_range(20, 50);
 
 	regmap_read(map, TEMPSENSE0, &val);
-	regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_MEASURE_TEMP);
-	regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_POWER_DOWN);
+
+	if (data->mode != THERMAL_DEVICE_ENABLED) {
+		regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_MEASURE_TEMP);
+		regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_POWER_DOWN);
+	}
 
 	if ((val & TEMPSENSE0_FINISHED) == 0) {
 		dev_dbg(&tz->device, "temp measurement never finished\n");
@@ -104,9 +136,24 @@ static int imx_get_temp(struct thermal_zone_device *tz, unsigned long *temp)
 	/* See imx_get_sensor_data() for formula derivation */
 	*temp = data->c2 + data->c1 * n_meas;
 
-	if (*temp != last_temp) {
+	/* Update alarm value to next higher trip point */
+	if (data->alarm_temp == data->temp_passive && *temp >= data->temp_passive)
+		imx_set_alarm_temp(data, data->temp_critical);
+	if (data->alarm_temp == data->temp_critical && *temp < data->temp_passive) {
+		imx_set_alarm_temp(data, data->temp_passive);
+		dev_dbg(&tz->device, "thermal alarm off: T < %lu\n",
+			data->alarm_temp / 1000);
+	}
+
+	if (*temp != data->last_temp) {
 		dev_dbg(&tz->device, "millicelsius: %ld\n", *temp);
-		last_temp = *temp;
+		data->last_temp = *temp;
+	}
+
+	/* Reenable alarm IRQ if temperature below alarm temperature */
+	if (!data->irq_enabled && *temp < data->alarm_temp) {
+		data->irq_enabled = true;
+		enable_irq(data->irq);
 	}
 
 	return 0;
@@ -126,13 +173,30 @@ static int imx_set_mode(struct thermal_zone_device *tz,
 			enum thermal_device_mode mode)
 {
 	struct imx_thermal_data *data = tz->devdata;
+	struct regmap *map = data->tempmon;
 
 	if (mode == THERMAL_DEVICE_ENABLED) {
 		tz->polling_delay = IMX_POLLING_DELAY;
 		tz->passive_delay = IMX_PASSIVE_DELAY;
+
+		regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_POWER_DOWN);
+		regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_MEASURE_TEMP);
+
+		if (!data->irq_enabled) {
+			data->irq_enabled = true;
+			enable_irq(data->irq);
+		}
 	} else {
+		regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_MEASURE_TEMP);
+		regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_POWER_DOWN);
+
 		tz->polling_delay = 0;
 		tz->passive_delay = 0;
+
+		if (data->irq_enabled) {
+			disable_irq(data->irq);
+			data->irq_enabled = false;
+		}
 	}
 
 	data->mode = mode;
@@ -181,6 +245,8 @@ static int imx_set_trip_temp(struct thermal_zone_device *tz, int trip,
 
 	data->temp_passive = temp;
 
+	imx_set_alarm_temp(data, temp);
+
 	return 0;
 }
 
@@ -299,11 +365,34 @@ static int imx_get_sensor_data(struct platform_device *pdev)
 	return 0;
 }
 
+static irqreturn_t imx_thermal_alarm_irq(int irq, void *dev)
+{
+	struct imx_thermal_data *data = dev;
+
+	disable_irq_nosync(irq);
+	data->irq_enabled = false;
+
+	return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t imx_thermal_alarm_irq_thread(int irq, void *dev)
+{
+	struct imx_thermal_data *data = dev;
+
+	dev_dbg(&data->tz->device, "THERMAL ALARM: T > %lu\n",
+		data->alarm_temp / 1000);
+
+	thermal_zone_device_update(data->tz);
+
+	return IRQ_HANDLED;
+}
+
 static int imx_thermal_probe(struct platform_device *pdev)
 {
 	struct imx_thermal_data *data;
 	struct cpumask clip_cpus;
 	struct regmap *map;
+	int measure_freq;
 	int ret;
 
 	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
@@ -318,6 +407,18 @@ static int imx_thermal_probe(struct platform_device *pdev)
 	}
 	data->tempmon = map;
 
+	data->irq = platform_get_irq(pdev, 0);
+	if (data->irq < 0)
+		return data->irq;
+
+	ret = devm_request_threaded_irq(&pdev->dev, data->irq,
+			imx_thermal_alarm_irq, imx_thermal_alarm_irq_thread,
+			0, "imx_thermal", data);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to request alarm irq: %d\n", ret);
+		return ret;
+	}
+
 	platform_set_drvdata(pdev, data);
 
 	ret = imx_get_sensor_data(pdev);
@@ -356,6 +457,15 @@ static int imx_thermal_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	/* Enable measurements at ~ 10 Hz */
+	regmap_write(map, TEMPSENSE1 + REG_CLR, TEMPSENSE1_MEASURE_FREQ);
+	measure_freq = DIV_ROUND_UP(32768, 10); /* 10 Hz */
+	regmap_write(map, TEMPSENSE1 + REG_SET, measure_freq);
+	imx_set_alarm_temp(data, data->temp_passive);
+	regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_POWER_DOWN);
+	regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_MEASURE_TEMP);
+
+	data->irq_enabled = true;
 	data->mode = THERMAL_DEVICE_ENABLED;
 
 	return 0;
@@ -364,6 +474,10 @@ static int imx_thermal_probe(struct platform_device *pdev)
 static int imx_thermal_remove(struct platform_device *pdev)
 {
 	struct imx_thermal_data *data = platform_get_drvdata(pdev);
+	struct regmap *map = data->tempmon;
+
+	/* Disable measurements */
+	regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_POWER_DOWN);
 
 	thermal_zone_device_unregister(data->tz);
 	cpufreq_cooling_unregister(data->cdev);
-- 
1.8.4.rc0

Re: [PATCH 2/2] thermal: imx: implement thermal alarm interrupt handling

[Shawn Guo]

Philipp,

Thanks for adding interrupt support for the driver.

On Thu, Aug 01, 2013 at 06:33:12PM +0200, Philipp Zabel wrote:
> Enable automatic measurements at 10 Hz and use the alarm interrupt to react
> more quickly to sudden temperature changes above the passive or critical
> temperature trip points.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/thermal/imx_thermal.c | 140 ++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 127 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index 9387e47..411be0a 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -12,6 +12,7 @@
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/init.h>
> +#include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
>  #include <linux/mfd/syscon.h>
> @@ -31,6 +32,8 @@
>  #define MISC0_REFTOP_SELBIASOFF		(1 << 3)
>  
>  #define TEMPSENSE0			0x0180
> +#define TEMPSENSE0_ALARM_VALUE_SHIFT	20
> +#define TEMPSENSE0_ALARM_VALUE_MASK	(0xfff << TEMPSENSE0_ALARM_VALUE_SHIFT)
>  #define TEMPSENSE0_TEMP_CNT_SHIFT	8
>  #define TEMPSENSE0_TEMP_CNT_MASK	(0xfff << TEMPSENSE0_TEMP_CNT_SHIFT)
>  #define TEMPSENSE0_FINISHED		(1 << 2)
> @@ -66,33 +69,62 @@ struct imx_thermal_data {
>  	int c1, c2; /* See formula in imx_get_sensor_data() */
>  	unsigned long temp_passive;
>  	unsigned long temp_critical;
> +	unsigned long alarm_temp;
> +	unsigned long last_temp;
> +	bool irq_enabled;
> +	int irq;
>  };
>  
> +static void imx_set_alarm_temp(struct imx_thermal_data *data,
> +			       signed long alarm_temp)
> +{
> +	struct regmap *map = data->tempmon;
> +	int alarm_value;
> +
> +	data->alarm_temp = alarm_temp;
> +	alarm_value = (alarm_temp - data->c2) / data->c1;
> +	regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_ALARM_VALUE_MASK);
> +	regmap_write(map, TEMPSENSE0 + REG_SET, alarm_value <<
> +			TEMPSENSE0_ALARM_VALUE_SHIFT);
> +}
> +
>  static int imx_get_temp(struct thermal_zone_device *tz, unsigned long *temp)
>  {
>  	struct imx_thermal_data *data = tz->devdata;
>  	struct regmap *map = data->tempmon;
> -	static unsigned long last_temp;
>  	unsigned int n_meas;
> +	bool wait;
>  	u32 val;
>  
> -	/*
> -	 * Every time we measure the temperature, we will power on the
> -	 * temperature sensor, enable measurements, take a reading,
> -	 * disable measurements, power off the temperature sensor.
> -	 */
> -	regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_POWER_DOWN);
> -	regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_MEASURE_TEMP);
> +	if (data->mode == THERMAL_DEVICE_ENABLED) {
> +		/* Check if a measurement is currently in progress */
> +		regmap_read(map, TEMPSENSE0, &val);
> +		wait = !(val & TEMPSENSE0_FINISHED);
> +	} else {
> +		/*
> +		 * Every time we measure the temperature, we will power on the
> +		 * temperature sensor, enable measurements, take a reading,
> +		 * disable measurements, power off the temperature sensor.
> +		 */
> +		regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_POWER_DOWN);
> +		regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_MEASURE_TEMP);

Could imx_get_temp() be possibly called when mode is
!THERMAL_DEVICE_ENABLED?  If not, this will be a piece of code which
will never be called but simply confusing people.

> +
> +		wait = true;
> +	}
>  
>  	/*
>  	 * According to the temp sensor designers, it may require up to ~17us
>  	 * to complete a measurement.
>  	 */
> -	usleep_range(20, 50);
> +	if (wait)
> +		usleep_range(20, 50);
>  
>  	regmap_read(map, TEMPSENSE0, &val);
> -	regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_MEASURE_TEMP);
> -	regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_POWER_DOWN);
> +
> +	if (data->mode != THERMAL_DEVICE_ENABLED) {
> +		regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_MEASURE_TEMP);
> +		regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_POWER_DOWN);
> +	}
>  
>  	if ((val & TEMPSENSE0_FINISHED) == 0) {
>  		dev_dbg(&tz->device, "temp measurement never finished\n");
> @@ -104,9 +136,24 @@ static int imx_get_temp(struct thermal_zone_device *tz, unsigned long *temp)
>  	/* See imx_get_sensor_data() for formula derivation */
>  	*temp = data->c2 + data->c1 * n_meas;
>  
> -	if (*temp != last_temp) {
> +	/* Update alarm value to next higher trip point */
> +	if (data->alarm_temp == data->temp_passive && *temp >= data->temp_passive)
> +		imx_set_alarm_temp(data, data->temp_critical);
> +	if (data->alarm_temp == data->temp_critical && *temp < data->temp_passive) {
> +		imx_set_alarm_temp(data, data->temp_passive);
> +		dev_dbg(&tz->device, "thermal alarm off: T < %lu\n",
> +			data->alarm_temp / 1000);
> +	}

Running the driver in polling mode with step_wise governor, we can have
cooling device (cpufreq) enter different cooling level, when temperature
increases or decreases.  For example, imx6q runs at 1GHz before reaching
passive temperature 85C, and will slow down to 800 MHz when temperature
reaches 85C.  It will continue slowing down 400 MHz if temperature
continues increasing to, saying 87C.  And if temperature decreases, the
frequency will go back to 800 MHz and then 1 GHz.

With interrupt mode, thermal zone update will only be triggered by alarm
temperature.  That says cooling device will transit between different
cooling levels only when passive or critical temperature is reached in
raising thermal trend.  For above example, even when temperature
decreases back to something original below 85C, CPU will still run at
400 MHz, I think.

> +
> +	if (*temp != data->last_temp) {
>  		dev_dbg(&tz->device, "millicelsius: %ld\n", *temp);
> -		last_temp = *temp;
> +		data->last_temp = *temp;
> +	}
> +
> +	/* Reenable alarm IRQ if temperature below alarm temperature */
> +	if (!data->irq_enabled && *temp < data->alarm_temp) {
> +		data->irq_enabled = true;
> +		enable_irq(data->irq);
>  	}
>  
>  	return 0;
> @@ -126,13 +173,30 @@ static int imx_set_mode(struct thermal_zone_device *tz,
>  			enum thermal_device_mode mode)
>  {
>  	struct imx_thermal_data *data = tz->devdata;
> +	struct regmap *map = data->tempmon;
>  
>  	if (mode == THERMAL_DEVICE_ENABLED) {
>  		tz->polling_delay = IMX_POLLING_DELAY;
>  		tz->passive_delay = IMX_PASSIVE_DELAY;
> +
> +		regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_POWER_DOWN);
> +		regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_MEASURE_TEMP);
> +
> +		if (!data->irq_enabled) {
> +			data->irq_enabled = true;
> +			enable_irq(data->irq);
> +		}
>  	} else {
> +		regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_MEASURE_TEMP);
> +		regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_POWER_DOWN);
> +
>  		tz->polling_delay = 0;
>  		tz->passive_delay = 0;
> +
> +		if (data->irq_enabled) {
> +			disable_irq(data->irq);
> +			data->irq_enabled = false;
> +		}
>  	}
>  
>  	data->mode = mode;
> @@ -181,6 +245,8 @@ static int imx_set_trip_temp(struct thermal_zone_device *tz, int trip,
>  
>  	data->temp_passive = temp;
>  
> +	imx_set_alarm_temp(data, temp);
> +
>  	return 0;
>  }
>  
> @@ -299,11 +365,34 @@ static int imx_get_sensor_data(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static irqreturn_t imx_thermal_alarm_irq(int irq, void *dev)
> +{
> +	struct imx_thermal_data *data = dev;
> +
> +	disable_irq_nosync(irq);
> +	data->irq_enabled = false;
> +
> +	return IRQ_WAKE_THREAD;
> +}
> +
> +static irqreturn_t imx_thermal_alarm_irq_thread(int irq, void *dev)
> +{
> +	struct imx_thermal_data *data = dev;
> +
> +	dev_dbg(&data->tz->device, "THERMAL ALARM: T > %lu\n",
> +		data->alarm_temp / 1000);
> +
> +	thermal_zone_device_update(data->tz);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int imx_thermal_probe(struct platform_device *pdev)
>  {
>  	struct imx_thermal_data *data;
>  	struct cpumask clip_cpus;
>  	struct regmap *map;
> +	int measure_freq;
>  	int ret;
>  
>  	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> @@ -318,6 +407,18 @@ static int imx_thermal_probe(struct platform_device *pdev)
>  	}
>  	data->tempmon = map;
>  
> +	data->irq = platform_get_irq(pdev, 0);
> +	if (data->irq < 0)
> +		return data->irq;
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, data->irq,
> +			imx_thermal_alarm_irq, imx_thermal_alarm_irq_thread,
> +			0, "imx_thermal", data);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to request alarm irq: %d\n", ret);
> +		return ret;
> +	}
> +
>  	platform_set_drvdata(pdev, data);
>  
>  	ret = imx_get_sensor_data(pdev);
> @@ -356,6 +457,15 @@ static int imx_thermal_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	/* Enable measurements at ~ 10 Hz */
> +	regmap_write(map, TEMPSENSE1 + REG_CLR, TEMPSENSE1_MEASURE_FREQ);
> +	measure_freq = DIV_ROUND_UP(32768, 10); /* 10 Hz */
> +	regmap_write(map, TEMPSENSE1 + REG_SET, measure_freq);
> +	imx_set_alarm_temp(data, data->temp_passive);
> +	regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_POWER_DOWN);
> +	regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_MEASURE_TEMP);

This is another thing that interrupt support changes comparing to
polling mode.  In polling mode, tempmon circuit consumes power only when
the measurement takes place, while now we have to keep the power on
since initialization.

Shawn

> +
> +	data->irq_enabled = true;
>  	data->mode = THERMAL_DEVICE_ENABLED;
>  
>  	return 0;
> @@ -364,6 +474,10 @@ static int imx_thermal_probe(struct platform_device *pdev)
>  static int imx_thermal_remove(struct platform_device *pdev)
>  {
>  	struct imx_thermal_data *data = platform_get_drvdata(pdev);
> +	struct regmap *map = data->tempmon;
> +
> +	/* Disable measurements */
> +	regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_POWER_DOWN);
>  
>  	thermal_zone_device_unregister(data->tz);
>  	cpufreq_cooling_unregister(data->cdev);
> -- 
> 1.8.4.rc0
> 

Re: [PATCH 2/2] thermal: imx: implement thermal alarm interrupt handling

[Philipp Zabel]

Hi Shawn,

Am Montag, den 05.08.2013, 11:22 +0800 schrieb Shawn Guo:
> Philipp,
> 
> Thanks for adding interrupt support for the driver.
> 
> On Thu, Aug 01, 2013 at 06:33:12PM +0200, Philipp Zabel wrote:
> > Enable automatic measurements at 10 Hz and use the alarm interrupt to react
> > more quickly to sudden temperature changes above the passive or critical
> > temperature trip points.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  drivers/thermal/imx_thermal.c | 140 ++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 127 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> > index 9387e47..411be0a 100644
> > --- a/drivers/thermal/imx_thermal.c
> > +++ b/drivers/thermal/imx_thermal.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/delay.h>
> >  #include <linux/device.h>
> >  #include <linux/init.h>
> > +#include <linux/interrupt.h>
> >  #include <linux/io.h>
> >  #include <linux/kernel.h>
> >  #include <linux/mfd/syscon.h>
> > @@ -31,6 +32,8 @@
> >  #define MISC0_REFTOP_SELBIASOFF		(1 << 3)
> >  
> >  #define TEMPSENSE0			0x0180
> > +#define TEMPSENSE0_ALARM_VALUE_SHIFT	20
> > +#define TEMPSENSE0_ALARM_VALUE_MASK	(0xfff << TEMPSENSE0_ALARM_VALUE_SHIFT)
> >  #define TEMPSENSE0_TEMP_CNT_SHIFT	8
> >  #define TEMPSENSE0_TEMP_CNT_MASK	(0xfff << TEMPSENSE0_TEMP_CNT_SHIFT)
> >  #define TEMPSENSE0_FINISHED		(1 << 2)
> > @@ -66,33 +69,62 @@ struct imx_thermal_data {
> >  	int c1, c2; /* See formula in imx_get_sensor_data() */
> >  	unsigned long temp_passive;
> >  	unsigned long temp_critical;
> > +	unsigned long alarm_temp;
> > +	unsigned long last_temp;
> > +	bool irq_enabled;
> > +	int irq;
> >  };
> >  
> > +static void imx_set_alarm_temp(struct imx_thermal_data *data,
> > +			       signed long alarm_temp)
> > +{
> > +	struct regmap *map = data->tempmon;
> > +	int alarm_value;
> > +
> > +	data->alarm_temp = alarm_temp;
> > +	alarm_value = (alarm_temp - data->c2) / data->c1;
> > +	regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_ALARM_VALUE_MASK);
> > +	regmap_write(map, TEMPSENSE0 + REG_SET, alarm_value <<
> > +			TEMPSENSE0_ALARM_VALUE_SHIFT);
> > +}
> > +
> >  static int imx_get_temp(struct thermal_zone_device *tz, unsigned long *temp)
> >  {
> >  	struct imx_thermal_data *data = tz->devdata;
> >  	struct regmap *map = data->tempmon;
> > -	static unsigned long last_temp;
> >  	unsigned int n_meas;
> > +	bool wait;
> >  	u32 val;
> >  
> > -	/*
> > -	 * Every time we measure the temperature, we will power on the
> > -	 * temperature sensor, enable measurements, take a reading,
> > -	 * disable measurements, power off the temperature sensor.
> > -	 */
> > -	regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_POWER_DOWN);
> > -	regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_MEASURE_TEMP);
> > +	if (data->mode == THERMAL_DEVICE_ENABLED) {
> > +		/* Check if a measurement is currently in progress */
> > +		regmap_read(map, TEMPSENSE0, &val);
> > +		wait = !(val & TEMPSENSE0_FINISHED);
> > +	} else {
> > +		/*
> > +		 * Every time we measure the temperature, we will power on the
> > +		 * temperature sensor, enable measurements, take a reading,
> > +		 * disable measurements, power off the temperature sensor.
> > +		 */
> > +		regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_POWER_DOWN);
> > +		regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_MEASURE_TEMP);
> 
> Could imx_get_temp() be possibly called when mode is
> !THERMAL_DEVICE_ENABLED?  If not, this will be a piece of code which
> will never be called but simply confusing people.

yes:
  echo disabled > /sys/class/thermal/thermal_zone0/mode
  cat /sys/class/thermal/thermal_zone0/temp

Maybe I should extend the comment?

> > +
> > +		wait = true;
> > +	}
> >  
> >  	/*
> >  	 * According to the temp sensor designers, it may require up to ~17us
> >  	 * to complete a measurement.
> >  	 */
> > -	usleep_range(20, 50);
> > +	if (wait)
> > +		usleep_range(20, 50);
> >  
> >  	regmap_read(map, TEMPSENSE0, &val);
> > -	regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_MEASURE_TEMP);
> > -	regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_POWER_DOWN);
> > +
> > +	if (data->mode != THERMAL_DEVICE_ENABLED) {
> > +		regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_MEASURE_TEMP);
> > +		regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_POWER_DOWN);
> > +	}
> >  
> >  	if ((val & TEMPSENSE0_FINISHED) == 0) {
> >  		dev_dbg(&tz->device, "temp measurement never finished\n");
> > @@ -104,9 +136,24 @@ static int imx_get_temp(struct thermal_zone_device *tz, unsigned long *temp)
> >  	/* See imx_get_sensor_data() for formula derivation */
> >  	*temp = data->c2 + data->c1 * n_meas;
> >  
> > -	if (*temp != last_temp) {
> > +	/* Update alarm value to next higher trip point */
> > +	if (data->alarm_temp == data->temp_passive && *temp >= data->temp_passive)
> > +		imx_set_alarm_temp(data, data->temp_critical);
> > +	if (data->alarm_temp == data->temp_critical && *temp < data->temp_passive) {
> > +		imx_set_alarm_temp(data, data->temp_passive);
> > +		dev_dbg(&tz->device, "thermal alarm off: T < %lu\n",
> > +			data->alarm_temp / 1000);
> > +	}
> 
> Running the driver in polling mode with step_wise governor, we can have
> cooling device (cpufreq) enter different cooling level, when temperature
> increases or decreases.  For example, imx6q runs at 1GHz before reaching
> passive temperature 85C, and will slow down to 800 MHz when temperature
> reaches 85C.  It will continue slowing down 400 MHz if temperature
> continues increasing to, saying 87C.  And if temperature decreases, the
> frequency will go back to 800 MHz and then 1 GHz.
> 
> With interrupt mode, thermal zone update will only be triggered by alarm
> temperature.  That says cooling device will transit between different
> cooling levels only when passive or critical temperature is reached in
> raising thermal trend.  For above example, even when temperature
> decreases back to something original below 85C, CPU will still run at
> 400 MHz, I think.

I have not disabled the polling (with 0.5 Hz below and 1 Hz above the
passive trip point), that still works as before.
The interrupt only forces an additional measurement right after the
alarm interrupt. I think for the step_wise governor we should probably
also raise the polling frequency for a while after getting interrupted.

> > +
> > +	if (*temp != data->last_temp) {
> >  		dev_dbg(&tz->device, "millicelsius: %ld\n", *temp);
> > -		last_temp = *temp;
> > +		data->last_temp = *temp;
> > +	}
> > +
> > +	/* Reenable alarm IRQ if temperature below alarm temperature */
> > +	if (!data->irq_enabled && *temp < data->alarm_temp) {
> > +		data->irq_enabled = true;
> > +		enable_irq(data->irq);
> >  	}
> >  
> >  	return 0;
> > @@ -126,13 +173,30 @@ static int imx_set_mode(struct thermal_zone_device *tz,
> >  			enum thermal_device_mode mode)
> >  {
> >  	struct imx_thermal_data *data = tz->devdata;
> > +	struct regmap *map = data->tempmon;
> >  
> >  	if (mode == THERMAL_DEVICE_ENABLED) {
> >  		tz->polling_delay = IMX_POLLING_DELAY;
> >  		tz->passive_delay = IMX_PASSIVE_DELAY;
> > +
> > +		regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_POWER_DOWN);
> > +		regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_MEASURE_TEMP);
> > +
> > +		if (!data->irq_enabled) {
> > +			data->irq_enabled = true;
> > +			enable_irq(data->irq);
> > +		}
> >  	} else {
> > +		regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_MEASURE_TEMP);
> > +		regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_POWER_DOWN);
> > +
> >  		tz->polling_delay = 0;
> >  		tz->passive_delay = 0;
> > +
> > +		if (data->irq_enabled) {
> > +			disable_irq(data->irq);
> > +			data->irq_enabled = false;
> > +		}
> >  	}
> >  
> >  	data->mode = mode;
> > @@ -181,6 +245,8 @@ static int imx_set_trip_temp(struct thermal_zone_device *tz, int trip,
> >  
> >  	data->temp_passive = temp;
> >  
> > +	imx_set_alarm_temp(data, temp);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -299,11 +365,34 @@ static int imx_get_sensor_data(struct platform_device *pdev)
> >  	return 0;
> >  }
> >  
> > +static irqreturn_t imx_thermal_alarm_irq(int irq, void *dev)
> > +{
> > +	struct imx_thermal_data *data = dev;
> > +
> > +	disable_irq_nosync(irq);
> > +	data->irq_enabled = false;
> > +
> > +	return IRQ_WAKE_THREAD;
> > +}
> > +
> > +static irqreturn_t imx_thermal_alarm_irq_thread(int irq, void *dev)
> > +{
> > +	struct imx_thermal_data *data = dev;
> > +
> > +	dev_dbg(&data->tz->device, "THERMAL ALARM: T > %lu\n",
> > +		data->alarm_temp / 1000);
> > +
> > +	thermal_zone_device_update(data->tz);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static int imx_thermal_probe(struct platform_device *pdev)
> >  {
> >  	struct imx_thermal_data *data;
> >  	struct cpumask clip_cpus;
> >  	struct regmap *map;
> > +	int measure_freq;
> >  	int ret;
> >  
> >  	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > @@ -318,6 +407,18 @@ static int imx_thermal_probe(struct platform_device *pdev)
> >  	}
> >  	data->tempmon = map;
> >  
> > +	data->irq = platform_get_irq(pdev, 0);
> > +	if (data->irq < 0)
> > +		return data->irq;
> > +
> > +	ret = devm_request_threaded_irq(&pdev->dev, data->irq,
> > +			imx_thermal_alarm_irq, imx_thermal_alarm_irq_thread,
> > +			0, "imx_thermal", data);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "failed to request alarm irq: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> >  	platform_set_drvdata(pdev, data);
> >  
> >  	ret = imx_get_sensor_data(pdev);
> > @@ -356,6 +457,15 @@ static int imx_thermal_probe(struct platform_device *pdev)
> >  		return ret;
> >  	}
> >  
> > +	/* Enable measurements at ~ 10 Hz */
> > +	regmap_write(map, TEMPSENSE1 + REG_CLR, TEMPSENSE1_MEASURE_FREQ);
> > +	measure_freq = DIV_ROUND_UP(32768, 10); /* 10 Hz */
> > +	regmap_write(map, TEMPSENSE1 + REG_SET, measure_freq);
> > +	imx_set_alarm_temp(data, data->temp_passive);
> > +	regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_POWER_DOWN);
> > +	regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_MEASURE_TEMP);
> 
> This is another thing that interrupt support changes comparing to
> polling mode.  In polling mode, tempmon circuit consumes power only when
> the measurement takes place, while now we have to keep the power on
> since initialization.

That is correct. Is there any information about how much this circuit is
actually consuming? Should this be made configurable?
Certainly polling in 100 ms intervals will consume more energy, and 2 s
intervals are short enough for all hardware configurations.

> Shawn
> 
> > +
> > +	data->irq_enabled = true;
> >  	data->mode = THERMAL_DEVICE_ENABLED;
> >  
> >  	return 0;
> > @@ -364,6 +474,10 @@ static int imx_thermal_probe(struct platform_device *pdev)
> >  static int imx_thermal_remove(struct platform_device *pdev)
> >  {
> >  	struct imx_thermal_data *data = platform_get_drvdata(pdev);
> > +	struct regmap *map = data->tempmon;
> > +
> > +	/* Disable measurements */
> > +	regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_POWER_DOWN);
> >  
> >  	thermal_zone_device_unregister(data->tz);
> >  	cpufreq_cooling_unregister(data->cdev);
> > -- 
> > 1.8.4.rc0
> > 

regards
Philipp

Re: [PATCH 2/2] thermal: imx: implement thermal alarm interrupt handling

[Shawn Guo]

On Mon, Aug 05, 2013 at 09:50:12AM +0200, Philipp Zabel wrote:
> > Could imx_get_temp() be possibly called when mode is
> > !THERMAL_DEVICE_ENABLED?  If not, this will be a piece of code which
> > will never be called but simply confusing people.
> 
> yes:
>   echo disabled > /sys/class/thermal/thermal_zone0/mode
>   cat /sys/class/thermal/thermal_zone0/temp
> 
> Maybe I should extend the comment?
> 
Unnecessary.  I was missing a very valid use case.

<snip>

> > Running the driver in polling mode with step_wise governor, we can have
> > cooling device (cpufreq) enter different cooling level, when temperature
> > increases or decreases.  For example, imx6q runs at 1GHz before reaching
> > passive temperature 85C, and will slow down to 800 MHz when temperature
> > reaches 85C.  It will continue slowing down 400 MHz if temperature
> > continues increasing to, saying 87C.  And if temperature decreases, the
> > frequency will go back to 800 MHz and then 1 GHz.
> > 
> > With interrupt mode, thermal zone update will only be triggered by alarm
> > temperature.  That says cooling device will transit between different
> > cooling levels only when passive or critical temperature is reached in
> > raising thermal trend.  For above example, even when temperature
> > decreases back to something original below 85C, CPU will still run at
> > 400 MHz, I think.
> 
> I have not disabled the polling (with 0.5 Hz below and 1 Hz above the
> passive trip point), that still works as before.
> The interrupt only forces an additional measurement right after the
> alarm interrupt. I think for the step_wise governor we should probably
> also raise the polling frequency for a while after getting interrupted.
> 
Yeah, I forgot about that polling still works, and somehow was confused
by the temperature behavior change that is actually caused by the recent
step_wise governor updates.  Sorry for the noise.

<snip>

> > This is another thing that interrupt support changes comparing to
> > polling mode.  In polling mode, tempmon circuit consumes power only when
> > the measurement takes place, while now we have to keep the power on
> > since initialization.
> 
> That is correct. Is there any information about how much this circuit is
> actually consuming? Should this be made configurable?

I do not have the data.  Consuming less power is desirable, but I think
it's more important to keep chip safe.  That said, I'm happy with the
patch.

Acked-by: Shawn Guo <shawn.guo@linaro.org>

> Certainly polling in 100 ms intervals will consume more energy, and 2 s
> intervals are short enough for all hardware configurations.