>Cc: linuxppc-dev@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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
