Skip to contentSkip to conversation

add bcm47xx watchdog driver

PATCH matthieu castet 4 participants 13 messages Jun 4, 2009 – Jun 10, 2009

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

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.

drivers/watchdog/Kconfig +6 -0
Index: linux-2.6/drivers/watchdog/Kconfig
===================================================================
@@ -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
drivers/watchdog/Makefile +1 -0
Index: linux-2.6/drivers/watchdog/Makefile
===================================================================
@@ -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
 
drivers/watchdog/bcm47xx_wdt.c new file +289 -0

289 lines not on this page · raw message

#1 lore raw

Salut Matthieu,

Le Thursday 04 June 2009 22:24:56 matthieu castet, vous avez écrit :

Quoted 9 lines

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.

Signature · 4 lines
Best regards, Florian Fainelli
Email : [email protected]
http://openwrt.org
-------------------------------
#2 lore raw

Hi Florian,

Quoting Florian Fainelli <[email protected]>:

Quoted 4 lines

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

#3 lore raw

Le Friday 05 June 2009 16:58:42 [email protected], vous avez écrit :

Quoted 7 lines

Hi Florian,

Quoting Florian Fainelli <[email protected]>:

Quoted 3 lines

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.

Quoted 5 lines

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).

Signature · 4 lines
Best regards, Florian Fainelli
Email : [email protected]
http://openwrt.org
-------------------------------
#4 lore raw

Re: add bcm47xx watchdog driver v2

In reply to Florian Fainelli
Quoted 6 lines · Florian Fainelli

Salut Matthieu,

Le Thursday 04 June 2009 22:24:56 matthieu castet, vous avez écrit :

Quoted 3 lines

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

Quoted 8 lines
Quoted 6 lines

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

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.

drivers/watchdog/Kconfig +6 -0
Index: linux-2.6/drivers/watchdog/Kconfig
===================================================================
@@ -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
drivers/watchdog/Makefile +1 -0
Index: linux-2.6/drivers/watchdog/Makefile
===================================================================
@@ -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
 
drivers/watchdog/bcm47xx_wdt.c new file +284 -0

284 lines not on this page · raw message

#5 lore raw
In reply to matthieu castet
Quoted 18 lines · On Thu, 04 Jun 2009 22:24:56 +0200 matthieu castet <[email protected]>…

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.

...

drivers/watchdog/Kconfig line 772

Quoted 9 lines
drivers/watchdog/Kconfig +6 -0
@@ -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.

Quoted 11 lines

...

drivers/watchdog/Kconfig +8 -0
+#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;
+

drivers/watchdog/Kconfig

Quoted 10 lines
drivers/watchdog/Kconfig +10 -0
+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()?

Quoted 11 lines
drivers/watchdog/Kconfig +11 -0
+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);

drivers/watchdog/Kconfig

Quoted 10 lines
drivers/watchdog/Kconfig +10 -0
+}
+
+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.

drivers/watchdog/Kconfig

Quoted 12 lines
drivers/watchdog/Kconfig +12 -0
+		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?

drivers/watchdog/Kconfig

Quoted 9 lines
drivers/watchdog/Kconfig +9 -0
+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.

Quoted 22 lines
drivers/watchdog/Kconfig +22 -0
+	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;
+

drivers/watchdog/Kconfig

Quoted 10 lines
drivers/watchdog/Kconfig +10 -0
+	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?

Quoted 11 lines
drivers/watchdog/Kconfig +8 -0
+		bcm47xx_wdt_start();
+	}
+
+	clear_bit(0, &bcm47xx_wdt_busy);
+	expect_release = 0;
+	return 0;
+}
+
 
...
#6 lore raw
Quoted 22 lines · Andrew Morton
Quoted 19 lines · On Thu, 04 Jun 2009 22:24:56 +0200 matthieu castet <[email protected]>…

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.

...

drivers/watchdog/Kconfig +0 -0
@@ -764,6 +764,12 @@
 	help
 	  Hardware driver for the built-in watchdog timer on TXx9 MIPS SoCs.
 
Quoted 9 lines

drivers/watchdog/Kconfig line 772

Quoted 6 lines
drivers/watchdog/Kconfig +6 -0
+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

Quoted 14 lines
Quoted 14 lines

...

drivers/watchdog/Kconfig +12 -0
+#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) ")");
+
Quoted 8 lines

drivers/watchdog/Kconfig

Quoted 6 lines
drivers/watchdog/Kconfig +6 -0
+#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.

Quoted 17 lines
Quoted 17 lines
drivers/watchdog/Kconfig +16 -0
+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);
Quoted 8 lines

drivers/watchdog/Kconfig

Quoted 5 lines
drivers/watchdog/Kconfig +5 -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.

Quoted 16 lines

drivers/watchdog/Kconfig

Quoted 13 lines
drivers/watchdog/Kconfig +12 -0
+		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

Quoted 4 lines
Quoted 4 lines
drivers/watchdog/Kconfig +4 -0
+static void bcm47xx_wdt_start(void)
+{
+	bcm47xx_wdt_pet();
+	bcm47xx_timer_tick(0);
Quoted 8 lines

drivers/watchdog/Kconfig

Quoted 5 lines
drivers/watchdog/Kconfig +5 -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()

Quoted 8 lines

drivers/watchdog/Kconfig

Quoted 6 lines
drivers/watchdog/Kconfig +6 -0
+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.

Quoted 9 lines

drivers/watchdog/Kconfig

Quoted 9 lines
drivers/watchdog/Kconfig +8 -0
+		bcm47xx_wdt_start();
+	}
+
+	clear_bit(0, &bcm47xx_wdt_busy);
+	expect_release = 0;
+	return 0;
+}
+

Thanks for the review.

I attach a new version.

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.

drivers/watchdog/Kconfig +6 -0
Index: linux-2.6/drivers/watchdog/Kconfig
===================================================================
@@ -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
drivers/watchdog/Makefile +1 -0
Index: linux-2.6/drivers/watchdog/Makefile
===================================================================
@@ -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
 
drivers/watchdog/bcm47xx_wdt.c new file +286 -0

286 lines not on this page · raw message

#7 lore raw

Le Friday 05 June 2009 22:30:21 matthieu castet, vous avez écrit :

Quoted 7 lines
Quoted 6 lines · Andrew Morton

On Thu, 04 Jun 2009 22:24:56 +0200

Quoted 3 lines · matthieu castet

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

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

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 !

Quoted 145 lines
Quoted 11 lines
Quoted 11 lines

...

--- 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

drivers/watchdog/Kconfig +0 -0
@@ -764,6 +764,12 @@
 	help
 	  Hardware driver for the built-in watchdog timer on TXx9 MIPS SoCs.
 
Quoted 8 lines

drivers/watchdog/Kconfig line 772

Quoted 6 lines
drivers/watchdog/Kconfig +6 -0
+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

Quoted 14 lines
Quoted 14 lines

...

drivers/watchdog/bcm47xx_wdt.c new file +12 -0
+#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) ")");
+
Quoted 8 lines

drivers/watchdog/bcm47xx_wdt.c line 47

Quoted 6 lines
drivers/watchdog/bcm47xx_wdt.c new file +6 -0
+#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.

Quoted 16 lines
Quoted 16 lines
drivers/watchdog/bcm47xx_wdt.c new file +16 -0
+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);
Quoted 8 lines

drivers/watchdog/bcm47xx_wdt.c

Quoted 5 lines
drivers/watchdog/bcm47xx_wdt.c new file +5 -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.

Quoted 6 lines
Quoted 6 lines
drivers/watchdog/bcm47xx_wdt.c new file +6 -0
+		bcm47xx_wdt_hw_start();
+		mod_timer(&wdt_timer, jiffies + HZ);
+	}
+	else {
+		printk(KERN_CRIT PFX "Watchdog will fire soon!!!.\n");
+	}
Quoted 8 lines

drivers/watchdog/bcm47xx_wdt.c line 78

Quoted 6 lines
drivers/watchdog/bcm47xx_wdt.c new file +6 -0
+}
+
+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

Quoted 4 lines
Quoted 4 lines
drivers/watchdog/bcm47xx_wdt.c new file +4 -0
+static void bcm47xx_wdt_start(void)
+{
+	bcm47xx_wdt_pet();
+	bcm47xx_timer_tick(0);
Quoted 8 lines

drivers/watchdog/bcm47xx_wdt.c

Quoted 5 lines
drivers/watchdog/bcm47xx_wdt.c new file +5 -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()

Quoted 9 lines
Quoted 7 lines
drivers/watchdog/bcm47xx_wdt.c new file +6 -0
+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.

Quoted 8 lines

drivers/watchdog/bcm47xx_wdt.c line 129

Quoted 8 lines
drivers/watchdog/bcm47xx_wdt.c new file +8 -0
+		bcm47xx_wdt_start();
+	}
+
+	clear_bit(0, &bcm47xx_wdt_busy);
+	expect_release = 0;
+	return 0;
+}
+

Thanks for the review.

I attach a new version.

Signature · 4 lines
Best regards, Florian Fainelli
Email : [email protected]
http://openwrt.org
-------------------------------
#8 lore raw

Hi Matthieu, Florian,

Quoted 3 lines

It works very well on my Netgear WGT634U, thanks !

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.

---

drivers/watchdog/Kconfig +6 -0
index 443e7e2..8d2fe51 100644
@@ -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
drivers/watchdog/Makefile +1 -0
index ee8c84c..83ed987 100644
@@ -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
drivers/watchdog/bcm47xx_wdt.c +0 -0

new file 100644

drivers/watchdog/bcm47xx_wdt.c new file +286 -0

286 lines not on this page · raw message

#9 lore raw

Hi,

Quoted 13 lines · Wim Van Sebroeck

Hi Matthieu, Florian,

Quoted 3 lines

It works very well on my Netgear WGT634U, thanks !

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.

drivers/watchdog/bcm47xx_wdt.c line 33

Quoted 3 lines
drivers/watchdog/bcm47xx_wdt.c new file +3 -0
+
+#define WDT_DEFAULT_TIME	30	/* seconds */
+#define WDT_MAX_TIME		255	/* seconds */

Why have changed this from 256 to 255 ?

drivers/watchdog/bcm47xx_wdt.c line 213

Quoted 4 lines
drivers/watchdog/bcm47xx_wdt.c new file +4 -0
+}
+
+static int bcm47xx_wdt_notify_sys(struct notifier_block *this,
+	nsigned long code, void *unused)
       ^^^^

Does this build ?

Matthieu

#10 lore raw

Hi Matthieu,

Quoted 4 lines

drivers/watchdog/bcm47xx_wdt.c line 33

Quoted 3 lines
drivers/watchdog/bcm47xx_wdt.c new file +3 -0
+
+#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.

Quoted 6 lines

drivers/watchdog/bcm47xx_wdt.c line 213

Quoted 4 lines
drivers/watchdog/bcm47xx_wdt.c new file +4 -0
+}
+
+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.

#11 lore raw
In reply to matthieu castet
Quoted 8 lines · On Thu, 04 Jun 2009 22:24:56 +0200 matthieu castet <[email protected]>…

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

...

drivers/watchdog/Kconfig line 771

Quoted 8 lines
drivers/watchdog/Kconfig +5 -0
@@ -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?

#12 lore raw
Quoted 14 lines · Andrew Morton
Quoted 11 lines · On Thu, 04 Jun 2009 22:24:56 +0200 matthieu castet <[email protected]>…

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

...

drivers/watchdog/Kconfig +0 -0
@@ -764,6 +764,12 @@
 	help
 	  Hardware driver for the built-in watchdog timer on TXx9 MIPS SoCs.
 
Quoted 7 lines

drivers/watchdog/Kconfig line 771

Quoted 5 lines
drivers/watchdog/Kconfig +5 -0
+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