Patchwork powerpc/85xx: don't init the mpic ipi for the SoC which has doorbell support

login
register
mail settings
Submitter Kevin Hao
Date Nov. 7, 2013, 7:17 a.m.
Message ID <1383808637-26769-1-git-send-email-haokexin@gmail.com>
Download mbox | patch
Permalink /patch/289214/
State Accepted, archived
Commit 455d23a8908319fa7ad450e65e4f09afb45057a7
Delegated to: Scott Wood
Headers show

Comments

Kevin Hao - Nov. 7, 2013, 7:17 a.m.
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(-)
Scott Wood - Nov. 7, 2013, 5:34 p.m.
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
Kevin Hao - Nov. 8, 2013, 1:54 a.m.
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
> 
> 
>
Scott Wood - Nov. 8, 2013, 9:16 p.m.
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
Kevin Hao - Nov. 9, 2013, 6:43 a.m.
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
> 
> 
>
Scott Wood - Nov. 11, 2013, 11:47 p.m.
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

Patch

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);