[PATCH 0/4] arm: psci: implement cpuidle_ops

[PATCH 0/4] arm: psci: implement cpuidle_ops

[Jisheng Zhang]

We'd like to use cpuidle-arm.c for both arm and arm64 with psci as backend.
For arm64, it works. But for arm, we miss cpuidle_ops, these patches try to
address this issue.

Has been tested on Marvell Berlin SoCs. These patches are rebased on the Mark
Rutland's latest psci unification work and Lorenzo Pieralisi's psci related
work[1].

[PATCH 1/4] renames psci_smp.c to psci.c to prepare for the next two changes.
[PATCH 2/4] enables PSCI on UP systems
[PATCH 3/4] move psci's cpu_suspend handling to generic code as suggested
	    by Lorenzo.
[PATCH 4/4] implement cpuidle_ops with psci finally.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/347352.html

Jisheng Zhang (4):
  arm: psci: rename psci_smp.c to psci.c
  arm: psci: enable PSCI on UP systems
  firmware: psci: move cpu_suspend handling to generic code
  arm: psci: add cpuidle_ops support

 arch/arm/kernel/Makefile               |  5 +-
 arch/arm/kernel/{psci_smp.c => psci.c} | 22 ++++++++
 arch/arm64/kernel/psci.c               | 95 ----------------------------------
 drivers/firmware/psci.c                | 95 ++++++++++++++++++++++++++++++++++
 include/linux/psci.h                   |  2 +
 5 files changed, 120 insertions(+), 99 deletions(-)
 rename arch/arm/kernel/{psci_smp.c => psci.c} (86%)

-- 
2.1.4

[PATCH 1/4] arm: psci: rename psci_smp.c to psci.c

[Jisheng Zhang]

We'd like to enable PSCI feature on UP kernels. The renaming in this
patch prepares for next commit to enable PSCI for UP systems.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 arch/arm/kernel/Makefile               | 2 +-
 arch/arm/kernel/{psci_smp.c => psci.c} | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename arch/arm/kernel/{psci_smp.c => psci.c} (100%)

diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 3b995f5..c57b2c0 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -90,7 +90,7 @@ obj-$(CONFIG_EARLY_PRINTK)	+= early_printk.o
 obj-$(CONFIG_ARM_VIRT_EXT)	+= hyp-stub.o
 ifeq ($(CONFIG_ARM_PSCI),y)
 obj-y				+= psci-call.o
-obj-$(CONFIG_SMP)		+= psci_smp.o
+obj-$(CONFIG_SMP)		+= psci.o
 endif
 
 extra-y := $(head-y) vmlinux.lds
diff --git a/arch/arm/kernel/psci_smp.c b/arch/arm/kernel/psci.c
similarity index 100%
rename from arch/arm/kernel/psci_smp.c
rename to arch/arm/kernel/psci.c
-- 
2.1.4

[PATCH 2/4] arm: psci: enable PSCI on UP systems

[Jisheng Zhang]

The PSCI can work on both SMP and UP. Now We'd like to enable PSCI
features on UP systems. This is to prepare for next commit to implement
cpuidle_ops with PSCI functions.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 arch/arm/kernel/Makefile | 5 +----
 arch/arm/kernel/psci.c   | 2 ++
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index c57b2c0..2b1a60c 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -88,9 +88,6 @@ obj-$(CONFIG_DEBUG_LL)	+= debug.o
 obj-$(CONFIG_EARLY_PRINTK)	+= early_printk.o
 
 obj-$(CONFIG_ARM_VIRT_EXT)	+= hyp-stub.o
-ifeq ($(CONFIG_ARM_PSCI),y)
-obj-y				+= psci-call.o
-obj-$(CONFIG_SMP)		+= psci.o
-endif
+obj-$(CONFIG_ARM_PSCI)		+= psci-call.o psci.o
 
 extra-y := $(head-y) vmlinux.lds
diff --git a/arch/arm/kernel/psci.c b/arch/arm/kernel/psci.c
index 61c04b0..7f6ff02 100644
--- a/arch/arm/kernel/psci.c
+++ b/arch/arm/kernel/psci.c
@@ -47,6 +47,7 @@
  *
  */
 
+#ifdef CONFIG_SMP
 extern void secondary_startup(void);
 
 static int psci_boot_secondary(unsigned int cpu, struct task_struct *idle)
@@ -128,3 +129,4 @@ struct smp_operations __initdata psci_smp_ops = {
 	.cpu_kill		= psci_cpu_kill,
 #endif
 };
+#endif
-- 
2.1.4

[PATCH 3/4] firmware: psci: move cpu_suspend handling to generic code

[Jisheng Zhang]

Functions implemented on arm64 to suspend cpu and translates the idle
state index passed by the cpu_suspend core call to a valid PSCI state
are not arm64 specific and should be moved to generic code so that they
can be reused on arm systems too.

This patch moves these functions to generic PSCI firmware layer code.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 arch/arm64/kernel/psci.c | 95 ------------------------------------------------
 drivers/firmware/psci.c  | 95 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/psci.h     |  2 +
 3 files changed, 97 insertions(+), 95 deletions(-)

diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
index 4be5972..06e5c9a 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;
-}
 
 #ifdef CONFIG_SMP
 
@@ -181,33 +113,6 @@ static int cpu_psci_cpu_kill(unsigned int cpu)
 #endif
 #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
diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
index 2f5d611..5da8aa2 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -20,12 +20,14 @@
 #include <linux/printk.h>
 #include <linux/psci.h>
 #include <linux/reboot.h>
+#include <linux/slab.h>
 
 #include <uapi/linux/psci.h>
 
 #include <asm/cputype.h>
 #include <asm/system_misc.h>
 #include <asm/smp_plat.h>
+#include <asm/suspend.h>
 
 /*
  * While a 64-bit OS can make calls with SMC32 calling conventions, for some
@@ -81,6 +83,8 @@ static u32 psci_function_id[PSCI_FN_MAX];
 
 static u32 psci_cpu_suspend_feature;
 
+static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
+
 static inline bool psci_has_ext_power_state(void)
 {
 	return psci_cpu_suspend_feature &
@@ -217,6 +221,97 @@ static void psci_sys_poweroff(void)
 	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
 }
 
+int 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 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 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;
+}
+
 static int __init psci_features(u32 psci_func_id)
 {
 	return invoke_psci_fn(PSCI_1_0_FN_PSCI_FEATURES,
diff --git a/include/linux/psci.h b/include/linux/psci.h
index 12c4865..cbb49a3 100644
--- a/include/linux/psci.h
+++ b/include/linux/psci.h
@@ -23,6 +23,8 @@
 bool psci_tos_resident_on(int cpu);
 bool psci_power_state_loses_context(u32 state);
 bool psci_power_state_is_valid(u32 state);
+int cpu_psci_cpu_init_idle(unsigned int cpu);
+int cpu_psci_cpu_suspend(unsigned long index);
 
 struct psci_operations {
 	int (*cpu_suspend)(u32 state, unsigned long entry_point);
-- 
2.1.4

[PATCH 4/4] arm: psci: add cpuidle_ops support

[Jisheng Zhang]

This patch implements cpuidle_ops using psci. After this patch, we can use
cpuidle-arm.c with psci backend for both arm and arm64.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 arch/arm/kernel/psci.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm/kernel/psci.c b/arch/arm/kernel/psci.c
index 7f6ff02..c5b94f5 100644
--- a/arch/arm/kernel/psci.c
+++ b/arch/arm/kernel/psci.c
@@ -13,6 +13,7 @@
  * Author: Will Deacon <will.deacon@arm.com>
  */
 
+#include <linux/cpuidle.h>
 #include <linux/init.h>
 #include <linux/smp.h>
 #include <linux/of.h>
@@ -21,6 +22,7 @@
 
 #include <uapi/linux/psci.h>
 
+#include <asm/cpuidle.h>
 #include <asm/psci.h>
 #include <asm/smp_plat.h>
 
@@ -47,6 +49,24 @@
  *
  */
 
+#ifdef CONFIG_CPU_IDLE
+static int psci_cpuidle_suspend(int cpu, unsigned long arg)
+{
+	return cpu_psci_cpu_suspend(arg);
+}
+
+static int psci_cpuidle_init(struct device_node *node, int cpu)
+{
+	return cpu_psci_cpu_init_idle(cpu);
+}
+
+static struct cpuidle_ops psci_cpuidle_ops __initdata = {
+	.suspend = psci_cpuidle_suspend,
+	.init = psci_cpuidle_init,
+};
+CPUIDLE_METHOD_OF_DECLARE(psci_idle, "psci", &psci_cpuidle_ops);
+#endif
+
 #ifdef CONFIG_SMP
 extern void secondary_startup(void);
 
-- 
2.1.4

Re: [PATCH 4/4] arm: psci: add cpuidle_ops support

[Russell King - ARM Linux]

On Wed, Jul 08, 2015 at 06:13:37PM +0800, Jisheng Zhang wrote:
> This patch implements cpuidle_ops using psci. After this patch, we can use
> cpuidle-arm.c with psci backend for both arm and arm64.

I really don't see the point of most of the patches in this series.

To summarise, what you're doing is:

1. Renaming arch/arm/kernel/psci_smp.c to arch/arm/kernel/psci.c
2. Adding a #ifdef CONFIG_SMP around _all_ the code in psci.c
3. Adding cpuidle code with an #ifdef CONFIG_CPU_IDLE around all the
   CPU idle code.

So, we end up with a file which contains:

/*
 header
 */
#include statements

/*
 some commentry relevant to SMP code
 */
#ifdef CONFIG_CPU_IDLE
... cpu idle code ...
#endif
#ifdef CONFIG_SMP
... smp code ...
#endif

which (a) is a mess, and (b) is unnecessary.  The only relevant bits which
are shared are the #include statements.

Please try this alternative approach:

1. Leave psci_smp.c alone.
2. Add arch/arm/kernel/psci_cpuidle.c containing the #include statements
   you need, and the CPU idle code.

I think such an approach will reduce your patch series to two patches,
one moving the ARM64 code, and one adding the cpuidle code.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 4/4] arm: psci: add cpuidle_ops support

[Jisheng Zhang]

Dear Russell,

On Wed, 8 Jul 2015 11:34:29 +0100
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Wed, Jul 08, 2015 at 06:13:37PM +0800, Jisheng Zhang wrote:
> > This patch implements cpuidle_ops using psci. After this patch, we can use
> > cpuidle-arm.c with psci backend for both arm and arm64.
> 
> I really don't see the point of most of the patches in this series.
> 
> To summarise, what you're doing is:
> 
> 1. Renaming arch/arm/kernel/psci_smp.c to arch/arm/kernel/psci.c
> 2. Adding a #ifdef CONFIG_SMP around _all_ the code in psci.c
> 3. Adding cpuidle code with an #ifdef CONFIG_CPU_IDLE around all the
>    CPU idle code.
> 
> So, we end up with a file which contains:
> 
> /*
>  header
>  */
> #include statements
> 
> /*
>  some commentry relevant to SMP code
>  */
> #ifdef CONFIG_CPU_IDLE
> ... cpu idle code ...
> #endif
> #ifdef CONFIG_SMP
> ... smp code ...
> #endif
> 
> which (a) is a mess, and (b) is unnecessary.  The only relevant bits which
> are shared are the #include statements.
> 
> Please try this alternative approach:
> 
> 1. Leave psci_smp.c alone.
> 2. Add arch/arm/kernel/psci_cpuidle.c containing the #include statements
>    you need, and the CPU idle code.
> 
> I think such an approach will reduce your patch series to two patches,
> one moving the ARM64 code, and one adding the cpuidle code.
> 

Good idea! Will refine the patches as you suggested.

Thanks a lot for your review,
Jisheng

Re: [PATCH 4/4] arm: psci: add cpuidle_ops support

[Jisheng Zhang]

Dear Russell,

On Wed, 8 Jul 2015 19:38:42 +0800
Jisheng Zhang <jszhang@marvell.com> wrote:

> Dear Russell,
> 
> On Wed, 8 Jul 2015 11:34:29 +0100
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > On Wed, Jul 08, 2015 at 06:13:37PM +0800, Jisheng Zhang wrote:
> > > This patch implements cpuidle_ops using psci. After this patch, we can use
> > > cpuidle-arm.c with psci backend for both arm and arm64.
> > 
> > I really don't see the point of most of the patches in this series.
> > 
> > To summarise, what you're doing is:
> > 
> > 1. Renaming arch/arm/kernel/psci_smp.c to arch/arm/kernel/psci.c
> > 2. Adding a #ifdef CONFIG_SMP around _all_ the code in psci.c
> > 3. Adding cpuidle code with an #ifdef CONFIG_CPU_IDLE around all the
> >    CPU idle code.
> > 
> > So, we end up with a file which contains:
> > 
> > /*
> >  header
> >  */
> > #include statements
> > 
> > /*
> >  some commentry relevant to SMP code
> >  */
> > #ifdef CONFIG_CPU_IDLE
> > ... cpu idle code ...
> > #endif
> > #ifdef CONFIG_SMP
> > ... smp code ...
> > #endif
> > 
> > which (a) is a mess, and (b) is unnecessary.  The only relevant bits which
> > are shared are the #include statements.
> > 
> > Please try this alternative approach:
> > 
> > 1. Leave psci_smp.c alone.
> > 2. Add arch/arm/kernel/psci_cpuidle.c containing the #include statements
> >    you need, and the CPU idle code.

After more consideration, I have one concern.

Currently, cpuidle_ops is defined as the following,

> struct cpuidle_ops {
>        int (*suspend)(int cpu, unsigned long arg);

the cpu may not be necessary, because to-be-suspended cpu is always the calling
cpu itself.

>	int (*init)(struct device_node *, int cpu);

the device_node here may not be necessary either, because we can get the node
via. of_get_cpu_node.

So if we refine cpuidle_ops defintion, the code would be as simple as

static struct cpuidle_ops psci_cpuidle_ops __initdata = {
        .suspend = cpu_psci_cpu_suspend,
        .init = cpu_psci_cpu_init_idle,
};
CPUIDLE_METHOD_OF_DECLARE(psci_idle, "psci", &psci_cpuidle_ops);

I'm not sure a new psci_cpuidle.c only with above 5 lines is acceptable or not.

Thanks,
Jisheng




> > 
> > I think such an approach will reduce your patch series to two patches,
> > one moving the ARM64 code, and one adding the cpuidle code.
> > 
> 
> Good idea! Will refine the patches as you suggested.
>