7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

[Benjamin Herrenschmidt]

(For those who haven't followed the beginning, current git locks up at
boot on most recent powermacs. It was tracked down to a weird problem
with the idle code. My latest experiments seem to show something dodgy
with MSR_POW). Help from Freescale folks would be appreciated.

On Sat, 2006-04-08 at 12:55 +1000, Paul Mackerras wrote: 

> This patch fixes it for me on my powerbook (1.5GHz albook).  The issue
> seems to be that the cpu objects to HID0_NAP being cleared in HID0.
> If I have this code power_save_6xx_restore, it hangs:
> 
> _GLOBAL(power_save_6xx_restore)
> 	mfspr	r11,SPRN_HID0
> 	rlwinm	r11,r11,0,10,8		/* Clear NAP */
> 	mtspr	SPRN_HID0,r11
> 	b	transfer_to_handler_cont
> 
> If I take out that rlwinm, it boots.  Bizaare.

If you do that, you cause the transfer_to_handler to always call
power_save_6xx_restore even when not coming back from idle.

I did a bit more tracking and it's very strange.... At first, I
discovered that adding a printk after the call to power_save fixed it. I
did all sort of tests and if my memory serves me well, a simple mb()
there will fix it too. In fact, what I noticed is that if I do

 if (mfmsr() & MSR_POW)
	printk("GACK !\n");

After calling ppc_md.power_save() and before local_irq_enable(), it does
trigger ! But with an mb() just before, it doesn't. In fact, you don't
need an mb()... all you need is another mfmsr(). Thus a dummy msmsr()
"fixes" the stale MSR_POW in there.

That is very dodgy. Looks like we get a stale MSR_POW upon return from
the exception that interrupted sleep, causing the next
local_irq_enable() to block forever.

The next question is how does it get there... my idea at first was that
we get MSR_POW in SRR1 in that exception and put it back in with rfi
(and the CPU gets it that way instead of ignoring it). Sounds like a
lovely explanation if we also add that a sync or an mfmsr "clears" this
weird condition. However, I added clearing of MSR_POW in r9 in
EXCEPTION_PROLOG_2() and it didn't fix it (but maybe I did something
wrong, I was tired).

I can't see right now why your hack to the restore code seems to fix it
as well... it should only cause us to do dodgy things on every exception
return path.

I have to go to bed now, maybe somebody will have found more useful data
by the time I wakeup ;) In the meantime, adding an mfmsr at the end of
idle_6xx might do the trick. Paul, we should check if MSR_POW is
supposed to be mirrored in SRR1... if it is, we can simplify/optimize
the code in transfer_to_handler to not load HID0 all the time. Also, we
should merge some of your other cleanups of the restore from idle in all
cases.

Ben.

Re: 7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

[Becky Bruce]

On Apr 13, 2006, at 5:20 AM, Benjamin Herrenschmidt wrote:

> (For those who haven't followed the beginning, current git locks up at
> boot on most recent powermacs. It was tracked down to a weird problem
> with the idle code. My latest experiments seem to show something dodgy
> with MSR_POW). Help from Freescale folks would be appreciated.
>

Ben, I think I know what the problem is - comments below.

> On Sat, 2006-04-08 at 12:55 +1000, Paul Mackerras wrote:
>
>> This patch fixes it for me on my powerbook (1.5GHz albook).  The  
>> issue
>> seems to be that the cpu objects to HID0_NAP being cleared in HID0.
>> If I have this code power_save_6xx_restore, it hangs:
>>
>> _GLOBAL(power_save_6xx_restore)
>> 	mfspr	r11,SPRN_HID0
>> 	rlwinm	r11,r11,0,10,8		/* Clear NAP */
>> 	mtspr	SPRN_HID0,r11
>> 	b	transfer_to_handler_cont
>>
>> If I take out that rlwinm, it boots.  Bizaare.
>
> If you do that, you cause the transfer_to_handler to always call
> power_save_6xx_restore even when not coming back from idle.
>
> I did a bit more tracking and it's very strange.... At first, I
> discovered that adding a printk after the call to power_save fixed  
> it. I
> did all sort of tests and if my memory serves me well, a simple mb()
> there will fix it too. In fact, what I noticed is that if I do
>
>  if (mfmsr() & MSR_POW)
> 	printk("GACK !\n");
>
> After calling ppc_md.power_save() and before local_irq_enable(), it  
> does
> trigger ! But with an mb() just before, it doesn't. In fact, you don't
> need an mb()... all you need is another mfmsr(). Thus a dummy msmsr()
> "fixes" the stale MSR_POW in there.
>
> That is very dodgy. Looks like we get a stale MSR_POW upon return from
> the exception that interrupted sleep, causing the next
> local_irq_enable() to block forever.

Actually, I think the problem is that the code linux is using to turn  
on nap mode is not guaranteed to put the processor in nap mode by the  
time the blr in ppc6xx_idle occurs.

This is at the bottom of ppc6xx_idle:

         mfmsr   r7
         ori     r7,r7,MSR_EE
         oris    r7,r7,MSR_POW@h
         sync
         isync
         mtmsr   r7
         isync
         sync
         blr

Unfortunately, NAP mode does not necessarily fully take effect for  
some number of cycles after the mtmsr, and the sync isn't enough to  
guarantee this.  So it's entirely possible that you execute the blr  
and carry on with the next function, which is local_irq_enable (or  
perhaps a MSR read in the case of your test code) which is going see  
the MSR value with POW set because you haven't started napping yet.

The above code should really look like this:

         mfmsr   r7
         ori     r7,r7,MSR_EE
         oris    r7,r7,MSR_POW@h
         sync
         isync
         mtmsr   r7
         isync
label:
         b label
	blr

>
> The next question is how does it get there... my idea at first was  
> that
> we get MSR_POW in SRR1 in that exception and put it back in with rfi
> (and the CPU gets it that way instead of ignoring it). Sounds like a
> lovely explanation if we also add that a sync or an mfmsr "clears"  
> this
> weird condition. However, I added clearing of MSR_POW in r9 in
> EXCEPTION_PROLOG_2() and it didn't fix it (but maybe I did something
> wrong, I was tired).

This wouldn't help - MSR[POW] is cleared on exception and is not a  
bit that is saved in SRR1.

Hope this helps - I don't have hardware to test this on, so I can't  
be sure, but it seems to explain the behavior you're seeing if I'm  
understanding the problem correctly.

Cheers,
Becky

Re: 7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

[Benjamin Herrenschmidt]

> 
> The above code should really look like this:
> 
>          mfmsr   r7
>          ori     r7,r7,MSR_EE
>          oris    r7,r7,MSR_POW@h
>          sync
>          isync
>          mtmsr   r7
>          isync
> label:
>          b label
> 	blr

Ohhh ... we always assumed mtmsr with MSR_POW was
immediate/synchronous ! That explains a lot. The problem with the above
though is that we'll never get out unless we also hack the exception
path to change the return address once an exception happens. It's not
that difficult especially since we already have a special case to handle
returning from NAP there, on ppc32 at least. ppc64 will need a bit more
investigation.

Do you see another way to loop until NAP has gone ? Maybe reading msr in
a loop until POW gets cleared would do the trick ?

> Hope this helps - I don't have hardware to test this on, so I can't  
> be sure, but it seems to explain the behavior you're seeing if I'm  
> understanding the problem correctly.

It definitely does ! Thanks a lot.

Ben.

Re: 7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

[Becky Bruce]

On Apr 13, 2006, at 3:55 PM, Benjamin Herrenschmidt wrote:

>
>>
>> The above code should really look like this:
>>
>>          mfmsr   r7
>>          ori     r7,r7,MSR_EE
>>          oris    r7,r7,MSR_POW@h
>>          sync
>>          isync
>>          mtmsr   r7
>>          isync
>> label:
>>          b label
>> 	blr
>
> Ohhh ... we always assumed mtmsr with MSR_POW was
> immediate/synchronous ! That explains a lot. The problem with the  
> above
> though is that we'll never get out unless we also hack the exception
> path to change the return address once an exception happens. It's not
> that difficult especially since we already have a special case to  
> handle
> returning from NAP there, on ppc32 at least. ppc64 will need a bit  
> more
> investigation.
>

Agreed, this is yuck :(

> Do you see another way to loop until NAP has gone ? Maybe reading  
> msr in
> a loop until POW gets cleared would do the trick ?

So, it makes sense to me that this would work, but I suspect there  
may be hardware wierdness - the user manual is very specific about  
the code sequence that should be used (although I've given you a  
slightly different sequence in my last mail that is also known to  
work and is cleaner, IMHO).  Let me check with one of our HW  
designers to see if this is OK.  It might be tomorrow before I have  
an answer - it's after 4:30 here and some of them are early birds,  
and might have already left for the day.

FYI, the user's manual recommends this sequence:
loop:
       sync
       mtmsr POW
       isync
       b     loop

>
>> Hope this helps - I don't have hardware to test this on, so I can't
>> be sure, but it seems to explain the behavior you're seeing if I'm
>> understanding the problem correctly.
>
> It definitely does ! Thanks a lot.
>

NP.

Cheers!
-B

Re: 7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

[Benjamin Herrenschmidt]

> FYI, the user's manual recommends this sequence:
> loop:
>        sync
>        mtmsr POW
>        isync
>        b     loop

Ok, that's what OS X does... I always wondered ...

So ideally, we should do something similar to the above and set some
global bit somewhere telling the exception path to change the return
address. In either case, the actual form of the loop becomes fairly
irrelevant.

I need to verify what's up with the 970. I noticed Apple has some
additional weird tricks involving setting the DEC to a short value but
setting POW without EE (though I don't remember for sure, I should dbl
check their code). I suppose I should ask some IBM folks there.

Ben.
 

Re: 7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

[Olof Johansson]

On Fri, Apr 14, 2006 at 08:37:21AM +1000, Benjamin Herrenschmidt wrote:

> I need to verify what's up with the 970. I noticed Apple has some

970 keeps executing too. I guess it's never bitten hard enough to
trigger anything serious.


-Olof

Re: 7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

[Paul Mackerras]

Becky Bruce writes:

> Actually, I think the problem is that the code linux is using to turn  
> on nap mode is not guaranteed to put the processor in nap mode by the  
> time the blr in ppc6xx_idle occurs.

Thanks, Becky.

This patch fixes it for me.  Comments, anyone?

Paul.

diff -urN powerpc-merge/arch/powerpc/kernel/idle_6xx.S pmac-2.6.17-rc1/arch/powerpc/kernel/idle_6xx.S
--- powerpc-merge/arch/powerpc/kernel/idle_6xx.S	2006-04-04 23:09:16.000000000 -0700
+++ pmac-2.6.17-rc1/arch/powerpc/kernel/idle_6xx.S	2006-04-14 10:29:54.000000000 -0700
@@ -151,41 +151,47 @@
 	isync
 	mtmsr	r7
 	isync
-	sync
-	blr
+1:	b	1b
 	
 /*
  * Return from NAP/DOZE mode, restore some CPU specific registers,
  * we are called with DR/IR still off and r2 containing physical
- * address of current.
+ * address of current.  R11 and CR contain HID0.  We have to preserve
+ * r10 and r12.
  */
 _GLOBAL(power_save_6xx_restore)
+	tophys(r11, r1)		/* Make the idle task do a blr */
+	lwz	r9,_LINK(r11)
+	stw	r9,_NIP(r11)
 	mfspr	r11,SPRN_HID0
-	rlwinm.	r11,r11,0,10,8	/* Clear NAP & copy NAP bit !state to cr1 EQ */
-	cror	4*cr1+eq,4*cr0+eq,4*cr0+eq
+	rlwinm	r11,r11,0,10,8	/* Clear NAP */
 BEGIN_FTR_SECTION
 	rlwinm	r11,r11,0,9,7	/* Clear DOZE */
 END_FTR_SECTION_IFSET(CPU_FTR_CAN_DOZE)
 	mtspr	SPRN_HID0, r11
 
 #ifdef DEBUG
-	beq	cr1,1f
+	bf	9,1f
 	lis	r11,(nap_return_count-KERNELBASE)@ha
 	lwz	r9,nap_return_count@l(r11)
 	addi	r9,r9,1
 	stw	r9,nap_return_count@l(r11)
 1:
 #endif
-	
+
+#ifdef CONFIG_SMP
 	rlwinm	r9,r1,0,0,18
 	tophys(r9,r9)
 	lwz	r11,TI_CPU(r9)
 	slwi	r11,r11,2
+#else
+	li	r11,0
+#endif
 	/* Todo make sure all these are in the same page
-	 * and load r22 (@ha part + CPU offset) only once
+	 * and load r11 (@ha part + CPU offset) only once
 	 */
 BEGIN_FTR_SECTION
-	beq	cr1,1f
+	bf	9,1f
 	addis	r9,r11,(nap_save_msscr0-KERNELBASE)@ha
 	lwz	r9,nap_save_msscr0@l(r9)
 	mtspr	SPRN_MSSCR0, r9

Re: 7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

[Olof Johansson]

Hi,

On Fri, Apr 14, 2006 at 12:07:23PM -0700, Paul Mackerras wrote:
> Becky Bruce writes:
> 
> > Actually, I think the problem is that the code linux is using to turn  
> > on nap mode is not guaranteed to put the processor in nap mode by the  
> > time the blr in ppc6xx_idle occurs.
> 
> Thanks, Becky.
> 
> This patch fixes it for me.  Comments, anyone?

The bf mnemonics had me scratching my head a while, it's not listed as
a simplified mnemonic in the 64-bit PEM. Two questions below.

>  _GLOBAL(power_save_6xx_restore)
> +	tophys(r11, r1)		/* Make the idle task do a blr */
> +	lwz	r9,_LINK(r11)
> +	stw	r9,_NIP(r11)
>  	mfspr	r11,SPRN_HID0
> -	rlwinm.	r11,r11,0,10,8	/* Clear NAP & copy NAP bit !state to cr1 EQ */
> -	cror	4*cr1+eq,4*cr0+eq,4*cr0+eq
> +	rlwinm	r11,r11,0,10,8	/* Clear NAP */
>  BEGIN_FTR_SECTION
>  	rlwinm	r11,r11,0,9,7	/* Clear DOZE */
>  END_FTR_SECTION_IFSET(CPU_FTR_CAN_DOZE)
>  	mtspr	SPRN_HID0, r11
>  
>  #ifdef DEBUG
> -	beq	cr1,1f
> +	bf	9,1f

Where is cr0 set now -- you took the dot off of rlwinm?

>  	lis	r11,(nap_return_count-KERNELBASE)@ha
>  	lwz	r9,nap_return_count@l(r11)
>  	addi	r9,r9,1
>  	stw	r9,nap_return_count@l(r11)
>  1:
>  #endif
> -	
> +
> +#ifdef CONFIG_SMP
>  	rlwinm	r9,r1,0,0,18
>  	tophys(r9,r9)
>  	lwz	r11,TI_CPU(r9)
>  	slwi	r11,r11,2
> +#else
> +	li	r11,0
> +#endif
>  	/* Todo make sure all these are in the same page
> -	 * and load r22 (@ha part + CPU offset) only once
> +	 * and load r11 (@ha part + CPU offset) only once
>  	 */
>  BEGIN_FTR_SECTION
> -	beq	cr1,1f
> +	bf	9,1f

Same comment as above w.r.t. cr0?



-Olof

Re: 7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

[Becky Bruce]

He's being sneaky - there's a copy of HID0 in the CR at this point  
from the caller, and bit 9 is the position for NAP.

-B

On Apr 14, 2006, at 2:54 PM, Olof Johansson wrote:

> Hi,
>
> On Fri, Apr 14, 2006 at 12:07:23PM -0700, Paul Mackerras wrote:
>> Becky Bruce writes:
>>
>>> Actually, I think the problem is that the code linux is using to  
>>> turn
>>> on nap mode is not guaranteed to put the processor in nap mode by  
>>> the
>>> time the blr in ppc6xx_idle occurs.
>>
>> Thanks, Becky.
>>
>> This patch fixes it for me.  Comments, anyone?
>
> The bf mnemonics had me scratching my head a while, it's not listed as
> a simplified mnemonic in the 64-bit PEM. Two questions below.
>
>>  _GLOBAL(power_save_6xx_restore)
>> +	tophys(r11, r1)		/* Make the idle task do a blr */
>> +	lwz	r9,_LINK(r11)
>> +	stw	r9,_NIP(r11)
>>  	mfspr	r11,SPRN_HID0
>> -	rlwinm.	r11,r11,0,10,8	/* Clear NAP & copy NAP bit !state to cr1  
>> EQ */
>> -	cror	4*cr1+eq,4*cr0+eq,4*cr0+eq
>> +	rlwinm	r11,r11,0,10,8	/* Clear NAP */
>>  BEGIN_FTR_SECTION
>>  	rlwinm	r11,r11,0,9,7	/* Clear DOZE */
>>  END_FTR_SECTION_IFSET(CPU_FTR_CAN_DOZE)
>>  	mtspr	SPRN_HID0, r11
>>
>>  #ifdef DEBUG
>> -	beq	cr1,1f
>> +	bf	9,1f
>
> Where is cr0 set now -- you took the dot off of rlwinm?
>
>>  	lis	r11,(nap_return_count-KERNELBASE)@ha
>>  	lwz	r9,nap_return_count@l(r11)
>>  	addi	r9,r9,1
>>  	stw	r9,nap_return_count@l(r11)
>>  1:
>>  #endif
>> -	
>> +
>> +#ifdef CONFIG_SMP
>>  	rlwinm	r9,r1,0,0,18
>>  	tophys(r9,r9)
>>  	lwz	r11,TI_CPU(r9)
>>  	slwi	r11,r11,2
>> +#else
>> +	li	r11,0
>> +#endif
>>  	/* Todo make sure all these are in the same page
>> -	 * and load r22 (@ha part + CPU offset) only once
>> +	 * and load r11 (@ha part + CPU offset) only once
>>  	 */
>>  BEGIN_FTR_SECTION
>> -	beq	cr1,1f
>> +	bf	9,1f
>
> Same comment as above w.r.t. cr0?
>
>
>
> -Olof

Re: 7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

[Benjamin Herrenschmidt]

On Fri, 2006-04-14 at 15:00 -0500, Becky Bruce wrote:
> He's being sneaky - there's a copy of HID0 in the CR at this point  
> from the caller, and bit 9 is the position for NAP.

It's a trick I learned from Darwin :) They do that regulary when code is
very cpu-feature dependant, like cache code for example, they put the
cpu features bitmask in CR and do branches based on individual bits of
it here or there.

Ben.

Re: 7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

[Paul Mackerras]

Olof Johansson writes:

> Where is cr0 set now -- you took the dot off of rlwinm?

transfer_to_handler does mfspr r11,SPRN_HID0; mtcr r11 before jumping
to power_save_6xx_restore.  The rlwinm. was wrong anyway since it was
setting cr0.eq based on all the *other* bits in HID0, not HID0_NAP
(doh!).

Paul.

Re: 7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

[Olof Johansson]

On Fri, Apr 14, 2006 at 01:19:36PM -0700, Paul Mackerras wrote:
> Olof Johansson writes:
> 
> > Where is cr0 set now -- you took the dot off of rlwinm?
> 
> transfer_to_handler does mfspr r11,SPRN_HID0; mtcr r11 before jumping
> to power_save_6xx_restore.  The rlwinm. was wrong anyway since it was
> setting cr0.eq based on all the *other* bits in HID0, not HID0_NAP
> (doh!).

Oh, right, you even updated the comment to reflect this. My bad.
Patch looks good to me then.


-Olof

Re: 7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

[Becky Bruce]

On Fri, 2006-04-14 at 12:07 -0700, Paul Mackerras wrote:

> Becky Bruce writes:
>
>
>> Actually, I think the problem is that the code linux is using to turn
>> on nap mode is not guaranteed to put the processor in nap mode by the
>> time the blr in ppc6xx_idle occurs.
>>
>
> Thanks, Becky.
>
> This patch fixes it for me.  Comments, anyone?

Patch LGTM, as well.  I like the approach.

Thanks!
-Becky

Re: 7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

[Benjamin Herrenschmidt]

On Fri, 2006-04-14 at 12:07 -0700, Paul Mackerras wrote:
> Becky Bruce writes:
> 
> > Actually, I think the problem is that the code linux is using to turn  
> > on nap mode is not guaranteed to put the processor in nap mode by the  
> > time the blr in ppc6xx_idle occurs.
> 
> Thanks, Becky.
> 
> This patch fixes it for me.  Comments, anyone?

Looks good to me except that we need the same for ppc64 since the 970
theorically has the same problem...

Ben.

Re: 7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

[Paul Mackerras]

Benjamin Herrenschmidt writes:

> Looks good to me except that we need the same for ppc64 since the 970
> theorically has the same problem...

OK, does this look OK to everyone, before I send it off to Linus?  I
now use a bit in the thread_info rather than using the HID0 bits
themselves to indicate that we're napping, since the m[ft]spr might be
slow.  I added a `local_flags' field to the thread_info struct for
things that are only changed by the task itself and therefore don't
need to be accessed atomically.

This version does the same sort of change for the 970 as for 6xx.

Oh, and I also fixed a stupid bug in the 32-bit stack overflow code,
where we put &_end into r11, and then if there was a stack overflow,
saved registers into the stack frame pointed to by r11. :)

Paul.

diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index 54b48f3..8f85c5e 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -91,6 +91,7 @@ #endif /* CONFIG_SPE */
 #endif /* CONFIG_PPC64 */
 
 	DEFINE(TI_FLAGS, offsetof(struct thread_info, flags));
+	DEFINE(TI_LOCAL_FLAGS, offsetof(struct thread_info, local_flags));
 	DEFINE(TI_PREEMPT, offsetof(struct thread_info, preempt_count));
 	DEFINE(TI_TASK, offsetof(struct thread_info, task));
 #ifdef CONFIG_PPC32
diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
index b3a9794..8866fd2 100644
--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S
@@ -128,37 +128,36 @@ #if defined(CONFIG_40x) || defined(CONFI
 	stw	r12,4(r11)
 #endif
 	b	3f
+
 2:	/* if from kernel, check interrupted DOZE/NAP mode and
          * check for stack overflow
          */
+	lwz	r9,THREAD_INFO-THREAD(r12)
+	cmplw	r1,r9			/* if r1 <= current->thread_info */
+	ble-	stack_ovf		/* then the kernel stack overflowed */
+5:
 #ifdef CONFIG_6xx
-	mfspr	r11,SPRN_HID0
-	mtcr	r11
-BEGIN_FTR_SECTION
-	bt-	8,4f			/* Check DOZE */
-END_FTR_SECTION_IFSET(CPU_FTR_CAN_DOZE)
-BEGIN_FTR_SECTION
-	bt-	9,4f			/* Check NAP */
-END_FTR_SECTION_IFSET(CPU_FTR_CAN_NAP)
+	tophys(r9,r9)			/* check local flags */
+	lwz	r12,TI_LOCAL_FLAGS(r9)
+	mtcrf	0x01,r12
+	bt-	31-TLF_NAPPING,4f
 #endif /* CONFIG_6xx */
 	.globl transfer_to_handler_cont
 transfer_to_handler_cont:
-	lwz	r11,THREAD_INFO-THREAD(r12)
-	cmplw	r1,r11			/* if r1 <= current->thread_info */
-	ble-	stack_ovf		/* then the kernel stack overflowed */
 3:
 	mflr	r9
 	lwz	r11,0(r9)		/* virtual address of handler */
 	lwz	r9,4(r9)		/* where to go when done */
-	FIX_SRR1(r10,r12)
 	mtspr	SPRN_SRR0,r11
 	mtspr	SPRN_SRR1,r10
 	mtlr	r9
 	SYNC
 	RFI				/* jump to handler, enable MMU */
 
-#ifdef CONFIG_6xx	
-4:	b	power_save_6xx_restore
+#ifdef CONFIG_6xx
+4:	rlwinm	r12,r12,0,~_TLF_NAPPING
+	stw	r12,TI_LOCAL_FLAGS(r9)
+	b	power_save_6xx_restore
 #endif
 
 /*
@@ -167,10 +166,10 @@ #endif
  */
 stack_ovf:
 	/* sometimes we use a statically-allocated stack, which is OK. */
-	lis	r11,_end@h
-	ori	r11,r11,_end@l
-	cmplw	r1,r11
-	ble	3b			/* r1 <= &_end is OK */
+	lis	r12,_end@h
+	ori	r12,r12,_end@l
+	cmplw	r1,r12
+	ble	5b			/* r1 <= &_end is OK */
 	SAVE_NVGPRS(r11)
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	lis	r1,init_thread_union@ha
diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
index a5ae04a..3b500dc 100644
--- a/arch/powerpc/kernel/head_64.S
+++ b/arch/powerpc/kernel/head_64.S
@@ -381,6 +381,7 @@ #define STD_EXCEPTION_COMMON_LITE(trap, 
 	.globl label##_common;				\
 label##_common:						\
 	EXCEPTION_PROLOG_COMMON(trap, PACA_EXGEN);	\
+	FINISH_NAP;					\
 	DISABLE_INTS;					\
 	bl	.ppc64_runlatch_on;			\
 	addi	r3,r1,STACK_FRAME_OVERHEAD;		\
@@ -388,6 +389,25 @@ label##_common:						\
 	b	.ret_from_except_lite
 
 /*
+ * When the idle code in power4_idle puts the CPU into NAP mode,
+ * it has to do so in a loop, and relies on the external interrupt
+ * and decrementer interrupt entry code to get it out of the loop.
+ * It sets the _TLF_NAPPING bit in current_thread_info()->local_flags
+ * to signal that it is in the loop and needs help to get out.
+ */
+#if defined(CONFIG_PPC_PMAC) || defined(CONFIG_PPC_MAPLE)
+#define FINISH_NAP				\
+BEGIN_FTR_SECTION				\
+	clrrdi	r11,r1,THREAD_SHIFT;		\
+	ld	r9,TI_LOCAL_FLAGS(r11);		\
+	andi.	r10,r9,_TLF_NAPPING;		\
+	bnel	power4_fixup_nap;		\
+END_FTR_SECTION_IFSET(CPU_FTR_CAN_NAP)
+#else
+#define FINISH_NAP
+#endif
+
+/*
  * Start of pSeries system interrupt routines
  */
 	. = 0x100
@@ -1034,12 +1054,22 @@ unrecov_slb:
 	.globl hardware_interrupt_entry
 hardware_interrupt_common:
 	EXCEPTION_PROLOG_COMMON(0x500, PACA_EXGEN)
+	FINISH_NAP
 hardware_interrupt_entry:
 	DISABLE_INTS
 	bl	.ppc64_runlatch_on
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	.do_IRQ
 	b	.ret_from_except_lite
+
+#if defined(CONFIG_PPC_PMAC) || defined(CONFIG_PPC_MAPLE)
+power4_fixup_nap:
+	andc	r9,r9,r10
+	std	r9,TI_LOCAL_FLAGS(r11)
+	ld	r10,_LINK(r1)		/* make idle task do the */
+	std	r10,_NIP(r1)		/* equivalent of a blr */
+	blr
+#endif
 
 	.align	7
 	.globl alignment_common
diff --git a/arch/powerpc/kernel/idle_6xx.S b/arch/powerpc/kernel/idle_6xx.S
index 12a4efb..b45fa0e 100644
--- a/arch/powerpc/kernel/idle_6xx.S
+++ b/arch/powerpc/kernel/idle_6xx.S
@@ -22,8 +22,6 @@ #include <asm/thread_info.h>
 #include <asm/ppc_asm.h>
 #include <asm/asm-offsets.h>
 
-#undef DEBUG
-
 	.text
 
 /*
@@ -109,12 +107,6 @@ BEGIN_FTR_SECTION
 	dcbf	0,r4
 	dcbf	0,r4
 END_FTR_SECTION_IFSET(CPU_FTR_NAP_DISABLE_L2_PR)
-#ifdef DEBUG
-	lis	r6,nap_enter_count@ha
-	lwz	r4,nap_enter_count@l(r6)
-	addi	r4,r4,1
-	stw	r4,nap_enter_count@l(r6)
-#endif	
 2:
 BEGIN_FTR_SECTION
 	/* Go to low speed mode on some 750FX */
@@ -144,48 +136,42 @@ BEGIN_FTR_SECTION
 	DSSALL
 	sync
 END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC)
+	rlwinm	r9,r1,0,0,31-THREAD_SHIFT	/* current thread_info */
+	lwz	r8,TI_LOCAL_FLAGS(r9)	/* set napping bit */
+	ori	r8,r8,_TLF_NAPPING	/* so when we take an exception */
+	stw	r8,TI_LOCAL_FLAGS(r9)	/* it will return to our caller */
 	mfmsr	r7
 	ori	r7,r7,MSR_EE
 	oris	r7,r7,MSR_POW@h
-	sync
-	isync
+1:	sync
 	mtmsr	r7
 	isync
-	sync
-	blr
-	
+	b	1b
+
 /*
  * Return from NAP/DOZE mode, restore some CPU specific registers,
  * we are called with DR/IR still off and r2 containing physical
- * address of current.
+ * address of current.  R11 points to the exception frame (physical
+ * address).  We have to preserve r10.
  */
 _GLOBAL(power_save_6xx_restore)
-	mfspr	r11,SPRN_HID0
-	rlwinm.	r11,r11,0,10,8	/* Clear NAP & copy NAP bit !state to cr1 EQ */
-	cror	4*cr1+eq,4*cr0+eq,4*cr0+eq
-BEGIN_FTR_SECTION
-	rlwinm	r11,r11,0,9,7	/* Clear DOZE */
-END_FTR_SECTION_IFSET(CPU_FTR_CAN_DOZE)
-	mtspr	SPRN_HID0, r11
-
-#ifdef DEBUG
-	beq	cr1,1f
-	lis	r11,(nap_return_count-KERNELBASE)@ha
-	lwz	r9,nap_return_count@l(r11)
-	addi	r9,r9,1
-	stw	r9,nap_return_count@l(r11)
-1:
-#endif
-	
-	rlwinm	r9,r1,0,0,18
-	tophys(r9,r9)
-	lwz	r11,TI_CPU(r9)
+	lwz	r9,_LINK(r11)		/* interrupted in ppc6xx_idle: */
+	stw	r9,_NIP(r11)		/* make it do a blr */
+
+#ifdef CONFIG_SMP
+	mfspr	r12,SPRN_SPRG3
+	lwz	r11,TI_CPU(r12)		/* get cpu number * 4 */
 	slwi	r11,r11,2
+#else
+	li	r11,0
+#endif
 	/* Todo make sure all these are in the same page
-	 * and load r22 (@ha part + CPU offset) only once
+	 * and load r11 (@ha part + CPU offset) only once
 	 */
 BEGIN_FTR_SECTION
-	beq	cr1,1f
+	mfspr	r9,SPRN_HID0
+	andis.	r9,r9,HID0_NAP@h
+	beq	1f
 	addis	r9,r11,(nap_save_msscr0-KERNELBASE)@ha
 	lwz	r9,nap_save_msscr0@l(r9)
 	mtspr	SPRN_MSSCR0, r9
@@ -210,10 +196,3 @@ _GLOBAL(nap_save_hid1)
 
 _GLOBAL(powersave_lowspeed)
 	.long	0
-
-#ifdef DEBUG
-_GLOBAL(nap_enter_count)
-	.space	4
-_GLOBAL(nap_return_count)
-	.space	4
-#endif
diff --git a/arch/powerpc/kernel/idle_power4.S b/arch/powerpc/kernel/idle_power4.S
index 6dad1c0..d85c7c9 100644
--- a/arch/powerpc/kernel/idle_power4.S
+++ b/arch/powerpc/kernel/idle_power4.S
@@ -35,12 +35,16 @@ BEGIN_FTR_SECTION
 	DSSALL
 	sync
 END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC)
+	clrrdi	r9,r1,THREAD_SHIFT	/* current thread_info */
+	ld	r8,TI_LOCAL_FLAGS(r9)	/* set napping bit */
+	ori	r8,r8,_TLF_NAPPING	/* so when we take an exception */
+	std	r8,TI_LOCAL_FLAGS(r9)	/* it will return to our caller */
 	mfmsr	r7
 	ori	r7,r7,MSR_EE
 	oris	r7,r7,MSR_POW@h
-	sync
+1:	sync
 	isync
 	mtmsrd	r7
 	isync
-	sync
-	blr
+	b	1b
+
diff --git a/include/asm-powerpc/thread_info.h b/include/asm-powerpc/thread_info.h
index ffc7462..88b553c 100644
--- a/include/asm-powerpc/thread_info.h
+++ b/include/asm-powerpc/thread_info.h
@@ -37,6 +37,8 @@ struct thread_info {
 	int		preempt_count;		/* 0 => preemptable,
 						   <0 => BUG */
 	struct restart_block restart_block;
+	unsigned long	local_flags;		/* private flags for thread */
+
 	/* low level flags - has atomic operations done on it */
 	unsigned long	flags ____cacheline_aligned_in_smp;
 };
@@ -142,6 +144,12 @@ #define _TIF_SYSCALL_T_OR_A	(_TIF_SYSCAL
 #define _TIF_USER_WORK_MASK	(_TIF_NOTIFY_RESUME | _TIF_SIGPENDING | \
 				 _TIF_NEED_RESCHED | _TIF_RESTORE_SIGMASK)
 #define _TIF_PERSYSCALL_MASK	(_TIF_RESTOREALL|_TIF_NOERROR)
+
+/* Bits in local_flags */
+/* Don't move TLF_NAPPING without adjusting the code in entry_32.S */
+#define TLF_NAPPING		0	/* idle thread enabled NAP mode */
+
+#define _TLF_NAPPING		(1 << TLF_NAPPING)
 
 #endif /* __KERNEL__ */
 

Re: 7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

[Benjamin Herrenschmidt]

On Tue, 2006-04-18 at 15:45 +1000, Paul Mackerras wrote:
> Benjamin Herrenschmidt writes:
> 
> > Looks good to me except that we need the same for ppc64 since the 970
> > theorically has the same problem...
> 
> OK, does this look OK to everyone, before I send it off to Linus?  I
> now use a bit in the thread_info rather than using the HID0 bits
> themselves to indicate that we're napping, since the m[ft]spr might be
> slow.  I added a `local_flags' field to the thread_info struct for
> things that are only changed by the task itself and therefore don't
> need to be accessed atomically.
> 
> This version does the same sort of change for the 970 as for 6xx.

Hrm...

The 970 version bloats the exception prolog significantly... I
understand now why you were talking about putting the code in the exit
path on irc ... I don't like it that way.... Also, if you want to keep
it, maybe use a separate CONFIG_PPC_970STYLE_NAP or something that gets
selected by platforms that can do it ?

I suppose a PACA field would be less inefficient but still sucks... the
exception return to userland code path already accesses thread_info and
definitely looks like a better place to put it... as long as we never
have to add dodgy workarounds when getting out of NAP like we do on 6xx.

Ben.

Re: 7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

[Paul Mackerras]

Benjamin Herrenschmidt writes:

> The 970 version bloats the exception prolog significantly... I

Four instructions, in the external and decrementer interrupt entry
paths - I don't think that's really significant bloat.

> understand now why you were talking about putting the code in the exit
> path on irc ... I don't like it that way.... Also, if you want to keep
> it, maybe use a separate CONFIG_PPC_970STYLE_NAP or something that gets
> selected by platforms that can do it ?

The config option makes sense.

> I suppose a PACA field would be less inefficient but still sucks... the
> exception return to userland code path already accesses thread_info and
> definitely looks like a better place to put it... as long as we never
> have to add dodgy workarounds when getting out of NAP like we do on 6xx.

More likely we'll get more situations like Cell where we come in
through the soft reset vector after sleep.

Paul.

Re: 7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

[Benjamin Herrenschmidt]

On Tue, 2006-04-18 at 16:32 +1000, Paul Mackerras wrote:
> Benjamin Herrenschmidt writes:
> 
> > The 970 version bloats the exception prolog significantly... I
> 
> Four instructions, in the external and decrementer interrupt entry
> paths - I don't think that's really significant bloat.

Yeah well.. including a load.. ok, I admit that should be fairly hot...
btw, I suppose you took care of having those local flags in some hot
cache line ? :)

> More likely we'll get more situations like Cell where we come in
> through the soft reset vector after sleep.

Yeah.

Ben.

Re: 7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

[Olof Johansson]

On Tue, Apr 18, 2006 at 04:32:46PM +1000, Paul Mackerras wrote:

> > understand now why you were talking about putting the code in the exit
> > path on irc ... I don't like it that way.... Also, if you want to keep
> > it, maybe use a separate CONFIG_PPC_970STYLE_NAP or something that gets
> > selected by platforms that can do it ?
> 
> The config option makes sense.

How about a CPU feature bit instead? That way it's possible to build a
multiplatform kernel without getting the overhead on other platforms.
4 nops should be manageable?


-Olof

Re: 7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

[Olof Johansson]

On Tue, Apr 18, 2006 at 09:56:00AM -0500, Olof Johansson wrote:
> On Tue, Apr 18, 2006 at 04:32:46PM +1000, Paul Mackerras wrote:
> 
> > > understand now why you were talking about putting the code in the exit
> > > path on irc ... I don't like it that way.... Also, if you want to keep
> > > it, maybe use a separate CONFIG_PPC_970STYLE_NAP or something that gets
> > > selected by platforms that can do it ?
> > 
> > The config option makes sense.
> 
> How about a CPU feature bit instead? That way it's possible to build a
> multiplatform kernel without getting the overhead on other platforms.
> 4 nops should be manageable?

DOH! I'm going blind, that's already there. So, Ben, you're unhappy
with nopping it out?


-Olof

Re: 7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

[Michael Schmitz]

> > Actually, I think the problem is that the code linux is using to turn
> > on nap mode is not guaranteed to put the processor in nap mode by the
> > time the blr in ppc6xx_idle occurs.
>
> Thanks, Becky.
>
> This patch fixes it for me.  Comments, anyone?

Works for me :-)

	Michael

Re: 7447A strange problem with MSR:POW (WAS: can't boot 2.6.17-rc1)

[Becky Bruce]

Paul,

This new version of the patch breaks 32-bit arch/ppc builds. The changes 
to the idle_6xx code are shared between architectures, but the entry.S 
code and asm-offsets.c are not.  

Here's a patch that puts the changes in arch/ppc as well.  Builds and 
boots on 834x (which is CONFIG_6xx).  

-B

ppc: Fix powersave on arch/ppc

Fix asm_offsets.c and entry.S to work with the new power save code.  
Changes in arch/powerpc needed to exist in arch/ppc as well since the 
idle code is shared by both ppc and powerpc..

Signed-off-by: Becky Bruce <becky.bruce@freescale.com>

---
commit c9b42c4b6ad17aea51066604b70ea7ec399d2e45
tree 38642212d6396ecad721efca967ae1fc8924e096
parent c85f35d06479bf7cb5cfc7cda0ea218a23ed2dc4
author Becky Bruce <becky.bruce@freescale.com> Tue, 18 Apr 2006 14:12:03 -0500
committer Becky Bruce <becky.bruce@freescale.com> Tue, 18 Apr 2006 14:12:03 -0500

 arch/ppc/kernel/asm-offsets.c |    1 +
 arch/ppc/kernel/entry.S       |   33 ++++++++++++++++-----------------
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/arch/ppc/kernel/asm-offsets.c b/arch/ppc/kernel/asm-offsets.c
index 77e4dc7..cc7c4ae 100644
--- a/arch/ppc/kernel/asm-offsets.c
+++ b/arch/ppc/kernel/asm-offsets.c
@@ -134,6 +134,7 @@ main(void)
 	DEFINE(TI_TASK, offsetof(struct thread_info, task));
 	DEFINE(TI_EXECDOMAIN, offsetof(struct thread_info, exec_domain));
 	DEFINE(TI_FLAGS, offsetof(struct thread_info, flags));
+	DEFINE(TI_LOCAL_FLAGS, offsetof(struct thread_info, flags));
 	DEFINE(TI_CPU, offsetof(struct thread_info, cpu));
 	DEFINE(TI_PREEMPT, offsetof(struct thread_info, preempt_count));
 
diff --git a/arch/ppc/kernel/entry.S b/arch/ppc/kernel/entry.S
index 5891ecb..1adc914 100644
--- a/arch/ppc/kernel/entry.S
+++ b/arch/ppc/kernel/entry.S
@@ -128,29 +128,26 @@ transfer_to_handler:
 	stw	r12,4(r11)
 #endif
 	b	3f
+
 2:	/* if from kernel, check interrupted DOZE/NAP mode and
          * check for stack overflow
          */
+	lwz	r9,THREAD_INFO-THREAD(r12)
+	cmplw	r1,r9			/* if r1 <= current->thread_info */
+	ble-	stack_ovf		/* then the kernel stack overflowed */
+5:
 #ifdef CONFIG_6xx
-	mfspr	r11,SPRN_HID0
-	mtcr	r11
-BEGIN_FTR_SECTION
-	bt-	8,4f			/* Check DOZE */
-END_FTR_SECTION_IFSET(CPU_FTR_CAN_DOZE)
-BEGIN_FTR_SECTION
-	bt-	9,4f			/* Check NAP */
-END_FTR_SECTION_IFSET(CPU_FTR_CAN_NAP)
+	tophys(r9,r9)			/* check local flags */
+	lwz	r12,TI_LOCAL_FLAGS(r9)
+	mtcrf	0x01,r12
+	bt-	31-TLF_NAPPING,4f
 #endif /* CONFIG_6xx */
 	.globl transfer_to_handler_cont
 transfer_to_handler_cont:
-	lwz	r11,THREAD_INFO-THREAD(r12)
-	cmplw	r1,r11			/* if r1 <= current->thread_info */
-	ble-	stack_ovf		/* then the kernel stack overflowed */
 3:
 	mflr	r9
 	lwz	r11,0(r9)		/* virtual address of handler */
 	lwz	r9,4(r9)		/* where to go when done */
-	FIX_SRR1(r10,r12)
 	mtspr	SPRN_SRR0,r11
 	mtspr	SPRN_SRR1,r10
 	mtlr	r9
@@ -158,7 +155,9 @@ transfer_to_handler_cont:
 	RFI				/* jump to handler, enable MMU */
 
 #ifdef CONFIG_6xx
-4:	b	power_save_6xx_restore
+4:	rlwinm	r12,r12,0,~_TLF_NAPPING
+	stw	r12,TI_LOCAL_FLAGS(r9)
+	b	power_save_6xx_restore
 #endif
 
 /*
@@ -167,10 +166,10 @@ transfer_to_handler_cont:
  */
 stack_ovf:
 	/* sometimes we use a statically-allocated stack, which is OK. */
-	lis	r11,_end@h
-	ori	r11,r11,_end@l
-	cmplw	r1,r11
-	ble	3b			/* r1 <= &_end is OK */
+	lis	r12,_end@h
+	ori	r12,r12,_end@l
+	cmplw	r1,r12
+	ble	5b			/* r1 <= &_end is OK */
 	SAVE_NVGPRS(r11)
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	lis	r1,init_thread_union@ha