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

[Li Yang-R58472]

>Cc: linuxppc-dev at lists.ozlabs.org; Li Yang-R58472
>Subject: Re: [PATCH v2 2/7] powerpc/85xx: add HOTPLUG_CPU support
>
>On 11/16/2011 03:55 AM, Zhao Chenhui wrote:
>> +static void __cpuinit smp_85xx_mach_cpu_die(void) {
>> +	unsigned int cpu = smp_processor_id();
>> +	register u32 tmp;
>> +
>> +	local_irq_disable();
>> +	idle_task_exit();
>> +	generic_set_cpu_dead(cpu);
>> +	mb();
>> +
>> +	mtspr(SPRN_TCR, 0);
>> +	mtspr(SPRN_TSR, TSR_ENW | TSR_WIS | TSR_DIS | TSR_FIS);
>
>Clearing these bits in TSR should be unnecessary since we clear TCR -- and
>doesn't really accomplish anything since the TSR bits can continue to be
>set.

I also recommend setting the CORE_IRQ_MASK and CORE_CI_MASK in the POWMGTCSR register so that no interrupt will wakeup the core from NAP.

>
>If watchdog is in use, we need to set the period to the highest possible
>to effectively disable it.

Setting it to the highest timeout doesn't really disable the watchdog.  The best way for disabling the wdt is to reset the core, although it's a bit too complex to do.

>
>> +	if (cpu_has_feature(CPU_FTR_CAN_NAP)) {
>
>Again, don't check this.  On 85xx, we *always* can and should use nap here.
>At best this is noise, at worst this will cause problems if
>CONFIG_BDI_SWITCH is enabled, or if CPU_FTR_CAN_NAP is cleared for any
>other reason (e.g. it's not set on e500mc, and the reason isn't that the
>nap implementation is different (which it is), but that it's not usable in
>the idle loop).
>
>> +static int __cpuinit smp_85xx_kick_cpu(int nr)
>> +
>>  {
>>  	unsigned long flags;
>>  	const u64 *cpu_rel_addr;
>> -	__iomem u32 *bptr_vaddr;
>> +	__iomem struct epapr_spin_table *epapr;
>
>Please don't call this just "epapr".  That's like calling a reference to
>any powerpc-specific struct "powerpc".
>
>How about "spin_table"?

You mean the name of the variable not the structure, right?  I agree.

>
>> -	out_be32(bptr_vaddr + BOOT_ENTRY_PIR, hw_cpu);
>> +	out_be32(&epapr->pir, hw_cpu);
>>  #ifdef CONFIG_PPC32
>> -	out_be32(bptr_vaddr + BOOT_ENTRY_ADDR_LOWER, __pa(__early_start));
>> +#ifdef CONFIG_HOTPLUG_CPU
>> +	/* Corresponding to generic_set_cpu_dead() */
>> +	generic_set_cpu_up(nr);
>> +
>> +	if (system_state == SYSTEM_RUNNING) {
>> +		out_be32(&epapr->addr_l, 0);
>> +
>> +		smp_85xx_set_bootpg((u32)(*cpu_rel_addr >> PAGE_SHIFT));
>
>As previously requested, please document why you're setting the boot page
>here.  This should really be done when you resume from deep sleep, rather
>than here, and should be a restoration of the value that the register held
>prior to deep sleep.
>
>>  struct smp_ops_t smp_85xx_ops = {
>>  	.kick_cpu = smp_85xx_kick_cpu,
>> +	.setup_cpu	= smp_85xx_setup_cpu,
>> +#ifdef CONFIG_HOTPLUG_CPU
>> +	.cpu_disable	= generic_cpu_disable,
>> +	.cpu_die	= generic_cpu_die,
>> +#endif
>
>Only fill these fields in on e500v1/v2, until we properly support e500mc.
>Likewise in ppc_md.cpu_die and anywhere else we advertise this
>functionality.

Is there a standard function call that can tell that it is an e500mc not legacy e500?

- Leo