m8xx_wdt: software watchdog reset/interrupt select

Hi,

Currently the mpc8xx_wdt driver installs an IRQ handler for PIT_INTERRUPT (SIU_LEVEL0, irq 1) to service the WDT until a userspace watchdog daemon takes over after boot.

However, if the "software watchdog reset/interrupt select" (SWRI) bit of SYPCR register is set, no interrupt is generated. In that configuration (the default) HRESET signal is generated if the WDT timeout expires, without kernel notification.

The following patch creates a kernel timer to service the WDT and rearm itself in case this configuration is detected, making it possible to boot the system with the watchdog turned on. The timer is shutdown once the userspace daemon open's the device.

Note: From my reading of the documentation, even if the SWRI bit is unset (interrupt select mode), an NMI at IRQ0 should cause the system to jump to exception vector 0x100, resetting the system.

So I'm wondering if the interrupt mode ever worked?

--- ../git/linux-2.6/arch/ppc/syslib/m8xx_wdt.c 2005-11-08
11:38:39.000000000 -0600
+++ linux-2.6-git-wednov02/arch/ppc/syslib/m8xx_wdt.c 2005-11-14
10:36:53.000000000 -0600

arch/ppc/syslib/m8xx_wdt.c renamed from git/linux-2.6/arch/ppc/syslib/m8xx_wdt.c +68 -26

@@ -45,35 +45,18 @@
 	return IRQ_HANDLED;
 }
 
-void __init m8xx_wdt_handler_install(bd_t * binfo)
+#define SYPCR_SWP 0x1
+#define SYPCR_SWRI 0x2
+#define SYPCR_SWE 0x4
+
+/* software watchdog reset/interrupt select */
+int m8xx_wdt_keepalive_mode = 0;
+
+void __init m8xx_wdt_install_irq(volatile immap_t *imap, bd_t *binfo)
 {
-	volatile immap_t *imap = (volatile immap_t *)IMAP_ADDR;
 	u32 pitc;
-	u32 sypcr;
 	u32 pitrtclk;
 
-	sypcr = in_be32(&imap->im_siu_conf.sc_sypcr);
-
-	if (!(sypcr & 0x04)) {
-		printk(KERN_NOTICE "m8xx_wdt: wdt disabled (SYPCR: 0x%08X)\n",
-		       sypcr);
-		return;
-	}
-
-	m8xx_wdt_reset();
-
-	printk(KERN_NOTICE
-	       "m8xx_wdt: active wdt found (SWTC: 0x%04X, SWP: 0x%01X)\n",
-	       (sypcr >> 16), sypcr & 0x01);
-
-	wdt_timeout = (sypcr >> 16) & 0xFFFF;
-
-	if (!wdt_timeout)
-		wdt_timeout = 0xFFFF;
-
-	if (sypcr & 0x01)
-		wdt_timeout *= 2048;
-
 	/*
 	 * Fire trigger if half of the wdt ticked down
 	 */

@@ -98,6 +81,66 @@
 	printk(KERN_NOTICE
 	       "m8xx_wdt: keep-alive trigger installed (PITC: 0x%04X)\n",  
 pitc);
 
+}
+
+static void m8xx_wdt_timer_func(unsigned long data);
+
+static struct timer_list m8xx_wdt_timer =
+	TIMER_INITIALIZER(m8xx_wdt_timer_func, 0, 0);
+
+void m8xx_wdt_stop_timer(void)
+{
+	del_timer(&m8xx_wdt_timer);
+}
+
+static void m8xx_wdt_timer_func(unsigned long data)
+{
+	m8xx_wdt_reset();
+	m8xx_wdt_timer.expires = jiffies + 25;
+	add_timer(&m8xx_wdt_timer);
+}
+
+void m8xx_wdt_install_timer(volatile immap_t *imap)
+{
+	m8xx_wdt_timer.expires = jiffies + 25;
+	add_timer(&m8xx_wdt_timer);
+}
+
+void __init m8xx_wdt_handler_install(bd_t * binfo)
+{
+	volatile immap_t *imap = (volatile immap_t *)IMAP_ADDR;
+	u32 sypcr;
+
+	sypcr = in_be32(&imap->im_siu_conf.sc_sypcr);
+
+	printk(KERN_NOTICE "m8xx_wdt SYPCR: 0x%08X)\n", sypcr);
+
+	if (!(sypcr & SYPCR_SWE)) {
+		printk(KERN_NOTICE "m8xx_wdt: wdt disabled (SYPCR: 0x%08X)\n",
+		       sypcr);
+		return;
+	}
+
+	m8xx_wdt_reset();
+
+	printk(KERN_NOTICE
+	       "m8xx_wdt: active wdt found (SWTC: 0x%04X, SWP: 0x%01X)\n",
+	       (sypcr >> 16), sypcr & SYPCR_SWP);
+
+	wdt_timeout = (sypcr >> 16) & 0xFFFF;
+
+	if (!wdt_timeout)
+		wdt_timeout = 0xFFFF;
+
+	if (sypcr & SYPCR_SWP)
+		wdt_timeout *= 2048;
+
+	m8xx_wdt_keepalive_mode = sypcr & SYPCR_SWRI;
+	if (m8xx_wdt_keepalive_mode)
+		m8xx_wdt_install_timer(imap);
+	else
+		m8xx_wdt_install_irq(imap, binfo);
+
 	wdt_timeout /= binfo->bi_intfreq;
 }
 
--- ../git/linux-2.6/arch/ppc/syslib/m8xx_wdt.h	2005-10-10  

18:06:12.000000000 -0500
+++ linux-2.6-git-wednov02/arch/ppc/syslib/m8xx_wdt.h 2005-11-14
10:37:39.000000000 -0600

arch/ppc/syslib/m8xx_wdt.h renamed from git/linux-2.6/arch/ppc/syslib/m8xx_wdt.h +4 -1

@@ -9,8 +9,12 @@
 #ifndef _PPC_SYSLIB_M8XX_WDT_H
 #define _PPC_SYSLIB_M8XX_WDT_H
 
+extern int m8xx_wdt_keepalive_mode;
+
 extern void m8xx_wdt_handler_install(bd_t * binfo);
 extern int m8xx_wdt_get_timeout(void);
 extern void m8xx_wdt_reset(void);
+extern void m8xx_wdt_install_timer(volatile immap_t *imap);
+extern void m8xx_wdt_stop_timer(void);
 
 #endif				/* _PPC_SYSLIB_M8XX_WDT_H */
--- ../git/linux-2.6/drivers/char/watchdog/mpc8xx_wdt.c	2005-10-10  

18:06:15.000000000 -0500
+++ linux-2.6-git-wednov02/drivers/char/watchdog/mpc8xx_wdt.c

2005-11-14 10:37:15.000000000 -0600

drivers/char/watchdog/mpc8xx_wdt.c renamed from git/linux-2.6/drivers/char/watchdog/mpc8xx_wdt.c +8 -2

@@ -27,7 +27,10 @@
 {
 	volatile immap_t *imap = (volatile immap_t *)IMAP_ADDR;
 
-	imap->im_sit.sit_piscr &= ~(PISCR_PIE | PISCR_PTE);
+	if (m8xx_wdt_keepalive_mode)
+		m8xx_wdt_stop_timer();
+	else
+		imap->im_sit.sit_piscr &= ~(PISCR_PIE | PISCR_PTE);
 
 	printk(KERN_NOTICE "mpc8xx_wdt: keep-alive handler deactivated\n");
 }

@@ -36,7 +39,10 @@
 {
 	volatile immap_t *imap = (volatile immap_t *)IMAP_ADDR;
 
-	imap->im_sit.sit_piscr |= PISCR_PIE | PISCR_PTE;
+	if (m8xx_wdt_keepalive_mode)
+		m8xx_wdt_install_timer(imap);
+	else
+		imap->im_sit.sit_piscr |= PISCR_PIE | PISCR_PTE;
 
 	printk(KERN_NOTICE "mpc8xx_wdt: keep-alive handler activated\n");
 }

_______________________________________________
Linuxppc-embedded mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-embedded

Can we put the WDT for 8xx in drivers/char/watchdog/?

Hi Kumar,

Quoted 1 line · Kumar Gala

Can we put the WDT for 8xx in drivers/char/watchdog/?

The userspace interface is already there at drivers/char/watchdog/mpc8xx_wdt.c. I don't any reason for having two files, will try to merge them into drivers/.

Quoted 8 lines · Marcelo Tosatti

Hi Kumar,

Quoted 1 line · Kumar Gala

Can we put the WDT for 8xx in drivers/char/watchdog/?

The userspace interface is already there at drivers/char/watchdog/mpc8xx_wdt.c. I don't any reason for having two files, will try to merge them into drivers/.

Actually, drivers/char/watchdog/mpc8xx_wdt.c is a module, while some of the initialization code must reside in the kernel image.

Thats the reason for the split - not sure if merging the two files is worth?

Actually, drivers/char/watchdog/mpc8xx_wdt.c is a module, while some of the initialization code must reside in the kernel image.

Thats the reason for the split - not sure if merging the two files is worth?

Note: From my reading of the documentation, even if the SWRI bit is unset (interrupt select mode), an NMI at IRQ0 should cause the system to jump to exception vector 0x100, resetting the system.

So I'm wondering if the interrupt mode ever worked?

Hi,

Quoted 5 lines

Note: From my reading of the documentation, even if the SWRI bit is unset (interrupt select mode), an NMI at IRQ0 should cause the system to jump to exception vector 0x100, resetting the system.

So I'm wondering if the interrupt mode ever worked?

Never tried the interrupt mode. IMHO doesn't make much sense for a watchdog. Why don't you simply set SWRI?

Quoted 8 lines

arch/ppc/syslib/m8xx_wdt.c renamed from git/linux-2.6/arch/ppc/syslib/m8xx_wdt.c +8 -0

+static void m8xx_wdt_timer_func(unsigned long data)
+{
+	m8xx_wdt_reset();
+	m8xx_wdt_timer.expires = jiffies + 25;
+	add_timer(&m8xx_wdt_timer);
+}
+
+void m8xx_wdt_install_timer(volatile immap_t *imap)

arch/ppc/syslib/m8xx_wdt.c line 101

Quoted 4 lines

arch/ppc/syslib/m8xx_wdt.c renamed from git/linux-2.6/arch/ppc/syslib/m8xx_wdt.c +4 -0

+{
+	m8xx_wdt_timer.expires = jiffies + 25;
+	add_timer(&m8xx_wdt_timer);
+}

m8xx_wdt_install_timer doesn't need imap and you could call it from m8xx_wdt_timer_func to re-engange the timer resulting in less duplicated code. Just a cosmetic thing though.

Hi Florian!

Quoted 10 lines · Florian Schirmer

Hi,

Quoted 5 lines

Note: From my reading of the documentation, even if the SWRI bit is unset (interrupt select mode), an NMI at IRQ0 should cause the system to jump to exception vector 0x100, resetting the system.

So I'm wondering if the interrupt mode ever worked?

Never tried the interrupt mode. IMHO doesn't make much sense for a watchdog. Why don't you simply set SWRI?

The SYPCR register can be set only _once_ at machine startup and the bootloader in question does not have an option to change the mode. Many bootloaders probably dont.

Quoted 8 lines

Quoted 8 lines

(quoted diff) +8 -0

+static void m8xx_wdt_timer_func(unsigned long data)
+{
+	m8xx_wdt_reset();
+	m8xx_wdt_timer.expires = jiffies + 25;
+	add_timer(&m8xx_wdt_timer);
+}
+
+void m8xx_wdt_install_timer(volatile immap_t *imap)

Quoted 8 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+{
+	m8xx_wdt_timer.expires = jiffies + 25;
+	add_timer(&m8xx_wdt_timer);
+}

m8xx_wdt_install_timer doesn't need imap and you could call it from m8xx_wdt_timer_func to re-engange the timer resulting in less duplicated code. Just a cosmetic thing though.

Indeed. Will update the patch and resend you for review.

Quoted 3 lines

The SYPCR register can be set only _once_ at machine startup and the bootloader in question does not have an option to change the mode. Many bootloaders probably dont.

Updated patch addresses code duplication issue you mentioned and also adds an error message in case timer interrupt frequency is higher than the watchdog frequency.

Can I add your Signed-off-by in case you're OK with it?

Hi,

Quoted 3 lines

Quoted 3 lines

The SYPCR register can be set only _once_ at machine startup and the bootloader in question does not have an option to change the mode. Many bootloaders probably dont.

Okay, i was asuming you have control over the bootloader.

Quoted 4 lines

Updated patch addresses code duplication issue you mentioned and also adds an error message in case timer interrupt frequency is higher than the watchdog frequency.

Quoted 1 line

Can I add your Signed-off-by in case you're OK with it?

Maybe i'm missing something obvious, but why is the interrupt driven reaming code not working for your configuration. Are you using the PIT for something else?

Otherwise i'm fine with the patch. Feel free to add my Signed-off-by line.

Anyway, the SWRI bit selects interrupt (0) or reset mode (1) for the watchdog.

On reset mode no interrupt is sent to the kernel - the watchdog logic resets the system with HRESET.

So, the timer in m8xx_wdt is _required_ for reset mode.

Does that make sense?

Quoted 1 line

Otherwise i'm fine with the patch. Feel free to add my Signed-off-by line.

Ok, lets sort this out first.

I wonder how interrupt mode is supposed to work, because the manual states that in interrupt mode (SWRI == 0) an NMI (IRQ0) is triggered, which jumps to 0x100 exception vector (SW reset).

Maybe I'm misunderstanding the interrupt mode?

Folks who wrote the patch claim it works on their 8xx's (as can be found on mailing list archives).

Hi,

okay here is what the current driver does:

During startup it installs a timer irq (PIT) handler and sets the frequency to half of the watchdog timeout. As soon as this timer irq triggers we reset the watchdog inside the irq handler.

If a userspace handler takes over the watchdog we deinstall the timer irq handler and let the userspace daemon handle the watchdog resets.

Please not that we're talking about the timer irq, not the watchdog interrupt.

I don't see why it should make a difference wether the watchdog generates a IRQ0/HRESET or a system reset directly since it should never trigger anyway.

All the patch does is to use a kernel timer instead of the hardware timer. So i'm little confused.

After your message I decided to check if the PIT is working, guess what: it isnt.

Our board uses an external RTC (the internal RTC/PIT HW seem to be nonexistant/nonfunctional).