Re: [PATCH] powerpc/85xx: don't init the mpic ipi for the SoC which has doorbell support

[Scott Wood <scottwood@freescale.com>]

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