Re: [lm-sensors] [PATCHv9 02/20] thermal: introduce device treeparser

[Zhang Rui]

On ä, 2013-11-15 at 09:07 +0100, Jean Delvare wrote:
> Hi Eduardo,
> 
> On Tue, 12 Nov 2013 15:46:04 -0400, Eduardo Valentin wrote:
> > This patch introduces a device tree bindings for
> > describing the hardware thermal behavior and limits.
> > Also a parser to read and interpret the data and feed
> > it in the thermal framework is presented.
> > 
> > This patch introduces a thermal data parser for device
> > tree. The parsed data is used to build thermal zones
> > and thermal binding parameters. The output data
> > can then be used to deploy thermal policies.
> > 
> > This patch adds also documentation regarding this
> > API and how to define tree nodes to use
> > this infrastructure.
> > 
> > Note that, in order to be able to have control
> > on the sensor registration on the DT thermal zone,
> > it was required to allow changing the thermal zone
> > .get_temp callback. For this reason, this patch
> > also removes the 'const' modifier from the .ops
> > field of thermal zone devices.
> > 
> > Cc: Zhang Rui <rui.zhang@xxxxxxxxx>
> > Cc: linux-pm@xxxxxxxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > Cc: Mark Rutland <mark.rutland@xxxxxxx>
> > Signed-off-by: Eduardo Valentin <eduardo.valentin@xxxxxx>
> > ---
> > (...)
> > +static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
> > +				enum thermal_trend *trend)
> > +{
> > +	struct __thermal_zone *data = tz->devdata;
> > +	long dev_trend;
> > +	int r;
> > +
> > +	if (!data->get_trend)
> > +		return -EINVAL;
> > +
> > +	r = data->get_trend(data->sensor_data, &dev_trend);
> > +	if (r)
> > +		return r;
> > +
> > +	/* TODO: These intervals might have some thresholds, but in core code */
> > +	if (dev_trend > 0)
> > +		*trend = THERMAL_TREND_RAISING;
> > +	else if (dev_trend < 0)
> > +		*trend = THERMAL_TREND_DROPPING;
> > +	else
> > +		*trend = THERMAL_TREND_STABLE;
> > +
> > +	return 0;
> > +}
> 
> I don't like the whole trend thing.
> 
> I've been writing hwmon drivers for the past decade and I've never seen
> a thermal sensor device being able to report trends. And as a rule of
> thumb, if the hardware can't do it then the (hardware-specific) drivers
> should not report it.

I agree that a sensor device should not be able to report trends. But
currently, the thermal zone driver is not only a sensor driver, it also
owns the platform thermal cooling strategy.
And some platforms do have this kind of knowledge, right?
e.g. ACPI spec provides an equation for processor passive cooling (ACPI
Spec 5.0 11.1.5).

> 
> Hwmon devices (and drivers) report temperature values, and sometimes
> historical min/max. They can do that because these are facts that need
> no interpretation.
> 
> Defining a trend, however, requires care and, more importantly,
> decisions.

We had the assumption that we will get an interrupt when the firmware
thinks there is an temperature change that should be noticed by OS.

>  For example, consider a thermal sensor which reports 50ÂC at
> time t, then 47ÂC at time t+3s,

does firmware thinks this should be noticed by OS?
If no, there is no interrupt and this temperature change is totally
transparent to the OS at all.
If yes, the thermal core will check if the system is overheating, if
yes, it throttles the devices, and if no, do nothing.

>  then 48ÂC at time t+6s. At t+7s someone
> asks for the trend,

the trend is needed when platform thermal driver calls
thermal_zone_device_update(), and mostly following a temperature change
interrupt.

>  what should the driver reply?
>  If you consider only
> the last 5 seconds, temperature is raising. If you consider the last 10
> seconds instead, temperature is dropping. How would the driver know
> what time frame the caller is interested in?
> 
> Another example: "small" temperature variations. If temperatures varies
> by 1ÂC in my kitchen's oven, I'd call it stable. If my body temperature
> varies by 1ÂC, I'd call it non-stable, and my doctor for an appointment
> also ;-)
> 

> My point is that only the caller, and not the driver, knows how to
> convert a series of measurements into a trend. So I don't think drivers
> should implement anything like get_trend(). Whatever piece of code
> needs to establish a trend should call get_temp() repeatedly, store the
> results and do its own analysis of the data.
> 
I think the key problem is that,
the thermal subsystem just provides a basic/minimum thermal control
framework in kernel, and it is totally platform independent. And any
platform driver can use this framework to do some basic thermal control
easily but just telling thermal core if the thermal core should take
some cooling actions (trip points + trend + current temperature) and how
(by choosing thermal governors).

So if you want to do more precise thermal control, you'd better disable
kernel thermal control and do it in userspace. In this case,
the .get_trend() will never be invoked at all.
Further more, I'm indeed considering introduce platform specific thermal
governors, so that platform thermal driver can tell thermal core what to
do at a certain point.

thanks,
rui

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/