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@0.85V)
>>>>>> + * 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@0x48 {
>>>>>> + ...
>>>>>> + 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
Attachment:
signature.asc
Description: OpenPGP digital signature
