[PATCH 2/7] powerpc/85xx: add HOTPLUG_CPU support

[Zhao Chenhui-B35336]

> -----Original Message-----
> From: Benjamin Herrenschmidt [mailto:benh at kernel.crashing.org]
> Sent: Friday, November 11, 2011 12:23 PM
> To: Zhao Chenhui-B35336
> Cc: linuxppc-dev at lists.ozlabs.org
> Subject: Re: [PATCH 2/7] powerpc/85xx: add HOTPLUG_CPU support
> 
> On Fri, 2011-11-04 at 20:31 +0800, Zhao Chenhui wrote:
> 
> >  #ifdef CONFIG_SMP
> >  /* When we get here, r24 needs to hold the CPU # */
> >  	.globl __secondary_start
> > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> > index 7bf2187..12a54f0 100644
> > --- a/arch/powerpc/kernel/smp.c
> > +++ b/arch/powerpc/kernel/smp.c
> > @@ -381,8 +381,14 @@ void generic_cpu_die(unsigned int cpu)
> >
> >  	for (i = 0; i < 100; i++) {
> >  		smp_rmb();
> > -		if (per_cpu(cpu_state, cpu) == CPU_DEAD)
> > +		if (per_cpu(cpu_state, cpu) == CPU_DEAD) {
> > +			/*
> > +			 * After another core sets cpu_state to CPU_DEAD,
> > +			 * it needs some time to die.
> > +			 */
> > +			msleep(10);
> >  			return;
> > +		}
> >  		msleep(100);
> >  	}
> >  	printk(KERN_ERR "CPU%d didn't die...\n", cpu);
> 
> I don't really see why you need to wait. This loop is about waiting for
> the CPU to mark itself DEAD, not necessarily to be physically powered
> off.
> 

If I don't add this delay, the kernel hangs sometimes when hotpluging a core.
I will move this function to 85xx/smp.c in the next revision.

> > +struct epapr_entry {
> > +	u32	addr_h;
> > +	u32	addr_l;
> > +	u32	r3_h;
> > +	u32	r3_l;
> > +	u32	reserved;
> > +	u32	pir;
> > +	u32	r6_h;
> > +	u32	r6_l;
> > +};
> 
> This should probably be in a generic location.
> 
> > +static int is_corenet;
> 
> This global is a bit gross...
> 
>  ...

Agree. I will remove it.

> 
> > +
> > +static void __cpuinit smp_85xx_reset_core(int nr)
> > +{
> > +	__iomem u32 *vaddr, *pir_vaddr;
> > +	u32 val, cpu_mask;
> > +
> > +	/* If CoreNet platform, use BRR as release register. */
> > +	if (is_corenet) {
> > +		cpu_mask = 1 << nr;
> > +		vaddr = ioremap(get_immrbase() + MPC85xx_BRR_OFF, 4);
> > +	} else {
> > +		cpu_mask = 1 << (24 + nr);
> > +		vaddr = ioremap(get_immrbase() + MPC85xx_ECM_EEBPCR_OFF, 4);
> > +	}
> 
> Instead, can't you instead have two functions using a common helper and
> pick/hook the right one ?
> 
> Also in what context is that reset_core() called ? Doing ioremap isn't
> something you can do at any random time...
> 
> Cheers,
> Ben.
> 
> 

Thanks. I will fix them.

-chenhui