[U-Boot] [PATCH] powerpc/fsl: support low power boot for e500 and later

[Scott Wood]

On Thu, 2015-01-15 at 14:04 +0800, Dongsheng Wang wrote:
> From: Wang Dongsheng <dongsheng.wang at freescale.com>
> 
> low power boot means u-boot will put non-boot cpus into a low power
> status. Non-boot cpus don't need any more spin wait. e500, e500v2 will
> going to DOZE status. e500mc, e5500, e6500rev1 will going to PW10 state.
> e6500rev2 will going to PW20 state.
> 
> e500/e500v2 will be kicked up by MPIC-IPI, e500mc later will be kicked up
> by doorbell.

This will break compatibility with existing kernels (and violate ePAPR
since you haven't changed the release-method string).  It must be
optional and (for now) off by default.

What benefits are we getting for this churn and complexity?  Do we
really need to optimize for the case where not all CPUs are used?

Where is this new mechanism documented?

> diff --git a/arch/powerpc/cpu/mpc85xx/release.S b/arch/powerpc/cpu/mpc85xx/release.S
> index a2c0ad4..a97a1b6 100644
> --- a/arch/powerpc/cpu/mpc85xx/release.S
> +++ b/arch/powerpc/cpu/mpc85xx/release.S
> @@ -297,10 +297,15 @@ __secondary_start_page:
>  	mtspr	SPRN_MAS7,r11
>  	tlbwe
>  
> +	li	r6, 0
> +
>  	/*
>  	 * __bootpg_addr has the address of __second_half_boot_page
>  	 * jump there in AS=1 space with cache enabled
>  	 */
> +	.align 6
> +	.global jump_half_boot_page
> +jump_half_boot_page:

"half_boot_page" is confusing.

>  	lis	r13,toreset(__bootpg_addr)@h
>  	ori	r13,r13,toreset(__bootpg_addr)@l
>  	lwz	r11,0(r13)
> @@ -371,6 +376,9 @@ __second_half_boot_page:
>  	 * };
>  	 * we pad this struct to 64 bytes so each entry is in its own cacheline
>  	 */
> +	cmpwi   r6, 1
> +	beq	3f

What does a value of 1 mean for r6?  What about 0?  Could you use
symbolic constants?

Why not just set the IVOR to where you really want to enter, rather than
conditionally branching there based on a value that happens to
correspond to whether you're entering via an interrupt?

>  	li	r3,0
>  	li	r8,1
>  	mfspr	r4,SPRN_PIR
> @@ -402,7 +410,132 @@ __second_half_boot_page:
>  #endif
>  	lwz	r4,ENTRY_ADDR_LOWER(r10)
>  	andi.	r11,r4,1
> -	bne	3b
> +	beq	6f
> +
> +	li	r6, 0
> +	addi	r6, r6, 1

Why not just "li r6, 1"?

> +	/* External Interrupt exception. */
> +	lis	r7, toreset(jump_half_boot_page)@h
> +	mtspr	SPRN_IVPR, r7
> +	li	r7, toreset(jump_half_boot_page)@l
> +
> +#ifdef	CONFIG_E500MC
> +	/* e500MC, e5500, e6500 will use doorbell to send ipi signal */
> +	mtspr	SPRN_IVOR36, r7
> +#endif
> +
> +	/*
> +	 * For e500mc later:
> +	 * EE will open in low power state, IVOR4 make sure we can ACK
> +	 * trash interrupt and keep we can loop in wait state again until
> +	 * the desired interrupt coming.

I don't understand "EE will open".  Do you mean "EE will be set"?
Why would we get unexpected interrupts?

> +	 *
> +	 * e500, e500v2:
> +	 * Kernel will use MPCI to send ipi signal, so we must set IVOR4.
> +	 */

MPIC

> +	mtspr	SPRN_IVOR4, r7
> +
> +	isync
> +
> +#ifdef	CONFIG_E500MC
> +	/* PW20 & AltiVec idle feature only exists for E6500 */
> +	mfspr   r0, SPRN_PVR
> +	rlwinm  r11, r0, 16, 16, 31
> +	lis     r12, 0
> +	ori     r12, r12, PVR_VER_E6500 at l
> +	cmpw    r11, r12
> +	bne     5f
> +
> +	/* Fix erratum, e6500 rev1 does not support PW20 & AltiVec idle */
> +	rlwinm  r11, r0, 0, 16, 31
> +	cmpwi   r11, 0x20
> +	blt     5f

PW10 isn't enough here?

> +#define PW20_WAIT_IDLE_BIT	50 /* 1ms, TB frequency is 41.66MHZ */

If you must use PW20 please set the timeout long enough that it's
obvious we won't hit it during normal bootup.


> +	/*
> +	 * Set all of cpu PIR-ID is 0, wait kernel send doorbell or MPIC-IPI
> +	 * signal.
> +	 *
> +	 * When kernel kick one of cpus, all cpus will be wakenup. To make
> +	 * sure that only the target cpu is effected, other cpus (by checking
> +	 * spin_table->addr_l) should go back to low power state.
> +	 *
> +	 * U-boot has renumber the cpu PIR Why we need to set all of PIR to
> +	 * the same value?
> +	 * A: Before kernel kicking cpu, the doorbell message was not configured
> +	 * for target cpu(cpu_messages->data). If we try to send a
> +	 * non-configured message to target cpu, it cannot correctly receive
> +	 * doorbell interrput. So SET ALL OF CPU'S PIR to the same value to
> +	 * let all cpus catch the interrupt.
> +	 *
> +	 * Why set PIR to zero?
> +	 * A: U-boot cannot know how many cpus will be kicked up(Kernel allow us
> +	 * to configure NR_CPUS) and IPI is a per_cpu variable, u-boot cannot
> +	 * set a appropriate PIR for every cpu, but the boot cpu(CPU0) always be
> +	 * there. So set PIR is zero as a default PIR ID for each CPUs.

What does NR_CPUS have to do with the appropriate PIR for each CPU?

> +	/*
> +	 * If proxy mode enable in MPIC, Read EPR to ACK INTERRUPT
> +	 * Or proxy mode disable, Kernel will read MPIC to ACK INTERRUPT.
> +	 */
> +	mfspr	r7, SPRN_EPR

Where do you limit this to cases when external proxy is enabled?  Why
would we care about external proxy at all?  The only chips we use IPIs
on don't have external proxy.

> +
> +	/* Wait EOI finish */
> +7:
> +	lwz	r7, ENTRY_PIR(r10)
> +	andi.	r7, r7, 0xffff
> +	bne	8f
> +	bl	7b
> +8:

Why are you reading PIR in a loop?  What does that have to do with an
EOI finishing?

-Scott