Re: [PATCH] Add watchdog driver for w90p910

[Wim Van Sebroeck <wim@iguana.be>]

Hi Wan,

>  I'm much obliged to you for helping me.

With pleasure :-).

> >> +static irqreturn_t w90p910_wdt_irq(int irq, void *dev_id)
> >> +{
> >> +     w90p910_wdt_keepalive();
> >> +     return IRQ_HANDLED;
> >> +}
> >
> > How does the interrupt work? what does the watchdog do when it times out?
> > (Is there a datasheet available somewhere?)
> >
> 
> The mechanism of watchdog of w90p910 is that the interrupt will occur
> periodicity every a given time interval.
> 
> Hmm, there are only two choices to confirm the system running, one to
> clear this WTIF and  WTR bits in handler of interrupt, the other to
> reset them in user application before interrupt occurs.

Hmm, I'm confused now. how does the watchdog then reboot your system?
The way we implement watchdog's in linux is that you start the watchdog and that userspace needs to trigger/ping/keep-the-watchdog-alive before the timeout period is finished/over. If the userspace daemon can keep doing this (or with other words: the system is healthy enough to operate normally without to much delay) your system doesn't get rebooted. If it doesn't manage to do it, then the watchdog knows that your system is not stable/healthy enough and will reboot your system.

Here I think that the watchdog is started. Then userspace start pinging the watchdog and everything is OK. If userspace doesn't ping the watchdog in time an interrupt is being generated and instead of rebooting the system, the watchdog is being triggered... If this is indeed what it does then this is wrong.

> > The read-write cyclus to REG_WTCR should also be guarded with a spinlock.
> > Can you incorporate the call to w90p910_wdt_start() in the ioctl code into this function?
> >
> 
> Do you mean that I should merge the "w90p910_wdt_settimeout" to
> "w90p910_wdt_start()"? or contrary?

I mean the contrary.
If your watchdog needs to be restarted when you set a new timeout, then it is best to have this in the settimeout function.
It is also better to do a ping to the watchdog once the new timeout was set. It might however be that a ping is enough and that a restart is not needed.
So something like the following diff could be it:
--- w90p910_wdt.c	2009-08-05 06:44:14.286477908 +0000
+++ w90p910_wdt.c	2009-08-05 06:46:04.496353102 +0000
@@ -144,6 +144,9 @@
 	__raw_writel(val, w90p910_wdt->wdt_base + REG_WTCR);
 
 	wdt_time = new_time_level;
+
+	w90p910_wdt_start();
+
 	return 0;
 }
 
@@ -182,9 +185,7 @@
 			return -EFAULT;
 		if (w90p910_wdt_settimeout(new_value))
 			return -EINVAL;
-
-		w90p910_wdt_start();
-
+		w90p910_wdt_keepalive();
 		return put_user(wdt_time, p);
 	case WDIOC_GETTIMEOUT:
 		return put_user(wdt_time, p);
---

I have an additional question: which timeout values can you set? is it 0, 1, 2 and 3 seconds?

Kind regards,
Wim.