[PATCH] vr41xx: fix problem with vr41xx_cpu_wait

[PATCH] vr41xx: fix problem with vr41xx_cpu_wait

[Ricardo Mendoza]

Hello,

Yoichi, please correct me if I am wrong but I think that the "standby"
instruction does not set IE bit on its own, so calling it with
interrupts disabled will loop the cpu away forever on standby state
being unable to come back due to no interrupts getting through.

Please ack the patch if you consider it correct.

Please apply afterwards, Ralf.


     Ricardo

---

Standby instruction can't be called with interrupts disabled
as it doesn't set IE bit on it's own.

Signed-off-by: Ricardo Mendoza <ricmm@gentoo.org>
---
 arch/mips/vr41xx/common/pmu.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/arch/mips/vr41xx/common/pmu.c b/arch/mips/vr41xx/common/pmu.c
index 028aaf7..6d651ec 100644
--- a/arch/mips/vr41xx/common/pmu.c
+++ b/arch/mips/vr41xx/common/pmu.c
@@ -48,14 +48,8 @@ static void __iomem *pmu_base;
 
 static void vr41xx_cpu_wait(void)
 {
-	local_irq_disable();
 	if (!need_resched())
-		/*
-		 * "standby" sets IE bit of the CP0_STATUS to 1.
-		 */
 		__asm__("standby;\n");
-	else
-		local_irq_enable();
 }
 
 static inline void software_reset(void)

Re: [PATCH] vr41xx: fix problem with vr41xx_cpu_wait

[Yoichi Yuasa]

Hi Ricardo,

On Tue, 5 Aug 2008 10:43:14 +0000
Ricardo Mendoza <ricmm@gentoo.org> wrote:

> Hello,
> 
> Yoichi, please correct me if I am wrong but I think that the "standby"
> instruction does not set IE bit on its own, so calling it with
> interrupts disabled will loop the cpu away forever on standby state
> being unable to come back due to no interrupts getting through.

In VR4100 series User's Manual, it's being written
"IE bit of the Status register in the CP0 is also set to 1".

Do you have any problem on your board?

Yoichi

Re: [PATCH] vr41xx: fix problem with vr41xx_cpu_wait

[Ricardo Mendoza]

On Wed, Aug 06, 2008 at 10:49:01AM +0900, Yoichi Yuasa wrote:
 
> In VR4100 series User's Manual, it's being written
> "IE bit of the Status register in the CP0 is also set to 1".
> 
> Do you have any problem on your board?

Hello Yoichi,

Just now I got my hands on the manual, I can see that the standby
instruction sets IE bit to 1 but only on Vr4131 and Vr4181A cores, all
others (such as my Vr4121) need to have interrupts enabled before going
into standby.

The patch will make it work on all Vr4100 derivates, or we could also
add code to build the function depending on CPU type. What do you think?


     Ricardo

Re: [PATCH] vr41xx: fix problem with vr41xx_cpu_wait

[Yoichi Yuasa]

Hello Ricardo,

On Wed, 6 Aug 2008 02:08:18 +0000
Ricardo Mendoza <ricmm@gentoo.org> wrote:

> On Wed, Aug 06, 2008 at 10:49:01AM +0900, Yoichi Yuasa wrote:
>  
> > In VR4100 series User's Manual, it's being written
> > "IE bit of the Status register in the CP0 is also set to 1".
> > 
> > Do you have any problem on your board?
> 
> Hello Yoichi,
> 
> Just now I got my hands on the manual, I can see that the standby
> instruction sets IE bit to 1 but only on Vr4131 and Vr4181A cores, all
> others (such as my Vr4121) need to have interrupts enabled before going
> into standby.
> 
> The patch will make it work on all Vr4100 derivates, or we could also
> add code to build the function depending on CPU type. What do you think?

local_irq_disable() is included in the sample code on the User's Manul. 
I think the following patch is good way of this.

Signed-off-by: Yoichi Yuasa <yoichi_yuasa@tripeaks.co.jp>

diff -pruN -X /home/yuasa/Memo/dontdiff linux-orig/arch/mips/vr41xx/common/pmu.c linux/arch/mips/vr41xx/common/pmu.c
--- linux-orig/arch/mips/vr41xx/common/pmu.c	2008-08-06 13:23:55.437185676 +0900
+++ linux/arch/mips/vr41xx/common/pmu.c	2008-08-06 13:32:56.744032999 +0900
@@ -46,11 +46,17 @@ static void __iomem *pmu_base;
 #define pmu_read(offset)		readw(pmu_base + (offset))
 #define pmu_write(offset, value)	writew((value), pmu_base + (offset))
 
+static void old_vr41xx_cpu_wait(void)
+{
+	__asm__("standby;\n");
+}
+
 static void vr41xx_cpu_wait(void)
 {
 	local_irq_disable();
 	if (!need_resched())
 		/*
+		 * VR4181A, VR4131 and later,
 		 * "standby" sets IE bit of the CP0_STATUS to 1.
 		 */
 		__asm__("standby;\n");
@@ -124,7 +130,17 @@ static int __init vr41xx_pmu_init(void)
 		return -EBUSY;
 	}
 
-	cpu_wait = vr41xx_cpu_wait;
+	switch (current_cpu_type()) {
+	case CPU_VR4111:
+	case CPU_VR4121:
+	case CPU_VR4122:
+		cpu_wait = old_vr41xx_cpu_wait;
+		break;
+	default:
+		cpu_wait = vr41xx_cpu_wait;
+		break;
+	}
+
 	_machine_restart = vr41xx_restart;
 	_machine_halt = vr41xx_halt;
 	pm_power_off = vr41xx_halt;

Re: [PATCH] vr41xx: fix problem with vr41xx_cpu_wait

[Atsushi Nemoto]

On Wed, 6 Aug 2008 13:42:13 +0900, Yoichi Yuasa <yoichi_yuasa@tripeaks.co.jp> wrote:
> > Just now I got my hands on the manual, I can see that the standby
> > instruction sets IE bit to 1 but only on Vr4131 and Vr4181A cores, all
> > others (such as my Vr4121) need to have interrupts enabled before going
> > into standby.
> > 
> > The patch will make it work on all Vr4100 derivates, or we could also
> > add code to build the function depending on CPU type. What do you think?
> 
> local_irq_disable() is included in the sample code on the User's Manul. 
> I think the following patch is good way of this.
...
> +static void old_vr41xx_cpu_wait(void)
> +{
> +	__asm__("standby;\n");
> +}

Then, old vr41 CPUs have potential latency problem as like as other
CPUs with WAIT instruction.

Please refer "WAIT vs. tickless kernel" thread on linux-mips ML
archive for details.

I don't complain about this patch itself.
---
Atsushi Nemoto

Re: [PATCH] vr41xx: fix problem with vr41xx_cpu_wait

[Ricardo Mendoza]

On Wed, Aug 06, 2008 at 10:30:33PM +0900, Atsushi Nemoto wrote:
 
> > local_irq_disable() is included in the sample code on the User's Manul. 
> > I think the following patch is good way of this.
> ...
> > +static void old_vr41xx_cpu_wait(void)
> > +{
> > +	__asm__("standby;\n");
> > +}
> 
> Then, old vr41 CPUs have potential latency problem as like as other
> CPUs with WAIT instruction.
> 
> Please refer "WAIT vs. tickless kernel" thread on linux-mips ML
> archive for details.
> 
> I don't complain about this patch itself.

I see, I hadn't read that thread before. Yoichi's rev of the patch
should go in but we should put some effort in finding a better route for
CPUs that are not forcefully aware by hardware of interrupts in their
wait call.

Was some work ever done on Ralf's idea in the "WAIT vs. tickless kernel"
ML thread? Regarding patching the return from exception code to contain
a check for this particular loop.


     Ricardo

Re: [PATCH] vr41xx: fix problem with vr41xx_cpu_wait

[Atsushi Nemoto]

On Wed, 6 Aug 2008 16:17:10 +0000, Ricardo Mendoza <ricmm@gentoo.org> wrote:
> I see, I hadn't read that thread before. Yoichi's rev of the patch
> should go in but we should put some effort in finding a better route for
> CPUs that are not forcefully aware by hardware of interrupts in their
> wait call.
> 
> Was some work ever done on Ralf's idea in the "WAIT vs. tickless kernel"
> ML thread? Regarding patching the return from exception code to contain
> a check for this particular loop.

Yes.  My last proposal is:

http://www.linux-mips.org/archives/linux-mips/2007-11/msg00123.html

To support vr41's standby instruction in same way, we might have to
synthesise rollback_handle_int, etc. or r4k_wait at runtime...

---
Atsushi Nemoto

Re: [PATCH] vr41xx: fix problem with vr41xx_cpu_wait

[Ralf Baechle]

On Thu, Aug 07, 2008 at 02:10:57AM +0900, Atsushi Nemoto wrote:

> http://www.linux-mips.org/archives/linux-mips/2007-11/msg00123.html
> 
> To support vr41's standby instruction in same way, we might have to
> synthesise rollback_handle_int, etc. or r4k_wait at runtime...

The infrastructure for that is now there :-)  Another question of course
is if that stuff really deserve this ultimate degree of optimization.

  Ralf

Re: [PATCH] vr41xx: fix problem with vr41xx_cpu_wait

[Atsushi Nemoto]

On Wed, 6 Aug 2008 18:15:31 +0100, Ralf Baechle <ralf@linux-mips.org> wrote:
> > http://www.linux-mips.org/archives/linux-mips/2007-11/msg00123.html
> > 
> > To support vr41's standby instruction in same way, we might have to
> > synthesise rollback_handle_int, etc. or r4k_wait at runtime...
> 
> The infrastructure for that is now there :-)  Another question of course
> is if that stuff really deserve this ultimate degree of optimization.

Well, this patch on top of my patch might be enough.

--- a/arch/mips/kernel/genex.S
+++ b/arch/mips/kernel/genex.S
@@ -152,7 +152,7 @@ LEAF(r4k_wait)
 	.set	push
 	.set	noat
 	MFC0	k0, CP0_EPC
-	PTR_LA	k1, r4k_wait
+	PTR_L	k1, cpu_wait
 	ori	k0, 0x1f	/* 32 byte rollback region */
 	xori	k0, 0x1f
 	bne	k0, k1, 9f

Of course old_vr41xx_cpu_wait should be modified as like as r4k_wait
and "rollback = (cpu_wait == r4k_wait)" lines in traps.c should be
changed...

---
Atsushi Nemoto