[RFC PATCH v2 0/3] Per SoC descriptor

[RFC PATCH v2 0/3] Per SoC descriptor

[Marc Zyngier]

This patch series introduces a per-soc descriptor which should, in the
end, contain most of the SoC specific operations.

This first patch series introduces the arm_soc_desc structure, adds
per-soc SMP and CPU hotplug operations, and converts both RealView and
VExpress to this new scheme.

Should there be a consensus on these patches, I'll convert all the
other SMP platforms.

Patches against next-20110831. Tested on VExpress (A5 and A15) and
RealView EB-11MP.

[RFC PATCH v2 1/3] ARM: SoC: Introduce per SoC descriptor

[Marc Zyngier]

The ARM core code expects the various SoCs to hide their
implementation differences behind a well established API.
The various sub-arch-specific bit are often either at
the machine descriptor level, or provided at link time
by the sub-arch code.

The SoC descriptor is a container that holds the SoC
specific bits that can be moved away from the machine
descriptor as well as an indirection point for the
global symbols (SMP and CPU hotplug support, for example).

This patch introduce this SoC descriptor, with the only field
being the name of the SoC.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Nicolas Pitre <nico@fluxnic.net>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/mach/arch.h |    2 ++
 arch/arm/include/asm/soc.h       |   21 +++++++++++++++++++++
 arch/arm/kernel/setup.c          |    9 +++++++++
 3 files changed, 32 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/include/asm/soc.h

diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
index 727da11..82883ca 100644
--- a/arch/arm/include/asm/mach/arch.h
+++ b/arch/arm/include/asm/mach/arch.h
@@ -13,6 +13,7 @@
 struct tag;
 struct meminfo;
 struct sys_timer;
+struct arm_soc_desc;
 
 struct machine_desc {
 	unsigned int		nr;		/* architecture number	*/
@@ -34,6 +35,7 @@ struct machine_desc {
 	unsigned int		reserve_lp1 :1;	/* never has lp1	*/
 	unsigned int		reserve_lp2 :1;	/* never has lp2	*/
 	unsigned int		soft_reboot :1;	/* soft reboot		*/
+	struct arm_soc_desc	*soc;		/* SoC descriptor	*/
 	void			(*fixup)(struct machine_desc *,
 					 struct tag *, char **,
 					 struct meminfo *);
diff --git a/arch/arm/include/asm/soc.h b/arch/arm/include/asm/soc.h
new file mode 100644
index 0000000..ce92784
--- /dev/null
+++ b/arch/arm/include/asm/soc.h
@@ -0,0 +1,21 @@
+/*
+ *  linux/arch/arm/include/asm/soc.h
+ *
+ *  Copyright (C) 2011 ARM Ltd.
+ *  All Rights Reserved
+ *
+ * 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.
+ */
+
+#ifndef __ASM_ARM_SOC_H
+#define __ASM_ARM_SOC_H
+
+struct arm_soc_desc {
+	const char		*name;
+};
+
+extern const struct arm_soc_desc	*soc_desc;
+
+#endif	/* __ASM_ARM_SOC_H */
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 192a24f..6bfa5f6 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -45,6 +45,7 @@
 #include <asm/cachetype.h>
 #include <asm/tlbflush.h>
 #include <asm/system.h>
+#include <asm/soc.h>
 
 #include <asm/prom.h>
 #include <asm/mach/arch.h>
@@ -140,6 +141,8 @@ static const char *cpu_name;
 static const char *machine_name;
 static char __initdata cmd_line[COMMAND_LINE_SIZE];
 struct machine_desc *machine_desc __initdata;
+const struct arm_soc_desc *soc_desc;
+static struct arm_soc_desc __soc_desc __read_mostly;
 
 static char default_command_line[COMMAND_LINE_SIZE] __initdata = CONFIG_CMDLINE;
 static union { char c[4]; unsigned long l; } endian_test __initdata = { { 'l', '?', '?', 'b' } };
@@ -909,6 +912,12 @@ void __init setup_arch(char **cmdline_p)
 		mdesc = setup_machine_tags(machine_arch_type);
 	machine_desc = mdesc;
 	machine_name = mdesc->name;
+	if (mdesc->soc) {
+		__soc_desc = *mdesc->soc;
+		soc_desc = &__soc_desc;
+		pr_info("SoC: %s\n", soc_desc->name);
+	} else
+		soc_desc = NULL;
 
 	if (mdesc->soft_reboot)
 		reboot_setup("s");
-- 
1.7.0.4

[RFC PATCH v2 2/3] ARM: SoC: Add per SoC SMP and CPU hotplug operations

[Marc Zyngier]

Populate the SoC descriptor structure with the SMP and CPU hotplug
operations. To allow the kernel to continue building, the platform
hooks are defined as weak symbols which are overrided by the
platform code. Once all platforms are converted, the "weak" attribute
will be removed and the function made static.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Nicolas Pitre <nico@fluxnic.net>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/soc.h |   18 ++++++++++++++++
 arch/arm/kernel/setup.c    |   11 ++++++++++
 arch/arm/kernel/smp.c      |   47 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/soc.h b/arch/arm/include/asm/soc.h
index ce92784..2593f90 100644
--- a/arch/arm/include/asm/soc.h
+++ b/arch/arm/include/asm/soc.h
@@ -12,10 +12,28 @@
 #ifndef __ASM_ARM_SOC_H
 #define __ASM_ARM_SOC_H
 
+struct task_struct;
+
+struct arm_soc_smp_ops {
+	void (*smp_init_cpus)(void);
+	void (*smp_prepare_cpus)(unsigned int max_cpus);
+	void (*smp_secondary_init)(unsigned int cpu);
+	int  (*smp_boot_secondary)(unsigned int cpu, struct task_struct *idle);
+#ifdef CONFIG_HOTPLUG_CPU
+	int  (*cpu_kill)(unsigned int cpu);
+	void (*cpu_die)(unsigned int cpu);
+	int  (*cpu_disable)(unsigned int cpu);
+#endif
+};
+
 struct arm_soc_desc {
 	const char		*name;
+#ifdef CONFIG_SMP
+	struct arm_soc_smp_ops	*smp_ops;
+#endif
 };
 
 extern const struct arm_soc_desc	*soc_desc;
+extern const struct arm_soc_smp_ops	*soc_smp_ops;
 
 #endif	/* __ASM_ARM_SOC_H */
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 6bfa5f6..972e43f 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -143,6 +143,10 @@ static char __initdata cmd_line[COMMAND_LINE_SIZE];
 struct machine_desc *machine_desc __initdata;
 const struct arm_soc_desc *soc_desc;
 static struct arm_soc_desc __soc_desc __read_mostly;
+#ifdef CONFIG_SMP
+const struct arm_soc_smp_ops *soc_smp_ops;
+static struct arm_soc_smp_ops __soc_smp_ops __read_mostly;
+#endif
 
 static char default_command_line[COMMAND_LINE_SIZE] __initdata = CONFIG_CMDLINE;
 static union { char c[4]; unsigned long l; } endian_test __initdata = { { 'l', '?', '?', 'b' } };
@@ -918,6 +922,13 @@ void __init setup_arch(char **cmdline_p)
 		pr_info("SoC: %s\n", soc_desc->name);
 	} else
 		soc_desc = NULL;
+#ifdef CONFIG_SMP
+	if (soc_desc && soc_desc->smp_ops) {
+		__soc_smp_ops = *soc_desc->smp_ops;
+		soc_smp_ops = &__soc_smp_ops;
+	} else
+		soc_smp_ops = NULL;
+#endif
 
 	if (mdesc->soft_reboot)
 		reboot_setup("s");
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 3f12ce9..8cb83ff 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -28,6 +28,7 @@
 #include <linux/completion.h>
 
 #include <linux/atomic.h>
+#include <asm/soc.h>
 #include <asm/cacheflush.h>
 #include <asm/cpu.h>
 #include <asm/cputype.h>
@@ -155,9 +156,55 @@ int __cpuinit __cpu_up(unsigned int cpu)
 	return ret;
 }
 
+/* SoC helpers */
+void __attribute__((weak)) smp_init_cpus(void)
+{
+	if (soc_smp_ops && soc_smp_ops->smp_init_cpus)
+		soc_smp_ops->smp_init_cpus();
+}
+
+void __attribute__((weak)) platform_smp_prepare_cpus(unsigned int max_cpus)
+{
+	if (soc_smp_ops && soc_smp_ops->smp_prepare_cpus)
+		soc_smp_ops->smp_prepare_cpus(max_cpus);
+}
+
+void __attribute__((weak)) platform_secondary_init(unsigned int cpu)
+{
+	if (soc_smp_ops && soc_smp_ops->smp_secondary_init)
+		soc_smp_ops->smp_secondary_init(cpu);
+}
+
+int __attribute__((weak)) boot_secondary(unsigned int cpu, struct task_struct *idle)
+{
+	if (soc_smp_ops && soc_smp_ops->smp_boot_secondary)
+		return soc_smp_ops->smp_boot_secondary(cpu, idle);
+	return -ENOSYS;
+}
+
 #ifdef CONFIG_HOTPLUG_CPU
 static void percpu_timer_stop(void);
 
+int __attribute__((weak)) platform_cpu_kill(unsigned int cpu)
+{
+	if (soc_smp_ops && soc_smp_ops->cpu_kill)
+		return soc_smp_ops->cpu_kill(cpu);
+	return 0;
+}
+
+void __attribute__((weak)) platform_cpu_die(unsigned int cpu)
+{
+	if (soc_smp_ops && soc_smp_ops->cpu_die)
+		soc_smp_ops->cpu_die(cpu);
+}
+
+int __attribute__((weak)) platform_cpu_disable(unsigned int cpu)
+{
+	if (soc_smp_ops && soc_smp_ops->cpu_disable)
+		return soc_smp_ops->cpu_disable(cpu);
+	return -EPERM;
+}
+
 /*
  * __cpu_disable runs on the processor to be shutdown.
  */
-- 
1.7.0.4

[RFC PATCH v2 2/3] ARM: SoC: Add per SoC SMP and CPU hotplug operations

[Santosh]

Marc,

On Friday 09 September 2011 08:16 PM, Marc Zyngier wrote:
> Populate the SoC descriptor structure with the SMP and CPU hotplug
> operations. To allow the kernel to continue building, the platform
> hooks are defined as weak symbols which are overrided by the
> platform code. Once all platforms are converted, the "weak" attribute
> will be removed and the function made static.
>
> Cc: Arnd Bergmann<arnd@arndb.de>
> Cc: Nicolas Pitre<nico@fluxnic.net>
> Signed-off-by: Marc Zyngier<marc.zyngier@arm.com>
> ---
>   arch/arm/include/asm/soc.h |   18 ++++++++++++++++
>   arch/arm/kernel/setup.c    |   11 ++++++++++
>   arch/arm/kernel/smp.c      |   47 ++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 76 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/include/asm/soc.h b/arch/arm/include/asm/soc.h
> index ce92784..2593f90 100644
> --- a/arch/arm/include/asm/soc.h
> +++ b/arch/arm/include/asm/soc.h
> @@ -12,10 +12,28 @@
>   #ifndef __ASM_ARM_SOC_H
>   #define __ASM_ARM_SOC_H
>
> +struct task_struct;
> +
> +struct arm_soc_smp_ops {
> +	void (*smp_init_cpus)(void);
> +	void (*smp_prepare_cpus)(unsigned int max_cpus);
> +	void (*smp_secondary_init)(unsigned int cpu);
> +	int  (*smp_boot_secondary)(unsigned int cpu, struct task_struct *idle);
> +#ifdef CONFIG_HOTPLUG_CPU
> +	int  (*cpu_kill)(unsigned int cpu);
> +	void (*cpu_die)(unsigned int cpu);
> +	int  (*cpu_disable)(unsigned int cpu);
> +#endif
> +};
Sorry for such a basic question but I don't understand the need
of these wrappers.
I am not upto speed on this topic but what is the motivation
behind the soc_smp_ops(). All of above functions are CPU specific
and not really soc specific though, I agree that every SOC,
implements it's own version.

Regards
Santosh

[RFC PATCH v2 2/3] ARM: SoC: Add per SoC SMP and CPU hotplug operations

[Arnd Bergmann]

On Friday 09 September 2011, Santosh wrote:
> Sorry for such a basic question but I don't understand the need
> of these wrappers.
> I am not upto speed on this topic but what is the motivation
> behind the soc_smp_ops(). All of above functions are CPU specific
> and not really soc specific though, I agree that every SOC,
> implements it's own version.

We have two separate problems to solve that we should not confuse:

1. Get multiple soc platforms to compile together in a single kernel
   binary.
2. Share as much code as possible between similar soc platforms.

Marc's patch only addresses the first problem, and we need that
anyway if we want to get e.g. cortex-a9 based systems to be
supported in the same kernel as Qualcomm and Marvell CPU based
ones.

The other part is useful too, but it's easier to do after this one.

	Arnd

[RFC PATCH v2 2/3] ARM: SoC: Add per SoC SMP and CPU hotplug operations

[Santosh]

On Friday 09 September 2011 08:36 PM, Arnd Bergmann wrote:
> On Friday 09 September 2011, Santosh wrote:
>> Sorry for such a basic question but I don't understand the need
>> of these wrappers.
>> I am not upto speed on this topic but what is the motivation
>> behind the soc_smp_ops(). All of above functions are CPU specific
>> and not really soc specific though, I agree that every SOC,
>> implements it's own version.
>
> We have two separate problems to solve that we should not confuse:
>
> 1. Get multiple soc platforms to compile together in a single kernel
>     binary.
> 2. Share as much code as possible between similar soc platforms.
>
> Marc's patch only addresses the first problem, and we need that
> anyway if we want to get e.g. cortex-a9 based systems to be
> supported in the same kernel as Qualcomm and Marvell CPU based
> ones.
>
> The other part is useful too, but it's easier to do after this one.
>
Agree. Thanks for the clarification.

Regards
Santosh

[RFC PATCH v2 2/3] ARM: SoC: Add per SoC SMP and CPU hotplug operations

[Marc Zyngier]

Hi Santosh,

On 09/09/11 15:55, Santosh wrote:
> Marc,
> 
> On Friday 09 September 2011 08:16 PM, Marc Zyngier wrote:
>> Populate the SoC descriptor structure with the SMP and CPU hotplug
>> operations. To allow the kernel to continue building, the platform
>> hooks are defined as weak symbols which are overrided by the
>> platform code. Once all platforms are converted, the "weak" attribute
>> will be removed and the function made static.
>>
>> Cc: Arnd Bergmann<arnd@arndb.de>
>> Cc: Nicolas Pitre<nico@fluxnic.net>
>> Signed-off-by: Marc Zyngier<marc.zyngier@arm.com>
>> ---
>>   arch/arm/include/asm/soc.h |   18 ++++++++++++++++
>>   arch/arm/kernel/setup.c    |   11 ++++++++++
>>   arch/arm/kernel/smp.c      |   47 ++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 76 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/soc.h b/arch/arm/include/asm/soc.h
>> index ce92784..2593f90 100644
>> --- a/arch/arm/include/asm/soc.h
>> +++ b/arch/arm/include/asm/soc.h
>> @@ -12,10 +12,28 @@
>>   #ifndef __ASM_ARM_SOC_H
>>   #define __ASM_ARM_SOC_H
>>
>> +struct task_struct;
>> +
>> +struct arm_soc_smp_ops {
>> +	void (*smp_init_cpus)(void);
>> +	void (*smp_prepare_cpus)(unsigned int max_cpus);
>> +	void (*smp_secondary_init)(unsigned int cpu);
>> +	int  (*smp_boot_secondary)(unsigned int cpu, struct task_struct *idle);
>> +#ifdef CONFIG_HOTPLUG_CPU
>> +	int  (*cpu_kill)(unsigned int cpu);
>> +	void (*cpu_die)(unsigned int cpu);
>> +	int  (*cpu_disable)(unsigned int cpu);
>> +#endif
>> +};
> Sorry for such a basic question but I don't understand the need
> of these wrappers.
> I am not upto speed on this topic but what is the motivation
> behind the soc_smp_ops(). All of above functions are CPU specific
> and not really soc specific though, I agree that every SOC,
> implements it's own version.

My understanding is that they really are SoC specific. If you compare
OMAP4, Tegra and VExpress (for example), you'll notice that these
functions are quite different, despite using the same A9 CPU.

For example, you boot a secondary CPU on OMAP4 by trapping into secure
mode, on Tegra by powering it on, and on VE by writing the expected
value to some location.

Indirecting these functions makes it possible to compile support for
multiple SMP platforms into the same image. Probably it is possible to
reduce the differences to something smaller than the above, but I'd
rather change one thing at a time.

Let's add the indirection first (which essentially preserves the
existing internal API), and only then let's see if we can spot common
patterns across the whole range of SMP implementations.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[RFC PATCH v2 2/3] ARM: SoC: Add per SoC SMP and CPU hotplug operations

[Santosh]

On Friday 09 September 2011 08:43 PM, Marc Zyngier wrote:
> Hi Santosh,
>
> On 09/09/11 15:55, Santosh wrote:
>> Marc,
>>
>> On Friday 09 September 2011 08:16 PM, Marc Zyngier wrote:
>>> Populate the SoC descriptor structure with the SMP and CPU hotplug
>>> operations. To allow the kernel to continue building, the platform
>>> hooks are defined as weak symbols which are overrided by the
>>> platform code. Once all platforms are converted, the "weak" attribute
>>> will be removed and the function made static.
>>>
>>> Cc: Arnd Bergmann<arnd@arndb.de>
>>> Cc: Nicolas Pitre<nico@fluxnic.net>
>>> Signed-off-by: Marc Zyngier<marc.zyngier@arm.com>
>>> ---
>>>    arch/arm/include/asm/soc.h |   18 ++++++++++++++++
>>>    arch/arm/kernel/setup.c    |   11 ++++++++++
>>>    arch/arm/kernel/smp.c      |   47 ++++++++++++++++++++++++++++++++++++++++++++
>>>    3 files changed, 76 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/arm/include/asm/soc.h b/arch/arm/include/asm/soc.h
>>> index ce92784..2593f90 100644
>>> --- a/arch/arm/include/asm/soc.h
>>> +++ b/arch/arm/include/asm/soc.h
>>> @@ -12,10 +12,28 @@
>>>    #ifndef __ASM_ARM_SOC_H
>>>    #define __ASM_ARM_SOC_H
>>>
>>> +struct task_struct;
>>> +
>>> +struct arm_soc_smp_ops {
>>> +	void (*smp_init_cpus)(void);
>>> +	void (*smp_prepare_cpus)(unsigned int max_cpus);
>>> +	void (*smp_secondary_init)(unsigned int cpu);
>>> +	int  (*smp_boot_secondary)(unsigned int cpu, struct task_struct *idle);
>>> +#ifdef CONFIG_HOTPLUG_CPU
>>> +	int  (*cpu_kill)(unsigned int cpu);
>>> +	void (*cpu_die)(unsigned int cpu);
>>> +	int  (*cpu_disable)(unsigned int cpu);
>>> +#endif
>>> +};
>> Sorry for such a basic question but I don't understand the need
>> of these wrappers.
>> I am not upto speed on this topic but what is the motivation
>> behind the soc_smp_ops(). All of above functions are CPU specific
>> and not really soc specific though, I agree that every SOC,
>> implements it's own version.
>
> My understanding is that they really are SoC specific. If you compare
> OMAP4, Tegra and VExpress (for example), you'll notice that these
> functions are quite different, despite using the same A9 CPU.
>
> For example, you boot a secondary CPU on OMAP4 by trapping into secure
> mode, on Tegra by powering it on, and on VE by writing the expected
> value to some location.
>
> Indirecting these functions makes it possible to compile support for
> multiple SMP platforms into the same image. Probably it is possible to
> reduce the differences to something smaller than the above, but I'd
> rather change one thing at a time.
>
> Let's add the indirection first (which essentially preserves the
> existing internal API), and only then let's see if we can spot common
> patterns across the whole range of SMP implementations.
>
Thanks Marc for the heads up. I agree with the current approach

[RFC PATCH v2 2/3] ARM: SoC: Add per SoC SMP and CPU hotplug operations

[Nicolas Pitre]

On Fri, 9 Sep 2011, Marc Zyngier wrote:

> Populate the SoC descriptor structure with the SMP and CPU hotplug
> operations. To allow the kernel to continue building, the platform
> hooks are defined as weak symbols which are overrided by the
> platform code. Once all platforms are converted, the "weak" attribute
> will be removed and the function made static.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Nicolas Pitre <nico@fluxnic.net>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Comments below.

> ---
>  arch/arm/include/asm/soc.h |   18 ++++++++++++++++
>  arch/arm/kernel/setup.c    |   11 ++++++++++
>  arch/arm/kernel/smp.c      |   47 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 76 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/include/asm/soc.h b/arch/arm/include/asm/soc.h
> index ce92784..2593f90 100644
> --- a/arch/arm/include/asm/soc.h
> +++ b/arch/arm/include/asm/soc.h
> @@ -12,10 +12,28 @@
>  #ifndef __ASM_ARM_SOC_H
>  #define __ASM_ARM_SOC_H
>  
> +struct task_struct;
> +
> +struct arm_soc_smp_ops {
> +	void (*smp_init_cpus)(void);
> +	void (*smp_prepare_cpus)(unsigned int max_cpus);
> +	void (*smp_secondary_init)(unsigned int cpu);
> +	int  (*smp_boot_secondary)(unsigned int cpu, struct task_struct *idle);
> +#ifdef CONFIG_HOTPLUG_CPU
> +	int  (*cpu_kill)(unsigned int cpu);
> +	void (*cpu_die)(unsigned int cpu);
> +	int  (*cpu_disable)(unsigned int cpu);
> +#endif
> +};

I don't like this aggregation.  Some of those functions are marked 
__init while some are not.  This is going to cause all sorts of bugs 
because people will confuse the lifetime rules for those functions.  The 
SMP init stuff has to be clearly separated from the functions that 
remain alive after boot.

>  struct arm_soc_desc {
>  	const char		*name;
> +#ifdef CONFIG_SMP
> +	struct arm_soc_smp_ops	*smp_ops;
> +#endif
>  };
>  
>  extern const struct arm_soc_desc	*soc_desc;
> +extern const struct arm_soc_smp_ops	*soc_smp_ops;
>  
>  #endif	/* __ASM_ARM_SOC_H */
> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> index 6bfa5f6..972e43f 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -143,6 +143,10 @@ static char __initdata cmd_line[COMMAND_LINE_SIZE];
>  struct machine_desc *machine_desc __initdata;
>  const struct arm_soc_desc *soc_desc;
>  static struct arm_soc_desc __soc_desc __read_mostly;
> +#ifdef CONFIG_SMP
> +const struct arm_soc_smp_ops *soc_smp_ops;
> +static struct arm_soc_smp_ops __soc_smp_ops __read_mostly;
> +#endif

Here in both cases you declare a pointer and storage for this data.  I 
suppose you did so in order to have the various SOC structures be marked 
__initdata discarded and only preserve the used one after boot.  But 
in doing so you are completely bypassing the linker's ability to identify 
bad cross section references (such as non __init code calling into an 
__init function).

I think this would be more appropriate to explicitly copy _only_ the 
references to data/code that is not located in a discarded section.  In 
practice the stuff here is probably going to be almost initialization 
stuff anyway and copying a reference to it into permanent storage is 
wrong.



Nicolas

[RFC PATCH v2 3/3] ARM: SoC: convert VExpress/RealView to SoC descriptor

[Marc Zyngier]

Convert both Realview and VExpress to use the SoC descriptor to
provide their SMP and CPU hotplug operation.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Nicolas Pitre <nico@fluxnic.net>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/mach-realview/core.c                  |    8 ++++++++
 arch/arm/mach-realview/core.h                  |    8 ++++++++
 arch/arm/mach-realview/hotplug.c               |    6 +++---
 arch/arm/mach-realview/platsmp.c               |   21 +++++++++++++++++----
 arch/arm/mach-realview/realview_eb.c           |    1 +
 arch/arm/mach-realview/realview_pb1176.c       |    1 +
 arch/arm/mach-realview/realview_pb11mp.c       |    1 +
 arch/arm/mach-realview/realview_pba8.c         |    1 +
 arch/arm/mach-realview/realview_pbx.c          |    1 +
 arch/arm/mach-vexpress/core.h                  |    8 ++++++++
 arch/arm/mach-vexpress/hotplug.c               |    6 +++---
 arch/arm/mach-vexpress/platsmp.c               |   21 +++++++++++++++++----
 arch/arm/mach-vexpress/v2m.c                   |   10 ++++++++++
 arch/arm/plat-versatile/include/plat/platsmp.h |   14 ++++++++++++++
 arch/arm/plat-versatile/platsmp.c              |    4 ++--
 15 files changed, 95 insertions(+), 16 deletions(-)
 create mode 100644 arch/arm/plat-versatile/include/plat/platsmp.h

diff --git a/arch/arm/mach-realview/core.c b/arch/arm/mach-realview/core.c
index 5c23450..6fbb951 100644
--- a/arch/arm/mach-realview/core.c
+++ b/arch/arm/mach-realview/core.c
@@ -33,6 +33,7 @@
 #include <linux/clkdev.h>
 #include <linux/mtd/physmap.h>
 
+#include <asm/soc.h>
 #include <asm/system.h>
 #include <mach/hardware.h>
 #include <asm/irq.h>
@@ -534,3 +535,10 @@ void realview_fixup(struct machine_desc *mdesc, struct tag *tags, char **from,
 	meminfo->nr_banks = 1;
 #endif
 }
+
+struct arm_soc_desc realview_soc_desc __initdata = {
+	.name		= "ARM RealView Platform",
+#ifdef CONFIG_SMP
+	.smp_ops	= &realview_soc_smp_ops,
+#endif
+};
diff --git a/arch/arm/mach-realview/core.h b/arch/arm/mach-realview/core.h
index 5c83d1e..b71a802 100644
--- a/arch/arm/mach-realview/core.h
+++ b/arch/arm/mach-realview/core.h
@@ -27,6 +27,7 @@
 
 #include <asm/setup.h>
 #include <asm/leds.h>
+#include <asm/soc.h>
 
 #define AMBA_DEVICE(name,busid,base,plat)			\
 static struct amba_device name##_device = {			\
@@ -67,4 +68,11 @@ extern void realview_fixup(struct machine_desc *mdesc, struct tag *tags,
 			   char **from, struct meminfo *meminfo);
 extern void (*realview_reset)(char);
 
+extern struct arm_soc_desc	realview_soc_desc;
+extern struct arm_soc_smp_ops	realview_soc_smp_ops;
+
+extern int  realview_cpu_kill(unsigned int cpu);
+extern void realview_cpu_die(unsigned int cpu);
+extern int  realview_cpu_disable(unsigned int cpu);
+
 #endif
diff --git a/arch/arm/mach-realview/hotplug.c b/arch/arm/mach-realview/hotplug.c
index a87523d..ce28104 100644
--- a/arch/arm/mach-realview/hotplug.c
+++ b/arch/arm/mach-realview/hotplug.c
@@ -87,7 +87,7 @@ static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
 	}
 }
 
-int platform_cpu_kill(unsigned int cpu)
+int realview_cpu_kill(unsigned int cpu)
 {
 	return 1;
 }
@@ -97,7 +97,7 @@ int platform_cpu_kill(unsigned int cpu)
  *
  * Called with IRQs disabled
  */
-void platform_cpu_die(unsigned int cpu)
+void realview_cpu_die(unsigned int cpu)
 {
 	int spurious = 0;
 
@@ -117,7 +117,7 @@ void platform_cpu_die(unsigned int cpu)
 		pr_warn("CPU%u: %u spurious wakeup calls\n", cpu, spurious);
 }
 
-int platform_cpu_disable(unsigned int cpu)
+int realview_cpu_disable(unsigned int cpu)
 {
 	/*
 	 * we don't allow CPU 0 to be shutdown (it is still too special
diff --git a/arch/arm/mach-realview/platsmp.c b/arch/arm/mach-realview/platsmp.c
index 4ae943b..417f77a 100644
--- a/arch/arm/mach-realview/platsmp.c
+++ b/arch/arm/mach-realview/platsmp.c
@@ -18,14 +18,15 @@
 #include <asm/mach-types.h>
 #include <asm/smp_scu.h>
 #include <asm/unified.h>
+#include <asm/soc.h>
 
 #include <mach/board-eb.h>
 #include <mach/board-pb11mp.h>
 #include <mach/board-pbx.h>
 
-#include "core.h"
+#include <plat/platsmp.h>
 
-extern void versatile_secondary_startup(void);
+#include "core.h"
 
 static void __iomem *scu_base_addr(void)
 {
@@ -44,7 +45,7 @@ static void __iomem *scu_base_addr(void)
  * Initialise the CPU possible map early - this describes the CPUs
  * which may be present or become present in the system.
  */
-void __init smp_init_cpus(void)
+static void __init realview_smp_init_cpus(void)
 {
 	void __iomem *scu_base = scu_base_addr();
 	unsigned int i, ncores;
@@ -66,7 +67,7 @@ void __init smp_init_cpus(void)
 	set_smp_cross_call(gic_raise_softirq);
 }
 
-void __init platform_smp_prepare_cpus(unsigned int max_cpus)
+static void __init realview_smp_prepare_cpus(unsigned int max_cpus)
 {
 
 	scu_enable(scu_base_addr());
@@ -80,3 +81,15 @@ void __init platform_smp_prepare_cpus(unsigned int max_cpus)
 	__raw_writel(BSYM(virt_to_phys(versatile_secondary_startup)),
 		     __io_address(REALVIEW_SYS_FLAGSSET));
 }
+
+struct arm_soc_smp_ops realview_soc_smp_ops __initdata = {
+	.smp_init_cpus		= realview_smp_init_cpus,
+	.smp_prepare_cpus	= realview_smp_prepare_cpus,
+	.smp_secondary_init	= versatile_secondary_init,
+	.smp_boot_secondary	= versatile_boot_secondary,
+#ifdef CONFIG_HOTPLUG_CPU
+	.cpu_kill		= realview_cpu_kill,
+	.cpu_die		= realview_cpu_die,
+	.cpu_disable		= realview_cpu_disable,
+#endif
+};
diff --git a/arch/arm/mach-realview/realview_eb.c b/arch/arm/mach-realview/realview_eb.c
index 026c66a..427e44e 100644
--- a/arch/arm/mach-realview/realview_eb.c
+++ b/arch/arm/mach-realview/realview_eb.c
@@ -464,6 +464,7 @@ static void __init realview_eb_init(void)
 MACHINE_START(REALVIEW_EB, "ARM-RealView EB")
 	/* Maintainer: ARM Ltd/Deep Blue Solutions Ltd */
 	.atag_offset	= 0x100,
+	.soc		= &realview_soc_desc,
 	.fixup		= realview_fixup,
 	.map_io		= realview_eb_map_io,
 	.init_early	= realview_init_early,
diff --git a/arch/arm/mach-realview/realview_pb1176.c b/arch/arm/mach-realview/realview_pb1176.c
index 7263dea..863f286 100644
--- a/arch/arm/mach-realview/realview_pb1176.c
+++ b/arch/arm/mach-realview/realview_pb1176.c
@@ -359,6 +359,7 @@ static void __init realview_pb1176_init(void)
 MACHINE_START(REALVIEW_PB1176, "ARM-RealView PB1176")
 	/* Maintainer: ARM Ltd/Deep Blue Solutions Ltd */
 	.atag_offset	= 0x100,
+	.soc		= &realview_soc_desc,
 	.fixup		= realview_pb1176_fixup,
 	.map_io		= realview_pb1176_map_io,
 	.init_early	= realview_init_early,
diff --git a/arch/arm/mach-realview/realview_pb11mp.c b/arch/arm/mach-realview/realview_pb11mp.c
index 671ad6d..6cb8318 100644
--- a/arch/arm/mach-realview/realview_pb11mp.c
+++ b/arch/arm/mach-realview/realview_pb11mp.c
@@ -361,6 +361,7 @@ static void __init realview_pb11mp_init(void)
 MACHINE_START(REALVIEW_PB11MP, "ARM-RealView PB11MPCore")
 	/* Maintainer: ARM Ltd/Deep Blue Solutions Ltd */
 	.atag_offset	= 0x100,
+	.soc		= &realview_soc_desc,
 	.fixup		= realview_fixup,
 	.map_io		= realview_pb11mp_map_io,
 	.init_early	= realview_init_early,
diff --git a/arch/arm/mach-realview/realview_pba8.c b/arch/arm/mach-realview/realview_pba8.c
index cbf22df..3db72c5 100644
--- a/arch/arm/mach-realview/realview_pba8.c
+++ b/arch/arm/mach-realview/realview_pba8.c
@@ -311,6 +311,7 @@ static void __init realview_pba8_init(void)
 MACHINE_START(REALVIEW_PBA8, "ARM-RealView PB-A8")
 	/* Maintainer: ARM Ltd/Deep Blue Solutions Ltd */
 	.atag_offset	= 0x100,
+	.soc		= &realview_soc_desc,
 	.fixup		= realview_fixup,
 	.map_io		= realview_pba8_map_io,
 	.init_early	= realview_init_early,
diff --git a/arch/arm/mach-realview/realview_pbx.c b/arch/arm/mach-realview/realview_pbx.c
index 8ec7e52..7e5d540 100644
--- a/arch/arm/mach-realview/realview_pbx.c
+++ b/arch/arm/mach-realview/realview_pbx.c
@@ -394,6 +394,7 @@ static void __init realview_pbx_init(void)
 MACHINE_START(REALVIEW_PBX, "ARM-RealView PBX")
 	/* Maintainer: ARM Ltd/Deep Blue Solutions Ltd */
 	.atag_offset	= 0x100,
+	.soc		= &realview_soc_desc,
 	.fixup		= realview_pbx_fixup,
 	.map_io		= realview_pbx_map_io,
 	.init_early	= realview_init_early,
diff --git a/arch/arm/mach-vexpress/core.h b/arch/arm/mach-vexpress/core.h
index f439715..e0fb5c7 100644
--- a/arch/arm/mach-vexpress/core.h
+++ b/arch/arm/mach-vexpress/core.h
@@ -17,3 +17,11 @@ struct amba_device name##_device = {		\
 	.irq		= IRQ_##base,		\
 	/* .dma		= DMA_##base,*/		\
 }
+
+struct arm_soc_smp_ops;
+
+extern struct arm_soc_smp_ops vexpress_soc_smp_ops;
+
+extern int  vexpress_cpu_kill(unsigned int cpu);
+extern void vexpress_cpu_die(unsigned int cpu);
+extern int  vexpress_cpu_disable(unsigned int cpu);
diff --git a/arch/arm/mach-vexpress/hotplug.c b/arch/arm/mach-vexpress/hotplug.c
index ea4cbfb..700cf5a 100644
--- a/arch/arm/mach-vexpress/hotplug.c
+++ b/arch/arm/mach-vexpress/hotplug.c
@@ -88,7 +88,7 @@ static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
 	}
 }
 
-int platform_cpu_kill(unsigned int cpu)
+int vexpress_cpu_kill(unsigned int cpu)
 {
 	return 1;
 }
@@ -98,7 +98,7 @@ int platform_cpu_kill(unsigned int cpu)
  *
  * Called with IRQs disabled
  */
-void platform_cpu_die(unsigned int cpu)
+void vexpress_cpu_die(unsigned int cpu)
 {
 	int spurious = 0;
 
@@ -118,7 +118,7 @@ void platform_cpu_die(unsigned int cpu)
 		pr_warn("CPU%u: %u spurious wakeup calls\n", cpu, spurious);
 }
 
-int platform_cpu_disable(unsigned int cpu)
+int vexpress_cpu_disable(unsigned int cpu)
 {
 	/*
 	 * we don't allow CPU 0 to be shutdown (it is still too special
diff --git a/arch/arm/mach-vexpress/platsmp.c b/arch/arm/mach-vexpress/platsmp.c
index 2b5f7ac..3b4ed87 100644
--- a/arch/arm/mach-vexpress/platsmp.c
+++ b/arch/arm/mach-vexpress/platsmp.c
@@ -13,25 +13,26 @@
 #include <linux/smp.h>
 #include <linux/io.h>
 
+#include <asm/soc.h>
 #include <asm/unified.h>
 
 #include <mach/motherboard.h>
 #define V2M_PA_CS7 0x10000000
 
-#include "core.h"
+#include <plat/platsmp.h>
 
-extern void versatile_secondary_startup(void);
+#include "core.h"
 
 /*
  * Initialise the CPU possible map early - this describes the CPUs
  * which may be present or become present in the system.
  */
-void __init smp_init_cpus(void)
+static void __init vexpress_smp_init_cpus(void)
 {
 	ct_desc->init_cpu_map();
 }
 
-void __init platform_smp_prepare_cpus(unsigned int max_cpus)
+static void __init vexpress_smp_prepare_cpus(unsigned int max_cpus)
 {
 	/*
 	 * Initialise the present map, which describes the set of CPUs
@@ -49,3 +50,15 @@ void __init platform_smp_prepare_cpus(unsigned int max_cpus)
 	writel(BSYM(virt_to_phys(versatile_secondary_startup)),
 		MMIO_P2V(V2M_SYS_FLAGSSET));
 }
+
+struct arm_soc_smp_ops vexpress_soc_smp_ops __initdata = {
+	.smp_init_cpus		= vexpress_smp_init_cpus,
+	.smp_prepare_cpus	= vexpress_smp_prepare_cpus,
+	.smp_secondary_init	= versatile_secondary_init,
+	.smp_boot_secondary	= versatile_boot_secondary,
+#ifdef CONFIG_HOTPLUG_CPU
+	.cpu_kill		= vexpress_cpu_kill,
+	.cpu_die		= vexpress_cpu_die,
+	.cpu_disable		= vexpress_cpu_disable,
+#endif
+};
diff --git a/arch/arm/mach-vexpress/v2m.c b/arch/arm/mach-vexpress/v2m.c
index 1fafc32..9108d3f 100644
--- a/arch/arm/mach-vexpress/v2m.c
+++ b/arch/arm/mach-vexpress/v2m.c
@@ -16,6 +16,7 @@
 #include <linux/mtd/physmap.h>
 
 #include <asm/mach-types.h>
+#include <asm/soc.h>
 #include <asm/sizes.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/map.h>
@@ -28,6 +29,7 @@
 #include <mach/motherboard.h>
 
 #include <plat/sched_clock.h>
+#include <plat/platsmp.h>
 
 #include "core.h"
 
@@ -442,8 +444,16 @@ static void __init v2m_init(void)
 	ct_desc->init_tile();
 }
 
+static struct arm_soc_desc vexpress_soc_desc __initdata = {
+	.name		= "ARM VE Platform",
+#ifdef CONFIG_SMP
+	.smp_ops	= &vexpress_soc_smp_ops,
+#endif
+};
+
 MACHINE_START(VEXPRESS, "ARM-Versatile Express")
 	.atag_offset	= 0x100,
+	.soc		= &vexpress_soc_desc,
 	.map_io		= v2m_map_io,
 	.init_early	= v2m_init_early,
 	.init_irq	= v2m_init_irq,
diff --git a/arch/arm/plat-versatile/include/plat/platsmp.h b/arch/arm/plat-versatile/include/plat/platsmp.h
new file mode 100644
index 0000000..50fb830
--- /dev/null
+++ b/arch/arm/plat-versatile/include/plat/platsmp.h
@@ -0,0 +1,14 @@
+/*
+ *  linux/arch/arm/plat-versatile/include/plat/platsmp.h
+ *
+ *  Copyright (C) 2011 ARM Ltd.
+ *  All Rights Reserved
+ *
+ * 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.
+ */
+
+extern void versatile_secondary_startup(void);
+extern void versatile_secondary_init(unsigned int cpu);
+extern int  versatile_boot_secondary(unsigned int cpu, struct task_struct *idle);
diff --git a/arch/arm/plat-versatile/platsmp.c b/arch/arm/plat-versatile/platsmp.c
index 51ecfea..ec6a80b 100644
--- a/arch/arm/plat-versatile/platsmp.c
+++ b/arch/arm/plat-versatile/platsmp.c
@@ -39,7 +39,7 @@ static void __cpuinit write_pen_release(int val)
 
 static DEFINE_SPINLOCK(boot_lock);
 
-void __cpuinit platform_secondary_init(unsigned int cpu)
+void __cpuinit versatile_secondary_init(unsigned int cpu)
 {
 	/*
 	 * if any interrupts are already enabled for the primary
@@ -61,7 +61,7 @@ void __cpuinit platform_secondary_init(unsigned int cpu)
 	spin_unlock(&boot_lock);
 }
 
-int __cpuinit boot_secondary(unsigned int cpu, struct task_struct *idle)
+int __cpuinit versatile_boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
 	unsigned long timeout;
 
-- 
1.7.0.4

[RFC PATCH v2 3/3] ARM: SoC: convert VExpress/RealView to SoC descriptor

[Arnd Bergmann]

On Friday 09 September 2011, Marc Zyngier wrote:
> Convert both Realview and VExpress to use the SoC descriptor to
> provide their SMP and CPU hotplug operation.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Nicolas Pitre <nico@fluxnic.net>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Yes, looks good to me.

>  }
> +
> +struct arm_soc_desc realview_soc_desc __initdata = {
> +       .name           = "ARM RealView Platform",
> +#ifdef CONFIG_SMP
> +       .smp_ops        = &realview_soc_smp_ops,
> +#endif
> +};

Do you think we should have a macro for this? Since this is
always the same, we could do something like

#ifdef CONFIG_SMP
#define soc_smp_ops(ops)	.smp_ops = &(ops),
#else
#define soc_smp_ops(ops)	/* empty */
#endif

	Arnd

[RFC PATCH v2 3/3] ARM: SoC: convert VExpress/RealView to SoC descriptor

[Marc Zyngier]

On 09/09/11 15:56, Arnd Bergmann wrote:
> On Friday 09 September 2011, Marc Zyngier wrote:
>> Convert both Realview and VExpress to use the SoC descriptor to
>> provide their SMP and CPU hotplug operation.
>>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Nicolas Pitre <nico@fluxnic.net>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> 
> Yes, looks good to me.
> 
>>  }
>> +
>> +struct arm_soc_desc realview_soc_desc __initdata = {
>> +       .name           = "ARM RealView Platform",
>> +#ifdef CONFIG_SMP
>> +       .smp_ops        = &realview_soc_smp_ops,
>> +#endif
>> +};
> 
> Do you think we should have a macro for this? Since this is
> always the same, we could do something like
> 
> #ifdef CONFIG_SMP
> #define soc_smp_ops(ops)	.smp_ops = &(ops),
> #else
> #define soc_smp_ops(ops)	/* empty */
> #endif

That would be an improvement indeed. Will queue that for v3.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...