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

[Daniel Lezcano]

Hi Rob,

a gentle ping on the questions below.


On 13/01/2020 18:52, Daniel Lezcano wrote:
> 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