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

[Rob Herring <robh@kernel.org>]

On Mon, Jan 13, 2020 at 11:52 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 13/01/2020 17:16, Rob Herring wrote:
> > On Sat, Jan 11, 2020 at 11:32 AM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> Hi Rob,
> >>
> >>
> >> On Wed, 8 Jan 2020 at 15:03, Rob Herring <robh@kernel.org> 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@linaro.org>
> >>>> ---
> >>>>  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.

So why can't you have:

cooling-device = <&cpu0 DVFS>;

cooling-device = <&cpu0 IDLE>;

(any additional cells omitted for simplicity)

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

What I meant was a phandle in the cooling cells, so #cooling-cells == 3:

cooling-device = <&cpu0 0 0 &cpu_idle_state>, <&cpu1 0 0 &cpu_idle_state>;

Phandle args being a phandle is a bit unusual, but certainly possible.

Rob