[v2,02/14] dt/bindings: update binding for PM domain idle states

Commit Message

Lina Iyer July 29, 2016, 9:56 p.m. UTC

From: Axel Haslam <[e-mail]>

Update DT bindings to describe idle states of PM domains.

Cc: <[e-mail]>
Signed-off-by: Marc Titinger <[e-mail]>
Signed-off-by: Lina Iyer <[e-mail]>
[Lina: Added state properties, removed state names, wakeup-latency,
added of_pm_genpd_init() API, pruned commit text]
Signed-off-by: Ulf Hansson <[e-mail]>
[Ulf: Moved around code to make it compile properly, rebased on top of multiple state support]
---
 .../devicetree/bindings/power/power_domain.txt     | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)

Comments

Rob Herring (Arm) Aug. 1, 2016, 4:30 p.m. UTC | #1

On Fri, Jul 29, 2016 at 03:56:13PM -0600, Lina Iyer wrote:
> From: Axel Haslam <[e-mail]>
> 
> Update DT bindings to describe idle states of PM domains.
> 
> Cc: <[e-mail]>
> Signed-off-by: Marc Titinger <[e-mail]>
> Signed-off-by: Lina Iyer <[e-mail]>
> [Lina: Added state properties, removed state names, wakeup-latency,
> added of_pm_genpd_init() API, pruned commit text]
> Signed-off-by: Ulf Hansson <[e-mail]>
> [Ulf: Moved around code to make it compile properly, rebased on top of multiple state support]
> ---
>  .../devicetree/bindings/power/power_domain.txt     | 39 ++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt
> index 025b5e7..69aa4e2 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 {
> @@ -55,6 +59,39 @@ Example 2:
>  		#power-domain-cells = <1>;
>  	};
>  
> +Example 3:
> +
> +	pm-domains {
> +		a57_pd: a57_pd@ {

The trailing '@' is not valid. If dtc doesn't complain about that, it 
should.

> +			/* will have a57 platform ARM_PD_METHOD_OF_DECLARE*/
> +			compatible = "arm,pd","arm,cortex-a57";
> +			#power-domain-cells = <0>;
> +			idle-states = <&CLUSTER_SLEEP_0>;

Is this supposed to be 'domain-idle-states'? The domain part is 
pointless IMO given these are power domain nodes.

> +		};
> +
> +		a53_pd: a53_pd@ {
> +			/* will have a a53 platform ARM_PD_METHOD_OF_DECLARE*/
> +			compatible = "arm,pd","arm,cortex-a53";
> +			#power-domain-cells = <0>;
> +			idle-states = <&CLUSTER_SLEEP_0>, <&CLUSTER_SLEEP_1>;
> +		};
> +
> +		CLUSTER_SLEEP_0: idle-state@0 {

A unit-address should have a matching reg value or be dropped. A reg 
property would be fine here, but I think it should correspond to MPIDR 
values. 

> +			compatible = "arm,idle-state";
> +			entry-latency-us = <1000>;
> +			exit-latency-us = <2000>;
> +			residency-us = <10000>;
> +		};
> +
> +		CLUSTER_SLEEP_1: idle-state@1 {
> +			compatible = "arm,idle-state";
> +			entry-latency-us = <5000>;
> +			exit-latency-us = <5000>;
> +			residency-us = <100000>;
> +		};
> +	};
> +
> +
>  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.
> @@ -76,3 +113,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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lina Iyer Aug. 1, 2016, 9 p.m. UTC | #2

On Mon, Aug 01 2016 at 10:30 -0600, Rob Herring wrote:
>On Fri, Jul 29, 2016 at 03:56:13PM -0600, Lina Iyer wrote:
>> From: Axel Haslam <[e-mail]>
>>
>> Update DT bindings to describe idle states of PM domains.
>>
>> Cc: <[e-mail]>
>> Signed-off-by: Marc Titinger <[e-mail]>
>> Signed-off-by: Lina Iyer <[e-mail]>
>> [Lina: Added state properties, removed state names, wakeup-latency,
>> added of_pm_genpd_init() API, pruned commit text]
>> Signed-off-by: Ulf Hansson <[e-mail]>
>> [Ulf: Moved around code to make it compile properly, rebased on top of multiple state support]
>> ---
>>  .../devicetree/bindings/power/power_domain.txt     | 39 ++++++++++++++++++++++
>>  1 file changed, 39 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt
>> index 025b5e7..69aa4e2 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 {
>> @@ -55,6 +59,39 @@ Example 2:
>>  		#power-domain-cells = <1>;
>>  	};
>>
>> +Example 3:
>> +
>> +	pm-domains {
>> +		a57_pd: a57_pd@ {
>
>The trailing '@' is not valid. If dtc doesn't complain about that, it
>should.
>
Will remove it.

>> +			/* will have a57 platform ARM_PD_METHOD_OF_DECLARE*/
>> +			compatible = "arm,pd","arm,cortex-a57";
>> +			#power-domain-cells = <0>;
>> +			idle-states = <&CLUSTER_SLEEP_0>;
>
>Is this supposed to be 'domain-idle-states'? The domain part is
>pointless IMO given these are power domain nodes.
>
It should domain-idle-states property. Well, the CPU's idle states are
called cpu-idle-states in the CPU node, so I named this
domain-idle-states to be in line.


>> +		};
>> +
>> +		a53_pd: a53_pd@ {
>> +			/* will have a a53 platform ARM_PD_METHOD_OF_DECLARE*/
>> +			compatible = "arm,pd","arm,cortex-a53";
>> +			#power-domain-cells = <0>;
>> +			idle-states = <&CLUSTER_SLEEP_0>, <&CLUSTER_SLEEP_1>;
>> +		};
>> +
>> +		CLUSTER_SLEEP_0: idle-state@0 {
>
>A unit-address should have a matching reg value or be dropped. A reg
>property would be fine here, but I think it should correspond to MPIDR
>values.
>
Will drop it in my next submission.

Thanks Rob.

-- Lina

>> +			compatible = "arm,idle-state";
>> +			entry-latency-us = <1000>;
>> +			exit-latency-us = <2000>;
>> +			residency-us = <10000>;
>> +		};
>> +
>> +		CLUSTER_SLEEP_1: idle-state@1 {
>> +			compatible = "arm,idle-state";
>> +			entry-latency-us = <5000>;
>> +			exit-latency-us = <5000>;
>> +			residency-us = <100000>;
>> +		};
>> +	};
>> +
>> +
>>  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.
>> @@ -76,3 +113,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
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Brendan Jackman Aug. 4, 2016, 3:24 p.m. UTC | #3

Hi Lina,

These bindings are the reason for my interest in this patchset; I'm hoping to be
able to do some work based on them in order to generically describe the cost of
idle states for use in the Energy Aware Scheduling (EAS)[1] energy model.

Mark Rutland expressed concern [2] in the thread for the previous version of
this patchset that there are now two possible locations for the list of idle
states; that hasn't been addressed. My own instinct is that this is OK: in the
real world, power domain (e.g. cluster) idle states are a property of the power
domain and not of the CPU it contains - the DT should reflect this.

However, since there are existing platform DTs with cluster-level suspend states
(which are platform-coordinated rather than OS-initiated) in cpu-idle-states, do
we have a backwards-compatibility issue? e.g. say we have a platform with a DT
like this:

	cpu@0 {
		/*...*/
		cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
	};

	idle-states {
		CPU_SLEEP: cpu-sleep {
			/*...*/
		};
		CLUSTER_SLEEP: cluster-sleep {
			/*...*/
		};
	};

and in order to enable OS-initiated cluster suspend it changes to this:

	cpu@0 {
		/*...*/
		cpu-idle-states = <&CPU_SLEEP>;
		power-domains = <CPU_PD>;
	};

	idle-states {
		CPU_SLEEP: cpu-sleep {
			/*...*/
		};
	};

	/*... elsewhere ... */

	CLUSTER_SLEEP: cluster-sleep {
		/*...*/
	};

	CPU_PD {
	/*...*/
		idle-states = <&CLUSTER_SLEEP>;
	};

Then old kernels which don't have CPU PM Domains will lose the ability to
suspend clusters. I've phrased this as a question because I'm not clear on what
we require in terms of backwards/forwards compatibility with DTs - excuse my
ignorance. What are your thoughts on this?

A couple of notes:

On Fri, Jul 29, 2016 at 03:56:13PM -0600, Lina Iyer wrote:
> +Example 3:
> +
> +	pm-domains {
> +		a57_pd: a57_pd@ {
> +			/* will have a57 platform ARM_PD_METHOD_OF_DECLARE*/
> +			compatible = "arm,pd","arm,cortex-a57";
> +			#power-domain-cells = <0>;
> +			idle-states = <&CLUSTER_SLEEP_0>;
> +		};
> +
> +		a53_pd: a53_pd@ {
> +			/* will have a a53 platform ARM_PD_METHOD_OF_DECLARE*/
> +			compatible = "arm,pd","arm,cortex-a53";
> +			#power-domain-cells = <0>;
> +			idle-states = <&CLUSTER_SLEEP_0>, <&CLUSTER_SLEEP_1>;
> +		};
> +
> +		CLUSTER_SLEEP_0: idle-state@0 {
> +			compatible = "arm,idle-state";
> +			entry-latency-us = <1000>;
> +			exit-latency-us = <2000>;
> +			residency-us = <10000>;
> +		};
> +
> +		CLUSTER_SLEEP_1: idle-state@1 {
> +			compatible = "arm,idle-state";
> +			entry-latency-us = <5000>;
> +			exit-latency-us = <5000>;
> +			residency-us = <100000>;
> +		};

I'm confused about the location of the idle state nodes. In this example,
they're under the pm-domains node which seems wrong to me. In your later patch
for msm8916.dsti they come under cpu-domain-states. I'm inexperienced here so
please excuse me again if I'm being ignorant.

idle-states.txt (to which this file refers) says that idle state nodes must come
under /cpus/idle-states. I don't think power domain idle states belong there, so
the documentation should be updated to reflect that.

> +	};
> +
> +
>  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.

This block refers to Example 2 - the hunk you added should be below.

[1] https://lwn.net/Articles/650426/
[2] https://patchwork.kernel.org/patch/9193651/

Regards,
Brendan

Brendan Jackman Aug. 4, 2016, 3:29 p.m. UTC | #4

Hi Lina,

Sorry, forgot to mention this in my other email:

On Fri, Jul 29, 2016 at 03:56:13PM -0600, Lina Iyer wrote:
>
> +Example 3:
> +
> +	pm-domains {
> +		a57_pd: a57_pd@ {
> +			/* will have a57 platform ARM_PD_METHOD_OF_DECLARE*/
> +			compatible = "arm,pd","arm,cortex-a57";
> +			#power-domain-cells = <0>;
> +			idle-states = <&CLUSTER_SLEEP_0>;
> +		};
> +
> +		a53_pd: a53_pd@ {
> +			/* will have a a53 platform ARM_PD_METHOD_OF_DECLARE*/
> +			compatible = "arm,pd","arm,cortex-a53";
> +			#power-domain-cells = <0>;
> +			idle-states = <&CLUSTER_SLEEP_0>, <&CLUSTER_SLEEP_1>;
> +		};
> +
> +		CLUSTER_SLEEP_0: idle-state@0 {
> +			compatible = "arm,idle-state";
> +			entry-latency-us = <1000>;
> +			exit-latency-us = <2000>;
> +			residency-us = <10000>;

Existing bindings for CPU idle states use "min-residency-us" instead of
"residency-us" here.

> +		};
> +
> +		CLUSTER_SLEEP_1: idle-state@1 {
> +			compatible = "arm,idle-state";
> +			entry-latency-us = <5000>;
> +			exit-latency-us = <5000>;
> +			residency-us = <100000>;
> +		};
> +	};

Regards,
Brendan

Lina Iyer Aug. 4, 2016, 4:28 p.m. UTC | #5

Hi Brenden,

On Thu, Aug 04 2016 at 09:24 -0600, Brendan Jackman wrote:
>Hi Lina,
>
>These bindings are the reason for my interest in this patchset; I'm hoping to be
>able to do some work based on them in order to generically describe the cost of
>idle states for use in the Energy Aware Scheduling (EAS)[1] energy model.
>
I think that's a fair idea - idle states accounting their own cost.

>Mark Rutland expressed concern [2] in the thread for the previous version of
>this patchset that there are now two possible locations for the list of idle
>states; that hasn't been addressed. My own instinct is that this is OK: in the
>real world, power domain (e.g. cluster) idle states are a property of the power
>domain and not of the CPU it contains - the DT should reflect this.
>
Absolutely.

>However, since there are existing platform DTs with cluster-level suspend states
>(which are platform-coordinated rather than OS-initiated) in cpu-idle-states, do
>we have a backwards-compatibility issue? e.g. say we have a platform with a DT
>like this:
>
Your concern is very valid and this is the exactly the difference
between Platform coordinated (PC) mode and OS-Initiated (OSI) mode. In
PC, the domain state is an extension of the CPU state and rightful place
for that is the cpu-idle-states property. Just like the example you
have.

>	cpu@0 {
>		/*...*/
>		cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
>	};
>
>	idle-states {
>		CPU_SLEEP: cpu-sleep {
>			/*...*/
>		};
>		CLUSTER_SLEEP: cluster-sleep {
>			/*...*/
>		};
>	};
>
>and in order to enable OS-initiated cluster suspend it changes to this:
>
Platforms that have OSI will have this format you mention below. If the
platform supports the OSI it will respond to the PSCI_FEATURES and
PSCI_SET_SUSPEND mode (patch 10 of this series). If the OSI mode is
available snd if the DT has the domains defined for the CPU, then the
OSI mode is chosen otherwise, it reverts to using PC mode. This code
snippet from my patches does exactly that -

if (psci_has_osi_pd) {
        int ret;
        const struct cpu_pd_ops psci_pd_ops = {
                .populate_state_data = psci_pd_populate_state_data,
                .power_off = psci_pd_power_off,
        };

        ret = of_setup_cpu_pd_single(cpu, &psci_pd_ops);
        if (!ret)
                ret = psci_set_suspend_mode_osi(true);
        if (ret)
                pr_warn("CPU%d: Error setting PSCI OSI mode\n", cpu);
}

>	cpu@0 {
>		/*...*/
>		cpu-idle-states = <&CPU_SLEEP>;
>		power-domains = <CPU_PD>;
>	};
>
>	idle-states {
>		CPU_SLEEP: cpu-sleep {
>			/*...*/
>		};
>	};
>
>	/*... elsewhere ... */
>
>	CLUSTER_SLEEP: cluster-sleep {
>		/*...*/
>	};
>
>	CPU_PD {
>	/*...*/
>		idle-states = <&CLUSTER_SLEEP>;
>	};
>
>Then old kernels which don't have CPU PM Domains will lose the ability to
>suspend clusters. I've phrased this as a question because I'm not clear on what
>we require in terms of backwards/forwards compatibility with DTs - excuse my
>ignorance. What are your thoughts on this?
>
So, if the DT has only support for cluster modes in cpu-idle-states and
not the OSI specific representation, then it would continue to use only
PC mode to power down the cluster, even though the firmware may have
been updated to support OSI.

That means, all the existing platforms will continue to work the way
they do even with these patches in place.

Moreover, the way the PSCI state ids are for PC and OS intiated fall in
line with how we represent in the DT. PC cluster states are represented
in the original format and the OSI follow the extended state format. The
composite is made by an OR of the CPU state and the cluster idle state.

>A couple of notes:
>
>On Fri, Jul 29, 2016 at 03:56:13PM -0600, Lina Iyer wrote:
>> +Example 3:
>> +
>> +	pm-domains {
>> +		a57_pd: a57_pd@ {
>> +			/* will have a57 platform ARM_PD_METHOD_OF_DECLARE*/
>> +			compatible = "arm,pd","arm,cortex-a57";
>> +			#power-domain-cells = <0>;
>> +			idle-states = <&CLUSTER_SLEEP_0>;
>> +		};
>> +
>> +		a53_pd: a53_pd@ {
>> +			/* will have a a53 platform ARM_PD_METHOD_OF_DECLARE*/
>> +			compatible = "arm,pd","arm,cortex-a53";
>> +			#power-domain-cells = <0>;
>> +			idle-states = <&CLUSTER_SLEEP_0>, <&CLUSTER_SLEEP_1>;
>> +		};
>> +
>> +		CLUSTER_SLEEP_0: idle-state@0 {
>> +			compatible = "arm,idle-state";
>> +			entry-latency-us = <1000>;
>> +			exit-latency-us = <2000>;
>> +			residency-us = <10000>;
>> +		};
>> +
>> +		CLUSTER_SLEEP_1: idle-state@1 {
>> +			compatible = "arm,idle-state";
>> +			entry-latency-us = <5000>;
>> +			exit-latency-us = <5000>;
>> +			residency-us = <100000>;
>> +		};
>
>I'm confused about the location of the idle state nodes. In this example,
>they're under the pm-domains node which seems wrong to me. In your later patch
>for msm8916.dsti they come under cpu-domain-states. I'm inexperienced here so
>please excuse me again if I'm being ignorant.
>
Valid question. These patches are intended to support CPU domains that
are controlled by Linux (ARM v7 style) and the PSCI way as well. The
example here is for a ARM v7 style domain where the domain controller
code exists in Linux.

The way my patches evolve the kernel is build up a generic CPU PM
domains framework and then PSCI firmware becomes a driver for platforms
that support OSI and the DT has domain definitions.

Your confusion should be resolved, if you look at the broader scope of
the CPU domain - domains that may be controlled in Linux and domains
outside Linux.

>idle-states.txt (to which this file refers) says that idle state nodes must come
>under /cpus/idle-states. I don't think power domain idle states belong there, so
>the documentation should be updated to reflect that.
>
Absolutely right. This should be fixed.

>> +	};
>> +
>> +
>>  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.
>
>This block refers to Example 2 - the hunk you added should be below.
>
OK. Thanks.

-- Lina

>[1] https://lwn.net/Articles/650426/
>[2] https://patchwork.kernel.org/patch/9193651/
>
>Regards,
>Brendan

Brendan Jackman Aug. 4, 2016, 6:15 p.m. UTC | #6

On Thu, Aug 04, 2016 at 10:28:44AM -0600, Lina Iyer wrote:
> Hi Brenden,
>
> On Thu, Aug 04 2016 at 09:24 -0600, Brendan Jackman wrote:
> >Hi Lina,
> >
> >These bindings are the reason for my interest in this patchset; I'm hoping to be
> >able to do some work based on them in order to generically describe the cost of
> >idle states for use in the Energy Aware Scheduling (EAS)[1] energy model.
> >
> I think that's a fair idea - idle states accounting their own cost.
>
> >Mark Rutland expressed concern [2] in the thread for the previous version of
> >this patchset that there are now two possible locations for the list of idle
> >states; that hasn't been addressed. My own instinct is that this is OK: in the
> >real world, power domain (e.g. cluster) idle states are a property of the power
> >domain and not of the CPU it contains - the DT should reflect this.
> >
> Absolutely.
>
> >However, since there are existing platform DTs with cluster-level suspend states
> >(which are platform-coordinated rather than OS-initiated) in cpu-idle-states, do
> >we have a backwards-compatibility issue? e.g. say we have a platform with a DT
> >like this:
> >
> Your concern is very valid and this is the exactly the difference
> between Platform coordinated (PC) mode and OS-Initiated (OSI) mode. In
> PC, the domain state is an extension of the CPU state and rightful place
> for that is the cpu-idle-states property. Just like the example you
> have.
>
> >	cpu@0 {
> >		/*...*/
> >		cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
> >	};
> >
> >	idle-states {
> >		CPU_SLEEP: cpu-sleep {
> >			/*...*/
> >		};
> >		CLUSTER_SLEEP: cluster-sleep {
> >			/*...*/
> >		};
> >	};
> >
> >and in order to enable OS-initiated cluster suspend it changes to this:
> >
> Platforms that have OSI will have this format you mention below. If the
> platform supports the OSI it will respond to the PSCI_FEATURES and
> PSCI_SET_SUSPEND mode (patch 10 of this series). If the OSI mode is
> available snd if the DT has the domains defined for the CPU, then the
> OSI mode is chosen otherwise, it reverts to using PC mode. This code
> snippet from my patches does exactly that -
>
> if (psci_has_osi_pd) {
>        int ret;
>        const struct cpu_pd_ops psci_pd_ops = {
>                .populate_state_data = psci_pd_populate_state_data,
>                .power_off = psci_pd_power_off,
>        };
>
>        ret = of_setup_cpu_pd_single(cpu, &psci_pd_ops);
>        if (!ret)
>                ret = psci_set_suspend_mode_osi(true);
>        if (ret)
>                pr_warn("CPU%d: Error setting PSCI OSI mode\n", cpu);
> }
>
> >	cpu@0 {
> >		/*...*/
> >		cpu-idle-states = <&CPU_SLEEP>;
> >		power-domains = <CPU_PD>;
> >	};
> >
> >	idle-states {
> >		CPU_SLEEP: cpu-sleep {
> >			/*...*/
> >		};
> >	};
> >
> >	/*... elsewhere ... */
> >
> >	CLUSTER_SLEEP: cluster-sleep {
> >		/*...*/
> >	};
> >
> >	CPU_PD {
> >	/*...*/
> >		idle-states = <&CLUSTER_SLEEP>;
> >	};
> >
> >Then old kernels which don't have CPU PM Domains will lose the ability to
> >suspend clusters. I've phrased this as a question because I'm not clear on what
> >we require in terms of backwards/forwards compatibility with DTs - excuse my
> >ignorance. What are your thoughts on this?
> >
> So, if the DT has only support for cluster modes in cpu-idle-states and
> not the OSI specific representation, then it would continue to use only
> PC mode to power down the cluster, even though the firmware may have
> been updated to support OSI.
>
> That means, all the existing platforms will continue to work the way
> they do even with these patches in place.
>
> Moreover, the way the PSCI state ids are for PC and OS intiated fall in
> line with how we represent in the DT. PC cluster states are represented
> in the original format and the OSI follow the extended state format. The
> composite is made by an OR of the CPU state and the cluster idle state.
>

OK, this makes sense - I understand that these patches will not affect the
behaviour if the DT stays the same. My question, though is what happens when a
new DT with the new OSI structure is given to an older kernel without these
patches applied.

Example: right now we support PC cluster suspend on the Juno platform (see
juno*.dts). Let's say Juno's firmware comes to support OSI suspend and we want
to use that in Linux. We apply these patches then update the .dts, adding a CPU
power domain tree, removing CLUSTER_SLEEP_0 from cpu-idle-states and adding it
to the relevant power domain node's idle-states. Now we have OSI suspend on
Juno. But then if we take our new DTB and feed it to a v4.7 kernel it will not
be able to enter CLUSTER_SLEEP_0 because it is not in cpu-idle-states. Before we
modified the DTB, v4.7 kernels were capable of entering CLUSTER_SLEEP_0 in PC
mode.

Does that make sense - do we expect newer DTBs to be compatible with older
kernels, and if so how can we add OSI support to existing platforms without
breaking older kernels?

Thanks,
Brendan

Lina Iyer Aug. 4, 2016, 7:02 p.m. UTC | #7

On Thu, Aug 04 2016 at 12:15 -0600, Brendan Jackman wrote:
>On Thu, Aug 04, 2016 at 10:28:44AM -0600, Lina Iyer wrote:
>> Hi Brenden,
>>
>> On Thu, Aug 04 2016 at 09:24 -0600, Brendan Jackman wrote:

[...]

>> >Then old kernels which don't have CPU PM Domains will lose the ability to
>> >suspend clusters. I've phrased this as a question because I'm not clear on what
>> >we require in terms of backwards/forwards compatibility with DTs - excuse my
>> >ignorance. What are your thoughts on this?
>> >
>> So, if the DT has only support for cluster modes in cpu-idle-states and
>> not the OSI specific representation, then it would continue to use only
>> PC mode to power down the cluster, even though the firmware may have
>> been updated to support OSI.
>>
>> That means, all the existing platforms will continue to work the way
>> they do even with these patches in place.
>>
>> Moreover, the way the PSCI state ids are for PC and OS intiated fall in
>> line with how we represent in the DT. PC cluster states are represented
>> in the original format and the OSI follow the extended state format. The
>> composite is made by an OR of the CPU state and the cluster idle state.
>>
>
>OK, this makes sense - I understand that these patches will not affect the
>behaviour if the DT stays the same. My question, though is what happens when a
>new DT with the new OSI structure is given to an older kernel without these
>patches applied.
>
>Example: right now we support PC cluster suspend on the Juno platform (see
>juno*.dts). Let's say Juno's firmware comes to support OSI suspend and we want
>to use that in Linux. We apply these patches then update the .dts, adding a CPU
>power domain tree, removing CLUSTER_SLEEP_0 from cpu-idle-states and adding it
>to the relevant power domain node's idle-states. Now we have OSI suspend on
>Juno. But then if we take our new DTB and feed it to a v4.7 kernel it will not
>be able to enter CLUSTER_SLEEP_0 because it is not in cpu-idle-states. Before we
>modified the DTB, v4.7 kernels were capable of entering CLUSTER_SLEEP_0 in PC
>mode.
>
>Does that make sense - do we expect newer DTBs to be compatible with older
>kernels, and if so how can we add OSI support to existing platforms without
>breaking older kernels?
>
I don't think it is a fair requirement to have newer DTB's run on older
kernels. But hypothetically, if you were to run the newer DTB with OSI
domains on a 4.7 kernel, it would NOT do cluster low power states, but
it would not fail because of OSI nodes. They will just be ignored.
Cluster low modes will not also happen, since you wouldn't have the
cpu-idle-states appending cluster modes after the CPU's modes.

Thanks,
Lina

Lina Iyer Aug. 4, 2016, 9:23 p.m. UTC | #8

On Thu, Aug 04 2016 at 10:28 -0600, Lina Iyer wrote:
>On Thu, Aug 04 2016 at 09:24 -0600, Brendan Jackman wrote:

>>idle-states.txt (to which this file refers) says that idle state nodes must come
>>under /cpus/idle-states. I don't think power domain idle states belong there, so
>>the documentation should be updated to reflect that.
>>
>Absolutely right. This should be fixed.

Sorry, I take it back. idle-states are cpuidle specific idle states and
possibly include Platform coordinated cluster modes as well (which is
fine). Its rightful place is under /cpus/idle-states. However, domain
idle states that are compatible with "arm,idle-state" would not have
idle-states as the parent node. They would be defined under the domain
provider node and not the CPUs.

Thanks,
Lina

Patch

diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt
index 025b5e7..69aa4e2 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 {
@@ -55,6 +59,39 @@  Example 2:
 		#power-domain-cells = <1>;
 	};
 
+Example 3:
+
+	pm-domains {
+		a57_pd: a57_pd@ {
+			/* will have a57 platform ARM_PD_METHOD_OF_DECLARE*/
+			compatible = "arm,pd","arm,cortex-a57";
+			#power-domain-cells = <0>;
+			idle-states = <&CLUSTER_SLEEP_0>;
+		};
+
+		a53_pd: a53_pd@ {
+			/* will have a a53 platform ARM_PD_METHOD_OF_DECLARE*/
+			compatible = "arm,pd","arm,cortex-a53";
+			#power-domain-cells = <0>;
+			idle-states = <&CLUSTER_SLEEP_0>, <&CLUSTER_SLEEP_1>;
+		};
+
+		CLUSTER_SLEEP_0: idle-state@0 {
+			compatible = "arm,idle-state";
+			entry-latency-us = <1000>;
+			exit-latency-us = <2000>;
+			residency-us = <10000>;
+		};
+
+		CLUSTER_SLEEP_1: idle-state@1 {
+			compatible = "arm,idle-state";
+			entry-latency-us = <5000>;
+			exit-latency-us = <5000>;
+			residency-us = <100000>;
+		};
+	};
+
+
 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.
@@ -76,3 +113,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