[PATCH] m8xx_wdt: software watchdog reset/interrupt select

[PATCH] m8xx_wdt: software watchdog reset/interrupt select

[Marcelo Tosatti]

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
@@ -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
@@ -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
@@ -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");
 }

Re: [PATCH] m8xx_wdt: software watchdog reset/interrupt select

[Kumar Gala]

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

- kumar

On Nov 14, 2005, at 8:38 AM, Marcelo Tosatti wrote:

> 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
> @@ -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
> @@ -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
> @@ -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
> Linuxppc-embedded@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-embedded

Re: [PATCH] m8xx_wdt: software watchdog reset/interrupt select\

[Marcelo Tosatti]

Hi Kumar,

On Mon, Nov 14, 2005 at 02:55:24PM -0600, Kumar Gala wrote:
> 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/.

Thanks.

Re: [PATCH] m8xx_wdt: software watchdog reset/interrupt select\

[Marcelo Tosatti]

On Mon, Nov 14, 2005 at 01:57:37PM -0200, Marcelo Tosatti wrote:
> Hi Kumar,
> 
> On Mon, Nov 14, 2005 at 02:55:24PM -0600, Kumar Gala wrote:
> > 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?

Re: [PATCH] m8xx_wdt: software watchdog reset/interrupt select\

[Kumar Gala]

On Nov 14, 2005, at 10:03 AM, Marcelo Tosatti wrote:

> On Mon, Nov 14, 2005 at 01:57:37PM -0200, Marcelo Tosatti wrote:
>> Hi Kumar,
>>
>> On Mon, Nov 14, 2005 at 02:55:24PM -0600, Kumar Gala wrote:
>>> 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?

Fair enough.

- kumar

Re: [PATCH] m8xx_wdt: software watchdog reset/interrupt select\

[Florian Schirmer]

Hi,

> 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?

If you have a short timeout (like the 2 secs we had) then you need to 
have the handler ready long before the module could be loaded.

Best,
   Florian

Re: [PATCH] m8xx_wdt: software watchdog reset/interrupt select

[Florian Schirmer]

Hi,

> 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?


> +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);
> +}

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.

Best,
   Florian

Re: [PATCH] m8xx_wdt: software watchdog reset/interrupt select

[Marcelo Tosatti]

Hi Florian!

On Tue, Nov 15, 2005 at 07:41:43AM +0100, Florian Schirmer wrote:
> Hi,
> 
> >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.

> >+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);
> >+}
> 
> 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.

Thanks

Re: [PATCH] m8xx_wdt: software watchdog reset/interrupt select

[Marcelo Tosatti]

On Tue, Nov 15, 2005 at 03:42:16AM -0200, Marcelo Tosatti wrote:
> Hi Florian!
> 
> On Tue, Nov 15, 2005 at 07:41:43AM +0100, Florian Schirmer wrote:
> > Hi,
> > 
> > >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.
> 
> > >+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);
> > >+}
> > 
> > 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.

Florian,

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?


diff --git a/arch/ppc/syslib/m8xx_wdt.c b/arch/ppc/syslib/m8xx_wdt.c
index a21632d..d73f8db 100644
--- a/arch/ppc/syslib/m8xx_wdt.c
+++ b/arch/ppc/syslib/m8xx_wdt.c
@@ -45,35 +45,18 @@ static irqreturn_t m8xx_wdt_interrupt(in
 	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,65 @@ void __init m8xx_wdt_handler_install(bd_
 	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);
+}
+
+void m8xx_wdt_install_timer(void)
+{
+	m8xx_wdt_timer.expires = jiffies + (HZ/2);
+	add_timer(&m8xx_wdt_timer);
+}
+
+static void m8xx_wdt_timer_func(unsigned long data)
+{
+	m8xx_wdt_reset();
+	m8xx_wdt_install_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);
+
+	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) {
+		if (wdt_timeout < (binfo->bi_intfreq/HZ))
+			printk(KERN_ERR "m8xx_wdt: timeout too short for ktimer!\n");
+		m8xx_wdt_install_timer();
+	} else
+		m8xx_wdt_install_irq(imap, binfo);
+
 	wdt_timeout /= binfo->bi_intfreq;
 }
 
diff --git a/arch/ppc/syslib/m8xx_wdt.h b/arch/ppc/syslib/m8xx_wdt.h
index 0d81a9f..c9ee9d7 100644
--- a/arch/ppc/syslib/m8xx_wdt.h
+++ b/arch/ppc/syslib/m8xx_wdt.h
@@ -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(void);
+extern void m8xx_wdt_stop_timer(void);
 
 #endif				/* _PPC_SYSLIB_M8XX_WDT_H */
diff --git a/drivers/char/watchdog/mpc8xx_wdt.c b/drivers/char/watchdog/mpc8xx_wdt.c
index 56d62ba..8ffa744 100644
--- a/drivers/char/watchdog/mpc8xx_wdt.c
+++ b/drivers/char/watchdog/mpc8xx_wdt.c
@@ -27,7 +27,10 @@ static void mpc8xx_wdt_handler_disable(v
 {
 	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 @@ static void mpc8xx_wdt_handler_enable(vo
 {
 	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();
+	else
+		imap->im_sit.sit_piscr |= PISCR_PIE | PISCR_PTE;
 
 	printk(KERN_NOTICE "mpc8xx_wdt: keep-alive handler activated\n");
 }

Re: [PATCH] m8xx_wdt: software watchdog reset/interrupt select

[Florian Schirmer]

Hi,

>> 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.

> 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?

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.

Best,
   Florian

Re: [PATCH] m8xx_wdt: software watchdog reset/interrupt select

[Marcelo Tosatti]

On Wed, Nov 16, 2005 at 12:47:31PM +0100, Florian Schirmer wrote:
> Hi,
> 
> >>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.
> 
> >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?
> 
> 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?

Nope.

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?

> 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).

Re: [PATCH] m8xx_wdt: software watchdog reset/interrupt select

[Florian Schirmer]

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.

And yes you should be right about the watchdog irq. It doesn't make 
sense to install a watchdog irq handler. It doesn't make any sense to 
put the watchdog into irq mode. As far as i know nobody ever tried to 
use that mode.

If you're bound to irq mode because the bootloader activates it the 
whole code should still work out of the box as long as the irq causes a 
system reset. But maybe i'm missing something obvious or the docs are 
incorrect/incomplete?

Best,
   Florian

> 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?
> 
>> 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).

Re: [PATCH] m8xx_wdt: software watchdog reset/interrupt select

[Marcelo Tosatti]

On Thu, Nov 17, 2005 at 10:13:00AM +0100, Florian Schirmer wrote:
> 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.

Florian,

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).

I've changed the m8xx_wdt code to deal with that situation by checking
the RTC enable bit of RTCSC as follows:

m8xx_has_internal_rtc = in_be16(&imap->im_sit.sit_rtcsc) & RTCSC_RTE;

Problem with it is that the kernel sets the RTCSC_RTE bit
unconditionally in m8xx_calibrate_decr(), so you can't really know its
original value. The following patch changes m8xx_calibrate_decr()'s
RTC initialization to a weak function which can be overriden by board
specific code.

Dan, can you think of any nicer to avoid m8xx_calibrate_decr() from 
overwriting RTCSC?

diff --git a/arch/ppc/syslib/m8xx_setup.c b/arch/ppc/syslib/m8xx_setup.c
index 1cc3abe..688616d 100644
--- a/arch/ppc/syslib/m8xx_setup.c
+++ b/arch/ppc/syslib/m8xx_setup.c
@@ -135,6 +135,16 @@ static struct irqaction tbint_irqaction 
 	.name = "tbint",
 };
 
+/* per-board overridable init_internal_rtc() function. */
+void __init __attribute__ ((weak))
+init_internal_rtc(void)
+{
+	/* Disable the RTC one second and alarm interrupts. */
+	out_be16(&((immap_t *)IMAP_ADDR)->im_sit.sit_rtcsc, in_be16(&((immap_t *)IMAP_ADDR)->im_sit.sit_rtcsc) & ~(RTCSC_SIE | RTCSC_ALE));
+	/* Enable the RTC */
+	out_be16(&((immap_t *)IMAP_ADDR)->im_sit.sit_rtcsc, in_be16(&((immap_t *)IMAP_ADDR)->im_sit.sit_rtcsc) | (RTCSC_RTF | RTCSC_RTE));
+}
+
 /* The decrementer counts at the system (internal) clock frequency divided by
  * sixteen, or external oscillator divided by four.  We force the processor
  * to use system clock divided by sixteen.
@@ -183,10 +193,7 @@ void __init m8xx_calibrate_decr(void)
 	out_be32(&((immap_t *)IMAP_ADDR)->im_sitk.sitk_rtcsck, KAPWR_KEY);
 	out_be32(&((immap_t *)IMAP_ADDR)->im_sitk.sitk_tbk, KAPWR_KEY);
 
-	/* Disable the RTC one second and alarm interrupts. */
-	out_be16(&((immap_t *)IMAP_ADDR)->im_sit.sit_rtcsc, in_be16(&((immap_t *)IMAP_ADDR)->im_sit.sit_rtcsc) & ~(RTCSC_SIE | RTCSC_ALE));
-	/* Enable the RTC */
-	out_be16(&((immap_t *)IMAP_ADDR)->im_sit.sit_rtcsc, in_be16(&((immap_t *)IMAP_ADDR)->im_sit.sit_rtcsc) | (RTCSC_RTF | RTCSC_RTE));
+	init_internal_rtc();
 
 	/* Enabling the decrementer also enables the timebase interrupts
 	 * (or from the other point of view, to get decrementer interrupts
diff --git a/arch/ppc/syslib/m8xx_wdt.c b/arch/ppc/syslib/m8xx_wdt.c
index a21632d..df6c955 100644
--- a/arch/ppc/syslib/m8xx_wdt.c
+++ b/arch/ppc/syslib/m8xx_wdt.c
@@ -19,6 +19,7 @@
 #include <syslib/m8xx_wdt.h>
 
 static int wdt_timeout;
+int m8xx_has_internal_rtc = 0;
 
 static irqreturn_t m8xx_wdt_interrupt(int, void *, struct pt_regs *);
 static struct irqaction m8xx_wdt_irqaction = {
@@ -45,35 +46,15 @@ static irqreturn_t m8xx_wdt_interrupt(in
 	return IRQ_HANDLED;
 }
 
-void __init m8xx_wdt_handler_install(bd_t * binfo)
+#define SYPCR_SWP 0x1
+#define SYPCR_SWE 0x4
+
+
+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 +79,67 @@ void __init m8xx_wdt_handler_install(bd_
 	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);
+}
+
+void m8xx_wdt_install_timer(void)
+{
+	m8xx_wdt_timer.expires = jiffies + (HZ/2);
+	add_timer(&m8xx_wdt_timer);
+}
+
+static void m8xx_wdt_timer_func(unsigned long data)
+{
+	m8xx_wdt_reset();
+	m8xx_wdt_install_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);
+
+	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_has_internal_rtc = in_be16(&imap->im_sit.sit_rtcsc) & RTCSC_RTE;
+
+	/* if the internal RTC is off use a kernel timer */
+	if (!m8xx_has_internal_rtc) {
+		if (wdt_timeout < (binfo->bi_intfreq/HZ))
+			printk(KERN_ERR "m8xx_wdt: timeout too short for ktimer!\n");
+		m8xx_wdt_install_timer();
+	} else
+		m8xx_wdt_install_irq(imap, binfo);
+
 	wdt_timeout /= binfo->bi_intfreq;
 }
 
diff --git a/arch/ppc/syslib/m8xx_wdt.h b/arch/ppc/syslib/m8xx_wdt.h
index 0d81a9f..e75835f 100644
--- a/arch/ppc/syslib/m8xx_wdt.h
+++ b/arch/ppc/syslib/m8xx_wdt.h
@@ -9,8 +9,12 @@
 #ifndef _PPC_SYSLIB_M8XX_WDT_H
 #define _PPC_SYSLIB_M8XX_WDT_H
 
+extern int m8xx_has_internal_rtc;
+
 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(void);
+extern void m8xx_wdt_stop_timer(void);
 
 #endif				/* _PPC_SYSLIB_M8XX_WDT_H */
diff --git a/drivers/char/watchdog/mpc8xx_wdt.c b/drivers/char/watchdog/mpc8xx_wdt.c
index 56d62ba..ac6fbac 100644
--- a/drivers/char/watchdog/mpc8xx_wdt.c
+++ b/drivers/char/watchdog/mpc8xx_wdt.c
@@ -27,7 +27,10 @@ static void mpc8xx_wdt_handler_disable(v
 {
 	volatile immap_t *imap = (volatile immap_t *)IMAP_ADDR;
 
-	imap->im_sit.sit_piscr &= ~(PISCR_PIE | PISCR_PTE);
+	if (!m8xx_has_internal_rtc)
+		m8xx_wdt_stop_timer();
+	else
+		out_be32(imap->im_sit.sit_piscr, in_be32(&imap->im_sit.sit_piscr) & ~(PISCR_PIE | PISCR_PTE));
 
 	printk(KERN_NOTICE "mpc8xx_wdt: keep-alive handler deactivated\n");
 }
@@ -36,7 +39,10 @@ static void mpc8xx_wdt_handler_enable(vo
 {
 	volatile immap_t *imap = (volatile immap_t *)IMAP_ADDR;
 
-	imap->im_sit.sit_piscr |= PISCR_PIE | PISCR_PTE;
+	if (!m8xx_has_internal_rtc)
+		m8xx_wdt_install_timer();
+	else
+		out_be32(&imap->im_sit.sit_piscr, in_be32(&imap->im_sit.sit_piscr) | PISCR_PIE | PISCR_PTE);
 
 	printk(KERN_NOTICE "mpc8xx_wdt: keep-alive handler activated\n");
 }
@@ -68,9 +74,6 @@ static int mpc8xx_wdt_release(struct ino
 static ssize_t mpc8xx_wdt_write(struct file *file, const char *data, size_t len,
 				loff_t * ppos)
 {
-	if (ppos != &file->f_pos)
-		return -ESPIPE;
-
 	if (len)
 		m8xx_wdt_reset();
 

Re: [PATCH] m8xx_wdt: software watchdog reset/interrupt select

[Florian Schirmer]

Hi,

> 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).

Now it starts to make sense :-) Patch looks good. If Dan doesn't come up 
with a better idea to solve the clock detection problem feel free to go 
ahead and send it upstream.

Best,
   Florian