MFGPT driver inhibits boot on some boards

The driver defaults to IRQ7. Our boards use this for the parallel port. This alone would be ok, but the parallel port is on a LPC SIO, so IRQ7 is routed to the LPC bus in MSR 51400025, so MFGPT IRQs are not received.

Possible solution 1: make geode_mfgpt_set_irq() clear the needed bit in MSR 51400025. This would break lp0, but at least Linux would boot and users could cat /proc/interrupts and see what's going on.

Possible solution 2: make geode_mfgpt_set_irq() check the bit and fail if the IRQ is routed to LPC. (I think I'd prefer this over 1.)

But that's not clean, even an IRQ not routed to LPC might be the wrong one. The BIOS we're using (Insyde BIOS, uses VSA and 5 MFGPTs, but leaves 3 timers available) exports a PCI header for MFGPT, which of course gets an IRQ assigned (LNKB-->IRQ11), and this is the right IRQ to use. Some autodetection instead of hardcoding IRQ7 would be perfect. But looking for the PCI header won't work, because AMD removed them from the spec, most BIOSes don't support them (any more) and some day our BIOS will hide them, too.

I guess MSR 51400022 aka MSR_PIC_ZSEL_LOW is the key here. geode_mfgpt_set_irq() overwrites this unconditionally, which I think is bad. If the BIOS has already set an IRQ here, the driver should keep it and assume it to be ok.

Possible solution 3: keep the IRQ already chosen in MSR_PIC_ZSEL_LOW unless it's 0, only in that case use IRQ7

But how about CMP1 and CMP2? geode_mfgpt_set_irq() currently sets IRQs for both, but I think it's better to only read/set the IRQ for the requested CMP, because they might differ.

In fact, they do differ, because pairs of MFGPTs always share an IRQ (stupid idea, that): E.g. VSA sets CMP1 of MFGPT7 to IRQ2, so its twin MFGPT3, which itself is available, has its CMP1 also set to IRQ2. But the clocksource driver only requests CMP2, so MFGPT3 could still be used.

Possible solution 4: only touch the CMP actually requested. But also fail if this CMP is set to IRQ2, because this means VSA is using its twin.

IMHO hardcoding IRQ7 is bad. Yes, you can override it, but this isn't some non-critical device driver like audio where only this one device won't work. And MFGPT can't be compiled as module, so initramfs can't do anything about it.
I think, if there is any chance to do it, Linux should be able to boot without any parameters given - even if with reduced functionality.

Now about the baseaddress issue:

The problem is that Linux upon PCI scan detects a resource conflict (which in fact is none, but Linux cannot know that) and moves the resources (MFGPT IO space among them) to some other place.

But init_lbars() in geode_32.c has saved all base addresses in an array _before_ the move, and geode_get_dev_base() and geode_mfgpt_write() keep using these addresses _after_ the move.

Possible solutions 5-7:
* drop the array and always read the baseaddr from the MSR - but this would impact performance quite a lot, I guess
* run init_lbars() only after the PCI scan - but they might be needed before that, and what if someone changes the base again, say with setpci?
* update the array every time the base is changed - but I have no idea how to do that ...

Anyway, the baseaddr issue is not that serious for me, I will probably be able to fix the bogus resource conflict that triggers the baseaddr move, and this issue will disappear. But still - mfgpt will still crash again if someone moves the resource for any reason ...

Well, that's what I've figured out so far. (Thanks if you're still
reading on!) :-) I might be able to implement some fixes for the IRQ
issue, but I'd like your opinion first. What do you think?

Quoted 9 lines · Jens Rottmann

The driver defaults to IRQ7. Our boards use this for the parallel port. This alone would be ok, but the parallel port is on a LPC SIO, so IRQ7 is routed to the LPC bus in MSR 51400025, so MFGPT IRQs are not received.

Possible solution 1: make geode_mfgpt_set_irq() clear the needed bit in MSR 51400025. This would break lp0, but at least Linux would boot and users could cat /proc/interrupts and see what's going on.

Quoted 2 lines

Possible solution 2: make geode_mfgpt_set_irq() check the bit and fail if the IRQ is routed to LPC. (I think I'd prefer this over 1.)

Either of these solutions is the "right" solution. I prefer automatically detecting the SIO interrupts and finding a free interrupt.

Quoted 9 lines

But that's not clean, even an IRQ not routed to LPC might be the wrong one. The BIOS we're using (Insyde BIOS, uses VSA and 5 MFGPTs, but leaves 3 timers available) exports a PCI header for MFGPT, which of course gets an IRQ assigned (LNKB-->IRQ11), and this is the right IRQ to use. Some autodetection instead of hardcoding IRQ7 would be perfect. But looking for the PCI header won't work, because AMD removed them from the spec, most BIOSes don't support them (any more) and some day our BIOS will hide them, too.

I know the platform you are talking about - that was a special test case the MFGPT header. It would have become standard, but then circumstances intervened, and we lost the resources to push that into the VSA and to all of our BIOS vendors. It is easiest for us to assume that we won't have the PCI header for the MFGPT.

Quoted 4 lines

I guess MSR 51400022 aka MSR_PIC_ZSEL_LOW is the key here. geode_mfgpt_set_irq() overwrites this unconditionally, which I think is bad. If the BIOS has already set an IRQ here, the driver should keep it and assume it to be ok.

Agreed.

Quoted 2 lines

Possible solution 3: keep the IRQ already chosen in MSR_PIC_ZSEL_LOW unless it's 0, only in that case use IRQ7

This goes against our basic policy of not trusting the BIOS.

Quoted 7 lines

IMHO hardcoding IRQ7 is bad. Yes, you can override it, but this isn't some non-critical device driver like audio where only this one device won't work. And MFGPT can't be compiled as module, so initramfs can't do anything about it.
I think, if there is any chance to do it, Linux should be able to boot without any parameters given - even if with reduced functionality.

I agree 100%.

On 31/07/08 09:23 +0200, Jens Rottmann wrote:

Quoted 7 lines

Now about the baseaddress issue:

The problem is that Linux upon PCI scan detects a resource conflict (which in fact is none, but Linux cannot know that) and moves the resources (MFGPT IO space among them) to some other place.

Possible solution 4: only touch the CMP actually requested. But also fail if this CMP is set to IRQ2, because this means VSA is using its twin.

Possible solution 3: keep the IRQ already chosen in MSR_PIC_ZSEL_LOW unless it's 0, only in that case use IRQ7

Possible solution 2: make geode_mfgpt_set_irq() check the bit and fail if the IRQ is routed to LPC. (I think I'd prefer this over 1.)

This goes against our basic policy of not trusting the BIOS.

The main problem with this is configuring your second timer wrong, and it fires on the wrong interrupt.

This does sound like a problem with your particular system - Linux can move the resource space, but if it has to, then I generally consider that to be a BIOS issue.

mfgpt: check IRQ before using MFGPT as clocksource

Adds a simple IRQ autodetection to the AMD Geode MFGPT driver, and more importantly, adds some checks, if IRQs can actually be received on the chosen line. This fixes cases where MFGPT is selected as clocksource though not producing any ticks, so the kernel simply starves during boot.

Signed-off-by Jens Rottmann <[email protected]>

---

include/asm-x86/geode.h +2 -1

@@ -50,6 +50,7 @@
 #define MSR_PIC_YSEL_HIGH	0x51400021
 #define MSR_PIC_ZSEL_LOW	0x51400022
 #define MSR_PIC_ZSEL_HIGH	0x51400023
+#define MSR_PIC_IRQM_LPC	0x51400025
 
 #define MSR_MFGPT_IRQ		0x51400028
 #define MSR_MFGPT_NR		0x51400029

@@ -237,7 +238,7 @@
 }
 
 extern int geode_mfgpt_toggle_event(int timer, int cmp, int event, int enable);
-extern int geode_mfgpt_set_irq(int timer, int cmp, int irq, int enable);
+extern int geode_mfgpt_set_irq(int timer, int cmp, int *irq, int enable);
 extern int geode_mfgpt_alloc_timer(int timer, int domain);
 
 #define geode_mfgpt_setup_irq(t, c, i) geode_mfgpt_set_irq((t), (c), (i), 1)

arch/x86/kernel/mfgpt_32.c +3 -1

@@ -33,6 +33,8 @@
 #include <linux/module.h>
 #include <asm/geode.h>
 
+#define MFGPT_DEFAULT_IRQ	7
+
 static struct mfgpt_timer_t {
 	unsigned int avail:1;
 } mfgpt_timers[MFGPT_MAX_TIMERS];

@@ -157,29 +159,41 @@
 }
 EXPORT_SYMBOL_GPL(geode_mfgpt_toggle_event);
 
-int geode_mfgpt_set_irq(int timer, int cmp, int irq, int enable)
+int geode_mfgpt_set_irq(int timer, int cmp, int *irq, int enable)
 {

arch/x86/kernel/mfgpt_32.c +24 -11

+	shift = ((cmp == MFGPT_CMP1 ? 0 : 4) + timer % 4) * 4;
+	rdmsr(MSR_PIC_ZSEL_LOW, zsel, dummy);
+	if (((zsel >> shift) & 0xF) == 2)
 		return -EIO;
 
-	rdmsr(MSR_PIC_ZSEL_LOW, val, dummy);
-
-	offset = (timer % 4) * 4;
+	/* Choose IRQ: if none supplied, keep IRQ already set or use default */
+	if (!*irq)
+		*irq = (zsel >> shift) & 0xF;
+	if (!*irq)
+		*irq = MFGPT_DEFAULT_IRQ;
 
-	val &= ~((0xF << offset) | (0xF << (offset + 16)));
+	/* Can't use IRQ if it's 0 (=disabled), 2, or routed to LPC */
+	if (*irq < 1 || *irq == 2 || *irq > 15)
+		return -EIO;
+	rdmsr(MSR_PIC_IRQM_LPC, lpc, dummy);
+	if (lpc & (1 << *irq))
+		return -EIO;
 
+	/* All chosen and checked - go for it */
+	if (geode_mfgpt_toggle_event(timer, cmp, MFGPT_EVENT_IRQ, enable))
+		return -EIO;
 	if (enable) {
-		val |= (irq & 0x0F) << (offset);
-		val |= (irq & 0x0F) << (offset + 16);
+		zsel = (zsel & ~(0xF << shift)) | (*irq << shift);
+		wrmsr(MSR_PIC_ZSEL_LOW, zsel, dummy);
 	}
 
-	wrmsr(MSR_PIC_ZSEL_LOW, val, dummy);
 	return 0;
 }
 

@@ -242,7 +256,7 @@
 static unsigned int mfgpt_tick_mode = CLOCK_EVT_MODE_SHUTDOWN;
 static u16 mfgpt_event_clock;
 
-static int irq = 7;
+static int irq;
 static int __init mfgpt_setup(char *str)
 {
 	get_option(&str, &irq);

@@ -346,7 +360,7 @@ int __init mfgpt_timer_setup(void)
 	mfgpt_event_clock = timer;
 
 	/* Set up the IRQ on the MFGPT side */
-	if (geode_mfgpt_setup_irq(mfgpt_event_clock, MFGPT_CMP2, irq)) {
+	if (geode_mfgpt_setup_irq(mfgpt_event_clock, MFGPT_CMP2, &irq)) {
 		printk(KERN_ERR "mfgpt-timer:  Could not set up IRQ %d\n", irq);
 		return -EIO;
 	}

@@ -374,13 +388,14 @@ int __init mfgpt_timer_setup(void)
 			&mfgpt_clockevent);
 
 	printk(KERN_INFO
-	       "mfgpt-timer:  registering the MFGPT timer as a clock event.\n");
+	       "mfgpt-timer:  Registering MFGPT timer %d as a clock event, using IRQ %d\n",
+	       timer, irq);
 	clockevents_register_device(&mfgpt_clockevent);
 
 	return 0;
 
 err:
-	geode_mfgpt_release_irq(mfgpt_event_clock, MFGPT_CMP2, irq);
+	geode_mfgpt_release_irq(mfgpt_event_clock, MFGPT_CMP2, &irq);
 	printk(KERN_ERR
 	       "mfgpt-timer:  Unable to set up the MFGPT clock source\n");
 	return -EIO;

_

Adds a simple IRQ autodetection to the AMD Geode MFGPT driver, and more importantly, adds some checks, if IRQs can actually be received on the chosen line. This fixes cases where MFGPT is selected as clocksource though not producing any ticks, so the kernel simply starves during boot.

Quoted 5 lines · Jens Rottmann

Adds a simple IRQ autodetection to the AMD Geode MFGPT driver, and more importantly, adds some checks, if IRQs can actually be received on the chosen line. This fixes cases where MFGPT is selected as clocksource though not producing any ticks, so the kernel simply starves during boot.

i've applied it to tip/x86/geode. Should it go into v2.6.27 via x86/urgent, or should wait for v2.6.28?

Quoted 9 lines · Ingo Molnar

Quoted 5 lines · Jens Rottmann

Adds a simple IRQ autodetection to the AMD Geode MFGPT driver, and more importantly, adds some checks, if IRQs can actually be received on the chosen line. This fixes cases where MFGPT is selected as clocksource though not producing any ticks, so the kernel simply starves during boot.

Quoted 2 lines

i've applied it to tip/x86/geode. Should it go into v2.6.27 via x86/urgent, or should wait for v2.6.28?

It fixes a real system hang for some folks, so 2.6.27 would be great if you and Linus agree.