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

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

[Dongsheng Wang]

From: Wang Dongsheng <dongsheng.wang@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 feature tested on:
POWER UP TEST:
P1022DS(e500v2),96k times.
P4080(e500mc),  110k times.
T1024(e5500),   83k times.
T4240(e6500),   150k times.

CPU HOTPLUG TEST:
P1022DS(e500v2),1.4 million times.
P4080(e500mc),  1.8 million times.
T1024(e5500),   1.3 million times.
T4240(e6500),   1.1 million times.

Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>

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:
 	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
+
 	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
+
+	/* 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.
+	 *
+	 * e500, e500v2:
+	 * Kernel will use MPCI to send ipi signal, so we must set IVOR4.
+	 */
+	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
+
+#define PW20_WAIT_IDLE_BIT	50 /* 1ms, TB frequency is 41.66MHZ */
+	/* Core Enter PW20 State */
+	mfspr   r7, SPRN_PWRMGTCR0
+	/* Set PW20_WAIT bit, enable pw20 state*/
+	ori     r7, r7, PWRMGTCR0_PW20_WAIT
+	li      r9, PW20_WAIT_IDLE_BIT
+	/* Set Automatic PW20 Core Idle Count */
+	rlwimi  r7, r9, PWRMGTCR0_PW20_ENT_SHIFT, PWRMGTCR0_PW20_ENT
+	mtspr   SPRN_PWRMGTCR0, r7
+
+#define AV_WAIT_IDLE_BIT	50 /* 1ms, TB frequency is 41.66MHZ */
+	/* Let Altivec Enter Idle State */
+	mfspr   r7, SPRN_PWRMGTCR0
+	/* Enable Altivec Idle */
+	oris    r7, r7, PWRMGTCR0_AV_IDLE_PD_EN at h
+	li      r9, AV_WAIT_IDLE_BIT
+	/* Set Automatic AltiVec Idle Count */
+	rlwimi  r7, r9, PWRMGTCR0_AV_IDLE_CNT_SHIFT, PWRMGTCR0_AV_IDLE_CNT
+	mtspr   SPRN_PWRMGTCR0, r7
+
+5:
+	/*
+	 * 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.
+	 */
+
+	li	r7, 0
+	mtspr	SPRN_PIR, r7
+
+	wrteei  1
+	wait
+#else
+	li	r5, 0
+	lis	r5, HID0_DOZE at h
+	mfspr	r7, SPRN_HID0
+	rlwinm	r7, r7, 0, ~(HID0_DOZE | HID0_NAP | HID0_SLEEP)
+	or	r7, r7, r5
+	isync
+	mtspr	SPRN_HID0, r7
+	isync
+
+	mfmsr	r7
+	oris	r7, r7, MSR_WE at h
+	ori	r7, r7, MSR_EE
+	msync
+	mtmsr	r7
+	isync
+5:	bl	5b
+#endif
+6:
+	wrteei	0
+
+	/*
+	 * 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
+
+	/* Wait EOI finish */
+7:
+	lwz	r7, ENTRY_PIR(r10)
+	andi.	r7, r7, 0xffff
+	bne	8f
+	bl	7b
+8:
+
 	isync
 
 	/* get the upper bits of the addr */
diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index 2ed51b1..4d49317 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -482,6 +482,7 @@
 #define SPRN_GIVOR8	0x1bb	/* Guest Interrupt Vector Offset Register 8 */
 #define SPRN_GIVOR13	0x1bc	/* Guest Interrupt Vector Offset Register 13 */
 #define SPRN_GIVOR14	0x1bd	/* Guest Interrupt Vector Offset Register 14 */
+#define SPRN_EPR	0x2be	/* External Proxy Register */
 
 /* e500 definitions */
 #define SPRN_L1CFG0	0x203	/* L1 Cache Configuration Register 0 */
@@ -565,6 +566,13 @@
 #define SPRN_BBTAR	0x202	/* Branch Buffer Target Address Register */
 #define SPRN_PID1	0x279	/* Process ID Register 1 */
 #define SPRN_PID2	0x27a	/* Process ID Register 2 */
+#define SPRN_PWRMGTCR0  0x3fb	/* Power management control register 0 */
+#define   PWRMGTCR0_PW20_WAIT		(1 << 14) /* PW20 state enable bit */
+#define   PWRMGTCR0_PW20_ENT_SHIFT	8
+#define   PWRMGTCR0_PW20_ENT		0x3f00
+#define   PWRMGTCR0_AV_IDLE_PD_EN	(1 << 22) /* Altivec idle enable */
+#define   PWRMGTCR0_AV_IDLE_CNT_SHIFT	16
+#define   PWRMGTCR0_AV_IDLE_CNT		0x3f0000
 #define SPRN_MCSR	0x23c	/* Machine Check Syndrome register */
 #define SPRN_MCAR	0x23d	/* Machine Check Address register */
 #define MCSR_MCS	0x80000000	/* Machine Check Summary */
-- 
2.1.0.27.g96db324

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

[Dongsheng.Wang at freescale.com]

Hi all,

Kernel patch link:
http://patchwork.ozlabs.org/patch/429259/

Regards,
-Dongsheng

> -----Original Message-----
> From: Dongsheng Wang [mailto:dongsheng.wang at freescale.com]
> Sent: Thursday, January 15, 2015 2:05 PM
> To: Sun York-R58495; Wood Scott-B07421; Li Yang-Leo-R58472
> Cc: Jin Zhengxiong-R64188; u-boot at lists.denx.de; Wang Dongsheng-B40534
> Subject: [PATCH] powerpc/fsl: support low power boot for e500 and later
> 
> From: Wang Dongsheng <dongsheng.wang@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 feature tested on:
> POWER UP TEST:
> P1022DS(e500v2),96k times.
> P4080(e500mc),  110k times.
> T1024(e5500),   83k times.
> T4240(e6500),   150k times.
> 
> CPU HOTPLUG TEST:
> P1022DS(e500v2),1.4 million times.
> P4080(e500mc),  1.8 million times.
> T1024(e5500),   1.3 million times.
> T4240(e6500),   1.1 million times.
> 
> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> 
> 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:
>  	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
> +
>  	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
> +
> +	/* 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.
> +	 *
> +	 * e500, e500v2:
> +	 * Kernel will use MPCI to send ipi signal, so we must set IVOR4.
> +	 */
> +	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
> +
> +#define PW20_WAIT_IDLE_BIT	50 /* 1ms, TB frequency is 41.66MHZ */
> +	/* Core Enter PW20 State */
> +	mfspr   r7, SPRN_PWRMGTCR0
> +	/* Set PW20_WAIT bit, enable pw20 state*/
> +	ori     r7, r7, PWRMGTCR0_PW20_WAIT
> +	li      r9, PW20_WAIT_IDLE_BIT
> +	/* Set Automatic PW20 Core Idle Count */
> +	rlwimi  r7, r9, PWRMGTCR0_PW20_ENT_SHIFT, PWRMGTCR0_PW20_ENT
> +	mtspr   SPRN_PWRMGTCR0, r7
> +
> +#define AV_WAIT_IDLE_BIT	50 /* 1ms, TB frequency is 41.66MHZ */
> +	/* Let Altivec Enter Idle State */
> +	mfspr   r7, SPRN_PWRMGTCR0
> +	/* Enable Altivec Idle */
> +	oris    r7, r7, PWRMGTCR0_AV_IDLE_PD_EN at h
> +	li      r9, AV_WAIT_IDLE_BIT
> +	/* Set Automatic AltiVec Idle Count */
> +	rlwimi  r7, r9, PWRMGTCR0_AV_IDLE_CNT_SHIFT, PWRMGTCR0_AV_IDLE_CNT
> +	mtspr   SPRN_PWRMGTCR0, r7
> +
> +5:
> +	/*
> +	 * 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.
> +	 */
> +
> +	li	r7, 0
> +	mtspr	SPRN_PIR, r7
> +
> +	wrteei  1
> +	wait
> +#else
> +	li	r5, 0
> +	lis	r5, HID0_DOZE at h
> +	mfspr	r7, SPRN_HID0
> +	rlwinm	r7, r7, 0, ~(HID0_DOZE | HID0_NAP | HID0_SLEEP)
> +	or	r7, r7, r5
> +	isync
> +	mtspr	SPRN_HID0, r7
> +	isync
> +
> +	mfmsr	r7
> +	oris	r7, r7, MSR_WE at h
> +	ori	r7, r7, MSR_EE
> +	msync
> +	mtmsr	r7
> +	isync
> +5:	bl	5b
> +#endif
> +6:
> +	wrteei	0
> +
> +	/*
> +	 * 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
> +
> +	/* Wait EOI finish */
> +7:
> +	lwz	r7, ENTRY_PIR(r10)
> +	andi.	r7, r7, 0xffff
> +	bne	8f
> +	bl	7b
> +8:
> +
>  	isync
> 
>  	/* get the upper bits of the addr */
> diff --git a/arch/powerpc/include/asm/processor.h
> b/arch/powerpc/include/asm/processor.h
> index 2ed51b1..4d49317 100644
> --- a/arch/powerpc/include/asm/processor.h
> +++ b/arch/powerpc/include/asm/processor.h
> @@ -482,6 +482,7 @@
>  #define SPRN_GIVOR8	0x1bb	/* Guest Interrupt Vector Offset Register 8 */
>  #define SPRN_GIVOR13	0x1bc	/* Guest Interrupt Vector Offset Register 13 */
>  #define SPRN_GIVOR14	0x1bd	/* Guest Interrupt Vector Offset Register 14 */
> +#define SPRN_EPR	0x2be	/* External Proxy Register */
> 
>  /* e500 definitions */
>  #define SPRN_L1CFG0	0x203	/* L1 Cache Configuration Register 0 */
> @@ -565,6 +566,13 @@
>  #define SPRN_BBTAR	0x202	/* Branch Buffer Target Address Register */
>  #define SPRN_PID1	0x279	/* Process ID Register 1 */
>  #define SPRN_PID2	0x27a	/* Process ID Register 2 */
> +#define SPRN_PWRMGTCR0  0x3fb	/* Power management control register 0 */
> +#define   PWRMGTCR0_PW20_WAIT		(1 << 14) /* PW20 state enable bit */
> +#define   PWRMGTCR0_PW20_ENT_SHIFT	8
> +#define   PWRMGTCR0_PW20_ENT		0x3f00
> +#define   PWRMGTCR0_AV_IDLE_PD_EN	(1 << 22) /* Altivec idle enable */
> +#define   PWRMGTCR0_AV_IDLE_CNT_SHIFT	16
> +#define   PWRMGTCR0_AV_IDLE_CNT		0x3f0000
>  #define SPRN_MCSR	0x23c	/* Machine Check Syndrome register */
>  #define SPRN_MCAR	0x23d	/* Machine Check Address register */
>  #define MCSR_MCS	0x80000000	/* Machine Check Summary */
> --
> 2.1.0.27.g96db324

[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@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

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

[Scott Wood]

On Wed, 2015-01-21 at 01:23 -0600, Wang Dongsheng-B40534 wrote:
> 
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, January 20, 2015 8:33 AM
> > To: Wang Dongsheng-B40534
> > Cc: Sun York-R58495; Li Yang-Leo-R58472; Jin Zhengxiong-R64188; u-
> > boot at lists.denx.de
> > Subject: Re: [PATCH] powerpc/fsl: support low power boot for e500 and later
> > 
> > On Thu, 2015-01-15 at 14:04 +0800, Dongsheng Wang wrote:
> > > From: Wang Dongsheng <dongsheng.wang@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.
> > 
> 
> Agree.
> 
> > 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?
> > 
> 
> Save the power is our purpose no matter at any stage. If we can save the power
> that we need to do it.

No, we don't "need to do it" if the savings are negligible compared to
the complexity, or if it's for an obscure use case.

Worse, this could end up increasing power savings due to quickly
entering and then leaving low power mode (unlikely with PW10, but if you
end up in PW20 during the normal boot sequence, it could).

> > Where is this new mechanism documented?
> >
> 
> I don't understand. Do you mean Hardware datasheet? 

No, I mean software documentation for the new bootloader/kernel
interface you're creating.  The existing interface is documented in
ePAPR.

> > > 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.
> > 
> 
> Why there are confused? 

I suspect you mean "jump to __second_half_boot_page" but it just doesn't
read that way when you omit the "second" (and it doesn't help that the
meaning of "second half" wasn't immediately obvious from context).  It
makes me ask questions like, "Is there a first_half_boot_page?" or "Is
there a full_boot_page?"

> The address of __bootpg_addr is not a physical address.
> Boot page will jump to a virtual address.

What does that have to do with the label name?

> > >  	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?
> > 
> 
> Ok, replace the magic number with macro.
> 
> 1 means the current process is come from kicked up flow.
> 0 means the current process is first boot up flow.
> 
> > 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?
> > 
> 
> Now the value of IVOR is jump_half_boot_page. In __bootpg_addr I can check the waken
> up cpu that is kernel want to kick. If not the cpu will loop in low power
> state again.

Whatever it is that you want to check for, the first thing you do when
you enter the interrupt is branch to 3f.  How does setting IVOR to
jump_half_boot_page rather than 3: help with what you're trying to do?

> > >  	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"?
> > 
> 
> Mask the cpu that cpu fall into low power state.

Forget the reason -- just tell me how the resulting state of "li r6,0;
addi r6, r6, 1" will be different from "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"?
> 
> Thanks... Yes, EE will be set.
> 
> > Why would we get unexpected interrupts?
> > 
> 
> When EE be set, cpu can receive external interrupt.

But what else would be sending an interrupt to this CPU at this point?

> If a cpu is not kicked in kernel that be triggered by external interrupt, we
> need to make sure the cpu can be loop in low power again.

No, you can just say that the first interrupt will wake the CPU and it's
up to the OS to program the MPIC properly.

> > > +	/* 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?
> > 
> 
> e6500rev2 support PW20, the PW20 lower than PW10, Why not?

See the beginning of the mail.

> > > +#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.
> > 
> 
> Sorry, I don?t understand "obvious we won't hit it"...

What I mean is that we do not want to enter PW20 during normal boot.
The precise time it takes before the OS releases the secondary CPUs can
vary, so the timeout should be large enough that we are highly confident
that the CPUs will be released before the PW20 timer expires.

> > > +	/*
> > > +	 * 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?
> > 
> 
> PIR is for each CPU. 

I am aware of this.

> The NR_CPUS just a number of kernel control to start the CPU.

It's the maximum number of CPUs that the kernel will support.

> We need to set the same value for each CPU PIR register.

No, we don't.

>  Please see my comments for Why we need to do it.

If you think you've answered something and I ask again, it generally
means I'm having a hard time understanding and it would be helpful to
reword rather than just copying and pasting or telling me to read them
again (I think I finally understand what you meant, but it's not clear).

> * 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.

So the problem is that you're using a Linux function to send the msgsnd
before it was meant to be called.  Don't do that.  Either set up the
required data earlier, or use a lower level function that doesn't have
that prerequisite.

> > > +	/*
> > > +	 * 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.
> > 
> 
> In kernel the MPCI IPI message not be supported on CORENET platform and later.

Yes, it is.  We just don't use it.

> After e500v2, proxy mode always enable in MPIC.

No, it's not.  It has to be manually enabled by the kernel.  U-Boot
cannot know if the kernel has done this (without reading the relevant
MPIC register).

Neither of the above two sentences answer my questions.  What stops you
from trying to read EPR on e500v2?

-Scott