> -----Original Message----- > From: Benjamin Herrenschmidt [mailto:b...@kernel.crashing.org] > Sent: Friday, November 11, 2011 12:23 PM > To: Zhao Chenhui-B35336 > Cc: linuxppc-dev@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 _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev