diff mbox

fix book E watchdog to take WDIOC_SETTIMEOUT arg in seconds

Message ID 4A8303C6.7070705@nortel.com (mailing list archive)
State Accepted, archived
Delegated to: Kumar Gala
Headers show

Commit Message

Chris Friesen Aug. 12, 2009, 6:02 p.m. UTC 
The WDIOC_SETTIMEOUT argument is supposed to be a "seconds" value.
However, the book E wdt currently treats it as a "period" which is
interpreted in a board-specific way.

This patch allows the user to pass in a "seconds" value and the driver
will set the smallest timeout that is at least as large as specified
by the user.  It's been tested on e500 hardware and works as
expected.

The patch only modifies the CONFIG_FSL_BOOKE case, the CONFIG_4xx case
is left unmodified as I don't have any hardware to test it on.

Signed-off-by: Chris Friesen <cfriesen@nortel.com>

Comments

Wim Van Sebroeck Aug. 27, 2009, 11:14 a.m. UTC | #1 
Hi Chris,

> The WDIOC_SETTIMEOUT argument is supposed to be a "seconds" value.
> However, the book E wdt currently treats it as a "period" which is
> interpreted in a board-specific way.
> 
> This patch allows the user to pass in a "seconds" value and the driver
> will set the smallest timeout that is at least as large as specified
> by the user.  It's been tested on e500 hardware and works as
> expected.
> 
> The patch only modifies the CONFIG_FSL_BOOKE case, the CONFIG_4xx case
> is left unmodified as I don't have any hardware to test it on.
> 
> Signed-off-by: Chris Friesen <cfriesen@nortel.com>

Added with some small changes to keep checkpatch happy. (removed trailing spaces + changed sizeof(struct watchdog_info) to sizeof(ident) and changed some spaces in tabs).

Now we only need someone that can look at the CONFIG_4xx cases still :-)

Kind regards,
Wim.
Josh Boyer Aug. 27, 2009, 12:08 p.m. UTC | #2 
On Thu, Aug 27, 2009 at 01:14:58PM +0200, Wim Van Sebroeck wrote:
>Hi Chris,
>
>> The WDIOC_SETTIMEOUT argument is supposed to be a "seconds" value.
>> However, the book E wdt currently treats it as a "period" which is
>> interpreted in a board-specific way.
>> 
>> This patch allows the user to pass in a "seconds" value and the driver
>> will set the smallest timeout that is at least as large as specified
>> by the user.  It's been tested on e500 hardware and works as
>> expected.
>> 
>> The patch only modifies the CONFIG_FSL_BOOKE case, the CONFIG_4xx case
>> is left unmodified as I don't have any hardware to test it on.
>> 
>> Signed-off-by: Chris Friesen <cfriesen@nortel.com>
>
>Added with some small changes to keep checkpatch happy. (removed trailing spaces + changed sizeof(struct watchdog_info) to sizeof(ident) and changed some spaces in tabs).
>
>Now we only need someone that can look at the CONFIG_4xx cases still :-)

It seems the FSL watchdog is much more flexible than the one found in 4xx
cores.  On 4xx, you basically have 4 static choices that represent specific
times determined by the clock frequency.  I'm concerned that the lack of
granularity here will result in less than desirable behavior.

For example, with a 400MHz clock you would get choices of roughly:

5.2 ms
83.9 ms
1.34 s
21.47 s

Personally, I consider the first two options basically unusable.  Considering
the second two, if a user were to say "Set the timeout for 2 seconds" they
would then get a timeout of 21 seconds with the framework Chris' patch has
set up.  That doesn't really seem to be ideal to me.

josh
Wim Van Sebroeck Aug. 27, 2009, 8:27 p.m. UTC | #3 
Hi Josh,

> >Now we only need someone that can look at the CONFIG_4xx cases still :-)
> 
> It seems the FSL watchdog is much more flexible than the one found in 4xx
> cores.  On 4xx, you basically have 4 static choices that represent specific
> times determined by the clock frequency.  I'm concerned that the lack of
> granularity here will result in less than desirable behavior.
> 
> For example, with a 400MHz clock you would get choices of roughly:
> 
> 5.2 ms
> 83.9 ms
> 1.34 s
> 21.47 s
> 
> Personally, I consider the first two options basically unusable.  Considering
> the second two, if a user were to say "Set the timeout for 2 seconds" they
> would then get a timeout of 21 seconds with the framework Chris' patch has
> set up.  That doesn't really seem to be ideal to me.

Hmm, my opinion: in that case we should use a timer that triggers the watchdog until userspaces times out (like we do for other watchdogs allready). Maybe we should split this driver. I have the same issue with the Freescale i.mx driver that is under review also.

Kind regards,
Wim.
diff mbox

Patch

diff --git a/drivers/watchdog/booke_wdt.c b/drivers/watchdog/booke_wdt.c
index 225398f..f1a3617 100644
--- a/drivers/watchdog/booke_wdt.c
+++ b/drivers/watchdog/booke_wdt.c
@@ -22,6 +22,8 @@ 
 
 #include <asm/reg_booke.h>
 #include <asm/system.h>
+#include <asm/time.h>
+#include <asm/div64.h>
 
 /* If the kernel parameter wdt=1, the watchdog will be enabled at boot.
  * Also, the wdt_period sets the watchdog timer period timeout.
@@ -32,7 +34,7 @@ 
  */
 
 #ifdef	CONFIG_FSL_BOOKE
-#define WDT_PERIOD_DEFAULT 63	/* Ex. wdt_period=28 bus=333Mhz,reset=~40sec */
+#define WDT_PERIOD_DEFAULT 38	/* Ex. wdt_period=28 bus=333Mhz,reset=~40sec */
 #else
 #define WDT_PERIOD_DEFAULT 3	/* Refer to the PPC40x and PPC4xx manuals */
 #endif				/* for timing information */
@@ -41,7 +43,7 @@  u32 booke_wdt_enabled;
 u32 booke_wdt_period = WDT_PERIOD_DEFAULT;
 
 #ifdef	CONFIG_FSL_BOOKE
-#define WDTP(x)		((((63-x)&0x3)<<30)|(((63-x)&0x3c)<<15))
+#define WDTP(x)		((((x)&0x3)<<30)|(((x)&0x3c)<<15))
 #define WDTP_MASK	(WDTP(0))
 #else
 #define WDTP(x)		(TCR_WP(x))
@@ -50,6 +52,45 @@  u32 booke_wdt_period = WDT_PERIOD_DEFAULT;
 
 static DEFINE_SPINLOCK(booke_wdt_lock);
 
+/* For the specified period, determine the number of seconds
+ * corresponding to the reset time.  There will be a watchdog
+ * exception at approximately 3/5 of this time.
+ *
+ * The formula to calculate this is given by:
+ * 2.5 * (2^(63-period+1)) / timebase_freq
+ *
+ * In order to simplify things, we assume that period is
+ * at least 1.  This will still result in a very long timeout.
+ */
+static unsigned long long period_to_sec(unsigned int period)
+{
+	unsigned long long tmp = 1ULL << (64 - period);
+	unsigned long tmp2 = ppc_tb_freq;
+	
+	/* tmp may be a very large number and we don't want to overflow,
+	 * so divide the timebase freq instead of multiplying tmp
+	 */
+	tmp2 = tmp2 / 5 * 2;
+	
+	do_div(tmp, tmp2);
+	return tmp;
+}
+
+/*
+ * This procedure will find the highest period which will give a timeout
+ * greater than the one required. e.g. for a bus speed of 66666666 and 
+ * and a parameter of 2 secs, then this procedure will return a value of 38.
+ */
+static unsigned int sec_to_period(unsigned int secs)
+{
+	unsigned int period;
+	for (period = 63; period > 0; period--) {
+		if (period_to_sec(period) >= secs)
+			return period;
+	}
+	return 0;
+}
+
 static void __booke_wdt_ping(void *data)
 {
 	mtspr(SPRN_TSR, TSR_ENW|TSR_WIS);
@@ -93,7 +134,7 @@  static long booke_wdt_ioctl(struct file *file,
 
 	switch (cmd) {
 	case WDIOC_GETSUPPORT:
-		if (copy_to_user(arg, &ident, sizeof(struct watchdog_info)))
+		if (copy_to_user((void *)arg, &ident, sizeof(struct watchdog_info)))
 			return -EFAULT;
 	case WDIOC_GETSTATUS:
 		return put_user(ident.options, p);
@@ -115,8 +156,16 @@  static long booke_wdt_ioctl(struct file *file,
 		booke_wdt_ping();
 		return 0;
 	case WDIOC_SETTIMEOUT:
-		if (get_user(booke_wdt_period, p))
+		if (get_user(tmp, p))
 			return -EFAULT;
+#ifdef	CONFIG_FSL_BOOKE
+		/* period of 1 gives the largest possible timeout */
+		if (tmp > period_to_sec(1))
+			return -EINVAL;
+                booke_wdt_period = sec_to_period(tmp);
+#else
+		booke_wdt_period = tmp;
+#endif
 		mtspr(SPRN_TCR, (mfspr(SPRN_TCR) & ~WDTP_MASK) |
 						WDTP(booke_wdt_period));
 		return 0;