Date:	Thu, 21 Nov 2013 11:48:08 -0400
From:	Eduardo Valentin <eduardo.valentin@...com>
To:	Tomasz Figa <t.figa@...sung.com>
CC:	Eduardo Valentin <eduardo.valentin@...com>,
	Tomasz Figa <tomasz.figa@...il.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 21-11-2013 10:57, Tomasz Figa wrote:
> On Friday 15 of November 2013 09:19:02 Eduardo Valentin wrote:
>> Hello Tomasz,
>>
>> On 14-11-2013 09:40, Tomasz Figa wrote:
>>> On Thursday 14 of November 2013 07:31:04 Eduardo Valentin wrote:
>>>> On 13-11-2013 12:57, Tomasz Figa wrote:
>>>>> Hi Eduardo,
>>>>>
>>>>
>>>> Hello Tomaz
>>>>
>>>>> On Tuesday 12 of November 2013 15:46:04 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.
>>>>> [snip]
>>>>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..59f5bd2
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
>>>>>> @@ -0,0 +1,586 @@
>>>>> [snip]
>>>>>> +* Cooling device nodes
>>>>>> +
>>>>>> +Cooling devices are nodes providing control on power dissipation. There
>>>>>> +are essentially two ways to provide control on power dissipation. First
>>>>>> +is by means of regulating device performance, which is known as passive
>>>>>> +cooling. A typical passive cooling is a CPU that has dynamic voltage and
>>>>>> +frequency scaling (DVFS), and uses lower frequencies as cooling states.
>>>>>> +Second is by means of activating devices in order to remove
>>>>>> +the dissipated heat, which is known as active cooling, e.g. regulating
>>>>>> +fan speeds. In both cases, cooling devices shall have a way to determine
>>>>>> +the state of cooling in which the device is.
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- cooling-min-state:	An integer indicating the smallest
>>>>>> +  Type: unsigned	cooling state accepted. Typically 0.
>>>>>> +  Size: one cell
>>>>>
>>>>> Could you explain (just in a reply) what a cooling state is and what are
>>>>> the min and max values used for?
>>>>
>>>> Cooling state is an unsigned integer which represents heat control that
>>>> a cooling device implies.
>>>
>>> OK. So you have a cooling device and it can have multiple cooling states,
>>> like a cooling fan that has multiple speed levels. Did I understand this
>>> correctly?
>>>
>>> IMHO this should be also explained in the documentation above, possibly
>>> with one or two examples.
>>
>>
>> There are more than one example in this file. Even explaining why
>> cooling min and max are used, and the difference of min and max
>> properties that appear in cooling device and those present in a cooling
>> specifier. Cooling devices and cooling state are described in the
>> paragraph above.
> 
> I mean, the definition I commented about is completely confusing. You
> should rewrite it in a more readable way. For example, "Cooling state is
> an unsigned integer which represents level of cooling performance of
> a thermal device." would be much more meaningful, if I understood the
> whole idea of "cooling state" correctly.

Yeah, I see your point. But I avoided describing cooling state as a
property, as you are requesting, because in fact it is not a property.
Its min and max values are properties, as you can see in the document.
Describing cooling state as a property in this document will confuse
people, because it won't be used in any part of the hardware description.

> 
> By example I mean simply listing one or two possible practical meanings,
> like "(e.g. speed level of a cooling fan, performance throttling level of
> CPU)".

Mark R. has already requested this example and I already wrote it. There
is a comment in the CPU example session explaining exactly what you are
asking. Have you missed that one?

> 
>>
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> +- cooling-max-state:	An integer indicating the largest
>>>>>> +  Type: unsigned	cooling state accepted.
>>>>>> +  Size: one cell
>>>>>> +
>>>>>> +- #cooling-cells:	Used to provide cooling device specific information
>>>>>> +  Type: unsigned	while referring to it. Must be at least 2, in order
>>>>>> +  Size: one cell      	to specify minimum and maximum cooling state used
>>>>>> +			in the reference. The first cell is the minimum
>>>>>> +			cooling state requested and the second cell is
>>>>>> +			the maximum cooling state requested in the reference.
>>>>>> +			See Cooling device maps section below for more details
>>>>>> +			on how consumers refer to cooling devices.
>>>>>> +
>>>>>> +* Trip points
>>>>>> +
>>>>>> +The trip node is a node to describe a point in the temperature domain
>>>>>> +in which the system takes an action. This node describes just the point,
>>>>>> +not the action.
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- temperature:		An integer indicating the trip temperature level,
>>>>>> +  Type: signed		in millicelsius.
>>>>>> +  Size: one cell
>>>>>> +
>>>>>> +- hysteresis:		A low hysteresis value on temperature property (above).
>>>>>> +  Type: unsigned	This is a relative value, in millicelsius.
>>>>>> +  Size: one cell
>>>>>
>>>>> What about replacing temperature and hysteresis with a single temperature
>>>>> property that can be either one cell for 0 hysteresis or two cells to
>>>>> specify lower and upper range of temperatures?
>>>>
>>>> What is the problem with using two properties? I think we loose
>>>> representation by gluing two properties into one just because one cell.
>>>
>>> Ranges is the representation widely used in other bindings. In addition
>>
>> Well that sentence is arguable. It is not like all properties in DT are
>> standardized as ranges, is it?
> 
> No, they are not, but property range is a common concept of representing
> range of some values.
> 
> Anyway, I won't insist on this, as it's just a minor detail. However I'd
> like to see comments from more people on this.
> 
>>
>>> I believe a range is more representative - when reading a DTS you don't
>>> need to think whether the hysteresis is in temperature units, percents or
>>> something else and also is less ambiguous, because you have clearly
>>> defined lower and upper bounds in one place.
>>
>> It is the other way around. For a person designing a thermal
>> representation for a specific board it is intuitive to think about
>> hysteresis in this case. It is a better representation because we are
>> really talking about a hysteresis here in order to give some room for
>> the system to react on temporary temperature transitions around that
>> point. It is possible to use ranges as you are suggesting, but it
>> becomes confusing.
>>
> 
> Probably it depends on what you are used to. I'd like to see opinion
> of more people on this.
> 
>>
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> +- type:			a string containing the trip type. Supported values are:
>>>>>> +	"active":	A trip point to enable active cooling
>>>>>> +	"passive":	A trip point to enable passive cooling
>>>>>
>>>>> The two above seem to be implying action, as opposed to the general
>>>>> comment about trip points.
>>>>
>>>> They do not imply action, they specify type of trip.
>>>
>>> For me "A trip point to enable active cooling" means that when this trip
>>> point is crossed, active cooling must be enabled. What is it if not
>>> implying action?
>>
>> But within a board there could be more than one active cooling actions
>> that could be done, it is not a 1 to 1 relation. Same thing applies to
>> passive cooling. The binding does not imply a specific action. Just the
>> trip type.
> 
> I'd prefer the "active" and "passive" states to be renamed an their
> descriptions rephrased. In general, the idea of having just four trip
> point types seems like bound to a single specific hardware platform.
> 

Not it is in fact not. These concepts are derived from an specification
that is there for decades actually, ACPI. I am not reinventing the wheel
here. There are several platforms based on that specification. The
concepts have been reused on top of non-ACPI platforms too.

> That's a wild guess, but maybe having an unsigned integer to represent
> trip point "attention level" would be a better idea?

It would be less representative and not standardized. Do you have a
class of boards, or at least a single board that is using the proposed
standard?

> 
>>
>>>
>>>>
>>>>>
>>>>>> +	"hot":		A trip point to notify emergency
>>>>>> +	"critical":	Hardware not reliable.
>>>>>> +  Type: string
>>>>>> +
>>>>> [snip]
>>>>>> +* Examples
>>>>>> +
>>>>>> +Below are several examples on how to use thermal data descriptors
>>>>>> +using device tree bindings:
>>>>>> +
>>>>>> +(a) - CPU thermal zone
>>>>>> +
>>>>>> +The CPU thermal zone example below describes how to setup one thermal zone
>>>>>> +using one single sensor as temperature source and many cooling devices and
>>>>>> +power dissipation control sources.
>>>>>> +
>>>>>> +#include <dt-bindings/thermal/thermal.h>
>>>>>> +
>>>>>> +cpus {
>>>>>> +	/*
>>>>>> +	 * Here is an example of describing a cooling device for a DVFS
>>>>>> +	 * capable CPU. The CPU node describes its four OPPs.
>>>>>> +	 * The cooling states possible are 0..3, and they are
>>>>>> +	 * used as OPP indexes. The minimum cooling state is 0, which means
>>>>>> +	 * all four OPPs can be available to the system. The maximum
>>>>>> +	 * cooling state is 3, which means only the lowest OPPs (198MHz@...5V)
>>>>>> +	 * can be available in the system.
>>>>>> +	 */
>>>>>> +	cpu0: cpu@0 {
>>>>>> +		...
>>>>>> +		operating-points = <
>>>>>> +			/* kHz    uV */
>>>>>> +			970000  1200000
>>>>>> +			792000  1100000
>>>>>> +			396000  950000
>>>>>> +			198000  850000
>>>>>> +		>;
>>>>>> +		cooling-min-state = <0>;
>>>>>> +		cooling-max-state = <3>;
>>>>>> +		#cooling-cells = <2>; /* min followed by max */
>>>>>
>>>>> I believe you don't need the min- and max-state properties here then. Your
>>>>> thermal core can simply query the cpufreq driver (which would be a cooling
>>>>> device here) about the range of states it supports
>>>>
>>>> This binding is not supposed to be aware of cpufreq, which is Linux
>>>> specific implementation.
>>>
>>> I didn't say anything about making the binding aware of cpufreq, but
>>> instead about getting rid of redundancy of data, that can be provided
>>> by respective drivers anyway.
>>
>> There are cases in which cooling devices don't need to use all states
>> for cooling, because either lower states does not provide cooling
>> effectiveness or higher states must be avoided at all. So, allowing
>> drivers to report this thermal info is possible, but questionable
>> design, as you would be spreading the thermal info. Besides, for the
>> example I gave, the driver would need to know board specifics, as the
>> range of states would vary anyway across board.
> 
> One thing is the allowed range of cooling states supported by the
> cooling device and another is the range of states that are usable on
> given board. The former is a property of the cooling device that
> in most cases is implied by its compatible string, so to eliminate the
> bad data redundancy, you should not make the user respecify that. The
> latter you have as a part of your cooling device specifier tuple.
> 

So, I still don't understand what is wrong with the binding, as it is
supposed to cover the situations you are talking about. I really don't
see to where this discussion is leading to.

> As an example of this, see PWM, GPIO or IRQ bindings. You don't have
> {pwm-channel,pwm-duty,interrupt,gpio}-{min,max} properties, because this
> is not needed by respective PWM, GPIO or IRQ drivers, as they already
> know these parameters.

Which are completely different devices and concepts.

> 
>>
>>>
>>>>
>>>> Besides, the cpufreq layer is populated by data already specified in DT.
>>>> .
>>>>>
>>>>>> +	};
>>>>>> +	...
>>>>>> +};
>>>>>> +
>>>>>> +&i2c1 {
>>>>>> +	...
>>>>>> +	/*
>>>>>> +	 * A simple fan controller which supports 10 speeds of operation
>>>>>> +	 * (represented as 0-9).
>>>>>> +	 */
>>>>>> +	fan0: fan@...8 {
>>>>>> +		...
>>>>>> +		cooling-min-state = <0>;
>>>>>> +		cooling-max-state = <9>;
>>>>>
>>>>> This is similar. The fan driver will probaby know about available
>>>>> fan speed levels and be able to report the range of states to thermal
>>>>> core.
>>>>
>>>> Then we loose also the flexibility to assign cooling states to trip
>>>> points, as described in this binding.
>>>
>>> I don't think you got my point correctly.
>>>
>>> Let's say you have a CPU, which has 4 operating points. You don't need
>>> to specify that min state is 0 and max state is 4, because it is implied
>>> by the list of operating points.
>>
>> Please read my explanation above.
>>
>>>
>>> Same goes for a fan controller that has, let's say, an 8-bit PWM duty
>>> cycle register, which in turn allows 256 different speed states. This
>>> implies that min state for it is 0 and max state 255 and you don't need
>>> to specify this in DT.
>>
>> ditto.
>>
>>>
>>> Now, both a CPU and fan controller will have their thermal drivers, which
>>> can report to the thermal core what ranges of cooling states they support,
>>> which again makes it wrong to specify such data in DT.
>>
>>
>> Please also read the examples I gave in the thermal binding. There are
>> case that the designer may want to assign a range of states to
>> temperature trip points. This binding is flexible enough to cover for
>> that situation. And without the representation of these limits it would
>> be hard to read the binding. It is not redundant data, please check the
>> documentation.
> 
> I don't see any problem with just dropping the min and max properties.
> All the use cases you listed above will still work, as you have the
> cooling state limits included in cooling device specifier.

Because it is not about making using cases to work, but describing the
hardware thermal limits.

> 
> Best regards,
> Tomasz
> 
> 
> 


-- 
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