Date:	Mon, 6 Jan 2014 14:52:29 -0400
From:	Eduardo Valentin <eduardo.valentin@...com>
To:	Matthew Longnecker <mlongnecker@...dia.com>
CC:	Eduardo Valentin <eduardo.valentin@...com>,
	<swarren@...dotorg.org>, <pawel.moll@....com>,
	<mark.rutland@....com>, <ian.campbell@...rix.com>,
	<rob.herring@...xeda.com>, <linux@...ck-us.net>,
	<rui.zhang@...el.com>, <wni@...dia.com>, <grant.likely@...aro.org>,
	<durgadoss.r@...el.com>, <linux-pm@...r.kernel.org>,
	<devicetree@...r.kernel.org>, <lm-sensors@...sensors.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCHv9 02/20] thermal: introduce device tree parser

On 02-01-2014 13:35, Matthew Longnecker wrote:
> Eduardo,

Hello Matthew,

> 
> For the most part, this binding is really well thought out. It makes a
> lot of sense to me (as someone who has been working with thermal
> management in Linux/Android-based mobile devices for a few years).

No issues, I also have been there.

> 
> However, I have one substantive criticism.
> 
> On 11/12/2013 11:46 AM, Eduardo Valentin wrote:
>> +* Thermal zone nodes
>> +
>> +The thermal zone node is the node containing all the required info
>> +for describing a thermal zone, including its cooling device bindings.
>> The
>> +thermal zone node must contain, apart from its own properties, one
>> sub-node
>> +containing trip nodes and one sub-node containing all the zone
>> cooling maps.
>> +
>> +Required properties:
> ...
>> +- thermal-sensors:    A list of thermal sensor phandles and sensor
>> specifier
>> +  Type: list of     used while monitoring the thermal zone.
>> +  phandles + sensor
>> +  specifier
> ...
>> +Optional property:
>> +- coefficients:        An array of integers (one signed cell) containing
>> +  Type: array        coefficients to compose a linear relation between
>> +  Elem size: one cell    the sensors listed in the thermal-sensors
>> property.
>> +  Elem type: signed    Coefficients defaults to 1, in case this property
>> +            is not specified. A simple linear polynomial is used:
>> +            Z = c0 * x0 + c1 + x1 + ... + c(n-1) * x(n-1) + cn.
>> +
>> +            The coefficients are ordered and they match with sensors
>> +            by means of sensor ID. Additional coefficients are
>> +            interpreted as constant offset.
> 
> 
> "coefficients" is a problematic way of describing the relationship
> between temperatures at various sensors and temperature at some other
> location. It would make sense if heat flowed infinitely quickly.
> However, in practice thermal capacitance means that we need to take into
> account the _history_ of temperature at sensors in order to predict heat
> coupled into a distant point.

I agree. But coefficients are targeting in fact the steady state cases.
As described in the DT binding documentation, the case of an offset due
to sensor location distance to hotspot is a perfect usage of this property.

The thermal capacitance behavior is described, so far, most based on the
requested time requirement for each zone. Of course, this point was a
major point for discussion during this thread. Hopefully I was able to
keep the minimal time behavior requirements in the DT definition.

> 
> For example, assuming that handset enclosure starts at ~25C, the CPU
> could burst to 100C for many minutes before the handset enclosure
> reaches ~40C. However, at steady-state, the CPU might only be able to
> sustain 65C without pushing the enclosure above 40C.


Yeah, but here you are maybe confusing two different constraints, and
possibly the behavior of two different zones. The current binding do not
limit you in the usage of hierarchical description. Thus, you can and
should describe two different zones to cover for two different
constraints you have in your description, one to cover for your silicon
junction (>100C) and another to cover for your case / device skin
hotspot (<45C). And in each zone you may have different limits and
thermal capacitance requirements. Pay attention that it does not mean
that you CPU (cooling) device cannot appear in two different zone. Thus
its contribution to one zone may be different to the other (each zone
can assume different coefficients).

> 
> I wouldn't be complaining except that you're proposing this as a DT
> definition. In this case, the binding you've proposed is poor
> abstraction of the hardware.

OK. The main reason I made this property optional is exactly because
different use case may require different usage of these relations. And
possibly people may have different experience and different solutions to
the same problem. But in the end what we want is to describe the
hardware (and possibly the physics) behind the use cases. And I do
believe we have been dealing with very similar cases here.

I don't think the property is a poor abstraction. Firstly because it is
not limiting you in anything. Secondly because you may be having a
different interpretation of it, as I explained above.

In fact the thermal behavior is much more dynamic than the steady state
situation. However, so far, I haven't seen a real case that we need to
describe these dynamics deeper in DT. Including for the example you gave
above. Dealing with the history can be derived by the OS based on the
already defined properties.

> 
> thanks,

No problem, thanks for your contribution. Again, if we are not covering
your use cases properly, let's go through them and narrow a proper
solution down.

> Matt Longnecker
> 
> p.s. I apologize for chiming in without having read the entire history
> of the patch set. Engineers on my team will be trying this out for Tegra
> within the next few weeks.
> 

OK.

> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


Download attachment "signature.asc" of type "application/pgp-signature" (296 bytes)

Powered by blists - more mailing lists