[PATCH] watchdog: make imx2_wdt report boot status correctly

[PATCH] watchdog: make imx2_wdt report boot status correctly

[Oskar Schirmer]

Ioctl WDIOC_GETBOOTSTATUS is supposed to return some information
on why the system did (re)boot recently, value WDIOF_CARDRESET
being used to indicate watchdog induced reboot.

Up to now, imx2_wdt did not provide a value here, always returning
zero to indicate normal boot.

Do evaluate the IMX Watchdog Reset Status Register and
produce WDIOF_CARDRESET with WDIOC_GETBOOTSTATUS in case
of a watchdog induced reset.

Signed-off-by: Oskar Schirmer <oskar@scara.com>
---
 drivers/watchdog/imx2_wdt.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index c44c333..940fd43 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -46,6 +46,9 @@
 #define IMX2_WDT_SEQ1		0x5555		/* -> service sequence 1 */
 #define IMX2_WDT_SEQ2		0xAAAA		/* -> service sequence 2 */
 
+#define IMX2_WDT_WRSR		0x04		/* Reset Status Register */
+#define IMX2_WDT_WRSR_TOUT	(1 << 1)	/* -> Reset due to Timeout */
+
 #define IMX2_WDT_MAX_TIME	128
 #define IMX2_WDT_DEFAULT_TIME	60		/* in seconds */
 
@@ -175,6 +178,7 @@ static long imx2_wdt_ioctl(struct file *file, unsigned int cmd,
 	void __user *argp = (void __user *)arg;
 	int __user *p = argp;
 	int new_value;
+	u16 val;
 
 	switch (cmd) {
 	case WDIOC_GETSUPPORT:
@@ -182,9 +186,15 @@ static long imx2_wdt_ioctl(struct file *file, unsigned int cmd,
 			sizeof(struct watchdog_info)) ? -EFAULT : 0;
 
 	case WDIOC_GETSTATUS:
-	case WDIOC_GETBOOTSTATUS:
 		return put_user(0, p);
 
+	case WDIOC_GETBOOTSTATUS:
+		val = __raw_readw(imx2_wdt.base + IMX2_WDT_WRSR);
+		new_value = 0;
+		if (val & IMX2_WDT_WRSR_TOUT)
+			new_value = WDIOF_CARDRESET;
+		return put_user(new_value, p);
+
 	case WDIOC_KEEPALIVE:
 		imx2_wdt_ping();
 		return 0;
-- 
1.7.5.4

Re: [PATCH] watchdog: make imx2_wdt report boot status correctly

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 591 bytes --]

Hi Oskar,

besides this minor thing (which may be just personal taste)...

> +	case WDIOC_GETBOOTSTATUS:
> +		val = __raw_readw(imx2_wdt.base + IMX2_WDT_WRSR);
> +		new_value = 0;
> +		if (val & IMX2_WDT_WRSR_TOUT)
> +			new_value = WDIOF_CARDRESET;

I'd go for this to save some lines:
		new_value = val & IMX2_WDT_WRSR_TOUT ? WDIOF_CARDRESET : 0;

But in general:

Acked-by: Wolfram Sang <w.sang@pengutronix.de>


-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] watchdog: make imx2_wdt report boot status correctly

[Oskar Schirmer]

Hi Wolfram,

On Thu, Feb 16, 2012 at 14:35:10 +0100, Wolfram Sang wrote:
> besides this minor thing (which may be just personal taste)...
> 
> > +	case WDIOC_GETBOOTSTATUS:
> > +		val = __raw_readw(imx2_wdt.base + IMX2_WDT_WRSR);
> > +		new_value = 0;
> > +		if (val & IMX2_WDT_WRSR_TOUT)
> > +			new_value = WDIOF_CARDRESET;
> 
> I'd go for this to save some lines:
> 		new_value = val & IMX2_WDT_WRSR_TOUT ? WDIOF_CARDRESET : 0;

A good alternative for sure.

Seen from the perspective of code compactness ("save some lines"),
one could try to be consequent here, and save the "val" variable
altogether, ending up with some "new_value = __raw_readw ... & ... ? ... : 0;"
But eventually there might be code readability issues, so it's
a good idea to keep the balance.

An argument for the original, longer version might be it is easier
to extend, when more flags need to be handled, like
"else if (val & ...) new_value = ...", while nested conditional
expressions would most likely become quite complex soon.

Actually, my personal taste doesn't show a preference for
one version or the other, so I'm ok with Your proposal, too.

  Oskar

Re: [PATCH] watchdog: make imx2_wdt report boot status correctly

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 910 bytes --]


> Seen from the perspective of code compactness ("save some lines"),
> one could try to be consequent here, and save the "val" variable
> altogether, ending up with some "new_value = __raw_readw ... & ... ? ... : 0;"
> But eventually there might be code readability issues, so it's
> a good idea to keep the balance.

Yes.

> An argument for the original, longer version might be it is easier
> to extend, when more flags need to be handled, like
> "else if (val & ...) new_value = ...", while nested conditional
> expressions would most likely become quite complex soon.

Once we have support to handle non-stoppable watchdogs in the the new
watchdog framework, the code will be greatly refactored anyhow :)

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]