Skip to contentSkip to conversation

SiByte fixes for 2.6.12

PATCH Applied Andrew Isaacson 6 participants 27 messages Jun 22, 2005 – Jan 23, 2006 MIPS

lore
Share & tools
lore

Andrew Isaacson

author
#0 lore raw

cover SiByte fixes for 2.6.12

The following series of patches address a few issues I ran into while getting 2.6.12 running on Swarm. Some of them are questionable, for example [3/5] is only an issue because of the oldish toolchain I happen to be using at the moment.

Also, Ralf, do you like these quilt-style patchbombs or do you prefer another submission format?

-andy

author
#1 lore raw

PATCH 1/5 SiByte fixes for 2.6.12

SB1 does not use the R4K TLB code.

Signed-off-by Andrew Isaacson <[email protected]>
arch/mips/kernel/cpu-probe.c +2 -0
Index: linux-2.6-work/arch/mips/kernel/cpu-probe.c
===================================================================
@@ -583,6 +583,8 @@
 	switch (c->processor_id & 0xff00) {
 	case PRID_IMP_SB1:
 		c->cputype = CPU_SB1;
+		c->options &= ~MIPS_CPU_4KTLB;
+		c->options |= MIPS_CPU_TLB;
 #ifdef CONFIG_SB1_PASS_1_WORKAROUNDS
 		/* FPU in pass1 is known to have issues. */
 		c->options &= ~(MIPS_CPU_FPU | MIPS_CPU_32FPR);

--

#2 lore raw
Quoted 1 line · Andrew Isaacson

SB1 does not use the R4K TLB code.

Well, the flag is not really to specify whether the common code is to be
used or not. It's about whether the TLB is like that of the R4k. Actually it's always been a mystery for me why the common code cannot be used for the SB1, but perhaps there is something specific that I could only discover in that "SB-1 Core User Manual" that I yet have to see, sigh...

Of course if your TLB is indeed different from that of the R4k, then you
shouldn't be setting cp0.config.mt to 1 in the first place...

Maciej

#3 lore raw
Quoted 10 lines · Maciej W. Rozycki
Quoted 1 line · Andrew Isaacson

SB1 does not use the R4K TLB code.

Well, the flag is not really to specify whether the common code is to be
used or not. It's about whether the TLB is like that of the R4k. Actually it's always been a mystery for me why the common code cannot be used for the SB1, but perhaps there is something specific that I could only discover in that "SB-1 Core User Manual" that I yet have to see, sigh...

Quoted 2 lines

Of course if your TLB is indeed different from that of the R4k, then you
shouldn't be setting cp0.config.mt to 1 in the first place...

So I don't know everything that went on during the SB1 MIPS port, but what I see at this point makes at least some sense to me. The SB1 has some fancy features that are used in c-sb1.c and thereabouts (the DMA pageops, and avoiding work by using the coherency guarantees, among others).

While it is worthwhile to abstract that code out some more (and I consider it a long-term goal to share as much code as possible with the generic r4k/mips32/mips64 code), it works as is and it's not obvious what small simplifications should be taken at this point. (Or at least, I'm not smart enough to see what obvious small simplifications should be done.) So for the time being I'm in favor of doing what's needed to keep the support from bit-rotting. I'll keep an eye on whatever cleanups you end up doing and keep them up to date with our internal tree...

That said, I'm open to suggestions as to what I should do in the short term to get the right results.

-andy

#4 lore raw
In reply to Maciej W. Rozycki
Quoted 9 lines · Maciej W. Rozycki

Well, the flag is not really to specify whether the common code is to be
used or not. It's about whether the TLB is like that of the R4k. Actually it's always been a mystery for me why the common code cannot be used for the SB1, but perhaps there is something specific that I could only discover in that "SB-1 Core User Manual" that I yet have to see, sigh...

Of course if your TLB is indeed different from that of the R4k, then you
shouldn't be setting cp0.config.mt to 1 in the first place...

The reason was primarily the tiny bit of extra performance because the SB1 doesn't need the hazard handling overhead. Also tlb-sb1 has a few changes that are needed to initialize a TLB in undefined state after powerup. That was needed to run Linux on firmware-less SB1 cores.

Ralf

#5 lore raw
Quoted 5 lines · Ralf Baechle
Quoted 2 lines

Of course if your TLB is indeed different from that of the R4k, then you
shouldn't be setting cp0.config.mt to 1 in the first place...

The reason was primarily the tiny bit of extra performance because the SB1 doesn't need the hazard handling overhead. Also tlb-sb1 has a few

That's hardly a justification for duplicating all the code; I've thought
the reason was actually historical -- hadn't it been simply written separately initially and never got merged properly afterwards?

Quoted 2 lines

changes that are needed to initialize a TLB in undefined state after powerup. That was needed to run Linux on firmware-less SB1 cores.

But that's true about the power-up state of the TLB on any MIPS CPU,
isn't it?

Maciej

#6 lore raw
Quoted 9 lines · Maciej W. Rozycki
Quoted 5 lines
Quoted 2 lines

Of course if your TLB is indeed different from that of the R4k, then you
shouldn't be setting cp0.config.mt to 1 in the first place...

The reason was primarily the tiny bit of extra performance because the SB1 doesn't need the hazard handling overhead. Also tlb-sb1 has a few

That's hardly a justification for duplicating all the code; I've thought
the reason was actually historical -- hadn't it been simply written separately initially and never got merged properly afterwards?

Historically even the R10000 had it's own copy of the TLB code - with the sole reason of existence being it having neither hazards nor suffering from potencial duplicate TLB entries. Well, maybe also the very first stages of MIPS SMP support.

Anyway, as you said that's little reason for an extra copy to exist and so I both got axed.

Quoted 5 lines
Quoted 2 lines

changes that are needed to initialize a TLB in undefined state after powerup. That was needed to run Linux on firmware-less SB1 cores.

But that's true about the power-up state of the TLB on any MIPS CPU,
isn't it?

Some come out of powerup with a cleared TLB but anyway, since normally some piece of firmware takes care of these issues it's not something Linux normally should need to worry about.

Ralf

#7 lore raw
In reply to Ralf Baechle
Quoted 9 lines · Ralf Baechle
Quoted 7 lines · Maciej W. Rozycki

Well, the flag is not really to specify whether the common code is to be
used or not. It's about whether the TLB is like that of the R4k. Actually it's always been a mystery for me why the common code cannot be used for the SB1, but perhaps there is something specific that I could only discover in that "SB-1 Core User Manual" that I yet have to see, sigh...

Quoted 7 lines
Quoted 2 lines

Of course if your TLB is indeed different from that of the R4k, then you
shouldn't be setting cp0.config.mt to 1 in the first place...

The reason was primarily the tiny bit of extra performance because the SB1 doesn't need the hazard handling overhead. Also tlb-sb1 has a few changes that are needed to initialize a TLB in undefined state after powerup. That was needed to run Linux on firmware-less SB1 cores.

FYI, all I have is a piece of hard evidence: this patch was the difference between not booting and booting for a Sentosa with CFE. Which isn't firmwareless and isn't a tiny bit of extra performance issue.

I'll try to give CVS HEAD a shot this week sometime.

Signature · 2 lines
Daniel Jacobowitz
CodeSourcery, LLC
#8 lore raw
Quoted 15 lines · Daniel Jacobowitz
Quoted 7 lines
Quoted 7 lines

Well, the flag is not really to specify whether the common code is to be
used or not. It's about whether the TLB is like that of the R4k. Actually it's always been a mystery for me why the common code cannot be used for the SB1, but perhaps there is something specific that I could only discover in that "SB-1 Core User Manual" that I yet have to see, sigh...

Quoted 7 lines
Quoted 2 lines

Of course if your TLB is indeed different from that of the R4k, then you
shouldn't be setting cp0.config.mt to 1 in the first place...

The reason was primarily the tiny bit of extra performance because the SB1 doesn't need the hazard handling overhead. Also tlb-sb1 has a few changes that are needed to initialize a TLB in undefined state after powerup. That was needed to run Linux on firmware-less SB1 cores.

Quoted 6 lines

FYI, all I have is a piece of hard evidence: this patch was the difference between not booting and booting for a Sentosa with CFE. Which isn't firmwareless and isn't a tiny bit of extra performance issue.

I'll try to give CVS HEAD a shot this week sometime.

Just as reminder for everybody - CVS is dead and frozen, the action is playing on git now ...

Ralf

#9 lore raw
In reply to Daniel Jacobowitz
Quoted 4 lines · Daniel Jacobowitz

FYI, all I have is a piece of hard evidence: this patch was the difference between not booting and booting for a Sentosa with CFE. Which isn't firmwareless and isn't a tiny bit of extra performance issue.

Actually workarounds have been floating around for some time. ;-) But
I'm glad this has now been fixed properly.

Maciej

#10 lore raw
Quoted 6 lines · Andrew Isaacson

SB1 does not use the R4K TLB code.

arch/mips/kernel/cpu-probe.c +2 -0
 		c->cputype = CPU_SB1;
+		c->options &= ~MIPS_CPU_4KTLB;
+		c->options |= MIPS_CPU_TLB;
 #ifdef CONFIG_SB1_PASS_1_WORKAROUNDS
Quoted 18 lines · Daniel Jacobowitz
Quoted 8 lines
Quoted 8 lines

Well, the flag is not really to specify whether the common code is to be
used or not. It's about whether the TLB is like that of the R4k. Actually it's always been a mystery for me why the common code cannot be used for the SB1, but perhaps there is something specific that I could only discover in that "SB-1 Core User Manual" that I yet have to see, sigh...
Of course if your TLB is indeed different from that of the R4k, then you
shouldn't be setting cp0.config.mt to 1 in the first place...

Quoted 4 lines

The reason was primarily the tiny bit of extra performance because the SB1 doesn't need the hazard handling overhead. Also tlb-sb1 has a few changes that are needed to initialize a TLB in undefined state after powerup. That was needed to run Linux on firmware-less SB1 cores.

FYI, all I have is a piece of hard evidence: this patch was the difference between not booting and booting for a Sentosa with CFE. Which isn't firmwareless and isn't a tiny bit of extra performance issue.

I'll try to give CVS HEAD a shot this week sometime.

Quoted 6 lines · Maciej W. Rozycki
Quoted 4 lines

FYI, all I have is a piece of hard evidence: this patch was the difference between not booting and booting for a Sentosa with CFE. Which isn't firmwareless and isn't a tiny bit of extra performance issue.

Actually workarounds have been floating around for some time. ;-) But
I'm glad this has now been fixed properly.

There was some discussion regarding this patch but no real conclusion. Is it working without this patch now, or should it be applied (or modified? - how?).

Signature · 2 lines
Martin Michlmayr
http://www.cyrius.com/
#11 lore raw
Quoted 3 lines · Martin Michlmayr

There was some discussion regarding this patch but no real conclusion. Is it working without this patch now, or should it be applied (or modified? - how?).

Something like that is actually in git, so never mind.

Signature · 2 lines
Martin Michlmayr
http://www.cyrius.com/
author
#13 lore raw

PATCH 2/5 SiByte fixes for 2.6.12

In reply to Andrew Isaacson

Update sb1250_set_affinity to use cpumask_t rather than hand-rolled bitmask code.

Signed-off-by Andrew Isaacson <[email protected]>
arch/mips/sibyte/sb1250/irq.c +4 -11
Index: linux-2.6-work/arch/mips/sibyte/sb1250/irq.c
===================================================================
@@ -53,7 +53,7 @@
 static unsigned int startup_sb1250_irq(unsigned int irq);
 static void ack_sb1250_irq(unsigned int irq);
 #ifdef CONFIG_SMP
-static void sb1250_set_affinity(unsigned int irq, unsigned long mask);
+static void sb1250_set_affinity(unsigned int irq, cpumask_t mask);
 #endif
 
 #ifdef CONFIG_SIBYTE_HAS_LDT

@@ -117,23 +117,16 @@
 }
 
 #ifdef CONFIG_SMP
-static void sb1250_set_affinity(unsigned int irq, unsigned long mask)
+static void sb1250_set_affinity(unsigned int irq, cpumask_t mask)
 {
 	int i = 0, old_cpu, cpu, int_on;
 	u64 cur_ints;
 	irq_desc_t *desc = irq_desc + irq;
 	unsigned long flags;
 
-	while (mask) {
-		if (mask & 1) {
-			mask >>= 1;
-			break;
-		}
-		mask >>= 1;
-		i++;
-	}
+	i = first_cpu(mask);
 
-	if (mask) {
+	if (cpus_weight(mask) > 1) {
 		printk("attempted to set irq affinity for irq %d to multiple CPUs\n", irq);
 		return;
 	}

--

author
#14 lore raw

PATCH 3/5 SiByte fixes for 2.6.12

In reply to Andrew Isaacson

Toolchain compat fix: gas 2.12.1 doesn't understand two-argument jalr, and the $ra is redundant anyways.

Signed-off-by Andrew Isaacson <[email protected]>
arch/mips/lib/uncached.c +1 -1
Index: linux-2.6-work/arch/mips/lib/uncached.c
===================================================================
@@ -26,7 +26,7 @@
 	__asm__ __volatile__ (
 		"	move $16, $sp\n"
 		"	move $sp, %1\n"
-		"	jalr $ra, %2\n"
+		"	jalr %2\n"
 		"	move $sp, $16"
 		: "=&r" (ret)
 		: "r" (usp), "r" (ufunc)

--

#15 lore raw
Quoted 2 lines · Andrew Isaacson

Toolchain compat fix: gas 2.12.1 doesn't understand two-argument jalr, and the $ra is redundant anyways.

Is it really the case? Perhaps it doesn't know the symbolic name of the
register which has only been added recently. Replacing it with $31 should fix the problem, but your patch is obviously correct regardless.

The code is broken anyway -- I've tried fixing it, but I've hit
shortcomings of macros in <asm/addrspace.h> (I've got working code, but GCC complains loudly), so it may yet take a while.

Maciej

#16 lore raw
Quoted 7 lines · Maciej W. Rozycki
Quoted 2 lines · Andrew Isaacson

Toolchain compat fix: gas 2.12.1 doesn't understand two-argument jalr, and the $ra is redundant anyways.

Is it really the case? Perhaps it doesn't know the symbolic name of the
register which has only been added recently. Replacing it with $31 should fix the problem, but your patch is obviously correct regardless.

Yeah, you're right, my old gas just doesn't know $ra. s/ra/31/g works as well.

-andy

#17 lore raw
Quoted 6 lines · Andy Isaacson
Quoted 3 lines

Is it really the case? Perhaps it doesn't know the symbolic name of the
register which has only been added recently. Replacing it with $31 should fix the problem, but your patch is obviously correct regardless.

Yeah, you're right, my old gas just doesn't know $ra. s/ra/31/g works as well.

Here's my proposal to fix run_uncached() -- it works correctly for me for
both 32-bit and 64-bit builds (current code crashes for me for 64 bits, because as a result of the bad calculation a jump outside any valid address space is attempted). I've inspected generated machine code to make sure it's correct as well.

Unfortunately with this code GCC spits out a few bogus warnings for
32-bit builds (this is supposed to be a "non-bug" of the compiler -- see "http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12963" for details). I'm not sure struggling hard to get rid of these warnings, possibly complicating code, is worth the hassle; GCC should be fixed instead.

Unless there are objections I'd like to apply this patch.

Maciej

patch-mips-2.6.12-20050620-run_uncached-8

arch/mips/lib/uncached.c +54 -14
diff -up --recursive --new-file linux-mips-2.6.12-20050620.macro/arch/mips/lib/uncached.c linux-mips-2.6.12-20050620/arch/mips/lib/uncached.c
@@ -4,31 +4,71 @@
  * for more details.
  *
  * Copyright (C) 2005 Thiemo Seufer
+ * Copyright (C) 2005  MIPS Technologies, Inc.  All rights reserved.
+ *	Author: Maciej W. Rozycki <[email protected]>
  */
+
 #include <linux/init.h>
 
 #include <asm/addrspace.h>
+#include <asm/bug.h>
+
+#ifndef CKSEG2
+#define CKSEG2 CKSSEG
+#endif
+#ifndef TO_PHYS_MASK
+#define TO_PHYS_MASK -1
+#endif
 
 /*
- * FUNC is executed in the uncached segment CKSEG1. This works only if
- * both code and stack live in CKSEG0. The stack handling works because
- * we don't handle stack arguments or more complex return values, so we
- * can avoid to share the same stack area between cached and uncached
- * mode.
+ * FUNC is executed in one of the uncached segments, depending on its
+ * original address as follows:
+ *
+ * 1. If the original address is in CKSEG0 or CKSEG1, then the uncached
+ *    segment used is CKSEG1.
+ * 2. If the original address is in XKPHYS, then the uncached segment
+ *    used is XKPHYS(2).
+ * 3. Otherwise it's a bug.
+ *
+ * The same remapping is done with the stack pointer.  Stack handling
+ * works because we don't handle stack arguments or more complex return
+ * values, so we can avoid sharing the same stack area between a cached
+ * and the uncached mode.
  */
 unsigned long __init run_uncached(void *func)
 {
-	register unsigned long sp __asm__("$sp");
-	register unsigned long ret __asm__("$2");
-	unsigned long usp = sp - CAC_BASE + UNCAC_BASE;
-	unsigned long ufunc = func - CAC_BASE + UNCAC_BASE;
+	register long sp __asm__("$sp");
+	register long ret __asm__("$2");
+	long lfunc = (long)func, ufunc;
+	long usp;
+
+	if (sp >= (long)CKSEG0 && sp < (long)CKSEG2)
+		usp = CKSEG1ADDR(sp);
+	else if ((long long)sp >= (long long)PHYS_TO_XKPHYS(0LL, 0) &&
+		 (long long)sp < (long long)PHYS_TO_XKPHYS(8LL, 0))
+		usp = PHYS_TO_XKPHYS((long long)K_CALG_UNCACHED,
+				     XKPHYS_TO_PHYS((long long)sp));
+	else {
+		BUG();
+		usp = sp;
+	}
+	if (lfunc >= (long)CKSEG0 && lfunc < (long)CKSEG2)
+		ufunc = CKSEG1ADDR(lfunc);
+	else if ((long long)lfunc >= (long long)PHYS_TO_XKPHYS(0LL, 0) &&
+		 (long long)lfunc < (long long)PHYS_TO_XKPHYS(8LL, 0))
+		ufunc = PHYS_TO_XKPHYS((long long)K_CALG_UNCACHED,
+				       XKPHYS_TO_PHYS((long long)lfunc));
+	else {
+		BUG();
+		ufunc = lfunc;
+	}
 
 	__asm__ __volatile__ (
-		"	move $16, $sp\n"
-		"	move $sp, %1\n"
-		"	jalr $ra, %2\n"
-		"	move $sp, $16"
-		: "=&r" (ret)
+		"	move	$16, $sp\n"
+		"	move	$sp, %1\n"
+		"	jalr	%2\n"
+		"	move	$sp, $16"
+		: "=r" (ret)
 		: "r" (usp), "r" (ufunc)
 		: "$16", "$31");
#18 lore raw
Quoted 1 line · Maciej W. Rozycki

Unless there are objections I'd like to apply this patch.

Thus it's now in.

Maciej

author
#19 lore raw

PATCH 4/5 SiByte fixes for 2.6.12

In reply to Andrew Isaacson

If the CPU Options get out of sync with the CONFIG_CPU_ options, cpu_cache_init() can end up being a noop. Stop with a useful message in that case rather than running on without cache functions.

Signed-off-by Andrew Isaacson <[email protected]>
arch/mips/mm/cache.c +2 -0
Index: linux-2.6-work/arch/mips/mm/cache.c
===================================================================
@@ -122,6 +122,8 @@
     defined(CONFIG_CPU_MIPS64) || defined(CONFIG_CPU_TX49XX) || \
     defined(CONFIG_CPU_RM7000) || defined(CONFIG_CPU_RM9000)
 		ld_mmu_r4xx0();
+#else
+    		panic("Unknown CPU with r4k TLB");
 #endif
 	} else switch (current_cpu_data.cputype) {
 #ifdef CONFIG_CPU_R3000

--

#20 lore raw
Quoted 3 lines · Andrew Isaacson

If the CPU Options get out of sync with the CONFIG_CPU_ options, cpu_cache_init() can end up being a noop. Stop with a useful message in that case rather than running on without cache functions.

Wouldn't a build-time error be a better option?

Maciej

#21 lore raw
Quoted 6 lines · Maciej W. Rozycki
Quoted 3 lines · Andrew Isaacson

If the CPU Options get out of sync with the CONFIG_CPU_ options, cpu_cache_init() can end up being a noop. Stop with a useful message in that case rather than running on without cache functions.

Wouldn't a build-time error be a better option?

The code looks like it's structured to be able to be compiled with support for multiple CPUs, say, r4k and SB1; using #error would seem to prevent that.

With the code as currently structured, you don't know it's going to be a noop until runtime comes along and cpu_has_4ktlb is true...

-andy

#22 lore raw
Quoted 6 lines · Andy Isaacson

The code looks like it's structured to be able to be compiled with support for multiple CPUs, say, r4k and SB1; using #error would seem to prevent that.

With the code as currently structured, you don't know it's going to be a noop until runtime comes along and cpu_has_4ktlb is true...

Well, I've had a look at the code and it's such a mess. Obviously
calling ld_mmu_r4xx0() (or any of the other variants) should not be compiled conditionally and more specific cases, i.e. based on PRId values should take precedence. I'll see if I can make it better.

Maciej

#23 lore raw
Quoted 5 lines · Maciej W. Rozycki
Quoted 4 lines · Andy Isaacson

The code looks like it's structured to be able to be compiled with support for multiple CPUs, say, r4k and SB1; using #error would seem to prevent that.

Quoted 7 lines
Quoted 2 lines

With the code as currently structured, you don't know it's going to be a noop until runtime comes along and cpu_has_4ktlb is true...

Well, I've had a look at the code and it's such a mess. Obviously
calling ld_mmu_r4xx0() (or any of the other variants) should not be compiled conditionally and more specific cases, i.e. based on PRId values should take precedence. I'll see if I can make it better.

I certainly won't argue with a cleanup of arch/mips/mm/cache.c, that code has annoyed me from first laying eyes on it...

-andy

#24 lore raw
Quoted 7 lines · Andy Isaacson
Quoted 4 lines

Well, I've had a look at the code and it's such a mess. Obviously
calling ld_mmu_r4xx0() (or any of the other variants) should not be compiled conditionally and more specific cases, i.e. based on PRId values should take precedence. I'll see if I can make it better.

I certainly won't argue with a cleanup of arch/mips/mm/cache.c, that code has annoyed me from first laying eyes on it...

So just did that, cpu_cache_init is looking bearable now.

Ralf

author
#25 lore raw

PATCH 5/5 SiByte fixes for 2.6.12

In reply to Andrew Isaacson

Fix a stale comment in c-sb1.c.

Signed-off-by Andrew Isaacson <[email protected]>
arch/mips/mm/c-sb1.c +1 -1
Index: linux-2.6-work/arch/mips/mm/c-sb1.c
===================================================================
@@ -492,7 +492,7 @@
 }
 
 /*
- * This is called from loadmmu.c.  We have to set up all the
+ * This is called from cache.c.  We have to set up all the
  * memory management function pointers, as well as initialize
  * the caches and tlbs
  */

--

Applied
#26 lore raw
Quoted 1 line · Andrew Isaacson

Fix a stale comment in c-sb1.c.

Applied,

Ralf