[PATCH v3 02/15] dt/bindings: Update binding for PM domain idle states

[PATCH v3 02/15] dt/bindings: Update binding for PM domain idle states

[Lina Iyer]

From: Axel Haslam <ahaslam+renesas@baylibre.com>

Update DT bindings to describe idle states of PM domains.

Cc: <devicetree@vger.kernel.org>
Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
[Lina: Added state properties, removed state names, wakeup-latency,
added of_pm_genpd_init() API, pruned commit text]
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
[Ulf: Moved around code to make it compile properly, rebased on top of multiple state support]
---
 .../devicetree/bindings/power/power_domain.txt     | 57 ++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt
index 025b5e7..4960486 100644
--- a/Documentation/devicetree/bindings/power/power_domain.txt
+++ b/Documentation/devicetree/bindings/power/power_domain.txt
@@ -29,6 +29,10 @@ Optional properties:
    specified by this binding. More details about power domain specifier are
    available in the next section.
 
+- domain-idle-states : A phandle of an idle-state that shall be soaked into a
+                generic domain power state. The idle state definitions are
+                compatible with arm,idle-state specified in [1].
+
 Example:
 
 	power: power-controller@12340000 {
@@ -59,6 +63,57 @@ The nodes above define two power controllers: 'parent' and 'child'.
 Domains created by the 'child' power controller are subdomains of '0' power
 domain provided by the 'parent' power controller.
 
+Example 3: ARM v7 style CPU PM domains (Linux domain controller)
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		CPU0: cpu@0 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a7", "arm,armv7";
+			reg = <0x0>;
+			power-domains = <&a7_pd>;
+		};
+
+		CPU1: cpu@1 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15", "arm,armv7";
+			reg = <0x0>;
+			power-domains = <&a15_pd>;
+		};
+	};
+
+	pm-domains {
+		a15_pd: a15_pd {
+			/* will have A15 platform ARM_PD_METHOD_OF_DECLARE*/
+			compatible = "arm,cortex-a15";
+			#power-domain-cells = <0>;
+			domain-idle-states = <&CLUSTER_SLEEP_0>;
+		};
+
+		a7_pd: a7_pd {
+			/* will have a A7 platform ARM_PD_METHOD_OF_DECLARE*/
+			compatible = "arm,cortex-a7";
+			#power-domain-cells = <0>;
+			domain-idle-states = <&CLUSTER_SLEEP_0>, <&CLUSTER_SLEEP_1>;
+		};
+
+		CLUSTER_SLEEP_0: state0 {
+			compatible = "arm,idle-state";
+			entry-latency-us = <1000>;
+			exit-latency-us = <2000>;
+			min-residency-us = <10000>;
+		};
+
+		CLUSTER_SLEEP_1: state1 {
+			compatible = "arm,idle-state";
+			entry-latency-us = <5000>;
+			exit-latency-us = <5000>;
+			min-residency-us = <100000>;
+		};
+	};
+
 ==PM domain consumers==
 
 Required properties:
@@ -76,3 +131,5 @@ Example:
 The node above defines a typical PM domain consumer device, which is located
 inside a PM domain with index 0 of a power controller represented by a node
 with the label "power".
+
+[1]. Documentation/devicetree/bindings/arm/idle-states.txt
-- 
2.7.4

Re: [PATCH v3 02/15] dt/bindings: Update binding for PM domain idle states

[Rob Herring]

On Thu, Aug 04, 2016 at 05:04:49PM -0600, Lina Iyer wrote:
> From: Axel Haslam <ahaslam+renesas@baylibre.com>
> 
> Update DT bindings to describe idle states of PM domains.
> 
> Cc: <devicetree@vger.kernel.org>
> Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> [Lina: Added state properties, removed state names, wakeup-latency,
> added of_pm_genpd_init() API, pruned commit text]
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> [Ulf: Moved around code to make it compile properly, rebased on top of multiple state support]

This change doesn't make sense given this is just a binding doc.

> ---
>  .../devicetree/bindings/power/power_domain.txt     | 57 ++++++++++++++++++++++
>  1 file changed, 57 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 02/15] dt/bindings: Update binding for PM domain idle states

[Sudeep Holla]

Hi Lina,

I have few concerns mainly due to the lack of description and not the 
binding per say.

On 05/08/16 00:04, Lina Iyer wrote:
> From: Axel Haslam <ahaslam+renesas@baylibre.com>
>
> Update DT bindings to describe idle states of PM domains.
>
> Cc: <devicetree@vger.kernel.org>
> Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> [Lina: Added state properties, removed state names, wakeup-latency,
> added of_pm_genpd_init() API, pruned commit text]
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> [Ulf: Moved around code to make it compile properly, rebased on top of multiple state support]
> ---
>  .../devicetree/bindings/power/power_domain.txt     | 57 ++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt
> index 025b5e7..4960486 100644
> --- a/Documentation/devicetree/bindings/power/power_domain.txt
> +++ b/Documentation/devicetree/bindings/power/power_domain.txt
> @@ -29,6 +29,10 @@ Optional properties:
>     specified by this binding. More details about power domain specifier are
>     available in the next section.
>
> +- domain-idle-states : A phandle of an idle-state that shall be soaked into a
> +                generic domain power state. The idle state definitions are
> +                compatible with arm,idle-state specified in [1].
> +

So I assume these can be used for the genpd states. Either we rename it 
domain-power-states or make it clear that these domain-idle-states can 
also represent the power-states for normal devices.

>  Example:
>
>  	power: power-controller@12340000 {
> @@ -59,6 +63,57 @@ The nodes above define two power controllers: 'parent' and 'child'.
>  Domains created by the 'child' power controller are subdomains of '0' power
>  domain provided by the 'parent' power controller.
>
> +Example 3: ARM v7 style CPU PM domains (Linux domain controller)
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		CPU0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a7", "arm,armv7";
> +			reg = <0x0>;
> +			power-domains = <&a7_pd>;

This example doesn't consider how do we deal with the presence off
cpu-idle-states property in CPU nodes.

IMO we need move even the cpu/core level idle states into its own power
domain. It also helps to solve other usecases like PMU, debug/coresight
devices attached to the core power domain(in most of the cases) while
they may be in separate PD like PMUs on OMAP. That will help OS whether
to save/restore the states on idle-entry.

In [PATCH v3 15/15] ARM64: dts: Define CPU power domain for MSM8916, the
idle-states are split across the cpu cpu-idle-states and pd
domain-idle-states property. That looks like a really mess to me.

We need to have all the idle state information at one place and in this
case PD seems more appropriate instead of splitting them across.

We can also keep the code clean and not break compatibility. Whenever
both PD and CPU contains idle-states, PD must take precedence.

Also these needs to be documented clearly in the binding.

> +		};
> +
> +		CPU1: cpu@1 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a15", "arm,armv7";
> +			reg = <0x0>;
> +			power-domains = <&a15_pd>;
> +		};
> +	};
> +
> +	pm-domains {
> +		a15_pd: a15_pd {
> +			/* will have A15 platform ARM_PD_METHOD_OF_DECLARE*/

the above comment make no sense in the binding document, remove it

-- 
Regards,
Sudeep

Re: [PATCH v3 02/15] dt/bindings: Update binding for PM domain idle states

[Lina Iyer]

Hi Sudeep,

On Wed, Aug 10 2016 at 09:15 -0600, Sudeep Holla wrote:
>Hi Lina,
>
>I have few concerns mainly due to the lack of description and not the 
>binding per say.
>
>On 05/08/16 00:04, Lina Iyer wrote:
>>From: Axel Haslam <ahaslam+renesas@baylibre.com>
>>
>>Update DT bindings to describe idle states of PM domains.
>>
>>Cc: <devicetree@vger.kernel.org>
>>Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com>
>>Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>[Lina: Added state properties, removed state names, wakeup-latency,
>>added of_pm_genpd_init() API, pruned commit text]
>>Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>[Ulf: Moved around code to make it compile properly, rebased on top of multiple state support]
>>---
>> .../devicetree/bindings/power/power_domain.txt     | 57 ++++++++++++++++++++++
>> 1 file changed, 57 insertions(+)
>>
>>diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt
>>index 025b5e7..4960486 100644
>>--- a/Documentation/devicetree/bindings/power/power_domain.txt
>>+++ b/Documentation/devicetree/bindings/power/power_domain.txt
>>@@ -29,6 +29,10 @@ Optional properties:
>>    specified by this binding. More details about power domain specifier are
>>    available in the next section.
>>
>>+- domain-idle-states : A phandle of an idle-state that shall be soaked into a
>>+                generic domain power state. The idle state definitions are
>>+                compatible with arm,idle-state specified in [1].
>>+
>
>So I assume these can be used for the genpd states. Either we rename 
>it domain-power-states or make it clear that these domain-idle-states 
>can also represent the power-states for normal devices.
>
These are the domains' idle states. These states are only used when the
domain goes into idle, not when the domain is active. These are not
power states that the domain can operate on either. Hence the idle-state
moniker.
Also, the bindings to describe the state are the same as arm,idle-state.
It made sense to call these domain idle states instead of
domain-power-states.

>> Example:
>>
>> 	power: power-controller@12340000 {
>>@@ -59,6 +63,57 @@ The nodes above define two power controllers: 'parent' and 'child'.
>> Domains created by the 'child' power controller are subdomains of '0' power
>> domain provided by the 'parent' power controller.
>>
>>+Example 3: ARM v7 style CPU PM domains (Linux domain controller)
>>+
>>+	cpus {
>>+		#address-cells = <1>;
>>+		#size-cells = <0>;
>>+
>>+		CPU0: cpu@0 {
>>+			device_type = "cpu";
>>+			compatible = "arm,cortex-a7", "arm,armv7";
>>+			reg = <0x0>;
>>+			power-domains = <&a7_pd>;
>
>This example doesn't consider how do we deal with the presence off
>cpu-idle-states property in CPU nodes.
>
I can amend the example. But to answer your underlying question, they
will exist as separate properties.

>IMO we need move even the cpu/core level idle states into its own power
>domain. It also helps to solve other usecases like PMU, debug/coresight
>devices attached to the core power domain(in most of the cases) while
>they may be in separate PD like PMUs on OMAP. That will help OS whether
>to save/restore the states on idle-entry.
>
This idea was brought up by Kevin earlier in the discussions, but we
shelved it for a later date.

>In [PATCH v3 15/15] ARM64: dts: Define CPU power domain for MSM8916, the
>idle-states are split across the cpu cpu-idle-states and pd
>domain-idle-states property. That looks like a really mess to me.
>
It is pretty clear that CPUs cannot not define the domain idle states.
Domains define their own idle states. Just as you mention above. CPU is
just a single component in its domain. There may be other devices like
PMUs, Coresights etc that also may have a say in the idle state the
domain may be put in, when the devices are idle. As such, adding domain
idle states to the CPU's idle state property is not appropriate.

Our kernel has runtime PM for devices and then there is CPUidle, both
are diverging without one knowing about the other. We have to start
unifying them inorder to have better holistic power management in the
SoC. To that regard, we have to start imagining CPUs as just another
device, albeit a special device. But for our purposes in determining
domain idle state, it will just be a device attached to the domain.

>We need to have all the idle state information at one place and in this
>case PD seems more appropriate instead of splitting them across.
>
That approach isn't correct. Where will we put the idle states of other
devices that are also part of the domain? We are thinking about a model,
where every device defines its own idle states and we define
relationships between those idle states and their parents' idle states.
Ofcourse, devices don't have idle states today, but that is something we
have been pondering over.

>We can also keep the code clean and not break compatibility. Whenever
>both PD and CPU contains idle-states, PD must take precedence.
>
Why?
The CPU and PD states are orthogonal. While the PD state is dependent on
the CPU state, the latter is not true. Devices determine their own
states. Based on the individual device states, we then determine the
state of the parent and bubble up on the hierarchy.

>Also these needs to be documented clearly in the binding.
>
>>+		};
>>+
>>+		CPU1: cpu@1 {
>>+			device_type = "cpu";
>>+			compatible = "arm,cortex-a15", "arm,armv7";
>>+			reg = <0x0>;
>>+			power-domains = <&a15_pd>;
>>+		};
>>+	};
>>+
>>+	pm-domains {
>>+		a15_pd: a15_pd {
>>+			/* will have A15 platform ARM_PD_METHOD_OF_DECLARE*/
>
>the above comment make no sense in the binding document, remove it
>
Yup. Will remove.

Thanks,
Lina

Re: [PATCH v3 02/15] dt/bindings: Update binding for PM domain idle states

[Sudeep Holla]

On 10/08/16 17:40, Lina Iyer wrote:
> Hi Sudeep,
>
> On Wed, Aug 10 2016 at 09:15 -0600, Sudeep Holla wrote:
>> Hi Lina,
>>
>> I have few concerns mainly due to the lack of description and not the
>> binding per say.

[...]

>>> +- domain-idle-states : A phandle of an idle-state that shall be
>>> soaked into a
>>> +                generic domain power state. The idle state
>>> definitions are
>>> +                compatible with arm,idle-state specified in [1].
>>> +
>>
>> So I assume these can be used for the genpd states. Either we rename
>> it domain-power-states or make it clear that these domain-idle-states
>> can also represent the power-states for normal devices.
>>
> These are the domains' idle states. These states are only used when the
> domain goes into idle, not when the domain is active. These are not
> power states that the domain can operate on either. Hence the idle-state
> moniker.

I am not sure if we can tell that the device is running in all it's
power states. E.g. in ACPI IIUC, only D0 state represent running state,
while D{1,2,3} are power states which consume less power than D0/running
state. I think genpd is designed on those lines.

So I was thinking if these idle-states can also if use from non-CPU
devices w.r.t binding, it will serve as D-state equivalent in ACPI

> Also, the bindings to describe the state are the same as arm,idle-state.
> It made sense to call these domain idle states instead of
> domain-power-states.
>

I am fine with that, but we have idle states compatible to distinguish 
it from normal device idle/power states.

>>> Example:
>>>
>>>     power: power-controller@12340000 {
>>> @@ -59,6 +63,57 @@ The nodes above define two power controllers:
>>> 'parent' and 'child'.
>>> Domains created by the 'child' power controller are subdomains of '0'
>>> power
>>> domain provided by the 'parent' power controller.
>>>
>>> +Example 3: ARM v7 style CPU PM domains (Linux domain controller)
>>> +
>>> +    cpus {
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +
>>> +        CPU0: cpu@0 {
>>> +            device_type = "cpu";
>>> +            compatible = "arm,cortex-a7", "arm,armv7";
>>> +            reg = <0x0>;
>>> +            power-domains = <&a7_pd>;
>>
>> This example doesn't consider how do we deal with the presence off
>> cpu-idle-states property in CPU nodes.
>>
> I can amend the example. But to answer your underlying question, they
> will exist as separate properties.
>

Which is asking for trouble IMO. One should take precedence over other
if both are present. Ideally new DTs can just have PD, we will continue
to support cpu-idle-states for old DT.

>> IMO we need move even the cpu/core level idle states into its own power
>> domain. It also helps to solve other usecases like PMU, debug/coresight
>> devices attached to the core power domain(in most of the cases) while
>> they may be in separate PD like PMUs on OMAP. That will help OS whether
>> to save/restore the states on idle-entry.
>>
> This idea was brought up by Kevin earlier in the discussions, but we
> shelved it for a later date.
>

Any particular reasons ? I will try to dig up. I would do in one shot
especially with respect to bindings. Implementation wise, it's fine we
can take up in stages. I don't see any issue adding it in first go. This
binding is good, you just make it hierarchical and add more description.

>> In [PATCH v3 15/15] ARM64: dts: Define CPU power domain for MSM8916, the
>> idle-states are split across the cpu cpu-idle-states and pd
>> domain-idle-states property. That looks like a really mess to me.
>>
> It is pretty clear that CPUs cannot not define the domain idle states.
> Domains define their own idle states. Just as you mention above. CPU is
> just a single component in its domain. There may be other devices like
> PMUs, Coresights etc that also may have a say in the idle state the
> domain may be put in, when the devices are idle. As such, adding domain
> idle states to the CPU's idle state property is not appropriate.
>

No I am not saying we need to add domain idle states to the CPU's idle
state property. I am saying we need to remove cpu-idle-states or ignore
it when PD is present. And get all the idle state information for PD.

I am objecting the split we are creating across CPU and higher level
power domains. And this binding document is incomplete as it skips all
those details. We just need PD handle in CPU and no idle state
information there. Create PD hierarchy and have all idle state
information at one place.

> Our kernel has runtime PM for devices and then there is CPUidle, both
> are diverging without one knowing about the other. We have to start
> unifying them inorder to have better holistic power management in the
> SoC. To that regard, we have to start imagining CPUs as just another
> device, albeit a special device. But for our purposes in determining
> domain idle state, it will just be a device attached to the domain.
>

Absolutely agree on that. No arguments. I am asking to go a step ahead
to include even cpu/core level power domains not just cluster/higher
level domains.

>> We need to have all the idle state information at one place and in this
>> case PD seems more appropriate instead of splitting them across.
>>
> That approach isn't correct. Where will we put the idle states of other
> devices that are also part of the domain? We are thinking about a model,
> where every device defines its own idle states and we define
> relationships between those idle states and their parents' idle states.

Yes I understand. You confused me here. Won't that be one-to-one 
relationship ? If not, how is that dealt in the current bindings ?

> Ofcourse, devices don't have idle states today, but that is something we
> have been pondering over.
>

Yes we these binding should be easily extensible, I don't see any issue.

>> We can also keep the code clean and not break compatibility. Whenever
>> both PD and CPU contains idle-states, PD must take precedence.
>>
> Why?
> The CPU and PD states are orthogonal. While the PD state is dependent on
> the CPU state, the latter is not true. Devices determine their own
> states. Based on the individual device states, we then determine the
> state of the parent and bubble up on the hierarchy.
>

I may be missing something. Now with your example in the binding, if
another device shares the cluster PD, can it have different idle states?
If so how does it map ?


In general whatever binding we come up must not just address OS 
coordinated mode. Also I was thinking to have better coverage in the 
description by having a bit more complex system like:

cluster0
	CLUSTER_RET(Retention)
	CLUSTER_PG(Power Gate)
	core0
		CORE_RET
		CORE_PG
	core1
		CORE_RET
		CORE_PG

cluster1
	CLUSTER_RET
	CLUSTER_PG
	core0
		CORE_RET
		CORE_PG
	core1
		CORE_RET
		CORE_PG

Platform Co-ordinate supports the following states and we should be able 
to determine that from the binding:

CORE_RET
CORE_PG
CORE_RET + CLUSTER_RET
CORE_PG + CLUSTER_RET
CORE_PG + CLUSTER_PG


-- 
Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 02/15] dt/bindings: Update binding for PM domain idle states

[Sudeep Holla]

On 10/08/16 19:09, Sudeep Holla wrote:
>
>
> On 10/08/16 17:40, Lina Iyer wrote:
>> Hi Sudeep,
>>
>> On Wed, Aug 10 2016 at 09:15 -0600, Sudeep Holla wrote:
>>> Hi Lina,
>>>
>>> I have few concerns mainly due to the lack of description and not the
>>> binding per say.
>
> [...]
>
>>>> +- domain-idle-states : A phandle of an idle-state that shall be
>>>> soaked into a
>>>> +                generic domain power state. The idle state
>>>> definitions are
>>>> +                compatible with arm,idle-state specified in [1].
>>>> +
>>>
>>> So I assume these can be used for the genpd states. Either we rename
>>> it domain-power-states or make it clear that these domain-idle-states
>>> can also represent the power-states for normal devices.
>>>
>> These are the domains' idle states. These states are only used when the
>> domain goes into idle, not when the domain is active. These are not
>> power states that the domain can operate on either. Hence the idle-state
>> moniker.
>
> I am not sure if we can tell that the device is running in all it's
> power states. E.g. in ACPI IIUC, only D0 state represent running state,
> while D{1,2,3} are power states which consume less power than D0/running
> state. I think genpd is designed on those lines.
>

I didn't complete this section earlier.

Just to clarify we can label them as idle states or whatever. Strictly
speaking today even cpu-idle-states have power off state, so the term is
not important.

What I intended to say is that it should be able to describe even device
idle(power states minus the active running) states.

-- 
Regards,
Sudeep

Re: [PATCH v3 02/15] dt/bindings: Update binding for PM domain idle states

[Lina Iyer]

On Wed, Aug 10 2016 at 12:09 -0600, Sudeep Holla wrote:
>
>
>On 10/08/16 17:40, Lina Iyer wrote:
>>Hi Sudeep,
>>
>>On Wed, Aug 10 2016 at 09:15 -0600, Sudeep Holla wrote:
>>>Hi Lina,
>>>
>>>I have few concerns mainly due to the lack of description and not the
>>>binding per say.
>
>[...]
>
>>It is pretty clear that CPUs cannot not define the domain idle states.
>>Domains define their own idle states. Just as you mention above. CPU is
>>just a single component in its domain. There may be other devices like
>>PMUs, Coresights etc that also may have a say in the idle state the
>>domain may be put in, when the devices are idle. As such, adding domain
>>idle states to the CPU's idle state property is not appropriate.
>>
>
>No I am not saying we need to add domain idle states to the CPU's idle
>state property. I am saying we need to remove cpu-idle-states or ignore
>it when PD is present. And get all the idle state information for PD.
>
>I am objecting the split we are creating across CPU and higher level
>power domains. And this binding document is incomplete as it skips all
>those details. We just need PD handle in CPU and no idle state
>information there. Create PD hierarchy and have all idle state
>information at one place.
>
Let me think about this a bit and see what I can come up with.

>>Our kernel has runtime PM for devices and then there is CPUidle, both
>>are diverging without one knowing about the other. We have to start
>>unifying them inorder to have better holistic power management in the
>>SoC. To that regard, we have to start imagining CPUs as just another
>>device, albeit a special device. But for our purposes in determining
>>domain idle state, it will just be a device attached to the domain.
>>
>
>Absolutely agree on that. No arguments. I am asking to go a step ahead
>to include even cpu/core level power domains not just cluster/higher
>level domains.
>
>>>We need to have all the idle state information at one place and in this
>>>case PD seems more appropriate instead of splitting them across.
>>>
>>That approach isn't correct. Where will we put the idle states of other
>>devices that are also part of the domain? We are thinking about a model,
>>where every device defines its own idle states and we define
>>relationships between those idle states and their parents' idle states.
>
>Yes I understand. You confused me here. Won't that be one-to-one 
>relationship ? If not, how is that dealt in the current bindings ?
>
>>Ofcourse, devices don't have idle states today, but that is something we
>>have been pondering over.
>>
>
>Yes we these binding should be easily extensible, I don't see any issue.
>
>>>We can also keep the code clean and not break compatibility. Whenever
>>>both PD and CPU contains idle-states, PD must take precedence.
>>>
>>Why?
>>The CPU and PD states are orthogonal. While the PD state is dependent on
>>the CPU state, the latter is not true. Devices determine their own
>>states. Based on the individual device states, we then determine the
>>state of the parent and bubble up on the hierarchy.
>>
>
>I may be missing something. Now with your example in the binding, if
>another device shares the cluster PD, can it have different idle states?
>If so how does it map ?
>
>
>In general whatever binding we come up must not just address OS 
>coordinated mode. Also I was thinking to have better coverage in the 
>description by having a bit more complex system like:
>
>cluster0
>	CLUSTER_RET(Retention)
>	CLUSTER_PG(Power Gate)
>	core0
>		CORE_RET
>		CORE_PG
>	core1
>		CORE_RET
>		CORE_PG
>
>cluster1
>	CLUSTER_RET
>	CLUSTER_PG
>	core0
>		CORE_RET
>		CORE_PG
>	core1
>		CORE_RET
>		CORE_PG
>
>Platform Co-ordinate supports the following states and we should be 
>able to determine that from the binding:
>
>CORE_RET
>CORE_PG
>CORE_RET + CLUSTER_RET

The problem that we have to sove here is knowing that 
CORE_RET + CLUSTER_PG (hypothetically) an invalid combination. Kevin and
I debated it in the earlier RFC and we dont have a good way to solve
this generically for all devices.

>CORE_PG + CLUSTER_RET
>CORE_PG + CLUSTER_PG
>
>
Thanks,
Lina

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 02/15] dt/bindings: Update binding for PM domain idle states

[Brendan Jackman]

Hi Lina

On 11/08/16 22:10, Lina Iyer wrote:
> On Wed, Aug 10 2016 at 12:09 -0600, Sudeep Holla wrote:
>>
>>
>> On 10/08/16 17:40, Lina Iyer wrote:
>>> Hi Sudeep,
>>>
>>> On Wed, Aug 10 2016 at 09:15 -0600, Sudeep Holla wrote:
>>>> Hi Lina,
>>>>
>>>> I have few concerns mainly due to the lack of description and not the
>>>> binding per say.
>>
>> [...]
>>
>>> It is pretty clear that CPUs cannot not define the domain idle states.
>>> Domains define their own idle states. Just as you mention above. CPU is
>>> just a single component in its domain. There may be other devices like
>>> PMUs, Coresights etc that also may have a say in the idle state the
>>> domain may be put in, when the devices are idle. As such, adding domain
>>> idle states to the CPU's idle state property is not appropriate.
>>>
>>
>> No I am not saying we need to add domain idle states to the CPU's idle
>> state property. I am saying we need to remove cpu-idle-states or ignore
>> it when PD is present. And get all the idle state information for PD.
>>
>> I am objecting the split we are creating across CPU and higher level
>> power domains. And this binding document is incomplete as it skips all
>> those details. We just need PD handle in CPU and no idle state
>> information there. Create PD hierarchy and have all idle state
>> information at one place.
>>
> Let me think about this a bit and see what I can come up with.
>
>>> Our kernel has runtime PM for devices and then there is CPUidle, both
>>> are diverging without one knowing about the other. We have to start
>>> unifying them inorder to have better holistic power management in the
>>> SoC. To that regard, we have to start imagining CPUs as just another
>>> device, albeit a special device. But for our purposes in determining
>>> domain idle state, it will just be a device attached to the domain.
>>>
>>
>> Absolutely agree on that. No arguments. I am asking to go a step ahead
>> to include even cpu/core level power domains not just cluster/higher
>> level domains.
>>
>>>> We need to have all the idle state information at one place and in this
>>>> case PD seems more appropriate instead of splitting them across.
>>>>
>>> That approach isn't correct. Where will we put the idle states of other
>>> devices that are also part of the domain? We are thinking about a model,
>>> where every device defines its own idle states and we define
>>> relationships between those idle states and their parents' idle states.
>>
>> Yes I understand. You confused me here. Won't that be one-to-one
>> relationship ? If not, how is that dealt in the current bindings ?
>>
>>> Ofcourse, devices don't have idle states today, but that is something we
>>> have been pondering over.
>>>
>>
>> Yes we these binding should be easily extensible, I don't see any issue.
>>
>>>> We can also keep the code clean and not break compatibility. Whenever
>>>> both PD and CPU contains idle-states, PD must take precedence.
>>>>
>>> Why?
>>> The CPU and PD states are orthogonal. While the PD state is dependent on
>>> the CPU state, the latter is not true. Devices determine their own
>>> states. Based on the individual device states, we then determine the
>>> state of the parent and bubble up on the hierarchy.
>>>
>>
>> I may be missing something. Now with your example in the binding, if
>> another device shares the cluster PD, can it have different idle states?
>> If so how does it map ?
>>
>>
>> In general whatever binding we come up must not just address OS
>> coordinated mode. Also I was thinking to have better coverage in the
>> description by having a bit more complex system like:
>>
>> cluster0
>>     CLUSTER_RET(Retention)
>>     CLUSTER_PG(Power Gate)
>>     core0
>>         CORE_RET
>>         CORE_PG
>>     core1
>>         CORE_RET
>>         CORE_PG
>>
>> cluster1
>>     CLUSTER_RET
>>     CLUSTER_PG
>>     core0
>>         CORE_RET
>>         CORE_PG
>>     core1
>>         CORE_RET
>>         CORE_PG
>>
>> Platform Co-ordinate supports the following states and we should be
>> able to determine that from the binding:
>>
>> CORE_RET
>> CORE_PG
>> CORE_RET + CLUSTER_RET
>
> The problem that we have to sove here is knowing that CORE_RET +
> CLUSTER_PG (hypothetically) an invalid combination. Kevin and
> I debated it in the earlier RFC and we dont have a good way to solve
> this generically for all devices.
>

This is interesting. I had been working on the assumption that a parent
power domain cannot enter any idle state until its children were all in
their deepest idle state. I now realise that it's easy to imagine
platforms where this isn't the case.

However, I don't understand how your current bindings solve this issue
and why using domain-power-states for all states (i.e. ignoring
cpu-idle-states and putting CPU idle states in the domain-idle-states of
a per-CPU power domain - I believe this is what Sudeep is suggesting)
makes it any more difficult.

Could you link to this previous discussion you mentioned? I'm having
trouble finding it (R.I.P Gmane).

>> CORE_PG + CLUSTER_RET
>> CORE_PG + CLUSTER_PG
>>
>>
> Thanks,
> Lina
>

Cheers,
Brendan
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 02/15] dt/bindings: Update binding for PM domain idle states

[Sudeep Holla]

On 11/08/16 22:10, Lina Iyer wrote:
> On Wed, Aug 10 2016 at 12:09 -0600, Sudeep Holla wrote:
>>

[...]

>> cluster0
>>     CLUSTER_RET(Retention)
>>     CLUSTER_PG(Power Gate)
>>     core0
>>         CORE_RET
>>         CORE_PG
>>     core1
>>         CORE_RET
>>         CORE_PG
>>
>> cluster1
>>     CLUSTER_RET
>>     CLUSTER_PG
>>     core0
>>         CORE_RET
>>         CORE_PG
>>     core1
>>         CORE_RET
>>         CORE_PG
>>
>> Platform Co-ordinate supports the following states and we should be
>> able to determine that from the binding:
>>
>> CORE_RET
>> CORE_PG
>> CORE_RET + CLUSTER_RET
>
> The problem that we have to sove here is knowing that CORE_RET +
> CLUSTER_PG (hypothetically) an invalid combination. Kevin and
> I debated it in the earlier RFC and we dont have a good way to solve
> this generically for all devices.
>

Yes, I agree it's complex. But that needs to be solved IMO.

I can think of 2 possible solutions:

1. Index the states(which people have not liked, but as along as we
    don't use it in the code as it for any other purpose, it should be
    fine) and then have each state mentioning what parent state can be
    entered at this child state(i.e. starting index and all states below
    it)

2. Something similar to (1) but without index instead phandles.

Again these are just thoughts, others may think of some better
solution(s). Sorry I haven't followed all the previous threads in detail.

-- 
Regards,
Sudeep

Re: [PATCH v3 02/15] dt/bindings: Update binding for PM domain idle states

[Lina Iyer]

On Fri, Aug 12 2016 at 04:08 -0600, Sudeep Holla wrote:
>
>
>On 11/08/16 22:10, Lina Iyer wrote:
>>On Wed, Aug 10 2016 at 12:09 -0600, Sudeep Holla wrote:
>>>
>
>[...]
>
>>>cluster0
>>>    CLUSTER_RET(Retention)
>>>    CLUSTER_PG(Power Gate)
>>>    core0
>>>        CORE_RET
>>>        CORE_PG
>>>    core1
>>>        CORE_RET
>>>        CORE_PG
>>>
>>>cluster1
>>>    CLUSTER_RET
>>>    CLUSTER_PG
>>>    core0
>>>        CORE_RET
>>>        CORE_PG
>>>    core1
>>>        CORE_RET
>>>        CORE_PG
>>>
>>>Platform Co-ordinate supports the following states and we should be
>>>able to determine that from the binding:
>>>
>>>CORE_RET
>>>CORE_PG
>>>CORE_RET + CLUSTER_RET
>>
>>The problem that we have to sove here is knowing that CORE_RET +
>>CLUSTER_PG (hypothetically) an invalid combination. Kevin and
>>I debated it in the earlier RFC and we dont have a good way to solve
>>this generically for all devices.
>>
>
>Yes, I agree it's complex. But that needs to be solved IMO.
>
>I can think of 2 possible solutions:
>
>1. Index the states(which people have not liked, but as along as we
>   don't use it in the code as it for any other purpose, it should be
>   fine) and then have each state mentioning what parent state can be
>   entered at this child state(i.e. starting index and all states below
>   it)
>
This is how QCOM solved it downstream.

>2. Something similar to (1) but without index instead phandles.
>

The problem is when you have non-CPU devices in the device tree and
since they do not have a way to represent states like CPU, we did not
have a clear path to that. Hence we punted that to later. Whatever we
do, we should solve it for a generic PM domain, not just CPU domains.

Thanks,
Lina

>Again these are just thoughts, others may think of some better
>solution(s). Sorry I haven't followed all the previous threads in detail.
>
>-- 
>Regards,
>Sudeep

Re: [PATCH v3 02/15] dt/bindings: Update binding for PM domain idle states

[Sudeep Holla]

On 15/08/16 17:08, Lina Iyer wrote:
> On Fri, Aug 12 2016 at 04:08 -0600, Sudeep Holla wrote:
>>
>>
>> On 11/08/16 22:10, Lina Iyer wrote:
>>> On Wed, Aug 10 2016 at 12:09 -0600, Sudeep Holla wrote:
>>>>
>>
>> [...]
>>
>>>> cluster0
>>>>    CLUSTER_RET(Retention)
>>>>    CLUSTER_PG(Power Gate)
>>>>    core0
>>>>        CORE_RET
>>>>        CORE_PG
>>>>    core1
>>>>        CORE_RET
>>>>        CORE_PG
>>>>
>>>> cluster1
>>>>    CLUSTER_RET
>>>>    CLUSTER_PG
>>>>    core0
>>>>        CORE_RET
>>>>        CORE_PG
>>>>    core1
>>>>        CORE_RET
>>>>        CORE_PG
>>>>
>>>> Platform Co-ordinate supports the following states and we should be
>>>> able to determine that from the binding:
>>>>
>>>> CORE_RET
>>>> CORE_PG
>>>> CORE_RET + CLUSTER_RET
>>>
>>> The problem that we have to sove here is knowing that CORE_RET +
>>> CLUSTER_PG (hypothetically) an invalid combination. Kevin and
>>> I debated it in the earlier RFC and we dont have a good way to solve
>>> this generically for all devices.
>>>
>>
>> Yes, I agree it's complex. But that needs to be solved IMO.
>>
>> I can think of 2 possible solutions:
>>
>> 1. Index the states(which people have not liked, but as along as we
>>   don't use it in the code as it for any other purpose, it should be
>>   fine) and then have each state mentioning what parent state can be
>>   entered at this child state(i.e. starting index and all states below
>>   it)
>>
> This is how QCOM solved it downstream.
>

Yes even ACPI has indices to solve this.

>> 2. Something similar to (1) but without index instead phandles.
>>
>
> The problem is when you have non-CPU devices in the device tree and
> since they do not have a way to represent states like CPU, we did not
> have a clear path to that. Hence we punted that to later. Whatever we
> do, we should solve it for a generic PM domain, not just CPU domains.
>

Yes bindings defined here should be applicable for devices to, but only
CPU's will have this hierarchy while the devices need not bother about
hierarchy. However the parent power domain can ever the state which is
least common denominator of all it's children power domain. That's my
understanding. No ?

-- 
Regards,
Sudeep

Re: [PATCH v3 02/15] dt/bindings: Update binding for PM domain idle states

[Lina Iyer]

On Mon, Aug 15 2016 at 10:14 -0600, Sudeep Holla wrote:
>
>
>On 15/08/16 17:08, Lina Iyer wrote:
>>On Fri, Aug 12 2016 at 04:08 -0600, Sudeep Holla wrote:
>>>
>>>
>>>On 11/08/16 22:10, Lina Iyer wrote:
>>>>On Wed, Aug 10 2016 at 12:09 -0600, Sudeep Holla wrote:
>>>>>
>>>
>>>[...]
>>>
>>>>>cluster0
>>>>>   CLUSTER_RET(Retention)
>>>>>   CLUSTER_PG(Power Gate)
>>>>>   core0
>>>>>       CORE_RET
>>>>>       CORE_PG
>>>>>   core1
>>>>>       CORE_RET
>>>>>       CORE_PG
>>>>>
>>>>>cluster1
>>>>>   CLUSTER_RET
>>>>>   CLUSTER_PG
>>>>>   core0
>>>>>       CORE_RET
>>>>>       CORE_PG
>>>>>   core1
>>>>>       CORE_RET
>>>>>       CORE_PG
>>>>>
>>>>>Platform Co-ordinate supports the following states and we should be
>>>>>able to determine that from the binding:
>>>>>
>>>>>CORE_RET
>>>>>CORE_PG
>>>>>CORE_RET + CLUSTER_RET
>>>>
>>>>The problem that we have to sove here is knowing that CORE_RET +
>>>>CLUSTER_PG (hypothetically) an invalid combination. Kevin and
>>>>I debated it in the earlier RFC and we dont have a good way to solve
>>>>this generically for all devices.
>>>>
>>>
>>>Yes, I agree it's complex. But that needs to be solved IMO.
>>>
>>>I can think of 2 possible solutions:
>>>
>>>1. Index the states(which people have not liked, but as along as we
>>>  don't use it in the code as it for any other purpose, it should be
>>>  fine) and then have each state mentioning what parent state can be
>>>  entered at this child state(i.e. starting index and all states below
>>>  it)
>>>
>>This is how QCOM solved it downstream.
>>
>
>Yes even ACPI has indices to solve this.
>
>>>2. Something similar to (1) but without index instead phandles.
>>>
>>
>>The problem is when you have non-CPU devices in the device tree and
>>since they do not have a way to represent states like CPU, we did not
>>have a clear path to that. Hence we punted that to later. Whatever we
>>do, we should solve it for a generic PM domain, not just CPU domains.
>>
>
>Yes bindings defined here should be applicable for devices to, but only
>CPU's will have this hierarchy while the devices need not bother about
>hierarchy. However the parent power domain can ever the state which is
>least common denominator of all it's children power domain. That's my
>understanding. No ?
>
That is correct. But say if all the CPUs choose CORE_RET + CLUSTER_PG,
which is invalid and the firmware has to ignore it and does CORE_RET +
CLUSTER_RET instead, then Linux may have an inconsistent view of the
state selection.

Thanks,
Lina

>-- 
>Regards,
>Sudeep

Re: [PATCH v3 02/15] dt/bindings: Update binding for PM domain idle states

[Brendan Jackman]

On 15/08/16 23:40, Lina Iyer wrote:
> On Mon, Aug 15 2016 at 10:14 -0600, Sudeep Holla wrote:
>>
>>
>> On 15/08/16 17:08, Lina Iyer wrote:
>>> On Fri, Aug 12 2016 at 04:08 -0600, Sudeep Holla wrote:
>>>>
>>>>
>>>> On 11/08/16 22:10, Lina Iyer wrote:
>>>>> On Wed, Aug 10 2016 at 12:09 -0600, Sudeep Holla wrote:
>>>>>>
>>>>
>>>> [...]
>>>>
>>>>>> cluster0
>>>>>>   CLUSTER_RET(Retention)
>>>>>>   CLUSTER_PG(Power Gate)
>>>>>>   core0
>>>>>>       CORE_RET
>>>>>>       CORE_PG
>>>>>>   core1
>>>>>>       CORE_RET
>>>>>>       CORE_PG
>>>>>>
>>>>>> cluster1
>>>>>>   CLUSTER_RET
>>>>>>   CLUSTER_PG
>>>>>>   core0
>>>>>>       CORE_RET
>>>>>>       CORE_PG
>>>>>>   core1
>>>>>>       CORE_RET
>>>>>>       CORE_PG
>>>>>>
>>>>>> Platform Co-ordinate supports the following states and we should be
>>>>>> able to determine that from the binding:
>>>>>>
>>>>>> CORE_RET
>>>>>> CORE_PG
>>>>>> CORE_RET + CLUSTER_RET
>>>>>
>>>>> The problem that we have to sove here is knowing that CORE_RET +
>>>>> CLUSTER_PG (hypothetically) an invalid combination. Kevin and
>>>>> I debated it in the earlier RFC and we dont have a good way to solve
>>>>> this generically for all devices.
>>>>>
>>>>
>>>> Yes, I agree it's complex. But that needs to be solved IMO.
>>>>
>>>> I can think of 2 possible solutions:
>>>>
>>>> 1. Index the states(which people have not liked, but as along as we
>>>>  don't use it in the code as it for any other purpose, it should be
>>>>  fine) and then have each state mentioning what parent state can be
>>>>  entered at this child state(i.e. starting index and all states below
>>>>  it)
>>>>
>>> This is how QCOM solved it downstream.
>>>
>>
>> Yes even ACPI has indices to solve this.
>>
>>>> 2. Something similar to (1) but without index instead phandles.
>>>>
>>>
>>> The problem is when you have non-CPU devices in the device tree and
>>> since they do not have a way to represent states like CPU, we did not
>>> have a clear path to that. Hence we punted that to later. Whatever we
>>> do, we should solve it for a generic PM domain, not just CPU domains.
>>>
>>
>> Yes bindings defined here should be applicable for devices to, but only
>> CPU's will have this hierarchy while the devices need not bother about
>> hierarchy. However the parent power domain can ever the state which is
>> least common denominator of all it's children power domain. That's my
>> understanding. No ?
>>

Are you saying that the parent can enter the shallowest idle state that
all its children are in (I.e if all its children are in "retention" then
it can enter "retention")? I don't know what the reality is on existing
platforms but it doesn't sound like 100% safe assumption to make. Also I
don't think you can necessarily correlate idle states at different
domain levels - i.e. here we've matched up the idea of "retention" at
core level with that of "retention" at cluster level. I may have
misunderstood you there..

> That is correct. But say if all the CPUs choose CORE_RET + CLUSTER_PG,
> which is invalid and the firmware has to ignore it and does CORE_RET +
> CLUSTER_RET instead, then Linux may have an inconsistent view of the
> state selection.
>
Perhaps a better starting point would be to go with the assumption that
a parent PD can only enter any idle state once its children are in their
deepest idle states.

That is, we'd miss out on CORE_RET + CLUSTER_RET but have no invalid ones.

Then a later addition to the bindings as discussed above could enable
the possibility of those combinations to be expressed.
> Thanks,
> Lina
>
>> --
>> Regards,
>> Sudeep
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 02/15] dt/bindings: Update binding for PM domain idle states

[Brendan Jackman]

Hi Lina,

Agh, sorry, sent with the "IMPORTANT NOTICE" again, still getting used to
mailing lists.. here's the message again without it.

On Mon, Aug 15, 2016 at 04:40:14PM -0600, Lina Iyer wrote:
> On Mon, Aug 15 2016 at 10:14 -0600, Sudeep Holla wrote:
> >
> >
> >On 15/08/16 17:08, Lina Iyer wrote:
> >>On Fri, Aug 12 2016 at 04:08 -0600, Sudeep Holla wrote:
> >>>
> >>>
> >>>On 11/08/16 22:10, Lina Iyer wrote:
> >>>>On Wed, Aug 10 2016 at 12:09 -0600, Sudeep Holla wrote:
> >>>>>
> >>>
> >>>[...]
> >>>
> >>>>>cluster0
> >>>>>  CLUSTER_RET(Retention)
> >>>>>  CLUSTER_PG(Power Gate)
> >>>>>  core0
> >>>>>      CORE_RET
> >>>>>      CORE_PG
> >>>>>  core1
> >>>>>      CORE_RET
> >>>>>      CORE_PG
> >>>>>
> >>>>>cluster1
> >>>>>  CLUSTER_RET
> >>>>>  CLUSTER_PG
> >>>>>  core0
> >>>>>      CORE_RET
> >>>>>      CORE_PG
> >>>>>  core1
> >>>>>      CORE_RET
> >>>>>      CORE_PG
> >>>>>
> >>>>>Platform Co-ordinate supports the following states and we should be
> >>>>>able to determine that from the binding:
> >>>>>
> >>>>>CORE_RET
> >>>>>CORE_PG
> >>>>>CORE_RET + CLUSTER_RET
> >>>>
> >>>>The problem that we have to sove here is knowing that CORE_RET +
> >>>>CLUSTER_PG (hypothetically) an invalid combination. Kevin and
> >>>>I debated it in the earlier RFC and we dont have a good way to solve
> >>>>this generically for all devices.
> >>>>
> >>>
> >>>Yes, I agree it's complex. But that needs to be solved IMO.
> >>>
> >>>I can think of 2 possible solutions:
> >>>
> >>>1. Index the states(which people have not liked, but as along as we
> >>> don't use it in the code as it for any other purpose, it should be
> >>> fine) and then have each state mentioning what parent state can be
> >>> entered at this child state(i.e. starting index and all states below
> >>> it)
> >>>
> >>This is how QCOM solved it downstream.
> >>
> >
> >Yes even ACPI has indices to solve this.
> >
> >>>2. Something similar to (1) but without index instead phandles.
> >>>
> >>
> >>The problem is when you have non-CPU devices in the device tree and
> >>since they do not have a way to represent states like CPU, we did not
> >>have a clear path to that. Hence we punted that to later. Whatever we
> >>do, we should solve it for a generic PM domain, not just CPU domains.
> >>
> >
> >Yes bindings defined here should be applicable for devices to, but only
> >CPU's will have this hierarchy while the devices need not bother about
> >hierarchy. However the parent power domain can ever the state which is
> >least common denominator of all it's children power domain. That's my
> >understanding. No?
> >


Are you saying that the parent can enter the shallowest idle state that all its
children are in (I.e if all its children are in "retention" then it can enter
"retention")? I don't know what the reality is on existing platforms but it
doesn't sound like 100% safe assumption to make. Also I don't think you can
necessarily correlate idle states at different domain levels - i.e. here we've
matched up the idea of "retention" at core level with that of "retention" at
cluster level. I may have misunderstood you there..

> That is correct. But say if all the CPUs choose CORE_RET + CLUSTER_PG,
> which is invalid and the firmware has to ignore it and does CORE_RET +
> CLUSTER_RET instead, then Linux may have an inconsistent view of the
> state selection.
>

Perhaps a better starting point would be to go with the assumption that a parent
PD can only enter any idle state once its children are in their deepest idle
states.

So in the example above we'd end up with

CORE_RET
CORE_PG
CORE_PG + CLUSTER_RET
CORE_PG + CLUSTER_PG

(Missing out on CORE_RET + CLUSTER_RET, even though that's a valid combination
from the hardware's perspective)

Then a later addition to the bindings as discussed above could enable the
possibility of those combinations to be expressed.

> Thanks,
> Lina
>
> >--
> >Regards,
> >Sudeep
>

Re: [PATCH v3 02/15] dt/bindings: Update binding for PM domain idle states

[Sudeep Holla]

On 16/08/16 09:41, Brendan Jackman wrote:
> Hi Lina,
> On Mon, Aug 15, 2016 at 04:40:14PM -0600, Lina Iyer wrote:
>> On Mon, Aug 15 2016 at 10:14 -0600, Sudeep Holla wrote:
[,,,]


>>>
>>> Yes even ACPI has indices to solve this.
>>>
>>>>> 2. Something similar to (1) but without index instead phandles.
>>>>>
>>>>
>>>> The problem is when you have non-CPU devices in the device tree and
>>>> since they do not have a way to represent states like CPU, we did not
>>>> have a clear path to that. Hence we punted that to later. Whatever we
>>>> do, we should solve it for a generic PM domain, not just CPU domains.
>>>>
>>>
>>> Yes bindings defined here should be applicable for devices to, but only
>>> CPU's will have this hierarchy while the devices need not bother about
>>> hierarchy. However the parent power domain can ever the state which is
>>> least common denominator of all it's children power domain. That's my
>>> understanding. No?
>>>
>
>
> Are you saying that the parent can enter the shallowest idle state that all its
> children are in (I.e if all its children are in "retention" then it can enter
> "retention")? I don't know what the reality is on existing platforms but it
> doesn't sound like 100% safe assumption to make.

I was referring to non-CPU/device power states above. For CPU we do need
a mechanism in place to indicate the dependency.

> Also I don't think you can
> necessarily correlate idle states at different domain levels - i.e. here we've
> matched up the idea of "retention" at core level with that of "retention" at
> cluster level. I may have misunderstood you there..
>

Correct for CPUs. For normal devices and their power domains, it could
straight forward. E.g if many devices are at-least at state D1(few may
be at state D2 or above), the parent can enter D1.(D0-runnning and D1-D3
are low power states in the above example)

>> That is correct. But say if all the CPUs choose CORE_RET + CLUSTER_PG,
>> which is invalid and the firmware has to ignore it and does CORE_RET +
>> CLUSTER_RET instead, then Linux may have an inconsistent view of the
>> state selection.
>>

1. First, CORE_RET + CLUSTER_PG should not be registered as valid idle
    state.
2. We do have inconsistent view already for platform co-ordinated idle
    In-fact it could happen even with OSC mode I believe. Platform can
    always demote the state, so OS can never get the exact view unless it
    queries the firmware for that explicitly(e.g. PSCI_STATS)

>
> Perhaps a better starting point would be to go with the assumption that a parent
> PD can only enter any idle state once its children are in their deepest idle
> states.
>
> So in the example above we'd end up with
>
> CORE_RET
> CORE_PG
> CORE_PG + CLUSTER_RET
> CORE_PG + CLUSTER_PG
>

Yes this assumption seems good enough to me. At-least no invalid
combination is ensured.

> (Missing out on CORE_RET + CLUSTER_RET, even though that's a valid combination
> from the hardware's perspective)
>

Yes, if it's a real issue then we need proper bindings to deal with
that. Otherwise we can manage without the extra information.

> Then a later addition to the bindings as discussed above could enable the
> possibility of those combinations to be expressed.
>

Seems feasible solution to me, but better to make this explicit in the
binding and check with few others. It looks fair enough assumption IMO.

-- 
-- 
Regards,
Sudeep

Re: [PATCH v3 02/15] dt/bindings: Update binding for PM domain idle states

[Brendan Jackman]

Hi Lina,

Apologies, I sent this reply before and automatically included an "IMPORTANT
NOTICE" footer, please disregard that email, here's the same thing without the
footer.

On Thu, Aug 11, 2016 at 03:10:23PM -0600, Lina Iyer wrote:
> On Wed, Aug 10 2016 at 12:09 -0600, Sudeep Holla wrote:
> >
> >
> >On 10/08/16 17:40, Lina Iyer wrote:
> >>Hi Sudeep,
> >>
> >>On Wed, Aug 10 2016 at 09:15 -0600, Sudeep Holla wrote:
> >>>Hi Lina,
> >>>
> >>>I have few concerns mainly due to the lack of description and not the
> >>>binding per say.
> >
> >[...]
> >
> >>It is pretty clear that CPUs cannot not define the domain idle states.
> >>Domains define their own idle states. Just as you mention above. CPU is
> >>just a single component in its domain. There may be other devices like
> >>PMUs, Coresights etc that also may have a say in the idle state the
> >>domain may be put in, when the devices are idle. As such, adding domain
> >>idle states to the CPU's idle state property is not appropriate.
> >>
> >
> >No I am not saying we need to add domain idle states to the CPU's idle
> >state property. I am saying we need to remove cpu-idle-states or ignore
> >it when PD is present. And get all the idle state information for PD.
> >
> >I am objecting the split we are creating across CPU and higher level
> >power domains. And this binding document is incomplete as it skips all
> >those details. We just need PD handle in CPU and no idle state
> >information there. Create PD hierarchy and have all idle state
> >information at one place.
> >
> Let me think about this a bit and see what I can come up with.
>
> >>Our kernel has runtime PM for devices and then there is CPUidle, both
> >>are diverging without one knowing about the other. We have to start
> >>unifying them inorder to have better holistic power management in the
> >>SoC. To that regard, we have to start imagining CPUs as just another
> >>device, albeit a special device. But for our purposes in determining
> >>domain idle state, it will just be a device attached to the domain.
> >>
> >
> >Absolutely agree on that. No arguments. I am asking to go a step ahead
> >to include even cpu/core level power domains not just cluster/higher
> >level domains.
> >
> >>>We need to have all the idle state information at one place and in this
> >>>case PD seems more appropriate instead of splitting them across.
> >>>
> >>That approach isn't correct. Where will we put the idle states of other
> >>devices that are also part of the domain? We are thinking about a model,
> >>where every device defines its own idle states and we define
> >>relationships between those idle states and their parents' idle states.
> >
> >Yes I understand. You confused me here. Won't that be one-to-one
> >relationship ? If not, how is that dealt in the current bindings ?
> >
> >>Ofcourse, devices don't have idle states today, but that is something we
> >>have been pondering over.
> >>
> >
> >Yes we these binding should be easily extensible, I don't see any issue.
> >
> >>>We can also keep the code clean and not break compatibility. Whenever
> >>>both PD and CPU contains idle-states, PD must take precedence.
> >>>
> >>Why?
> >>The CPU and PD states are orthogonal. While the PD state is dependent on
> >>the CPU state, the latter is not true. Devices determine their own
> >>states. Based on the individual device states, we then determine the
> >>state of the parent and bubble up on the hierarchy.
> >>
> >
> >I may be missing something. Now with your example in the binding, if
> >another device shares the cluster PD, can it have different idle states?
> >If so how does it map ?
> >
> >
> >In general whatever binding we come up must not just address OS
> >coordinated mode. Also I was thinking to have better coverage in
> >the description by having a bit more complex system like:
> >
> >cluster0
> >	CLUSTER_RET(Retention)
> >	CLUSTER_PG(Power Gate)
> >	core0
> >		CORE_RET
> >		CORE_PG
> >	core1
> >		CORE_RET
> >		CORE_PG
> >
> >cluster1
> >	CLUSTER_RET
> >	CLUSTER_PG
> >	core0
> >		CORE_RET
> >		CORE_PG
> >	core1
> >		CORE_RET
> >		CORE_PG
> >
> >Platform Co-ordinate supports the following states and we should
> >be able to determine that from the binding:
> >
> >CORE_RET
> >CORE_PG
> >CORE_RET + CLUSTER_RET
>
> The problem that we have to sove here is knowing that CORE_RET +
> CLUSTER_PG (hypothetically) an invalid combination. Kevin and
> I debated it in the earlier RFC and we dont have a good way to solve
> this generically for all devices.
>


This is interesting. I had been working on the assumption that a parent
power domain cannot enter any idle state until its children were all in
their deepest idle state. I now realise that it's easy to imagine
platforms where this isn't the case.

However, I don't understand how your current bindings solve this issue
and why using domain-power-states for all states (i.e. ignoring
cpu-idle-states and putting CPU idle states in the domain-idle-states of
a per-CPU power domain - I believe this is what Sudeep is suggesting)
makes it any more difficult.

Could you link to this previous discussion you mentioned? I'm having
trouble finding it (R.I.P Gmane).

> >CORE_PG + CLUSTER_RET
> >CORE_PG + CLUSTER_PG
> >

Cheers,
Brendan

Re: [PATCH v3 02/15] dt/bindings: Update binding for PM domain idle states

[Lina Iyer]

On Fri, Aug 12 2016 at 06:35 -0600, Brendan Jackman wrote:
>> >In general whatever binding we come up must not just address OS
>> >coordinated mode. Also I was thinking to have better coverage in
>> >the description by having a bit more complex system like:
>> >
>> >cluster0
>> >	CLUSTER_RET(Retention)
>> >	CLUSTER_PG(Power Gate)
>> >	core0
>> >		CORE_RET
>> >		CORE_PG
>> >	core1
>> >		CORE_RET
>> >		CORE_PG
>> >
>> >cluster1
>> >	CLUSTER_RET
>> >	CLUSTER_PG
>> >	core0
>> >		CORE_RET
>> >		CORE_PG
>> >	core1
>> >		CORE_RET
>> >		CORE_PG
>> >
>> >Platform Co-ordinate supports the following states and we should
>> >be able to determine that from the binding:
>> >
>> >CORE_RET
>> >CORE_PG
>> >CORE_RET + CLUSTER_RET
>>
>> The problem that we have to sove here is knowing that CORE_RET +
>> CLUSTER_PG (hypothetically) an invalid combination. Kevin and
>> I debated it in the earlier RFC and we dont have a good way to solve
>> this generically for all devices.
>>
>
>
>This is interesting. I had been working on the assumption that a parent
>power domain cannot enter any idle state until its children were all in
>their deepest idle state. I now realise that it's easy to imagine
>platforms where this isn't the case.
>
>However, I don't understand how your current bindings solve this issue
>and why using domain-power-states for all states (i.e. ignoring
>cpu-idle-states and putting CPU idle states in the domain-idle-states of
>a per-CPU power domain - I believe this is what Sudeep is suggesting)
>makes it any more difficult.
>
You are right, my current bindings don't solve it. I imagined one would
solve it by writing their own CPU PM Domain governor. In the context of
platform coordinated, we dont have a choice in Linux. May be the
firmware can assert that intelligence in not choosing those states. So,
we may have states added to cpuidle that are invalid and never get
chosen by the firmware. I am not sure, but may be that is acceptable.

>Could you link to this previous discussion you mentioned? I'm having
>trouble finding it (R.I.P Gmane).
>
Sigh. So hard to search. Let me see where it is, if it in mail or IRC
communication.

Thanks,
Lina

>> >CORE_PG + CLUSTER_RET
>> >CORE_PG + CLUSTER_PG
>> >
>
>Cheers,
>Brendan

Re: [PATCH v3 02/15] dt/bindings: Update binding for PM domain idle states

[Kevin Hilman]

Sudeep Holla <sudeep.holla@arm.com> writes:

[...]

> In general whatever binding we come up must not just address OS
> coordinated mode. Also I was thinking to have better coverage in the
> description by having a bit more complex system like:
>
> cluster0
> 	CLUSTER_RET(Retention)
> 	CLUSTER_PG(Power Gate)
> 	core0
> 		CORE_RET
> 		CORE_PG
> 	core1
> 		CORE_RET
> 		CORE_PG

Also, remember that a power domain may contain more than just CPUs, so
this will also need to handle things like:

	device0..N
        	DEV_CLK_GATE
                DEV_RET
                DEV_PG

So, as (I think) Lina was trying to say, including CPU idle states
inside domain idles states doesn't really scale well because it would
also imply domain states would also include device idle states.

IMO, the device-specific states belong in the device nodes, and that
includes CPUs.

It's up to the domain (genpd) governor to look at *all* devices in the
domain, check their state and make a domain-wide decision.

The tricky part remains, IMO, the mapping between device/CPU states and
allowable domain states.

As was suggested earlier, a good potential starting point would be that
all devices/CPUs would need to be in their deepest state before the
domain would make any decisions.  While that leaves soem power savings
on the table, it maps well to how genpd works today with only on/off
states and could be extended with more complicated governors down the
road.

Kevin

Re: [PATCH v3 02/15] dt/bindings: Update binding for PM domain idle states

[Sudeep Holla]

On 19/08/16 19:10, Kevin Hilman wrote:
> Sudeep Holla <sudeep.holla@arm.com> writes:
>
> [...]
>
>> In general whatever binding we come up must not just address OS
>> coordinated mode. Also I was thinking to have better coverage in the
>> description by having a bit more complex system like:
>>
>> cluster0
>> 	CLUSTER_RET(Retention)
>> 	CLUSTER_PG(Power Gate)
>> 	core0
>> 		CORE_RET
>> 		CORE_PG
>> 	core1
>> 		CORE_RET
>> 		CORE_PG
>
> Also, remember that a power domain may contain more than just CPUs, so
> this will also need to handle things like:
>
> 	device0..N
>         	DEV_CLK_GATE
>                 DEV_RET
>                 DEV_PG
>
> So, as (I think) Lina was trying to say, including CPU idle states
> inside domain idles states doesn't really scale well because it would
> also imply domain states would also include device idle states.
>
> IMO, the device-specific states belong in the device nodes, and that
> includes CPUs.
>

OK, IIUC we don't have device idle states binding today, so we are not
breaking anything there. Can you elaborate on the issue you see if we
just have domain idle-states ? Is it because we currently create genpd
domain for each entry ?

If a CPU/Device can be enter idle-state(s) it means that it is in a
power domain on its own, so I don't see any issue in such representation.

> It's up to the domain (genpd) governor to look at *all* devices in the
> domain, check their state and make a domain-wide decision.
>

Lets not mix the current genpd implementation in the kernel into this
discussion for simplicity. How is the implementation in the kernel today
and what can be done is a separate topic.

What this discussion should aim at is to present the idle states in the
system in the device tree so that it address the issues we have
currently and extensible in near future with any compatibility issues.

> The tricky part remains, IMO, the mapping between device/CPU states and
> allowable domain states.
>
> As was suggested earlier, a good potential starting point would be that
> all devices/CPUs would need to be in their deepest state before the
> domain would make any decisions.  While that leaves soem power savings
> on the table, it maps well to how genpd works today with only on/off
> states and could be extended with more complicated governors down the
> road.
>

Agreed.

Some example below for discussion, feel free to add more cases.

--
Regards,
Sudeep


--->8

1. Dual cluster with 2 CPUs in each cluster with powerdown at both CPU and
    cluster level

   idle-states {
     CPU_SLEEP_0: cpu-sleep-0 {
       ...
       entry-latency-us = <300>;
       ...
     };
     CLUSTER_SLEEP_0: cluster-sleep-0 {
       ...
       entry-latency-us = <300>;
       ...
     };
   };

   cpu@0 {
     ...
     /*
      * implentation may ignore cpu-idle-states if power-domains
      * has idle-states, DT's may have both for backward compatibility
      */
     cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
     power-domains = <&CPU_0_1_PD>;
     ...
   };

   cpu@1 {
     ...
     cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
     power-domains = <&CPU_0_1_PD>;
     ...
   };

   cpu@100 {
     ...
     cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
     power-domains = <&CPU_1_0_PD>;
     ...
   };

   cpu@101 {
     ...
     cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
     power-domains = <&CPU_1_1_PD>;
     ...
   };

   power-domains {
     CLUSTER_0_PD: cluster-0-pd {
       #power-domain-cells = <0>;
       domain-idle-states = <&CLUSTER_SLEEP_0>;
     };
     CPU_0_0_PD: cpu-0-0-pd@0 {
       #power-domain-cells = <0>;
       domain-idle-states = <&CPU_SLEEP_0>;
       power-domains = <&CLUSTER_0_PD>;
     };
     CPU_0_1_PD: cpu-0-1-pd@1 {
       #power-domain-cells = <0>;
       domain-idle-states = <&CPU_SLEEP_0>;
       power-domains = <&CLUSTER_0_PD>;
     };
     CLUSTER_1_PD: cluster-1-pd {
       #power-domain-cells = <0>;
       domain-idle-states = <&CLUSTER_SLEEP_0>;
     };
     CPU_1_0_PD: cpu-1-0-pd@0 {
       #power-domain-cells = <0>;
       domain-idle-states = <&CPU_SLEEP_0>;
       power-domains = <&CLUSTER_1_PD>;
     };
     CPU_1_1_PD: cpu-1-1-pd@1 {
       #power-domain-cells = <0>;
       domain-idle-states = <&CPU_SLEEP_0>;
       power-domains = <&CLUSTER_1_PD>;
     };
   };

2. Dual cluster with 2 CPUs in each cluster with retention and powerdown 
at both
    CPU and cluster level

   idle-states {
     CPU_SLEEP_0: cpu-sleep-0 { /* Retention */
       ...
       entry-latency-us = <100>;
       ...
     };
     CPU_SLEEP_1: cpu-sleep-1 { /* Power-down */
       ...
       entry-latency-us = <500>;
       ...
     };
     CLUSTER_SLEEP_0: cluster-sleep-0 { /* Retention */
       ...
       entry-latency-us = <300>;
       ...
     };
     CLUSTER_SLEEP_1: cluster-sleep-1 {/* Power-down */
       ...
       entry-latency-us = <1000>;
       ...
     };
   };

   cpu@0 {
     ...
     power-domains = <&CPU_0_1_PD>;
     ...
   };

   cpu@1 {
     ...
     power-domains = <&CPU_0_1_PD>;
     ...
   };

   cpu@100 {
     ...
     power-domains = <&CPU_1_0_PD>;
     ...
   };

   cpu@101 {
     ...
     power-domains = <&CPU_1_1_PD>;
     ...
   };

   power-domains {
     /*
      * Each cluster/core PD may point to different idle states,
      * it's all same here in the example to keep it short and
      * simple
      */
     CLUSTER_0_PD: cluster-0-pd {
       #power-domain-cells = <0>;
       domain-idle-states = <&CLUSTER_SLEEP_0 &CLUSTER_SLEEP_1>;
     };
     CPU_0_0_PD: cpu-0-0-pd@0 {
       #power-domain-cells = <0>;
       domain-idle-states = <&CPU_SLEEP_0 &CPU_SLEEP_1>;
       power-domains = <&CLUSTER_0_PD>;
     };
     CPU_0_1_PD: cpu-0-1-pd@1 {
       #power-domain-cells = <0>;
       domain-idle-states = <&CPU_SLEEP_0 &CPU_SLEEP_1>;
       power-domains = <&CLUSTER_0_PD>;
     };
     CLUSTER_1_PD: cluster-1-pd {
       #power-domain-cells = <0>;
       domain-idle-states = <&CLUSTER_SLEEP_0 &CLUSTER_SLEEP_1>;
     };
     CPU_1_0_PD: cpu-1-0-pd@0 {
       #power-domain-cells = <0>;
       domain-idle-states = <&CPU_SLEEP_0 &CPU_SLEEP_1>;
       power-domains = <&CLUSTER_1_PD>;
     };
     CPU_1_1_PD: cpu-1-1-pd@1 {
       #power-domain-cells = <0>;
       domain-idle-states = <&CPU_SLEEP_0 &CPU_SLEEP_1>;
       power-domains = <&CLUSTER_1_PD>;
     };
   };

3. Dual cluster with 2 CPUs in each cluster with retention and powerdown at
    just cluster level

   idle-states {
     CLUSTER_SLEEP_0: cluster-sleep-0 { /* Retention */
       ...
       entry-latency-us = <300>;
       ...
     };
     CLUSTER_SLEEP_1: cluster-sleep-1 {/* Power-down */
       ...
       entry-latency-us = <1000>;
       ...
     };
   };

   cpu@0 {
     ...
     power-domains = <&CLUSTER_0_PD>;
     ...
   };

   cpu@1 {
     ...
     power-domains = <&CLUSTER_0_PD>;
     ...
   };

   cpu@100 {
     ...
     power-domains = <&CLUSTER_1_PD>;
     ...
   };

   cpu@101 {
     ...
     power-domains = <&CLUSTER_1_PD>;
     ...
   };

   power-domains {
     CLUSTER_0_PD: cluster-0-pd {
       #power-domain-cells = <0>;
       domain-idle-states = <&CLUSTER_SLEEP_0 &CLUSTER_SLEEP_1>;
     };
     CLUSTER_1_PD: cluster-1-pd {
       #power-domain-cells = <0>;
       domain-idle-states = <&CLUSTER_SLEEP_0 &CLUSTER_SLEEP_1>;
     };
   };

4. 4 devices sharing the power domain.

   idle-states {
     /*
      * Device idle states may differ from CPU idle states in terms
      * of the list of properties
      */
     DEVPD_SLEEP_0: devpd-sleep-0 { /* Retention */
       ...
       entry-latency-us = <300>;
       ...
     };
     DEVPD_SLEEP_1: devpd-sleep-1 {/* Power-down */
       ...
       entry-latency-us = <1000>;
       ...
     };
   };

   dev@0 {
     ...
     power-domains = <&DEV_PD_0>;
     ...
   };

   dev@1 {
     ...
     power-domains = <&DEV_PD_0>;
     ...
   };

   dev@2 {
     ...
     power-domains = <&DEV_PD_0>;
     ...
   };

   dev@3 {
     ...
     power-domains = <&DEV_PD_0>;
     ...
   };

   power-domains {
     DEV_PD_0: device-pd-0 {
       #power-domain-cells = <0>;
       domain-idle-states = <&DEVPD_SLEEP_0 &DEVPD_SLEEP_1>;
     };
   };

5. 4 devices sharing the power domain + another device sharing the power
    domain but has it's own sub-domain

   idle-states {
     DEVPD_0_SLEEP_0: devpd-sleep-0 { /* Retention */
       ...
       entry-latency-us = <300>;
       ...
     };
     DEVPD_0_SLEEP_1: devpd-sleep-1 {/* Power-down */
       ...
       entry-latency-us = <1000>;
       ...
     };
     DEVPD_1_SLEEP_0: devpd-sleep-0 { /* Retention */
       ...
       entry-latency-us = <300>;
       ...
     };
     DEVPD_1_SLEEP_1: devpd-sleep-1 {/* Power-down */
       ...
       entry-latency-us = <1000>;
       ...
     };
   };

   dev@0 {
     ...
     power-domains = <&DEV_PD_0>;
     ...
   };

   dev@1 {
     ...
     power-domains = <&DEV_PD_0>;
     ...
   };

   dev@2 {
     ...
     power-domains = <&DEV_PD_0>;
     ...
   };

   dev@3 {
     ...
     power-domains = <&DEV_PD_0>;
     ...
   };

   dev@4 {
     ...
     power-domains = <&DEV_PD_1>;
     ...
   };

   power-domains {
     DEV_PD_0: device-pd-0 {
       #power-domain-cells = <0>;
       domain-idle-states = <&DEVPD_0_SLEEP_0 &DEVPD_0_SLEEP_1>;
     };
     DEV_PD_1: device-pd-1 {
       #power-domain-cells = <0>;
       power-domains = <&DEV_PD_0>;
       domain-idle-states = <&DEVPD_1_SLEEP_0 &DEVPD_1_SLEEP_1>;
     };
   };

[RFC 0/6] Illustration of using domain-idle-states for CPU idle states

[Brendan Jackman]

Hi,

I've proposed before that, in order to avoid splitting idle state information
between cpu-idle-states (or a hypothetical device-idle-states for devices) and
domain-idle-states, we use domain-idle-states for everything. If a device or CPU
has an idle state, we declare that it's in a power (sub)domain of its own, and
add the idle state as a property of that (sub)domain.

I'm just posting these patches to illustrate the idea. With these patches,
cpu-idle-states is totally ignored when a cpu node has a power-domains property
and instead the power-domains tree is walked to find states. The states are
linearised under the assumption that no parent domain can enter an idle state
until its children are in their deepest possible state.

This _doesn't_ include OSI suspend so not much of the work is done. In order to
extend this for OSI suspend, we'd need to detect whether OSI is supported by the
FW (as in Lina's patch [1]). If it is not supported, we'd add all the states to
cpuidle as in these patches. If it _is_, we'd add only the CPU-level states to
cpuidle, and the remaining states to the CPU PM domains. I'm not clear on how
that would be architected so my next step is to investigate that.

[1] drivers: firmware: psci: Allow OS Initiated suspend mode
    https://patchwork.kernel.org/patch/9264543/

Brendan Jackman (6):
These three are just awkward implementation details for the proof-of-concept:
  cpuidle: Rename cpuidle_get_{cpu->dev}_driver
  cpuidle: Add public funcion to get driver from CPU index
  cpuidle: Add device_node pointer in cpuidle_state

This is the important one that demonstrates the behaviour I'm talking about:
  cpuidle: dt: Add support for reading states from power domains

These two illustrate how the DT would look with this model; ARM Juno r0 is the
only platform I have for the moment.
  arm64: dts: Add Juno r0 CPU power domain tree
  arm64: dts: Add domain-idle-states for Juno r0 power domains

 arch/arm64/boot/dts/arm/juno.dts |  61 +++++++++++
 drivers/cpuidle/cpuidle.c        |  10 +-
 drivers/cpuidle/driver.c         |  22 ++--
 drivers/cpuidle/dt_idle_states.c | 220 ++++++++++++++++++++++++++++++++-------
 drivers/cpuidle/sysfs.c          |   6 +-
 drivers/firmware/psci.c          |  38 +++----
 include/linux/cpuidle.h          |  23 ++--
 kernel/sched/idle.c              |   2 +-
 8 files changed, 297 insertions(+), 85 deletions(-)

--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 1/6] cpuidle: Rename cpuidle_get_{cpu->dev}_driver

[Brendan Jackman]

The current cpuidle_get_cpu_driver takes a cpuidle_device. Rename this
function to cpuidle_get_dev_driver, so that we can create a public
cpuidle_get_cpu_driver that takes a logcal CPU number.
---
 drivers/cpuidle/cpuidle.c | 10 +++++-----
 drivers/cpuidle/driver.c  |  8 ++++----
 drivers/cpuidle/sysfs.c   |  6 +++---
 include/linux/cpuidle.h   |  4 ++--
 kernel/sched/idle.c       |  2 +-
 5 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index c73207a..3027a35 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -58,7 +58,7 @@ bool cpuidle_not_available(struct cpuidle_driver *drv,
 int cpuidle_play_dead(void)
 {
 	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
-	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
+	struct cpuidle_driver *drv = cpuidle_get_dev_driver(dev);
 	int i;
 
 	if (!drv)
@@ -367,7 +367,7 @@ int cpuidle_enable_device(struct cpuidle_device *dev)
 	if (dev->enabled)
 		return 0;
 
-	drv = cpuidle_get_cpu_driver(dev);
+	drv = cpuidle_get_dev_driver(dev);
 
 	if (!drv || !cpuidle_curr_governor)
 		return -EIO;
@@ -407,7 +407,7 @@ EXPORT_SYMBOL_GPL(cpuidle_enable_device);
  */
 void cpuidle_disable_device(struct cpuidle_device *dev)
 {
-	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
+	struct cpuidle_driver *drv = cpuidle_get_dev_driver(dev);
 
 	if (!dev || !dev->enabled)
 		return;
@@ -428,7 +428,7 @@ EXPORT_SYMBOL_GPL(cpuidle_disable_device);
 
 static void __cpuidle_unregister_device(struct cpuidle_device *dev)
 {
-	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
+	struct cpuidle_driver *drv = cpuidle_get_dev_driver(dev);
 
 	list_del(&dev->device_list);
 	per_cpu(cpuidle_devices, dev->cpu) = NULL;
@@ -453,7 +453,7 @@ static void __cpuidle_device_init(struct cpuidle_device *dev)
 static int __cpuidle_register_device(struct cpuidle_device *dev)
 {
 	int ret;
-	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
+	struct cpuidle_driver *drv = cpuidle_get_dev_driver(dev);
 
 	if (!try_module_get(drv->owner))
 		return -EINVAL;
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index 389ade4..9e0ec15 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -329,20 +329,20 @@ struct cpuidle_driver *cpuidle_get_driver(void)
 EXPORT_SYMBOL_GPL(cpuidle_get_driver);
 
 /**
- * cpuidle_get_cpu_driver - return the driver registered for a CPU.
+ * cpuidle_get_dev_driver - return the driver registered for a cpuidle device.
  * @dev: a valid pointer to a struct cpuidle_device
  *
  * Returns a struct cpuidle_driver pointer, or NULL if no driver is registered
  * for the CPU associated with @dev.
  */
-struct cpuidle_driver *cpuidle_get_cpu_driver(struct cpuidle_device *dev)
+struct cpuidle_driver *cpuidle_get_dev_driver(struct cpuidle_device *dev)
 {
 	if (!dev)
 		return NULL;
 
-	return __cpuidle_get_cpu_driver(dev->cpu);
+	return cpuidle_get_cpu_driver(dev->cpu);
 }
-EXPORT_SYMBOL_GPL(cpuidle_get_cpu_driver);
+EXPORT_SYMBOL_GPL(cpuidle_get_dev_driver);
 
 /**
  * cpuidle_driver_ref - get a reference to the driver.
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
index 832a2c3..9c35bdd 100644
--- a/drivers/cpuidle/sysfs.c
+++ b/drivers/cpuidle/sysfs.c
@@ -398,7 +398,7 @@ static int cpuidle_add_state_sysfs(struct cpuidle_device *device)
 	int i, ret = -ENOMEM;
 	struct cpuidle_state_kobj *kobj;
 	struct cpuidle_device_kobj *kdev = device->kobj_dev;
-	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(device);
+	struct cpuidle_driver *drv = cpuidle_get_dev_driver(device);
 
 	/* state statistics */
 	for (i = 0; i < drv->state_count; i++) {
@@ -433,7 +433,7 @@ error_state:
  */
 static void cpuidle_remove_state_sysfs(struct cpuidle_device *device)
 {
-	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(device);
+	struct cpuidle_driver *drv = cpuidle_get_dev_driver(device);
 	int i;
 
 	for (i = 0; i < drv->state_count; i++)
@@ -529,7 +529,7 @@ static int cpuidle_add_driver_sysfs(struct cpuidle_device *dev)
 {
 	struct cpuidle_driver_kobj *kdrv;
 	struct cpuidle_device_kobj *kdev = dev->kobj_dev;
-	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
+	struct cpuidle_driver *drv = cpuidle_get_dev_driver(dev);
 	int ret;
 
 	kdrv = kzalloc(sizeof(*kdrv), GFP_KERNEL);
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index bb31373..e32c72b 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -151,7 +151,7 @@ extern int cpuidle_enable_device(struct cpuidle_device *dev);
 extern void cpuidle_disable_device(struct cpuidle_device *dev);
 extern int cpuidle_play_dead(void);
 
-extern struct cpuidle_driver *cpuidle_get_cpu_driver(struct cpuidle_device *dev);
+extern struct cpuidle_driver *cpuidle_get_dev_driver(struct cpuidle_device *dev);
 static inline struct cpuidle_device *cpuidle_get_device(void)
 {return __this_cpu_read(cpuidle_devices); }
 #else
@@ -187,7 +187,7 @@ static inline int cpuidle_enable_device(struct cpuidle_device *dev)
 {return -ENODEV; }
 static inline void cpuidle_disable_device(struct cpuidle_device *dev) { }
 static inline int cpuidle_play_dead(void) {return -ENODEV; }
-static inline struct cpuidle_driver *cpuidle_get_cpu_driver(
+static inline struct cpuidle_driver *cpuidle_get_dev_driver(
 	struct cpuidle_device *dev) {return NULL; }
 static inline struct cpuidle_device *cpuidle_get_device(void) {return NULL; }
 #endif
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index 9fb873c..bb34461 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -128,7 +128,7 @@ static int call_cpuidle(struct cpuidle_driver *drv, struct cpuidle_device *dev,
 static void cpuidle_idle_call(void)
 {
 	struct cpuidle_device *dev = cpuidle_get_device();
-	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
+	struct cpuidle_driver *drv = cpuidle_get_dev_driver(dev);
 	int next_state, entered_state;
 
 	/*
-- 
2.9.3

[RFC 2/6] cpuidle: Add public funcion to get driver from CPU index

[Brendan Jackman]

This will be used to enable the PSCI driver to get a reference to the
cpuidle driver, in absence of a registered cpuidle_device, in order to
get the idle states that the cpuidle driver init discovered.
---
 drivers/cpuidle/driver.c | 14 +++++++-------
 include/linux/cpuidle.h  |  3 +++
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index 9e0ec15..9cb4300 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -24,13 +24,13 @@ DEFINE_SPINLOCK(cpuidle_driver_lock);
 static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers);
 
 /**
- * __cpuidle_get_cpu_driver - return the cpuidle driver tied to a CPU.
+ * cpuidle_get_cpu_driver - return the cpuidle driver tied to a CPU.
  * @cpu: the CPU handled by the driver
  *
  * Returns a pointer to struct cpuidle_driver or NULL if no driver has been
  * registered for @cpu.
  */
-static struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
+struct cpuidle_driver *cpuidle_get_cpu_driver(int cpu)
 {
 	return per_cpu(cpuidle_drivers, cpu);
 }
@@ -49,7 +49,7 @@ static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
 
 	for_each_cpu(cpu, drv->cpumask) {
 
-		if (drv != __cpuidle_get_cpu_driver(cpu))
+		if (drv != cpuidle_get_cpu_driver(cpu))
 			continue;
 
 		per_cpu(cpuidle_drivers, cpu) = NULL;
@@ -71,7 +71,7 @@ static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
 
 	for_each_cpu(cpu, drv->cpumask) {
 
-		if (__cpuidle_get_cpu_driver(cpu)) {
+		if (cpuidle_get_cpu_driver(cpu)) {
 			__cpuidle_unset_driver(drv);
 			return -EBUSY;
 		}
@@ -87,13 +87,13 @@ static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
 static struct cpuidle_driver *cpuidle_curr_driver;
 
 /**
- * __cpuidle_get_cpu_driver - return the global cpuidle driver pointer.
+ * cpuidle_get_cpu_driver - return the global cpuidle driver pointer.
  * @cpu: ignored without the multiple driver support
  *
  * Return a pointer to a struct cpuidle_driver object or NULL if no driver was
  * previously registered.
  */
-static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
+inline struct cpuidle_driver *cpuidle_get_cpu_driver(int cpu)
 {
 	return cpuidle_curr_driver;
 }
@@ -321,7 +321,7 @@ struct cpuidle_driver *cpuidle_get_driver(void)
 	int cpu;
 
 	cpu = get_cpu();
-	drv = __cpuidle_get_cpu_driver(cpu);
+	drv = cpuidle_get_cpu_driver(cpu);
 	put_cpu();
 
 	return drv;
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index e32c72b..8435577 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -134,6 +134,7 @@ extern int cpuidle_enter(struct cpuidle_driver *drv,
 extern void cpuidle_reflect(struct cpuidle_device *dev, int index);
 
 extern int cpuidle_register_driver(struct cpuidle_driver *drv);
+extern struct cpuidle_driver *cpuidle_get_cpu_driver(int cpu);
 extern struct cpuidle_driver *cpuidle_get_driver(void);
 extern struct cpuidle_driver *cpuidle_driver_ref(void);
 extern void cpuidle_driver_unref(void);
@@ -168,6 +169,8 @@ static inline int cpuidle_enter(struct cpuidle_driver *drv,
 static inline void cpuidle_reflect(struct cpuidle_device *dev, int index) { }
 static inline int cpuidle_register_driver(struct cpuidle_driver *drv)
 {return -ENODEV; }
+static inline struct cpuidle_driver *cpuidle_get_cpu_driver(int cpu)
+{return NULL; }
 static inline struct cpuidle_driver *cpuidle_get_driver(void) {return NULL; }
 static inline struct cpuidle_driver *cpuidle_driver_ref(void) {return NULL; }
 static inline void cpuidle_driver_unref(void) {}
-- 
2.9.3

[RFC 3/6] cpuidle: Add device_node pointer in cpuidle_state

[Brendan Jackman]

Store a pointer in the cpuidle_state structure to the Device Tree node
from which that state originated, if such a node exists.

This will allow plaform-specific components to extract extra idle state
information without duplicating the logic that finds the idle state
nodes.
---
 drivers/cpuidle/dt_idle_states.c |  3 +++
 include/linux/cpuidle.h          | 18 ++++++++++--------
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c
index a5c111b..61a70ec 100644
--- a/drivers/cpuidle/dt_idle_states.c
+++ b/drivers/cpuidle/dt_idle_states.c
@@ -88,6 +88,9 @@ static int init_state_node(struct cpuidle_state *idle_state,
 	 */
 	strncpy(idle_state->name, state_node->name, CPUIDLE_NAME_LEN - 1);
 	strncpy(idle_state->desc, desc, CPUIDLE_DESC_LEN - 1);
+
+	idle_state->of_node = state_node;
+
 	return 0;
 }
 
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 8435577..d4ea1e4 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -14,6 +14,7 @@
 #include <linux/percpu.h>
 #include <linux/list.h>
 #include <linux/hrtimer.h>
+#include <linux/of.h>
 
 #define CPUIDLE_STATE_MAX	10
 #define CPUIDLE_NAME_LEN	16
@@ -36,14 +37,15 @@ struct cpuidle_state_usage {
 };
 
 struct cpuidle_state {
-	char		name[CPUIDLE_NAME_LEN];
-	char		desc[CPUIDLE_DESC_LEN];
-
-	unsigned int	flags;
-	unsigned int	exit_latency; /* in US */
-	int		power_usage; /* in mW */
-	unsigned int	target_residency; /* in US */
-	bool		disabled; /* disabled on all CPUs */
+	char			name[CPUIDLE_NAME_LEN];
+	char			desc[CPUIDLE_DESC_LEN];
+
+	unsigned int		flags;
+	unsigned int		exit_latency; /* in US */
+	int			power_usage; /* in mW */
+	unsigned int		target_residency; /* in US */
+	bool			disabled; /* disabled on all CPUs */
+	struct device_node	*of_node;
 
 	int (*enter)	(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv,
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 6/6] arm64: dts: Add domain-idle-states for Juno r0 power domains

[Brendan Jackman]

---
 arch/arm64/boot/dts/arm/juno.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
index 4e086ec..d2bc4e3 100644
--- a/arch/arm64/boot/dts/arm/juno.dts
+++ b/arch/arm64/boot/dts/arm/juno.dts
@@ -160,40 +160,48 @@
 	power-domains {
 		CLUSTER_A57_PD: cluster-a57-pd {
 			#power-domain-cells = <0>;
+			domain-idle-states = <&CLUSTER_SLEEP_0>;
 		};
 
 		A57_0_PD: a57-pd@0 {
 			#power-domain-cells = <0>;
+			domain-idle-states = <&CPU_SLEEP_0>;
 			power-domains = <&CLUSTER_A57_PD>;
 		};
 
 		A57_1_PD: a57-pd@1 {
 			#power-domain-cells = <0>;
+			domain-idle-states = <&CPU_SLEEP_0>;
 			power-domains = <&CLUSTER_A57_PD>;
 		};
 
 
 		CLUSTER_A53_PD: cluster-a53-pd {
 			#power-domain-cells = <0>;
+			domain-idle-states = <&CLUSTER_SLEEP_0>;
 		};
 
 		A53_0_PD: a53-pd@0 {
 			#power-domain-cells = <0>;
+			domain-idle-states = <&CPU_SLEEP_0>;
 			power-domains = <&CLUSTER_A53_PD>;
 		};
 
 		A53_1_PD: a53-pd@1 {
 			#power-domain-cells = <0>;
+			domain-idle-states = <&CPU_SLEEP_0>;
 			power-domains = <&CLUSTER_A53_PD>;
 		};
 
 		A53_2_PD: a53-pd@2 {
 			#power-domain-cells = <0>;
+			domain-idle-states = <&CPU_SLEEP_0>;
 			power-domains = <&CLUSTER_A53_PD>;
 		};
 
 		A53_3_PD: a53-pd@3 {
 			#power-domain-cells = <0>;
+			domain-idle-states = <&CPU_SLEEP_0>;
 			power-domains = <&CLUSTER_A53_PD>;
 		};
 	};
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 4/6] cpuidle: dt: Add support for reading states from power domains

[Brendan Jackman]

This commit allows the list of phandles to idle state nodes for a CPU to
be read from its hierarchy of power domain nodes, in preference to its
cpu-idle-states property. Each CPU's power domain hierarchy is walked,
and at each power domain topology level, all idle states discovered are
appended to the list for that CPU. If a CPU node does not contain a
power-domains property (i.e. has no power domain hierarchy), we fall
back to cpu-idle-states.

This will later be used for the derivation of idle state energy cost
data for Energy Aware Scheduling, for which we need to know the power
domain topology level to which an idle state applies in hardware.

The logic for locating idle state nodes was previously trivial (simply
read the cpu-idle-states property), so was duplicated between
dt_idle_states.c and the PSCI driver. To avoid duplicating the new
logic, the PSCI driver is modified to locate the idle state nodes via
the cpuidle driver's states list.
---

NB This assumes that #power-domain-cells is always 0. I'm not sure how we'd
   work this out for platforms where multiple power domains share a controller
   and are therefore represented by the same node. I'm guessing in those cases
   all of those power domains would share idle states, and if not we'd add
   intermediary subdomains in which we could point to the states.

 drivers/cpuidle/dt_idle_states.c | 205 ++++++++++++++++++++++++++++++++-------
 drivers/firmware/psci.c          |  38 ++++----
 2 files changed, 187 insertions(+), 56 deletions(-)

diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c
index 61a70ec..c2b590f 100644
--- a/drivers/cpuidle/dt_idle_states.c
+++ b/drivers/cpuidle/dt_idle_states.c
@@ -18,9 +18,128 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/slab.h>

 #include "dt_idle_states.h"

+struct {
+	unsigned int count;
+	struct device_node *nodes[CPUIDLE_STATE_MAX + 1];
+} *state_node_arrays;
+
+static unsigned int __init parse_phandles(struct device_node *np,
+					  const char *prop_name,
+					  struct device_node **nodes_out,
+					  int nodes_out_len)
+{
+	unsigned int i;
+
+	for (i = 0; i < nodes_out_len; i++) {
+		struct device_node *state_node;
+
+		state_node = of_parse_phandle(np, prop_name, i);
+		if (!state_node)
+			break;
+
+		nodes_out[i] = state_node;
+
+		of_node_put(state_node);
+	}
+
+	return i;
+}
+
+static unsigned int __init find_cpu_state_nodes(struct device_node *cpu_node,
+						struct device_node **nodes_out,
+						int nodes_out_len)
+{
+	struct device_node *consumer_node;
+	unsigned int i, state_idx;
+
+	if (!of_property_read_bool(cpu_node, "power-domains")) {
+		return parse_phandles(cpu_node, "cpu-idle-states",
+				      nodes_out, nodes_out_len);
+	}
+
+	consumer_node = of_node_get(cpu_node);
+
+	/*
+	 * Use a loop counter, i, so we don't infiniloop if there's a cycle in
+	 * the power-domains graph (i.e. the DT is borken).
+	 */
+	for (i = 0, state_idx = 0;
+	     (i < 8) && (state_idx < nodes_out_len);
+	     i++) {
+		/*
+		 * TODO
+		 * - Make this work with parent PDs that are parent DT nodes too
+		 *   (i.e. see if parent node has #power-domain-cells)
+		 */
+		struct device_node *pd_node = of_parse_phandle(
+			consumer_node, "power-domains", 0);
+		if (!pd_node) {
+			of_node_put(consumer_node);
+			break;
+		}
+
+		state_idx += parse_phandles(pd_node, "domain-idle-states",
+					    &nodes_out[state_idx],
+					    nodes_out_len - state_idx);
+
+		of_node_put(consumer_node);
+		consumer_node = pd_node;
+	}
+
+	if (i == 8) {
+		pr_warn("%s: DT CPU power-domains graph too deep.\n", __func__);
+		pr_warn("%s: Some idle states will be ignored\n", __func__);
+	}
+	if (state_idx == nodes_out_len)
+		pr_warn("%s: Too many idle state phandles in DT, ignoring some",
+			__func__);
+
+	return state_idx;
+}
+
+static int __init find_state_nodes(void)
+{
+	int cpu;
+
+	if (state_node_arrays)
+		return 0;
+
+	state_node_arrays = kmalloc_array(nr_cpu_ids,
+					  sizeof(state_node_arrays[0]),
+					  GFP_KERNEL);
+	if (!state_node_arrays)
+		return -ENOMEM;
+
+	for_each_possible_cpu(cpu) {
+		struct device_node *cpu_node;
+
+		cpu_node = of_node_get(get_cpu_device(cpu)->of_node);
+
+		if (WARN_ON(!cpu_node))
+			return -EINVAL;
+
+		state_node_arrays[cpu].count = find_cpu_state_nodes(
+			cpu_node, state_node_arrays[cpu].nodes,
+			ARRAY_SIZE(state_node_arrays[cpu].nodes));
+
+		of_node_put(cpu_node);
+	}
+
+	return 0;
+}
+
+static int __init free_state_node_arrays(void)
+{
+	kfree(state_node_arrays);
+	state_node_arrays = NULL;
+	return 0;
+}
+late_initcall(free_state_node_arrays);
+
 static int init_state_node(struct cpuidle_state *idle_state,
 			   const struct of_device_id *matches,
 			   struct device_node *state_node)
@@ -95,38 +214,40 @@ static int init_state_node(struct cpuidle_state *idle_state,
 }

 /*
- * Check that the idle state is uniform across all CPUs in the CPUidle driver
+ * Check that the idle states are uniform across all CPUs in the CPUidle driver
  * cpumask
  */
-static bool idle_state_valid(struct device_node *state_node, unsigned int idx,
-			     const cpumask_t *cpumask)
+static bool idle_states_valid(struct device_node **state_nodes,
+			      unsigned int count, const cpumask_t *cpumask)
 {
 	int cpu;
-	struct device_node *cpu_node, *curr_state_node;
-	bool valid = true;
+	struct device_node **curr_state_nodes;
+	unsigned int curr_count;
+	unsigned int i;

 	/*
-	 * Compare idle state phandles for index idx on all CPUs in the
+	 * Compare idle states on all CPUs in the
 	 * CPUidle driver cpumask. Start from next logical cpu following
-	 * cpumask_first(cpumask) since that's the CPU state_node was
+	 * cpumask_first(cpumask) since that's the CPU state_nodes were
 	 * retrieved from. If a mismatch is found bail out straight
 	 * away since we certainly hit a firmware misconfiguration.
 	 */
 	for (cpu = cpumask_next(cpumask_first(cpumask), cpumask);
 	     cpu < nr_cpu_ids; cpu = cpumask_next(cpu, cpumask)) {
-		cpu_node = of_cpu_device_node_get(cpu);
-		curr_state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
-						   idx);
-		if (state_node != curr_state_node)
-			valid = false;

-		of_node_put(curr_state_node);
-		of_node_put(cpu_node);
-		if (!valid)
-			break;
+		curr_state_nodes = state_node_arrays[cpu].nodes;
+		curr_count = state_node_arrays[cpu].count;
+
+		if (curr_count != count)
+			return false;
+
+		for (i = 0; i < count; i++) {
+			if (state_nodes[i] != curr_state_nodes[i])
+				return false;
+		}
 	}

-	return valid;
+	return true;
 }

 /**
@@ -156,13 +277,21 @@ int dt_init_idle_driver(struct cpuidle_driver *drv,
 			unsigned int start_idx)
 {
 	struct cpuidle_state *idle_state;
-	struct device_node *state_node, *cpu_node;
+	struct device_node *state_node;
 	int i, err = 0;
 	const cpumask_t *cpumask;
+	int cpu;
 	unsigned int state_idx = start_idx;
+	struct device_node **state_nodes;
+	unsigned int state_nodes_count;

 	if (state_idx >= CPUIDLE_STATE_MAX)
 		return -EINVAL;
+
+	err = find_state_nodes();
+	if (err)
+		return err;
+
 	/*
 	 * We get the idle states for the first logical cpu in the
 	 * driver mask (or cpu_possible_mask if the driver cpumask is not set)
@@ -170,27 +299,28 @@ int dt_init_idle_driver(struct cpuidle_driver *drv,
 	 * across CPUs, otherwise we hit a firmware misconfiguration.
 	 */
 	cpumask = drv->cpumask ? : cpu_possible_mask;
-	cpu_node = of_cpu_device_node_get(cpumask_first(cpumask));
+	cpu = cpumask_first(cpumask);
+	state_nodes = state_node_arrays[cpu].nodes;
+	state_nodes_count = state_node_arrays[cpu].count;

-	for (i = 0; ; i++) {
-		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
-		if (!state_node)
-			break;
+	if (start_idx + state_nodes_count > CPUIDLE_STATE_MAX) {
+		pr_warn("%s: Too many idle state nodes, ignoring some\n",
+			__func__);
+		state_nodes_count = (CPUIDLE_STATE_MAX - start_idx);
+	}

-		if (!of_device_is_available(state_node))
-			continue;
+	if (!idle_states_valid(state_nodes, state_nodes_count, cpumask)) {
+		pr_warn("CPU%d idle states not valid, bailing out\n", cpu);
+		return -EINVAL;
+	}

-		if (!idle_state_valid(state_node, i, cpumask)) {
-			pr_warn("%s idle state not valid, bailing out\n",
-				state_node->full_name);
-			err = -EINVAL;
-			break;
-		}
+	for (i = 0; i < state_nodes_count; i++) {
+		state_node = of_node_get(state_nodes[i]);
+		if (WARN_ON(!state_node))
+			return -EINVAL;

-		if (state_idx == CPUIDLE_STATE_MAX) {
-			pr_warn("State index reached static CPU idle driver states array size\n");
-			break;
-		}
+		if (!of_device_is_available(state_node))
+			goto next;

 		idle_state = &drv->states[state_idx++];
 		err = init_state_node(idle_state, matches, state_node);
@@ -198,13 +328,14 @@ int dt_init_idle_driver(struct cpuidle_driver *drv,
 			pr_err("Parsing idle state node %s failed with err %d\n",
 			       state_node->full_name, err);
 			err = -EINVAL;
+			of_node_put(state_node);
 			break;
 		}
+
+next:
 		of_node_put(state_node);
 	}

-	of_node_put(state_node);
-	of_node_put(cpu_node);
 	if (err)
 		return err;
 	/*
diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
index 8263429..4bdf0f2 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -25,6 +25,7 @@
 #include <linux/reboot.h>
 #include <linux/slab.h>
 #include <linux/suspend.h>
+#include <linux/cpuidle.h>

 #include <uapi/linux/psci.h>

@@ -251,21 +252,21 @@ static int __init psci_features(u32 psci_func_id)
 #ifdef CONFIG_CPU_IDLE
 static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);

-static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
+static int psci_dt_cpu_init_idle(int cpu)
 {
-	int i, ret, count = 0;
+	int i, ret, count;
 	u32 *psci_states;
-	struct device_node *state_node;
+	struct cpuidle_driver *cpuidle_drv;
+	struct device_node *state_node = NULL;

-	/* Count idle states */
-	while ((state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
-					      count))) {
-		count++;
-		of_node_put(state_node);
+	cpuidle_drv = cpuidle_get_cpu_driver(cpu);
+	if (!cpuidle_drv) {
+		pr_warn("%s: Couldn't get cpuidle driver for CPU%d\n",
+			__func__, cpu);
+		return -ENODEV;
 	}

-	if (!count)
-		return -ENODEV;
+	count = cpuidle_drv->state_count - 1;

 	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
 	if (!psci_states)
@@ -274,7 +275,13 @@ static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
 	for (i = 0; i < count; i++) {
 		u32 state;

-		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
+		state_node = of_node_get(cpuidle_drv->states[i + 1].of_node);
+		if (!state_node) {
+			pr_warn("%s: No DT node for cpuidle state %d on CPU%d\n",
+				__func__, i, cpu);
+			ret = -ENODEV;
+			goto free_mem;
+		}

 		ret = of_property_read_u32(state_node,
 					   "arm,psci-suspend-param",
@@ -354,7 +361,6 @@ static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu)

 int psci_cpu_init_idle(unsigned int cpu)
 {
-	struct device_node *cpu_node;
 	int ret;

 	/*
@@ -367,13 +373,7 @@ int psci_cpu_init_idle(unsigned int cpu)
 	if (!acpi_disabled)
 		return psci_acpi_cpu_init_idle(cpu);

-	cpu_node = of_get_cpu_node(cpu, NULL);
-	if (!cpu_node)
-		return -ENODEV;
-
-	ret = psci_dt_cpu_init_idle(cpu_node, cpu);
-
-	of_node_put(cpu_node);
+	ret = psci_dt_cpu_init_idle(cpu);

 	return ret;
 }
--
2.9.3

[RFC 5/6] arm64: dts: Add Juno r0 CPU power domain tree

[Brendan Jackman]

---
 arch/arm64/boot/dts/arm/juno.dts | 47 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
index a7270ef..4e086ec 100644
--- a/arch/arm64/boot/dts/arm/juno.dts
+++ b/arch/arm64/boot/dts/arm/juno.dts
@@ -90,6 +90,7 @@
 			next-level-cache = <&A57_L2>;
 			clocks = <&scpi_dvfs 0>;
 			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
+			power-domains = <&A57_0_PD>;
 		};
 
 		A57_1: cpu@1 {
@@ -100,6 +101,7 @@
 			next-level-cache = <&A57_L2>;
 			clocks = <&scpi_dvfs 0>;
 			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
+			power-domains = <&A57_1_PD>;
 		};
 
 		A53_0: cpu@100 {
@@ -110,6 +112,7 @@
 			next-level-cache = <&A53_L2>;
 			clocks = <&scpi_dvfs 1>;
 			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
+			power-domains = <&A53_0_PD>;
 		};
 
 		A53_1: cpu@101 {
@@ -120,6 +123,7 @@
 			next-level-cache = <&A53_L2>;
 			clocks = <&scpi_dvfs 1>;
 			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
+			power-domains = <&A53_1_PD>;
 		};
 
 		A53_2: cpu@102 {
@@ -130,6 +134,7 @@
 			next-level-cache = <&A53_L2>;
 			clocks = <&scpi_dvfs 1>;
 			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
+			power-domains = <&A53_2_PD>;
 		};
 
 		A53_3: cpu@103 {
@@ -140,6 +145,7 @@
 			next-level-cache = <&A53_L2>;
 			clocks = <&scpi_dvfs 1>;
 			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
+			power-domains = <&A53_3_PD>;
 		};
 
 		A57_L2: l2-cache0 {
@@ -151,6 +157,47 @@
 		};
 	};
 
+	power-domains {
+		CLUSTER_A57_PD: cluster-a57-pd {
+			#power-domain-cells = <0>;
+		};
+
+		A57_0_PD: a57-pd@0 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_A57_PD>;
+		};
+
+		A57_1_PD: a57-pd@1 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_A57_PD>;
+		};
+
+
+		CLUSTER_A53_PD: cluster-a53-pd {
+			#power-domain-cells = <0>;
+		};
+
+		A53_0_PD: a53-pd@0 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_A53_PD>;
+		};
+
+		A53_1_PD: a53-pd@1 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_A53_PD>;
+		};
+
+		A53_2_PD: a53-pd@2 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_A53_PD>;
+		};
+
+		A53_3_PD: a53-pd@3 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_A53_PD>;
+		};
+	};
+
 	pmu_a57 {
 		compatible = "arm,cortex-a57-pmu";
 		interrupts = <GIC_SPI 02 IRQ_TYPE_LEVEL_HIGH>,
-- 
2.9.3

[PATCH v3 14/15] ARM64: dts: Add PSCI cpuidle support for MSM8916

[Lina Iyer]

Add device bindings for CPUs to suspend using PSCI as the enable-method.

Cc: <devicetree@vger.kernel.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8916.dtsi | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index 9681200..3029773 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -62,6 +62,8 @@
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0>;
 			next-level-cache = <&L2_0>;
+			enable-method = "psci";
+			cpu-idle-states = <&CPU_SPC>;
 		};
 
 		CPU1: cpu@1 {
@@ -69,6 +71,8 @@
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x1>;
 			next-level-cache = <&L2_0>;
+			enable-method = "psci";
+			cpu-idle-states = <&CPU_SPC>;
 		};
 
 		CPU2: cpu@2 {
@@ -76,6 +80,8 @@
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x2>;
 			next-level-cache = <&L2_0>;
+			enable-method = "psci";
+			cpu-idle-states = <&CPU_SPC>;
 		};
 
 		CPU3: cpu@3 {
@@ -83,12 +89,30 @@
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x3>;
 			next-level-cache = <&L2_0>;
+			enable-method = "psci";
+			cpu-idle-states = <&CPU_SPC>;
 		};
 
 		L2_0: l2-cache {
 		      compatible = "cache";
 		      cache-level = <2>;
 		};
+
+		idle-states {
+			CPU_SPC: spc {
+				compatible = "arm,idle-state";
+				arm,psci-suspend-param = <0x40000002>;
+				entry-latency-us = <130>;
+				exit-latency-us = <150>;
+				min-residency-us = <2000>;
+				local-timer-stop;
+			};
+		};
+	};
+
+	psci {
+		compatible = "arm,psci-1.0";
+		method = "smc";
 	};
 
 	timer {
-- 
2.7.4

[PATCH v3 15/15] ARM64: dts: Define CPU power domain for MSM8916

[Lina Iyer]

Define power domain and the power states for the domain as defined by
the PSCI firmware. The 8916 firmware supports OS initiated method of
powering off the CPU clusters.

Cc: <devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Signed-off-by: Lina Iyer <lina.iyer-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm64/boot/dts/qcom/msm8916.dtsi | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index 3029773..eb0aaed 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -64,6 +64,7 @@
 			next-level-cache = <&L2_0>;
 			enable-method = "psci";
 			cpu-idle-states = <&CPU_SPC>;
+			power-domains = <&CPU_PD>;
 		};
 
 		CPU1: cpu@1 {
@@ -73,6 +74,7 @@
 			next-level-cache = <&L2_0>;
 			enable-method = "psci";
 			cpu-idle-states = <&CPU_SPC>;
+			power-domains = <&CPU_PD>;
 		};
 
 		CPU2: cpu@2 {
@@ -82,6 +84,7 @@
 			next-level-cache = <&L2_0>;
 			enable-method = "psci";
 			cpu-idle-states = <&CPU_SPC>;
+			power-domains = <&CPU_PD>;
 		};
 
 		CPU3: cpu@3 {
@@ -91,6 +94,7 @@
 			next-level-cache = <&L2_0>;
 			enable-method = "psci";
 			cpu-idle-states = <&CPU_SPC>;
+			power-domains = <&CPU_PD>;
 		};
 
 		L2_0: l2-cache {
@@ -113,6 +117,29 @@
 	psci {
 		compatible = "arm,psci-1.0";
 		method = "smc";
+
+		CPU_PD: cpu-pd@0 {
+			#power-domain-cells = <0>;
+			domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWR_DWN>;
+		};
+
+		domain-states {
+			CLUSTER_RET: domain_ret {
+				compatible = "arm,idle-state";
+				arm,psci-suspend-param = <0x1000010>;
+				entry-latency-us = <500>;
+				exit-latency-us = <500>;
+				min-residency-us = <2000>;
+			};
+
+			CLUSTER_PWR_DWN: domain_gdhs {
+				compatible = "arm,idle-state";
+				arm,psci-suspend-param = <0x1000030>;
+				entry-latency-us = <2000>;
+				exit-latency-us = <2000>;
+				min-residency-us = <6000>;
+			};
+		};
 	};
 
 	timer {
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 15/15] ARM64: dts: Define CPU power domain for MSM8916

[Sudeep Holla]

On 05/08/16 00:05, Lina Iyer wrote:
> Define power domain and the power states for the domain as defined by
> the PSCI firmware.

> The 8916 firmware supports OS initiated method of
> powering off the CPU clusters.

How is that related to the this DTS change, more details below ?

>
> Cc: <devicetree@vger.kernel.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/msm8916.dtsi | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> index 3029773..eb0aaed 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> @@ -64,6 +64,7 @@
>  			next-level-cache = <&L2_0>;
>  			enable-method = "psci";
>  			cpu-idle-states = <&CPU_SPC>;
> +			power-domains = <&CPU_PD>;

This is really messy. We need to have idle state information at one 
place. I prefer to have a hierarchal representation of power-domains
for CPU with idle-states at each level.

>  		};
>
>  		CPU1: cpu@1 {
> @@ -73,6 +74,7 @@
>  			next-level-cache = <&L2_0>;
>  			enable-method = "psci";
>  			cpu-idle-states = <&CPU_SPC>;
> +			power-domains = <&CPU_PD>;
>  		};
>
>  		CPU2: cpu@2 {
> @@ -82,6 +84,7 @@
>  			next-level-cache = <&L2_0>;
>  			enable-method = "psci";
>  			cpu-idle-states = <&CPU_SPC>;
> +			power-domains = <&CPU_PD>;
>  		};
>
>  		CPU3: cpu@3 {
> @@ -91,6 +94,7 @@
>  			next-level-cache = <&L2_0>;
>  			enable-method = "psci";
>  			cpu-idle-states = <&CPU_SPC>;
> +			power-domains = <&CPU_PD>;
>  		};
>
>  		L2_0: l2-cache {
> @@ -113,6 +117,29 @@
>  	psci {
>  		compatible = "arm,psci-1.0";
>  		method = "smc";

Why is it inside PSCI node ? I don't see a need for that.
If it needs to be here, then amend the binding document.

> +
> +		CPU_PD: cpu-pd@0 {
> +			#power-domain-cells = <0>;
> +			domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWR_DWN>;
> +		};
> +
> +		domain-states {
> +			CLUSTER_RET: domain_ret {
> +				compatible = "arm,idle-state";
> +				arm,psci-suspend-param = <0x1000010>;
> +				entry-latency-us = <500>;
> +				exit-latency-us = <500>;
> +				min-residency-us = <2000>;
> +			};
> +
> +			CLUSTER_PWR_DWN: domain_gdhs {
> +				compatible = "arm,idle-state";
> +				arm,psci-suspend-param = <0x1000030>;
> +				entry-latency-us = <2000>;
> +				exit-latency-us = <2000>;
> +				min-residency-us = <6000>;
> +			};
> +		};

So how do you collapse these states into the cpu level states ?
We should be able to cope up with platform co-ordinated mode of idle.
For me, this binding and the representation here is designed only to
address OS co-ordinated mode of idle support but it should be other way
around. Design the bindings that can cater any mode (platform and OS
co-ordinated)

-- 
Regards,
Sudeep

Re: [PATCH v3 15/15] ARM64: dts: Define CPU power domain for MSM8916

[Lina Iyer]

On Wed, Aug 10 2016 at 09:27 -0600, Sudeep Holla wrote:
>
>
>On 05/08/16 00:05, Lina Iyer wrote:
>>Define power domain and the power states for the domain as defined by
>>the PSCI firmware.
>
>>The 8916 firmware supports OS initiated method of
>>powering off the CPU clusters.
>
>How is that related to the this DTS change, more details below ?
>
>>
>>Cc: <devicetree@vger.kernel.org>
>>Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>---
>> arch/arm64/boot/dts/qcom/msm8916.dtsi | 27 +++++++++++++++++++++++++++
>> 1 file changed, 27 insertions(+)
>>
>>diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
>>index 3029773..eb0aaed 100644
>>--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
>>+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
>>@@ -64,6 +64,7 @@
>> 			next-level-cache = <&L2_0>;
>> 			enable-method = "psci";
>> 			cpu-idle-states = <&CPU_SPC>;
>>+			power-domains = <&CPU_PD>;
>
>This is really messy. We need to have idle state information at one 
>place. I prefer to have a hierarchal representation of power-domains
>for CPU with idle-states at each level.
>
>> 		};
I see where are going with that. We need to then isolate idle states
from all devices (including CPU) and put them under the umberella of the
domain/parent idle states.

We also need to remember that domain idle states are not just for CPU
domains. There are generic PM domains that also define their idle
states. For some, that hierarchy may not make sense. So forcing it on
all domains is not correct as well.

>>
>> 		CPU1: cpu@1 {
>>@@ -73,6 +74,7 @@
>> 			next-level-cache = <&L2_0>;
>> 			enable-method = "psci";
>> 			cpu-idle-states = <&CPU_SPC>;
>>+			power-domains = <&CPU_PD>;
>> 		};
>>
>> 		CPU2: cpu@2 {
>>@@ -82,6 +84,7 @@
>> 			next-level-cache = <&L2_0>;
>> 			enable-method = "psci";
>> 			cpu-idle-states = <&CPU_SPC>;
>>+			power-domains = <&CPU_PD>;
>> 		};
>>
>> 		CPU3: cpu@3 {
>>@@ -91,6 +94,7 @@
>> 			next-level-cache = <&L2_0>;
>> 			enable-method = "psci";
>> 			cpu-idle-states = <&CPU_SPC>;
>>+			power-domains = <&CPU_PD>;
>> 		};
>>
>> 		L2_0: l2-cache {
>>@@ -113,6 +117,29 @@
>> 	psci {
>> 		compatible = "arm,psci-1.0";
>> 		method = "smc";
>
>Why is it inside PSCI node ? I don't see a need for that.
>If it needs to be here, then amend the binding document.
>
It is described in patch 13/15.

It is inside PSCI node, because PSCI has the domain controller.

>>+
>>+		CPU_PD: cpu-pd@0 {
>>+			#power-domain-cells = <0>;
>>+			domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWR_DWN>;
>>+		};
>>+
>>+		domain-states {
>>+			CLUSTER_RET: domain_ret {
>>+				compatible = "arm,idle-state";
>>+				arm,psci-suspend-param = <0x1000010>;
>>+				entry-latency-us = <500>;
>>+				exit-latency-us = <500>;
>>+				min-residency-us = <2000>;
>>+			};
>>+
>>+			CLUSTER_PWR_DWN: domain_gdhs {
>>+				compatible = "arm,idle-state";
>>+				arm,psci-suspend-param = <0x1000030>;
>>+				entry-latency-us = <2000>;
>>+				exit-latency-us = <2000>;
>>+				min-residency-us = <6000>;
>>+			};
>>+		};
>
>So how do you collapse these states into the cpu level states ?
Why do you have to collapse?

>We should be able to cope up with platform co-ordinated mode of idle.
>For me, this binding and the representation here is designed only to
>address OS co-ordinated mode of idle support but it should be other way
>around. Design the bindings that can cater any mode (platform and OS
>co-ordinated)
As explained to Brendan on the version2 of the series, OSI and PC are
orthagonal to each other. The idle state definition in the devicetree
exactly matches the unique approaches of these 2 modes.

In platform coordinated, the CPU determines the idle state of the domain
and selects the state, hence it makes sense to extend the
cpu-idle-states to cover those domain states.

In OSI, the CPUs only determine their idle states. When they are done
with their idle state, they bubble up and let the domain choose its idle
state and therefore the domain-idle-states is part of the domain
controller.

With this addition platform coordinated representation is not broken. If
your SoC supports both platform and os modes, then you can specify the
idle states of both of them in the DT. The clause in firmware/psci.c
will however, choose OSI if its available. I am not sure we want to
dynamically switch betweeen OSI and PC at runtime.

Thanks,
Lina

Re: [PATCH v3 15/15] ARM64: dts: Define CPU power domain for MSM8916

[Sudeep Holla]

On 10/08/16 18:35, Lina Iyer wrote:
> On Wed, Aug 10 2016 at 09:27 -0600, Sudeep Holla wrote:
>>
>>
>> On 05/08/16 00:05, Lina Iyer wrote:
>>> Define power domain and the power states for the domain as defined by
>>> the PSCI firmware.
>>
>>> The 8916 firmware supports OS initiated method of
>>> powering off the CPU clusters.
>>
>> How is that related to the this DTS change, more details below ?
>>
>>>
>>> Cc: <devicetree@vger.kernel.org>
>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>> ---
>>> arch/arm64/boot/dts/qcom/msm8916.dtsi | 27 +++++++++++++++++++++++++++
>>> 1 file changed, 27 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi
>>> b/arch/arm64/boot/dts/qcom/msm8916.dtsi
>>> index 3029773..eb0aaed 100644
>>> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
>>> @@ -64,6 +64,7 @@
>>>             next-level-cache = <&L2_0>;
>>>             enable-method = "psci";
>>>             cpu-idle-states = <&CPU_SPC>;
>>> +            power-domains = <&CPU_PD>;
>>
>> This is really messy. We need to have idle state information at one
>> place. I prefer to have a hierarchal representation of power-domains
>> for CPU with idle-states at each level.
>>
>>>         };
> I see where are going with that. We need to then isolate idle states
> from all devices (including CPU) and put them under the umberella of the
> domain/parent idle states.
>
> We also need to remember that domain idle states are not just for CPU
> domains. There are generic PM domains that also define their idle
> states. For some, that hierarchy may not make sense. So forcing it on
> all domains is not correct as well.
>

Yes again I absolutely agree with that. But just represent the hierarchy
present in the hardware in the DT. If it doesn't exist in the hardware
for some device, then it won't be represented.

>>>
>>>         CPU1: cpu@1 {
>>> @@ -73,6 +74,7 @@
>>>             next-level-cache = <&L2_0>;
>>>             enable-method = "psci";
>>>             cpu-idle-states = <&CPU_SPC>;
>>> +            power-domains = <&CPU_PD>;
>>>         };
>>>
>>>         CPU2: cpu@2 {
>>> @@ -82,6 +84,7 @@
>>>             next-level-cache = <&L2_0>;
>>>             enable-method = "psci";
>>>             cpu-idle-states = <&CPU_SPC>;
>>> +            power-domains = <&CPU_PD>;
>>>         };
>>>
>>>         CPU3: cpu@3 {
>>> @@ -91,6 +94,7 @@
>>>             next-level-cache = <&L2_0>;
>>>             enable-method = "psci";
>>>             cpu-idle-states = <&CPU_SPC>;
>>> +            power-domains = <&CPU_PD>;
>>>         };
>>>
>>>         L2_0: l2-cache {
>>> @@ -113,6 +117,29 @@
>>>     psci {
>>>         compatible = "arm,psci-1.0";
>>>         method = "smc";
>>
>> Why is it inside PSCI node ? I don't see a need for that.
>> If it needs to be here, then amend the binding document.
>>
> It is described in patch 13/15.
>
> It is inside PSCI node, because PSCI has the domain controller.
>

OK, I haven't gone through all the patches, I was just interested in
the binding. Sorry for that.

>>> +
>>> +        CPU_PD: cpu-pd@0 {
>>> +            #power-domain-cells = <0>;
>>> +            domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWR_DWN>;
>>> +        };
>>> +
>>> +        domain-states {
>>> +            CLUSTER_RET: domain_ret {
>>> +                compatible = "arm,idle-state";
>>> +                arm,psci-suspend-param = <0x1000010>;
>>> +                entry-latency-us = <500>;
>>> +                exit-latency-us = <500>;
>>> +                min-residency-us = <2000>;
>>> +            };
>>> +
>>> +            CLUSTER_PWR_DWN: domain_gdhs {
>>> +                compatible = "arm,idle-state";
>>> +                arm,psci-suspend-param = <0x1000030>;
>>> +                entry-latency-us = <2000>;
>>> +                exit-latency-us = <2000>;
>>> +                min-residency-us = <6000>;
>>> +            };
>>> +        };
>>
>> So how do you collapse these states into the cpu level states ?
> Why do you have to collapse?
>

To deal with platform co-ordinated mode of cpu idle support. I think I
gave an example in the other email.

>> We should be able to cope up with platform co-ordinated mode of idle.
>> For me, this binding and the representation here is designed only to
>> address OS co-ordinated mode of idle support but it should be other way
>> around. Design the bindings that can cater any mode (platform and OS
>> co-ordinated)
> As explained to Brendan on the version2 of the series, OSI and PC are
> orthagonal to each other. The idle state definition in the devicetree
> exactly matches the unique approaches of these 2 modes.
>

We don't design bindings that depend on the software approaches or
configurations. It should represent the hardware.

> In platform coordinated, the CPU determines the idle state of the domain
> and selects the state, hence it makes sense to extend the
> cpu-idle-states to cover those domain states.
>

So, are you saying that old bindings are for platform co-ordinated mode
and these new ones are for OS co-ordinated mode. Make it clear in the
binding document and get explicit approval from the DT maintainers for
that. I really don't like that approach.

> In OSI, the CPUs only determine their idle states. When they are done
> with their idle state, they bubble up and let the domain choose its idle
> state and therefore the domain-idle-states is part of the domain
> controller.
>

I understand that, but I don't like the approach taken to define the DT
bindings. DT should have one or the other form and any mode can be used
as long as firmware is queried and put into appropriate mode of operation.

> With this addition platform coordinated representation is not broken. If
> your SoC supports both platform and os modes, then you can specify the
> idle states of both of them in the DT. The clause in firmware/psci.c
> will however, choose OSI if its available. I am not sure we want to
> dynamically switch betweeen OSI and PC at runtime.
>

How ? cpu-idle-states in CPU node can't have both:
1. just it's idles states, and
2. complete list of flattened idle states as it' is present today.

-- 
Regards,
Sudeep

Re: [PATCH v3 13/15] dt/bindings: Add PSCI OS-Initiated PM Domains bindings

[Lina Iyer]

+devicetree

On Thu, Aug 04 2016 at 17:06 -0600, Lina Iyer wrote:
>Add bindings for defining a OS-Initiated based CPU PM domain.
>
>Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>---
> Documentation/devicetree/bindings/arm/psci.txt | 64 ++++++++++++++++++++++++++
> 1 file changed, 64 insertions(+)
>
>diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
>index a2c4f1d..e1b2926 100644
>--- a/Documentation/devicetree/bindings/arm/psci.txt
>+++ b/Documentation/devicetree/bindings/arm/psci.txt
>@@ -105,7 +105,71 @@ Case 3: PSCI v0.2 and PSCI v0.1.
> 		...
> 	};
>
>+PSCI v1.0 onwards, supports OS-Initiated mode for powering off CPU domains
>+from the firmware. Such PM domains for which the PSCI firmware driver acts
>+as pseudo-controller, may also be specified in the DT along with the domain
>+idle states.
>+
>+The domain definitions must follow the domain idle state specifications per
>+[3].
>+
>+The domain idle states must be specified using the optional node -
>+
>+- domain-states
>+
>+The domain states themselves must be compatible with 'arm,idle-state' defined
>+in [1]  and need to specify the arm,psci-suspend-param property for each idle
>+state. More information on defining CPU PM domains is available in [4].
>+
>+Example: OS-Iniated PSCI based PM domains
>+
>+	cpus {
>+		#address-cells = <1>;
>+		#size-cells = <0>;
>+
>+		CPU0: cpu@0 {
>+			device_type = "cpu";
>+			compatible = "arm,cortex-a53", "arm,armv8";
>+			reg = <0x0>;
>+			enable-method = "psci";
>+			power-domains = <&CPU_PD>;
>+		};
>+	};
>+
>+	psci {
>+		compatible = "arm,psci-1.0";
>+		method = "smc";
>+
>+		CPU_PD: cpu-pd@0 {
>+			#power-domain-cells = <0>;
>+			domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWR_DWN>;
>+		};
>+
>+		domain-states {
>+			CLUSTER_RET: domain_ret {
>+				compatible = "arm,idle-state";
>+				arm,psci-suspend-param = <0x1000010>;
>+				entry-latency-us = <500>;
>+				exit-latency-us = <500>;
>+				min-residency-us = <2000>;
>+			};
>+
>+			CLUSTER_PWR_DWN: domain_gdhs {
>+				compatible = "arm,idle-state";
>+				arm,psci-suspend-param = <0x1000030>;
>+				entry-latency-us = <2000>;
>+				exit-latency-us = <2000>;
>+				min-residency-us = <6000>;
>+			};
>+		};
>+	};
>+
>+
> [1] Kernel documentation - ARM idle states bindings
>     Documentation/devicetree/bindings/arm/idle-states.txt
> [2] Power State Coordination Interface (PSCI) specification
>     http://infocenter.arm.com/help/topic/com.arm.doc.den0022c/DEN0022C_Power_State_Coordination_Interface.pdf
>+[3]. PM Domains description
>+    Documentation/devicetree/bindings/power/power_domain.txt
>+[4]. CPU PM Domains description
>+    Documentation/power/cpu_domains.txt
>-- 
>2.7.4
>