[PATCH 0/2 v8] arm: introduce psci_smp_ops

[PATCH 0/2 v8] arm: introduce psci_smp_ops

[Stefano Stabellini]

Hi all,
this is the eighth version of the patch series to move virt_smp_ops out
of mach-virt and make it a generic set of reusable PSCI based smp_ops.
psci_smp_ops are preferred over the platform smp_ops.
The last patch introduces smp_init.


Jon Medhurst (1):
      ARM: Enable selection of SMP operations at boot time

Stefano Stabellini (1):
      arm: introduce psci_smp_ops

 arch/arm/include/asm/mach/arch.h |    4 ++
 arch/arm/include/asm/psci.h      |   29 ++++++++++++++++
 arch/arm/kernel/Makefile         |    5 ++-
 arch/arm/kernel/psci.c           |    7 ++--
 arch/arm/kernel/psci_smp.c       |   67 ++++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/setup.c          |    9 ++++-
 arch/arm/mach-virt/Makefile      |    1 -
 arch/arm/mach-virt/platsmp.c     |   58 --------------------------------
 arch/arm/mach-virt/virt.c        |    3 --
 9 files changed, 115 insertions(+), 68 deletions(-)

git://git.kernel.org/pub/scm/linux/kernel/git/sstabellini/xen.git 3.9-rc3-psci-ops-8-tag

Cheers,

Stefano

[PATCH v8 1/2] arm: introduce psci_smp_ops

[Stefano Stabellini]

Rename virt_smp_ops to psci_smp_ops and move them to arch/arm/kernel/psci_smp.c.
Remove mach-virt/platsmp.c, now unused.
Compile psci_smp if CONFIG_ARM_PSCI and CONFIG_SMP.

Add a cpu_die smp_op based on psci_ops.cpu_off.

Initialize PSCI before setting smp_ops in setup_arch.
Use psci_smp_ops if the platform doesn't provide its own smp_ops.

If PSCI is available on the platform, prefer psci_smp_ops over the
platform smp_ops.

Changes in v8:
- merge "prefer psci_smp_ops over mdesc->smp" into this patch.

Changes in v6:
- fixed return values for psci_smp_available and psci_init ifndef
CONFIG_ARM_PSCI.

Changes in v5:
- document psci_operations;
- psci_init returns NULL.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 arch/arm/include/asm/psci.h  |   29 ++++++++++++++++++
 arch/arm/kernel/Makefile     |    5 ++-
 arch/arm/kernel/psci.c       |    7 ++--
 arch/arm/kernel/psci_smp.c   |   67 ++++++++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/setup.c      |    7 ++++-
 arch/arm/mach-virt/Makefile  |    1 -
 arch/arm/mach-virt/platsmp.c |   58 ------------------------------------
 arch/arm/mach-virt/virt.c    |    3 --
 8 files changed, 109 insertions(+), 68 deletions(-)
 create mode 100644 arch/arm/kernel/psci_smp.c
 delete mode 100644 arch/arm/mach-virt/platsmp.c

diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h
index ce0dbe7..fa44417 100644
--- a/arch/arm/include/asm/psci.h
+++ b/arch/arm/include/asm/psci.h
@@ -23,6 +23,26 @@ struct psci_power_state {
 	u8	affinity_level;
 };
 
+/*
+ * cpu_suspend   Suspend the execution on a CPU
+ * @state        we don't currently describe affinity levels, so just pass 0.
+ * @entry_point  the first instruction to be executed on return
+ * returns 0  success, < 0 on failure
+ *
+ * cpu_off       Power down a CPU
+ * @state        we don't currently describe affinity levels, so just pass 0.
+ * no return on successful call
+ *
+ * cpu_on        Power up a CPU
+ * @cpuid        cpuid of target CPU, as from MPIDR
+ * @entry_point  the first instruction to be executed on return
+ * returns 0  success, < 0 on failure
+ *
+ * migrate       Migrate the context to a different CPU
+ * @cpuid        cpuid of target CPU, as from MPIDR
+ * returns 0  success, < 0 on failure
+ *
+ */
 struct psci_operations {
 	int (*cpu_suspend)(struct psci_power_state state,
 			   unsigned long entry_point);
@@ -32,5 +52,14 @@ struct psci_operations {
 };
 
 extern struct psci_operations psci_ops;
+extern struct smp_operations psci_smp_ops;
+
+#ifdef CONFIG_ARM_PSCI
+void psci_init(void);
+bool psci_smp_available(void);
+#else
+static inline void psci_init(void) { }
+static inline bool psci_smp_available(void) { return false; }
+#endif
 
 #endif /* __ASM_ARM_PSCI_H */
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 5f3338e..dd9d90a 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -82,6 +82,9 @@ obj-$(CONFIG_DEBUG_LL)	+= debug.o
 obj-$(CONFIG_EARLY_PRINTK)	+= early_printk.o
 
 obj-$(CONFIG_ARM_VIRT_EXT)	+= hyp-stub.o
-obj-$(CONFIG_ARM_PSCI)		+= psci.o
+ifeq ($(CONFIG_ARM_PSCI),y)
+obj-y				+= psci.o
+obj-$(CONFIG_SMP)		+= psci_smp.o
+endif
 
 extra-y := $(head-y) vmlinux.lds
diff --git a/arch/arm/kernel/psci.c b/arch/arm/kernel/psci.c
index 3653164..4693188 100644
--- a/arch/arm/kernel/psci.c
+++ b/arch/arm/kernel/psci.c
@@ -158,7 +158,7 @@ static const struct of_device_id psci_of_match[] __initconst = {
 	{},
 };
 
-static int __init psci_init(void)
+void __init psci_init(void)
 {
 	struct device_node *np;
 	const char *method;
@@ -166,7 +166,7 @@ static int __init psci_init(void)
 
 	np = of_find_matching_node(NULL, psci_of_match);
 	if (!np)
-		return 0;
+		return;
 
 	pr_info("probing function IDs from device-tree\n");
 
@@ -206,6 +206,5 @@ static int __init psci_init(void)
 
 out_put_node:
 	of_node_put(np);
-	return 0;
+	return;
 }
-early_initcall(psci_init);
diff --git a/arch/arm/kernel/psci_smp.c b/arch/arm/kernel/psci_smp.c
new file mode 100644
index 0000000..66b0f77
--- /dev/null
+++ b/arch/arm/kernel/psci_smp.c
@@ -0,0 +1,67 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * Copyright (C) 2012 ARM Limited
+ *
+ * Author: Will Deacon <will.deacon@arm.com>
+ */
+
+#include <linux/init.h>
+#include <linux/irqchip/arm-gic.h>
+#include <linux/smp.h>
+#include <linux/of.h>
+
+#include <asm/psci.h>
+#include <asm/smp_plat.h>
+
+extern void secondary_startup(void);
+
+static int __cpuinit psci_boot_secondary(unsigned int cpu,
+					 struct task_struct *idle)
+{
+	if (psci_ops.cpu_on)
+		return psci_ops.cpu_on(cpu_logical_map(cpu),
+				       __pa(secondary_startup));
+	return -ENODEV;
+}
+
+static void __cpuinit psci_secondary_init(unsigned int cpu)
+{
+	gic_secondary_init(0);
+}
+
+#ifdef CONFIG_HOTPLUG_CPU
+void __ref psci_cpu_die(unsigned int cpu)
+{
+       const struct psci_power_state ps = {
+               .type = PSCI_POWER_STATE_TYPE_POWER_DOWN,
+       };
+
+       if (psci_ops.cpu_off)
+               psci_ops.cpu_off(ps);
+
+       /* We should never return */
+       panic("psci: cpu %d failed to shutdown\n", cpu);
+}
+#else
+#define psci_cpu_die NULL
+#endif
+
+bool __init psci_smp_available(void)
+{
+	/* is cpu_on available at least? */
+	return (psci_ops.cpu_on != NULL);
+}
+
+struct smp_operations __initdata psci_smp_ops = {
+	.smp_secondary_init	= psci_secondary_init,
+	.smp_boot_secondary	= psci_boot_secondary,
+	.cpu_die		= psci_cpu_die,
+};
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 3f6cbb2..dad3048 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -36,6 +36,7 @@
 #include <asm/cputype.h>
 #include <asm/elf.h>
 #include <asm/procinfo.h>
+#include <asm/psci.h>
 #include <asm/sections.h>
 #include <asm/setup.h>
 #include <asm/smp_plat.h>
@@ -766,9 +767,13 @@ void __init setup_arch(char **cmdline_p)
 	unflatten_device_tree();
 
 	arm_dt_init_cpu_maps();
+	psci_init();
 #ifdef CONFIG_SMP
 	if (is_smp()) {
-		smp_set_ops(mdesc->smp);
+		if (psci_smp_available())
+			smp_set_ops(&psci_smp_ops);
+		else if (mdesc->smp)
+			smp_set_ops(mdesc->smp);
 		smp_init_cpus();
 	}
 #endif
diff --git a/arch/arm/mach-virt/Makefile b/arch/arm/mach-virt/Makefile
index 042afc1..7ddbfa6 100644
--- a/arch/arm/mach-virt/Makefile
+++ b/arch/arm/mach-virt/Makefile
@@ -3,4 +3,3 @@
 #
 
 obj-y					:= virt.o
-obj-$(CONFIG_SMP)			+= platsmp.o
diff --git a/arch/arm/mach-virt/platsmp.c b/arch/arm/mach-virt/platsmp.c
deleted file mode 100644
index 8badaab..0000000
--- a/arch/arm/mach-virt/platsmp.c
+++ /dev/null
@@ -1,58 +0,0 @@
-/*
- * Dummy Virtual Machine - does what it says on the tin.
- *
- * Copyright (C) 2012 ARM Ltd
- * Author: Will Deacon <will.deacon@arm.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-
-#include <linux/init.h>
-#include <linux/smp.h>
-#include <linux/of.h>
-
-#include <linux/irqchip/arm-gic.h>
-
-#include <asm/psci.h>
-#include <asm/smp_plat.h>
-
-extern void secondary_startup(void);
-
-static void __init virt_smp_init_cpus(void)
-{
-}
-
-static void __init virt_smp_prepare_cpus(unsigned int max_cpus)
-{
-}
-
-static int __cpuinit virt_boot_secondary(unsigned int cpu,
-					 struct task_struct *idle)
-{
-	if (psci_ops.cpu_on)
-		return psci_ops.cpu_on(cpu_logical_map(cpu),
-				       __pa(secondary_startup));
-	return -ENODEV;
-}
-
-static void __cpuinit virt_secondary_init(unsigned int cpu)
-{
-	gic_secondary_init(0);
-}
-
-struct smp_operations __initdata virt_smp_ops = {
-	.smp_init_cpus		= virt_smp_init_cpus,
-	.smp_prepare_cpus	= virt_smp_prepare_cpus,
-	.smp_secondary_init	= virt_secondary_init,
-	.smp_boot_secondary	= virt_boot_secondary,
-};
diff --git a/arch/arm/mach-virt/virt.c b/arch/arm/mach-virt/virt.c
index 31666f6..4438ed0 100644
--- a/arch/arm/mach-virt/virt.c
+++ b/arch/arm/mach-virt/virt.c
@@ -43,12 +43,9 @@ static const char *virt_dt_match[] = {
 	NULL
 };
 
-extern struct smp_operations virt_smp_ops;
-
 DT_MACHINE_START(VIRT, "Dummy Virtual Machine")
 	.init_irq	= irqchip_init,
 	.init_time	= virt_timer_init,
 	.init_machine	= virt_init,
-	.smp		= smp_ops(virt_smp_ops),
 	.dt_compat	= virt_dt_match,
 MACHINE_END
-- 
1.7.2.5

[PATCH v8 1/2] arm: introduce psci_smp_ops

[Will Deacon]

Hi Stefano,

On Wed, Apr 24, 2013 at 07:40:18PM +0100, Stefano Stabellini wrote:
> Rename virt_smp_ops to psci_smp_ops and move them to arch/arm/kernel/psci_smp.c.
> Remove mach-virt/platsmp.c, now unused.
> Compile psci_smp if CONFIG_ARM_PSCI and CONFIG_SMP.
> 
> Add a cpu_die smp_op based on psci_ops.cpu_off.
> 
> Initialize PSCI before setting smp_ops in setup_arch.
> Use psci_smp_ops if the platform doesn't provide its own smp_ops.
> 
> If PSCI is available on the platform, prefer psci_smp_ops over the
> platform smp_ops.
> 
> Changes in v8:
> - merge "prefer psci_smp_ops over mdesc->smp" into this patch.
> 
> Changes in v6:
> - fixed return values for psci_smp_available and psci_init ifndef
> CONFIG_ARM_PSCI.
> 
> Changes in v5:
> - document psci_operations;
> - psci_init returns NULL.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  arch/arm/include/asm/psci.h  |   29 ++++++++++++++++++
>  arch/arm/kernel/Makefile     |    5 ++-
>  arch/arm/kernel/psci.c       |    7 ++--
>  arch/arm/kernel/psci_smp.c   |   67 ++++++++++++++++++++++++++++++++++++++++++
>  arch/arm/kernel/setup.c      |    7 ++++-
>  arch/arm/mach-virt/Makefile  |    1 -
>  arch/arm/mach-virt/platsmp.c |   58 ------------------------------------
>  arch/arm/mach-virt/virt.c    |    3 --
>  8 files changed, 109 insertions(+), 68 deletions(-)
>  create mode 100644 arch/arm/kernel/psci_smp.c
>  delete mode 100644 arch/arm/mach-virt/platsmp.c
> 
> diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h
> index ce0dbe7..fa44417 100644
> --- a/arch/arm/include/asm/psci.h
> +++ b/arch/arm/include/asm/psci.h
> @@ -23,6 +23,26 @@ struct psci_power_state {
>  	u8	affinity_level;
>  };
>  
> +/*
> + * cpu_suspend   Suspend the execution on a CPU
> + * @state        we don't currently describe affinity levels, so just pass 0.
> + * @entry_point  the first instruction to be executed on return
> + * returns 0  success, < 0 on failure
> + *
> + * cpu_off       Power down a CPU
> + * @state        we don't currently describe affinity levels, so just pass 0.
> + * no return on successful call
> + *
> + * cpu_on        Power up a CPU
> + * @cpuid        cpuid of target CPU, as from MPIDR
> + * @entry_point  the first instruction to be executed on return
> + * returns 0  success, < 0 on failure
> + *
> + * migrate       Migrate the context to a different CPU
> + * @cpuid        cpuid of target CPU, as from MPIDR
> + * returns 0  success, < 0 on failure
> + *
> + */

Can you move these comments into psci-smp.c please? They're really specific
to the implementation there, and if we put them in a header we're lying to
ourselves about the parameters actually described by the PSCI specification.

With that change:

  Acked-by: Will Deacon <will.deacon@arm.com>

Will

[PATCH v8 1/2] arm: introduce psci_smp_ops

[Stefano Stabellini]

On Thu, 25 Apr 2013, Will Deacon wrote:
> > +/*
> > + * cpu_suspend   Suspend the execution on a CPU
> > + * @state        we don't currently describe affinity levels, so just pass 0.
> > + * @entry_point  the first instruction to be executed on return
> > + * returns 0  success, < 0 on failure
> > + *
> > + * cpu_off       Power down a CPU
> > + * @state        we don't currently describe affinity levels, so just pass 0.
> > + * no return on successful call
> > + *
> > + * cpu_on        Power up a CPU
> > + * @cpuid        cpuid of target CPU, as from MPIDR
> > + * @entry_point  the first instruction to be executed on return
> > + * returns 0  success, < 0 on failure
> > + *
> > + * migrate       Migrate the context to a different CPU
> > + * @cpuid        cpuid of target CPU, as from MPIDR
> > + * returns 0  success, < 0 on failure
> > + *
> > + */
> 
> Can you move these comments into psci-smp.c please? They're really specific
> to the implementation there, and if we put them in a header we're lying to
> ourselves about the parameters actually described by the PSCI specification.

You have a good point about the PSCI spec.

However from the Linux POV these comments should regard the functions
exported by psci_operations, not the firmware interface, this is why I
think it makes sense to keep them in psci.h.
What we are saying is for example that psci_operations.cpu_on returns 0
on success and < 0 on failure, and it takes a cpuid and an entry point
as parameters. We are not saying anything about the firmware interface.

Maybe I should add@the top:

"psci_operations functions and parameters, might different from the
firmware interface:"

[PATCH v8 1/2] arm: introduce psci_smp_ops

[Will Deacon]

On Thu, Apr 25, 2013 at 11:12:54AM +0100, Stefano Stabellini wrote:
> On Thu, 25 Apr 2013, Will Deacon wrote:
> > > +/*
> > > + * cpu_suspend   Suspend the execution on a CPU
> > > + * @state        we don't currently describe affinity levels, so just pass 0.
> > > + * @entry_point  the first instruction to be executed on return
> > > + * returns 0  success, < 0 on failure
> > > + *
> > > + * cpu_off       Power down a CPU
> > > + * @state        we don't currently describe affinity levels, so just pass 0.
> > > + * no return on successful call
> > > + *
> > > + * cpu_on        Power up a CPU
> > > + * @cpuid        cpuid of target CPU, as from MPIDR
> > > + * @entry_point  the first instruction to be executed on return
> > > + * returns 0  success, < 0 on failure
> > > + *
> > > + * migrate       Migrate the context to a different CPU
> > > + * @cpuid        cpuid of target CPU, as from MPIDR
> > > + * returns 0  success, < 0 on failure
> > > + *
> > > + */
> > 
> > Can you move these comments into psci-smp.c please? They're really specific
> > to the implementation there, and if we put them in a header we're lying to
> > ourselves about the parameters actually described by the PSCI specification.
> 
> You have a good point about the PSCI spec.
> 
> However from the Linux POV these comments should regard the functions
> exported by psci_operations, not the firmware interface, this is why I
> think it makes sense to keep them in psci.h.
> What we are saying is for example that psci_operations.cpu_on returns 0
> on success and < 0 on failure, and it takes a cpuid and an entry point
> as parameters. We are not saying anything about the firmware interface.

I disagree. You're explicitly stating that we pass the `cpuid of target CPU,
as from MPIDR'. That's simply not true -- the firmware could choose any
numbering scheme to identify the CPUs. For KVM and Xen, it *is* the mpidr,
which is why psci-smp.c works at all, but that's where the comment belongs,
not in this header file.

Will

[PATCH v8 1/2] arm: introduce psci_smp_ops

[Stefano Stabellini]

On Thu, 25 Apr 2013, Will Deacon wrote:
> On Thu, Apr 25, 2013 at 11:12:54AM +0100, Stefano Stabellini wrote:
> > On Thu, 25 Apr 2013, Will Deacon wrote:
> > > > +/*
> > > > + * cpu_suspend   Suspend the execution on a CPU
> > > > + * @state        we don't currently describe affinity levels, so just pass 0.
> > > > + * @entry_point  the first instruction to be executed on return
> > > > + * returns 0  success, < 0 on failure
> > > > + *
> > > > + * cpu_off       Power down a CPU
> > > > + * @state        we don't currently describe affinity levels, so just pass 0.
> > > > + * no return on successful call
> > > > + *
> > > > + * cpu_on        Power up a CPU
> > > > + * @cpuid        cpuid of target CPU, as from MPIDR
> > > > + * @entry_point  the first instruction to be executed on return
> > > > + * returns 0  success, < 0 on failure
> > > > + *
> > > > + * migrate       Migrate the context to a different CPU
> > > > + * @cpuid        cpuid of target CPU, as from MPIDR
> > > > + * returns 0  success, < 0 on failure
> > > > + *
> > > > + */
> > > 
> > > Can you move these comments into psci-smp.c please? They're really specific
> > > to the implementation there, and if we put them in a header we're lying to
> > > ourselves about the parameters actually described by the PSCI specification.
> > 
> > You have a good point about the PSCI spec.
> > 
> > However from the Linux POV these comments should regard the functions
> > exported by psci_operations, not the firmware interface, this is why I
> > think it makes sense to keep them in psci.h.
> > What we are saying is for example that psci_operations.cpu_on returns 0
> > on success and < 0 on failure, and it takes a cpuid and an entry point
> > as parameters. We are not saying anything about the firmware interface.
> 
> I disagree. You're explicitly stating that we pass the `cpuid of target CPU,
> as from MPIDR'. That's simply not true -- the firmware could choose any
> numbering scheme to identify the CPUs. For KVM and Xen, it *is* the mpidr,
> which is why psci-smp.c works at all, but that's where the comment belongs,
> not in this header file.

I see, you want to keep psci_operations true to the firmware interface
while explaining that psci_smp makes some assumptions about it.
So the comment should be something like:

/*
 * psci_smp assumes that the following is true about PSCI:
 * 
 * cpu_suspend   Suspend the execution on a CPU
 * @state        we don't currently describe affinity levels, so just pass 0.
 * @entry_point  the first instruction to be executed on return
 * returns 0  success, < 0 on failure
 *
 * cpu_off       Power down a CPU
 * @state        we don't currently describe affinity levels, so just pass 0.
 * no return on successful call
 *
 * cpu_on        Power up a CPU
 * @cpuid        cpuid of target CPU, as from MPIDR
 * @entry_point  the first instruction to be executed on return
 * returns 0  success, < 0 on failure
 *
 * migrate       Migrate the context to a different CPU
 * @cpuid        cpuid of target CPU, as from MPIDR
 * returns 0  success, < 0 on failure
 *
 */

[PATCH v8 1/2] arm: introduce psci_smp_ops

[Will Deacon]

On Thu, Apr 25, 2013 at 12:08:02PM +0100, Stefano Stabellini wrote:
> On Thu, 25 Apr 2013, Will Deacon wrote:
> > On Thu, Apr 25, 2013 at 11:12:54AM +0100, Stefano Stabellini wrote:
> > > However from the Linux POV these comments should regard the functions
> > > exported by psci_operations, not the firmware interface, this is why I
> > > think it makes sense to keep them in psci.h.
> > > What we are saying is for example that psci_operations.cpu_on returns 0
> > > on success and < 0 on failure, and it takes a cpuid and an entry point
> > > as parameters. We are not saying anything about the firmware interface.
> > 
> > I disagree. You're explicitly stating that we pass the `cpuid of target CPU,
> > as from MPIDR'. That's simply not true -- the firmware could choose any
> > numbering scheme to identify the CPUs. For KVM and Xen, it *is* the mpidr,
> > which is why psci-smp.c works at all, but that's where the comment belongs,
> > not in this header file.
> 
> I see, you want to keep psci_operations true to the firmware interface
> while explaining that psci_smp makes some assumptions about it.

Precisely! :)

> So the comment should be something like:
> 
> /*
>  * psci_smp assumes that the following is true about PSCI:
>  * 
>  * cpu_suspend   Suspend the execution on a CPU
>  * @state        we don't currently describe affinity levels, so just pass 0.
>  * @entry_point  the first instruction to be executed on return
>  * returns 0  success, < 0 on failure
>  *
>  * cpu_off       Power down a CPU
>  * @state        we don't currently describe affinity levels, so just pass 0.
>  * no return on successful call
>  *
>  * cpu_on        Power up a CPU
>  * @cpuid        cpuid of target CPU, as from MPIDR
>  * @entry_point  the first instruction to be executed on return
>  * returns 0  success, < 0 on failure
>  *
>  * migrate       Migrate the context to a different CPU
>  * @cpuid        cpuid of target CPU, as from MPIDR
>  * returns 0  success, < 0 on failure
>  *
>  */

That's certainly better, but I'd still rather see the comment with the
implementation as there's a greater potential for confusion having it here.

Will

[PATCH v8 1/2] arm: introduce psci_smp_ops

[Stefano Stabellini]

On Thu, 25 Apr 2013, Will Deacon wrote:
> On Thu, Apr 25, 2013 at 12:08:02PM +0100, Stefano Stabellini wrote:
> > On Thu, 25 Apr 2013, Will Deacon wrote:
> > > On Thu, Apr 25, 2013 at 11:12:54AM +0100, Stefano Stabellini wrote:
> > > > However from the Linux POV these comments should regard the functions
> > > > exported by psci_operations, not the firmware interface, this is why I
> > > > think it makes sense to keep them in psci.h.
> > > > What we are saying is for example that psci_operations.cpu_on returns 0
> > > > on success and < 0 on failure, and it takes a cpuid and an entry point
> > > > as parameters. We are not saying anything about the firmware interface.
> > > 
> > > I disagree. You're explicitly stating that we pass the `cpuid of target CPU,
> > > as from MPIDR'. That's simply not true -- the firmware could choose any
> > > numbering scheme to identify the CPUs. For KVM and Xen, it *is* the mpidr,
> > > which is why psci-smp.c works at all, but that's where the comment belongs,
> > > not in this header file.
> > 
> > I see, you want to keep psci_operations true to the firmware interface
> > while explaining that psci_smp makes some assumptions about it.
> 
> Precisely! :)
> 
> > So the comment should be something like:
> > 
> > /*
> >  * psci_smp assumes that the following is true about PSCI:
> >  * 
> >  * cpu_suspend   Suspend the execution on a CPU
> >  * @state        we don't currently describe affinity levels, so just pass 0.
> >  * @entry_point  the first instruction to be executed on return
> >  * returns 0  success, < 0 on failure
> >  *
> >  * cpu_off       Power down a CPU
> >  * @state        we don't currently describe affinity levels, so just pass 0.
> >  * no return on successful call
> >  *
> >  * cpu_on        Power up a CPU
> >  * @cpuid        cpuid of target CPU, as from MPIDR
> >  * @entry_point  the first instruction to be executed on return
> >  * returns 0  success, < 0 on failure
> >  *
> >  * migrate       Migrate the context to a different CPU
> >  * @cpuid        cpuid of target CPU, as from MPIDR
> >  * returns 0  success, < 0 on failure
> >  *
> >  */
> 
> That's certainly better, but I'd still rather see the comment with the
> implementation as there's a greater potential for confusion having it here.

Yeah, I forgot to write that it was supposed to go in psci_smp.c.
I am OK with that, I'll repost with this change.

[PATCH v8 1/2] arm: introduce psci_smp_ops

[Nicolas Pitre]

On Thu, 25 Apr 2013, Will Deacon wrote:

> On Thu, Apr 25, 2013 at 11:12:54AM +0100, Stefano Stabellini wrote:
> > On Thu, 25 Apr 2013, Will Deacon wrote:
> > > > +/*
> > > > + * cpu_suspend   Suspend the execution on a CPU
> > > > + * @state        we don't currently describe affinity levels, so just pass 0.
> > > > + * @entry_point  the first instruction to be executed on return
> > > > + * returns 0  success, < 0 on failure
> > > > + *
> > > > + * cpu_off       Power down a CPU
> > > > + * @state        we don't currently describe affinity levels, so just pass 0.
> > > > + * no return on successful call
> > > > + *
> > > > + * cpu_on        Power up a CPU
> > > > + * @cpuid        cpuid of target CPU, as from MPIDR
> > > > + * @entry_point  the first instruction to be executed on return
> > > > + * returns 0  success, < 0 on failure
> > > > + *
> > > > + * migrate       Migrate the context to a different CPU
> > > > + * @cpuid        cpuid of target CPU, as from MPIDR
> > > > + * returns 0  success, < 0 on failure
> > > > + *
> > > > + */
> > > 
> > > Can you move these comments into psci-smp.c please? They're really specific
> > > to the implementation there, and if we put them in a header we're lying to
> > > ourselves about the parameters actually described by the PSCI specification.
> > 
> > You have a good point about the PSCI spec.
> > 
> > However from the Linux POV these comments should regard the functions
> > exported by psci_operations, not the firmware interface, this is why I
> > think it makes sense to keep them in psci.h.
> > What we are saying is for example that psci_operations.cpu_on returns 0
> > on success and < 0 on failure, and it takes a cpuid and an entry point
> > as parameters. We are not saying anything about the firmware interface.
> 
> I disagree. You're explicitly stating that we pass the `cpuid of target CPU,
> as from MPIDR'. That's simply not true -- the firmware could choose any
> numbering scheme to identify the CPUs. For KVM and Xen, it *is* the mpidr,
> which is why psci-smp.c works at all, but that's where the comment belongs,
> not in this header file.

At some point, the _kernel_ API for interfacing with the firmware's PSCI 
will have to ensure uniformity somehow.  The PSCI interface code could 
translate the passed MPIDR into whatever the firmware decided to use for 
identifying CPUs if needed, keeping this issue localized.


Nicolas

[PATCH v8 1/2] arm: introduce psci_smp_ops

[Stefano Stabellini]

On Thu, 25 Apr 2013, Nicolas Pitre wrote:
> On Thu, 25 Apr 2013, Will Deacon wrote:
> 
> > On Thu, Apr 25, 2013 at 11:12:54AM +0100, Stefano Stabellini wrote:
> > > On Thu, 25 Apr 2013, Will Deacon wrote:
> > > > > +/*
> > > > > + * cpu_suspend   Suspend the execution on a CPU
> > > > > + * @state        we don't currently describe affinity levels, so just pass 0.
> > > > > + * @entry_point  the first instruction to be executed on return
> > > > > + * returns 0  success, < 0 on failure
> > > > > + *
> > > > > + * cpu_off       Power down a CPU
> > > > > + * @state        we don't currently describe affinity levels, so just pass 0.
> > > > > + * no return on successful call
> > > > > + *
> > > > > + * cpu_on        Power up a CPU
> > > > > + * @cpuid        cpuid of target CPU, as from MPIDR
> > > > > + * @entry_point  the first instruction to be executed on return
> > > > > + * returns 0  success, < 0 on failure
> > > > > + *
> > > > > + * migrate       Migrate the context to a different CPU
> > > > > + * @cpuid        cpuid of target CPU, as from MPIDR
> > > > > + * returns 0  success, < 0 on failure
> > > > > + *
> > > > > + */
> > > > 
> > > > Can you move these comments into psci-smp.c please? They're really specific
> > > > to the implementation there, and if we put them in a header we're lying to
> > > > ourselves about the parameters actually described by the PSCI specification.
> > > 
> > > You have a good point about the PSCI spec.
> > > 
> > > However from the Linux POV these comments should regard the functions
> > > exported by psci_operations, not the firmware interface, this is why I
> > > think it makes sense to keep them in psci.h.
> > > What we are saying is for example that psci_operations.cpu_on returns 0
> > > on success and < 0 on failure, and it takes a cpuid and an entry point
> > > as parameters. We are not saying anything about the firmware interface.
> > 
> > I disagree. You're explicitly stating that we pass the `cpuid of target CPU,
> > as from MPIDR'. That's simply not true -- the firmware could choose any
> > numbering scheme to identify the CPUs. For KVM and Xen, it *is* the mpidr,
> > which is why psci-smp.c works at all, but that's where the comment belongs,
> > not in this header file.
> 
> At some point, the _kernel_ API for interfacing with the firmware's PSCI 
> will have to ensure uniformity somehow.  The PSCI interface code could 
> translate the passed MPIDR into whatever the firmware decided to use for 
> identifying CPUs if needed, keeping this issue localized.

That is what I had in mind when I said to keep the comment in psci.h
before.
We have to draw the line somewhere to expose a uniform internal kernel
API. However it is a bit difficult to do now given that we have only one
user of the API.

I don't feel to strongly about this, please let me know what is the
final decision and I'll update the code accordingly. I remind you that
the merge window is approaching :-)

[PATCH v8 1/2] arm: introduce psci_smp_ops

[Will Deacon]

On Fri, Apr 26, 2013 at 04:36:26PM +0100, Stefano Stabellini wrote:
> On Thu, 25 Apr 2013, Nicolas Pitre wrote:
> > On Thu, 25 Apr 2013, Will Deacon wrote:
> > > I disagree. You're explicitly stating that we pass the `cpuid of target CPU,
> > > as from MPIDR'. That's simply not true -- the firmware could choose any
> > > numbering scheme to identify the CPUs. For KVM and Xen, it *is* the mpidr,
> > > which is why psci-smp.c works at all, but that's where the comment belongs,
> > > not in this header file.
> > 
> > At some point, the _kernel_ API for interfacing with the firmware's PSCI 
> > will have to ensure uniformity somehow.  The PSCI interface code could 
> > translate the passed MPIDR into whatever the firmware decided to use for 
> > identifying CPUs if needed, keeping this issue localized.
> 
> That is what I had in mind when I said to keep the comment in psci.h
> before.
> We have to draw the line somewhere to expose a uniform internal kernel
> API. However it is a bit difficult to do now given that we have only one
> user of the API.

I see psci.h as representing the firmware interface, and psci-smp.c or
whatever sits on top as exposing the kernel `API'.

> I don't feel to strongly about this, please let me know what is the
> final decision and I'll update the code accordingly. I remind you that
> the merge window is approaching :-)

I'd still like the comment to be in psci-smp.c, or a header separate from
the firmware bits.

Will

[PATCH v8 2/2] ARM: Enable selection of SMP operations at boot time

[Stefano Stabellini]

From: Jon Medhurst <tixy@linaro.org>

Add a new 'smp_init' hook to machine_desc so platforms can specify a
function to be used to setup smp ops instead of having a statically
defined value.  The hook must return true when smp_ops are initialized.
If false the static mdesc->smp_ops will be used by default.

Signed-off-by: Jon Medhurst <tixy@linaro.org>
Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 arch/arm/include/asm/mach/arch.h |    4 ++++
 arch/arm/kernel/setup.c          |   10 ++++++----
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
index 308ad7d..4669e0b 100644
--- a/arch/arm/include/asm/mach/arch.h
+++ b/arch/arm/include/asm/mach/arch.h
@@ -16,8 +16,10 @@ struct pt_regs;
 struct smp_operations;
 #ifdef CONFIG_SMP
 #define smp_ops(ops) (&(ops))
+#define smp_init_ops(ops) (&(ops))
 #else
 #define smp_ops(ops) (struct smp_operations *)NULL
+#define smp_init_ops(ops) (bool (*)(void))NULL
 #endif
 
 struct machine_desc {
@@ -41,6 +43,8 @@ struct machine_desc {
 	unsigned char		reserve_lp2 :1;	/* never has lp2	*/
 	char			restart_mode;	/* default restart mode	*/
 	struct smp_operations	*smp;		/* SMP operations	*/
+	/* Detect smp_ops dynamically, based on DT */
+	bool			(*smp_init)(void);
 	void			(*fixup)(struct tag *, char **,
 					 struct meminfo *);
 	void			(*reserve)(void);/* reserve mem blocks	*/
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index dad3048..22cc863 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -770,10 +770,12 @@ void __init setup_arch(char **cmdline_p)
 	psci_init();
 #ifdef CONFIG_SMP
 	if (is_smp()) {
-		if (psci_smp_available())
-			smp_set_ops(&psci_smp_ops);
-		else if (mdesc->smp)
-			smp_set_ops(mdesc->smp);
+		if (!mdesc->smp_init || !mdesc->smp_init()) {
+			if (psci_smp_available())
+				smp_set_ops(&psci_smp_ops);
+			else if (mdesc->smp)
+				smp_set_ops(mdesc->smp);
+		}
 		smp_init_cpus();
 	}
 #endif
-- 
1.7.2.5

[PATCH v8 2/2] ARM: Enable selection of SMP operations at boot time

[Will Deacon]

On Wed, Apr 24, 2013 at 07:40:19PM +0100, Stefano Stabellini wrote:
> From: Jon Medhurst <tixy@linaro.org>
> 
> Add a new 'smp_init' hook to machine_desc so platforms can specify a
> function to be used to setup smp ops instead of having a statically
> defined value.  The hook must return true when smp_ops are initialized.
> If false the static mdesc->smp_ops will be used by default.
> 
> Signed-off-by: Jon Medhurst <tixy@linaro.org>
> Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> ---
>  arch/arm/include/asm/mach/arch.h |    4 ++++
>  arch/arm/kernel/setup.c          |   10 ++++++----
>  2 files changed, 10 insertions(+), 4 deletions(-)

[...]

>  	if (is_smp()) {
> -		if (psci_smp_available())
> -			smp_set_ops(&psci_smp_ops);
> -		else if (mdesc->smp)
> -			smp_set_ops(mdesc->smp);
> +		if (!mdesc->smp_init || !mdesc->smp_init()) {

Minor nit, but this feels backwards to me. We usually return 0 on success,
yet we're saying here that if mdesc->smp_init() returns 0, then we go and
override the smp ops.

Will

> +			if (psci_smp_available())
> +				smp_set_ops(&psci_smp_ops);
> +			else if (mdesc->smp)
> +				smp_set_ops(mdesc->smp);
> +		}
>  		smp_init_cpus();
>  	}
>  #endif
> -- 
> 1.7.2.5
> 
> 

[PATCH v8 2/2] ARM: Enable selection of SMP operations at boot time

[Stefano Stabellini]

On Thu, 25 Apr 2013, Will Deacon wrote:
> On Wed, Apr 24, 2013 at 07:40:19PM +0100, Stefano Stabellini wrote:
> > From: Jon Medhurst <tixy@linaro.org>
> > 
> > Add a new 'smp_init' hook to machine_desc so platforms can specify a
> > function to be used to setup smp ops instead of having a statically
> > defined value.  The hook must return true when smp_ops are initialized.
> > If false the static mdesc->smp_ops will be used by default.
> > 
> > Signed-off-by: Jon Medhurst <tixy@linaro.org>
> > Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> > ---
> >  arch/arm/include/asm/mach/arch.h |    4 ++++
> >  arch/arm/kernel/setup.c          |   10 ++++++----
> >  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> [...]
> 
> >  	if (is_smp()) {
> > -		if (psci_smp_available())
> > -			smp_set_ops(&psci_smp_ops);
> > -		else if (mdesc->smp)
> > -			smp_set_ops(mdesc->smp);
> > +		if (!mdesc->smp_init || !mdesc->smp_init()) {
> 
> Minor nit, but this feels backwards to me. We usually return 0 on success,
> yet we're saying here that if mdesc->smp_init() returns 0, then we go and
> override the smp ops.

I am OK with changing the interface to return an int instead of a bool,
but given that Nicolas is the main user of smp_init at the moment, I'll
wait for his opinion.

[PATCH v8 2/2] ARM: Enable selection of SMP operations at boot time

[Nicolas Pitre]

On Thu, 25 Apr 2013, Will Deacon wrote:

> On Wed, Apr 24, 2013 at 07:40:19PM +0100, Stefano Stabellini wrote:
> > From: Jon Medhurst <tixy@linaro.org>
> > 
> > Add a new 'smp_init' hook to machine_desc so platforms can specify a
> > function to be used to setup smp ops instead of having a statically
> > defined value.  The hook must return true when smp_ops are initialized.
> > If false the static mdesc->smp_ops will be used by default.
> > 
> > Signed-off-by: Jon Medhurst <tixy@linaro.org>
> > Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> > ---
> >  arch/arm/include/asm/mach/arch.h |    4 ++++
> >  arch/arm/kernel/setup.c          |   10 ++++++----
> >  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> [...]
> 
> >  	if (is_smp()) {
> > -		if (psci_smp_available())
> > -			smp_set_ops(&psci_smp_ops);
> > -		else if (mdesc->smp)
> > -			smp_set_ops(mdesc->smp);
> > +		if (!mdesc->smp_init || !mdesc->smp_init()) {
> 
> Minor nit, but this feels backwards to me. We usually return 0 on success,
> yet we're saying here that if mdesc->smp_init() returns 0, then we go and
> override the smp ops.

It doesn't return 0, but true or false.  So, semantically, if ->smp_init 
returns false, that means it didn't initialize anything.


Nicolas

[PATCH v8 2/2] ARM: Enable selection of SMP operations at boot time

[Stefano Stabellini]

On Thu, 25 Apr 2013, Nicolas Pitre wrote:
> On Thu, 25 Apr 2013, Will Deacon wrote:
> 
> > On Wed, Apr 24, 2013 at 07:40:19PM +0100, Stefano Stabellini wrote:
> > > From: Jon Medhurst <tixy@linaro.org>
> > > 
> > > Add a new 'smp_init' hook to machine_desc so platforms can specify a
> > > function to be used to setup smp ops instead of having a statically
> > > defined value.  The hook must return true when smp_ops are initialized.
> > > If false the static mdesc->smp_ops will be used by default.
> > > 
> > > Signed-off-by: Jon Medhurst <tixy@linaro.org>
> > > Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> > > ---
> > >  arch/arm/include/asm/mach/arch.h |    4 ++++
> > >  arch/arm/kernel/setup.c          |   10 ++++++----
> > >  2 files changed, 10 insertions(+), 4 deletions(-)
> > 
> > [...]
> > 
> > >  	if (is_smp()) {
> > > -		if (psci_smp_available())
> > > -			smp_set_ops(&psci_smp_ops);
> > > -		else if (mdesc->smp)
> > > -			smp_set_ops(mdesc->smp);
> > > +		if (!mdesc->smp_init || !mdesc->smp_init()) {
> > 
> > Minor nit, but this feels backwards to me. We usually return 0 on success,
> > yet we're saying here that if mdesc->smp_init() returns 0, then we go and
> > override the smp ops.
> 
> It doesn't return 0, but true or false.  So, semantically, if ->smp_init 
> returns false, that means it didn't initialize anything.

In absence of other comments, I am going to leave as it is now.