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

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

[Daniel Lezcano]

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(+)

diff --git a/Documentation/devicetree/bindings/arm/idle-states.txt b/Documentation/devicetree/bindings/arm/idle-states.txt
index 771f5d20ae18..702a19b6b0b2 100644
--- a/Documentation/devicetree/bindings/arm/idle-states.txt
+++ b/Documentation/devicetree/bindings/arm/idle-states.txt
@@ -346,6 +346,17 @@ follows:
 	idle-states node. Please refer to the entry-method bindings
 	documentation for properties definitions.
 
+	When the idle state is used as a cooling device, the #cooling-cells
+	must be added in order to allow registering it.
+
+	- #cooling-cells:
+		Usage: Optional
+		Type: unsigned
+		Size: one cell
+		Definition: A cooling device specific information. At least
+			    must be two. Refer to the thermal DT bindings for
+			    more details.
+
 ===========================================
 4 - Examples
 ===========================================
-- 
2.17.1

[PATCH 2/2] thermal: cpuidle: Register cpuidle cooling device

[Daniel Lezcano]

The cpuidle driver can be used as a cooling device by injecting idle
cycles. The DT binding for the idle state added an optional

When the property is set, register the cpuidle driver with the idle
state node pointer as a cooling device. The thermal framework will do
the association automatically with the thermal zone via the
cooling-device defined in the device tree cooling-maps section.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/dt_idle_states.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c
index d06d21a9525d..34bd65197342 100644
--- a/drivers/cpuidle/dt_idle_states.c
+++ b/drivers/cpuidle/dt_idle_states.c
@@ -8,6 +8,7 @@
 
 #define pr_fmt(fmt) "DT idle-states: " fmt
 
+#include <linux/cpu_cooling.h>
 #include <linux/cpuidle.h>
 #include <linux/cpumask.h>
 #include <linux/errno.h>
@@ -205,6 +206,13 @@ int dt_init_idle_driver(struct cpuidle_driver *drv,
 			err = -EINVAL;
 			break;
 		}
+
+		if (of_find_property(state_node, "#cooling-cells", NULL)) {
+			err = cpuidle_of_cooling_register(state_node, drv);
+			if (err)
+				break;
+		}
+
 		of_node_put(state_node);
 	}
 
-- 
2.17.1

Re: [PATCH 2/2] thermal: cpuidle: Register cpuidle cooling device

[Matthias Kaehlcke]

Hi Daniel,

On Thu, Dec 19, 2019 at 11:19:28PM +0100, Daniel Lezcano wrote:
> The cpuidle driver can be used as a cooling device by injecting idle
> cycles. The DT binding for the idle state added an optional
> 
> When the property is set, register the cpuidle driver with the idle
> state node pointer as a cooling device. The thermal framework will do
> the association automatically with the thermal zone via the
> cooling-device defined in the device tree cooling-maps section.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/cpuidle/dt_idle_states.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c
> index d06d21a9525d..34bd65197342 100644
> --- a/drivers/cpuidle/dt_idle_states.c
> +++ b/drivers/cpuidle/dt_idle_states.c
> @@ -8,6 +8,7 @@
>  
>  #define pr_fmt(fmt) "DT idle-states: " fmt
>  
> +#include <linux/cpu_cooling.h>
>  #include <linux/cpuidle.h>
>  #include <linux/cpumask.h>
>  #include <linux/errno.h>
> @@ -205,6 +206,13 @@ int dt_init_idle_driver(struct cpuidle_driver *drv,
>  			err = -EINVAL;
>  			break;
>  		}
> +
> +		if (of_find_property(state_node, "#cooling-cells", NULL)) {
> +			err = cpuidle_of_cooling_register(state_node, drv);

cpuidle_of_cooling_register() returns a struct thermal_cooling_device *,
so you probably want to use PTR_ERR() here.

Could it be a problem that the cooling device isn't unregistered even when all
associated cores are taken offline?

Re: [PATCH 2/2] thermal: cpuidle: Register cpuidle cooling device

[Daniel Lezcano]

On 19/12/2019 23:51, Matthias Kaehlcke wrote:
> Hi Daniel,
> 
> On Thu, Dec 19, 2019 at 11:19:28PM +0100, Daniel Lezcano wrote:
>> The cpuidle driver can be used as a cooling device by injecting idle
>> cycles. The DT binding for the idle state added an optional
>>
>> When the property is set, register the cpuidle driver with the idle
>> state node pointer as a cooling device. The thermal framework will do
>> the association automatically with the thermal zone via the
>> cooling-device defined in the device tree cooling-maps section.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  drivers/cpuidle/dt_idle_states.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c
>> index d06d21a9525d..34bd65197342 100644
>> --- a/drivers/cpuidle/dt_idle_states.c
>> +++ b/drivers/cpuidle/dt_idle_states.c
>> @@ -8,6 +8,7 @@
>>  
>>  #define pr_fmt(fmt) "DT idle-states: " fmt
>>  
>> +#include <linux/cpu_cooling.h>
>>  #include <linux/cpuidle.h>
>>  #include <linux/cpumask.h>
>>  #include <linux/errno.h>
>> @@ -205,6 +206,13 @@ int dt_init_idle_driver(struct cpuidle_driver *drv,
>>  			err = -EINVAL;
>>  			break;
>>  		}
>> +
>> +		if (of_find_property(state_node, "#cooling-cells", NULL)) {
>> +			err = cpuidle_of_cooling_register(state_node, drv);
> 
> cpuidle_of_cooling_register() returns a struct thermal_cooling_device *,
> so you probably want to use PTR_ERR() here.

Right, I'm about the send the V6 which returns an int.

> Could it be a problem that the cooling device isn't unregistered even when all
> associated cores are taken offline?

The cooling device relies on the powercap/idle_inject.c which is based
on the smpboot API. This one takes care of parking the per cpu pinned
tasks. So AFAICS, it is fine.

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

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

[Rob Herring]

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.

Rob

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

[Daniel Lezcano]

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?

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

[Rob Herring]

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? It's
really only 1 device.

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

Rob

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

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

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

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

[Rob Herring]

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