| Message ID | 1383808637-26769-1-git-send-email-haokexin@gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 455d23a8908319fa7ad450e65e4f09afb45057a7 |
| Delegated to: | Scott Wood |
| Headers | show |
On Thu, 2013-11-07 at 15:17 +0800, Kevin Hao wrote: > It makes no sense to initialize the mpic ipi for the SoC which has > doorbell support. So set the smp_85xx_ops.probe to NULL for this > case. Since the smp_85xx_ops.probe is also used in function > smp_85xx_setup_cpu() to check if we need to invoke > mpic_setup_this_cpu(), we introduce a new setup_cpu function > smp_85xx_basic_setup() to remove this dependency. Is there any harm caused by setting up the IPIs? What about other MPIC setup, such as setting the current task priority register? > Signed-off-by: Kevin Hao <haokexin@gmail.com> > --- > > Boot test on p2020rdb and p5020ds. > > arch/powerpc/platforms/85xx/smp.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c > index 281b7f01df63..d3b310f87ce9 100644 > --- a/arch/powerpc/platforms/85xx/smp.c > +++ b/arch/powerpc/platforms/85xx/smp.c > @@ -388,15 +388,18 @@ static void mpc85xx_smp_machine_kexec(struct kimage *image) > } > #endif /* CONFIG_KEXEC */ > > -static void smp_85xx_setup_cpu(int cpu_nr) > +static void smp_85xx_basic_setup(int cpu_nr) > { > - if (smp_85xx_ops.probe == smp_mpic_probe) > - mpic_setup_this_cpu(); > - > if (cpu_has_feature(CPU_FTR_DBELL)) > doorbell_setup_this_cpu(); > } > > +static void smp_85xx_setup_cpu(int cpu_nr) > +{ > + mpic_setup_this_cpu(); > + smp_85xx_basic_setup(cpu_nr); > +} > + > static const struct of_device_id mpc85xx_smp_guts_ids[] = { > { .compatible = "fsl,mpc8572-guts", }, > { .compatible = "fsl,p1020-guts", }, > @@ -411,13 +414,14 @@ void __init mpc85xx_smp_init(void) > { > struct device_node *np; > > - smp_85xx_ops.setup_cpu = smp_85xx_setup_cpu; > > np = of_find_node_by_type(NULL, "open-pic"); > if (np) { > smp_85xx_ops.probe = smp_mpic_probe; > + smp_85xx_ops.setup_cpu = smp_85xx_setup_cpu; > smp_85xx_ops.message_pass = smp_mpic_message_pass; > - } > + } else > + smp_85xx_ops.setup_cpu = smp_85xx_basic_setup; > > if (cpu_has_feature(CPU_FTR_DBELL)) { > /* > @@ -426,6 +430,7 @@ void __init mpc85xx_smp_init(void) > */ > smp_85xx_ops.message_pass = NULL; > smp_85xx_ops.cause_ipi = doorbell_cause_ipi; > + smp_85xx_ops.probe = NULL; > } BTW, what exactly is probe() supposed to be doing? It looks like its main effect (with smp_mpic_probe) is to request IPIs, but the caller seems to treat it mainly as a way to determine CPU count. I looked at the caller of .probe() (which is smp_prepare_cpus()) to see what happens when probe is NULL, and the handling of max_cpus doesn't make much sense. At first I was concerned by the gratuitous difference between smp_mpic_probe() using cpu_possible_mask versus smp_prepare_cpus() using NR_CPUS, but the value isn't even used (all the code that consumed max_cpus after setting it has been removed), and the value passed in to smp_prepare_cpus() is ignored. -Scott
On Thu, Nov 07, 2013 at 11:34:51AM -0600, Scott Wood wrote: > On Thu, 2013-11-07 at 15:17 +0800, Kevin Hao wrote: > > It makes no sense to initialize the mpic ipi for the SoC which has > > doorbell support. So set the smp_85xx_ops.probe to NULL for this > > case. Since the smp_85xx_ops.probe is also used in function > > smp_85xx_setup_cpu() to check if we need to invoke > > mpic_setup_this_cpu(), we introduce a new setup_cpu function > > smp_85xx_basic_setup() to remove this dependency. > > Is there any harm caused by setting up the IPIs? No real harm. Just make no sense to do so and it does cause confusion when you cat /proc/interrupts and get something like this: 507: 0 0 OpenPIC 2043 Edge ipi call function 508: 0 0 OpenPIC 2044 Edge ipi reschedule 509: 0 0 OpenPIC 2045 Edge ipi call function single DBL: 7053 10137 Doorbell interrupts > > What about other MPIC setup, such as setting the current task priority > register? This is done by the invoking of function mpic_setup_this_cpu() in smp_85xx_setup_cpu(). > BTW, what exactly is probe() supposed to be doing? It is supposed to do the platform specific smp initialization and then return the CPU count. > It looks like its > main effect (with smp_mpic_probe) is to request IPIs, but the caller > seems to treat it mainly as a way to determine CPU count. > > I looked at the caller of .probe() (which is smp_prepare_cpus()) to see > what happens when probe is NULL, and the handling of max_cpus doesn't > make much sense. At first I was concerned by the gratuitous difference > between smp_mpic_probe() using cpu_possible_mask versus > smp_prepare_cpus() using NR_CPUS, but the value isn't even used (all the > code that consumed max_cpus after setting it has been removed), and the > value passed in to smp_prepare_cpus() is ignored. Yes, the return value of .probe makes no sense now. Actually there is already a patch to remove the check of the return value of .probe in function smp_prepare_cpus(). http://patchwork.ozlabs.org/patch/260574/ Thanks, Kevin > > -Scott > > >
On Fri, 2013-11-08 at 09:54 +0800, Kevin Hao wrote: > On Thu, Nov 07, 2013 at 11:34:51AM -0600, Scott Wood wrote: > > On Thu, 2013-11-07 at 15:17 +0800, Kevin Hao wrote: > > > It makes no sense to initialize the mpic ipi for the SoC which has > > > doorbell support. So set the smp_85xx_ops.probe to NULL for this > > > case. Since the smp_85xx_ops.probe is also used in function > > > smp_85xx_setup_cpu() to check if we need to invoke > > > mpic_setup_this_cpu(), we introduce a new setup_cpu function > > > smp_85xx_basic_setup() to remove this dependency. > > > > Is there any harm caused by setting up the IPIs? > > No real harm. Just make no sense to do so and it does cause confusion > when you cat /proc/interrupts and get something like this: > 507: 0 0 OpenPIC 2043 Edge ipi call function > 508: 0 0 OpenPIC 2044 Edge ipi reschedule > 509: 0 0 OpenPIC 2045 Edge ipi call function single > DBL: 7053 10137 Doorbell interrupts > > > > > What about other MPIC setup, such as setting the current task priority > > register? > > This is done by the invoking of function mpic_setup_this_cpu() in > smp_85xx_setup_cpu(). OK... Why are you splitting out smp_85xx_basic_setup()? Where do you call it other than from smp_85xx_setup_cpu()? Couldn't you have just removed the conditional without splitting up the function? The change log says it's "to check if we need to invoke mpic_setup_this_cpu()" which doesn't make sense since we always want to call mpic_setup_this_cpu() if we have an MPIC. -Scott
On Fri, Nov 08, 2013 at 03:16:12PM -0600, Scott Wood wrote: > OK... Why are you splitting out smp_85xx_basic_setup()? In the current implementation of smp_85xx_setup_cpu(), we only invoke the function mpic_setup_this_cpu() when the smp_85xx_ops.probe is set to smp_mpic_probe(). So if we set smp_85xx_ops.probe to NULL when doorbell is available, we must make sure that the mpic_setup_this_cpu() is also invoked when there does have a mpic. The smp_85xx_basic_setup() is for the board which has no mpic. static void smp_85xx_setup_cpu(int cpu_nr) { if (smp_85xx_ops.probe == smp_mpic_probe) mpic_setup_this_cpu(); if (cpu_has_feature(CPU_FTR_DBELL)) doorbell_setup_this_cpu(); } > Where do you > call it other than from smp_85xx_setup_cpu()? We would set the .setup_cpu() to smp_85xx_basic_setup() if it is a non-mpic board. The following is quoted form the patch: np = of_find_node_by_type(NULL, "open-pic"); if (np) { smp_85xx_ops.probe = smp_mpic_probe; + smp_85xx_ops.setup_cpu = smp_85xx_setup_cpu; smp_85xx_ops.message_pass = smp_mpic_message_pass; - } + } else + smp_85xx_ops.setup_cpu = smp_85xx_basic_setup; > Couldn't you have just > removed the conditional without splitting up the function? This will break the non-mpic board. > The change > log says it's "to check if we need to invoke mpic_setup_this_cpu()" > which doesn't make sense since we always want to call > mpic_setup_this_cpu() if we have an MPIC. If we have a mpic, we will call mpic_setup_this_cpu(). But if not, we would set the .setup_cpu to smp_85xx_basic_setup() to avoid the invoking of mpic_setup_this_cpu(). Thanks, Kevin > > -Scott > > >
On Sat, 2013-11-09 at 14:43 +0800, Kevin Hao wrote: > On Fri, Nov 08, 2013 at 03:16:12PM -0600, Scott Wood wrote: > > OK... Why are you splitting out smp_85xx_basic_setup()? > > In the current implementation of smp_85xx_setup_cpu(), we only invoke > the function mpic_setup_this_cpu() when the smp_85xx_ops.probe is set > to smp_mpic_probe(). So if we set smp_85xx_ops.probe to NULL when doorbell > is available, we must make sure that the mpic_setup_this_cpu() is also invoked > when there does have a mpic. The smp_85xx_basic_setup() is for the board which > has no mpic. > > static void smp_85xx_setup_cpu(int cpu_nr) > { > if (smp_85xx_ops.probe == smp_mpic_probe) > mpic_setup_this_cpu(); > > if (cpu_has_feature(CPU_FTR_DBELL)) > doorbell_setup_this_cpu(); > } > > > Where do you > > call it other than from smp_85xx_setup_cpu()? > > We would set the .setup_cpu() to smp_85xx_basic_setup() if it is a > non-mpic board. The following is quoted form the patch: > np = of_find_node_by_type(NULL, "open-pic"); > if (np) { > smp_85xx_ops.probe = smp_mpic_probe; > + smp_85xx_ops.setup_cpu = smp_85xx_setup_cpu; > smp_85xx_ops.message_pass = smp_mpic_message_pass; > - } > + } else > + smp_85xx_ops.setup_cpu = smp_85xx_basic_setup; OK, somehow I missed the change to .setup_cpu before. :-P -Scott
diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c index 281b7f01df63..d3b310f87ce9 100644 --- a/arch/powerpc/platforms/85xx/smp.c +++ b/arch/powerpc/platforms/85xx/smp.c @@ -388,15 +388,18 @@ static void mpc85xx_smp_machine_kexec(struct kimage *image) } #endif /* CONFIG_KEXEC */ -static void smp_85xx_setup_cpu(int cpu_nr) +static void smp_85xx_basic_setup(int cpu_nr) { - if (smp_85xx_ops.probe == smp_mpic_probe) - mpic_setup_this_cpu(); - if (cpu_has_feature(CPU_FTR_DBELL)) doorbell_setup_this_cpu(); } +static void smp_85xx_setup_cpu(int cpu_nr) +{ + mpic_setup_this_cpu(); + smp_85xx_basic_setup(cpu_nr); +} + static const struct of_device_id mpc85xx_smp_guts_ids[] = { { .compatible = "fsl,mpc8572-guts", }, { .compatible = "fsl,p1020-guts", }, @@ -411,13 +414,14 @@ void __init mpc85xx_smp_init(void) { struct device_node *np; - smp_85xx_ops.setup_cpu = smp_85xx_setup_cpu; np = of_find_node_by_type(NULL, "open-pic"); if (np) { smp_85xx_ops.probe = smp_mpic_probe; + smp_85xx_ops.setup_cpu = smp_85xx_setup_cpu; smp_85xx_ops.message_pass = smp_mpic_message_pass; - } + } else + smp_85xx_ops.setup_cpu = smp_85xx_basic_setup; if (cpu_has_feature(CPU_FTR_DBELL)) { /* @@ -426,6 +430,7 @@ void __init mpc85xx_smp_init(void) */ smp_85xx_ops.message_pass = NULL; smp_85xx_ops.cause_ipi = doorbell_cause_ipi; + smp_85xx_ops.probe = NULL; } np = of_find_matching_node(NULL, mpc85xx_smp_guts_ids);
It makes no sense to initialize the mpic ipi for the SoC which has doorbell support. So set the smp_85xx_ops.probe to NULL for this case. Since the smp_85xx_ops.probe is also used in function smp_85xx_setup_cpu() to check if we need to invoke mpic_setup_this_cpu(), we introduce a new setup_cpu function smp_85xx_basic_setup() to remove this dependency. Signed-off-by: Kevin Hao <haokexin@gmail.com> --- Boot test on p2020rdb and p5020ds. arch/powerpc/platforms/85xx/smp.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)