| Message ID | E1cn8qC-0002uz-PB@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Eduardo Valentin |
| Headers | show |
On Sun, Mar 12, 2017 at 07:07:40PM +0000, Russell King wrote: > Allow hwmon devices to be optionally created for of-thermal zones, > rather than permanently denying them "in case" there is a hwmon > driver duplicating the thermal driver. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > --- > Without this, "sensors" is unable to report the temperature of the > Dove SoC when it supports thermal zones. > > Documentation/devicetree/bindings/thermal/thermal.txt | 3 +++ > arch/arm/boot/dts/dove.dtsi | 1 + > drivers/thermal/of-thermal.c | 3 ++- > 3 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt > index 88b6ea1ad290..1478735fff85 100644 > --- a/Documentation/devicetree/bindings/thermal/thermal.txt > +++ b/Documentation/devicetree/bindings/thermal/thermal.txt > @@ -175,6 +175,9 @@ containing trip nodes and one sub-node containing all the zone cooling maps. > 2000mW, while on a 10'' tablet is around > 4500mW. > > +- linux,hwmon: Allow Linux to create hwmon devices for the thermal > + Type: bool zone. > + > Note: The delay properties are bound to the maximum dT/dt (temperature > derivative over time) in two situations for a thermal zone: > (i) - when passive cooling is activated (polling-delay-passive); and > diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi > index 40fb98687230..00f5971cd039 100644 > --- a/arch/arm/boot/dts/dove.dtsi > +++ b/arch/arm/boot/dts/dove.dtsi > @@ -804,6 +804,7 @@ > > thermal-zones { > soc-thermal { > + linux,hwmon; I'd prefer to see a black or white list of sensor compatibles here. Then the dtb doesn't need to change if things move between hwmon and thermal zones. Rob
On Mon, Mar 20, 2017 at 12:15:52PM -0500, Rob Herring wrote: > On Sun, Mar 12, 2017 at 07:07:40PM +0000, Russell King wrote: > > Allow hwmon devices to be optionally created for of-thermal zones, > > rather than permanently denying them "in case" there is a hwmon > > driver duplicating the thermal driver. > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > --- > > Without this, "sensors" is unable to report the temperature of the > > Dove SoC when it supports thermal zones. > > > > Documentation/devicetree/bindings/thermal/thermal.txt | 3 +++ > > arch/arm/boot/dts/dove.dtsi | 1 + > > drivers/thermal/of-thermal.c | 3 ++- > > 3 files changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt > > index 88b6ea1ad290..1478735fff85 100644 > > --- a/Documentation/devicetree/bindings/thermal/thermal.txt > > +++ b/Documentation/devicetree/bindings/thermal/thermal.txt > > @@ -175,6 +175,9 @@ containing trip nodes and one sub-node containing all the zone cooling maps. > > 2000mW, while on a 10'' tablet is around > > 4500mW. > > > > +- linux,hwmon: Allow Linux to create hwmon devices for the thermal > > + Type: bool zone. > > + > > Note: The delay properties are bound to the maximum dT/dt (temperature > > derivative over time) in two situations for a thermal zone: > > (i) - when passive cooling is activated (polling-delay-passive); and > > diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi > > index 40fb98687230..00f5971cd039 100644 > > --- a/arch/arm/boot/dts/dove.dtsi > > +++ b/arch/arm/boot/dts/dove.dtsi > > @@ -804,6 +804,7 @@ > > > > thermal-zones { > > soc-thermal { > > + linux,hwmon; > > I'd prefer to see a black or white list of sensor compatibles here. Then > the dtb doesn't need to change if things move between hwmon and thermal > zones. I'm not sure what you mean, and I can't see how that would be coded up. The thermal layer doesn't give the option of individually selecting which sensors have hwmon stuff created - it seems to be all or nothing. I suspect what you're asking would require something of a rewrite of the thermal code. The sensors are registered completely independently of parsing the thermal zone data.
On Fri, Mar 24, 2017 at 01:23:13PM +0000, Russell King - ARM Linux wrote: > On Mon, Mar 20, 2017 at 12:15:52PM -0500, Rob Herring wrote: > > On Sun, Mar 12, 2017 at 07:07:40PM +0000, Russell King wrote: > > > Allow hwmon devices to be optionally created for of-thermal zones, > > > rather than permanently denying them "in case" there is a hwmon > > > driver duplicating the thermal driver. > > > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > > --- > > > Without this, "sensors" is unable to report the temperature of the > > > Dove SoC when it supports thermal zones. > > > > > > Documentation/devicetree/bindings/thermal/thermal.txt | 3 +++ > > > arch/arm/boot/dts/dove.dtsi | 1 + > > > drivers/thermal/of-thermal.c | 3 ++- > > > 3 files changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt > > > index 88b6ea1ad290..1478735fff85 100644 > > > --- a/Documentation/devicetree/bindings/thermal/thermal.txt > > > +++ b/Documentation/devicetree/bindings/thermal/thermal.txt > > > @@ -175,6 +175,9 @@ containing trip nodes and one sub-node containing all the zone cooling maps. > > > 2000mW, while on a 10'' tablet is around > > > 4500mW. > > > > > > +- linux,hwmon: Allow Linux to create hwmon devices for the thermal > > > + Type: bool zone. > > > + > > > Note: The delay properties are bound to the maximum dT/dt (temperature > > > derivative over time) in two situations for a thermal zone: > > > (i) - when passive cooling is activated (polling-delay-passive); and > > > diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi > > > index 40fb98687230..00f5971cd039 100644 > > > --- a/arch/arm/boot/dts/dove.dtsi > > > +++ b/arch/arm/boot/dts/dove.dtsi > > > @@ -804,6 +804,7 @@ > > > > > > thermal-zones { > > > soc-thermal { > > > + linux,hwmon; > > > > I'd prefer to see a black or white list of sensor compatibles here. Then > > the dtb doesn't need to change if things move between hwmon and thermal > > zones. > > I'm not sure what you mean, and I can't see how that would be coded up. > The thermal layer doesn't give the option of individually selecting > which sensors have hwmon stuff created - it seems to be all or nothing. > I suspect what you're asking would require something of a rewrite of > the thermal code. The sensors are registered completely independently > of parsing the thermal zone data. Okay, I'm convinced that what you're asking for (if I'm understanding correctly) is just not possible with how the thermal code is (horribly) architected at the moment. While trying to understand how all the thermal zone stuff fits together, I found this comment: /* For now, thermal framework supports only 1 sensor per zone */ ret = of_parse_phandle_with_args(child, "thermal-sensors", "#thermal-sensor-cells", 0, &sensor_specs); so it's really not setup to deal with multiple sensors in DT right now (and there's no checking by the code that this is even the case.) The way the code is architected, the thermal zone devices and hwmon support is setup _completely_ independently from attaching the single sensor to the zone - the setup of the thermal zone devices and registering the sysfs entry happens before the sensors. The hwmon support is coded such that there is one hwmon device for all thermal zones. Each thermal zone device exports exactly one hwmon temperature sensor value with one set of trip points. A thermal zone device can support multiple sensors, but from the code and the above comment, the code does not support this - the sensors are not individually exported through hwmon. So, I think what you're asking for is not possible and is based upon a mis-understanding - thermal does not export sensors, but exports the _zone_ itself as a hwmon sensor. The zone temperature is exported, along with its thresholds. Obviously, the separation of thermal vs hwmon is purely a Linux phenomenon, and I've wondered why people have to rewrite hwmon drivers as thermal drivers just to make use of the thermal subsystem - it seems to me like NIH syndrome. I can't see why hwmon isn't able to export individual temperature sensors to thermal and have thermal import the values from there. For example, you could have a hwmon device that has multiple temperature sensors (some do) which monitor different parts of the system, and a hwmon device should be able to be incorporated into a thermal zone for management. That doesn't seem possible, you have to implement a thermal driver to interface your sensor to the thermal stuff, and then have the _zone_ statistics exported through hwmon. If you want individual sensors exported, you have to implement a driver which has a dual-personality, it has to register with hwmon and thermal. I'd say that, arguably, the whole "no_hwmon" thing in thermal is also wrong - it may _seem_ to be correct because thermal only supports one sensor, and we don't want such a dual-personality driver to have two entries in sysfs, but that's rather moot when you consider that one is exporting the zone (which has DT configured thresholds) and one is exporting the sensor itself. So, my conclusion is that this is all rather messed up, and certainly beyond my willingness to put in a lot of effort, creating lots of patches to try and bring the code to a point where what you're asking for is possible (iow, where we export individual sensors to hwmon, and make individual sensors exportable.) Therefore, I believe that my implementation is entirely reasonable - the linux,hwmon flag indicates whether the state of the thermal _zone_ (not the _sensors_) should be exported via hwmon.
Hello, On Fri, Mar 24, 2017 at 02:40:29PM +0000, Russell King - ARM Linux wrote: > On Fri, Mar 24, 2017 at 01:23:13PM +0000, Russell King - ARM Linux wrote: > > On Mon, Mar 20, 2017 at 12:15:52PM -0500, Rob Herring wrote: > > > On Sun, Mar 12, 2017 at 07:07:40PM +0000, Russell King wrote: > > > > Allow hwmon devices to be optionally created for of-thermal zones, > > > > rather than permanently denying them "in case" there is a hwmon > > > > driver duplicating the thermal driver. > > > > > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > > > --- > > > > Without this, "sensors" is unable to report the temperature of the > > > > Dove SoC when it supports thermal zones. > > > > > > > > Documentation/devicetree/bindings/thermal/thermal.txt | 3 +++ > > > > arch/arm/boot/dts/dove.dtsi | 1 + > > > > drivers/thermal/of-thermal.c | 3 ++- > > > > 3 files changed, 6 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt > > > > index 88b6ea1ad290..1478735fff85 100644 > > > > --- a/Documentation/devicetree/bindings/thermal/thermal.txt > > > > +++ b/Documentation/devicetree/bindings/thermal/thermal.txt > > > > @@ -175,6 +175,9 @@ containing trip nodes and one sub-node containing all the zone cooling maps. > > > > 2000mW, while on a 10'' tablet is around > > > > 4500mW. > > > > > > > > +- linux,hwmon: Allow Linux to create hwmon devices for the thermal > > > > + Type: bool zone. > > > > + > > > > Note: The delay properties are bound to the maximum dT/dt (temperature > > > > derivative over time) in two situations for a thermal zone: > > > > (i) - when passive cooling is activated (polling-delay-passive); and > > > > diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi > > > > index 40fb98687230..00f5971cd039 100644 > > > > --- a/arch/arm/boot/dts/dove.dtsi > > > > +++ b/arch/arm/boot/dts/dove.dtsi > > > > @@ -804,6 +804,7 @@ > > > > > > > > thermal-zones { > > > > soc-thermal { > > > > + linux,hwmon; New DT properties have to get an Ack from OF maintainers. > > > > > > I'd prefer to see a black or white list of sensor compatibles here. Then > > > the dtb doesn't need to change if things move between hwmon and thermal > > > zones. > > > > I'm not sure what you mean, and I can't see how that would be coded up. > > The thermal layer doesn't give the option of individually selecting > > which sensors have hwmon stuff created - it seems to be all or nothing. > > I suspect what you're asking would require something of a rewrite of > > the thermal code. The sensors are registered completely independently > > of parsing the thermal zone data. > I agree here that Rob could have been more clear. Maybe he just wants to avoid DTB changes if the driver decides to move from one subsystem to the other. > Okay, I'm convinced that what you're asking for (if I'm understanding > correctly) is just not possible with how the thermal code is (horribly) > architected at the moment. > > While trying to understand how all the thermal zone stuff fits together, > I found this comment: > > /* For now, thermal framework supports only 1 sensor per zone */ > ret = of_parse_phandle_with_args(child, "thermal-sensors", > "#thermal-sensor-cells", > 0, &sensor_specs); > > so it's really not setup to deal with multiple sensors in DT right now > (and there's no checking by the code that this is even the case.) > Yes, the thermal core code still does not really support multiple sensors per zone. Its original design decision to have one sensor representing one zone is still kept, even though there has been attempts to change this. So, the of thermal code, although the DT thermal descriptors support multiple sensors per zone, is still limited to one sensor per zone, otherwise it would be pretty much a fork from thermal core. > The way the code is architected, the thermal zone devices and hwmon > support is setup _completely_ independently from attaching the single > sensor to the zone - the setup of the thermal zone devices and > registering the sysfs entry happens before the sensors. > > The hwmon support is coded such that there is one hwmon device for all > thermal zones. Each thermal zone device exports exactly one hwmon > temperature sensor value with one set of trip points. > Based on git log, the hwmon support was historically written some time before some of the recent changes on the hwmon subsystem. Looking at both of them now, I must say the thermal_hwmon.c needs to be rewritten, given that most of the sysfs interfaces are already handled by the hwmon core. Also, there seams to be thermal subsystem awareness now on hwmon core. > A thermal zone device can support multiple sensors, but from the code > and the above comment, the code does not support this - the sensors > are not individually exported through hwmon. As I mentioned above, the DT thermal descriptor does not limit the relationship to 1:1, but the code is limited, yes. > > So, I think what you're asking for is not possible and is based upon > a mis-understanding - thermal does not export sensors, but exports the > _zone_ itself as a hwmon sensor. The zone temperature is exported, > along with its thresholds. > > Obviously, the separation of thermal vs hwmon is purely a Linux > phenomenon, and I've wondered why people have to rewrite hwmon drivers > as thermal drivers just to make use of the thermal subsystem - it seems > to me like NIH syndrome. I can't see why hwmon isn't able to export > individual temperature sensors to thermal and have thermal import the > values from there. > Not sure if I got this point, but I do not see drivers being rewritten. That is more like, organically, some drivers went into hwmon, some went into thermal subsystem. I believe most of the drivers in the thermal subsystem decided to go that route due to the control algorithm to keep up on silicon temperature, given that most of them are on zones to control temperature of cores. > For example, you could have a hwmon device that has multiple temperature > sensors (some do) which monitor different parts of the system, and a > hwmon device should be able to be incorporated into a thermal zone for > management. > I think this is the idea behind the most recent changes related to thermal on the hwmon core, so hwmon temperature sensors could be exposed as zones, and therefore, system engineers could benefit of the temperature control in kernel. But I frankly did not really use this late support to really judge to which extent we get on the control. > That doesn't seem possible, you have to implement a thermal driver to > interface your sensor to the thermal stuff, and then have the _zone_ > statistics exported through hwmon. > > If you want individual sensors exported, you have to implement a driver > which has a dual-personality, it has to register with hwmon and thermal. > > I'd say that, arguably, the whole "no_hwmon" thing in thermal is also > wrong - it may _seem_ to be correct because thermal only supports one > sensor, and we don't want such a dual-personality driver to have two > entries in sysfs, but that's rather moot when you consider that one is > exporting the zone (which has DT configured thresholds) and one is > exporting the sensor itself. > > So, my conclusion is that this is all rather messed up, and certainly > beyond my willingness to put in a lot of effort, creating lots of > patches to try and bring the code to a point where what you're asking > for is possible (iow, where we export individual sensors to hwmon, > and make individual sensors exportable.) > > Therefore, I believe that my implementation is entirely reasonable - > the linux,hwmon flag indicates whether the state of the thermal _zone_ > (not the _sensors_) should be exported via hwmon. > Yeah, here your patch seams to be willing to use the thermal_hwmon support, which goes from thermal subsystem to hwmon. And yes, the no_hwmon, in this case, is used to avoid having two hwmon sysfs entry for the same sensor, one coming from a real hwmon driver, another one exported from the thermal zone. The only reason one would still want to have both hwmon interfaces, it would be in case a thermal zone has, for example, some extrapolation rule defined in DT, and therefore reading the raw hwmon sysfs entry would not necessarily mean you have the same thermal zone temperature value. Then again, the zone temperature value can still, be read from the thermal zone sysfs entry. But,... Thinking of this use case, and considering that more and more drivers are now using extrapolation rules on DT, at least the simple linear extrapolation, then I would suggest simply always adding the hwmon interface from of thermal, instead. In this way, userspace that knows only about hwmon entries may be able to know of the raw values and the extrapolated values based on the zone. Then again, this needs to be clear to avoid confusion, so people understand the differentiation. > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net.
diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt index 88b6ea1ad290..1478735fff85 100644 --- a/Documentation/devicetree/bindings/thermal/thermal.txt +++ b/Documentation/devicetree/bindings/thermal/thermal.txt @@ -175,6 +175,9 @@ containing trip nodes and one sub-node containing all the zone cooling maps. 2000mW, while on a 10'' tablet is around 4500mW. +- linux,hwmon: Allow Linux to create hwmon devices for the thermal + Type: bool zone. + Note: The delay properties are bound to the maximum dT/dt (temperature derivative over time) in two situations for a thermal zone: (i) - when passive cooling is activated (polling-delay-passive); and diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi index 40fb98687230..00f5971cd039 100644 --- a/arch/arm/boot/dts/dove.dtsi +++ b/arch/arm/boot/dts/dove.dtsi @@ -804,6 +804,7 @@ thermal-zones { soc-thermal { + linux,hwmon; polling-delay-passive = <250>; /* ms */ polling-delay = <1000>; /* ms */ thermal-sensors = <&thermal>; diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c index d04ec3b9e5ff..5cd1838405f0 100644 --- a/drivers/thermal/of-thermal.c +++ b/drivers/thermal/of-thermal.c @@ -995,7 +995,8 @@ int __init of_parse_thermal_zones(void) } /* No hwmon because there might be hwmon drivers registering */ - tzp->no_hwmon = true; + if (!of_property_read_bool(child, "linux,hwmon")) + tzp->no_hwmon = true; if (!of_property_read_u32(child, "sustainable-power", &prop)) tzp->sustainable_power = prop;
Allow hwmon devices to be optionally created for of-thermal zones, rather than permanently denying them "in case" there is a hwmon driver duplicating the thermal driver. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- Without this, "sensors" is unable to report the temperature of the Dove SoC when it supports thermal zones. Documentation/devicetree/bindings/thermal/thermal.txt | 3 +++ arch/arm/boot/dts/dove.dtsi | 1 + drivers/thermal/of-thermal.c | 3 ++- 3 files changed, 6 insertions(+), 1 deletion(-)