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

[Caesar Wang <caesar.upstream-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

在 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


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