[PATCH] dt: psci: Update DT bindings to support hierarchical PSCI states

[PATCH] dt: psci: Update DT bindings to support hierarchical PSCI states

[Ulf Hansson]

From: Lina Iyer <lina.iyer@linaro.org>

Update DT bindings to represent hierarchical CPU and CPU domain idle states
for PSCI. Also update the PSCI examples to clearly show how flattened and
hierarchical idle states can be represented in DT.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

For your information, I have picked up the work from Lina Iyer around the so
called CPU cluster idling series [1,2] and I working on new versions. However,
I decided to post the updates to the PSCI DT bindings first, as they will be
needed to be agreed upon before further changes can be done to the PSCI firmware
driver.

Note, these bindings have been discussed over and over again, at LKML, but
especially also at various Linux conferences, like LPC and Linaro Connect. We
finally came to a conclusion and the changes we agreed upon, should be reflected
in this update.

Of course, it's a while ago since the latest discussions, but hopefully people
don't have too hard time to remember.

Kind regards
Uffe

[1]
https://www.spinics.net/lists/arm-kernel/msg566200.html

[2]
https://lwn.net/Articles/716300/

---
 Documentation/devicetree/bindings/arm/psci.txt | 152 +++++++++++++++++++++++++
 1 file changed, 152 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
index a2c4f1d..5a8f11b 100644
--- a/Documentation/devicetree/bindings/arm/psci.txt
+++ b/Documentation/devicetree/bindings/arm/psci.txt
@@ -105,7 +105,159 @@ Case 3: PSCI v0.2 and PSCI v0.1.
 		...
 	};
 
+PSCI v1.0 onwards, supports OS-Initiated mode for powering off CPUs and CPU
+clusters from the firmware. For such topologies the PSCI firmware driver acts
+as pseudo-controller, which may be specified in the psci DT node. The
+definitions of the CPU and the CPU cluster topology, must conform to the domain
+idle state specification [3]. The domain idle states themselves, must be
+compatible with the defined 'domain-idle-state' binding [1], and also need to
+specify the arm,psci-suspend-param property for each idle state.
+
+DT allows representing CPU and CPU cluster idle states in two different ways -
+
+The flattened model as given in Example 1, lists CPU's idle states followed by
+the domain idle state that the CPUs may choose. This is the general practice
+followed in PSCI firmwares that support Platform Coordinated mode. Note that
+the idle states are all compatible with "arm,idle-state".
+
+Example 2 represents the hierarchical model of CPU and domain idle states.
+CPUs define their domain provider in their DT node. The domain controls the
+power to the CPU and possibly other h/w blocks that would be powered off when
+the CPU is powered off. The CPU's idle states may therefore be considered as
+the domain's idle states and have the compatible "arm,idle-state". Such domains
+may be embedded within another domain that represents common h/w blocks between
+these CPUs viz. the cluster. The idle states of the cluster would be
+represented as the domain's idle states. In order to use OS-Initiated mode of
+PSCI in the firmware, the hierarchical representation must be used.
+
+Example 1: Flattened representation of CPU and domain idle states
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		CPU0: cpu@0 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a53", "arm,armv8";
+			reg = <0x0>;
+			enable-method = "psci";
+			cpu-idle-states = <&CPU_PWRDN>, <&CLUSTER_RET>,
+					  <&CLUSTER_PWR_DWN>;
+		};
+
+		CPU1: cpu@1 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a57", "arm,armv8";
+			reg = <0x100>;
+			enable-method = "psci";
+			cpu-idle-states = <&CPU_PWRDN>, <&CLUSTER_RET>,
+					  <&CLUSTER_PWR_DWN>;
+		};
+
+		idle-states {
+			CPU_PWRDN: cpu_power_down{
+				compatible = "arm,idle-state";
+				arm,psci-suspend-param = <0x000001>;
+				entry-latency-us = <10>;
+				exit-latency-us = <10>;
+				min-residency-us = <100>;
+			};
+
+			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_off {
+				compatible = "arm,idle-state";
+				arm,psci-suspend-param = <0x1000030>;
+				entry-latency-us = <2000>;
+				exit-latency-us = <2000>;
+				min-residency-us = <6000>;
+			};
+	};
+
+	psci {
+		compatible = "arm,psci-0.2";
+		method = "smc";
+	};
+
+Example 2: Hierarchical representation of CPU and domain idle states
+
+	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_PD0>;
+		};
+
+		CPU1: cpu@1 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a57", "arm,armv8";
+			reg = <0x100>;
+			enable-method = "psci";
+			power-domains = <&CPU_PD1>;
+		};
+
+		idle-states {
+			CPU_PWRDN: cpu_power_down{
+				compatible = "arm,idle-state";
+				arm,psci-suspend-param = <0x000001>;
+				entry-latency-us = <10>;
+				exit-latency-us = <10>;
+				min-residency-us = <100>;
+			};
+
+			CLUSTER_RET: domain_ret {
+				compatible = "domain-idle-state";
+				arm,psci-suspend-param = <0x1000010>;
+				entry-latency-us = <500>;
+				exit-latency-us = <500>;
+				min-residency-us = <2000>;
+			};
+
+			CLUSTER_PWR_DWN: domain_off {
+				compatible = "domain-idle-state";
+				arm,psci-suspend-param = <0x1000030>;
+				entry-latency-us = <2000>;
+				exit-latency-us = <2000>;
+				min-residency-us = <6000>;
+			};
+		};
+	};
+
+	psci {
+		compatible = "arm,psci-1.0";
+		method = "smc";
+
+		CPU_PD0: cpu-pd@0 {
+			#power-domain-cells = <0>;
+			domain-idle-states = <&CPU_PWRDN>;
+			power-domains = <&CLUSTER_PD>;
+		};
+
+		CPU_PD1: cpu-pd@1 {
+			#power-domain-cells = <0>;
+			domain-idle-states =  <&CPU_PWRDN>;
+			power-domains = <&CLUSTER_PD>;
+		};
+
+		CLUSTER_PD: cluster-pd@0 {
+			#power-domain-cells = <0>;
+			domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWR_DWN>;
+		};
+	};
+
 [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
-- 
2.7.4

Re: [PATCH] dt: psci: Update DT bindings to support hierarchical PSCI states

[Rob Herring]

On Fri, Dec 22, 2017 at 03:32:07PM +0100, Ulf Hansson wrote:
> From: Lina Iyer <lina.iyer-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 
> Update DT bindings to represent hierarchical CPU and CPU domain idle states
> for PSCI. Also update the PSCI examples to clearly show how flattened and
> hierarchical idle states can be represented in DT.
> 
> Signed-off-by: Lina Iyer <lina.iyer-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> 
> For your information, I have picked up the work from Lina Iyer around the so
> called CPU cluster idling series [1,2] and I working on new versions. However,
> I decided to post the updates to the PSCI DT bindings first, as they will be
> needed to be agreed upon before further changes can be done to the PSCI firmware
> driver.
> 
> Note, these bindings have been discussed over and over again, at LKML, but
> especially also at various Linux conferences, like LPC and Linaro Connect. We
> finally came to a conclusion and the changes we agreed upon, should be reflected
> in this update.
> 
> Of course, it's a while ago since the latest discussions, but hopefully people
> don't have too hard time to remember.

Vaguely...

> 
> Kind regards
> Uffe
> 
> [1]
> https://www.spinics.net/lists/arm-kernel/msg566200.html
> 
> [2]
> https://lwn.net/Articles/716300/
> 
> ---
>  Documentation/devicetree/bindings/arm/psci.txt | 152 +++++++++++++++++++++++++
>  1 file changed, 152 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
> index a2c4f1d..5a8f11b 100644
> --- a/Documentation/devicetree/bindings/arm/psci.txt
> +++ b/Documentation/devicetree/bindings/arm/psci.txt
> @@ -105,7 +105,159 @@ Case 3: PSCI v0.2 and PSCI v0.1.
>  		...
>  	};
>  
> +PSCI v1.0 onwards, supports OS-Initiated mode for powering off CPUs and CPU
> +clusters from the firmware. For such topologies the PSCI firmware driver acts
> +as pseudo-controller, which may be specified in the psci DT node. The
> +definitions of the CPU and the CPU cluster topology, must conform to the domain
> +idle state specification [3]. The domain idle states themselves, must be
> +compatible with the defined 'domain-idle-state' binding [1], and also need to
> +specify the arm,psci-suspend-param property for each idle state.
> +
> +DT allows representing CPU and CPU cluster idle states in two different ways -
> +
> +The flattened model as given in Example 1, lists CPU's idle states followed by
> +the domain idle state that the CPUs may choose. This is the general practice
> +followed in PSCI firmwares that support Platform Coordinated mode. Note that
> +the idle states are all compatible with "arm,idle-state".
> +
> +Example 2 represents the hierarchical model of CPU and domain idle states.
> +CPUs define their domain provider in their DT node. The domain controls the
> +power to the CPU and possibly other h/w blocks that would be powered off when
> +the CPU is powered off. The CPU's idle states may therefore be considered as
> +the domain's idle states and have the compatible "arm,idle-state". Such domains
> +may be embedded within another domain that represents common h/w blocks between
> +these CPUs viz. the cluster. The idle states of the cluster would be
> +represented as the domain's idle states. In order to use OS-Initiated mode of
> +PSCI in the firmware, the hierarchical representation must be used.
> +
> +Example 1: Flattened representation of CPU and domain idle states
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		CPU0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a53", "arm,armv8";
> +			reg = <0x0>;
> +			enable-method = "psci";
> +			cpu-idle-states = <&CPU_PWRDN>, <&CLUSTER_RET>,
> +					  <&CLUSTER_PWR_DWN>;
> +		};
> +
> +		CPU1: cpu@1 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a57", "arm,armv8";
> +			reg = <0x100>;
> +			enable-method = "psci";
> +			cpu-idle-states = <&CPU_PWRDN>, <&CLUSTER_RET>,
> +					  <&CLUSTER_PWR_DWN>;
> +		};
> +
> +		idle-states {
> +			CPU_PWRDN: cpu_power_down{

Use '-' rather than '_'. dtc has more warnings since last time...

> +				compatible = "arm,idle-state";
> +				arm,psci-suspend-param = <0x000001>;
> +				entry-latency-us = <10>;
> +				exit-latency-us = <10>;
> +				min-residency-us = <100>;
> +			};
> +
> +			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_off {
> +				compatible = "arm,idle-state";
> +				arm,psci-suspend-param = <0x1000030>;
> +				entry-latency-us = <2000>;
> +				exit-latency-us = <2000>;
> +				min-residency-us = <6000>;
> +			};
> +	};
> +
> +	psci {
> +		compatible = "arm,psci-0.2";
> +		method = "smc";
> +	};
> +
> +Example 2: Hierarchical representation of CPU and domain idle states
> +
> +	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_PD0>;
> +		};
> +
> +		CPU1: cpu@1 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a57", "arm,armv8";
> +			reg = <0x100>;
> +			enable-method = "psci";
> +			power-domains = <&CPU_PD1>;
> +		};
> +
> +		idle-states {
> +			CPU_PWRDN: cpu_power_down{
> +				compatible = "arm,idle-state";
> +				arm,psci-suspend-param = <0x000001>;
> +				entry-latency-us = <10>;
> +				exit-latency-us = <10>;
> +				min-residency-us = <100>;
> +			};
> +
> +			CLUSTER_RET: domain_ret {
> +				compatible = "domain-idle-state";
> +				arm,psci-suspend-param = <0x1000010>;
> +				entry-latency-us = <500>;
> +				exit-latency-us = <500>;
> +				min-residency-us = <2000>;
> +			};
> +
> +			CLUSTER_PWR_DWN: domain_off {
> +				compatible = "domain-idle-state";
> +				arm,psci-suspend-param = <0x1000030>;
> +				entry-latency-us = <2000>;
> +				exit-latency-us = <2000>;
> +				min-residency-us = <6000>;
> +			};
> +		};
> +	};
> +
> +	psci {
> +		compatible = "arm,psci-1.0";
> +		method = "smc";
> +
> +		CPU_PD0: cpu-pd@0 {

A unit address without reg property is now a warning.

> +			#power-domain-cells = <0>;
> +			domain-idle-states = <&CPU_PWRDN>;
> +			power-domains = <&CLUSTER_PD>;
> +		};
> +
> +		CPU_PD1: cpu-pd@1 {
> +			#power-domain-cells = <0>;
> +			domain-idle-states =  <&CPU_PWRDN>;
> +			power-domains = <&CLUSTER_PD>;

Could this node be a child of CLUSTER_PD rather than having a phandle? 
Doesn't matter so much here, but when you have 3 levels?

> +		};

These 2 nodes are identical, so why do you need both?

> +
> +		CLUSTER_PD: cluster-pd@0 {
> +			#power-domain-cells = <0>;
> +			domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWR_DWN>;
> +		};
> +	};
> +
>  [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
> -- 
> 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] dt: psci: Update DT bindings to support hierarchical PSCI states

[Ulf Hansson]

On 26 December 2017 at 23:13, Rob Herring <robh@kernel.org> wrote:
> On Fri, Dec 22, 2017 at 03:32:07PM +0100, Ulf Hansson wrote:
>> From: Lina Iyer <lina.iyer@linaro.org>
>>
>> Update DT bindings to represent hierarchical CPU and CPU domain idle states
>> for PSCI. Also update the PSCI examples to clearly show how flattened and
>> hierarchical idle states can be represented in DT.
>>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>
>> For your information, I have picked up the work from Lina Iyer around the so
>> called CPU cluster idling series [1,2] and I working on new versions. However,
>> I decided to post the updates to the PSCI DT bindings first, as they will be
>> needed to be agreed upon before further changes can be done to the PSCI firmware
>> driver.
>>
>> Note, these bindings have been discussed over and over again, at LKML, but
>> especially also at various Linux conferences, like LPC and Linaro Connect. We
>> finally came to a conclusion and the changes we agreed upon, should be reflected
>> in this update.
>>
>> Of course, it's a while ago since the latest discussions, but hopefully people
>> don't have too hard time to remember.
>
> Vaguely...
>
>>
>> Kind regards
>> Uffe
>>
>> [1]
>> https://www.spinics.net/lists/arm-kernel/msg566200.html
>>
>> [2]
>> https://lwn.net/Articles/716300/
>>
>> ---
>>  Documentation/devicetree/bindings/arm/psci.txt | 152 +++++++++++++++++++++++++
>>  1 file changed, 152 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
>> index a2c4f1d..5a8f11b 100644
>> --- a/Documentation/devicetree/bindings/arm/psci.txt
>> +++ b/Documentation/devicetree/bindings/arm/psci.txt
>> @@ -105,7 +105,159 @@ Case 3: PSCI v0.2 and PSCI v0.1.
>>               ...
>>       };
>>
>> +PSCI v1.0 onwards, supports OS-Initiated mode for powering off CPUs and CPU
>> +clusters from the firmware. For such topologies the PSCI firmware driver acts
>> +as pseudo-controller, which may be specified in the psci DT node. The
>> +definitions of the CPU and the CPU cluster topology, must conform to the domain
>> +idle state specification [3]. The domain idle states themselves, must be
>> +compatible with the defined 'domain-idle-state' binding [1], and also need to
>> +specify the arm,psci-suspend-param property for each idle state.
>> +
>> +DT allows representing CPU and CPU cluster idle states in two different ways -
>> +
>> +The flattened model as given in Example 1, lists CPU's idle states followed by
>> +the domain idle state that the CPUs may choose. This is the general practice
>> +followed in PSCI firmwares that support Platform Coordinated mode. Note that
>> +the idle states are all compatible with "arm,idle-state".
>> +
>> +Example 2 represents the hierarchical model of CPU and domain idle states.
>> +CPUs define their domain provider in their DT node. The domain controls the
>> +power to the CPU and possibly other h/w blocks that would be powered off when
>> +the CPU is powered off. The CPU's idle states may therefore be considered as
>> +the domain's idle states and have the compatible "arm,idle-state". Such domains
>> +may be embedded within another domain that represents common h/w blocks between
>> +these CPUs viz. the cluster. The idle states of the cluster would be
>> +represented as the domain's idle states. In order to use OS-Initiated mode of
>> +PSCI in the firmware, the hierarchical representation must be used.
>> +
>> +Example 1: Flattened representation of CPU and domain idle states
>> +     cpus {
>> +             #address-cells = <1>;
>> +             #size-cells = <0>;
>> +
>> +             CPU0: cpu@0 {
>> +                     device_type = "cpu";
>> +                     compatible = "arm,cortex-a53", "arm,armv8";
>> +                     reg = <0x0>;
>> +                     enable-method = "psci";
>> +                     cpu-idle-states = <&CPU_PWRDN>, <&CLUSTER_RET>,
>> +                                       <&CLUSTER_PWR_DWN>;
>> +             };
>> +
>> +             CPU1: cpu@1 {
>> +                     device_type = "cpu";
>> +                     compatible = "arm,cortex-a57", "arm,armv8";
>> +                     reg = <0x100>;
>> +                     enable-method = "psci";
>> +                     cpu-idle-states = <&CPU_PWRDN>, <&CLUSTER_RET>,
>> +                                       <&CLUSTER_PWR_DWN>;
>> +             };
>> +
>> +             idle-states {
>> +                     CPU_PWRDN: cpu_power_down{
>
> Use '-' rather than '_'. dtc has more warnings since last time...

Yes, I found this on a couple of more places and fixed them all.

>
>> +                             compatible = "arm,idle-state";
>> +                             arm,psci-suspend-param = <0x000001>;
>> +                             entry-latency-us = <10>;
>> +                             exit-latency-us = <10>;
>> +                             min-residency-us = <100>;
>> +                     };
>> +
>> +                     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_off {
>> +                             compatible = "arm,idle-state";
>> +                             arm,psci-suspend-param = <0x1000030>;
>> +                             entry-latency-us = <2000>;
>> +                             exit-latency-us = <2000>;
>> +                             min-residency-us = <6000>;
>> +                     };
>> +     };
>> +
>> +     psci {
>> +             compatible = "arm,psci-0.2";
>> +             method = "smc";
>> +     };
>> +
>> +Example 2: Hierarchical representation of CPU and domain idle states
>> +
>> +     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_PD0>;
>> +             };
>> +
>> +             CPU1: cpu@1 {
>> +                     device_type = "cpu";
>> +                     compatible = "arm,cortex-a57", "arm,armv8";
>> +                     reg = <0x100>;
>> +                     enable-method = "psci";
>> +                     power-domains = <&CPU_PD1>;
>> +             };
>> +
>> +             idle-states {
>> +                     CPU_PWRDN: cpu_power_down{
>> +                             compatible = "arm,idle-state";
>> +                             arm,psci-suspend-param = <0x000001>;
>> +                             entry-latency-us = <10>;
>> +                             exit-latency-us = <10>;
>> +                             min-residency-us = <100>;
>> +                     };
>> +
>> +                     CLUSTER_RET: domain_ret {
>> +                             compatible = "domain-idle-state";
>> +                             arm,psci-suspend-param = <0x1000010>;
>> +                             entry-latency-us = <500>;
>> +                             exit-latency-us = <500>;
>> +                             min-residency-us = <2000>;
>> +                     };
>> +
>> +                     CLUSTER_PWR_DWN: domain_off {
>> +                             compatible = "domain-idle-state";
>> +                             arm,psci-suspend-param = <0x1000030>;
>> +                             entry-latency-us = <2000>;
>> +                             exit-latency-us = <2000>;
>> +                             min-residency-us = <6000>;
>> +                     };
>> +             };
>> +     };
>> +
>> +     psci {
>> +             compatible = "arm,psci-1.0";
>> +             method = "smc";
>> +
>> +             CPU_PD0: cpu-pd@0 {
>
> A unit address without reg property is now a warning.

There is currently no need for reg property, so let me replace this with:

CPU_PD0: cpu-pd0

>
>> +                     #power-domain-cells = <0>;
>> +                     domain-idle-states = <&CPU_PWRDN>;
>> +                     power-domains = <&CLUSTER_PD>;
>> +             };
>> +
>> +             CPU_PD1: cpu-pd@1 {
>> +                     #power-domain-cells = <0>;
>> +                     domain-idle-states =  <&CPU_PWRDN>;
>> +                     power-domains = <&CLUSTER_PD>;
>
> Could this node be a child of CLUSTER_PD rather than having a phandle?
> Doesn't matter so much here, but when you have 3 levels?

This follows existing bindings for power-domains as per
Documentation/devicetree/bindings/power/power_domain.txt. I guess it's
better to stick to that!?

>
>> +             };
>
> These 2 nodes are identical, so why do you need both?

Each node represents a power domain, which contains devices (in this
case CPUs) that are specific to each power domain.

In other words, we are trying to describe the HW using a hierarchical layout.

>
>> +
>> +             CLUSTER_PD: cluster-pd@0 {
>> +                     #power-domain-cells = <0>;
>> +                     domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWR_DWN>;
>> +             };
>> +     };
>> +
>>  [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
>> --
>> 2.7.4
>>

Kind regards
Uffe