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

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

[Ulf Hansson]

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

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

Do note, most of the code in this series have been posted earlier, but from the
latest version being reviewed, we agreed on that it was better to re-work the
PSCI backend driver as a first step, simply to get a clean interface towards the
cpuidle driver.

Summary of the main-changes since the last submission [1]:

 - Moved implementation from the psci FW dricer into cpuidle-psci.

 - Re-requesting review of the DT bindings, as we have moved to yaml. No
   changes as such, but tried to clarify a few things in the text.

 - Drop the patch enabling support for CPU hotplug, postponing that to the next
   step.

 - Respect the hierarchical topology in DT only when OSI mode is supported.
   This is to start simple and we can always extend the support on top.

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

Kind regards
Ulf Hansson

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


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

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

 .../devicetree/bindings/arm/psci.yaml         | 153 +++++++++
 arch/arm64/boot/dts/qcom/msm8916.dtsi         |  57 +++-
 drivers/cpuidle/Makefile                      |   4 +-
 drivers/cpuidle/cpuidle-psci-domain.c         | 302 ++++++++++++++++++
 drivers/cpuidle/cpuidle-psci.c                | 106 ++++--
 drivers/cpuidle/cpuidle-psci.h                |  17 +
 drivers/cpuidle/dt_idle_states.c              |   5 +-
 drivers/firmware/psci/psci.c                  |  18 +-
 drivers/of/base.c                             |  36 +++
 include/linux/of.h                            |   8 +
 include/linux/psci.h                          |   2 +
 11 files changed, 673 insertions(+), 35 deletions(-)
 create mode 100644 drivers/cpuidle/cpuidle-psci-domain.c
 create mode 100644 drivers/cpuidle/cpuidle-psci.h

-- 
2.17.1

[PATCH 01/13] cpuidle: psci: Fix potential access to unmapped memory

[Ulf Hansson]

When the WFI state have been selected, the in-parameter idx to
psci_enter_idle_state() is zero. In this case, we must not index the state
array as "state[idx - 1]", as it means accessing data outside the array.
Fix the bug by pre-checking if idx is zero.

Fixes: 9ffeb6d08c3a ("PSCI: cpuidle: Refactor CPU suspend power_state parameter handling")
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/cpuidle/cpuidle-psci.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index f3c1a2396f98..2e91c8d6c211 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -27,10 +27,10 @@ static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
 static int psci_enter_idle_state(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv, int idx)
 {
-	u32 *state = __this_cpu_read(psci_power_state);
+	u32 *states = __this_cpu_read(psci_power_state);
+	u32 state = idx ? states[idx - 1] : 0;
 
-	return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter,
-					   idx, state[idx - 1]);
+	return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
 }
 
 static struct cpuidle_driver psci_idle_driver __initdata = {
-- 
2.17.1

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

[Ulf Hansson]

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

Let's also take the opportunity to update the examples to clarify the
difference between the currently supported flattened layout vs the new
hierarchical layout.

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

Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 .../devicetree/bindings/arm/psci.yaml         | 153 ++++++++++++++++++
 1 file changed, 153 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
index 7abdf58b335e..360579bfa591 100644
--- a/Documentation/devicetree/bindings/arm/psci.yaml
+++ b/Documentation/devicetree/bindings/arm/psci.yaml
@@ -160,4 +160,157 @@ examples:
       cpu_on = <0x95c10002>;
       cpu_off = <0x95c10001>;
     };
+
+  - |+
+
+    // Case 4: PSCI v1.0, PSCI v0.2, PSCI v0.1.
+
+    /*
+     * ARM systems can have multiple cores, sometimes in an hierarchical
+     * arrangement. This often, but not always, maps directly to the processor
+     * power topology of the system. Individual nodes in a topology have their
+     * own specific power states and can be better represented hierarchically.
+     *
+     * For these cases, the definitions of the idle states for the CPUs and the
+     * CPU topology, must conform to the power domain description [3]. The idle
+     * states themselves must conform to the domain idle state description [4]
+     * and must specify the arm,psci-suspend-param property.
+     *
+     * This allows two options to represent CPUs and CPU idle states. By using
+     * a flattened model as given in the first example below and by using a
+     * hierarchical model as given in the second example.
+     *
+     * It should also be noted that, in PSCI firmware v1.0 the OS-Initiated
+     * (OSI) CPU suspend mode is introduced. Using the hierarchical
+     * representation helps to implement support for OSI mode and OS
+     * implementations may choose to mandate it.
+     *
+     * [3] Kernel documentation - Power domain description
+     *   Documentation/devicetree/bindings/power/power_domain.txt
+     * [4] Kernel documentation - Domain idle state description
+     *   Documentation/devicetree/bindings/power/domain-idle-state.txt
+     */
+
+     // The flattened model
+     cpus {
+
+       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 = <0x0000001>;
+           entry-latency-us = <10>;
+           exit-latency-us = <10>;
+           min-residency-us = <100>;
+         };
+
+         CLUSTER_RET: cluster-retention {
+           compatible = "arm,idle-state";
+           arm,psci-suspend-param = <0x1000011>;
+           entry-latency-us = <500>;
+           exit-latency-us = <500>;
+           min-residency-us = <2000>;
+         };
+
+         CLUSTER_PWRDN: cluster-power-down {
+           compatible = "arm,idle-state";
+           arm,psci-suspend-param = <0x1000031>;
+           entry-latency-us = <2000>;
+           exit-latency-us = <2000>;
+           min-residency-us = <6000>;
+         };
+       };
+     };
+
+     psci {
+       compatible = "arm,psci-1.0";
+       method = "smc";
+     };
+
+     // The hierarchical model
+     cpus {
+
+       CPU0: cpu@0 {
+         device_type = "cpu";
+         compatible = "arm,cortex-a53", "arm,armv8";
+         reg = <0x0>;
+         enable-method = "psci";
+         power-domains = <&CPU_PD0>;
+         power-domain-names = "psci";
+       };
+
+       CPU1: cpu@1 {
+         device_type = "cpu";
+         compatible = "arm,cortex-a57", "arm,armv8";
+         reg = <0x100>;
+         enable-method = "psci";
+         power-domains = <&CPU_PD1>;
+         power-domain-names = "psci";
+       };
+
+       idle-states {
+
+         CPU_PWRDN: cpu-power-down {
+           compatible = "arm,idle-state";
+           arm,psci-suspend-param = <0x0000001>;
+           entry-latency-us = <10>;
+           exit-latency-us = <10>;
+           min-residency-us = <100>;
+         };
+
+         CLUSTER_RET: cluster-retention {
+           compatible = "domain-idle-state";
+           arm,psci-suspend-param = <0x1000011>;
+           entry-latency-us = <500>;
+           exit-latency-us = <500>;
+           min-residency-us = <2000>;
+         };
+
+         CLUSTER_PWRDN: cluster-power-down {
+           compatible = "domain-idle-state";
+           arm,psci-suspend-param = <0x1000031>;
+           entry-latency-us = <2000>;
+           exit-latency-us = <2000>;
+           min-residency-us = <6000>;
+         };
+       };
+     };
+
+     psci {
+       compatible = "arm,psci-1.0";
+       method = "smc";
+
+       CPU_PD0: cpu-pd0 {
+         #power-domain-cells = <0>;
+         domain-idle-states = <&CPU_PWRDN>;
+         power-domains = <&CLUSTER_PD>;
+       };
+
+       CPU_PD1: cpu-pd1 {
+         #power-domain-cells = <0>;
+         domain-idle-states =  <&CPU_PWRDN>;
+         power-domains = <&CLUSTER_PD>;
+       };
+
+       CLUSTER_PD: cluster-pd {
+         #power-domain-cells = <0>;
+         domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWRDN>;
+       };
+     };
 ...
-- 
2.17.1

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

[Ulf Hansson]

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

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

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/firmware/psci/psci.c | 18 ++++++++++++++++--
 include/linux/psci.h         |  2 ++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index 84f4ff351c62..76f3a991d4d7 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -89,7 +89,7 @@ static inline bool psci_has_ext_power_state(void)
 				PSCI_1_0_FEATURES_CPU_SUSPEND_PF_MASK;
 }
 
-static inline bool psci_has_osi_support(void)
+bool psci_has_osi_support(void)
 {
 	return psci_cpu_suspend_feature & PSCI_1_0_OS_INITIATED;
 }
@@ -154,6 +154,15 @@ static u32 psci_get_version(void)
 	return invoke_psci_fn(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
 }
 
+int psci_set_osi_mode(void)
+{
+	int err;
+
+	err = invoke_psci_fn(PSCI_1_0_FN_SET_SUSPEND_MODE,
+			     PSCI_1_0_SUSPEND_MODE_OSI, 0, 0);
+	return psci_to_linux_errno(err);
+}
+
 static int psci_cpu_suspend(u32 state, unsigned long entry_point)
 {
 	int err;
@@ -536,9 +545,14 @@ static int __init psci_1_0_init(struct device_node *np)
 	if (err)
 		return err;
 
-	if (psci_has_osi_support())
+	if (psci_has_osi_support()) {
 		pr_info("OSI mode supported.\n");
 
+		/* Default to PC mode. */
+		invoke_psci_fn(PSCI_1_0_FN_SET_SUSPEND_MODE,
+			       PSCI_1_0_SUSPEND_MODE_PC, 0, 0);
+	}
+
 	return 0;
 }
 
diff --git a/include/linux/psci.h b/include/linux/psci.h
index e2bacc6fd2f2..f76b45341adf 100644
--- a/include/linux/psci.h
+++ b/include/linux/psci.h
@@ -17,6 +17,8 @@ bool psci_tos_resident_on(int cpu);
 
 int psci_cpu_suspend_enter(u32 state);
 bool psci_power_state_is_valid(u32 state);
+int psci_set_osi_mode(void);
+bool psci_has_osi_support(void);
 
 enum psci_conduit {
 	PSCI_CONDUIT_NONE,
-- 
2.17.1

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

[Ulf Hansson]

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

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

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

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

Suggested-by: Sudeep Holla <sudeep.holla@arm.com>
Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/of/base.c  | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/of.h |  8 ++++++++
 2 files changed, 44 insertions(+)

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

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

[Ulf Hansson]

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

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

Suggested-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Co-developed-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/cpuidle/dt_idle_states.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

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

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

[Ulf Hansson]

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

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/cpuidle/cpuidle-psci.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index 2e91c8d6c211..1195a1056139 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -73,28 +73,22 @@ static int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
 	return 0;
 }
 
-static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
+static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
+				unsigned int state_nodes, int cpu)
 {
-	int i, ret = 0, count = 0;
+	int i, ret = 0;
 	u32 *psci_states;
 	struct device_node *state_node;
 
-	/* Count idle states */
-	while ((state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
-					      count))) {
-		count++;
-		of_node_put(state_node);
-	}
-
-	if (!count)
-		return -ENODEV;
-
-	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
+	psci_states = kcalloc(state_nodes, sizeof(*psci_states), GFP_KERNEL);
 	if (!psci_states)
 		return -ENOMEM;
 
-	for (i = 0; i < count; i++) {
+	for (i = 0; i < state_nodes; i++) {
 		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
+		if (!state_node)
+			break;
+
 		ret = psci_dt_parse_state_node(state_node, &psci_states[i]);
 		of_node_put(state_node);
 
@@ -104,6 +98,11 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
 		pr_debug("psci-power-state %#x index %d\n", psci_states[i], i);
 	}
 
+	if (i != state_nodes) {
+		ret = -ENODEV;
+		goto free_mem;
+	}
+
 	/* Idle states parsed correctly, initialize per-cpu pointer */
 	per_cpu(psci_power_state, cpu) = psci_states;
 	return 0;
@@ -113,7 +112,7 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
 	return ret;
 }
 
-static __init int psci_cpu_init_idle(unsigned int cpu)
+static __init int psci_cpu_init_idle(unsigned int cpu, unsigned int state_nodes)
 {
 	struct device_node *cpu_node;
 	int ret;
@@ -129,7 +128,7 @@ static __init int psci_cpu_init_idle(unsigned int cpu)
 	if (!cpu_node)
 		return -ENODEV;
 
-	ret = psci_dt_cpu_init_idle(cpu_node, cpu);
+	ret = psci_dt_cpu_init_idle(cpu_node, state_nodes, cpu);
 
 	of_node_put(cpu_node);
 
@@ -185,7 +184,7 @@ static int __init psci_idle_init_cpu(int cpu)
 	/*
 	 * Initialize PSCI idle states.
 	 */
-	ret = psci_cpu_init_idle(cpu);
+	ret = psci_cpu_init_idle(cpu, ret);
 	if (ret) {
 		pr_err("CPU %d failed to PSCI idle\n", cpu);
 		goto out_kfree_drv;
-- 
2.17.1

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

[Ulf Hansson]

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

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/cpuidle/cpuidle-psci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index 1195a1056139..5c30f23a8a7b 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -85,7 +85,7 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
 		return -ENOMEM;
 
 	for (i = 0; i < state_nodes; 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 08/13] cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains

[Ulf Hansson]

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

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

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

This allows subsequent patches that implements support for PM domains for
cpuidle-psci, to write the selected idle state parameter for the cluster
into the domain_state variable. Furthermore, let's share the needed
functions in a header file, to enable the support for PM domains to be
implemented in a separate c-file.

Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/cpuidle/cpuidle-psci.c | 31 ++++++++++++++++++++++++++++---
 drivers/cpuidle/cpuidle-psci.h | 11 +++++++++++
 2 files changed, 39 insertions(+), 3 deletions(-)
 create mode 100644 drivers/cpuidle/cpuidle-psci.h

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index 5c30f23a8a7b..a16467daf99d 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -20,17 +20,42 @@
 
 #include <asm/cpuidle.h>
 
+#include "cpuidle-psci.h"
 #include "dt_idle_states.h"
 
 static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
+static DEFINE_PER_CPU(u32, domain_state);
+
+void psci_set_domain_state(u32 state)
+{
+	__this_cpu_write(domain_state, state);
+}
+
+static inline u32 psci_get_domain_state(void)
+{
+	return __this_cpu_read(domain_state);
+}
+
+static int __psci_enter_idle_state(int idx, u32 state)
+{
+	return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
+}
 
 static int psci_enter_idle_state(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv, int idx)
 {
+	int ret;
 	u32 *states = __this_cpu_read(psci_power_state);
-	u32 state = idx ? states[idx - 1] : 0;
+	u32 state = psci_get_domain_state();
 
-	return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
+	if (!state && idx)
+		state = states[idx - 1];
+
+	ret = __psci_enter_idle_state(idx, state);
+
+	/* Clear the domain state to start fresh when back from idle. */
+	psci_set_domain_state(0);
+	return ret;
 }
 
 static struct cpuidle_driver psci_idle_driver __initdata = {
@@ -56,7 +81,7 @@ static const struct of_device_id psci_idle_state_match[] __initconst = {
 	{ },
 };
 
-static int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
+int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
 {
 	int err = of_property_read_u32(np, "arm,psci-suspend-param", state);
 
diff --git a/drivers/cpuidle/cpuidle-psci.h b/drivers/cpuidle/cpuidle-psci.h
new file mode 100644
index 000000000000..e593de1784c3
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-psci.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __CPUIDLE_PSCI_H
+#define __CPUIDLE_PSCI_H
+
+struct device_node;
+
+void psci_set_domain_state(u32 state);
+int __init psci_dt_parse_state_node(struct device_node *np, u32 *state);
+
+#endif /* __CPUIDLE_PSCI_H */
-- 
2.17.1

[PATCH 09/13] cpuidle: psci: Add support for PM domains by using genpd

[Ulf Hansson]

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

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

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

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

Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/cpuidle/Makefile              |   4 +-
 drivers/cpuidle/cpuidle-psci-domain.c | 281 ++++++++++++++++++++++++++
 2 files changed, 284 insertions(+), 1 deletion(-)
 create mode 100644 drivers/cpuidle/cpuidle-psci-domain.c

diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index ee70d5cc5b99..cc8c769d7fa9 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -21,7 +21,9 @@ obj-$(CONFIG_ARM_U8500_CPUIDLE)         += cpuidle-ux500.o
 obj-$(CONFIG_ARM_AT91_CPUIDLE)          += cpuidle-at91.o
 obj-$(CONFIG_ARM_EXYNOS_CPUIDLE)        += cpuidle-exynos.o
 obj-$(CONFIG_ARM_CPUIDLE)		+= cpuidle-arm.o
-obj-$(CONFIG_ARM_PSCI_CPUIDLE)		+= cpuidle-psci.o
+obj-$(CONFIG_ARM_PSCI_CPUIDLE)		+= cpuidle_psci.o
+cpuidle_psci-y				:= cpuidle-psci.o
+cpuidle_psci-$(CONFIG_PM_GENERIC_DOMAINS_OF) += cpuidle-psci-domain.o
 
 ###############################################################################
 # MIPS drivers
diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
new file mode 100644
index 000000000000..3f5143ccc3e0
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-psci-domain.c
@@ -0,0 +1,281 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PM domains for CPUs via genpd - managed by cpuidle-psci.
+ *
+ * Copyright (C) 2019 Linaro Ltd.
+ * Author: Ulf Hansson <ulf.hansson@linaro.org>
+ *
+ */
+
+#define pr_fmt(fmt) "CPUidle PSCI: " fmt
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/pm_domain.h>
+#include <linux/psci.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+
+#include "cpuidle-psci.h"
+
+struct psci_pd_provider {
+	struct list_head link;
+	struct device_node *node;
+};
+
+static LIST_HEAD(psci_pd_providers);
+static bool osi_mode_enabled;
+
+static int psci_pd_power_off(struct generic_pm_domain *pd)
+{
+	struct genpd_power_state *state = &pd->states[pd->state_idx];
+	u32 *pd_state;
+
+	/* If we have failed to enable OSI mode, then abort power off. */
+	if (!osi_mode_enabled)
+		return -EBUSY;
+
+	if (!state->data)
+		return 0;
+
+	/* OSI mode is enabled, set the corresponding domain state. */
+	pd_state = state->data;
+	psci_set_domain_state(*pd_state);
+
+	return 0;
+}
+
+static int __init psci_pd_parse_state_nodes(struct genpd_power_state *states,
+					int state_count)
+{
+	int i, ret;
+	u32 psci_state, *psci_state_buf;
+
+	for (i = 0; i < state_count; i++) {
+		ret = psci_dt_parse_state_node(to_of_node(states[i].fwnode),
+					&psci_state);
+		if (ret)
+			goto free_state;
+
+		psci_state_buf = kmalloc(sizeof(u32), GFP_KERNEL);
+		if (!psci_state_buf) {
+			ret = -ENOMEM;
+			goto free_state;
+		}
+		*psci_state_buf = psci_state;
+		states[i].data = psci_state_buf;
+	}
+
+	return 0;
+
+free_state:
+	i--;
+	for (; i >= 0; i--)
+		kfree(states[i].data);
+	return ret;
+}
+
+static int __init psci_pd_parse_states(struct device_node *np,
+			struct genpd_power_state **states, int *state_count)
+{
+	int ret;
+
+	/* Parse the domain idle states. */
+	ret = of_genpd_parse_idle_states(np, states, state_count);
+	if (ret)
+		return ret;
+
+	/* Fill out the PSCI specifics for each found state. */
+	ret = psci_pd_parse_state_nodes(*states, *state_count);
+	if (ret)
+		kfree(*states);
+
+	return ret;
+}
+
+static void psci_pd_free_states(struct genpd_power_state *states,
+				unsigned int state_count)
+{
+	int i;
+
+	for (i = 0; i < state_count; i++)
+		kfree(states[i].data);
+	kfree(states);
+}
+
+static int __init psci_pd_init(struct device_node *np)
+{
+	struct generic_pm_domain *pd;
+	struct psci_pd_provider *pd_provider;
+	struct dev_power_governor *pd_gov;
+	struct genpd_power_state *states = NULL;
+	int ret = -ENOMEM, state_count = 0;
+
+	pd = kzalloc(sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		goto out;
+
+	pd_provider = kzalloc(sizeof(*pd_provider), GFP_KERNEL);
+	if (!pd_provider)
+		goto free_pd;
+
+	pd->name = kasprintf(GFP_KERNEL, "%pOF", np);
+	if (!pd->name)
+		goto free_pd_prov;
+
+	/*
+	 * Parse the domain idle states and let genpd manage the state selection
+	 * for those being compatible with "domain-idle-state".
+	 */
+	ret = psci_pd_parse_states(np, &states, &state_count);
+	if (ret)
+		goto free_name;
+
+	pd->free_states = psci_pd_free_states;
+	pd->name = kbasename(pd->name);
+	pd->power_off = psci_pd_power_off;
+	pd->states = states;
+	pd->state_count = state_count;
+	pd->flags |= GENPD_FLAG_IRQ_SAFE | GENPD_FLAG_CPU_DOMAIN;
+
+	/* Use governor for CPU PM domains if it has some states to manage. */
+	pd_gov = state_count > 0 ? &pm_domain_cpu_gov : NULL;
+
+	ret = pm_genpd_init(pd, pd_gov, false);
+	if (ret) {
+		psci_pd_free_states(states, state_count);
+		goto free_name;
+	}
+
+	ret = of_genpd_add_provider_simple(np, pd);
+	if (ret)
+		goto remove_pd;
+
+	pd_provider->node = of_node_get(np);
+	list_add(&pd_provider->link, &psci_pd_providers);
+
+	pr_debug("init PM domain %s\n", pd->name);
+	return 0;
+
+remove_pd:
+	pm_genpd_remove(pd);
+free_name:
+	kfree(pd->name);
+free_pd_prov:
+	kfree(pd_provider);
+free_pd:
+	kfree(pd);
+out:
+	pr_err("failed to init PM domain ret=%d %pOF\n", ret, np);
+	return ret;
+}
+
+static void __init psci_pd_remove(void)
+{
+	struct psci_pd_provider *pd_provider, *it;
+	struct generic_pm_domain *genpd;
+
+	list_for_each_entry_safe(pd_provider, it, &psci_pd_providers, link) {
+		of_genpd_del_provider(pd_provider->node);
+
+		genpd = of_genpd_remove_last(pd_provider->node);
+		if (!IS_ERR(genpd))
+			kfree(genpd);
+
+		of_node_put(pd_provider->node);
+		list_del(&pd_provider->link);
+		kfree(pd_provider);
+	}
+}
+
+static int __init psci_pd_init_topology(struct device_node *np)
+{
+	struct device_node *node;
+	struct of_phandle_args child, parent;
+	int ret;
+
+	for_each_child_of_node(np, node) {
+		if (of_parse_phandle_with_args(node, "power-domains",
+					"#power-domain-cells", 0, &parent))
+			continue;
+
+		child.np = node;
+		child.args_count = 0;
+
+		ret = of_genpd_add_subdomain(&parent, &child);
+		of_node_put(parent.np);
+		if (ret) {
+			of_node_put(node);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static const struct of_device_id psci_of_match[] __initconst = {
+	{ .compatible = "arm,psci" },
+	{ .compatible = "arm,psci-0.2" },
+	{ .compatible = "arm,psci-1.0" },
+	{}
+};
+
+static int __init psci_idle_init_domains(void)
+{
+	struct device_node *np = of_find_matching_node(NULL, psci_of_match);
+	struct device_node *node;
+	int ret = 0, pd_count = 0;
+
+	if (!np)
+		return -ENODEV;
+
+	/* Currently limit the hierarchical topology to be used in OSI mode. */
+	if (!psci_has_osi_support())
+		goto out;
+
+	/*
+	 * Parse child nodes for the "#power-domain-cells" property and
+	 * initialize a genpd/genpd-of-provider pair when it's found.
+	 */
+	for_each_child_of_node(np, node) {
+		if (!of_find_property(node, "#power-domain-cells", NULL))
+			continue;
+
+		ret = psci_pd_init(node);
+		if (ret)
+			goto put_node;
+
+		pd_count++;
+	}
+
+	/* Bail out if not using the hierarchical CPU topology. */
+	if (!pd_count)
+		goto out;
+
+	/* Link genpd masters/subdomains to model the CPU topology. */
+	ret = psci_pd_init_topology(np);
+	if (ret)
+		goto remove_pd;
+
+	/* Try to enable OSI mode. */
+	ret = psci_set_osi_mode();
+	if (ret)
+		pr_warn("failed to enable OSI mode: %d\n", ret);
+	else
+		osi_mode_enabled = true;
+
+	of_node_put(np);
+	pr_info("Initialized CPU PM domain topology\n");
+	return pd_count;
+
+put_node:
+	of_node_put(node);
+remove_pd:
+	if (pd_count)
+		psci_pd_remove();
+	pr_err("failed to create CPU PM domains ret=%d\n", ret);
+out:
+	of_node_put(np);
+	return ret;
+}
+subsys_initcall(psci_idle_init_domains);
-- 
2.17.1

[PATCH 10/13] cpuidle: psci: Add a helper to attach a CPU to its PM domain

[Ulf Hansson]

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

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

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/cpuidle/cpuidle-psci-domain.c | 21 +++++++++++++++++++++
 drivers/cpuidle/cpuidle-psci.h        |  6 ++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
index 3f5143ccc3e0..7429fd7626a1 100644
--- a/drivers/cpuidle/cpuidle-psci-domain.c
+++ b/drivers/cpuidle/cpuidle-psci-domain.c
@@ -9,9 +9,11 @@
 
 #define pr_fmt(fmt) "CPUidle PSCI: " fmt
 
+#include <linux/cpu.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
 #include <linux/psci.h>
 #include <linux/slab.h>
 #include <linux/string.h>
@@ -279,3 +281,22 @@ static int __init psci_idle_init_domains(void)
 	return ret;
 }
 subsys_initcall(psci_idle_init_domains);
+
+struct device *psci_dt_attach_cpu(int cpu)
+{
+	struct device *dev;
+
+	/* Currently limit the hierarchical topology to be used in OSI mode. */
+	if (!psci_has_osi_support())
+		return NULL;
+
+	dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
+	if (IS_ERR_OR_NULL(dev))
+		return dev;
+
+	pm_runtime_irq_safe(dev);
+	if (cpu_online(cpu))
+		pm_runtime_get_sync(dev);
+
+	return dev;
+}
diff --git a/drivers/cpuidle/cpuidle-psci.h b/drivers/cpuidle/cpuidle-psci.h
index e593de1784c3..d2e55cad9ac6 100644
--- a/drivers/cpuidle/cpuidle-psci.h
+++ b/drivers/cpuidle/cpuidle-psci.h
@@ -8,4 +8,10 @@ struct device_node;
 void psci_set_domain_state(u32 state);
 int __init psci_dt_parse_state_node(struct device_node *np, u32 *state);
 
+#ifdef CONFIG_PM_GENERIC_DOMAINS_OF
+struct device *psci_dt_attach_cpu(int cpu);
+#else
+static inline struct device *psci_dt_attach_cpu(int cpu) { return NULL; }
+#endif
+
 #endif /* __CPUIDLE_PSCI_H */
-- 
2.17.1

[PATCH 11/13] cpuidle: psci: Attach CPU devices to their PM domains

[Ulf Hansson]

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

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

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/cpuidle/cpuidle-psci.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index a16467daf99d..1510422c7a53 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -23,7 +23,12 @@
 #include "cpuidle-psci.h"
 #include "dt_idle_states.h"
 
-static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
+struct psci_cpuidle_data {
+	u32 *psci_states;
+	struct device *dev;
+};
+
+static DEFINE_PER_CPU_READ_MOSTLY(struct psci_cpuidle_data, psci_cpuidle_data);
 static DEFINE_PER_CPU(u32, domain_state);
 
 void psci_set_domain_state(u32 state)
@@ -45,7 +50,7 @@ static int psci_enter_idle_state(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv, int idx)
 {
 	int ret;
-	u32 *states = __this_cpu_read(psci_power_state);
+	u32 *states = __this_cpu_read(psci_cpuidle_data.psci_states);
 	u32 state = psci_get_domain_state();
 
 	if (!state && idx)
@@ -103,7 +108,9 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
 {
 	int i, ret = 0;
 	u32 *psci_states;
+	struct device *dev;
 	struct device_node *state_node;
+	struct psci_cpuidle_data *data = per_cpu_ptr(&psci_cpuidle_data, cpu);
 
 	psci_states = kcalloc(state_nodes, sizeof(*psci_states), GFP_KERNEL);
 	if (!psci_states)
@@ -128,8 +135,16 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
 		goto free_mem;
 	}
 
-	/* Idle states parsed correctly, initialize per-cpu pointer */
-	per_cpu(psci_power_state, cpu) = psci_states;
+	dev = psci_dt_attach_cpu(cpu);
+	if (IS_ERR(dev)) {
+		ret = PTR_ERR(dev);
+		goto free_mem;
+	}
+
+	data->dev = dev;
+
+	/* Idle states parsed correctly, store them in the per-cpu struct. */
+	data->psci_states = psci_states;
 	return 0;
 
 free_mem:
-- 
2.17.1

[PATCH 12/13] cpuidle: psci: Manage runtime PM in the idle path

[Ulf Hansson]

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

To set the triggering point for when runtime PM reference counting should
be done, let's store the index of deepest idle state for the CPU in the per
CPU struct. Then use this index to compare the selected idle state index
when entering idle, as to understand whether runtime PM reference counting
is needed or not.

Note that, from the hierarchical point view, there may be good reasons to
do runtime PM reference counting even on shallower idle states, but at this
point this isn't supported, mainly due to limitations set by the generic PM
domain.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/cpuidle/cpuidle-psci.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index 1510422c7a53..0919b40c1a85 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -16,6 +16,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/psci.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 
 #include <asm/cpuidle.h>
@@ -25,6 +26,7 @@
 
 struct psci_cpuidle_data {
 	u32 *psci_states;
+	u32 rpm_state_id;
 	struct device *dev;
 };
 
@@ -50,14 +52,28 @@ static int psci_enter_idle_state(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv, int idx)
 {
 	int ret;
-	u32 *states = __this_cpu_read(psci_cpuidle_data.psci_states);
-	u32 state = psci_get_domain_state();
+	struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
+	u32 *states = data->psci_states;
+	struct device *pd_dev = data->dev;
+	bool runtime_pm = (pd_dev && data->rpm_state_id == idx);
+	u32 state;
 
+	/*
+	 * Do runtime PM if we are using the hierarchical CPU toplogy, but only
+	 * when cpuidle have selected the deepest idle state for the CPU.
+	 */
+	if (runtime_pm)
+		pm_runtime_put_sync_suspend(pd_dev);
+
+	state = psci_get_domain_state();
 	if (!state && idx)
 		state = states[idx - 1];
 
 	ret = __psci_enter_idle_state(idx, state);
 
+	if (runtime_pm)
+		pm_runtime_get_sync(pd_dev);
+
 	/* Clear the domain state to start fresh when back from idle. */
 	psci_set_domain_state(0);
 	return ret;
@@ -142,6 +158,7 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
 	}
 
 	data->dev = dev;
+	data->rpm_state_id = state_nodes;
 
 	/* Idle states parsed correctly, store them in the per-cpu struct. */
 	data->psci_states = psci_states;
-- 
2.17.1

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

[Ulf Hansson]

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

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

Cc: Andy Gross <andy.gross@linaro.org>
Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8916.dtsi | 57 +++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 4 deletions(-)

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

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

[Ulf Hansson]

On Thu, 10 Oct 2019 at 13:40, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> This series enables initial support for hierarchical CPU arrangement, managed
> by PSCI and its corresponding cpuidle driver. It's based on using the generic
> PM domain (genpd), which nowadays also supports devices belonging to CPUs.
>
> The last DTS patch enables the hierarchical topology to be used for the Qcom
> 410c Dragonboard, which supports the PSCI OS-initiated mode.
>
> Do note, most of the code in this series have been posted earlier, but from the
> latest version being reviewed, we agreed on that it was better to re-work the
> PSCI backend driver as a first step, simply to get a clean interface towards the
> cpuidle driver.
>
> Summary of the main-changes since the last submission [1]:
>
>  - Moved implementation from the psci FW dricer into cpuidle-psci.
>
>  - Re-requesting review of the DT bindings, as we have moved to yaml. No
>    changes as such, but tried to clarify a few things in the text.
>
>  - Drop the patch enabling support for CPU hotplug, postponing that to the next
>    step.
>
>  - Respect the hierarchical topology in DT only when OSI mode is supported.
>    This is to start simple and we can always extend the support on top.
>
> The series is also available at:
> git.linaro.org/people/ulf.hansson/linux-pm.git next
>
> Kind regards
> Ulf Hansson
>
> [1]
> https://lwn.net/Articles/788306/
>
>
> Lina Iyer (1):
>   cpuidle: dt: Support hierarchical CPU idle states
>
> Ulf Hansson (12):
>   cpuidle: psci: Fix potential access to unmapped memory
>   dt: psci: Update DT bindings to support hierarchical PSCI states
>   firmware: psci: Export functions to manage the OSI mode
>   of: base: Add of_get_cpu_state_node() to get idle states for a CPU
>     node
>   cpuidle: psci: Simplify OF parsing of CPU idle state nodes
>   cpuidle: psci: Support hierarchical CPU idle states
>   cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains
>   cpuidle: psci: Add support for PM domains by using genpd
>   cpuidle: psci: Add a helper to attach a CPU to its PM domain
>   cpuidle: psci: Attach CPU devices to their PM domains
>   cpuidle: psci: Manage runtime PM in the idle path
>   arm64: dts: Convert to the hierarchical CPU topology layout for
>     MSM8916
>
>  .../devicetree/bindings/arm/psci.yaml         | 153 +++++++++
>  arch/arm64/boot/dts/qcom/msm8916.dtsi         |  57 +++-
>  drivers/cpuidle/Makefile                      |   4 +-
>  drivers/cpuidle/cpuidle-psci-domain.c         | 302 ++++++++++++++++++
>  drivers/cpuidle/cpuidle-psci.c                | 106 ++++--
>  drivers/cpuidle/cpuidle-psci.h                |  17 +
>  drivers/cpuidle/dt_idle_states.c              |   5 +-
>  drivers/firmware/psci/psci.c                  |  18 +-
>  drivers/of/base.c                             |  36 +++
>  include/linux/of.h                            |   8 +
>  include/linux/psci.h                          |   2 +
>  11 files changed, 673 insertions(+), 35 deletions(-)
>  create mode 100644 drivers/cpuidle/cpuidle-psci-domain.c
>  create mode 100644 drivers/cpuidle/cpuidle-psci.h
>
> --
> 2.17.1
>

Sudeep, Lorenzo,

Just wanted to give you a gentle ping about this series, especially
patch1 is kind of urgent.

Kind regards
Uffe

Re: [PATCH 01/13] cpuidle: psci: Fix potential access to unmapped memory

[Lorenzo Pieralisi]

On Thu, Oct 10, 2019 at 01:39:25PM +0200, Ulf Hansson wrote:
> When the WFI state have been selected, the in-parameter idx to
> psci_enter_idle_state() is zero. In this case, we must not index the state
> array as "state[idx - 1]", as it means accessing data outside the array.
> Fix the bug by pre-checking if idx is zero.
> 
> Fixes: 9ffeb6d08c3a ("PSCI: cpuidle: Refactor CPU suspend power_state parameter handling")
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/cpuidle/cpuidle-psci.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> index f3c1a2396f98..2e91c8d6c211 100644
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -27,10 +27,10 @@ static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
>  static int psci_enter_idle_state(struct cpuidle_device *dev,
>  				struct cpuidle_driver *drv, int idx)
>  {
> -	u32 *state = __this_cpu_read(psci_power_state);
> +	u32 *states = __this_cpu_read(psci_power_state);
> +	u32 state = idx ? states[idx - 1] : 0;
>  
> -	return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter,
> -					   idx, state[idx - 1]);
> +	return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);

Technically we don't dereference that array entry but I agree this
is ugly and potentially broken.

My preference is aligning it with ACPI code and allocate one more
entry in the psci_power_state array (useless for wfi, agreed but
at least we remove this (-1) handling from the code).

Thanks,
Lorenzo

>  }
>  
>  static struct cpuidle_driver psci_idle_driver __initdata = {
> -- 
> 2.17.1
> 

Re: [PATCH 01/13] cpuidle: psci: Fix potential access to unmapped memory

[Ulf Hansson]

On Fri, 18 Oct 2019 at 11:38, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Thu, Oct 10, 2019 at 01:39:25PM +0200, Ulf Hansson wrote:
> > When the WFI state have been selected, the in-parameter idx to
> > psci_enter_idle_state() is zero. In this case, we must not index the state
> > array as "state[idx - 1]", as it means accessing data outside the array.
> > Fix the bug by pre-checking if idx is zero.
> >
> > Fixes: 9ffeb6d08c3a ("PSCI: cpuidle: Refactor CPU suspend power_state parameter handling")
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/cpuidle/cpuidle-psci.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > index f3c1a2396f98..2e91c8d6c211 100644
> > --- a/drivers/cpuidle/cpuidle-psci.c
> > +++ b/drivers/cpuidle/cpuidle-psci.c
> > @@ -27,10 +27,10 @@ static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
> >  static int psci_enter_idle_state(struct cpuidle_device *dev,
> >                               struct cpuidle_driver *drv, int idx)
> >  {
> > -     u32 *state = __this_cpu_read(psci_power_state);
> > +     u32 *states = __this_cpu_read(psci_power_state);
> > +     u32 state = idx ? states[idx - 1] : 0;
> >
> > -     return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter,
> > -                                        idx, state[idx - 1]);
> > +     return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
>
> Technically we don't dereference that array entry but I agree this
> is ugly and potentially broken.

No sure understand the non-deference part.

If the governor selects WFI, the idx will be 0 - and thus we end up
using state[-1], doesn't that dereference an invalid address, no?

>
> My preference is aligning it with ACPI code and allocate one more
> entry in the psci_power_state array (useless for wfi, agreed but
> at least we remove this (-1) handling from the code).

I can do that, but sounds like a slightly bigger change. Are you fine
if I do that on top, so we can get this sent as fix for v5.4-rc[n]?

Kind regards
Uffe

Re: [PATCH 01/13] cpuidle: psci: Fix potential access to unmapped memory

[Lorenzo Pieralisi]

On Fri, Oct 18, 2019 at 11:51:11AM +0200, Ulf Hansson wrote:
> On Fri, 18 Oct 2019 at 11:38, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> >
> > On Thu, Oct 10, 2019 at 01:39:25PM +0200, Ulf Hansson wrote:
> > > When the WFI state have been selected, the in-parameter idx to
> > > psci_enter_idle_state() is zero. In this case, we must not index the state
> > > array as "state[idx - 1]", as it means accessing data outside the array.
> > > Fix the bug by pre-checking if idx is zero.
> > >
> > > Fixes: 9ffeb6d08c3a ("PSCI: cpuidle: Refactor CPU suspend power_state parameter handling")
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >  drivers/cpuidle/cpuidle-psci.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > > index f3c1a2396f98..2e91c8d6c211 100644
> > > --- a/drivers/cpuidle/cpuidle-psci.c
> > > +++ b/drivers/cpuidle/cpuidle-psci.c
> > > @@ -27,10 +27,10 @@ static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
> > >  static int psci_enter_idle_state(struct cpuidle_device *dev,
> > >                               struct cpuidle_driver *drv, int idx)
> > >  {
> > > -     u32 *state = __this_cpu_read(psci_power_state);
> > > +     u32 *states = __this_cpu_read(psci_power_state);
> > > +     u32 state = idx ? states[idx - 1] : 0;
> > >
> > > -     return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter,
> > > -                                        idx, state[idx - 1]);
> > > +     return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
> >
> > Technically we don't dereference that array entry but I agree this
> > is ugly and potentially broken.
> 
> No sure understand the non-deference part.
> 
> If the governor selects WFI, the idx will be 0 - and thus we end up
> using state[-1], doesn't that dereference an invalid address, no?

No because CPU_PM_CPU_IDLE_ENTER_PARAM is a macro, the code it
preprocesses to won't dereference state[idx - 1] if idx == 0.

I agree it is *very* ugly but technically code is not broken.

> > My preference is aligning it with ACPI code and allocate one more
> > entry in the psci_power_state array (useless for wfi, agreed but
> > at least we remove this (-1) handling from the code).
> 
> I can do that, but sounds like a slightly bigger change. Are you fine
> if I do that on top, so we can get this sent as fix for v5.4-rc[n]?

Technically we are not fixing anything; it is not such a big
change, we need to allocate one entry more and update the array
indexing.

Lorenzo

Re: [PATCH 01/13] cpuidle: psci: Fix potential access to unmapped memory

[Ulf Hansson]

On Fri, 18 Oct 2019 at 12:03, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Fri, Oct 18, 2019 at 11:51:11AM +0200, Ulf Hansson wrote:
> > On Fri, 18 Oct 2019 at 11:38, Lorenzo Pieralisi
> > <lorenzo.pieralisi@arm.com> wrote:
> > >
> > > On Thu, Oct 10, 2019 at 01:39:25PM +0200, Ulf Hansson wrote:
> > > > When the WFI state have been selected, the in-parameter idx to
> > > > psci_enter_idle_state() is zero. In this case, we must not index the state
> > > > array as "state[idx - 1]", as it means accessing data outside the array.
> > > > Fix the bug by pre-checking if idx is zero.
> > > >
> > > > Fixes: 9ffeb6d08c3a ("PSCI: cpuidle: Refactor CPU suspend power_state parameter handling")
> > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > ---
> > > >  drivers/cpuidle/cpuidle-psci.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > > > index f3c1a2396f98..2e91c8d6c211 100644
> > > > --- a/drivers/cpuidle/cpuidle-psci.c
> > > > +++ b/drivers/cpuidle/cpuidle-psci.c
> > > > @@ -27,10 +27,10 @@ static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
> > > >  static int psci_enter_idle_state(struct cpuidle_device *dev,
> > > >                               struct cpuidle_driver *drv, int idx)
> > > >  {
> > > > -     u32 *state = __this_cpu_read(psci_power_state);
> > > > +     u32 *states = __this_cpu_read(psci_power_state);
> > > > +     u32 state = idx ? states[idx - 1] : 0;
> > > >
> > > > -     return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter,
> > > > -                                        idx, state[idx - 1]);
> > > > +     return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
> > >
> > > Technically we don't dereference that array entry but I agree this
> > > is ugly and potentially broken.
> >
> > No sure understand the non-deference part.
> >
> > If the governor selects WFI, the idx will be 0 - and thus we end up
> > using state[-1], doesn't that dereference an invalid address, no?
>
> No because CPU_PM_CPU_IDLE_ENTER_PARAM is a macro, the code it
> preprocesses to won't dereference state[idx - 1] if idx == 0.
>
> I agree it is *very* ugly but technically code is not broken.

Ahh, got it, thanks!

>
> > > My preference is aligning it with ACPI code and allocate one more
> > > entry in the psci_power_state array (useless for wfi, agreed but
> > > at least we remove this (-1) handling from the code).
> >
> > I can do that, but sounds like a slightly bigger change. Are you fine
> > if I do that on top, so we can get this sent as fix for v5.4-rc[n]?
>
> Technically we are not fixing anything; it is not such a big
> change, we need to allocate one entry more and update the array
> indexing.

Okay, let me do the change - and it seems like it doesn't even have to
be sent as a fix then. Right?

Kind regards
Uffe

Re: [PATCH 01/13] cpuidle: psci: Fix potential access to unmapped memory

[Lorenzo Pieralisi]

On Fri, Oct 18, 2019 at 12:29:54PM +0200, Ulf Hansson wrote:

[...]

> > Technically we are not fixing anything; it is not such a big
> > change, we need to allocate one entry more and update the array
> > indexing.
> 
> Okay, let me do the change - and it seems like it doesn't even have to
> be sent as a fix then. Right?

No it does not (even though I agree that's misleading and "fixing"
it for v5.4 would not hurt either).

Lorenzo

[PATCH] cpuidle: psci: Align psci_power_state count with idle state count

[Sudeep Holla]

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

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

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

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/cpuidle/cpuidle-psci.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Hi Ulf, Lorenzo,

Just to avoid confusion, I thought I will just write this patch as I was
about to make reference to this in my review.

Regards,
Sudeep

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index f3c1a2396f98..361985f52ddd 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -30,7 +30,7 @@ static int psci_enter_idle_state(struct cpuidle_device *dev,
 	u32 *state = __this_cpu_read(psci_power_state);

 	return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter,
-					   idx, state[idx - 1]);
+					   idx, state[idx]);
 }

 static struct cpuidle_driver psci_idle_driver __initdata = {
@@ -89,12 +89,14 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
 	if (!count)
 		return -ENODEV;

+	count++; /* Add WFI state too */
 	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
 	if (!psci_states)
 		return -ENOMEM;

-	for (i = 0; i < count; i++) {
-		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
+	for (i = 1; i < count; i++) {
+		state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
+					      i - 1);
 		ret = psci_dt_parse_state_node(state_node, &psci_states[i]);
 		of_node_put(state_node);

--
2.17.1

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

[Sudeep Holla]

On Thu, Oct 10, 2019 at 01:39:26PM +0200, Ulf Hansson wrote:
> Update PSCI DT bindings to allow to represent idle states for CPUs and the
> CPU topology, by using a hierarchical layout. Primarily this is done by
> re-using the existing power domain description [1] and the domain idle
> state description [2].
>
> Let's also take the opportunity to update the examples to clarify the
> difference between the currently supported flattened layout vs the new
> hierarchical layout.
>

This looks fine to me. FWIW:

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

But before this gets merged, I would like to add another but "the golden"
example Qcom *always* referred during ACPI LPI discussions. Ofcourse, it
can be addition patch and if I get time, I can write this but no promise
ATM.

Hierarchical Representation:
System
1. SYSTEM_RET
2. SYSTEM_PG

	Cluster#0
	1. CLUSTER_RET
	2. CLUSTER_PG

		Core#0
		1. CORE_CG
		2. CORE_RET
		3. CORE_PG

		Core#1
		1. CORE_CG
		2. CORE_RET
		3. CORE_PG
	Cluster#1 (ditto)

Flattened Representation:

Core#0
	1 CORE_CG
	2 CORE_RET
	3 CORE_RET + CLUSTER_RET
	4 CORE_RET + CLUSTER_RET + SYSTEM_RET
	5 CORE_PG
	6 CORE_PG  + CLUSTER_RET
	7 CORE_PG  + CLUSTER_RET + SYSTEM_RET
	8 CORE_PG  + CLUSTER_PG
	9 CORE_PG  + CLUSTER_PG  + SYSTEM_RET
       10 CORE_PG  + CLUSTER_PG  + SYSTEM_PG

Though we may not implement everything needed to support this, but
we must ensure we don't have to end up in a situation breaking backward
compatibility trying to support the same.

--
Regards,
Sudeep

Re: [PATCH 03/13] firmware: psci: Export functions to manage the OSI mode

[Sudeep Holla]

On Thu, Oct 10, 2019 at 01:39:27PM +0200, Ulf Hansson wrote:
> To allow subsequent changes to implement support for OSI mode through the
> cpuidle-psci driver, export the existing psci_has_osi_support(). Export
> also a new function, psci_set_osi_mode(), that allows its caller to enable
> the OS-initiated CPU-suspend mode in the PSCI FW.
>
> To deal with backwards compatibility for a kernel started through a kexec
> call, default to set the CPU-suspend mode to the Platform Coordinated mode
> during boot.
>

FWIW,

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

--
Regards,
Sudeep

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

[Sudeep Holla]

On Thu, Oct 10, 2019 at 01:39:28PM +0200, Ulf Hansson wrote:
> The CPU's idle state nodes are currently parsed at the common cpuidle DT
> library, but also when initializing data for specific CPU idle operations,
> as in the PSCI cpuidle driver case and qcom-spm cpuidle case.
> 
> To avoid open-coding, let's introduce of_get_cpu_state_node(), which takes
> the device node for the CPU and the index to the requested idle state node,
> as in-parameters. In case a corresponding idle state node is found, it
> returns the node with the refcount incremented for it, else it returns
> NULL.
> 
> Moreover, for PSCI there are two options to describe the CPU's idle states
> [1], either via a flattened description or a hierarchical layout. Hence,
> let's take both options into account.
> 
> [1] Documentation/devicetree/bindings/arm/psci.yaml
> 
> Suggested-by: Sudeep Holla <sudeep.holla@arm.com>

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

--
Regards,
Sudeep

Re: [PATCH 05/13] cpuidle: dt: Support hierarchical CPU idle states

[Sudeep Holla]

On Thu, Oct 10, 2019 at 01:39:29PM +0200, Ulf Hansson wrote:
> From: Lina Iyer <lina.iyer@linaro.org>
> 
> Currently CPU's idle states are represented using the flattened model.
> Let's add support for the hierarchical layout, via converting to use
> of_get_cpu_state_node().
> 
> Suggested-by: Sudeep Holla <sudeep.holla@arm.com>

Looks simpler now, thanks.

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

--
Regards,
Sudeep

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

[Sudeep Holla]

On Thu, Oct 10, 2019 at 01:39:30PM +0200, Ulf Hansson wrote:
> Iterating through the idle state nodes in DT, to find out the number of
> states that needs to be allocated is unnecessary, as it has already been
> done from dt_init_idle_driver(). Therefore, drop the iteration and use the
> number we already have at hand.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/cpuidle/cpuidle-psci.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> index 2e91c8d6c211..1195a1056139 100644
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -73,28 +73,22 @@ static int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
>  	return 0;
>  }
>
> -static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
> +static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
> +				unsigned int state_nodes, int cpu)

[super nit] Too much in the beginning of the patch to not notice this ;)
May need some '(' alignment here and other places in general.

>  {
> -	int i, ret = 0, count = 0;
> +	int i, ret = 0;
>  	u32 *psci_states;
>  	struct device_node *state_node;
>
> -	/* Count idle states */
> -	while ((state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
> -					      count))) {
> -		count++;
> -		of_node_put(state_node);
> -	}
> -
> -	if (!count)
> -		return -ENODEV;
> -
> -	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
> +	psci_states = kcalloc(state_nodes, sizeof(*psci_states), GFP_KERNEL);
>  	if (!psci_states)
>  		return -ENOMEM;
>
> -	for (i = 0; i < count; i++) {
> +	for (i = 0; i < state_nodes; i++) {
>  		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);

Can we move above to use of_get_cpu_state_node ? Since it also handles
domain-idle-states.

> +		if (!state_node)
> +			break;
> +
>  		ret = psci_dt_parse_state_node(state_node, &psci_states[i]);
>  		of_node_put(state_node);
>
> @@ -104,6 +98,11 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
>  		pr_debug("psci-power-state %#x index %d\n", psci_states[i], i);
>  	}
>
> +	if (i != state_nodes) {
> +		ret = -ENODEV;
> +		goto free_mem;
> +	}
> +
>  	/* Idle states parsed correctly, initialize per-cpu pointer */
>  	per_cpu(psci_power_state, cpu) = psci_states;
>  	return 0;
> @@ -113,7 +112,7 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
>  	return ret;
>  }
>
> -static __init int psci_cpu_init_idle(unsigned int cpu)
> +static __init int psci_cpu_init_idle(unsigned int cpu, unsigned int state_nodes)

Does it make sense to rename it as state_count or something similar ?
And it may need + 1 once we add wfi also as entry as suggested by
Lorenzo.

--
Regards,
Sudeep

Re: [PATCH 07/13] cpuidle: psci: Support hierarchical CPU idle states

[Sudeep Holla]

On Thu, Oct 10, 2019 at 01:39:31PM +0200, Ulf Hansson wrote:
> Currently CPU's idle states are represented using the flattened model.
> Let's add support for the hierarchical layout, via converting to use
> of_get_cpu_state_node().
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/cpuidle/cpuidle-psci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> index 1195a1056139..5c30f23a8a7b 100644
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -85,7 +85,7 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
>  		return -ENOMEM;
>  
>  	for (i = 0; i < state_nodes; i++) {
> -		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
> +		state_node = of_get_cpu_state_node(cpu_node, i);

Ah, here we go. Sorry, ignore my comment in previous patch then.

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

--
Regards,
Sudeep

Re: [PATCH 08/13] cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains

[Sudeep Holla]

On Thu, Oct 10, 2019 at 01:39:32PM +0200, Ulf Hansson wrote:
> The per CPU variable psci_power_state, contains an array of fixed values,
> which reflects the corresponding arm,psci-suspend-param parsed from DT, for
> each of the available CPU idle states.
>
> This isn't sufficient when using the hierarchical CPU topology in DT, in
> combination with having PSCI OS initiated (OSI) mode enabled. More
> precisely, in OSI mode, Linux is responsible of telling the PSCI FW what
> idle state the cluster (a group of CPUs) should enter, while in PSCI
> Platform Coordinated (PC) mode, each CPU independently votes for an idle
> state of the cluster.
>
> For this reason, introduce a per CPU variable called domain_state and
> implement two helper functions to read/write its value. Then let the
> domain_state take precedence over the regular selected state, when idling
> the CPU in psci_enter_idle_state().
>
> This allows subsequent patches that implements support for PM domains for
> cpuidle-psci, to write the selected idle state parameter for the cluster
> into the domain_state variable. Furthermore, let's share the needed
> functions in a header file, to enable the support for PM domains to be
> implemented in a separate c-file.
>
> Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/cpuidle/cpuidle-psci.c | 31 ++++++++++++++++++++++++++++---
>  drivers/cpuidle/cpuidle-psci.h | 11 +++++++++++
>  2 files changed, 39 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/cpuidle/cpuidle-psci.h
>
> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> index 5c30f23a8a7b..a16467daf99d 100644
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -20,17 +20,42 @@
>
>  #include <asm/cpuidle.h>
>
> +#include "cpuidle-psci.h"
>  #include "dt_idle_states.h"
>
>  static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
> +static DEFINE_PER_CPU(u32, domain_state);
> +

[nit] for me name 'domain_state' is per domain and for one each in the
hierarchical topology. But here it's per cpu, just wondering if we can
rename it to provide implicit meaning ?

> +void psci_set_domain_state(u32 state)
> +{
> +	__this_cpu_write(domain_state, state);
> +}
> +
> +static inline u32 psci_get_domain_state(void)
> +{
> +	return __this_cpu_read(domain_state);
> +}
> +
> +static int __psci_enter_idle_state(int idx, u32 state)
> +{
> +	return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
> +}
>
>  static int psci_enter_idle_state(struct cpuidle_device *dev,
>  				struct cpuidle_driver *drv, int idx)
>  {
> +	int ret;
>  	u32 *states = __this_cpu_read(psci_power_state);
> -	u32 state = idx ? states[idx - 1] : 0;
> +	u32 state = psci_get_domain_state();
>
> -	return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
> +	if (!state && idx)
> +		state = states[idx - 1];

This can go as mentioned earlier once we have entry for WFI also.

> +
> +	ret = __psci_enter_idle_state(idx, state);
> +
> +	/* Clear the domain state to start fresh when back from idle. */
> +	psci_set_domain_state(0);
> +	return ret;
>  }
>
>  static struct cpuidle_driver psci_idle_driver __initdata = {
> @@ -56,7 +81,7 @@ static const struct of_device_id psci_idle_state_match[] __initconst = {
>  	{ },
>  };
>
> -static int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
> +int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)

Can this me made part of patch that uses it instead of here ?

--
Regards,
Sudeep

Re: [PATCH 09/13] cpuidle: psci: Add support for PM domains by using genpd

[Sudeep Holla]

On Thu, Oct 10, 2019 at 01:39:33PM +0200, Ulf Hansson wrote:
> When the hierarchical CPU topology layout is used in DT and the PSCI OSI
> mode is supported by the PSCI FW, let's initialize a corresponding PM
> domain topology by using genpd. This enables a CPU and a group of CPUs,
> when attached to the topology, to be power-managed accordingly.
>
> To trigger the attempt to initialize the genpd data structures a
> subsys_initcall is used, which should be early enough to allow CPUs, but
> also other devices to be attached.
>
> The initialization consists of parsing the PSCI OF node for the topology
> and the "domain idle states" DT bindings. In case the idle states are
> compatible with "domain-idle-state", the initialized genpd becomes
> responsible of selecting an idle state for the PM domain, via assigning it
> genpd governor.
>
> Note that, a successful initialization of the genpd data structures, is
> followed by a call to psci_set_osi_mode(), as to try to enable the OSI mode
> in the PSCI FW. In case this fails, we fall back into a degraded mode
> rather than bailing out and returning an error code.
>
> Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/cpuidle/Makefile              |   4 +-
>  drivers/cpuidle/cpuidle-psci-domain.c | 281 ++++++++++++++++++++++++++
>  2 files changed, 284 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/cpuidle/cpuidle-psci-domain.c
>
> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> index ee70d5cc5b99..cc8c769d7fa9 100644
> --- a/drivers/cpuidle/Makefile
> +++ b/drivers/cpuidle/Makefile
> @@ -21,7 +21,9 @@ obj-$(CONFIG_ARM_U8500_CPUIDLE)         += cpuidle-ux500.o
>  obj-$(CONFIG_ARM_AT91_CPUIDLE)          += cpuidle-at91.o
>  obj-$(CONFIG_ARM_EXYNOS_CPUIDLE)        += cpuidle-exynos.o
>  obj-$(CONFIG_ARM_CPUIDLE)		+= cpuidle-arm.o
> -obj-$(CONFIG_ARM_PSCI_CPUIDLE)		+= cpuidle-psci.o
> +obj-$(CONFIG_ARM_PSCI_CPUIDLE)		+= cpuidle_psci.o
> +cpuidle_psci-y				:= cpuidle-psci.o
> +cpuidle_psci-$(CONFIG_PM_GENERIC_DOMAINS_OF) += cpuidle-psci-domain.o
>
>  ###############################################################################
>  # MIPS drivers
> diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> new file mode 100644
> index 000000000000..3f5143ccc3e0
> --- /dev/null
> +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> @@ -0,0 +1,281 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PM domains for CPUs via genpd - managed by cpuidle-psci.
> + *
> + * Copyright (C) 2019 Linaro Ltd.
> + * Author: Ulf Hansson <ulf.hansson@linaro.org>
> + *
> + */
> +
> +#define pr_fmt(fmt) "CPUidle PSCI: " fmt
> +
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/pm_domain.h>
> +#include <linux/psci.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +
> +#include "cpuidle-psci.h"
> +
> +struct psci_pd_provider {
> +	struct list_head link;
> +	struct device_node *node;
> +};
> +
> +static LIST_HEAD(psci_pd_providers);
> +static bool osi_mode_enabled;
> +
> +static int psci_pd_power_off(struct generic_pm_domain *pd)
> +{
> +	struct genpd_power_state *state = &pd->states[pd->state_idx];
> +	u32 *pd_state;
> +
> +	/* If we have failed to enable OSI mode, then abort power off. */
> +	if (!osi_mode_enabled)
> +		return -EBUSY;
> +
> +	if (!state->data)
> +		return 0;
> +
> +	/* OSI mode is enabled, set the corresponding domain state. */
> +	pd_state = state->data;
> +	psci_set_domain_state(*pd_state);
> +
> +	return 0;
> +}
> +
> +static int __init psci_pd_parse_state_nodes(struct genpd_power_state *states,
> +					int state_count)
> +{
> +	int i, ret;
> +	u32 psci_state, *psci_state_buf;
> +
> +	for (i = 0; i < state_count; i++) {
> +		ret = psci_dt_parse_state_node(to_of_node(states[i].fwnode),
> +					&psci_state);
> +		if (ret)
> +			goto free_state;
> +
> +		psci_state_buf = kmalloc(sizeof(u32), GFP_KERNEL);
> +		if (!psci_state_buf) {
> +			ret = -ENOMEM;
> +			goto free_state;
> +		}
> +		*psci_state_buf = psci_state;
> +		states[i].data = psci_state_buf;
> +	}
> +
> +	return 0;
> +
> +free_state:
> +	i--;
> +	for (; i >= 0; i--)
> +		kfree(states[i].data);
> +	return ret;
> +}
> +
> +static int __init psci_pd_parse_states(struct device_node *np,
> +			struct genpd_power_state **states, int *state_count)
> +{
> +	int ret;
> +
> +	/* Parse the domain idle states. */
> +	ret = of_genpd_parse_idle_states(np, states, state_count);
> +	if (ret)
> +		return ret;
> +
> +	/* Fill out the PSCI specifics for each found state. */
> +	ret = psci_pd_parse_state_nodes(*states, *state_count);
> +	if (ret)
> +		kfree(*states);
> +
> +	return ret;
> +}
> +
> +static void psci_pd_free_states(struct genpd_power_state *states,
> +				unsigned int state_count)
> +{
> +	int i;
> +
> +	for (i = 0; i < state_count; i++)
> +		kfree(states[i].data);
> +	kfree(states);
> +}
> +
> +static int __init psci_pd_init(struct device_node *np)
> +{
> +	struct generic_pm_domain *pd;
> +	struct psci_pd_provider *pd_provider;
> +	struct dev_power_governor *pd_gov;
> +	struct genpd_power_state *states = NULL;
> +	int ret = -ENOMEM, state_count = 0;
> +
> +	pd = kzalloc(sizeof(*pd), GFP_KERNEL);
> +	if (!pd)
> +		goto out;
> +
> +	pd_provider = kzalloc(sizeof(*pd_provider), GFP_KERNEL);
> +	if (!pd_provider)
> +		goto free_pd;
> +
> +	pd->name = kasprintf(GFP_KERNEL, "%pOF", np);

I was bit confused initially and hence asked if we can name it better in
earlier patch. Now IIUC, "power-domain-names" are for the pd consumers to
attach to them by names using genpd_dev_pm_attach_by_name and not the
name of the power domain provider.

> +	if (!pd->name)
> +		goto free_pd_prov;
> +
> +	/*
> +	 * Parse the domain idle states and let genpd manage the state selection
> +	 * for those being compatible with "domain-idle-state".
> +	 */
> +	ret = psci_pd_parse_states(np, &states, &state_count);
> +	if (ret)
> +		goto free_name;
> +
> +	pd->free_states = psci_pd_free_states;
> +	pd->name = kbasename(pd->name);

Not sure as I have not used it before, but can't you directly use %pOFn
above to do the same ?

I haven't gone through all the error handling paths in detail. I will keep
it for next time ;)

--
Regards,
Sudeep

Re: [PATCH] cpuidle: psci: Align psci_power_state count with idle state count

[Ulf Hansson]

On Thu, 24 Oct 2019 at 17:18, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> Instead of allocating 'n-1' states in psci_power_state to manage 'n'
> idle states which include "ARM WFI" state, it would be simpler to have
> 1:1 mapping between psci_power_state and cpuidle driver states.
>
> ARM WFI state(i.e. idx == 0) is handled specially in the generic macro
> CPU_PM_CPU_IDLE_ENTER_PARAM and hence state[-1] is not possible. However
> for sake of code readability, it is better to have 1:1 mapping and not
> use [idx - 1] to access psci_power_state corresponding to driver cpuidle
> state for idx.
>
> psci_power_state[0] is default initialised to 0 and is never accessed
> while entering WFI state.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---
>  drivers/cpuidle/cpuidle-psci.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> Hi Ulf, Lorenzo,
>
> Just to avoid confusion, I thought I will just write this patch as I was
> about to make reference to this in my review.

As discussed with Lorenzo, I said I was going to adopt his review
comments, which means I already have a patch for this locally.

Nevermind this time, but I would appreciate if this kind of
bikeshedding can been avoided future wise.

Kind regards
Uffe

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

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

[Ulf Hansson]

On Thu, 24 Oct 2019 at 17:26, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Oct 10, 2019 at 01:39:26PM +0200, Ulf Hansson wrote:
> > Update PSCI DT bindings to allow to represent idle states for CPUs and the
> > CPU topology, by using a hierarchical layout. Primarily this is done by
> > re-using the existing power domain description [1] and the domain idle
> > state description [2].
> >
> > Let's also take the opportunity to update the examples to clarify the
> > difference between the currently supported flattened layout vs the new
> > hierarchical layout.
> >
>
> This looks fine to me. FWIW:
>
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
>
> But before this gets merged, I would like to add another but "the golden"
> example Qcom *always* referred during ACPI LPI discussions. Ofcourse, it
> can be addition patch and if I get time, I can write this but no promise
> ATM.

I like the description below, thanks for clarifying that.

Although, as you say, we can for sure add it on top. As a matter of
fact, I think that is even the best way forward, as currently we can't
support it (because of limitations in genpd, that I have started
working on a bit).

>
> Hierarchical Representation:
> System
> 1. SYSTEM_RET
> 2. SYSTEM_PG
>
>         Cluster#0
>         1. CLUSTER_RET
>         2. CLUSTER_PG
>
>                 Core#0
>                 1. CORE_CG
>                 2. CORE_RET
>                 3. CORE_PG
>
>                 Core#1
>                 1. CORE_CG
>                 2. CORE_RET
>                 3. CORE_PG
>         Cluster#1 (ditto)
>
> Flattened Representation:
>
> Core#0
>         1 CORE_CG
>         2 CORE_RET
>         3 CORE_RET + CLUSTER_RET
>         4 CORE_RET + CLUSTER_RET + SYSTEM_RET
>         5 CORE_PG
>         6 CORE_PG  + CLUSTER_RET
>         7 CORE_PG  + CLUSTER_RET + SYSTEM_RET
>         8 CORE_PG  + CLUSTER_PG
>         9 CORE_PG  + CLUSTER_PG  + SYSTEM_RET
>        10 CORE_PG  + CLUSTER_PG  + SYSTEM_PG
>
> Though we may not implement everything needed to support this, but
> we must ensure we don't have to end up in a situation breaking backward
> compatibility trying to support the same.

Yep, right. I don't see any issue in regards to backward compatibility
to support this above.

Thanks for reviewing!

Kind regards
Uffe

Re: [PATCH 10/13] cpuidle: psci: Add a helper to attach a CPU to its PM domain

[Sudeep Holla]

On Thu, Oct 10, 2019 at 01:39:34PM +0200, Ulf Hansson wrote:
> Introduce a PSCI DT helper function, psci_dt_attach_cpu(), which takes a
> CPU number as an in-parameter and tries to attach the CPU's struct device
> to its corresponding PM domain.
> 
> Let's makes use of dev_pm_domain_attach_by_name(), as it allows us to
> specify "psci" as the "name" of the PM domain to attach to. Additionally,
> let's also prepare the attached device to be power managed via runtime PM.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/cpuidle/cpuidle-psci-domain.c | 21 +++++++++++++++++++++
>  drivers/cpuidle/cpuidle-psci.h        |  6 ++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> index 3f5143ccc3e0..7429fd7626a1 100644
> --- a/drivers/cpuidle/cpuidle-psci-domain.c
> +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> @@ -9,9 +9,11 @@
>  
>  #define pr_fmt(fmt) "CPUidle PSCI: " fmt
>  
> +#include <linux/cpu.h>
>  #include <linux/device.h>
>  #include <linux/kernel.h>
>  #include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/psci.h>
>  #include <linux/slab.h>
>  #include <linux/string.h>
> @@ -279,3 +281,22 @@ static int __init psci_idle_init_domains(void)
>  	return ret;
>  }
>  subsys_initcall(psci_idle_init_domains);
> +
> +struct device *psci_dt_attach_cpu(int cpu)
> +{
> +	struct device *dev;
> +
> +	/* Currently limit the hierarchical topology to be used in OSI mode. */
> +	if (!psci_has_osi_support())
> +		return NULL;
> +
> +	dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");

This clarifies the need for the fixed name. But why not just go by index 0
as the consumer of these psci power-domains will have only one power domain
entry. Why do we need this name compulsory ? Further, it's specified as
optional in the generic binding, do we make it "required" for this psci
idle states binding anywhere that I missed ?

--
Regards,
Sudeep

Re: [PATCH 12/13] cpuidle: psci: Manage runtime PM in the idle path

[Sudeep Holla]

On Thu, Oct 10, 2019 at 01:39:36PM +0200, Ulf Hansson wrote:
> In case we have succeeded to attach a CPU to its PM domain, let's deploy
> runtime PM support for the corresponding attached device, to allow the CPU
> to be powered-managed accordingly.
>
> To set the triggering point for when runtime PM reference counting should
> be done, let's store the index of deepest idle state for the CPU in the per
> CPU struct. Then use this index to compare the selected idle state index
> when entering idle, as to understand whether runtime PM reference counting
> is needed or not.
>
> Note that, from the hierarchical point view, there may be good reasons to
> do runtime PM reference counting even on shallower idle states, but at this
> point this isn't supported, mainly due to limitations set by the generic PM
> domain.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/cpuidle/cpuidle-psci.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> index 1510422c7a53..0919b40c1a85 100644
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -16,6 +16,7 @@
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/psci.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/slab.h>
>
>  #include <asm/cpuidle.h>
> @@ -25,6 +26,7 @@
>
>  struct psci_cpuidle_data {
>  	u32 *psci_states;
> +	u32 rpm_state_id;
>  	struct device *dev;
>  };
>
> @@ -50,14 +52,28 @@ static int psci_enter_idle_state(struct cpuidle_device *dev,
>  				struct cpuidle_driver *drv, int idx)
>  {
>  	int ret;
> -	u32 *states = __this_cpu_read(psci_cpuidle_data.psci_states);
> -	u32 state = psci_get_domain_state();
> +	struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
> +	u32 *states = data->psci_states;
> +	struct device *pd_dev = data->dev;
> +	bool runtime_pm = (pd_dev && data->rpm_state_id == idx);
> +	u32 state;

Wonder if we can have separate psci_enter_idle_state for OSI mode so
that all these runtime extra check can be reduced ? It will also make
sure there's no additional latency for PC mode because of OSI.

--
Regards,
Sudeep

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

[Ulf Hansson]

On Thu, 24 Oct 2019 at 17:36, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Oct 10, 2019 at 01:39:30PM +0200, Ulf Hansson wrote:
> > Iterating through the idle state nodes in DT, to find out the number of
> > states that needs to be allocated is unnecessary, as it has already been
> > done from dt_init_idle_driver(). Therefore, drop the iteration and use the
> > number we already have at hand.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/cpuidle/cpuidle-psci.c | 33 ++++++++++++++++-----------------
> >  1 file changed, 16 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > index 2e91c8d6c211..1195a1056139 100644
> > --- a/drivers/cpuidle/cpuidle-psci.c
> > +++ b/drivers/cpuidle/cpuidle-psci.c
> > @@ -73,28 +73,22 @@ static int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
> >       return 0;
> >  }
> >
> > -static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
> > +static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
> > +                             unsigned int state_nodes, int cpu)
>
> [super nit] Too much in the beginning of the patch to not notice this ;)
> May need some '(' alignment here and other places in general.

I was trying to find a consistent way of doing it, according to the
existing code, but I failed. :-)

Two cases exist where calls/functions crosses one line, one use solely
tabs and the other uses tab+whitespace to align "exactly". You are
saying that you prefer the latter? If so, I can adopt to that.

>
> >  {
> > -     int i, ret = 0, count = 0;
> > +     int i, ret = 0;
> >       u32 *psci_states;
> >       struct device_node *state_node;
> >
> > -     /* Count idle states */
> > -     while ((state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
> > -                                           count))) {
> > -             count++;
> > -             of_node_put(state_node);
> > -     }
> > -
> > -     if (!count)
> > -             return -ENODEV;
> > -
> > -     psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
> > +     psci_states = kcalloc(state_nodes, sizeof(*psci_states), GFP_KERNEL);
> >       if (!psci_states)
> >               return -ENOMEM;
> >
> > -     for (i = 0; i < count; i++) {
> > +     for (i = 0; i < state_nodes; i++) {
> >               state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
>
> Can we move above to use of_get_cpu_state_node ? Since it also handles
> domain-idle-states.
>
> > +             if (!state_node)
> > +                     break;
> > +
> >               ret = psci_dt_parse_state_node(state_node, &psci_states[i]);
> >               of_node_put(state_node);
> >
> > @@ -104,6 +98,11 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
> >               pr_debug("psci-power-state %#x index %d\n", psci_states[i], i);
> >       }
> >
> > +     if (i != state_nodes) {
> > +             ret = -ENODEV;
> > +             goto free_mem;
> > +     }
> > +
> >       /* Idle states parsed correctly, initialize per-cpu pointer */
> >       per_cpu(psci_power_state, cpu) = psci_states;
> >       return 0;
> > @@ -113,7 +112,7 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
> >       return ret;
> >  }
> >
> > -static __init int psci_cpu_init_idle(unsigned int cpu)
> > +static __init int psci_cpu_init_idle(unsigned int cpu, unsigned int state_nodes)
>
> Does it make sense to rename it as state_count or something similar ?

Let me check to see if it makes sense to change it. Rebasing on top of
your recently submitted patch, might tell better.

> And it may need + 1 once we add wfi also as entry as suggested by
> Lorenzo.

Yep.

Kind regards
Uffe

Re: [PATCH 11/13] cpuidle: psci: Attach CPU devices to their PM domains

[Sudeep Holla]

On Thu, Oct 10, 2019 at 01:39:35PM +0200, Ulf Hansson wrote:
> In order to enable a CPU to be power managed through its PM domain, let's
> try to attach it by calling psci_dt_attach_cpu() during the cpuidle
> initialization.
>
> psci_dt_attach_cpu() returns a pointer to the attached struct device, which
> later should be used for runtime PM, hence we need to store it somewhere.
> Rather than adding yet another per CPU variable, let's create a per CPU
> struct to collect the relevant per CPU variables.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/cpuidle/cpuidle-psci.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> index a16467daf99d..1510422c7a53 100644
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -23,7 +23,12 @@
>  #include "cpuidle-psci.h"
>  #include "dt_idle_states.h"
>
> -static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
> +struct psci_cpuidle_data {
> +	u32 *psci_states;
> +	struct device *dev;
> +};
> +
> +static DEFINE_PER_CPU_READ_MOSTLY(struct psci_cpuidle_data, psci_cpuidle_data);
>  static DEFINE_PER_CPU(u32, domain_state);
>

/me just thinking still: If it make sense to keep psci_states separate
and domain_state and only other things needed for RPM/OSI in the
structure. I do understand that we modify domain_state and hence
we can't use READ_MOSTLY then. Let's see, for now keep it as is, thought
I will think out aloud.

--
Regards,
Sudeep

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

[Sudeep Holla]

On Thu, Oct 10, 2019 at 01:39:37PM +0200, Ulf Hansson wrote:
> To enable the OS to better support PSCI OS initiated CPU suspend mode,
> let's convert from the flattened layout to the hierarchical layout.
> 
> In the hierarchical layout, let's create a power domain provider per CPU
> and describe the idle states for each CPU inside the power domain provider
> node. To group the CPUs into a cluster, let's add another power domain
> provider and make it act as the master domain. Note that, the CPU's idle
> states remains compatible with "arm,idle-state", while the cluster's idle
> state becomes compatible with "domain-idle-state".
> 
> Cc: Andy Gross <andy.gross@linaro.org>
> Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/msm8916.dtsi | 57 +++++++++++++++++++++++++--
>  1 file changed, 53 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> index 5ea9fb8f2f87..1ece0c763592 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> @@ -102,10 +102,11 @@
>  			reg = <0x0>;
>  			next-level-cache = <&L2_0>;
>  			enable-method = "psci";
> -			cpu-idle-states = <&CPU_SLEEP_0>;
>  			clocks = <&apcs>;
>  			operating-points-v2 = <&cpu_opp_table>;
>  			#cooling-cells = <2>;
> +			power-domains = <&CPU_PD0>;
> +			power-domain-names = "psci";

As mentioned in the patch: Do we really need to make power-domain-names
compulsory ?

--
Regards,
Sudeep

Re: [PATCH 10/13] cpuidle: psci: Add a helper to attach a CPU to its PM domain

[Ulf Hansson]

On Thu, 24 Oct 2019 at 18:31, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Oct 10, 2019 at 01:39:34PM +0200, Ulf Hansson wrote:
> > Introduce a PSCI DT helper function, psci_dt_attach_cpu(), which takes a
> > CPU number as an in-parameter and tries to attach the CPU's struct device
> > to its corresponding PM domain.
> >
> > Let's makes use of dev_pm_domain_attach_by_name(), as it allows us to
> > specify "psci" as the "name" of the PM domain to attach to. Additionally,
> > let's also prepare the attached device to be power managed via runtime PM.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/cpuidle/cpuidle-psci-domain.c | 21 +++++++++++++++++++++
> >  drivers/cpuidle/cpuidle-psci.h        |  6 ++++++
> >  2 files changed, 27 insertions(+)
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> > index 3f5143ccc3e0..7429fd7626a1 100644
> > --- a/drivers/cpuidle/cpuidle-psci-domain.c
> > +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> > @@ -9,9 +9,11 @@
> >
> >  #define pr_fmt(fmt) "CPUidle PSCI: " fmt
> >
> > +#include <linux/cpu.h>
> >  #include <linux/device.h>
> >  #include <linux/kernel.h>
> >  #include <linux/pm_domain.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/psci.h>
> >  #include <linux/slab.h>
> >  #include <linux/string.h>
> > @@ -279,3 +281,22 @@ static int __init psci_idle_init_domains(void)
> >       return ret;
> >  }
> >  subsys_initcall(psci_idle_init_domains);
> > +
> > +struct device *psci_dt_attach_cpu(int cpu)
> > +{
> > +     struct device *dev;
> > +
> > +     /* Currently limit the hierarchical topology to be used in OSI mode. */
> > +     if (!psci_has_osi_support())
> > +             return NULL;
> > +
> > +     dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
>
> This clarifies the need for the fixed name. But why not just go by index 0
> as the consumer of these psci power-domains will have only one power domain
> entry. Why do we need this name compulsory ?

The idea is to be future proof. If I recall correctly, the CPU node on
some QCOM SoCs may also have "CPR" PM domain specified, thus
"multiple" power-domains could be specified.

In any case, using "psci" doesn't really hurt, right?

> Further, it's specified as
> optional in the generic binding, do we make it "required" for this psci
> idle states binding anywhere that I missed ?

Good point! Unless you tell me differently, I will update the DT doc
to clarify this is "required".

Kind regards
Uffe

Re: [PATCH 11/13] cpuidle: psci: Attach CPU devices to their PM domains

[Ulf Hansson]

On Thu, 24 Oct 2019 at 18:35, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Oct 10, 2019 at 01:39:35PM +0200, Ulf Hansson wrote:
> > In order to enable a CPU to be power managed through its PM domain, let's
> > try to attach it by calling psci_dt_attach_cpu() during the cpuidle
> > initialization.
> >
> > psci_dt_attach_cpu() returns a pointer to the attached struct device, which
> > later should be used for runtime PM, hence we need to store it somewhere.
> > Rather than adding yet another per CPU variable, let's create a per CPU
> > struct to collect the relevant per CPU variables.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/cpuidle/cpuidle-psci.c | 23 +++++++++++++++++++----
> >  1 file changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > index a16467daf99d..1510422c7a53 100644
> > --- a/drivers/cpuidle/cpuidle-psci.c
> > +++ b/drivers/cpuidle/cpuidle-psci.c
> > @@ -23,7 +23,12 @@
> >  #include "cpuidle-psci.h"
> >  #include "dt_idle_states.h"
> >
> > -static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
> > +struct psci_cpuidle_data {
> > +     u32 *psci_states;
> > +     struct device *dev;
> > +};
> > +
> > +static DEFINE_PER_CPU_READ_MOSTLY(struct psci_cpuidle_data, psci_cpuidle_data);
> >  static DEFINE_PER_CPU(u32, domain_state);
> >
>
> /me just thinking still: If it make sense to keep psci_states separate
> and domain_state and only other things needed for RPM/OSI in the
> structure. I do understand that we modify domain_state and hence
> we can't use READ_MOSTLY then. Let's see, for now keep it as is, thought
> I will think out aloud.

I believe we are striving towards the same goal, which likely means to
separate the non-OSI path vs OSI path, as much as possible. Simply to
avoid any unnecessary operation being done in the non-OSI path. Right?

However, while I was trying to address that, I realized that it would
probably introduce even more changes to the series. Therefore, it
thought it may be better to address these kind of changes on top, as
improvements.

Does it make sense?

Kind regards
Uffe

Re: [PATCH 12/13] cpuidle: psci: Manage runtime PM in the idle path

[Ulf Hansson]

On Thu, 24 Oct 2019 at 18:33, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Oct 10, 2019 at 01:39:36PM +0200, Ulf Hansson wrote:
> > In case we have succeeded to attach a CPU to its PM domain, let's deploy
> > runtime PM support for the corresponding attached device, to allow the CPU
> > to be powered-managed accordingly.
> >
> > To set the triggering point for when runtime PM reference counting should
> > be done, let's store the index of deepest idle state for the CPU in the per
> > CPU struct. Then use this index to compare the selected idle state index
> > when entering idle, as to understand whether runtime PM reference counting
> > is needed or not.
> >
> > Note that, from the hierarchical point view, there may be good reasons to
> > do runtime PM reference counting even on shallower idle states, but at this
> > point this isn't supported, mainly due to limitations set by the generic PM
> > domain.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/cpuidle/cpuidle-psci.c | 21 +++++++++++++++++++--
> >  1 file changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > index 1510422c7a53..0919b40c1a85 100644
> > --- a/drivers/cpuidle/cpuidle-psci.c
> > +++ b/drivers/cpuidle/cpuidle-psci.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> >  #include <linux/psci.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/slab.h>
> >
> >  #include <asm/cpuidle.h>
> > @@ -25,6 +26,7 @@
> >
> >  struct psci_cpuidle_data {
> >       u32 *psci_states;
> > +     u32 rpm_state_id;
> >       struct device *dev;
> >  };
> >
> > @@ -50,14 +52,28 @@ static int psci_enter_idle_state(struct cpuidle_device *dev,
> >                               struct cpuidle_driver *drv, int idx)
> >  {
> >       int ret;
> > -     u32 *states = __this_cpu_read(psci_cpuidle_data.psci_states);
> > -     u32 state = psci_get_domain_state();
> > +     struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
> > +     u32 *states = data->psci_states;
> > +     struct device *pd_dev = data->dev;
> > +     bool runtime_pm = (pd_dev && data->rpm_state_id == idx);
> > +     u32 state;
>
> Wonder if we can have separate psci_enter_idle_state for OSI mode so
> that all these runtime extra check can be reduced ? It will also make
> sure there's no additional latency for PC mode because of OSI.

Good idea, that's the plan. See previous answer.

Perhaps if I add a patch on top, implementing your suggestion, would
you be happy with that?

Kind regards
Uffe

Re: [PATCH 08/13] cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains

[Ulf Hansson]

On Thu, 24 Oct 2019 at 17:42, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Oct 10, 2019 at 01:39:32PM +0200, Ulf Hansson wrote:
> > The per CPU variable psci_power_state, contains an array of fixed values,
> > which reflects the corresponding arm,psci-suspend-param parsed from DT, for
> > each of the available CPU idle states.
> >
> > This isn't sufficient when using the hierarchical CPU topology in DT, in
> > combination with having PSCI OS initiated (OSI) mode enabled. More
> > precisely, in OSI mode, Linux is responsible of telling the PSCI FW what
> > idle state the cluster (a group of CPUs) should enter, while in PSCI
> > Platform Coordinated (PC) mode, each CPU independently votes for an idle
> > state of the cluster.
> >
> > For this reason, introduce a per CPU variable called domain_state and
> > implement two helper functions to read/write its value. Then let the
> > domain_state take precedence over the regular selected state, when idling
> > the CPU in psci_enter_idle_state().
> >
> > This allows subsequent patches that implements support for PM domains for
> > cpuidle-psci, to write the selected idle state parameter for the cluster
> > into the domain_state variable. Furthermore, let's share the needed
> > functions in a header file, to enable the support for PM domains to be
> > implemented in a separate c-file.
> >
> > Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/cpuidle/cpuidle-psci.c | 31 ++++++++++++++++++++++++++++---
> >  drivers/cpuidle/cpuidle-psci.h | 11 +++++++++++
> >  2 files changed, 39 insertions(+), 3 deletions(-)
> >  create mode 100644 drivers/cpuidle/cpuidle-psci.h
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > index 5c30f23a8a7b..a16467daf99d 100644
> > --- a/drivers/cpuidle/cpuidle-psci.c
> > +++ b/drivers/cpuidle/cpuidle-psci.c
> > @@ -20,17 +20,42 @@
> >
> >  #include <asm/cpuidle.h>
> >
> > +#include "cpuidle-psci.h"
> >  #include "dt_idle_states.h"
> >
> >  static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
> > +static DEFINE_PER_CPU(u32, domain_state);
> > +
>
> [nit] for me name 'domain_state' is per domain and for one each in the
> hierarchical topology. But here it's per cpu, just wondering if we can
> rename it to provide implicit meaning ?

Feel free to suggest something. I couldn't find any better. :-)

>
> > +void psci_set_domain_state(u32 state)
> > +{
> > +     __this_cpu_write(domain_state, state);
> > +}
> > +
> > +static inline u32 psci_get_domain_state(void)
> > +{
> > +     return __this_cpu_read(domain_state);
> > +}
> > +
> > +static int __psci_enter_idle_state(int idx, u32 state)
> > +{
> > +     return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
> > +}
> >
> >  static int psci_enter_idle_state(struct cpuidle_device *dev,
> >                               struct cpuidle_driver *drv, int idx)
> >  {
> > +     int ret;
> >       u32 *states = __this_cpu_read(psci_power_state);
> > -     u32 state = idx ? states[idx - 1] : 0;
> > +     u32 state = psci_get_domain_state();
> >
> > -     return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
> > +     if (!state && idx)
> > +             state = states[idx - 1];
>
> This can go as mentioned earlier once we have entry for WFI also.
>
> > +
> > +     ret = __psci_enter_idle_state(idx, state);
> > +
> > +     /* Clear the domain state to start fresh when back from idle. */
> > +     psci_set_domain_state(0);
> > +     return ret;
> >  }
> >
> >  static struct cpuidle_driver psci_idle_driver __initdata = {
> > @@ -56,7 +81,7 @@ static const struct of_device_id psci_idle_state_match[] __initconst = {
> >       { },
> >  };
> >
> > -static int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
> > +int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
>
> Can this me made part of patch that uses it instead of here ?

Sure, just wanted to keep the all the preparations for the PM domains
support together.

But you have a point, let me move this part to the patch that needs it.

Kind regards
Uffe

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

[Ulf Hansson]

- Andy

On Thu, 24 Oct 2019 at 18:41, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Oct 10, 2019 at 01:39:37PM +0200, Ulf Hansson wrote:
> > To enable the OS to better support PSCI OS initiated CPU suspend mode,
> > let's convert from the flattened layout to the hierarchical layout.
> >
> > In the hierarchical layout, let's create a power domain provider per CPU
> > and describe the idle states for each CPU inside the power domain provider
> > node. To group the CPUs into a cluster, let's add another power domain
> > provider and make it act as the master domain. Note that, the CPU's idle
> > states remains compatible with "arm,idle-state", while the cluster's idle
> > state becomes compatible with "domain-idle-state".
> >
> > Cc: Andy Gross <andy.gross@linaro.org>
> > Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/msm8916.dtsi | 57 +++++++++++++++++++++++++--
> >  1 file changed, 53 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > index 5ea9fb8f2f87..1ece0c763592 100644
> > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > @@ -102,10 +102,11 @@
> >                       reg = <0x0>;
> >                       next-level-cache = <&L2_0>;
> >                       enable-method = "psci";
> > -                     cpu-idle-states = <&CPU_SLEEP_0>;
> >                       clocks = <&apcs>;
> >                       operating-points-v2 = <&cpu_opp_table>;
> >                       #cooling-cells = <2>;
> > +                     power-domains = <&CPU_PD0>;
> > +                     power-domain-names = "psci";
>
> As mentioned in the patch: Do we really need to make power-domain-names
> compulsory ?

Yes, I think that is a good idea. However, let's discuss in the other
thread instead.

Again, thanks a lot for reviewing!

Kind regards
Uffe

Re: [PATCH 12/13] cpuidle: psci: Manage runtime PM in the idle path

[Lorenzo Pieralisi]

On Thu, Oct 24, 2019 at 07:00:38PM +0200, Ulf Hansson wrote:
> On Thu, 24 Oct 2019 at 18:33, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Oct 10, 2019 at 01:39:36PM +0200, Ulf Hansson wrote:
> > > In case we have succeeded to attach a CPU to its PM domain, let's deploy
> > > runtime PM support for the corresponding attached device, to allow the CPU
> > > to be powered-managed accordingly.
> > >
> > > To set the triggering point for when runtime PM reference counting should
> > > be done, let's store the index of deepest idle state for the CPU in the per
> > > CPU struct. Then use this index to compare the selected idle state index
> > > when entering idle, as to understand whether runtime PM reference counting
> > > is needed or not.
> > >
> > > Note that, from the hierarchical point view, there may be good reasons to
> > > do runtime PM reference counting even on shallower idle states, but at this
> > > point this isn't supported, mainly due to limitations set by the generic PM
> > > domain.
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >  drivers/cpuidle/cpuidle-psci.c | 21 +++++++++++++++++++--
> > >  1 file changed, 19 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > > index 1510422c7a53..0919b40c1a85 100644
> > > --- a/drivers/cpuidle/cpuidle-psci.c
> > > +++ b/drivers/cpuidle/cpuidle-psci.c
> > > @@ -16,6 +16,7 @@
> > >  #include <linux/of.h>
> > >  #include <linux/of_device.h>
> > >  #include <linux/psci.h>
> > > +#include <linux/pm_runtime.h>
> > >  #include <linux/slab.h>
> > >
> > >  #include <asm/cpuidle.h>
> > > @@ -25,6 +26,7 @@
> > >
> > >  struct psci_cpuidle_data {
> > >       u32 *psci_states;
> > > +     u32 rpm_state_id;
> > >       struct device *dev;
> > >  };
> > >
> > > @@ -50,14 +52,28 @@ static int psci_enter_idle_state(struct cpuidle_device *dev,
> > >                               struct cpuidle_driver *drv, int idx)
> > >  {
> > >       int ret;
> > > -     u32 *states = __this_cpu_read(psci_cpuidle_data.psci_states);
> > > -     u32 state = psci_get_domain_state();
> > > +     struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
> > > +     u32 *states = data->psci_states;
> > > +     struct device *pd_dev = data->dev;
> > > +     bool runtime_pm = (pd_dev && data->rpm_state_id == idx);
> > > +     u32 state;
> >
> > Wonder if we can have separate psci_enter_idle_state for OSI mode so
> > that all these runtime extra check can be reduced ? It will also make
> > sure there's no additional latency for PC mode because of OSI.
> 
> Good idea, that's the plan. See previous answer.
> 
> Perhaps if I add a patch on top, implementing your suggestion, would
> you be happy with that?

Separating idle entry functions was the main idea behind moving PSCI
CPUidle into drivers/cpuidle, I don't think that's complicated to
change and given that the series is not queued yet we can make these
changes in the series itself rather than on top.

Thanks,
Lorenzo

Re: [PATCH 12/13] cpuidle: psci: Manage runtime PM in the idle path

[Ulf Hansson]

On Fri, 25 Oct 2019 at 10:29, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Thu, Oct 24, 2019 at 07:00:38PM +0200, Ulf Hansson wrote:
> > On Thu, 24 Oct 2019 at 18:33, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Thu, Oct 10, 2019 at 01:39:36PM +0200, Ulf Hansson wrote:
> > > > In case we have succeeded to attach a CPU to its PM domain, let's deploy
> > > > runtime PM support for the corresponding attached device, to allow the CPU
> > > > to be powered-managed accordingly.
> > > >
> > > > To set the triggering point for when runtime PM reference counting should
> > > > be done, let's store the index of deepest idle state for the CPU in the per
> > > > CPU struct. Then use this index to compare the selected idle state index
> > > > when entering idle, as to understand whether runtime PM reference counting
> > > > is needed or not.
> > > >
> > > > Note that, from the hierarchical point view, there may be good reasons to
> > > > do runtime PM reference counting even on shallower idle states, but at this
> > > > point this isn't supported, mainly due to limitations set by the generic PM
> > > > domain.
> > > >
> > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > ---
> > > >  drivers/cpuidle/cpuidle-psci.c | 21 +++++++++++++++++++--
> > > >  1 file changed, 19 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > > > index 1510422c7a53..0919b40c1a85 100644
> > > > --- a/drivers/cpuidle/cpuidle-psci.c
> > > > +++ b/drivers/cpuidle/cpuidle-psci.c
> > > > @@ -16,6 +16,7 @@
> > > >  #include <linux/of.h>
> > > >  #include <linux/of_device.h>
> > > >  #include <linux/psci.h>
> > > > +#include <linux/pm_runtime.h>
> > > >  #include <linux/slab.h>
> > > >
> > > >  #include <asm/cpuidle.h>
> > > > @@ -25,6 +26,7 @@
> > > >
> > > >  struct psci_cpuidle_data {
> > > >       u32 *psci_states;
> > > > +     u32 rpm_state_id;
> > > >       struct device *dev;
> > > >  };
> > > >
> > > > @@ -50,14 +52,28 @@ static int psci_enter_idle_state(struct cpuidle_device *dev,
> > > >                               struct cpuidle_driver *drv, int idx)
> > > >  {
> > > >       int ret;
> > > > -     u32 *states = __this_cpu_read(psci_cpuidle_data.psci_states);
> > > > -     u32 state = psci_get_domain_state();
> > > > +     struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
> > > > +     u32 *states = data->psci_states;
> > > > +     struct device *pd_dev = data->dev;
> > > > +     bool runtime_pm = (pd_dev && data->rpm_state_id == idx);
> > > > +     u32 state;
> > >
> > > Wonder if we can have separate psci_enter_idle_state for OSI mode so
> > > that all these runtime extra check can be reduced ? It will also make
> > > sure there's no additional latency for PC mode because of OSI.
> >
> > Good idea, that's the plan. See previous answer.
> >
> > Perhaps if I add a patch on top, implementing your suggestion, would
> > you be happy with that?
>
> Separating idle entry functions was the main idea behind moving PSCI
> CPUidle into drivers/cpuidle, I don't think that's complicated to
> change and given that the series is not queued yet we can make these
> changes in the series itself rather than on top.

Okay, no problem. I fold in a patch (or amend existing ones, if that
is better) into the series that addresses this.

Thanks for reviewing and enjoy your weekend!

Kind regards
Uffe

Re: [PATCH] cpuidle: psci: Align psci_power_state count with idle state count

[Sudeep Holla]

On Thu, Oct 24, 2019 at 06:10:09PM +0200, Ulf Hansson wrote:
> On Thu, 24 Oct 2019 at 17:18, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > Instead of allocating 'n-1' states in psci_power_state to manage 'n'
> > idle states which include "ARM WFI" state, it would be simpler to have
> > 1:1 mapping between psci_power_state and cpuidle driver states.
> >
> > ARM WFI state(i.e. idx == 0) is handled specially in the generic macro
> > CPU_PM_CPU_IDLE_ENTER_PARAM and hence state[-1] is not possible. However
> > for sake of code readability, it is better to have 1:1 mapping and not
> > use [idx - 1] to access psci_power_state corresponding to driver cpuidle
> > state for idx.
> >
> > psci_power_state[0] is default initialised to 0 and is never accessed
> > while entering WFI state.
> >
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>
> Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> > ---
> >  drivers/cpuidle/cpuidle-psci.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > Hi Ulf, Lorenzo,
> >
> > Just to avoid confusion, I thought I will just write this patch as I was
> > about to make reference to this in my review.
>
> As discussed with Lorenzo, I said I was going to adopt his review
> comments, which means I already have a patch for this locally.
>
> Nevermind this time, but I would appreciate if this kind of
> bikeshedding can been avoided future wise.
>

That's one of the reason I just wrote the patch as I felt describing it
it words was difficult compared to patch :). Sorry if you felt this was
bikeshedding.

--
Regards,
Sudeep

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

[Sudeep Holla]

On Thu, Oct 24, 2019 at 06:33:00PM +0200, Ulf Hansson wrote:
> On Thu, 24 Oct 2019 at 17:36, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Oct 10, 2019 at 01:39:30PM +0200, Ulf Hansson wrote:
> > > Iterating through the idle state nodes in DT, to find out the number of
> > > states that needs to be allocated is unnecessary, as it has already been
> > > done from dt_init_idle_driver(). Therefore, drop the iteration and use the
> > > number we already have at hand.
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >  drivers/cpuidle/cpuidle-psci.c | 33 ++++++++++++++++-----------------
> > >  1 file changed, 16 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > > index 2e91c8d6c211..1195a1056139 100644
> > > --- a/drivers/cpuidle/cpuidle-psci.c
> > > +++ b/drivers/cpuidle/cpuidle-psci.c
> > > @@ -73,28 +73,22 @@ static int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
> > >       return 0;
> > >  }
> > >
> > > -static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
> > > +static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
> > > +                             unsigned int state_nodes, int cpu)
> >
> > [super nit] Too much in the beginning of the patch to not notice this ;)
> > May need some '(' alignment here and other places in general.
>
> I was trying to find a consistent way of doing it, according to the
> existing code, but I failed. :-)
>
> Two cases exist where calls/functions crosses one line, one use solely
> tabs and the other uses tab+whitespace to align "exactly". You are
> saying that you prefer the latter? If so, I can adopt to that.
>

I am not too picky on these, just found it in the beginning of the patch
and hence mentioned it. If it was in the middle, I am sure I wouldn't have
noticed.

> >
> > >  {
> > > -     int i, ret = 0, count = 0;
> > > +     int i, ret = 0;
> > >       u32 *psci_states;
> > >       struct device_node *state_node;
> > >
> > > -     /* Count idle states */
> > > -     while ((state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
> > > -                                           count))) {
> > > -             count++;
> > > -             of_node_put(state_node);
> > > -     }
> > > -
> > > -     if (!count)
> > > -             return -ENODEV;
> > > -
> > > -     psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
> > > +     psci_states = kcalloc(state_nodes, sizeof(*psci_states), GFP_KERNEL);
> > >       if (!psci_states)
> > >               return -ENOMEM;
> > >
> > > -     for (i = 0; i < count; i++) {
> > > +     for (i = 0; i < state_nodes; i++) {
> > >               state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
> >
> > Can we move above to use of_get_cpu_state_node ? Since it also handles
> > domain-idle-states.
> >
> > > +             if (!state_node)
> > > +                     break;
> > > +
> > >               ret = psci_dt_parse_state_node(state_node, &psci_states[i]);
> > >               of_node_put(state_node);
> > >
> > > @@ -104,6 +98,11 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
> > >               pr_debug("psci-power-state %#x index %d\n", psci_states[i], i);
> > >       }
> > >
> > > +     if (i != state_nodes) {
> > > +             ret = -ENODEV;
> > > +             goto free_mem;
> > > +     }
> > > +
> > >       /* Idle states parsed correctly, initialize per-cpu pointer */
> > >       per_cpu(psci_power_state, cpu) = psci_states;
> > >       return 0;
> > > @@ -113,7 +112,7 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
> > >       return ret;
> > >  }
> > >
> > > -static __init int psci_cpu_init_idle(unsigned int cpu)
> > > +static __init int psci_cpu_init_idle(unsigned int cpu, unsigned int state_nodes)
> >
> > Does it make sense to rename it as state_count or something similar ?
>
> Let me check to see if it makes sense to change it. Rebasing on top of
> your recently submitted patch, might tell better.
>

Sure.

--
Regards,
Sudeep

Re: [PATCH 10/13] cpuidle: psci: Add a helper to attach a CPU to its PM domain

[Sudeep Holla]

On Thu, Oct 24, 2019 at 06:47:43PM +0200, Ulf Hansson wrote:
> On Thu, 24 Oct 2019 at 18:31, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Oct 10, 2019 at 01:39:34PM +0200, Ulf Hansson wrote:
> > > Introduce a PSCI DT helper function, psci_dt_attach_cpu(), which takes a
> > > CPU number as an in-parameter and tries to attach the CPU's struct device
> > > to its corresponding PM domain.
> > >
> > > Let's makes use of dev_pm_domain_attach_by_name(), as it allows us to
> > > specify "psci" as the "name" of the PM domain to attach to. Additionally,
> > > let's also prepare the attached device to be power managed via runtime PM.
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >  drivers/cpuidle/cpuidle-psci-domain.c | 21 +++++++++++++++++++++
> > >  drivers/cpuidle/cpuidle-psci.h        |  6 ++++++
> > >  2 files changed, 27 insertions(+)
> > >
> > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> > > index 3f5143ccc3e0..7429fd7626a1 100644
> > > --- a/drivers/cpuidle/cpuidle-psci-domain.c
> > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> > > @@ -9,9 +9,11 @@
> > >
> > >  #define pr_fmt(fmt) "CPUidle PSCI: " fmt
> > >
> > > +#include <linux/cpu.h>
> > >  #include <linux/device.h>
> > >  #include <linux/kernel.h>
> > >  #include <linux/pm_domain.h>
> > > +#include <linux/pm_runtime.h>
> > >  #include <linux/psci.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/string.h>
> > > @@ -279,3 +281,22 @@ static int __init psci_idle_init_domains(void)
> > >       return ret;
> > >  }
> > >  subsys_initcall(psci_idle_init_domains);
> > > +
> > > +struct device *psci_dt_attach_cpu(int cpu)
> > > +{
> > > +     struct device *dev;
> > > +
> > > +     /* Currently limit the hierarchical topology to be used in OSI mode. */
> > > +     if (!psci_has_osi_support())
> > > +             return NULL;
> > > +
> > > +     dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
> >
> > This clarifies the need for the fixed name. But why not just go by index 0
> > as the consumer of these psci power-domains will have only one power domain
> > entry. Why do we need this name compulsory ?
>
> The idea is to be future proof. If I recall correctly, the CPU node on
> some QCOM SoCs may also have "CPR" PM domain specified, thus
> "multiple" power-domains could be specified.
>

I am sure we don't want to mx-n-match any power domain provider with
psci. And also I expect in these above mentioned cases, there won't be any
psci power domains.

> In any case, using "psci" doesn't really hurt, right?
>

Doesn't but I don't see need for one as only one should exist, as mentioned
above we don't want mix-n-match with psci ever.

> > Further, it's specified as
> > optional in the generic binding, do we make it "required" for this psci
> > idle states binding anywhere that I missed ?
>
> Good point! Unless you tell me differently, I will update the DT doc
> to clarify this is "required".
>

No but why is my question ? We don't have to. If firmware/DT wants to
specify the name, sure. But it must remain optional IMO.

--
Regards,
Sudeep

Re: [PATCH 11/13] cpuidle: psci: Attach CPU devices to their PM domains

[Sudeep Holla]

On Thu, Oct 24, 2019 at 06:55:50PM +0200, Ulf Hansson wrote:
> On Thu, 24 Oct 2019 at 18:35, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Oct 10, 2019 at 01:39:35PM +0200, Ulf Hansson wrote:
> > > In order to enable a CPU to be power managed through its PM domain, let's
> > > try to attach it by calling psci_dt_attach_cpu() during the cpuidle
> > > initialization.
> > >
> > > psci_dt_attach_cpu() returns a pointer to the attached struct device, which
> > > later should be used for runtime PM, hence we need to store it somewhere.
> > > Rather than adding yet another per CPU variable, let's create a per CPU
> > > struct to collect the relevant per CPU variables.
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >  drivers/cpuidle/cpuidle-psci.c | 23 +++++++++++++++++++----
> > >  1 file changed, 19 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > > index a16467daf99d..1510422c7a53 100644
> > > --- a/drivers/cpuidle/cpuidle-psci.c
> > > +++ b/drivers/cpuidle/cpuidle-psci.c
> > > @@ -23,7 +23,12 @@
> > >  #include "cpuidle-psci.h"
> > >  #include "dt_idle_states.h"
> > >
> > > -static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
> > > +struct psci_cpuidle_data {
> > > +     u32 *psci_states;
> > > +     struct device *dev;
> > > +};
> > > +
> > > +static DEFINE_PER_CPU_READ_MOSTLY(struct psci_cpuidle_data, psci_cpuidle_data);
> > >  static DEFINE_PER_CPU(u32, domain_state);
> > >
> >
> > /me just thinking still: If it make sense to keep psci_states separate
> > and domain_state and only other things needed for RPM/OSI in the
> > structure. I do understand that we modify domain_state and hence
> > we can't use READ_MOSTLY then. Let's see, for now keep it as is, thought
> > I will think out aloud.
> 
> I believe we are striving towards the same goal, which likely means to
> separate the non-OSI path vs OSI path, as much as possible. Simply to
> avoid any unnecessary operation being done in the non-OSI path. Right?
>

Yes

> However, while I was trying to address that, I realized that it would
> probably introduce even more changes to the series. Therefore, it
> thought it may be better to address these kind of changes on top, as
> improvements.
>

If possible better to amend this unless it's too complicated.

--
Regards,
Sudeep

Re: [PATCH 12/13] cpuidle: psci: Manage runtime PM in the idle path

[Sudeep Holla]

On Thu, Oct 24, 2019 at 07:00:38PM +0200, Ulf Hansson wrote:
> On Thu, 24 Oct 2019 at 18:33, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Oct 10, 2019 at 01:39:36PM +0200, Ulf Hansson wrote:
> > > In case we have succeeded to attach a CPU to its PM domain, let's deploy
> > > runtime PM support for the corresponding attached device, to allow the CPU
> > > to be powered-managed accordingly.
> > >
> > > To set the triggering point for when runtime PM reference counting should
> > > be done, let's store the index of deepest idle state for the CPU in the per
> > > CPU struct. Then use this index to compare the selected idle state index
> > > when entering idle, as to understand whether runtime PM reference counting
> > > is needed or not.
> > >
> > > Note that, from the hierarchical point view, there may be good reasons to
> > > do runtime PM reference counting even on shallower idle states, but at this
> > > point this isn't supported, mainly due to limitations set by the generic PM
> > > domain.
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >  drivers/cpuidle/cpuidle-psci.c | 21 +++++++++++++++++++--
> > >  1 file changed, 19 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > > index 1510422c7a53..0919b40c1a85 100644
> > > --- a/drivers/cpuidle/cpuidle-psci.c
> > > +++ b/drivers/cpuidle/cpuidle-psci.c
> > > @@ -16,6 +16,7 @@
> > >  #include <linux/of.h>
> > >  #include <linux/of_device.h>
> > >  #include <linux/psci.h>
> > > +#include <linux/pm_runtime.h>
> > >  #include <linux/slab.h>
> > >
> > >  #include <asm/cpuidle.h>
> > > @@ -25,6 +26,7 @@
> > >
> > >  struct psci_cpuidle_data {
> > >       u32 *psci_states;
> > > +     u32 rpm_state_id;
> > >       struct device *dev;
> > >  };
> > >
> > > @@ -50,14 +52,28 @@ static int psci_enter_idle_state(struct cpuidle_device *dev,
> > >                               struct cpuidle_driver *drv, int idx)
> > >  {
> > >       int ret;
> > > -     u32 *states = __this_cpu_read(psci_cpuidle_data.psci_states);
> > > -     u32 state = psci_get_domain_state();
> > > +     struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
> > > +     u32 *states = data->psci_states;
> > > +     struct device *pd_dev = data->dev;
> > > +     bool runtime_pm = (pd_dev && data->rpm_state_id == idx);
> > > +     u32 state;
> >
> > Wonder if we can have separate psci_enter_idle_state for OSI mode so
> > that all these runtime extra check can be reduced ? It will also make
> > sure there's no additional latency for PC mode because of OSI.
> 
> Good idea, that's the plan. See previous answer.
> 
> Perhaps if I add a patch on top, implementing your suggestion, would
> you be happy with that?

No, I prefer to amend this itself to keep it easy to be able to bisect.

--
Regards,
Sudeep

Re: [PATCH 10/13] cpuidle: psci: Add a helper to attach a CPU to its PM domain

[Ulf Hansson]

On Sun, 27 Oct 2019 at 03:30, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Oct 24, 2019 at 06:47:43PM +0200, Ulf Hansson wrote:
> > On Thu, 24 Oct 2019 at 18:31, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Thu, Oct 10, 2019 at 01:39:34PM +0200, Ulf Hansson wrote:
> > > > Introduce a PSCI DT helper function, psci_dt_attach_cpu(), which takes a
> > > > CPU number as an in-parameter and tries to attach the CPU's struct device
> > > > to its corresponding PM domain.
> > > >
> > > > Let's makes use of dev_pm_domain_attach_by_name(), as it allows us to
> > > > specify "psci" as the "name" of the PM domain to attach to. Additionally,
> > > > let's also prepare the attached device to be power managed via runtime PM.
> > > >
> > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > ---
> > > >  drivers/cpuidle/cpuidle-psci-domain.c | 21 +++++++++++++++++++++
> > > >  drivers/cpuidle/cpuidle-psci.h        |  6 ++++++
> > > >  2 files changed, 27 insertions(+)
> > > >
> > > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> > > > index 3f5143ccc3e0..7429fd7626a1 100644
> > > > --- a/drivers/cpuidle/cpuidle-psci-domain.c
> > > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> > > > @@ -9,9 +9,11 @@
> > > >
> > > >  #define pr_fmt(fmt) "CPUidle PSCI: " fmt
> > > >
> > > > +#include <linux/cpu.h>
> > > >  #include <linux/device.h>
> > > >  #include <linux/kernel.h>
> > > >  #include <linux/pm_domain.h>
> > > > +#include <linux/pm_runtime.h>
> > > >  #include <linux/psci.h>
> > > >  #include <linux/slab.h>
> > > >  #include <linux/string.h>
> > > > @@ -279,3 +281,22 @@ static int __init psci_idle_init_domains(void)
> > > >       return ret;
> > > >  }
> > > >  subsys_initcall(psci_idle_init_domains);
> > > > +
> > > > +struct device *psci_dt_attach_cpu(int cpu)
> > > > +{
> > > > +     struct device *dev;
> > > > +
> > > > +     /* Currently limit the hierarchical topology to be used in OSI mode. */
> > > > +     if (!psci_has_osi_support())
> > > > +             return NULL;
> > > > +
> > > > +     dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
> > >
> > > This clarifies the need for the fixed name. But why not just go by index 0
> > > as the consumer of these psci power-domains will have only one power domain
> > > entry. Why do we need this name compulsory ?
> >
> > The idea is to be future proof. If I recall correctly, the CPU node on
> > some QCOM SoCs may also have "CPR" PM domain specified, thus
> > "multiple" power-domains could be specified.
> >
>
> I am sure we don't want to mx-n-match any power domain provider with
> psci. And also I expect in these above mentioned cases, there won't be any
> psci power domains.
>
> > In any case, using "psci" doesn't really hurt, right?
> >
>
> Doesn't but I don't see need for one as only one should exist, as mentioned
> above we don't want mix-n-match with psci ever.

Not sure I get your point, sorry.

The CPU could very well be attached to more than one power-domain. Of
course not multiple "PSCI power-domains". One could be an PSCI power
domain and another one could be the QCOM CPR (Core power reduction)
power domain.

Have a look at these binding, there are already upstream, perhaps that
clarifies this?
Documentation/devicetree/bindings/opp/qcom-nvmem-cpufreq.txt

>
> > > Further, it's specified as
> > > optional in the generic binding, do we make it "required" for this psci
> > > idle states binding anywhere that I missed ?
> >
> > Good point! Unless you tell me differently, I will update the DT doc
> > to clarify this is "required".
> >
>
> No but why is my question ? We don't have to. If firmware/DT wants to
> specify the name, sure. But it must remain optional IMO.

According the QCOM CPR case, we need a way to distinguish what power
domain we should attach the CPU to. If we don't use power-domain-names
to do that, what else should we use?

Kind regards
Uffe

Re: [PATCH 10/13] cpuidle: psci: Add a helper to attach a CPU to its PM domain

[Sudeep Holla]

On Mon, Oct 28, 2019 at 08:35:55AM +0100, Ulf Hansson wrote:
> On Sun, 27 Oct 2019 at 03:30, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Oct 24, 2019 at 06:47:43PM +0200, Ulf Hansson wrote:
> > > On Thu, 24 Oct 2019 at 18:31, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> > > > On Thu, Oct 10, 2019 at 01:39:34PM +0200, Ulf Hansson wrote:
> > > > > Introduce a PSCI DT helper function, psci_dt_attach_cpu(), which takes a
> > > > > CPU number as an in-parameter and tries to attach the CPU's struct device
> > > > > to its corresponding PM domain.
> > > > >
> > > > > Let's makes use of dev_pm_domain_attach_by_name(), as it allows us to
> > > > > specify "psci" as the "name" of the PM domain to attach to. Additionally,
> > > > > let's also prepare the attached device to be power managed via runtime PM.
> > > > >
> > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > ---
> > > > >  drivers/cpuidle/cpuidle-psci-domain.c | 21 +++++++++++++++++++++
> > > > >  drivers/cpuidle/cpuidle-psci.h        |  6 ++++++
> > > > >  2 files changed, 27 insertions(+)
> > > > >
> > > > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> > > > > index 3f5143ccc3e0..7429fd7626a1 100644
> > > > > --- a/drivers/cpuidle/cpuidle-psci-domain.c
> > > > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> > > > > @@ -9,9 +9,11 @@
> > > > >
> > > > >  #define pr_fmt(fmt) "CPUidle PSCI: " fmt
> > > > >
> > > > > +#include <linux/cpu.h>
> > > > >  #include <linux/device.h>
> > > > >  #include <linux/kernel.h>
> > > > >  #include <linux/pm_domain.h>
> > > > > +#include <linux/pm_runtime.h>
> > > > >  #include <linux/psci.h>
> > > > >  #include <linux/slab.h>
> > > > >  #include <linux/string.h>
> > > > > @@ -279,3 +281,22 @@ static int __init psci_idle_init_domains(void)
> > > > >       return ret;
> > > > >  }
> > > > >  subsys_initcall(psci_idle_init_domains);
> > > > > +
> > > > > +struct device *psci_dt_attach_cpu(int cpu)
> > > > > +{
> > > > > +     struct device *dev;
> > > > > +
> > > > > +     /* Currently limit the hierarchical topology to be used in OSI mode. */
> > > > > +     if (!psci_has_osi_support())
> > > > > +             return NULL;
> > > > > +
> > > > > +     dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
> > > >
> > > > This clarifies the need for the fixed name. But why not just go by index 0
> > > > as the consumer of these psci power-domains will have only one power domain
> > > > entry. Why do we need this name compulsory ?
> > >
> > > The idea is to be future proof. If I recall correctly, the CPU node on
> > > some QCOM SoCs may also have "CPR" PM domain specified, thus
> > > "multiple" power-domains could be specified.
> > >
> >
> > I am sure we don't want to mx-n-match any power domain provider with
> > psci. And also I expect in these above mentioned cases, there won't be any
> > psci power domains.
> >
> > > In any case, using "psci" doesn't really hurt, right?
> > >
> >
> > Doesn't but I don't see need for one as only one should exist, as mentioned
> > above we don't want mix-n-match with psci ever.
>
> Not sure I get your point, sorry.
>
> The CPU could very well be attached to more than one power-domain. Of
> course not multiple "PSCI power-domains". One could be an PSCI power
> domain and another one could be the QCOM CPR (Core power reduction)
> power domain.
>

And who controls QCOM CPR ? If it's OSPM, this model is broken.
I mean OSPM can vote, but the control *has* to be in PSCI firmware to
change any CPU power state.

If it's firmware controlled, then there's no need to explicitly specify
both to OSPM.

> Have a look at these binding, there are already upstream, perhaps that
> clarifies this?
> Documentation/devicetree/bindings/opp/qcom-nvmem-cpufreq.txt
>

OK, I will have a look.

--
Regards,
Sudeep

Re: [PATCH 10/13] cpuidle: psci: Add a helper to attach a CPU to its PM domain

[Ulf Hansson]

+ Niklas

On Mon, 28 Oct 2019 at 08:49, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Mon, Oct 28, 2019 at 08:35:55AM +0100, Ulf Hansson wrote:
> > On Sun, 27 Oct 2019 at 03:30, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Thu, Oct 24, 2019 at 06:47:43PM +0200, Ulf Hansson wrote:
> > > > On Thu, 24 Oct 2019 at 18:31, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > >
> > > > > On Thu, Oct 10, 2019 at 01:39:34PM +0200, Ulf Hansson wrote:
> > > > > > Introduce a PSCI DT helper function, psci_dt_attach_cpu(), which takes a
> > > > > > CPU number as an in-parameter and tries to attach the CPU's struct device
> > > > > > to its corresponding PM domain.
> > > > > >
> > > > > > Let's makes use of dev_pm_domain_attach_by_name(), as it allows us to
> > > > > > specify "psci" as the "name" of the PM domain to attach to. Additionally,
> > > > > > let's also prepare the attached device to be power managed via runtime PM.
> > > > > >
> > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > > ---
> > > > > >  drivers/cpuidle/cpuidle-psci-domain.c | 21 +++++++++++++++++++++
> > > > > >  drivers/cpuidle/cpuidle-psci.h        |  6 ++++++
> > > > > >  2 files changed, 27 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> > > > > > index 3f5143ccc3e0..7429fd7626a1 100644
> > > > > > --- a/drivers/cpuidle/cpuidle-psci-domain.c
> > > > > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> > > > > > @@ -9,9 +9,11 @@
> > > > > >
> > > > > >  #define pr_fmt(fmt) "CPUidle PSCI: " fmt
> > > > > >
> > > > > > +#include <linux/cpu.h>
> > > > > >  #include <linux/device.h>
> > > > > >  #include <linux/kernel.h>
> > > > > >  #include <linux/pm_domain.h>
> > > > > > +#include <linux/pm_runtime.h>
> > > > > >  #include <linux/psci.h>
> > > > > >  #include <linux/slab.h>
> > > > > >  #include <linux/string.h>
> > > > > > @@ -279,3 +281,22 @@ static int __init psci_idle_init_domains(void)
> > > > > >       return ret;
> > > > > >  }
> > > > > >  subsys_initcall(psci_idle_init_domains);
> > > > > > +
> > > > > > +struct device *psci_dt_attach_cpu(int cpu)
> > > > > > +{
> > > > > > +     struct device *dev;
> > > > > > +
> > > > > > +     /* Currently limit the hierarchical topology to be used in OSI mode. */
> > > > > > +     if (!psci_has_osi_support())
> > > > > > +             return NULL;
> > > > > > +
> > > > > > +     dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
> > > > >
> > > > > This clarifies the need for the fixed name. But why not just go by index 0
> > > > > as the consumer of these psci power-domains will have only one power domain
> > > > > entry. Why do we need this name compulsory ?
> > > >
> > > > The idea is to be future proof. If I recall correctly, the CPU node on
> > > > some QCOM SoCs may also have "CPR" PM domain specified, thus
> > > > "multiple" power-domains could be specified.
> > > >
> > >
> > > I am sure we don't want to mx-n-match any power domain provider with
> > > psci. And also I expect in these above mentioned cases, there won't be any
> > > psci power domains.
> > >
> > > > In any case, using "psci" doesn't really hurt, right?
> > > >
> > >
> > > Doesn't but I don't see need for one as only one should exist, as mentioned
> > > above we don't want mix-n-match with psci ever.
> >
> > Not sure I get your point, sorry.
> >
> > The CPU could very well be attached to more than one power-domain. Of
> > course not multiple "PSCI power-domains". One could be an PSCI power
> > domain and another one could be the QCOM CPR (Core power reduction)
> > power domain.
> >
>
> And who controls QCOM CPR ? If it's OSPM, this model is broken.
> I mean OSPM can vote, but the control *has* to be in PSCI firmware to
> change any CPU power state.
>
> If it's firmware controlled, then there's no need to explicitly specify
> both to OSPM.

This is about OPP and CPUFreq, so it has nothing to do with PSCI.

>
> > Have a look at these binding, there are already upstream, perhaps that
> > clarifies this?
> > Documentation/devicetree/bindings/opp/qcom-nvmem-cpufreq.txt
> >
>
> OK, I will have a look.

Great.

I have looped in Niklas Casell, he should be able to answer any more
detailed questions in regards to QCOM CPR, if that is needed.

In any case, we are discussing whether we should require a
power-domain-names set to "psci" for the CPU node - and I don't see
how that could hurt. Right?

Kind regards
Uffe

Re: [PATCH 12/13] cpuidle: psci: Manage runtime PM in the idle path

[Ulf Hansson]

On Sun, 27 Oct 2019 at 03:34, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Oct 24, 2019 at 07:00:38PM +0200, Ulf Hansson wrote:
> > On Thu, 24 Oct 2019 at 18:33, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Thu, Oct 10, 2019 at 01:39:36PM +0200, Ulf Hansson wrote:
> > > > In case we have succeeded to attach a CPU to its PM domain, let's deploy
> > > > runtime PM support for the corresponding attached device, to allow the CPU
> > > > to be powered-managed accordingly.
> > > >
> > > > To set the triggering point for when runtime PM reference counting should
> > > > be done, let's store the index of deepest idle state for the CPU in the per
> > > > CPU struct. Then use this index to compare the selected idle state index
> > > > when entering idle, as to understand whether runtime PM reference counting
> > > > is needed or not.
> > > >
> > > > Note that, from the hierarchical point view, there may be good reasons to
> > > > do runtime PM reference counting even on shallower idle states, but at this
> > > > point this isn't supported, mainly due to limitations set by the generic PM
> > > > domain.
> > > >
> > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > ---
> > > >  drivers/cpuidle/cpuidle-psci.c | 21 +++++++++++++++++++--
> > > >  1 file changed, 19 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > > > index 1510422c7a53..0919b40c1a85 100644
> > > > --- a/drivers/cpuidle/cpuidle-psci.c
> > > > +++ b/drivers/cpuidle/cpuidle-psci.c
> > > > @@ -16,6 +16,7 @@
> > > >  #include <linux/of.h>
> > > >  #include <linux/of_device.h>
> > > >  #include <linux/psci.h>
> > > > +#include <linux/pm_runtime.h>
> > > >  #include <linux/slab.h>
> > > >
> > > >  #include <asm/cpuidle.h>
> > > > @@ -25,6 +26,7 @@
> > > >
> > > >  struct psci_cpuidle_data {
> > > >       u32 *psci_states;
> > > > +     u32 rpm_state_id;
> > > >       struct device *dev;
> > > >  };
> > > >
> > > > @@ -50,14 +52,28 @@ static int psci_enter_idle_state(struct cpuidle_device *dev,
> > > >                               struct cpuidle_driver *drv, int idx)
> > > >  {
> > > >       int ret;
> > > > -     u32 *states = __this_cpu_read(psci_cpuidle_data.psci_states);
> > > > -     u32 state = psci_get_domain_state();
> > > > +     struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
> > > > +     u32 *states = data->psci_states;
> > > > +     struct device *pd_dev = data->dev;
> > > > +     bool runtime_pm = (pd_dev && data->rpm_state_id == idx);
> > > > +     u32 state;
> > >
> > > Wonder if we can have separate psci_enter_idle_state for OSI mode so
> > > that all these runtime extra check can be reduced ? It will also make
> > > sure there's no additional latency for PC mode because of OSI.
> >
> > Good idea, that's the plan. See previous answer.
> >
> > Perhaps if I add a patch on top, implementing your suggestion, would
> > you be happy with that?
>
> No, I prefer to amend this itself to keep it easy to be able to bisect.

Alright!

So I explored this a little bit more - and it actually forced me to
re-order some of the patches in the series, but it seems to have
turned out well.

In the new approach I have taken, I haven't replaced the actual
callback for the idle state, but instead make an early decision in
psci_enter_idle_state(), based on one single read of a per CPU
variable/struct. This tell what path to go.

I am running some final test, but should be able to post a new version
tomorrow. Although, if you still don't think the new approach is good
enough, we can always invent a callback, that we can assign when the
CPU is attached to PM domain. In any case, you will have to tell what
you think, after I posted the new version, just wanted to give you a
heads up.

Kind regards
Uffe

Re: [PATCH 10/13] cpuidle: psci: Add a helper to attach a CPU to its PM domain

[Sudeep Holla]

On Mon, Oct 28, 2019 at 10:45:22AM +0100, Ulf Hansson wrote:
> + Niklas
>
> On Mon, 28 Oct 2019 at 08:49, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Mon, Oct 28, 2019 at 08:35:55AM +0100, Ulf Hansson wrote:
> > > On Sun, 27 Oct 2019 at 03:30, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> > > > On Thu, Oct 24, 2019 at 06:47:43PM +0200, Ulf Hansson wrote:
> > > > > On Thu, 24 Oct 2019 at 18:31, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > > >
> > > > > > On Thu, Oct 10, 2019 at 01:39:34PM +0200, Ulf Hansson wrote:
> > > > > > > Introduce a PSCI DT helper function, psci_dt_attach_cpu(), which takes a
> > > > > > > CPU number as an in-parameter and tries to attach the CPU's struct device
> > > > > > > to its corresponding PM domain.
> > > > > > >
> > > > > > > Let's makes use of dev_pm_domain_attach_by_name(), as it allows us to
> > > > > > > specify "psci" as the "name" of the PM domain to attach to. Additionally,
> > > > > > > let's also prepare the attached device to be power managed via runtime PM.
> > > > > > >
> > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > > > ---
> > > > > > >  drivers/cpuidle/cpuidle-psci-domain.c | 21 +++++++++++++++++++++
> > > > > > >  drivers/cpuidle/cpuidle-psci.h        |  6 ++++++
> > > > > > >  2 files changed, 27 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> > > > > > > index 3f5143ccc3e0..7429fd7626a1 100644
> > > > > > > --- a/drivers/cpuidle/cpuidle-psci-domain.c
> > > > > > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> > > > > > > @@ -9,9 +9,11 @@
> > > > > > >
> > > > > > >  #define pr_fmt(fmt) "CPUidle PSCI: " fmt
> > > > > > >
> > > > > > > +#include <linux/cpu.h>
> > > > > > >  #include <linux/device.h>
> > > > > > >  #include <linux/kernel.h>
> > > > > > >  #include <linux/pm_domain.h>
> > > > > > > +#include <linux/pm_runtime.h>
> > > > > > >  #include <linux/psci.h>
> > > > > > >  #include <linux/slab.h>
> > > > > > >  #include <linux/string.h>
> > > > > > > @@ -279,3 +281,22 @@ static int __init psci_idle_init_domains(void)
> > > > > > >       return ret;
> > > > > > >  }
> > > > > > >  subsys_initcall(psci_idle_init_domains);
> > > > > > > +
> > > > > > > +struct device *psci_dt_attach_cpu(int cpu)
> > > > > > > +{
> > > > > > > +     struct device *dev;
> > > > > > > +
> > > > > > > +     /* Currently limit the hierarchical topology to be used in OSI mode. */
> > > > > > > +     if (!psci_has_osi_support())
> > > > > > > +             return NULL;
> > > > > > > +
> > > > > > > +     dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
> > > > > >
> > > > > > This clarifies the need for the fixed name. But why not just go by index 0
> > > > > > as the consumer of these psci power-domains will have only one power domain
> > > > > > entry. Why do we need this name compulsory ?
> > > > >
> > > > > The idea is to be future proof. If I recall correctly, the CPU node on
> > > > > some QCOM SoCs may also have "CPR" PM domain specified, thus
> > > > > "multiple" power-domains could be specified.
> > > > >
> > > >
> > > > I am sure we don't want to mx-n-match any power domain provider with
> > > > psci. And also I expect in these above mentioned cases, there won't be any
> > > > psci power domains.
> > > >
> > > > > In any case, using "psci" doesn't really hurt, right?
> > > > >
> > > >
> > > > Doesn't but I don't see need for one as only one should exist, as mentioned
> > > > above we don't want mix-n-match with psci ever.
> > >
> > > Not sure I get your point, sorry.
> > >
> > > The CPU could very well be attached to more than one power-domain. Of
> > > course not multiple "PSCI power-domains". One could be an PSCI power
> > > domain and another one could be the QCOM CPR (Core power reduction)
> > > power domain.
> > >
> >
> > And who controls QCOM CPR ? If it's OSPM, this model is broken.
> > I mean OSPM can vote, but the control *has* to be in PSCI firmware to
> > change any CPU power state.
> >
> > If it's firmware controlled, then there's no need to explicitly specify
> > both to OSPM.
>
> This is about OPP and CPUFreq, so it has nothing to do with PSCI.
>
> >
> > > Have a look at these binding, there are already upstream, perhaps that
> > > clarifies this?
> > > Documentation/devicetree/bindings/opp/qcom-nvmem-cpufreq.txt
> > >
> >
> > OK, I will have a look.
>
> Great.
>
> I have looped in Niklas Casell, he should be able to answer any more
> detailed questions in regards to QCOM CPR, if that is needed.
>

So had a look at the DT bindings and standalone it looks fine.
But when it's mixed like the way you describe: yikes!

Why does a power(oh wait it's actually performance domain!) is combined
with a device whose actual power is controlled by only by PSCI/firmware
is associated along with another power(again actally performance)
domain.

This whole representation of performance domain as power domain in the
bindings is a *mess*. If Linux kernel chose to implement it as part
of genpd, that's fine. But we should have had a separate binding for
that.

> In any case, we are discussing whether we should require a
> power-domain-names set to "psci" for the CPU node - and I don't see
> how that could hurt. Right?
>

Honestly I don't like this, but we don't have any choice I think.
So yes, but you need to update the binding. Hope new platform move
all these performance domain control part into firmware and have single
control from kernel unlike the present generation which OPP through
clock or cpufreq and the voltage/performance comtrol via genpd.

--
Regards,
Sudeep

Re: [PATCH 10/13] cpuidle: psci: Add a helper to attach a CPU to its PM domain

[Niklas Cassel]

On Tue, Oct 29, 2019 at 01:34:24PM +0800, Sudeep Holla wrote:
> On Mon, Oct 28, 2019 at 10:45:22AM +0100, Ulf Hansson wrote:
> > + Niklas
> >
> > On Mon, 28 Oct 2019 at 08:49, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Mon, Oct 28, 2019 at 08:35:55AM +0100, Ulf Hansson wrote:
> > > > On Sun, 27 Oct 2019 at 03:30, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > >
> > > > > On Thu, Oct 24, 2019 at 06:47:43PM +0200, Ulf Hansson wrote:
> > > > > > On Thu, 24 Oct 2019 at 18:31, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > > > >
> > > > > > > On Thu, Oct 10, 2019 at 01:39:34PM +0200, Ulf Hansson wrote:
> > > > > > > > Introduce a PSCI DT helper function, psci_dt_attach_cpu(), which takes a
> > > > > > > > CPU number as an in-parameter and tries to attach the CPU's struct device
> > > > > > > > to its corresponding PM domain.
> > > > > > > >
> > > > > > > > Let's makes use of dev_pm_domain_attach_by_name(), as it allows us to
> > > > > > > > specify "psci" as the "name" of the PM domain to attach to. Additionally,
> > > > > > > > let's also prepare the attached device to be power managed via runtime PM.
> > > > > > > >
> > > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > > > > ---
> > > > > > > >  drivers/cpuidle/cpuidle-psci-domain.c | 21 +++++++++++++++++++++
> > > > > > > >  drivers/cpuidle/cpuidle-psci.h        |  6 ++++++
> > > > > > > >  2 files changed, 27 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> > > > > > > > index 3f5143ccc3e0..7429fd7626a1 100644
> > > > > > > > --- a/drivers/cpuidle/cpuidle-psci-domain.c
> > > > > > > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> > > > > > > > @@ -9,9 +9,11 @@
> > > > > > > >
> > > > > > > >  #define pr_fmt(fmt) "CPUidle PSCI: " fmt
> > > > > > > >
> > > > > > > > +#include <linux/cpu.h>
> > > > > > > >  #include <linux/device.h>
> > > > > > > >  #include <linux/kernel.h>
> > > > > > > >  #include <linux/pm_domain.h>
> > > > > > > > +#include <linux/pm_runtime.h>
> > > > > > > >  #include <linux/psci.h>
> > > > > > > >  #include <linux/slab.h>
> > > > > > > >  #include <linux/string.h>
> > > > > > > > @@ -279,3 +281,22 @@ static int __init psci_idle_init_domains(void)
> > > > > > > >       return ret;
> > > > > > > >  }
> > > > > > > >  subsys_initcall(psci_idle_init_domains);
> > > > > > > > +
> > > > > > > > +struct device *psci_dt_attach_cpu(int cpu)
> > > > > > > > +{
> > > > > > > > +     struct device *dev;
> > > > > > > > +
> > > > > > > > +     /* Currently limit the hierarchical topology to be used in OSI mode. */
> > > > > > > > +     if (!psci_has_osi_support())
> > > > > > > > +             return NULL;
> > > > > > > > +
> > > > > > > > +     dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
> > > > > > >
> > > > > > > This clarifies the need for the fixed name. But why not just go by index 0
> > > > > > > as the consumer of these psci power-domains will have only one power domain
> > > > > > > entry. Why do we need this name compulsory ?
> > > > > >
> > > > > > The idea is to be future proof. If I recall correctly, the CPU node on
> > > > > > some QCOM SoCs may also have "CPR" PM domain specified, thus
> > > > > > "multiple" power-domains could be specified.
> > > > > >
> > > > >
> > > > > I am sure we don't want to mx-n-match any power domain provider with
> > > > > psci. And also I expect in these above mentioned cases, there won't be any
> > > > > psci power domains.
> > > > >
> > > > > > In any case, using "psci" doesn't really hurt, right?
> > > > > >
> > > > >
> > > > > Doesn't but I don't see need for one as only one should exist, as mentioned
> > > > > above we don't want mix-n-match with psci ever.
> > > >
> > > > Not sure I get your point, sorry.
> > > >
> > > > The CPU could very well be attached to more than one power-domain. Of
> > > > course not multiple "PSCI power-domains". One could be an PSCI power
> > > > domain and another one could be the QCOM CPR (Core power reduction)
> > > > power domain.
> > > >
> > >
> > > And who controls QCOM CPR ? If it's OSPM, this model is broken.
> > > I mean OSPM can vote, but the control *has* to be in PSCI firmware to
> > > change any CPU power state.
> > >
> > > If it's firmware controlled, then there's no need to explicitly specify
> > > both to OSPM.
> >
> > This is about OPP and CPUFreq, so it has nothing to do with PSCI.
> >
> > >
> > > > Have a look at these binding, there are already upstream, perhaps that
> > > > clarifies this?
> > > > Documentation/devicetree/bindings/opp/qcom-nvmem-cpufreq.txt
> > > >
> > >
> > > OK, I will have a look.
> >
> > Great.
> >
> > I have looped in Niklas Casell, he should be able to answer any more
> > detailed questions in regards to QCOM CPR, if that is needed.
> >
> 
> So had a look at the DT bindings and standalone it looks fine.
> But when it's mixed like the way you describe: yikes!
> 
> Why does a power(oh wait it's actually performance domain!) is combined
> with a device whose actual power is controlled by only by PSCI/firmware
> is associated along with another power(again actally performance)
> domain.
> 
> This whole representation of performance domain as power domain in the
> bindings is a *mess*. If Linux kernel chose to implement it as part
> of genpd, that's fine. But we should have had a separate binding for
> that.
> 
> > In any case, we are discussing whether we should require a
> > power-domain-names set to "psci" for the CPU node - and I don't see
> > how that could hurt. Right?
> >
> 
> Honestly I don't like this, but we don't have any choice I think.
> So yes, but you need to update the binding. Hope new platform move
> all these performance domain control part into firmware and have single
> control from kernel unlike the present generation which OPP through
> clock or cpufreq and the voltage/performance comtrol via genpd.

FWIW, in newer generation Qualcomm SoCs like SDM845,
the voltage/performance control is done in firmware,
by the OSM (drivers/cpufreq/qcom-cpufreq-hw.c).


Kind regards,
Niklas

Re: [PATCH 10/13] cpuidle: psci: Add a helper to attach a CPU to its PM domain

[Sudeep Holla]

On Tue, Oct 29, 2019 at 10:44:44AM +0100, Niklas Cassel wrote:
> On Tue, Oct 29, 2019 at 01:34:24PM +0800, Sudeep Holla wrote:
> > On Mon, Oct 28, 2019 at 10:45:22AM +0100, Ulf Hansson wrote:
> > > + Niklas
> > >
> > > On Mon, 28 Oct 2019 at 08:49, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> > > > On Mon, Oct 28, 2019 at 08:35:55AM +0100, Ulf Hansson wrote:
> > > > > On Sun, 27 Oct 2019 at 03:30, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > > >
> > > > > > On Thu, Oct 24, 2019 at 06:47:43PM +0200, Ulf Hansson wrote:
> > > > > > > On Thu, 24 Oct 2019 at 18:31, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > > > > >
> > > > > > > > On Thu, Oct 10, 2019 at 01:39:34PM +0200, Ulf Hansson wrote:
> > > > > > > > > Introduce a PSCI DT helper function, psci_dt_attach_cpu(), which takes a
> > > > > > > > > CPU number as an in-parameter and tries to attach the CPU's struct device
> > > > > > > > > to its corresponding PM domain.
> > > > > > > > >
> > > > > > > > > Let's makes use of dev_pm_domain_attach_by_name(), as it allows us to
> > > > > > > > > specify "psci" as the "name" of the PM domain to attach to. Additionally,
> > > > > > > > > let's also prepare the attached device to be power managed via runtime PM.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > > > > > ---
> > > > > > > > >  drivers/cpuidle/cpuidle-psci-domain.c | 21 +++++++++++++++++++++
> > > > > > > > >  drivers/cpuidle/cpuidle-psci.h        |  6 ++++++
> > > > > > > > >  2 files changed, 27 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> > > > > > > > > index 3f5143ccc3e0..7429fd7626a1 100644
> > > > > > > > > --- a/drivers/cpuidle/cpuidle-psci-domain.c
> > > > > > > > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> > > > > > > > > @@ -9,9 +9,11 @@
> > > > > > > > >
> > > > > > > > >  #define pr_fmt(fmt) "CPUidle PSCI: " fmt
> > > > > > > > >
> > > > > > > > > +#include <linux/cpu.h>
> > > > > > > > >  #include <linux/device.h>
> > > > > > > > >  #include <linux/kernel.h>
> > > > > > > > >  #include <linux/pm_domain.h>
> > > > > > > > > +#include <linux/pm_runtime.h>
> > > > > > > > >  #include <linux/psci.h>
> > > > > > > > >  #include <linux/slab.h>
> > > > > > > > >  #include <linux/string.h>
> > > > > > > > > @@ -279,3 +281,22 @@ static int __init psci_idle_init_domains(void)
> > > > > > > > >       return ret;
> > > > > > > > >  }
> > > > > > > > >  subsys_initcall(psci_idle_init_domains);
> > > > > > > > > +
> > > > > > > > > +struct device *psci_dt_attach_cpu(int cpu)
> > > > > > > > > +{
> > > > > > > > > +     struct device *dev;
> > > > > > > > > +
> > > > > > > > > +     /* Currently limit the hierarchical topology to be used in OSI mode. */
> > > > > > > > > +     if (!psci_has_osi_support())
> > > > > > > > > +             return NULL;
> > > > > > > > > +
> > > > > > > > > +     dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
> > > > > > > >
> > > > > > > > This clarifies the need for the fixed name. But why not just go by index 0
> > > > > > > > as the consumer of these psci power-domains will have only one power domain
> > > > > > > > entry. Why do we need this name compulsory ?
> > > > > > >
> > > > > > > The idea is to be future proof. If I recall correctly, the CPU node on
> > > > > > > some QCOM SoCs may also have "CPR" PM domain specified, thus
> > > > > > > "multiple" power-domains could be specified.
> > > > > > >
> > > > > >
> > > > > > I am sure we don't want to mx-n-match any power domain provider with
> > > > > > psci. And also I expect in these above mentioned cases, there won't be any
> > > > > > psci power domains.
> > > > > >
> > > > > > > In any case, using "psci" doesn't really hurt, right?
> > > > > > >
> > > > > >
> > > > > > Doesn't but I don't see need for one as only one should exist, as mentioned
> > > > > > above we don't want mix-n-match with psci ever.
> > > > >
> > > > > Not sure I get your point, sorry.
> > > > >
> > > > > The CPU could very well be attached to more than one power-domain. Of
> > > > > course not multiple "PSCI power-domains". One could be an PSCI power
> > > > > domain and another one could be the QCOM CPR (Core power reduction)
> > > > > power domain.
> > > > >
> > > >
> > > > And who controls QCOM CPR ? If it's OSPM, this model is broken.
> > > > I mean OSPM can vote, but the control *has* to be in PSCI firmware to
> > > > change any CPU power state.
> > > >
> > > > If it's firmware controlled, then there's no need to explicitly specify
> > > > both to OSPM.
> > >
> > > This is about OPP and CPUFreq, so it has nothing to do with PSCI.
> > >
> > > >
> > > > > Have a look at these binding, there are already upstream, perhaps that
> > > > > clarifies this?
> > > > > Documentation/devicetree/bindings/opp/qcom-nvmem-cpufreq.txt
> > > > >
> > > >
> > > > OK, I will have a look.
> > >
> > > Great.
> > >
> > > I have looped in Niklas Casell, he should be able to answer any more
> > > detailed questions in regards to QCOM CPR, if that is needed.
> > >
> >
> > So had a look at the DT bindings and standalone it looks fine.
> > But when it's mixed like the way you describe: yikes!
> >
> > Why does a power(oh wait it's actually performance domain!) is combined
> > with a device whose actual power is controlled by only by PSCI/firmware
> > is associated along with another power(again actally performance)
> > domain.
> >
> > This whole representation of performance domain as power domain in the
> > bindings is a *mess*. If Linux kernel chose to implement it as part
> > of genpd, that's fine. But we should have had a separate binding for
> > that.
> >
> > > In any case, we are discussing whether we should require a
> > > power-domain-names set to "psci" for the CPU node - and I don't see
> > > how that could hurt. Right?
> > >
> >
> > Honestly I don't like this, but we don't have any choice I think.
> > So yes, but you need to update the binding. Hope new platform move
> > all these performance domain control part into firmware and have single
> > control from kernel unlike the present generation which OPP through
> > clock or cpufreq and the voltage/performance comtrol via genpd.
>
> FWIW, in newer generation Qualcomm SoCs like SDM845,
> the voltage/performance control is done in firmware,
> by the OSM (drivers/cpufreq/qcom-cpufreq-hw.c).
>

Indeed, I was implicitly referring to this platform but just wanted to be
assured that we are not going back.

--
Regards,
Sudeep