[PATCH v3 0/2] Enabling PSCI based idle on ARM 32-bit platforms

[PATCH v3 0/2] Enabling PSCI based idle on ARM 32-bit platforms

[Lorenzo Pieralisi]

This patch series is v3 of a previous posting:

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-October/376418.html

v2->v3

- Removed additional PSCI file, merged functions in psci.c
- Added missing static scope to ARM psci_cpuidle_ops
- Applied review tags

v1->v2:

- Refactored patch 1 to remove cpu parameter from cpuidle_ops
  suspend hook
- Refactored psci_cpu_init_idle to stub out dt parsing function and
  make it usable on both ARM/ARM64 with no additional changes
- Updated ARM cpuidle_ops to new interfaces

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 remove
the cpu parameter from the cpuidle_ops suspend hook, in preparation
for a common PSCI implementation for ARM/ARM64 PSCI idle.

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:

git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/linux.git tags/firmware/psci-1.0

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

Lorenzo Pieralisi (2):
  ARM: cpuidle: remove cpu parameter from the cpuidle_ops suspend hook
  ARM64: kernel: PSCI: move PSCI idle management code to
    drivers/firmware

 arch/arm/include/asm/cpuidle.h |   2 +-
 arch/arm/kernel/cpuidle.c      |   2 +-
 arch/arm64/kernel/psci.c       |  99 +---------------------------------
 drivers/firmware/psci.c        | 119 +++++++++++++++++++++++++++++++++++++++++
 drivers/soc/qcom/spm.c         |  10 ++--
 include/linux/psci.h           |   3 ++
 6 files changed, 131 insertions(+), 104 deletions(-)

-- 
2.5.1

[PATCH v3 1/2] ARM: cpuidle: remove cpu parameter from the cpuidle_ops suspend hook

[Lorenzo Pieralisi]

The suspend() hook in the cpuidle_ops struct is always called on
the cpu entering idle, which means that the cpu parameter passed
to the suspend hook always corresponds to the local cpu, making
it somewhat redundant.

This patch removes the logical cpu parameter from the ARM
cpuidle_ops.suspend hook and updates all the existing kernel
implementations to reflect this change.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Reviewed-by: Lina Iyer <lina.iyer@linaro.org>
Tested-by: Lina Iyer <lina.iyer@linaro.org>
Tested-by: Jisheng Zhang <jszhang@marvell.com> [psci]
Cc: Lina Iyer <lina.iyer@linaro.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm/include/asm/cpuidle.h |  2 +-
 arch/arm/kernel/cpuidle.c      |  2 +-
 drivers/soc/qcom/spm.c         | 10 +++++-----
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h
index 0f84249..3848259 100644
--- a/arch/arm/include/asm/cpuidle.h
+++ b/arch/arm/include/asm/cpuidle.h
@@ -30,7 +30,7 @@ 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 (*suspend)(unsigned long arg);
 	int (*init)(struct device_node *, int cpu);
 };
 
diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
index 318da33..703926e 100644
--- a/arch/arm/kernel/cpuidle.c
+++ b/arch/arm/kernel/cpuidle.c
@@ -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;
 }
diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
index b04b05a..0ad66fa 100644
--- a/drivers/soc/qcom/spm.c
+++ b/drivers/soc/qcom/spm.c
@@ -116,7 +116,7 @@ static const struct spm_reg_data spm_reg_8064_cpu = {
 
 static DEFINE_PER_CPU(struct spm_driver_data *, cpu_spm_drv);
 
-typedef int (*idle_fn)(int);
+typedef int (*idle_fn)(void);
 static DEFINE_PER_CPU(idle_fn*, qcom_idle_ops);
 
 static inline void spm_register_write(struct spm_driver_data *drv,
@@ -179,10 +179,10 @@ static int qcom_pm_collapse(unsigned long int unused)
 	return -1;
 }
 
-static int qcom_cpu_spc(int cpu)
+static int qcom_cpu_spc(void)
 {
 	int ret;
-	struct spm_driver_data *drv = per_cpu(cpu_spm_drv, cpu);
+	struct spm_driver_data *drv = __this_cpu_read(cpu_spm_drv);
 
 	spm_set_low_power_mode(drv, PM_SLEEP_MODE_SPC);
 	ret = cpu_suspend(0, qcom_pm_collapse);
@@ -197,9 +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)
 {
-	return per_cpu(qcom_idle_ops, cpu)[index](cpu);
+	return __this_cpu_read(qcom_idle_ops)[index]();
 }
 
 static const struct of_device_id qcom_idle_state_match[] __initconst = {
-- 
2.5.1

[PATCH v3 1/2] ARM: cpuidle: remove cpu parameter from the cpuidle_ops suspend hook

[Daniel Lezcano]

On 10/16/2015 06:02 PM, Lorenzo Pieralisi wrote:
> The suspend() hook in the cpuidle_ops struct is always called on
> the cpu entering idle, which means that the cpu parameter passed
> to the suspend hook always corresponds to the local cpu, making
> it somewhat redundant.
>
> This patch removes the logical cpu parameter from the ARM
> cpuidle_ops.suspend hook and updates all the existing kernel
> implementations to reflect this change.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Reviewed-by: Lina Iyer <lina.iyer@linaro.org>
> Tested-by: Lina Iyer <lina.iyer@linaro.org>
> Tested-by: Jisheng Zhang <jszhang@marvell.com> [psci]
> Cc: Lina Iyer <lina.iyer@linaro.org>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

-- 
  <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

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

[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, so that the interface to initialize and
enter idle states can actually be shared by ARM and ARM64 arches
back-ends.

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.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Acked-by: Catalin Marinas <catalin.marinas@arm.com> [arch/arm64]
Acked-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Jisheng Zhang <jszhang@marvell.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Jisheng Zhang <jszhang@marvell.com>
---
 arch/arm64/kernel/psci.c |  99 +--------------------------------------
 drivers/firmware/psci.c  | 119 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/psci.h     |   3 ++
 3 files changed, 124 insertions(+), 97 deletions(-)

diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
index f67f35b..42816be 100644
--- a/arch/arm64/kernel/psci.c
+++ b/arch/arm64/kernel/psci.c
@@ -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,
diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
index 492db42..73046de 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -13,17 +13,21 @@
 
 #define pr_fmt(fmt) "psci: " fmt
 
+#include <linux/cpuidle.h>
 #include <linux/errno.h>
 #include <linux/linkage.h>
 #include <linux/of.h>
+#include <linux/percpu.h>
 #include <linux/pm.h>
 #include <linux/printk.h>
 #include <linux/psci.h>
 #include <linux/reboot.h>
+#include <linux/slab.h>
 #include <linux/suspend.h>
 
 #include <uapi/linux/psci.h>
 
+#include <asm/cpuidle.h>
 #include <asm/cputype.h>
 #include <asm/system_misc.h>
 #include <asm/smp_plat.h>
@@ -225,6 +229,121 @@ static int __init psci_features(u32 psci_func_id)
 			      psci_func_id, 0, 0);
 }
 
+static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
+
+static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
+{
+	int i, ret, count = 0;
+	u32 *psci_states;
+	struct device_node *state_node;
+
+	/*
+	 * 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;
+}
+
+int psci_cpu_init_idle(unsigned int cpu)
+{
+	struct device_node *cpu_node;
+	int ret;
+
+	cpu_node = of_get_cpu_node(cpu, NULL);
+	if (!cpu_node)
+		return -ENODEV;
+
+	ret = psci_dt_cpu_init_idle(cpu_node, cpu);
+
+	of_node_put(cpu_node);
+
+	return ret;
+}
+
+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));
+}
+
+int psci_cpu_suspend_enter(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;
+}
+
+/* ARM specific CPU idle operations */
+#ifdef CONFIG_ARM
+static struct cpuidle_ops psci_cpuidle_ops __initdata = {
+	.suspend = psci_cpu_suspend_enter,
+	.init = psci_dt_cpu_init_idle,
+};
+
+CPUIDLE_METHOD_OF_DECLARE(psci, "arm,psci", &psci_cpuidle_ops);
+#endif
+
 static int psci_system_suspend(unsigned long unused)
 {
 	return invoke_psci_fn(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND),
diff --git a/include/linux/psci.h b/include/linux/psci.h
index 12c4865..393efe2 100644
--- a/include/linux/psci.h
+++ b/include/linux/psci.h
@@ -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);
-- 
2.5.1

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

[Daniel Lezcano]

On 10/16/2015 06:02 PM, Lorenzo Pieralisi wrote:
> 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, so that the interface to initialize and
> enter idle states can actually be shared by ARM and ARM64 arches
> back-ends.
>
> 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.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com> [arch/arm64]
> Acked-by: Mark Rutland <mark.rutland@arm.com>
> Tested-by: Jisheng Zhang <jszhang@marvell.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Jisheng Zhang <jszhang@marvell.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>


-- 
  <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

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

[Russell King - ARM Linux]

On Fri, Oct 16, 2015 at 05:02:59PM +0100, Lorenzo Pieralisi wrote:
> 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, so that the interface to initialize and
> enter idle states can actually be shared by ARM and ARM64 arches
> back-ends.
> 
> 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.

On ARM64, it causes build breakage though:

drivers/built-in.o: In function `psci_suspend_finisher':
arm_pmu.c:(.text+0xc6494): undefined reference to `cpu_resume'
arm_pmu.c:(.text+0xc6498): undefined reference to `cpu_resume'
drivers/built-in.o: In function `psci_cpu_suspend_enter':
arm_pmu.c:(.text+0xc66c0): undefined reference to `cpu_suspend'

The code which has been moved looks similar.  However, when it lived
in arch/arm64/kernel/psci.c, it was protected by
#ifdef CONFIG_HOTPLUG_CPU.  In its new location, there are no ifdefs
around it, and so if it gets built without CONFIG_ARM_CPU_SUSPEND on
ARM, or CONFIG_CPU_PM for ARM64, it will error out like the above.

As this is causing a regression, and I've now closed my tree, I will
be doing what I said yesterday: I'll be dropping this patch for this
merge window in order to stabilise my tree.  Sorry.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

[Lorenzo Pieralisi]

On Tue, Jan 05, 2016 at 10:59:01AM +0000, Russell King - ARM Linux wrote:
> On Fri, Oct 16, 2015 at 05:02:59PM +0100, Lorenzo Pieralisi wrote:
> > 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, so that the interface to initialize and
> > enter idle states can actually be shared by ARM and ARM64 arches
> > back-ends.
> > 
> > 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.
> 
> On ARM64, it causes build breakage though:
> 
> drivers/built-in.o: In function `psci_suspend_finisher':
> arm_pmu.c:(.text+0xc6494): undefined reference to `cpu_resume'
> arm_pmu.c:(.text+0xc6498): undefined reference to `cpu_resume'
> drivers/built-in.o: In function `psci_cpu_suspend_enter':
> arm_pmu.c:(.text+0xc66c0): undefined reference to `cpu_suspend'
> 
> The code which has been moved looks similar.  However, when it lived
> in arch/arm64/kernel/psci.c, it was protected by
> #ifdef CONFIG_HOTPLUG_CPU.  In its new location, there are no ifdefs
> around it, and so if it gets built without CONFIG_ARM_CPU_SUSPEND on
> ARM, or CONFIG_CPU_PM for ARM64, it will error out like the above.
> 
> As this is causing a regression, and I've now closed my tree, I will
> be doing what I said yesterday: I'll be dropping this patch for this
> merge window in order to stabilise my tree.  Sorry.

My bad, I apologise, I will likely have to add a config option to
make sure cpu_{suspend/resume} code is compiled in (on both ARM/ARM64),
thanks for spotting it.

Lorenzo

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

[Russell King - ARM Linux]

On Tue, Jan 05, 2016 at 12:31:34PM +0000, Lorenzo Pieralisi wrote:
> On Tue, Jan 05, 2016 at 10:59:01AM +0000, Russell King - ARM Linux wrote:
> > On Fri, Oct 16, 2015 at 05:02:59PM +0100, Lorenzo Pieralisi wrote:
> > > 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, so that the interface to initialize and
> > > enter idle states can actually be shared by ARM and ARM64 arches
> > > back-ends.
> > > 
> > > 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.
> > 
> > On ARM64, it causes build breakage though:
> > 
> > drivers/built-in.o: In function `psci_suspend_finisher':
> > arm_pmu.c:(.text+0xc6494): undefined reference to `cpu_resume'
> > arm_pmu.c:(.text+0xc6498): undefined reference to `cpu_resume'
> > drivers/built-in.o: In function `psci_cpu_suspend_enter':
> > arm_pmu.c:(.text+0xc66c0): undefined reference to `cpu_suspend'
> > 
> > The code which has been moved looks similar.  However, when it lived
> > in arch/arm64/kernel/psci.c, it was protected by
> > #ifdef CONFIG_HOTPLUG_CPU.  In its new location, there are no ifdefs
> > around it, and so if it gets built without CONFIG_ARM_CPU_SUSPEND on
> > ARM, or CONFIG_CPU_PM for ARM64, it will error out like the above.
> > 
> > As this is causing a regression, and I've now closed my tree, I will
> > be doing what I said yesterday: I'll be dropping this patch for this
> > merge window in order to stabilise my tree.  Sorry.
> 
> My bad, I apologise, I will likely have to add a config option to
> make sure cpu_{suspend/resume} code is compiled in (on both ARM/ARM64),
> thanks for spotting it.

On ARM, that option is CONFIG_ARM_CPU_SUSPEND - that option solely
controls whether cpu_suspend/resume are present.  ARM64 just needs
to adopt this, and use that to control the code which is built in
drivers/firmware/psci.c.

However, I don't think it's as simple as just adding that to ARM64,
as you need to be careful of the Kconfig dependencies.  For ARM,
this is:

Generic code:
- SUSPEND defaults to y, depends on ARCH_SUSPEND_POSSIBLE (which is set for
   any cpu_suspend enabled CPU.)
- PM_SLEEP if SUSPEND || HIBERNATE_CALLBACKS
ARM sets:
- CPU_PM if SUSPEND || CPU_IDLE.
- ARM_CPU_SUSPEND if PM_SLEEP || BL_SWITCHER || (ARCH_PXA && PM)

What this means is that CPU_PM is entirely independent of
ARM_CPU_SUSPEND.  One does not imply the other, so I think you need
to consider carefully what ifdef you need in drivers/firmware/psci.c.

This is why I think fixing this is not simple as it first looks.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

[Lorenzo Pieralisi]

On Tue, Jan 05, 2016 at 12:51:42PM +0000, Russell King - ARM Linux wrote:

[...]

> > > > On ARM64 this patch provides no functional change.
> > > 
> > > On ARM64, it causes build breakage though:
> > > 
> > > drivers/built-in.o: In function `psci_suspend_finisher':
> > > arm_pmu.c:(.text+0xc6494): undefined reference to `cpu_resume'
> > > arm_pmu.c:(.text+0xc6498): undefined reference to `cpu_resume'
> > > drivers/built-in.o: In function `psci_cpu_suspend_enter':
> > > arm_pmu.c:(.text+0xc66c0): undefined reference to `cpu_suspend'
> > > 
> > > The code which has been moved looks similar.  However, when it lived
> > > in arch/arm64/kernel/psci.c, it was protected by
> > > #ifdef CONFIG_HOTPLUG_CPU.  In its new location, there are no ifdefs
> > > around it, and so if it gets built without CONFIG_ARM_CPU_SUSPEND on
> > > ARM, or CONFIG_CPU_PM for ARM64, it will error out like the above.
> > > 
> > > As this is causing a regression, and I've now closed my tree, I will
> > > be doing what I said yesterday: I'll be dropping this patch for this
> > > merge window in order to stabilise my tree.  Sorry.
> > 
> > My bad, I apologise, I will likely have to add a config option to
> > make sure cpu_{suspend/resume} code is compiled in (on both ARM/ARM64),
> > thanks for spotting it.
> 
> On ARM, that option is CONFIG_ARM_CPU_SUSPEND - that option solely
> controls whether cpu_suspend/resume are present.  ARM64 just needs
> to adopt this, and use that to control the code which is built in
> drivers/firmware/psci.c.
> 
> However, I don't think it's as simple as just adding that to ARM64,
> as you need to be careful of the Kconfig dependencies.  For ARM,
> this is:
> 
> Generic code:
> - SUSPEND defaults to y, depends on ARCH_SUSPEND_POSSIBLE (which is set for
>    any cpu_suspend enabled CPU.)
> - PM_SLEEP if SUSPEND || HIBERNATE_CALLBACKS
> ARM sets:
> - CPU_PM if SUSPEND || CPU_IDLE.
> - ARM_CPU_SUSPEND if PM_SLEEP || BL_SWITCHER || (ARCH_PXA && PM)
> 
> What this means is that CPU_PM is entirely independent of
> ARM_CPU_SUSPEND.  One does not imply the other, so I think you need
> to consider carefully what ifdef you need in drivers/firmware/psci.c.
> 
> This is why I think fixing this is not simple as it first looks.

Not saying it is nice, but unless I find a cleaner way I was keener on
adding a silent config entry in drivers/firmware, say:

config ARM_PSCI_CPU_IDLE
	def_bool ARM_PSCI_FW && CPU_IDLE
	select ARM_CPU_SUSPEND if ARM

and use that to either guard the code or stub it out and compile it
if that config option is enabled.

I will post a v4 at -rc1.

Thanks,
Lorenzo

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

[Russell King - ARM Linux]

On Tue, Jan 05, 2016 at 01:27:01PM +0000, Lorenzo Pieralisi wrote:
> On Tue, Jan 05, 2016 at 12:51:42PM +0000, Russell King - ARM Linux wrote:
> > On ARM, that option is CONFIG_ARM_CPU_SUSPEND - that option solely
> > controls whether cpu_suspend/resume are present.  ARM64 just needs
> > to adopt this, and use that to control the code which is built in
> > drivers/firmware/psci.c.
> > 
> > However, I don't think it's as simple as just adding that to ARM64,
> > as you need to be careful of the Kconfig dependencies.  For ARM,
> > this is:
> > 
> > Generic code:
> > - SUSPEND defaults to y, depends on ARCH_SUSPEND_POSSIBLE (which is set for
> >    any cpu_suspend enabled CPU.)
> > - PM_SLEEP if SUSPEND || HIBERNATE_CALLBACKS
> > ARM sets:
> > - CPU_PM if SUSPEND || CPU_IDLE.
> > - ARM_CPU_SUSPEND if PM_SLEEP || BL_SWITCHER || (ARCH_PXA && PM)
> > 
> > What this means is that CPU_PM is entirely independent of
> > ARM_CPU_SUSPEND.  One does not imply the other, so I think you need
> > to consider carefully what ifdef you need in drivers/firmware/psci.c.
> > 
> > This is why I think fixing this is not simple as it first looks.
> 
> Not saying it is nice, but unless I find a cleaner way I was keener on
> adding a silent config entry in drivers/firmware, say:
> 
> config ARM_PSCI_CPU_IDLE
> 	def_bool ARM_PSCI_FW && CPU_IDLE
> 	select ARM_CPU_SUSPEND if ARM
> 
> and use that to either guard the code or stub it out and compile it
> if that config option is enabled.

That immediately worries me, because it bypasses the CPU dependencies
for ARM_CPU_SUSPEND implicitly applied via ARCH_SUSPEND_POSSIBLE.  I'd
prefer instead:

config ARM_PSCI_CPU_IDLE
	def_bool ARM_PSCI_FW && CPU_IDLE && (!ARM || ARM_CPU_SUSPEND)

Really, the answer is to stop ARM64 diverging from ARM, so we stop
having these architecture conditionals all over the place.  If ARM64
builds its cpu_suspend code in the same way that ARM does (iow,
keyed off ARM_CPU_SUSPEND, which it can select), then we end up
with the above being:

config ARM_PSCI_CPU_IDLE
	def_bool ARM_PSCI_FW && CPU_IDLE && ARM_CPU_SUSPEND

which is a helll of a lot simpler.  The dependency on ARM_PSCI_FW
could be regarded as redundant if we're only using ARM_PSCI_CPU_IDLE
to control code built within drivers/firmware/psci.c, as that won't
be built unless ARM_PSCI_FW is set.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

[Lorenzo Pieralisi]

On Tue, Jan 05, 2016 at 01:34:47PM +0000, Russell King - ARM Linux wrote:
> On Tue, Jan 05, 2016 at 01:27:01PM +0000, Lorenzo Pieralisi wrote:
> > On Tue, Jan 05, 2016 at 12:51:42PM +0000, Russell King - ARM Linux wrote:
> > > On ARM, that option is CONFIG_ARM_CPU_SUSPEND - that option solely
> > > controls whether cpu_suspend/resume are present.  ARM64 just needs
> > > to adopt this, and use that to control the code which is built in
> > > drivers/firmware/psci.c.
> > > 
> > > However, I don't think it's as simple as just adding that to ARM64,
> > > as you need to be careful of the Kconfig dependencies.  For ARM,
> > > this is:
> > > 
> > > Generic code:
> > > - SUSPEND defaults to y, depends on ARCH_SUSPEND_POSSIBLE (which is set for
> > >    any cpu_suspend enabled CPU.)
> > > - PM_SLEEP if SUSPEND || HIBERNATE_CALLBACKS
> > > ARM sets:
> > > - CPU_PM if SUSPEND || CPU_IDLE.
> > > - ARM_CPU_SUSPEND if PM_SLEEP || BL_SWITCHER || (ARCH_PXA && PM)
> > > 
> > > What this means is that CPU_PM is entirely independent of
> > > ARM_CPU_SUSPEND.  One does not imply the other, so I think you need
> > > to consider carefully what ifdef you need in drivers/firmware/psci.c.
> > > 
> > > This is why I think fixing this is not simple as it first looks.
> > 
> > Not saying it is nice, but unless I find a cleaner way I was keener on
> > adding a silent config entry in drivers/firmware, say:
> > 
> > config ARM_PSCI_CPU_IDLE
> > 	def_bool ARM_PSCI_FW && CPU_IDLE
> > 	select ARM_CPU_SUSPEND if ARM
> > 
> > and use that to either guard the code or stub it out and compile it
> > if that config option is enabled.
> 
> That immediately worries me, because it bypasses the CPU dependencies
> for ARM_CPU_SUSPEND implicitly applied via ARCH_SUSPEND_POSSIBLE.  I'd
> prefer instead:
> 
> config ARM_PSCI_CPU_IDLE
> 	def_bool ARM_PSCI_FW && CPU_IDLE && (!ARM || ARM_CPU_SUSPEND)

Ok, I think the above is reasonable, only question I have is if on ARM:

CONFIG_SUSPEND=n
CONFIG_CPU_IDLE=y

this would imply:

CONFIG_ARM_CPU_SUSPEND=n => ARM_PSCI_CPU_IDLE=n

(which is a questionable setup but possible) we can't do PSCI based
CPUidle (I agree with you that bypassing the dependencies is not ideal
or correct in the first place though), that's a problem for all
subsystems "selecting" ARM_CPU_SUSPEND.

> Really, the answer is to stop ARM64 diverging from ARM, so we stop
> having these architecture conditionals all over the place.  If ARM64
> builds its cpu_suspend code in the same way that ARM does (iow,
> keyed off ARM_CPU_SUSPEND, which it can select), then we end up
> with the above being:
> 
> config ARM_PSCI_CPU_IDLE
> 	def_bool ARM_PSCI_FW && CPU_IDLE && ARM_CPU_SUSPEND

Yes, we could do that, on ARM64 should be = SUSPEND || CPU_IDLE,
ergo the value of CPU_PM on ARM64, that's why we do not have another
config entry at present.

> which is a helll of a lot simpler.  The dependency on ARM_PSCI_FW
> could be regarded as redundant if we're only using ARM_PSCI_CPU_IDLE
> to control code built within drivers/firmware/psci.c, as that won't
> be built unless ARM_PSCI_FW is set.

Yep, that's a good point and I will remove ARM_PSCI_FW from the first
option you provided above.

Thanks,
Lorenzo

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

[Lorenzo Pieralisi]

On Tue, Jan 05, 2016 at 01:34:47PM +0000, Russell King - ARM Linux wrote:
> On Tue, Jan 05, 2016 at 01:27:01PM +0000, Lorenzo Pieralisi wrote:
> > On Tue, Jan 05, 2016 at 12:51:42PM +0000, Russell King - ARM Linux wrote:
> > > On ARM, that option is CONFIG_ARM_CPU_SUSPEND - that option solely
> > > controls whether cpu_suspend/resume are present.  ARM64 just needs
> > > to adopt this, and use that to control the code which is built in
> > > drivers/firmware/psci.c.
> > > 
> > > However, I don't think it's as simple as just adding that to ARM64,
> > > as you need to be careful of the Kconfig dependencies.  For ARM,
> > > this is:
> > > 
> > > Generic code:
> > > - SUSPEND defaults to y, depends on ARCH_SUSPEND_POSSIBLE (which is set for
> > >    any cpu_suspend enabled CPU.)
> > > - PM_SLEEP if SUSPEND || HIBERNATE_CALLBACKS
> > > ARM sets:
> > > - CPU_PM if SUSPEND || CPU_IDLE.
> > > - ARM_CPU_SUSPEND if PM_SLEEP || BL_SWITCHER || (ARCH_PXA && PM)
> > > 
> > > What this means is that CPU_PM is entirely independent of
> > > ARM_CPU_SUSPEND.  One does not imply the other, so I think you need
> > > to consider carefully what ifdef you need in drivers/firmware/psci.c.
> > > 
> > > This is why I think fixing this is not simple as it first looks.
> > 
> > Not saying it is nice, but unless I find a cleaner way I was keener on
> > adding a silent config entry in drivers/firmware, say:
> > 
> > config ARM_PSCI_CPU_IDLE
> > 	def_bool ARM_PSCI_FW && CPU_IDLE
> > 	select ARM_CPU_SUSPEND if ARM
> > 
> > and use that to either guard the code or stub it out and compile it
> > if that config option is enabled.
> 
> That immediately worries me, because it bypasses the CPU dependencies
> for ARM_CPU_SUSPEND implicitly applied via ARCH_SUSPEND_POSSIBLE.  I'd
> prefer instead:
> 
> config ARM_PSCI_CPU_IDLE
> 	def_bool ARM_PSCI_FW && CPU_IDLE && (!ARM || ARM_CPU_SUSPEND)

If you are not against it, I could make ARM_PSCI select ARM_CPU_SUSPEND,
the CPU dependency would be taken into account (ie CPU_V7) and this
would mirror what's done for the eg BL_SWITCHER.

This would remove the need for ARM_PSCI_CPU_IDLE above altogether and
I could guard the code in drivers/firmware/psci.c through CONFIG_CPU_IDLE.

It is just an option, please let me know.

Thanks,
Lorenzo

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

[Russell King - ARM Linux]

On Wed, Jan 06, 2016 at 04:55:45PM +0000, Lorenzo Pieralisi wrote:
> On Tue, Jan 05, 2016 at 01:34:47PM +0000, Russell King - ARM Linux wrote:
> > That immediately worries me, because it bypasses the CPU dependencies
> > for ARM_CPU_SUSPEND implicitly applied via ARCH_SUSPEND_POSSIBLE.  I'd
> > prefer instead:
> > 
> > config ARM_PSCI_CPU_IDLE
> > 	def_bool ARM_PSCI_FW && CPU_IDLE && (!ARM || ARM_CPU_SUSPEND)
> 
> If you are not against it, I could make ARM_PSCI select ARM_CPU_SUSPEND,
> the CPU dependency would be taken into account (ie CPU_V7) and this
> would mirror what's done for the eg BL_SWITCHER.

If you're proposing to always build the code in psci.c when ARM_PSCI_FW
is enabled, I'd rather do this:

config ARM_CPU_SUSPEND
        def_bool PM_SLEEP || BL_SWITCHER || ARM_PSCI_FW
	depends on ARCH_SUSPEND_POSSIBLE

rather than have stuff select this option.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

[Lorenzo Pieralisi]

On Wed, Jan 06, 2016 at 09:44:39PM +0000, Russell King - ARM Linux wrote:
> On Wed, Jan 06, 2016 at 04:55:45PM +0000, Lorenzo Pieralisi wrote:
> > On Tue, Jan 05, 2016 at 01:34:47PM +0000, Russell King - ARM Linux wrote:
> > > That immediately worries me, because it bypasses the CPU dependencies
> > > for ARM_CPU_SUSPEND implicitly applied via ARCH_SUSPEND_POSSIBLE.  I'd
> > > prefer instead:
> > > 
> > > config ARM_PSCI_CPU_IDLE
> > > 	def_bool ARM_PSCI_FW && CPU_IDLE && (!ARM || ARM_CPU_SUSPEND)
> > 
> > If you are not against it, I could make ARM_PSCI select ARM_CPU_SUSPEND,
> > the CPU dependency would be taken into account (ie CPU_V7) and this
> > would mirror what's done for the eg BL_SWITCHER.
> 
> If you're proposing to always build the code in psci.c when ARM_PSCI_FW
> is enabled, I'd rather do this:
> 
> config ARM_CPU_SUSPEND
>         def_bool PM_SLEEP || BL_SWITCHER || ARM_PSCI_FW
> 	depends on ARCH_SUSPEND_POSSIBLE
> 
> rather than have stuff select this option.

Agreed, I will do that with two patches (ie one to update the
BL_SWITCHER config entry). It has the side effect of pulling in
ARM_CPU_SUSPEND even if !SUSPEND && !CPU_IDLE when ARM_PSCI
is selected but I do not think that's a real issue, to be confirmed.

Still, some ARM CPUidle drivers will have to select ARM_CPU_SUSPEND
on a case by case basis, I do not know how we can improve that, but
that's not related to this patch series per-se.

Thanks,
Lorenzo