| Message ID | 1403856699-2140-2-git-send-email-mperttunen@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Eduardo Valentin |
| Headers | show |
On 06/27/2014 02:11 AM, Mikko Perttunen wrote: > 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 sensor driver > callback `set_trips' is then called with the temperatures. > If there is no trip point above or below the current temperature, > the passed trip temperature will be LONG_MAX or LONG_MIN 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 the `set_trips' callback is not implemented (is NULL), the framework > behaves as before. Is there no "core thermal" code? I would have expected this new feature to be implemented in "core" code rather than of/dt "support" code. Perhaps there would also be some additions to the of/dt code, but I'd still expect the bulk of the feature to be complete independant of of/dt. Systems still using board files or ACPI or ... would surely benefit from this too? > diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c > +static int of_thermal_set_trips(struct thermal_zone_device *tz, long temp) s/tz/tzd/ or s/tz/tzdev/ ? Since "tz" says "thermal zone" to me, but it's actually a "thermal zone device". > + struct __thermal_zone *data = tz->devdata; s/data/tz/ ? "data" is a rather generic term, and "tz" seems like a good abbreviation for a __thermal_zone. > + for (i = 0; i < data->ntrips; ++i) { > + struct __thermal_trip *trip = data->trips + i; > + long trip_low = trip->temperature - trip->hysteresis; > + > + if (trip_low < temp && trip_low > low) > + low = trip_low; > + > + if (trip->temperature > temp && trip->temperature < high) > + high = trip->temperature; > + } That seems to always apply hysteresis to the low end of a trip object. Don't you need to apply the hysteresis to either the low or high end of the range, depending on whether the temperature is currently below/above the range, and hence which direction the edge will be crossed? Similar comments elsewhere. Perhaps the patch is consistent with some existing confusing naming style though? > +static int of_thermal_update_trips(struct thermal_zone_device *tz) > +{ > + long temp; > + int err; > + > + err = of_thermal_get_temp(tz, &temp); > + if (err) > + return err; > + > + err = of_thermal_set_trips(tz, temp); Doesn't this patch modify of_thermal_get_temp() to call of_thermal_set_trips() itself? > @@ -384,7 +467,8 @@ thermal_zone_of_add_sensor(struct device_node *zone, > struct thermal_zone_device * > thermal_zone_of_sensor_register(struct device *dev, int sensor_id, > void *data, int (*get_temp)(void *, long *), > - int (*get_trend)(void *, long *)) > + int (*get_trend)(void *, long *), > + int (*set_trips)(void *, long, long)) Passing in a struct containing fields for all the ops seem better than forever extending this function with more and more function pointers. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Inline. On 01/07/14 00:08, Stephen Warren wrote: > On 06/27/2014 02:11 AM, Mikko Perttunen wrote: >> 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 sensor driver >> callback `set_trips' is then called with the temperatures. >> If there is no trip point above or below the current temperature, >> the passed trip temperature will be LONG_MAX or LONG_MIN 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 the `set_trips' callback is not implemented (is NULL), the framework >> behaves as before. > > Is there no "core thermal" code? I would have expected this new feature > to be implemented in "core" code rather than of/dt "support" code. > Perhaps there would also be some additions to the of/dt code, but I'd > still expect the bulk of the feature to be complete independant of > of/dt. Systems still using board files or ACPI or ... would surely > benefit from this too? The thermal core only supports a fixed number of trip points for each driver and the core informs the driver of any changes to those, so drivers using the core framework can already have hardware trip points, but just a fixed number of them. The way of-thermal works, is it reads all the trip points from the device tree, registers a new thermal_zone_device with that number of trip points and then handles the trip points completely independently. Of course, if we're just polling, this is fine, since the thermal core also knows about those trip points and will trigger cooling when polling the each zone. However, the driver doesn't, so it cannot setup any interrupts to call thermal_zone_device_update. > >> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c > >> +static int of_thermal_set_trips(struct thermal_zone_device *tz, long temp) > > s/tz/tzd/ or s/tz/tzdev/ ? Since "tz" says "thermal zone" to me, but > it's actually a "thermal zone device". I followed the existing convention; "tz" is the name used most often by both the core and the of-thermal framework. > >> + struct __thermal_zone *data = tz->devdata; > > s/data/tz/ ? "data" is a rather generic term, and "tz" seems like a good > abbreviation for a __thermal_zone. Same, though here both "data" and "tz" seem to be used.. > >> + for (i = 0; i < data->ntrips; ++i) { >> + struct __thermal_trip *trip = data->trips + i; >> + long trip_low = trip->temperature - trip->hysteresis; >> + >> + if (trip_low < temp && trip_low > low) >> + low = trip_low; >> + >> + if (trip->temperature > temp && trip->temperature < high) >> + high = trip->temperature; >> + } > > That seems to always apply hysteresis to the low end of a trip object. > Don't you need to apply the hysteresis to either the low or high end of > the range, depending on whether the temperature is currently below/above > the range, and hence which direction the edge will be crossed? I believe applying only to the low end is correct. Say that we have a trip point at 40C and hysteresis of 2C. When we exceed 40C cooling will start immediately, but it will only be stopped when we cool down to 38C. At that point there is again a 2C gap between the current temperature and the trip point. It would seem that this is the interpretation used by our downstream kernel and also some people on the Internet (however trustworthy they may be..) If you don't feel this is right, please elaborate. > > Similar comments elsewhere. Perhaps the patch is consistent with some > existing confusing naming style though? > >> +static int of_thermal_update_trips(struct thermal_zone_device *tz) >> +{ >> + long temp; >> + int err; >> + >> + err = of_thermal_get_temp(tz, &temp); >> + if (err) >> + return err; >> + >> + err = of_thermal_set_trips(tz, temp); > > Doesn't this patch modify of_thermal_get_temp() to call > of_thermal_set_trips() itself? You're right. I suppose this function is unneeded. > >> @@ -384,7 +467,8 @@ thermal_zone_of_add_sensor(struct device_node *zone, >> struct thermal_zone_device * >> thermal_zone_of_sensor_register(struct device *dev, int sensor_id, >> void *data, int (*get_temp)(void *, long *), >> - int (*get_trend)(void *, long *)) >> + int (*get_trend)(void *, long *), >> + int (*set_trips)(void *, long, long)) > > Passing in a struct containing fields for all the ops seem better than > forever extending this function with more and more function pointers. > Very true. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/01/2014 01:27 AM, Mikko Perttunen wrote: > Inline. > > On 01/07/14 00:08, Stephen Warren wrote: >> On 06/27/2014 02:11 AM, Mikko Perttunen wrote: >>> 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 sensor driver >>> callback `set_trips' is then called with the temperatures. >>> If there is no trip point above or below the current temperature, >>> the passed trip temperature will be LONG_MAX or LONG_MIN 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 the `set_trips' callback is not implemented (is NULL), the framework >>> behaves as before. >> >> Is there no "core thermal" code? I would have expected this new feature >> to be implemented in "core" code rather than of/dt "support" code. >> Perhaps there would also be some additions to the of/dt code, but I'd >> still expect the bulk of the feature to be complete independant of >> of/dt. Systems still using board files or ACPI or ... would surely >> benefit from this too? > > The thermal core only supports a fixed number of trip points for each > driver and the core informs the driver of any changes to those, so > drivers using the core framework can already have hardware trip points, > but just a fixed number of them. > > The way of-thermal works, is it reads all the trip points from the > device tree, registers a new thermal_zone_device with that number of > trip points and then handles the trip points completely independently. > Of course, if we're just polling, this is fine, since the thermal core > also knows about those trip points and will trigger cooling when polling > the each zone. However, the driver doesn't, so it cannot setup any > interrupts to call thermal_zone_device_update. Is there any possibility of cleaning that up? It's obviously horribly inconsistent if core driver functionality works completely differently simply because the list of trip-points comes from DT rather than a static table in the driver. of_thermal should be limited to DT parsing and related device instantiation/lookup, not introducing a completely different functionality model. >>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c >>> + for (i = 0; i < data->ntrips; ++i) { >>> + struct __thermal_trip *trip = data->trips + i; >>> + long trip_low = trip->temperature - trip->hysteresis; >>> + >>> + if (trip_low < temp && trip_low > low) >>> + low = trip_low; >>> + >>> + if (trip->temperature > temp && trip->temperature < high) >>> + high = trip->temperature; >>> + } >> >> That seems to always apply hysteresis to the low end of a trip object. >> Don't you need to apply the hysteresis to either the low or high end of >> the range, depending on whether the temperature is currently below/above >> the range, and hence which direction the edge will be crossed? > > I believe applying only to the low end is correct. Say that we have a > trip point at 40C and hysteresis of 2C. When we exceed 40C cooling will > start immediately, but it will only be stopped when we cool down to 38C. > At that point there is again a 2C gap between the current temperature > and the trip point. It would seem that this is the interpretation used > by our downstream kernel and also some people on the Internet (however > trustworthy they may be..) > > If you don't feel this is right, please elaborate. Ah, the point I was missing is that each trip point is a single temperature, not a temperature range. As such, the code in your patch is correct. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/07/14 21:15, Stephen Warren wrote: >> The thermal core only supports a fixed number of trip points for each >> driver and the core informs the driver of any changes to those, so >> drivers using the core framework can already have hardware trip points, >> but just a fixed number of them. >> >> The way of-thermal works, is it reads all the trip points from the >> device tree, registers a new thermal_zone_device with that number of >> trip points and then handles the trip points completely independently. >> Of course, if we're just polling, this is fine, since the thermal core >> also knows about those trip points and will trigger cooling when polling >> the each zone. However, the driver doesn't, so it cannot setup any >> interrupts to call thermal_zone_device_update. > > Is there any possibility of cleaning that up? It's obviously horribly > inconsistent if core driver functionality works completely differently > simply because the list of trip-points comes from DT rather than a > static table in the driver. of_thermal should be limited to DT parsing > and related device instantiation/lookup, not introducing a completely > different functionality model. I guess the smallest possible change would be to add a #hardware-trip-cells property to the thermal driver node (this would need to designate both the thermal zone and the trip point) and a hardware-trip-point phandle node to trip points. Then trip points could point to a hardware trip point that would get programmed. Since this is just adding properties, it would be backwards-compatible as well. This is starting to sound like a good idea. Will have to give think about it some more. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/1/2014 11:15 AM, Stephen Warren wrote: > On 07/01/2014 01:27 AM, Mikko Perttunen wrote: >> >Inline. >> > >> >On 01/07/14 00:08, Stephen Warren wrote: >>> >>On 06/27/2014 02:11 AM, Mikko Perttunen wrote: >>>> >>>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 sensor driver >>>> >>>callback `set_trips' is then called with the temperatures. >>>> >>>If there is no trip point above or below the current temperature, >>>> >>>the passed trip temperature will be LONG_MAX or LONG_MIN 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 the `set_trips' callback is not implemented (is NULL), the framework >>>> >>>behaves as before. >>> >> >>> >>Is there no "core thermal" code? I would have expected this new feature >>> >>to be implemented in "core" code rather than of/dt "support" code. >>> >>Perhaps there would also be some additions to the of/dt code, but I'd >>> >>still expect the bulk of the feature to be complete independant of >>> >>of/dt. Systems still using board files or ACPI or ... would surely >>> >>benefit from this too? Stephen, the "core thermal code" defined two major object types: * a thermal zone * a cooling device Each thermal zone corresponds to a physical region in space within a device. A sane person might expect there to be one physical temperature sensor associated with each zone. But, that's not always the case. Because the thermal core did not expose the concept of a "sensor" separately from a "zone", many drivers which register thermal zones are messy -- they include a mix of device driver code (i.e. poking at register for a thermal sensor IP block) and device-agnostic code for implementing behaviors required of a thermal zone. of-thermal does more than just parse device tree. It registers thermal zones using its own implementation of that device-agnostic code. Having that centralized implementation of the thermal zone code allows thermal sensor device drivers which talk to of-thermal to be simpler than drivers which try to talk directly to the thermal core. The fact that you need to use DT to make use of the simpler sensor driver interface doesn't seem like a good reason to push back on Mikko's change. -Matt -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Terve Mikko, On Fri, Jun 27, 2014 at 11:11:34AM +0300, Mikko Perttunen wrote: > 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 sensor driver One thing I don't follow on your proposal is the groundings you need to 'set_trips' whenever temperature changes. Given your intention is to add support to interrupt driven devices, shouldn't we 'set_trips' just when we cross the previously set trips range? > callback `set_trips' is then called with the temperatures. > If there is no trip point above or below the current temperature, > the passed trip temperature will be LONG_MAX or LONG_MIN 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 the `set_trips' callback is not implemented (is NULL), the framework > behaves as before. As already mentioned by swarren, the proposal must be wider. We shall keep the same support in case the device is used in a system without device tree. In other words, if you want to see extra functionality for interrupt driven devices, you shall update the core part too, and draft a common messaging path. In general, interrupt driven devices are not mapped in the current thermal framework. That is, the current code is timer interrupt driven. Other interrupt updates from devices are propagated to the framework using thermal_zone_device_update(). In other words, you would reprogram your hardware trips from your interrupt handler/workqueue then just let the framework know what is going on with temperature, via a simple thermal_zone_device_update(). The way I see this going forward it would be a common interface to configure the thermal zones to be monitored: a. via polling only b. via interrupt only c. both a + b obviously, the above shall be informative only for userland. keep in mind also that changing interrupt configuration for high and low temperature thresholds can be racy. This feature was kept in the TODO list of the of-thermal.c because the we lack a proper support from the thermal framework (never came out of the TODO list, I know, I apologize for this). And this missing feature was spotted by the hwmon folks also, as they do have such support. So, the major missing improvements on interrupt driven devices shall come in three steps: (i) thermal framework, (ii) of-thermal (iii) thermal framework and hwmon interface. Cheers, > > Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> > --- > drivers/thermal/of-thermal.c | 97 ++++++++++++++++++++++++++++++++++++++++++-- > include/linux/thermal.h | 3 +- > 2 files changed, 95 insertions(+), 5 deletions(-) > > diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c > index 04b1be7..bfccea5 100644 > --- a/drivers/thermal/of-thermal.c > +++ b/drivers/thermal/of-thermal.c > @@ -89,6 +89,7 @@ struct __thermal_zone { > /* trip data */ > int ntrips; > struct __thermal_trip *trips; > + long prev_low_trip, prev_high_trip; > > /* cooling binding data */ > int num_tbps; > @@ -98,19 +99,66 @@ struct __thermal_zone { > void *sensor_data; > int (*get_temp)(void *, long *); > int (*get_trend)(void *, long *); > + int (*set_trips)(void *, long, long); > }; > > +/*** Automatic trip handling ***/ > + > +static int of_thermal_set_trips(struct thermal_zone_device *tz, long temp) > +{ > + struct __thermal_zone *data = tz->devdata; > + long low = LONG_MIN, high = LONG_MAX; > + int i; > + > + /* Hardware trip points not supported */ > + if (!data->set_trips) > + return 0; > + > + /* No need to change trip points */ > + if (temp > data->prev_low_trip && temp < data->prev_high_trip) > + return 0; > + > + for (i = 0; i < data->ntrips; ++i) { > + struct __thermal_trip *trip = data->trips + i; > + long trip_low = trip->temperature - trip->hysteresis; > + > + if (trip_low < temp && trip_low > low) > + low = trip_low; > + > + if (trip->temperature > temp && trip->temperature < high) > + high = trip->temperature; > + } > + > + dev_dbg(&tz->device, > + "temperature %ld, updating trip points to %ld, %ld\n", > + temp, low, high); > + > + data->prev_low_trip = low; > + data->prev_high_trip = high; > + > + return data->set_trips(data->sensor_data, low, high); > +} > + > /*** DT thermal zone device callbacks ***/ > > static int of_thermal_get_temp(struct thermal_zone_device *tz, > unsigned long *temp) > { > struct __thermal_zone *data = tz->devdata; > + int err; > > if (!data->get_temp) > return -EINVAL; > > - return data->get_temp(data->sensor_data, temp); > + err = data->get_temp(data->sensor_data, temp); > + if (err) > + return err; > + > + err = of_thermal_set_trips(tz, *temp); Here, if you update trips whenever you get_temp, you are possibly reprogramming your trips on every poll. Remember, this function will be called on every poll, in the current implementation. > + if (err) > + return err; > + > + return 0; > } > > static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip, > @@ -222,6 +270,22 @@ static int of_thermal_set_mode(struct thermal_zone_device *tz, > return 0; > } > > +static int of_thermal_update_trips(struct thermal_zone_device *tz) > +{ > + long temp; > + int err; > + > + err = of_thermal_get_temp(tz, &temp); > + if (err) > + return err; > + > + err = of_thermal_set_trips(tz, temp); > + if (err) > + return err; > + > + return 0; > +} > + > static int of_thermal_get_trip_type(struct thermal_zone_device *tz, int trip, > enum thermal_trip_type *type) > { > @@ -252,6 +316,7 @@ static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip, > unsigned long temp) > { > struct __thermal_zone *data = tz->devdata; > + int err; > > if (trip >= data->ntrips || trip < 0) > return -EDOM; > @@ -259,6 +324,10 @@ static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip, > /* thermal framework should take care of data->mask & (1 << trip) */ > data->trips[trip].temperature = temp; > > + err = of_thermal_update_trips(tz); > + if (err) > + return err; > + > return 0; > } > > @@ -279,6 +348,7 @@ static int of_thermal_set_trip_hyst(struct thermal_zone_device *tz, int trip, > unsigned long hyst) > { > struct __thermal_zone *data = tz->devdata; > + int err; > > if (trip >= data->ntrips || trip < 0) > return -EDOM; > @@ -286,6 +356,10 @@ static int of_thermal_set_trip_hyst(struct thermal_zone_device *tz, int trip, > /* thermal framework should take care of data->mask & (1 << trip) */ > data->trips[trip].hysteresis = hyst; > > + err = of_thermal_update_trips(tz); > + if (err) > + return err; > + > return 0; > } > > @@ -325,10 +399,12 @@ static struct thermal_zone_device * > thermal_zone_of_add_sensor(struct device_node *zone, > struct device_node *sensor, void *data, > int (*get_temp)(void *, long *), > - int (*get_trend)(void *, long *)) > + int (*get_trend)(void *, long *), > + int (*set_trips)(void *, long, long)) we need to clean the above arguments. they should become a .ops. > { > struct thermal_zone_device *tzd; > struct __thermal_zone *tz; > + int err; > > tzd = thermal_zone_get_zone_by_name(zone->name); > if (IS_ERR(tzd)) > @@ -339,8 +415,15 @@ thermal_zone_of_add_sensor(struct device_node *zone, > mutex_lock(&tzd->lock); > tz->get_temp = get_temp; > tz->get_trend = get_trend; > + tz->set_trips = set_trips; > tz->sensor_data = data; > > + err = of_thermal_update_trips(tzd); > + if (err) { > + mutex_unlock(&tzd->lock); > + return ERR_PTR(err); > + } > + > tzd->ops->get_temp = of_thermal_get_temp; > tzd->ops->get_trend = of_thermal_get_trend; > mutex_unlock(&tzd->lock); > @@ -384,7 +467,8 @@ thermal_zone_of_add_sensor(struct device_node *zone, > struct thermal_zone_device * > thermal_zone_of_sensor_register(struct device *dev, int sensor_id, > void *data, int (*get_temp)(void *, long *), > - int (*get_trend)(void *, long *)) > + int (*get_trend)(void *, long *), > + int (*set_trips)(void *, long, long)) ditto. > { > struct device_node *np, *child, *sensor_np; > > @@ -422,7 +506,8 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, > return thermal_zone_of_add_sensor(child, sensor_np, > data, > get_temp, > - get_trend); > + get_trend, > + set_trips); ditto. > } > } > of_node_put(np); > @@ -466,6 +551,7 @@ void thermal_zone_of_sensor_unregister(struct device *dev, > > tz->get_temp = NULL; > tz->get_trend = NULL; > + tz->set_trips = NULL; > tz->sensor_data = NULL; > mutex_unlock(&tzd->lock); > } > @@ -671,6 +757,9 @@ thermal_of_build_thermal_zone(struct device_node *np) > /* trips */ > child = of_get_child_by_name(np, "trips"); > > + tz->prev_high_trip = LONG_MIN; > + tz->prev_low_trip = LONG_MAX; > + > /* No trips provided */ > if (!child) > goto finish; > diff --git a/include/linux/thermal.h b/include/linux/thermal.h > index f7e11c7..2f8951c 100644 > --- a/include/linux/thermal.h > +++ b/include/linux/thermal.h > @@ -248,7 +248,8 @@ struct thermal_genl_event { > struct thermal_zone_device * > thermal_zone_of_sensor_register(struct device *dev, int id, > void *data, int (*get_temp)(void *, long *), > - int (*get_trend)(void *, long *)); > + int (*get_trend)(void *, long *), > + int (*set_trips)(void *, long, long)); > void thermal_zone_of_sensor_unregister(struct device *dev, > struct thermal_zone_device *tz); > #else > -- > 1.8.1.5 > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Moi Eduardo :) On 30/07/14 17:16, Eduardo Valentin wrote: > Terve Mikko, > > On Fri, Jun 27, 2014 at 11:11:34AM +0300, Mikko Perttunen wrote: >> 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 sensor driver > > One thing I don't follow on your proposal is the groundings you need to > 'set_trips' whenever temperature changes. Given your intention is to add > support to interrupt driven devices, shouldn't we 'set_trips' just when > we cross the previously set trips range? I think the reasoning for this was that I didn't want to make changes to thermal_core and thermal_zone_device_update only calls get_temp on the zone, so I had to add this code to get_temp. set_trips would anyway only be called if we had crossed a trip point. > >> callback `set_trips' is then called with the temperatures. >> If there is no trip point above or below the current temperature, >> the passed trip temperature will be LONG_MAX or LONG_MIN 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 the `set_trips' callback is not implemented (is NULL), the framework >> behaves as before. > > As already mentioned by swarren, the proposal must be wider. We shall > keep the same support in case the device is used in a system without > device tree. In other words, if you want to see extra functionality for > interrupt driven devices, you shall update the core part too, and draft > a common messaging path. Yeah, this is sensible. A simpler solution would be to just tell of-thermal drivers about the trip points and let the driver do whatever it wants. That would mirror the way normal thermal_core drivers are done. What is your opinion on that? > > In general, interrupt driven devices are not mapped in the current > thermal framework. That is, the current code is timer interrupt driven. > Other interrupt updates from devices are propagated to > the framework using thermal_zone_device_update(). In other words, you would > reprogram your hardware trips from your interrupt handler/workqueue then > just let the framework know what is going on with temperature, via a simple > thermal_zone_device_update(). > > The way I see this going forward it would be a common interface to > configure the thermal zones to be monitored: > a. via polling only > b. via interrupt only > c. both a + b > > obviously, the above shall be informative only for userland. > > keep in mind also that changing interrupt configuration for high and low > temperature thresholds can be racy. > > This feature was kept in the TODO list of the of-thermal.c because the > we lack a proper support from the thermal framework (never came out of > the TODO list, I know, I apologize for this). And this missing feature > was spotted by the hwmon folks also, as they do have such support. So, > the major missing improvements on interrupt driven devices shall come in > three steps: (i) thermal framework, (ii) of-thermal (iii) thermal > framework and hwmon interface. For now, I think I'll submit a driver with just polling support so that we can get some support in. > > > Cheers, > > Thanks, Mikko >> >> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> >> --- >> drivers/thermal/of-thermal.c | 97 ++++++++++++++++++++++++++++++++++++++++++-- >> include/linux/thermal.h | 3 +- >> 2 files changed, 95 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c >> index 04b1be7..bfccea5 100644 >> --- a/drivers/thermal/of-thermal.c >> +++ b/drivers/thermal/of-thermal.c >> @@ -89,6 +89,7 @@ struct __thermal_zone { >> /* trip data */ >> int ntrips; >> struct __thermal_trip *trips; >> + long prev_low_trip, prev_high_trip; >> >> /* cooling binding data */ >> int num_tbps; >> @@ -98,19 +99,66 @@ struct __thermal_zone { >> void *sensor_data; >> int (*get_temp)(void *, long *); >> int (*get_trend)(void *, long *); >> + int (*set_trips)(void *, long, long); >> }; >> >> +/*** Automatic trip handling ***/ >> + >> +static int of_thermal_set_trips(struct thermal_zone_device *tz, long temp) >> +{ >> + struct __thermal_zone *data = tz->devdata; >> + long low = LONG_MIN, high = LONG_MAX; >> + int i; >> + >> + /* Hardware trip points not supported */ >> + if (!data->set_trips) >> + return 0; >> + >> + /* No need to change trip points */ >> + if (temp > data->prev_low_trip && temp < data->prev_high_trip) >> + return 0; >> + >> + for (i = 0; i < data->ntrips; ++i) { >> + struct __thermal_trip *trip = data->trips + i; >> + long trip_low = trip->temperature - trip->hysteresis; >> + >> + if (trip_low < temp && trip_low > low) >> + low = trip_low; >> + >> + if (trip->temperature > temp && trip->temperature < high) >> + high = trip->temperature; >> + } >> + >> + dev_dbg(&tz->device, >> + "temperature %ld, updating trip points to %ld, %ld\n", >> + temp, low, high); >> + >> + data->prev_low_trip = low; >> + data->prev_high_trip = high; >> + >> + return data->set_trips(data->sensor_data, low, high); >> +} >> + >> /*** DT thermal zone device callbacks ***/ >> >> static int of_thermal_get_temp(struct thermal_zone_device *tz, >> unsigned long *temp) >> { >> struct __thermal_zone *data = tz->devdata; >> + int err; >> >> if (!data->get_temp) >> return -EINVAL; >> >> - return data->get_temp(data->sensor_data, temp); >> + err = data->get_temp(data->sensor_data, temp); >> + if (err) >> + return err; >> + >> + err = of_thermal_set_trips(tz, *temp); > > Here, if you update trips whenever you get_temp, you are possibly > reprogramming your trips on every poll. Remember, this function will be > called on every poll, in the current implementation. > >> + if (err) >> + return err; >> + >> + return 0; >> } >> >> static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip, >> @@ -222,6 +270,22 @@ static int of_thermal_set_mode(struct thermal_zone_device *tz, >> return 0; >> } >> >> +static int of_thermal_update_trips(struct thermal_zone_device *tz) >> +{ >> + long temp; >> + int err; >> + >> + err = of_thermal_get_temp(tz, &temp); >> + if (err) >> + return err; >> + >> + err = of_thermal_set_trips(tz, temp); >> + if (err) >> + return err; >> + >> + return 0; >> +} >> + >> static int of_thermal_get_trip_type(struct thermal_zone_device *tz, int trip, >> enum thermal_trip_type *type) >> { >> @@ -252,6 +316,7 @@ static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip, >> unsigned long temp) >> { >> struct __thermal_zone *data = tz->devdata; >> + int err; >> >> if (trip >= data->ntrips || trip < 0) >> return -EDOM; >> @@ -259,6 +324,10 @@ static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip, >> /* thermal framework should take care of data->mask & (1 << trip) */ >> data->trips[trip].temperature = temp; >> >> + err = of_thermal_update_trips(tz); >> + if (err) >> + return err; >> + >> return 0; >> } >> >> @@ -279,6 +348,7 @@ static int of_thermal_set_trip_hyst(struct thermal_zone_device *tz, int trip, >> unsigned long hyst) >> { >> struct __thermal_zone *data = tz->devdata; >> + int err; >> >> if (trip >= data->ntrips || trip < 0) >> return -EDOM; >> @@ -286,6 +356,10 @@ static int of_thermal_set_trip_hyst(struct thermal_zone_device *tz, int trip, >> /* thermal framework should take care of data->mask & (1 << trip) */ >> data->trips[trip].hysteresis = hyst; >> >> + err = of_thermal_update_trips(tz); >> + if (err) >> + return err; >> + >> return 0; >> } >> >> @@ -325,10 +399,12 @@ static struct thermal_zone_device * >> thermal_zone_of_add_sensor(struct device_node *zone, >> struct device_node *sensor, void *data, >> int (*get_temp)(void *, long *), >> - int (*get_trend)(void *, long *)) >> + int (*get_trend)(void *, long *), >> + int (*set_trips)(void *, long, long)) > > we need to clean the above arguments. they should become a .ops. > >> { >> struct thermal_zone_device *tzd; >> struct __thermal_zone *tz; >> + int err; >> >> tzd = thermal_zone_get_zone_by_name(zone->name); >> if (IS_ERR(tzd)) >> @@ -339,8 +415,15 @@ thermal_zone_of_add_sensor(struct device_node *zone, >> mutex_lock(&tzd->lock); >> tz->get_temp = get_temp; >> tz->get_trend = get_trend; >> + tz->set_trips = set_trips; >> tz->sensor_data = data; >> >> + err = of_thermal_update_trips(tzd); >> + if (err) { >> + mutex_unlock(&tzd->lock); >> + return ERR_PTR(err); >> + } >> + >> tzd->ops->get_temp = of_thermal_get_temp; >> tzd->ops->get_trend = of_thermal_get_trend; >> mutex_unlock(&tzd->lock); >> @@ -384,7 +467,8 @@ thermal_zone_of_add_sensor(struct device_node *zone, >> struct thermal_zone_device * >> thermal_zone_of_sensor_register(struct device *dev, int sensor_id, >> void *data, int (*get_temp)(void *, long *), >> - int (*get_trend)(void *, long *)) >> + int (*get_trend)(void *, long *), >> + int (*set_trips)(void *, long, long)) > > ditto. > >> { >> struct device_node *np, *child, *sensor_np; >> >> @@ -422,7 +506,8 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, >> return thermal_zone_of_add_sensor(child, sensor_np, >> data, >> get_temp, >> - get_trend); >> + get_trend, >> + set_trips); > > ditto. > >> } >> } >> of_node_put(np); >> @@ -466,6 +551,7 @@ void thermal_zone_of_sensor_unregister(struct device *dev, >> >> tz->get_temp = NULL; >> tz->get_trend = NULL; >> + tz->set_trips = NULL; >> tz->sensor_data = NULL; >> mutex_unlock(&tzd->lock); >> } >> @@ -671,6 +757,9 @@ thermal_of_build_thermal_zone(struct device_node *np) >> /* trips */ >> child = of_get_child_by_name(np, "trips"); >> >> + tz->prev_high_trip = LONG_MIN; >> + tz->prev_low_trip = LONG_MAX; >> + >> /* No trips provided */ >> if (!child) >> goto finish; >> diff --git a/include/linux/thermal.h b/include/linux/thermal.h >> index f7e11c7..2f8951c 100644 >> --- a/include/linux/thermal.h >> +++ b/include/linux/thermal.h >> @@ -248,7 +248,8 @@ struct thermal_genl_event { >> struct thermal_zone_device * >> thermal_zone_of_sensor_register(struct device *dev, int id, >> void *data, int (*get_temp)(void *, long *), >> - int (*get_trend)(void *, long *)); >> + int (*get_trend)(void *, long *), >> + int (*set_trips)(void *, long, long)); >> void thermal_zone_of_sensor_unregister(struct device *dev, >> struct thermal_zone_device *tz); >> #else >> -- >> 1.8.1.5 >> > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Moro, On Fri, Aug 1, 2014 at 7:42 AM, Mikko Perttunen <mperttunen@nvidia.com> wrote: > Moi Eduardo :) > > > On 30/07/14 17:16, Eduardo Valentin wrote: >> >> Terve Mikko, >> >> On Fri, Jun 27, 2014 at 11:11:34AM +0300, Mikko Perttunen wrote: >>> >>> 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 sensor driver >> >> >> One thing I don't follow on your proposal is the groundings you need to >> 'set_trips' whenever temperature changes. Given your intention is to add >> support to interrupt driven devices, shouldn't we 'set_trips' just when >> we cross the previously set trips range? > > > I think the reasoning for this was that I didn't want to make changes to > thermal_core and thermal_zone_device_update only calls get_temp on the zone, > so I had to add this code to get_temp. set_trips would anyway only be called > if we had crossed a trip point. > > I see. >> >>> callback `set_trips' is then called with the temperatures. >>> If there is no trip point above or below the current temperature, >>> the passed trip temperature will be LONG_MAX or LONG_MIN 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 the `set_trips' callback is not implemented (is NULL), the framework >>> behaves as before. >> >> >> As already mentioned by swarren, the proposal must be wider. We shall >> keep the same support in case the device is used in a system without >> device tree. In other words, if you want to see extra functionality for >> interrupt driven devices, you shall update the core part too, and draft >> a common messaging path. > > > Yeah, this is sensible. A simpler solution would be to just tell of-thermal > drivers about the trip points and let the driver do whatever it wants. That > would mirror the way normal thermal_core drivers are done. What is your > opinion on that? > > In fact, with the current implementation in the thermal framework, there is nothing else left than coding the feature in the driver itself. The framework would know only about the existance of the trips. This approach would need to be cleaned whenever we write the fix in the core though. For this reason, I would prefer to clean the core first, then write the driver as a proof that the changes in core are properly covering the existing drivers and interrupt driven drivers. >> >> In general, interrupt driven devices are not mapped in the current >> thermal framework. That is, the current code is timer interrupt driven. >> Other interrupt updates from devices are propagated to >> the framework using thermal_zone_device_update(). In other words, you >> would >> reprogram your hardware trips from your interrupt handler/workqueue then >> just let the framework know what is going on with temperature, via a >> simple >> thermal_zone_device_update(). >> >> The way I see this going forward it would be a common interface to >> configure the thermal zones to be monitored: >> a. via polling only >> b. via interrupt only >> c. both a + b >> >> obviously, the above shall be informative only for userland. >> >> keep in mind also that changing interrupt configuration for high and low >> temperature thresholds can be racy. >> >> This feature was kept in the TODO list of the of-thermal.c because the >> we lack a proper support from the thermal framework (never came out of >> the TODO list, I know, I apologize for this). And this missing feature >> was spotted by the hwmon folks also, as they do have such support. So, >> the major missing improvements on interrupt driven devices shall come in >> three steps: (i) thermal framework, (ii) of-thermal (iii) thermal >> framework and hwmon interface. > > > For now, I think I'll submit a driver with just polling support so that we > can get some support in. I see. I will be looking into the changes in core. Whenever I have a working RFC I will share with the list. Terveydeksi!, > >> >> >> Cheers, >> >> > > Thanks, Mikko > > >>> >>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> >>> --- >>> drivers/thermal/of-thermal.c | 97 >>> ++++++++++++++++++++++++++++++++++++++++++-- >>> include/linux/thermal.h | 3 +- >>> 2 files changed, 95 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c >>> index 04b1be7..bfccea5 100644 >>> --- a/drivers/thermal/of-thermal.c >>> +++ b/drivers/thermal/of-thermal.c >>> @@ -89,6 +89,7 @@ struct __thermal_zone { >>> /* trip data */ >>> int ntrips; >>> struct __thermal_trip *trips; >>> + long prev_low_trip, prev_high_trip; >>> >>> /* cooling binding data */ >>> int num_tbps; >>> @@ -98,19 +99,66 @@ struct __thermal_zone { >>> void *sensor_data; >>> int (*get_temp)(void *, long *); >>> int (*get_trend)(void *, long *); >>> + int (*set_trips)(void *, long, long); >>> }; >>> >>> +/*** Automatic trip handling ***/ >>> + >>> +static int of_thermal_set_trips(struct thermal_zone_device *tz, long >>> temp) >>> +{ >>> + struct __thermal_zone *data = tz->devdata; >>> + long low = LONG_MIN, high = LONG_MAX; >>> + int i; >>> + >>> + /* Hardware trip points not supported */ >>> + if (!data->set_trips) >>> + return 0; >>> + >>> + /* No need to change trip points */ >>> + if (temp > data->prev_low_trip && temp < data->prev_high_trip) >>> + return 0; >>> + >>> + for (i = 0; i < data->ntrips; ++i) { >>> + struct __thermal_trip *trip = data->trips + i; >>> + long trip_low = trip->temperature - trip->hysteresis; >>> + >>> + if (trip_low < temp && trip_low > low) >>> + low = trip_low; >>> + >>> + if (trip->temperature > temp && trip->temperature < high) >>> + high = trip->temperature; >>> + } >>> + >>> + dev_dbg(&tz->device, >>> + "temperature %ld, updating trip points to %ld, %ld\n", >>> + temp, low, high); >>> + >>> + data->prev_low_trip = low; >>> + data->prev_high_trip = high; >>> + >>> + return data->set_trips(data->sensor_data, low, high); >>> +} >>> + >>> /*** DT thermal zone device callbacks ***/ >>> >>> static int of_thermal_get_temp(struct thermal_zone_device *tz, >>> unsigned long *temp) >>> { >>> struct __thermal_zone *data = tz->devdata; >>> + int err; >>> >>> if (!data->get_temp) >>> return -EINVAL; >>> >>> - return data->get_temp(data->sensor_data, temp); >>> + err = data->get_temp(data->sensor_data, temp); >>> + if (err) >>> + return err; >>> + >>> + err = of_thermal_set_trips(tz, *temp); >> >> >> Here, if you update trips whenever you get_temp, you are possibly >> reprogramming your trips on every poll. Remember, this function will be >> called on every poll, in the current implementation. >> >>> + if (err) >>> + return err; >>> + >>> + return 0; >>> } >>> >>> static int of_thermal_get_trend(struct thermal_zone_device *tz, int >>> trip, >>> @@ -222,6 +270,22 @@ static int of_thermal_set_mode(struct >>> thermal_zone_device *tz, >>> return 0; >>> } >>> >>> +static int of_thermal_update_trips(struct thermal_zone_device *tz) >>> +{ >>> + long temp; >>> + int err; >>> + >>> + err = of_thermal_get_temp(tz, &temp); >>> + if (err) >>> + return err; >>> + >>> + err = of_thermal_set_trips(tz, temp); >>> + if (err) >>> + return err; >>> + >>> + return 0; >>> +} >>> + >>> static int of_thermal_get_trip_type(struct thermal_zone_device *tz, int >>> trip, >>> enum thermal_trip_type *type) >>> { >>> @@ -252,6 +316,7 @@ static int of_thermal_set_trip_temp(struct >>> thermal_zone_device *tz, int trip, >>> unsigned long temp) >>> { >>> struct __thermal_zone *data = tz->devdata; >>> + int err; >>> >>> if (trip >= data->ntrips || trip < 0) >>> return -EDOM; >>> @@ -259,6 +324,10 @@ static int of_thermal_set_trip_temp(struct >>> thermal_zone_device *tz, int trip, >>> /* thermal framework should take care of data->mask & (1 << trip) >>> */ >>> data->trips[trip].temperature = temp; >>> >>> + err = of_thermal_update_trips(tz); >>> + if (err) >>> + return err; >>> + >>> return 0; >>> } >>> >>> @@ -279,6 +348,7 @@ static int of_thermal_set_trip_hyst(struct >>> thermal_zone_device *tz, int trip, >>> unsigned long hyst) >>> { >>> struct __thermal_zone *data = tz->devdata; >>> + int err; >>> >>> if (trip >= data->ntrips || trip < 0) >>> return -EDOM; >>> @@ -286,6 +356,10 @@ static int of_thermal_set_trip_hyst(struct >>> thermal_zone_device *tz, int trip, >>> /* thermal framework should take care of data->mask & (1 << trip) >>> */ >>> data->trips[trip].hysteresis = hyst; >>> >>> + err = of_thermal_update_trips(tz); >>> + if (err) >>> + return err; >>> + >>> return 0; >>> } >>> >>> @@ -325,10 +399,12 @@ static struct thermal_zone_device * >>> thermal_zone_of_add_sensor(struct device_node *zone, >>> struct device_node *sensor, void *data, >>> int (*get_temp)(void *, long *), >>> - int (*get_trend)(void *, long *)) >>> + int (*get_trend)(void *, long *), >>> + int (*set_trips)(void *, long, long)) >> >> >> we need to clean the above arguments. they should become a .ops. >> >>> { >>> struct thermal_zone_device *tzd; >>> struct __thermal_zone *tz; >>> + int err; >>> >>> tzd = thermal_zone_get_zone_by_name(zone->name); >>> if (IS_ERR(tzd)) >>> @@ -339,8 +415,15 @@ thermal_zone_of_add_sensor(struct device_node *zone, >>> mutex_lock(&tzd->lock); >>> tz->get_temp = get_temp; >>> tz->get_trend = get_trend; >>> + tz->set_trips = set_trips; >>> tz->sensor_data = data; >>> >>> + err = of_thermal_update_trips(tzd); >>> + if (err) { >>> + mutex_unlock(&tzd->lock); >>> + return ERR_PTR(err); >>> + } >>> + >>> tzd->ops->get_temp = of_thermal_get_temp; >>> tzd->ops->get_trend = of_thermal_get_trend; >>> mutex_unlock(&tzd->lock); >>> @@ -384,7 +467,8 @@ thermal_zone_of_add_sensor(struct device_node *zone, >>> struct thermal_zone_device * >>> thermal_zone_of_sensor_register(struct device *dev, int sensor_id, >>> void *data, int (*get_temp)(void *, long >>> *), >>> - int (*get_trend)(void *, long *)) >>> + int (*get_trend)(void *, long *), >>> + int (*set_trips)(void *, long, long)) >> >> >> ditto. >> >>> { >>> struct device_node *np, *child, *sensor_np; >>> >>> @@ -422,7 +506,8 @@ thermal_zone_of_sensor_register(struct device *dev, >>> int sensor_id, >>> return thermal_zone_of_add_sensor(child, >>> sensor_np, >>> data, >>> get_temp, >>> - get_trend); >>> + get_trend, >>> + set_trips); >> >> >> ditto. >> >>> } >>> } >>> of_node_put(np); >>> @@ -466,6 +551,7 @@ void thermal_zone_of_sensor_unregister(struct device >>> *dev, >>> >>> tz->get_temp = NULL; >>> tz->get_trend = NULL; >>> + tz->set_trips = NULL; >>> tz->sensor_data = NULL; >>> mutex_unlock(&tzd->lock); >>> } >>> @@ -671,6 +757,9 @@ thermal_of_build_thermal_zone(struct device_node *np) >>> /* trips */ >>> child = of_get_child_by_name(np, "trips"); >>> >>> + tz->prev_high_trip = LONG_MIN; >>> + tz->prev_low_trip = LONG_MAX; >>> + >>> /* No trips provided */ >>> if (!child) >>> goto finish; >>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h >>> index f7e11c7..2f8951c 100644 >>> --- a/include/linux/thermal.h >>> +++ b/include/linux/thermal.h >>> @@ -248,7 +248,8 @@ struct thermal_genl_event { >>> struct thermal_zone_device * >>> thermal_zone_of_sensor_register(struct device *dev, int id, >>> void *data, int (*get_temp)(void *, long >>> *), >>> - int (*get_trend)(void *, long *)); >>> + int (*get_trend)(void *, long *), >>> + int (*set_trips)(void *, long, long)); >>> void thermal_zone_of_sensor_unregister(struct device *dev, >>> struct thermal_zone_device *tz); >>> #else >>> -- >>> 1.8.1.5 >>> >> >
diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c index 04b1be7..bfccea5 100644 --- a/drivers/thermal/of-thermal.c +++ b/drivers/thermal/of-thermal.c @@ -89,6 +89,7 @@ struct __thermal_zone { /* trip data */ int ntrips; struct __thermal_trip *trips; + long prev_low_trip, prev_high_trip; /* cooling binding data */ int num_tbps; @@ -98,19 +99,66 @@ struct __thermal_zone { void *sensor_data; int (*get_temp)(void *, long *); int (*get_trend)(void *, long *); + int (*set_trips)(void *, long, long); }; +/*** Automatic trip handling ***/ + +static int of_thermal_set_trips(struct thermal_zone_device *tz, long temp) +{ + struct __thermal_zone *data = tz->devdata; + long low = LONG_MIN, high = LONG_MAX; + int i; + + /* Hardware trip points not supported */ + if (!data->set_trips) + return 0; + + /* No need to change trip points */ + if (temp > data->prev_low_trip && temp < data->prev_high_trip) + return 0; + + for (i = 0; i < data->ntrips; ++i) { + struct __thermal_trip *trip = data->trips + i; + long trip_low = trip->temperature - trip->hysteresis; + + if (trip_low < temp && trip_low > low) + low = trip_low; + + if (trip->temperature > temp && trip->temperature < high) + high = trip->temperature; + } + + dev_dbg(&tz->device, + "temperature %ld, updating trip points to %ld, %ld\n", + temp, low, high); + + data->prev_low_trip = low; + data->prev_high_trip = high; + + return data->set_trips(data->sensor_data, low, high); +} + /*** DT thermal zone device callbacks ***/ static int of_thermal_get_temp(struct thermal_zone_device *tz, unsigned long *temp) { struct __thermal_zone *data = tz->devdata; + int err; if (!data->get_temp) return -EINVAL; - return data->get_temp(data->sensor_data, temp); + err = data->get_temp(data->sensor_data, temp); + if (err) + return err; + + err = of_thermal_set_trips(tz, *temp); + if (err) + return err; + + return 0; } static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip, @@ -222,6 +270,22 @@ static int of_thermal_set_mode(struct thermal_zone_device *tz, return 0; } +static int of_thermal_update_trips(struct thermal_zone_device *tz) +{ + long temp; + int err; + + err = of_thermal_get_temp(tz, &temp); + if (err) + return err; + + err = of_thermal_set_trips(tz, temp); + if (err) + return err; + + return 0; +} + static int of_thermal_get_trip_type(struct thermal_zone_device *tz, int trip, enum thermal_trip_type *type) { @@ -252,6 +316,7 @@ static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip, unsigned long temp) { struct __thermal_zone *data = tz->devdata; + int err; if (trip >= data->ntrips || trip < 0) return -EDOM; @@ -259,6 +324,10 @@ static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip, /* thermal framework should take care of data->mask & (1 << trip) */ data->trips[trip].temperature = temp; + err = of_thermal_update_trips(tz); + if (err) + return err; + return 0; } @@ -279,6 +348,7 @@ static int of_thermal_set_trip_hyst(struct thermal_zone_device *tz, int trip, unsigned long hyst) { struct __thermal_zone *data = tz->devdata; + int err; if (trip >= data->ntrips || trip < 0) return -EDOM; @@ -286,6 +356,10 @@ static int of_thermal_set_trip_hyst(struct thermal_zone_device *tz, int trip, /* thermal framework should take care of data->mask & (1 << trip) */ data->trips[trip].hysteresis = hyst; + err = of_thermal_update_trips(tz); + if (err) + return err; + return 0; } @@ -325,10 +399,12 @@ static struct thermal_zone_device * thermal_zone_of_add_sensor(struct device_node *zone, struct device_node *sensor, void *data, int (*get_temp)(void *, long *), - int (*get_trend)(void *, long *)) + int (*get_trend)(void *, long *), + int (*set_trips)(void *, long, long)) { struct thermal_zone_device *tzd; struct __thermal_zone *tz; + int err; tzd = thermal_zone_get_zone_by_name(zone->name); if (IS_ERR(tzd)) @@ -339,8 +415,15 @@ thermal_zone_of_add_sensor(struct device_node *zone, mutex_lock(&tzd->lock); tz->get_temp = get_temp; tz->get_trend = get_trend; + tz->set_trips = set_trips; tz->sensor_data = data; + err = of_thermal_update_trips(tzd); + if (err) { + mutex_unlock(&tzd->lock); + return ERR_PTR(err); + } + tzd->ops->get_temp = of_thermal_get_temp; tzd->ops->get_trend = of_thermal_get_trend; mutex_unlock(&tzd->lock); @@ -384,7 +467,8 @@ thermal_zone_of_add_sensor(struct device_node *zone, struct thermal_zone_device * thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data, int (*get_temp)(void *, long *), - int (*get_trend)(void *, long *)) + int (*get_trend)(void *, long *), + int (*set_trips)(void *, long, long)) { struct device_node *np, *child, *sensor_np; @@ -422,7 +506,8 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, return thermal_zone_of_add_sensor(child, sensor_np, data, get_temp, - get_trend); + get_trend, + set_trips); } } of_node_put(np); @@ -466,6 +551,7 @@ void thermal_zone_of_sensor_unregister(struct device *dev, tz->get_temp = NULL; tz->get_trend = NULL; + tz->set_trips = NULL; tz->sensor_data = NULL; mutex_unlock(&tzd->lock); } @@ -671,6 +757,9 @@ thermal_of_build_thermal_zone(struct device_node *np) /* trips */ child = of_get_child_by_name(np, "trips"); + tz->prev_high_trip = LONG_MIN; + tz->prev_low_trip = LONG_MAX; + /* No trips provided */ if (!child) goto finish; diff --git a/include/linux/thermal.h b/include/linux/thermal.h index f7e11c7..2f8951c 100644 --- a/include/linux/thermal.h +++ b/include/linux/thermal.h @@ -248,7 +248,8 @@ struct thermal_genl_event { struct thermal_zone_device * thermal_zone_of_sensor_register(struct device *dev, int id, void *data, int (*get_temp)(void *, long *), - int (*get_trend)(void *, long *)); + int (*get_trend)(void *, long *), + int (*set_trips)(void *, long, long)); void thermal_zone_of_sensor_unregister(struct device *dev, struct thermal_zone_device *tz); #else
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 sensor driver callback `set_trips' is then called with the temperatures. If there is no trip point above or below the current temperature, the passed trip temperature will be LONG_MAX or LONG_MIN 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 the `set_trips' callback is not implemented (is NULL), the framework behaves as before. Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> --- drivers/thermal/of-thermal.c | 97 ++++++++++++++++++++++++++++++++++++++++++-- include/linux/thermal.h | 3 +- 2 files changed, 95 insertions(+), 5 deletions(-)