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

[Scott Wood <scottwood@freescale.com>]

On 11/08/2011 04:05 AM, Li Yang-R58472 wrote:
>> Subject: Re: [PATCH 2/7] powerpc/85xx: add HOTPLUG_CPU support
>>
>> On 11/04/2011 07:31 AM, Zhao Chenhui wrote:
>>> +static int is_corenet;
>>> +static void __cpuinit smp_85xx_setup_cpu(int cpu_nr);
>>> +
>>> +#if defined(CONFIG_HOTPLUG_CPU) && defined(CONFIG_PPC32)
>>
>> Why PPC32?
> 
> Not sure if it is the same for e5500.  E5500 support will be verified later.

It's better to have 64-bit silently do nothing here?

>>> +	flush_disable_L1();
>>
>> You'll also need to take down L2 on e500mc.
> 
> Right.  E500mc support is beyond this patch series.  We will work on it after the e500v2 support is finalized.

I saw some support with "is_corenet"...  If we don't support e500mc,
make sure the code doesn't try to run on e500mc.

This isn't an e500v2-only BSP you're putting the code into. :-)

>>> +	tmp = 0;
>>> +	if (cpu_has_feature(CPU_FTR_CAN_NAP))
>>> +		tmp = HID0_NAP;
>>> +	else if (cpu_has_feature(CPU_FTR_CAN_DOZE))
>>> +		tmp = HID0_DOZE;
>>
>> Those FTR bits are for what we can do in idle, and can be cleared if the
>> user sets CONFIG_BDI_SWITCH.
> 
> It is powersave_nap variable shows what we can do in idle.

No, that shows what the user wants to do in idle.

> I think it's correct to use the CPU_FTR_CAN_* macro as CPU capability.

This is 85xx-specific code.  We always can, and want to, nap here
(though the way we enter nap will be different on e500mc and up) -- even
if it's broken to use it for idle (such as on p4080, which would miss
doorbells as wake events).

>>> +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);
>>> +	}
>>
>> Please use the device tree node, not get_immrbase().
> 
> The get_immrbase() implementation uses the device tree node.

I mean the guts node.  get_immrbase() should be avoided where possible.

Also, some people might care about latency to enter/exit deep sleep.
Searching through the device tree at this point (rather than on init)
slows that down.

>>> +static int __cpuinit smp_85xx_map_bootpg(u32 page)
>>> +{
>>> +	__iomem u32 *bootpg_ptr;
>>> +	u32 bptr;
>>> +
>>> +	/* Get the BPTR */
>>> +	bootpg_ptr = ioremap(get_immrbase() + MPC85xx_BPTR_OFF, 4);
>>> +
>>> +	/* Set the BPTR to the secondary boot page */
>>> +	bptr = MPC85xx_BPTR_EN | (page & MPC85xx_BPTR_BOOT_PAGE_MASK);
>>> +	out_be32(bootpg_ptr, bptr);
>>> +
>>> +	iounmap(bootpg_ptr);
>>> +	return 0;
>>> +}
>>
>> Shouldn't the boot page already be set by U-Boot?
> 
> The register should be lost after a deep sleep.   Better to do it again.

How can it be lost after a deep sleep if we're relying on it to hold our
wakeup code?

It's not "better to do it again" if we're making a bad assumption about
the code and the table being in the same page.

>>>  	local_irq_save(flags);
>>>
>>> -	out_be32(bptr_vaddr + BOOT_ENTRY_PIR, nr);
>>> +	out_be32(&epapr->pir, hw_cpu);
>>>  #ifdef CONFIG_PPC32
>>> -	out_be32(bptr_vaddr + BOOT_ENTRY_ADDR_LOWER, __pa(__early_start));
>>> +#ifdef CONFIG_HOTPLUG_CPU
>>> +	if (system_state == SYSTEM_RUNNING) {
>>> +		out_be32(&epapr->addr_l, 0);
>>> +		smp_85xx_map_bootpg((u32)(*cpu_rel_addr >> PAGE_SHIFT));
>>
>> Why is this inside PPC32?
> 
> Not tested on 64-bit yet.  Might require a different implementation on PPC64.

Please make a reasonable effort to do things in a way that works on
both.  It shouldn't be a big deal to test that it doesn't break booting
on p5020.

>>>  	if (!ioremappable)
>>> -		flush_dcache_range((ulong)bptr_vaddr,
>>> -				(ulong)(bptr_vaddr + SIZE_BOOT_ENTRY));
>>> +		flush_dcache_range((ulong)epapr,
>>> +				(ulong)epapr + sizeof(struct epapr_entry));
>>
>> We don't wait for the core to come up on 64-bit?
> 
> Not sure about it.  But at least the normal boot up should be tested on P5020, right?

Well, that's a special case in that it only has one secondary. :-)

Or we could be getting lucky with timing.  It's not a new issue with
this patch, it just looks odd.

-Scott