[PATCH 0/4] Thermal: Support for hardware-tracked trip points

[PATCH 0/4] Thermal: Support for hardware-tracked trip points

[Caesar Wang]

The history patches come from Mikko and Sascha.
http://thread.gmane.org/gmane.linux.power-management.general/59451

Now, I pick them up to continue upstream.
Nevermind!

Tis series adds support for hardware trip points. It picks up earlier
work from Mikko Perttunen. Mikko implemented hardware trip points as part
of the device tree support. It was suggested back then to move the
functionality to the thermal core instead of putting more code into the
device tree support. This series does exactly that.

This series patches rebase the conflicts.
Note that the hardware-tracked trip points are very well tested currently.

Verified and tested on https://github.com/Caesar-github/rockchip/tree/wip/fixes-thermal-0425
That's based on linux-kernel 20160422.

[    0.000000] Linux version 4.6.0-rc4-next-20160422-00016-g0ac0bfb-dirty



Sascha Hauer (4):
  thermal: Add support for hardware-tracked trip points
  thermal: of: implement .set_trips for device tree thermal zones
  thermal: streamline get_trend callbacks
  thermal: bang-bang governor: act on lower trip boundary

 drivers/thermal/gov_bang_bang.c                    |  2 +-
 drivers/thermal/of-thermal.c                       | 23 ++++++-----
 drivers/thermal/thermal_core.c                     | 48 ++++++++++++++++++++++
 drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 25 ++++-------
 include/linux/thermal.h                            |  9 +++-
 5 files changed, 78 insertions(+), 29 deletions(-)

-- 
1.9.1

[PATCH 1/4] thermal: Add support for hardware-tracked trip points

[Caesar Wang]

From: Sascha Hauer <s.hauer@pengutronix.de>

This adds support for hardware-tracked trip points to the device tree
thermal sensor framework.

The framework supports an arbitrary number of trip points. Whenever
the current temperature is updated, the trip points immediately
below and above the current temperature are found. A .set_trips
callback is then called with the temperatures. If there is no trip
point above or below the current temperature, the passed trip
temperature will be -INT_MAX or INT_MAX respectively. In this callback,
the driver should program the hardware such that it is notified
when either of these trip points are triggered. When a trip point
is triggered, the driver should call `thermal_zone_device_update'
for the respective thermal zone. This will cause the trip points
to be updated again.

If .set_trips is not implemented, the framework behaves as before.

This patch is based on an earlier version from Mikko Perttunen
<mikko.perttunen@kapsi.fi>

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Caesar Wang <wxt@rock-chips.com>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Cc: linux-pm@vger.kernel.org
---

 drivers/thermal/thermal_core.c | 48 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/thermal.h        |  3 +++
 2 files changed, 51 insertions(+)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index f1db496..cfef8cc 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -520,6 +520,47 @@ exit:
 }
 EXPORT_SYMBOL_GPL(thermal_zone_get_temp);
 
+static void thermal_zone_set_trips(struct thermal_zone_device *tz)
+{
+	int low = -INT_MAX;
+	int high = INT_MAX;
+	int trip_temp, hysteresis;
+	int temp = tz->temperature;
+	int i, ret;
+
+	if (!tz->ops->set_trips)
+		return;
+
+	for (i = 0; i < tz->trips; i++) {
+		int trip_low;
+
+		tz->ops->get_trip_temp(tz, i, &trip_temp);
+		tz->ops->get_trip_hyst(tz, i, &hysteresis);
+
+		trip_low = trip_temp - hysteresis;
+
+		if (trip_low < temp && trip_low > low)
+			low = trip_low;
+
+		if (trip_temp > temp && trip_temp < high)
+			high = trip_temp;
+	}
+
+	/* No need to change trip points */
+	if (tz->prev_low_trip == low && tz->prev_high_trip == high)
+		return;
+
+	tz->prev_low_trip = low;
+	tz->prev_high_trip = high;
+
+	dev_dbg(&tz->device, "new temperature boundaries: %d < x < %d\n",
+			low, high);
+
+	ret = tz->ops->set_trips(tz, low, high);
+	if (ret)
+		dev_err(&tz->device, "Failed to set trips: %d\n", ret);
+}
+
 static void update_temperature(struct thermal_zone_device *tz)
 {
 	int temp, ret;
@@ -569,6 +610,8 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
 
 	update_temperature(tz);
 
+	thermal_zone_set_trips(tz);
+
 	for (count = 0; count < tz->trips; count++)
 		handle_thermal_trip(tz, count);
 }
@@ -754,6 +797,9 @@ trip_point_hyst_store(struct device *dev, struct device_attribute *attr,
 	 */
 	ret = tz->ops->set_trip_hyst(tz, trip, temperature);
 
+	if (!ret)
+		thermal_zone_set_trips(tz);
+
 	return ret ? ret : count;
 }
 
@@ -1843,6 +1889,8 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
 	tz->trips = trips;
 	tz->passive_delay = passive_delay;
 	tz->polling_delay = polling_delay;
+	tz->prev_low_trip = INT_MAX;
+	tz->prev_high_trip = -INT_MAX;
 	/* A new thermal zone needs to be updated anyway. */
 	atomic_set(&tz->need_update, 1);
 
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index e45abe7..e258359 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -98,6 +98,7 @@ struct thermal_zone_device_ops {
 	int (*unbind) (struct thermal_zone_device *,
 		       struct thermal_cooling_device *);
 	int (*get_temp) (struct thermal_zone_device *, int *);
+	int (*set_trips) (struct thermal_zone_device *, int, int);
 	int (*get_mode) (struct thermal_zone_device *,
 			 enum thermal_device_mode *);
 	int (*set_mode) (struct thermal_zone_device *,
@@ -199,6 +200,8 @@ struct thermal_zone_device {
 	int last_temperature;
 	int emul_temperature;
 	int passive;
+	int prev_low_trip;
+	int prev_high_trip;
 	unsigned int forced_passive;
 	atomic_t need_update;
 	struct thermal_zone_device_ops *ops;
-- 
1.9.1

[PATCH 2/4] thermal: of: implement .set_trips for device tree thermal zones

[Caesar Wang]

From: Sascha Hauer <s.hauer@pengutronix.de>

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Caesar Wang <wxt@rock-chips.com>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Cc: linux-pm@vger.kernel.org
---

 drivers/thermal/of-thermal.c | 12 ++++++++++++
 include/linux/thermal.h      |  4 ++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index b8e509c..8722e63 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -101,6 +101,17 @@ static int of_thermal_get_temp(struct thermal_zone_device *tz,
 	return data->ops->get_temp(data->sensor_data, temp);
 }
 
+static int of_thermal_set_trips(struct thermal_zone_device *tz,
+				int low, int high)
+{
+	struct __thermal_zone *data = tz->devdata;
+
+	if (!data->ops || !data->ops->set_trips)
+		return -EINVAL;
+
+	return data->ops->set_trips(data->sensor_data, low, high);
+}
+
 /**
  * of_thermal_get_ntrips - function to export number of available trip
  *			   points.
@@ -427,6 +438,7 @@ thermal_zone_of_add_sensor(struct device_node *zone,
 
 	tzd->ops->get_temp = of_thermal_get_temp;
 	tzd->ops->get_trend = of_thermal_get_trend;
+	tzd->ops->set_trips = of_thermal_set_trips;
 	tzd->ops->set_emul_temp = of_thermal_set_emul_temp;
 	mutex_unlock(&tzd->lock);
 
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index e258359..cb64866 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -336,12 +336,16 @@ struct thermal_genl_event {
  *
  * Optional:
  * @get_trend: a pointer to a function that reads the sensor temperature trend.
+ * @@set_trips: a pointer to a function that sets a temperature window. When
+ *		this window is left the driver must inform the thermal core via
+ *              thermal_zone_device_update.
  * @set_emul_temp: a pointer to a function that sets sensor emulated
  *		   temperature.
  */
 struct thermal_zone_of_device_ops {
 	int (*get_temp)(void *, int *);
 	int (*get_trend)(void *, long *);
+	int (*set_trips)(void *, int, int);
 	int (*set_emul_temp)(void *, int);
 	int (*set_trip_temp)(void *, int, int);
 };
-- 
1.9.1

[PATCH 3/4] thermal: streamline get_trend callbacks

[Caesar Wang]

From: Sascha Hauer <s.hauer@pengutronix.de>

The .get_trend callback in struct thermal_zone_device_ops has
the prototype:
        int (*get_trend) (struct thermal_zone_device *, int,
                          enum thermal_trend *);
whereas the .get_trend callback in struct thermal_zone_of_device_ops
has:
        int (*get_trend)(void *, long *);

Streamline both prototypes and add the trip argument to the OF callback
aswell and use enum thermal_trend * instead of an integer pointer.

While the OF prototype may be the better one, this should be decided at
framework level and not on OF level.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Caesar Wang <wxt@rock-chips.com>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Cc: linux-pm@vger.kernel.org
---

 drivers/thermal/of-thermal.c                       | 11 +---------
 drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 25 +++++++---------------
 include/linux/thermal.h                            |  2 +-
 3 files changed, 10 insertions(+), 28 deletions(-)

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index 8722e63..13833d9 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -202,24 +202,15 @@ static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
 				enum thermal_trend *trend)
 {
 	struct __thermal_zone *data = tz->devdata;
-	long dev_trend;
 	int r;
 
 	if (!data->ops->get_trend)
 		return -EINVAL;
 
-	r = data->ops->get_trend(data->sensor_data, &dev_trend);
+	r = data->ops->get_trend(data->sensor_data, trip, trend);
 	if (r)
 		return r;
 
-	/* TODO: These intervals might have some thresholds, but in core code */
-	if (dev_trend > 0)
-		*trend = THERMAL_TREND_RAISING;
-	else if (dev_trend < 0)
-		*trend = THERMAL_TREND_DROPPING;
-	else
-		*trend = THERMAL_TREND_STABLE;
-
 	return 0;
 }
 
diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
index 15c0a9a..4a6757c 100644
--- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
+++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
@@ -239,7 +239,7 @@ static int ti_thermal_get_trip_temp(struct thermal_zone_device *thermal,
 	return 0;
 }
 
-static int __ti_thermal_get_trend(void *p, long *trend)
+static int __ti_thermal_get_trend(void *p, int trip, enum thermal_trend *trend)
 {
 	struct ti_thermal_data *data = p;
 	struct ti_bandgap *bgp;
@@ -252,22 +252,6 @@ static int __ti_thermal_get_trend(void *p, long *trend)
 	if (ret)
 		return ret;
 
-	*trend = tr;
-
-	return 0;
-}
-
-/* Get the temperature trend callback functions for thermal zone */
-static int ti_thermal_get_trend(struct thermal_zone_device *thermal,
-				int trip, enum thermal_trend *trend)
-{
-	int ret;
-	long tr;
-
-	ret = __ti_thermal_get_trend(thermal->devdata, &tr);
-	if (ret)
-		return ret;
-
 	if (tr > 0)
 		*trend = THERMAL_TREND_RAISING;
 	else if (tr < 0)
@@ -278,6 +262,13 @@ static int ti_thermal_get_trend(struct thermal_zone_device *thermal,
 	return 0;
 }
 
+/* Get the temperature trend callback functions for thermal zone */
+static int ti_thermal_get_trend(struct thermal_zone_device *thermal,
+				int trip, enum thermal_trend *trend)
+{
+	return __ti_thermal_get_trend(thermal->devdata, trip, trend);
+}
+
 /* Get critical temperature callback functions for thermal zone */
 static int ti_thermal_get_crit_temp(struct thermal_zone_device *thermal,
 				    int *temp)
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index cb64866..3b96961 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -344,7 +344,7 @@ struct thermal_genl_event {
  */
 struct thermal_zone_of_device_ops {
 	int (*get_temp)(void *, int *);
-	int (*get_trend)(void *, long *);
+	int (*get_trend)(void *, int, enum thermal_trend *);
 	int (*set_trips)(void *, int, int);
 	int (*set_emul_temp)(void *, int);
 	int (*set_trip_temp)(void *, int, int);
-- 
1.9.1

[PATCH 4/4] thermal: bang-bang governor: act on lower trip boundary

[Caesar Wang]

From: Sascha Hauer <s.hauer@pengutronix.de>

With interrupt driven thermal zones we pass the lower and upper
temperature on which shall be acted, so in the governor we have to act on
the exact lower temperature to be consistent. Otherwise an interrupt maybe
generated on the exact lower temperature, but the bang bang governor does
not react.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Caesar Wang <wxt@rock-chips.com>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Cc: linux-pm@vger.kernel.org

---

 drivers/thermal/gov_bang_bang.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/gov_bang_bang.c b/drivers/thermal/gov_bang_bang.c
index 70836c5..9d1dfea 100644
--- a/drivers/thermal/gov_bang_bang.c
+++ b/drivers/thermal/gov_bang_bang.c
@@ -59,7 +59,7 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
 		if (instance->target == 0 && tz->temperature >= trip_temp)
 			instance->target = 1;
 		else if (instance->target == 1 &&
-				tz->temperature < trip_temp - trip_hyst)
+				tz->temperature <= trip_temp - trip_hyst)
 			instance->target = 0;
 
 		dev_dbg(&instance->cdev->device, "target=%d\n",
-- 
1.9.1

Re: [PATCH 1/4] thermal: Add support for hardware-tracked trip points

[Eduardo Valentin]

A couple of comments as follows,

On Mon, Apr 25, 2016 at 11:02:44AM +0800, Caesar Wang wrote:
> From: Sascha Hauer <s.hauer@pengutronix.de>
> 
> This adds support for hardware-tracked trip points to the device tree
> thermal sensor framework.
> 
> The framework supports an arbitrary number of trip points. Whenever
> the current temperature is updated, the trip points immediately
> below and above the current temperature are found. A .set_trips
> callback is then called with the temperatures. If there is no trip
> point above or below the current temperature, the passed trip
> temperature will be -INT_MAX or INT_MAX respectively. In this callback,
> the driver should program the hardware such that it is notified
> when either of these trip points are triggered. When a trip point
> is triggered, the driver should call `thermal_zone_device_update'
> for the respective thermal zone. This will cause the trip points
> to be updated again.
> 
> If .set_trips is not implemented, the framework behaves as before.
> 
> This patch is based on an earlier version from Mikko Perttunen
> <mikko.perttunen@kapsi.fi>
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> Cc: linux-pm@vger.kernel.org
> ---
> 
>  drivers/thermal/thermal_core.c | 48 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/thermal.h        |  3 +++

Given that this is adding a new feature in the framework, I would prefer
if you could also describe the .set_trips() in the sysfs-api.txt
documentation file.

A short description of the expectation of what the framework is going to
do is also welcome. For example, are drivers supposed to setup the
polling together with the threshold based approach?

>  2 files changed, 51 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index f1db496..cfef8cc 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -520,6 +520,47 @@ exit:
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_get_temp);
>  
> +static void thermal_zone_set_trips(struct thermal_zone_device *tz)
> +{
> +	int low = -INT_MAX;
> +	int high = INT_MAX;
> +	int trip_temp, hysteresis;
> +	int temp = tz->temperature;
> +	int i, ret;
> +
> +	if (!tz->ops->set_trips)
> +		return;
> +
> +	for (i = 0; i < tz->trips; i++) {
> +		int trip_low;
> +
> +		tz->ops->get_trip_temp(tz, i, &trip_temp);
> +		tz->ops->get_trip_hyst(tz, i, &hysteresis);
> +
> +		trip_low = trip_temp - hysteresis;
> +
> +		if (trip_low < temp && trip_low > low)
> +			low = trip_low;
> +
> +		if (trip_temp > temp && trip_temp < high)
> +			high = trip_temp;
> +	}

Did I understand correctly that you will be flooded by IRQs when you
have:
1. One single trip point.
2. Your temp is above trip_temp

With the above, you would program as threshold:
high == trip_temp
low == trip_temp - hyst

And the IRQ would fire immediattely, causing a device update, causing a
reprogramming, causing another irq, and this would continue, until the
temperature goes below trip_temp, right?

> +
> +	/* No need to change trip points */
> +	if (tz->prev_low_trip == low && tz->prev_high_trip == high)
> +		return;
> +
> +	tz->prev_low_trip = low;
> +	tz->prev_high_trip = high;
> +
> +	dev_dbg(&tz->device, "new temperature boundaries: %d < x < %d\n",
> +			low, high);
> +
> +	ret = tz->ops->set_trips(tz, low, high);
> +	if (ret)
> +		dev_err(&tz->device, "Failed to set trips: %d\n", ret);
> +}
> +
>  static void update_temperature(struct thermal_zone_device *tz)
>  {
>  	int temp, ret;
> @@ -569,6 +610,8 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
>  
>  	update_temperature(tz);
>  
> +	thermal_zone_set_trips(tz);
> +
>  	for (count = 0; count < tz->trips; count++)
>  		handle_thermal_trip(tz, count);
>  }
> @@ -754,6 +797,9 @@ trip_point_hyst_store(struct device *dev, struct device_attribute *attr,
>  	 */
>  	ret = tz->ops->set_trip_hyst(tz, trip, temperature);
>  
> +	if (!ret)
> +		thermal_zone_set_trips(tz);
> +

You would probably want to do the same on trip_point_temp_store().

>  	return ret ? ret : count;
>  }
>  
> @@ -1843,6 +1889,8 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>  	tz->trips = trips;
>  	tz->passive_delay = passive_delay;
>  	tz->polling_delay = polling_delay;
> +	tz->prev_low_trip = INT_MAX;
> +	tz->prev_high_trip = -INT_MAX;
>  	/* A new thermal zone needs to be updated anyway. */
>  	atomic_set(&tz->need_update, 1);
>  
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index e45abe7..e258359 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -98,6 +98,7 @@ struct thermal_zone_device_ops {
>  	int (*unbind) (struct thermal_zone_device *,
>  		       struct thermal_cooling_device *);
>  	int (*get_temp) (struct thermal_zone_device *, int *);
> +	int (*set_trips) (struct thermal_zone_device *, int, int);
>  	int (*get_mode) (struct thermal_zone_device *,
>  			 enum thermal_device_mode *);
>  	int (*set_mode) (struct thermal_zone_device *,
> @@ -199,6 +200,8 @@ struct thermal_zone_device {
>  	int last_temperature;
>  	int emul_temperature;
>  	int passive;
> +	int prev_low_trip;
> +	int prev_high_trip;
>  	unsigned int forced_passive;
>  	atomic_t need_update;
>  	struct thermal_zone_device_ops *ops;
> -- 
> 1.9.1
> 

Re: [PATCH 2/4] thermal: of: implement .set_trips for device tree thermal zones

[Eduardo Valentin]

On Mon, Apr 25, 2016 at 11:02:45AM +0800, Caesar Wang wrote:
> From: Sascha Hauer <s.hauer@pengutronix.de>
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> Cc: linux-pm@vger.kernel.org
> ---
> 
>  drivers/thermal/of-thermal.c | 12 ++++++++++++
>  include/linux/thermal.h      |  4 ++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index b8e509c..8722e63 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -101,6 +101,17 @@ static int of_thermal_get_temp(struct thermal_zone_device *tz,
>  	return data->ops->get_temp(data->sensor_data, temp);
>  }
>  
> +static int of_thermal_set_trips(struct thermal_zone_device *tz,
> +				int low, int high)
> +{
> +	struct __thermal_zone *data = tz->devdata;
> +
> +	if (!data->ops || !data->ops->set_trips)
> +		return -EINVAL;
> +
> +	return data->ops->set_trips(data->sensor_data, low, high);
> +}
> +
>  /**
>   * of_thermal_get_ntrips - function to export number of available trip
>   *			   points.
> @@ -427,6 +438,7 @@ thermal_zone_of_add_sensor(struct device_node *zone,
>  
>  	tzd->ops->get_temp = of_thermal_get_temp;
>  	tzd->ops->get_trend = of_thermal_get_trend;
> +	tzd->ops->set_trips = of_thermal_set_trips;
>  	tzd->ops->set_emul_temp = of_thermal_set_emul_temp;
>  	mutex_unlock(&tzd->lock);
>  
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index e258359..cb64866 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -336,12 +336,16 @@ struct thermal_genl_event {
>   *
>   * Optional:
>   * @get_trend: a pointer to a function that reads the sensor temperature trend.
> + * @@set_trips: a pointer to a function that sets a temperature window. When
> + *		this window is left the driver must inform the thermal core via
> + *              thermal_zone_device_update.

Ok. We start to see some documentation and expectation being stated
here. Nice. Please respin the comment on thermal core too, so drivers
that dont use OF will also be aware of this feature and how to use them.

>   * @set_emul_temp: a pointer to a function that sets sensor emulated
>   *		   temperature.
>   */
>  struct thermal_zone_of_device_ops {
>  	int (*get_temp)(void *, int *);
>  	int (*get_trend)(void *, long *);
> +	int (*set_trips)(void *, int, int);
>  	int (*set_emul_temp)(void *, int);
>  	int (*set_trip_temp)(void *, int, int);
>  };
> -- 
> 1.9.1
> 

Re: [PATCH 4/4] thermal: bang-bang governor: act on lower trip boundary

[Eduardo Valentin]

On Mon, Apr 25, 2016 at 11:02:47AM +0800, Caesar Wang wrote:
> From: Sascha Hauer <s.hauer@pengutronix.de>
> 
> With interrupt driven thermal zones we pass the lower and upper
> temperature on which shall be acted, so in the governor we have to act on
> the exact lower temperature to be consistent. Otherwise an interrupt maybe
> generated on the exact lower temperature, but the bang bang governor does
> not react.

What is the expected impact on polling driven zones that use bang bang
after this change?

> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> Cc: linux-pm@vger.kernel.org
> 
> ---
> 
>  drivers/thermal/gov_bang_bang.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/gov_bang_bang.c b/drivers/thermal/gov_bang_bang.c
> index 70836c5..9d1dfea 100644
> --- a/drivers/thermal/gov_bang_bang.c
> +++ b/drivers/thermal/gov_bang_bang.c
> @@ -59,7 +59,7 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
>  		if (instance->target == 0 && tz->temperature >= trip_temp)
>  			instance->target = 1;
>  		else if (instance->target == 1 &&
> -				tz->temperature < trip_temp - trip_hyst)
> +				tz->temperature <= trip_temp - trip_hyst)
>  			instance->target = 0;
>  
>  		dev_dbg(&instance->cdev->device, "target=%d\n",
> -- 
> 1.9.1
> 

Re: [PATCH 4/4] thermal: bang-bang governor: act on lower trip boundary

[Sascha Hauer]

On Wed, Apr 27, 2016 at 02:54:26PM -0700, Eduardo Valentin wrote:
> On Mon, Apr 25, 2016 at 11:02:47AM +0800, Caesar Wang wrote:
> > From: Sascha Hauer <s.hauer@pengutronix.de>
> > 
> > With interrupt driven thermal zones we pass the lower and upper
> > temperature on which shall be acted, so in the governor we have to act on
> > the exact lower temperature to be consistent. Otherwise an interrupt maybe
> > generated on the exact lower temperature, but the bang bang governor does
> > not react.
> 
> What is the expected impact on polling driven zones that use bang bang
> after this change?

Polling driven zones may have to be one step cooler before the governor
reacts, otherwise the behaviour should be unaffected.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 4/4] thermal: bang-bang governor: act on lower trip boundary

[Eduardo Valentin]

On Thu, Apr 28, 2016 at 08:30:18AM +0200, Sascha Hauer wrote:
> On Wed, Apr 27, 2016 at 02:54:26PM -0700, Eduardo Valentin wrote:
> > On Mon, Apr 25, 2016 at 11:02:47AM +0800, Caesar Wang wrote:
> > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > 
> > > With interrupt driven thermal zones we pass the lower and upper
> > > temperature on which shall be acted, so in the governor we have to act on
> > > the exact lower temperature to be consistent. Otherwise an interrupt maybe
> > > generated on the exact lower temperature, but the bang bang governor does
> > > not react.
> > 
> > What is the expected impact on polling driven zones that use bang bang
> > after this change?
> 
> Polling driven zones may have to be one step cooler before the governor
> reacts, otherwise the behaviour should be unaffected.

OK. Can we add this description of the expectation of what will happen
to polling driver zones in the next version? An explanation on how this
has bee tested on polling driven zones it is also welcome.

> 
> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/4] thermal: Add support for hardware-tracked trip points

[Caesar Wang]

在 2016年04月28日 05:48, Eduardo Valentin 写道:
> A couple of comments as follows,
>
> On Mon, Apr 25, 2016 at 11:02:44AM +0800, Caesar Wang wrote:
>> From: Sascha Hauer <s.hauer@pengutronix.de>
>>
>> This adds support for hardware-tracked trip points to the device tree
>> thermal sensor framework.
>>
>> The framework supports an arbitrary number of trip points. Whenever
>> the current temperature is updated, the trip points immediately
>> below and above the current temperature are found. A .set_trips
>> callback is then called with the temperatures. If there is no trip
>> point above or below the current temperature, the passed trip
>> temperature will be -INT_MAX or INT_MAX respectively. In this callback,
>> the driver should program the hardware such that it is notified
>> when either of these trip points are triggered. When a trip point
>> is triggered, the driver should call `thermal_zone_device_update'
>> for the respective thermal zone. This will cause the trip points
>> to be updated again.
>>
>> If .set_trips is not implemented, the framework behaves as before.
>>
>> This patch is based on an earlier version from Mikko Perttunen
>> <mikko.perttunen@kapsi.fi>
>>
>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
>> Cc: Zhang Rui <rui.zhang@intel.com>
>> Cc: Eduardo Valentin <edubezval@gmail.com>
>> Cc: linux-pm@vger.kernel.org
>> ---
>>
>>   drivers/thermal/thermal_core.c | 48 ++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/thermal.h        |  3 +++
> Given that this is adding a new feature in the framework, I would prefer
> if you could also describe the .set_trips() in the sysfs-api.txt
> documentation file.

Okay, done.
>
> A short description of the expectation of what the framework is going to
> do is also welcome. For example, are drivers supposed to setup the
> polling together with the threshold based approach?
>
>>   2 files changed, 51 insertions(+)
>>
>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>> index f1db496..cfef8cc 100644
>> --- a/drivers/thermal/thermal_core.c
>> +++ b/drivers/thermal/thermal_core.c
>> @@ -520,6 +520,47 @@ exit:
>>   }
>>   EXPORT_SYMBOL_GPL(thermal_zone_get_temp);
>>   
>> +static void thermal_zone_set_trips(struct thermal_zone_device *tz)
>> +{
>> +	int low = -INT_MAX;
>> +	int high = INT_MAX;
>> +	int trip_temp, hysteresis;
>> +	int temp = tz->temperature;
>> +	int i, ret;
>> +
>> +	if (!tz->ops->set_trips)
>> +		return;
>> +
>> +	for (i = 0; i < tz->trips; i++) {
>> +		int trip_low;
>> +
>> +		tz->ops->get_trip_temp(tz, i, &trip_temp);
>> +		tz->ops->get_trip_hyst(tz, i, &hysteresis);
>> +
>> +		trip_low = trip_temp - hysteresis;
>> +
>> +		if (trip_low < temp && trip_low > low)
>> +			low = trip_low;
>> +
>> +		if (trip_temp > temp && trip_temp < high)
>> +			high = trip_temp;
>> +	}
> Did I understand correctly that you will be flooded by IRQs when you
> have:
> 1. One single trip point.
> 2. Your temp is above trip_temp
>
> With the above, you would program as threshold:
> high == trip_temp
> low == trip_temp - hyst
>
> And the IRQ would fire immediattely, causing a device update, causing a
> reprogramming, causing another irq, and this would continue, until the
> temperature goes below trip_temp, right?

Right, As the example tested by the rockchip platform. (70/75/ degree is 
the trip point)

..
[  663.984327] rockchip-thermal ff260000.tsadc: sensor 0 - temp: 66111, 
retval: 0
[  664.048326] rockchip-thermal ff260000.tsadc: sensor 1 - temp: 68333, 
retval: 0
[  664.055600] rockchip-thermal ff260000.tsadc: 
rockchip_thermal_set_trips: sensor 1: low: 68000, high 70000
[  664.992322] rockchip-thermal ff260000.tsadc: sensor 0 - temp: 68888, 
retval: 0
[  664.999579] rockchip-thermal ff260000.tsadc: 
rockchip_thermal_set_trips: sensor 0: low: 68000, high 70000
[  665.066322] rockchip-thermal ff260000.tsadc: sensor 1 - temp: 68333, 
retval: 0
...

[ 4250.474102] rockchip-thermal ff260000.tsadc: sensor 0 - temp: 73333, 
retval: 0
[ 4250.481432] rockchip-thermal ff260000.tsadc: sensor 1 - temp: 74444, 
retval: 0
[ 4250.488792] rockchip-thermal ff260000.tsadc: 
rockchip_thermal_set_trips: sensor 1: low: 73000, high 75000
[ 4250.581360] rockchip-thermal ff260000.tsadc: sensor 0 - temp: 72777, 
retval: 0
[ 4250.588767] rockchip-thermal ff260000.tsadc: 
rockchip_thermal_set_trips: sensor 0: low: 68000, high 75000
[ 4250.598735] rockchip-thermal ff260000.tsadc: sensor 1 - temp: 75000, 
retval: 0
[ 4250.606065] rockchip-thermal ff260000.tsadc: 
rockchip_thermal_set_trips: sensor 1: low: 73000, high 95000
...


>> +
>> +	/* No need to change trip points */
>> +	if (tz->prev_low_trip == low && tz->prev_high_trip == high)
>> +		return;
>> +
>> +	tz->prev_low_trip = low;
>> +	tz->prev_high_trip = high;
>> +
>> +	dev_dbg(&tz->device, "new temperature boundaries: %d < x < %d\n",
>> +			low, high);
>> +
>> +	ret = tz->ops->set_trips(tz, low, high);
>> +	if (ret)
>> +		dev_err(&tz->device, "Failed to set trips: %d\n", ret);
>> +}
>> +
>>   static void update_temperature(struct thermal_zone_device *tz)
>>   {
>>   	int temp, ret;
>> @@ -569,6 +610,8 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
>>   
>>   	update_temperature(tz);
>>   
>> +	thermal_zone_set_trips(tz);
>> +
>>   	for (count = 0; count < tz->trips; count++)
>>   		handle_thermal_trip(tz, count);
>>   }
>> @@ -754,6 +797,9 @@ trip_point_hyst_store(struct device *dev, struct device_attribute *attr,
>>   	 */
>>   	ret = tz->ops->set_trip_hyst(tz, trip, temperature);
>>   
>> +	if (!ret)
>> +		thermal_zone_set_trips(tz);
>> +
> You would probably want to do the same on trip_point_temp_store().

Sure.
>>   	return ret ? ret : count;
>>   }
>>   
>> @@ -1843,6 +1889,8 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>>   	tz->trips = trips;
>>   	tz->passive_delay = passive_delay;
>>   	tz->polling_delay = polling_delay;
>> +	tz->prev_low_trip = INT_MAX;
>> +	tz->prev_high_trip = -INT_MAX;
>>   	/* A new thermal zone needs to be updated anyway. */
>>   	atomic_set(&tz->need_update, 1);
>>   
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index e45abe7..e258359 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -98,6 +98,7 @@ struct thermal_zone_device_ops {
>>   	int (*unbind) (struct thermal_zone_device *,
>>   		       struct thermal_cooling_device *);
>>   	int (*get_temp) (struct thermal_zone_device *, int *);
>> +	int (*set_trips) (struct thermal_zone_device *, int, int);
>>   	int (*get_mode) (struct thermal_zone_device *,
>>   			 enum thermal_device_mode *);
>>   	int (*set_mode) (struct thermal_zone_device *,
>> @@ -199,6 +200,8 @@ struct thermal_zone_device {
>>   	int last_temperature;
>>   	int emul_temperature;
>>   	int passive;
>> +	int prev_low_trip;
>> +	int prev_high_trip;
>>   	unsigned int forced_passive;
>>   	atomic_t need_update;
>>   	struct thermal_zone_device_ops *ops;
>> -- 
>> 1.9.1
>>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip


-- 
Thanks,
Caesar

Re: [PATCH 2/4] thermal: of: implement .set_trips for device tree thermal zones

[Caesar Wang]

在 2016年04月28日 05:52, Eduardo Valentin 写道:
> On Mon, Apr 25, 2016 at 11:02:45AM +0800, Caesar Wang wrote:
>> From: Sascha Hauer <s.hauer@pengutronix.de>
>>
>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
>> Cc: Zhang Rui <rui.zhang@intel.com>
>> Cc: Eduardo Valentin <edubezval@gmail.com>
>> Cc: linux-pm@vger.kernel.org
>> ---
>>
>>   drivers/thermal/of-thermal.c | 12 ++++++++++++
>>   include/linux/thermal.h      |  4 ++++
>>   2 files changed, 16 insertions(+)
>>
>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>> index b8e509c..8722e63 100644
>> --- a/drivers/thermal/of-thermal.c
>> +++ b/drivers/thermal/of-thermal.c
>> @@ -101,6 +101,17 @@ static int of_thermal_get_temp(struct thermal_zone_device *tz,
>>   	return data->ops->get_temp(data->sensor_data, temp);
>>   }
>>   
>> +static int of_thermal_set_trips(struct thermal_zone_device *tz,
>> +				int low, int high)
>> +{
>> +	struct __thermal_zone *data = tz->devdata;
>> +
>> +	if (!data->ops || !data->ops->set_trips)
>> +		return -EINVAL;
>> +
>> +	return data->ops->set_trips(data->sensor_data, low, high);
>> +}
>> +
>>   /**
>>    * of_thermal_get_ntrips - function to export number of available trip
>>    *			   points.
>> @@ -427,6 +438,7 @@ thermal_zone_of_add_sensor(struct device_node *zone,
>>   
>>   	tzd->ops->get_temp = of_thermal_get_temp;
>>   	tzd->ops->get_trend = of_thermal_get_trend;
>> +	tzd->ops->set_trips = of_thermal_set_trips;
>>   	tzd->ops->set_emul_temp = of_thermal_set_emul_temp;
>>   	mutex_unlock(&tzd->lock);
>>   
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index e258359..cb64866 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -336,12 +336,16 @@ struct thermal_genl_event {
>>    *
>>    * Optional:
>>    * @get_trend: a pointer to a function that reads the sensor temperature trend.
>> + * @@set_trips: a pointer to a function that sets a temperature window. When
>> + *		this window is left the driver must inform the thermal core via
>> + *              thermal_zone_device_update.
> Ok. We start to see some documentation and expectation being stated
> here. Nice. Please respin the comment on thermal core too, so drivers
> that dont use OF will also be aware of this feature and how to use them.

Okay, done

>
>>    * @set_emul_temp: a pointer to a function that sets sensor emulated
>>    *		   temperature.
>>    */
>>   struct thermal_zone_of_device_ops {
>>   	int (*get_temp)(void *, int *);
>>   	int (*get_trend)(void *, long *);
>> +	int (*set_trips)(void *, int, int);
>>   	int (*set_emul_temp)(void *, int);
>>   	int (*set_trip_temp)(void *, int, int);
>>   };
>> -- 
>> 1.9.1
>>
>
>

-- 
caesar wang | software engineer | wxt@rock-chip.com

Re: [PATCH 1/4] thermal: Add support for hardware-tracked trip points

[Caesar Wang]

在 2016年04月28日 05:48, Eduardo Valentin 写道:
> This patch is based on an earlier version from Mikko Perttunen
> <mikko.perttunen@kapsi.fi>
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> Cc: linux-pm@vger.kernel.org
> ---
>
>   drivers/thermal/thermal_core.c | 48 ++++++++++++++++++++++++++++++++++++++++++
>   include/linux/thermal.h        |  3 +++
>>
>
>   static void update_temperature(struct thermal_zone_device *tz)
>   {
>   	int temp, ret;
> @@ -569,6 +610,8 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
>   
>   	update_temperature(tz);
>   
> +	thermal_zone_set_trips(tz);
> +
>   	for (count = 0; count < tz->trips; count++)
>   		handle_thermal_trip(tz, count);
>   }
> @@ -754,6 +797,9 @@ trip_point_hyst_store(struct device *dev, struct device_attribute *attr,
>   	 */
>   	ret = tz->ops->set_trip_hyst(tz, trip, temperature);
>   
> +	if (!ret)
> +		thermal_zone_set_trips(tz);
> +
> You would probably want to do the same on trip_point_temp_store().
>

Sorry, that has been set in thermal_zone_device_update().

static ssize_t trip_point_temp_store()
{
     ..
     thermal_zone_device_update(tz);
     ..
}

> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip


-- 
Thanks,
Caesar