* [PATCH 1/2] DT: bindings: Add cooling cells for idle states @ 2019-12-19 22:19 Daniel Lezcano 2019-12-19 22:19 ` [PATCH 2/2] thermal: cpuidle: Register cpuidle cooling device Daniel Lezcano 2020-01-08 14:03 ` [PATCH 1/2] DT: bindings: Add cooling cells for idle states Rob Herring 0 siblings, 2 replies; 10+ messages in thread From: Daniel Lezcano @ 2019-12-19 22:19 UTC (permalink / raw) To: daniel.lezcano Cc: Rob Herring, Mark Rutland, Amit Kucheria, Geert Uytterhoeven, Mauro Carvalho Chehab, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] thermal: cpuidle: Register cpuidle cooling device 2019-12-19 22:19 [PATCH 1/2] DT: bindings: Add cooling cells for idle states Daniel Lezcano @ 2019-12-19 22:19 ` Daniel Lezcano 2019-12-19 22:51 ` Matthias Kaehlcke 2020-01-08 14:03 ` [PATCH 1/2] DT: bindings: Add cooling cells for idle states Rob Herring 1 sibling, 1 reply; 10+ messages in thread From: Daniel Lezcano @ 2019-12-19 22:19 UTC (permalink / raw) To: daniel.lezcano Cc: Rafael J. Wysocki, open list:CPU IDLE TIME MANAGEMENT FRAMEWORK, open list 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] thermal: cpuidle: Register cpuidle cooling device 2019-12-19 22:19 ` [PATCH 2/2] thermal: cpuidle: Register cpuidle cooling device Daniel Lezcano @ 2019-12-19 22:51 ` Matthias Kaehlcke 2019-12-19 22:57 ` Daniel Lezcano 0 siblings, 1 reply; 10+ messages in thread From: Matthias Kaehlcke @ 2019-12-19 22:51 UTC (permalink / raw) To: Daniel Lezcano Cc: Rafael J. Wysocki, open list:CPU IDLE TIME MANAGEMENT FRAMEWORK, open list 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? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] thermal: cpuidle: Register cpuidle cooling device 2019-12-19 22:51 ` Matthias Kaehlcke @ 2019-12-19 22:57 ` Daniel Lezcano 0 siblings, 0 replies; 10+ messages in thread From: Daniel Lezcano @ 2019-12-19 22:57 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Rafael J. Wysocki, open list:CPU IDLE TIME MANAGEMENT FRAMEWORK, open list 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] DT: bindings: Add cooling cells for idle states 2019-12-19 22:19 [PATCH 1/2] DT: bindings: Add cooling cells for idle states Daniel Lezcano 2019-12-19 22:19 ` [PATCH 2/2] thermal: cpuidle: Register cpuidle cooling device Daniel Lezcano @ 2020-01-08 14:03 ` Rob Herring 2020-01-11 17:32 ` Daniel Lezcano 1 sibling, 1 reply; 10+ messages in thread From: Rob Herring @ 2020-01-08 14:03 UTC (permalink / raw) To: Daniel Lezcano Cc: Mark Rutland, Amit Kucheria, Geert Uytterhoeven, Mauro Carvalho Chehab, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] DT: bindings: Add cooling cells for idle states 2020-01-08 14:03 ` [PATCH 1/2] DT: bindings: Add cooling cells for idle states Rob Herring @ 2020-01-11 17:32 ` Daniel Lezcano 2020-01-13 16:16 ` Rob Herring 0 siblings, 1 reply; 10+ messages in thread From: Daniel Lezcano @ 2020-01-11 17:32 UTC (permalink / raw) To: Rob Herring Cc: Mark Rutland, Amit Kucheria, Geert Uytterhoeven, Mauro Carvalho Chehab, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list 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? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] DT: bindings: Add cooling cells for idle states 2020-01-11 17:32 ` Daniel Lezcano @ 2020-01-13 16:16 ` Rob Herring 2020-01-13 17:52 ` Daniel Lezcano 0 siblings, 1 reply; 10+ messages in thread From: Rob Herring @ 2020-01-13 16:16 UTC (permalink / raw) To: Daniel Lezcano Cc: Mark Rutland, Amit Kucheria, Geert Uytterhoeven, Mauro Carvalho Chehab, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] DT: bindings: Add cooling cells for idle states 2020-01-13 16:16 ` Rob Herring @ 2020-01-13 17:52 ` Daniel Lezcano 2020-01-27 18:29 ` Daniel Lezcano 2020-01-28 0:21 ` Rob Herring 0 siblings, 2 replies; 10+ messages in thread From: Daniel Lezcano @ 2020-01-13 17:52 UTC (permalink / raw) To: Rob Herring Cc: Mark Rutland, Amit Kucheria, Geert Uytterhoeven, Mauro Carvalho Chehab, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] DT: bindings: Add cooling cells for idle states 2020-01-13 17:52 ` Daniel Lezcano @ 2020-01-27 18:29 ` Daniel Lezcano 2020-01-28 0:21 ` Rob Herring 1 sibling, 0 replies; 10+ messages in thread From: Daniel Lezcano @ 2020-01-27 18:29 UTC (permalink / raw) To: Rob Herring Cc: Mark Rutland, Amit Kucheria, Geert Uytterhoeven, Mauro Carvalho Chehab, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] DT: bindings: Add cooling cells for idle states 2020-01-13 17:52 ` Daniel Lezcano 2020-01-27 18:29 ` Daniel Lezcano @ 2020-01-28 0:21 ` Rob Herring 1 sibling, 0 replies; 10+ messages in thread From: Rob Herring @ 2020-01-28 0:21 UTC (permalink / raw) To: Daniel Lezcano Cc: Mark Rutland, Amit Kucheria, Geert Uytterhoeven, Mauro Carvalho Chehab, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-01-28 0:22 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-19 22:19 [PATCH 1/2] DT: bindings: Add cooling cells for idle states Daniel Lezcano 2019-12-19 22:19 ` [PATCH 2/2] thermal: cpuidle: Register cpuidle cooling device Daniel Lezcano 2019-12-19 22:51 ` Matthias Kaehlcke 2019-12-19 22:57 ` Daniel Lezcano 2020-01-08 14:03 ` [PATCH 1/2] DT: bindings: Add cooling cells for idle states Rob Herring 2020-01-11 17:32 ` Daniel Lezcano 2020-01-13 16:16 ` Rob Herring 2020-01-13 17:52 ` Daniel Lezcano 2020-01-27 18:29 ` Daniel Lezcano 2020-01-28 0:21 ` Rob Herring
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).