[PATCH 1/3] watchdog: octeon: convert to WATCHDOG_CORE API

[PATCH 1/3] watchdog: octeon: convert to WATCHDOG_CORE API

[Aaro Koskinen]

Convert OCTEON watchdog to WATCHDOG_CORE API. This enables support
for multiple watchdogs on OCTEON boards.

Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
---
 drivers/watchdog/Kconfig           |   1 +
 drivers/watchdog/octeon-wdt-main.c | 185 ++++++++-----------------------------
 2 files changed, 39 insertions(+), 147 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 16f2023..0fcd60b 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1190,6 +1190,7 @@ config OCTEON_WDT
 	tristate "Cavium OCTEON SOC family Watchdog Timer"
 	depends on CAVIUM_OCTEON_SOC
 	default y
+	select WATCHDOG_CORE
 	select EXPORT_UASM if OCTEON_WDT = m
 	help
 	  Hardware driver for OCTEON's on chip watchdog timer.
diff --git a/drivers/watchdog/octeon-wdt-main.c b/drivers/watchdog/octeon-wdt-main.c
index 8453531..9aa5121 100644
--- a/drivers/watchdog/octeon-wdt-main.c
+++ b/drivers/watchdog/octeon-wdt-main.c
@@ -3,6 +3,8 @@
  *
  * Copyright (C) 2007, 2008, 2009, 2010 Cavium Networks
  *
+ * Converted to use WATCHDOG_CORE by Aaro Koskinen <aaro.koskinen@iki.fi>.
+ *
  * Some parts derived from wdt.c
  *
  *	(c) Copyright 1996-1997 Alan Cox <alan@lxorguk.ukuu.org.uk>,
@@ -103,9 +105,6 @@ MODULE_PARM_DESC(nowayout,
 	"Watchdog cannot be stopped once started (default="
 				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
-static unsigned long octeon_wdt_is_open;
-static char expect_close;
-
 static u32 __initdata nmi_stage1_insns[64];
 /* We need one branch and therefore one relocation per target label. */
 static struct uasm_label __initdata labels[5];
@@ -444,7 +443,7 @@ static int octeon_wdt_cpu_callback(struct notifier_block *nfb,
 	return NOTIFY_OK;
 }
 
-static void octeon_wdt_ping(void)
+static int octeon_wdt_ping(struct watchdog_device __always_unused *wdog)
 {
 	int cpu;
 	int coreid;
@@ -461,6 +460,7 @@ static void octeon_wdt_ping(void)
 			cpumask_set_cpu(cpu, &irq_enabled_cpus);
 		}
 	}
+	return 0;
 }
 
 static void octeon_wdt_calc_parameters(int t)
@@ -489,7 +489,8 @@ static void octeon_wdt_calc_parameters(int t)
 	timeout_cnt = ((octeon_get_io_clock_rate() >> 8) * timeout_sec) >> 8;
 }
 
-static int octeon_wdt_set_heartbeat(int t)
+static int octeon_wdt_set_timeout(struct watchdog_device *wdog,
+				  unsigned int t)
 {
 	int cpu;
 	int coreid;
@@ -509,158 +510,45 @@ static int octeon_wdt_set_heartbeat(int t)
 		cvmx_write_csr(CVMX_CIU_WDOGX(coreid), ciu_wdog.u64);
 		cvmx_write_csr(CVMX_CIU_PP_POKEX(coreid), 1);
 	}
-	octeon_wdt_ping(); /* Get the irqs back on. */
+	octeon_wdt_ping(wdog); /* Get the irqs back on. */
 	return 0;
 }
 
-/**
- *	octeon_wdt_write:
- *	@file: file handle to the watchdog
- *	@buf: buffer to write (unused as data does not matter here
- *	@count: count of bytes
- *	@ppos: pointer to the position to write. No seeks allowed
- *
- *	A write to a watchdog device is defined as a keepalive signal. Any
- *	write of data will do, as we we don't define content meaning.
- */
-
-static ssize_t octeon_wdt_write(struct file *file, const char __user *buf,
-				size_t count, loff_t *ppos)
-{
-	if (count) {
-		if (!nowayout) {
-			size_t i;
-
-			/* In case it was set long ago */
-			expect_close = 0;
-
-			for (i = 0; i != count; i++) {
-				char c;
-				if (get_user(c, buf + i))
-					return -EFAULT;
-				if (c == 'V')
-					expect_close = 1;
-			}
-		}
-		octeon_wdt_ping();
-	}
-	return count;
-}
-
-/**
- *	octeon_wdt_ioctl:
- *	@file: file handle to the device
- *	@cmd: watchdog command
- *	@arg: argument pointer
- *
- *	The watchdog API defines a common set of functions for all
- *	watchdogs according to their available features. We only
- *	actually usefully support querying capabilities and setting
- *	the timeout.
- */
-
-static long octeon_wdt_ioctl(struct file *file, unsigned int cmd,
-			     unsigned long arg)
-{
-	void __user *argp = (void __user *)arg;
-	int __user *p = argp;
-	int new_heartbeat;
-
-	static struct watchdog_info ident = {
-		.options =		WDIOF_SETTIMEOUT|
-					WDIOF_MAGICCLOSE|
-					WDIOF_KEEPALIVEPING,
-		.firmware_version =	1,
-		.identity =		"OCTEON",
-	};
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		return copy_to_user(argp, &ident, sizeof(ident)) ? -EFAULT : 0;
-	case WDIOC_GETSTATUS:
-	case WDIOC_GETBOOTSTATUS:
-		return put_user(0, p);
-	case WDIOC_KEEPALIVE:
-		octeon_wdt_ping();
-		return 0;
-	case WDIOC_SETTIMEOUT:
-		if (get_user(new_heartbeat, p))
-			return -EFAULT;
-		if (octeon_wdt_set_heartbeat(new_heartbeat))
-			return -EINVAL;
-		/* Fall through. */
-	case WDIOC_GETTIMEOUT:
-		return put_user(heartbeat, p);
-	default:
-		return -ENOTTY;
-	}
-}
-
-/**
- *	octeon_wdt_open:
- *	@inode: inode of device
- *	@file: file handle to device
- *
- *	The watchdog device has been opened. The watchdog device is single
- *	open and on opening we do a ping to reset the counters.
- */
-
-static int octeon_wdt_open(struct inode *inode, struct file *file)
+static int octeon_wdt_start(struct watchdog_device *wdog)
 {
-	if (test_and_set_bit(0, &octeon_wdt_is_open))
-		return -EBUSY;
-	/*
-	 *	Activate
-	 */
-	octeon_wdt_ping();
+	octeon_wdt_ping(wdog);
 	do_coundown = 1;
-	return nonseekable_open(inode, file);
+	return 0;
 }
 
-/**
- *	octeon_wdt_release:
- *	@inode: inode to board
- *	@file: file handle to board
- *
- *	The watchdog has a configurable API. There is a religious dispute
- *	between people who want their watchdog to be able to shut down and
- *	those who want to be sure if the watchdog manager dies the machine
- *	reboots. In the former case we disable the counters, in the latter
- *	case you have to open it again very soon.
- */
-
-static int octeon_wdt_release(struct inode *inode, struct file *file)
+static int octeon_wdt_stop(struct watchdog_device *wdog)
 {
-	if (expect_close) {
-		do_coundown = 0;
-		octeon_wdt_ping();
-	} else {
-		pr_crit("WDT device closed unexpectedly.  WDT will not stop!\n");
-	}
-	clear_bit(0, &octeon_wdt_is_open);
-	expect_close = 0;
+	do_coundown = 0;
+	octeon_wdt_ping(wdog);
 	return 0;
 }
 
-static const struct file_operations octeon_wdt_fops = {
-	.owner		= THIS_MODULE,
-	.llseek		= no_llseek,
-	.write		= octeon_wdt_write,
-	.unlocked_ioctl	= octeon_wdt_ioctl,
-	.open		= octeon_wdt_open,
-	.release	= octeon_wdt_release,
+static struct notifier_block octeon_wdt_cpu_notifier = {
+	.notifier_call = octeon_wdt_cpu_callback,
 };
 
-static struct miscdevice octeon_wdt_miscdev = {
-	.minor	= WATCHDOG_MINOR,
-	.name	= "watchdog",
-	.fops	= &octeon_wdt_fops,
+static const struct watchdog_info octeon_wdt_info = {
+	.options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING,
+	.identity = "OCTEON",
 };
 
-static struct notifier_block octeon_wdt_cpu_notifier = {
-	.notifier_call = octeon_wdt_cpu_callback,
+static const struct watchdog_ops octeon_wdt_ops = {
+	.owner		= THIS_MODULE,
+	.start		= octeon_wdt_start,
+	.stop		= octeon_wdt_stop,
+	.ping		= octeon_wdt_ping,
+	.set_timeout	= octeon_wdt_set_timeout,
 };
 
+static struct watchdog_device octeon_wdt = {
+	.info	= &octeon_wdt_info,
+	.ops	= &octeon_wdt_ops,
+};
 
 /**
  * Module/ driver initialization.
@@ -694,11 +582,15 @@ static int __init octeon_wdt_init(void)
 
 	pr_info("Initial granularity %d Sec\n", timeout_sec);
 
-	ret = misc_register(&octeon_wdt_miscdev);
+	octeon_wdt.timeout	= timeout_sec;
+	octeon_wdt.max_timeout	= UINT_MAX;
+
+	watchdog_set_nowayout(&octeon_wdt, nowayout);
+
+	ret = watchdog_register_device(&octeon_wdt);
 	if (ret) {
-		pr_err("cannot register miscdev on minor=%d (err=%d)\n",
-		       WATCHDOG_MINOR, ret);
-		goto out;
+		pr_err("watchdog_register_device() failed: %d\n", ret);
+		return ret;
 	}
 
 	/* Build the NMI handler ... */
@@ -721,8 +613,7 @@ static int __init octeon_wdt_init(void)
 	__register_hotcpu_notifier(&octeon_wdt_cpu_notifier);
 	cpu_notifier_register_done();
 
-out:
-	return ret;
+	return 0;
 }
 
 /**
@@ -732,7 +623,7 @@ static void __exit octeon_wdt_cleanup(void)
 {
 	int cpu;
 
-	misc_deregister(&octeon_wdt_miscdev);
+	watchdog_unregister_device(&octeon_wdt);
 
 	cpu_notifier_register_begin();
 	__unregister_hotcpu_notifier(&octeon_wdt_cpu_notifier);
-- 
2.2.0

[PATCH 2/3] watchdog: octeon: fix some trivial coding style issues

[Aaro Koskinen]

Fix some trivial coding style issues to reduce noise from static analyzers.

Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
---
 drivers/watchdog/octeon-wdt-main.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/octeon-wdt-main.c b/drivers/watchdog/octeon-wdt-main.c
index 9aa5121..728840c 100644
--- a/drivers/watchdog/octeon-wdt-main.c
+++ b/drivers/watchdog/octeon-wdt-main.c
@@ -105,10 +105,10 @@ MODULE_PARM_DESC(nowayout,
 	"Watchdog cannot be stopped once started (default="
 				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
-static u32 __initdata nmi_stage1_insns[64];
+static u32 nmi_stage1_insns[64] __initdata;
 /* We need one branch and therefore one relocation per target label. */
-static struct uasm_label __initdata labels[5];
-static struct uasm_reloc __initdata relocs[5];
+static struct uasm_label labels[5] __initdata;
+static struct uasm_reloc relocs[5] __initdata;
 
 enum lable_id {
 	label_enter_bootloader = 1
@@ -217,7 +217,8 @@ static void __init octeon_wdt_build_stage1(void)
 	pr_debug("\t.set pop\n");
 
 	if (len > 32)
-		panic("NMI stage 1 handler exceeds 32 instructions, was %d\n", len);
+		panic("NMI stage 1 handler exceeds 32 instructions, was %d\n",
+		      len);
 }
 
 static int cpu2core(int cpu)
@@ -293,6 +294,7 @@ static void octeon_wdt_write_hex(u64 value, int digits)
 {
 	int d;
 	int v;
+
 	for (d = 0; d < digits; d++) {
 		v = (value >> ((digits - d - 1) * 4)) & 0xf;
 		if (v >= 10)
@@ -302,7 +304,7 @@ static void octeon_wdt_write_hex(u64 value, int digits)
 	}
 }
 
-const char *reg_name[] = {
+static const char *reg_name[] = {
 	"$0", "at", "v0", "v1", "a0", "a1", "a2", "a3",
 	"a4", "a5", "a6", "a7", "t0", "t1", "t2", "t3",
 	"s0", "s1", "s2", "s3", "s4", "s5", "s6", "s7",
@@ -456,6 +458,7 @@ static int octeon_wdt_ping(struct watchdog_device __always_unused *wdog)
 		    !cpumask_test_cpu(cpu, &irq_enabled_cpus)) {
 			/* We have to enable the irq */
 			int irq = OCTEON_IRQ_WDOG0 + coreid;
+
 			enable_irq(irq);
 			cpumask_set_cpu(cpu, &irq_enabled_cpus);
 		}
@@ -573,7 +576,8 @@ static int __init octeon_wdt_init(void)
 	max_timeout_sec = 6;
 	do {
 		max_timeout_sec--;
-		timeout_cnt = ((octeon_get_io_clock_rate() >> 8) * max_timeout_sec) >> 8;
+		timeout_cnt = ((octeon_get_io_clock_rate() >> 8) *
+			      max_timeout_sec) >> 8;
 	} while (timeout_cnt > 65535);
 
 	BUG_ON(timeout_cnt == 0);
-- 
2.2.0

[PATCH 3/3] watchdog: octeon: use fixed length string for register names

[Aaro Koskinen]

Use fixed length string for register names. This saves 416 bytes
in text size.

Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
---
 drivers/watchdog/octeon-wdt-main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/octeon-wdt-main.c b/drivers/watchdog/octeon-wdt-main.c
index 728840c..14521c8 100644
--- a/drivers/watchdog/octeon-wdt-main.c
+++ b/drivers/watchdog/octeon-wdt-main.c
@@ -304,7 +304,7 @@ static void octeon_wdt_write_hex(u64 value, int digits)
 	}
 }
 
-static const char *reg_name[] = {
+static const char reg_name[][3] = {
 	"$0", "at", "v0", "v1", "a0", "a1", "a2", "a3",
 	"a4", "a5", "a6", "a7", "t0", "t1", "t2", "t3",
 	"s0", "s1", "s2", "s3", "s4", "s5", "s6", "s7",
-- 
2.2.0

Re: [PATCH 1/3] watchdog: octeon: convert to WATCHDOG_CORE API

[Guenter Roeck]

On 03/28/2015 11:05 AM, Aaro Koskinen wrote:
> Convert OCTEON watchdog to WATCHDOG_CORE API. This enables support
> for multiple watchdogs on OCTEON boards.
>
> Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Re: [PATCH 2/3] watchdog: octeon: fix some trivial coding style issues

[Guenter Roeck]

On 03/28/2015 11:05 AM, Aaro Koskinen wrote:
> Fix some trivial coding style issues to reduce noise from static analyzers.
>
> Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>


Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Re: [PATCH 3/3] watchdog: octeon: use fixed length string for register names

[Guenter Roeck]

On 03/28/2015 11:05 AM, Aaro Koskinen wrote:
> Use fixed length string for register names. This saves 416 bytes
> in text size.
>
> Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>


Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Re: [PATCH 1/3] watchdog: octeon: convert to WATCHDOG_CORE API

[David Daney]

On 03/28/2015 11:05 AM, Aaro Koskinen wrote:
> Convert OCTEON watchdog to WATCHDOG_CORE API. This enables support
> for multiple watchdogs on OCTEON boards.
>
> Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> ---
>   drivers/watchdog/Kconfig           |   1 +
>   drivers/watchdog/octeon-wdt-main.c | 185 ++++++++-----------------------------
>   2 files changed, 39 insertions(+), 147 deletions(-)
>
[...]

You didn't seem to say how it was tested.

If you have verified that "echo > /dev/watchdog" produces register dumps 
and reboots the board, then the whole series:

Acked-by: David Daney <david.daney@cavium.com>



Thanks for doing this, I had been meaning to make the conversion myself.

David.

Re: [PATCH 1/3] watchdog: octeon: convert to WATCHDOG_CORE API

[Aaro Koskinen]

Hi,

On Mon, Mar 30, 2015 at 11:16:16AM -0700, David Daney wrote:
> On 03/28/2015 11:05 AM, Aaro Koskinen wrote:
> >Convert OCTEON watchdog to WATCHDOG_CORE API. This enables support
> >for multiple watchdogs on OCTEON boards.
> >
> >Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> >---
> >  drivers/watchdog/Kconfig           |   1 +
> >  drivers/watchdog/octeon-wdt-main.c | 185 ++++++++-----------------------------
> >  2 files changed, 39 insertions(+), 147 deletions(-)
> >
> [...]
> 
> You didn't seem to say how it was tested.
> 
> If you have verified that "echo > /dev/watchdog" produces register dumps and
> reboots the board, then the whole series:
> 
> Acked-by: David Daney <david.daney@cavium.com>

Yes, that was exactly my test case:

	root@dsr-1000n:~$ cat /dev/watchdog
	cat: read error: Invalid argument[   44.835019] watchdog watchdog0: watchdog did not stop!

	root@dsr-1000n:~$
	*** NMI Watchdog interrupt on Core 0x0 ***
	        $0      0x0000000000000000      at      0x0000000010008ce0
	[...]
	        sum0    0x0100000000000000      en0     0x01206f4500008000
	*** Chip soft reset soon ***


	U-Boot 1.1.1 (Development build, svnversion: exported) (Build time: Mar 22 2010
- 12:14:14)

	Warning: Board descriptor tuple not found in eeprom, using defaults
	CUST_DSR1000 board revision major:2, minor:0, serial #: unknown
	OCTEON CN5010-SCP pass 1.1, Core clock: 500 MHz, DDR clock: 200 MHz (400 Mhz data rate)

> Thanks for doing this, I had been meaning to make the conversion myself.

One further improvement idea would be to get the register dump logged
also with printk() or panic() so that it could be captured with netconsole
or mtdoops (at least in some cases). Not sure what it would require.
Calling printk() or panic() from the NMI handler didn't quite produce
an expected result...

A.

Re: [PATCH 1/3] watchdog: octeon: convert to WATCHDOG_CORE API

[David Daney]

On 03/30/2015 11:51 AM, Aaro Koskinen wrote:
[...]
>
>> Thanks for doing this, I had been meaning to make the conversion myself.
>
> One further improvement idea would be to get the register dump logged
> also with printk() or panic() so that it could be captured with netconsole
> or mtdoops (at least in some cases). Not sure what it would require.
> Calling printk() or panic() from the NMI handler didn't quite produce
> an expected result...

In the handler we are running with a private stack, and at 
CP0_Status[ERL].  The result is that no access to the current task 
structure is possible, and interrupts are disabled.  So there is very 
little you can do.

Polling characters out of the UART works fine though.


>
> A.
>

Re: [PATCH 1/3] watchdog: octeon: convert to WATCHDOG_CORE API

[David Daney]

On 03/30/2015 11:51 AM, Aaro Koskinen wrote:
[...]
>
>> Thanks for doing this, I had been meaning to make the conversion myself.
>
> One further improvement idea would be to get the register dump logged
> also with printk() or panic() so that it could be captured with netconsole
> or mtdoops (at least in some cases). Not sure what it would require.
> Calling printk() or panic() from the NMI handler didn't quite produce
> an expected result...

In the handler we are running with a private stack, and at 
CP0_Status[ERL].  The result is that no access to the current task 
structure is possible, and interrupts are disabled.  So there is very 
little you can do.

Polling characters out of the UART works fine though.


>
> A.
>