[PATCH v5 00/15] cpuidle: psci: Support hierarchical CPU arrangement

[PATCH v5 00/15] cpuidle: psci: Support hierarchical CPU arrangement

[Ulf Hansson]

Changes in v5:
	- Remove PM domains in the error path, when failing to enable OSI.
	- Folded in a patch that adds a genpd OF helper to remove subdomains.
	- Added reviewed-by tags.

Changes in v4:
	- Move the check for OSI support from psci_dt_attach_cpu() to the
	caller's side of it.
	- Add comment in the code about using the deepest idle state as the
	triggering point for the domain state selection.
	- Folded in a patch to enable support for CPU hotplug.

Changes in v3:
	- Take one step further to completely avoid executing any OSI specific
	code from the ->enter() callback, while operating in the default PSCI
	Platform Coordinated mode.
	- Update example for the PSCI DT bindings to make it compile with
	"make dt_binding_check"

Changes in v2:
	- Avoid to affect the non-OSI path with specific changes for OSI. This
	forced me to re-order the series and a caused more or less minor changes
	to most of the patches.
	- Updated the DT bindings for PSCI to clarify and to include the "psci"
	name of the PM domain to attach to.
	- Replaced patch1 with another patch from Sudeep, solving the same
	problem, but in a different way.

This series enables initial support for hierarchical CPU arrangement, managed
by PSCI and its corresponding cpuidle driver. It's based on using the generic
PM domain (genpd), which nowadays also supports devices belonging to CPUs.

The last DTS patch enables the hierarchical topology to be used for the Qcom
410c Dragonboard, which supports the PSCI OS-initiated mode.

More detailed background can be found from previous submissions [1].

The series is also available at:
git.linaro.org/people/ulf.hansson/linux-pm.git next

Kind regards
Ulf Hansson

[1]
https://lwn.net/Articles/788306/

Lina Iyer (1):
  cpuidle: dt: Support hierarchical CPU idle states

Sudeep Holla (1):
  cpuidle: psci: Align psci_power_state count with idle state count

Ulf Hansson (13):
  dt: psci: Update DT bindings to support hierarchical PSCI states
  firmware: psci: Export functions to manage the OSI mode
  of: base: Add of_get_cpu_state_node() to get idle states for a CPU
    node
  cpuidle: psci: Simplify OF parsing of CPU idle state nodes
  cpuidle: psci: Support hierarchical CPU idle states
  cpuidle: psci: Add a helper to attach a CPU to its PM domain
  cpuidle: psci: Attach CPU devices to their PM domains
  cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains
  cpuidle: psci: Manage runtime PM in the idle path
  cpuidle: psci: Support CPU hotplug for the hierarchical model
  PM / Domains: Introduce a genpd OF helper that removes a subdomain
  cpuidle: psci: Add support for PM domains by using genpd
  arm64: dts: Convert to the hierarchical CPU topology layout for
    MSM8916

 .../devicetree/bindings/arm/cpus.yaml         |  15 +
 .../devicetree/bindings/arm/psci.yaml         | 104 ++++++
 arch/arm64/boot/dts/qcom/msm8916.dtsi         |  57 +++-
 drivers/base/power/domain.c                   |  38 +++
 drivers/cpuidle/Makefile                      |   4 +-
 drivers/cpuidle/cpuidle-psci-domain.c         | 308 ++++++++++++++++++
 drivers/cpuidle/cpuidle-psci.c                | 161 +++++++--
 drivers/cpuidle/cpuidle-psci.h                |  17 +
 drivers/cpuidle/dt_idle_states.c              |   5 +-
 drivers/firmware/psci/psci.c                  |  18 +-
 drivers/of/base.c                             |  36 ++
 include/linux/cpuhotplug.h                    |   1 +
 include/linux/of.h                            |   8 +
 include/linux/pm_domain.h                     |   8 +
 include/linux/psci.h                          |   2 +
 15 files changed, 747 insertions(+), 35 deletions(-)
 create mode 100644 drivers/cpuidle/cpuidle-psci-domain.c
 create mode 100644 drivers/cpuidle/cpuidle-psci.h

-- 
2.17.1

[PATCH v5 01/15] cpuidle: psci: Align psci_power_state count with idle state count

[Ulf Hansson]

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

Instead of allocating 'n-1' states in psci_power_state to manage 'n'
idle states which include "ARM WFI" state, it would be simpler to have
1:1 mapping between psci_power_state and cpuidle driver states.

ARM WFI state(i.e. idx == 0) is handled specially in the generic macro
CPU_PM_CPU_IDLE_ENTER_PARAM and hence state[-1] is not possible. However
for sake of code readability, it is better to have 1:1 mapping and not
use [idx - 1] to access psci_power_state corresponding to driver cpuidle
state for idx.

psci_power_state[0] is default initialised to 0 and is never accessed
while entering WFI state.

Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v5:
	- None.

---
 drivers/cpuidle/cpuidle-psci.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index f3c1a2396f98..361985f52ddd 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -30,7 +30,7 @@ static int psci_enter_idle_state(struct cpuidle_device *dev,
 	u32 *state = __this_cpu_read(psci_power_state);
 
 	return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter,
-					   idx, state[idx - 1]);
+					   idx, state[idx]);
 }
 
 static struct cpuidle_driver psci_idle_driver __initdata = {
@@ -89,12 +89,14 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
 	if (!count)
 		return -ENODEV;
 
+	count++; /* Add WFI state too */
 	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
 	if (!psci_states)
 		return -ENOMEM;
 
-	for (i = 0; i < count; i++) {
-		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
+	for (i = 1; i < count; i++) {
+		state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
+					      i - 1);
 		ret = psci_dt_parse_state_node(state_node, &psci_states[i]);
 		of_node_put(state_node);
 
-- 
2.17.1

[PATCH v5 02/15] dt: psci: Update DT bindings to support hierarchical PSCI states

[Ulf Hansson]

Update PSCI DT bindings to allow to represent idle states for CPUs and the
CPU topology, by using a hierarchical layout. Primarily this is done by
re-using the existing DT bindings for PM domains [1] and for PM domain idle
states [2].

Let's also add an example into the document for the PSCI DT bindings, to
clearly show the new hierarchical based layout. The currently supported
flattened layout, is already described in the ARM idle states bindings [3],
so let's leave that as is.

[1] Documentation/devicetree/bindings/power/power_domain.txt
[2] Documentation/devicetree/bindings/power/domain-idle-state.txt
[3] Documentation/devicetree/bindings/arm/idle-states.txt

Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v5:
	- None.

---
 .../devicetree/bindings/arm/cpus.yaml         |  15 +++
 .../devicetree/bindings/arm/psci.yaml         | 104 ++++++++++++++++++
 2 files changed, 119 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
index c23c24ff7575..7a9c3ce2dbef 100644
--- a/Documentation/devicetree/bindings/arm/cpus.yaml
+++ b/Documentation/devicetree/bindings/arm/cpus.yaml
@@ -242,6 +242,21 @@ properties:
 
       where voltage is in V, frequency is in MHz.
 
+  power-domains:
+    $ref: '/schemas/types.yaml#/definitions/phandle-array'
+    description:
+      List of phandles and PM domain specifiers, as defined by bindings of the
+      PM domain provider (see also ../power_domain.txt).
+
+  power-domain-names:
+    $ref: '/schemas/types.yaml#/definitions/string-array'
+    description:
+      A list of power domain name strings sorted in the same order as the
+      power-domains property.
+
+      For PSCI based platforms, the name corresponding to the index of the PSCI
+      PM domain provider, must be "psci".
+
   qcom,saw:
     $ref: '/schemas/types.yaml#/definitions/phandle'
     description: |
diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
index 7abdf58b335e..8ef85420b2ab 100644
--- a/Documentation/devicetree/bindings/arm/psci.yaml
+++ b/Documentation/devicetree/bindings/arm/psci.yaml
@@ -102,6 +102,34 @@ properties:
       [1] Kernel documentation - ARM idle states bindings
         Documentation/devicetree/bindings/arm/idle-states.txt
 
+  "#power-domain-cells":
+    description:
+      The number of cells in a PM domain specifier as per binding in [3].
+      Must be 0 as to represent a single PM domain.
+
+      ARM systems can have multiple cores, sometimes in an hierarchical
+      arrangement. This often, but not always, maps directly to the processor
+      power topology of the system. Individual nodes in a topology have their
+      own specific power states and can be better represented hierarchically.
+
+      For these cases, the definitions of the idle states for the CPUs and the
+      CPU topology, must conform to the binding in [3]. The idle states
+      themselves must conform to the binding in [4] and must specify the
+      arm,psci-suspend-param property.
+
+      It should also be noted that, in PSCI firmware v1.0 the OS-Initiated
+      (OSI) CPU suspend mode is introduced. Using a hierarchical representation
+      helps to implement support for OSI mode and OS implementations may choose
+      to mandate it.
+
+      [3] Documentation/devicetree/bindings/power/power_domain.txt
+      [4] Documentation/devicetree/bindings/power/domain-idle-state.txt
+
+  power-domains:
+    $ref: '/schemas/types.yaml#/definitions/phandle-array'
+    description:
+      List of phandles and PM domain specifiers, as defined by bindings of the
+      PM domain provider.
 
 required:
   - compatible
@@ -160,4 +188,80 @@ examples:
       cpu_on = <0x95c10002>;
       cpu_off = <0x95c10001>;
     };
+
+  - |+
+
+    // Case 4: CPUs and CPU idle states described using the hierarchical model.
+
+    cpus {
+      #size-cells = <0>;
+      #address-cells = <1>;
+
+      CPU0: cpu@0 {
+        device_type = "cpu";
+        compatible = "arm,cortex-a53", "arm,armv8";
+        reg = <0x0>;
+        enable-method = "psci";
+        power-domains = <&CPU_PD0>;
+        power-domain-names = "psci";
+      };
+
+      CPU1: cpu@1 {
+        device_type = "cpu";
+        compatible = "arm,cortex-a57", "arm,armv8";
+        reg = <0x100>;
+        enable-method = "psci";
+        power-domains = <&CPU_PD1>;
+        power-domain-names = "psci";
+      };
+
+      idle-states {
+
+        CPU_PWRDN: cpu-power-down {
+          compatible = "arm,idle-state";
+          arm,psci-suspend-param = <0x0000001>;
+          entry-latency-us = <10>;
+          exit-latency-us = <10>;
+          min-residency-us = <100>;
+        };
+
+        CLUSTER_RET: cluster-retention {
+          compatible = "domain-idle-state";
+          arm,psci-suspend-param = <0x1000011>;
+          entry-latency-us = <500>;
+          exit-latency-us = <500>;
+          min-residency-us = <2000>;
+        };
+
+        CLUSTER_PWRDN: cluster-power-down {
+          compatible = "domain-idle-state";
+          arm,psci-suspend-param = <0x1000031>;
+          entry-latency-us = <2000>;
+          exit-latency-us = <2000>;
+          min-residency-us = <6000>;
+        };
+      };
+    };
+
+    psci {
+      compatible = "arm,psci-1.0";
+      method = "smc";
+
+      CPU_PD0: cpu-pd0 {
+        #power-domain-cells = <0>;
+        domain-idle-states = <&CPU_PWRDN>;
+        power-domains = <&CLUSTER_PD>;
+      };
+
+      CPU_PD1: cpu-pd1 {
+        #power-domain-cells = <0>;
+        domain-idle-states =  <&CPU_PWRDN>;
+        power-domains = <&CLUSTER_PD>;
+      };
+
+      CLUSTER_PD: cluster-pd {
+        #power-domain-cells = <0>;
+        domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWRDN>;
+      };
+    };
 ...
-- 
2.17.1

Re: [PATCH v5 02/15] dt: psci: Update DT bindings to support hierarchical PSCI states

[Rob Herring]

On Mon, Dec 30, 2019 at 8:44 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> Update PSCI DT bindings to allow to represent idle states for CPUs and the
> CPU topology, by using a hierarchical layout. Primarily this is done by
> re-using the existing DT bindings for PM domains [1] and for PM domain idle
> states [2].
>
> Let's also add an example into the document for the PSCI DT bindings, to
> clearly show the new hierarchical based layout. The currently supported
> flattened layout, is already described in the ARM idle states bindings [3],
> so let's leave that as is.
>
> [1] Documentation/devicetree/bindings/power/power_domain.txt
> [2] Documentation/devicetree/bindings/power/domain-idle-state.txt
> [3] Documentation/devicetree/bindings/arm/idle-states.txt
>
> Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> Changes in v5:
>         - None.

First I'm seeing this as the DT list was not copied. The example has
problems when running 'make dt_binding_check':

Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
compatible: Additional items are not allowed ('arm,armv8' was
unexpected)
Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
compatible: ['arm,cortex-a53', 'arm,armv8'] is too long
Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
compatible: Additional items are not allowed ('arm,armv8' was
unexpected)
Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
compatible: ['arm,cortex-a57', 'arm,armv8'] is too long

'arm,armv8' is only valid for s/w models.

Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
idle-states: cluster-retention:compatible:0: 'arm,idle-state' was
expected
Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
idle-states: cluster-power-down:compatible:0: 'arm,idle-state' was
expected

The last 2 are due to my conversion of the idle-states binding which
is in my tree now. Probably need to add 'domain-idle-state' as a
compatible at a minimum. It looks like domain-idle-state.txt is pretty
much the same as arm/idle-state.txt, so we should perhaps merge them.

There's some bigger issues though.

> ---
>  .../devicetree/bindings/arm/cpus.yaml         |  15 +++
>  .../devicetree/bindings/arm/psci.yaml         | 104 ++++++++++++++++++
>  2 files changed, 119 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
> index c23c24ff7575..7a9c3ce2dbef 100644
> --- a/Documentation/devicetree/bindings/arm/cpus.yaml
> +++ b/Documentation/devicetree/bindings/arm/cpus.yaml
> @@ -242,6 +242,21 @@ properties:
>
>        where voltage is in V, frequency is in MHz.
>
> +  power-domains:
> +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> +    description:
> +      List of phandles and PM domain specifiers, as defined by bindings of the
> +      PM domain provider (see also ../power_domain.txt).
> +
> +  power-domain-names:
> +    $ref: '/schemas/types.yaml#/definitions/string-array'
> +    description:
> +      A list of power domain name strings sorted in the same order as the
> +      power-domains property.
> +
> +      For PSCI based platforms, the name corresponding to the index of the PSCI
> +      PM domain provider, must be "psci".
> +
>    qcom,saw:
>      $ref: '/schemas/types.yaml#/definitions/phandle'
>      description: |
> diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
> index 7abdf58b335e..8ef85420b2ab 100644
> --- a/Documentation/devicetree/bindings/arm/psci.yaml
> +++ b/Documentation/devicetree/bindings/arm/psci.yaml
> @@ -102,6 +102,34 @@ properties:
>        [1] Kernel documentation - ARM idle states bindings
>          Documentation/devicetree/bindings/arm/idle-states.txt
>
> +  "#power-domain-cells":

This is wrong because you are saying the /psci node should have these
properties. You need to define the child nodes (at least a pattern you
can match on) and put these properties there.

> +    description:
> +      The number of cells in a PM domain specifier as per binding in [3].
> +      Must be 0 as to represent a single PM domain.
> +
> +      ARM systems can have multiple cores, sometimes in an hierarchical
> +      arrangement. This often, but not always, maps directly to the processor
> +      power topology of the system. Individual nodes in a topology have their
> +      own specific power states and can be better represented hierarchically.
> +
> +      For these cases, the definitions of the idle states for the CPUs and the
> +      CPU topology, must conform to the binding in [3]. The idle states
> +      themselves must conform to the binding in [4] and must specify the
> +      arm,psci-suspend-param property.
> +
> +      It should also be noted that, in PSCI firmware v1.0 the OS-Initiated
> +      (OSI) CPU suspend mode is introduced. Using a hierarchical representation
> +      helps to implement support for OSI mode and OS implementations may choose
> +      to mandate it.
> +
> +      [3] Documentation/devicetree/bindings/power/power_domain.txt
> +      [4] Documentation/devicetree/bindings/power/domain-idle-state.txt
> +
> +  power-domains:
> +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> +    description:
> +      List of phandles and PM domain specifiers, as defined by bindings of the
> +      PM domain provider.

A schema for 'domain-idle-states' property is missing.

>
>  required:
>    - compatible
> @@ -160,4 +188,80 @@ examples:
>        cpu_on = <0x95c10002>;
>        cpu_off = <0x95c10001>;
>      };
> +
> +  - |+
> +
> +    // Case 4: CPUs and CPU idle states described using the hierarchical model.
> +
> +    cpus {
> +      #size-cells = <0>;
> +      #address-cells = <1>;
> +
> +      CPU0: cpu@0 {
> +        device_type = "cpu";
> +        compatible = "arm,cortex-a53", "arm,armv8";
> +        reg = <0x0>;
> +        enable-method = "psci";
> +        power-domains = <&CPU_PD0>;
> +        power-domain-names = "psci";
> +      };
> +
> +      CPU1: cpu@1 {
> +        device_type = "cpu";
> +        compatible = "arm,cortex-a57", "arm,armv8";
> +        reg = <0x100>;
> +        enable-method = "psci";
> +        power-domains = <&CPU_PD1>;
> +        power-domain-names = "psci";
> +      };
> +
> +      idle-states {
> +
> +        CPU_PWRDN: cpu-power-down {
> +          compatible = "arm,idle-state";
> +          arm,psci-suspend-param = <0x0000001>;
> +          entry-latency-us = <10>;
> +          exit-latency-us = <10>;
> +          min-residency-us = <100>;
> +        };
> +
> +        CLUSTER_RET: cluster-retention {
> +          compatible = "domain-idle-state";
> +          arm,psci-suspend-param = <0x1000011>;
> +          entry-latency-us = <500>;
> +          exit-latency-us = <500>;
> +          min-residency-us = <2000>;
> +        };
> +
> +        CLUSTER_PWRDN: cluster-power-down {
> +          compatible = "domain-idle-state";
> +          arm,psci-suspend-param = <0x1000031>;
> +          entry-latency-us = <2000>;
> +          exit-latency-us = <2000>;
> +          min-residency-us = <6000>;
> +        };
> +      };
> +    };
> +
> +    psci {
> +      compatible = "arm,psci-1.0";
> +      method = "smc";
> +
> +      CPU_PD0: cpu-pd0 {
> +        #power-domain-cells = <0>;
> +        domain-idle-states = <&CPU_PWRDN>;
> +        power-domains = <&CLUSTER_PD>;
> +      };
> +
> +      CPU_PD1: cpu-pd1 {
> +        #power-domain-cells = <0>;
> +        domain-idle-states =  <&CPU_PWRDN>;
> +        power-domains = <&CLUSTER_PD>;
> +      };
> +
> +      CLUSTER_PD: cluster-pd {
> +        #power-domain-cells = <0>;
> +        domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWRDN>;
> +      };
> +    };
>  ...
> --
> 2.17.1
>

Re: [PATCH v5 02/15] dt: psci: Update DT bindings to support hierarchical PSCI states

[Ulf Hansson]

On Mon, 13 Jan 2020 at 20:53, Rob Herring <robh+dt@kernel.org> wrote:
>
> On Mon, Dec 30, 2019 at 8:44 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > Update PSCI DT bindings to allow to represent idle states for CPUs and the
> > CPU topology, by using a hierarchical layout. Primarily this is done by
> > re-using the existing DT bindings for PM domains [1] and for PM domain idle
> > states [2].
> >
> > Let's also add an example into the document for the PSCI DT bindings, to
> > clearly show the new hierarchical based layout. The currently supported
> > flattened layout, is already described in the ARM idle states bindings [3],
> > so let's leave that as is.
> >
> > [1] Documentation/devicetree/bindings/power/power_domain.txt
> > [2] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > [3] Documentation/devicetree/bindings/arm/idle-states.txt
> >
> > Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >
> > Changes in v5:
> >         - None.
>
> First I'm seeing this as the DT list was not copied. The example has
> problems when running 'make dt_binding_check':
>
> Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> compatible: Additional items are not allowed ('arm,armv8' was
> unexpected)
> Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> compatible: ['arm,cortex-a53', 'arm,armv8'] is too long
> Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> compatible: Additional items are not allowed ('arm,armv8' was
> unexpected)
> Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> compatible: ['arm,cortex-a57', 'arm,armv8'] is too long
>
> 'arm,armv8' is only valid for s/w models.

Perhaps you have a different version of the tools than I have (I have
tried both on v.5.5-rc5 and todays linux-next), because I can't
reproduce these errors at my side when running "make
dt_binding_check".

Can you please check again?

>
> Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> idle-states: cluster-retention:compatible:0: 'arm,idle-state' was
> expected
> Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> idle-states: cluster-power-down:compatible:0: 'arm,idle-state' was
> expected
>
> The last 2 are due to my conversion of the idle-states binding which
> is in my tree now. Probably need to add 'domain-idle-state' as a
> compatible at a minimum. It looks like domain-idle-state.txt is pretty
> much the same as arm/idle-state.txt, so we should perhaps merge them.

Ahh, so maybe *all* of the above problems are caused by conflicts in
the arm-soc tree with changes from your tree!?

In regards to merging files, I am fine by that if that helps.

>
> There's some bigger issues though.
>
> > ---
> >  .../devicetree/bindings/arm/cpus.yaml         |  15 +++
> >  .../devicetree/bindings/arm/psci.yaml         | 104 ++++++++++++++++++
> >  2 files changed, 119 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
> > index c23c24ff7575..7a9c3ce2dbef 100644
> > --- a/Documentation/devicetree/bindings/arm/cpus.yaml
> > +++ b/Documentation/devicetree/bindings/arm/cpus.yaml
> > @@ -242,6 +242,21 @@ properties:
> >
> >        where voltage is in V, frequency is in MHz.
> >
> > +  power-domains:
> > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > +    description:
> > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > +      PM domain provider (see also ../power_domain.txt).
> > +
> > +  power-domain-names:
> > +    $ref: '/schemas/types.yaml#/definitions/string-array'
> > +    description:
> > +      A list of power domain name strings sorted in the same order as the
> > +      power-domains property.
> > +
> > +      For PSCI based platforms, the name corresponding to the index of the PSCI
> > +      PM domain provider, must be "psci".
> > +
> >    qcom,saw:
> >      $ref: '/schemas/types.yaml#/definitions/phandle'
> >      description: |
> > diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
> > index 7abdf58b335e..8ef85420b2ab 100644
> > --- a/Documentation/devicetree/bindings/arm/psci.yaml
> > +++ b/Documentation/devicetree/bindings/arm/psci.yaml
> > @@ -102,6 +102,34 @@ properties:
> >        [1] Kernel documentation - ARM idle states bindings
> >          Documentation/devicetree/bindings/arm/idle-states.txt
> >
> > +  "#power-domain-cells":
>
> This is wrong because you are saying the /psci node should have these
> properties. You need to define the child nodes (at least a pattern you
> can match on) and put these properties there.

Right, good point.

I searched for some similar examples for how to encode this, but
couldn't really find something useful. One more thing, it seems like
this change is also needed for the common power-domain bindings, as
that also specifies parent/childs domains.

Anyway, I would really appreciate if you can suggest something more
detailed for you think this should be done!?

>
> > +    description:
> > +      The number of cells in a PM domain specifier as per binding in [3].
> > +      Must be 0 as to represent a single PM domain.
> > +
> > +      ARM systems can have multiple cores, sometimes in an hierarchical
> > +      arrangement. This often, but not always, maps directly to the processor
> > +      power topology of the system. Individual nodes in a topology have their
> > +      own specific power states and can be better represented hierarchically.
> > +
> > +      For these cases, the definitions of the idle states for the CPUs and the
> > +      CPU topology, must conform to the binding in [3]. The idle states
> > +      themselves must conform to the binding in [4] and must specify the
> > +      arm,psci-suspend-param property.
> > +
> > +      It should also be noted that, in PSCI firmware v1.0 the OS-Initiated
> > +      (OSI) CPU suspend mode is introduced. Using a hierarchical representation
> > +      helps to implement support for OSI mode and OS implementations may choose
> > +      to mandate it.
> > +
> > +      [3] Documentation/devicetree/bindings/power/power_domain.txt
> > +      [4] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > +
> > +  power-domains:
> > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > +    description:
> > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > +      PM domain provider.
>
> A schema for 'domain-idle-states' property is missing.

Right, let's figure out the best way for how to add that.

>
> >
> >  required:
> >    - compatible
> > @@ -160,4 +188,80 @@ examples:
> >        cpu_on = <0x95c10002>;
> >        cpu_off = <0x95c10001>;
> >      };
> > +
> > +  - |+
> > +
> > +    // Case 4: CPUs and CPU idle states described using the hierarchical model.
> > +
> > +    cpus {
> > +      #size-cells = <0>;
> > +      #address-cells = <1>;
> > +
> > +      CPU0: cpu@0 {
> > +        device_type = "cpu";
> > +        compatible = "arm,cortex-a53", "arm,armv8";
> > +        reg = <0x0>;
> > +        enable-method = "psci";
> > +        power-domains = <&CPU_PD0>;
> > +        power-domain-names = "psci";
> > +      };
> > +
> > +      CPU1: cpu@1 {
> > +        device_type = "cpu";
> > +        compatible = "arm,cortex-a57", "arm,armv8";
> > +        reg = <0x100>;
> > +        enable-method = "psci";
> > +        power-domains = <&CPU_PD1>;
> > +        power-domain-names = "psci";
> > +      };
> > +
> > +      idle-states {
> > +
> > +        CPU_PWRDN: cpu-power-down {
> > +          compatible = "arm,idle-state";
> > +          arm,psci-suspend-param = <0x0000001>;
> > +          entry-latency-us = <10>;
> > +          exit-latency-us = <10>;
> > +          min-residency-us = <100>;
> > +        };
> > +
> > +        CLUSTER_RET: cluster-retention {
> > +          compatible = "domain-idle-state";
> > +          arm,psci-suspend-param = <0x1000011>;
> > +          entry-latency-us = <500>;
> > +          exit-latency-us = <500>;
> > +          min-residency-us = <2000>;
> > +        };
> > +
> > +        CLUSTER_PWRDN: cluster-power-down {
> > +          compatible = "domain-idle-state";
> > +          arm,psci-suspend-param = <0x1000031>;
> > +          entry-latency-us = <2000>;
> > +          exit-latency-us = <2000>;
> > +          min-residency-us = <6000>;
> > +        };
> > +      };
> > +    };
> > +
> > +    psci {
> > +      compatible = "arm,psci-1.0";
> > +      method = "smc";
> > +
> > +      CPU_PD0: cpu-pd0 {
> > +        #power-domain-cells = <0>;
> > +        domain-idle-states = <&CPU_PWRDN>;
> > +        power-domains = <&CLUSTER_PD>;
> > +      };
> > +
> > +      CPU_PD1: cpu-pd1 {
> > +        #power-domain-cells = <0>;
> > +        domain-idle-states =  <&CPU_PWRDN>;
> > +        power-domains = <&CLUSTER_PD>;
> > +      };
> > +
> > +      CLUSTER_PD: cluster-pd {
> > +        #power-domain-cells = <0>;
> > +        domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWRDN>;
> > +      };
> > +    };
> >  ...
> > --
> > 2.17.1
> >

Thanks for your feedback!

Kind regards
Uffe

Re: [PATCH v5 02/15] dt: psci: Update DT bindings to support hierarchical PSCI states

[Rob Herring]

On Tue, Jan 14, 2020 at 11:55 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Mon, 13 Jan 2020 at 20:53, Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Mon, Dec 30, 2019 at 8:44 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > Update PSCI DT bindings to allow to represent idle states for CPUs and the
> > > CPU topology, by using a hierarchical layout. Primarily this is done by
> > > re-using the existing DT bindings for PM domains [1] and for PM domain idle
> > > states [2].
> > >
> > > Let's also add an example into the document for the PSCI DT bindings, to
> > > clearly show the new hierarchical based layout. The currently supported
> > > flattened layout, is already described in the ARM idle states bindings [3],
> > > so let's leave that as is.
> > >
> > > [1] Documentation/devicetree/bindings/power/power_domain.txt
> > > [2] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > [3] Documentation/devicetree/bindings/arm/idle-states.txt
> > >
> > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >
> > > Changes in v5:
> > >         - None.
> >
> > First I'm seeing this as the DT list was not copied. The example has
> > problems when running 'make dt_binding_check':
> >
> > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > compatible: Additional items are not allowed ('arm,armv8' was
> > unexpected)
> > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > compatible: ['arm,cortex-a53', 'arm,armv8'] is too long
> > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > compatible: Additional items are not allowed ('arm,armv8' was
> > unexpected)
> > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > compatible: ['arm,cortex-a57', 'arm,armv8'] is too long
> >
> > 'arm,armv8' is only valid for s/w models.
>
> Perhaps you have a different version of the tools than I have (I have
> tried both on v.5.5-rc5 and todays linux-next), because I can't
> reproduce these errors at my side when running "make
> dt_binding_check".
>
> Can you please check again?

Are you setting DT_SCHEMA_FILES? If so, then arm/cpus.yaml (or any
other schema) isn't loaded and used for validation. That schema is the
source of this error.

It is failing in my CI job:
https://gitlab.com/robherring/linux-dt-bindings/-/jobs/405298185

Is dt-schema up to date? Though I can't think of any recent changes
that would impact this. This check has been there a while and I fixed
all the dts files.

Do you see psci.example.dt.yaml getting built?

> > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > idle-states: cluster-retention:compatible:0: 'arm,idle-state' was
> > expected
> > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > idle-states: cluster-power-down:compatible:0: 'arm,idle-state' was
> > expected
> >
> > The last 2 are due to my conversion of the idle-states binding which
> > is in my tree now. Probably need to add 'domain-idle-state' as a
> > compatible at a minimum. It looks like domain-idle-state.txt is pretty
> > much the same as arm/idle-state.txt, so we should perhaps merge them.
>
> Ahh, so maybe *all* of the above problems are caused by conflicts in
> the arm-soc tree with changes from your tree!?

Shouldn't be. arm/cpus.yaml has been in place for a few cycles now.

>
> In regards to merging files, I am fine by that if that helps.
>
> >
> > There's some bigger issues though.
> >
> > > ---
> > >  .../devicetree/bindings/arm/cpus.yaml         |  15 +++
> > >  .../devicetree/bindings/arm/psci.yaml         | 104 ++++++++++++++++++
> > >  2 files changed, 119 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > index c23c24ff7575..7a9c3ce2dbef 100644
> > > --- a/Documentation/devicetree/bindings/arm/cpus.yaml
> > > +++ b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > @@ -242,6 +242,21 @@ properties:
> > >
> > >        where voltage is in V, frequency is in MHz.
> > >
> > > +  power-domains:
> > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > +    description:
> > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > +      PM domain provider (see also ../power_domain.txt).
> > > +
> > > +  power-domain-names:
> > > +    $ref: '/schemas/types.yaml#/definitions/string-array'
> > > +    description:
> > > +      A list of power domain name strings sorted in the same order as the
> > > +      power-domains property.
> > > +
> > > +      For PSCI based platforms, the name corresponding to the index of the PSCI
> > > +      PM domain provider, must be "psci".
> > > +
> > >    qcom,saw:
> > >      $ref: '/schemas/types.yaml#/definitions/phandle'
> > >      description: |
> > > diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
> > > index 7abdf58b335e..8ef85420b2ab 100644
> > > --- a/Documentation/devicetree/bindings/arm/psci.yaml
> > > +++ b/Documentation/devicetree/bindings/arm/psci.yaml
> > > @@ -102,6 +102,34 @@ properties:
> > >        [1] Kernel documentation - ARM idle states bindings
> > >          Documentation/devicetree/bindings/arm/idle-states.txt
> > >
> > > +  "#power-domain-cells":
> >
> > This is wrong because you are saying the /psci node should have these
> > properties. You need to define the child nodes (at least a pattern you
> > can match on) and put these properties there.
>
> Right, good point.
>
> I searched for some similar examples for how to encode this, but
> couldn't really find something useful.

You need something like:

patternProperties:
  '^(cluster|cpu)-pd[0-9a-f]+$':
    type: object
    properties:
      ... and then the properties in the child nodes

Note that its going to look weird for the 10th PD with 'cpu-pda'. So
maybe add a '-'.

> One more thing, it seems like
> this change is also needed for the common power-domain bindings, as
> that also specifies parent/childs domains.

Normally, we'd have a $ref to power-domain.yaml, but for that to work
here, you'll have to expand the node names ($nodename).

>
> Anyway, I would really appreciate if you can suggest something more
> detailed for you think this should be done!?
>
> >
> > > +    description:
> > > +      The number of cells in a PM domain specifier as per binding in [3].
> > > +      Must be 0 as to represent a single PM domain.
> > > +
> > > +      ARM systems can have multiple cores, sometimes in an hierarchical
> > > +      arrangement. This often, but not always, maps directly to the processor
> > > +      power topology of the system. Individual nodes in a topology have their
> > > +      own specific power states and can be better represented hierarchically.
> > > +
> > > +      For these cases, the definitions of the idle states for the CPUs and the
> > > +      CPU topology, must conform to the binding in [3]. The idle states
> > > +      themselves must conform to the binding in [4] and must specify the
> > > +      arm,psci-suspend-param property.
> > > +
> > > +      It should also be noted that, in PSCI firmware v1.0 the OS-Initiated
> > > +      (OSI) CPU suspend mode is introduced. Using a hierarchical representation
> > > +      helps to implement support for OSI mode and OS implementations may choose
> > > +      to mandate it.
> > > +
> > > +      [3] Documentation/devicetree/bindings/power/power_domain.txt
> > > +      [4] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > +
> > > +  power-domains:
> > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > +    description:
> > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > +      PM domain provider.
> >
> > A schema for 'domain-idle-states' property is missing.
>
> Right, let's figure out the best way for how to add that.

If power-domain.yaml is referenced, then don't need anything else
unless you can define the number of phandles (looks like you can't?).

Rob

Re: [PATCH v5 02/15] dt: psci: Update DT bindings to support hierarchical PSCI states

[Ulf Hansson]

On Thu, 16 Jan 2020 at 19:19, Rob Herring <robh+dt@kernel.org> wrote:
>
> On Tue, Jan 14, 2020 at 11:55 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Mon, 13 Jan 2020 at 20:53, Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Mon, Dec 30, 2019 at 8:44 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > Update PSCI DT bindings to allow to represent idle states for CPUs and the
> > > > CPU topology, by using a hierarchical layout. Primarily this is done by
> > > > re-using the existing DT bindings for PM domains [1] and for PM domain idle
> > > > states [2].
> > > >
> > > > Let's also add an example into the document for the PSCI DT bindings, to
> > > > clearly show the new hierarchical based layout. The currently supported
> > > > flattened layout, is already described in the ARM idle states bindings [3],
> > > > so let's leave that as is.
> > > >
> > > > [1] Documentation/devicetree/bindings/power/power_domain.txt
> > > > [2] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > [3] Documentation/devicetree/bindings/arm/idle-states.txt
> > > >
> > > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> > > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > ---
> > > >
> > > > Changes in v5:
> > > >         - None.
> > >
> > > First I'm seeing this as the DT list was not copied. The example has
> > > problems when running 'make dt_binding_check':
> > >
> > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > compatible: Additional items are not allowed ('arm,armv8' was
> > > unexpected)
> > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > compatible: ['arm,cortex-a53', 'arm,armv8'] is too long
> > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > compatible: Additional items are not allowed ('arm,armv8' was
> > > unexpected)
> > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > compatible: ['arm,cortex-a57', 'arm,armv8'] is too long
> > >
> > > 'arm,armv8' is only valid for s/w models.
> >
> > Perhaps you have a different version of the tools than I have (I have
> > tried both on v.5.5-rc5 and todays linux-next), because I can't
> > reproduce these errors at my side when running "make
> > dt_binding_check".
> >
> > Can you please check again?
>
> Are you setting DT_SCHEMA_FILES? If so, then arm/cpus.yaml (or any
> other schema) isn't loaded and used for validation. That schema is the
> source of this error.

Yes. Aha, that's why then. Perhaps that needs to be clarified
somewhere in the documentation of tool.

Anyway, I used because it was kind of hard to process all the error
output one gets when building all yaml files at once.

>
> It is failing in my CI job:
> https://gitlab.com/robherring/linux-dt-bindings/-/jobs/405298185
>
> Is dt-schema up to date? Though I can't think of any recent changes
> that would impact this. This check has been there a while and I fixed
> all the dts files.
>
> Do you see psci.example.dt.yaml getting built?

Yes, but with using DT_SCHEMA_FILES.

Anyway, now I can re-produced the errors, so then I should be able to
fix them. :-)

>
> > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > idle-states: cluster-retention:compatible:0: 'arm,idle-state' was
> > > expected
> > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > idle-states: cluster-power-down:compatible:0: 'arm,idle-state' was
> > > expected
> > >
> > > The last 2 are due to my conversion of the idle-states binding which
> > > is in my tree now. Probably need to add 'domain-idle-state' as a
> > > compatible at a minimum. It looks like domain-idle-state.txt is pretty
> > > much the same as arm/idle-state.txt, so we should perhaps merge them.
> >
> > Ahh, so maybe *all* of the above problems are caused by conflicts in
> > the arm-soc tree with changes from your tree!?
>
> Shouldn't be. arm/cpus.yaml has been in place for a few cycles now.
>
> >
> > In regards to merging files, I am fine by that if that helps.
> >
> > >
> > > There's some bigger issues though.
> > >
> > > > ---
> > > >  .../devicetree/bindings/arm/cpus.yaml         |  15 +++
> > > >  .../devicetree/bindings/arm/psci.yaml         | 104 ++++++++++++++++++
> > > >  2 files changed, 119 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > index c23c24ff7575..7a9c3ce2dbef 100644
> > > > --- a/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > +++ b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > @@ -242,6 +242,21 @@ properties:
> > > >
> > > >        where voltage is in V, frequency is in MHz.
> > > >
> > > > +  power-domains:
> > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > +    description:
> > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > +      PM domain provider (see also ../power_domain.txt).
> > > > +
> > > > +  power-domain-names:
> > > > +    $ref: '/schemas/types.yaml#/definitions/string-array'
> > > > +    description:
> > > > +      A list of power domain name strings sorted in the same order as the
> > > > +      power-domains property.
> > > > +
> > > > +      For PSCI based platforms, the name corresponding to the index of the PSCI
> > > > +      PM domain provider, must be "psci".
> > > > +
> > > >    qcom,saw:
> > > >      $ref: '/schemas/types.yaml#/definitions/phandle'
> > > >      description: |
> > > > diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > index 7abdf58b335e..8ef85420b2ab 100644
> > > > --- a/Documentation/devicetree/bindings/arm/psci.yaml
> > > > +++ b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > @@ -102,6 +102,34 @@ properties:
> > > >        [1] Kernel documentation - ARM idle states bindings
> > > >          Documentation/devicetree/bindings/arm/idle-states.txt
> > > >
> > > > +  "#power-domain-cells":
> > >
> > > This is wrong because you are saying the /psci node should have these
> > > properties. You need to define the child nodes (at least a pattern you
> > > can match on) and put these properties there.
> >
> > Right, good point.
> >
> > I searched for some similar examples for how to encode this, but
> > couldn't really find something useful.
>
> You need something like:
>
> patternProperties:
>   '^(cluster|cpu)-pd[0-9a-f]+$':
>     type: object
>     properties:
>       ... and then the properties in the child nodes
>
> Note that its going to look weird for the 10th PD with 'cpu-pda'. So
> maybe add a '-'.
>

Great, I try this! Thanks.

> > One more thing, it seems like
> > this change is also needed for the common power-domain bindings, as
> > that also specifies parent/childs domains.
>
> Normally, we'd have a $ref to power-domain.yaml, but for that to work
> here, you'll have to expand the node names ($nodename).

Not sure I get that, but interpret this as it's not a good idea to use
a $ref to power-domain.yaml. Right?

>
> >
> > Anyway, I would really appreciate if you can suggest something more
> > detailed for you think this should be done!?
> >
> > >
> > > > +    description:
> > > > +      The number of cells in a PM domain specifier as per binding in [3].
> > > > +      Must be 0 as to represent a single PM domain.
> > > > +
> > > > +      ARM systems can have multiple cores, sometimes in an hierarchical
> > > > +      arrangement. This often, but not always, maps directly to the processor
> > > > +      power topology of the system. Individual nodes in a topology have their
> > > > +      own specific power states and can be better represented hierarchically.
> > > > +
> > > > +      For these cases, the definitions of the idle states for the CPUs and the
> > > > +      CPU topology, must conform to the binding in [3]. The idle states
> > > > +      themselves must conform to the binding in [4] and must specify the
> > > > +      arm,psci-suspend-param property.
> > > > +
> > > > +      It should also be noted that, in PSCI firmware v1.0 the OS-Initiated
> > > > +      (OSI) CPU suspend mode is introduced. Using a hierarchical representation
> > > > +      helps to implement support for OSI mode and OS implementations may choose
> > > > +      to mandate it.
> > > > +
> > > > +      [3] Documentation/devicetree/bindings/power/power_domain.txt
> > > > +      [4] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > +
> > > > +  power-domains:
> > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > +    description:
> > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > +      PM domain provider.
> > >
> > > A schema for 'domain-idle-states' property is missing.
> >
> > Right, let's figure out the best way for how to add that.
>
> If power-domain.yaml is referenced, then don't need anything else
> unless you can define the number of phandles (looks like you can't?).

The number phandles should be one. At least, I think we can start with
that and extend the binding if needed.

How would that look like then? Apologize for my ignorance (I really
need to spend some time learning this better)...

Kind regards
Uffe

Re: [PATCH v5 02/15] dt: psci: Update DT bindings to support hierarchical PSCI states

[Rob Herring]

On Fri, Jan 17, 2020 at 10:42 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Thu, 16 Jan 2020 at 19:19, Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Tue, Jan 14, 2020 at 11:55 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Mon, 13 Jan 2020 at 20:53, Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > On Mon, Dec 30, 2019 at 8:44 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > Update PSCI DT bindings to allow to represent idle states for CPUs and the
> > > > > CPU topology, by using a hierarchical layout. Primarily this is done by
> > > > > re-using the existing DT bindings for PM domains [1] and for PM domain idle
> > > > > states [2].
> > > > >
> > > > > Let's also add an example into the document for the PSCI DT bindings, to
> > > > > clearly show the new hierarchical based layout. The currently supported
> > > > > flattened layout, is already described in the ARM idle states bindings [3],
> > > > > so let's leave that as is.
> > > > >
> > > > > [1] Documentation/devicetree/bindings/power/power_domain.txt
> > > > > [2] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > > [3] Documentation/devicetree/bindings/arm/idle-states.txt
> > > > >
> > > > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> > > > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > > > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > ---
> > > > >
> > > > > Changes in v5:
> > > > >         - None.
> > > >
> > > > First I'm seeing this as the DT list was not copied. The example has
> > > > problems when running 'make dt_binding_check':
> > > >
> > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > > compatible: Additional items are not allowed ('arm,armv8' was
> > > > unexpected)
> > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > > compatible: ['arm,cortex-a53', 'arm,armv8'] is too long
> > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > > compatible: Additional items are not allowed ('arm,armv8' was
> > > > unexpected)
> > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > > compatible: ['arm,cortex-a57', 'arm,armv8'] is too long
> > > >
> > > > 'arm,armv8' is only valid for s/w models.
> > >
> > > Perhaps you have a different version of the tools than I have (I have
> > > tried both on v.5.5-rc5 and todays linux-next), because I can't
> > > reproduce these errors at my side when running "make
> > > dt_binding_check".
> > >
> > > Can you please check again?
> >
> > Are you setting DT_SCHEMA_FILES? If so, then arm/cpus.yaml (or any
> > other schema) isn't loaded and used for validation. That schema is the
> > source of this error.
>
> Yes. Aha, that's why then. Perhaps that needs to be clarified
> somewhere in the documentation of tool.

Patches welcome. :) I'm kind of tired of writing documentation that no
one comments on and and seemingly only sometimes read. </rant> :(

> Anyway, I used because it was kind of hard to process all the error
> output one gets when building all yaml files at once.

dtbs_check has a lot which is where setting DT_SCHEMA_FILES is
primarily useful. dt_binding_check should be error/warning free, but
yes linux-next and rc1/2 are frequently broken.

> > It is failing in my CI job:
> > https://gitlab.com/robherring/linux-dt-bindings/-/jobs/405298185
> >
> > Is dt-schema up to date? Though I can't think of any recent changes
> > that would impact this. This check has been there a while and I fixed
> > all the dts files.
> >
> > Do you see psci.example.dt.yaml getting built?
>
> Yes, but with using DT_SCHEMA_FILES.
>
> Anyway, now I can re-produced the errors, so then I should be able to
> fix them. :-)
>
> >
> > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > > idle-states: cluster-retention:compatible:0: 'arm,idle-state' was
> > > > expected
> > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > > idle-states: cluster-power-down:compatible:0: 'arm,idle-state' was
> > > > expected
> > > >
> > > > The last 2 are due to my conversion of the idle-states binding which
> > > > is in my tree now. Probably need to add 'domain-idle-state' as a
> > > > compatible at a minimum. It looks like domain-idle-state.txt is pretty
> > > > much the same as arm/idle-state.txt, so we should perhaps merge them.
> > >
> > > Ahh, so maybe *all* of the above problems are caused by conflicts in
> > > the arm-soc tree with changes from your tree!?
> >
> > Shouldn't be. arm/cpus.yaml has been in place for a few cycles now.
> >
> > >
> > > In regards to merging files, I am fine by that if that helps.
> > >
> > > >
> > > > There's some bigger issues though.
> > > >
> > > > > ---
> > > > >  .../devicetree/bindings/arm/cpus.yaml         |  15 +++
> > > > >  .../devicetree/bindings/arm/psci.yaml         | 104 ++++++++++++++++++
> > > > >  2 files changed, 119 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > index c23c24ff7575..7a9c3ce2dbef 100644
> > > > > --- a/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > +++ b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > @@ -242,6 +242,21 @@ properties:
> > > > >
> > > > >        where voltage is in V, frequency is in MHz.
> > > > >
> > > > > +  power-domains:
> > > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > > +    description:
> > > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > > +      PM domain provider (see also ../power_domain.txt).
> > > > > +
> > > > > +  power-domain-names:
> > > > > +    $ref: '/schemas/types.yaml#/definitions/string-array'
> > > > > +    description:
> > > > > +      A list of power domain name strings sorted in the same order as the
> > > > > +      power-domains property.
> > > > > +
> > > > > +      For PSCI based platforms, the name corresponding to the index of the PSCI
> > > > > +      PM domain provider, must be "psci".
> > > > > +
> > > > >    qcom,saw:
> > > > >      $ref: '/schemas/types.yaml#/definitions/phandle'
> > > > >      description: |
> > > > > diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > index 7abdf58b335e..8ef85420b2ab 100644
> > > > > --- a/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > +++ b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > @@ -102,6 +102,34 @@ properties:
> > > > >        [1] Kernel documentation - ARM idle states bindings
> > > > >          Documentation/devicetree/bindings/arm/idle-states.txt
> > > > >
> > > > > +  "#power-domain-cells":
> > > >
> > > > This is wrong because you are saying the /psci node should have these
> > > > properties. You need to define the child nodes (at least a pattern you
> > > > can match on) and put these properties there.
> > >
> > > Right, good point.
> > >
> > > I searched for some similar examples for how to encode this, but
> > > couldn't really find something useful.
> >
> > You need something like:
> >
> > patternProperties:
> >   '^(cluster|cpu)-pd[0-9a-f]+$':
> >     type: object
> >     properties:
> >       ... and then the properties in the child nodes
> >
> > Note that its going to look weird for the 10th PD with 'cpu-pda'. So
> > maybe add a '-'.
> >
>
> Great, I try this! Thanks.
>
> > > One more thing, it seems like
> > > this change is also needed for the common power-domain bindings, as
> > > that also specifies parent/childs domains.
> >
> > Normally, we'd have a $ref to power-domain.yaml, but for that to work
> > here, you'll have to expand the node names ($nodename).
>
> Not sure I get that, but interpret this as it's not a good idea to use
> a $ref to power-domain.yaml. Right?

It means either this binding is odd or power-domain.yaml needs some
more work or both. Ideally, we only have 1 type definition of any
property name.

Probably the easiest thing to do is extend the node name pattern to
something like this:

pattern: "^(power-controller|power-domain)([@\-].*)?$"

And then name your nodes like this:

power-domain-cpu-0
power-domain-cluster

That's more consistent anyways.

> > > Anyway, I would really appreciate if you can suggest something more
> > > detailed for you think this should be done!?
> > >
> > > >
> > > > > +    description:
> > > > > +      The number of cells in a PM domain specifier as per binding in [3].
> > > > > +      Must be 0 as to represent a single PM domain.
> > > > > +
> > > > > +      ARM systems can have multiple cores, sometimes in an hierarchical
> > > > > +      arrangement. This often, but not always, maps directly to the processor
> > > > > +      power topology of the system. Individual nodes in a topology have their
> > > > > +      own specific power states and can be better represented hierarchically.
> > > > > +
> > > > > +      For these cases, the definitions of the idle states for the CPUs and the
> > > > > +      CPU topology, must conform to the binding in [3]. The idle states
> > > > > +      themselves must conform to the binding in [4] and must specify the
> > > > > +      arm,psci-suspend-param property.
> > > > > +
> > > > > +      It should also be noted that, in PSCI firmware v1.0 the OS-Initiated
> > > > > +      (OSI) CPU suspend mode is introduced. Using a hierarchical representation
> > > > > +      helps to implement support for OSI mode and OS implementations may choose
> > > > > +      to mandate it.
> > > > > +
> > > > > +      [3] Documentation/devicetree/bindings/power/power_domain.txt
> > > > > +      [4] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > > +
> > > > > +  power-domains:
> > > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > > +    description:
> > > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > > +      PM domain provider.
> > > >
> > > > A schema for 'domain-idle-states' property is missing.
> > >
> > > Right, let's figure out the best way for how to add that.
> >
> > If power-domain.yaml is referenced, then don't need anything else
> > unless you can define the number of phandles (looks like you can't?).
>
> The number phandles should be one. At least, I think we can start with
> that and extend the binding if needed.

But there's 2 for the cluster in the example.

> How would that look like then? Apologize for my ignorance (I really
> need to spend some time learning this better)...

Basically, anything common that can be an multiple entries is defined as:

power-domains:
  maxItems: 1

if only 1 entry or for multiple you need to define each entry:

power-domains:
  items:
    - description: what the first power domain is
    - description: what the 2nd power domain is

Rob

Re: [PATCH v5 02/15] dt: psci: Update DT bindings to support hierarchical PSCI states

[Ulf Hansson]

On Fri, 17 Jan 2020 at 18:36, Rob Herring <robh+dt@kernel.org> wrote:
>
> On Fri, Jan 17, 2020 at 10:42 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Thu, 16 Jan 2020 at 19:19, Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Tue, Jan 14, 2020 at 11:55 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > On Mon, 13 Jan 2020 at 20:53, Rob Herring <robh+dt@kernel.org> wrote:
> > > > >
> > > > > On Mon, Dec 30, 2019 at 8:44 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > >
> > > > > > Update PSCI DT bindings to allow to represent idle states for CPUs and the
> > > > > > CPU topology, by using a hierarchical layout. Primarily this is done by
> > > > > > re-using the existing DT bindings for PM domains [1] and for PM domain idle
> > > > > > states [2].
> > > > > >
> > > > > > Let's also add an example into the document for the PSCI DT bindings, to
> > > > > > clearly show the new hierarchical based layout. The currently supported
> > > > > > flattened layout, is already described in the ARM idle states bindings [3],
> > > > > > so let's leave that as is.
> > > > > >
> > > > > > [1] Documentation/devicetree/bindings/power/power_domain.txt
> > > > > > [2] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > > > [3] Documentation/devicetree/bindings/arm/idle-states.txt
> > > > > >
> > > > > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> > > > > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > > > > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > > ---
> > > > > >
> > > > > > Changes in v5:
> > > > > >         - None.
> > > > >
> > > > > First I'm seeing this as the DT list was not copied. The example has
> > > > > problems when running 'make dt_binding_check':
> > > > >
> > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > > > compatible: Additional items are not allowed ('arm,armv8' was
> > > > > unexpected)
> > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > > > compatible: ['arm,cortex-a53', 'arm,armv8'] is too long
> > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > > > compatible: Additional items are not allowed ('arm,armv8' was
> > > > > unexpected)
> > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > > > compatible: ['arm,cortex-a57', 'arm,armv8'] is too long
> > > > >
> > > > > 'arm,armv8' is only valid for s/w models.
> > > >
> > > > Perhaps you have a different version of the tools than I have (I have
> > > > tried both on v.5.5-rc5 and todays linux-next), because I can't
> > > > reproduce these errors at my side when running "make
> > > > dt_binding_check".
> > > >
> > > > Can you please check again?
> > >
> > > Are you setting DT_SCHEMA_FILES? If so, then arm/cpus.yaml (or any
> > > other schema) isn't loaded and used for validation. That schema is the
> > > source of this error.
> >
> > Yes. Aha, that's why then. Perhaps that needs to be clarified
> > somewhere in the documentation of tool.
>
> Patches welcome. :) I'm kind of tired of writing documentation that no
> one comments on and and seemingly only sometimes read. </rant> :(

I understand your concerns. A patch is on it's way.

>
> > Anyway, I used because it was kind of hard to process all the error
> > output one gets when building all yaml files at once.
>
> dtbs_check has a lot which is where setting DT_SCHEMA_FILES is
> primarily useful. dt_binding_check should be error/warning free, but
> yes linux-next and rc1/2 are frequently broken.
>
> > > It is failing in my CI job:
> > > https://gitlab.com/robherring/linux-dt-bindings/-/jobs/405298185
> > >
> > > Is dt-schema up to date? Though I can't think of any recent changes
> > > that would impact this. This check has been there a while and I fixed
> > > all the dts files.
> > >
> > > Do you see psci.example.dt.yaml getting built?
> >
> > Yes, but with using DT_SCHEMA_FILES.
> >
> > Anyway, now I can re-produced the errors, so then I should be able to
> > fix them. :-)
> >
> > >
> > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > > > idle-states: cluster-retention:compatible:0: 'arm,idle-state' was
> > > > > expected
> > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > > > idle-states: cluster-power-down:compatible:0: 'arm,idle-state' was
> > > > > expected
> > > > >
> > > > > The last 2 are due to my conversion of the idle-states binding which
> > > > > is in my tree now. Probably need to add 'domain-idle-state' as a
> > > > > compatible at a minimum. It looks like domain-idle-state.txt is pretty
> > > > > much the same as arm/idle-state.txt, so we should perhaps merge them.
> > > >
> > > > Ahh, so maybe *all* of the above problems are caused by conflicts in
> > > > the arm-soc tree with changes from your tree!?
> > >
> > > Shouldn't be. arm/cpus.yaml has been in place for a few cycles now.
> > >
> > > >
> > > > In regards to merging files, I am fine by that if that helps.
> > > >
> > > > >
> > > > > There's some bigger issues though.
> > > > >
> > > > > > ---
> > > > > >  .../devicetree/bindings/arm/cpus.yaml         |  15 +++
> > > > > >  .../devicetree/bindings/arm/psci.yaml         | 104 ++++++++++++++++++
> > > > > >  2 files changed, 119 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > index c23c24ff7575..7a9c3ce2dbef 100644
> > > > > > --- a/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > @@ -242,6 +242,21 @@ properties:
> > > > > >
> > > > > >        where voltage is in V, frequency is in MHz.
> > > > > >
> > > > > > +  power-domains:
> > > > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > > > +    description:
> > > > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > > > +      PM domain provider (see also ../power_domain.txt).
> > > > > > +
> > > > > > +  power-domain-names:
> > > > > > +    $ref: '/schemas/types.yaml#/definitions/string-array'
> > > > > > +    description:
> > > > > > +      A list of power domain name strings sorted in the same order as the
> > > > > > +      power-domains property.
> > > > > > +
> > > > > > +      For PSCI based platforms, the name corresponding to the index of the PSCI
> > > > > > +      PM domain provider, must be "psci".
> > > > > > +
> > > > > >    qcom,saw:
> > > > > >      $ref: '/schemas/types.yaml#/definitions/phandle'
> > > > > >      description: |
> > > > > > diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > index 7abdf58b335e..8ef85420b2ab 100644
> > > > > > --- a/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > @@ -102,6 +102,34 @@ properties:
> > > > > >        [1] Kernel documentation - ARM idle states bindings
> > > > > >          Documentation/devicetree/bindings/arm/idle-states.txt
> > > > > >
> > > > > > +  "#power-domain-cells":
> > > > >
> > > > > This is wrong because you are saying the /psci node should have these
> > > > > properties. You need to define the child nodes (at least a pattern you
> > > > > can match on) and put these properties there.
> > > >
> > > > Right, good point.
> > > >
> > > > I searched for some similar examples for how to encode this, but
> > > > couldn't really find something useful.
> > >
> > > You need something like:
> > >
> > > patternProperties:
> > >   '^(cluster|cpu)-pd[0-9a-f]+$':
> > >     type: object
> > >     properties:
> > >       ... and then the properties in the child nodes
> > >
> > > Note that its going to look weird for the 10th PD with 'cpu-pda'. So
> > > maybe add a '-'.
> > >
> >
> > Great, I try this! Thanks.
> >
> > > > One more thing, it seems like
> > > > this change is also needed for the common power-domain bindings, as
> > > > that also specifies parent/childs domains.
> > >
> > > Normally, we'd have a $ref to power-domain.yaml, but for that to work
> > > here, you'll have to expand the node names ($nodename).
> >
> > Not sure I get that, but interpret this as it's not a good idea to use
> > a $ref to power-domain.yaml. Right?
>
> It means either this binding is odd or power-domain.yaml needs some
> more work or both. Ideally, we only have 1 type definition of any
> property name.
>
> Probably the easiest thing to do is extend the node name pattern to
> something like this:
>
> pattern: "^(power-controller|power-domain)([@\-].*)?$"
>
> And then name your nodes like this:
>
> power-domain-cpu-0
> power-domain-cluster
>
> That's more consistent anyways.

Looks like a good idea! I try that.

>
> > > > Anyway, I would really appreciate if you can suggest something more
> > > > detailed for you think this should be done!?
> > > >
> > > > >
> > > > > > +    description:
> > > > > > +      The number of cells in a PM domain specifier as per binding in [3].
> > > > > > +      Must be 0 as to represent a single PM domain.
> > > > > > +
> > > > > > +      ARM systems can have multiple cores, sometimes in an hierarchical
> > > > > > +      arrangement. This often, but not always, maps directly to the processor
> > > > > > +      power topology of the system. Individual nodes in a topology have their
> > > > > > +      own specific power states and can be better represented hierarchically.
> > > > > > +
> > > > > > +      For these cases, the definitions of the idle states for the CPUs and the
> > > > > > +      CPU topology, must conform to the binding in [3]. The idle states
> > > > > > +      themselves must conform to the binding in [4] and must specify the
> > > > > > +      arm,psci-suspend-param property.
> > > > > > +
> > > > > > +      It should also be noted that, in PSCI firmware v1.0 the OS-Initiated
> > > > > > +      (OSI) CPU suspend mode is introduced. Using a hierarchical representation
> > > > > > +      helps to implement support for OSI mode and OS implementations may choose
> > > > > > +      to mandate it.
> > > > > > +
> > > > > > +      [3] Documentation/devicetree/bindings/power/power_domain.txt
> > > > > > +      [4] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > > > +
> > > > > > +  power-domains:
> > > > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > > > +    description:
> > > > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > > > +      PM domain provider.
> > > > >
> > > > > A schema for 'domain-idle-states' property is missing.
> > > >
> > > > Right, let's figure out the best way for how to add that.
> > >
> > > If power-domain.yaml is referenced, then don't need anything else
> > > unless you can define the number of phandles (looks like you can't?).
> >
> > The number phandles should be one. At least, I think we can start with
> > that and extend the binding if needed.
>
> But there's 2 for the cluster in the example.

What example do you refer to?

For each power controller node for psci, only one phandle needs to be
specified in "power-domains", as that should be sufficient to describe
the topology.

When it comes to the CPU node, multiple phandles may need to specified
in "power-domains".

>
> > How would that look like then? Apologize for my ignorance (I really
> > need to spend some time learning this better)...
>
> Basically, anything common that can be an multiple entries is defined as:
>
> power-domains:
>   maxItems: 1
>
> if only 1 entry or for multiple you need to define each entry:
>
> power-domains:
>   items:
>     - description: what the first power domain is
>     - description: what the 2nd power domain is

Okay.

This should be fine for the PSCI power controller node, but for the
generic binding in cpus.yaml I am not sure how to describe this as
that would be SoC specific.

The number of phandles in power-domains could differ between SoCs at
CPU node. However, at most, one would be a phandle for PSCI. If
additional those would be for something different.

Kind regards
Uffe

Re: [PATCH v5 02/15] dt: psci: Update DT bindings to support hierarchical PSCI states

[Rob Herring]

On Mon, Jan 20, 2020 at 6:57 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 17 Jan 2020 at 18:36, Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Fri, Jan 17, 2020 at 10:42 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Thu, 16 Jan 2020 at 19:19, Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > On Tue, Jan 14, 2020 at 11:55 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > On Mon, 13 Jan 2020 at 20:53, Rob Herring <robh+dt@kernel.org> wrote:
> > > > > >
> > > > > > On Mon, Dec 30, 2019 at 8:44 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > > >
> > > > > > > Update PSCI DT bindings to allow to represent idle states for CPUs and the
> > > > > > > CPU topology, by using a hierarchical layout. Primarily this is done by
> > > > > > > re-using the existing DT bindings for PM domains [1] and for PM domain idle
> > > > > > > states [2].
> > > > > > >
> > > > > > > Let's also add an example into the document for the PSCI DT bindings, to
> > > > > > > clearly show the new hierarchical based layout. The currently supported
> > > > > > > flattened layout, is already described in the ARM idle states bindings [3],
> > > > > > > so let's leave that as is.
> > > > > > >
> > > > > > > [1] Documentation/devicetree/bindings/power/power_domain.txt
> > > > > > > [2] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > > > > [3] Documentation/devicetree/bindings/arm/idle-states.txt
> > > > > > >
> > > > > > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> > > > > > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > > > > > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > > > ---
> > > > > > >
> > > > > > > Changes in v5:
> > > > > > >         - None.
> > > > > >
> > > > > > First I'm seeing this as the DT list was not copied. The example has
> > > > > > problems when running 'make dt_binding_check':
> > > > > >
> > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > > > > compatible: Additional items are not allowed ('arm,armv8' was
> > > > > > unexpected)
> > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > > > > compatible: ['arm,cortex-a53', 'arm,armv8'] is too long
> > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > > > > compatible: Additional items are not allowed ('arm,armv8' was
> > > > > > unexpected)
> > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > > > > compatible: ['arm,cortex-a57', 'arm,armv8'] is too long
> > > > > >
> > > > > > 'arm,armv8' is only valid for s/w models.
> > > > >
> > > > > Perhaps you have a different version of the tools than I have (I have
> > > > > tried both on v.5.5-rc5 and todays linux-next), because I can't
> > > > > reproduce these errors at my side when running "make
> > > > > dt_binding_check".
> > > > >
> > > > > Can you please check again?
> > > >
> > > > Are you setting DT_SCHEMA_FILES? If so, then arm/cpus.yaml (or any
> > > > other schema) isn't loaded and used for validation. That schema is the
> > > > source of this error.
> > >
> > > Yes. Aha, that's why then. Perhaps that needs to be clarified
> > > somewhere in the documentation of tool.
> >
> > Patches welcome. :) I'm kind of tired of writing documentation that no
> > one comments on and and seemingly only sometimes read. </rant> :(
>
> I understand your concerns. A patch is on it's way.
>
> >
> > > Anyway, I used because it was kind of hard to process all the error
> > > output one gets when building all yaml files at once.
> >
> > dtbs_check has a lot which is where setting DT_SCHEMA_FILES is
> > primarily useful. dt_binding_check should be error/warning free, but
> > yes linux-next and rc1/2 are frequently broken.
> >
> > > > It is failing in my CI job:
> > > > https://gitlab.com/robherring/linux-dt-bindings/-/jobs/405298185
> > > >
> > > > Is dt-schema up to date? Though I can't think of any recent changes
> > > > that would impact this. This check has been there a while and I fixed
> > > > all the dts files.
> > > >
> > > > Do you see psci.example.dt.yaml getting built?
> > >
> > > Yes, but with using DT_SCHEMA_FILES.
> > >
> > > Anyway, now I can re-produced the errors, so then I should be able to
> > > fix them. :-)
> > >
> > > >
> > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > > > > idle-states: cluster-retention:compatible:0: 'arm,idle-state' was
> > > > > > expected
> > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > > > > idle-states: cluster-power-down:compatible:0: 'arm,idle-state' was
> > > > > > expected
> > > > > >
> > > > > > The last 2 are due to my conversion of the idle-states binding which
> > > > > > is in my tree now. Probably need to add 'domain-idle-state' as a
> > > > > > compatible at a minimum. It looks like domain-idle-state.txt is pretty
> > > > > > much the same as arm/idle-state.txt, so we should perhaps merge them.
> > > > >
> > > > > Ahh, so maybe *all* of the above problems are caused by conflicts in
> > > > > the arm-soc tree with changes from your tree!?
> > > >
> > > > Shouldn't be. arm/cpus.yaml has been in place for a few cycles now.
> > > >
> > > > >
> > > > > In regards to merging files, I am fine by that if that helps.
> > > > >
> > > > > >
> > > > > > There's some bigger issues though.
> > > > > >
> > > > > > > ---
> > > > > > >  .../devicetree/bindings/arm/cpus.yaml         |  15 +++
> > > > > > >  .../devicetree/bindings/arm/psci.yaml         | 104 ++++++++++++++++++
> > > > > > >  2 files changed, 119 insertions(+)
> > > > > > >
> > > > > > > diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > > index c23c24ff7575..7a9c3ce2dbef 100644
> > > > > > > --- a/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > > +++ b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > > @@ -242,6 +242,21 @@ properties:
> > > > > > >
> > > > > > >        where voltage is in V, frequency is in MHz.
> > > > > > >
> > > > > > > +  power-domains:
> > > > > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > > > > +    description:
> > > > > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > > > > +      PM domain provider (see also ../power_domain.txt).
> > > > > > > +
> > > > > > > +  power-domain-names:
> > > > > > > +    $ref: '/schemas/types.yaml#/definitions/string-array'
> > > > > > > +    description:
> > > > > > > +      A list of power domain name strings sorted in the same order as the
> > > > > > > +      power-domains property.
> > > > > > > +
> > > > > > > +      For PSCI based platforms, the name corresponding to the index of the PSCI
> > > > > > > +      PM domain provider, must be "psci".
> > > > > > > +
> > > > > > >    qcom,saw:
> > > > > > >      $ref: '/schemas/types.yaml#/definitions/phandle'
> > > > > > >      description: |
> > > > > > > diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > > index 7abdf58b335e..8ef85420b2ab 100644
> > > > > > > --- a/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > > +++ b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > > @@ -102,6 +102,34 @@ properties:
> > > > > > >        [1] Kernel documentation - ARM idle states bindings
> > > > > > >          Documentation/devicetree/bindings/arm/idle-states.txt
> > > > > > >
> > > > > > > +  "#power-domain-cells":
> > > > > >
> > > > > > This is wrong because you are saying the /psci node should have these
> > > > > > properties. You need to define the child nodes (at least a pattern you
> > > > > > can match on) and put these properties there.
> > > > >
> > > > > Right, good point.
> > > > >
> > > > > I searched for some similar examples for how to encode this, but
> > > > > couldn't really find something useful.
> > > >
> > > > You need something like:
> > > >
> > > > patternProperties:
> > > >   '^(cluster|cpu)-pd[0-9a-f]+$':
> > > >     type: object
> > > >     properties:
> > > >       ... and then the properties in the child nodes
> > > >
> > > > Note that its going to look weird for the 10th PD with 'cpu-pda'. So
> > > > maybe add a '-'.
> > > >
> > >
> > > Great, I try this! Thanks.
> > >
> > > > > One more thing, it seems like
> > > > > this change is also needed for the common power-domain bindings, as
> > > > > that also specifies parent/childs domains.
> > > >
> > > > Normally, we'd have a $ref to power-domain.yaml, but for that to work
> > > > here, you'll have to expand the node names ($nodename).
> > >
> > > Not sure I get that, but interpret this as it's not a good idea to use
> > > a $ref to power-domain.yaml. Right?
> >
> > It means either this binding is odd or power-domain.yaml needs some
> > more work or both. Ideally, we only have 1 type definition of any
> > property name.
> >
> > Probably the easiest thing to do is extend the node name pattern to
> > something like this:
> >
> > pattern: "^(power-controller|power-domain)([@\-].*)?$"
> >
> > And then name your nodes like this:
> >
> > power-domain-cpu-0
> > power-domain-cluster
> >
> > That's more consistent anyways.
>
> Looks like a good idea! I try that.
>
> >
> > > > > Anyway, I would really appreciate if you can suggest something more
> > > > > detailed for you think this should be done!?
> > > > >
> > > > > >
> > > > > > > +    description:
> > > > > > > +      The number of cells in a PM domain specifier as per binding in [3].
> > > > > > > +      Must be 0 as to represent a single PM domain.
> > > > > > > +
> > > > > > > +      ARM systems can have multiple cores, sometimes in an hierarchical
> > > > > > > +      arrangement. This often, but not always, maps directly to the processor
> > > > > > > +      power topology of the system. Individual nodes in a topology have their
> > > > > > > +      own specific power states and can be better represented hierarchically.
> > > > > > > +
> > > > > > > +      For these cases, the definitions of the idle states for the CPUs and the
> > > > > > > +      CPU topology, must conform to the binding in [3]. The idle states
> > > > > > > +      themselves must conform to the binding in [4] and must specify the
> > > > > > > +      arm,psci-suspend-param property.
> > > > > > > +
> > > > > > > +      It should also be noted that, in PSCI firmware v1.0 the OS-Initiated
> > > > > > > +      (OSI) CPU suspend mode is introduced. Using a hierarchical representation
> > > > > > > +      helps to implement support for OSI mode and OS implementations may choose
> > > > > > > +      to mandate it.
> > > > > > > +
> > > > > > > +      [3] Documentation/devicetree/bindings/power/power_domain.txt
> > > > > > > +      [4] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > > > > +
> > > > > > > +  power-domains:
> > > > > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > > > > +    description:
> > > > > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > > > > +      PM domain provider.
> > > > > >
> > > > > > A schema for 'domain-idle-states' property is missing.
> > > > >
> > > > > Right, let's figure out the best way for how to add that.
> > > >
> > > > If power-domain.yaml is referenced, then don't need anything else
> > > > unless you can define the number of phandles (looks like you can't?).
> > >
> > > The number phandles should be one. At least, I think we can start with
> > > that and extend the binding if needed.
> >
> > But there's 2 for the cluster in the example.
>
> What example do you refer to?
>
> For each power controller node for psci, only one phandle needs to be
> specified in "power-domains", as that should be sufficient to describe
> the topology.

I was referring to 'domain-idle-states' in this patch:

+      CLUSTER_PD: cluster-pd {
+        #power-domain-cells = <0>;
+        domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWRDN>;
+      };


Rob

Re: [PATCH v5 02/15] dt: psci: Update DT bindings to support hierarchical PSCI states

[Rob Herring]

On Tue, Jan 21, 2020 at 10:59 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Mon, Jan 20, 2020 at 6:57 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Fri, 17 Jan 2020 at 18:36, Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Fri, Jan 17, 2020 at 10:42 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > On Thu, 16 Jan 2020 at 19:19, Rob Herring <robh+dt@kernel.org> wrote:
> > > > >
> > > > > On Tue, Jan 14, 2020 at 11:55 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > >
> > > > > > On Mon, 13 Jan 2020 at 20:53, Rob Herring <robh+dt@kernel.org> wrote:
> > > > > > >
> > > > > > > On Mon, Dec 30, 2019 at 8:44 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > > > >
> > > > > > > > Update PSCI DT bindings to allow to represent idle states for CPUs and the
> > > > > > > > CPU topology, by using a hierarchical layout. Primarily this is done by
> > > > > > > > re-using the existing DT bindings for PM domains [1] and for PM domain idle
> > > > > > > > states [2].
> > > > > > > >
> > > > > > > > Let's also add an example into the document for the PSCI DT bindings, to
> > > > > > > > clearly show the new hierarchical based layout. The currently supported
> > > > > > > > flattened layout, is already described in the ARM idle states bindings [3],
> > > > > > > > so let's leave that as is.
> > > > > > > >
> > > > > > > > [1] Documentation/devicetree/bindings/power/power_domain.txt
> > > > > > > > [2] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > > > > > [3] Documentation/devicetree/bindings/arm/idle-states.txt
> > > > > > > >
> > > > > > > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> > > > > > > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > > > > > > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > > > > ---
> > > > > > > >
> > > > > > > > Changes in v5:
> > > > > > > >         - None.
> > > > > > >
> > > > > > > First I'm seeing this as the DT list was not copied. The example has
> > > > > > > problems when running 'make dt_binding_check':
> > > > > > >
> > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > > > > > compatible: Additional items are not allowed ('arm,armv8' was
> > > > > > > unexpected)
> > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > > > > > compatible: ['arm,cortex-a53', 'arm,armv8'] is too long
> > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > > > > > compatible: Additional items are not allowed ('arm,armv8' was
> > > > > > > unexpected)
> > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > > > > > compatible: ['arm,cortex-a57', 'arm,armv8'] is too long
> > > > > > >
> > > > > > > 'arm,armv8' is only valid for s/w models.
> > > > > >
> > > > > > Perhaps you have a different version of the tools than I have (I have
> > > > > > tried both on v.5.5-rc5 and todays linux-next), because I can't
> > > > > > reproduce these errors at my side when running "make
> > > > > > dt_binding_check".
> > > > > >
> > > > > > Can you please check again?
> > > > >
> > > > > Are you setting DT_SCHEMA_FILES? If so, then arm/cpus.yaml (or any
> > > > > other schema) isn't loaded and used for validation. That schema is the
> > > > > source of this error.
> > > >
> > > > Yes. Aha, that's why then. Perhaps that needs to be clarified
> > > > somewhere in the documentation of tool.
> > >
> > > Patches welcome. :) I'm kind of tired of writing documentation that no
> > > one comments on and and seemingly only sometimes read. </rant> :(
> >
> > I understand your concerns. A patch is on it's way.
> >
> > >
> > > > Anyway, I used because it was kind of hard to process all the error
> > > > output one gets when building all yaml files at once.
> > >
> > > dtbs_check has a lot which is where setting DT_SCHEMA_FILES is
> > > primarily useful. dt_binding_check should be error/warning free, but
> > > yes linux-next and rc1/2 are frequently broken.
> > >
> > > > > It is failing in my CI job:
> > > > > https://gitlab.com/robherring/linux-dt-bindings/-/jobs/405298185
> > > > >
> > > > > Is dt-schema up to date? Though I can't think of any recent changes
> > > > > that would impact this. This check has been there a while and I fixed
> > > > > all the dts files.
> > > > >
> > > > > Do you see psci.example.dt.yaml getting built?
> > > >
> > > > Yes, but with using DT_SCHEMA_FILES.
> > > >
> > > > Anyway, now I can re-produced the errors, so then I should be able to
> > > > fix them. :-)
> > > >
> > > > >
> > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > > > > > idle-states: cluster-retention:compatible:0: 'arm,idle-state' was
> > > > > > > expected
> > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > > > > > idle-states: cluster-power-down:compatible:0: 'arm,idle-state' was
> > > > > > > expected
> > > > > > >
> > > > > > > The last 2 are due to my conversion of the idle-states binding which
> > > > > > > is in my tree now. Probably need to add 'domain-idle-state' as a
> > > > > > > compatible at a minimum. It looks like domain-idle-state.txt is pretty
> > > > > > > much the same as arm/idle-state.txt, so we should perhaps merge them.
> > > > > >
> > > > > > Ahh, so maybe *all* of the above problems are caused by conflicts in
> > > > > > the arm-soc tree with changes from your tree!?
> > > > >
> > > > > Shouldn't be. arm/cpus.yaml has been in place for a few cycles now.
> > > > >
> > > > > >
> > > > > > In regards to merging files, I am fine by that if that helps.
> > > > > >
> > > > > > >
> > > > > > > There's some bigger issues though.
> > > > > > >
> > > > > > > > ---
> > > > > > > >  .../devicetree/bindings/arm/cpus.yaml         |  15 +++
> > > > > > > >  .../devicetree/bindings/arm/psci.yaml         | 104 ++++++++++++++++++
> > > > > > > >  2 files changed, 119 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > > > index c23c24ff7575..7a9c3ce2dbef 100644
> > > > > > > > --- a/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > > > +++ b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > > > @@ -242,6 +242,21 @@ properties:
> > > > > > > >
> > > > > > > >        where voltage is in V, frequency is in MHz.
> > > > > > > >
> > > > > > > > +  power-domains:
> > > > > > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > > > > > +    description:
> > > > > > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > > > > > +      PM domain provider (see also ../power_domain.txt).
> > > > > > > > +
> > > > > > > > +  power-domain-names:
> > > > > > > > +    $ref: '/schemas/types.yaml#/definitions/string-array'
> > > > > > > > +    description:
> > > > > > > > +      A list of power domain name strings sorted in the same order as the
> > > > > > > > +      power-domains property.
> > > > > > > > +
> > > > > > > > +      For PSCI based platforms, the name corresponding to the index of the PSCI
> > > > > > > > +      PM domain provider, must be "psci".
> > > > > > > > +
> > > > > > > >    qcom,saw:
> > > > > > > >      $ref: '/schemas/types.yaml#/definitions/phandle'
> > > > > > > >      description: |
> > > > > > > > diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > > > index 7abdf58b335e..8ef85420b2ab 100644
> > > > > > > > --- a/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > > > +++ b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > > > @@ -102,6 +102,34 @@ properties:
> > > > > > > >        [1] Kernel documentation - ARM idle states bindings
> > > > > > > >          Documentation/devicetree/bindings/arm/idle-states.txt
> > > > > > > >
> > > > > > > > +  "#power-domain-cells":
> > > > > > >
> > > > > > > This is wrong because you are saying the /psci node should have these
> > > > > > > properties. You need to define the child nodes (at least a pattern you
> > > > > > > can match on) and put these properties there.
> > > > > >
> > > > > > Right, good point.
> > > > > >
> > > > > > I searched for some similar examples for how to encode this, but
> > > > > > couldn't really find something useful.
> > > > >
> > > > > You need something like:
> > > > >
> > > > > patternProperties:
> > > > >   '^(cluster|cpu)-pd[0-9a-f]+$':
> > > > >     type: object
> > > > >     properties:
> > > > >       ... and then the properties in the child nodes
> > > > >
> > > > > Note that its going to look weird for the 10th PD with 'cpu-pda'. So
> > > > > maybe add a '-'.
> > > > >
> > > >
> > > > Great, I try this! Thanks.
> > > >
> > > > > > One more thing, it seems like
> > > > > > this change is also needed for the common power-domain bindings, as
> > > > > > that also specifies parent/childs domains.
> > > > >
> > > > > Normally, we'd have a $ref to power-domain.yaml, but for that to work
> > > > > here, you'll have to expand the node names ($nodename).
> > > >
> > > > Not sure I get that, but interpret this as it's not a good idea to use
> > > > a $ref to power-domain.yaml. Right?
> > >
> > > It means either this binding is odd or power-domain.yaml needs some
> > > more work or both. Ideally, we only have 1 type definition of any
> > > property name.
> > >
> > > Probably the easiest thing to do is extend the node name pattern to
> > > something like this:
> > >
> > > pattern: "^(power-controller|power-domain)([@\-].*)?$"
> > >
> > > And then name your nodes like this:
> > >
> > > power-domain-cpu-0
> > > power-domain-cluster
> > >
> > > That's more consistent anyways.
> >
> > Looks like a good idea! I try that.
> >
> > >
> > > > > > Anyway, I would really appreciate if you can suggest something more
> > > > > > detailed for you think this should be done!?
> > > > > >
> > > > > > >
> > > > > > > > +    description:
> > > > > > > > +      The number of cells in a PM domain specifier as per binding in [3].
> > > > > > > > +      Must be 0 as to represent a single PM domain.
> > > > > > > > +
> > > > > > > > +      ARM systems can have multiple cores, sometimes in an hierarchical
> > > > > > > > +      arrangement. This often, but not always, maps directly to the processor
> > > > > > > > +      power topology of the system. Individual nodes in a topology have their
> > > > > > > > +      own specific power states and can be better represented hierarchically.
> > > > > > > > +
> > > > > > > > +      For these cases, the definitions of the idle states for the CPUs and the
> > > > > > > > +      CPU topology, must conform to the binding in [3]. The idle states
> > > > > > > > +      themselves must conform to the binding in [4] and must specify the
> > > > > > > > +      arm,psci-suspend-param property.
> > > > > > > > +
> > > > > > > > +      It should also be noted that, in PSCI firmware v1.0 the OS-Initiated
> > > > > > > > +      (OSI) CPU suspend mode is introduced. Using a hierarchical representation
> > > > > > > > +      helps to implement support for OSI mode and OS implementations may choose
> > > > > > > > +      to mandate it.
> > > > > > > > +
> > > > > > > > +      [3] Documentation/devicetree/bindings/power/power_domain.txt
> > > > > > > > +      [4] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > > > > > +
> > > > > > > > +  power-domains:
> > > > > > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > > > > > +    description:
> > > > > > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > > > > > +      PM domain provider.
> > > > > > >
> > > > > > > A schema for 'domain-idle-states' property is missing.
> > > > > >
> > > > > > Right, let's figure out the best way for how to add that.
> > > > >
> > > > > If power-domain.yaml is referenced, then don't need anything else
> > > > > unless you can define the number of phandles (looks like you can't?).
> > > >
> > > > The number phandles should be one. At least, I think we can start with
> > > > that and extend the binding if needed.
> > >
> > > But there's 2 for the cluster in the example.
> >
> > What example do you refer to?
> >
> > For each power controller node for psci, only one phandle needs to be
> > specified in "power-domains", as that should be sufficient to describe
> > the topology.
>
> I was referring to 'domain-idle-states' in this patch:
>
> +      CLUSTER_PD: cluster-pd {
> +        #power-domain-cells = <0>;
> +        domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWRDN>;
> +      };

Going to send a patch for all this? I'd like to not have warnings in v5.6.

Rob

Re: [PATCH v5 02/15] dt: psci: Update DT bindings to support hierarchical PSCI states

[Ulf Hansson]

On Mon, 24 Feb 2020 at 22:07, Rob Herring <robh+dt@kernel.org> wrote:
>
> On Tue, Jan 21, 2020 at 10:59 AM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Mon, Jan 20, 2020 at 6:57 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Fri, 17 Jan 2020 at 18:36, Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > On Fri, Jan 17, 2020 at 10:42 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > On Thu, 16 Jan 2020 at 19:19, Rob Herring <robh+dt@kernel.org> wrote:
> > > > > >
> > > > > > On Tue, Jan 14, 2020 at 11:55 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > > >
> > > > > > > On Mon, 13 Jan 2020 at 20:53, Rob Herring <robh+dt@kernel.org> wrote:
> > > > > > > >
> > > > > > > > On Mon, Dec 30, 2019 at 8:44 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > Update PSCI DT bindings to allow to represent idle states for CPUs and the
> > > > > > > > > CPU topology, by using a hierarchical layout. Primarily this is done by
> > > > > > > > > re-using the existing DT bindings for PM domains [1] and for PM domain idle
> > > > > > > > > states [2].
> > > > > > > > >
> > > > > > > > > Let's also add an example into the document for the PSCI DT bindings, to
> > > > > > > > > clearly show the new hierarchical based layout. The currently supported
> > > > > > > > > flattened layout, is already described in the ARM idle states bindings [3],
> > > > > > > > > so let's leave that as is.
> > > > > > > > >
> > > > > > > > > [1] Documentation/devicetree/bindings/power/power_domain.txt
> > > > > > > > > [2] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > > > > > > [3] Documentation/devicetree/bindings/arm/idle-states.txt
> > > > > > > > >
> > > > > > > > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> > > > > > > > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > > > > > > > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > Changes in v5:
> > > > > > > > >         - None.
> > > > > > > >
> > > > > > > > First I'm seeing this as the DT list was not copied. The example has
> > > > > > > > problems when running 'make dt_binding_check':
> > > > > > > >
> > > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > > > > > > compatible: Additional items are not allowed ('arm,armv8' was
> > > > > > > > unexpected)
> > > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > > > > > > compatible: ['arm,cortex-a53', 'arm,armv8'] is too long
> > > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > > > > > > compatible: Additional items are not allowed ('arm,armv8' was
> > > > > > > > unexpected)
> > > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > > > > > > compatible: ['arm,cortex-a57', 'arm,armv8'] is too long
> > > > > > > >
> > > > > > > > 'arm,armv8' is only valid for s/w models.
> > > > > > >
> > > > > > > Perhaps you have a different version of the tools than I have (I have
> > > > > > > tried both on v.5.5-rc5 and todays linux-next), because I can't
> > > > > > > reproduce these errors at my side when running "make
> > > > > > > dt_binding_check".
> > > > > > >
> > > > > > > Can you please check again?
> > > > > >
> > > > > > Are you setting DT_SCHEMA_FILES? If so, then arm/cpus.yaml (or any
> > > > > > other schema) isn't loaded and used for validation. That schema is the
> > > > > > source of this error.
> > > > >
> > > > > Yes. Aha, that's why then. Perhaps that needs to be clarified
> > > > > somewhere in the documentation of tool.
> > > >
> > > > Patches welcome. :) I'm kind of tired of writing documentation that no
> > > > one comments on and and seemingly only sometimes read. </rant> :(
> > >
> > > I understand your concerns. A patch is on it's way.
> > >
> > > >
> > > > > Anyway, I used because it was kind of hard to process all the error
> > > > > output one gets when building all yaml files at once.
> > > >
> > > > dtbs_check has a lot which is where setting DT_SCHEMA_FILES is
> > > > primarily useful. dt_binding_check should be error/warning free, but
> > > > yes linux-next and rc1/2 are frequently broken.
> > > >
> > > > > > It is failing in my CI job:
> > > > > > https://gitlab.com/robherring/linux-dt-bindings/-/jobs/405298185
> > > > > >
> > > > > > Is dt-schema up to date? Though I can't think of any recent changes
> > > > > > that would impact this. This check has been there a while and I fixed
> > > > > > all the dts files.
> > > > > >
> > > > > > Do you see psci.example.dt.yaml getting built?
> > > > >
> > > > > Yes, but with using DT_SCHEMA_FILES.
> > > > >
> > > > > Anyway, now I can re-produced the errors, so then I should be able to
> > > > > fix them. :-)
> > > > >
> > > > > >
> > > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > > > > > > idle-states: cluster-retention:compatible:0: 'arm,idle-state' was
> > > > > > > > expected
> > > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > > > > > > idle-states: cluster-power-down:compatible:0: 'arm,idle-state' was
> > > > > > > > expected
> > > > > > > >
> > > > > > > > The last 2 are due to my conversion of the idle-states binding which
> > > > > > > > is in my tree now. Probably need to add 'domain-idle-state' as a
> > > > > > > > compatible at a minimum. It looks like domain-idle-state.txt is pretty
> > > > > > > > much the same as arm/idle-state.txt, so we should perhaps merge them.
> > > > > > >
> > > > > > > Ahh, so maybe *all* of the above problems are caused by conflicts in
> > > > > > > the arm-soc tree with changes from your tree!?
> > > > > >
> > > > > > Shouldn't be. arm/cpus.yaml has been in place for a few cycles now.
> > > > > >
> > > > > > >
> > > > > > > In regards to merging files, I am fine by that if that helps.
> > > > > > >
> > > > > > > >
> > > > > > > > There's some bigger issues though.
> > > > > > > >
> > > > > > > > > ---
> > > > > > > > >  .../devicetree/bindings/arm/cpus.yaml         |  15 +++
> > > > > > > > >  .../devicetree/bindings/arm/psci.yaml         | 104 ++++++++++++++++++
> > > > > > > > >  2 files changed, 119 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > > > > index c23c24ff7575..7a9c3ce2dbef 100644
> > > > > > > > > --- a/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > > > > +++ b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > > > > @@ -242,6 +242,21 @@ properties:
> > > > > > > > >
> > > > > > > > >        where voltage is in V, frequency is in MHz.
> > > > > > > > >
> > > > > > > > > +  power-domains:
> > > > > > > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > > > > > > +    description:
> > > > > > > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > > > > > > +      PM domain provider (see also ../power_domain.txt).
> > > > > > > > > +
> > > > > > > > > +  power-domain-names:
> > > > > > > > > +    $ref: '/schemas/types.yaml#/definitions/string-array'
> > > > > > > > > +    description:
> > > > > > > > > +      A list of power domain name strings sorted in the same order as the
> > > > > > > > > +      power-domains property.
> > > > > > > > > +
> > > > > > > > > +      For PSCI based platforms, the name corresponding to the index of the PSCI
> > > > > > > > > +      PM domain provider, must be "psci".
> > > > > > > > > +
> > > > > > > > >    qcom,saw:
> > > > > > > > >      $ref: '/schemas/types.yaml#/definitions/phandle'
> > > > > > > > >      description: |
> > > > > > > > > diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > > > > index 7abdf58b335e..8ef85420b2ab 100644
> > > > > > > > > --- a/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > > > > +++ b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > > > > @@ -102,6 +102,34 @@ properties:
> > > > > > > > >        [1] Kernel documentation - ARM idle states bindings
> > > > > > > > >          Documentation/devicetree/bindings/arm/idle-states.txt
> > > > > > > > >
> > > > > > > > > +  "#power-domain-cells":
> > > > > > > >
> > > > > > > > This is wrong because you are saying the /psci node should have these
> > > > > > > > properties. You need to define the child nodes (at least a pattern you
> > > > > > > > can match on) and put these properties there.
> > > > > > >
> > > > > > > Right, good point.
> > > > > > >
> > > > > > > I searched for some similar examples for how to encode this, but
> > > > > > > couldn't really find something useful.
> > > > > >
> > > > > > You need something like:
> > > > > >
> > > > > > patternProperties:
> > > > > >   '^(cluster|cpu)-pd[0-9a-f]+$':
> > > > > >     type: object
> > > > > >     properties:
> > > > > >       ... and then the properties in the child nodes
> > > > > >
> > > > > > Note that its going to look weird for the 10th PD with 'cpu-pda'. So
> > > > > > maybe add a '-'.
> > > > > >
> > > > >
> > > > > Great, I try this! Thanks.
> > > > >
> > > > > > > One more thing, it seems like
> > > > > > > this change is also needed for the common power-domain bindings, as
> > > > > > > that also specifies parent/childs domains.
> > > > > >
> > > > > > Normally, we'd have a $ref to power-domain.yaml, but for that to work
> > > > > > here, you'll have to expand the node names ($nodename).
> > > > >
> > > > > Not sure I get that, but interpret this as it's not a good idea to use
> > > > > a $ref to power-domain.yaml. Right?
> > > >
> > > > It means either this binding is odd or power-domain.yaml needs some
> > > > more work or both. Ideally, we only have 1 type definition of any
> > > > property name.
> > > >
> > > > Probably the easiest thing to do is extend the node name pattern to
> > > > something like this:
> > > >
> > > > pattern: "^(power-controller|power-domain)([@\-].*)?$"
> > > >
> > > > And then name your nodes like this:
> > > >
> > > > power-domain-cpu-0
> > > > power-domain-cluster
> > > >
> > > > That's more consistent anyways.
> > >
> > > Looks like a good idea! I try that.
> > >
> > > >
> > > > > > > Anyway, I would really appreciate if you can suggest something more
> > > > > > > detailed for you think this should be done!?
> > > > > > >
> > > > > > > >
> > > > > > > > > +    description:
> > > > > > > > > +      The number of cells in a PM domain specifier as per binding in [3].
> > > > > > > > > +      Must be 0 as to represent a single PM domain.
> > > > > > > > > +
> > > > > > > > > +      ARM systems can have multiple cores, sometimes in an hierarchical
> > > > > > > > > +      arrangement. This often, but not always, maps directly to the processor
> > > > > > > > > +      power topology of the system. Individual nodes in a topology have their
> > > > > > > > > +      own specific power states and can be better represented hierarchically.
> > > > > > > > > +
> > > > > > > > > +      For these cases, the definitions of the idle states for the CPUs and the
> > > > > > > > > +      CPU topology, must conform to the binding in [3]. The idle states
> > > > > > > > > +      themselves must conform to the binding in [4] and must specify the
> > > > > > > > > +      arm,psci-suspend-param property.
> > > > > > > > > +
> > > > > > > > > +      It should also be noted that, in PSCI firmware v1.0 the OS-Initiated
> > > > > > > > > +      (OSI) CPU suspend mode is introduced. Using a hierarchical representation
> > > > > > > > > +      helps to implement support for OSI mode and OS implementations may choose
> > > > > > > > > +      to mandate it.
> > > > > > > > > +
> > > > > > > > > +      [3] Documentation/devicetree/bindings/power/power_domain.txt
> > > > > > > > > +      [4] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > > > > > > +
> > > > > > > > > +  power-domains:
> > > > > > > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > > > > > > +    description:
> > > > > > > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > > > > > > +      PM domain provider.
> > > > > > > >
> > > > > > > > A schema for 'domain-idle-states' property is missing.
> > > > > > >
> > > > > > > Right, let's figure out the best way for how to add that.
> > > > > >
> > > > > > If power-domain.yaml is referenced, then don't need anything else
> > > > > > unless you can define the number of phandles (looks like you can't?).
> > > > >
> > > > > The number phandles should be one. At least, I think we can start with
> > > > > that and extend the binding if needed.
> > > >
> > > > But there's 2 for the cluster in the example.
> > >
> > > What example do you refer to?
> > >
> > > For each power controller node for psci, only one phandle needs to be
> > > specified in "power-domains", as that should be sufficient to describe
> > > the topology.
> >
> > I was referring to 'domain-idle-states' in this patch:
> >
> > +      CLUSTER_PD: cluster-pd {
> > +        #power-domain-cells = <0>;
> > +        domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWRDN>;
> > +      };
>
> Going to send a patch for all this? I'd like to not have warnings in v5.6.

Yes, it's in the pipe, apologize for the delay. I have been chasing
regressions for MMC that broke boot for some boards.

I am very soon switching back to this.

Kind regards
Uffe

[PATCH v5 03/15] firmware: psci: Export functions to manage the OSI mode

[Ulf Hansson]

To allow subsequent changes to implement support for OSI mode through the
cpuidle-psci driver, export the existing psci_has_osi_support(). Export
also a new function, psci_set_osi_mode(), that allows its caller to enable
the OS-initiated CPU-suspend mode in the PSCI FW.

To deal with backwards compatibility for a kernel started through a kexec
call, default to set the CPU-suspend mode to the Platform Coordinated mode
during boot.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
---

Changes in v5:
	- None.

---
 drivers/firmware/psci/psci.c | 18 ++++++++++++++++--
 include/linux/psci.h         |  2 ++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index b3b6c15e7b36..2937d44b5df4 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -97,7 +97,7 @@ static inline bool psci_has_ext_power_state(void)
 				PSCI_1_0_FEATURES_CPU_SUSPEND_PF_MASK;
 }
 
-static inline bool psci_has_osi_support(void)
+bool psci_has_osi_support(void)
 {
 	return psci_cpu_suspend_feature & PSCI_1_0_OS_INITIATED;
 }
@@ -162,6 +162,15 @@ static u32 psci_get_version(void)
 	return invoke_psci_fn(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
 }
 
+int psci_set_osi_mode(void)
+{
+	int err;
+
+	err = invoke_psci_fn(PSCI_1_0_FN_SET_SUSPEND_MODE,
+			     PSCI_1_0_SUSPEND_MODE_OSI, 0, 0);
+	return psci_to_linux_errno(err);
+}
+
 static int psci_cpu_suspend(u32 state, unsigned long entry_point)
 {
 	int err;
@@ -544,9 +553,14 @@ static int __init psci_1_0_init(struct device_node *np)
 	if (err)
 		return err;
 
-	if (psci_has_osi_support())
+	if (psci_has_osi_support()) {
 		pr_info("OSI mode supported.\n");
 
+		/* Default to PC mode. */
+		invoke_psci_fn(PSCI_1_0_FN_SET_SUSPEND_MODE,
+			       PSCI_1_0_SUSPEND_MODE_PC, 0, 0);
+	}
+
 	return 0;
 }
 
diff --git a/include/linux/psci.h b/include/linux/psci.h
index ebe0a881d13d..a67712b73b6c 100644
--- a/include/linux/psci.h
+++ b/include/linux/psci.h
@@ -18,6 +18,8 @@ bool psci_tos_resident_on(int cpu);
 
 int psci_cpu_suspend_enter(u32 state);
 bool psci_power_state_is_valid(u32 state);
+int psci_set_osi_mode(void);
+bool psci_has_osi_support(void);
 
 enum smccc_version {
 	SMCCC_VERSION_1_0,
-- 
2.17.1

[PATCH v5 04/15] of: base: Add of_get_cpu_state_node() to get idle states for a CPU node

[Ulf Hansson]

The CPU's idle state nodes are currently parsed at the common cpuidle DT
library, but also when initializing data for specific CPU idle operations,
as in the PSCI cpuidle driver case and qcom-spm cpuidle case.

To avoid open-coding, let's introduce of_get_cpu_state_node(), which takes
the device node for the CPU and the index to the requested idle state node,
as in-parameters. In case a corresponding idle state node is found, it
returns the node with the refcount incremented for it, else it returns
NULL.

Moreover, for PSCI there are two options to describe the CPU's idle states
[1], either via a flattened description or a hierarchical layout. Hence,
let's take both options into account.

[1] Documentation/devicetree/bindings/arm/psci.yaml

Suggested-by: Sudeep Holla <sudeep.holla@arm.com>
Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
---

Changes in v5:
	- None.

---
 drivers/of/base.c  | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/of.h |  8 ++++++++
 2 files changed, 44 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index db7fbc0c0893..614f0c674995 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -489,6 +489,42 @@ int of_cpu_node_to_id(struct device_node *cpu_node)
 }
 EXPORT_SYMBOL(of_cpu_node_to_id);
 
+/**
+ * of_get_cpu_state_node - Get CPU's idle state node at the given index
+ *
+ * @cpu_node: The device node for the CPU
+ * @index: The index in the list of the idle states
+ *
+ * Two generic methods can be used to describe a CPU's idle states, either via
+ * a flattened description through the "cpu-idle-states" binding or via the
+ * hierarchical layout, using the "power-domains" and the "domain-idle-states"
+ * bindings. This function check for both and returns the idle state node for
+ * the requested index.
+ *
+ * In case an idle state node is found at @index, the refcount is incremented
+ * for it, so call of_node_put() on it when done. Returns NULL if not found.
+ */
+struct device_node *of_get_cpu_state_node(struct device_node *cpu_node,
+					  int index)
+{
+	struct of_phandle_args args;
+	int err;
+
+	err = of_parse_phandle_with_args(cpu_node, "power-domains",
+					"#power-domain-cells", 0, &args);
+	if (!err) {
+		struct device_node *state_node =
+			of_parse_phandle(args.np, "domain-idle-states", index);
+
+		of_node_put(args.np);
+		if (state_node)
+			return state_node;
+	}
+
+	return of_parse_phandle(cpu_node, "cpu-idle-states", index);
+}
+EXPORT_SYMBOL(of_get_cpu_state_node);
+
 /**
  * __of_device_is_compatible() - Check if the node matches given constraints
  * @device: pointer to node
diff --git a/include/linux/of.h b/include/linux/of.h
index 844f89e1b039..c669c0a4732f 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -351,6 +351,8 @@ extern const void *of_get_property(const struct device_node *node,
 				int *lenp);
 extern struct device_node *of_get_cpu_node(int cpu, unsigned int *thread);
 extern struct device_node *of_get_next_cpu_node(struct device_node *prev);
+extern struct device_node *of_get_cpu_state_node(struct device_node *cpu_node,
+						 int index);
 
 #define for_each_property_of_node(dn, pp) \
 	for (pp = dn->properties; pp != NULL; pp = pp->next)
@@ -765,6 +767,12 @@ static inline struct device_node *of_get_next_cpu_node(struct device_node *prev)
 	return NULL;
 }
 
+static inline struct device_node *of_get_cpu_state_node(struct device_node *cpu_node,
+					int index)
+{
+	return NULL;
+}
+
 static inline int of_n_addr_cells(struct device_node *np)
 {
 	return 0;
-- 
2.17.1

[PATCH v5 05/15] cpuidle: dt: Support hierarchical CPU idle states

[Ulf Hansson]

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

Currently CPU's idle states are represented using the flattened model.
Let's add support for the hierarchical layout, via converting to use
of_get_cpu_state_node().

Suggested-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Co-developed-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
---

Changes in v5:
	- None.

---
 drivers/cpuidle/dt_idle_states.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c
index d06d21a9525d..252f2a9686a6 100644
--- a/drivers/cpuidle/dt_idle_states.c
+++ b/drivers/cpuidle/dt_idle_states.c
@@ -111,8 +111,7 @@ static bool idle_state_valid(struct device_node *state_node, unsigned int idx,
 	for (cpu = cpumask_next(cpumask_first(cpumask), cpumask);
 	     cpu < nr_cpu_ids; cpu = cpumask_next(cpu, cpumask)) {
 		cpu_node = of_cpu_device_node_get(cpu);
-		curr_state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
-						   idx);
+		curr_state_node = of_get_cpu_state_node(cpu_node, idx);
 		if (state_node != curr_state_node)
 			valid = false;
 
@@ -170,7 +169,7 @@ int dt_init_idle_driver(struct cpuidle_driver *drv,
 	cpu_node = of_cpu_device_node_get(cpumask_first(cpumask));
 
 	for (i = 0; ; i++) {
-		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
+		state_node = of_get_cpu_state_node(cpu_node, i);
 		if (!state_node)
 			break;
 
-- 
2.17.1

[PATCH v5 06/15] cpuidle: psci: Simplify OF parsing of CPU idle state nodes

[Ulf Hansson]

Iterating through the idle state nodes in DT, to find out the number of
states that needs to be allocated is unnecessary, as it has already been
done from dt_init_idle_driver(). Therefore, drop the iteration and use the
number we already have at hand.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
---

Changes in v5:
	- None.

---
 drivers/cpuidle/cpuidle-psci.c | 35 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index 361985f52ddd..761359be50f2 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -73,30 +73,24 @@ static int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
 	return 0;
 }
 
-static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
+static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
+					unsigned int state_count, int cpu)
 {
-	int i, ret = 0, count = 0;
+	int i, ret = 0;
 	u32 *psci_states;
 	struct device_node *state_node;
 
-	/* Count idle states */
-	while ((state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
-					      count))) {
-		count++;
-		of_node_put(state_node);
-	}
-
-	if (!count)
-		return -ENODEV;
-
-	count++; /* Add WFI state too */
-	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
+	state_count++; /* Add WFI state too */
+	psci_states = kcalloc(state_count, sizeof(*psci_states), GFP_KERNEL);
 	if (!psci_states)
 		return -ENOMEM;
 
-	for (i = 1; i < count; i++) {
+	for (i = 1; i < state_count; i++) {
 		state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
 					      i - 1);
+		if (!state_node)
+			break;
+
 		ret = psci_dt_parse_state_node(state_node, &psci_states[i]);
 		of_node_put(state_node);
 
@@ -106,6 +100,11 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
 		pr_debug("psci-power-state %#x index %d\n", psci_states[i], i);
 	}
 
+	if (i != state_count) {
+		ret = -ENODEV;
+		goto free_mem;
+	}
+
 	/* Idle states parsed correctly, initialize per-cpu pointer */
 	per_cpu(psci_power_state, cpu) = psci_states;
 	return 0;
@@ -115,7 +114,7 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
 	return ret;
 }
 
-static __init int psci_cpu_init_idle(unsigned int cpu)
+static __init int psci_cpu_init_idle(unsigned int cpu, unsigned int state_count)
 {
 	struct device_node *cpu_node;
 	int ret;
@@ -131,7 +130,7 @@ static __init int psci_cpu_init_idle(unsigned int cpu)
 	if (!cpu_node)
 		return -ENODEV;
 
-	ret = psci_dt_cpu_init_idle(cpu_node, cpu);
+	ret = psci_dt_cpu_init_idle(cpu_node, state_count, cpu);
 
 	of_node_put(cpu_node);
 
@@ -187,7 +186,7 @@ static int __init psci_idle_init_cpu(int cpu)
 	/*
 	 * Initialize PSCI idle states.
 	 */
-	ret = psci_cpu_init_idle(cpu);
+	ret = psci_cpu_init_idle(cpu, ret);
 	if (ret) {
 		pr_err("CPU %d failed to PSCI idle\n", cpu);
 		goto out_kfree_drv;
-- 
2.17.1

[PATCH v5 07/15] cpuidle: psci: Support hierarchical CPU idle states

[Ulf Hansson]

Currently CPU's idle states are represented using the flattened model.
Let's add support for the hierarchical layout, via converting to use
of_get_cpu_state_node().

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
---

Changes in v5:
	- None.

---
 drivers/cpuidle/cpuidle-psci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index 761359be50f2..830995b8a56f 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -86,8 +86,7 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
 		return -ENOMEM;
 
 	for (i = 1; i < state_count; i++) {
-		state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
-					      i - 1);
+		state_node = of_get_cpu_state_node(cpu_node, i - 1);
 		if (!state_node)
 			break;
 
-- 
2.17.1

[PATCH v5 08/15] cpuidle: psci: Add a helper to attach a CPU to its PM domain

[Ulf Hansson]

Introduce a PSCI DT helper function, psci_dt_attach_cpu(), which takes a
CPU number as an in-parameter and tries to attach the CPU's struct device
to its corresponding PM domain.

Let's makes use of dev_pm_domain_attach_by_name(), as it allows us to
specify "psci" as the "name" of the PM domain to attach to. Additionally,
let's also prepare the attached device to be power managed via runtime PM.

Note that, the implementation of the new helper function is in a new
separate c-file, which may seems a bit too much at this point. However,
subsequent changes that implements the remaining part of the PM domain
support for cpuidle-psci, helps to justify this split.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
---

Changes in v5:
	- Declare psci_dt_attach_cpu() as __init.

---
 drivers/cpuidle/Makefile              |  4 +++-
 drivers/cpuidle/cpuidle-psci-domain.c | 31 +++++++++++++++++++++++++++
 drivers/cpuidle/cpuidle-psci.h        | 12 +++++++++++
 3 files changed, 46 insertions(+), 1 deletion(-)
 create mode 100644 drivers/cpuidle/cpuidle-psci-domain.c
 create mode 100644 drivers/cpuidle/cpuidle-psci.h

diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index ee70d5cc5b99..cc8c769d7fa9 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -21,7 +21,9 @@ obj-$(CONFIG_ARM_U8500_CPUIDLE)         += cpuidle-ux500.o
 obj-$(CONFIG_ARM_AT91_CPUIDLE)          += cpuidle-at91.o
 obj-$(CONFIG_ARM_EXYNOS_CPUIDLE)        += cpuidle-exynos.o
 obj-$(CONFIG_ARM_CPUIDLE)		+= cpuidle-arm.o
-obj-$(CONFIG_ARM_PSCI_CPUIDLE)		+= cpuidle-psci.o
+obj-$(CONFIG_ARM_PSCI_CPUIDLE)		+= cpuidle_psci.o
+cpuidle_psci-y				:= cpuidle-psci.o
+cpuidle_psci-$(CONFIG_PM_GENERIC_DOMAINS_OF) += cpuidle-psci-domain.o
 
 ###############################################################################
 # MIPS drivers
diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
new file mode 100644
index 000000000000..87f881c21af1
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-psci-domain.c
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PM domains for CPUs via genpd - managed by cpuidle-psci.
+ *
+ * Copyright (C) 2019 Linaro Ltd.
+ * Author: Ulf Hansson <ulf.hansson@linaro.org>
+ *
+ */
+
+#include <linux/cpu.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
+
+#include "cpuidle-psci.h"
+
+struct device __init *psci_dt_attach_cpu(int cpu)
+{
+	struct device *dev;
+
+	dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
+	if (IS_ERR_OR_NULL(dev))
+		return dev;
+
+	pm_runtime_irq_safe(dev);
+	if (cpu_online(cpu))
+		pm_runtime_get_sync(dev);
+
+	return dev;
+}
diff --git a/drivers/cpuidle/cpuidle-psci.h b/drivers/cpuidle/cpuidle-psci.h
new file mode 100644
index 000000000000..8609482cf490
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-psci.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __CPUIDLE_PSCI_H
+#define __CPUIDLE_PSCI_H
+
+#ifdef CONFIG_PM_GENERIC_DOMAINS_OF
+struct device __init *psci_dt_attach_cpu(int cpu);
+#else
+static inline struct device __init *psci_dt_attach_cpu(int cpu) { return NULL; }
+#endif
+
+#endif /* __CPUIDLE_PSCI_H */
-- 
2.17.1

[PATCH v5 09/15] cpuidle: psci: Attach CPU devices to their PM domains

[Ulf Hansson]

In order to enable a CPU to be power managed through its PM domain, let's
try to attach it by calling psci_dt_attach_cpu() during the cpuidle
initialization.

psci_dt_attach_cpu() returns a pointer to the attached struct device, which
later should be used for runtime PM, hence we need to store it somewhere.
Rather than adding yet another per CPU variable, let's create a per CPU
struct to collect the relevant per CPU variables.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
---

Changes in v5:
	- None.

---
 drivers/cpuidle/cpuidle-psci.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index 830995b8a56f..6a87848be3c3 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -20,14 +20,20 @@
 
 #include <asm/cpuidle.h>
 
+#include "cpuidle-psci.h"
 #include "dt_idle_states.h"
 
-static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
+struct psci_cpuidle_data {
+	u32 *psci_states;
+	struct device *dev;
+};
+
+static DEFINE_PER_CPU_READ_MOSTLY(struct psci_cpuidle_data, psci_cpuidle_data);
 
 static int psci_enter_idle_state(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv, int idx)
 {
-	u32 *state = __this_cpu_read(psci_power_state);
+	u32 *state = __this_cpu_read(psci_cpuidle_data.psci_states);
 
 	return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter,
 					   idx, state[idx]);
@@ -79,6 +85,7 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
 	int i, ret = 0;
 	u32 *psci_states;
 	struct device_node *state_node;
+	struct psci_cpuidle_data *data = per_cpu_ptr(&psci_cpuidle_data, cpu);
 
 	state_count++; /* Add WFI state too */
 	psci_states = kcalloc(state_count, sizeof(*psci_states), GFP_KERNEL);
@@ -104,8 +111,17 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
 		goto free_mem;
 	}
 
-	/* Idle states parsed correctly, initialize per-cpu pointer */
-	per_cpu(psci_power_state, cpu) = psci_states;
+	/* Currently limit the hierarchical topology to be used in OSI mode. */
+	if (psci_has_osi_support()) {
+		data->dev = psci_dt_attach_cpu(cpu);
+		if (IS_ERR(data->dev)) {
+			ret = PTR_ERR(data->dev);
+			goto free_mem;
+		}
+	}
+
+	/* Idle states parsed correctly, store them in the per-cpu struct. */
+	data->psci_states = psci_states;
 	return 0;
 
 free_mem:
-- 
2.17.1

[PATCH v5 10/15] cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains

[Ulf Hansson]

The per CPU variable psci_power_state, contains an array of fixed values,
which reflects the corresponding arm,psci-suspend-param parsed from DT, for
each of the available CPU idle states.

This isn't sufficient when using the hierarchical CPU topology in DT, in
combination with having PSCI OS initiated (OSI) mode enabled. More
precisely, in OSI mode, Linux is responsible of telling the PSCI FW what
idle state the cluster (a group of CPUs) should enter, while in PSCI
Platform Coordinated (PC) mode, each CPU independently votes for an idle
state of the cluster.

For this reason, introduce a per CPU variable called domain_state and
implement two helper functions to read/write its value. Then let the
domain_state take precedence over the regular selected state, when entering
and idle state.

To avoid executing the above OSI specific code in the ->enter() callback,
while operating in the default PSCI Platform Coordinated mode, let's also
add a new enter-function and use it for OSI.

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

Changes in v5:
	- None.

---
 drivers/cpuidle/cpuidle-psci.c | 56 ++++++++++++++++++++++++++++++----
 1 file changed, 50 insertions(+), 6 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index 6a87848be3c3..9600fe674a89 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -29,14 +29,47 @@ struct psci_cpuidle_data {
 };
 
 static DEFINE_PER_CPU_READ_MOSTLY(struct psci_cpuidle_data, psci_cpuidle_data);
+static DEFINE_PER_CPU(u32, domain_state);
+
+static inline void psci_set_domain_state(u32 state)
+{
+	__this_cpu_write(domain_state, state);
+}
+
+static inline u32 psci_get_domain_state(void)
+{
+	return __this_cpu_read(domain_state);
+}
+
+static inline int psci_enter_state(int idx, u32 state)
+{
+	return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
+}
+
+static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
+					struct cpuidle_driver *drv, int idx)
+{
+	struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
+	u32 *states = data->psci_states;
+	u32 state = psci_get_domain_state();
+	int ret;
+
+	if (!state)
+		state = states[idx];
+
+	ret = psci_enter_state(idx, state);
+
+	/* Clear the domain state to start fresh when back from idle. */
+	psci_set_domain_state(0);
+	return ret;
+}
 
 static int psci_enter_idle_state(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv, int idx)
 {
 	u32 *state = __this_cpu_read(psci_cpuidle_data.psci_states);
 
-	return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter,
-					   idx, state[idx]);
+	return psci_enter_state(idx, state[idx]);
 }
 
 static struct cpuidle_driver psci_idle_driver __initdata = {
@@ -79,7 +112,8 @@ static int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
 	return 0;
 }
 
-static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
+static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
+					struct device_node *cpu_node,
 					unsigned int state_count, int cpu)
 {
 	int i, ret = 0;
@@ -118,6 +152,15 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
 			ret = PTR_ERR(data->dev);
 			goto free_mem;
 		}
+
+		/*
+		 * Using the deepest state for the CPU to trigger a potential
+		 * selection of a shared state for the domain, assumes the
+		 * domain states are all deeper states.
+		 */
+		if (data->dev)
+			drv->states[state_count - 1].enter =
+				psci_enter_domain_idle_state;
 	}
 
 	/* Idle states parsed correctly, store them in the per-cpu struct. */
@@ -129,7 +172,8 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
 	return ret;
 }
 
-static __init int psci_cpu_init_idle(unsigned int cpu, unsigned int state_count)
+static __init int psci_cpu_init_idle(struct cpuidle_driver *drv,
+				     unsigned int cpu, unsigned int state_count)
 {
 	struct device_node *cpu_node;
 	int ret;
@@ -145,7 +189,7 @@ static __init int psci_cpu_init_idle(unsigned int cpu, unsigned int state_count)
 	if (!cpu_node)
 		return -ENODEV;
 
-	ret = psci_dt_cpu_init_idle(cpu_node, state_count, cpu);
+	ret = psci_dt_cpu_init_idle(drv, cpu_node, state_count, cpu);
 
 	of_node_put(cpu_node);
 
@@ -201,7 +245,7 @@ static int __init psci_idle_init_cpu(int cpu)
 	/*
 	 * Initialize PSCI idle states.
 	 */
-	ret = psci_cpu_init_idle(cpu, ret);
+	ret = psci_cpu_init_idle(drv, cpu, ret);
 	if (ret) {
 		pr_err("CPU %d failed to PSCI idle\n", cpu);
 		goto out_kfree_drv;
-- 
2.17.1

Re: [PATCH v5 10/15] cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains

[Sudeep Holla]

On Mon, Dec 30, 2019 at 03:43:57PM +0100, Ulf Hansson wrote:
> The per CPU variable psci_power_state, contains an array of fixed values,
> which reflects the corresponding arm,psci-suspend-param parsed from DT, for
> each of the available CPU idle states.
> 
> This isn't sufficient when using the hierarchical CPU topology in DT, in
> combination with having PSCI OS initiated (OSI) mode enabled. More
> precisely, in OSI mode, Linux is responsible of telling the PSCI FW what
> idle state the cluster (a group of CPUs) should enter, while in PSCI
> Platform Coordinated (PC) mode, each CPU independently votes for an idle
> state of the cluster.
> 
> For this reason, introduce a per CPU variable called domain_state and
> implement two helper functions to read/write its value. Then let the
> domain_state take precedence over the regular selected state, when entering
> and idle state.
> 
> To avoid executing the above OSI specific code in the ->enter() callback,
> while operating in the default PSCI Platform Coordinated mode, let's also
> add a new enter-function and use it for OSI.
> 
> Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> 
> Changes in v5:
> 	- None.
> 
> ---
>  drivers/cpuidle/cpuidle-psci.c | 56 ++++++++++++++++++++++++++++++----
>  1 file changed, 50 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> index 6a87848be3c3..9600fe674a89 100644
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -29,14 +29,47 @@ struct psci_cpuidle_data {
>  };
>  
>  static DEFINE_PER_CPU_READ_MOSTLY(struct psci_cpuidle_data, psci_cpuidle_data);
> +static DEFINE_PER_CPU(u32, domain_state);
> +
> +static inline void psci_set_domain_state(u32 state)
> +{
> +	__this_cpu_write(domain_state, state);
> +}
> +
> +static inline u32 psci_get_domain_state(void)
> +{
> +	return __this_cpu_read(domain_state);
> +}
> +
> +static inline int psci_enter_state(int idx, u32 state)
> +{
> +	return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
> +}
> +
> +static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
> +					struct cpuidle_driver *drv, int idx)
> +{
> +	struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
> +	u32 *states = data->psci_states;
> +	u32 state = psci_get_domain_state();
> +	int ret;
> +
> +	if (!state)
> +		state = states[idx];
> +
> +	ret = psci_enter_state(idx, state);
> +
> +	/* Clear the domain state to start fresh when back from idle. */
> +	psci_set_domain_state(0);
> +	return ret;
> +}
>  
>  static int psci_enter_idle_state(struct cpuidle_device *dev,
>  				struct cpuidle_driver *drv, int idx)
>  {
>  	u32 *state = __this_cpu_read(psci_cpuidle_data.psci_states);
>  
> -	return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter,
> -					   idx, state[idx]);
> +	return psci_enter_state(idx, state[idx]);
>  }
>  
>  static struct cpuidle_driver psci_idle_driver __initdata = {
> @@ -79,7 +112,8 @@ static int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
>  	return 0;
>  }
>  
> -static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
> +static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
> +					struct device_node *cpu_node,
>  					unsigned int state_count, int cpu)
>  {
>  	int i, ret = 0;
> @@ -118,6 +152,15 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
>  			ret = PTR_ERR(data->dev);
>  			goto free_mem;
>  		}
> +
> +		/*
> +		 * Using the deepest state for the CPU to trigger a potential
> +		 * selection of a shared state for the domain, assumes the
> +		 * domain states are all deeper states.
> +		 */
> +		if (data->dev)

Do we still need this check ? I thought we won't attach genpd if OSI is
not enabled. If possible, please drop the check.

Other than that, looks good to me.

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep

Re: [PATCH v5 10/15] cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains

[Ulf Hansson]

On Mon, 30 Dec 2019 at 17:35, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Mon, Dec 30, 2019 at 03:43:57PM +0100, Ulf Hansson wrote:
> > The per CPU variable psci_power_state, contains an array of fixed values,
> > which reflects the corresponding arm,psci-suspend-param parsed from DT, for
> > each of the available CPU idle states.
> >
> > This isn't sufficient when using the hierarchical CPU topology in DT, in
> > combination with having PSCI OS initiated (OSI) mode enabled. More
> > precisely, in OSI mode, Linux is responsible of telling the PSCI FW what
> > idle state the cluster (a group of CPUs) should enter, while in PSCI
> > Platform Coordinated (PC) mode, each CPU independently votes for an idle
> > state of the cluster.
> >
> > For this reason, introduce a per CPU variable called domain_state and
> > implement two helper functions to read/write its value. Then let the
> > domain_state take precedence over the regular selected state, when entering
> > and idle state.
> >
> > To avoid executing the above OSI specific code in the ->enter() callback,
> > while operating in the default PSCI Platform Coordinated mode, let's also
> > add a new enter-function and use it for OSI.
> >
> > Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >
> > Changes in v5:
> >       - None.
> >
> > ---
> >  drivers/cpuidle/cpuidle-psci.c | 56 ++++++++++++++++++++++++++++++----
> >  1 file changed, 50 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > index 6a87848be3c3..9600fe674a89 100644
> > --- a/drivers/cpuidle/cpuidle-psci.c
> > +++ b/drivers/cpuidle/cpuidle-psci.c
> > @@ -29,14 +29,47 @@ struct psci_cpuidle_data {
> >  };
> >
> >  static DEFINE_PER_CPU_READ_MOSTLY(struct psci_cpuidle_data, psci_cpuidle_data);
> > +static DEFINE_PER_CPU(u32, domain_state);
> > +
> > +static inline void psci_set_domain_state(u32 state)
> > +{
> > +     __this_cpu_write(domain_state, state);
> > +}
> > +
> > +static inline u32 psci_get_domain_state(void)
> > +{
> > +     return __this_cpu_read(domain_state);
> > +}
> > +
> > +static inline int psci_enter_state(int idx, u32 state)
> > +{
> > +     return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
> > +}
> > +
> > +static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
> > +                                     struct cpuidle_driver *drv, int idx)
> > +{
> > +     struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
> > +     u32 *states = data->psci_states;
> > +     u32 state = psci_get_domain_state();
> > +     int ret;
> > +
> > +     if (!state)
> > +             state = states[idx];
> > +
> > +     ret = psci_enter_state(idx, state);
> > +
> > +     /* Clear the domain state to start fresh when back from idle. */
> > +     psci_set_domain_state(0);
> > +     return ret;
> > +}
> >
> >  static int psci_enter_idle_state(struct cpuidle_device *dev,
> >                               struct cpuidle_driver *drv, int idx)
> >  {
> >       u32 *state = __this_cpu_read(psci_cpuidle_data.psci_states);
> >
> > -     return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter,
> > -                                        idx, state[idx]);
> > +     return psci_enter_state(idx, state[idx]);
> >  }
> >
> >  static struct cpuidle_driver psci_idle_driver __initdata = {
> > @@ -79,7 +112,8 @@ static int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
> >       return 0;
> >  }
> >
> > -static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
> > +static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
> > +                                     struct device_node *cpu_node,
> >                                       unsigned int state_count, int cpu)
> >  {
> >       int i, ret = 0;
> > @@ -118,6 +152,15 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
> >                       ret = PTR_ERR(data->dev);
> >                       goto free_mem;
> >               }
> > +
> > +             /*
> > +              * Using the deepest state for the CPU to trigger a potential
> > +              * selection of a shared state for the domain, assumes the
> > +              * domain states are all deeper states.
> > +              */
> > +             if (data->dev)
>
> Do we still need this check ? I thought we won't attach genpd if OSI is
> not enabled. If possible, please drop the check.

This check is still needed. psci_dt_attach_cpu() now returns NULL when
OSI has not been enabled (see patch 14), as we agreed on during the
discussions around the previous version.

>
> Other than that, looks good to me.
>
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

Thanks!

Kind regards
Uffe

[PATCH v5 11/15] cpuidle: psci: Manage runtime PM in the idle path

[Ulf Hansson]

In case we have succeeded to attach a CPU to its PM domain, let's deploy
runtime PM support for the corresponding attached device, to allow the CPU
to be powered-managed accordingly.

The triggering point for when runtime PM reference counting should be done,
has been selected to the deepest idle state for the CPU. However, from the
hierarchical point view, there may be good reasons to do runtime PM
reference counting even on shallower idle states, but at this point this
isn't supported, mainly due to limitations set by the generic PM domain.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
---

Changes in v5:
	- None.

---
 drivers/cpuidle/cpuidle-psci.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index 9600fe674a89..6e7804e697ed 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -16,6 +16,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/psci.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 
 #include <asm/cpuidle.h>
@@ -51,14 +52,21 @@ static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
 {
 	struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
 	u32 *states = data->psci_states;
-	u32 state = psci_get_domain_state();
+	struct device *pd_dev = data->dev;
+	u32 state;
 	int ret;
 
+	/* Do runtime PM to manage a hierarchical CPU toplogy. */
+	pm_runtime_put_sync_suspend(pd_dev);
+
+	state = psci_get_domain_state();
 	if (!state)
 		state = states[idx];
 
 	ret = psci_enter_state(idx, state);
 
+	pm_runtime_get_sync(pd_dev);
+
 	/* Clear the domain state to start fresh when back from idle. */
 	psci_set_domain_state(0);
 	return ret;
-- 
2.17.1

[PATCH v5 12/15] cpuidle: psci: Support CPU hotplug for the hierarchical model

[Ulf Hansson]

When the hierarchical CPU topology is used and when a CPU is put offline,
that CPU prevents its PM domain from being powered off, which is because
genpd observes the corresponding attached device as being active from a
runtime PM point of view. Furthermore, any potential master PM domains are
also prevented from being powered off.

To address this limitation, let's add add a new CPU hotplug state
(CPUHP_AP_CPU_PM_STARTING) and register up/down callbacks for it, which
allows us to deal with runtime PM accordingly.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
---

Changes in v5:
	- Make CPUHP function/variable initdata.

---
 drivers/cpuidle/cpuidle-psci.c | 45 +++++++++++++++++++++++++++++++++-
 include/linux/cpuhotplug.h     |  1 +
 2 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index 6e7804e697ed..9d779be27071 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -8,6 +8,7 @@
 
 #define pr_fmt(fmt) "CPUidle PSCI: " fmt
 
+#include <linux/cpuhotplug.h>
 #include <linux/cpuidle.h>
 #include <linux/cpumask.h>
 #include <linux/cpu_pm.h>
@@ -31,6 +32,7 @@ struct psci_cpuidle_data {
 
 static DEFINE_PER_CPU_READ_MOSTLY(struct psci_cpuidle_data, psci_cpuidle_data);
 static DEFINE_PER_CPU(u32, domain_state);
+static bool psci_cpuidle_use_cpuhp __initdata;
 
 static inline void psci_set_domain_state(u32 state)
 {
@@ -72,6 +74,44 @@ static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
 	return ret;
 }
 
+static int psci_idle_cpuhp_up(unsigned int cpu)
+{
+	struct device *pd_dev = __this_cpu_read(psci_cpuidle_data.dev);
+
+	if (pd_dev)
+		pm_runtime_get_sync(pd_dev);
+
+	return 0;
+}
+
+static int psci_idle_cpuhp_down(unsigned int cpu)
+{
+	struct device *pd_dev = __this_cpu_read(psci_cpuidle_data.dev);
+
+	if (pd_dev) {
+		pm_runtime_put_sync(pd_dev);
+		/* Clear domain state to start fresh at next online. */
+		psci_set_domain_state(0);
+	}
+
+	return 0;
+}
+
+static void __init psci_idle_init_cpuhp(void)
+{
+	int err;
+
+	if (!psci_cpuidle_use_cpuhp)
+		return;
+
+	err = cpuhp_setup_state_nocalls(CPUHP_AP_CPU_PM_STARTING,
+					"cpuidle/psci:online",
+					psci_idle_cpuhp_up,
+					psci_idle_cpuhp_down);
+	if (err)
+		pr_warn("Failed %d while setup cpuhp state\n", err);
+}
+
 static int psci_enter_idle_state(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv, int idx)
 {
@@ -166,9 +206,11 @@ static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
 		 * selection of a shared state for the domain, assumes the
 		 * domain states are all deeper states.
 		 */
-		if (data->dev)
+		if (data->dev) {
 			drv->states[state_count - 1].enter =
 				psci_enter_domain_idle_state;
+			psci_cpuidle_use_cpuhp = true;
+		}
 	}
 
 	/* Idle states parsed correctly, store them in the per-cpu struct. */
@@ -289,6 +331,7 @@ static int __init psci_idle_init(void)
 			goto out_fail;
 	}
 
+	psci_idle_init_cpuhp();
 	return 0;
 
 out_fail:
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index e51ee772b9f5..01f04ed6ad92 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -95,6 +95,7 @@ enum cpuhp_state {
 	CPUHP_AP_OFFLINE,
 	CPUHP_AP_SCHED_STARTING,
 	CPUHP_AP_RCUTREE_DYING,
+	CPUHP_AP_CPU_PM_STARTING,
 	CPUHP_AP_IRQ_GIC_STARTING,
 	CPUHP_AP_IRQ_HIP04_STARTING,
 	CPUHP_AP_IRQ_ARMADA_XP_STARTING,
-- 
2.17.1

[PATCH v5 13/15] PM / Domains: Introduce a genpd OF helper that removes a subdomain

[Ulf Hansson]

We already have the of_genpd_add_subdomain() helper, but no corresponding
of_genpd_remove_subdomain(), so let's add it. Subsequent changes starts to
make use of it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v5:
	- New patch.

---
 drivers/base/power/domain.c | 38 +++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h   |  8 ++++++++
 2 files changed, 46 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 8e5725b11ee8..959d6d5eb000 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2302,6 +2302,44 @@ int of_genpd_add_subdomain(struct of_phandle_args *parent_spec,
 }
 EXPORT_SYMBOL_GPL(of_genpd_add_subdomain);
 
+/**
+ * of_genpd_remove_subdomain - Remove a subdomain from an I/O PM domain.
+ * @parent_spec: OF phandle args to use for parent PM domain look-up
+ * @subdomain_spec: OF phandle args to use for subdomain look-up
+ *
+ * Looks-up a parent PM domain and subdomain based upon phandle args
+ * provided and removes the subdomain from the parent PM domain. Returns a
+ * negative error code on failure.
+ */
+int of_genpd_remove_subdomain(struct of_phandle_args *parent_spec,
+			      struct of_phandle_args *subdomain_spec)
+{
+	struct generic_pm_domain *parent, *subdomain;
+	int ret;
+
+	mutex_lock(&gpd_list_lock);
+
+	parent = genpd_get_from_provider(parent_spec);
+	if (IS_ERR(parent)) {
+		ret = PTR_ERR(parent);
+		goto out;
+	}
+
+	subdomain = genpd_get_from_provider(subdomain_spec);
+	if (IS_ERR(subdomain)) {
+		ret = PTR_ERR(subdomain);
+		goto out;
+	}
+
+	ret = pm_genpd_remove_subdomain(parent, subdomain);
+
+out:
+	mutex_unlock(&gpd_list_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(of_genpd_remove_subdomain);
+
 /**
  * of_genpd_remove_last - Remove the last PM domain registered for a provider
  * @provider: Pointer to device structure associated with provider
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 5a31c711b896..9ec78ee53652 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -284,6 +284,8 @@ void of_genpd_del_provider(struct device_node *np);
 int of_genpd_add_device(struct of_phandle_args *args, struct device *dev);
 int of_genpd_add_subdomain(struct of_phandle_args *parent_spec,
 			   struct of_phandle_args *subdomain_spec);
+int of_genpd_remove_subdomain(struct of_phandle_args *parent_spec,
+			      struct of_phandle_args *subdomain_spec);
 struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
 int of_genpd_parse_idle_states(struct device_node *dn,
 			       struct genpd_power_state **states, int *n);
@@ -322,6 +324,12 @@ static inline int of_genpd_add_subdomain(struct of_phandle_args *parent_spec,
 	return -ENODEV;
 }
 
+static inline int of_genpd_remove_subdomain(struct of_phandle_args *parent_spec,
+					struct of_phandle_args *subdomain_spec)
+{
+	return -ENODEV;
+}
+
 static inline int of_genpd_parse_idle_states(struct device_node *dn,
 			struct genpd_power_state **states, int *n)
 {
-- 
2.17.1

[PATCH v5 14/15] cpuidle: psci: Add support for PM domains by using genpd

[Ulf Hansson]

When the hierarchical CPU topology layout is used in DT and the PSCI OSI
mode is supported by the PSCI FW, let's initialize a corresponding PM
domain topology by using genpd. This enables a CPU and a group of CPUs,
when attached to the topology, to be power-managed accordingly.

To trigger the attempt to initialize the genpd data structures let's use a
subsys_initcall, which should be early enough to allow CPUs, but also other
devices to be attached.

The initialization consists of parsing the PSCI OF node for the topology
and the "domain idle states" DT bindings. In case the idle states are
compatible with "domain-idle-state", the initialized genpd becomes
responsible of selecting an idle state for the PM domain, via assigning it
a genpd governor.

Note that, a successful initialization of the genpd data structures, is
followed by a call to psci_set_osi_mode(), as to try to enable the OSI mode
in the PSCI FW. In case this fails, we fall back into a degraded mode
rather than bailing out and returning error codes.

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

Changes in v5:
	- Remove genpds in the error path, when enabling OSI failed.
	- Let psci_dt_attach_cpu() return NULL if OSI mode isn't enabled.
	- Make the osi_mode_enabled variable __initdata.
	- Drop some DT compatible strings, as PSCI v1.0 introduces OSI.

---
 drivers/cpuidle/cpuidle-psci-domain.c | 277 ++++++++++++++++++++++++++
 drivers/cpuidle/cpuidle-psci.c        |   4 +-
 drivers/cpuidle/cpuidle-psci.h        |   5 +
 3 files changed, 284 insertions(+), 2 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
index 87f881c21af1..423f03bbeb74 100644
--- a/drivers/cpuidle/cpuidle-psci-domain.c
+++ b/drivers/cpuidle/cpuidle-psci-domain.c
@@ -7,18 +7,295 @@
  *
  */
 
+#define pr_fmt(fmt) "CPUidle PSCI: " fmt
+
 #include <linux/cpu.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
+#include <linux/psci.h>
+#include <linux/slab.h>
+#include <linux/string.h>
 
 #include "cpuidle-psci.h"
 
+struct psci_pd_provider {
+	struct list_head link;
+	struct device_node *node;
+};
+
+static LIST_HEAD(psci_pd_providers);
+static bool osi_mode_enabled __initdata;
+
+static int psci_pd_power_off(struct generic_pm_domain *pd)
+{
+	struct genpd_power_state *state = &pd->states[pd->state_idx];
+	u32 *pd_state;
+
+	if (!state->data)
+		return 0;
+
+	/* OSI mode is enabled, set the corresponding domain state. */
+	pd_state = state->data;
+	psci_set_domain_state(*pd_state);
+
+	return 0;
+}
+
+static int __init psci_pd_parse_state_nodes(struct genpd_power_state *states,
+					int state_count)
+{
+	int i, ret;
+	u32 psci_state, *psci_state_buf;
+
+	for (i = 0; i < state_count; i++) {
+		ret = psci_dt_parse_state_node(to_of_node(states[i].fwnode),
+					&psci_state);
+		if (ret)
+			goto free_state;
+
+		psci_state_buf = kmalloc(sizeof(u32), GFP_KERNEL);
+		if (!psci_state_buf) {
+			ret = -ENOMEM;
+			goto free_state;
+		}
+		*psci_state_buf = psci_state;
+		states[i].data = psci_state_buf;
+	}
+
+	return 0;
+
+free_state:
+	i--;
+	for (; i >= 0; i--)
+		kfree(states[i].data);
+	return ret;
+}
+
+static int __init psci_pd_parse_states(struct device_node *np,
+			struct genpd_power_state **states, int *state_count)
+{
+	int ret;
+
+	/* Parse the domain idle states. */
+	ret = of_genpd_parse_idle_states(np, states, state_count);
+	if (ret)
+		return ret;
+
+	/* Fill out the PSCI specifics for each found state. */
+	ret = psci_pd_parse_state_nodes(*states, *state_count);
+	if (ret)
+		kfree(*states);
+
+	return ret;
+}
+
+static void psci_pd_free_states(struct genpd_power_state *states,
+				unsigned int state_count)
+{
+	int i;
+
+	for (i = 0; i < state_count; i++)
+		kfree(states[i].data);
+	kfree(states);
+}
+
+static int __init psci_pd_init(struct device_node *np)
+{
+	struct generic_pm_domain *pd;
+	struct psci_pd_provider *pd_provider;
+	struct dev_power_governor *pd_gov;
+	struct genpd_power_state *states = NULL;
+	int ret = -ENOMEM, state_count = 0;
+
+	pd = kzalloc(sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		goto out;
+
+	pd_provider = kzalloc(sizeof(*pd_provider), GFP_KERNEL);
+	if (!pd_provider)
+		goto free_pd;
+
+	pd->name = kasprintf(GFP_KERNEL, "%pOF", np);
+	if (!pd->name)
+		goto free_pd_prov;
+
+	/*
+	 * Parse the domain idle states and let genpd manage the state selection
+	 * for those being compatible with "domain-idle-state".
+	 */
+	ret = psci_pd_parse_states(np, &states, &state_count);
+	if (ret)
+		goto free_name;
+
+	pd->free_states = psci_pd_free_states;
+	pd->name = kbasename(pd->name);
+	pd->power_off = psci_pd_power_off;
+	pd->states = states;
+	pd->state_count = state_count;
+	pd->flags |= GENPD_FLAG_IRQ_SAFE | GENPD_FLAG_CPU_DOMAIN;
+
+	/* Use governor for CPU PM domains if it has some states to manage. */
+	pd_gov = state_count > 0 ? &pm_domain_cpu_gov : NULL;
+
+	ret = pm_genpd_init(pd, pd_gov, false);
+	if (ret) {
+		psci_pd_free_states(states, state_count);
+		goto free_name;
+	}
+
+	ret = of_genpd_add_provider_simple(np, pd);
+	if (ret)
+		goto remove_pd;
+
+	pd_provider->node = of_node_get(np);
+	list_add(&pd_provider->link, &psci_pd_providers);
+
+	pr_debug("init PM domain %s\n", pd->name);
+	return 0;
+
+remove_pd:
+	pm_genpd_remove(pd);
+free_name:
+	kfree(pd->name);
+free_pd_prov:
+	kfree(pd_provider);
+free_pd:
+	kfree(pd);
+out:
+	pr_err("failed to init PM domain ret=%d %pOF\n", ret, np);
+	return ret;
+}
+
+static void __init psci_pd_remove(void)
+{
+	struct psci_pd_provider *pd_provider, *it;
+	struct generic_pm_domain *genpd;
+
+	list_for_each_entry_safe(pd_provider, it, &psci_pd_providers, link) {
+		of_genpd_del_provider(pd_provider->node);
+
+		genpd = of_genpd_remove_last(pd_provider->node);
+		if (!IS_ERR(genpd))
+			kfree(genpd);
+
+		of_node_put(pd_provider->node);
+		list_del(&pd_provider->link);
+		kfree(pd_provider);
+	}
+}
+
+static int __init psci_pd_init_topology(struct device_node *np, bool add)
+{
+	struct device_node *node;
+	struct of_phandle_args child, parent;
+	int ret;
+
+	for_each_child_of_node(np, node) {
+		if (of_parse_phandle_with_args(node, "power-domains",
+					"#power-domain-cells", 0, &parent))
+			continue;
+
+		child.np = node;
+		child.args_count = 0;
+
+		ret = add ? of_genpd_add_subdomain(&parent, &child) :
+			of_genpd_remove_subdomain(&parent, &child);
+		of_node_put(parent.np);
+		if (ret) {
+			of_node_put(node);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int __init psci_pd_add_topology(struct device_node *np)
+{
+	return psci_pd_init_topology(np, true);
+}
+
+static void __init psci_pd_remove_topology(struct device_node *np)
+{
+	psci_pd_init_topology(np, false);
+}
+
+static const struct of_device_id psci_of_match[] __initconst = {
+	{ .compatible = "arm,psci-1.0" },
+	{}
+};
+
+static int __init psci_idle_init_domains(void)
+{
+	struct device_node *np = of_find_matching_node(NULL, psci_of_match);
+	struct device_node *node;
+	int ret = 0, pd_count = 0;
+
+	if (!np)
+		return -ENODEV;
+
+	/* Currently limit the hierarchical topology to be used in OSI mode. */
+	if (!psci_has_osi_support())
+		goto out;
+
+	/*
+	 * Parse child nodes for the "#power-domain-cells" property and
+	 * initialize a genpd/genpd-of-provider pair when it's found.
+	 */
+	for_each_child_of_node(np, node) {
+		if (!of_find_property(node, "#power-domain-cells", NULL))
+			continue;
+
+		ret = psci_pd_init(node);
+		if (ret)
+			goto put_node;
+
+		pd_count++;
+	}
+
+	/* Bail out if not using the hierarchical CPU topology. */
+	if (!pd_count)
+		goto out;
+
+	/* Link genpd masters/subdomains to model the CPU topology. */
+	ret = psci_pd_add_topology(np);
+	if (ret)
+		goto remove_pd;
+
+	/* Try to enable OSI mode. */
+	ret = psci_set_osi_mode();
+	if (ret) {
+		pr_warn("failed to enable OSI mode: %d\n", ret);
+		psci_pd_remove_topology(np);
+		goto remove_pd;
+	}
+
+	osi_mode_enabled = true;
+	of_node_put(np);
+	pr_info("Initialized CPU PM domain topology\n");
+	return pd_count;
+
+put_node:
+	of_node_put(node);
+remove_pd:
+	if (pd_count)
+		psci_pd_remove();
+	pr_err("failed to create CPU PM domains ret=%d\n", ret);
+out:
+	of_node_put(np);
+	return ret;
+}
+subsys_initcall(psci_idle_init_domains);
+
 struct device __init *psci_dt_attach_cpu(int cpu)
 {
 	struct device *dev;
 
+	if (!osi_mode_enabled)
+		return NULL;
+
 	dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
 	if (IS_ERR_OR_NULL(dev))
 		return dev;
diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index 9d779be27071..edd7a54ef0d3 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -34,7 +34,7 @@ static DEFINE_PER_CPU_READ_MOSTLY(struct psci_cpuidle_data, psci_cpuidle_data);
 static DEFINE_PER_CPU(u32, domain_state);
 static bool psci_cpuidle_use_cpuhp __initdata;
 
-static inline void psci_set_domain_state(u32 state)
+void psci_set_domain_state(u32 state)
 {
 	__this_cpu_write(domain_state, state);
 }
@@ -143,7 +143,7 @@ static const struct of_device_id psci_idle_state_match[] __initconst = {
 	{ },
 };
 
-static int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
+int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
 {
 	int err = of_property_read_u32(np, "arm,psci-suspend-param", state);
 
diff --git a/drivers/cpuidle/cpuidle-psci.h b/drivers/cpuidle/cpuidle-psci.h
index 8609482cf490..7299a04dd467 100644
--- a/drivers/cpuidle/cpuidle-psci.h
+++ b/drivers/cpuidle/cpuidle-psci.h
@@ -3,6 +3,11 @@
 #ifndef __CPUIDLE_PSCI_H
 #define __CPUIDLE_PSCI_H
 
+struct device_node;
+
+void psci_set_domain_state(u32 state);
+int __init psci_dt_parse_state_node(struct device_node *np, u32 *state);
+
 #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
 struct device __init *psci_dt_attach_cpu(int cpu);
 #else
-- 
2.17.1

Re: [PATCH v5 14/15] cpuidle: psci: Add support for PM domains by using genpd

[Sudeep Holla]

On Mon, Dec 30, 2019 at 03:44:01PM +0100, Ulf Hansson wrote:
> When the hierarchical CPU topology layout is used in DT and the PSCI OSI
> mode is supported by the PSCI FW, let's initialize a corresponding PM
> domain topology by using genpd. This enables a CPU and a group of CPUs,
> when attached to the topology, to be power-managed accordingly.
> 
> To trigger the attempt to initialize the genpd data structures let's use a
> subsys_initcall, which should be early enough to allow CPUs, but also other
> devices to be attached.
> 
> The initialization consists of parsing the PSCI OF node for the topology
> and the "domain idle states" DT bindings. In case the idle states are
> compatible with "domain-idle-state", the initialized genpd becomes
> responsible of selecting an idle state for the PM domain, via assigning it
> a genpd governor.
> 
> Note that, a successful initialization of the genpd data structures, is
> followed by a call to psci_set_osi_mode(), as to try to enable the OSI mode
> in the PSCI FW. In case this fails, we fall back into a degraded mode
> rather than bailing out and returning error codes.
> 
> Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> 
> Changes in v5:
> 	- Remove genpds in the error path, when enabling OSI failed.
> 	- Let psci_dt_attach_cpu() return NULL if OSI mode isn't enabled.
> 	- Make the osi_mode_enabled variable __initdata.
> 	- Drop some DT compatible strings, as PSCI v1.0 introduces OSI.
> 
> ---
>  drivers/cpuidle/cpuidle-psci-domain.c | 277 ++++++++++++++++++++++++++
>  drivers/cpuidle/cpuidle-psci.c        |   4 +-
>  drivers/cpuidle/cpuidle-psci.h        |   5 +
>  3 files changed, 284 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> index 87f881c21af1..423f03bbeb74 100644
> --- a/drivers/cpuidle/cpuidle-psci-domain.c
> +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> @@ -7,18 +7,295 @@
>   *
>   */
>  
> +#define pr_fmt(fmt) "CPUidle PSCI: " fmt
> +

Looks like a candidate for cpuidle-psci.h so that it's effective even for
cpuidle-psci.c, if you re-spin(I don't see any reasons to) or if you are
sending PR, you can fix-up this along with dropping data->dev :)

Anyways,

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

>  #include <linux/cpu.h>
>  #include <linux/device.h>
>  #include <linux/kernel.h>
>  #include <linux/pm_domain.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/psci.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
>  
>  #include "cpuidle-psci.h"
>  
> +struct psci_pd_provider {
> +	struct list_head link;
> +	struct device_node *node;
> +};
> +
> +static LIST_HEAD(psci_pd_providers);
> +static bool osi_mode_enabled __initdata;
> +
> +static int psci_pd_power_off(struct generic_pm_domain *pd)
> +{
> +	struct genpd_power_state *state = &pd->states[pd->state_idx];
> +	u32 *pd_state;
> +
> +	if (!state->data)
> +		return 0;
> +
> +	/* OSI mode is enabled, set the corresponding domain state. */
> +	pd_state = state->data;
> +	psci_set_domain_state(*pd_state);
> +
> +	return 0;
> +}
> +
> +static int __init psci_pd_parse_state_nodes(struct genpd_power_state *states,
> +					int state_count)
> +{
> +	int i, ret;
> +	u32 psci_state, *psci_state_buf;
> +
> +	for (i = 0; i < state_count; i++) {
> +		ret = psci_dt_parse_state_node(to_of_node(states[i].fwnode),
> +					&psci_state);
> +		if (ret)
> +			goto free_state;
> +
> +		psci_state_buf = kmalloc(sizeof(u32), GFP_KERNEL);
> +		if (!psci_state_buf) {
> +			ret = -ENOMEM;
> +			goto free_state;
> +		}
> +		*psci_state_buf = psci_state;
> +		states[i].data = psci_state_buf;
> +	}
> +
> +	return 0;
> +
> +free_state:
> +	i--;
> +	for (; i >= 0; i--)
> +		kfree(states[i].data);
> +	return ret;
> +}
> +
> +static int __init psci_pd_parse_states(struct device_node *np,
> +			struct genpd_power_state **states, int *state_count)
> +{
> +	int ret;
> +
> +	/* Parse the domain idle states. */
> +	ret = of_genpd_parse_idle_states(np, states, state_count);
> +	if (ret)
> +		return ret;
> +
> +	/* Fill out the PSCI specifics for each found state. */
> +	ret = psci_pd_parse_state_nodes(*states, *state_count);
> +	if (ret)
> +		kfree(*states);
> +
> +	return ret;
> +}
> +
> +static void psci_pd_free_states(struct genpd_power_state *states,
> +				unsigned int state_count)
> +{
> +	int i;
> +
> +	for (i = 0; i < state_count; i++)
> +		kfree(states[i].data);
> +	kfree(states);
> +}
> +
> +static int __init psci_pd_init(struct device_node *np)
> +{
> +	struct generic_pm_domain *pd;
> +	struct psci_pd_provider *pd_provider;
> +	struct dev_power_governor *pd_gov;
> +	struct genpd_power_state *states = NULL;
> +	int ret = -ENOMEM, state_count = 0;
> +
> +	pd = kzalloc(sizeof(*pd), GFP_KERNEL);
> +	if (!pd)
> +		goto out;
> +
> +	pd_provider = kzalloc(sizeof(*pd_provider), GFP_KERNEL);
> +	if (!pd_provider)
> +		goto free_pd;
> +
> +	pd->name = kasprintf(GFP_KERNEL, "%pOF", np);
> +	if (!pd->name)
> +		goto free_pd_prov;
> +
> +	/*
> +	 * Parse the domain idle states and let genpd manage the state selection
> +	 * for those being compatible with "domain-idle-state".
> +	 */
> +	ret = psci_pd_parse_states(np, &states, &state_count);
> +	if (ret)
> +		goto free_name;
> +
> +	pd->free_states = psci_pd_free_states;
> +	pd->name = kbasename(pd->name);
> +	pd->power_off = psci_pd_power_off;
> +	pd->states = states;
> +	pd->state_count = state_count;
> +	pd->flags |= GENPD_FLAG_IRQ_SAFE | GENPD_FLAG_CPU_DOMAIN;
> +
> +	/* Use governor for CPU PM domains if it has some states to manage. */
> +	pd_gov = state_count > 0 ? &pm_domain_cpu_gov : NULL;
> +
> +	ret = pm_genpd_init(pd, pd_gov, false);
> +	if (ret) {
> +		psci_pd_free_states(states, state_count);
> +		goto free_name;
> +	}
> +
> +	ret = of_genpd_add_provider_simple(np, pd);
> +	if (ret)
> +		goto remove_pd;
> +
> +	pd_provider->node = of_node_get(np);
> +	list_add(&pd_provider->link, &psci_pd_providers);
> +
> +	pr_debug("init PM domain %s\n", pd->name);
> +	return 0;
> +
> +remove_pd:
> +	pm_genpd_remove(pd);
> +free_name:
> +	kfree(pd->name);
> +free_pd_prov:
> +	kfree(pd_provider);
> +free_pd:
> +	kfree(pd);
> +out:
> +	pr_err("failed to init PM domain ret=%d %pOF\n", ret, np);
> +	return ret;
> +}
> +
> +static void __init psci_pd_remove(void)
> +{
> +	struct psci_pd_provider *pd_provider, *it;
> +	struct generic_pm_domain *genpd;
> +
> +	list_for_each_entry_safe(pd_provider, it, &psci_pd_providers, link) {
> +		of_genpd_del_provider(pd_provider->node);
> +
> +		genpd = of_genpd_remove_last(pd_provider->node);
> +		if (!IS_ERR(genpd))
> +			kfree(genpd);
> +
> +		of_node_put(pd_provider->node);
> +		list_del(&pd_provider->link);
> +		kfree(pd_provider);
> +	}
> +}
> +
> +static int __init psci_pd_init_topology(struct device_node *np, bool add)
> +{
> +	struct device_node *node;
> +	struct of_phandle_args child, parent;
> +	int ret;
> +
> +	for_each_child_of_node(np, node) {
> +		if (of_parse_phandle_with_args(node, "power-domains",
> +					"#power-domain-cells", 0, &parent))
> +			continue;
> +
> +		child.np = node;
> +		child.args_count = 0;
> +
> +		ret = add ? of_genpd_add_subdomain(&parent, &child) :
> +			of_genpd_remove_subdomain(&parent, &child);
> +		of_node_put(parent.np);
> +		if (ret) {
> +			of_node_put(node);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int __init psci_pd_add_topology(struct device_node *np)
> +{
> +	return psci_pd_init_topology(np, true);
> +}
> +
> +static void __init psci_pd_remove_topology(struct device_node *np)
> +{
> +	psci_pd_init_topology(np, false);
> +}
> +
> +static const struct of_device_id psci_of_match[] __initconst = {
> +	{ .compatible = "arm,psci-1.0" },
> +	{}
> +};
> +
> +static int __init psci_idle_init_domains(void)
> +{
> +	struct device_node *np = of_find_matching_node(NULL, psci_of_match);
> +	struct device_node *node;
> +	int ret = 0, pd_count = 0;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	/* Currently limit the hierarchical topology to be used in OSI mode. */
> +	if (!psci_has_osi_support())
> +		goto out;
> +
> +	/*
> +	 * Parse child nodes for the "#power-domain-cells" property and
> +	 * initialize a genpd/genpd-of-provider pair when it's found.
> +	 */
> +	for_each_child_of_node(np, node) {
> +		if (!of_find_property(node, "#power-domain-cells", NULL))
> +			continue;
> +
> +		ret = psci_pd_init(node);
> +		if (ret)
> +			goto put_node;
> +
> +		pd_count++;
> +	}
> +
> +	/* Bail out if not using the hierarchical CPU topology. */
> +	if (!pd_count)
> +		goto out;
> +
> +	/* Link genpd masters/subdomains to model the CPU topology. */
> +	ret = psci_pd_add_topology(np);
> +	if (ret)
> +		goto remove_pd;
> +
> +	/* Try to enable OSI mode. */
> +	ret = psci_set_osi_mode();
> +	if (ret) {
> +		pr_warn("failed to enable OSI mode: %d\n", ret);
> +		psci_pd_remove_topology(np);
> +		goto remove_pd;
> +	}
> +
> +	osi_mode_enabled = true;
> +	of_node_put(np);
> +	pr_info("Initialized CPU PM domain topology\n");
> +	return pd_count;
> +
> +put_node:
> +	of_node_put(node);
> +remove_pd:
> +	if (pd_count)
> +		psci_pd_remove();
> +	pr_err("failed to create CPU PM domains ret=%d\n", ret);
> +out:
> +	of_node_put(np);
> +	return ret;
> +}
> +subsys_initcall(psci_idle_init_domains);
> +
>  struct device __init *psci_dt_attach_cpu(int cpu)
>  {
>  	struct device *dev;
>  
> +	if (!osi_mode_enabled)
> +		return NULL;
> +
>  	dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
>  	if (IS_ERR_OR_NULL(dev))
>  		return dev;
> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> index 9d779be27071..edd7a54ef0d3 100644
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -34,7 +34,7 @@ static DEFINE_PER_CPU_READ_MOSTLY(struct psci_cpuidle_data, psci_cpuidle_data);
>  static DEFINE_PER_CPU(u32, domain_state);
>  static bool psci_cpuidle_use_cpuhp __initdata;
>  
> -static inline void psci_set_domain_state(u32 state)
> +void psci_set_domain_state(u32 state)
>  {
>  	__this_cpu_write(domain_state, state);
>  }
> @@ -143,7 +143,7 @@ static const struct of_device_id psci_idle_state_match[] __initconst = {
>  	{ },
>  };
>  
> -static int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
> +int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
>  {
>  	int err = of_property_read_u32(np, "arm,psci-suspend-param", state);
>  
> diff --git a/drivers/cpuidle/cpuidle-psci.h b/drivers/cpuidle/cpuidle-psci.h
> index 8609482cf490..7299a04dd467 100644
> --- a/drivers/cpuidle/cpuidle-psci.h
> +++ b/drivers/cpuidle/cpuidle-psci.h
> @@ -3,6 +3,11 @@
>  #ifndef __CPUIDLE_PSCI_H
>  #define __CPUIDLE_PSCI_H
>  
> +struct device_node;
> +
> +void psci_set_domain_state(u32 state);
> +int __init psci_dt_parse_state_node(struct device_node *np, u32 *state);
> +
>  #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
>  struct device __init *psci_dt_attach_cpu(int cpu);
>  #else
> -- 
> 2.17.1
> 

Re: [PATCH v5 14/15] cpuidle: psci: Add support for PM domains by using genpd

[Ulf Hansson]

On Mon, 30 Dec 2019 at 17:38, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Mon, Dec 30, 2019 at 03:44:01PM +0100, Ulf Hansson wrote:
> > When the hierarchical CPU topology layout is used in DT and the PSCI OSI
> > mode is supported by the PSCI FW, let's initialize a corresponding PM
> > domain topology by using genpd. This enables a CPU and a group of CPUs,
> > when attached to the topology, to be power-managed accordingly.
> >
> > To trigger the attempt to initialize the genpd data structures let's use a
> > subsys_initcall, which should be early enough to allow CPUs, but also other
> > devices to be attached.
> >
> > The initialization consists of parsing the PSCI OF node for the topology
> > and the "domain idle states" DT bindings. In case the idle states are
> > compatible with "domain-idle-state", the initialized genpd becomes
> > responsible of selecting an idle state for the PM domain, via assigning it
> > a genpd governor.
> >
> > Note that, a successful initialization of the genpd data structures, is
> > followed by a call to psci_set_osi_mode(), as to try to enable the OSI mode
> > in the PSCI FW. In case this fails, we fall back into a degraded mode
> > rather than bailing out and returning error codes.
> >
> > Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >
> > Changes in v5:
> >       - Remove genpds in the error path, when enabling OSI failed.
> >       - Let psci_dt_attach_cpu() return NULL if OSI mode isn't enabled.
> >       - Make the osi_mode_enabled variable __initdata.
> >       - Drop some DT compatible strings, as PSCI v1.0 introduces OSI.
> >
> > ---
> >  drivers/cpuidle/cpuidle-psci-domain.c | 277 ++++++++++++++++++++++++++
> >  drivers/cpuidle/cpuidle-psci.c        |   4 +-
> >  drivers/cpuidle/cpuidle-psci.h        |   5 +
> >  3 files changed, 284 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> > index 87f881c21af1..423f03bbeb74 100644
> > --- a/drivers/cpuidle/cpuidle-psci-domain.c
> > +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> > @@ -7,18 +7,295 @@
> >   *
> >   */
> >
> > +#define pr_fmt(fmt) "CPUidle PSCI: " fmt
> > +
>
> Looks like a candidate for cpuidle-psci.h so that it's effective even for
> cpuidle-psci.c, if you re-spin(I don't see any reasons to) or if you are
> sending PR, you can fix-up this along with dropping data->dev :)

As stated in the other reply, the data->dev check is still needed.

That said, I defer your suggestion to be done on top (or via a PR if
Rafael like to get one).

>
> Anyways,
>
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

Thanks!

Kind regards
Uffe


>
> >  #include <linux/cpu.h>
> >  #include <linux/device.h>
> >  #include <linux/kernel.h>
> >  #include <linux/pm_domain.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/psci.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> >
> >  #include "cpuidle-psci.h"
> >
> > +struct psci_pd_provider {
> > +     struct list_head link;
> > +     struct device_node *node;
> > +};
> > +
> > +static LIST_HEAD(psci_pd_providers);
> > +static bool osi_mode_enabled __initdata;
> > +
> > +static int psci_pd_power_off(struct generic_pm_domain *pd)
> > +{
> > +     struct genpd_power_state *state = &pd->states[pd->state_idx];
> > +     u32 *pd_state;
> > +
> > +     if (!state->data)
> > +             return 0;
> > +
> > +     /* OSI mode is enabled, set the corresponding domain state. */
> > +     pd_state = state->data;
> > +     psci_set_domain_state(*pd_state);
> > +
> > +     return 0;
> > +}
> > +
> > +static int __init psci_pd_parse_state_nodes(struct genpd_power_state *states,
> > +                                     int state_count)
> > +{
> > +     int i, ret;
> > +     u32 psci_state, *psci_state_buf;
> > +
> > +     for (i = 0; i < state_count; i++) {
> > +             ret = psci_dt_parse_state_node(to_of_node(states[i].fwnode),
> > +                                     &psci_state);
> > +             if (ret)
> > +                     goto free_state;
> > +
> > +             psci_state_buf = kmalloc(sizeof(u32), GFP_KERNEL);
> > +             if (!psci_state_buf) {
> > +                     ret = -ENOMEM;
> > +                     goto free_state;
> > +             }
> > +             *psci_state_buf = psci_state;
> > +             states[i].data = psci_state_buf;
> > +     }
> > +
> > +     return 0;
> > +
> > +free_state:
> > +     i--;
> > +     for (; i >= 0; i--)
> > +             kfree(states[i].data);
> > +     return ret;
> > +}
> > +
> > +static int __init psci_pd_parse_states(struct device_node *np,
> > +                     struct genpd_power_state **states, int *state_count)
> > +{
> > +     int ret;
> > +
> > +     /* Parse the domain idle states. */
> > +     ret = of_genpd_parse_idle_states(np, states, state_count);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Fill out the PSCI specifics for each found state. */
> > +     ret = psci_pd_parse_state_nodes(*states, *state_count);
> > +     if (ret)
> > +             kfree(*states);
> > +
> > +     return ret;
> > +}
> > +
> > +static void psci_pd_free_states(struct genpd_power_state *states,
> > +                             unsigned int state_count)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < state_count; i++)
> > +             kfree(states[i].data);
> > +     kfree(states);
> > +}
> > +
> > +static int __init psci_pd_init(struct device_node *np)
> > +{
> > +     struct generic_pm_domain *pd;
> > +     struct psci_pd_provider *pd_provider;
> > +     struct dev_power_governor *pd_gov;
> > +     struct genpd_power_state *states = NULL;
> > +     int ret = -ENOMEM, state_count = 0;
> > +
> > +     pd = kzalloc(sizeof(*pd), GFP_KERNEL);
> > +     if (!pd)
> > +             goto out;
> > +
> > +     pd_provider = kzalloc(sizeof(*pd_provider), GFP_KERNEL);
> > +     if (!pd_provider)
> > +             goto free_pd;
> > +
> > +     pd->name = kasprintf(GFP_KERNEL, "%pOF", np);
> > +     if (!pd->name)
> > +             goto free_pd_prov;
> > +
> > +     /*
> > +      * Parse the domain idle states and let genpd manage the state selection
> > +      * for those being compatible with "domain-idle-state".
> > +      */
> > +     ret = psci_pd_parse_states(np, &states, &state_count);
> > +     if (ret)
> > +             goto free_name;
> > +
> > +     pd->free_states = psci_pd_free_states;
> > +     pd->name = kbasename(pd->name);
> > +     pd->power_off = psci_pd_power_off;
> > +     pd->states = states;
> > +     pd->state_count = state_count;
> > +     pd->flags |= GENPD_FLAG_IRQ_SAFE | GENPD_FLAG_CPU_DOMAIN;
> > +
> > +     /* Use governor for CPU PM domains if it has some states to manage. */
> > +     pd_gov = state_count > 0 ? &pm_domain_cpu_gov : NULL;
> > +
> > +     ret = pm_genpd_init(pd, pd_gov, false);
> > +     if (ret) {
> > +             psci_pd_free_states(states, state_count);
> > +             goto free_name;
> > +     }
> > +
> > +     ret = of_genpd_add_provider_simple(np, pd);
> > +     if (ret)
> > +             goto remove_pd;
> > +
> > +     pd_provider->node = of_node_get(np);
> > +     list_add(&pd_provider->link, &psci_pd_providers);
> > +
> > +     pr_debug("init PM domain %s\n", pd->name);
> > +     return 0;
> > +
> > +remove_pd:
> > +     pm_genpd_remove(pd);
> > +free_name:
> > +     kfree(pd->name);
> > +free_pd_prov:
> > +     kfree(pd_provider);
> > +free_pd:
> > +     kfree(pd);
> > +out:
> > +     pr_err("failed to init PM domain ret=%d %pOF\n", ret, np);
> > +     return ret;
> > +}
> > +
> > +static void __init psci_pd_remove(void)
> > +{
> > +     struct psci_pd_provider *pd_provider, *it;
> > +     struct generic_pm_domain *genpd;
> > +
> > +     list_for_each_entry_safe(pd_provider, it, &psci_pd_providers, link) {
> > +             of_genpd_del_provider(pd_provider->node);
> > +
> > +             genpd = of_genpd_remove_last(pd_provider->node);
> > +             if (!IS_ERR(genpd))
> > +                     kfree(genpd);
> > +
> > +             of_node_put(pd_provider->node);
> > +             list_del(&pd_provider->link);
> > +             kfree(pd_provider);
> > +     }
> > +}
> > +
> > +static int __init psci_pd_init_topology(struct device_node *np, bool add)
> > +{
> > +     struct device_node *node;
> > +     struct of_phandle_args child, parent;
> > +     int ret;
> > +
> > +     for_each_child_of_node(np, node) {
> > +             if (of_parse_phandle_with_args(node, "power-domains",
> > +                                     "#power-domain-cells", 0, &parent))
> > +                     continue;
> > +
> > +             child.np = node;
> > +             child.args_count = 0;
> > +
> > +             ret = add ? of_genpd_add_subdomain(&parent, &child) :
> > +                     of_genpd_remove_subdomain(&parent, &child);
> > +             of_node_put(parent.np);
> > +             if (ret) {
> > +                     of_node_put(node);
> > +                     return ret;
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int __init psci_pd_add_topology(struct device_node *np)
> > +{
> > +     return psci_pd_init_topology(np, true);
> > +}
> > +
> > +static void __init psci_pd_remove_topology(struct device_node *np)
> > +{
> > +     psci_pd_init_topology(np, false);
> > +}
> > +
> > +static const struct of_device_id psci_of_match[] __initconst = {
> > +     { .compatible = "arm,psci-1.0" },
> > +     {}
> > +};
> > +
> > +static int __init psci_idle_init_domains(void)
> > +{
> > +     struct device_node *np = of_find_matching_node(NULL, psci_of_match);
> > +     struct device_node *node;
> > +     int ret = 0, pd_count = 0;
> > +
> > +     if (!np)
> > +             return -ENODEV;
> > +
> > +     /* Currently limit the hierarchical topology to be used in OSI mode. */
> > +     if (!psci_has_osi_support())
> > +             goto out;
> > +
> > +     /*
> > +      * Parse child nodes for the "#power-domain-cells" property and
> > +      * initialize a genpd/genpd-of-provider pair when it's found.
> > +      */
> > +     for_each_child_of_node(np, node) {
> > +             if (!of_find_property(node, "#power-domain-cells", NULL))
> > +                     continue;
> > +
> > +             ret = psci_pd_init(node);
> > +             if (ret)
> > +                     goto put_node;
> > +
> > +             pd_count++;
> > +     }
> > +
> > +     /* Bail out if not using the hierarchical CPU topology. */
> > +     if (!pd_count)
> > +             goto out;
> > +
> > +     /* Link genpd masters/subdomains to model the CPU topology. */
> > +     ret = psci_pd_add_topology(np);
> > +     if (ret)
> > +             goto remove_pd;
> > +
> > +     /* Try to enable OSI mode. */
> > +     ret = psci_set_osi_mode();
> > +     if (ret) {
> > +             pr_warn("failed to enable OSI mode: %d\n", ret);
> > +             psci_pd_remove_topology(np);
> > +             goto remove_pd;
> > +     }
> > +
> > +     osi_mode_enabled = true;
> > +     of_node_put(np);
> > +     pr_info("Initialized CPU PM domain topology\n");
> > +     return pd_count;
> > +
> > +put_node:
> > +     of_node_put(node);
> > +remove_pd:
> > +     if (pd_count)
> > +             psci_pd_remove();
> > +     pr_err("failed to create CPU PM domains ret=%d\n", ret);
> > +out:
> > +     of_node_put(np);
> > +     return ret;
> > +}
> > +subsys_initcall(psci_idle_init_domains);
> > +
> >  struct device __init *psci_dt_attach_cpu(int cpu)
> >  {
> >       struct device *dev;
> >
> > +     if (!osi_mode_enabled)
> > +             return NULL;
> > +
> >       dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
> >       if (IS_ERR_OR_NULL(dev))
> >               return dev;
> > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > index 9d779be27071..edd7a54ef0d3 100644
> > --- a/drivers/cpuidle/cpuidle-psci.c
> > +++ b/drivers/cpuidle/cpuidle-psci.c
> > @@ -34,7 +34,7 @@ static DEFINE_PER_CPU_READ_MOSTLY(struct psci_cpuidle_data, psci_cpuidle_data);
> >  static DEFINE_PER_CPU(u32, domain_state);
> >  static bool psci_cpuidle_use_cpuhp __initdata;
> >
> > -static inline void psci_set_domain_state(u32 state)
> > +void psci_set_domain_state(u32 state)
> >  {
> >       __this_cpu_write(domain_state, state);
> >  }
> > @@ -143,7 +143,7 @@ static const struct of_device_id psci_idle_state_match[] __initconst = {
> >       { },
> >  };
> >
> > -static int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
> > +int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
> >  {
> >       int err = of_property_read_u32(np, "arm,psci-suspend-param", state);
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci.h b/drivers/cpuidle/cpuidle-psci.h
> > index 8609482cf490..7299a04dd467 100644
> > --- a/drivers/cpuidle/cpuidle-psci.h
> > +++ b/drivers/cpuidle/cpuidle-psci.h
> > @@ -3,6 +3,11 @@
> >  #ifndef __CPUIDLE_PSCI_H
> >  #define __CPUIDLE_PSCI_H
> >
> > +struct device_node;
> > +
> > +void psci_set_domain_state(u32 state);
> > +int __init psci_dt_parse_state_node(struct device_node *np, u32 *state);
> > +
> >  #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
> >  struct device __init *psci_dt_attach_cpu(int cpu);
> >  #else
> > --
> > 2.17.1
> >

[PATCH v5 15/15] arm64: dts: Convert to the hierarchical CPU topology layout for MSM8916

[Ulf Hansson]

To enable the OS to better support PSCI OS initiated CPU suspend mode,
let's convert from the flattened layout to the hierarchical layout.

In the hierarchical layout, let's create a power domain provider per CPU
and describe the idle states for each CPU inside the power domain provider
node. To group the CPUs into a cluster, let's add another power domain
provider and make it act as the master domain. Note that, the CPU's idle
states remains compatible with "arm,idle-state", while the cluster's idle
state becomes compatible with "domain-idle-state".

Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
---

Changes in v5:
	- None.

---
 arch/arm64/boot/dts/qcom/msm8916.dtsi | 57 +++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index 8686e101905c..282c36c8fa3b 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -102,10 +102,11 @@
 			reg = <0x0>;
 			next-level-cache = <&L2_0>;
 			enable-method = "psci";
-			cpu-idle-states = <&CPU_SLEEP_0>;
 			clocks = <&apcs>;
 			operating-points-v2 = <&cpu_opp_table>;
 			#cooling-cells = <2>;
+			power-domains = <&CPU_PD0>;
+			power-domain-names = "psci";
 		};
 
 		CPU1: cpu@1 {
@@ -114,10 +115,11 @@
 			reg = <0x1>;
 			next-level-cache = <&L2_0>;
 			enable-method = "psci";
-			cpu-idle-states = <&CPU_SLEEP_0>;
 			clocks = <&apcs>;
 			operating-points-v2 = <&cpu_opp_table>;
 			#cooling-cells = <2>;
+			power-domains = <&CPU_PD1>;
+			power-domain-names = "psci";
 		};
 
 		CPU2: cpu@2 {
@@ -126,10 +128,11 @@
 			reg = <0x2>;
 			next-level-cache = <&L2_0>;
 			enable-method = "psci";
-			cpu-idle-states = <&CPU_SLEEP_0>;
 			clocks = <&apcs>;
 			operating-points-v2 = <&cpu_opp_table>;
 			#cooling-cells = <2>;
+			power-domains = <&CPU_PD2>;
+			power-domain-names = "psci";
 		};
 
 		CPU3: cpu@3 {
@@ -138,10 +141,11 @@
 			reg = <0x3>;
 			next-level-cache = <&L2_0>;
 			enable-method = "psci";
-			cpu-idle-states = <&CPU_SLEEP_0>;
 			clocks = <&apcs>;
 			operating-points-v2 = <&cpu_opp_table>;
 			#cooling-cells = <2>;
+			power-domains = <&CPU_PD3>;
+			power-domain-names = "psci";
 		};
 
 		L2_0: l2-cache {
@@ -161,12 +165,57 @@
 				min-residency-us = <2000>;
 				local-timer-stop;
 			};
+
+			CLUSTER_RET: cluster-retention {
+				compatible = "domain-idle-state";
+				arm,psci-suspend-param = <0x41000012>;
+				entry-latency-us = <500>;
+				exit-latency-us = <500>;
+				min-residency-us = <2000>;
+			};
+
+			CLUSTER_PWRDN: cluster-gdhs {
+				compatible = "domain-idle-state";
+				arm,psci-suspend-param = <0x41000032>;
+				entry-latency-us = <2000>;
+				exit-latency-us = <2000>;
+				min-residency-us = <6000>;
+			};
 		};
 	};
 
 	psci {
 		compatible = "arm,psci-1.0";
 		method = "smc";
+
+		CPU_PD0: cpu-pd0 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&CPU_SLEEP_0>;
+		};
+
+		CPU_PD1: cpu-pd1 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&CPU_SLEEP_0>;
+		};
+
+		CPU_PD2: cpu-pd2 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&CPU_SLEEP_0>;
+		};
+
+		CPU_PD3: cpu-pd3 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&CPU_SLEEP_0>;
+		};
+
+		CLUSTER_PD: cluster-pd {
+			#power-domain-cells = <0>;
+			domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWRDN>;
+		};
 	};
 
 	pmu {
-- 
2.17.1

Re: [PATCH v5 00/15] cpuidle: psci: Support hierarchical CPU arrangement

[Ulf Hansson]

Rafael,

On Mon, 30 Dec 2019 at 15:44, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> Changes in v5:
>         - Remove PM domains in the error path, when failing to enable OSI.
>         - Folded in a patch that adds a genpd OF helper to remove subdomains.
>         - Added reviewed-by tags.
>
> Changes in v4:
>         - Move the check for OSI support from psci_dt_attach_cpu() to the
>         caller's side of it.
>         - Add comment in the code about using the deepest idle state as the
>         triggering point for the domain state selection.
>         - Folded in a patch to enable support for CPU hotplug.
>
> Changes in v3:
>         - Take one step further to completely avoid executing any OSI specific
>         code from the ->enter() callback, while operating in the default PSCI
>         Platform Coordinated mode.
>         - Update example for the PSCI DT bindings to make it compile with
>         "make dt_binding_check"
>
> Changes in v2:
>         - Avoid to affect the non-OSI path with specific changes for OSI. This
>         forced me to re-order the series and a caused more or less minor changes
>         to most of the patches.
>         - Updated the DT bindings for PSCI to clarify and to include the "psci"
>         name of the PM domain to attach to.
>         - Replaced patch1 with another patch from Sudeep, solving the same
>         problem, but in a different way.
>
> This series enables initial support for hierarchical CPU arrangement, managed
> by PSCI and its corresponding cpuidle driver. It's based on using the generic
> PM domain (genpd), which nowadays also supports devices belonging to CPUs.
>
> The last DTS patch enables the hierarchical topology to be used for the Qcom
> 410c Dragonboard, which supports the PSCI OS-initiated mode.
>
> More detailed background can be found from previous submissions [1].
>
> The series is also available at:
> git.linaro.org/people/ulf.hansson/linux-pm.git next
>
> Kind regards
> Ulf Hansson
>
> [1]
> https://lwn.net/Articles/788306/
>
> Lina Iyer (1):
>   cpuidle: dt: Support hierarchical CPU idle states
>
> Sudeep Holla (1):
>   cpuidle: psci: Align psci_power_state count with idle state count
>
> Ulf Hansson (13):
>   dt: psci: Update DT bindings to support hierarchical PSCI states
>   firmware: psci: Export functions to manage the OSI mode
>   of: base: Add of_get_cpu_state_node() to get idle states for a CPU
>     node
>   cpuidle: psci: Simplify OF parsing of CPU idle state nodes
>   cpuidle: psci: Support hierarchical CPU idle states
>   cpuidle: psci: Add a helper to attach a CPU to its PM domain
>   cpuidle: psci: Attach CPU devices to their PM domains
>   cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains
>   cpuidle: psci: Manage runtime PM in the idle path
>   cpuidle: psci: Support CPU hotplug for the hierarchical model
>   PM / Domains: Introduce a genpd OF helper that removes a subdomain
>   cpuidle: psci: Add support for PM domains by using genpd
>   arm64: dts: Convert to the hierarchical CPU topology layout for
>     MSM8916
>
>  .../devicetree/bindings/arm/cpus.yaml         |  15 +
>  .../devicetree/bindings/arm/psci.yaml         | 104 ++++++
>  arch/arm64/boot/dts/qcom/msm8916.dtsi         |  57 +++-
>  drivers/base/power/domain.c                   |  38 +++
>  drivers/cpuidle/Makefile                      |   4 +-
>  drivers/cpuidle/cpuidle-psci-domain.c         | 308 ++++++++++++++++++
>  drivers/cpuidle/cpuidle-psci.c                | 161 +++++++--
>  drivers/cpuidle/cpuidle-psci.h                |  17 +
>  drivers/cpuidle/dt_idle_states.c              |   5 +-
>  drivers/firmware/psci/psci.c                  |  18 +-
>  drivers/of/base.c                             |  36 ++
>  include/linux/cpuhotplug.h                    |   1 +
>  include/linux/of.h                            |   8 +
>  include/linux/pm_domain.h                     |   8 +
>  include/linux/psci.h                          |   2 +
>  15 files changed, 747 insertions(+), 35 deletions(-)
>  create mode 100644 drivers/cpuidle/cpuidle-psci-domain.c
>  create mode 100644 drivers/cpuidle/cpuidle-psci.h
>
> --
> 2.17.1
>

This series is ready to go (unless you have some objections of
course), would you mind queuing it up?

Of course, patch15 can be left for arm-soc.

If you prefer a pull-request, just tell me.

Kind regards
Uffe

Re: [PATCH v5 00/15] cpuidle: psci: Support hierarchical CPU arrangement

[Rafael J. Wysocki]

On Tue, Dec 31, 2019 at 11:38 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> Rafael,
>
> On Mon, 30 Dec 2019 at 15:44, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > Changes in v5:
> >         - Remove PM domains in the error path, when failing to enable OSI.
> >         - Folded in a patch that adds a genpd OF helper to remove subdomains.
> >         - Added reviewed-by tags.
> >
> > Changes in v4:
> >         - Move the check for OSI support from psci_dt_attach_cpu() to the
> >         caller's side of it.
> >         - Add comment in the code about using the deepest idle state as the
> >         triggering point for the domain state selection.
> >         - Folded in a patch to enable support for CPU hotplug.
> >
> > Changes in v3:
> >         - Take one step further to completely avoid executing any OSI specific
> >         code from the ->enter() callback, while operating in the default PSCI
> >         Platform Coordinated mode.
> >         - Update example for the PSCI DT bindings to make it compile with
> >         "make dt_binding_check"
> >
> > Changes in v2:
> >         - Avoid to affect the non-OSI path with specific changes for OSI. This
> >         forced me to re-order the series and a caused more or less minor changes
> >         to most of the patches.
> >         - Updated the DT bindings for PSCI to clarify and to include the "psci"
> >         name of the PM domain to attach to.
> >         - Replaced patch1 with another patch from Sudeep, solving the same
> >         problem, but in a different way.
> >
> > This series enables initial support for hierarchical CPU arrangement, managed
> > by PSCI and its corresponding cpuidle driver. It's based on using the generic
> > PM domain (genpd), which nowadays also supports devices belonging to CPUs.
> >
> > The last DTS patch enables the hierarchical topology to be used for the Qcom
> > 410c Dragonboard, which supports the PSCI OS-initiated mode.
> >
> > More detailed background can be found from previous submissions [1].
> >
> > The series is also available at:
> > git.linaro.org/people/ulf.hansson/linux-pm.git next
> >
> > Kind regards
> > Ulf Hansson
> >
> > [1]
> > https://lwn.net/Articles/788306/
> >
> > Lina Iyer (1):
> >   cpuidle: dt: Support hierarchical CPU idle states
> >
> > Sudeep Holla (1):
> >   cpuidle: psci: Align psci_power_state count with idle state count
> >
> > Ulf Hansson (13):
> >   dt: psci: Update DT bindings to support hierarchical PSCI states
> >   firmware: psci: Export functions to manage the OSI mode
> >   of: base: Add of_get_cpu_state_node() to get idle states for a CPU
> >     node
> >   cpuidle: psci: Simplify OF parsing of CPU idle state nodes
> >   cpuidle: psci: Support hierarchical CPU idle states
> >   cpuidle: psci: Add a helper to attach a CPU to its PM domain
> >   cpuidle: psci: Attach CPU devices to their PM domains
> >   cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains
> >   cpuidle: psci: Manage runtime PM in the idle path
> >   cpuidle: psci: Support CPU hotplug for the hierarchical model
> >   PM / Domains: Introduce a genpd OF helper that removes a subdomain
> >   cpuidle: psci: Add support for PM domains by using genpd
> >   arm64: dts: Convert to the hierarchical CPU topology layout for
> >     MSM8916
> >
> >  .../devicetree/bindings/arm/cpus.yaml         |  15 +
> >  .../devicetree/bindings/arm/psci.yaml         | 104 ++++++
> >  arch/arm64/boot/dts/qcom/msm8916.dtsi         |  57 +++-
> >  drivers/base/power/domain.c                   |  38 +++
> >  drivers/cpuidle/Makefile                      |   4 +-
> >  drivers/cpuidle/cpuidle-psci-domain.c         | 308 ++++++++++++++++++
> >  drivers/cpuidle/cpuidle-psci.c                | 161 +++++++--
> >  drivers/cpuidle/cpuidle-psci.h                |  17 +
> >  drivers/cpuidle/dt_idle_states.c              |   5 +-
> >  drivers/firmware/psci/psci.c                  |  18 +-
> >  drivers/of/base.c                             |  36 ++
> >  include/linux/cpuhotplug.h                    |   1 +
> >  include/linux/of.h                            |   8 +
> >  include/linux/pm_domain.h                     |   8 +
> >  include/linux/psci.h                          |   2 +
> >  15 files changed, 747 insertions(+), 35 deletions(-)
> >  create mode 100644 drivers/cpuidle/cpuidle-psci-domain.c
> >  create mode 100644 drivers/cpuidle/cpuidle-psci.h
> >
> > --
> > 2.17.1
> >
>
> This series is ready to go (unless you have some objections of
> course), would you mind queuing it up?
>
> Of course, patch15 can be left for arm-soc.
>
> If you prefer a pull-request, just tell me.

So I would prefer arm-soc to take the whole lot if that's not a problem.

Feel free to add ACKs from me to the patches for this purpose if that helps

Thanks!

Re: [PATCH v5 00/15] cpuidle: psci: Support hierarchical CPU arrangement

[Ulf Hansson]

On Tue, 31 Dec 2019 at 11:43, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Dec 31, 2019 at 11:38 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > Rafael,
> >
> > On Mon, 30 Dec 2019 at 15:44, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > Changes in v5:
> > >         - Remove PM domains in the error path, when failing to enable OSI.
> > >         - Folded in a patch that adds a genpd OF helper to remove subdomains.
> > >         - Added reviewed-by tags.
> > >
> > > Changes in v4:
> > >         - Move the check for OSI support from psci_dt_attach_cpu() to the
> > >         caller's side of it.
> > >         - Add comment in the code about using the deepest idle state as the
> > >         triggering point for the domain state selection.
> > >         - Folded in a patch to enable support for CPU hotplug.
> > >
> > > Changes in v3:
> > >         - Take one step further to completely avoid executing any OSI specific
> > >         code from the ->enter() callback, while operating in the default PSCI
> > >         Platform Coordinated mode.
> > >         - Update example for the PSCI DT bindings to make it compile with
> > >         "make dt_binding_check"
> > >
> > > Changes in v2:
> > >         - Avoid to affect the non-OSI path with specific changes for OSI. This
> > >         forced me to re-order the series and a caused more or less minor changes
> > >         to most of the patches.
> > >         - Updated the DT bindings for PSCI to clarify and to include the "psci"
> > >         name of the PM domain to attach to.
> > >         - Replaced patch1 with another patch from Sudeep, solving the same
> > >         problem, but in a different way.
> > >
> > > This series enables initial support for hierarchical CPU arrangement, managed
> > > by PSCI and its corresponding cpuidle driver. It's based on using the generic
> > > PM domain (genpd), which nowadays also supports devices belonging to CPUs.
> > >
> > > The last DTS patch enables the hierarchical topology to be used for the Qcom
> > > 410c Dragonboard, which supports the PSCI OS-initiated mode.
> > >
> > > More detailed background can be found from previous submissions [1].
> > >
> > > The series is also available at:
> > > git.linaro.org/people/ulf.hansson/linux-pm.git next
> > >
> > > Kind regards
> > > Ulf Hansson
> > >
> > > [1]
> > > https://lwn.net/Articles/788306/
> > >
> > > Lina Iyer (1):
> > >   cpuidle: dt: Support hierarchical CPU idle states
> > >
> > > Sudeep Holla (1):
> > >   cpuidle: psci: Align psci_power_state count with idle state count
> > >
> > > Ulf Hansson (13):
> > >   dt: psci: Update DT bindings to support hierarchical PSCI states
> > >   firmware: psci: Export functions to manage the OSI mode
> > >   of: base: Add of_get_cpu_state_node() to get idle states for a CPU
> > >     node
> > >   cpuidle: psci: Simplify OF parsing of CPU idle state nodes
> > >   cpuidle: psci: Support hierarchical CPU idle states
> > >   cpuidle: psci: Add a helper to attach a CPU to its PM domain
> > >   cpuidle: psci: Attach CPU devices to their PM domains
> > >   cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains
> > >   cpuidle: psci: Manage runtime PM in the idle path
> > >   cpuidle: psci: Support CPU hotplug for the hierarchical model
> > >   PM / Domains: Introduce a genpd OF helper that removes a subdomain
> > >   cpuidle: psci: Add support for PM domains by using genpd
> > >   arm64: dts: Convert to the hierarchical CPU topology layout for
> > >     MSM8916
> > >
> > >  .../devicetree/bindings/arm/cpus.yaml         |  15 +
> > >  .../devicetree/bindings/arm/psci.yaml         | 104 ++++++
> > >  arch/arm64/boot/dts/qcom/msm8916.dtsi         |  57 +++-
> > >  drivers/base/power/domain.c                   |  38 +++
> > >  drivers/cpuidle/Makefile                      |   4 +-
> > >  drivers/cpuidle/cpuidle-psci-domain.c         | 308 ++++++++++++++++++
> > >  drivers/cpuidle/cpuidle-psci.c                | 161 +++++++--
> > >  drivers/cpuidle/cpuidle-psci.h                |  17 +
> > >  drivers/cpuidle/dt_idle_states.c              |   5 +-
> > >  drivers/firmware/psci/psci.c                  |  18 +-
> > >  drivers/of/base.c                             |  36 ++
> > >  include/linux/cpuhotplug.h                    |   1 +
> > >  include/linux/of.h                            |   8 +
> > >  include/linux/pm_domain.h                     |   8 +
> > >  include/linux/psci.h                          |   2 +
> > >  15 files changed, 747 insertions(+), 35 deletions(-)
> > >  create mode 100644 drivers/cpuidle/cpuidle-psci-domain.c
> > >  create mode 100644 drivers/cpuidle/cpuidle-psci.h
> > >
> > > --
> > > 2.17.1
> > >
> >
> > This series is ready to go (unless you have some objections of
> > course), would you mind queuing it up?
> >
> > Of course, patch15 can be left for arm-soc.
> >
> > If you prefer a pull-request, just tell me.
>
> So I would prefer arm-soc to take the whole lot if that's not a problem.

Not really, I can send a PR to those guys within a day or two.

Although, I assume you are aware that most of the changes are in
drivers/cpuidle/  ...?

>
> Feel free to add ACKs from me to the patches for this purpose if that helps
>
> Thanks!

Thanks!

Kind regards
Uffe