LinuxLists.cc - 2.6.24-rc8 hangs at mfgpt-timer

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

On Wed, 16 Jan 2008 18:44:07 +0100
Arnd Hannemann <[email protected]> wrote:

> Hi,
>
> I'm trying to boot 2.6.24-rc8 on a GEODE LX board (ALIX.3),
> and it hangs during boot:
>
> [ 12.689971] NET: Registered protocol family 16
> [ 12.703329] geode-mfgpt: Registered timer 0
> [ 12.716149] mfgpt-timer: registering the MFGT timer as a clock event...
>

What BIOS are you using? It's possible that our detection code is
failing to detect in-use timers.

2008-01-16 21:56:06

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

On Wed, 16 Jan 2008 16:19:12 -0500
Andres Salomon <[email protected]> wrote:

> On Wed, 16 Jan 2008 18:44:07 +0100
> Arnd Hannemann <[email protected]> wrote:
>
> > Hi,
> >
> > I'm trying to boot 2.6.24-rc8 on a GEODE LX board (ALIX.3),
> > and it hangs during boot:
> >
> > [ 12.689971] NET: Registered protocol family 16
> > [ 12.703329] geode-mfgpt: Registered timer 0
> > [ 12.716149] mfgpt-timer: registering the MFGT timer as a clock event...
> >
>
>
> What BIOS are you using? It's possible that our detection code is
> failing to detect in-use timers.

Also, could you provide a log with the following (untested) patch? I'm
curious how many MFGPTs we're actually detecting as being available, and
the existing code is backwards.

>From 0ea825fd564056ac653507517beb5cea9034e464 Mon Sep 17 00:00:00 2001
From: Andres Salomon <[email protected]>
Date: Wed, 16 Jan 2008 16:53:16 -0500
Subject: [PATCH] x86: geode: fix up ordering of messages during MFGPT detection

We're printing out informational messages in the wrong order while
probing for MFGPTs. This changes the order so that we first display
the number of available timers that were detected before displaying
messages about how we actually are using those timers.

Signed-off-by: Andres Salomon <[email protected]>
---
arch/x86/kernel/geode_32.c | 5 +----
arch/x86/kernel/mfgpt_32.c | 7 +++----
include/asm-x86/geode.h | 2 +-
3 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/geode_32.c b/arch/x86/kernel/geode_32.c
index f12d8c5..00b45ae 100644
--- a/arch/x86/kernel/geode_32.c
+++ b/arch/x86/kernel/geode_32.c
@@ -145,14 +145,11 @@ EXPORT_SYMBOL_GPL(geode_gpio_setup_event);

static int __init geode_southbridge_init(void)
{
- int timers;
-
if (!is_geode())
return -ENODEV;

init_lbars();
- timers = geode_mfgpt_detect();
- printk(KERN_INFO "geode: %d MFGPT timers available.\n", timers);
+ geode_mfgpt_detect();
return 0;
}

diff --git a/arch/x86/kernel/mfgpt_32.c b/arch/x86/kernel/mfgpt_32.c
index 0ab680f..d11cba3 100644
--- a/arch/x86/kernel/mfgpt_32.c
+++ b/arch/x86/kernel/mfgpt_32.c
@@ -70,14 +70,14 @@ __setup("nomfgpt", mfgpt_disable);
* In other cases (such as with VSAless OpenFirmware), the system firmware
* leaves timers available for us to use.
*/
-int __init geode_mfgpt_detect(void)
+void __init geode_mfgpt_detect(void)
{
int count = 0, i;
u16 val;

if (disable) {
printk(KERN_INFO "geode-mfgpt: Skipping MFGPT setup\n");
- return 0;
+ return;
}

for (i = 0; i < MFGPT_MAX_TIMERS; i++) {
@@ -87,11 +87,10 @@ int __init geode_mfgpt_detect(void)
count++;
}
}
+ printk(KERN_INFO "geode-mfgpt: %d MFGPT timers available.\n", count);

/* set up clock event device, if desired */
i = mfgpt_timer_setup();
-
- return count;
}

int geode_mfgpt_toggle_event(int timer, int cmp, int event, int enable)
diff --git a/include/asm-x86/geode.h b/include/asm-x86/geode.h
index 771af33..1cb22cf 100644
--- a/include/asm-x86/geode.h
+++ b/include/asm-x86/geode.h
@@ -200,7 +200,7 @@ static inline u16 geode_mfgpt_read(int timer, u16 reg)
return inw(base + reg + (timer * 8));
}

-extern int __init geode_mfgpt_detect(void);
+extern void __init geode_mfgpt_detect(void);
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_alloc_timer(int timer, int domain, struct module *owner);
--
1.5.3.5

2008-01-17 09:54:13

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

Andres Salomon schrieb:
> On Wed, 16 Jan 2008 16:19:12 -0500
> Andres Salomon <[email protected]> wrote:
>
>> On Wed, 16 Jan 2008 18:44:07 +0100
>> Arnd Hannemann <[email protected]> wrote:
>>
>>> Hi,
>>>
>>> I'm trying to boot 2.6.24-rc8 on a GEODE LX board (ALIX.3),
>>> and it hangs during boot:
>>>
>>> [ 12.689971] NET: Registered protocol family 16
>>> [ 12.703329] geode-mfgpt: Registered timer 0
>>> [ 12.716149] mfgpt-timer: registering the MFGT timer as a clock event...
>>>
>>
>> What BIOS are you using? It's possible that our detection code is
>> failing to detect in-use timers.

I'm using v0.99 (latest available).
Also note when I do enable the mysterios "MFGPT workaround" option in
the bios the machine hangs directly after:
[ 36.780990] NET: Registered protocol family 16

>
>
> Also, could you provide a log with the following (untested) patch? I'm
> curious how many MFGPTs we're actually detecting as being available, and
> the existing code is backwards.

The relevant part would be:
[ 23.092507] NET: Registered protocol family 16
[ 23.105875] geode-mfgpt: 8 MFGPT timers available
[ 23.120247] geode-mfgpt: Registered timer 0
[ 23.133076] mfgpt-timer: registering the MFGT timer as a clock event.

Best regards,
Arnd Hannemann

2008-01-17 18:40:31

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

On Thu, 17 Jan 2008 10:54:30 +0100
Arnd Hannemann <[email protected]> wrote:

> Andres Salomon schrieb:
> > On Wed, 16 Jan 2008 16:19:12 -0500
> > Andres Salomon <[email protected]> wrote:
> >
> >> On Wed, 16 Jan 2008 18:44:07 +0100
> >> Arnd Hannemann <[email protected]> wrote:
> >>
> >>> Hi,
> >>>
> >>> I'm trying to boot 2.6.24-rc8 on a GEODE LX board (ALIX.3),
> >>> and it hangs during boot:
> >>>
> >>> [ 12.689971] NET: Registered protocol family 16
> >>> [ 12.703329] geode-mfgpt: Registered timer 0
> >>> [ 12.716149] mfgpt-timer: registering the MFGT timer as a clock event...
> >>>
> >>
> >> What BIOS are you using? It's possible that our detection code is
> >> failing to detect in-use timers.
>
> I'm using v0.99 (latest available).

v0.99 of what? Jordan seems to think it's an Award BIOS, but I'd like
to make sure.

> Also note when I do enable the mysterios "MFGPT workaround" option in
> the bios the machine hangs directly after:
> [ 36.780990] NET: Registered protocol family 16

"MFGPT workaround"? That sounds a bit frightening.

Presumably, the BIOS is using the MFGPTs, but we're not detecting them as
being in use.

I'm assuming that booting with 'nomfgpt' works for you?

>
> >
> >
> > Also, could you provide a log with the following (untested) patch? I'm
> > curious how many MFGPTs we're actually detecting as being available, and
> > the existing code is backwards.
>
> The relevant part would be:
> [ 23.092507] NET: Registered protocol family 16
> [ 23.105875] geode-mfgpt: 8 MFGPT timers available
> [ 23.120247] geode-mfgpt: Registered timer 0
> [ 23.133076] mfgpt-timer: registering the MFGT timer as a clock event.
>
> Best regards,
> Arnd Hannemann
>

2008-01-17 19:53:31

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

diff -uNr linux-2.6.16.56/arch/i386/kernel/geode-mfgpt.c linux-2.6.16.56.patched/arch/i386/kernel/geode-mfgpt.c
--- linux-2.6.16.56/arch/i386/kernel/geode-mfgpt.c 1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.16.56.patched/arch/i386/kernel/geode-mfgpt.c 2007-12-11 14:57:36.000000000 +0100
@@ -0,0 +1,312 @@
+/* Driver/API for AMD Geode Multi-Function General Purpose Timers (MFGPT)
+ *
+ * Copyright (C) 2006, Advanced Micro Devices, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/pci.h>
+#include <linux/pci_ids.h>
+#include <linux/interrupt.h>
+#include <linux/cdev.h>
+#include <asm/geode-mfgpt.h>
+#include <asm/io.h>
+#include <asm/msr.h>
+
+#define WORKAROUND
+
+#define MFGPT_IRQ_MSR 0x51400028
+#define MFGPT_NR_MSR 0x51400029
+
+#define MFGPT_MAX_TIMERS 7
+#define MFGPT_PCI_BAR 2
+
+#define F_AVAIL 0x01
+#define F_RESERVED 0x02
+
+static void *mfgpt_iobase;
+static int reserved_mask = 0;
+static struct class *mfgpt_class;
+
+static struct mfgpt_timer_t {
+ int index;
+ int flags;
+ struct module *owner;
+ struct class_device *cdev;
+} mfgpt_timers[MFGPT_MAX_TIMERS];
+
+void
+geode_mfgpt_write(int i, u16 r, u16 v)
+{
+ iowrite16(v, mfgpt_iobase + (r + (i * 8)));
+}
+
+EXPORT_SYMBOL(geode_mfgpt_write);
+
+u16
+geode_mfgpt_read(int i, u16 r)
+{
+ return ioread16(mfgpt_iobase + (r + (i * 8)));
+}
+
+EXPORT_SYMBOL(geode_mfgpt_read);
+
+static ssize_t
+sys_print_register(struct class_device *dev, char *buf, int reg)
+{
+ struct mfgpt_timer_t *timer = class_get_devdata(dev);
+ u16 val = geode_mfgpt_read(timer->index, reg);
+
+ return sprintf(buf, "%4.4X\n", val);
+}
+
+static ssize_t
+sys_show_setup(struct class_device *dev, char *buf)
+{
+ return sys_print_register(dev, buf, MFGPT_REG_SETUP);
+}
+
+static ssize_t
+sys_show_counter(struct class_device *dev, char *buf)
+{
+ return sys_print_register(dev, buf, MFGPT_REG_COUNTER);
+}
+
+static ssize_t
+sys_show_cmp1(struct class_device *dev, char *buf)
+{
+ return sys_print_register(dev, buf, MFGPT_REG_CMP1);
+}
+
+static ssize_t
+sys_show_cmp2(struct class_device *dev, char *buf)
+{
+ return sys_print_register(dev, buf, MFGPT_REG_CMP2);
+}
+
+static struct class_device_attribute mfgpt_attrs[] = {
+ __ATTR(setup, S_IRUGO, sys_show_setup, NULL),
+ __ATTR(counter, S_IRUGO, sys_show_counter, NULL),
+ __ATTR(cmp1, S_IRUGO, sys_show_cmp1, NULL),
+ __ATTR(cmp2, S_IRUGO, sys_show_cmp2, NULL),
+};
+
+void
+geode_mfgpt_toggle_event(int timer, int cmp, int event, int setup)
+{
+ u32 msr, mask, value, dummy;
+ int shift = (cmp == MFGPT_CMP1) ? 0 : 8;
+
+ switch(event) {
+ case MFGPT_EVENT_RESET:
+ msr = MFGPT_NR_MSR;
+ mask = 1 << (timer + 24);
+ break;
+
+ case MFGPT_EVENT_NMI:
+ msr = MFGPT_NR_MSR;
+ mask = 1 << (timer + shift);
+ break;
+
+ case MFGPT_EVENT_IRQ:
+ msr = MFGPT_IRQ_MSR;
+ mask = 1 << (timer + shift);
+ break;
+
+ default:
+ return;
+ }
+
+ rdmsr(msr, value, dummy);
+
+ if (setup)
+ value |= mask;
+ else
+ value &= ~mask;
+
+ wrmsr(msr, value, dummy);
+}
+
+EXPORT_SYMBOL(geode_mfgpt_toggle_event);
+
+void
+geode_mfgpt_set_irq(int timer, int cmp, int irq, int setup)
+{
+ u32 val, dummy;
+ int offset;
+
+ geode_mfgpt_toggle_event(timer, cmp, MFGPT_EVENT_IRQ, setup);
+
+ rdmsr(0x51400022, val, dummy);
+
+ offset = (timer % 4) * 4;
+
+ val &= ~((0xF << offset) | (0xF << (offset + 16)));
+
+ if (setup) {
+ val |= (irq & 0x0F) << (offset);
+ val |= (irq & 0x0F) << (offset + 16);
+ }
+
+ wrmsr(0x51400022, val, dummy);
+}
+
+int
+geode_mfgpt_alloc_timer(int timer, int domain, struct module *owner)
+{
+ int i;
+
+ /* If they requested a specific timer, try to honor that */
+ if (mfgpt_iobase == NULL)
+ return -ENODEV;
+
+ if (timer != MFGPT_TIMER_ANY) {
+ if (mfgpt_timers[timer].flags & F_AVAIL) {
+ mfgpt_timers[timer].flags &= ~F_AVAIL;
+ mfgpt_timers[timer].owner = owner;
+
+ printk("geode-mfgpt: Registered timer %d\n", timer);
+ return timer;
+ }
+ else if (mfgpt_timers[timer].owner == owner)
+ return timer;
+
+ /* Request failed - somebody else owns it */
+ return -1;
+ }
+
+ /* Try to find an available timer */
+
+ for(i = 0; i < MFGPT_MAX_TIMERS; i++) {
+
+ if ((mfgpt_timers[i].flags & F_AVAIL) &&
+ !(mfgpt_timers[i].flags & F_RESERVED)) {
+ mfgpt_timers[i].flags &= ~F_AVAIL;
+ mfgpt_timers[i].owner = owner;
+
+ printk("geode-mfgpt: Registered timer %d\n", i);
+ return i;
+ }
+
+ if (i == 5 && domain == MFGPT_DOMAIN_WORKING)
+ break;
+ }
+
+ /* No timers available - too bad */
+ return -1;
+}
+
+EXPORT_SYMBOL(geode_mfgpt_alloc_timer);
+
+static int
+mfgpt_setup_timer(struct pci_dev *pdev, int timer)
+{
+ dev_t devid = MKDEV(0, 0);
+ u16 val = geode_mfgpt_read(timer, MFGPT_REG_SETUP);
+ mfgpt_timers[timer].index = timer;
+
+ if (reserved_mask & (1 << timer))
+ mfgpt_timers[timer].flags |= F_RESERVED;
+
+ if (!(val & MFGPT_SETUP_SETUP)) {
+ int v;
+
+ mfgpt_timers[timer].flags = F_AVAIL;
+
+ /* Add the class device */
+ mfgpt_timers[timer].cdev =
+ class_device_create(mfgpt_class, NULL, devid,
+ &pdev->dev, "mfgpt%d", timer);
+
+ class_set_devdata(mfgpt_timers[timer].cdev, &mfgpt_timers[timer]);
+
+ for(v = 0; v < ARRAY_SIZE(mfgpt_attrs); v++)
+ if (class_device_create_file(mfgpt_timers[timer].cdev,
+ &mfgpt_attrs[v]))
+ printk(KERN_ERR "geode-mfpgt: Couldn't create %s\n",
+ mfgpt_attrs[v].attr.name);
+
+ return 1;
+ }
+
+ return 0;
+}
+
+static struct pci_device_id geode_sbdevs[] = {
+ { PCI_DEVICE(PCI_VENDOR_ID_NS, PCI_DEVICE_ID_NS_CS5535_ISA) },
+ { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_CS5536_ISA) }
+};
+
+static int __init
+geode_mfgpt_init(void)
+{
+ struct pci_dev *pdev = NULL;
+ int i, ret, dev, count = 0;
+
+#ifdef WORKAROUND
+ u32 val, dummy;
+
+ /* The following udocumented bit resets the MFGPT timers */
+
+ val = 0xFF;
+ wrmsr(0x5140002B, val, dummy);
+#endif
+
+ for (dev = 0; dev < ARRAY_SIZE(geode_sbdevs); dev++) {
+ pdev = pci_get_device(geode_sbdevs[dev].vendor,
+ geode_sbdevs[dev].device, NULL);
+
+ if (pdev != NULL)
+ break;
+ }
+
+ if (pdev == NULL) {
+ printk(KERN_ERR "geode-mfgpt: No PCI devices found\n");
+ goto err;
+ }
+
+ if ((ret = pci_enable_device_bars(pdev, 1 << MFGPT_PCI_BAR)))
+ goto err;
+
+ if ((ret = pci_request_region(pdev, MFGPT_PCI_BAR, "geode-mfgpt")))
+ goto err;
+
+ mfgpt_iobase = pci_iomap(pdev, MFGPT_PCI_BAR, 64);
+
+ if (mfgpt_iobase == NULL)
+ goto ereq;
+
+ mfgpt_class = class_create(THIS_MODULE, "mfgpt");
+
+ if (IS_ERR(mfgpt_class)) {
+ printk(KERN_ERR "geode-mfgpt: Unable to allocate the class.\n");
+ goto eiobase;
+ }
+
+ for(i = 0; i < MFGPT_MAX_TIMERS; i++)
+ count += mfgpt_setup_timer(pdev, i);
+
+ printk("geode-mfgpt: %d timers available.\n", count);
+ return 0;
+
+ eiobase:
+ pci_iounmap(pdev, mfgpt_iobase);
+ mfgpt_iobase = NULL;
+
+ ereq:
+ pci_release_region(pdev, MFGPT_PCI_BAR);
+
+ err:
+ printk("geode-mfgpt: Error initalizing the timers\n");
+ return -1;
+}
+
+device_initcall(geode_mfgpt_init);
+MODULE_AUTHOR("Advanced Micro Devices, Inc");
+MODULE_DESCRIPTION("Geode GX/LX MFGPT Driver");
+MODULE_LICENSE("GPL");
diff -uNr linux-2.6.16.56/arch/i386/kernel/Makefile linux-2.6.16.56.patched/arch/i386/kernel/Makefile
--- linux-2.6.16.56/arch/i386/kernel/Makefile 2007-11-01 03:23:29.000000000 +0100
+++ linux-2.6.16.56.patched/arch/i386/kernel/Makefile 2007-12-12 15:18:48.000000000 +0100
@@ -37,6 +37,7 @@
obj-$(CONFIG_DOUBLEFAULT) += doublefault.o
obj-$(CONFIG_VM86) += vm86.o
obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
+obj-m += geode-mfgpt.o

EXTRA_AFLAGS := -traditional

diff -uNr linux-2.6.16.56/drivers/char/watchdog/geodewdt.c linux-2.6.16.56.patched/drivers/char/watchdog/geodewdt.c
--- linux-2.6.16.56/drivers/char/watchdog/geodewdt.c 1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.16.56.patched/drivers/char/watchdog/geodewdt.c 2007-12-12 15:22:31.000000000 +0100
@@ -0,0 +1,247 @@
+/* Watchdog timer for the Geode GX/LX
+ *
+ * Copyright (C) 2006, Advanced Micro Devices, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/miscdevice.h>
+#include <linux/watchdog.h>
+#include <linux/fs.h>
+#include <linux/notifier.h>
+#include <linux/reboot.h>
+
+#include <asm/uaccess.h>
+#include <asm/geode-mfgpt.h>
+
+#define GEODEWDT_HZ 500
+#define GEODEWDT_SCALE 6
+#define GEODEWDT_MAX_SECONDS 131
+
+#define WDT_FLAGS_OPEN 1
+#define WDT_FLAGS_ORPHAN 2
+
+/* The defaults for the other timers are 60, so we'll use that too */
+
+static int cur_interval = 60;
+static int wdt_timer;
+static unsigned long wdt_flags;
+static int safe_close;
+
+static void geodewdt_ping(void)
+{
+ printk("PING\n");
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_SETUP, 0);
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_COUNTER, 0);
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_SETUP, MFGPT_SETUP_CNTEN);
+}
+
+static void geodewdt_stop(void)
+{
+ printk("STOP\n");
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_SETUP, 0);
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_COUNTER, 0);
+}
+
+static int geodewdt_set_heartbeat(int val)
+{
+ if (val < 1 || val > GEODEWDT_MAX_SECONDS)
+ return -EINVAL;
+
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_SETUP, 0);
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_CMP2, val * GEODEWDT_HZ);
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_COUNTER, 0);
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_SETUP, MFGPT_SETUP_CNTEN);
+
+ printk("HEARTBEAT %d\n", val);
+ cur_interval = val;
+ return 0;
+}
+
+static int
+geodewdt_open(struct inode *inode, struct file *file)
+{
+ if (test_and_set_bit(WDT_FLAGS_OPEN, &wdt_flags))
+ return -EBUSY;
+
+ if (!test_and_clear_bit(WDT_FLAGS_ORPHAN, &wdt_flags))
+ __module_get(THIS_MODULE);
+
+ geodewdt_ping();
+ return nonseekable_open(inode, file);
+}
+
+static int
+geodewdt_release(struct inode *inode, struct file *file)
+{
+ if (safe_close) {
+ geodewdt_stop();
+ module_put(THIS_MODULE);
+ }
+ else {
+ printk(KERN_CRIT "Unexpected close - watchdog is not stopping.\n");
+ geodewdt_ping();
+
+ set_bit(WDT_FLAGS_ORPHAN, &wdt_flags);
+ }
+
+ clear_bit(WDT_FLAGS_OPEN, &wdt_flags);
+ safe_close = 0;
+ return 0;
+}
+
+static ssize_t
+geodewdt_write(struct file *file, const char __user *data, size_t len,
+ loff_t *ppos)
+{
+ if(len) {
+ size_t i;
+ safe_close = 0;
+
+ for (i = 0; i != len; i++) {
+ char c;
+
+ if (get_user(c, data + i))
+ return -EFAULT;
+
+ if (c == 'V')
+ safe_close = 1;
+ }
+ }
+
+ geodewdt_ping();
+ return len;
+}
+
+static int
+geodewdt_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ void __user *argp = (void __user *)arg;
+ int __user *p = argp;
+ int interval;
+
+ static struct watchdog_info ident = {
+ .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING
+ | WDIOF_MAGICCLOSE,
+ .firmware_version = 0,
+ .identity = "Geode Watchdog",
+ };
+
+ switch(cmd) {
+ case WDIOC_GETSUPPORT:
+ return copy_to_user(argp, &ident,
+ sizeof(ident)) ? -EFAULT : 0;
+ break;
+
+ case WDIOC_GETSTATUS:
+ case WDIOC_GETBOOTSTATUS:
+ return put_user(0, p);
+
+ case WDIOC_KEEPALIVE:
+ geodewdt_ping();
+ return 0;
+
+ case WDIOC_SETTIMEOUT:
+ if (get_user(interval, p))
+ return -EFAULT;
+
+ if (geodewdt_set_heartbeat(interval))
+ return -EINVAL;
+
+/* Fall through */
+
+ case WDIOC_GETTIMEOUT:
+ return put_user(cur_interval, p);
+ }
+
+ return -ENOTTY;
+}
+
+static int geodewdt_notify_sys(struct notifier_block *this,
+ unsigned long code, void *unused)
+{
+ if(code==SYS_DOWN || code==SYS_HALT)
+ geodewdt_stop();
+
+ return NOTIFY_DONE;
+}
+
+static const struct file_operations geodewdt_fops = {
+ .owner = THIS_MODULE,
+ .llseek = no_llseek,
+ .write = geodewdt_write,
+ .ioctl = geodewdt_ioctl,
+ .open = geodewdt_open,
+ .release = geodewdt_release,
+};
+
+static struct miscdevice geodewdt_miscdev = {
+ .minor = WATCHDOG_MINOR,
+ .name = "geode-watchdog",
+ .fops = &geodewdt_fops
+};
+
+static struct notifier_block geodewdt_notifier = {
+ .notifier_call = geodewdt_notify_sys
+};
+
+static int __init geodewdt_init(void)
+{
+ int ret, timer;
+
+ timer = geode_mfgpt_alloc_timer(MFGPT_TIMER_ANY,
+ MFGPT_DOMAIN_ANY, THIS_MODULE);
+
+ if (timer == -1) {
+ printk(KERN_ERR "geodewdt: No timers were available\n");
+ return -ENODEV;
+ }
+
+ wdt_timer = timer;
+
+ /* Set up the timer */
+
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_SETUP,
+ GEODEWDT_SCALE | (3 << 8));
+
+ /* Set up comparator 2 to reset when the event fires */
+ geode_mfgpt_set_event(wdt_timer, MFGPT_CMP2, MFGPT_EVENT_RESET);
+
+ /* Set up the initial timeout */
+
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_CMP2,
+ cur_interval * GEODEWDT_HZ);
+
+ ret = misc_register(&geodewdt_miscdev);
+ if (ret)
+ return ret;
+
+ ret = register_reboot_notifier(&geodewdt_notifier);
+
+ if (ret)
+ misc_deregister(&geodewdt_miscdev);
+
+ return ret;
+}
+
+static void __exit
+geodewdt_exit(void)
+{
+ misc_deregister(&geodewdt_miscdev);
+ unregister_reboot_notifier(&geodewdt_notifier);
+}
+
+module_init(geodewdt_init);
+module_exit(geodewdt_exit);
+
+MODULE_AUTHOR("Advanced Micro Devices, Inc");
+MODULE_DESCRIPTION("Geode GX/LX Watchdog Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
diff -uNr linux-2.6.16.56/drivers/char/watchdog/Kconfig linux-2.6.16.56.patched/drivers/char/watchdog/Kconfig
--- linux-2.6.16.56/drivers/char/watchdog/Kconfig 2007-11-01 03:23:29.000000000 +0100
+++ linux-2.6.16.56.patched/drivers/char/watchdog/Kconfig 2007-12-12 15:22:31.000000000 +0100
@@ -209,6 +209,14 @@
You can compile this driver directly into the kernel, or use
it as a module. The module will be called sc520_wdt.

+config GEODE_WDT
+ tristate "AMD Geode GX/LX Watchdog Timer"
+ depends on WATCHDOG && X86
+ help
+ Enable support for a hardware based watchdog timer running
+ on the MFGPT timers available on AMD Geode GX and LX based
+ platforms.
+
config EUROTECH_WDT
tristate "Eurotech CPU-1220/1410 Watchdog Timer"
depends on WATCHDOG && X86
diff -uNr linux-2.6.16.56/drivers/char/watchdog/Makefile linux-2.6.16.56.patched/drivers/char/watchdog/Makefile
--- linux-2.6.16.56/drivers/char/watchdog/Makefile 2007-11-01 03:23:29.000000000 +0100
+++ linux-2.6.16.56.patched/drivers/char/watchdog/Makefile 2007-12-12 15:22:31.000000000 +0100
@@ -38,6 +38,7 @@
obj-$(CONFIG_ALIM7101_WDT) += alim7101_wdt.o
obj-$(CONFIG_SC520_WDT) += sc520_wdt.o
obj-$(CONFIG_EUROTECH_WDT) += eurotechwdt.o
+obj-$(CONFIG_GEODE_WDT) += geodewdt.o
obj-$(CONFIG_IB700_WDT) += ib700wdt.o
obj-$(CONFIG_IBMASR) += ibmasr.o
obj-$(CONFIG_WAFER_WDT) += wafer5823wdt.o
diff -uNr linux-2.6.16.56/include/asm-i386/geode-mfgpt.h linux-2.6.16.56.patched/include/asm-i386/geode-mfgpt.h
--- linux-2.6.16.56/include/asm-i386/geode-mfgpt.h 1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.16.56.patched/include/asm-i386/geode-mfgpt.h 2007-12-11 14:57:37.000000000 +0100
@@ -0,0 +1,57 @@
+/* Driver/API for AMD Geode Multi-Function General Purpose Timers (MFGPT)
+ *
+ * Copyright (C) 2006, Advanced Micro Devices, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ */
+
+#ifndef MFGPT_GEODE_H_
+#define MFGPT_GEODE_H_
+
+#define MFGPT_TIMER_ANY -1
+
+#define MFGPT_DOMAIN_WORKING 1
+#define MFGPT_DOMAIN_STANDBY 2
+#define MFGPT_DOMAIN_ANY (MFGPT_DOMAIN_WORKING | MFGPT_DOMAIN_STANDBY)
+
+#define MFGPT_CMP1 0
+#define MFGPT_CMP2 1
+
+#define MFGPT_EVENT_IRQ 0
+#define MFGPT_EVENT_NMI 1
+#define MFGPT_EVENT_RESET 3
+
+#define MFGPT_REG_CMP1 0
+#define MFGPT_REG_CMP2 2
+#define MFGPT_REG_COUNTER 4
+#define MFGPT_REG_SETUP 6
+
+#define MFGPT_SETUP_CNTEN (1 << 15)
+#define MFGPT_SETUP_CMP2 (1 << 14)
+#define MFGPT_SETUP_CMP1 (1 << 13)
+#define MFGPT_SETUP_SETUP (1 << 12)
+#define MFGPT_SETUP_STOPEN (1 << 11)
+#define MFGPT_SETUP_EXTEN (1 << 10)
+#define MFGPT_SETUP_REVEN (1 << 5)
+#define MFGPT_SETUP_CLKSEL (1 << 4)
+
+extern void geode_mfgpt_toggle_event(int, int, int, int);
+
+#define geode_mfgpt_set_event(t,c,e) geode_mfgpt_toggle_event(t,c,e,1)
+#define geode_mfgpt_clear_event(t,c,e) geode_mfgpt_toggle_event(t,c,e,0)
+
+extern void geode_mfgpt_set_irq(int, int, int, int);
+
+#define geode_mfgpt_setup_irq(t, c, i) geode_mfgpt_set_irq(t,c,i,1)
+#define geode_mfgpt_release_irq(t, c, i) geode_mfgpt_set_irq(t,c,i,0)
+
+extern void geode_mfgpt_write(int, u16, u16);
+extern u16 geode_mfgpt_read(int, u16);
+
+extern int geode_mfgpt_alloc_timer(int, int, struct module *);
+
+#endif

Attachments:

patch_12_cs5535_gpt_wdt_alix_2.6.16 (17.00 kB)

2008-01-17 20:42:18

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

On Thu, 17 Jan 2008 20:53:57 +0100
Arnd Hannemann <[email protected]> wrote:

> Andres Salomon schrieb:
> > On Thu, 17 Jan 2008 10:54:30 +0100
> > Arnd Hannemann <[email protected]> wrote:
> >
> >> Andres Salomon schrieb:
> >>> On Wed, 16 Jan 2008 16:19:12 -0500
> >>> Andres Salomon <[email protected]> wrote:
> >>>
> >>>> On Wed, 16 Jan 2008 18:44:07 +0100
> >>>> Arnd Hannemann <[email protected]> wrote:
> >>>>
> >>>>> Hi,
> >>>>>
> >>>>> I'm trying to boot 2.6.24-rc8 on a GEODE LX board (ALIX.3),
> >>>>> and it hangs during boot:
> >>>>>
> >>>>> [ 12.689971] NET: Registered protocol family 16
> >>>>> [ 12.703329] geode-mfgpt: Registered timer 0
> >>>>> [ 12.716149] mfgpt-timer: registering the MFGT timer as a clock event...
> >>>>>
> >>>> What BIOS are you using? It's possible that our detection code is
> >>>> failing to detect in-use timers.
> >> I'm using v0.99 (latest available).
> >
> >
> > v0.99 of what? Jordan seems to think it's an Award BIOS, but I'd like
> > to make sure.
>
> Its an ALIX board from PCEngines, they have their own BIOS
> implementation (tinyBios).
> http://www.pcengines.ch/alix.htm
>

Ah, okay. I couldn't find anything about MFGPTs in the available
source, but I did find the following:

http://article.gmane.org/gmane.linux.distributions.voyage.general/1824

It would appear that something between 0.90 and 0.92 changed that
has broken things. There's also a changelog here:

http://forum.pfsense.org/index.php?topic=6729.15

Note two MFGPT-related entries in 0.92:

- disable CS5536 diverse device power management to avoid
MFGPT /
interrupt issues.

- MFGPT issues: please observe AMD CS5536 data book section
5.16.3,
incorrect initialization sequence can HANG the system.

I'm assuming the first is messing w/ DIVIL_GLD_MSR_PM...

2008-01-17 21:19:17

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

On 17/01/08 20:53 +0100, Arnd Hannemann wrote:
> Andres Salomon schrieb:
> > On Thu, 17 Jan 2008 10:54:30 +0100
> > Arnd Hannemann <[email protected]> wrote:
> >
> >> Andres Salomon schrieb:
> >>> On Wed, 16 Jan 2008 16:19:12 -0500
> >>> Andres Salomon <[email protected]> wrote:
> >>>
> >>>> On Wed, 16 Jan 2008 18:44:07 +0100
> >>>> Arnd Hannemann <[email protected]> wrote:
> >>>>
> >>>>> Hi,
> >>>>>
> >>>>> I'm trying to boot 2.6.24-rc8 on a GEODE LX board (ALIX.3),
> >>>>> and it hangs during boot:
> >>>>>
> >>>>> [ 12.689971] NET: Registered protocol family 16
> >>>>> [ 12.703329] geode-mfgpt: Registered timer 0
> >>>>> [ 12.716149] mfgpt-timer: registering the MFGT timer as a clock event...
> >>>>>
> >>>> What BIOS are you using? It's possible that our detection code is
> >>>> failing to detect in-use timers.
> >> I'm using v0.99 (latest available).
> >
> >
> > v0.99 of what? Jordan seems to think it's an Award BIOS, but I'd like
> > to make sure.
>
> Its an ALIX board from PCEngines, they have their own BIOS
> implementation (tinyBios).
> http://www.pcengines.ch/alix.htm
>
> >
> >> Also note when I do enable the mysterios "MFGPT workaround" option in
> >> the bios the machine hangs directly after:
> >> [ 36.780990] NET: Registered protocol family 16
> >
> >
> > "MFGPT workaround"? That sounds a bit frightening.
> >
> > Presumably, the BIOS is using the MFGPTs, but we're not detecting them as
> > being in use.
> Yes I think so too, for the fun of it I compiled a 2.6.16.29 kernel with
> the attached patch from fi4l.

Okay - thats an MFPGT patch from pre-OLPC days. I am the guilty and
dubious party. We changed the API to work better with the timer tick,
and thats the version that ended up in the kernel.

I really wish I could take back this patch, because it keeps coming back
to torment me. We must, as a people, put it behind us and forgot it. :)

> relevant output is this:
> [ 31.015425] geode-mfgpt: 7 timers available.
> ...
> [ 31.245875] geode-mfgpt: Registered timer 0

> So the above kernel detects only 7 timers not 8, and it works. But note
> that timer 0 is not used as a clock event source but as a watchdog,
> which btw actually works fine :-)

It detects 7 timers because of a bug in the code - there really are 8
timers, which the current code correctly identifies.

> The funny thing is the #define workaround part of this dubious patch and
> its interaction with the bios:
>
> #ifdef WORKAROUND:
> I have to turn the "MFPGT workaround" option in the bios ON, to boot
> the kernel probably.
>
> #ifndef WORKAROUND:
> I have to turn the "MFPGT workaround" option in the bios OFF, to boot
> the kernel probably.

So the workaround works around the workaround. Fun. I think that Mitch
Bradley verified that if you write the magic MSR when all the clocks are
already clear that bad things happen. The workaround probably adds a
dummy clock in. Notice that the "magic MSR" no longer is in the vanilla
code, and thats the way it should be. If the BIOS doesn't allow use of
the clocks, then we have to live with that.

So, based on everything you are saying, I think its clear that our
problem isn't in the MFGPT, but rather in the timer tick (because, as
you said, the watchdog works). We try to use IRQ 7 for the tick, which
Andres and I totally plucked out of thin air based on what we had to work
with on OLPC. Its totally possible that the TinyBIOS had other ideas.
Please try to boot with nomfgpt, and see which interrupts are free, and
use mfgpt_irq= to change it to something else if 7 is in use. Based on
your findings above, you'll probably need to leave the MFGPT workaround
off from now on.

I'll port the watchdog timer to the new API, and we can use that instead
of the timer tick to just make sure that it isn't the timer that is broken.
Also, hopefully that will cease the stream of angry emails asking me why
the ancient patch doesn't work on a current kernel... :)

Jordan

2008-01-17 21:51:39

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

Jordan Crouse schrieb:
> On 17/01/08 20:53 +0100, Arnd Hannemann wrote:
>> Andres Salomon schrieb:
>>> On Thu, 17 Jan 2008 10:54:30 +0100
>>> Arnd Hannemann <[email protected]> wrote:
>>>
>>>> Andres Salomon schrieb:
>>>>> On Wed, 16 Jan 2008 16:19:12 -0500
>>>>> Andres Salomon <[email protected]> wrote:
>>>>>
>>>>>> On Wed, 16 Jan 2008 18:44:07 +0100
>>>>>> Arnd Hannemann <[email protected]> wrote:
>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> I'm trying to boot 2.6.24-rc8 on a GEODE LX board (ALIX.3),
>>>>>>> and it hangs during boot:
>>>>>>>
>>>>>>> [ 12.689971] NET: Registered protocol family 16
>>>>>>> [ 12.703329] geode-mfgpt: Registered timer 0
>>>>>>> [ 12.716149] mfgpt-timer: registering the MFGT timer as a clock event...
>>>>>>>
>>>>>> What BIOS are you using? It's possible that our detection code is
>>>>>> failing to detect in-use timers.
>>>> I'm using v0.99 (latest available).
>>>
>>> v0.99 of what? Jordan seems to think it's an Award BIOS, but I'd like
>>> to make sure.
>> Its an ALIX board from PCEngines, they have their own BIOS
>> implementation (tinyBios).
>> http://www.pcengines.ch/alix.htm
>>
>>>> Also note when I do enable the mysterios "MFGPT workaround" option in
>>>> the bios the machine hangs directly after:
>>>> [ 36.780990] NET: Registered protocol family 16
>>>
>>> "MFGPT workaround"? That sounds a bit frightening.
>>>
>>> Presumably, the BIOS is using the MFGPTs, but we're not detecting them as
>>> being in use.
>> Yes I think so too, for the fun of it I compiled a 2.6.16.29 kernel with
>> the attached patch from fi4l.
>
> Okay - thats an MFPGT patch from pre-OLPC days. I am the guilty and
> dubious party. We changed the API to work better with the timer tick,
> and thats the version that ended up in the kernel.
>
> I really wish I could take back this patch, because it keeps coming back
> to torment me. We must, as a people, put it behind us and forgot it. :)
>
>> relevant output is this:
>> [ 31.015425] geode-mfgpt: 7 timers available.
>> ...
>> [ 31.245875] geode-mfgpt: Registered timer 0
>
>> So the above kernel detects only 7 timers not 8, and it works. But note
>> that timer 0 is not used as a clock event source but as a watchdog,
>> which btw actually works fine :-)
>
> It detects 7 timers because of a bug in the code - there really are 8
> timers, which the current code correctly identifies.
Yes I can confirm this, changed MFGPT_MAX_TIMERS from 7 to 8 in the old
kernel and it still works.

>> The funny thing is the #define workaround part of this dubious patch and
>> its interaction with the bios:
>>
>> #ifdef WORKAROUND:
>> I have to turn the "MFPGT workaround" option in the bios ON, to boot
>> the kernel probably.
>>
>> #ifndef WORKAROUND:
>> I have to turn the "MFPGT workaround" option in the bios OFF, to boot
>> the kernel probably.
>
> So the workaround works around the workaround. Fun. I think that Mitch
> Bradley verified that if you write the magic MSR when all the clocks are
> already clear that bad things happen. The workaround probably adds a
> dummy clock in. Notice that the "magic MSR" no longer is in the vanilla
> code, and thats the way it should be. If the BIOS doesn't allow use of
> the clocks, then we have to live with that.
>
> So, based on everything you are saying, I think its clear that our
> problem isn't in the MFGPT, but rather in the timer tick (because, as
> you said, the watchdog works). We try to use IRQ 7 for the tick, which
> Andres and I totally plucked out of thin air based on what we had to work
> with on OLPC. Its totally possible that the TinyBIOS had other ideas.
> Please try to boot with nomfgpt, and see which interrupts are free, and
> use mfgpt_irq= to change it to something else if 7 is in use. Based on
> your findings above, you'll probably need to leave the MFGPT workaround
> off from now on.
Great analysis! I think I can confirm this too. I tried the following:

First in mfgpt_timer_setup I commented out "clockevents_register_device"
result: the system still hangs with "registering the MFGT timer as a
clock event" !

Then I also commented out "ret = setup_irq(irq, &mfgptirq)".
result: system boots, voila!

However the vendor claims that 7 should be used (from the bios changelog):
"v0.90 (IRQ7 is no longer directed to the LPC bus, used as a default
interrupt for MFGPT high resolution timer."

There is also a interrupt map in the bios[0] readme:

IRQ0 timer
IRQ1 KBD (LPC)
IRQ2 cascade
IRQ3 COM1 serial (internal / LPC)
IRQ4 COM2 serial (LPC)
IRQ5 audio (CS5536)
IRQ6 FDC (LPC)
IRQ7 spare, used for MFGPT high resolution timer

IRQ8 RTC
IRQ9 PCI INTA
IRQ10 PCI INTB
IRQ11 PCI INTC
IRQ12 PCI INTD
IRQ13 floating point
IRQ14 IDE HDD
IRQ15 USB (CS5536)

/proc/interrupts on a running system looks like this:
CPU0
0: 12329 XT-PIC timer
2: 0 XT-PIC cascade
4: 240 XT-PIC serial
8: 3 XT-PIC rtc
9: 558 XT-PIC wifi0
10: 67591 XT-PIC eth0
11: 622 XT-PIC wifi1
NMI: 0
ERR: 0

> I'll port the watchdog timer to the new API, and we can use that instead
> of the timer tick to just make sure that it isn't the timer that is broken.
> Also, hopefully that will cease the stream of angry emails asking me why
> the ancient patch doesn't work on a current kernel... :)
Watchdog for the new API would be great :-)

> Jordan
Thanks for your effort!

[0] http://www.pcengines.ch/file/alixb099.zip

2008-01-17 22:39:29

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

On 17/01/08 22:50 +0100, Arnd Hannemann wrote:
> Jordan Crouse schrieb:
> > On 17/01/08 20:53 +0100, Arnd Hannemann wrote:
> >> Andres Salomon schrieb:
> >>> On Thu, 17 Jan 2008 10:54:30 +0100
> >>> Arnd Hannemann <[email protected]> wrote:
> >>>
> >>>> Andres Salomon schrieb:
> >>>>> On Wed, 16 Jan 2008 16:19:12 -0500
> >>>>> Andres Salomon <[email protected]> wrote:
> >>>>>
> >>>>>> On Wed, 16 Jan 2008 18:44:07 +0100
> >>>>>> Arnd Hannemann <[email protected]> wrote:
> >>>>>>
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> I'm trying to boot 2.6.24-rc8 on a GEODE LX board (ALIX.3),
> >>>>>>> and it hangs during boot:
> >>>>>>>
> >>>>>>> [ 12.689971] NET: Registered protocol family 16
> >>>>>>> [ 12.703329] geode-mfgpt: Registered timer 0
> >>>>>>> [ 12.716149] mfgpt-timer: registering the MFGT timer as a clock event...
> >>>>>>>
> >>>>>> What BIOS are you using? It's possible that our detection code is
> >>>>>> failing to detect in-use timers.
> >>>> I'm using v0.99 (latest available).
> >>>
> >>> v0.99 of what? Jordan seems to think it's an Award BIOS, but I'd like
> >>> to make sure.
> >> Its an ALIX board from PCEngines, they have their own BIOS
> >> implementation (tinyBios).
> >> http://www.pcengines.ch/alix.htm
> >>
> >>>> Also note when I do enable the mysterios "MFGPT workaround" option in
> >>>> the bios the machine hangs directly after:
> >>>> [ 36.780990] NET: Registered protocol family 16
> >>>
> >>> "MFGPT workaround"? That sounds a bit frightening.
> >>>
> >>> Presumably, the BIOS is using the MFGPTs, but we're not detecting them as
> >>> being in use.
> >> Yes I think so too, for the fun of it I compiled a 2.6.16.29 kernel with
> >> the attached patch from fi4l.
> >
> > Okay - thats an MFPGT patch from pre-OLPC days. I am the guilty and
> > dubious party. We changed the API to work better with the timer tick,
> > and thats the version that ended up in the kernel.
> >
> > I really wish I could take back this patch, because it keeps coming back
> > to torment me. We must, as a people, put it behind us and forgot it. :)
> >
> >> relevant output is this:
> >> [ 31.015425] geode-mfgpt: 7 timers available.
> >> ...
> >> [ 31.245875] geode-mfgpt: Registered timer 0
> >
> >> So the above kernel detects only 7 timers not 8, and it works. But note
> >> that timer 0 is not used as a clock event source but as a watchdog,
> >> which btw actually works fine :-)
> >
> > It detects 7 timers because of a bug in the code - there really are 8
> > timers, which the current code correctly identifies.
> Yes I can confirm this, changed MFGPT_MAX_TIMERS from 7 to 8 in the old
> kernel and it still works.
>
> >> The funny thing is the #define workaround part of this dubious patch and
> >> its interaction with the bios:
> >>
> >> #ifdef WORKAROUND:
> >> I have to turn the "MFPGT workaround" option in the bios ON, to boot
> >> the kernel probably.
> >>
> >> #ifndef WORKAROUND:
> >> I have to turn the "MFPGT workaround" option in the bios OFF, to boot
> >> the kernel probably.
> >
> > So the workaround works around the workaround. Fun. I think that Mitch
> > Bradley verified that if you write the magic MSR when all the clocks are
> > already clear that bad things happen. The workaround probably adds a
> > dummy clock in. Notice that the "magic MSR" no longer is in the vanilla
> > code, and thats the way it should be. If the BIOS doesn't allow use of
> > the clocks, then we have to live with that.
> >
> > So, based on everything you are saying, I think its clear that our
> > problem isn't in the MFGPT, but rather in the timer tick (because, as
> > you said, the watchdog works). We try to use IRQ 7 for the tick, which
> > Andres and I totally plucked out of thin air based on what we had to work
> > with on OLPC. Its totally possible that the TinyBIOS had other ideas.
> > Please try to boot with nomfgpt, and see which interrupts are free, and
> > use mfgpt_irq= to change it to something else if 7 is in use. Based on
> > your findings above, you'll probably need to leave the MFGPT workaround
> > off from now on.
> Great analysis! I think I can confirm this too. I tried the following:
>
> First in mfgpt_timer_setup I commented out "clockevents_register_device"
> result: the system still hangs with "registering the MFGT timer as a
> clock event" !
>
> Then I also commented out "ret = setup_irq(irq, &mfgptirq)".
> result: system boots, voila!

Hmmm - not sure whats happening here. I wonder if we're stuck in an
interrupt storm of some sort as soon as you register the interrupt handler.
But I would think that whatever was causing the interrupt storm would be
running well before we hit setup_irq(), and you would be recording "nobody
cared" interrupts left and right.

The thing that scares me is that the TinyBIOS seems to know that we want
to use the MFGPT timers, and I wonder if they did anything behind the scenes
to "help us out" even though we didn't ask for it.

I don't know how easy it would be for you - but can you try reading
MSRs 0x51400020 - 0x51400023? If you need a command line app to do it,
you can use rdmsr from here:

http://wiki.laptop.org/go/Flashing_LinuxBIOS_on_A-Test_Boards

> Watchdog for the new API would be great :-)

Coming soon.

Jordan
--
Jordan Crouse
Systems Software Development Engineer
Advanced Micro Devices, Inc.

2008-01-17 22:55:30

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

Jordan Crouse wrote:
> On 17/01/08 22:50 +0100, Arnd Hannemann wrote:
>> Jordan Crouse schrieb:
>>> On 17/01/08 20:53 +0100, Arnd Hannemann wrote:
>>>> Andres Salomon schrieb:
>>>>> On Thu, 17 Jan 2008 10:54:30 +0100
>>>>> Arnd Hannemann <[email protected]> wrote:
>>>>>
>>>>>> Andres Salomon schrieb:
>>>>>>> On Wed, 16 Jan 2008 16:19:12 -0500
>>>>>>> Andres Salomon <[email protected]> wrote:
>>>>>>>
>>>>>>>> On Wed, 16 Jan 2008 18:44:07 +0100
>>>>>>>> Arnd Hannemann <[email protected]> wrote:
>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> I'm trying to boot 2.6.24-rc8 on a GEODE LX board (ALIX.3),
>>>>>>>>> and it hangs during boot:
>>>>>>>>>
>>>>>>>>> [ 12.689971] NET: Registered protocol family 16
>>>>>>>>> [ 12.703329] geode-mfgpt: Registered timer 0
>>>>>>>>> [ 12.716149] mfgpt-timer: registering the MFGT timer as a clock event...
>>>>>>>>>
>>>>>>>> What BIOS are you using? It's possible that our detection code is
>>>>>>>> failing to detect in-use timers.
>>>>>> I'm using v0.99 (latest available).
>>>>> v0.99 of what? Jordan seems to think it's an Award BIOS, but I'd like
>>>>> to make sure.
>>>> Its an ALIX board from PCEngines, they have their own BIOS
>>>> implementation (tinyBios).
>>>> http://www.pcengines.ch/alix.htm
>>>>
>>>>>> Also note when I do enable the mysterios "MFGPT workaround" option in
>>>>>> the bios the machine hangs directly after:
>>>>>> [ 36.780990] NET: Registered protocol family 16
>>>>> "MFGPT workaround"? That sounds a bit frightening.
>>>>>
>>>>> Presumably, the BIOS is using the MFGPTs, but we're not detecting them as
>>>>> being in use.
>>>> Yes I think so too, for the fun of it I compiled a 2.6.16.29 kernel with
>>>> the attached patch from fi4l.
>>> Okay - thats an MFPGT patch from pre-OLPC days. I am the guilty and
>>> dubious party. We changed the API to work better with the timer tick,
>>> and thats the version that ended up in the kernel.
>>>
>>> I really wish I could take back this patch, because it keeps coming back
>>> to torment me. We must, as a people, put it behind us and forgot it. :)
>>>
>>>> relevant output is this:
>>>> [ 31.015425] geode-mfgpt: 7 timers available.
>>>> ...
>>>> [ 31.245875] geode-mfgpt: Registered timer 0
>>>> So the above kernel detects only 7 timers not 8, and it works. But note
>>>> that timer 0 is not used as a clock event source but as a watchdog,
>>>> which btw actually works fine :-)
>>> It detects 7 timers because of a bug in the code - there really are 8
>>> timers, which the current code correctly identifies.
>> Yes I can confirm this, changed MFGPT_MAX_TIMERS from 7 to 8 in the old
>> kernel and it still works.
>>
>>>> The funny thing is the #define workaround part of this dubious patch and
>>>> its interaction with the bios:
>>>>
>>>> #ifdef WORKAROUND:
>>>> I have to turn the "MFPGT workaround" option in the bios ON, to boot
>>>> the kernel probably.
>>>>
>>>> #ifndef WORKAROUND:
>>>> I have to turn the "MFPGT workaround" option in the bios OFF, to boot
>>>> the kernel probably.
>>> So the workaround works around the workaround. Fun. I think that Mitch
>>> Bradley verified that if you write the magic MSR when all the clocks are
>>> already clear that bad things happen. The workaround probably adds a
>>> dummy clock in. Notice that the "magic MSR" no longer is in the vanilla
>>> code, and thats the way it should be. If the BIOS doesn't allow use of
>>> the clocks, then we have to live with that.
>>>
>>> So, based on everything you are saying, I think its clear that our
>>> problem isn't in the MFGPT, but rather in the timer tick (because, as
>>> you said, the watchdog works). We try to use IRQ 7 for the tick, which
>>> Andres and I totally plucked out of thin air based on what we had to work
>>> with on OLPC. Its totally possible that the TinyBIOS had other ideas.
>>> Please try to boot with nomfgpt, and see which interrupts are free, and
>>> use mfgpt_irq= to change it to something else if 7 is in use. Based on
>>> your findings above, you'll probably need to leave the MFGPT workaround
>>> off from now on.
>> Great analysis! I think I can confirm this too. I tried the following:
>>
>> First in mfgpt_timer_setup I commented out "clockevents_register_device"
>> result: the system still hangs with "registering the MFGT timer as a
>> clock event" !
>>
>> Then I also commented out "ret = setup_irq(irq, &mfgptirq)".
>> result: system boots, voila!
>
> Hmmm - not sure whats happening here. I wonder if we're stuck in an
> interrupt storm of some sort as soon as you register the interrupt handler.
> But I would think that whatever was causing the interrupt storm would be
> running well before we hit setup_irq(), and you would be recording "nobody
> cared" interrupts left and right.

Interesting thing is that it hangs not in setup_irq() but later, right
after printing the newline of the printk.

> The thing that scares me is that the TinyBIOS seems to know that we want
> to use the MFGPT timers, and I wonder if they did anything behind the scenes
> to "help us out" even though we didn't ask for it.
>
> I don't know how easy it would be for you - but can you try reading
> MSRs 0x51400020 - 0x51400023? If you need a command line app to do it,
> you can use rdmsr from here:
>
> http://wiki.laptop.org/go/Flashing_LinuxBIOS_on_A-Test_Boards

MSR register 0x51400020 => b7:ef:5f:f4:bf:d1:95:68
MSR register 0x51400021 => b7:fd:1f:f4:bf:cf:5a:d8
MSR register 0x51400022 => b7:f3:bf:f4:bf:f5:fb:a8
MSR register 0x51400023 => b7:fb:9f:f4:bf:fd:d9:f8

>
>> Watchdog for the new API would be great :-)
>
> Coming soon.
>
> Jordan

Arnd

2008-01-17 22:59:27

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

On 17/01/08 23:52 +0100, Arnd Hannemann wrote:

<snip>

> > Hmmm - not sure whats happening here. I wonder if we're stuck in an
> > interrupt storm of some sort as soon as you register the interrupt handler.
> > But I would think that whatever was causing the interrupt storm would be
> > running well before we hit setup_irq(), and you would be recording "nobody
> > cared" interrupts left and right.
>
> Interesting thing is that it hangs not in setup_irq() but later, right
> after printing the newline of the printk.

THat makes me think interrupt storm even more.

> > The thing that scares me is that the TinyBIOS seems to know that we want
> > to use the MFGPT timers, and I wonder if they did anything behind the scenes
> > to "help us out" even though we didn't ask for it.
> >
> > I don't know how easy it would be for you - but can you try reading
> > MSRs 0x51400020 - 0x51400023? If you need a command line app to do it,
> > you can use rdmsr from here:
> >
> > http://wiki.laptop.org/go/Flashing_LinuxBIOS_on_A-Test_Boards
>
> MSR register 0x51400020 => b7:ef:5f:f4:bf:d1:95:68
> MSR register 0x51400021 => b7:fd:1f:f4:bf:cf:5a:d8
> MSR register 0x51400022 => b7:f3:bf:f4:bf:f5:fb:a8
> MSR register 0x51400023 => b7:fb:9f:f4:bf:fd:d9:f8

Hmmm - those look wrong. Is /dev/cpu/0/msr there? The applet on the
wiki has a bug that doesn't check for it.

Jordan

--
Jordan Crouse
Systems Software Development Engineer
Advanced Micro Devices, Inc.

2008-01-17 23:39:09

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

Jordan Crouse schrieb:
> On 17/01/08 23:52 +0100, Arnd Hannemann wrote:
>
> <snip>
>
>>> Hmmm - not sure whats happening here. I wonder if we're stuck in an
>>> interrupt storm of some sort as soon as you register the interrupt handler.
>>> But I would think that whatever was causing the interrupt storm would be
>>> running well before we hit setup_irq(), and you would be recording "nobody
>>> cared" interrupts left and right.
>> Interesting thing is that it hangs not in setup_irq() but later, right
>> after printing the newline of the printk.
>
> THat makes me think interrupt storm even more.
>
>>> The thing that scares me is that the TinyBIOS seems to know that we want
>>> to use the MFGPT timers, and I wonder if they did anything behind the scenes
>>> to "help us out" even though we didn't ask for it.
>>>
>>> I don't know how easy it would be for you - but can you try reading
>>> MSRs 0x51400020 - 0x51400023? If you need a command line app to do it,
>>> you can use rdmsr from here:
>>>
>>> http://wiki.laptop.org/go/Flashing_LinuxBIOS_on_A-Test_Boards
>> MSR register 0x51400020 => b7:ef:5f:f4:bf:d1:95:68
>> MSR register 0x51400021 => b7:fd:1f:f4:bf:cf:5a:d8
>> MSR register 0x51400022 => b7:f3:bf:f4:bf:f5:fb:a8
>> MSR register 0x51400023 => b7:fb:9f:f4:bf:fd:d9:f8
>
> Hmmm - those look wrong. Is /dev/cpu/0/msr there? The applet on the
> wiki has a bug that doesn't check for it.
I'm sorry, I should have checked: I didn't execute rdmsr as root.
The correct ones:

MSR register 0x51400020 => 00:00:00:00:00:00:0f:00
MSR register 0x51400021 => 00:00:00:00:04:00:00:00
MSR register 0x51400022 => 00:00:00:00:00:00:00:00
MSR register 0x51400023 => 00:00:00:00:00:0c:ba:90

Arnd

2008-01-18 01:17:22

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

On 18/01/08 00:39 +0100, Arnd Hannemann wrote:
> Jordan Crouse schrieb:
> > On 17/01/08 23:52 +0100, Arnd Hannemann wrote:
> >
> > <snip>
> >
> >>> Hmmm - not sure whats happening here. I wonder if we're stuck in an
> >>> interrupt storm of some sort as soon as you register the interrupt handler.
> >>> But I would think that whatever was causing the interrupt storm would be
> >>> running well before we hit setup_irq(), and you would be recording "nobody
> >>> cared" interrupts left and right.
> >> Interesting thing is that it hangs not in setup_irq() but later, right
> >> after printing the newline of the printk.
> >
> > THat makes me think interrupt storm even more.
> >
> >>> The thing that scares me is that the TinyBIOS seems to know that we want
> >>> to use the MFGPT timers, and I wonder if they did anything behind the scenes
> >>> to "help us out" even though we didn't ask for it.
> >>>
> >>> I don't know how easy it would be for you - but can you try reading
> >>> MSRs 0x51400020 - 0x51400023? If you need a command line app to do it,
> >>> you can use rdmsr from here:
> >>>
> >>> http://wiki.laptop.org/go/Flashing_LinuxBIOS_on_A-Test_Boards
> >> MSR register 0x51400020 => b7:ef:5f:f4:bf:d1:95:68
> >> MSR register 0x51400021 => b7:fd:1f:f4:bf:cf:5a:d8
> >> MSR register 0x51400022 => b7:f3:bf:f4:bf:f5:fb:a8
> >> MSR register 0x51400023 => b7:fb:9f:f4:bf:fd:d9:f8
> >
> > Hmmm - those look wrong. Is /dev/cpu/0/msr there? The applet on the
> > wiki has a bug that doesn't check for it.
> I'm sorry, I should have checked: I didn't execute rdmsr as root.
> The correct ones:
>
> MSR register 0x51400020 => 00:00:00:00:00:00:0f:00
> MSR register 0x51400021 => 00:00:00:00:04:00:00:00
> MSR register 0x51400022 => 00:00:00:00:00:00:00:00
> MSR register 0x51400023 => 00:00:00:00:00:0c:ba:90

Okay - those are sane. Those are the IRQ routing MSRs - each nibble is an
IRQ for something in the southbridge - since 7 doesn't appear, a rogue
interrupt isn't likely. Also, [0:31] in 0x51400022 are the MFGPT interrupts
specifically, and they are 0. The timer tick code would set the first nibble
to 7 (in a sane system).

So, everything _seems_ okay from the hardware side. The next step would be
to comment out the setup_irq() function and write a C app or something to
verify that all the MFGPT registers are set up like we expect them to be.
However, I think that maybe we can accomplish the same thing with the
forthcoming watchdog timer, since it will prove the MFGPT is behaving
well (though it doesn't use the interrupt wrapper, but one step at a time).

Jordan
--
Jordan Crouse
Systems Software Development Engineer
Advanced Micro Devices, Inc.

2008-01-19 01:15:31

[permalink] [raw]

Subject: [GEODE] Geode GX/LX watchdog timer (was 2.6.24-rc8 hangs at mfgpt-timer)

[GEODE] Add a watchdog driver based on the CS5535/CS5536 MFGPT timers

From: Jordan Crouse <[email protected]>

Add a watchdog timer based on the MFGPT timers in the CS5535/CS5536
companion chips to the AMD Geode GX and LX processors. Only caveat
is that the BIOS must provide at least a one free timer, and most
do not.

Signed-off-by: Jordan Crouse <[email protected]>
---

drivers/watchdog/Kconfig | 13 ++
drivers/watchdog/Makefile | 1
drivers/watchdog/geodewdt.c | 321 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 335 insertions(+), 0 deletions(-)

Index: git/drivers/watchdog/Kconfig
===================================================================
--- git.orig/drivers/watchdog/Kconfig 2008-01-18 15:06:44.000000000 -0700
+++ git/drivers/watchdog/Kconfig 2008-01-18 17:50:25.000000000 -0700
@@ -295,6 +295,20 @@

Most people will say N.

+config GEODE_WDT
+ tristate "AMD Geode CS5535/CS5536 Watchdog"
+ depends on MGEODE_LX
+ default n
+ help
+ This driver enables a watchdog capability built into the
+ CS5535/CS5536 companion chips for the AMD Geode GX and LX
+ processors. This watchdog watches your kernel to make sure
+ it doesn't freeze, and if it does, it reboots your computer after
+ a certain amount of time.
+
+ You can compile this driver directly into the kernel, or use
+ it as a module. The module will be called geodewdt.
+
config SC520_WDT
tristate "AMD Elan SC520 processor Watchdog"
depends on X86
Index: git/drivers/watchdog/Makefile
===================================================================
--- git.orig/drivers/watchdog/Makefile 2008-01-18 15:06:44.000000000 -0700
+++ git/drivers/watchdog/Makefile 2008-01-18 16:32:15.000000000 -0700
@@ -59,6 +59,7 @@
obj-$(CONFIG_ADVANTECH_WDT) += advantechwdt.o
obj-$(CONFIG_ALIM1535_WDT) += alim1535_wdt.o
obj-$(CONFIG_ALIM7101_WDT) += alim7101_wdt.o
+obj-$(CONFIG_GEODE_WDT) += geodewdt.o
obj-$(CONFIG_SC520_WDT) += sc520_wdt.o
obj-$(CONFIG_EUROTECH_WDT) += eurotechwdt.o
obj-$(CONFIG_IB700_WDT) += ib700wdt.o
Index: git/drivers/watchdog/geodewdt.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ git/drivers/watchdog/geodewdt.c 2008-01-18 17:47:39.000000000 -0700
@@ -0,0 +1,308 @@
+/* Watchdog timer for the Geode GX/LX with the CS5535/CS5536 companion chip
+ *
+ * Copyright (C) 2006-2007, Advanced Micro Devices, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/types.h>
+#include <linux/miscdevice.h>
+#include <linux/watchdog.h>
+#include <linux/fs.h>
+#include <linux/platform_device.h>
+#include <linux/reboot.h>
+
+#include <asm/uaccess.h>
+#include <asm/geode.h>
+
+#define GEODEWDT_HZ 500
+#define GEODEWDT_SCALE 6
+#define GEODEWDT_MAX_SECONDS 131
+
+#define WDT_FLAGS_OPEN 1
+#define WDT_FLAGS_ORPHAN 2
+
+#define DRV_NAME "geodewdt"
+#define WATCHDOG_NAME "Geode GX/LX WDT"
+#define WATCHDOG_TIMEOUT 60
+
+static int timeout = WATCHDOG_TIMEOUT;
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds. 1<= timeout <=131, default=" __MODULE_STRING(WATCHDOG_TIMEOUT) ".");
+
+static int nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+static struct platform_device *geodewdt_platform_device;
+static unsigned long wdt_flags;
+static int wdt_timer;
+static int safe_close;
+
+static void geodewdt_ping(void)
+{
+ /* Stop the counter */
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_SETUP, 0);
+
+ /* Reset the counter */
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_COUNTER, 0);
+
+ /* Enable the counter */
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_SETUP, MFGPT_SETUP_CNTEN);
+}
+
+static void geodewdt_disable(void)
+{
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_SETUP, 0);
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_COUNTER, 0);
+}
+
+static int geodewdt_set_heartbeat(int val)
+{
+ if (val < 1 || val > GEODEWDT_MAX_SECONDS)
+ return -EINVAL;
+
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_SETUP, 0);
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_CMP2, val * GEODEWDT_HZ);
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_COUNTER, 0);
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_SETUP, MFGPT_SETUP_CNTEN);
+
+ timeout = val;
+ return 0;
+}
+
+static int
+geodewdt_open(struct inode *inode, struct file *file)
+{
+ if (test_and_set_bit(WDT_FLAGS_OPEN, &wdt_flags))
+ return -EBUSY;
+
+ if (!test_and_clear_bit(WDT_FLAGS_ORPHAN, &wdt_flags))
+ __module_get(THIS_MODULE);
+
+ geodewdt_ping();
+ return nonseekable_open(inode, file);
+}
+
+static int
+geodewdt_release(struct inode *inode, struct file *file)
+{
+ if (safe_close) {
+ geodewdt_disable();
+ module_put(THIS_MODULE);
+ }
+ else {
+ printk(KERN_CRIT "Unexpected close - watchdog is not stopping.\n");
+ geodewdt_ping();
+
+ set_bit(WDT_FLAGS_ORPHAN, &wdt_flags);
+ }
+
+ clear_bit(WDT_FLAGS_OPEN, &wdt_flags);
+ safe_close = 0;
+ return 0;
+}
+
+static ssize_t
+geodewdt_write(struct file *file, const char __user *data, size_t len,
+ loff_t *ppos)
+{
+ if(len) {
+ if (!nowayout) {
+ size_t i;
+ safe_close = 0;
+
+ for (i = 0; i != len; i++) {
+ char c;
+
+ if (get_user(c, data + i))
+ return -EFAULT;
+
+ if (c == 'V')
+ safe_close = 1;
+ }
+ }
+
+ geodewdt_ping();
+ }
+ return len;
+}
+
+static int
+geodewdt_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ void __user *argp = (void __user *)arg;
+ int __user *p = argp;
+ int interval;
+
+ static struct watchdog_info ident = {
+ .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING
+ | WDIOF_MAGICCLOSE,
+ .firmware_version = 1,
+ .identity = WATCHDOG_NAME,
+ };
+
+ switch(cmd) {
+ case WDIOC_GETSUPPORT:
+ return copy_to_user(argp, &ident,
+ sizeof(ident)) ? -EFAULT : 0;
+ break;
+
+ case WDIOC_GETSTATUS:
+ case WDIOC_GETBOOTSTATUS:
+ return put_user(0, p);
+
+ case WDIOC_KEEPALIVE:
+ geodewdt_ping();
+ return 0;
+
+ case WDIOC_SETTIMEOUT:
+ if (get_user(interval, p))
+ return -EFAULT;
+
+ if (geodewdt_set_heartbeat(interval))
+ return -EINVAL;
+
+/* Fall through */
+
+ case WDIOC_GETTIMEOUT:
+ return put_user(timeout, p);
+
+ case WDIOC_SETOPTIONS:
+ {
+ int options, ret = -EINVAL;
+
+ if (get_user(options, p))
+ return -EFAULT;
+
+ if (options & WDIOS_DISABLECARD) {
+ geodewdt_disable();
+ ret = 0;
+ }
+
+ if (options & WDIOS_ENABLECARD) {
+ geodewdt_ping();
+ ret = 0;
+ }
+
+ return ret;
+ }
+ default:
+ return -ENOTTY;
+ }
+
+ return 0;
+}
+
+static const struct file_operations geodewdt_fops = {
+ .owner = THIS_MODULE,
+ .llseek = no_llseek,
+ .write = geodewdt_write,
+ .ioctl = geodewdt_ioctl,
+ .open = geodewdt_open,
+ .release = geodewdt_release,
+};
+
+static struct miscdevice geodewdt_miscdev = {
+ .minor = WATCHDOG_MINOR,
+ .name = "geode-watchdog",
+ .fops = &geodewdt_fops
+};
+
+static int __devinit
+geodewdt_probe(struct platform_device *dev)
+{
+ int ret, timer;
+
+ timer = geode_mfgpt_alloc_timer(MFGPT_TIMER_ANY,
+ MFGPT_DOMAIN_WORKING, THIS_MODULE);
+
+ if (timer == -1) {
+ printk(KERN_ERR "geodewdt: No timers were available\n");
+ return -ENODEV;
+ }
+
+ wdt_timer = timer;
+
+ /* Set up the timer */
+
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_SETUP,
+ GEODEWDT_SCALE | (3 << 8));
+
+ /* Set up comparator 2 to reset when the event fires */
+ geode_mfgpt_toggle_event(wdt_timer, MFGPT_CMP2, MFGPT_EVENT_RESET, 1);
+
+ /* Set up the initial timeout */
+
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_CMP2,
+ timeout * GEODEWDT_HZ);
+
+ ret = misc_register(&geodewdt_miscdev);
+
+ return ret;
+}
+
+static int __devexit
+geodewdt_remove(struct platform_device *dev)
+{
+ misc_deregister(&geodewdt_miscdev);
+}
+
+static void
+geodewdt_shutdown(struct platform_device *dev)
+{
+ geodewdt_disable();
+}
+
+static struct platform_driver geodewdt_driver = {
+ .probe = geodewdt_probe,
+ .remove = __devexit_p(geodewdt_remove),
+ .shutdown = geodewdt_shutdown,
+ .driver = {
+ .owner = THIS_MODULE,
+ .name = DRV_NAME,
+ },
+};
+
+static int __init
+geodewdt_init(void)
+{
+ int ret;
+
+ ret = platform_driver_register(&geodewdt_driver);
+ if (ret)
+ return ret;
+
+ geodewdt_platform_device = platform_device_register_simple(DRV_NAME, -1, NULL, 0);
+ if (IS_ERR(geodewdt_platform_device)) {
+ ret = PTR_ERR(geodewdt_platform_device);
+ goto err;
+ }
+
+ return 0;
+err:
+ platform_driver_unregister(&geodewdt_driver);
+ return ret;
+}
+
+static void __exit
+geodewdt_exit(void)
+{
+ platform_device_unregister(geodewdt_platform_device);
+ platform_driver_unregister(&geodewdt_driver);
+}
+
+module_init(geodewdt_init);
+module_exit(geodewdt_exit);
+
+MODULE_AUTHOR("Advanced Micro Devices, Inc");
+MODULE_DESCRIPTION("Geode GX/LX Watchdog Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
Index: git/arch/x86/kernel/mfgpt_32.c
===================================================================
--- git.orig/arch/x86/kernel/mfgpt_32.c 2008-01-18 16:57:50.000000000 -0700
+++ git/arch/x86/kernel/mfgpt_32.c 2008-01-18 17:48:22.000000000 -0700
@@ -142,6 +142,8 @@
return 0;
}

+EXPORT_SYMBOL_GPL(geode_mfgpt_toggle_event);
+
int geode_mfgpt_set_irq(int timer, int cmp, int irq, int enable)
{
u32 val, dummy;
@@ -204,6 +206,7 @@
return -1;
}

+EXPORT_SYMBOL_GPL(geode_mfgpt_alloc_timer);

#ifdef CONFIG_GEODE_MFGPT_TIMER

Attachments:

(No filename) (1.63 kB)
geode-watchdog.patch (9.70 kB)
Download all attachments

2008-01-19 07:01:34

[permalink] [raw]

Subject: Re: [GEODE] Geode GX/LX watchdog timer (was 2.6.24-rc8 hangs at mfgpt-timer)

On Fri, Jan 18, 2008 at 06:06:24PM -0700, Jordan Crouse wrote:
> I don't know how much of a hassle it would be for Andres to get a 2.6.24
> kernel running on the OLPC to make sure that this isn't a regression
> in the timer tick code (I suspect it isn't a regression, but you never
> know). I also think that it would probably be in our best interest to
> default CONFIG_GEODE_MFGPT_TIMER to 'n' until we get this figured
> out. Since most BIOSen don't have timers available, that shouldn't affect
> too many people.

Well, I've successfully used earlier version of this code with 2.6.22
on a PCEngines ALIX motherboard equipped with LX800/CS5536. It boots
on a TinyBIOS.

I will try 2.6.24 + this patch on these boards when I have some time.

Willy

2008-01-20 13:22:25

[permalink] [raw]

Subject: Re: [GEODE] Geode GX/LX watchdog timer (was 2.6.24-rc8 hangs at mfgpt-timer)

Hi,

Jordan Crouse wrote:
> On 17/01/08 23:52 +0100, Arnd Hannemann wrote:
>>>> Watchdog for the new API would be great :-)
>>> Coming soon.
>
> As promised, a watchdog driver for the Geode GX/LX processors is attached.
> I basically just ported the previous patch forward to 2.6.24.

Great work!

>
> I also have good news or bad news depending on your perspective. I wanted
> to test this against 2.6.24, and OLPC is stuck at an older kernel version,
> so I had to test this with coreboot (LinuxBIOS) on another Geode
> platform. Like all BIOSen execpt for the OLPC firmware, coreboot uses
> VSA (SMM handler) which consumes all the timers.
>
> So I used the magical MSR and surprise! - the timer tick hung.
> I compiled out the timer tick, and tested the watchdog timer instead,
> and it worked fine on timer 0. So I don't think the MFGPTs themselves
> have anything to do with this problem, but I do think it might be
> related to VSA and possibly interrupts too. I'm going to invoke the
> strong BIOS fu of our LinuxBIOS / BIOS expert Marc Jones, and see what
> he comes up with.
>
> I don't know how much of a hassle it would be for Andres to get a 2.6.24
> kernel running on the OLPC to make sure that this isn't a regression
> in the timer tick code (I suspect it isn't a regression, but you never
> know). I also think that it would probably be in our best interest to
> default CONFIG_GEODE_MFGPT_TIMER to 'n' until we get this figured
> out. Since most BIOSen don't have timers available, that shouldn't affect
> too many people.
>
> So, anyway, enjoy the watchdog timer - I hope it meets everybody's
> expectations for the 2.6.25 kernel.

Thanks a lot for this, it works great! (with CONFIG_GEODE_MFGPT_TIMER
not set).
However some minor issues:
Could the name of the /dev entry perhaps be changed from
"geode-watchdog" to "watchdog" instead?
I think all other watchdogs use "watchdog", and using two different
watchdogs in the same machine won't work anyway, because of the same
minor number, right?

As a second point my gcc (4.1.2) issues a warning:

drivers/watchdog/geodewdt.c: In function ‘geodewdt_remove’:
drivers/watchdog/geodewdt.c:256: warning: control reaches end of
non-void function

which I think is a valid one.

>
> Jordan
>
Best regards,
Arnd

2008-01-20 16:36:54

[permalink] [raw]

Subject: Re: Geode GX/LX watchdog timer (was 2.6.24-rc8 hangs at mfgpt-timer)

On 20/01/08 14:22 +0100, Arnd Hannemann wrote:
> Hi,
>
> Jordan Crouse wrote:
> > On 17/01/08 23:52 +0100, Arnd Hannemann wrote:
> >>>> Watchdog for the new API would be great :-)
> >>> Coming soon.
> >
> > As promised, a watchdog driver for the Geode GX/LX processors is attached.
> > I basically just ported the previous patch forward to 2.6.24.
>
> Great work!
>
> >
> > I also have good news or bad news depending on your perspective. I wanted
> > to test this against 2.6.24, and OLPC is stuck at an older kernel version,
> > so I had to test this with coreboot (LinuxBIOS) on another Geode
> > platform. Like all BIOSen execpt for the OLPC firmware, coreboot uses
> > VSA (SMM handler) which consumes all the timers.
> >
> > So I used the magical MSR and surprise! - the timer tick hung.
> > I compiled out the timer tick, and tested the watchdog timer instead,
> > and it worked fine on timer 0. So I don't think the MFGPTs themselves
> > have anything to do with this problem, but I do think it might be
> > related to VSA and possibly interrupts too. I'm going to invoke the
> > strong BIOS fu of our LinuxBIOS / BIOS expert Marc Jones, and see what
> > he comes up with.
> >
> > I don't know how much of a hassle it would be for Andres to get a 2.6.24
> > kernel running on the OLPC to make sure that this isn't a regression
> > in the timer tick code (I suspect it isn't a regression, but you never
> > know). I also think that it would probably be in our best interest to
> > default CONFIG_GEODE_MFGPT_TIMER to 'n' until we get this figured
> > out. Since most BIOSen don't have timers available, that shouldn't affect
> > too many people.
> >
> > So, anyway, enjoy the watchdog timer - I hope it meets everybody's
> > expectations for the 2.6.25 kernel.
>
> Thanks a lot for this, it works great! (with CONFIG_GEODE_MFGPT_TIMER
> not set).
> However some minor issues:
> Could the name of the /dev entry perhaps be changed from
> "geode-watchdog" to "watchdog" instead?
> I think all other watchdogs use "watchdog", and using two different
> watchdogs in the same machine won't work anyway, because of the same
> minor number, right?

Very much yes - complete oversight on my part.

> As a second point my gcc (4.1.2) issues a warning:
>
> drivers/watchdog/geodewdt.c: In function ‘geodewdt_remove’:
> drivers/watchdog/geodewdt.c:256: warning: control reaches end of
> non-void function
>
> which I think is a valid one.

Yes again. I'll refactor. Thanks for your comments.

Jordan
--
Jordan Crouse
Systems Software Development Engineer
Advanced Micro Devices, Inc.

2008-01-20 20:16:17

by Lennart Sorensen

[permalink] [raw]

Subject: Re: [GEODE] Geode GX/LX watchdog timer (was 2.6.24-rc8 hangs at mfgpt-timer)

On Fri, Jan 18, 2008 at 06:06:24PM -0700, Jordan Crouse wrote:
> As promised, a watchdog driver for the Geode GX/LX processors is attached.
> I basically just ported the previous patch forward to 2.6.24.
>
> I also have good news or bad news depending on your perspective. I wanted
> to test this against 2.6.24, and OLPC is stuck at an older kernel version,
> so I had to test this with coreboot (LinuxBIOS) on another Geode
> platform. Like all BIOSen execpt for the OLPC firmware, coreboot uses
> VSA (SMM handler) which consumes all the timers.

Hmm, the Geode LX platform I work with uses VSA/SMM and I have had no
problems using timer 2 for a watchdog. Some of the timers are certainly
already setup by something in the BIOS, but quite a few are not in use.
The board I am using is the compulab iGLX module. I did have to mangle
together a watchdog driver myself some time ago since there wasn't one
for the Geode LX at the time.

Of course I haven't gone past 2.6.18 kernel for now (I am tracking
Debian stable for the most part), so I have no idea if something in a
newer kernel breaks things.

> So I used the magical MSR and surprise! - the timer tick hung.
> I compiled out the timer tick, and tested the watchdog timer instead,
> and it worked fine on timer 0. So I don't think the MFGPTs themselves
> have anything to do with this problem, but I do think it might be
> related to VSA and possibly interrupts too. I'm going to invoke the
> strong BIOS fu of our LinuxBIOS / BIOS expert Marc Jones, and see what
> he comes up with.
>
> I don't know how much of a hassle it would be for Andres to get a 2.6.24
> kernel running on the OLPC to make sure that this isn't a regression
> in the timer tick code (I suspect it isn't a regression, but you never
> know). I also think that it would probably be in our best interest to
> default CONFIG_GEODE_MFGPT_TIMER to 'n' until we get this figured
> out. Since most BIOSen don't have timers available, that shouldn't affect
> too many people.
>
> So, anyway, enjoy the watchdog timer - I hope it meets everybody's
> expectations for the 2.6.25 kernel.

--
Len Sorensen

2008-01-21 17:51:11

[permalink] [raw]

Subject: Re: Geode GX/LX watchdog timer (RESEND)

[GEODE] Add a watchdog driver based on the CS5535/CS5536 MFGPT timers

From: Jordan Crouse <[email protected]>

Add a watchdog timer based on the MFGPT timers in the CS5535/CS5536
companion chips to the AMD Geode GX and LX processors. Only caveat
is that the BIOS must provide at least a one free timer, and most
do not.

Signed-off-by: Jordan Crouse <[email protected]>
---

drivers/watchdog/Kconfig | 13 ++
drivers/watchdog/Makefile | 1
drivers/watchdog/geodewdt.c | 309 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 323 insertions(+), 0 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 52dff40..15b4c29 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -295,6 +295,19 @@ config ALIM7101_WDT

Most people will say N.

+config GEODE_WDT
+ tristate "AMD Geode CS5535/CS5536 Watchdog"
+ depends on MGEODE_LX
+ help
+ This driver enables a watchdog capability built into the
+ CS5535/CS5536 companion chips for the AMD Geode GX and LX
+ processors. This watchdog watches your kernel to make sure
+ it doesn't freeze, and if it does, it reboots your computer after
+ a certain amount of time.
+
+ You can compile this driver directly into the kernel, or use
+ it as a module. The module will be called geodewdt.
+
config SC520_WDT
tristate "AMD Elan SC520 processor Watchdog"
depends on X86
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 87483cc..830ce70 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -59,6 +59,7 @@ obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
obj-$(CONFIG_ADVANTECH_WDT) += advantechwdt.o
obj-$(CONFIG_ALIM1535_WDT) += alim1535_wdt.o
obj-$(CONFIG_ALIM7101_WDT) += alim7101_wdt.o
+obj-$(CONFIG_GEODE_WDT) += geodewdt.o
obj-$(CONFIG_SC520_WDT) += sc520_wdt.o
obj-$(CONFIG_EUROTECH_WDT) += eurotechwdt.o
obj-$(CONFIG_IB700_WDT) += ib700wdt.o
diff --git a/drivers/watchdog/geodewdt.c b/drivers/watchdog/geodewdt.c
new file mode 100644
index 0000000..f85b196
--- /dev/null
+++ b/drivers/watchdog/geodewdt.c
@@ -0,0 +1,309 @@
+/* Watchdog timer for the Geode GX/LX with the CS5535/CS5536 companion chip
+ *
+ * Copyright (C) 2006-2007, Advanced Micro Devices, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/types.h>
+#include <linux/miscdevice.h>
+#include <linux/watchdog.h>
+#include <linux/fs.h>
+#include <linux/platform_device.h>
+#include <linux/reboot.h>
+
+#include <asm/uaccess.h>
+#include <asm/geode.h>
+
+#define GEODEWDT_HZ 500
+#define GEODEWDT_SCALE 6
+#define GEODEWDT_MAX_SECONDS 131
+
+#define WDT_FLAGS_OPEN 1
+#define WDT_FLAGS_ORPHAN 2
+
+#define DRV_NAME "geodewdt"
+#define WATCHDOG_NAME "Geode GX/LX WDT"
+#define WATCHDOG_TIMEOUT 60
+
+static int timeout = WATCHDOG_TIMEOUT;
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds. 1<= timeout <=131, default=" __MODULE_STRING(WATCHDOG_TIMEOUT) ".");
+
+static int nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+static struct platform_device *geodewdt_platform_device;
+static unsigned long wdt_flags;
+static int wdt_timer;
+static int safe_close;
+
+static void geodewdt_ping(void)
+{
+ /* Stop the counter */
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_SETUP, 0);
+
+ /* Reset the counter */
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_COUNTER, 0);
+
+ /* Enable the counter */
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_SETUP, MFGPT_SETUP_CNTEN);
+}
+
+static void geodewdt_disable(void)
+{
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_SETUP, 0);
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_COUNTER, 0);
+}
+
+static int geodewdt_set_heartbeat(int val)
+{
+ if (val < 1 || val > GEODEWDT_MAX_SECONDS)
+ return -EINVAL;
+
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_SETUP, 0);
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_CMP2, val * GEODEWDT_HZ);
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_COUNTER, 0);
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_SETUP, MFGPT_SETUP_CNTEN);
+
+ timeout = val;
+ return 0;
+}
+
+static int
+geodewdt_open(struct inode *inode, struct file *file)
+{
+ if (test_and_set_bit(WDT_FLAGS_OPEN, &wdt_flags))
+ return -EBUSY;
+
+ if (!test_and_clear_bit(WDT_FLAGS_ORPHAN, &wdt_flags))
+ __module_get(THIS_MODULE);
+
+ geodewdt_ping();
+ return nonseekable_open(inode, file);
+}
+
+static int
+geodewdt_release(struct inode *inode, struct file *file)
+{
+ if (safe_close) {
+ geodewdt_disable();
+ module_put(THIS_MODULE);
+ }
+ else {
+ printk(KERN_CRIT "Unexpected close - watchdog is not stopping.\n");
+ geodewdt_ping();
+
+ set_bit(WDT_FLAGS_ORPHAN, &wdt_flags);
+ }
+
+ clear_bit(WDT_FLAGS_OPEN, &wdt_flags);
+ safe_close = 0;
+ return 0;
+}
+
+static ssize_t
+geodewdt_write(struct file *file, const char __user *data, size_t len,
+ loff_t *ppos)
+{
+ if(len) {
+ if (!nowayout) {
+ size_t i;
+ safe_close = 0;
+
+ for (i = 0; i != len; i++) {
+ char c;
+
+ if (get_user(c, data + i))
+ return -EFAULT;
+
+ if (c == 'V')
+ safe_close = 1;
+ }
+ }
+
+ geodewdt_ping();
+ }
+ return len;
+}
+
+static int
+geodewdt_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ void __user *argp = (void __user *)arg;
+ int __user *p = argp;
+ int interval;
+
+ static struct watchdog_info ident = {
+ .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING
+ | WDIOF_MAGICCLOSE,
+ .firmware_version = 1,
+ .identity = WATCHDOG_NAME,
+ };
+
+ switch(cmd) {
+ case WDIOC_GETSUPPORT:
+ return copy_to_user(argp, &ident,
+ sizeof(ident)) ? -EFAULT : 0;
+ break;
+
+ case WDIOC_GETSTATUS:
+ case WDIOC_GETBOOTSTATUS:
+ return put_user(0, p);
+
+ case WDIOC_KEEPALIVE:
+ geodewdt_ping();
+ return 0;
+
+ case WDIOC_SETTIMEOUT:
+ if (get_user(interval, p))
+ return -EFAULT;
+
+ if (geodewdt_set_heartbeat(interval))
+ return -EINVAL;
+
+/* Fall through */
+
+ case WDIOC_GETTIMEOUT:
+ return put_user(timeout, p);
+
+ case WDIOC_SETOPTIONS:
+ {
+ int options, ret = -EINVAL;
+
+ if (get_user(options, p))
+ return -EFAULT;
+
+ if (options & WDIOS_DISABLECARD) {
+ geodewdt_disable();
+ ret = 0;
+ }
+
+ if (options & WDIOS_ENABLECARD) {
+ geodewdt_ping();
+ ret = 0;
+ }
+
+ return ret;
+ }
+ default:
+ return -ENOTTY;
+ }
+
+ return 0;
+}
+
+static const struct file_operations geodewdt_fops = {
+ .owner = THIS_MODULE,
+ .llseek = no_llseek,
+ .write = geodewdt_write,
+ .ioctl = geodewdt_ioctl,
+ .open = geodewdt_open,
+ .release = geodewdt_release,
+};
+
+static struct miscdevice geodewdt_miscdev = {
+ .minor = WATCHDOG_MINOR,
+ .name = "watchdog",
+ .fops = &geodewdt_fops
+};
+
+static int __devinit
+geodewdt_probe(struct platform_device *dev)
+{
+ int ret, timer;
+
+ timer = geode_mfgpt_alloc_timer(MFGPT_TIMER_ANY,
+ MFGPT_DOMAIN_WORKING, THIS_MODULE);
+
+ if (timer == -1) {
+ printk(KERN_ERR "geodewdt: No timers were available\n");
+ return -ENODEV;
+ }
+
+ wdt_timer = timer;
+
+ /* Set up the timer */
+
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_SETUP,
+ GEODEWDT_SCALE | (3 << 8));
+
+ /* Set up comparator 2 to reset when the event fires */
+ geode_mfgpt_toggle_event(wdt_timer, MFGPT_CMP2, MFGPT_EVENT_RESET, 1);
+
+ /* Set up the initial timeout */
+
+ geode_mfgpt_write(wdt_timer, MFGPT_REG_CMP2,
+ timeout * GEODEWDT_HZ);
+
+ ret = misc_register(&geodewdt_miscdev);
+
+ return ret;
+}
+
+static int __devexit
+geodewdt_remove(struct platform_device *dev)
+{
+ misc_deregister(&geodewdt_miscdev);
+ return 0;
+}
+
+static void
+geodewdt_shutdown(struct platform_device *dev)
+{
+ geodewdt_disable();
+}
+
+static struct platform_driver geodewdt_driver = {
+ .probe = geodewdt_probe,
+ .remove = __devexit_p(geodewdt_remove),
+ .shutdown = geodewdt_shutdown,
+ .driver = {
+ .owner = THIS_MODULE,
+ .name = DRV_NAME,
+ },
+};
+
+static int __init
+geodewdt_init(void)
+{
+ int ret;
+
+ ret = platform_driver_register(&geodewdt_driver);
+ if (ret)
+ return ret;
+
+ geodewdt_platform_device = platform_device_register_simple(DRV_NAME, -1, NULL, 0);
+ if (IS_ERR(geodewdt_platform_device)) {
+ ret = PTR_ERR(geodewdt_platform_device);
+ goto err;
+ }
+
+ return 0;
+err:
+ platform_driver_unregister(&geodewdt_driver);
+ return ret;
+}
+
+static void __exit
+geodewdt_exit(void)
+{
+ platform_device_unregister(geodewdt_platform_device);
+ platform_driver_unregister(&geodewdt_driver);
+}
+
+module_init(geodewdt_init);
+module_exit(geodewdt_exit);
+
+MODULE_AUTHOR("Advanced Micro Devices, Inc");
+MODULE_DESCRIPTION("Geode GX/LX Watchdog Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);

Attachments:

(No filename) (2.42 kB)
geode-watchdog.patch (8.98 kB)
Download all attachments

2008-01-21 18:36:51

[permalink] [raw]

Subject: Re: Geode GX/LX watchdog timer (RESEND)

Jordan Crouse schrieb:
> On 20/01/08 14:22 +0100, Arnd Hannemann wrote:
>> Hi,
>>
>> Jordan Crouse wrote:
>>> On 17/01/08 23:52 +0100, Arnd Hannemann wrote:
>>>>>> Watchdog for the new API would be great :-)
>>>>> Coming soon.
>>> As promised, a watchdog driver for the Geode GX/LX processors is attached.
>>> I basically just ported the previous patch forward to 2.6.24.
>> Great work!
>>
>>> I also have good news or bad news depending on your perspective. I wanted
>>> to test this against 2.6.24, and OLPC is stuck at an older kernel version,
>>> so I had to test this with coreboot (LinuxBIOS) on another Geode
>>> platform. Like all BIOSen execpt for the OLPC firmware, coreboot uses
>>> VSA (SMM handler) which consumes all the timers.
>>>
>>> So I used the magical MSR and surprise! - the timer tick hung.
>>> I compiled out the timer tick, and tested the watchdog timer instead,
>>> and it worked fine on timer 0. So I don't think the MFGPTs themselves
>>> have anything to do with this problem, but I do think it might be
>>> related to VSA and possibly interrupts too. I'm going to invoke the
>>> strong BIOS fu of our LinuxBIOS / BIOS expert Marc Jones, and see what
>>> he comes up with.
>>>
>>> I don't know how much of a hassle it would be for Andres to get a 2.6.24
>>> kernel running on the OLPC to make sure that this isn't a regression
>>> in the timer tick code (I suspect it isn't a regression, but you never
>>> know). I also think that it would probably be in our best interest to
>>> default CONFIG_GEODE_MFGPT_TIMER to 'n' until we get this figured
>>> out. Since most BIOSen don't have timers available, that shouldn't affect
>>> too many people.
>>>
>>> So, anyway, enjoy the watchdog timer - I hope it meets everybody's
>>> expectations for the 2.6.25 kernel.
>> Thanks a lot for this, it works great! (with CONFIG_GEODE_MFGPT_TIMER
>> not set).
>> However some minor issues:
>> Could the name of the /dev entry perhaps be changed from
>> "geode-watchdog" to "watchdog" instead?
>> I think all other watchdogs use "watchdog", and using two different
>> watchdogs in the same machine won't work anyway, because of the same
>> minor number, right?
>>
>> As a second point my gcc (4.1.2) issues a warning:
>>
>> drivers/watchdog/geodewdt.c: In function ‘geodewdt_remove’:
>> drivers/watchdog/geodewdt.c:256: warning: control reaches end of
>> non-void function
>>
>> which I think is a valid one.
>
> Resending with Arnd's concern's addressed. Thanks.

I can confirm that it is still working fine :-)

>
> Jordan
>
Greetings,
Arnd

2008-01-21 23:28:07

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

diff --git a/arch/x86/kernel/mfgpt_32.c b/arch/x86/kernel/mfgpt_32.c
index 0ab680f..5b4fa24 100644
--- a/arch/x86/kernel/mfgpt_32.c
+++ b/arch/x86/kernel/mfgpt_32.c
@@ -74,18 +74,31 @@ int __init geode_mfgpt_detect(void)
{
int count = 0, i;
u16 val;
+ u32 hi, lo;

if (disable) {
printk(KERN_INFO "geode-mfgpt: Skipping MFGPT setup\n");
return 0;
}

+ rdmsr(0x51400028, lo, hi);
+ printk(KERN_INFO "geode-mfgpt: IRQ MSR=%x:%x\n", hi, lo);
+
+ rdmsr(0x51400029, lo, hi);
+ printk(KERN_INFO "geode-mfgpt: NMI MSR=%x:%x\n", hi, lo);
+
+ rdmsr(0x51400022, lo, hi);
+ printk(KERN_INFO "geode-mfgpt: Unrestricted sources=%x\n", lo);
+
for (i = 0; i < MFGPT_MAX_TIMERS; i++) {
val = geode_mfgpt_read(i, MFGPT_REG_SETUP);
if (!(val & MFGPT_SETUP_SETUP)) {
mfgpt_timers[i].flags = F_AVAIL;
count++;
}
+ else {
+ printk(KERN_INFO "geode-mfgpt: [%d] is already setup=%x\n", i, val);
+ }
}

/* set up clock event device, if desired */

Attachments:

(No filename) (994.00 B)
mfgpt-debug-spew.patch (973.00 B)
Download all attachments

2008-01-22 00:00:44

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

Hi Jordan,

On Mon, Jan 21, 2008 at 04:27:09PM -0700, Jordan Crouse wrote:
> Okay - I've been exploring a little bit more. I talked to the TinyBIOS
> developer, and he verified that TinyBIOS shouldn't use any MFGPT timers.
> He also told me that the mysterious "MFGPT workaround" was in fact the
> magic MFGPT erasing MSR that was in the old kernel driver.
>
> So with the "MFGPT workaround" turned off, TinyBIOS should be acting like
> the OLPC firmware with regards to timers, yet it is not. So that is
> curious. I think I might have identified a race condition in the code,
> but I'm not 100% sure thats the same problem that the ALIX platform is
> seeing.
>
> Anrd and others - will you please try the attached patch on your platform
> with the "MFGPT workaround" turned off and mfgpts enabled, and send out
> the dmesg?

Yes of course :

[ 44.013100] NET: Registered protocol family 16
[ 44.066308] geode-mfgpt: IRQ MSR=0:0
[ 44.110161] geode-mfgpt: NMI MSR=0:0
[ 44.154037] geode-mfgpt: Unrestricted sources=0

Then it hangs here.
In another mail I sent privately to Andres, I noticed that it was the
following line which hangs at first iteration (i == 0) :

val = geode_mfgpt_read(i, MFGPT_REG_SETUP);

I have a tinybios 0.98 with no workaround option configurable. I also
have CONFIG_GEODE_MFGPT_TIMER=n.

Hoping this helps,
Willy

2008-01-22 09:02:38

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

Jordan Crouse wrote:
> Okay - I've been exploring a little bit more. I talked to the TinyBIOS
> developer, and he verified that TinyBIOS shouldn't use any MFGPT timers.
> He also told me that the mysterious "MFGPT workaround" was in fact the
> magic MFGPT erasing MSR that was in the old kernel driver.
>
> So with the "MFGPT workaround" turned off, TinyBIOS should be acting like
> the OLPC firmware with regards to timers, yet it is not. So that is
> curious. I think I might have identified a race condition in the code,
> but I'm not 100% sure thats the same problem that the ALIX platform is
> seeing.
>
> Anrd and others - will you please try the attached patch on your platform
> with the "MFGPT workaround" turned off and mfgpts enabled, and send out
> the dmesg?

Of course, tinyBios version v0.99, "MFGPT workaround" turned off,
CONFIG_GEODE_MFGPT_TIMER=n:

[ 67.369697] NET: Registered protocol family 16
[ 67.383059] geode-mfgpt: IRQ MSR=0:0
[ 67.394058] geode-mfgpt: NMI MSR=0:0
[ 67.405049] geode-mfgpt: Unrestricted sources=0
[ 67.418909] geode: 8 MFGPT timers available.
[ 67.433211] PCI: PCI BIOS revision 2.10 entry at 0xfcd03, last bus=0

same with CONFIG_GEODE_MFGPT_TIMER=y (sorry, without move printk patch):

[ 22.289349] NET: Registered protocol family 16
[ 22.302716] geode-mfgpt: IRQ MSR=0:0
[ 22.313716] geode-mfgpt: NMI MSR=0:0
[ 22.324704] geode-mfgpt: Unrestricted sources=0
[ 22.338566] geode-mfgpt: Registered timer 0
[ 22.351393] mfgpt-timer: registering the MFGT timer as a clock event.
^^^^ Hangs here

>
> This will give us some debug information that I can use to ensure that
> the interrupts are set up correctly. You can leave the timer tick disabled
> if you want.

>
> Thanks,
> Jordan
>

Arnd

2008-01-22 10:33:55

by Lars Heete

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

Hello,

On Tuesday 22 January 2008 10:03:08 am Arnd Hannemann wrote:
> Jordan Crouse wrote:
> > Okay - I've been exploring a little bit more. I talked to the TinyBIOS
> > developer, and he verified that TinyBIOS shouldn't use any MFGPT timers.
> > He also told me that the mysterious "MFGPT workaround" was in fact the
> > magic MFGPT erasing MSR that was in the old kernel driver.
> >
> > So with the "MFGPT workaround" turned off, TinyBIOS should be acting like
> > the OLPC firmware with regards to timers, yet it is not. So that is
> > curious. I think I might have identified a race condition in the code,
> > but I'm not 100% sure thats the same problem that the ALIX platform is
> > seeing.
> >
> > Anrd and others - will you please try the attached patch on your platform
> > with the "MFGPT workaround" turned off and mfgpts enabled, and send out
> > the dmesg?
>
> Of course, tinyBios version v0.99, "MFGPT workaround" turned off,
> CONFIG_GEODE_MFGPT_TIMER=n:
>
> [ 67.369697] NET: Registered protocol family 16
> [ 67.383059] geode-mfgpt: IRQ MSR=0:0
> [ 67.394058] geode-mfgpt: NMI MSR=0:0
> [ 67.405049] geode-mfgpt: Unrestricted sources=0
> [ 67.418909] geode: 8 MFGPT timers available.
> [ 67.433211] PCI: PCI BIOS revision 2.10 entry at 0xfcd03, last bus=0
>
> same with CONFIG_GEODE_MFGPT_TIMER=y (sorry, without move printk patch):
>
> [ 22.289349] NET: Registered protocol family 16
> [ 22.302716] geode-mfgpt: IRQ MSR=0:0
> [ 22.313716] geode-mfgpt: NMI MSR=0:0
> [ 22.324704] geode-mfgpt: Unrestricted sources=0
> [ 22.338566] geode-mfgpt: Registered timer 0
> [ 22.351393] mfgpt-timer: registering the MFGT timer as a clock event.
> ^^^^ Hangs here
I had the same problem with MFGPT Timers and alix (BIOS v0.99). I found that
if you use a different interrupt than the default 7 for MFGPT (just append
mfgpt_irq=8 to the kernel commandline), the timer seems to work.

CPU0
0: 33 XT-PIC-XT timer
2: 0 XT-PIC-XT cascade
4: 1662 XT-PIC-XT serial
8: 766 XT-PIC-XT mfgpt-timer
14: 473 XT-PIC-XT libata
NMI: 0
ERR: 0

> > This will give us some debug information that I can use to ensure that
> > the interrupts are set up correctly. You can leave the timer tick
> > disabled if you want.
> >
> >
> > Thanks,
> > Jordan
>
> Arnd

Lars

2008-01-22 11:18:18

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

Hi Lars,

Lars Heete schrieb:
> Hello,
>
> On Tuesday 22 January 2008 10:03:08 am Arnd Hannemann wrote:
>> Jordan Crouse wrote:
>>> Okay - I've been exploring a little bit more. I talked to the TinyBIOS
>>> developer, and he verified that TinyBIOS shouldn't use any MFGPT timers.
>>> He also told me that the mysterious "MFGPT workaround" was in fact the
>>> magic MFGPT erasing MSR that was in the old kernel driver.
>>>
>>> So with the "MFGPT workaround" turned off, TinyBIOS should be acting like
>>> the OLPC firmware with regards to timers, yet it is not. So that is
>>> curious. I think I might have identified a race condition in the code,
>>> but I'm not 100% sure thats the same problem that the ALIX platform is
>>> seeing.
>>>
>>> Anrd and others - will you please try the attached patch on your platform
>>> with the "MFGPT workaround" turned off and mfgpts enabled, and send out
>>> the dmesg?
>> Of course, tinyBios version v0.99, "MFGPT workaround" turned off,
>> CONFIG_GEODE_MFGPT_TIMER=n:
>>
>> [ 67.369697] NET: Registered protocol family 16
>> [ 67.383059] geode-mfgpt: IRQ MSR=0:0
>> [ 67.394058] geode-mfgpt: NMI MSR=0:0
>> [ 67.405049] geode-mfgpt: Unrestricted sources=0
>> [ 67.418909] geode: 8 MFGPT timers available.
>> [ 67.433211] PCI: PCI BIOS revision 2.10 entry at 0xfcd03, last bus=0
>>
>> same with CONFIG_GEODE_MFGPT_TIMER=y (sorry, without move printk patch):
>>
>> [ 22.289349] NET: Registered protocol family 16
>> [ 22.302716] geode-mfgpt: IRQ MSR=0:0
>> [ 22.313716] geode-mfgpt: NMI MSR=0:0
>> [ 22.324704] geode-mfgpt: Unrestricted sources=0
>> [ 22.338566] geode-mfgpt: Registered timer 0
>> [ 22.351393] mfgpt-timer: registering the MFGT timer as a clock event.
>> ^^^^ Hangs here
> I had the same problem with MFGPT Timers and alix (BIOS v0.99). I found that
> if you use a different interrupt than the default 7 for MFGPT (just append
> mfgpt_irq=8 to the kernel commandline), the timer seems to work.

Indeed.
Strange, it works at least with mfgpt_irq=8 (rtc) and mfgpt_irq=5 (audio):

[ 21.805129] geode-mfgpt: IRQ MSR=0:0
[ 21.816129] geode-mfgpt: NMI MSR=0:0
[ 21.827116] geode-mfgpt: Unrestricted sources=0
[ 21.840979] geode-mfgpt: Registered timer 0
[ 21.853806] mfgpt-timer: registering the MFGT timer as a clock event.
[ 21.873576] geode: 8 MFGPT timers available.
[ 21.887962] PCI: PCI BIOS revision 2.10 entry at 0xfcd03, last bus=0

/proc/interrupts with mfgpt_irq=5:
CPU0
0: 48 XT-PIC-XT timer
2: 0 XT-PIC-XT cascade
4: 674 XT-PIC-XT serial
5: 13706 XT-PIC-XT mfgpt-timer
8: 3 XT-PIC-XT rtc
10: 57137 XT-PIC-XT eth0
15: 1 XT-PIC-XT ehci_hcd:usb1, ohci_hcd:usb2
NMI: 0 Non-maskable interrupts
TRM: 0 Thermal event interrupts
SPU: 0 Spurious interrupts
ERR: 0

/proc/interrupts with mfgpt_irq=8:
CPU0
0: 48 XT-PIC-XT timer
2: 0 XT-PIC-XT cascade
4: 2511 XT-PIC-XT serial
8: 29061 XT-PIC-XT mfgpt-timer
10: 60701 XT-PIC-XT eth0
15: 1 XT-PIC-XT ehci_hcd:usb1, ohci_hcd:usb2
NMI: 0 Non-maskable interrupts
TRM: 0 Thermal event interrupts
SPU: 0 Spurious interrupts
ERR: 0

I noted that "rtc" disappeared, do I have any drawback of this?
I assume that the mfgpt-timer cannot be shared?

>
> CPU0
> 0: 33 XT-PIC-XT timer
> 2: 0 XT-PIC-XT cascade
> 4: 1662 XT-PIC-XT serial
> 8: 766 XT-PIC-XT mfgpt-timer
> 14: 473 XT-PIC-XT libata
> NMI: 0
> ERR: 0
>
>
>>> This will give us some debug information that I can use to ensure that
>>> the interrupts are set up correctly. You can leave the timer tick
>>> disabled if you want.
>>>
>>>
>>> Thanks,
>>> Jordan
>> Arnd
>
> Lars
>

Thanks for the hint!
Arnd

2008-01-22 18:16:55

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

On 22/01/08 12:18 +0100, Arnd Hannemann wrote:
> Hi Lars,
>
> Lars Heete schrieb:
> > Hello,
> >
> > On Tuesday 22 January 2008 10:03:08 am Arnd Hannemann wrote:
> >> Jordan Crouse wrote:
> >>> Okay - I've been exploring a little bit more. I talked to the TinyBIOS
> >>> developer, and he verified that TinyBIOS shouldn't use any MFGPT timers.
> >>> He also told me that the mysterious "MFGPT workaround" was in fact the
> >>> magic MFGPT erasing MSR that was in the old kernel driver.
> >>>
> >>> So with the "MFGPT workaround" turned off, TinyBIOS should be acting like
> >>> the OLPC firmware with regards to timers, yet it is not. So that is
> >>> curious. I think I might have identified a race condition in the code,
> >>> but I'm not 100% sure thats the same problem that the ALIX platform is
> >>> seeing.
> >>>
> >>> Anrd and others - will you please try the attached patch on your platform
> >>> with the "MFGPT workaround" turned off and mfgpts enabled, and send out
> >>> the dmesg?
> >> Of course, tinyBios version v0.99, "MFGPT workaround" turned off,
> >> CONFIG_GEODE_MFGPT_TIMER=n:
> >>
> >> [ 67.369697] NET: Registered protocol family 16
> >> [ 67.383059] geode-mfgpt: IRQ MSR=0:0
> >> [ 67.394058] geode-mfgpt: NMI MSR=0:0
> >> [ 67.405049] geode-mfgpt: Unrestricted sources=0
> >> [ 67.418909] geode: 8 MFGPT timers available.
> >> [ 67.433211] PCI: PCI BIOS revision 2.10 entry at 0xfcd03, last bus=0
> >>
> >> same with CONFIG_GEODE_MFGPT_TIMER=y (sorry, without move printk patch):
> >>
> >> [ 22.289349] NET: Registered protocol family 16
> >> [ 22.302716] geode-mfgpt: IRQ MSR=0:0
> >> [ 22.313716] geode-mfgpt: NMI MSR=0:0
> >> [ 22.324704] geode-mfgpt: Unrestricted sources=0
> >> [ 22.338566] geode-mfgpt: Registered timer 0
> >> [ 22.351393] mfgpt-timer: registering the MFGT timer as a clock event.
> >> ^^^^ Hangs here
> > I had the same problem with MFGPT Timers and alix (BIOS v0.99). I found that
> > if you use a different interrupt than the default 7 for MFGPT (just append
> > mfgpt_irq=8 to the kernel commandline), the timer seems to work.
>
> Indeed.
> Strange, it works at least with mfgpt_irq=8 (rtc) and mfgpt_irq=5 (audio):

That is very unfortunate. We must continue to investigate.

> [ 21.805129] geode-mfgpt: IRQ MSR=0:0
> [ 21.816129] geode-mfgpt: NMI MSR=0:0
> [ 21.827116] geode-mfgpt: Unrestricted sources=0
> [ 21.840979] geode-mfgpt: Registered timer 0
> [ 21.853806] mfgpt-timer: registering the MFGT timer as a clock event.
> [ 21.873576] geode: 8 MFGPT timers available.
> [ 21.887962] PCI: PCI BIOS revision 2.10 entry at 0xfcd03, last bus=0
>
<snip>

> I noted that "rtc" disappeared, do I have any drawback of this?
> I assume that the mfgpt-timer cannot be shared?

No. We figured that was the best way to go. I don't know if there
are any technical reasons why it can't be shared, other then latency
problems.

But I would still prefer to figure out why 7 doesn't work - because there
is nothing in the hardware or kernel code that should prevent that.

Arnd - if you don't mind doing a little bit more debug work. You've already
given us the MSRs for 0x51400020 - 0x51400023. Can you now give us
0x51400024-0x51400027, as well as 5140004E? Our current working theory
is that IRQ7 (traditionally used by the LPT port) is still conflicting
somewhere.

Jordan
--
Jordan Crouse
Systems Software Development Engineer
Advanced Micro Devices, Inc.

2008-01-22 19:27:27

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

[GEODE] fix a race condition in the MFGPT timer tick

From: Jordan Crouse <[email protected]>

When we set the MFGPT timer tick, there is a chance that we'll
immediately assert an event. If for some reason the IRQ routing
for this clock has been setup for some other purpose, then we
could end up firing an interrupt into the SMM handler or worse.

This rearranges the timer tick init function to initalize the handler
before we set up the MFGPT clock to make sure that even if we get
an event, it will go to the handler.

Furthermore, in the handler we need to make sure that we clear the
event, even if the timer isn't running.

Signed-off-by: Jordan Crouse <[email protected]>
---

arch/x86/kernel/mfgpt_32.c | 9 +++++----
1 files changed, 5 insertions(+), 4 deletions(-)

Index: git/arch/x86/kernel/mfgpt_32.c
===================================================================
--- git.orig/arch/x86/kernel/mfgpt_32.c 2008-01-21 17:09:54.000000000 -0700
+++ git/arch/x86/kernel/mfgpt_32.c 2008-01-22 12:21:21.000000000 -0700
@@ -278,12 +278,12 @@

static irqreturn_t mfgpt_tick(int irq, void *dev_id)
{
+ /* Turn off the clock (and clear the event) */
+ mfgpt_disable_timer(mfgpt_event_clock);
+
if (mfgpt_tick_mode == CLOCK_EVT_MODE_SHUTDOWN)
return IRQ_HANDLED;

- /* Turn off the clock */
- mfgpt_disable_timer(mfgpt_event_clock);
-
/* Clear the counter */
geode_mfgpt_write(mfgpt_event_clock, MFGPT_REG_COUNTER, 0);

@@ -319,10 +319,6 @@
}

mfgpt_event_clock = timer;
- /* Set the clock scale and enable the event mode for CMP2 */
- val = MFGPT_SCALE | (3 << 8);
-
- geode_mfgpt_write(mfgpt_event_clock, MFGPT_REG_SETUP, val);

/* Set up the IRQ on the MFGPT side */
if (geode_mfgpt_setup_irq(mfgpt_event_clock, MFGPT_CMP2, irq)) {
@@ -339,6 +335,11 @@
goto err;
}

+ /* Set the clock scale and enable the event mode for CMP2 */
+ val = MFGPT_SCALE | (3 << 8);
+
+ geode_mfgpt_write(mfgpt_event_clock, MFGPT_REG_SETUP, val);
+
/* Set up the clock event */
mfgpt_clockevent.mult = div_sc(MFGPT_HZ, NSEC_PER_SEC, 32);
mfgpt_clockevent.min_delta_ns = clockevent_delta2ns(0xF,

Attachments:

(No filename) (1.06 kB)
fix-mfgpt-race.patch (2.08 kB)
Download all attachments

2008-01-22 20:44:50

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

Hi guys,

On Tue, Jan 22, 2008 at 12:32:26AM +0100, Willy Tarreau wrote:
> [ 44.013100] NET: Registered protocol family 16
> [ 44.066308] geode-mfgpt: IRQ MSR=0:0
> [ 44.110161] geode-mfgpt: NMI MSR=0:0
> [ 44.154037] geode-mfgpt: Unrestricted sources=0
>
> Then it hangs here.
> In another mail I sent privately to Andres, I noticed that it was the
> following line which hangs at first iteration (i == 0) :
>
> val = geode_mfgpt_read(i, MFGPT_REG_SETUP);
>
> I have a tinybios 0.98 with no workaround option configurable. I also
> have CONFIG_GEODE_MFGPT_TIMER=n.

Good news! I read the mfgpt patch for 2.6.22 and saw what the workaround
consisted in (writing 0xff at MSR 0x5140002B). So I tried adding the
following on top of 2.6.24-rc8 :

static int __init mfgpt_fix(char *s)
{
u32 val, dummy;

/* The following udocumented bit resets the MFGPT timers */
val = 0xFF;
wrmsr(0x5140002B, val, dummy);

return 1;
}
__setup("mfgptfix", mfgpt_fix);

and booted with the newly added option (mfgptfix). It worked like a charm :

[ 29.173796] NET: Registered protocol family 16
[ 29.226986] geode: lbars[0].base = 0x9d00
[ 29.275003] geode: lbars[1].base = 0x9c00
[ 29.323042] geode: lbars[2].base = 0x6100
[ 29.371082] geode: lbars[3].base = 0x6200
[ 29.419121] geode-mfgpt: IRQ MSR=0:0
[ 29.463000] geode-mfgpt: NMI MSR=0:0
[ 29.506881] geode-mfgpt: Unrestricted sources=0
[ 29.562202] geode_mfgpt: reading 0x6200 + 0x6 + (0x0 * 8) = 0x6206
[ 29.636237] geode_mfgpt: reading 0x6200 + 0x6 + (0x1 * 8) = 0x620e
[ 29.710270] geode_mfgpt: reading 0x6200 + 0x6 + (0x2 * 8) = 0x6216
[ 29.784305] geode_mfgpt: reading 0x6200 + 0x6 + (0x3 * 8) = 0x621e
[ 29.858338] geode_mfgpt: reading 0x6200 + 0x6 + (0x4 * 8) = 0x6226
[ 29.932376] geode_mfgpt: reading 0x6200 + 0x6 + (0x5 * 8) = 0x622e
[ 30.006409] geode_mfgpt: reading 0x6200 + 0x6 + (0x6 * 8) = 0x6236
[ 30.080444] geode_mfgpt: reading 0x6200 + 0x6 + (0x7 * 8) = 0x623e
[ 30.154475] geode: 8 MFGPT timers available.

So it seems like applying the workaround on top of TinyBIOS 0.98 undoes
this BIOS's workaround. I'm now wondering how we could detect whether
the workaround was applied or not :-/

Next, I retried with MFGPT_TIMER=y + latest fix you posted moving the
initialization race, and here's what I get now :

root@ALOHA-500:~# cat /proc/interrupts
CPU0
0: 77 XT-PIC-XT timer
2: 0 XT-PIC-XT cascade
4: 348 XT-PIC-XT serial
7: 12273 XT-PIC-XT mfgpt-timer
8: 0 XT-PIC-XT rtc
10: 80 XT-PIC-XT eth0
11: 0 XT-PIC-XT eth1
12: 1 XT-PIC-XT eth2
14: 13614 XT-PIC-XT ide0
NMI: 0 Non-maskable interrupts
TRM: 0 Thermal event interrupts
SPU: 0 Spurious interrupts
ERR: 0

Interestingly, during the boot, I got thousands of the following line :
geode_mfgpt: reading 0x6200 + 0x6 + (0x0 * 8) = 0x6206

It's a debug line I added in geode_mfgpt_read() which writes the timer and
reg being read. It slowed the boot down due to being written to a serial
console, but fortunately stopped when syslogd had changed the console log
level. Just checking...

# grep mfgpt /proc/interrupts;sleep 10;grep mfgpt /proc/interrupts
7: 82320 XT-PIC-XT mfgpt-timer
7: 84882 XT-PIC-XT mfgpt-timer

It delivers 256 IRQs/s. That makes me think about this comment in mfgpt_32.c :

* We are using the 32Khz input clock - its the only one that has the
* ranges we find desirable. The following table lists the suitable
* divisors and the associated hz, minimum interval
* and the maximum interval:
*
* Divisor Hz Min Delta (S) Max Delta (S)
* 1 32000 .0005 2.048
* 2 16000 .001 4.096
* 4 8000 .002 8.192
...

When you say a 32kHz clock, you mean 32000 Hz. Are you really sure ? Most
32 kHz clocks everywhere are really 32768 Hz (the watch quartz). BTW, I'm
seeing a 32.768 kHz xtal close to the CS5536, and the numbers above seem
to support this suggestion too.

So right now that I've found what caused old kernel to unexpectedly work,
I'm planning a BIOS upgrade. I'm still just wondering what we can do to
detect that the workaround should be needed. I suspect nothing, of course,
but just in case... Maybe we can detect the effects of the workaround ?

Best regards,
Willy

2008-01-22 20:53:52

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

Hi,

Jordan Crouse schrieb:
>> Indeed.
>> Strange, it works at least with mfgpt_irq=8 (rtc) and mfgpt_irq=5 (audio):
>
> I have most excellent news. I was able to get tinyBIOS booting on my
> development platform. I looked at the problem with the debugger and
> I think I might have found something. It looks like the interrupt is
> firing immediately before the clock is enabled. In the handler, we
> were returning immediately if the clock wasn't enabled (and not clearing
> the event), so we were caught in a classic interrupt storm.
>
> The attached patch rearranges the code so that the handler is installed
> before we setup the interrupt (so we have somebody to listen to the
> immediate interrupt), and it makes sure that we clear the event in the IRQ
> handler regardless of the state of the timer tick.

This patch indeed solves the problem. The board boots fine. Great work!

0: 48 XT-PIC-XT timer
2: 0 XT-PIC-XT cascade
4: 493 XT-PIC-XT serial
7: 25875 XT-PIC-XT mfgpt-timer
8: 3 XT-PIC-XT rtc
10: 56963 XT-PIC-XT eth0
15: 1 XT-PIC-XT ehci_hcd:usb1, ohci_hcd:usb2
NMI: 0 Non-maskable interrupts
TRM: 0 Thermal event interrupts
SPU: 0 Spurious interrupts
ERR: 0

>
> I'm not 100% sure why this happens on IRQ7 but not on 5 or 8, but it might
> have something to do with the interrupts already being enabled on the other
> vectors. Anyway, please try this test patch and let me know what happens.

Congratulations for this long but successful remote debugging ;-)

Greetings,
Arnd

2008-01-22 21:09:45

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

On 22/01/08 21:15 +0100, Willy Tarreau wrote:
> Good news! I read the mfgpt patch for 2.6.22 and saw what the workaround
> consisted in (writing 0xff at MSR 0x5140002B). So I tried adding the
> following on top of 2.6.24-rc8 :
>
> static int __init mfgpt_fix(char *s)
> {
> u32 val, dummy;
>
> /* The following udocumented bit resets the MFGPT timers */
> val = 0xFF;
> wrmsr(0x5140002B, val, dummy);
>
> return 1;
> }
> __setup("mfgptfix", mfgpt_fix);
>
> and booted with the newly added option (mfgptfix). It worked like a charm :
>
<snip>

> So it seems like applying the workaround on top of TinyBIOS 0.98 undoes
> this BIOS's workaround. I'm now wondering how we could detect whether
> the workaround was applied or not :-/

Actually, the TinyBIOS "workaround" is the same thing.

Like I may have said before, there is a reason why this MSR is undocumented.
It works, but the behavior is unvalidated, and obviously erratic. When we
first developed this code, we were using the Geode GX on the OLPC with VSA,
so using the MSR was nessesary if we wanted to get our hands on any
timers at all. Mitch Bradley (author of OpenFirmware) determined through
testing that the MSR was erratic, especially when you ran it when all the
timers were already clear. I suspect thats the problem that we see here -
writing the MSR in TinyBIOS unstablizied the registers, and writing the
MSR again kicked it back. Of course, the unfortunate corollary is that
when the workaround isn't in v0.99, if you run the MSR in the kernel, then
you will end up destabilizing everything.

Furthermore, using the MSR is okay with TinyBIOS, but not okay with the
other Geode BIOSen (Insyde, General Software, and for the moment LinuxBIOS)
because VSA (the SMM handler) _does_ use some of the timers. So needless
to say, I'm concerned.

> Interestingly, during the boot, I got thousands of the following line :
> geode_mfgpt: reading 0x6200 + 0x6 + (0x0 * 8) = 0x6206
>
> It's a debug line I added in geode_mfgpt_read() which writes the timer and
> reg being read. It slowed the boot down due to being written to a serial
> console, but fortunately stopped when syslogd had changed the console log
> level. Just checking...

We control the timer and the status bits for the timer in the setup
register, which is what you are showing above. Thats fine.

> # grep mfgpt /proc/interrupts;sleep 10;grep mfgpt /proc/interrupts
> 7: 82320 XT-PIC-XT mfgpt-timer
> 7: 84882 XT-PIC-XT mfgpt-timer
>
> It delivers 256 IRQs/s. That makes me think about this comment in mfgpt_32.c :

Hmm - are you running with nohz? I ran the same thing on the OLPC
and I'm getting 81 IRQ/s which is okay, considering that sugar was running
in the background.

> * We are using the 32Khz input clock - its the only one that has the
> * ranges we find desirable. The following table lists the suitable
> * divisors and the associated hz, minimum interval
> * and the maximum interval:
> *
> * Divisor Hz Min Delta (S) Max Delta (S)
> * 1 32000 .0005 2.048
> * 2 16000 .001 4.096
> * 4 8000 .002 8.192
> ...
>
> When you say a 32kHz clock, you mean 32000 Hz. Are you really sure ? Most
> 32 kHz clocks everywhere are really 32768 Hz (the watch quartz). BTW, I'm
> seeing a 32.768 kHz xtal close to the CS5536, and the numbers above seem
> to support this suggestion too.

Hmm - yeah, my math is off there - it is a 32768 Hz clock. That
shouldn't affect things too negatively - if it does we can adjust the
divisor we use - currently we use 16.

> So right now that I've found what caused old kernel to unexpectedly work,
> I'm planning a BIOS upgrade. I'm still just wondering what we can do to
> detect that the workaround should be needed. I suspect nothing, of course,
> but just in case... Maybe we can detect the effects of the workaround ?

I'm not sure if we can - all we can tell is if the registers are zero or
not. Like I said, running the MSR is probably dangerous in 9 out of 10
situations, the one good use being the one you determined. I would
support adding the mfgptfix bit though - just as long as it isn't
automagic.

Thank you very much for your help!
Jordan

--
Jordan Crouse
Systems Software Development Engineer
Advanced Micro Devices, Inc.

2008-01-22 21:10:41

by Ingo Molnar

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

* Arnd Hannemann <[email protected]> wrote:

> > The attached patch rearranges the code so that the handler is
> > installed before we setup the interrupt (so we have somebody to
> > listen to the immediate interrupt), and it makes sure that we clear
> > the event in the IRQ handler regardless of the state of the timer
> > tick.
>
> This patch indeed solves the problem. The board boots fine. Great
> work!

since this driver is new in 2.6.24, perhaps we should apply this fix to
v2.6.24?

Ingo

2008-01-22 21:45:10

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

On Tue, Jan 22, 2008 at 02:08:58PM -0700, Jordan Crouse wrote:
> On 22/01/08 21:15 +0100, Willy Tarreau wrote:
> > So it seems like applying the workaround on top of TinyBIOS 0.98 undoes
> > this BIOS's workaround. I'm now wondering how we could detect whether
> > the workaround was applied or not :-/
>
> Actually, the TinyBIOS "workaround" is the same thing.
>
> Like I may have said before, there is a reason why this MSR is undocumented.
> It works, but the behavior is unvalidated, and obviously erratic. When we
> first developed this code, we were using the Geode GX on the OLPC with VSA,
> so using the MSR was nessesary if we wanted to get our hands on any
> timers at all. Mitch Bradley (author of OpenFirmware) determined through
> testing that the MSR was erratic, especially when you ran it when all the
> timers were already clear. I suspect thats the problem that we see here -
> writing the MSR in TinyBIOS unstablizied the registers, and writing the
> MSR again kicked it back. Of course, the unfortunate corollary is that
> when the workaround isn't in v0.99, if you run the MSR in the kernel, then
> you will end up destabilizing everything.

Indeed, that looks exactly like what's happening.

> Furthermore, using the MSR is okay with TinyBIOS, but not okay with the
> other Geode BIOSen (Insyde, General Software, and for the moment LinuxBIOS)
> because VSA (the SMM handler) _does_ use some of the timers. So needless
> to say, I'm concerned.

I certainly can understand.

> > Interestingly, during the boot, I got thousands of the following line :
> > geode_mfgpt: reading 0x6200 + 0x6 + (0x0 * 8) = 0x6206
> > It's a debug line I added in geode_mfgpt_read() which writes the timer and
> > reg being read. It slowed the boot down due to being written to a serial
> > console, but fortunately stopped when syslogd had changed the console log
> > level. Just checking...
>
> We control the timer and the status bits for the timer in the setup
> register, which is what you are showing above. Thats fine.

OK.

> > # grep mfgpt /proc/interrupts;sleep 10;grep mfgpt /proc/interrupts
> > 7: 82320 XT-PIC-XT mfgpt-timer
> > 7: 84882 XT-PIC-XT mfgpt-timer
> >
> > It delivers 256 IRQs/s. That makes me think about this comment in mfgpt_32.c :
>
> Hmm - are you running with nohz?

No but I have CONFIG_HZ=250. That makes sense then :

#define MFGPT_HZ (32000 / MFGPT_DIVISOR)
#define MFGPT_PERIODIC (MFGPT_HZ / HZ)

=> MFGPT_PERIODIC = (32000 / 16) / 250 = 8
Given that it's 32.768 kHz in fact, we get 32768/16/8 = 256 Hz

> > * We are using the 32Khz input clock - its the only one that has the
> > * ranges we find desirable. The following table lists the suitable
> > * divisors and the associated hz, minimum interval
> > * and the maximum interval:
> > *
> > * Divisor Hz Min Delta (S) Max Delta (S)
> > * 1 32000 .0005 2.048
> > * 2 16000 .001 4.096
> > * 4 8000 .002 8.192
> > ...
> >
> > When you say a 32kHz clock, you mean 32000 Hz. Are you really sure ? Most
> > 32 kHz clocks everywhere are really 32768 Hz (the watch quartz). BTW, I'm
> > seeing a 32.768 kHz xtal close to the CS5536, and the numbers above seem
> > to support this suggestion too.
>
> Hmm - yeah, my math is off there - it is a 32768 Hz clock. That
> shouldn't affect things too negatively - if it does we can adjust the
> divisor we use - currently we use 16.

Why not just ajust the constant in MFGPT_HZ since it's the only place (aside
the comments) where this is referenced ?

If you want I can send you a patch and adjust the comments at the same time.

> > So right now that I've found what caused old kernel to unexpectedly work,
> > I'm planning a BIOS upgrade. I'm still just wondering what we can do to
> > detect that the workaround should be needed. I suspect nothing, of course,
> > but just in case... Maybe we can detect the effects of the workaround ?
>
> I'm not sure if we can - all we can tell is if the registers are zero or
> not. Like I said, running the MSR is probably dangerous in 9 out of 10
> situations, the one good use being the one you determined. I would
> support adding the mfgptfix bit though - just as long as it isn't
> automagic.

OK I perfectly understand. While I see the current behaviour as a regression
compared to 2.6.22-mainline (since 2.6.24-rc8 does not boot off from this
board without nomfgpt), the first problem is in the BIOS and it requires an
upgrade. Maybe we should extend the scope of CONFIG_GEODE_MFGPT_TIMER to
allow it to completely disable the detection logic when it's off ? I certainly
can work a clean patch for "mfgptfix", but for users who will get caught with
this board not booting at all anymore, adding an option in their boot loaders
may be as problematic as upgrading the BIOS. I'm also thinking about the ones
preparing new software updates from a recent boards and deploying them miles
away without knowing they are running a 0.98 BIOS which will prevent their
new kernel from booting.

> Thank you very much for your help!

You're welcome, thanks to you too :-)
Willy

2008-01-22 21:50:44

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

2008-01-22 21:53:57

by Thomas Gleixner

[permalink] [raw]

Subject: [git pull] was: Re: 2.6.24-rc8 hangs at mfgpt-timer

On Tue, 22 Jan 2008, Ingo Molnar wrote:
> * Arnd Hannemann <[email protected]> wrote:
>
> > > The attached patch rearranges the code so that the handler is
> > > installed before we setup the interrupt (so we have somebody to
> > > listen to the immediate interrupt), and it makes sure that we clear
> > > the event in the IRQ handler regardless of the state of the timer
> > > tick.
> >
> > This patch indeed solves the problem. The board boots fine. Great
> > work!
>
> since this driver is new in 2.6.24, perhaps we should apply this fix to
> v2.6.24?

Yes. It is definitely a safe change and makes otherwise broken systems
work. There is no impact to anything else than the GEODEs.

Linus,

if you agree, please pull the fix from:

ssh://master.kernel.org/pub/scm/linux/kernel/git/x86/linux-2.6-x86.git

Thanks,

tglx

2008-01-23 16:37:56

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

On 22/01/08 22:15 +0100, Willy Tarreau wrote:
> On Tue, Jan 22, 2008 at 02:08:58PM -0700, Jordan Crouse wrote:
> > On 22/01/08 21:15 +0100, Willy Tarreau wrote:
> > > So it seems like applying the workaround on top of TinyBIOS 0.98 undoes
> > > this BIOS's workaround. I'm now wondering how we could detect whether
> > > the workaround was applied or not :-/
> >
> > Actually, the TinyBIOS "workaround" is the same thing.
> >
> > Like I may have said before, there is a reason why this MSR is undocumented.
> > It works, but the behavior is unvalidated, and obviously erratic. When we
> > first developed this code, we were using the Geode GX on the OLPC with VSA,
> > so using the MSR was nessesary if we wanted to get our hands on any
> > timers at all. Mitch Bradley (author of OpenFirmware) determined through
> > testing that the MSR was erratic, especially when you ran it when all the
> > timers were already clear. I suspect thats the problem that we see here -
> > writing the MSR in TinyBIOS unstablizied the registers, and writing the
> > MSR again kicked it back. Of course, the unfortunate corollary is that
> > when the workaround isn't in v0.99, if you run the MSR in the kernel, then
> > you will end up destabilizing everything.
>
> Indeed, that looks exactly like what's happening.
>
> > Furthermore, using the MSR is okay with TinyBIOS, but not okay with the
> > other Geode BIOSen (Insyde, General Software, and for the moment LinuxBIOS)
> > because VSA (the SMM handler) _does_ use some of the timers. So needless
> > to say, I'm concerned.
>
> I certainly can understand.
>
> > > Interestingly, during the boot, I got thousands of the following line :
> > > geode_mfgpt: reading 0x6200 + 0x6 + (0x0 * 8) = 0x6206
> > > It's a debug line I added in geode_mfgpt_read() which writes the timer and
> > > reg being read. It slowed the boot down due to being written to a serial
> > > console, but fortunately stopped when syslogd had changed the console log
> > > level. Just checking...
> >
> > We control the timer and the status bits for the timer in the setup
> > register, which is what you are showing above. Thats fine.
>
> OK.
>
> > > # grep mfgpt /proc/interrupts;sleep 10;grep mfgpt /proc/interrupts
> > > 7: 82320 XT-PIC-XT mfgpt-timer
> > > 7: 84882 XT-PIC-XT mfgpt-timer
> > >
> > > It delivers 256 IRQs/s. That makes me think about this comment in mfgpt_32.c :
> >
> > Hmm - are you running with nohz?
>
> No but I have CONFIG_HZ=250. That makes sense then :
>
> #define MFGPT_HZ (32000 / MFGPT_DIVISOR)
> #define MFGPT_PERIODIC (MFGPT_HZ / HZ)
>
> => MFGPT_PERIODIC = (32000 / 16) / 250 = 8
> Given that it's 32.768 kHz in fact, we get 32768/16/8 = 256 Hz
>
> > > * We are using the 32Khz input clock - its the only one that has the
> > > * ranges we find desirable. The following table lists the suitable
> > > * divisors and the associated hz, minimum interval
> > > * and the maximum interval:
> > > *
> > > * Divisor Hz Min Delta (S) Max Delta (S)
> > > * 1 32000 .0005 2.048
> > > * 2 16000 .001 4.096
> > > * 4 8000 .002 8.192
> > > ...
> > >
> > > When you say a 32kHz clock, you mean 32000 Hz. Are you really sure ? Most
> > > 32 kHz clocks everywhere are really 32768 Hz (the watch quartz). BTW, I'm
> > > seeing a 32.768 kHz xtal close to the CS5536, and the numbers above seem
> > > to support this suggestion too.
> >
> > Hmm - yeah, my math is off there - it is a 32768 Hz clock. That
> > shouldn't affect things too negatively - if it does we can adjust the
> > divisor we use - currently we use 16.
>
> Why not just ajust the constant in MFGPT_HZ since it's the only place (aside
> the comments) where this is referenced ?
>
> If you want I can send you a patch and adjust the comments at the same time.

That would be great.

> > > So right now that I've found what caused old kernel to unexpectedly work,
> > > I'm planning a BIOS upgrade. I'm still just wondering what we can do to
> > > detect that the workaround should be needed. I suspect nothing, of course,
> > > but just in case... Maybe we can detect the effects of the workaround ?
> >
> > I'm not sure if we can - all we can tell is if the registers are zero or
> > not. Like I said, running the MSR is probably dangerous in 9 out of 10
> > situations, the one good use being the one you determined. I would
> > support adding the mfgptfix bit though - just as long as it isn't
> > automagic.
>
> OK I perfectly understand. While I see the current behaviour as a regression
> compared to 2.6.22-mainline (since 2.6.24-rc8 does not boot off from this
> board without nomfgpt), the first problem is in the BIOS and it requires an
> upgrade. Maybe we should extend the scope of CONFIG_GEODE_MFGPT_TIMER to
> allow it to completely disable the detection logic when it's off ? I certainly
> can work a clean patch for "mfgptfix", but for users who will get caught with
> this board not booting at all anymore, adding an option in their boot loaders
> may be as problematic as upgrading the BIOS. I'm also thinking about the ones
> preparing new software updates from a recent boards and deploying them miles
> away without knowing they are running a 0.98 BIOS which will prevent their
> new kernel from booting.

I think not allowing MFGPT timers for broken BIOSen is the right way to go.
We can do just-in-time detection the first time one of the drivers (timer
tick or watchdog) asks for a timer, instead of the current way of probing
automatically.

That way, you can go merrily on your way if you don't try to use any of
the MFGPTs. TinyBIOS users can compile out CONFIG_GEODE_MFGPT_TIMER and
boot.

I'll write up a patch.

Thanks,
Jordan

--
Jordan Crouse
Systems Software Development Engineer
Advanced Micro Devices, Inc.

2008-01-23 16:41:16

[permalink] [raw]

Subject: Re: 2.6.24-rc8 hangs at mfgpt-timer

On Wed, Jan 23, 2008 at 09:36:46AM -0700, Jordan Crouse wrote:
> > Why not just ajust the constant in MFGPT_HZ since it's the only place (aside
> > the comments) where this is referenced ?
> >
> > If you want I can send you a patch and adjust the comments at the same time.
>
> That would be great.

OK will do ASAP.

> I think not allowing MFGPT timers for broken BIOSen is the right way to go.
> We can do just-in-time detection the first time one of the drivers (timer
> tick or watchdog) asks for a timer, instead of the current way of probing
> automatically.
>
> That way, you can go merrily on your way if you don't try to use any of
> the MFGPTs. TinyBIOS users can compile out CONFIG_GEODE_MFGPT_TIMER and
> boot.

I like this method a lot. At least, people with broken bioses would have
trouble only if they modprobe geodewdt or do such things. They will easily
find the relation between their action and the problem. Right now, we only
know that it hangs after "NET: ...".

> I'll write up a patch.

Fine.

Thanks,
Willy

2008-01-23 21:48:47

[permalink] [raw]

Subject: [PATCH 0/2] Was: 2.6.24-rc8 hangs at mfgpt-timer

On Tue, Jan 22, 2008 at 10:10:14PM +0100, Ingo Molnar wrote:
>
> * Arnd Hannemann <[email protected]> wrote:
>
> > > The attached patch rearranges the code so that the handler is
> > > installed before we setup the interrupt (so we have somebody to
> > > listen to the immediate interrupt), and it makes sure that we clear
> > > the event in the IRQ handler regardless of the state of the timer
> > > tick.
> >
> > This patch indeed solves the problem. The board boots fine. Great
> > work!
>
> since this driver is new in 2.6.24, perhaps we should apply this fix to
> v2.6.24?

Ingo, Jordan,

I have written the two minor patches we were talking about in previous
mail. The first one fixes the input clock from 32000 to 32768 Hz, and
the second one adds an "mfgptfix" boot parameter to enable the workaround
in order to fix a regression on motherboards with a broken BIOS on which
2.6.24-rc8 does not boot anymore whether mfgpt timers are enabled or not.

Jordan, if you think you'll push your changes for 2.6.24, feel free to
merge this patch with your work.

Both patches will be sent as a reply to this mail.

Regards,
Willy

2008-01-23 21:49:55

[permalink] [raw]

Subject: [PATCH 1/2] x86: GEODE fix MFGPT input clock value

The GEODE MFGPT code assumed that 32kHz was 32000 Hz while the boards
run on a 32.768 kHz digital watch crystal. In practise, it will not
change the timer's frequency as the skew was only 2.4%, but it
should provide more accurate intervals.

Signed-off-by: Willy Tarreau <[email protected]>
---
arch/x86/kernel/mfgpt_32.c | 24 ++++++++++++------------
1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/mfgpt_32.c b/arch/x86/kernel/mfgpt_32.c
index 3960ab7..5519091 100644
--- a/arch/x86/kernel/mfgpt_32.c
+++ b/arch/x86/kernel/mfgpt_32.c
@@ -12,21 +12,21 @@
*/

/*
- * We are using the 32Khz input clock - its the only one that has the
+ * We are using the 32.768Khz input clock - its the only one that has the
* ranges we find desirable. The following table lists the suitable
* divisors and the associated hz, minimum interval
* and the maximum interval:
*
- * Divisor Hz Min Delta (S) Max Delta (S)
- * 1 32000 .0005 2.048
- * 2 16000 .001 4.096
- * 4 8000 .002 8.192
- * 8 4000 .004 16.384
- * 16 2000 .008 32.768
- * 32 1000 .016 65.536
- * 64 500 .032 131.072
- * 128 250 .064 262.144
- * 256 125 .128 524.288
+ * Divisor Hz Min Delta (S) Max Delta (S)
+ * 1 32768 .00048828125 2.000
+ * 2 16384 .0009765625 4.000
+ * 4 8192 .001953125 8.000
+ * 8 4096 .00390625 16.000
+ * 16 2048 .0078125 32.000
+ * 32 1024 .015625 64.000
+ * 64 512 .03125 128.000
+ * 128 256 .0625 256.000
+ * 256 128 .125 512.000
*/

#include <linux/kernel.h>
@@ -45,7 +45,7 @@ static struct mfgpt_timer_t {

#define MFGPT_DIVISOR 16
#define MFGPT_SCALE 4 /* divisor = 2^(scale) */
-#define MFGPT_HZ (32000 / MFGPT_DIVISOR)
+#define MFGPT_HZ (32768 / MFGPT_DIVISOR)
#define MFGPT_PERIODIC (MFGPT_HZ / HZ)

#ifdef CONFIG_GEODE_MFGPT_TIMER
--
1.5.3.4

2008-01-23 21:51:47

[permalink] [raw]

Subject: [PATCH 2/2] x86: GEODE add the "mfgptfix" boot time option to fix MFGPT timers

The new "mfgptfix" boot command line option may be usd to fix MFGPT
timers on AMD Geode platforms when the BIOS has incorrectly applied
a workaround. TinyBIOS version 0.98 is known to be affected, 0.99
fixes the problem by letting the user disable the workaround.

Signed-off-by: Willy Tarreau <[email protected]>
---
Documentation/kernel-parameters.txt | 5 +++++
arch/x86/kernel/mfgpt_32.c | 15 +++++++++++++++
2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index c417877..83c6704 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1051,6 +1051,11 @@ and is between 256 and 4096 characters. It is defined in the file
Multi-Function General Purpose Timers on AMD Geode
platforms.

+ mfgptfix [X86-32] Fix MFGPT timers on AMD Geode platforms when
+ the BIOS has incorrectly applied a workaround. TinyBIOS
+ version 0.98 is known to be affected, 0.99 fixes the
+ problem by letting the user disable the workaround.
+
mga= [HW,DRM]

mousedev.tap_time=
diff --git a/arch/x86/kernel/mfgpt_32.c b/arch/x86/kernel/mfgpt_32.c
index 5519091..f38d4a9 100644
--- a/arch/x86/kernel/mfgpt_32.c
+++ b/arch/x86/kernel/mfgpt_32.c
@@ -63,6 +63,21 @@ static int __init mfgpt_disable(char *s)
}
__setup("nomfgpt", mfgpt_disable);

+/* Reset the MFGPT timers. This is required by some broken BIOSes which already
+ * do the same and leave the system in an unstable state. TinyBIOS 0.98 is
+ * affected at least (0.99 is OK with MFGPT workaround left to off).
+ */
+static int __init mfgpt_fix(char *s)
+{
+ u32 val, dummy;
+
+ /* The following udocumented bit resets the MFGPT timers */
+ val = 0xFF; dummy = 0;
+ wrmsr(0x5140002B, val, dummy);
+ return 1;
+}
+__setup("mfgptfix", mfgpt_fix);
+
/*
* Check whether any MFGPTs are available for the kernel to use. In most
* cases, firmware that uses AMD's VSA code will claim all timers during
--
1.5.3.4

2008-01-23 22:00:29

by H. Peter Anvin

[permalink] [raw]

Subject: Re: [PATCH 1/2] x86: GEODE fix MFGPT input clock value

Willy Tarreau wrote:
> The GEODE MFGPT code assumed that 32kHz was 32000 Hz while the boards
> run on a 32.768 kHz digital watch crystal. In practise, it will not
> change the timer's frequency as the skew was only 2.4%, but it
> should provide more accurate intervals.
> - * Divisor Hz Min Delta (S) Max Delta (S)

Seconds are "s", not "S" (S = siemens.)

-hpa

2008-01-23 22:12:06

[permalink] [raw]

Subject: Re: [PATCH 1/2] x86: GEODE fix MFGPT input clock value

Hi Peter,

On Wed, Jan 23, 2008 at 01:59:32PM -0800, H. Peter Anvin wrote:
> Willy Tarreau wrote:
> >The GEODE MFGPT code assumed that 32kHz was 32000 Hz while the boards
> >run on a 32.768 kHz digital watch crystal. In practise, it will not
> >change the timer's frequency as the skew was only 2.4%, but it
> >should provide more accurate intervals.
> >- * Divisor Hz Min Delta (S) Max Delta (S)
>
> Seconds are "s", not "S" (S = siemens.)

You're quite right. Same as we should write kHz and not Khz. But I'm
used to change other people's work and particularly comments the least
possible. Do you want an update ?

Willy

2008-01-23 22:23:35

by H. Peter Anvin

[permalink] [raw]

Subject: Re: [PATCH 1/2] x86: GEODE fix MFGPT input clock value

Willy Tarreau wrote:
> Hi Peter,
>
> On Wed, Jan 23, 2008 at 01:59:32PM -0800, H. Peter Anvin wrote:
>> Willy Tarreau wrote:
>>> The GEODE MFGPT code assumed that 32kHz was 32000 Hz while the boards
>>> run on a 32.768 kHz digital watch crystal. In practise, it will not
>>> change the timer's frequency as the skew was only 2.4%, but it
>>> should provide more accurate intervals.
>>> - * Divisor Hz Min Delta (S) Max Delta (S)
>> Seconds are "s", not "S" (S = siemens.)
>
> You're quite right. Same as we should write kHz and not Khz. But I'm
> used to change other people's work and particularly comments the least
> possible. Do you want an update ?
>

I much prefer to see this done right. I have a pretty case-sensitive
brain, it seems.

-hpa

2008-01-23 22:37:41

[permalink] [raw]

Subject: Re: x86: GEODE fix MFGPT input clock value

On 23/01/08 23:11 +0100, Willy Tarreau wrote:
> Hi Peter,
>
> On Wed, Jan 23, 2008 at 01:59:32PM -0800, H. Peter Anvin wrote:
> > Willy Tarreau wrote:
> > >The GEODE MFGPT code assumed that 32kHz was 32000 Hz while the boards
> > >run on a 32.768 kHz digital watch crystal. In practise, it will not
> > >change the timer's frequency as the skew was only 2.4%, but it
> > >should provide more accurate intervals.
> > >- * Divisor Hz Min Delta (S) Max Delta (S)
> >
> > Seconds are "s", not "S" (S = siemens.)
>
> You're quite right. Same as we should write kHz and not Khz. But I'm
> used to change other people's work and particularly comments the least
> possible. Do you want an update ?

Please do - I don't mind if you change the comments - better that they
are right.

Jordan

--
Jordan Crouse
Systems Software Development Engineer
Advanced Micro Devices, Inc.

2008-01-23 22:42:13

[permalink] [raw]

Subject: Re: [PATCH 1/2] x86: GEODE fix MFGPT input clock value

On Wed, Jan 23, 2008 at 02:22:59PM -0800, H. Peter Anvin wrote:
> Willy Tarreau wrote:
> >Hi Peter,
> >
> >On Wed, Jan 23, 2008 at 01:59:32PM -0800, H. Peter Anvin wrote:
> >>Willy Tarreau wrote:
> >>>The GEODE MFGPT code assumed that 32kHz was 32000 Hz while the boards
> >>>run on a 32.768 kHz digital watch crystal. In practise, it will not
> >>>change the timer's frequency as the skew was only 2.4%, but it
> >>>should provide more accurate intervals.
> >>>- * Divisor Hz Min Delta (S) Max Delta (S)
> >>Seconds are "s", not "S" (S = siemens.)
> >
> >You're quite right. Same as we should write kHz and not Khz. But I'm
> >used to change other people's work and particularly comments the least
> >possible. Do you want an update ?
> >
>
> I much prefer to see this done right. I have a pretty case-sensitive
> brain, it seems.

Oh rest assured that you're not alone. I was smiling reading Andrew's
comments about people who "cnat tpye", and I must admit that I too am
often quite irritated by incorrect case and inverted letters, but I try
to refrain from whining, people say that I do it too much and for nothing :-)

OK, here it comes updated.

Willy

>From 3a314dd5c2a694f5b0a1c1b8b5690ee28f711b5e Mon Sep 17 00:00:00 2001
From: Willy Tarreau <[email protected]>
Date: Wed, 23 Jan 2008 23:05:50 +0100
Subject: [PATCH 1/2] x86: GEODE fix MFGPT input clock value

The GEODE MFGPT code assumed that 32kHz was 32000 Hz while the boards
run on a 32.768 kHz digital watch crystal. In practise, it will not
change the timer's frequency as the skew was only 2.4%, but it
should provide more accurate intervals.

Signed-off-by: Willy Tarreau <[email protected]>
---
arch/x86/kernel/mfgpt_32.c | 27 +++++++++++++--------------
1 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/mfgpt_32.c b/arch/x86/kernel/mfgpt_32.c
index 3960ab7..f97e6e3 100644
--- a/arch/x86/kernel/mfgpt_32.c
+++ b/arch/x86/kernel/mfgpt_32.c
@@ -12,21 +12,20 @@
*/

/*
- * We are using the 32Khz input clock - its the only one that has the
+ * We are using the 32.768kHz input clock - its the only one that has the
* ranges we find desirable. The following table lists the suitable
- * divisors and the associated hz, minimum interval
- * and the maximum interval:
+ * divisors and the associated Hz, minimum interval and the maximum interval:
*
- * Divisor Hz Min Delta (S) Max Delta (S)
- * 1 32000 .0005 2.048
- * 2 16000 .001 4.096
- * 4 8000 .002 8.192
- * 8 4000 .004 16.384
- * 16 2000 .008 32.768
- * 32 1000 .016 65.536
- * 64 500 .032 131.072
- * 128 250 .064 262.144
- * 256 125 .128 524.288
+ * Divisor Hz Min Delta (s) Max Delta (s)
+ * 1 32768 .00048828125 2.000
+ * 2 16384 .0009765625 4.000
+ * 4 8192 .001953125 8.000
+ * 8 4096 .00390625 16.000
+ * 16 2048 .0078125 32.000
+ * 32 1024 .015625 64.000
+ * 64 512 .03125 128.000
+ * 128 256 .0625 256.000
+ * 256 128 .125 512.000
*/

#include <linux/kernel.h>
@@ -45,7 +44,7 @@ static struct mfgpt_timer_t {

#define MFGPT_DIVISOR 16
#define MFGPT_SCALE 4 /* divisor = 2^(scale) */
-#define MFGPT_HZ (32000 / MFGPT_DIVISOR)
+#define MFGPT_HZ (32768 / MFGPT_DIVISOR)
#define MFGPT_PERIODIC (MFGPT_HZ / HZ)

#ifdef CONFIG_GEODE_MFGPT_TIMER
--
1.5.3.3

2008-01-23 23:17:22

[permalink] [raw]

Subject: Re: x86: GEODE fix MFGPT input clock value

Hi,

Jordan Crouse schrieb:
> On 23/01/08 23:11 +0100, Willy Tarreau wrote:
>> Hi Peter,
>>
>> On Wed, Jan 23, 2008 at 01:59:32PM -0800, H. Peter Anvin wrote:
>>> Willy Tarreau wrote:
>>>> The GEODE MFGPT code assumed that 32kHz was 32000 Hz while the boards
>>>> run on a 32.768 kHz digital watch crystal. In practise, it will not
>>>> change the timer's frequency as the skew was only 2.4%, but it
>>>> should provide more accurate intervals.
>>>> - * Divisor Hz Min Delta (S) Max Delta (S)
>>> Seconds are "s", not "S" (S = siemens.)
>> You're quite right. Same as we should write kHz and not Khz. But I'm
>> used to change other people's work and particularly comments the least
>> possible. Do you want an update ?
>
> Please do - I don't mind if you change the comments - better that they
> are right.

While you are on it, please include this one:

--- a/arch/x86/kernel/mfgpt_32.c 2008-01-24 00:08:20.000000000 +0100
+++ b/arch/x86/kernel/mfgpt_32.c 2008-01-24 00:05:17.000000000 +0100
@@ -361,7 +361,7 @@
&mfgpt_clockevent);

printk(KERN_INFO
- "mfgpt-timer: registering the MFGT timer as a clock event.\n");
+ "mfgpt-timer: registering the MFGPT timer as a clock event.\n");
clockevents_register_device(&mfgpt_clockevent);

return 0;

> Jordan
>

Arnd

2008-02-17 14:21:17

by Iain Paton

[permalink] [raw]

Subject: Re: Geode GX/LX watchdog timer (RESEND)

Arnd Hannemann wrote:

> I can confirm that it is still working fine :-)

Hi,

Has anyone managed to build this as a module against the full 2.6.24
release ?

I am seeing the following error:

CC [M] lib/zlib_inflate/infutil.o
CC [M] lib/zlib_inflate/inftrees.o
CC [M] lib/zlib_inflate/inflate_syms.o
LD [M] lib/zlib_inflate/zlib_inflate.o
Building modules, stage 2.
MODPOST 251 modules
ERROR: "geode_mfgpt_toggle_event" [drivers/watchdog/geodewdt.ko] undefined!
ERROR: "geode_mfgpt_alloc_timer" [drivers/watchdog/geodewdt.ko] undefined!
make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

arch/x86/kernel/mfgpt_32.c where these are defined seems to have been
built ok and they appear in System.map.

The only other thing I've done is to add "[PATCH] pata_cs5536 Fix
secondary port configuration" recently posted by Martin K. Petersen but
I don't believe that would cause this.

.config is available at http://pastebin.ca/907299

Iain

2008-02-17 14:46:19

[permalink] [raw]

Subject: Re: Geode GX/LX watchdog timer (RESEND)

Hi,

Iain Paton wrote:
> Arnd Hannemann wrote:
>
>> I can confirm that it is still working fine :-)
>
> Hi,
>
> Has anyone managed to build this as a module against the full 2.6.24
> release ?
>
> I am seeing the following error:
>
> CC [M] lib/zlib_inflate/infutil.o
> CC [M] lib/zlib_inflate/inftrees.o
> CC [M] lib/zlib_inflate/inflate_syms.o
> LD [M] lib/zlib_inflate/zlib_inflate.o
> Building modules, stage 2.
> MODPOST 251 modules
> ERROR: "geode_mfgpt_toggle_event" [drivers/watchdog/geodewdt.ko] undefined!
> ERROR: "geode_mfgpt_alloc_timer" [drivers/watchdog/geodewdt.ko] undefined!
> make[1]: *** [__modpost] Error 1
> make: *** [modules] Error 2
>
>
> arch/x86/kernel/mfgpt_32.c where these are defined seems to have been
> built ok and they appear in System.map.
>
> The only other thing I've done is to add "[PATCH] pata_cs5536 Fix
> secondary port configuration" recently posted by Martin K. Petersen but
> I don't believe that would cause this.
>
> .config is available at http://pastebin.ca/907299

Never tried to built it as a module.
Probably there are issues with that. If I remember correctly I saw a patch in 2.6.25-rc which
mentioned that using mfgpt in modules won't work. Does this apply to 2.6.24 as well?

Best regards,
Arnd Hannemann

2008-02-17 14:55:28

by Adrian Bunk

[permalink] [raw]

Subject: Re: Geode GX/LX watchdog timer (RESEND)

On Sun, Feb 17, 2008 at 03:46:01PM +0100, Arnd Hannemann wrote:
> Hi,
>
> Iain Paton wrote:
> > Arnd Hannemann wrote:
> >
> >> I can confirm that it is still working fine :-)
> >
> > Hi,
> >
> > Has anyone managed to build this as a module against the full 2.6.24
> > release ?
> >
> > I am seeing the following error:
> >
> > CC [M] lib/zlib_inflate/infutil.o
> > CC [M] lib/zlib_inflate/inftrees.o
> > CC [M] lib/zlib_inflate/inflate_syms.o
> > LD [M] lib/zlib_inflate/zlib_inflate.o
> > Building modules, stage 2.
> > MODPOST 251 modules
> > ERROR: "geode_mfgpt_toggle_event" [drivers/watchdog/geodewdt.ko] undefined!
> > ERROR: "geode_mfgpt_alloc_timer" [drivers/watchdog/geodewdt.ko] undefined!
> > make[1]: *** [__modpost] Error 1
> > make: *** [modules] Error 2
> >
> >
> > arch/x86/kernel/mfgpt_32.c where these are defined seems to have been
> > built ok and they appear in System.map.
> >
> > The only other thing I've done is to add "[PATCH] pata_cs5536 Fix
> > secondary port configuration" recently posted by Martin K. Petersen but
> > I don't believe that would cause this.
> >
> > .config is available at http://pastebin.ca/907299
>
> Never tried to built it as a module.
> Probably there are issues with that. If I remember correctly I saw a patch in 2.6.25-rc which
> mentioned that using mfgpt in modules won't work. Does this apply to 2.6.24 as well?

For using code from modules it must be explicitely EXPORT_SYMBOL{,GPL}'ed.

Adding
EXPORT_SYMBOL_GPL(geode_mfgpt_toggle_event);
and
EXPORT_SYMBOL_GPL(geode_mfgpt_alloc_timer);
below the respective functions in mfgpt_32.c should fix this issue.

> Best regards,
> Arnd Hannemann

cu
Adrian

--

"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed

2008-02-17 16:10:27

by Iain Paton

[permalink] [raw]

Subject: Re: Geode GX/LX watchdog timer (RESEND)

Adrian Bunk wrote:
> On Sun, Feb 17, 2008 at 03:46:01PM +0100, Arnd Hannemann wrote:
>> Never tried to built it as a module.
>> Probably there are issues with that. If I remember correctly I saw a patch in 2.6.25-rc which
>> mentioned that using mfgpt in modules won't work. Does this apply to 2.6.24 as well?
>
> For using code from modules it must be explicitely EXPORT_SYMBOL{,GPL}'ed.
>
> Adding
> EXPORT_SYMBOL_GPL(geode_mfgpt_toggle_event);
> and
> EXPORT_SYMBOL_GPL(geode_mfgpt_alloc_timer);
> below the respective functions in mfgpt_32.c should fix this issue.

I couldn't find the patch Arnd mentioned on lkml or in Linus git tree,
but adding the lines suggested by Adrian gets me a working module.

Hopefully the patch will arrive in the mainline tree at some point.

Thanks to both of you for the help!

Iain

2008-02-17 17:30:23

[permalink] [raw]

Subject: Re: Geode GX/LX watchdog timer (RESEND)

On Sun, 17 Feb 2008 16:10:09 +0000
Iain Paton <[email protected]> wrote:

> Adrian Bunk wrote:
> > On Sun, Feb 17, 2008 at 03:46:01PM +0100, Arnd Hannemann wrote:
> >> Never tried to built it as a module.
> >> Probably there are issues with that. If I remember correctly I saw a patch in 2.6.25-rc which
> >> mentioned that using mfgpt in modules won't work. Does this apply to 2.6.24 as well?
> >
> > For using code from modules it must be explicitely EXPORT_SYMBOL{,GPL}'ed.
> >
> > Adding
> > EXPORT_SYMBOL_GPL(geode_mfgpt_toggle_event);
> > and
> > EXPORT_SYMBOL_GPL(geode_mfgpt_alloc_timer);
> > below the respective functions in mfgpt_32.c should fix this issue.
>
> I couldn't find the patch Arnd mentioned on lkml or in Linus git tree,
> but adding the lines suggested by Adrian gets me a working module.
>
> Hopefully the patch will arrive in the mainline tree at some point.
>

This was originally split out into two separate patches; one that exported
the proper symbols, and the other containing the watchdog timer. I merged
them in the geode tree. The patch is here:

http://git.infradead.org/?p=geode.git;a=commitdiff;h=5a840828ddb5bb7381435509a9460e0ba4aab550

That's also checkpatch.pl happy (or at least, it was when I committed it).

2008-02-17 19:47:22