[PATCH v5 02/16] dt/bindings: Update binding for PM domain idle states

[PATCH v5 02/16] 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 v5 02/16] dt/bindings: Update binding for PM domain idle states

[Sudeep Holla]

On 26/08/16 21:17, Lina Iyer wrote:
> From: Axel Haslam <ahaslam+renesas-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>
> Update DT bindings to describe idle states of PM domains.
>
> Cc: <devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Signed-off-by: Marc Titinger <mtitinger+renesas-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Lina Iyer <lina.iyer-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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>;
> +		};
> +	};
> +

This version is *not very descriptive*. Also the discussion we had on v3
version has not yet concluded IMO. So can I take that we agreed on what
was proposed there or not ?

We could have better example above *really* based on the discussions we
had so far. This example always makes me think it's well crafted to
avoid any sort of discussions. We need to consider different use-cases
e.g. what about CPU level states ?

IMO, we need to discuss this DT binding in detail and arrive at some
conclusion before you take all the troubles to respin the series.
Also it's better to keep the DT binding separate until we have some
conclusion instead of posting the implementation for each version.
That's just my opinion(I would be least bothered about implementation
until I know it will be accepted before I can peek into the code, others
may differ.

-- 
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 v5 02/16] dt/bindings: Update binding for PM domain idle states

[Lina Iyer]

On Fri, Sep 02 2016 at 07:21 -0700, Sudeep Holla wrote:
>
>
>On 26/08/16 21:17, 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].
>>+
>> 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>;
>>+		};
>>+	};
>>+
>
>This version is *not very descriptive*. Also the discussion we had on v3
>version has not yet concluded IMO. So can I take that we agreed on what
>was proposed there or not ?
>
Sorry, this example is not very descriptive. Pls. check the 8916 dtsi
for the new changes in the following patches. Let me know if that makes
sense.

Thanks,
Lina

>We could have better example above *really* based on the discussions we
>had so far. This example always makes me think it's well crafted to
>avoid any sort of discussions. We need to consider different use-cases
>e.g. what about CPU level states ?
>
>IMO, we need to discuss this DT binding in detail and arrive at some
>conclusion before you take all the troubles to respin the series.
>Also it's better to keep the DT binding separate until we have some
>conclusion instead of posting the implementation for each version.
>That's just my opinion(I would be least bothered about implementation
>until I know it will be accepted before I can peek into the code, others
>may differ.
>
>-- 
>Regards,
>Sudeep

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

[Brendan Jackman]

Hi Lina,

Sorry for the delay here, Sudeep and I were both been on holiday last week.

On Fri, Sep 02 2016 at 21:16, Lina Iyer wrote:
> On Fri, Sep 02 2016 at 07:21 -0700, Sudeep Holla wrote:
[...]
>>This version is *not very descriptive*. Also the discussion we had on v3
>>version has not yet concluded IMO. So can I take that we agreed on what
>>was proposed there or not ?
>>
> Sorry, this example is not very descriptive. Pls. check the 8916 dtsi
> for the new changes in the following patches. Let me know if that makes
> sense.

The not-yet-concluded discussion Sudeep is referring to is at [1].

In that thread we initially proposed the idea of, instead of splitting
state phandles between cpu-idle-states and domain-idle-states, putting
CPUs in their own domains and using domain-idle-states for _all_
phandles, deprecating cpu-idle-states. I've brought this up in other
threads [2] but discussion keeps petering out, and neither this example
nor the 8916 dtsi in this patch series reflect the idea.

It would be great if we could go back to the thread at [1] where Sudeep
has posted examples and come to a clear consensus on the binding design
before reviewing implementation patches. Ideally with input from Ulf,
Rob and Kevin.

[1] https://patchwork.kernel.org/patch/9264507
[2] http://www.spinics.net/lists/devicetree/msg141024.html
>
> Thanks,
> Lina
>
>>We could have better example above *really* based on the discussions we
>>had so far. This example always makes me think it's well crafted to
>>avoid any sort of discussions. We need to consider different use-cases
>>e.g. what about CPU level states ?
>>
>>IMO, we need to discuss this DT binding in detail and arrive at someq
>>conclusion before you take all the troubles to respin the series.
>>Also it's better to keep the DT binding separate until we have some
>>conclusion instead of posting the implementation for each version.
>>That's just my opinion(I would be least bothered about implementation
>>until I know it will be accepted before I can peek into the code, others
>>may differ.
>>
>>--
>>Regards,
>>Sudeep

Cheers,
Brendan
--
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 v5 02/16] dt/bindings: Update binding for PM domain idle states

[Lina Iyer]

On Mon, Sep 12 2016 at 09:19 -0600, Brendan Jackman wrote:
>
>Hi Lina,
>
>Sorry for the delay here, Sudeep and I were both been on holiday last week.
>
>On Fri, Sep 02 2016 at 21:16, Lina Iyer wrote:
>> On Fri, Sep 02 2016 at 07:21 -0700, Sudeep Holla wrote:
>[...]
>>>This version is *not very descriptive*. Also the discussion we had on v3
>>>version has not yet concluded IMO. So can I take that we agreed on what
>>>was proposed there or not ?
>>>
>> Sorry, this example is not very descriptive. Pls. check the 8916 dtsi
>> for the new changes in the following patches. Let me know if that makes
>> sense.
>
>The not-yet-concluded discussion Sudeep is referring to is at [1].
>
>In that thread we initially proposed the idea of, instead of splitting
>state phandles between cpu-idle-states and domain-idle-states, putting
>CPUs in their own domains and using domain-idle-states for _all_
>phandles, deprecating cpu-idle-states. I've brought this up in other
>threads [2] but discussion keeps petering out, and neither this example
>nor the 8916 dtsi in this patch series reflect the idea.
>
Brendan, while your idea is good and will work for CPUs, I do not expect
other domains and possibly CPU domains on some architectures to follow
this model. There is nothing that prevents you from doing this today,
you can specify domains around CPUs in your devicetree and CPU PM will
handle the hierarchy. I don't think its fair to force it on all SoCs
using CPU domains. This patchset does not restrict you from organizing
the idle states the way you want it. This revision of the series, clubs
CPU and domain idle states under idle-states umbrella. So part of your
requirement is also satisfied.

You can follow up the series with your new additions, I don't see a
conflict with this change.

Thanks,
Lina


>It would be great if we could go back to the thread at [1] where Sudeep
>has posted examples and come to a clear consensus on the binding design
>before reviewing implementation patches. Ideally with input from Ulf,
>Rob and Kevin.
>
>[1] https://patchwork.kernel.org/patch/9264507
>[2] http://www.spinics.net/lists/devicetree/msg141024.html
>>
>> Thanks,
>> Lina
>>
>>>We could have better example above *really* based on the discussions we
>>>had so far. This example always makes me think it's well crafted to
>>>avoid any sort of discussions. We need to consider different use-cases
>>>e.g. what about CPU level states ?
>>>
>>>IMO, we need to discuss this DT binding in detail and arrive at someq
>>>conclusion before you take all the troubles to respin the series.
>>>Also it's better to keep the DT binding separate until we have some
>>>conclusion instead of posting the implementation for each version.
>>>That's just my opinion(I would be least bothered about implementation
>>>until I know it will be accepted before I can peek into the code, others
>>>may differ.
>>>
>>>--
>>>Regards,
>>>Sudeep
>
>Cheers,
>Brendan

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

[Sudeep Holla]

On 12/09/16 17:16, Lina Iyer wrote:
> On Mon, Sep 12 2016 at 09:19 -0600, Brendan Jackman wrote:
>>
>> Hi Lina,
>>
>> Sorry for the delay here, Sudeep and I were both been on holiday last
>> week.
>>
>> On Fri, Sep 02 2016 at 21:16, Lina Iyer wrote:
>>> On Fri, Sep 02 2016 at 07:21 -0700, Sudeep Holla wrote:
>> [...]
>>>> This version is *not very descriptive*. Also the discussion we had
>>>> on v3
>>>> version has not yet concluded IMO. So can I take that we agreed on what
>>>> was proposed there or not ?
>>>>
>>> Sorry, this example is not very descriptive. Pls. check the 8916 dtsi
>>> for the new changes in the following patches. Let me know if that makes
>>> sense.

Please add all possible use-cases in the bindings. Though one can refer
the usage examples, it might not cover all usage descriptions. It helps
preventing people from defining their own when they don't see examples.
Again DT bindings are like specifications, it should be descriptive
especially this kind of generic ones.

>>
>> The not-yet-concluded discussion Sudeep is referring to is at [1].
>>
>> In that thread we initially proposed the idea of, instead of splitting
>> state phandles between cpu-idle-states and domain-idle-states, putting
>> CPUs in their own domains and using domain-idle-states for _all_
>> phandles, deprecating cpu-idle-states. I've brought this up in other
>> threads [2] but discussion keeps petering out, and neither this example
>> nor the 8916 dtsi in this patch series reflect the idea.
>>
> Brendan, while your idea is good and will work for CPUs, I do not expect
> other domains and possibly CPU domains on some architectures to follow
> this model. There is nothing that prevents you from doing this today,
> you can specify domains around CPUs in your devicetree and CPU PM will
> handle the hierarchy. I don't think its fair to force it on all SoCs
> using CPU domains.

I disagree. We are defining DT bindings here and it *should* be same for
all the SoC unless there is a compelling reason not to. I am fine if
those reasons are stated and agreed.

> This patchset does not restrict you from organizing
> the idle states the way you want it. This revision of the series, clubs
> CPU and domain idle states under idle-states umbrella. So part of your
> requirement is also satisfied.
>

I will look at the DTS changes in the series. But we *must* have more
description with more examples in the binding document.

> You can follow up the series with your new additions, I don't see a
> conflict with this change.
>

If we just need additions, then it should be fine.

-- 
Regards,
Sudeep

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

[Brendan Jackman]

On Mon, Sep 12 2016 at 18:09, Sudeep Holla wrote:
> On 12/09/16 17:16, Lina Iyer wrote:
>> On Mon, Sep 12 2016 at 09:19 -0600, Brendan Jackman wrote:
>>>
>>> Hi Lina,
>>>
>>> Sorry for the delay here, Sudeep and I were both been on holiday last
>>> week.
>>>
>>> On Fri, Sep 02 2016 at 21:16, Lina Iyer wrote:
>>>> On Fri, Sep 02 2016 at 07:21 -0700, Sudeep Holla wrote:
>>> [...]
>>>>> This version is *not very descriptive*. Also the discussion we had
>>>>> on v3
>>>>> version has not yet concluded IMO. So can I take that we agreed on what
>>>>> was proposed there or not ?
>>>>>
>>>> Sorry, this example is not very descriptive. Pls. check the 8916 dtsi
>>>> for the new changes in the following patches. Let me know if that makes
>>>> sense.
>
> Please add all possible use-cases in the bindings. Though one can refer
> the usage examples, it might not cover all usage descriptions. It helps
> preventing people from defining their own when they don't see examples.
> Again DT bindings are like specifications, it should be descriptive
> especially this kind of generic ones.
>
>>>
>>> The not-yet-concluded discussion Sudeep is referring to is at [1].
>>>
>>> In that thread we initially proposed the idea of, instead of splitting
>>> state phandles between cpu-idle-states and domain-idle-states, putting
>>> CPUs in their own domains and using domain-idle-states for _all_
>>> phandles, deprecating cpu-idle-states. I've brought this up in other
>>> threads [2] but discussion keeps petering out, and neither this example
>>> nor the 8916 dtsi in this patch series reflect the idea.
>>>
>> Brendan, while your idea is good and will work for CPUs, I do not expect
>> other domains and possibly CPU domains on some architectures to follow
>> this model. There is nothing that prevents you from doing this today,

As I understand it your opposition to this approach is this:

There may be devices/CPUs which have idle states which do not constitute
"power off". If we put those  devices in their own power domain for the
purpose of putting their (non-power-off) idle state phandles in
domain-idle-states, we are "lying" because no true power domain exists
there.

Am I correct that that's your opposition?

If so, it seems we essentially disagree on the definition of a power
domain, i.e. you define it as a set of devices that are powered on/off
together while I define it as a set of devices whose power states
(including idle states, not just on/off) are tied together. I said
something similar on another thread [1] which died out.

Do you agree that this is basically where we disagree, or am I missing
something else?

[2] http://www.spinics.net/lists/devicetree/msg141050.html

>> you can specify domains around CPUs in your devicetree and CPU PM will
>> handle the hierarchy. I don't think its fair to force it on all SoCs
>> using CPU domains.
>
> I disagree. We are defining DT bindings here and it *should* be same for
> all the SoC unless there is a compelling reason not to. I am fine if
> those reasons are stated and agreed.
>
>> This patchset does not restrict you from organizing
>> the idle states the way you want it. This revision of the series, clubs
>> CPU and domain idle states under idle-states umbrella. So part of your
>> requirement is also satisfied.
>>
>
> I will look at the DTS changes in the series. But we *must* have more
> description with more examples in the binding document.
>
>> You can follow up the series with your new additions, I don't see a
>> conflict with this change.
>>
>
> If we just need additions, then it should be fine.
--
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 v5 02/16] dt/bindings: Update binding for PM domain idle states

[Lina Iyer]

On Tue, Sep 13 2016 at 11:50 -0600, Brendan Jackman wrote:
>
>On Mon, Sep 12 2016 at 18:09, Sudeep Holla wrote:
>> On 12/09/16 17:16, Lina Iyer wrote:
>>> On Mon, Sep 12 2016 at 09:19 -0600, Brendan Jackman wrote:
>>>>
>>>> Hi Lina,
>>>>
>>>> Sorry for the delay here, Sudeep and I were both been on holiday last
>>>> week.
>>>>
>>>> On Fri, Sep 02 2016 at 21:16, Lina Iyer wrote:
>>>>> On Fri, Sep 02 2016 at 07:21 -0700, Sudeep Holla wrote:
>>>> [...]
>>>>>> This version is *not very descriptive*. Also the discussion we had
>>>>>> on v3
>>>>>> version has not yet concluded IMO. So can I take that we agreed on what
>>>>>> was proposed there or not ?
>>>>>>
>>>>> Sorry, this example is not very descriptive. Pls. check the 8916 dtsi
>>>>> for the new changes in the following patches. Let me know if that makes
>>>>> sense.
>>
>> Please add all possible use-cases in the bindings. Though one can refer
>> the usage examples, it might not cover all usage descriptions. It helps
>> preventing people from defining their own when they don't see examples.
>> Again DT bindings are like specifications, it should be descriptive
>> especially this kind of generic ones.
>>
>>>>
>>>> The not-yet-concluded discussion Sudeep is referring to is at [1].
>>>>
>>>> In that thread we initially proposed the idea of, instead of splitting
>>>> state phandles between cpu-idle-states and domain-idle-states, putting
>>>> CPUs in their own domains and using domain-idle-states for _all_
>>>> phandles, deprecating cpu-idle-states. I've brought this up in other
>>>> threads [2] but discussion keeps petering out, and neither this example
>>>> nor the 8916 dtsi in this patch series reflect the idea.
>>>>
>>> Brendan, while your idea is good and will work for CPUs, I do not expect
>>> other domains and possibly CPU domains on some architectures to follow
>>> this model. There is nothing that prevents you from doing this today,
>
>As I understand it your opposition to this approach is this:
>
>There may be devices/CPUs which have idle states which do not constitute
>"power off". If we put those  devices in their own power domain for the
>purpose of putting their (non-power-off) idle state phandles in
>domain-idle-states, we are "lying" because no true power domain exists
>there.
>
>Am I correct that that's your opposition?
>
>If so, it seems we essentially disagree on the definition of a power
>domain, i.e. you define it as a set of devices that are powered on/off
>together while I define it as a set of devices whose power states
>(including idle states, not just on/off) are tied together. I said
>something similar on another thread [1] which died out.
>
>Do you agree that this is basically where we disagree, or am I missing
>something else?
>
>[2] http://www.spinics.net/lists/devicetree/msg141050.html
>
Yes, you are right, I disagree with the definition of a domain around a
device. However, as long as you don't force SoC's to define devices in
the CPU PM domain to have their own virtual domains, I have no problem.
You are welcome to define it the way you want for Juno or any other
platform. I don't want that to be the forced and expected out of all
SoCs. All I am saying here is that the current implementation would
handle your case as well.

Thanks,
Lina

>>> you can specify domains around CPUs in your devicetree and CPU PM will
>>> handle the hierarchy. I don't think its fair to force it on all SoCs
>>> using CPU domains.
>>
>> I disagree. We are defining DT bindings here and it *should* be same for
>> all the SoC unless there is a compelling reason not to. I am fine if
>> those reasons are stated and agreed.
>>
>>> This patchset does not restrict you from organizing
>>> the idle states the way you want it. This revision of the series, clubs
>>> CPU and domain idle states under idle-states umbrella. So part of your
>>> requirement is also satisfied.
>>>
>>
>> I will look at the DTS changes in the series. But we *must* have more
>> description with more examples in the binding document.
>>
>>> You can follow up the series with your new additions, I don't see a
>>> conflict with this change.
>>>
>>
>> If we just need additions, then it should be fine.

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

[Brendan Jackman]

On Tue, Sep 13 2016 at 20:38, Lina Iyer wrote:
> On Tue, Sep 13 2016 at 11:50 -0600, Brendan Jackman wrote:
>>
>>On Mon, Sep 12 2016 at 18:09, Sudeep Holla wrote:
>>> On 12/09/16 17:16, Lina Iyer wrote:
>>>> On Mon, Sep 12 2016 at 09:19 -0600, Brendan Jackman wrote:
>>>>>
>>>>> Hi Lina,
>>>>>
>>>>> Sorry for the delay here, Sudeep and I were both been on holiday last
>>>>> week.
>>>>>
>>>>> On Fri, Sep 02 2016 at 21:16, Lina Iyer wrote:
>>>>>> On Fri, Sep 02 2016 at 07:21 -0700, Sudeep Holla wrote:
>>>>> [...]
>>>>>>> This version is *not very descriptive*. Also the discussion we had
>>>>>>> on v3
>>>>>>> version has not yet concluded IMO. So can I take that we agreed on what
>>>>>>> was proposed there or not ?
>>>>>>>
>>>>>> Sorry, this example is not very descriptive. Pls. check the 8916 dtsi
>>>>>> for the new changes in the following patches. Let me know if that makes
>>>>>> sense.
>>>
>>> Please add all possible use-cases in the bindings. Though one can refer
>>> the usage examples, it might not cover all usage descriptions. It helps
>>> preventing people from defining their own when they don't see examples.
>>> Again DT bindings are like specifications, it should be descriptive
>>> especially this kind of generic ones.
>>>
>>>>>
>>>>> The not-yet-concluded discussion Sudeep is referring to is at [1].
>>>>>
>>>>> In that thread we initially proposed the idea of, instead of splitting
>>>>> state phandles between cpu-idle-states and domain-idle-states, putting
>>>>> CPUs in their own domains and using domain-idle-states for _all_
>>>>> phandles, deprecating cpu-idle-states. I've brought this up in other
>>>>> threads [2] but discussion keeps petering out, and neither this example
>>>>> nor the 8916 dtsi in this patch series reflect the idea.
>>>>>
>>>> Brendan, while your idea is good and will work for CPUs, I do not expect
>>>> other domains and possibly CPU domains on some architectures to follow
>>>> this model. There is nothing that prevents you from doing this today,
>>
>>As I understand it your opposition to this approach is this:
>>
>>There may be devices/CPUs which have idle states which do not constitute
>>"power off". If we put those  devices in their own power domain for the
>>purpose of putting their (non-power-off) idle state phandles in
>>domain-idle-states, we are "lying" because no true power domain exists
>>there.
>>
>>Am I correct that that's your opposition?
>>
>>If so, it seems we essentially disagree on the definition of a power
>>domain, i.e. you define it as a set of devices that are powered on/off
>>together while I define it as a set of devices whose power states
>>(including idle states, not just on/off) are tied together. I said
>>something similar on another thread [1] which died out.
>>
>>Do you agree that this is basically where we disagree, or am I missing
>>something else?
>>
>>[2] http://www.spinics.net/lists/devicetree/msg141050.html
>>
> Yes, you are right, I disagree with the definition of a domain around a
> device.
OK, great.
> However, as long as you don't force SoC's to define devices in
> the CPU PM domain to have their own virtual domains, I have no problem.
> You are welcome to define it the way you want for Juno or any other
> platform.
I don't think that's true; the bindings have to work the same way for
all platforms. If for Juno we put CPU idle state phandles in a
domain-idle-states property for per-CPU domains then, with the current
implementation, the CPU-level idle states would be duplicated between
cpuidle and the CPU PM domains.
> I don't want that to be the forced and expected out of all
> SoCs. All I am saying here is that the current implementation would
> handle your case as well.

The current implementation certainly does cover the work I want to
do. The suggestion of per-device power domains for devices/CPUs with
their own idle states is simply intended to minimise the binding design,
since we'd no longer need cpu-idle-states or device-idle-states
(the latter was proposed elsewhere).

I am fine with the bindings as they are implemented currently so long
as:

- The binding doc makes clear how idle state phandles should be split
  between cpu-idle-states and domain-idle-states. It should make it
  obvious that no phandle should ever appear in both properties. It
  would even be worth briefly going over the backward-compatibility
  implications (e.g. what happens with old-kernel/new-DT and
  new-kernel/old-DT combos if a platform has OSI and PC support and we
  move cluster-level idle state phandles out of cpu-idle-states and into
  domai-idle-states).

- We have a reason against the definition of power domains as "a set of
  devices bound by a common power (including idle) state", since that
  definition would simplify the bindings. In my view, "nobody thinks
  that's what a power domain is" _is_ a compelling reason, so if others
  on the list get involved I'm convinced. I think I speak for Sudeep
  here too.

Cheers,
Brendan

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

[Ulf Hansson]

>>>
>> Yes, you are right, I disagree with the definition of a domain around a
>> device.

To fill in, I agree with Lina (and Kevin).

>From my point of view, a domain is per definition containing resources
which are shared among devices. Having one device per domain, does in
general not make sense.

> OK, great.
>> However, as long as you don't force SoC's to define devices in
>> the CPU PM domain to have their own virtual domains, I have no problem.
>> You are welcome to define it the way you want for Juno or any other
>> platform.
> I don't think that's true; the bindings have to work the same way for
> all platforms. If for Juno we put CPU idle state phandles in a
> domain-idle-states property for per-CPU domains then, with the current
> implementation, the CPU-level idle states would be duplicated between
> cpuidle and the CPU PM domains.
>> I don't want that to be the forced and expected out of all
>> SoCs. All I am saying here is that the current implementation would
>> handle your case as well.
>
> The current implementation certainly does cover the work I want to
> do. The suggestion of per-device power domains for devices/CPUs with
> their own idle states is simply intended to minimise the binding design,
> since we'd no longer need cpu-idle-states or device-idle-states
> (the latter was proposed elsewhere).

I see your point, but IMHO that would be to simplify the description
of the hardware. And I don't think it's sufficient to cover all
existing cases.

>
> I am fine with the bindings as they are implemented currently so long
> as:
>
> - The binding doc makes clear how idle state phandles should be split
>   between cpu-idle-states and domain-idle-states. It should make it
>   obvious that no phandle should ever appear in both properties. It
>   would even be worth briefly going over the backward-compatibility
>   implications (e.g. what happens with old-kernel/new-DT and
>   new-kernel/old-DT combos if a platform has OSI and PC support and we
>   move cluster-level idle state phandles out of cpu-idle-states and into
>   domai-idle-states).
>
> - We have a reason against the definition of power domains as "a set of
>   devices bound by a common power (including idle) state", since that
>   definition would simplify the bindings. In my view, "nobody thinks
>   that's what a power domain is" _is_ a compelling reason, so if others
>   on the list get involved I'm convinced. I think I speak for Sudeep
>   here too.
>

>From a CPU point of view, I think it may very well be considered as
any other device. Yes, we have treated them in a specific manner
regarding the idle state definitions we currently have - and we can
continue to do that.

Although, in the long run, I think we needs something more flexible
that can be used for domains and devices.

Kind regards
Uffe
--
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 v5 02/16] dt/bindings: Update binding for PM domain idle states

[Lina Iyer]

On Wed, Sep 14 2016 at 04:18 -0600, Brendan Jackman wrote:
>
>On Tue, Sep 13 2016 at 20:38, Lina Iyer wrote:
>> On Tue, Sep 13 2016 at 11:50 -0600, Brendan Jackman wrote:
>>>
>>>On Mon, Sep 12 2016 at 18:09, Sudeep Holla wrote:
>>>> On 12/09/16 17:16, Lina Iyer wrote:
>>>>> On Mon, Sep 12 2016 at 09:19 -0600, Brendan Jackman wrote:
>>>>>>
>>>>>> Hi Lina,
>>>>>>
>>>>>> Sorry for the delay here, Sudeep and I were both been on holiday last
>>>>>> week.
>>>>>>
>>>>>> On Fri, Sep 02 2016 at 21:16, Lina Iyer wrote:
>>>>>>> On Fri, Sep 02 2016 at 07:21 -0700, Sudeep Holla wrote:
>>>>>> [...]
>>>>>>>> This version is *not very descriptive*. Also the discussion we had
>>>>>>>> on v3
>>>>>>>> version has not yet concluded IMO. So can I take that we agreed on what
>>>>>>>> was proposed there or not ?
>>>>>>>>
>>>>>>> Sorry, this example is not very descriptive. Pls. check the 8916 dtsi
>>>>>>> for the new changes in the following patches. Let me know if that makes
>>>>>>> sense.
>>>>
>>>> Please add all possible use-cases in the bindings. Though one can refer
>>>> the usage examples, it might not cover all usage descriptions. It helps
>>>> preventing people from defining their own when they don't see examples.
>>>> Again DT bindings are like specifications, it should be descriptive
>>>> especially this kind of generic ones.
>>>>
>>>>>>
>>>>>> The not-yet-concluded discussion Sudeep is referring to is at [1].
>>>>>>
>>>>>> In that thread we initially proposed the idea of, instead of splitting
>>>>>> state phandles between cpu-idle-states and domain-idle-states, putting
>>>>>> CPUs in their own domains and using domain-idle-states for _all_
>>>>>> phandles, deprecating cpu-idle-states. I've brought this up in other
>>>>>> threads [2] but discussion keeps petering out, and neither this example
>>>>>> nor the 8916 dtsi in this patch series reflect the idea.
>>>>>>
>>>>> Brendan, while your idea is good and will work for CPUs, I do not expect
>>>>> other domains and possibly CPU domains on some architectures to follow
>>>>> this model. There is nothing that prevents you from doing this today,
>>>
>>>As I understand it your opposition to this approach is this:
>>>
>>>There may be devices/CPUs which have idle states which do not constitute
>>>"power off". If we put those  devices in their own power domain for the
>>>purpose of putting their (non-power-off) idle state phandles in
>>>domain-idle-states, we are "lying" because no true power domain exists
>>>there.
>>>
>>>Am I correct that that's your opposition?
>>>
>>>If so, it seems we essentially disagree on the definition of a power
>>>domain, i.e. you define it as a set of devices that are powered on/off
>>>together while I define it as a set of devices whose power states
>>>(including idle states, not just on/off) are tied together. I said
>>>something similar on another thread [1] which died out.
>>>
>>>Do you agree that this is basically where we disagree, or am I missing
>>>something else?
>>>
>>>[2] http://www.spinics.net/lists/devicetree/msg141050.html
>>>
>> Yes, you are right, I disagree with the definition of a domain around a
>> device.
>OK, great.
>> However, as long as you don't force SoC's to define devices in
>> the CPU PM domain to have their own virtual domains, I have no problem.
>> You are welcome to define it the way you want for Juno or any other
>> platform.
>I don't think that's true; the bindings have to work the same way for
>all platforms. If for Juno we put CPU idle state phandles in a
>domain-idle-states property for per-CPU domains then, with the current
>implementation, the CPU-level idle states would be duplicated between
>cpuidle and the CPU PM domains.

We don't have the code today. Your patches would add the functionality
of parsing domain idle states and attaching them to cpu-idle-states if
the firmware support and the mode is Platform-coordinated. And that
functionality is an easy addition. Nobody is making this change to
platforms with PC to use the CPU PM domains yet. 

What you are referring to is just a convergence PC and OSI to use
the same domain hierarchy. This definition is not impacted by your 
desire. I have my own doubts of defining PC domains this way, but I
would leave that to you to submit the relevant RFC and bring forth the
discussion. (Per DT, the definition of PC domain states is already
immutable from how it is defined today in DT. You have to be careful in
breaking it up.)

>> I don't want that to be the forced and expected out of all
>> SoCs. All I am saying here is that the current implementation would
>> handle your case as well.
>
>The current implementation certainly does cover the work I want to
>do. The suggestion of per-device power domains for devices/CPUs with
>their own idle states is simply intended to minimise the binding design,
>since we'd no longer need cpu-idle-states or device-idle-states
>(the latter was proposed elsewhere).
>
>I am fine with the bindings as they are implemented currently so long
>as:
>
>- The binding doc makes clear how idle state phandles should be split
>  between cpu-idle-states and domain-idle-states. It should make it
>  obvious that no phandle should ever appear in both properties. It
>  would even be worth briefly going over the backward-compatibility
>  implications (e.g. what happens with old-kernel/new-DT and
>  new-kernel/old-DT combos if a platform has OSI and PC support and we
>  move cluster-level idle state phandles out of cpu-idle-states and into
>  domai-idle-states).
>
Since, I have been only defining OSI initiated PM domains, this is not a
problem. I have clearly distinguished the explanation to be OSI
specific, for now.

>- We have a reason against the definition of power domains as "a set of
>  devices bound by a common power (including idle) state", since that
>  definition would simplify the bindings. In my view, "nobody thinks
>  that's what a power domain is" _is_ a compelling reason, so if others
>  on the list get involved I'm convinced. I think I speak for Sudeep
>  here too.
>

Look outside the context of the CPU - a generic PM domain is collective
of generic devices that share the same power island. A PM Domain may
also have other domains as sub-domains as well. So it is exactly that.
A CPU is just a specialized device.

Hope this helps.

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 v5 02/16] dt/bindings: Update binding for PM domain idle states

[Kevin Hilman]

Brendan Jackman <brendan.jackman@arm.com> writes:

> On Tue, Sep 13 2016 at 20:38, Lina Iyer wrote:
>> On Tue, Sep 13 2016 at 11:50 -0600, Brendan Jackman wrote:
>>>
>>>On Mon, Sep 12 2016 at 18:09, Sudeep Holla wrote:
>>>> On 12/09/16 17:16, Lina Iyer wrote:
>>>>> On Mon, Sep 12 2016 at 09:19 -0600, Brendan Jackman wrote:
>>>>>>
>>>>>> Hi Lina,
>>>>>>
>>>>>> Sorry for the delay here, Sudeep and I were both been on holiday last
>>>>>> week.
>>>>>>
>>>>>> On Fri, Sep 02 2016 at 21:16, Lina Iyer wrote:
>>>>>>> On Fri, Sep 02 2016 at 07:21 -0700, Sudeep Holla wrote:
>>>>>> [...]
>>>>>>>> This version is *not very descriptive*. Also the discussion we had
>>>>>>>> on v3
>>>>>>>> version has not yet concluded IMO. So can I take that we agreed on what
>>>>>>>> was proposed there or not ?
>>>>>>>>
>>>>>>> Sorry, this example is not very descriptive. Pls. check the 8916 dtsi
>>>>>>> for the new changes in the following patches. Let me know if that makes
>>>>>>> sense.
>>>>
>>>> Please add all possible use-cases in the bindings. Though one can refer
>>>> the usage examples, it might not cover all usage descriptions. It helps
>>>> preventing people from defining their own when they don't see examples.
>>>> Again DT bindings are like specifications, it should be descriptive
>>>> especially this kind of generic ones.
>>>>
>>>>>>
>>>>>> The not-yet-concluded discussion Sudeep is referring to is at [1].
>>>>>>
>>>>>> In that thread we initially proposed the idea of, instead of splitting
>>>>>> state phandles between cpu-idle-states and domain-idle-states, putting
>>>>>> CPUs in their own domains and using domain-idle-states for _all_
>>>>>> phandles, deprecating cpu-idle-states. I've brought this up in other
>>>>>> threads [2] but discussion keeps petering out, and neither this example
>>>>>> nor the 8916 dtsi in this patch series reflect the idea.
>>>>>>
>>>>> Brendan, while your idea is good and will work for CPUs, I do not expect
>>>>> other domains and possibly CPU domains on some architectures to follow
>>>>> this model. There is nothing that prevents you from doing this today,
>>>
>>>As I understand it your opposition to this approach is this:
>>>
>>>There may be devices/CPUs which have idle states which do not constitute
>>>"power off". If we put those  devices in their own power domain for the
>>>purpose of putting their (non-power-off) idle state phandles in
>>>domain-idle-states, we are "lying" because no true power domain exists
>>>there.
>>>
>>>Am I correct that that's your opposition?
>>>
>>>If so, it seems we essentially disagree on the definition of a power
>>>domain, i.e. you define it as a set of devices that are powered on/off
>>>together while I define it as a set of devices whose power states
>>>(including idle states, not just on/off) are tied together. I said
>>>something similar on another thread [1] which died out.
>>>
>>>Do you agree that this is basically where we disagree, or am I missing
>>>something else?
>>>
>>>[2] http://www.spinics.net/lists/devicetree/msg141050.html
>>>
>> Yes, you are right, I disagree with the definition of a domain around a
>> device.
> OK, great.
>> However, as long as you don't force SoC's to define devices in
>> the CPU PM domain to have their own virtual domains, I have no problem.
>> You are welcome to define it the way you want for Juno or any other
>> platform.
> I don't think that's true; the bindings have to work the same way for
> all platforms. If for Juno we put CPU idle state phandles in a
> domain-idle-states property for per-CPU domains then, with the current
> implementation, the CPU-level idle states would be duplicated between
> cpuidle and the CPU PM domains.
>> I don't want that to be the forced and expected out of all
>> SoCs. All I am saying here is that the current implementation would
>> handle your case as well.
>
> The current implementation certainly does cover the work I want to
> do. The suggestion of per-device power domains for devices/CPUs with
> their own idle states is simply intended to minimise the binding design,
> since we'd no longer need cpu-idle-states or device-idle-states
> (the latter was proposed elsewhere).
>
> I am fine with the bindings as they are implemented currently so long
> as:
>
> - The binding doc makes clear how idle state phandles should be split
>   between cpu-idle-states and domain-idle-states. It should make it
>   obvious that no phandle should ever appear in both properties. It
>   would even be worth briefly going over the backward-compatibility
>   implications (e.g. what happens with old-kernel/new-DT and
>   new-kernel/old-DT combos if a platform has OSI and PC support and we
>   move cluster-level idle state phandles out of cpu-idle-states and into
>   domai-idle-states).
>
> - We have a reason against the definition of power domains as "a set of
>   devices bound by a common power (including idle) state", since that
>   definition would simplify the bindings. In my view, "nobody thinks
>   that's what a power domain is" _is_ a compelling reason, so if others
>   on the list get involved I'm convinced. I think I speak for Sudeep
>   here too.

I think we're having some terminology issues...

FWIW, the kernel terminolgy is actually "PM domain", not power domain.
This was intentional because the goal of the PM domain was to group
devices that some PM features.  To be very specific to the kernel, they
us the same set of PM callbacks.  Today, this is most commonly used to
model power domains, where a group of devices share a power rail, but it
does not need to be limited to that.

That being said, I'm having a hard time understanding the root of the
disagreement.

It seems that you and Sudeep would like to use domain-idle-states to
replace/superceed cpu-idle-states with the primary goal (and benefit)
being that it simplifies the DT bindings.  Is that correct?

The objections have come in because that means that implies that CPUs
become their own domains, which may not be the case in hardware in the
sense that they share a power rail.

However, IMO, thinking of a CPU as it's own "PM domain" may make some
sense based on the terminology above.

I think the other objection may be that using a genpd to model domain
with only a single device in it may be overkill, and I agree with that.
But, I'm not sure if making CPUs use domain-idle-states implies that
they necessarily have to use genpd is what you are proposing.  Maybe
someone could clarify that?

Kevin

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

[Sudeep Holla]

Hi Kevin,

Thanks for looking at this and simplifying various discussions we had so
far. I was thinking of summarizing something very similar. I couldn't
due to lack of time.

On 16/09/16 18:13, Kevin Hilman wrote:

[...]

> I think we're having some terminology issues...
>
> FWIW, the kernel terminolgy is actually "PM domain", not power domain.
> This was intentional because the goal of the PM domain was to group
> devices that some PM features.  To be very specific to the kernel, they
> us the same set of PM callbacks.  Today, this is most commonly used to
> model power domains, where a group of devices share a power rail, but it
> does not need to be limited to that.
>

Agreed/Understood.

> That being said, I'm having a hard time understanding the root of the
> disagreement.
>

Yes. I tried to convey the same earlier, but have failed. The only
disagreement is about a small part of this DT bindings. We would like to
make it completely hierarchical up to CPU nodes. More comments on that
below.

> It seems that you and Sudeep would like to use domain-idle-states to
> replace/superceed cpu-idle-states with the primary goal (and benefit)
> being that it simplifies the DT bindings.  Is that correct?
>

Correct, we want to deprecate cpu-idle-states with the introduction of
this hierarchical PM bindings. Yes IMO, it simplifies things and avoids
any ABI break we might trigger if we miss to consider some use-case now.

> The objections have come in because that means that implies that CPUs
> become their own domains, which may not be the case in hardware in the
> sense that they share a power rail.
>

Agreed.

> However, IMO, thinking of a CPU as it's own "PM domain" may make some
> sense based on the terminology above.
>

Thanks for that, we do understand that it may not be 100% correct when
we strictly considers hardware terminologies instead of above ones.
As along as we see no issues with the above terminologies it should be fine.

> I think the other objection may be that using a genpd to model domain
> with only a single device in it may be overkill, and I agree with that.

I too agree with that. Just because we represent that in DT in that way
doesn't mean we need to create a genpd to model domain. We can always
skip that if not required. That's pure implementation specifics and I
have tried to convey the same in my previous emails. I must say you have
summarized it very clearly in this email. Thanks again for that.

> But, I'm not sure if making CPUs use domain-idle-states implies that
> they necessarily have to use genpd is what you are proposing.  Maybe
> someone could clarify that?
>

No, I have not proposing anything around implementation in the whole
discussion so far. I have constrained myself just to DT bindings so far.
That's the main reason why I was opposed to mentions of OS vs platform
co-ordinated modes of CPU suspend in this discussion. IMO that's
completely out of scope of this DT binding we are defining here.

Hope that helps/clarifies the misunderstanding/disagreement.

-- 
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 v5 02/16] dt/bindings: Update binding for PM domain idle states

[Brendan Jackman]

On Fri, Sep 16 2016 at 18:39, Sudeep Holla <sudeep.holla@arm.com> wrote:
> Hi Kevin,
>
> Thanks for looking at this and simplifying various discussions we had so
> far. I was thinking of summarizing something very similar. I couldn't
> due to lack of time.
>
> On 16/09/16 18:13, Kevin Hilman wrote:
>
> [...]
>
>> I think we're having some terminology issues...
>>
>> FWIW, the kernel terminolgy is actually "PM domain", not power domain.
>> This was intentional because the goal of the PM domain was to group
>> devices that some PM features.  To be very specific to the kernel, they
>> us the same set of PM callbacks.  Today, this is most commonly used to
>> model power domains, where a group of devices share a power rail, but it
>> does not need to be limited to that.
>>
>
> Agreed/Understood.
>
>> That being said, I'm having a hard time understanding the root of the
>> disagreement.
>>
>
> Yes. I tried to convey the same earlier, but have failed. The only
> disagreement is about a small part of this DT bindings. We would like to
> make it completely hierarchical up to CPU nodes. More comments on that
> below.
>
>> It seems that you and Sudeep would like to use domain-idle-states to
>> replace/superceed cpu-idle-states with the primary goal (and benefit)
>> being that it simplifies the DT bindings.  Is that correct?
>>
>
> Correct, we want to deprecate cpu-idle-states with the introduction of
> this hierarchical PM bindings. Yes IMO, it simplifies things and avoids
> any ABI break we might trigger if we miss to consider some use-case now.
>
>> The objections have come in because that means that implies that CPUs
>> become their own domains, which may not be the case in hardware in the
>> sense that they share a power rail.
>>
>
> Agreed.
>
>> However, IMO, thinking of a CPU as it's own "PM domain" may make some
>> sense based on the terminology above.
>>
>
> Thanks for that, we do understand that it may not be 100% correct when
> we strictly considers hardware terminologies instead of above ones.
> As along as we see no issues with the above terminologies it should be fine.
>
>> I think the other objection may be that using a genpd to model domain
>> with only a single device in it may be overkill, and I agree with that.
>
> I too agree with that. Just because we represent that in DT in that way
> doesn't mean we need to create a genpd to model domain. We can always
> skip that if not required. That's pure implementation specifics and I
> have tried to convey the same in my previous emails. I must say you have
> summarized it very clearly in this email. Thanks again for that.
>
>> But, I'm not sure if making CPUs use domain-idle-states implies that
>> they necessarily have to use genpd is what you are proposing.  Maybe
>> someone could clarify that?
>>
>
> No, I have not proposing anything around implementation in the whole
> discussion so far. I have constrained myself just to DT bindings so far.
> That's the main reason why I was opposed to mentions of OS vs platform
> co-ordinated modes of CPU suspend in this discussion. IMO that's
> completely out of scope of this DT binding we are defining here.
>
> Hope that helps/clarifies the misunderstanding/disagreement.

Indeed. My intention was that the proposal would result in the exact
same kernel behaviour as Lina's current patchset, i.e. there is one
genpd per cluster, and CPU-level idle states are still handled by
cpuidle.

The only change from the current patchset would be in initialisation
code: some coordination would need to be done to determine which idle
states go into cpuidle and which go into the genpds (whereas with the
current bindings, states from cpu-idle-states go into cpuidle and states
from domain-idle-states go into genpd). So you could say that this would
be a trade-off between binding simplicity and implementation simplicity.

Cheers,
Brendan

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

[Lina Iyer]

On Mon, Sep 19 2016 at 09:09 -0600, Brendan Jackman wrote:
>
>On Fri, Sep 16 2016 at 18:39, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> Hi Kevin,
>>
>> Thanks for looking at this and simplifying various discussions we had so
>> far. I was thinking of summarizing something very similar. I couldn't
>> due to lack of time.
>>
>> On 16/09/16 18:13, Kevin Hilman wrote:
>>
>> [...]
>>
>>> I think we're having some terminology issues...
>>>
>>> FWIW, the kernel terminolgy is actually "PM domain", not power domain.
>>> This was intentional because the goal of the PM domain was to group
>>> devices that some PM features.  To be very specific to the kernel, they
>>> us the same set of PM callbacks.  Today, this is most commonly used to
>>> model power domains, where a group of devices share a power rail, but it
>>> does not need to be limited to that.
>>>
>>
>> Agreed/Understood.
>>
>>> That being said, I'm having a hard time understanding the root of the
>>> disagreement.
>>>
>>
>> Yes. I tried to convey the same earlier, but have failed. The only
>> disagreement is about a small part of this DT bindings. We would like to
>> make it completely hierarchical up to CPU nodes. More comments on that
>> below.
>>
>>> It seems that you and Sudeep would like to use domain-idle-states to
>>> replace/superceed cpu-idle-states with the primary goal (and benefit)
>>> being that it simplifies the DT bindings.  Is that correct?
>>>
>>
>> Correct, we want to deprecate cpu-idle-states with the introduction of
>> this hierarchical PM bindings. Yes IMO, it simplifies things and avoids
>> any ABI break we might trigger if we miss to consider some use-case now.
>>
>>> The objections have come in because that means that implies that CPUs
>>> become their own domains, which may not be the case in hardware in the
>>> sense that they share a power rail.
>>>
>>
>> Agreed.
>>
>>> However, IMO, thinking of a CPU as it's own "PM domain" may make some
>>> sense based on the terminology above.
>>>
>>
>> Thanks for that, we do understand that it may not be 100% correct when
>> we strictly considers hardware terminologies instead of above ones.
>> As along as we see no issues with the above terminologies it should be fine.
>>
>>> I think the other objection may be that using a genpd to model domain
>>> with only a single device in it may be overkill, and I agree with that.
>>
>> I too agree with that. Just because we represent that in DT in that way
>> doesn't mean we need to create a genpd to model domain. We can always
>> skip that if not required. That's pure implementation specifics and I
>> have tried to convey the same in my previous emails. I must say you have
>> summarized it very clearly in this email. Thanks again for that.
>>
>>> But, I'm not sure if making CPUs use domain-idle-states implies that
>>> they necessarily have to use genpd is what you are proposing.  Maybe
>>> someone could clarify that?
>>>
>>
>> No, I have not proposing anything around implementation in the whole
>> discussion so far. I have constrained myself just to DT bindings so far.
>> That's the main reason why I was opposed to mentions of OS vs platform
>> co-ordinated modes of CPU suspend in this discussion. IMO that's
>> completely out of scope of this DT binding we are defining here.
>>
Fair. But understand the PM Domain bindings do not impose any
requirements of hierarchy. Domain idle states are defined by the
property domain-idle-states in the domain node. How the DT bindings are
organized is immaterial to the PM Domain core.

It is a different exercise all together to look at CPU PSCI modes and
have a unified way of representing them in DT. The current set of
patches does not dictate where the domain idle states be located (pardon
my example in the patch, which was not updated to reflect that). That
said, I do require that domains that are controlled by the PSCI f/w be
defined under the 'psci' node in DT, which is fair. All the domain needs
are phandles to the idle state definitions; how the nodes are arranged
in DT is not of consequence to the driver.

In my mind providing a structure to CPU PM domains that can be used for
both OSI and PC is a separate effort. It may also club what Brendan
mentions below as part of the effort. The hierarchy that is presented in
[1] is inherent in the PM domain hierarchy and idle states don't have to
duplicate that information.

>> Hope that helps/clarifies the misunderstanding/disagreement.
>
>Indeed. My intention was that the proposal would result in the exact
>same kernel behaviour as Lina's current patchset, i.e. there is one
>genpd per cluster, and CPU-level idle states are still handled by
>cpuidle.
>
>The only change from the current patchset would be in initialisation
>code: some coordination would need to be done to determine which idle
>states go into cpuidle and which go into the genpds (whereas with the
>current bindings, states from cpu-idle-states go into cpuidle and states
>from domain-idle-states go into genpd). So you could say that this would
>be a trade-off between binding simplicity and implementation simplicity.
>
I would not oppose the idea of virtual domains around CPUs (I admit I am
not comfortable with the idea though), if that is the right thing to do.
But the scope of that work is extensive and should not be clubbed as
part of this proposal. It is an extensive code rework spanning cpuidle
drivers and PSCI and there are hooks in this code to help you achieve
that.

Thanks,
Lina

[1]. https://patchwork.kernel.org/patch/9264507/

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

[Brendan Jackman]

On Tue, Sep 20 2016 at 17:17, Lina Iyer <lina.iyer@linaro.org> wrote:
> On Mon, Sep 19 2016 at 09:09 -0600, Brendan Jackman wrote:
>>
>>On Fri, Sep 16 2016 at 18:39, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>> Hi Kevin,
>>>
>>> Thanks for looking at this and simplifying various discussions we had so
>>> far. I was thinking of summarizing something very similar. I couldn't
>>> due to lack of time.
>>>
>>> On 16/09/16 18:13, Kevin Hilman wrote:
>>>
>>> [...]
>>>
>>>> I think we're having some terminology issues...
>>>>
>>>> FWIW, the kernel terminolgy is actually "PM domain", not power domain.
>>>> This was intentional because the goal of the PM domain was to group
>>>> devices that some PM features.  To be very specific to the kernel, they
>>>> us the same set of PM callbacks.  Today, this is most commonly used to
>>>> model power domains, where a group of devices share a power rail, but it
>>>> does not need to be limited to that.
>>>>
>>>
>>> Agreed/Understood.
>>>
>>>> That being said, I'm having a hard time understanding the root of the
>>>> disagreement.
>>>>
>>>
>>> Yes. I tried to convey the same earlier, but have failed. The only
>>> disagreement is about a small part of this DT bindings. We would like to
>>> make it completely hierarchical up to CPU nodes. More comments on that
>>> below.
>>>
>>>> It seems that you and Sudeep would like to use domain-idle-states to
>>>> replace/superceed cpu-idle-states with the primary goal (and benefit)
>>>> being that it simplifies the DT bindings.  Is that correct?
>>>>
>>>
>>> Correct, we want to deprecate cpu-idle-states with the introduction of
>>> this hierarchical PM bindings. Yes IMO, it simplifies things and avoids
>>> any ABI break we might trigger if we miss to consider some use-case now.
>>>
>>>> The objections have come in because that means that implies that CPUs
>>>> become their own domains, which may not be the case in hardware in the
>>>> sense that they share a power rail.
>>>>
>>>
>>> Agreed.
>>>
>>>> However, IMO, thinking of a CPU as it's own "PM domain" may make some
>>>> sense based on the terminology above.
>>>>
>>>
>>> Thanks for that, we do understand that it may not be 100% correct when
>>> we strictly considers hardware terminologies instead of above ones.
>>> As along as we see no issues with the above terminologies it should be fine.
>>>
>>>> I think the other objection may be that using a genpd to model domain
>>>> with only a single device in it may be overkill, and I agree with that.
>>>
>>> I too agree with that. Just because we represent that in DT in that way
>>> doesn't mean we need to create a genpd to model domain. We can always
>>> skip that if not required. That's pure implementation specifics and I
>>> have tried to convey the same in my previous emails. I must say you have
>>> summarized it very clearly in this email. Thanks again for that.
>>>
>>>> But, I'm not sure if making CPUs use domain-idle-states implies that
>>>> they necessarily have to use genpd is what you are proposing.  Maybe
>>>> someone could clarify that?
>>>>
>>>
>>> No, I have not proposing anything around implementation in the whole
>>> discussion so far. I have constrained myself just to DT bindings so far.
>>> That's the main reason why I was opposed to mentions of OS vs platform
>>> co-ordinated modes of CPU suspend in this discussion. IMO that's
>>> completely out of scope of this DT binding we are defining here.
>>>
> Fair. But understand the PM Domain bindings do not impose any
> requirements of hierarchy. Domain idle states are defined by the
> property domain-idle-states in the domain node. How the DT bindings are
> organized is immaterial to the PM Domain core.
>
> It is a different exercise all together to look at CPU PSCI modes and
> have a unified way of representing them in DT. The current set of
> patches does not dictate where the domain idle states be located (pardon
> my example in the patch, which was not updated to reflect that). That
> said, I do require that domains that are controlled by the PSCI f/w be
> defined under the 'psci' node in DT, which is fair. All the domain needs
> are phandles to the idle state definitions; how the nodes are arranged
> in DT is not of consequence to the driver.
>
> In my mind providing a structure to CPU PM domains that can be used for
> both OSI and PC is a separate effort.

Do you mean a structure in the kernel or in DT? If the former, I agree,
if the latter I strongly disagree. I think DT bindings should be totally
unaware of PSCI suspend modes.

> It may also club what Brendan
> mentions below as part of the effort. The hierarchy that is presented in
> [1] is inherent in the PM domain hierarchy and idle states don't have to
> duplicate that information.
>
>>> Hope that helps/clarifies the misunderstanding/disagreement.
>>
>>Indeed. My intention was that the proposal would result in the exact
>>same kernel behaviour as Lina's current patchset, i.e. there is one
>>genpd per cluster, and CPU-level idle states are still handled by
>>cpuidle.
>>
>>The only change from the current patchset would be in initialisation
>>code: some coordination would need to be done to determine which idle
>>states go into cpuidle and which go into the genpds (whereas with the
>>current bindings, states from cpu-idle-states go into cpuidle and states
>>from domain-idle-states go into genpd). So you could say that this would
>>be a trade-off between binding simplicity and implementation simplicity.
>>
> I would not oppose the idea of virtual domains around CPUs (I admit I am
> not comfortable with the idea though), if that is the right thing to do.
> But the scope of that work is extensive and should not be clubbed as
> part of this proposal. It is an extensive code rework spanning cpuidle
> drivers and PSCI and there are hooks in this code to help you achieve
> that.

If we want to take the per-CPU domains approach, we _have_ to do it as
part of this proposal; it's a different set of semantics for the
cpu-idle-states/domain-idle-states properties. It would mean
cpu-idle-states is _superseded_ by domain idle states - implementing one
solution (where cpu-idle-states and domain idle states are both taken
into consideration by the implementation) then later switching to the
alternative (where cpu-idle-states is ignored when a CPU PM domain tree
is present) wouldn't make sense from a backward-compatibility
perspective.

You're right that implementing the alternative proposal in the Linux
kernel would mean quite a big rework. But, idealistically speaking,
Linux-specific implementation realities shouldn't be a factor in Device
Tree binding designs, right?

If people think that using both cpu-idle-states and domain-idle-states
is the pragmatic choice (or object fundamentally to the idea of devices
with idle states as being in their own PM domain) then that's fine IMO,
but it's a one-time decision and I think we should be clear about why
we're making it.

Cheers,
Brendan

[PATCH v5 15/16] dt/bindings: Add PSCI OS-Initiated PM Domains bindings

[Lina Iyer]

Add bindings for defining a OS-Initiated based CPU PM domain.

Cc: <devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Lina Iyer <lina.iyer-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 Documentation/devicetree/bindings/arm/psci.txt | 79 ++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
index a2c4f1d..63a229b 100644
--- a/Documentation/devicetree/bindings/arm/psci.txt
+++ b/Documentation/devicetree/bindings/arm/psci.txt
@@ -105,7 +105,86 @@ 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 under the psci node. The
+domain definitions must follow the domain idle state specifications per [3].
+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 with 1 CPU in each domain
+
+	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>;
+			power-domains = <&CPU_PD0>;
+		};
+
+		CPU1: cpu@1 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a57", "arm,armv8";
+			reg = <0x100>;
+			enable-method = "psci";
+			cpu-idle-states = <&CPU_PWRDN>;
+			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 = "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>;
+			};
+	};
+
+	psci {
+		compatible = "arm,psci-1.0";
+		method = "smc";
+
+		CPU_PD0: cpu-pd@0 {
+			#power-domain-cells = <0>;
+			domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWR_DWN>;
+		};
+
+		CPU_PD1: cpu-pd@1 {
+			#power-domain-cells = <0>;
+			domain-idle-states =  <&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
+[4]. CPU PM Domains description
+    Documentation/power/cpu_domains.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

[PATCH v5 16/16] 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@vger.kernel.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8916.dtsi | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index 11bdc24..e6d8c3b 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -99,6 +99,7 @@
 			next-level-cache = <&L2_0>;
 			enable-method = "psci";
 			cpu-idle-states = <&CPU_SPC>;
+			power-domains = <&CPU_PD>;
 		};
 
 		CPU1: cpu@1 {
@@ -108,6 +109,7 @@
 			next-level-cache = <&L2_0>;
 			enable-method = "psci";
 			cpu-idle-states = <&CPU_SPC>;
+			power-domains = <&CPU_PD>;
 		};
 
 		CPU2: cpu@2 {
@@ -117,6 +119,7 @@
 			next-level-cache = <&L2_0>;
 			enable-method = "psci";
 			cpu-idle-states = <&CPU_SPC>;
+			power-domains = <&CPU_PD>;
 		};
 
 		CPU3: cpu@3 {
@@ -126,6 +129,7 @@
 			next-level-cache = <&L2_0>;
 			enable-method = "psci";
 			cpu-idle-states = <&CPU_SPC>;
+			power-domains = <&CPU_PD>;
 		};
 
 		L2_0: l2-cache {
@@ -142,12 +146,33 @@
 				min-residency-us = <2000>;
 				local-timer-stop;
 			};
+
+			CLUSTER_RET: cluster_retention {
+				compatible = "arm,idle-state";
+				arm,psci-suspend-param = <0x1000010>;
+				entry-latency-us = <500>;
+				exit-latency-us = <500>;
+				min-residency-us = <2000>;
+			};
+
+			CLUSTER_PWR_DWN: cluster_gdhs {
+				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-1.0";
 		method = "smc";
+
+		CPU_PD: cpu-pd@0 {
+			#power-domain-cells = <0>;
+			domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWR_DWN>;
+		};
 	};
 
 	pmu {
-- 
2.7.4