From: Mikko Perttunen <mperttunen@nvidia.com> Date: 2014-06-27 08:12:30
Hi everyone,
this series adds support for hardware-tracked thermal trip points
for the device tree thermal framework and introduces a new Tegra124 thermal
driver that uses them.
Hardware-tracked trip points are trip points that do not need to be polled;
the hardware gives an interrupt when the trip point is reached. The device
tree thermal framework has not previously given the sensor driver any
information about set trip points, so using these has been impossible.
This series adds a new callback from of-thermal to the driver to allow telling
the driver about trip points. The driver only needs to track two trip points,
the framework ensures that the current temperature lies between those two.
Behavior for drivers that do not include this callback is unchanged.
The Tegra124 SOCTHERM thermal driver that is included exposes four thermal zones
(the thermctl thermal zones) with hardware-tracked trip point support. While the
hardware supports four tracked trip points, only one is used.
Mikko Perttunen (6):
thermal: of: Add support for hardware-tracked trip points
of: Add bindings for nvidia,tegra124-soctherm
ARM: tegra: Add thermal trip points for Jetson TK1
ARM: tegra: Add soctherm and thermal zones to Tegra124 device tree
clk: tegra: Add soctherm and tsensor clocks to Tegra124 init table
thermal: Add Tegra SOCTHERM thermal management driver
.../devicetree/bindings/thermal/tegra-soctherm.txt | 32 ++
arch/arm/boot/dts/tegra124-jetson-tk1.dts | 32 ++
arch/arm/boot/dts/tegra124.dtsi | 48 ++
drivers/clk/tegra/clk-tegra124.c | 2 +
drivers/thermal/Kconfig | 7 +
drivers/thermal/Makefile | 1 +
drivers/thermal/of-thermal.c | 97 +++-
drivers/thermal/tegra_soctherm.c | 553 +++++++++++++++++++++
include/dt-bindings/thermal/tegra124-soctherm.h | 15 +
include/linux/thermal.h | 3 +-
10 files changed, 785 insertions(+), 5 deletions(-)
create mode 100644 Documentation/devicetree/bindings/thermal/tegra-soctherm.txt
create mode 100644 drivers/thermal/tegra_soctherm.c
create mode 100644 include/dt-bindings/thermal/tegra124-soctherm.h
--
1.8.1.5
From: Mikko Perttunen <mperttunen@nvidia.com> Date: 2014-06-27 08:12:34
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(-)
@@ -89,6 +89,7 @@ struct __thermal_zone {/* trip data */intntrips;struct__thermal_trip*trips;+longprev_low_trip,prev_high_trip;/* cooling binding data */intnum_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 ***/++staticintof_thermal_set_trips(structthermal_zone_device*tz,longtemp)+{+struct__thermal_zone*data=tz->devdata;+longlow=LONG_MIN,high=LONG_MAX;+inti;++/* Hardware trip points not supported */+if(!data->set_trips)+return0;++/* No need to change trip points */+if(temp>data->prev_low_trip&&temp<data->prev_high_trip)+return0;++for(i=0;i<data->ntrips;++i){+struct__thermal_trip*trip=data->trips+i;+longtrip_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;++returndata->set_trips(data->sensor_data,low,high);+}+/*** DT thermal zone device callbacks ***/staticintof_thermal_get_temp(structthermal_zone_device*tz,unsignedlong*temp){struct__thermal_zone*data=tz->devdata;+interr;if(!data->get_temp)return-EINVAL;-returndata->get_temp(data->sensor_data,temp);+err=data->get_temp(data->sensor_data,temp);+if(err)+returnerr;++err=of_thermal_set_trips(tz,*temp);+if(err)+returnerr;++return0;}staticintof_thermal_get_trend(structthermal_zone_device*tz,inttrip,
@@ -222,6 +270,22 @@ static int of_thermal_set_mode(struct thermal_zone_device *tz,return0;}+staticintof_thermal_update_trips(structthermal_zone_device*tz)+{+longtemp;+interr;++err=of_thermal_get_temp(tz,&temp);+if(err)+returnerr;++err=of_thermal_set_trips(tz,temp);+if(err)+returnerr;++return0;+}+staticintof_thermal_get_trip_type(structthermal_zone_device*tz,inttrip,enumthermal_trip_type*type){
@@ -252,6 +316,7 @@ static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip,unsignedlongtemp){struct__thermal_zone*data=tz->devdata;+interr;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)+returnerr;+return0;}
@@ -279,6 +348,7 @@ static int of_thermal_set_trip_hyst(struct thermal_zone_device *tz, int trip,unsignedlonghyst){struct__thermal_zone*data=tz->devdata;+interr;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)+returnerr;+return0;}
From: Mikko Perttunen <mperttunen@nvidia.com> Date: 2014-06-27 08:12:39
This adds critical trip points to the Jetson TK1 device tree.
The device will do a controlled shutdown when either the CPU, GPU
or MEM thermal zone reaches 101 degrees Celsius.
Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
arch/arm/boot/dts/tegra124-jetson-tk1.dts | 32 +++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
From: Mikko Perttunen <mperttunen@nvidia.com> Date: 2014-06-27 08:12:43
This adds the soctherm thermal sensing and management unit to the
Tegra124 device tree along with the four thermal zones it exports.
Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
arch/arm/boot/dts/tegra124.dtsi | 48 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
From: Mikko Perttunen <mperttunen@nvidia.com> Date: 2014-06-27 08:12:50
This adds support for the Tegra SOCTHERM thermal sensing and management
system found in the Tegra124 system-on-chip. This initial driver supports
the four thermal zones with hardware-tracked trip points.
Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
drivers/thermal/Kconfig | 7 +
drivers/thermal/Makefile | 1 +
drivers/thermal/tegra_soctherm.c | 553 +++++++++++++++++++++++++++++++++++++++
3 files changed, 561 insertions(+)
create mode 100644 drivers/thermal/tegra_soctherm.c
From: Mikko Perttunen <mperttunen@nvidia.com> Date: 2014-06-27 08:13:20
This adds the two clocks, soctherm and tsensor, to the T124 initialization table.
They are required for soctherm-based thermal sensing.
Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
Peter, one more zero for TSENSOR, please :)
drivers/clk/tegra/clk-tegra124.c | 2 ++
1 file changed, 2 insertions(+)
@@ -1369,6 +1369,8 @@ static struct tegra_clk_init_table init_table[] __initdata = {{TEGRA124_CLK_XUSB_HS_SRC,TEGRA124_CLK_PLL_U_60M,60000000,0},{TEGRA124_CLK_XUSB_FALCON_SRC,TEGRA124_CLK_PLL_RE_OUT,224000000,0},{TEGRA124_CLK_XUSB_HOST_SRC,TEGRA124_CLK_PLL_RE_OUT,112000000,0},+{TEGRA124_CLK_SOC_THERM,TEGRA124_CLK_PLL_P,51000000,0},+{TEGRA124_CLK_TSENSOR,TEGRA124_CLK_CLK_M,400000,0},/* This MUST be the last entry. */{TEGRA124_CLK_CLK_MAX,TEGRA124_CLK_CLK_MAX,0,0},};
@@ -0,0 +1,32 @@+Tegra124 SOCTHERM thermal management system++Required properties :+- compatible : "nvidia,tegra124-soctherm".+- reg : Should contain 1 entry:+ - SOCTHERM register set+- interrupts : Defines the interrupt used by SOCTHERM+- clocks : Must contain an entry for each entry in clock-names.+ See ../clocks/clock-bindings.txt for details.+- clock-names : Must include the following entries:+ - tsensor+ - soctherm+- resets : Must contain an entry for each entry in reset-names.+ See ../reset/reset.txt for details.+- reset-names : Must include the following entries:+ - soctherm+- #thermal-sensor-cells : For thermctl sensors. Should be 1.++Example :++ soctherm at 0,700e2000 {+ compatible = "nvidia,tegra124-soctherm";+ reg = <0x0 0x700e2000 0x0 0x1000>;+ interrupts = <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>;+ clocks = <&tegra_car TEGRA124_CLK_TSENSOR>,+ <&tegra_car TEGRA124_CLK_SOC_THERM>;+ clock-names = "tsensor", "soctherm";+ resets = <&tegra_car 78>;+ reset-names = "soctherm";++ #thermal-sensor-cells = <1>;+ };
From: Peter De Schrijver <hidden> Date: 2014-06-27 12:18:38
On Fri, Jun 27, 2014 at 10:11:38AM +0200, Mikko Perttunen wrote:
This adds the two clocks, soctherm and tsensor, to the T124 initialization table.
They are required for soctherm-based thermal sensing.
Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
Peter, one more zero for TSENSOR, please :)
Ah :) I will take this into clk-tegra-3.17
Cheers,
Peter.
+- #thermal-sensor-cells : For thermctl sensors. Should be 1.
I'd prefer a tiny bit more information here: A link to whatever other
document defines the meaning of #thermal-sensor-cells, and a link to the
header that defines the meaning of the data in the cell. Perhaps:
#thermal-sensor-cells : Should be 1. See ./thermal.txt for a description
of this property. See <dt-bindings/thermal/tegra124-soctherm.h> for a
list of valid values.
From: Stephen Warren <hidden> Date: 2014-06-30 20:45:34
On 06/27/2014 02:11 AM, Mikko Perttunen wrote:
This adds critical trip points to the Jetson TK1 device tree.
The device will do a controlled shutdown when either the CPU, GPU
or MEM thermal zone reaches 101 degrees Celsius.
It would be more typical to order the patches with changes to
tegra124.dtsi first, then changes to board files (that build on the core
SoC support) second.
Can we name that simply "critical"? DT node names are supposed to
represent type of object more than identity of object. Even "critical"
is a bit of an identity, and "trip at 0" would be better, but I imagine the
core thermal DT bindings precluded that?
I think we should still define a polling delay so that if there's SW
that doesn't support HW trip points/interrupts, it still knows how often
it should reasonably check the sensor.
Perhaps a delay of 0 is used to determine whether to use HW trip points
vs polling (I haven't read patch 1 yet)? If so, I'd prefer not to do
that. Rather, the driver should advertize its ability to provide HW trip
points, and it would be up to the core to then make use of them. The DT
should just describe the HW, not assume it can influence SW's choice of
whether to use HW trip points.
From: Stephen Warren <hidden> Date: 2014-06-30 21:08:29
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?
+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?
quoted hunk
@@ -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.
From: Stephen Warren <hidden> Date: 2014-06-30 21:23:09
On 06/27/2014 02:11 AM, Mikko Perttunen wrote:
This adds support for the Tegra SOCTHERM thermal sensing and management
system found in the Tegra124 system-on-chip. This initial driver supports
the four thermal zones with hardware-tracked trip points.
I wonder why some of those fields are named "fuse_xxx" when the values
are hard-coded in these tables rather than read from fuses? These values
don't seem to be used to adjust values read from fuses.
+static int tegra_thermctl_get_temp(void *data, long *out_temp)
+ switch (zone->sensor) {
+ case 0:
+ val = readl(zone->tegra->regs + SENSOR_TEMP1)
+ >> SENSOR_TEMP1_CPU_TEMP_SHIFT;
Can't the register offset and shift be stored in *zone, so that this
whole switch can be replaced with something generic:
val = readl(zone->tegra->regs + zone->reg_offset) >> zone->value_shift;
+static int tegra_soctherm_probe(struct platform_device *pdev)
Why request the same IRQ 4 times here. Rather, shouldn't the IRQ be
requested once, and the ISR simply loop over the status register (or
whatever there are 4 of)?
From: Mikko Perttunen <mperttunen@nvidia.com> Date: 2014-07-01 07:27:43
Inline.
On 01/07/14 00:08, Stephen Warren wrote:
On 06/27/2014 02:11 AM, Mikko Perttunen wrote:
quoted
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.
+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.
quoted
+ 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..
quoted
+ 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?
quoted
+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.
quoted
@@ -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.
I think we should still define a polling delay so that if there's SW
that doesn't support HW trip points/interrupts, it still knows how often
it should reasonably check the sensor.
Perhaps a delay of 0 is used to determine whether to use HW trip points
vs polling (I haven't read patch 1 yet)? If so, I'd prefer not to do
that. Rather, the driver should advertize its ability to provide HW trip
points, and it would be up to the core to then make use of them. The DT
should just describe the HW, not assume it can influence SW's choice of
whether to use HW trip points.
Yes, a delay of 0 disables polling in the thermal core. (The hw trip
code doesn't do anything with it) One way to fix this would be to export
a rate changing function in the thermal core and have of-thermal set the
polling rate to 0 or the value from device tree depending on if hw trip
point programming succeeded or not. This would also be good for error
handling, since if hw trip poing programming failed for whatever reason,
we could still fall back to polling.
I don't see any real need for that blank line. If there was, there would
probably be more blank lines in the big list of properties above.
The reasoning was that #thermal-sensor-cells as a cells-property is a
bit different from the rest, so separate them. But I can remove the
blank line just as well.
From: Mikko Perttunen <mperttunen@nvidia.com> Date: 2014-07-01 08:06:36
Inline.
On 01/07/14 00:23, Stephen Warren wrote:
On 06/27/2014 02:11 AM, Mikko Perttunen wrote:
quoted
This adds support for the Tegra SOCTHERM thermal sensing and management
system found in the Tegra124 system-on-chip. This initial driver supports
the four thermal zones with hardware-tracked trip points.
I wonder why some of those fields are named "fuse_xxx" when the values
are hard-coded in these tables rather than read from fuses? These values
don't seem to be used to adjust values read from fuses.
They are used to when calculating the thermal calibration in
calculate_tsensor_calibration, which is based on the value read from the
fuse. Downstream calls them fuse correction values, so I kept that. (I
guess the meaning of corr might not be obvious..) On downstream there is
another set of these correction values used depending on the fuse
revision, but I believe the older revision is only found internally.
quoted
+static int tegra_thermctl_get_temp(void *data, long *out_temp)
quoted
+ switch (zone->sensor) {
+ case 0:
+ val = readl(zone->tegra->regs + SENSOR_TEMP1)
+ >> SENSOR_TEMP1_CPU_TEMP_SHIFT;
Can't the register offset and shift be stored in *zone, so that this
whole switch can be replaced with something generic:
val = readl(zone->tegra->regs + zone->reg_offset) >> zone->value_shift;
Yes, certainly doable.
quoted
+static int tegra_soctherm_probe(struct platform_device *pdev)
Why "4"? Should the loop count be the ARRAY_SIZE(some array)? At the
very least, a named constant that describes the value would be useful...
The thermctl sensors have been unchanged for a few chip generations, so
I was thinking that just hardcoding this wouldn't be so bad. But I guess
an array would look nicer here. Will fix.
Why request the same IRQ 4 times here. Rather, shouldn't the IRQ be
requested once, and the ISR simply loop over the status register (or
whatever there are 4 of)?
I had that variant as well, but since we need to pass the list of
tripped sensors to soctherm_isr_thread somehow, I guess some kind of
locking or atomic is needed. This version doesn't need that, so I went
with it.
From: Stephen Warren <hidden> Date: 2014-07-01 18:16:03
On 07/01/2014 01:27 AM, Mikko Perttunen wrote:
Inline.
On 01/07/14 00:08, Stephen Warren wrote:
quoted
On 06/27/2014 02:11 AM, Mikko Perttunen wrote:
quoted
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.
+ 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.
From: Stephen Warren <hidden> Date: 2014-07-01 18:26:48
On 07/01/2014 02:06 AM, Mikko Perttunen wrote:
Inline.
On 01/07/14 00:23, Stephen Warren wrote:
quoted
On 06/27/2014 02:11 AM, Mikko Perttunen wrote:
quoted
This adds support for the Tegra SOCTHERM thermal sensing and management
system found in the Tegra124 system-on-chip. This initial driver
supports
the four thermal zones with hardware-tracked trip points.
I wonder why some of those fields are named "fuse_xxx" when the values
are hard-coded in these tables rather than read from fuses? These values
don't seem to be used to adjust values read from fuses.
They are used to when calculating the thermal calibration in
calculate_tsensor_calibration, which is based on the value read from the
fuse. Downstream calls them fuse correction values, so I kept that. (I
guess the meaning of corr might not be obvious..) On downstream there is
another set of these correction values used depending on the fuse
revision, but I believe the older revision is only found internally.
Ah, so there's some manufacturing calibration process that sets some
fuse value, and the HW uses a combination of that fuse value, and some
parameters of the manufacturing process as represented by the
SENSOR_CONFIG2 register, to apply the calibration? I wonder why
SENSOR_CONFIG2 is a register not a fuse in that case, but anyway...
Perhaps some comments or kerneldoc in the definition of struct
tegra_tsensor would be useful?
I did notice some inconsistency in bracketing at:
Why request the same IRQ 4 times here. Rather, shouldn't the IRQ be
requested once, and the ISR simply loop over the status register (or
whatever there are 4 of)?
I had that variant as well, but since we need to pass the list of
tripped sensors to soctherm_isr_thread somehow, I guess some kind of
locking or atomic is needed. This version doesn't need that, so I went
with it.
Why not read THERMCTL_INTR_STATUS inside the IRQ thread. IIRC, if the
ISR wakes an IRQ thread, the interrupt remains disable until the thread
has run its course, so there's no issue deferring the register read
until the thread runs, at which point, the thread can simply loop over
all the sensors.
From: Mikko Perttunen <mperttunen@nvidia.com> Date: 2014-07-03 13:51:37
On 01/07/14 21:26, Stephen Warren wrote:
Ah, so there's some manufacturing calibration process that sets some
fuse value, and the HW uses a combination of that fuse value, and some
parameters of the manufacturing process as represented by the
SENSOR_CONFIG2 register, to apply the calibration? I wonder why
SENSOR_CONFIG2 is a register not a fuse in that case, but anyway...
Perhaps some comments or kerneldoc in the definition of struct
tegra_tsensor would be useful?
Yes, I'll add some comments.
Why not read THERMCTL_INTR_STATUS inside the IRQ thread. IIRC, if the
ISR wakes an IRQ thread, the interrupt remains disable until the thread
has run its course, so there's no issue deferring the register read
until the thread runs, at which point, the thread can simply loop over
all the sensors.
If that's the case, then that's definitely a better way to do it.
From: Mikko Perttunen <mperttunen@nvidia.com> Date: 2014-07-03 14:15:47
On 01/07/14 21:15, Stephen Warren wrote:
quoted
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.
This adds support for the Tegra SOCTHERM thermal sensing and management
system found in the Tegra124 system-on-chip. This initial driver supports
the four thermal zones with hardware-tracked trip points.
I think we need to disable the interrupt here, and enable it again after
updating the high/low limited values. If not, there will trigger mass of
interrupts.
From: Mikko Perttunen <mperttunen@nvidia.com> Date: 2014-07-04 11:52:55
On 04/07/14 11:43, Wei Ni wrote:
On 06/27/2014 04:11 PM, Mikko Perttunen wrote:
quoted
This adds support for the Tegra SOCTHERM thermal sensing and management
system found in the Tegra124 system-on-chip. This initial driver supports
the four thermal zones with hardware-tracked trip points.
I think we need to disable the interrupt here, and enable it again after
updating the high/low limited values. If not, there will trigger mass of
interrupts.
Good point. It works now because of-thermal will set up the new trip
points during soctherm_thread_isr, and apparently the interrupt is kept
disabled until after that.
I guess for this one it should be simple; same driver probe and teardown
as normally. Although currently suspend doesn't support LP0 so no-op is
enough.
Hi, Eduardo,
what do you think of this patch set?
thanks,
rui
On Fri, 2014-06-27 at 11:11 +0300, Mikko Perttunen wrote:
Hi everyone,
this series adds support for hardware-tracked thermal trip points
for the device tree thermal framework and introduces a new Tegra124 thermal
driver that uses them.
Hardware-tracked trip points are trip points that do not need to be polled;
the hardware gives an interrupt when the trip point is reached. The device
tree thermal framework has not previously given the sensor driver any
information about set trip points, so using these has been impossible.
This series adds a new callback from of-thermal to the driver to allow telling
the driver about trip points. The driver only needs to track two trip points,
the framework ensures that the current temperature lies between those two.
Behavior for drivers that do not include this callback is unchanged.
The Tegra124 SOCTHERM thermal driver that is included exposes four thermal zones
(the thermctl thermal zones) with hardware-tracked trip point support. While the
hardware supports four tracked trip points, only one is used.
Mikko Perttunen (6):
thermal: of: Add support for hardware-tracked trip points
of: Add bindings for nvidia,tegra124-soctherm
ARM: tegra: Add thermal trip points for Jetson TK1
ARM: tegra: Add soctherm and thermal zones to Tegra124 device tree
clk: tegra: Add soctherm and tsensor clocks to Tegra124 init table
thermal: Add Tegra SOCTHERM thermal management driver
.../devicetree/bindings/thermal/tegra-soctherm.txt | 32 ++
arch/arm/boot/dts/tegra124-jetson-tk1.dts | 32 ++
arch/arm/boot/dts/tegra124.dtsi | 48 ++
drivers/clk/tegra/clk-tegra124.c | 2 +
drivers/thermal/Kconfig | 7 +
drivers/thermal/Makefile | 1 +
drivers/thermal/of-thermal.c | 97 +++-
drivers/thermal/tegra_soctherm.c | 553 +++++++++++++++++++++
include/dt-bindings/thermal/tegra124-soctherm.h | 15 +
include/linux/thermal.h | 3 +-
10 files changed, 785 insertions(+), 5 deletions(-)
create mode 100644 Documentation/devicetree/bindings/thermal/tegra-soctherm.txt
create mode 100644 drivers/thermal/tegra_soctherm.c
create mode 100644 include/dt-bindings/thermal/tegra124-soctherm.h
From: Matthew Longnecker <hidden> Date: 2014-07-21 23:20:07
On 6/27/2014 1:11 AM, Mikko Perttunen wrote:
This adds the soctherm thermal sensing and management unit to the
Tegra124 device tree along with the four thermal zones it exports.
Mikko, soctherm doesn't "export thermal zones". I would rewrite your
desription like this:
Extend the Tegra124 device tree by adding the soctherm thermal
sensing and management unit and by defining four thermal zones --
one for each temperature sensor in soctherm.
System integrators have some flexibility in deciding how many thermal
zones to define for their platform. For example, an integrator could
define a single zone for the entire Tegra chip (giving a simple system
at runtime) or with multiple zones (giving potentially higher
performance near thermal limits). That's why I don't like the
implication that soctherm dictates the existence of particular thermal
zones.
-Matt
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,
@@ -89,6 +89,7 @@ struct __thermal_zone {/* trip data */intntrips;struct__thermal_trip*trips;+longprev_low_trip,prev_high_trip;/* cooling binding data */intnum_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 ***/++staticintof_thermal_set_trips(structthermal_zone_device*tz,longtemp)+{+struct__thermal_zone*data=tz->devdata;+longlow=LONG_MIN,high=LONG_MAX;+inti;++/* Hardware trip points not supported */+if(!data->set_trips)+return0;++/* No need to change trip points */+if(temp>data->prev_low_trip&&temp<data->prev_high_trip)+return0;++for(i=0;i<data->ntrips;++i){+struct__thermal_trip*trip=data->trips+i;+longtrip_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;++returndata->set_trips(data->sensor_data,low,high);+}+/*** DT thermal zone device callbacks ***/staticintof_thermal_get_temp(structthermal_zone_device*tz,unsignedlong*temp){struct__thermal_zone*data=tz->devdata;+interr;if(!data->get_temp)return-EINVAL;-returndata->get_temp(data->sensor_data,temp);+err=data->get_temp(data->sensor_data,temp);+if(err)+returnerr;++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.
quoted hunk
+ 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.
quoted hunk
{
struct thermal_zone_device *tzd;
struct __thermal_zone *tz;
+ int err;
tzd = thermal_zone_get_zone_by_name(zone->name);
if (IS_ERR(tzd))
@@ -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))
From: Mikko Perttunen <mperttunen@nvidia.com> Date: 2014-08-01 11:42:11
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:
quoted
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.
quoted
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.
@@ -89,6 +89,7 @@ struct __thermal_zone {/* trip data */intntrips;struct__thermal_trip*trips;+longprev_low_trip,prev_high_trip;/* cooling binding data */intnum_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 ***/++staticintof_thermal_set_trips(structthermal_zone_device*tz,longtemp)+{+struct__thermal_zone*data=tz->devdata;+longlow=LONG_MIN,high=LONG_MAX;+inti;++/* Hardware trip points not supported */+if(!data->set_trips)+return0;++/* No need to change trip points */+if(temp>data->prev_low_trip&&temp<data->prev_high_trip)+return0;++for(i=0;i<data->ntrips;++i){+struct__thermal_trip*trip=data->trips+i;+longtrip_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;++returndata->set_trips(data->sensor_data,low,high);+}+/*** DT thermal zone device callbacks ***/staticintof_thermal_get_temp(structthermal_zone_device*tz,unsignedlong*temp){struct__thermal_zone*data=tz->devdata;+interr;if(!data->get_temp)return-EINVAL;-returndata->get_temp(data->sensor_data,temp);+err=data->get_temp(data->sensor_data,temp);+if(err)+returnerr;++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.
quoted
+ 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.
quoted
{
struct thermal_zone_device *tzd;
struct __thermal_zone *tz;
+ int err;
tzd = thermal_zone_get_zone_by_name(zone->name);
if (IS_ERR(tzd))
@@ -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))
Moro,
On Fri, Aug 1, 2014 at 7:42 AM, Mikko Perttunen [off-list ref] wrote:
Moi Eduardo :)
On 30/07/14 17:16, Eduardo Valentin wrote:
quoted
Terve Mikko,
On Fri, Jun 27, 2014 at 11:11:34AM +0300, Mikko Perttunen wrote:
quoted
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.
quoted
quoted
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.
quoted
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!,
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.
quoted
+ 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.
quoted
{
struct thermal_zone_device *tzd;
struct __thermal_zone *tz;
+ int err;
tzd = thermal_zone_get_zone_by_name(zone->name);
if (IS_ERR(tzd))