[PATCH v9 00/11] PM / Domains: Support hierarchical CPU arrangement (PSCI/ARM) (a subset)

[PATCH v9 00/11] PM / Domains: Support hierarchical CPU arrangement (PSCI/ARM) (a subset)

[Ulf Hansson]

I have digested the review comments so far, including a recent offlist chat
with with Lorenzo Pieralisi around the debatable PSCI changes. More or less I
have a plan for how to move forward.

However, to avoid re-posting non-changed patches over and over again, I decided
to withhold the more debatable part from this v9, hence this is not the complete
series to make things play. In v9, I have just included the trivial changes,
which are either already acked/reviewed or hopefully can be rather soon/easily.

My hope is to get this queued for v4.20, to move things forward. I know it's
late, but there are more or less nothing new here since v8.

Kind regards
Ulf Hansson

Changes in v9:
 - Collect only a subset from the changes in v8.
 - Patch 3 is new, documenting existing genpd flags. Future wise, this means
when a new genpd flag is invented, we must also properly document it.
 - No changes have been made to the patches picked from v8.
 - Dropped the text from v8 cover-letter[1], to avoid confusion. When posting v10
(or whatever the next version containing the rest becomes), I am going re-write
the cover-letter to clarify, more exactly, the problems this series intends to
solve. The earlier text was simply too vague.

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

Changes in v8:
 - Added some tags for reviews and acks.
 - Cleanup timer patch (patch6) according to comments from Rafael.
 - Rebased series on top of v4.18rc1 - it applied cleanly, except for patch 5.
 - While adopting patch 5 to new genpd changes, I took the opportunity to
   improve the new function description a bit.
 - Corrected malformed SPDX-License-Identifier in patch20.

Changes in v7:
 - Addressed comments concerning the PSCI changes from Mark Rutland, which moves
   the psci firmware driver to a new firmware subdir and change to force PSCI PC
   mode during boot to cope with kexec'ed booted kernels.
 - Added some maintainers in cc for the timer/nohz patches.
 - Minor update to the new genpd governor, taking into account the state's
   poweroff latency while validating the sleep duration time.
 - Addressed a problem pointed out by Geert Uytterhoeven, around calling
   pm_runtime_get|put() for CPUs that has not been attached to a CPU PM domain.
 - Re-based on Linus' latest master.


Lina Iyer (3):
  dt: psci: Update DT bindings to support hierarchical PSCI states
  cpuidle: dt: Support hierarchical CPU idle states
  drivers: firmware: psci: Support hierarchical CPU idle states

Ulf Hansson (8):
  PM / Domains: Don't treat zero found compatible idle states as an
    error
  PM / Domains: Deal with multiple states but no governor in genpd
  PM / Domains: Document flags for genpd
  of: base: Add of_get_cpu_state_node() to get idle states for a CPU
    node
  drivers: firmware: psci: Move psci to separate directory
  MAINTAINERS: Update files for PSCI
  drivers: firmware: psci: Split psci_dt_cpu_init_idle()
  drivers: firmware: psci: Simplify error path of psci_dt_init()

 .../devicetree/bindings/arm/psci.txt          | 156 ++++++++++++++++++
 MAINTAINERS                                   |   2 +-
 drivers/base/power/domain.c                   |  20 ++-
 drivers/cpuidle/dt_idle_states.c              |   5 +-
 drivers/firmware/Kconfig                      |  15 +-
 drivers/firmware/Makefile                     |   3 +-
 drivers/firmware/psci/Kconfig                 |  13 ++
 drivers/firmware/psci/Makefile                |   4 +
 drivers/firmware/{ => psci}/psci.c            |  70 ++++----
 drivers/firmware/{ => psci}/psci_checker.c    |   0
 drivers/of/base.c                             |  35 ++++
 include/linux/of.h                            |   8 +
 include/linux/pm_domain.h                     |  35 +++-
 13 files changed, 302 insertions(+), 64 deletions(-)
 create mode 100644 drivers/firmware/psci/Kconfig
 create mode 100644 drivers/firmware/psci/Makefile
 rename drivers/firmware/{ => psci}/psci.c (95%)
 rename drivers/firmware/{ => psci}/psci_checker.c (100%)

-- 
2.17.1

[PATCH v9 01/11] PM / Domains: Don't treat zero found compatible idle states as an error

[Ulf Hansson]

Instead of returning -EINVAL from of_genpd_parse_idle_states() in case none
compatible states was found, let's return 0 to indicate success. Assign
also the out-parameter *states to NULL and *n to 0, to indicate to the
caller that zero states have been found/allocated.

This enables the caller of of_genpd_parse_idle_states() to easier act on
the returned error code.

Cc: Lina Iyer <ilina@codeaurora.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Lina Iyer <ilina@codeaurora.org>
---
 drivers/base/power/domain.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 4b5714199490..e1bbddb02871 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2478,8 +2478,8 @@ static int genpd_iterate_idle_states(struct device_node *dn,
  *
  * Returns the device states parsed from the OF node. The memory for the states
  * is allocated by this function and is the responsibility of the caller to
- * free the memory after use. If no domain idle states is found it returns
- * -EINVAL and in case of errors, a negative error code.
+ * free the memory after use. If any or zero compatible domain idle states is
+ * found it returns 0 and in case of errors, a negative error code is returned.
  */
 int of_genpd_parse_idle_states(struct device_node *dn,
 			struct genpd_power_state **states, int *n)
@@ -2488,8 +2488,14 @@ int of_genpd_parse_idle_states(struct device_node *dn,
 	int ret;
 
 	ret = genpd_iterate_idle_states(dn, NULL);
-	if (ret <= 0)
-		return ret < 0 ? ret : -EINVAL;
+	if (ret < 0)
+		return ret;
+
+	if (!ret) {
+		*states = NULL;
+		*n = 0;
+		return 0;
+	}
 
 	st = kcalloc(ret, sizeof(*st), GFP_KERNEL);
 	if (!st)
-- 
2.17.1

[PATCH v9 02/11] PM / Domains: Deal with multiple states but no governor in genpd

[Ulf Hansson]

A caller of pm_genpd_init() that provides some states for the genpd via the
->states pointer in the struct generic_pm_domain, should also provide a
governor. This because it's the job of the governor to pick a state that
satisfies the constraints.

Therefore, let's print a warning to inform the user about such bogus
configuration and avoid to bail out, by instead picking the shallowest
state before genpd invokes the ->power_off() callback.

Cc: Lina Iyer <ilina@codeaurora.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Lina Iyer <ilina@codeaurora.org>
---
 drivers/base/power/domain.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index e1bbddb02871..7f38a92b444a 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -467,6 +467,10 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
 			return -EAGAIN;
 	}
 
+	/* Default to shallowest state. */
+	if (!genpd->gov)
+		genpd->state_idx = 0;
+
 	if (genpd->power_off) {
 		int ret;
 
@@ -1687,6 +1691,8 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 		ret = genpd_set_default_power_state(genpd);
 		if (ret)
 			return ret;
+	} else if (!gov) {
+		pr_warn("%s : no governor for states\n", genpd->name);
 	}
 
 	device_initialize(&genpd->dev);
-- 
2.17.1

[PATCH v9 03/11] PM / Domains: Document flags for genpd

[Ulf Hansson]

The current documented description of the GENPD_FLAG_* flags, are too
simplified, so let's extend them.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 include/linux/pm_domain.h | 35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 776c546d581a..3b5d7280e52e 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -17,11 +17,36 @@
 #include <linux/notifier.h>
 #include <linux/spinlock.h>
 
-/* Defines used for the flags field in the struct generic_pm_domain */
-#define GENPD_FLAG_PM_CLK	 (1U << 0) /* PM domain uses PM clk */
-#define GENPD_FLAG_IRQ_SAFE	 (1U << 1) /* PM domain operates in atomic */
-#define GENPD_FLAG_ALWAYS_ON	 (1U << 2) /* PM domain is always powered on */
-#define GENPD_FLAG_ACTIVE_WAKEUP (1U << 3) /* Keep devices active if wakeup */
+/*
+ * Flags to control the behaviour of a genpd.
+ *
+ * These flags may be set in the struct generic_pm_domain's flags field by a
+ * genpd backend driver. The flags must be set before it calls pm_genpd_init(),
+ * which initializes a genpd.
+ *
+ * GENPD_FLAG_PM_CLK:		Instructs genpd to use the PM clk framework,
+ *				while powering on/off attached devices.
+ *
+ * GENPD_FLAG_IRQ_SAFE:		This informs genpd that its backend callbacks,
+ *				->power_on|off(), doesn't sleep. Hence, these
+ *				can be invoked from within atomic context, which
+ *				enables genpd to power on/off the PM domain,
+ *				even when pm_runtime_is_irq_safe() returns true,
+ *				for any of its attached devices. Note that, a
+ *				genpd having this flag set, requires its
+ *				masterdomains to also have it set.
+ *
+ * GENPD_FLAG_ALWAYS_ON:	Instructs genpd to always keep the PM domain
+ *				powered on.
+ *
+ * GENPD_FLAG_ACTIVE_WAKEUP:	Instructs genpd to keep the PM domain powered
+ *				on, in case any of its attached devices is used
+ *				in the wakeup path to serve system wakeups.
+ */
+#define GENPD_FLAG_PM_CLK	 (1U << 0)
+#define GENPD_FLAG_IRQ_SAFE	 (1U << 1)
+#define GENPD_FLAG_ALWAYS_ON	 (1U << 2)
+#define GENPD_FLAG_ACTIVE_WAKEUP (1U << 3)
 
 enum gpd_status {
 	GPD_STATE_ACTIVE = 0,	/* PM domain is active */
-- 
2.17.1

[PATCH v9 04/11] dt: psci: Update DT bindings to support hierarchical PSCI states

[Ulf Hansson]

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

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

Cc: Lina Iyer <ilina@codeaurora.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Co-developed-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
---
 .../devicetree/bindings/arm/psci.txt          | 156 ++++++++++++++++++
 1 file changed, 156 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
index a2c4f1d52492..17aa3d3a1c8e 100644
--- a/Documentation/devicetree/bindings/arm/psci.txt
+++ b/Documentation/devicetree/bindings/arm/psci.txt
@@ -105,7 +105,163 @@ Case 3: PSCI v0.2 and PSCI v0.1.
 		...
 	};
 
+ARM systems can have multiple cores sometimes in 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 in DT hierarchically.
+
+For these cases, the definitions of the idle states for the CPUs and the CPU
+topology, must conform to the domain idle state specification [3]. The domain
+idle states themselves, must be compatible with the defined 'domain-idle-state'
+binding [1], and also need to specify the arm,psci-suspend-param property for
+each idle state.
+
+DT allows representing CPUs and CPU idle states in two different ways -
+
+The flattened model as given in Example 1, lists CPU's idle states followed by
+the domain idle state that the CPUs may choose. Note that the idle states are
+all compatible with "arm,idle-state".
+
+Example 2 represents the hierarchical model of CPUs and domain idle states.
+CPUs define their domain provider in their psci DT node. The domain controls
+the power to the CPU and possibly other h/w blocks that would enter an idle
+state along with the CPU. The CPU's idle states may therefore be considered as
+the domain's idle states and have the compatible "arm,idle-state". Such domains
+may also be embedded within another domain that may represent common h/w blocks
+between these CPUs. The idle states of the CPU topology shall be represented as
+the domain's idle states.
+
+In PSCI firmware v1.0, the OS-Initiated mode is introduced. In order to use it,
+the hierarchical representation must be used.
+
+Example 1: Flattened representation of CPU and domain idle states
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		CPU0: cpu@0 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a53", "arm,armv8";
+			reg = <0x0>;
+			enable-method = "psci";
+			cpu-idle-states = <&CPU_PWRDN>, <&CLUSTER_RET>,
+					  <&CLUSTER_PWRDN>;
+		};
+
+		CPU1: cpu@1 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a57", "arm,armv8";
+			reg = <0x100>;
+			enable-method = "psci";
+			cpu-idle-states = <&CPU_PWRDN>, <&CLUSTER_RET>,
+					  <&CLUSTER_PWRDN>;
+		};
+
+		idle-states {
+			CPU_PWRDN: cpu-power-down {
+				compatible = "arm,idle-state";
+				arm,psci-suspend-param = <0x000001>;
+				entry-latency-us = <10>;
+				exit-latency-us = <10>;
+				min-residency-us = <100>;
+			};
+
+			CLUSTER_RET: cluster-retention {
+				compatible = "arm,idle-state";
+				arm,psci-suspend-param = <0x1000010>;
+				entry-latency-us = <500>;
+				exit-latency-us = <500>;
+				min-residency-us = <2000>;
+			};
+
+			CLUSTER_PWRDN: cluster-power-down {
+				compatible = "arm,idle-state";
+				arm,psci-suspend-param = <0x1000030>;
+				entry-latency-us = <2000>;
+				exit-latency-us = <2000>;
+				min-residency-us = <6000>;
+			};
+	};
+
+	psci {
+		compatible = "arm,psci-0.2";
+		method = "smc";
+	};
+
+Example 2: Hierarchical representation of CPU and domain idle states
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		CPU0: cpu@0 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a53", "arm,armv8";
+			reg = <0x0>;
+			enable-method = "psci";
+			power-domains = <&CPU_PD0>;
+		};
+
+		CPU1: cpu@1 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a57", "arm,armv8";
+			reg = <0x100>;
+			enable-method = "psci";
+			power-domains = <&CPU_PD1>;
+		};
+
+		idle-states {
+			CPU_PWRDN: cpu-power-down {
+				compatible = "arm,idle-state";
+				arm,psci-suspend-param = <0x000001>;
+				entry-latency-us = <10>;
+				exit-latency-us = <10>;
+				min-residency-us = <100>;
+			};
+
+			CLUSTER_RET: cluster-retention {
+				compatible = "domain-idle-state";
+				arm,psci-suspend-param = <0x1000010>;
+				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 = <0x1000030>;
+				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>;
+		};
+	};
+
 [1] Kernel documentation - ARM idle states bindings
     Documentation/devicetree/bindings/arm/idle-states.txt
 [2] Power State Coordination Interface (PSCI) specification
     http://infocenter.arm.com/help/topic/com.arm.doc.den0022c/DEN0022C_Power_State_Coordination_Interface.pdf
+[3]. PM Domains description
+    Documentation/devicetree/bindings/power/power_domain.txt
-- 
2.17.1

[PATCH v9 05/11] 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 back-end data for the arch specific CPU
operations, as in the PSCI driver 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 ARM, there are two generic methods, to describe the CPU's
idle states, either via the flattened description through the
"cpu-idle-states" binding [1] or via the hierarchical layout, using the
"power-domains" and the "domain-idle-states" bindings [2]. Hence, let's
take both options into account.

[1]
Documentation/devicetree/bindings/arm/idle-states.txt
[2]
Documentation/devicetree/bindings/arm/psci.txt

Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: Lina Iyer <ilina@codeaurora.org>
Suggested-by: Sudeep Holla <sudeep.holla@arm.com>
Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 drivers/of/base.c  | 35 +++++++++++++++++++++++++++++++++++
 include/linux/of.h |  8 ++++++++
 2 files changed, 43 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 74eaedd5b860..bf1d5fa34899 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -424,6 +424,41 @@ 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 and idle state node is found at index, the refcount 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);
+		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 99b0ebf49632..42dae048674c 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -353,6 +353,8 @@ extern const void *of_get_property(const struct device_node *node,
 				const char *name,
 				int *lenp);
 extern struct device_node *of_get_cpu_node(int cpu, unsigned int *thread);
+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)
 
@@ -754,6 +756,12 @@ static inline struct device_node *of_get_cpu_node(int cpu,
 	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 v9 06/11] cpuidle: dt: Support hierarchical CPU idle states

[Ulf Hansson]

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

Currently CPU's idle states are represented in a flattened model, via the
"cpu-idle-states" binding from within the CPU's device nodes.

Support the hierarchical layout during parsing and validating of the CPU's
idle states. This is simply done by calling the new OF helper,
of_get_cpu_state_node().

Cc: Lina Iyer <ilina@codeaurora.org>
Suggested-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Co-developed-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 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 53342b7f1010..13f9b7cd32d1 100644
--- a/drivers/cpuidle/dt_idle_states.c
+++ b/drivers/cpuidle/dt_idle_states.c
@@ -118,8 +118,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;
 
@@ -176,7 +175,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 v9 07/11] drivers: firmware: psci: Move psci to separate directory

[Ulf Hansson]

Some following changes extends the PSCI driver with some additional new
files.  Let's avoid to continue cluttering the toplevel firmware directory
and first move the PSCI files into a PSCI sub-directory.

Suggested-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/firmware/Kconfig                   | 15 +--------------
 drivers/firmware/Makefile                  |  3 +--
 drivers/firmware/psci/Kconfig              | 13 +++++++++++++
 drivers/firmware/psci/Makefile             |  4 ++++
 drivers/firmware/{ => psci}/psci.c         |  0
 drivers/firmware/{ => psci}/psci_checker.c |  0
 6 files changed, 19 insertions(+), 16 deletions(-)
 create mode 100644 drivers/firmware/psci/Kconfig
 create mode 100644 drivers/firmware/psci/Makefile
 rename drivers/firmware/{ => psci}/psci.c (100%)
 rename drivers/firmware/{ => psci}/psci_checker.c (100%)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 6e83880046d7..923c42d5a2e6 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -5,20 +5,6 @@
 
 menu "Firmware Drivers"
 
-config ARM_PSCI_FW
-	bool
-
-config ARM_PSCI_CHECKER
-	bool "ARM PSCI checker"
-	depends on ARM_PSCI_FW && HOTPLUG_CPU && CPU_IDLE && !TORTURE_TEST
-	help
-	  Run the PSCI checker during startup. This checks that hotplug and
-	  suspend operations work correctly when using PSCI.
-
-	  The torture tests may interfere with the PSCI checker by turning CPUs
-	  on and off through hotplug, so for now torture tests and PSCI checker
-	  are mutually exclusive.
-
 config ARM_SCMI_PROTOCOL
 	bool "ARM System Control and Management Interface (SCMI) Message Protocol"
 	depends on ARM || ARM64 || COMPILE_TEST
@@ -286,6 +272,7 @@ config TI_SCI_PROTOCOL
 config HAVE_ARM_SMCCC
 	bool
 
+source "drivers/firmware/psci/Kconfig"
 source "drivers/firmware/broadcom/Kconfig"
 source "drivers/firmware/google/Kconfig"
 source "drivers/firmware/efi/Kconfig"
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index e18a041cfc53..ea284e551dc8 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -2,8 +2,6 @@
 #
 # Makefile for the linux kernel.
 #
-obj-$(CONFIG_ARM_PSCI_FW)	+= psci.o
-obj-$(CONFIG_ARM_PSCI_CHECKER)	+= psci_checker.o
 obj-$(CONFIG_ARM_SCPI_PROTOCOL)	+= arm_scpi.o
 obj-$(CONFIG_ARM_SCPI_POWER_DOMAIN) += scpi_pm_domain.o
 obj-$(CONFIG_ARM_SDE_INTERFACE)	+= arm_sdei.o
@@ -26,6 +24,7 @@ CFLAGS_qcom_scm-32.o :=$(call as-instr,.arch armv7-a\n.arch_extension sec,-DREQU
 obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
 
 obj-$(CONFIG_ARM_SCMI_PROTOCOL)	+= arm_scmi/
+obj-y				+= psci/
 obj-y				+= broadcom/
 obj-y				+= meson/
 obj-$(CONFIG_GOOGLE_FIRMWARE)	+= google/
diff --git a/drivers/firmware/psci/Kconfig b/drivers/firmware/psci/Kconfig
new file mode 100644
index 000000000000..26a3b32bf7ab
--- /dev/null
+++ b/drivers/firmware/psci/Kconfig
@@ -0,0 +1,13 @@
+config ARM_PSCI_FW
+	bool
+
+config ARM_PSCI_CHECKER
+	bool "ARM PSCI checker"
+	depends on ARM_PSCI_FW && HOTPLUG_CPU && CPU_IDLE && !TORTURE_TEST
+	help
+	  Run the PSCI checker during startup. This checks that hotplug and
+	  suspend operations work correctly when using PSCI.
+
+	  The torture tests may interfere with the PSCI checker by turning CPUs
+	  on and off through hotplug, so for now torture tests and PSCI checker
+	  are mutually exclusive.
diff --git a/drivers/firmware/psci/Makefile b/drivers/firmware/psci/Makefile
new file mode 100644
index 000000000000..1956b882470f
--- /dev/null
+++ b/drivers/firmware/psci/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+obj-$(CONFIG_ARM_PSCI_FW)	+= psci.o
+obj-$(CONFIG_ARM_PSCI_CHECKER)	+= psci_checker.o
diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci/psci.c
similarity index 100%
rename from drivers/firmware/psci.c
rename to drivers/firmware/psci/psci.c
diff --git a/drivers/firmware/psci_checker.c b/drivers/firmware/psci/psci_checker.c
similarity index 100%
rename from drivers/firmware/psci_checker.c
rename to drivers/firmware/psci/psci_checker.c
-- 
2.17.1

[PATCH v9 08/11] MAINTAINERS: Update files for PSCI

[Ulf Hansson]

The files for the PSCI firmware driver were moved to a sub-directory,
let's update MAINTAINERS to reflect that.

Suggested-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index b22e7fdfd2ea..710b906037ce 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11624,7 +11624,7 @@ M:	Mark Rutland <mark.rutland@arm.com>
 M:	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
 L:	linux-arm-kernel@lists.infradead.org
 S:	Maintained
-F:	drivers/firmware/psci*.c
+F:	drivers/firmware/psci/
 F:	include/linux/psci.h
 F:	include/uapi/linux/psci.h
 
-- 
2.17.1

[PATCH v9 09/11] drivers: firmware: psci: Split psci_dt_cpu_init_idle()

[Ulf Hansson]

Let's split psci_dt_cpu_init_idle() function into two functions, as to
allow following changes to re-use some of the code.

Cc: Lina Iyer <ilina@codeaurora.org>
Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/firmware/psci/psci.c | 42 ++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index c80ec1d03274..9788bfc1cf8b 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -270,9 +270,26 @@ static int __init psci_features(u32 psci_func_id)
 #ifdef CONFIG_CPU_IDLE
 static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
 
+static int psci_dt_parse_state_node(struct device_node *np, u32 *state)
+{
+	int err = of_property_read_u32(np, "arm,psci-suspend-param", state);
+
+	if (err) {
+		pr_warn("%pOF missing arm,psci-suspend-param property\n", np);
+		return err;
+	}
+
+	if (!psci_power_state_is_valid(*state)) {
+		pr_warn("Invalid PSCI power state %#x\n", *state);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
 {
-	int i, ret, count = 0;
+	int i, ret = 0, count = 0;
 	u32 *psci_states;
 	struct device_node *state_node;
 
@@ -291,29 +308,16 @@ static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
 		return -ENOMEM;
 
 	for (i = 0; i < count; i++) {
-		u32 state;
-
 		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
+		ret = psci_dt_parse_state_node(state_node, &psci_states[i]);
+		of_node_put(state_node);
 
-		ret = of_property_read_u32(state_node,
-					   "arm,psci-suspend-param",
-					   &state);
-		if (ret) {
-			pr_warn(" * %pOF missing arm,psci-suspend-param property\n",
-				state_node);
-			of_node_put(state_node);
+		if (ret)
 			goto free_mem;
-		}
 
-		of_node_put(state_node);
-		pr_debug("psci-power-state %#x index %d\n", state, i);
-		if (!psci_power_state_is_valid(state)) {
-			pr_warn("Invalid PSCI power state %#x\n", state);
-			ret = -EINVAL;
-			goto free_mem;
-		}
-		psci_states[i] = state;
+		pr_debug("psci-power-state %#x index %d\n", psci_states[i], i);
 	}
+
 	/* Idle states parsed correctly, initialize per-cpu pointer */
 	per_cpu(psci_power_state, cpu) = psci_states;
 	return 0;
-- 
2.17.1

[PATCH v9 10/11] drivers: firmware: psci: Support hierarchical CPU idle states

[Ulf Hansson]

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

Currently CPU's idle states are represented in a flattened model, via the
"cpu-idle-states" binding from within the CPU's device nodes.

Support the hierarchical layout, simply by converting to calling the new OF
helper, of_get_cpu_state_node().

Cc: Lina Iyer <ilina@codeaurora.org>
Suggested-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Co-developed-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/firmware/psci/psci.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index 9788bfc1cf8b..256b4edbb20a 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -294,8 +294,7 @@ static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
 	struct device_node *state_node;
 
 	/* Count idle states */
-	while ((state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
-					      count))) {
+	while ((state_node = of_get_cpu_state_node(cpu_node, count))) {
 		count++;
 		of_node_put(state_node);
 	}
@@ -308,7 +307,7 @@ static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
 		return -ENOMEM;
 
 	for (i = 0; i < count; i++) {
-		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
+		state_node = of_get_cpu_state_node(cpu_node, i);
 		ret = psci_dt_parse_state_node(state_node, &psci_states[i]);
 		of_node_put(state_node);
 
-- 
2.17.1

[PATCH v9 11/11] drivers: firmware: psci: Simplify error path of psci_dt_init()

[Ulf Hansson]

Instead of having each psci init function taking care of the of_node_put(),
let's deal with that from psci_dt_init(), as this enables a bit simpler
error path for each psci init function.

Cc: Lina Iyer <ilina@codeaurora.org>
Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Acked-by: Mark Rutland <mark.rutland@arm.com>
---
 drivers/firmware/psci/psci.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index 256b4edbb20a..38881007584e 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -608,9 +608,9 @@ static int __init psci_0_2_init(struct device_node *np)
 	int err;
 
 	err = get_set_conduit_method(np);
-
 	if (err)
-		goto out_put_node;
+		return err;
+
 	/*
 	 * Starting with v0.2, the PSCI specification introduced a call
 	 * (PSCI_VERSION) that allows probing the firmware version, so
@@ -618,11 +618,7 @@ static int __init psci_0_2_init(struct device_node *np)
 	 * can be carried out according to the specific version reported
 	 * by firmware
 	 */
-	err = psci_probe();
-
-out_put_node:
-	of_node_put(np);
-	return err;
+	return psci_probe();
 }
 
 /*
@@ -634,9 +630,8 @@ static int __init psci_0_1_init(struct device_node *np)
 	int err;
 
 	err = get_set_conduit_method(np);
-
 	if (err)
-		goto out_put_node;
+		return err;
 
 	pr_info("Using PSCI v0.1 Function IDs from DT\n");
 
@@ -660,9 +655,7 @@ static int __init psci_0_1_init(struct device_node *np)
 		psci_ops.migrate = psci_migrate;
 	}
 
-out_put_node:
-	of_node_put(np);
-	return err;
+	return 0;
 }
 
 static const struct of_device_id psci_of_match[] __initconst = {
@@ -677,6 +670,7 @@ int __init psci_dt_init(void)
 	struct device_node *np;
 	const struct of_device_id *matched_np;
 	psci_initcall_t init_fn;
+	int ret;
 
 	np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np);
 
@@ -684,7 +678,10 @@ int __init psci_dt_init(void)
 		return -ENODEV;
 
 	init_fn = (psci_initcall_t)matched_np->data;
-	return init_fn(np);
+	ret = init_fn(np);
+
+	of_node_put(np);
+	return ret;
 }
 
 #ifdef CONFIG_ACPI
-- 
2.17.1

Re: [PATCH v9 00/11] PM / Domains: Support hierarchical CPU arrangement (PSCI/ARM) (a subset)

[Rafael J. Wysocki]

On Wed, Oct 3, 2018 at 4:39 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> I have digested the review comments so far, including a recent offlist chat
> with with Lorenzo Pieralisi around the debatable PSCI changes. More or less I
> have a plan for how to move forward.
>
> However, to avoid re-posting non-changed patches over and over again, I decided
> to withhold the more debatable part from this v9, hence this is not the complete
> series to make things play. In v9, I have just included the trivial changes,
> which are either already acked/reviewed or hopefully can be rather soon/easily.
>
> My hope is to get this queued for v4.20, to move things forward. I know it's
> late, but there are more or less nothing new here since v8.

I have no problems with the first three patches in this series, so I
can apply them right away.  Do you want me to do that?

As for the rest, the cpuidle driver patch looks OK to me, but the
PSCI-related ones need ACKs.

Thanks,
Rafael

Re: [PATCH v9 00/11] PM / Domains: Support hierarchical CPU arrangement (PSCI/ARM) (a subset)

[Ulf Hansson]

On 4 October 2018 at 10:39, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Oct 3, 2018 at 4:39 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>
>> I have digested the review comments so far, including a recent offlist chat
>> with with Lorenzo Pieralisi around the debatable PSCI changes. More or less I
>> have a plan for how to move forward.
>>
>> However, to avoid re-posting non-changed patches over and over again, I decided
>> to withhold the more debatable part from this v9, hence this is not the complete
>> series to make things play. In v9, I have just included the trivial changes,
>> which are either already acked/reviewed or hopefully can be rather soon/easily.
>>
>> My hope is to get this queued for v4.20, to move things forward. I know it's
>> late, but there are more or less nothing new here since v8.
>
> I have no problems with the first three patches in this series, so I
> can apply them right away.  Do you want me to do that?

Yes, please.

>
> As for the rest, the cpuidle driver patch looks OK to me, but the
> PSCI-related ones need ACKs.

For some yes, but I think you can go ahead with a few more.

Patch 4, 5 is already acked/reviewed.

Patch 6 should be fine (if you are okay with it else wait for an ack
from Daniel)

Patch 7 and 8 should be fine. They were suggested by Mark.

Patch 9 and 10 needs acks.

Patch 11 has been acked, but depends on the other PSCI changes.

Kind regards
Uffe

Re: [PATCH v9 00/11] PM / Domains: Support hierarchical CPU arrangement (PSCI/ARM) (a subset)

[Rafael J. Wysocki]

On Thursday, October 4, 2018 10:58:53 AM CEST Ulf Hansson wrote:
> On 4 October 2018 at 10:39, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Wed, Oct 3, 2018 at 4:39 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >>
> >> I have digested the review comments so far, including a recent offlist chat
> >> with with Lorenzo Pieralisi around the debatable PSCI changes. More or less I
> >> have a plan for how to move forward.
> >>
> >> However, to avoid re-posting non-changed patches over and over again, I decided
> >> to withhold the more debatable part from this v9, hence this is not the complete
> >> series to make things play. In v9, I have just included the trivial changes,
> >> which are either already acked/reviewed or hopefully can be rather soon/easily.
> >>
> >> My hope is to get this queued for v4.20, to move things forward. I know it's
> >> late, but there are more or less nothing new here since v8.
> >
> > I have no problems with the first three patches in this series, so I
> > can apply them right away.  Do you want me to do that?
> 
> Yes, please.
> 
> >
> > As for the rest, the cpuidle driver patch looks OK to me, but the
> > PSCI-related ones need ACKs.
> 
> For some yes, but I think you can go ahead with a few more.
> 
> Patch 4, 5 is already acked/reviewed.
> 
> Patch 6 should be fine (if you are okay with it else wait for an ack
> from Daniel)

OK, thanks.

Do the 4-6 depend on the 1-3?

> Patch 7 and 8 should be fine. They were suggested by Mark.

I'd rather have ACKs on these two as well.

> Patch 9 and 10 needs acks.
> 
> Patch 11 has been acked, but depends on the other PSCI changes.

OK

Re: [PATCH v9 00/11] PM / Domains: Support hierarchical CPU arrangement (PSCI/ARM) (a subset)

[Rafael J. Wysocki]

On Thu, Oct 4, 2018 at 11:04 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Thursday, October 4, 2018 10:58:53 AM CEST Ulf Hansson wrote:
> > On 4 October 2018 at 10:39, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > On Wed, Oct 3, 2018 at 4:39 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >>
> > >> I have digested the review comments so far, including a recent offlist chat
> > >> with with Lorenzo Pieralisi around the debatable PSCI changes. More or less I
> > >> have a plan for how to move forward.
> > >>
> > >> However, to avoid re-posting non-changed patches over and over again, I decided
> > >> to withhold the more debatable part from this v9, hence this is not the complete
> > >> series to make things play. In v9, I have just included the trivial changes,
> > >> which are either already acked/reviewed or hopefully can be rather soon/easily.
> > >>
> > >> My hope is to get this queued for v4.20, to move things forward. I know it's
> > >> late, but there are more or less nothing new here since v8.
> > >
> > > I have no problems with the first three patches in this series, so I
> > > can apply them right away.  Do you want me to do that?
> >
> > Yes, please.
> >
> > >
> > > As for the rest, the cpuidle driver patch looks OK to me, but the
> > > PSCI-related ones need ACKs.
> >
> > For some yes, but I think you can go ahead with a few more.
> >
> > Patch 4, 5 is already acked/reviewed.
> >
> > Patch 6 should be fine (if you are okay with it else wait for an ack
> > from Daniel)
>
> OK, thanks.
>
> Do the 4-6 depend on the 1-3?

I don't see any dependency there, so I'll queue up the 1-3 in
pm-domains and the 4-6 in pm-cpuidle.

Thanks,
Rafael

Re: [PATCH v9 00/11] PM / Domains: Support hierarchical CPU arrangement (PSCI/ARM) (a subset)

[Ulf Hansson]

On 4 October 2018 at 11:32, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, Oct 4, 2018 at 11:04 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>
>> On Thursday, October 4, 2018 10:58:53 AM CEST Ulf Hansson wrote:
>> > On 4 October 2018 at 10:39, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> > > On Wed, Oct 3, 2018 at 4:39 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> > >>
>> > >> I have digested the review comments so far, including a recent offlist chat
>> > >> with with Lorenzo Pieralisi around the debatable PSCI changes. More or less I
>> > >> have a plan for how to move forward.
>> > >>
>> > >> However, to avoid re-posting non-changed patches over and over again, I decided
>> > >> to withhold the more debatable part from this v9, hence this is not the complete
>> > >> series to make things play. In v9, I have just included the trivial changes,
>> > >> which are either already acked/reviewed or hopefully can be rather soon/easily.
>> > >>
>> > >> My hope is to get this queued for v4.20, to move things forward. I know it's
>> > >> late, but there are more or less nothing new here since v8.
>> > >
>> > > I have no problems with the first three patches in this series, so I
>> > > can apply them right away.  Do you want me to do that?
>> >
>> > Yes, please.
>> >
>> > >
>> > > As for the rest, the cpuidle driver patch looks OK to me, but the
>> > > PSCI-related ones need ACKs.
>> >
>> > For some yes, but I think you can go ahead with a few more.
>> >
>> > Patch 4, 5 is already acked/reviewed.
>> >
>> > Patch 6 should be fine (if you are okay with it else wait for an ack
>> > from Daniel)
>>
>> OK, thanks.
>>
>> Do the 4-6 depend on the 1-3?
>
> I don't see any dependency there, so I'll queue up the 1-3 in
> pm-domains and the 4-6 in pm-cpuidle.

Great, thanks!

Make sure you put the remaining of the PSCI changes on pm-cpuidle
(once acked), as there are dependency.

Kind regards
Uffe

Re: [PATCH v9 03/11] PM / Domains: Document flags for genpd

[Tony Lindgren]

Hi,

* Ulf Hansson <ulf.hansson@linaro.org> [181003 14:43]:
> + * GENPD_FLAG_IRQ_SAFE:		This informs genpd that its backend callbacks,
> + *				->power_on|off(), doesn't sleep. Hence, these
> + *				can be invoked from within atomic context, which
> + *				enables genpd to power on/off the PM domain,
> + *				even when pm_runtime_is_irq_safe() returns true,
> + *				for any of its attached devices. Note that, a
> + *				genpd having this flag set, requires its
> + *				masterdomains to also have it set.
> + *

Let's try to avoid adding more irq_safe stuff because of having that
propagate to the masterdomains..

I think you can just flag the power_on/off in genpd, then have cpu_pm
callbacks do it.

Regards,

Tony

Re: [PATCH v9 03/11] PM / Domains: Document flags for genpd

[Ulf Hansson]

On 4 October 2018 at 15:48, Tony Lindgren <tony@atomide.com> wrote:
> Hi,
>
> * Ulf Hansson <ulf.hansson@linaro.org> [181003 14:43]:
>> + * GENPD_FLAG_IRQ_SAFE:              This informs genpd that its backend callbacks,
>> + *                           ->power_on|off(), doesn't sleep. Hence, these
>> + *                           can be invoked from within atomic context, which
>> + *                           enables genpd to power on/off the PM domain,
>> + *                           even when pm_runtime_is_irq_safe() returns true,
>> + *                           for any of its attached devices. Note that, a
>> + *                           genpd having this flag set, requires its
>> + *                           masterdomains to also have it set.
>> + *
>
> Let's try to avoid adding more irq_safe stuff because of having that
> propagate to the masterdomains..

I am not sure I get your point. This is just documenting existing
functionality in genpd, there is nothing new here.

>
> I think you can just flag the power_on/off in genpd, then have cpu_pm
> callbacks do it.

Not really sure what you propose, but feel free to send a patch.

Do note, genpd has since the beginning of its time tried to cope with
pm_runtime_irq_safe() devices. I admit it's quite complicated, however
GENPD_FLAG_IRQ_SAFE greatly improved the support for such devices and
their PM domains. Moreover, we need this functionality, in one way or
the other, as long as there users of pm_runtime_irq_safe().

Kind regards
Uffe

Re: [PATCH v9 00/11] PM / Domains: Support hierarchical CPU arrangement (PSCI/ARM) (a subset)

[Lorenzo Pieralisi]

On Thu, Oct 04, 2018 at 11:32:41AM +0200, Rafael J. Wysocki wrote:
> On Thu, Oct 4, 2018 at 11:04 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > On Thursday, October 4, 2018 10:58:53 AM CEST Ulf Hansson wrote:
> > > On 4 October 2018 at 10:39, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > On Wed, Oct 3, 2018 at 4:39 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >>
> > > >> I have digested the review comments so far, including a recent offlist chat
> > > >> with with Lorenzo Pieralisi around the debatable PSCI changes. More or less I
> > > >> have a plan for how to move forward.
> > > >>
> > > >> However, to avoid re-posting non-changed patches over and over again, I decided
> > > >> to withhold the more debatable part from this v9, hence this is not the complete
> > > >> series to make things play. In v9, I have just included the trivial changes,
> > > >> which are either already acked/reviewed or hopefully can be rather soon/easily.
> > > >>
> > > >> My hope is to get this queued for v4.20, to move things forward. I know it's
> > > >> late, but there are more or less nothing new here since v8.
> > > >
> > > > I have no problems with the first three patches in this series, so I
> > > > can apply them right away.  Do you want me to do that?
> > >
> > > Yes, please.
> > >
> > > >
> > > > As for the rest, the cpuidle driver patch looks OK to me, but the
> > > > PSCI-related ones need ACKs.
> > >
> > > For some yes, but I think you can go ahead with a few more.
> > >
> > > Patch 4, 5 is already acked/reviewed.
> > >
> > > Patch 6 should be fine (if you are okay with it else wait for an ack
> > > from Daniel)
> >
> > OK, thanks.
> >
> > Do the 4-6 depend on the 1-3?
> 
> I don't see any dependency there, so I'll queue up the 1-3 in
> pm-domains and the 4-6 in pm-cpuidle.

I do not see why we should merge patches 4-6 for v4.20; they add legacy
(DT bindings and related parsing code) with no user in the kernel; we
may still want to tweak them, in particular PSCI DT bindings.

Likewise, it makes no sense to merge patches 7-8 without the rest of
the PSCI patches.

Why do not we target v4.20-rc1 for the whole series re-posting and we take
it from there given that we are at -rc6 tail end ?

Thanks,
Lorenzo

Re: [PATCH v9 03/11] PM / Domains: Document flags for genpd

[Tony Lindgren]

* Ulf Hansson <ulf.hansson@linaro.org> [181004 15:02]:
> On 4 October 2018 at 15:48, Tony Lindgren <tony@atomide.com> wrote:
> > Hi,
> >
> > * Ulf Hansson <ulf.hansson@linaro.org> [181003 14:43]:
> >> + * GENPD_FLAG_IRQ_SAFE:              This informs genpd that its backend callbacks,
> >> + *                           ->power_on|off(), doesn't sleep. Hence, these
> >> + *                           can be invoked from within atomic context, which
> >> + *                           enables genpd to power on/off the PM domain,
> >> + *                           even when pm_runtime_is_irq_safe() returns true,
> >> + *                           for any of its attached devices. Note that, a
> >> + *                           genpd having this flag set, requires its
> >> + *                           masterdomains to also have it set.
> >> + *
> >
> > Let's try to avoid adding more irq_safe stuff because of having that
> > propagate to the masterdomains..
> 
> I am not sure I get your point. This is just documenting existing
> functionality in genpd, there is nothing new here.

Right, and I'm just bringing up that this FLAG_IRQ_SAFE is not a
good way to in the long run :)

> > I think you can just flag the power_on/off in genpd, then have cpu_pm
> > callbacks do it.
> 
> Not really sure what you propose, but feel free to send a patch.

Well there is not much to really patch, just don't attempt to
do irq_safe stuff from genpd and have cpu_idle callbacks to do
it instead. And then no need for GENPD_FLAG_IRQ_SAFE :)

> Do note, genpd has since the beginning of its time tried to cope with
> pm_runtime_irq_safe() devices. I admit it's quite complicated, however
> GENPD_FLAG_IRQ_SAFE greatly improved the support for such devices and
> their PM domains. Moreover, we need this functionality, in one way or
> the other, as long as there users of pm_runtime_irq_safe().

Right, and I'm still struggling years after with legacy device drivers
that have done pm_runtime_irq_safe() and come to the conclusion that
it should not be used at all. Getting rid of GENPD_FLAG_IRQ_SAFE
might just safe you years of pain later on.

Regards,

Tony

Re: [PATCH v9 00/11] PM / Domains: Support hierarchical CPU arrangement (PSCI/ARM) (a subset)

[Rafael J. Wysocki]

On Thu, Oct 4, 2018 at 5:58 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Thu, Oct 04, 2018 at 11:32:41AM +0200, Rafael J. Wysocki wrote:
> > On Thu, Oct 4, 2018 at 11:04 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >
> > > On Thursday, October 4, 2018 10:58:53 AM CEST Ulf Hansson wrote:
> > > > On 4 October 2018 at 10:39, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > On Wed, Oct 3, 2018 at 4:39 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >>
> > > > >> I have digested the review comments so far, including a recent offlist chat
> > > > >> with with Lorenzo Pieralisi around the debatable PSCI changes. More or less I
> > > > >> have a plan for how to move forward.
> > > > >>
> > > > >> However, to avoid re-posting non-changed patches over and over again, I decided
> > > > >> to withhold the more debatable part from this v9, hence this is not the complete
> > > > >> series to make things play. In v9, I have just included the trivial changes,
> > > > >> which are either already acked/reviewed or hopefully can be rather soon/easily.
> > > > >>
> > > > >> My hope is to get this queued for v4.20, to move things forward. I know it's
> > > > >> late, but there are more or less nothing new here since v8.
> > > > >
> > > > > I have no problems with the first three patches in this series, so I
> > > > > can apply them right away.  Do you want me to do that?
> > > >
> > > > Yes, please.
> > > >
> > > > >
> > > > > As for the rest, the cpuidle driver patch looks OK to me, but the
> > > > > PSCI-related ones need ACKs.
> > > >
> > > > For some yes, but I think you can go ahead with a few more.
> > > >
> > > > Patch 4, 5 is already acked/reviewed.
> > > >
> > > > Patch 6 should be fine (if you are okay with it else wait for an ack
> > > > from Daniel)
> > >
> > > OK, thanks.
> > >
> > > Do the 4-6 depend on the 1-3?
> >
> > I don't see any dependency there, so I'll queue up the 1-3 in
> > pm-domains and the 4-6 in pm-cpuidle.
>
> I do not see why we should merge patches 4-6 for v4.20; they add legacy
> (DT bindings and related parsing code) with no user in the kernel; we
> may still want to tweak them, in particular PSCI DT bindings.

My impression was that 4-6 have been agreed on due to the ACKs they
carry.  I'll drop them if that's not the case.

> Likewise, it makes no sense to merge patches 7-8 without the rest of
> the PSCI patches.

OK

I'll let the ARM camp sort out the PSCI material then.

Thanks,
Rafael

Re: [PATCH v9 00/11] PM / Domains: Support hierarchical CPU arrangement (PSCI/ARM) (a subset)

[Lorenzo Pieralisi]

On Thu, Oct 04, 2018 at 07:07:27PM +0200, Rafael J. Wysocki wrote:

[...]

> > > I don't see any dependency there, so I'll queue up the 1-3 in
> > > pm-domains and the 4-6 in pm-cpuidle.
> >
> > I do not see why we should merge patches 4-6 for v4.20; they add legacy
> > (DT bindings and related parsing code) with no user in the kernel; we
> > may still want to tweak them, in particular PSCI DT bindings.
> 
> My impression was that 4-6 have been agreed on due to the ACKs they
> carry.  I'll drop them if that's not the case.

I have not expressed myself correctly: they have been agreed (even
though as I said they may require some tweaking) but I see no urgency
of merging them in v4.20 since they have no user. They contain DT
bindings, that create ABI/legacy, I think it is better to have code
that uses them in the kernel before merging them and creating a
dependency that is not needed.

> > Likewise, it makes no sense to merge patches 7-8 without the rest of
> > the PSCI patches.
> 
> OK
> 
> I'll let the ARM camp sort out the PSCI material then.

We will do.

Thanks,
Lorenzo

Re: [PATCH v9 00/11] PM / Domains: Support hierarchical CPU arrangement (PSCI/ARM) (a subset)

[Ulf Hansson]

On 4 October 2018 at 19:21, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Thu, Oct 04, 2018 at 07:07:27PM +0200, Rafael J. Wysocki wrote:
>
> [...]
>
>> > > I don't see any dependency there, so I'll queue up the 1-3 in
>> > > pm-domains and the 4-6 in pm-cpuidle.
>> >
>> > I do not see why we should merge patches 4-6 for v4.20; they add legacy
>> > (DT bindings and related parsing code) with no user in the kernel; we
>> > may still want to tweak them, in particular PSCI DT bindings.
>>
>> My impression was that 4-6 have been agreed on due to the ACKs they
>> carry.  I'll drop them if that's not the case.
>
> I have not expressed myself correctly: they have been agreed (even
> though as I said they may require some tweaking) but I see no urgency
> of merging them in v4.20 since they have no user. They contain DT
> bindings, that create ABI/legacy, I think it is better to have code
> that uses them in the kernel before merging them and creating a
> dependency that is not needed.

There is already code using the new bindings, for the idle states.
Please have look at patch 5, 6 and 11.

Moreover, you have had plenty on time to look at the series, as those
patches haven't changed since a very long time.

May I suggest you do the review instead, so we can move things
forward, please. The changes in the v9 series should be trivial to
review.

>
>> > Likewise, it makes no sense to merge patches 7-8 without the rest of
>> > the PSCI patches.

Well, those patches are part of this series, because Mark wanted me to
move the files. Is really such a big deal? I think it makes sense, no
matter what happens afterwards.

[...]

Kind regards
Uffe

Re: [PATCH v9 00/11] PM / Domains: Support hierarchical CPU arrangement (PSCI/ARM) (a subset)

[Ulf Hansson]

On 4 October 2018 at 20:36, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 4 October 2018 at 19:21, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
>> On Thu, Oct 04, 2018 at 07:07:27PM +0200, Rafael J. Wysocki wrote:
>>
>> [...]
>>
>>> > > I don't see any dependency there, so I'll queue up the 1-3 in
>>> > > pm-domains and the 4-6 in pm-cpuidle.
>>> >
>>> > I do not see why we should merge patches 4-6 for v4.20; they add legacy
>>> > (DT bindings and related parsing code) with no user in the kernel; we
>>> > may still want to tweak them, in particular PSCI DT bindings.
>>>
>>> My impression was that 4-6 have been agreed on due to the ACKs they
>>> carry.  I'll drop them if that's not the case.
>>
>> I have not expressed myself correctly: they have been agreed (even
>> though as I said they may require some tweaking) but I see no urgency
>> of merging them in v4.20 since they have no user. They contain DT
>> bindings, that create ABI/legacy, I think it is better to have code
>> that uses them in the kernel before merging them and creating a
>> dependency that is not needed.
>
> There is already code using the new bindings, for the idle states.
> Please have look at patch 5, 6 and 11.

Should be 5, 6 and 10, sorry.

>
> Moreover, you have had plenty on time to look at the series, as those
> patches haven't changed since a very long time.
>
> May I suggest you do the review instead, so we can move things
> forward, please. The changes in the v9 series should be trivial to
> review.
>
>>
>>> > Likewise, it makes no sense to merge patches 7-8 without the rest of
>>> > the PSCI patches.
>
> Well, those patches are part of this series, because Mark wanted me to
> move the files. Is really such a big deal? I think it makes sense, no
> matter what happens afterwards.
>
> [...]
>
> Kind regards
> Uffe

Re: [PATCH v9 00/11] PM / Domains: Support hierarchical CPU arrangement (PSCI/ARM) (a subset)

[Lorenzo Pieralisi]

On Thu, Oct 04, 2018 at 08:36:24PM +0200, Ulf Hansson wrote:
> On 4 October 2018 at 19:21, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > On Thu, Oct 04, 2018 at 07:07:27PM +0200, Rafael J. Wysocki wrote:
> >
> > [...]
> >
> >> > > I don't see any dependency there, so I'll queue up the 1-3 in
> >> > > pm-domains and the 4-6 in pm-cpuidle.
> >> >
> >> > I do not see why we should merge patches 4-6 for v4.20; they add legacy
> >> > (DT bindings and related parsing code) with no user in the kernel; we
> >> > may still want to tweak them, in particular PSCI DT bindings.
> >>
> >> My impression was that 4-6 have been agreed on due to the ACKs they
> >> carry.  I'll drop them if that's not the case.
> >
> > I have not expressed myself correctly: they have been agreed (even
> > though as I said they may require some tweaking) but I see no urgency
> > of merging them in v4.20 since they have no user. They contain DT
> > bindings, that create ABI/legacy, I think it is better to have code
> > that uses them in the kernel before merging them and creating a
> > dependency that is not needed.
> 
> There is already code using the new bindings, for the idle states.
> Please have look at patch 5, 6 and 11.

I had a look before replying and I reiterate the point, there is
no reason to merge those patches without the rest of the series,
none. There is already a way to describe idle states in the kernel
and it works very well, we will add one when we need it not before.

> Moreover, you have had plenty on time to look at the series, as those
> patches haven't changed since a very long time.

So ?

> May I suggest you do the review instead, so we can move things
> forward, please. The changes in the v9 series should be trivial to
> review.

There is no reason to merge patches [4, 5, 6, 10] stand-alone, they
are not solving any problem and they do not provide any benefit
other than adding useless ABI/legacy, they make sense when we look
at the whole series.

> >> > Likewise, it makes no sense to merge patches 7-8 without the rest of
> >> > the PSCI patches.
> 
> Well, those patches are part of this series, because Mark wanted me to
> move the files. Is really such a big deal? I think it makes sense, no
> matter what happens afterwards.

We can merge patches [7-8] even if there is no urgency at all to do so,
usually PSCI patches go via arm-soc whose patches queue is now closed
and I do not think that's a problem at all.

Lorenzo

Re: [PATCH v9 00/11] PM / Domains: Support hierarchical CPU arrangement (PSCI/ARM) (a subset)

[Ulf Hansson]

On 5 October 2018 at 12:47, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Thu, Oct 04, 2018 at 08:36:24PM +0200, Ulf Hansson wrote:
>> On 4 October 2018 at 19:21, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
>> > On Thu, Oct 04, 2018 at 07:07:27PM +0200, Rafael J. Wysocki wrote:
>> >
>> > [...]
>> >
>> >> > > I don't see any dependency there, so I'll queue up the 1-3 in
>> >> > > pm-domains and the 4-6 in pm-cpuidle.
>> >> >
>> >> > I do not see why we should merge patches 4-6 for v4.20; they add legacy
>> >> > (DT bindings and related parsing code) with no user in the kernel; we
>> >> > may still want to tweak them, in particular PSCI DT bindings.
>> >>
>> >> My impression was that 4-6 have been agreed on due to the ACKs they
>> >> carry.  I'll drop them if that's not the case.
>> >
>> > I have not expressed myself correctly: they have been agreed (even
>> > though as I said they may require some tweaking) but I see no urgency
>> > of merging them in v4.20 since they have no user. They contain DT
>> > bindings, that create ABI/legacy, I think it is better to have code
>> > that uses them in the kernel before merging them and creating a
>> > dependency that is not needed.
>>
>> There is already code using the new bindings, for the idle states.
>> Please have look at patch 5, 6 and 11.
>
> I had a look before replying and I reiterate the point, there is
> no reason to merge those patches without the rest of the series,
> none. There is already a way to describe idle states in the kernel
> and it works very well, we will add one when we need it not before.

Okay, let's defer them.

>
>> Moreover, you have had plenty on time to look at the series, as those
>> patches haven't changed since a very long time.
>
> So ?
>
>> May I suggest you do the review instead, so we can move things
>> forward, please. The changes in the v9 series should be trivial to
>> review.
>
> There is no reason to merge patches [4, 5, 6, 10] stand-alone, they
> are not solving any problem and they do not provide any benefit
> other than adding useless ABI/legacy, they make sense when we look
> at the whole series.

Okay, let's defer them.

>
>> >> > Likewise, it makes no sense to merge patches 7-8 without the rest of
>> >> > the PSCI patches.
>>
>> Well, those patches are part of this series, because Mark wanted me to
>> move the files. Is really such a big deal? I think it makes sense, no
>> matter what happens afterwards.
>
> We can merge patches [7-8] even if there is no urgency at all to do so,
> usually PSCI patches go via arm-soc whose patches queue is now closed
> and I do not think that's a problem at all.

Okay, let's defer them.

That said, can please review the patches?

Kind regards
Uffe

Re: [PATCH v9 04/11] dt: psci: Update DT bindings to support hierarchical PSCI states

[Sudeep Holla]

On Wed, Oct 03, 2018 at 04:38:17PM +0200, Ulf Hansson wrote:
> From: Lina Iyer <lina.iyer@linaro.org>
> 
> Update DT bindings to represent hierarchical CPU and CPU PM domain idle
> states for PSCI. Also update the PSCI examples to clearly show how
> flattened and hierarchical idle states can be represented in DT.
> 
> Cc: Lina Iyer <ilina@codeaurora.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> Co-developed-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  .../devicetree/bindings/arm/psci.txt          | 156 ++++++++++++++++++
>  1 file changed, 156 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
> index a2c4f1d52492..17aa3d3a1c8e 100644
> --- a/Documentation/devicetree/bindings/arm/psci.txt
> +++ b/Documentation/devicetree/bindings/arm/psci.txt
> @@ -105,7 +105,163 @@ Case 3: PSCI v0.2 and PSCI v0.1.
>  		...
>  	};
>  
> +ARM systems can have multiple cores sometimes in 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 in DT hierarchically.
> +
> +For these cases, the definitions of the idle states for the CPUs and the CPU
> +topology, must conform to the domain idle state specification [3]. The domain
> +idle states themselves, must be compatible with the defined 'domain-idle-state'
> +binding [1], and also need to specify the arm,psci-suspend-param property for
> +each idle state.
> +
> +DT allows representing CPUs and CPU idle states in two different ways -
> +
> +The flattened model as given in Example 1, lists CPU's idle states followed by
> +the domain idle state that the CPUs may choose. Note that the idle states are
> +all compatible with "arm,idle-state".
> +
> +Example 2 represents the hierarchical model of CPUs and domain idle states.
> +CPUs define their domain provider in their psci DT node. The domain controls
> +the power to the CPU and possibly other h/w blocks that would enter an idle
> +state along with the CPU. The CPU's idle states may therefore be considered as
> +the domain's idle states and have the compatible "arm,idle-state". Such domains
> +may also be embedded within another domain that may represent common h/w blocks
> +between these CPUs. The idle states of the CPU topology shall be represented as
> +the domain's idle states.
> +
> +In PSCI firmware v1.0, the OS-Initiated mode is introduced. In order to use it,
> +the hierarchical representation must be used.
> +
> +Example 1: Flattened representation of CPU and domain idle states

[...]

> +Example 2: Hierarchical representation of CPU and domain idle states

I understand that this may not be of interest for this series, but do
we need to add any suggestions on how to arrive Flattened representation
of CPU idle states from its hierarchical representation. If the DT has
latter and PSCI call returns as PC mode only for idle. We need to deal
with that case.

--
Regards,
Sudeep

Re: [PATCH v9 05/11] of: base: Add of_get_cpu_state_node() to get idle states for a CPU node

[Sudeep Holla]

On Wed, Oct 03, 2018 at 04:38:18PM +0200, Ulf Hansson wrote:
> The CPU's idle state nodes are currently parsed at the common cpuidle DT
> library, but also when initializing back-end data for the arch specific CPU
> operations, as in the PSCI driver 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 ARM, there are two generic methods, to describe the CPU's
> idle states, either via the flattened description through the
> "cpu-idle-states" binding [1] or via the hierarchical layout, using the
> "power-domains" and the "domain-idle-states" bindings [2]. Hence, let's
> take both options into account.
> 
> [1]
> Documentation/devicetree/bindings/arm/idle-states.txt
> [2]
> Documentation/devicetree/bindings/arm/psci.txt
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: Lina Iyer <ilina@codeaurora.org>
> Suggested-by: Sudeep Holla <sudeep.holla@arm.com>
> Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>  drivers/of/base.c  | 35 +++++++++++++++++++++++++++++++++++
>  include/linux/of.h |  8 ++++++++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 74eaedd5b860..bf1d5fa34899 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -424,6 +424,41 @@ 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 and idle state node is found at index, the refcount 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,

No strong opinion but I am wondering if it makes sense to get cpu
logical index and fetch the cpu of_node here to contain the refcount on
it within this function while we are trying to consolidate. I do see
that may not be so useful in psci.c but keeping refcount for cpu of_node
here keeps the user free from that.

I am fine as it is if there are reasons not to do that.

--
Regards,
Sudeep

Re: [PATCH v9 06/11] cpuidle: dt: Support hierarchical CPU idle states

[Sudeep Holla]

On Wed, Oct 03, 2018 at 04:38:19PM +0200, Ulf Hansson wrote:
> From: Lina Iyer <lina.iyer@linaro.org>
> 
> Currently CPU's idle states are represented in a flattened model, via the
> "cpu-idle-states" binding from within the CPU's device nodes.
> 
> Support the hierarchical layout during parsing and validating of the CPU's
> idle states. This is simply done by calling the new OF helper,
> of_get_cpu_state_node().
> 
> Cc: Lina Iyer <ilina@codeaurora.org>
> Suggested-by: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> Co-developed-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  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 53342b7f1010..13f9b7cd32d1 100644
> --- a/drivers/cpuidle/dt_idle_states.c
> +++ b/drivers/cpuidle/dt_idle_states.c
> @@ -118,8 +118,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);

We can get rid of above and the of_node_put below if we move this into
of_get_cpu_state_node as suggested in earlier patch.

Apart from these, I don't see any issues with the subset unless there
are users for these. I will dig the v8 and comment.

-- 
Regards,
Sudeep

Re: [PATCH v9 04/11] dt: psci: Update DT bindings to support hierarchical PSCI states

[Ulf Hansson]

+Raju

On 10 October 2018 at 17:03, Sudeep Holla <sudeep.holla@arm.com> wrote:
> On Wed, Oct 03, 2018 at 04:38:17PM +0200, Ulf Hansson wrote:
>> From: Lina Iyer <lina.iyer@linaro.org>
>>
>> Update DT bindings to represent hierarchical CPU and CPU PM domain idle
>> states for PSCI. Also update the PSCI examples to clearly show how
>> flattened and hierarchical idle states can be represented in DT.
>>
>> Cc: Lina Iyer <ilina@codeaurora.org>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> Co-developed-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>  .../devicetree/bindings/arm/psci.txt          | 156 ++++++++++++++++++
>>  1 file changed, 156 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
>> index a2c4f1d52492..17aa3d3a1c8e 100644
>> --- a/Documentation/devicetree/bindings/arm/psci.txt
>> +++ b/Documentation/devicetree/bindings/arm/psci.txt
>> @@ -105,7 +105,163 @@ Case 3: PSCI v0.2 and PSCI v0.1.
>>               ...
>>       };
>>
>> +ARM systems can have multiple cores sometimes in 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 in DT hierarchically.
>> +
>> +For these cases, the definitions of the idle states for the CPUs and the CPU
>> +topology, must conform to the domain idle state specification [3]. The domain
>> +idle states themselves, must be compatible with the defined 'domain-idle-state'
>> +binding [1], and also need to specify the arm,psci-suspend-param property for
>> +each idle state.
>> +
>> +DT allows representing CPUs and CPU idle states in two different ways -
>> +
>> +The flattened model as given in Example 1, lists CPU's idle states followed by
>> +the domain idle state that the CPUs may choose. Note that the idle states are
>> +all compatible with "arm,idle-state".
>> +
>> +Example 2 represents the hierarchical model of CPUs and domain idle states.
>> +CPUs define their domain provider in their psci DT node. The domain controls
>> +the power to the CPU and possibly other h/w blocks that would enter an idle
>> +state along with the CPU. The CPU's idle states may therefore be considered as
>> +the domain's idle states and have the compatible "arm,idle-state". Such domains
>> +may also be embedded within another domain that may represent common h/w blocks
>> +between these CPUs. The idle states of the CPU topology shall be represented as
>> +the domain's idle states.
>> +
>> +In PSCI firmware v1.0, the OS-Initiated mode is introduced. In order to use it,
>> +the hierarchical representation must be used.
>> +
>> +Example 1: Flattened representation of CPU and domain idle states
>
> [...]
>
>> +Example 2: Hierarchical representation of CPU and domain idle states
>
> I understand that this may not be of interest for this series, but do
> we need to add any suggestions on how to arrive Flattened representation
> of CPU idle states from its hierarchical representation. If the DT has
> latter and PSCI call returns as PC mode only for idle. We need to deal
> with that case.

For sure, I think this is valid comment for this series (or at least
v8 which contains the hole set).

The approach I have taken, so far, is to closely tie the support for
PSCI OSI mode to the hierarchical representation in DT of the idle
states. Simply because of changing as little as possible, in a first
step, then build on top.

However, in the offlist discussion I had with Lorenzo, he raised a
concern, very similar to what you are bringing up here. There are
indeed platform configurations, using PSCI PC mode, which would
benefit from the using the hierarchical representation. BTW, that is
kind of what Raju just tried to get working for QCOM SDM845 [1], but
let's discuss that separately.

The conclusion I made, is that no matter of we are using the PC mode
or the OSI mode, the hierarchical representation shall just work.

To me, this means that I have to re-work the series, such that the
PSCI driver (and cpuidle), dynamically in runtime, can agree on which
of the idle states that are "shared among a group of CPUs" and which
are CPU specific. Exactly how, I am still exploring a few different
approaches on.

Does it make sense?

So, when it comes to the updated DT bindings in regards to $subject
patch, I think it's good as is. Or do you think there is something
that needs to be clarified?

Thanks for reviewing!

Kind regards
Uffe

[1]
https://lkml.org/lkml/2018/10/11/28

Re: [PATCH v9 05/11] of: base: Add of_get_cpu_state_node() to get idle states for a CPU node

[Ulf Hansson]

On 10 October 2018 at 17:03, Sudeep Holla <sudeep.holla@arm.com> wrote:
> On Wed, Oct 03, 2018 at 04:38:18PM +0200, Ulf Hansson wrote:
>> The CPU's idle state nodes are currently parsed at the common cpuidle DT
>> library, but also when initializing back-end data for the arch specific CPU
>> operations, as in the PSCI driver 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 ARM, there are two generic methods, to describe the CPU's
>> idle states, either via the flattened description through the
>> "cpu-idle-states" binding [1] or via the hierarchical layout, using the
>> "power-domains" and the "domain-idle-states" bindings [2]. Hence, let's
>> take both options into account.
>>
>> [1]
>> Documentation/devicetree/bindings/arm/idle-states.txt
>> [2]
>> Documentation/devicetree/bindings/arm/psci.txt
>>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: devicetree@vger.kernel.org
>> Cc: Lina Iyer <ilina@codeaurora.org>
>> Suggested-by: Sudeep Holla <sudeep.holla@arm.com>
>> Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> ---
>>  drivers/of/base.c  | 35 +++++++++++++++++++++++++++++++++++
>>  include/linux/of.h |  8 ++++++++
>>  2 files changed, 43 insertions(+)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index 74eaedd5b860..bf1d5fa34899 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -424,6 +424,41 @@ 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 and idle state node is found at index, the refcount 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,
>
> No strong opinion but I am wondering if it makes sense to get cpu
> logical index and fetch the cpu of_node here to contain the refcount on
> it within this function while we are trying to consolidate. I do see
> that may not be so useful in psci.c but keeping refcount for cpu of_node
> here keeps the user free from that.

I see your point.

However, I am hesitating doing that change, as it would be a waste for
the psci case, but I also think that it may becomes a bit less
straight forward to use this helper function.

Although, no strong opinions from my side either, but since Rob is
happy with this, there is no need to change it, I think.

>
> I am fine as it is if there are reasons not to do that.
>

Right.

Kind regards
Uffe

Re: [PATCH v9 05/11] of: base: Add of_get_cpu_state_node() to get idle states for a CPU node

[Sudeep Holla]

On Thu, Oct 11, 2018 at 05:05:07PM +0200, Ulf Hansson wrote:
> On 10 October 2018 at 17:03, Sudeep Holla <sudeep.holla@arm.com> wrote:
[...]
> >
> > No strong opinion but I am wondering if it makes sense to get cpu
> > logical index and fetch the cpu of_node here to contain the refcount on
> > it within this function while we are trying to consolidate. I do see
> > that may not be so useful in psci.c but keeping refcount for cpu of_node
> > here keeps the user free from that.
> 
> I see your point.
> 
> However, I am hesitating doing that change, as it would be a waste for
> the psci case, but I also think that it may becomes a bit less
> straight forward to use this helper function.
> 
> Although, no strong opinions from my side either, but since Rob is
> happy with this, there is no need to change it, I think.
>

Sure, no problem.

--
Regards,
Sudeep

Re: [PATCH v9 04/11] dt: psci: Update DT bindings to support hierarchical PSCI states

[Sudeep Holla]

On Thu, Oct 11, 2018 at 04:44:07PM +0200, Ulf Hansson wrote:
> +Raju
>
> On 10 October 2018 at 17:03, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > On Wed, Oct 03, 2018 at 04:38:17PM +0200, Ulf Hansson wrote:
> >> From: Lina Iyer <lina.iyer@linaro.org>
> >>
> >> Update DT bindings to represent hierarchical CPU and CPU PM domain idle
> >> states for PSCI. Also update the PSCI examples to clearly show how
> >> flattened and hierarchical idle states can be represented in DT.
> >>
> >> Cc: Lina Iyer <ilina@codeaurora.org>
> >> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> >> Co-developed-by: Ulf Hansson <ulf.hansson@linaro.org>
> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> >> Reviewed-by: Rob Herring <robh@kernel.org>
> >> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> >> ---
> >>  .../devicetree/bindings/arm/psci.txt          | 156 ++++++++++++++++++
> >>  1 file changed, 156 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
> >> index a2c4f1d52492..17aa3d3a1c8e 100644
> >> --- a/Documentation/devicetree/bindings/arm/psci.txt
> >> +++ b/Documentation/devicetree/bindings/arm/psci.txt
> >> @@ -105,7 +105,163 @@ Case 3: PSCI v0.2 and PSCI v0.1.
> >>               ...
> >>       };
> >>
> >> +ARM systems can have multiple cores sometimes in 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 in DT hierarchically.
> >> +
> >> +For these cases, the definitions of the idle states for the CPUs and the CPU
> >> +topology, must conform to the domain idle state specification [3]. The domain
> >> +idle states themselves, must be compatible with the defined 'domain-idle-state'
> >> +binding [1], and also need to specify the arm,psci-suspend-param property for
> >> +each idle state.
> >> +
> >> +DT allows representing CPUs and CPU idle states in two different ways -
> >> +
> >> +The flattened model as given in Example 1, lists CPU's idle states followed by
> >> +the domain idle state that the CPUs may choose. Note that the idle states are
> >> +all compatible with "arm,idle-state".
> >> +
> >> +Example 2 represents the hierarchical model of CPUs and domain idle states.
> >> +CPUs define their domain provider in their psci DT node. The domain controls
> >> +the power to the CPU and possibly other h/w blocks that would enter an idle
> >> +state along with the CPU. The CPU's idle states may therefore be considered as
> >> +the domain's idle states and have the compatible "arm,idle-state". Such domains
> >> +may also be embedded within another domain that may represent common h/w blocks
> >> +between these CPUs. The idle states of the CPU topology shall be represented as
> >> +the domain's idle states.
> >> +
> >> +In PSCI firmware v1.0, the OS-Initiated mode is introduced. In order to use it,
> >> +the hierarchical representation must be used.
> >> +
> >> +Example 1: Flattened representation of CPU and domain idle states
> >
> > [...]
> >
> >> +Example 2: Hierarchical representation of CPU and domain idle states
> >
> > I understand that this may not be of interest for this series, but do
> > we need to add any suggestions on how to arrive Flattened representation
> > of CPU idle states from its hierarchical representation. If the DT has
> > latter and PSCI call returns as PC mode only for idle. We need to deal
> > with that case.
>
> For sure, I think this is valid comment for this series (or at least
> v8 which contains the hole set).
>

Thanks.

> The approach I have taken, so far, is to closely tie the support for
> PSCI OSI mode to the hierarchical representation in DT of the idle
> states. Simply because of changing as little as possible, in a first
> step, then build on top.
>

That's fine. I just want a note in the bindings to state that we can use
the hierarchical representation and generate flattened list for PC.

> However, in the offlist discussion I had with Lorenzo, he raised a
> concern, very similar to what you are bringing up here. There are
> indeed platform configurations, using PSCI PC mode, which would
> benefit from the using the hierarchical representation. BTW, that is
> kind of what Raju just tried to get working for QCOM SDM845 [1], but
> let's discuss that separately.
>

OK, we can discuss details in that thread, but I don't even see the PSCI
domains there.

> The conclusion I made, is that no matter of we are using the PC mode
> or the OSI mode, the hierarchical representation shall just work.
>

Indeed.

> To me, this means that I have to re-work the series, such that the
> PSCI driver (and cpuidle), dynamically in runtime, can agree on which
> of the idle states that are "shared among a group of CPUs" and which
> are CPU specific. Exactly how, I am still exploring a few different
> approaches on.
>
> Does it make sense?
>

Yes

> So, when it comes to the updated DT bindings in regards to $subject
> patch, I think it's good as is. Or do you think there is something
> that needs to be clarified?
>

Yes, nearly there. Just thought good to add a note that the representation
has no affinity towards any PSCI idle state mechanism(PC or OSI). So
that it's never assumed or misunderstood.

--
Regards,
Sudeep

Re: [PATCH v9 04/11] dt: psci: Update DT bindings to support hierarchical PSCI states

[Ulf Hansson]

On 11 October 2018 at 18:41, Sudeep Holla <sudeep.holla@arm.com> wrote:
> On Thu, Oct 11, 2018 at 04:44:07PM +0200, Ulf Hansson wrote:
>> +Raju
>>
>> On 10 October 2018 at 17:03, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> > On Wed, Oct 03, 2018 at 04:38:17PM +0200, Ulf Hansson wrote:
>> >> From: Lina Iyer <lina.iyer@linaro.org>
>> >>
>> >> Update DT bindings to represent hierarchical CPU and CPU PM domain idle
>> >> states for PSCI. Also update the PSCI examples to clearly show how
>> >> flattened and hierarchical idle states can be represented in DT.
>> >>
>> >> Cc: Lina Iyer <ilina@codeaurora.org>
>> >> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> >> Co-developed-by: Ulf Hansson <ulf.hansson@linaro.org>
>> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> >> Reviewed-by: Rob Herring <robh@kernel.org>
>> >> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
>> >> ---
>> >>  .../devicetree/bindings/arm/psci.txt          | 156 ++++++++++++++++++
>> >>  1 file changed, 156 insertions(+)
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
>> >> index a2c4f1d52492..17aa3d3a1c8e 100644
>> >> --- a/Documentation/devicetree/bindings/arm/psci.txt
>> >> +++ b/Documentation/devicetree/bindings/arm/psci.txt
>> >> @@ -105,7 +105,163 @@ Case 3: PSCI v0.2 and PSCI v0.1.
>> >>               ...
>> >>       };
>> >>
>> >> +ARM systems can have multiple cores sometimes in 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 in DT hierarchically.
>> >> +
>> >> +For these cases, the definitions of the idle states for the CPUs and the CPU
>> >> +topology, must conform to the domain idle state specification [3]. The domain
>> >> +idle states themselves, must be compatible with the defined 'domain-idle-state'
>> >> +binding [1], and also need to specify the arm,psci-suspend-param property for
>> >> +each idle state.
>> >> +
>> >> +DT allows representing CPUs and CPU idle states in two different ways -
>> >> +
>> >> +The flattened model as given in Example 1, lists CPU's idle states followed by
>> >> +the domain idle state that the CPUs may choose. Note that the idle states are
>> >> +all compatible with "arm,idle-state".
>> >> +
>> >> +Example 2 represents the hierarchical model of CPUs and domain idle states.
>> >> +CPUs define their domain provider in their psci DT node. The domain controls
>> >> +the power to the CPU and possibly other h/w blocks that would enter an idle
>> >> +state along with the CPU. The CPU's idle states may therefore be considered as
>> >> +the domain's idle states and have the compatible "arm,idle-state". Such domains
>> >> +may also be embedded within another domain that may represent common h/w blocks
>> >> +between these CPUs. The idle states of the CPU topology shall be represented as
>> >> +the domain's idle states.
>> >> +
>> >> +In PSCI firmware v1.0, the OS-Initiated mode is introduced. In order to use it,
>> >> +the hierarchical representation must be used.
>> >> +
>> >> +Example 1: Flattened representation of CPU and domain idle states
>> >
>> > [...]
>> >
>> >> +Example 2: Hierarchical representation of CPU and domain idle states
>> >
>> > I understand that this may not be of interest for this series, but do
>> > we need to add any suggestions on how to arrive Flattened representation
>> > of CPU idle states from its hierarchical representation. If the DT has
>> > latter and PSCI call returns as PC mode only for idle. We need to deal
>> > with that case.
>>
>> For sure, I think this is valid comment for this series (or at least
>> v8 which contains the hole set).
>>
>
> Thanks.
>
>> The approach I have taken, so far, is to closely tie the support for
>> PSCI OSI mode to the hierarchical representation in DT of the idle
>> states. Simply because of changing as little as possible, in a first
>> step, then build on top.
>>
>
> That's fine. I just want a note in the bindings to state that we can use
> the hierarchical representation and generate flattened list for PC.

OK, let's discuss it below.

>
>> However, in the offlist discussion I had with Lorenzo, he raised a
>> concern, very similar to what you are bringing up here. There are
>> indeed platform configurations, using PSCI PC mode, which would
>> benefit from the using the hierarchical representation. BTW, that is
>> kind of what Raju just tried to get working for QCOM SDM845 [1], but
>> let's discuss that separately.
>>
>
> OK, we can discuss details in that thread, but I don't even see the PSCI
> domains there.
>
>> The conclusion I made, is that no matter of we are using the PC mode
>> or the OSI mode, the hierarchical representation shall just work.
>>
>
> Indeed.
>
>> To me, this means that I have to re-work the series, such that the
>> PSCI driver (and cpuidle), dynamically in runtime, can agree on which
>> of the idle states that are "shared among a group of CPUs" and which
>> are CPU specific. Exactly how, I am still exploring a few different
>> approaches on.
>>
>> Does it make sense?
>>
>
> Yes

Great!

>
>> So, when it comes to the updated DT bindings in regards to $subject
>> patch, I think it's good as is. Or do you think there is something
>> that needs to be clarified?
>>
>
> Yes, nearly there. Just thought good to add a note that the representation
> has no affinity towards any PSCI idle state mechanism(PC or OSI). So
> that it's never assumed or misunderstood.

I understand your point. However, I think the following sentence still
makes sense (exist in the suggest change above).

"In PSCI firmware v1.0, the OS-Initiated mode is introduced. In order
to use it, the hierarchical representation must be used."

How about if I add: "For the default platform-coordinated mode, both
representations are viable options."

Kind regards
Uffe

Re: [PATCH v9 04/11] dt: psci: Update DT bindings to support hierarchical PSCI states

[Sudeep Holla]

On Fri, Oct 12, 2018 at 11:43:11AM +0200, Ulf Hansson wrote:
> On 11 October 2018 at 18:41, Sudeep Holla <sudeep.holla@arm.com> wrote:
[...]

> > Yes, nearly there. Just thought good to add a note that the representation
> > has no affinity towards any PSCI idle state mechanism(PC or OSI). So
> > that it's never assumed or misunderstood.
>
> I understand your point. However, I think the following sentence still
> makes sense (exist in the suggest change above).
>
> "In PSCI firmware v1.0, the OS-Initiated mode is introduced. In order
> to use it, the hierarchical representation must be used."
>
> How about if I add: "For the default platform-coordinated mode, both
> representations are viable options."
>
I would also add couple of things, how about this order:

In PSCI firmware v1.0, the OS-Initiated mode is introduced. However the
flattened vs hierarchical DT representation of power domains is orthogonal
to OS-Initiated vs platform-coordinated PSCI CPU suspend modes and
should be considered independent of each other.

The hierarchical representation helps and makes it easy to implement
OSI mode and OS implementations may choose to mandate it.

For the default platform-coordinated mode, both representations are
viable options.

--
Regards,
Sudeep

Re: [PATCH v9 04/11] dt: psci: Update DT bindings to support hierarchical PSCI states

[Ulf Hansson]

On 12 October 2018 at 12:13, Sudeep Holla <sudeep.holla@arm.com> wrote:
> On Fri, Oct 12, 2018 at 11:43:11AM +0200, Ulf Hansson wrote:
>> On 11 October 2018 at 18:41, Sudeep Holla <sudeep.holla@arm.com> wrote:
> [...]
>
>> > Yes, nearly there. Just thought good to add a note that the representation
>> > has no affinity towards any PSCI idle state mechanism(PC or OSI). So
>> > that it's never assumed or misunderstood.
>>
>> I understand your point. However, I think the following sentence still
>> makes sense (exist in the suggest change above).
>>
>> "In PSCI firmware v1.0, the OS-Initiated mode is introduced. In order
>> to use it, the hierarchical representation must be used."
>>
>> How about if I add: "For the default platform-coordinated mode, both
>> representations are viable options."
>>
> I would also add couple of things, how about this order:
>
> In PSCI firmware v1.0, the OS-Initiated mode is introduced. However the
> flattened vs hierarchical DT representation of power domains is orthogonal
> to OS-Initiated vs platform-coordinated PSCI CPU suspend modes and
> should be considered independent of each other.
>
> The hierarchical representation helps and makes it easy to implement
> OSI mode and OS implementations may choose to mandate it.
>
> For the default platform-coordinated mode, both representations are
> viable options.

This looks great! I am adding it to the next version, thanks!

Kind regards
Uffe