Skip to contentSkip to conversation

Enabling PSCI based idle on ARM 32-bit platforms

PATCH Superseded Lorenzo Pieralisi 2 participants 5 messages Oct 1, 2015 – Oct 2, 2015

Subsystems (5)

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

cover Enabling PSCI based idle on ARM 32-bit platforms

PSCI firmware provides a kernel API that, through a standard interface, allows to manage power states transitions in a seamless manner for ARM and ARM64 systems.

Current PSCI code that initializes CPUidle states on PSCI based systems lives in arch/arm64 directory but it is not ARM64 specific and can be shared with ARM 32-bit systems so that the generic ARM CPUidle driver can leverage the common PSCI interface.

This patch series moves PSCI CPUidle management code to drivers/firmware directory so that ARM and ARM64 architecture can actually share the code.

It is made up of two patches:

Patch 1 refactors ARM 32-bit generic idle implementation to make it compliant with ARM64 CPUidle operations, and patches the existing CPUidle implementation based on the generic CPUidle driver.

Patch 2 moves ARM64 PSCI CPUidle functions implementation to drivers/firmware so that it can be shared with ARM 32-bit platforms code. This patch also adds a PSCI entry section on ARM 32-bit systems so that the PSCI CPUidle back-end can be enabled when the enable-method corresponds to PSCI.

Patches apply on top of current patch stack to enable PSCI 1.0:

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/351470.html

Tested on Juno board (ARM64), compile tested only on ARM 32-bit systems.

Lorenzo Pieralisi (2):
ARM: cpuidle: make cpuidle_ops interfaces ARM64 compliant
ARM64: kernel: PSCI: move PSCI idle management code to
drivers/firmware

 arch/arm/include/asm/cpuidle.h |   4 +-
 arch/arm/kernel/cpuidle.c      |   4 +-
 arch/arm64/kernel/psci.c       |  99 +--------------------------------
 drivers/firmware/Makefile      |   2 +-
 drivers/firmware/psci_cpuops.c | 121 +++++++++++++++++++++++++++++++++++++++++
 drivers/soc/qcom/spm.c         |   9 ++-
 include/linux/psci.h           |   3 +
 7 files changed, 138 insertions(+), 104 deletions(-)
 create mode 100644 drivers/firmware/psci_cpuops.c
Signature · 1 line
2.5.1
#1 lore raw

PATCH 1/2 ARM: cpuidle: make cpuidle_ops interfaces ARM64 compliant

In preparation for sharing PSCI idle operations on ARM and ARM64, this patch refines the ARM cpuidle_ops interfaces so that the two arches can use the same interfaces seamlessly.

Cc Lina Iyer <[email protected]>
Cc Russell King <[email protected]>
Cc Daniel Lezcano <[email protected]>
---
 arch/arm/include/asm/cpuidle.h | 4 ++--
 arch/arm/kernel/cpuidle.c      | 4 ++--
 drivers/soc/qcom/spm.c         | 9 +++++++--
 3 files changed, 11 insertions(+), 6 deletions(-)
arch/arm/include/asm/cpuidle.h +2 -2
index 0f84249..ee36dd4 100644
@@ -30,8 +30,8 @@ static inline int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
 struct device_node;
 
 struct cpuidle_ops {
-	int (*suspend)(int cpu, unsigned long arg);
-	int (*init)(struct device_node *, int cpu);
+	int (*suspend)(unsigned long arg);
+	int (*init)(unsigned int cpu);
 };
 
 struct of_cpuidle_method {
arch/arm/kernel/cpuidle.c +2 -2
index 318da33..a75b7ae 100644
@@ -56,7 +56,7 @@ int arm_cpuidle_suspend(int index)
 	int cpu = smp_processor_id();
 
 	if (cpuidle_ops[cpu].suspend)
-		ret = cpuidle_ops[cpu].suspend(cpu, index);
+		ret = cpuidle_ops[cpu].suspend(index);
 
 	return ret;
 }
@@ -144,7 +144,7 @@ int __init arm_cpuidle_init(int cpu)
 
 	ret = arm_cpuidle_read_ops(cpu_node, cpu);
 	if (!ret && cpuidle_ops[cpu].init)
-		ret = cpuidle_ops[cpu].init(cpu_node, cpu);
+		ret = cpuidle_ops[cpu].init(cpu);
 
 	of_node_put(cpu_node);
 
drivers/soc/qcom/spm.c +7 -2
index b04b05a..8deaea0 100644
@@ -197,8 +197,9 @@ static int qcom_cpu_spc(int cpu)
 	return ret;
 }
 
-static int qcom_idle_enter(int cpu, unsigned long index)
+static int qcom_idle_enter(unsigned long index)
 {
+	int cpu = smp_processor_id();
 	return per_cpu(qcom_idle_ops, cpu)[index](cpu);
 }
 
@@ -207,7 +208,7 @@ static const struct of_device_id qcom_idle_state_match[] __initconst = {
 	{ },
 };
 
-static int __init qcom_cpuidle_init(struct device_node *cpu_node, int cpu)
+static int __init qcom_cpuidle_init(unsigned int cpu)
 {
 	const struct of_device_id *match_id;
 	struct device_node *state_node;
@@ -217,6 +218,10 @@ static int __init qcom_cpuidle_init(struct device_node *cpu_node, int cpu)
 	idle_fn *fns;
 	cpumask_t mask;
 	bool use_scm_power_down = false;
+	struct device_node *cpu_node = of_get_cpu_node(cpu, NULL);
+
+	if (!cpu_node)
+		return -ENODEV;
 
 	for (i = 0; ; i++) {
 		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
Signature · 1 line
2.5.1
#2 lore raw

PATCH 1/2 ARM: cpuidle: make cpuidle_ops interfaces ARM64 compliant

Hi Lorenzo,

Quoted 20 lines · Lorenzo Pieralisi

In preparation for sharing PSCI idle operations on ARM and ARM64, this patch refines the ARM cpuidle_ops interfaces so that the two arches can use the same interfaces seamlessly.

Cc Lina Iyer <[email protected]>
Cc Russell King <[email protected]>
Cc Daniel Lezcano <[email protected]>
---
  arch/arm/include/asm/cpuidle.h | 4 ++--
  arch/arm/kernel/cpuidle.c      | 4 ++--
  drivers/soc/qcom/spm.c         | 9 +++++++--
  3 files changed, 11 insertions(+), 6 deletions(-)
arch/arm/include/asm/cpuidle.h +0 -0
index 0f84249..ee36dd4 100644

arch/arm/include/asm/cpuidle.h static inline int arm_cpuidle_simple_enter(struct cpuidle_device *dev, · line 34

Quoted 5 lines
arch/arm/include/asm/cpuidle.h +0 -2
@@ -30,8 +30,8 @@ static inline int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
  struct device_node;
 
  struct cpuidle_ops {
-	int (*suspend)(int cpu, unsigned long arg);
-	int (*init)(struct device_node *, int cpu);

I don't know the reason why the 'cpu_psci_cpu_init_idle' signature was changed. It has before the same signature as this ops.

As in ARM, we can have more than one driver using the cpuidle_ops infrastructure, removing the struct device_node * parameter will force to add a call to of_get_cpu_node(cpu, NULL) in each driver with a call to of_node_put (btw it is missing below in qcom_cpuidle_init), the rollback labels and this will lead to duplicated code.

So why not change ARM64 signature instead ?

Quoted 63 lines
arch/arm/include/asm/cpuidle.h +2 -0
+	int (*suspend)(unsigned long arg);
+	int (*init)(unsigned int cpu);
  };
 
  struct of_cpuidle_method {
arch/arm/kernel/cpuidle.c +2 -2
index 318da33..a75b7ae 100644
@@ -56,7 +56,7 @@ int arm_cpuidle_suspend(int index)
  	int cpu = smp_processor_id();
 
  	if (cpuidle_ops[cpu].suspend)
-		ret = cpuidle_ops[cpu].suspend(cpu, index);
+		ret = cpuidle_ops[cpu].suspend(index);
 
  	return ret;
  }
@@ -144,7 +144,7 @@ int __init arm_cpuidle_init(int cpu)
 
  	ret = arm_cpuidle_read_ops(cpu_node, cpu);
  	if (!ret && cpuidle_ops[cpu].init)
-		ret = cpuidle_ops[cpu].init(cpu_node, cpu);
+		ret = cpuidle_ops[cpu].init(cpu);
 
  	of_node_put(cpu_node);
 
drivers/soc/qcom/spm.c +7 -2
index b04b05a..8deaea0 100644
@@ -197,8 +197,9 @@ static int qcom_cpu_spc(int cpu)
  	return ret;
  }
 
-static int qcom_idle_enter(int cpu, unsigned long index)
+static int qcom_idle_enter(unsigned long index)
  {
+	int cpu = smp_processor_id();
  	return per_cpu(qcom_idle_ops, cpu)[index](cpu);
  }
 
@@ -207,7 +208,7 @@ static const struct of_device_id qcom_idle_state_match[] __initconst = {
  	{ },
  };
 
-static int __init qcom_cpuidle_init(struct device_node *cpu_node, int cpu)
+static int __init qcom_cpuidle_init(unsigned int cpu)
  {
  	const struct of_device_id *match_id;
  	struct device_node *state_node;
@@ -217,6 +218,10 @@ static int __init qcom_cpuidle_init(struct device_node *cpu_node, int cpu)
  	idle_fn *fns;
  	cpumask_t mask;
  	bool use_scm_power_down = false;
+	struct device_node *cpu_node = of_get_cpu_node(cpu, NULL);
+
+	if (!cpu_node)
+		return -ENODEV;
 
  	for (i = 0; ; i++) {
  		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
Signature · 5 lines
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
#3 lore raw

PATCH 1/2 ARM: cpuidle: make cpuidle_ops interfaces ARM64 compliant

On Fri, Oct 02, 2015 at 10:37:58AM +0100, Daniel Lezcano wrote:

[...]

Quoted 7 lines
Quoted 7 lines
arch/arm/include/asm/cpuidle.h +0 -0
index 0f84249..ee36dd4 100644
@@ -30,8 +30,8 @@ static inline int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
  struct device_node;
 
Quoted 6 lines

arch/arm/include/asm/cpuidle.h static inline int arm_cpuidle_simple_enter(struct cpuidle_device *dev, · line 34

Quoted 3 lines
arch/arm/include/asm/cpuidle.h +0 -2
  struct cpuidle_ops {
-	int (*suspend)(int cpu, unsigned long arg);
-	int (*init)(struct device_node *, int cpu);

I don't know the reason why the 'cpu_psci_cpu_init_idle' signature was changed. It has before the same signature as this ops.

Short answer, ACPI (ie device_node is useless on ACPI systems, so there is no point in passing it).

Quoted 7 lines

As in ARM, we can have more than one driver using the cpuidle_ops infrastructure, removing the struct device_node * parameter will force to add a call to of_get_cpu_node(cpu, NULL) in each driver with a call to of_node_put (btw it is missing below in qcom_cpuidle_init), the rollback labels and this will lead to duplicated code.

So why not change ARM64 signature instead ?

See above, I will see what I can do and I understand your concern, it makes sense.

Thanks,
Lorenzo

#4 lore raw

PATCH 2/2 ARM64: kernel: PSCI: move PSCI idle management code to drivers/firmware

In reply to Lorenzo Pieralisi

ARM64 PSCI kernel interfaces that initialize idle states and implement the suspend API to enter them are generic and can be shared with the ARM architecture.

To achieve that goal, this patch moves ARM64 PSCI idle management code to drivers/firmware by creating a file that contains PSCI helper functions implementing the common kernel interface required by ARM and ARM64 to share the PSCI idle management code.

The ARM generic CPUidle implementation also requires the definition of a cpuidle_ops section entry for the kernel to initialize the CPUidle operations at boot based on the enable-method (ie ARM64 has the statically initialized cpu_ops counterparts for that purpose); therefore this patch also adds the required section entry on CONFIG_ARM for PSCI so that the kernel can initialize the PSCI CPUidle back-end when PSCI is the probed enable-method.

On ARM64 this patch provides no functional change.

Cc Will Deacon <[email protected]>
Cc Sudeep Holla <[email protected]>
Cc Russell King <[email protected]>
Cc Daniel Lezcano <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Jisheng Zhang <[email protected]>
---
 arch/arm64/kernel/psci.c       |  99 +--------------------------------
 drivers/firmware/Makefile      |   2 +-
 drivers/firmware/psci_cpuops.c | 121 +++++++++++++++++++++++++++++++++++++++++
 include/linux/psci.h           |   3 +
 4 files changed, 127 insertions(+), 98 deletions(-)
 create mode 100644 drivers/firmware/psci_cpuops.c
arch/arm64/kernel/psci.c +2 -97
index f67f35b..42816be 100644
@@ -20,7 +20,6 @@
 #include <linux/smp.h>
 #include <linux/delay.h>
 #include <linux/psci.h>
-#include <linux/slab.h>
 
 #include <uapi/linux/psci.h>
 
@@ -28,73 +27,6 @@
 #include <asm/cpu_ops.h>
 #include <asm/errno.h>
 #include <asm/smp_plat.h>
-#include <asm/suspend.h>
-
-static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
-
-static int __maybe_unused cpu_psci_cpu_init_idle(unsigned int cpu)
-{
-	int i, ret, count = 0;
-	u32 *psci_states;
-	struct device_node *state_node, *cpu_node;
-
-	cpu_node = of_get_cpu_node(cpu, NULL);
-	if (!cpu_node)
-		return -ENODEV;
-
-	/*
-	 * If the PSCI cpu_suspend function hook has not been initialized
-	 * idle states must not be enabled, so bail out
-	 */
-	if (!psci_ops.cpu_suspend)
-		return -EOPNOTSUPP;
-
-	/* 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);
-	if (!psci_states)
-		return -ENOMEM;
-
-	for (i = 0; i < count; i++) {
-		u32 state;
-
-		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
-
-		ret = of_property_read_u32(state_node,
-					   "arm,psci-suspend-param",
-					   &state);
-		if (ret) {
-			pr_warn(" * %s missing arm,psci-suspend-param property\n",
-				state_node->full_name);
-			of_node_put(state_node);
-			goto free_mem;
-		}
-
-		of_node_put(state_node);
-		pr_debug("psci-power-state %#x index %d\n", state, i);
-		if (!psci_power_state_is_valid(state)) {
-			pr_warn("Invalid PSCI power state %#x\n", state);
-			ret = -EINVAL;
-			goto free_mem;
-		}
-		psci_states[i] = state;
-	}
-	/* Idle states parsed correctly, initialize per-cpu pointer */
-	per_cpu(psci_power_state, cpu) = psci_states;
-	return 0;
-
-free_mem:
-	kfree(psci_states);
-	return ret;
-}
 
 static int __init cpu_psci_cpu_init(unsigned int cpu)
 {
@@ -178,38 +110,11 @@ static int cpu_psci_cpu_kill(unsigned int cpu)
 }
 #endif
 
-static int psci_suspend_finisher(unsigned long index)
-{
-	u32 *state = __this_cpu_read(psci_power_state);
-
-	return psci_ops.cpu_suspend(state[index - 1],
-				    virt_to_phys(cpu_resume));
-}
-
-static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index)
-{
-	int ret;
-	u32 *state = __this_cpu_read(psci_power_state);
-	/*
-	 * idle state index 0 corresponds to wfi, should never be called
-	 * from the cpu_suspend operations
-	 */
-	if (WARN_ON_ONCE(!index))
-		return -EINVAL;
-
-	if (!psci_power_state_loses_context(state[index - 1]))
-		ret = psci_ops.cpu_suspend(state[index - 1], 0);
-	else
-		ret = cpu_suspend(index, psci_suspend_finisher);
-
-	return ret;
-}
-
 const struct cpu_operations cpu_psci_ops = {
 	.name		= "psci",
 #ifdef CONFIG_CPU_IDLE
-	.cpu_init_idle	= cpu_psci_cpu_init_idle,
-	.cpu_suspend	= cpu_psci_cpu_suspend,
+	.cpu_init_idle	= psci_cpu_init_idle,
+	.cpu_suspend	= psci_cpu_suspend_enter,
 #endif
 	.cpu_init	= cpu_psci_cpu_init,
 	.cpu_prepare	= cpu_psci_cpu_prepare,
drivers/firmware/Makefile +1 -1
index 000830f..77553ab 100644
@@ -1,7 +1,7 @@
 #
 # Makefile for the linux kernel.
 #
-obj-$(CONFIG_ARM_PSCI_FW)	+= psci.o
+obj-$(CONFIG_ARM_PSCI_FW)	+= psci.o psci_cpuops.o
 obj-$(CONFIG_DMI)		+= dmi_scan.o
 obj-$(CONFIG_DMI_SYSFS)		+= dmi-sysfs.o
 obj-$(CONFIG_EDD)		+= edd.o
drivers/firmware/psci_cpuops.c new file +121 -0

121 lines not on this page · raw message

include/linux/psci.h +3 -0
index 12c4865..393efe2 100644
@@ -24,6 +24,9 @@ bool psci_tos_resident_on(int cpu);
 bool psci_power_state_loses_context(u32 state);
 bool psci_power_state_is_valid(u32 state);
 
+int psci_cpu_init_idle(unsigned int cpu);
+int psci_cpu_suspend_enter(unsigned long index);
+
 struct psci_operations {
 	int (*cpu_suspend)(u32 state, unsigned long entry_point);
 	int (*cpu_off)(u32 state);
Signature · 1 line
2.5.1