All of lore.kernel.org
 help / color / mirror / Atom feed
* add bcm47xx watchdog driver
@ 2009-06-04 20:24 matthieu castet
  2009-06-05 13:58 ` Florian Fainelli
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: matthieu castet @ 2009-06-04 20:24 UTC (permalink / raw)
  To: wim, Linux Kernel list, linux-mips; +Cc: Aleksandar Radovanovic

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



[-- Attachment #2: bcm47xx_watchdog.diff --]
[-- Type: text/x-diff, Size: 8392 bytes --]

This add watchdog driver for broadcom 47xx device.
It uses the ssb subsytem to access embeded watchdog device.

Because the watchdog timeout is very short (about 2s), a soft timer is used
to increase the watchdog period.

Note : A patch for exporting the ssb_watchdog_timer_set will
be submitted on next linux-mips merge. Without this patch it can't 
be build as a module.

Signed-off-by: Aleksandar Radovanovic <biblbroks@sezampro.rs>
Signed-off-by: Matthieu CASTET <castet.matthieu@free.fr>
Index: linux-2.6/drivers/watchdog/Kconfig
===================================================================
--- linux-2.6.orig/drivers/watchdog/Kconfig	2009-05-25 22:22:02.000000000 +0200
+++ linux-2.6/drivers/watchdog/Kconfig	2009-05-25 22:26:06.000000000 +0200
@@ -764,6 +764,12 @@
 	help
 	  Hardware driver for the built-in watchdog timer on TXx9 MIPS SoCs.
 
+config BCM47XX_WDT
+    tristate "Broadcom BCM47xx Watchdog Timer"
+    depends on BCM47XX
+    help
+      Hardware driver for the Broadcom BCM47xx Watchog Timer.
+
 # PARISC Architecture
 
 # POWERPC Architecture
Index: linux-2.6/drivers/watchdog/Makefile
===================================================================
--- linux-2.6.orig/drivers/watchdog/Makefile	2009-05-25 22:22:02.000000000 +0200
+++ linux-2.6/drivers/watchdog/Makefile	2009-05-25 23:02:27.000000000 +0200
@@ -105,6 +105,7 @@
 obj-$(CONFIG_SIBYTE_WDOG) += sb_wdog.o
 obj-$(CONFIG_AR7_WDT) += ar7_wdt.o
 obj-$(CONFIG_TXX9_WDT) += txx9wdt.o
+obj-$(CONFIG_BCM47XX_WDT) += bcm47xx_wdt.o
 
 # PARISC Architecture
 
Index: linux-2.6/drivers/watchdog/bcm47xx_wdt.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6/drivers/watchdog/bcm47xx_wdt.c	2009-05-25 23:29:25.000000000 +0200
@@ -0,0 +1,289 @@
+/*
+ *  Watchdog driver for Broadcom BCM47XX
+ *
+ *  Copyright (C) 2008 Aleksandar Radovanovic <biblbroks@sezampro.rs>
+ *
+ *  This program is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU General Public License
+ *  as published by the Free Software Foundation; either version
+ *  2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/bitops.h>
+#include <linux/errno.h>
+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/notifier.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+#include <linux/watchdog.h>
+#include <linux/timer.h>
+#include <linux/jiffies.h>
+#include <linux/ssb/ssb_embedded.h>
+#include <asm/mach-bcm47xx/bcm47xx.h>
+
+#define DRV_NAME		"bcm47xx_wdt"
+
+#define WDT_DEFAULT_TIME	30	/* seconds */
+#define WDT_MAX_TIME		256	/* seconds */
+
+static int wdt_time = WDT_DEFAULT_TIME;
+static int nowayout = WATCHDOG_NOWAYOUT;
+
+module_param(wdt_time, int, 0);
+MODULE_PARM_DESC(wdt_time, "Watchdog time in seconds. (default="
+				__MODULE_STRING(WDT_DEFAULT_TIME) ")");
+
+#ifdef CONFIG_WATCHDOG_NOWAYOUT
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout,
+		"Watchdog cannot be stopped once started (default="
+				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+#endif
+
+static struct platform_device *bcm47xx_wdt_platform_device;
+
+static unsigned long bcm47xx_wdt_busy;
+static char expect_release;
+static struct timer_list wdt_timer;
+static atomic_t ticks;
+
+static inline void bcm47xx_wdt_hw_start(void)
+{
+	/* this is 2,5s on 100Mhz clock  and 2s on 133 Mhz */
+	ssb_watchdog_timer_set(&ssb_bcm47xx, 0xfffffff);
+}
+
+static inline int bcm47xx_wdt_hw_stop(void)
+{
+	return ssb_watchdog_timer_set(&ssb_bcm47xx, 0);
+}
+
+static void bcm47xx_timer_tick(unsigned long unused)
+{
+	if(!atomic_dec_and_test(&ticks)) {
+		bcm47xx_wdt_hw_start();
+		mod_timer(&wdt_timer, jiffies + HZ);
+	}
+	else {
+		printk(KERN_CRIT PFX "Watchdog will fire soon!!!.\n");
+	}
+}
+
+static inline void bcm47xx_wdt_pet(void)
+{
+	atomic_set(&ticks, wdt_time);
+}
+
+static void bcm47xx_wdt_start(void)
+{
+	bcm47xx_wdt_pet();
+	bcm47xx_timer_tick(0);
+}
+
+static void bcm47xx_wdt_pause(void)
+{
+	del_timer(&wdt_timer);
+	bcm47xx_wdt_hw_stop();
+}
+
+static void bcm47xx_wdt_stop(void)
+{
+	bcm47xx_wdt_pause();
+}
+
+static int bcm47xx_wdt_settimeout(int new_time)
+{
+	if ((new_time <= 0) || (new_time > WDT_MAX_TIME))
+		return -EINVAL;
+
+	wdt_time = new_time;
+	return 0;
+}
+
+static int bcm47xx_wdt_open(struct inode *inode, struct file *file)
+{
+	if (test_and_set_bit(0, &bcm47xx_wdt_busy))
+		return -EBUSY;
+
+	bcm47xx_wdt_start();
+	return nonseekable_open(inode, file);
+}
+
+static int bcm47xx_wdt_release(struct inode *inode, struct file *file)
+{
+	if (expect_release == 42) {
+		bcm47xx_wdt_stop();
+	} else {
+		printk(KERN_CRIT DRV_NAME ": Unexpected close, not stopping watchdog!\n");
+		bcm47xx_wdt_start();
+	}
+
+	clear_bit(0, &bcm47xx_wdt_busy);
+	expect_release = 0;
+	return 0;
+}
+
+static ssize_t bcm47xx_wdt_write (struct file *file, const char __user *data,
+			      size_t len, loff_t * ppos)
+{
+	if (len) {
+		if (!nowayout) {
+			size_t i;
+
+			expect_release = 0;
+
+			for (i = 0; i != len; i++) {
+				char c;
+				if (get_user(c, data + i))
+					return -EFAULT;
+				if (c == 'V')
+					expect_release = 42;
+			}
+		}
+		bcm47xx_wdt_pet();
+	}
+	return len;
+}
+
+static struct watchdog_info bcm47xx_wdt_info = {
+	.identity 	= DRV_NAME,
+	.options 	= WDIOF_SETTIMEOUT |
+				WDIOF_KEEPALIVEPING |
+				WDIOF_MAGICCLOSE,
+};
+
+static long bcm47xx_wdt_ioctl(struct file *file,
+					unsigned int cmd, unsigned long arg)
+{
+	void __user *argp = (void __user *)arg;
+	int __user *p = argp;
+	int new_value, retval = -EINVAL;;
+
+	switch (cmd) {
+	case WDIOC_GETSUPPORT:
+		return copy_to_user(argp, &bcm47xx_wdt_info,
+				sizeof(bcm47xx_wdt_info)) ? -EFAULT : 0;
+
+	case WDIOC_GETSTATUS:
+	case WDIOC_GETBOOTSTATUS:
+		return put_user(0, p);
+
+	case WDIOC_SETOPTIONS:
+		if (get_user(new_value, p))
+			return -EFAULT;
+
+		if (new_value & WDIOS_DISABLECARD) {
+			bcm47xx_wdt_stop();
+			retval = 0;
+		}
+
+		if (new_value & WDIOS_ENABLECARD) {
+			bcm47xx_wdt_start();
+			retval = 0;
+		}
+
+		return retval;
+
+	case WDIOC_KEEPALIVE:
+		bcm47xx_wdt_pet();
+		return 0;
+
+	case WDIOC_SETTIMEOUT:
+		if (get_user(new_value, p))
+			return -EFAULT;
+
+		if (bcm47xx_wdt_settimeout(new_value))
+			return -EINVAL;
+
+		bcm47xx_wdt_pet();
+
+	case WDIOC_GETTIMEOUT:
+		return put_user(wdt_time, p);
+
+	default:
+		return -ENOTTY;
+	}
+}
+
+static int bcm47xx_wdt_notify_sys(struct notifier_block *this, unsigned long code,
+	void *unused)
+{
+	if (code == SYS_DOWN || code == SYS_HALT)
+		bcm47xx_wdt_stop();
+	return NOTIFY_DONE;
+}
+
+static const struct file_operations bcm47xx_wdt_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.unlocked_ioctl	= bcm47xx_wdt_ioctl,
+	.open		= bcm47xx_wdt_open,
+	.release	= bcm47xx_wdt_release,
+	.write		= bcm47xx_wdt_write,
+};
+
+static struct miscdevice bcm47xx_wdt_miscdev = {
+	.minor		= WATCHDOG_MINOR,
+	.name		= "watchdog",
+	.fops		= &bcm47xx_wdt_fops,
+};
+
+static struct notifier_block bcm47xx_wdt_notifier = {
+	.notifier_call = bcm47xx_wdt_notify_sys,
+};
+
+static int __init bcm47xx_wdt_init(void)
+{
+	int ret;
+
+	if (bcm47xx_wdt_hw_stop() < 0)
+		return -ENODEV;
+
+	setup_timer(&wdt_timer, bcm47xx_timer_tick, 0L);
+
+	if (bcm47xx_wdt_settimeout(wdt_time)) {
+		bcm47xx_wdt_settimeout(WDT_DEFAULT_TIME);
+		printk(KERN_INFO DRV_NAME ": wdt_time value must be 1 <= wdt_time <= 256, using %d\n",
+			wdt_time);
+	}
+
+	ret = register_reboot_notifier(&bcm47xx_wdt_notifier);
+	if (ret)
+		ret;
+
+	ret = misc_register(&bcm47xx_wdt_miscdev);
+	if (ret) {
+		unregister_reboot_notifier(&bcm47xx_wdt_notifier);
+		return ret;
+	}
+
+	printk(KERN_INFO "BCM47xx Watchdog Timer enabled (%d seconds%s)\n",
+				wdt_time, nowayout ? ", nowayout" : "");
+	return 0;
+}
+
+static void __exit bcm47xx_wdt_exit(void)
+{
+	if (!nowayout)
+		bcm47xx_wdt_stop();
+
+	misc_deregister(&bcm47xx_wdt_miscdev);
+
+	unregister_reboot_notifier(&bcm47xx_wdt_notifier);
+
+	return 0;
+}
+
+module_init(bcm47xx_wdt_init);
+module_exit(bcm47xx_wdt_exit);
+
+MODULE_AUTHOR("Aleksandar Radovanovic");
+MODULE_DESCRIPTION("Watchdog driver for Broadcom BCM47xx");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
+MODULE_ALIAS("platform:bcm47xx_wdt");

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: add bcm47xx watchdog driver
  2009-06-04 20:24 add bcm47xx watchdog driver matthieu castet
@ 2009-06-05 13:58 ` Florian Fainelli
  2009-06-05 14:58   ` castet.matthieu
  2009-06-05 16:57   ` add bcm47xx watchdog driver v2 matthieu castet
  2009-06-05 19:48 ` add bcm47xx watchdog driver Andrew Morton
  2009-06-05 19:50 ` Andrew Morton
  2 siblings, 2 replies; 13+ messages in thread
From: Florian Fainelli @ 2009-06-05 13:58 UTC (permalink / raw)
  To: matthieu castet
  Cc: wim, Linux Kernel list, linux-mips, Aleksandar Radovanovic

Salut Matthieu,

Le Thursday 04 June 2009 22:24:56 matthieu castet, vous avez écrit :
> This add watchdog driver for broadcom 47xx device.
> It uses the ssb subsytem to access embeded watchdog device.
>
> Because the watchdog timeout is very short (about 2s), a soft timer is used
> to increase the watchdog period.
>
> Note : A patch for exporting the ssb_watchdog_timer_set will
> be submitted on next linux-mips merge. Without this patch it can't
> be build as a module.

Your driver looks good, could you turn this into a platform device/driver 
instead ? You declare bcm47xx_wdt_platform_device which is unused and you 
also declare a MODULE_ALIAS which suggets it is one.

You are also missing your name in both the header the the MODULE_AUTHOR macro.
-- 
Best regards, Florian Fainelli
Email : florian@openwrt.org
http://openwrt.org
-------------------------------

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: add bcm47xx watchdog driver
  2009-06-05 13:58 ` Florian Fainelli
@ 2009-06-05 14:58   ` castet.matthieu
  2009-06-06 10:58     ` Florian Fainelli
  2009-06-05 16:57   ` add bcm47xx watchdog driver v2 matthieu castet
  1 sibling, 1 reply; 13+ messages in thread
From: castet.matthieu @ 2009-06-05 14:58 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: matthieu castet, wim, Linux Kernel list, linux-mips,
	Aleksandar Radovanovic

Hi Florian,

Quoting Florian Fainelli <florian@openwrt.org>:

>
> Your driver looks good, could you turn this into a platform device/driver
> instead ? You declare bcm47xx_wdt_platform_device which is unused and you
> also declare a MODULE_ALIAS which suggets it is one.
What's the advantage of using platform device/driver ?
Not all watchdog driver use it (for example softdog).
This seems useless in this case because the driver don't have any resource,
don't care about suspend/resume and add complexity in the code (2 registers in
module probe, ...).


Matthieu

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: add bcm47xx watchdog driver v2
  2009-06-05 13:58 ` Florian Fainelli
  2009-06-05 14:58   ` castet.matthieu
@ 2009-06-05 16:57   ` matthieu castet
  1 sibling, 0 replies; 13+ messages in thread
From: matthieu castet @ 2009-06-05 16:57 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: wim, Linux Kernel list, linux-mips, Aleksandar Radovanovic

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

Florian Fainelli wrote:
> Salut Matthieu,
> 
> Le Thursday 04 June 2009 22:24:56 matthieu castet, vous avez écrit :
>> This add watchdog driver for broadcom 47xx device.
>> It uses the ssb subsytem to access embeded watchdog device.
>>
>> Because the watchdog timeout is very short (about 2s), a soft timer is used
>> to increase the watchdog period.
>>
>> Note : A patch for exporting the ssb_watchdog_timer_set will
>> be submitted on next linux-mips merge. Without this patch it can't
>> be build as a module.
> 
> Your driver looks good, could you turn this into a platform device/driver 
> instead ? You declare bcm47xx_wdt_platform_device which is unused and you 
> also declare a MODULE_ALIAS which suggets it is one.
oops I attached the wrong patch.

This one should fix some build failure ;)

Matthieu

[-- Attachment #2: bcm47xx_watchdog.diff --]
[-- Type: text/x-diff, Size: 8285 bytes --]

This add watchdog driver for broadcom 47xx device.
It uses the ssb subsytem to access embeded watchdog device.

Because the watchdog timeout is very short (about 2s), a soft timer is used
to increase the watchdog period.

Note : A patch for exporting the ssb_watchdog_timer_set will
be submitted on next linux-mips merge. Without this patch it can't 
be build as a module.

Signed-off-by: Aleksandar Radovanovic <biblbroks@sezampro.rs>
Signed-off-by: Matthieu CASTET <castet.matthieu@free.fr>
Index: linux-2.6/drivers/watchdog/Kconfig
===================================================================
--- linux-2.6.orig/drivers/watchdog/Kconfig	2009-05-25 22:22:02.000000000 +0200
+++ linux-2.6/drivers/watchdog/Kconfig	2009-05-25 22:26:06.000000000 +0200
@@ -764,6 +764,12 @@
 	help
 	  Hardware driver for the built-in watchdog timer on TXx9 MIPS SoCs.
 
+config BCM47XX_WDT
+    tristate "Broadcom BCM47xx Watchdog Timer"
+    depends on BCM47XX
+    help
+      Hardware driver for the Broadcom BCM47xx Watchog Timer.
+
 # PARISC Architecture
 
 # POWERPC Architecture
Index: linux-2.6/drivers/watchdog/Makefile
===================================================================
--- linux-2.6.orig/drivers/watchdog/Makefile	2009-05-25 22:22:02.000000000 +0200
+++ linux-2.6/drivers/watchdog/Makefile	2009-05-25 23:02:27.000000000 +0200
@@ -105,6 +105,7 @@
 obj-$(CONFIG_SIBYTE_WDOG) += sb_wdog.o
 obj-$(CONFIG_AR7_WDT) += ar7_wdt.o
 obj-$(CONFIG_TXX9_WDT) += txx9wdt.o
+obj-$(CONFIG_BCM47XX_WDT) += bcm47xx_wdt.o
 
 # PARISC Architecture
 
Index: linux-2.6/drivers/watchdog/bcm47xx_wdt.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6/drivers/watchdog/bcm47xx_wdt.c	2009-06-05 18:52:51.000000000 +0200
@@ -0,0 +1,284 @@
+/*
+ *  Watchdog driver for Broadcom BCM47XX
+ *
+ *  Copyright (C) 2008 Aleksandar Radovanovic <biblbroks@sezampro.rs>
+ *
+ *  This program is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU General Public License
+ *  as published by the Free Software Foundation; either version
+ *  2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/bitops.h>
+#include <linux/errno.h>
+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/reboot.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+#include <linux/watchdog.h>
+#include <linux/timer.h>
+#include <linux/jiffies.h>
+#include <linux/ssb/ssb_embedded.h>
+#include <asm/mach-bcm47xx/bcm47xx.h>
+
+#define DRV_NAME		"bcm47xx_wdt"
+
+#define WDT_DEFAULT_TIME	30	/* seconds */
+#define WDT_MAX_TIME		256	/* seconds */
+
+static int wdt_time = WDT_DEFAULT_TIME;
+static int nowayout = WATCHDOG_NOWAYOUT;
+
+module_param(wdt_time, int, 0);
+MODULE_PARM_DESC(wdt_time, "Watchdog time in seconds. (default="
+				__MODULE_STRING(WDT_DEFAULT_TIME) ")");
+
+#ifdef CONFIG_WATCHDOG_NOWAYOUT
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout,
+		"Watchdog cannot be stopped once started (default="
+				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+#endif
+
+static unsigned long bcm47xx_wdt_busy;
+static char expect_release;
+static struct timer_list wdt_timer;
+static atomic_t ticks;
+
+static inline void bcm47xx_wdt_hw_start(void)
+{
+	/* this is 2,5s on 100Mhz clock  and 2s on 133 Mhz */
+	ssb_watchdog_timer_set(&ssb_bcm47xx, 0xfffffff);
+}
+
+static inline int bcm47xx_wdt_hw_stop(void)
+{
+	return ssb_watchdog_timer_set(&ssb_bcm47xx, 0);
+}
+
+static void bcm47xx_timer_tick(unsigned long unused)
+{
+	if(!atomic_dec_and_test(&ticks)) {
+		bcm47xx_wdt_hw_start();
+		mod_timer(&wdt_timer, jiffies + HZ);
+	}
+	else {
+		printk(KERN_CRIT DRV_NAME "Watchdog will fire soon!!!\n");
+	}
+}
+
+static inline void bcm47xx_wdt_pet(void)
+{
+	atomic_set(&ticks, wdt_time);
+}
+
+static void bcm47xx_wdt_start(void)
+{
+	bcm47xx_wdt_pet();
+	bcm47xx_timer_tick(0);
+}
+
+static void bcm47xx_wdt_pause(void)
+{
+	del_timer(&wdt_timer);
+	bcm47xx_wdt_hw_stop();
+}
+
+static void bcm47xx_wdt_stop(void)
+{
+	bcm47xx_wdt_pause();
+}
+
+static int bcm47xx_wdt_settimeout(int new_time)
+{
+	if ((new_time <= 0) || (new_time > WDT_MAX_TIME))
+		return -EINVAL;
+
+	wdt_time = new_time;
+	return 0;
+}
+
+static int bcm47xx_wdt_open(struct inode *inode, struct file *file)
+{
+	if (test_and_set_bit(0, &bcm47xx_wdt_busy))
+		return -EBUSY;
+
+	bcm47xx_wdt_start();
+	return nonseekable_open(inode, file);
+}
+
+static int bcm47xx_wdt_release(struct inode *inode, struct file *file)
+{
+	if (expect_release == 42) {
+		bcm47xx_wdt_stop();
+	} else {
+		printk(KERN_CRIT DRV_NAME ": Unexpected close, not stopping watchdog!\n");
+		bcm47xx_wdt_start();
+	}
+
+	clear_bit(0, &bcm47xx_wdt_busy);
+	expect_release = 0;
+	return 0;
+}
+
+static ssize_t bcm47xx_wdt_write (struct file *file, const char __user *data,
+			      size_t len, loff_t * ppos)
+{
+	if (len) {
+		if (!nowayout) {
+			size_t i;
+
+			expect_release = 0;
+
+			for (i = 0; i != len; i++) {
+				char c;
+				if (get_user(c, data + i))
+					return -EFAULT;
+				if (c == 'V')
+					expect_release = 42;
+			}
+		}
+		bcm47xx_wdt_pet();
+	}
+	return len;
+}
+
+static struct watchdog_info bcm47xx_wdt_info = {
+	.identity 	= DRV_NAME,
+	.options 	= WDIOF_SETTIMEOUT |
+				WDIOF_KEEPALIVEPING |
+				WDIOF_MAGICCLOSE,
+};
+
+static long bcm47xx_wdt_ioctl(struct file *file,
+					unsigned int cmd, unsigned long arg)
+{
+	void __user *argp = (void __user *)arg;
+	int __user *p = argp;
+	int new_value, retval = -EINVAL;;
+
+	switch (cmd) {
+	case WDIOC_GETSUPPORT:
+		return copy_to_user(argp, &bcm47xx_wdt_info,
+				sizeof(bcm47xx_wdt_info)) ? -EFAULT : 0;
+
+	case WDIOC_GETSTATUS:
+	case WDIOC_GETBOOTSTATUS:
+		return put_user(0, p);
+
+	case WDIOC_SETOPTIONS:
+		if (get_user(new_value, p))
+			return -EFAULT;
+
+		if (new_value & WDIOS_DISABLECARD) {
+			bcm47xx_wdt_stop();
+			retval = 0;
+		}
+
+		if (new_value & WDIOS_ENABLECARD) {
+			bcm47xx_wdt_start();
+			retval = 0;
+		}
+
+		return retval;
+
+	case WDIOC_KEEPALIVE:
+		bcm47xx_wdt_pet();
+		return 0;
+
+	case WDIOC_SETTIMEOUT:
+		if (get_user(new_value, p))
+			return -EFAULT;
+
+		if (bcm47xx_wdt_settimeout(new_value))
+			return -EINVAL;
+
+		bcm47xx_wdt_pet();
+
+	case WDIOC_GETTIMEOUT:
+		return put_user(wdt_time, p);
+
+	default:
+		return -ENOTTY;
+	}
+}
+
+static int bcm47xx_wdt_notify_sys(struct notifier_block *this, unsigned long code,
+	void *unused)
+{
+	if (code == SYS_DOWN || code == SYS_HALT)
+		bcm47xx_wdt_stop();
+	return NOTIFY_DONE;
+}
+
+static const struct file_operations bcm47xx_wdt_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.unlocked_ioctl	= bcm47xx_wdt_ioctl,
+	.open		= bcm47xx_wdt_open,
+	.release	= bcm47xx_wdt_release,
+	.write		= bcm47xx_wdt_write,
+};
+
+static struct miscdevice bcm47xx_wdt_miscdev = {
+	.minor		= WATCHDOG_MINOR,
+	.name		= "watchdog",
+	.fops		= &bcm47xx_wdt_fops,
+};
+
+static struct notifier_block bcm47xx_wdt_notifier = {
+	.notifier_call = bcm47xx_wdt_notify_sys,
+};
+
+static int __init bcm47xx_wdt_init(void)
+{
+	int ret;
+
+	if (bcm47xx_wdt_hw_stop() < 0)
+		return -ENODEV;
+
+	setup_timer(&wdt_timer, bcm47xx_timer_tick, 0L);
+
+	if (bcm47xx_wdt_settimeout(wdt_time)) {
+		bcm47xx_wdt_settimeout(WDT_DEFAULT_TIME);
+		printk(KERN_INFO DRV_NAME ": wdt_time value must be 1 <= wdt_time <= 256, using %d\n",
+			wdt_time);
+	}
+
+	ret = register_reboot_notifier(&bcm47xx_wdt_notifier);
+	if (ret)
+		return ret;
+
+	ret = misc_register(&bcm47xx_wdt_miscdev);
+	if (ret) {
+		unregister_reboot_notifier(&bcm47xx_wdt_notifier);
+		return ret;
+	}
+
+	printk(KERN_INFO "BCM47xx Watchdog Timer enabled (%d seconds%s)\n",
+				wdt_time, nowayout ? ", nowayout" : "");
+	return 0;
+}
+
+static void __exit bcm47xx_wdt_exit(void)
+{
+	if (!nowayout)
+		bcm47xx_wdt_stop();
+
+	misc_deregister(&bcm47xx_wdt_miscdev);
+
+	unregister_reboot_notifier(&bcm47xx_wdt_notifier);
+}
+
+module_init(bcm47xx_wdt_init);
+module_exit(bcm47xx_wdt_exit);
+
+MODULE_AUTHOR("Aleksandar Radovanovic");
+MODULE_DESCRIPTION("Watchdog driver for Broadcom BCM47xx");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: add bcm47xx watchdog driver
  2009-06-04 20:24 add bcm47xx watchdog driver matthieu castet
  2009-06-05 13:58 ` Florian Fainelli
@ 2009-06-05 19:48 ` Andrew Morton
  2009-06-05 20:30   ` matthieu castet
  2009-06-05 19:50 ` Andrew Morton
  2 siblings, 1 reply; 13+ messages in thread
From: Andrew Morton @ 2009-06-05 19:48 UTC (permalink / raw)
  To: matthieu castet; +Cc: wim, linux-kernel, linux-mips, biblbroks

On Thu, 04 Jun 2009 22:24:56 +0200
matthieu castet <castet.matthieu@free.fr> wrote:

> 
>
> This add watchdog driver for broadcom 47xx device.
> It uses the ssb subsytem to access embeded watchdog device.
> 
> Because the watchdog timeout is very short (about 2s), a soft timer is used
> to increase the watchdog period.
> 
> Note : A patch for exporting the ssb_watchdog_timer_set will
> be submitted on next linux-mips merge. Without this patch it can't 
> be build as a module.
> 
>
> ...
>
> --- linux-2.6.orig/drivers/watchdog/Kconfig	2009-05-25 22:22:02.000000000 +0200
> +++ linux-2.6/drivers/watchdog/Kconfig	2009-05-25 22:26:06.000000000 +0200
> @@ -764,6 +764,12 @@
>  	help
>  	  Hardware driver for the built-in watchdog timer on TXx9 MIPS SoCs.
>  
> +config BCM47XX_WDT
> +    tristate "Broadcom BCM47xx Watchdog Timer"
> +    depends on BCM47XX
> +    help
> +      Hardware driver for the Broadcom BCM47xx Watchog Timer.
> +

Please indent the kconfig body with a single tab character.

>
> ...
>
> +#define DRV_NAME		"bcm47xx_wdt"
> +
> +#define WDT_DEFAULT_TIME	30	/* seconds */
> +#define WDT_MAX_TIME		256	/* seconds */
> +
> +static int wdt_time = WDT_DEFAULT_TIME;
> +static int nowayout = WATCHDOG_NOWAYOUT;
> +
> +module_param(wdt_time, int, 0);
> +MODULE_PARM_DESC(wdt_time, "Watchdog time in seconds. (default="
> +				__MODULE_STRING(WDT_DEFAULT_TIME) ")");
> +
> +#ifdef CONFIG_WATCHDOG_NOWAYOUT
> +module_param(nowayout, int, 0);
> +MODULE_PARM_DESC(nowayout,
> +		"Watchdog cannot be stopped once started (default="
> +				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +#endif

hm, now what's happening with the third arg to module_param()?

> +static struct platform_device *bcm47xx_wdt_platform_device;
> +
> +static unsigned long bcm47xx_wdt_busy;
> +static char expect_release;
> +static struct timer_list wdt_timer;
> +static atomic_t ticks;
> +
> +static inline void bcm47xx_wdt_hw_start(void)
> +{
> +	/* this is 2,5s on 100Mhz clock  and 2s on 133 Mhz */
> +	ssb_watchdog_timer_set(&ssb_bcm47xx, 0xfffffff);
> +}
> +
> +static inline int bcm47xx_wdt_hw_stop(void)
> +{
> +	return ssb_watchdog_timer_set(&ssb_bcm47xx, 0);
> +}
> +
> +static void bcm47xx_timer_tick(unsigned long unused)
> +{
> +	if(!atomic_dec_and_test(&ticks)) {

Please pass this patch (and all others) through scripts/checkpatch.pl
and review the resulting output.

> +		bcm47xx_wdt_hw_start();
> +		mod_timer(&wdt_timer, jiffies + HZ);
> +	}
> +	else {
> +		printk(KERN_CRIT PFX "Watchdog will fire soon!!!.\n");
> +	}
> +}
> +
> +static inline void bcm47xx_wdt_pet(void)
> +{
> +	atomic_set(&ticks, wdt_time);
> +}

What does "pet" stand for?

> +static void bcm47xx_wdt_start(void)
> +{
> +	bcm47xx_wdt_pet();
> +	bcm47xx_timer_tick(0);
> +}
> +
> +static void bcm47xx_wdt_pause(void)
> +{
> +	del_timer(&wdt_timer);

Should this be del_timer_sync()?  The timer callback can still be
executing after del_timer() returns.

> +	bcm47xx_wdt_hw_stop();
> +}
> +
> +static void bcm47xx_wdt_stop(void)
> +{
> +	bcm47xx_wdt_pause();
> +}
> +
> +static int bcm47xx_wdt_settimeout(int new_time)
> +{
> +	if ((new_time <= 0) || (new_time > WDT_MAX_TIME))
> +		return -EINVAL;
> +
> +	wdt_time = new_time;
> +	return 0;
> +}
> +
> +static int bcm47xx_wdt_open(struct inode *inode, struct file *file)
> +{
> +	if (test_and_set_bit(0, &bcm47xx_wdt_busy))
> +		return -EBUSY;
> +
> +	bcm47xx_wdt_start();
> +	return nonseekable_open(inode, file);
> +}
> +
> +static int bcm47xx_wdt_release(struct inode *inode, struct file *file)
> +{
> +	if (expect_release == 42) {
> +		bcm47xx_wdt_stop();
> +	} else {
> +		printk(KERN_CRIT DRV_NAME ": Unexpected close, not stopping watchdog!\n");

Can this happen?

> +		bcm47xx_wdt_start();
> +	}
> +
> +	clear_bit(0, &bcm47xx_wdt_busy);
> +	expect_release = 0;
> +	return 0;
> +}
> +
>
> ...
>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: add bcm47xx watchdog driver
  2009-06-04 20:24 add bcm47xx watchdog driver matthieu castet
  2009-06-05 13:58 ` Florian Fainelli
  2009-06-05 19:48 ` add bcm47xx watchdog driver Andrew Morton
@ 2009-06-05 19:50 ` Andrew Morton
  2009-06-05 20:03   ` matthieu castet
  2 siblings, 1 reply; 13+ messages in thread
From: Andrew Morton @ 2009-06-05 19:50 UTC (permalink / raw)
  To: matthieu castet; +Cc: wim, linux-kernel, linux-mips, biblbroks

On Thu, 04 Jun 2009 22:24:56 +0200
matthieu castet <castet.matthieu@free.fr> wrote:

> This add watchdog driver for broadcom 47xx device.
> It uses the ssb subsytem to access embeded watchdog device.
> 
> ...
>
> --- linux-2.6.orig/drivers/watchdog/Kconfig	2009-05-25 22:22:02.000000000 +0200
> +++ linux-2.6/drivers/watchdog/Kconfig	2009-05-25 22:26:06.000000000 +0200
> @@ -764,6 +764,12 @@
>  	help
>  	  Hardware driver for the built-in watchdog timer on TXx9 MIPS SoCs.
>  
> +config BCM47XX_WDT
> +    tristate "Broadcom BCM47xx Watchdog Timer"
> +    depends on BCM47XX
> +    help
> +      Hardware driver for the Broadcom BCM47xx Watchog Timer.

Shouldn't there be some SSB dependencies in Kconfig here?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: add bcm47xx watchdog driver
  2009-06-05 19:50 ` Andrew Morton
@ 2009-06-05 20:03   ` matthieu castet
  0 siblings, 0 replies; 13+ messages in thread
From: matthieu castet @ 2009-06-05 20:03 UTC (permalink / raw)
  To: Andrew Morton; +Cc: wim, linux-kernel, linux-mips, biblbroks

Andrew Morton wrote:
> On Thu, 04 Jun 2009 22:24:56 +0200
> matthieu castet <castet.matthieu@free.fr> wrote:
> 
>> This add watchdog driver for broadcom 47xx device.
>> It uses the ssb subsytem to access embeded watchdog device.
>>
>> ...
>>
>> --- linux-2.6.orig/drivers/watchdog/Kconfig	2009-05-25 22:22:02.000000000 +0200
>> +++ linux-2.6/drivers/watchdog/Kconfig	2009-05-25 22:26:06.000000000 +0200
>> @@ -764,6 +764,12 @@
>>  	help
>>  	  Hardware driver for the built-in watchdog timer on TXx9 MIPS SoCs.
>>  
>> +config BCM47XX_WDT
>> +    tristate "Broadcom BCM47xx Watchdog Timer"
>> +    depends on BCM47XX
>> +    help
>> +      Hardware driver for the Broadcom BCM47xx Watchog Timer.
> 
> Shouldn't there be some SSB dependencies in Kconfig here?
No, because :
config BCM47XX
     bool "BCM47XX based boards"
[...]
     select SSB
     select SSB_DRIVER_MIPS
     select SSB_DRIVER_EXTIF
     select SSB_EMBEDDED
     select SSB_PCICORE_HOSTMODE if PCI

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: add bcm47xx watchdog driver
  2009-06-05 19:48 ` add bcm47xx watchdog driver Andrew Morton
@ 2009-06-05 20:30   ` matthieu castet
  2009-06-08 14:15     ` Florian Fainelli
  0 siblings, 1 reply; 13+ messages in thread
From: matthieu castet @ 2009-06-05 20:30 UTC (permalink / raw)
  To: Andrew Morton; +Cc: wim, linux-kernel, linux-mips, biblbroks

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

Andrew Morton wrote:
> On Thu, 04 Jun 2009 22:24:56 +0200
> matthieu castet <castet.matthieu@free.fr> wrote:
> 
>>
>> This add watchdog driver for broadcom 47xx device.
>> It uses the ssb subsytem to access embeded watchdog device.
>>
>> Because the watchdog timeout is very short (about 2s), a soft timer is used
>> to increase the watchdog period.
>>
>> Note : A patch for exporting the ssb_watchdog_timer_set will
>> be submitted on next linux-mips merge. Without this patch it can't 
>> be build as a module.
>>
>>
>> ...
>>
>> --- linux-2.6.orig/drivers/watchdog/Kconfig	2009-05-25 22:22:02.000000000 +0200
>> +++ linux-2.6/drivers/watchdog/Kconfig	2009-05-25 22:26:06.000000000 +0200
>> @@ -764,6 +764,12 @@
>>  	help
>>  	  Hardware driver for the built-in watchdog timer on TXx9 MIPS SoCs.
>>  
>> +config BCM47XX_WDT
>> +    tristate "Broadcom BCM47xx Watchdog Timer"
>> +    depends on BCM47XX
>> +    help
>> +      Hardware driver for the Broadcom BCM47xx Watchog Timer.
>> +
> 
> Please indent the kconfig body with a single tab character.
> 
Done

>> ...
>>
>> +#define DRV_NAME		"bcm47xx_wdt"
>> +
>> +#define WDT_DEFAULT_TIME	30	/* seconds */
>> +#define WDT_MAX_TIME		256	/* seconds */
>> +
>> +static int wdt_time = WDT_DEFAULT_TIME;
>> +static int nowayout = WATCHDOG_NOWAYOUT;
>> +
>> +module_param(wdt_time, int, 0);
>> +MODULE_PARM_DESC(wdt_time, "Watchdog time in seconds. (default="
>> +				__MODULE_STRING(WDT_DEFAULT_TIME) ")");
>> +
>> +#ifdef CONFIG_WATCHDOG_NOWAYOUT
>> +module_param(nowayout, int, 0);
>> +MODULE_PARM_DESC(nowayout,
>> +		"Watchdog cannot be stopped once started (default="
>> +				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>> +#endif
> 
> hm, now what's happening with the third arg to module_param()?
I don't understand what you mean.
This thing is common in watchdog drivers. For example 
drivers/watchdog/at91rm9200_wdt.c does the same thing.

> 
>> +static struct platform_device *bcm47xx_wdt_platform_device;
>> +
>> +static unsigned long bcm47xx_wdt_busy;
>> +static char expect_release;
>> +static struct timer_list wdt_timer;
>> +static atomic_t ticks;
>> +
>> +static inline void bcm47xx_wdt_hw_start(void)
>> +{
>> +	/* this is 2,5s on 100Mhz clock  and 2s on 133 Mhz */
>> +	ssb_watchdog_timer_set(&ssb_bcm47xx, 0xfffffff);
>> +}
>> +
>> +static inline int bcm47xx_wdt_hw_stop(void)
>> +{
>> +	return ssb_watchdog_timer_set(&ssb_bcm47xx, 0);
>> +}
>> +
>> +static void bcm47xx_timer_tick(unsigned long unused)
>> +{
>> +	if(!atomic_dec_and_test(&ticks)) {
> 
> Please pass this patch (and all others) through scripts/checkpatch.pl
> and review the resulting output.
Done, everything is ok, expect a printk line over 80 characters.

> 
>> +		bcm47xx_wdt_hw_start();
>> +		mod_timer(&wdt_timer, jiffies + HZ);
>> +	}
>> +	else {
>> +		printk(KERN_CRIT PFX "Watchdog will fire soon!!!.\n");
>> +	}
>> +}
>> +
>> +static inline void bcm47xx_wdt_pet(void)
>> +{
>> +	atomic_set(&ticks, wdt_time);
>> +}
> 
> What does "pet" stand for?
> 
A watchdog timer is a computer hardware timing device that triggers a 
system reset if the main program, due to some fault condition, such as a 
hang, neglects to regularly service the watchdog (writing a “service 
pulse” to it, also referred to as “petting the dog” [1]

[1] http://en.wikipedia.org/wiki/Watchdog_timer

But I can change the name if you want. Note that pet appear in 
drivers/watchdog/sb_wdog.c and drivers/watchdog/sbc_epx_c3.c

>> +static void bcm47xx_wdt_start(void)
>> +{
>> +	bcm47xx_wdt_pet();
>> +	bcm47xx_timer_tick(0);
>> +}
>> +
>> +static void bcm47xx_wdt_pause(void)
>> +{
>> +	del_timer(&wdt_timer);
> 
> Should this be del_timer_sync()?  The timer callback can still be
> executing after del_timer() returns.
Yes, changed to del_timer_sync()

>> +static int bcm47xx_wdt_release(struct inode *inode, struct file *file)
>> +{
>> +	if (expect_release == 42) {
>> +		bcm47xx_wdt_stop();
>> +	} else {
>> +		printk(KERN_CRIT DRV_NAME ": Unexpected close, not stopping watchdog!\n");
> 
> Can this happen?
yes : this is a common pattern in watchdog driver (check for example 
softdog) :
- expect_release is in bss (set to 0)
- we set expect_release to this magic value, only if we get a write with 
a special character and we are not in nowayout.
- So for example doing a "cat /dev/watchdog" should go in this path.

> 
>> +		bcm47xx_wdt_start();
>> +	}
>> +
>> +	clear_bit(0, &bcm47xx_wdt_busy);
>> +	expect_release = 0;
>> +	return 0;
>> +}
>> +

Thanks for the review.

I attach a new version.

[-- Attachment #2: bcm47xx_watchdog_v3.diff --]
[-- Type: text/x-diff, Size: 8352 bytes --]

This add watchdog driver for broadcom 47xx device.
It uses the ssb subsytem to access embeded watchdog device.

Because the watchdog timeout is very short (about 2s), a soft timer is used
to increase the watchdog period.

Note : A patch for exporting the ssb_watchdog_timer_set will
be submitted on next linux-mips merge. Without this patch it can't 
be build as a module.

Signed-off-by: Aleksandar Radovanovic <biblbroks@sezampro.rs>
Signed-off-by: Matthieu CASTET <castet.matthieu@free.fr>
Index: linux-2.6/drivers/watchdog/Kconfig
===================================================================
--- linux-2.6.orig/drivers/watchdog/Kconfig	2009-05-25 22:22:02.000000000 +0200
+++ linux-2.6/drivers/watchdog/Kconfig	2009-06-05 22:05:19.000000000 +0200
@@ -764,6 +764,12 @@
 	help
 	  Hardware driver for the built-in watchdog timer on TXx9 MIPS SoCs.
 
+config BCM47XX_WDT
+	tristate "Broadcom BCM47xx Watchdog Timer"
+	depends on BCM47XX
+	help
+	  Hardware driver for the Broadcom BCM47xx Watchog Timer.
+
 # PARISC Architecture
 
 # POWERPC Architecture
Index: linux-2.6/drivers/watchdog/Makefile
===================================================================
--- linux-2.6.orig/drivers/watchdog/Makefile	2009-05-25 22:22:02.000000000 +0200
+++ linux-2.6/drivers/watchdog/Makefile	2009-05-25 23:02:27.000000000 +0200
@@ -105,6 +105,7 @@
 obj-$(CONFIG_SIBYTE_WDOG) += sb_wdog.o
 obj-$(CONFIG_AR7_WDT) += ar7_wdt.o
 obj-$(CONFIG_TXX9_WDT) += txx9wdt.o
+obj-$(CONFIG_BCM47XX_WDT) += bcm47xx_wdt.o
 
 # PARISC Architecture
 
Index: linux-2.6/drivers/watchdog/bcm47xx_wdt.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6/drivers/watchdog/bcm47xx_wdt.c	2009-06-05 22:25:37.000000000 +0200
@@ -0,0 +1,286 @@
+/*
+ *  Watchdog driver for Broadcom BCM47XX
+ *
+ *  Copyright (C) 2008 Aleksandar Radovanovic <biblbroks@sezampro.rs>
+ *  Copyright (C) 2009 Matthieu CASTET <castet.matthieu@free.fr>
+ *
+ *  This program is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU General Public License
+ *  as published by the Free Software Foundation; either version
+ *  2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/bitops.h>
+#include <linux/errno.h>
+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/reboot.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+#include <linux/watchdog.h>
+#include <linux/timer.h>
+#include <linux/jiffies.h>
+#include <linux/ssb/ssb_embedded.h>
+#include <asm/mach-bcm47xx/bcm47xx.h>
+
+#define DRV_NAME		"bcm47xx_wdt"
+
+#define WDT_DEFAULT_TIME	30	/* seconds */
+#define WDT_MAX_TIME		256	/* seconds */
+
+static int wdt_time = WDT_DEFAULT_TIME;
+static int nowayout = WATCHDOG_NOWAYOUT;
+
+module_param(wdt_time, int, 0);
+MODULE_PARM_DESC(wdt_time, "Watchdog time in seconds. (default="
+				__MODULE_STRING(WDT_DEFAULT_TIME) ")");
+
+#ifdef CONFIG_WATCHDOG_NOWAYOUT
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout,
+		"Watchdog cannot be stopped once started (default="
+				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+#endif
+
+static unsigned long bcm47xx_wdt_busy;
+static char expect_release;
+static struct timer_list wdt_timer;
+static atomic_t ticks;
+
+static inline void bcm47xx_wdt_hw_start(void)
+{
+	/* this is 2,5s on 100Mhz clock  and 2s on 133 Mhz */
+	ssb_watchdog_timer_set(&ssb_bcm47xx, 0xfffffff);
+}
+
+static inline int bcm47xx_wdt_hw_stop(void)
+{
+	return ssb_watchdog_timer_set(&ssb_bcm47xx, 0);
+}
+
+static void bcm47xx_timer_tick(unsigned long unused)
+{
+	if (!atomic_dec_and_test(&ticks)) {
+		bcm47xx_wdt_hw_start();
+		mod_timer(&wdt_timer, jiffies + HZ);
+	} else {
+		printk(KERN_CRIT DRV_NAME "Watchdog will fire soon!!!\n");
+	}
+}
+
+static inline void bcm47xx_wdt_pet(void)
+{
+	atomic_set(&ticks, wdt_time);
+}
+
+static void bcm47xx_wdt_start(void)
+{
+	bcm47xx_wdt_pet();
+	bcm47xx_timer_tick(0);
+}
+
+static void bcm47xx_wdt_pause(void)
+{
+	del_timer_sync(&wdt_timer);
+	bcm47xx_wdt_hw_stop();
+}
+
+static void bcm47xx_wdt_stop(void)
+{
+	bcm47xx_wdt_pause();
+}
+
+static int bcm47xx_wdt_settimeout(int new_time)
+{
+	if ((new_time <= 0) || (new_time > WDT_MAX_TIME))
+		return -EINVAL;
+
+	wdt_time = new_time;
+	return 0;
+}
+
+static int bcm47xx_wdt_open(struct inode *inode, struct file *file)
+{
+	if (test_and_set_bit(0, &bcm47xx_wdt_busy))
+		return -EBUSY;
+
+	bcm47xx_wdt_start();
+	return nonseekable_open(inode, file);
+}
+
+static int bcm47xx_wdt_release(struct inode *inode, struct file *file)
+{
+	if (expect_release == 42) {
+		bcm47xx_wdt_stop();
+	} else {
+		printk(KERN_CRIT DRV_NAME
+			": Unexpected close, not stopping watchdog!\n");
+		bcm47xx_wdt_start();
+	}
+
+	clear_bit(0, &bcm47xx_wdt_busy);
+	expect_release = 0;
+	return 0;
+}
+
+static ssize_t bcm47xx_wdt_write(struct file *file, const char __user *data,
+			      size_t len, loff_t *ppos)
+{
+	if (len) {
+		if (!nowayout) {
+			size_t i;
+
+			expect_release = 0;
+
+			for (i = 0; i != len; i++) {
+				char c;
+				if (get_user(c, data + i))
+					return -EFAULT;
+				if (c == 'V')
+					expect_release = 42;
+			}
+		}
+		bcm47xx_wdt_pet();
+	}
+	return len;
+}
+
+static struct watchdog_info bcm47xx_wdt_info = {
+	.identity 	= DRV_NAME,
+	.options 	= WDIOF_SETTIMEOUT |
+				WDIOF_KEEPALIVEPING |
+				WDIOF_MAGICCLOSE,
+};
+
+static long bcm47xx_wdt_ioctl(struct file *file,
+					unsigned int cmd, unsigned long arg)
+{
+	void __user *argp = (void __user *)arg;
+	int __user *p = argp;
+	int new_value, retval = -EINVAL;;
+
+	switch (cmd) {
+	case WDIOC_GETSUPPORT:
+		return copy_to_user(argp, &bcm47xx_wdt_info,
+				sizeof(bcm47xx_wdt_info)) ? -EFAULT : 0;
+
+	case WDIOC_GETSTATUS:
+	case WDIOC_GETBOOTSTATUS:
+		return put_user(0, p);
+
+	case WDIOC_SETOPTIONS:
+		if (get_user(new_value, p))
+			return -EFAULT;
+
+		if (new_value & WDIOS_DISABLECARD) {
+			bcm47xx_wdt_stop();
+			retval = 0;
+		}
+
+		if (new_value & WDIOS_ENABLECARD) {
+			bcm47xx_wdt_start();
+			retval = 0;
+		}
+
+		return retval;
+
+	case WDIOC_KEEPALIVE:
+		bcm47xx_wdt_pet();
+		return 0;
+
+	case WDIOC_SETTIMEOUT:
+		if (get_user(new_value, p))
+			return -EFAULT;
+
+		if (bcm47xx_wdt_settimeout(new_value))
+			return -EINVAL;
+
+		bcm47xx_wdt_pet();
+
+	case WDIOC_GETTIMEOUT:
+		return put_user(wdt_time, p);
+
+	default:
+		return -ENOTTY;
+	}
+}
+
+static int bcm47xx_wdt_notify_sys(struct notifier_block *this,
+	   unsigned long code, void *unused)
+{
+	if (code == SYS_DOWN || code == SYS_HALT)
+		bcm47xx_wdt_stop();
+	return NOTIFY_DONE;
+}
+
+static const struct file_operations bcm47xx_wdt_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.unlocked_ioctl	= bcm47xx_wdt_ioctl,
+	.open		= bcm47xx_wdt_open,
+	.release	= bcm47xx_wdt_release,
+	.write		= bcm47xx_wdt_write,
+};
+
+static struct miscdevice bcm47xx_wdt_miscdev = {
+	.minor		= WATCHDOG_MINOR,
+	.name		= "watchdog",
+	.fops		= &bcm47xx_wdt_fops,
+};
+
+static struct notifier_block bcm47xx_wdt_notifier = {
+	.notifier_call = bcm47xx_wdt_notify_sys,
+};
+
+static int __init bcm47xx_wdt_init(void)
+{
+	int ret;
+
+	if (bcm47xx_wdt_hw_stop() < 0)
+		return -ENODEV;
+
+	setup_timer(&wdt_timer, bcm47xx_timer_tick, 0L);
+
+	if (bcm47xx_wdt_settimeout(wdt_time)) {
+		bcm47xx_wdt_settimeout(WDT_DEFAULT_TIME);
+		printk(KERN_INFO DRV_NAME
+			": wdt_time value must be 1 <= wdt_time <= 256, using %d\n",
+			wdt_time);
+	}
+
+	ret = register_reboot_notifier(&bcm47xx_wdt_notifier);
+	if (ret)
+		return ret;
+
+	ret = misc_register(&bcm47xx_wdt_miscdev);
+	if (ret) {
+		unregister_reboot_notifier(&bcm47xx_wdt_notifier);
+		return ret;
+	}
+
+	printk(KERN_INFO "BCM47xx Watchdog Timer enabled (%d seconds%s)\n",
+				wdt_time, nowayout ? ", nowayout" : "");
+	return 0;
+}
+
+static void __exit bcm47xx_wdt_exit(void)
+{
+	if (!nowayout)
+		bcm47xx_wdt_stop();
+
+	misc_deregister(&bcm47xx_wdt_miscdev);
+
+	unregister_reboot_notifier(&bcm47xx_wdt_notifier);
+}
+
+module_init(bcm47xx_wdt_init);
+module_exit(bcm47xx_wdt_exit);
+
+MODULE_AUTHOR("Aleksandar Radovanovic");
+MODULE_DESCRIPTION("Watchdog driver for Broadcom BCM47xx");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: add bcm47xx watchdog driver
  2009-06-05 14:58   ` castet.matthieu
@ 2009-06-06 10:58     ` Florian Fainelli
  0 siblings, 0 replies; 13+ messages in thread
From: Florian Fainelli @ 2009-06-06 10:58 UTC (permalink / raw)
  To: castet.matthieu
  Cc: wim, Linux Kernel list, linux-mips, Aleksandar Radovanovic

Le Friday 05 June 2009 16:58:42 castet.matthieu@free.fr, vous avez écrit :
> Hi Florian,
>
> Quoting Florian Fainelli <florian@openwrt.org>:
> > Your driver looks good, could you turn this into a platform device/driver
> > instead ? You declare bcm47xx_wdt_platform_device which is unused and you
> > also declare a MODULE_ALIAS which suggets it is one.
>
> What's the advantage of using platform device/driver ?
> Not all watchdog driver use it (for example softdog).
> This seems useless in this case because the driver don't have any resource,
> don't care about suspend/resume and add complexity in the code (2 registers
> in module probe, ...).

Indeed, I was suggesting that either you turn it into a full platform driver 
or your remove references to it (bcm47xx_wdt_platform_device and 
MODULE_ALIAS).
-- 
Best regards, Florian Fainelli
Email : florian@openwrt.org
http://openwrt.org
-------------------------------

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: add bcm47xx watchdog driver
  2009-06-05 20:30   ` matthieu castet
@ 2009-06-08 14:15     ` Florian Fainelli
  2009-06-10 17:17       ` Wim Van Sebroeck
  0 siblings, 1 reply; 13+ messages in thread
From: Florian Fainelli @ 2009-06-08 14:15 UTC (permalink / raw)
  To: matthieu castet; +Cc: Andrew Morton, wim, linux-kernel, linux-mips, biblbroks

Le Friday 05 June 2009 22:30:21 matthieu castet, vous avez écrit :
> Andrew Morton wrote:
> > On Thu, 04 Jun 2009 22:24:56 +0200
> >
> > matthieu castet <castet.matthieu@free.fr> wrote:
> >> This add watchdog driver for broadcom 47xx device.
> >> It uses the ssb subsytem to access embeded watchdog device.
> >>
> >> Because the watchdog timeout is very short (about 2s), a soft timer is
> >> used to increase the watchdog period.
> >>
> >> Note : A patch for exporting the ssb_watchdog_timer_set will
> >> be submitted on next linux-mips merge. Without this patch it can't
> >> be build as a module.

It works very well on my Netgear WGT634U, thanks !

Tested-by: Florian Fainelli <florian@openwrt.org>

> >>
> >>
> >> ...
> >>
> >> --- linux-2.6.orig/drivers/watchdog/Kconfig	2009-05-25
> >> 22:22:02.000000000 +0200 +++
> >> linux-2.6/drivers/watchdog/Kconfig	2009-05-25 22:26:06.000000000 +0200
> >> @@ -764,6 +764,12 @@
> >>  	help
> >>  	  Hardware driver for the built-in watchdog timer on TXx9 MIPS SoCs.
> >>
> >> +config BCM47XX_WDT
> >> +    tristate "Broadcom BCM47xx Watchdog Timer"
> >> +    depends on BCM47XX
> >> +    help
> >> +      Hardware driver for the Broadcom BCM47xx Watchog Timer.
> >> +
> >
> > Please indent the kconfig body with a single tab character.
>
> Done
>
> >> ...
> >>
> >> +#define DRV_NAME		"bcm47xx_wdt"
> >> +
> >> +#define WDT_DEFAULT_TIME	30	/* seconds */
> >> +#define WDT_MAX_TIME		256	/* seconds */
> >> +
> >> +static int wdt_time = WDT_DEFAULT_TIME;
> >> +static int nowayout = WATCHDOG_NOWAYOUT;
> >> +
> >> +module_param(wdt_time, int, 0);
> >> +MODULE_PARM_DESC(wdt_time, "Watchdog time in seconds. (default="
> >> +				__MODULE_STRING(WDT_DEFAULT_TIME) ")");
> >> +
> >> +#ifdef CONFIG_WATCHDOG_NOWAYOUT
> >> +module_param(nowayout, int, 0);
> >> +MODULE_PARM_DESC(nowayout,
> >> +		"Watchdog cannot be stopped once started (default="
> >> +				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> >> +#endif
> >
> > hm, now what's happening with the third arg to module_param()?
>
> I don't understand what you mean.
> This thing is common in watchdog drivers. For example
> drivers/watchdog/at91rm9200_wdt.c does the same thing.
>
> >> +static struct platform_device *bcm47xx_wdt_platform_device;
> >> +
> >> +static unsigned long bcm47xx_wdt_busy;
> >> +static char expect_release;
> >> +static struct timer_list wdt_timer;
> >> +static atomic_t ticks;
> >> +
> >> +static inline void bcm47xx_wdt_hw_start(void)
> >> +{
> >> +	/* this is 2,5s on 100Mhz clock  and 2s on 133 Mhz */
> >> +	ssb_watchdog_timer_set(&ssb_bcm47xx, 0xfffffff);
> >> +}
> >> +
> >> +static inline int bcm47xx_wdt_hw_stop(void)
> >> +{
> >> +	return ssb_watchdog_timer_set(&ssb_bcm47xx, 0);
> >> +}
> >> +
> >> +static void bcm47xx_timer_tick(unsigned long unused)
> >> +{
> >> +	if(!atomic_dec_and_test(&ticks)) {
> >
> > Please pass this patch (and all others) through scripts/checkpatch.pl
> > and review the resulting output.
>
> Done, everything is ok, expect a printk line over 80 characters.
>
> >> +		bcm47xx_wdt_hw_start();
> >> +		mod_timer(&wdt_timer, jiffies + HZ);
> >> +	}
> >> +	else {
> >> +		printk(KERN_CRIT PFX "Watchdog will fire soon!!!.\n");
> >> +	}
> >> +}
> >> +
> >> +static inline void bcm47xx_wdt_pet(void)
> >> +{
> >> +	atomic_set(&ticks, wdt_time);
> >> +}
> >
> > What does "pet" stand for?
>
> A watchdog timer is a computer hardware timing device that triggers a
> system reset if the main program, due to some fault condition, such as a
> hang, neglects to regularly service the watchdog (writing a “service
> pulse” to it, also referred to as “petting the dog” [1]
>
> [1] http://en.wikipedia.org/wiki/Watchdog_timer
>
> But I can change the name if you want. Note that pet appear in
> drivers/watchdog/sb_wdog.c and drivers/watchdog/sbc_epx_c3.c
>
> >> +static void bcm47xx_wdt_start(void)
> >> +{
> >> +	bcm47xx_wdt_pet();
> >> +	bcm47xx_timer_tick(0);
> >> +}
> >> +
> >> +static void bcm47xx_wdt_pause(void)
> >> +{
> >> +	del_timer(&wdt_timer);
> >
> > Should this be del_timer_sync()?  The timer callback can still be
> > executing after del_timer() returns.
>
> Yes, changed to del_timer_sync()
>
> >> +static int bcm47xx_wdt_release(struct inode *inode, struct file *file)
> >> +{
> >> +	if (expect_release == 42) {
> >> +		bcm47xx_wdt_stop();
> >> +	} else {
> >> +		printk(KERN_CRIT DRV_NAME ": Unexpected close, not stopping
> >> watchdog!\n");
> >
> > Can this happen?
>
> yes : this is a common pattern in watchdog driver (check for example
> softdog) :
> - expect_release is in bss (set to 0)
> - we set expect_release to this magic value, only if we get a write with
> a special character and we are not in nowayout.
> - So for example doing a "cat /dev/watchdog" should go in this path.
>
> >> +		bcm47xx_wdt_start();
> >> +	}
> >> +
> >> +	clear_bit(0, &bcm47xx_wdt_busy);
> >> +	expect_release = 0;
> >> +	return 0;
> >> +}
> >> +
>
> Thanks for the review.
>
> I attach a new version.



-- 
Best regards, Florian Fainelli
Email : florian@openwrt.org
http://openwrt.org
-------------------------------

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: add bcm47xx watchdog driver
  2009-06-08 14:15     ` Florian Fainelli
@ 2009-06-10 17:17       ` Wim Van Sebroeck
  2009-06-10 18:47         ` matthieu castet
  0 siblings, 1 reply; 13+ messages in thread
From: Wim Van Sebroeck @ 2009-06-10 17:17 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: matthieu castet, Andrew Morton, linux-kernel, linux-mips, biblbroks

Hi Matthieu, Florian,

> It works very well on my Netgear WGT634U, thanks !
> 
> Tested-by: Florian Fainelli <florian@openwrt.org>

I incorporated the changes of Andrew and did a clean-up here and there to keep checkpatch happy.
Could you test the attached diff to see that it still works.

Thanks,
Wim.

---
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 443e7e2..8d2fe51 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -729,6 +729,12 @@ config SBC_EPX_C3_WATCHDOG
 
 # MIPS Architecture
 
+config BCM47XX_WDT
+	tristate "Broadcom BCM47xx Watchdog Timer"
+	depends on BCM47XX
+	help
+	  Hardware driver for the Broadcom BCM47xx Watchog Timer.
+
 config RC32434_WDT
 	tristate "IDT RC32434 SoC Watchdog Timer"
 	depends on MIKROTIK_RB532
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index ee8c84c..83ed987 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -101,6 +101,7 @@ obj-$(CONFIG_SBC_EPX_C3_WATCHDOG) += sbc_epx_c3.o
 # M68KNOMMU Architecture
 
 # MIPS Architecture
+obj-$(CONFIG_BCM47XX_WDT) += bcm47xx_wdt.o
 obj-$(CONFIG_RC32434_WDT) += rc32434_wdt.o
 obj-$(CONFIG_INDYDOG) += indydog.o
 obj-$(CONFIG_WDT_MTX1) += mtx-1_wdt.o
diff --git a/drivers/watchdog/bcm47xx_wdt.c b/drivers/watchdog/bcm47xx_wdt.c
new file 100644
--- /dev/null
+++ b/drivers/watchdog/bcm47xx_wdt.c
@@ -0,0 +1,286 @@
+/*
+ *  Watchdog driver for Broadcom BCM47XX
+ *
+ *  Copyright (C) 2008 Aleksandar Radovanovic <biblbroks@sezampro.rs>
+ *  Copyright (C) 2009 Matthieu CASTET <castet.matthieu@free.fr>
+ *
+ *  This program is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU General Public License
+ *  as published by the Free Software Foundation; either version
+ *  2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/bitops.h>
+#include <linux/errno.h>
+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/reboot.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+#include <linux/watchdog.h>
+#include <linux/timer.h>
+#include <linux/jiffies.h>
+#include <linux/ssb/ssb_embedded.h>
+#include <asm/mach-bcm47xx/bcm47xx.h>
+
+#define DRV_NAME		"bcm47xx_wdt"
+
+#define WDT_DEFAULT_TIME	30	/* seconds */
+#define WDT_MAX_TIME		255	/* seconds */
+
+static int wdt_time = WDT_DEFAULT_TIME;
+static int nowayout = WATCHDOG_NOWAYOUT;
+
+module_param(wdt_time, int, 0);
+MODULE_PARM_DESC(wdt_time, "Watchdog time in seconds. (default="
+				__MODULE_STRING(WDT_DEFAULT_TIME) ")");
+
+#ifdef CONFIG_WATCHDOG_NOWAYOUT
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout,
+		"Watchdog cannot be stopped once started (default="
+				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+#endif
+
+static unsigned long bcm47xx_wdt_busy;
+static char expect_release;
+static struct timer_list wdt_timer;
+static atomic_t ticks;
+
+static inline void bcm47xx_wdt_hw_start(void)
+{
+	/* this is 2,5s on 100Mhz clock  and 2s on 133 Mhz */
+	ssb_watchdog_timer_set(&ssb_bcm47xx, 0xfffffff);
+}
+
+static inline int bcm47xx_wdt_hw_stop(void)
+{
+	return ssb_watchdog_timer_set(&ssb_bcm47xx, 0);
+}
+
+static void bcm47xx_timer_tick(unsigned long unused)
+{
+	if (!atomic_dec_and_test(&ticks)) {
+		bcm47xx_wdt_hw_start();
+		mod_timer(&wdt_timer, jiffies + HZ);
+	} else {
+		printk(KERN_CRIT DRV_NAME "Watchdog will fire soon!!!\n");
+	}
+}
+
+static inline void bcm47xx_wdt_pet(void)
+{
+	atomic_set(&ticks, wdt_time);
+}
+
+static void bcm47xx_wdt_start(void)
+{
+	bcm47xx_wdt_pet();
+	bcm47xx_timer_tick(0);
+}
+
+static void bcm47xx_wdt_pause(void)
+{
+	del_timer_sync(&wdt_timer);
+	bcm47xx_wdt_hw_stop();
+}
+
+static void bcm47xx_wdt_stop(void)
+{
+	bcm47xx_wdt_pause();
+}
+
+static int bcm47xx_wdt_settimeout(int new_time)
+{
+	if ((new_time <= 0) || (new_time > WDT_MAX_TIME))
+		return -EINVAL;
+
+	wdt_time = new_time;
+	return 0;
+}
+
+static int bcm47xx_wdt_open(struct inode *inode, struct file *file)
+{
+	if (test_and_set_bit(0, &bcm47xx_wdt_busy))
+		return -EBUSY;
+
+	bcm47xx_wdt_start();
+	return nonseekable_open(inode, file);
+}
+
+static int bcm47xx_wdt_release(struct inode *inode, struct file *file)
+{
+	if (expect_release == 42) {
+		bcm47xx_wdt_stop();
+	} else {
+		printk(KERN_CRIT DRV_NAME
+			": Unexpected close, not stopping watchdog!\n");
+		bcm47xx_wdt_start();
+	}
+
+	clear_bit(0, &bcm47xx_wdt_busy);
+	expect_release = 0;
+	return 0;
+}
+
+static ssize_t bcm47xx_wdt_write(struct file *file, const char __user *data,
+				size_t len, loff_t *ppos)
+{
+	if (len) {
+		if (!nowayout) {
+			size_t i;
+
+			expect_release = 0;
+
+			for (i = 0; i != len; i++) {
+				char c;
+				if (get_user(c, data + i))
+					return -EFAULT;
+				if (c == 'V')
+					expect_release = 42;
+			}
+		}
+		bcm47xx_wdt_pet();
+	}
+	return len;
+}
+
+static struct watchdog_info bcm47xx_wdt_info = {
+	.identity 	= DRV_NAME,
+	.options 	= WDIOF_SETTIMEOUT |
+				WDIOF_KEEPALIVEPING |
+				WDIOF_MAGICCLOSE,
+};
+
+static long bcm47xx_wdt_ioctl(struct file *file,
+					unsigned int cmd, unsigned long arg)
+{
+	void __user *argp = (void __user *)arg;
+	int __user *p = argp;
+	int new_value, retval = -EINVAL;;
+
+	switch (cmd) {
+	case WDIOC_GETSUPPORT:
+		return copy_to_user(argp, &bcm47xx_wdt_info,
+				sizeof(bcm47xx_wdt_info)) ? -EFAULT : 0;
+
+	case WDIOC_GETSTATUS:
+	case WDIOC_GETBOOTSTATUS:
+		return put_user(0, p);
+
+	case WDIOC_SETOPTIONS:
+		if (get_user(new_value, p))
+			return -EFAULT;
+
+		if (new_value & WDIOS_DISABLECARD) {
+			bcm47xx_wdt_stop();
+			retval = 0;
+		}
+
+		if (new_value & WDIOS_ENABLECARD) {
+			bcm47xx_wdt_start();
+			retval = 0;
+		}
+
+		return retval;
+
+	case WDIOC_KEEPALIVE:
+		bcm47xx_wdt_pet();
+		return 0;
+
+	case WDIOC_SETTIMEOUT:
+		if (get_user(new_value, p))
+			return -EFAULT;
+
+		if (bcm47xx_wdt_settimeout(new_value))
+			return -EINVAL;
+
+		bcm47xx_wdt_pet();
+
+	case WDIOC_GETTIMEOUT:
+		return put_user(wdt_time, p);
+
+	default:
+		return -ENOTTY;
+	}
+}
+
+static int bcm47xx_wdt_notify_sys(struct notifier_block *this,
+	nsigned long code, void *unused)
+{
+	if (code == SYS_DOWN || code == SYS_HALT)
+		bcm47xx_wdt_stop();
+	return NOTIFY_DONE;
+}
+
+static const struct file_operations bcm47xx_wdt_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.unlocked_ioctl	= bcm47xx_wdt_ioctl,
+	.open		= bcm47xx_wdt_open,
+	.release	= bcm47xx_wdt_release,
+	.write		= bcm47xx_wdt_write,
+};
+
+static struct miscdevice bcm47xx_wdt_miscdev = {
+	.minor		= WATCHDOG_MINOR,
+	.name		= "watchdog",
+	.fops		= &bcm47xx_wdt_fops,
+};
+
+static struct notifier_block bcm47xx_wdt_notifier = {
+	.notifier_call = bcm47xx_wdt_notify_sys,
+};
+
+static int __init bcm47xx_wdt_init(void)
+{
+	int ret;
+
+	if (bcm47xx_wdt_hw_stop() < 0)
+		return -ENODEV;
+
+	setup_timer(&wdt_timer, bcm47xx_timer_tick, 0L);
+
+	if (bcm47xx_wdt_settimeout(wdt_time)) {
+		bcm47xx_wdt_settimeout(WDT_DEFAULT_TIME);
+		printk(KERN_INFO DRV_NAME ": "
+			"wdt_time value must be 0 < wdt_time < %d, using %d\n",
+			(WDT_MAX_TIME + 1), wdt_time);
+	}
+
+	ret = register_reboot_notifier(&bcm47xx_wdt_notifier);
+	if (ret)
+		return ret;
+
+	ret = misc_register(&bcm47xx_wdt_miscdev);
+	if (ret) {
+		unregister_reboot_notifier(&bcm47xx_wdt_notifier);
+		return ret;
+	}
+
+	printk(KERN_INFO "BCM47xx Watchdog Timer enabled (%d seconds%s)\n",
+				wdt_time, nowayout ? ", nowayout" : "");
+	return 0;
+}
+
+static void __exit bcm47xx_wdt_exit(void)
+{
+	if (!nowayout)
+		bcm47xx_wdt_stop();
+
+	misc_deregister(&bcm47xx_wdt_miscdev);
+
+	unregister_reboot_notifier(&bcm47xx_wdt_notifier);
+}
+
+module_init(bcm47xx_wdt_init);
+module_exit(bcm47xx_wdt_exit);
+
+MODULE_AUTHOR("Aleksandar Radovanovic");
+MODULE_DESCRIPTION("Watchdog driver for Broadcom BCM47xx");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: add bcm47xx watchdog driver
  2009-06-10 17:17       ` Wim Van Sebroeck
@ 2009-06-10 18:47         ` matthieu castet
  2009-06-10 19:06           ` Wim Van Sebroeck
  0 siblings, 1 reply; 13+ messages in thread
From: matthieu castet @ 2009-06-10 18:47 UTC (permalink / raw)
  To: Wim Van Sebroeck
  Cc: Florian Fainelli, Andrew Morton, linux-kernel, linux-mips, biblbroks

Hi,

Wim Van Sebroeck wrote:
> Hi Matthieu, Florian,
> 
>> It works very well on my Netgear WGT634U, thanks !
>>
>> Tested-by: Florian Fainelli <florian@openwrt.org>
> 
> I incorporated the changes of Andrew and did a clean-up here and there to keep checkpatch happy.
> Could you test the attached diff to see that it still works.
> 
> Thanks,
> Wim.
> 

> +
> +#define WDT_DEFAULT_TIME	30	/* seconds */
> +#define WDT_MAX_TIME		255	/* seconds */
Why have changed this from 256 to 255 ?

> +}
> +
> +static int bcm47xx_wdt_notify_sys(struct notifier_block *this,
> +	nsigned long code, void *unused)
        ^^^^
Does this build ?


Matthieu

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: add bcm47xx watchdog driver
  2009-06-10 18:47         ` matthieu castet
@ 2009-06-10 19:06           ` Wim Van Sebroeck
  0 siblings, 0 replies; 13+ messages in thread
From: Wim Van Sebroeck @ 2009-06-10 19:06 UTC (permalink / raw)
  To: matthieu castet
  Cc: Florian Fainelli, Andrew Morton, linux-kernel, linux-mips, biblbroks

Hi Matthieu,

>> +
>> +#define WDT_DEFAULT_TIME	30	/* seconds */
>> +#define WDT_MAX_TIME		255	/* seconds */
> Why have changed this from 256 to 255 ?

Since you use a timer to control the real watchdog, the wdt_time function is arbitraty anyway.
Most watchdog drivers use 0xFFFF. Since you only went to 256 it made more sense to have it as 0xFF which is 255.
We can make it 65535 also. 

>> +}
>> +
>> +static int bcm47xx_wdt_notify_sys(struct notifier_block *this,
>> +	nsigned long code, void *unused)
>        ^^^^
> Does this build ?

Nope. should be unsigned.

Kind regards,
Wim.


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2009-06-10 19:06 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-06-04 20:24 add bcm47xx watchdog driver matthieu castet
2009-06-05 13:58 ` Florian Fainelli
2009-06-05 14:58   ` castet.matthieu
2009-06-06 10:58     ` Florian Fainelli
2009-06-05 16:57   ` add bcm47xx watchdog driver v2 matthieu castet
2009-06-05 19:48 ` add bcm47xx watchdog driver Andrew Morton
2009-06-05 20:30   ` matthieu castet
2009-06-08 14:15     ` Florian Fainelli
2009-06-10 17:17       ` Wim Van Sebroeck
2009-06-10 18:47         ` matthieu castet
2009-06-10 19:06           ` Wim Van Sebroeck
2009-06-05 19:50 ` Andrew Morton
2009-06-05 20:03   ` matthieu castet

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.