Hi Andres,
I've got the problem that Linux kernels with MFGPT clocksource support
won't boot on any of our Geode-LX/CS5536 based boards (4 different
models), and I'd be grateful for your opinion/help. Sorry for the long
mail, but things are a bit complex.
The symptom is that the MFGPT driver registers itself, but is not
producing any timer ticks. Due to its rating, MFGPT soon is selected as
clocksource, and the kernel simply starves on the next occasion. Users
see a hang on boot, no errors nor warnings, but lots of innocent
messages after MFGPT init to get them off track.
Reason: mfgpt is using both wrong IRQ and baseaddress. I'll get to the
IRQ issue first.
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?
Best regards,
Jens Rottmann
On 31/07/08 09:23 +0200, Jens Rottmann wrote:
> 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.)
Either of these solutions is the "right" solution. I prefer automatically
detecting the SIO interrupts and finding a free interrupt.
> 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.
> 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.
> 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.
> 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.
The main problem with this is configuring your second timer wrong, and
it fires on the wrong interrupt. I believe thats why we added this
code in the first place. I wouldn't be opposed to removing it, though,
assuming that we can avoid confusion when trying to use paired GPIOs.
> 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%.
> 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 ...
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. But that said, the point of Linux is to work
around buggy BIOSen. So, lets consider:
The GPIO code has to run before the PCI code for various reasons, so moving
it really isn't an option. Reading the base MSR every time wouldn't affect
performance too badly (its an MSR, not I/O), but that would be ugly.
I guess i don't know much about how PCI manages changes to the resources
through setpci and other mechanisms. This seems like it would break a
lot of drivers. If there is a notifier or something that we can tie into
great, but perhaps this is a case of "user beware". Anybody else concur?
>
> 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?
Yes, please do - the interrupt problem only bites us occasionally, but
when it does, it hurts.
Thanks,
Jordan
--
Jordan Crouse
Systems Software Development Engineer
Advanced Micro Devices, Inc.
On Thu, 31 Jul 2008 08:36:35 -0600
Jordan Crouse <[email protected]> wrote:
> On 31/07/08 09:23 +0200, Jens Rottmann wrote:
> > 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.)
>
> Either of these solutions is the "right" solution. I prefer
> automatically detecting the SIO interrupts and finding a free
> interrupt.
>
Except the broken lp0 would be a problem, no?
> > 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.
>
> > 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.
>
> > 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.
>
At some level, we have to trust the BIOS (especially when its is
filling in MSRs for us). I think I prefer #3. At the same time,
Jordan has a better idea of how BIOSes out there might manage to
screw up the entries in PIC_ZSEL_LOW...
[...]
> > 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%.
Ditto.
On Thu, 31 Jul 2008 08:36:35 -0600
Jordan Crouse <[email protected]> wrote:
> On 31/07/08 09:23 +0200, Jens Rottmann wrote:
[...]
> > 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.
> >
Can you please provide more info on this? Specifically, dmesg output
about which resources conflict would be helpful..
I've implemented all of 4,3,2, see patch at the very end:
> 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.)
Jordan Crouse wrote:
> This goes against our basic policy of not trusting the BIOS.
I'd say distrust, yes, but not ignore it.
If a preset IRQ exists, my implementation prefers this over defaulting
to 7. "mfgpt_irq=nn" overrides both.
> The main problem with this is configuring your second timer wrong, and
> it fires on the wrong interrupt.
Can't happen, I think. Now geode_mfgpt_set_irq() checks if the requested
CMP can be safely used on a timer. If the user of the MFGPT hasn't
called geode_mfgpt_set_irq() for a given CMP, geode_mfgpt_toggle_event()
will not have been called either, so no IRQ is triggered, even if the
user erroneously programs the wrong CMP.
Now about the baseaddr issue:
> 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.
Yes, I know it is. I did't go into detail, because this is the
Linux-kernel mailing list after all, and my mail was long enough
already. Luckily, I have access to our BIOS' sources, that's why I
wrote in my first mail "The baseaddr issue is not that serious for me, I
will probably be able to fix the bogus resource conflict."
Actually there are 2 bogus conflicts:
The IO resources shown in the distinct headers for GPIO, MFGPT and SMB
(00:10.0 to 00:10.2) are identical with BAR0,1,2 in the ISA header
(00:0F.0). Of course they are identical, because that's 2 PCI headers
for the same thing, but Linux cannot know that and sees a collision.
This conflict will go away when the headers 10.0 to 10.2 get hidden, so
I think I'll do just that. No one is using them anyway. I guess I'll try
and create a BIOS setup entry, but make it default to 'disabled'.
But this alone isn't enough.
The second 'conflict' is due to a bug in the VSA code that implements
the reads and writes to the virtual PCI headers. When Linux (or anyone)
writes FFFFFFFF to the BAR to probe the size, reads return wrong values.
Because of this, the IO resource size for SMB is read as 8 KB instead of
8 bytes. And this supposedly bigger sized SMB collides with MFGPT IO
space, which is located 256 bytes past SMB.
Funny enough, as I found out today, responsible for this mess is again
crappy VSA code that was needed to synchronize the mentioned double PCI
headers for GPIO, MFGPT and SMB in 0F.0 and 10.x. :-(
Have a nice weekend.
Jens
_____________________________________________________________________
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]>
---
--- linux-2.6.26/include/asm-x86/geode.h
+++ mfgpt-irq-fix/include/asm-x86/geode.h
@@ -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)
--- linux-2.6.26/arch/x86/kernel/mfgpt_32.c
+++ mfgpt-irq-fix/arch/x86/kernel/mfgpt_32.c
@@ -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)
{
- u32 val, dummy;
- int offset;
+ u32 zsel, lpc, dummy;
+ int shift;
if (timer < 0 || timer >= MFGPT_MAX_TIMERS)
return -EIO;
- if (geode_mfgpt_toggle_event(timer, cmp, MFGPT_EVENT_IRQ, enable))
+ /* IRQ already set to 2 means VSA is using the timer's Siamese twin */
+ 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;
_
On 01/08/08 17:25 +0200, Jens Rottmann wrote:
> 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]>
One comment below - otherwise, I reviewed it and it looks good, but
I haven't tried it.
Jordan
> ---
>
> --- linux-2.6.26/include/asm-x86/geode.h
> +++ mfgpt-irq-fix/include/asm-x86/geode.h
> @@ -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)
> --- linux-2.6.26/arch/x86/kernel/mfgpt_32.c
> +++ mfgpt-irq-fix/arch/x86/kernel/mfgpt_32.c
> @@ -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)
> {
> - u32 val, dummy;
> - int offset;
> + u32 zsel, lpc, dummy;
> + int shift;
>
> if (timer < 0 || timer >= MFGPT_MAX_TIMERS)
> return -EIO;
>
> - if (geode_mfgpt_toggle_event(timer, cmp, MFGPT_EVENT_IRQ, enable))
> + /* IRQ already set to 2 means VSA is using the timer's Siamese twin */
Please make this more descriptive - something like this:
/* If the IRQ is set to 2, then VSA is using the other timer in the pair */
Siamese twin might be confusing.
> + 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.
Signed-off-by: Jens Rottmann <[email protected]>
---
Hi Jordan,
extended (and hopefully clarified) the comment. (However, kept the
expression Siamese twin, because I happen to like it. :-)
Do you think this might qualify as patch for the stable series?? After
all, it fixes a hang ... though only on some boards and one can work
around it with a cmdline param ...
Regards,
Jens
--- linux-2.6.26.1/include/asm-x86/geode.h
+++ mfgpt-irq-fix/include/asm-x86/geode.h
@@ -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)
--- linux-2.6.26/arch/x86/kernel/mfgpt_32.c
+++ mfgpt-irq-fix/arch/x86/kernel/mfgpt_32.c
@@ -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,48 @@
}
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)
{
- u32 val, dummy;
- int offset;
+ u32 zsel, lpc, dummy;
+ int shift;
if (timer < 0 || timer >= MFGPT_MAX_TIMERS)
return -EIO;
- if (geode_mfgpt_toggle_event(timer, cmp, MFGPT_EVENT_IRQ, enable))
+ /*
+ * Unfortunately, MFGPTs come in pairs sharing their IRQ lines. If VSA
+ * is using the same CMP of the timer's Siamese twin, the IRQ is set to
+ * 2, and we mustn't use nor change it.
+ * XXX: Likewise, 2 Linux drivers might clash if the 2nd overwrites the
+ * IRQ of the 1st. This can only happen if forcing an IRQ, calling this
+ * with *irq==0 is safe. Currently there _are_ no 2 drivers.
+ */
+ rdmsr(MSR_PIC_ZSEL_LOW, zsel, dummy);
+ shift = ((cmp == MFGPT_CMP1 ? 0 : 4) + timer % 4) * 4;
+ 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 +263,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 +367,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 +395,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;
_
* Jens Rottmann <[email protected]> wrote:
> 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?
Ingo
On 15/08/08 17:11 +0200, Ingo Molnar wrote:
>
> * Jens Rottmann <[email protected]> wrote:
>
> > 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?
It fixes a real system hang for some folks, so 2.6.27 would be great
if you and Linus agree.
Thanks,
Jordan
--
Jordan Crouse
Systems Software Development Engineer
Advanced Micro Devices, Inc.
* Jordan Crouse <[email protected]> wrote:
> On 15/08/08 17:11 +0200, Ingo Molnar wrote:
> >
> > * Jens Rottmann <[email protected]> wrote:
> >
> > > 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?
>
> It fixes a real system hang for some folks, so 2.6.27 would be great
> if you and Linus agree.
ok - i've put it into tip/x86/urgent as well.
Ingo