Re: [PATCH 1/2] DT: bindings: Add cooling cells for idle states

[Daniel Lezcano]

On 13/01/2020 17:16, Rob Herring wrote:
> On Sat, Jan 11, 2020 at 11:32 AM Daniel Lezcano
> <daniel.lezcano@xxxxxxxxxx> wrote:
>>
>> Hi Rob,
>>
>>
>> On Wed, 8 Jan 2020 at 15:03, Rob Herring <robh@xxxxxxxxxx> wrote:
>>>
>>> On Thu, Dec 19, 2019 at 11:19:27PM +0100, Daniel Lezcano wrote:
>>>> Add DT documentation to add an idle state as a cooling device. The CPU
>>>> is actually the cooling device but the definition is already used by
>>>> frequency capping. As we need to make cpufreq capping and idle
>>>> injection to co-exist together on the system in order to mitigate at
>>>> different trip points, the CPU can not be used as the cooling device
>>>> for idle injection. The idle state can be seen as an hardware feature
>>>> and therefore as a component for the passive mitigation.
>>>>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
>>>> ---
>>>>  Documentation/devicetree/bindings/arm/idle-states.txt | 11 +++++++++++
>>>>  1 file changed, 11 insertions(+)
>>>
>>> This is now a schema in my tree. Can you rebase on that and I'll pick up
>>> the binding change.
>>
>> Mmh, I'm now having some doubts about this binding because it will
>> restrict any improvement of the cooling device for the future.
>>
>> It looks like adding a node to the CPU for the cooling device is more
>> adequate.
>> eg:
>> CPU0: cpu@300 {
>>    device_type = "cpu";
>>    compatible = "arm,cortex-a9";
>>    reg = <0x300>;
>>    /* cpufreq controls */
>>    operating-points = <998400 0
>>           800000 0
>>           400000 0
>>           200000 0>;
>>    clocks = <&prcmu_clk PRCMU_ARMSS>;
>>    clock-names = "cpu";
>>    clock-latency = <20000>;
>>    #cooling-cells = <2>;
>>    thermal-idle {
>>       #cooling-cells = <2>;
>>    };
>> };
>>
>> [ ... ]
>>
>> cooling-device = <&{/cpus/cpu@300/thermal-idle}
>>                         THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
>>
>> A quick test with different configurations combination shows it is much
>> more flexible and it is open for future changes.
>>
>> What do you think?
> 
> Why do you need #cooling-cells in both cpu node and a child node?

The cooling-cells in the CPU node is for the cpufreq cooling device and
the one in the thermal-idle is for the idle cooling device. The first
one is for backward compatibility. If no cpufreq cooling device exists
then the first cooling-cells is not needed. May be we can define
"thermal-dvfs" at the same time, so we do the change for both and
prevent mixing the old and new bindings?

> It's really only 1 device.

The main problem is how the thermal framework is designed. When we
register a cooling device we pass the node pointer and the core
framework checks it has a #cooling-cells. Then cooling-maps must have a
phandle to the node we registered before as a cooling device. This is
when the thermal-zone <-> cooling device association is done.

With the cpufreq cooling device, the "CPU slot" is now used and we can't
point to it without ambiguity as we can have different cooling device
strategies for the same CPU at different temperatures.

Is it acceptable the following?

CPU0: cpu@300 {
   [ ... ]
   thermal-idle {
      #cooling-cells = <2>;
   };

   thermal-dvfs {
      #cooling-cells = <2>;
   }
};

Or alternatively, can we define a passive-cooling node?

thermal-cooling: passive0 {
   #cooling-cells = <2>;
   strategy="dvfs" | "idle"
   cooling-device=<&CPU0>
};

cooling-device = <&passive0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;

> Maybe you could add another cell to contain an idle state node if that
helps?

(Assuming you are referring to a phandle to an idle state) The idle
states are grouped per cluster because the CPUs belonging to the same
cluster have the same idle states characteristics. Because of that, the
phandle will point to the same node and it will be impossible to specify
a per cpu cooling device, only per cluster.



-- 
 <http://www.linaro.org/> Linaro.org â Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog