| Message ID | 1366828819-10745-2-git-send-email-stefano.stabellini@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > >
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.
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
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.
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