* [PATCH] powerpc/85xx: don't init the mpic ipi for the SoC which has doorbell support
@ 2013-11-07 7:17 Kevin Hao
2013-11-07 17:34 ` Scott Wood
0 siblings, 1 reply; 6+ messages in thread
From: Kevin Hao @ 2013-11-07 7:17 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc
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(-)
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);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/85xx: don't init the mpic ipi for the SoC which has doorbell support
2013-11-07 7:17 [PATCH] powerpc/85xx: don't init the mpic ipi for the SoC which has doorbell support Kevin Hao
@ 2013-11-07 17:34 ` Scott Wood
2013-11-08 1:54 ` Kevin Hao
0 siblings, 1 reply; 6+ messages in thread
From: Scott Wood @ 2013-11-07 17:34 UTC (permalink / raw)
To: Kevin Hao; +Cc: linuxppc
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/85xx: don't init the mpic ipi for the SoC which has doorbell support
2013-11-07 17:34 ` Scott Wood
@ 2013-11-08 1:54 ` Kevin Hao
2013-11-08 21:16 ` Scott Wood
0 siblings, 1 reply; 6+ messages in thread
From: Kevin Hao @ 2013-11-08 1:54 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc
[-- Attachment #1: Type: text/plain, Size: 2217 bytes --]
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
>
>
>
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/85xx: don't init the mpic ipi for the SoC which has doorbell support
2013-11-08 1:54 ` Kevin Hao
@ 2013-11-08 21:16 ` Scott Wood
2013-11-09 6:43 ` Kevin Hao
0 siblings, 1 reply; 6+ messages in thread
From: Scott Wood @ 2013-11-08 21:16 UTC (permalink / raw)
To: Kevin Hao; +Cc: linuxppc
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/85xx: don't init the mpic ipi for the SoC which has doorbell support
2013-11-08 21:16 ` Scott Wood
@ 2013-11-09 6:43 ` Kevin Hao
2013-11-11 23:47 ` Scott Wood
0 siblings, 1 reply; 6+ messages in thread
From: Kevin Hao @ 2013-11-09 6:43 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc
[-- Attachment #1: Type: text/plain, Size: 1745 bytes --]
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
>
>
>
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/85xx: don't init the mpic ipi for the SoC which has doorbell support
2013-11-09 6:43 ` Kevin Hao
@ 2013-11-11 23:47 ` Scott Wood
0 siblings, 0 replies; 6+ messages in thread
From: Scott Wood @ 2013-11-11 23:47 UTC (permalink / raw)
To: Kevin Hao; +Cc: linuxppc
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-11-11 23:48 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-07 7:17 [PATCH] powerpc/85xx: don't init the mpic ipi for the SoC which has doorbell support Kevin Hao
2013-11-07 17:34 ` Scott Wood
2013-11-08 1:54 ` Kevin Hao
2013-11-08 21:16 ` Scott Wood
2013-11-09 6:43 ` Kevin Hao
2013-11-11 23:47 ` Scott Wood
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).