Watchdog timer for Intel IXP4xx CPUs
PATCH 1/1 Watchdog timer for Intel IXP4xx CPUs
Following patch against 2.6.6 adds a driver for the watchdogs on the Intel IXP4xx family of network processors (ARM). Please apply.
Tnx,
~Deepak
drivers/char/watchdog/Kconfig +11 -0
@@ -84,6 +84,17 @@ Not sure? It's safe to say N. +config IXP4XX_WATCHDOG + tristate "IXP4xx Watchdog" + depends on WATCHDOG && ARCH_IXP4XX + help + Say Y here if to include support for the watchdog timer + in the Intel IXP4xx network processors. This driver can + be built as a module by choosing M. The module will + be called ixp4xx_wdt. + + Say N if you are unsure. + config SA1100_WATCHDOG tristate "SA1100 watchdog" depends on WATCHDOG && ARCH_SA1100
drivers/char/watchdog/Makefile +1 -0
@@ -35,3 +35,4 @@ obj-$(CONFIG_INDYDOG) += indydog.o obj-$(CONFIG_PCIPCWATCHDOG) += pcwd_pci.o obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o +obj-$(CONFIG_IXP4XX_WATCHDOG) += ixp4xx_wdt.o
drivers/char/watchdog/ixp4xx_wdt.c +229 -0
229 lines not on this page · raw message
Signature · 4 lines
Deepak Saxena - dsaxena at plexity dot net - http://www.plexity.net/ "Unlike me, many of you have accepted the situation of your imprisonment and will die here like rotten cabbages." - Number 6
Review
Quoted 8 lines · Deepak Saxena
Following patch against 2.6.6 adds a driver for the watchdogs on the Intel IXP4xx family of network processors (ARM). Please apply.
...
(quoted diff) +2
-0+ + clear_bit(1, &wdt_status);
It'd be nice to enumerate the bits in wdt_status.
drivers/char/watchdog/ixp4xx_wdt.c line 139
Quoted 13 lines
drivers/char/watchdog/ixp4xx_wdt.c +13
-0+ +case WDIOC_SETTIMEOUT : + ret =get_user (time, (int *)arg); +if (ret) +break ; + +if (time <=0 || time >60 ) { + ret = -EINVAL ; +break ; + } + + heartbeat = time; +wdt_enable ();
Missing a break here?
Quoted 10 lines
drivers/char/watchdog/ixp4xx_wdt.c +10
-0+case WDIOC_GETTIMEOUT : + ret =put_user (heartbeat, (int *)arg); +break ; + +case WDIOC_KEEPALIVE : +wdt_enable (); + ret =0 ; +break ; + } +return ret;
On May 11 2004, at 14:33, Andrew Morton was caught saying:
Quoted 11 lines
Review
Quoted 8 lines · Deepak Saxena
Following patch against 2.6.6 adds a driver for the watchdogs on the Intel IXP4xx family of network processors (ARM). Please apply.
...
(quoted diff) +2
-0+ + clear_bit(1, &wdt_status);It'd be nice to enumerate the bits in
wdt_status.
Added #define of the bit meaning. I just copied and pasted from other wdt drivers. :)
Quoted 6 lines
Quoted 6 lines
(quoted diff) +6
-0+ case WDIOC_SETTIMEOUT: + ret = get_user(time, (int *)arg); + if (ret) + break; + + if (time <= 0 || time > 60) {
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ ret = -EINVAL; + break; + } + + heartbeat = time; + wdt_enable();Missing a break here?
Nope. The SETTIMEOUT case fallsthrough to the GETTIMEOT and returns the actual timeout value. Added a comment like in other drivers stating that it falls through.
Updated patch attached.
~Deepak
Signature · 4 lines
Deepak Saxena - dsaxena at plexity dot net - http://www.plexity.net/ "Unlike me, many of you have accepted the situation of your imprisonment and will die here like rotten cabbages." - Number 6
drivers/char/watchdog/Kconfig +11 -0
@@ -84,6 +84,17 @@ Not sure? It's safe to say N. +config IXP4XX_WATCHDOG + tristate "IXP4xx Watchdog" + depends on WATCHDOG && ARCH_IXP4XX + help + Say Y here if to include support for the watchdog timer + in the Intel IXP4xx network processors. This driver can + be built as a module by choosing M. The module will + be called ixp4xx_wdt. + + Say N if you are unsure. + config SA1100_WATCHDOG tristate "SA1100 watchdog" depends on WATCHDOG && ARCH_SA1100
drivers/char/watchdog/Makefile +1 -0
@@ -35,3 +35,4 @@ obj-$(CONFIG_INDYDOG) += indydog.o obj-$(CONFIG_PCIPCWATCHDOG) += pcwd_pci.o obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o +obj-$(CONFIG_IXP4XX_WATCHDOG) += ixp4xx_wdt.o
drivers/char/watchdog/ixp4xx_wdt.c +233 -0
233 lines not on this page · raw message
drivers/char/watchdog/ixp4xx_wdt.c line 37
Quoted 9 lines · Deepak Saxena
On May 11 2004, at 14:33, Andrew Morton was caught saying:
drivers/char/watchdog/ixp4xx_wdt.c +8
-0+#ifdef CONFIG_WATCHDOG_NOWAYOUT +static int nowayout =1 ; +#else +static int nowayout =0 ; +#endif +static int heartbeat =60 ;/* (secs) Default is 1 minute */ +static unsigned long wdt_status =0 ; +static unsigned long boot_status =0 ;
Wastes bss(?) space to explicitly zero statics.
Quoted 12 lines
drivers/char/watchdog/ixp4xx_wdt.c +12
-0+#define WDT_TICK_RATE (IXP4XX_PERIPHERAL_BUS_CLOCK * 1000000UL) + +#define WDT_IN_USE 0 +#define WDT_OK_TO_CLOSE 1 + +static void +wdt_enable (void ) +{ + *IXP4XX_OSWK =IXP4XX_WDT_KEY ; + *IXP4XX_OSWE =0 ; + *IXP4XX_OSWT =WDT_TICK_RATE * heartbeat; + *IXP4XX_OSWE =IXP4XX_WDT_COUNT_ENABLE |IXP4XX_WDT_RESET_ENABLE ;
drivers/char/watchdog/ixp4xx_wdt.c line 60
Quoted 10 lines
drivers/char/watchdog/ixp4xx_wdt.c +10
-0+ *IXP4XX_OSWK =0 ; +} + +static void +wdt_disable (void ) +{ + *IXP4XX_OSWK =IXP4XX_WDT_KEY ; + *IXP4XX_OSWE =0 ; + *IXP4XX_OSWK =0 ; +}
Are these magic CPU addresses you're writing to? Normally one uses bus macros to read/write to devices. But SoC and embedded stuff is often special...
drivers/char/watchdog/ixp4xx_wdt.c line 73
Quoted 12 lines
drivers/char/watchdog/ixp4xx_wdt.c +12
-0+static int +ixp4xx_wdt_open (struct inode *inode,struct file *file) +{ +if (test_and_set_bit (WDT_IN_USE , &wdt_status)) +return -EBUSY ; + +clear_bit (WDT_OK_TO_CLOSE , &wdt_status); + +wdt_enable (); + +return 0 ; +}
You typically want semaphores rather than bit tests.
And what about blocking on multiple openers?
Quoted 14 lines
drivers/char/watchdog/ixp4xx_wdt.c +14
-0+static ssize_t +ixp4xx_wdt_write (struct file *file,const char *data,size_t len,loff_t *ppos) +{ +/* Can't seek (pwrite) on this device */ +if (ppos != &file->f_pos) +return -ESPIPE ; + +if (len) { +if (!nowayout) { +size_t i; + +clear_bit (WDT_OK_TO_CLOSE , &wdt_status); + +for (i =0 ; i != len; i++) {
drivers/char/watchdog/ixp4xx_wdt.c line 98
Quoted 10 lines
drivers/char/watchdog/ixp4xx_wdt.c +10
-0+char c; + +if (get_user (c, data + i)) +return -EFAULT ; +if (c =='V' ) +set_bit (WDT_OK_TO_CLOSE , &wdt_status); + } + } +wdt_enable (); + }
do other watchdog drivers really twiddle a bit like this on each write?
Otherwise looks OK.
Jeff
Quoted 12 lines · Jeff Garzik
drivers/char/watchdog/ixp4xx_wdt.c line 37
Quoted 9 lines · Deepak Saxena
On May 11 2004, at 14:33, Andrew Morton was caught saying:
drivers/char/watchdog/ixp4xx_wdt.c +8
-0+#ifdef CONFIG_WATCHDOG_NOWAYOUT +static int nowayout =1 ; +#else +static int nowayout =0 ; +#endif +static int heartbeat =60 ;/* (secs) Default is 1 minute */ +static unsigned long wdt_status =0 ; +static unsigned long boot_status =0 ;Wastes bss(?) space to explicitly zero statics.
BTW, BSS is the stuff that is zeroed automatically. Explicitly zeroing a variable is a waste of DATA and requires space in the executable.
On May 11 2004, at 18:37, Jeff Garzik was caught saying:
Quoted 12 lines
Review
Quoted 9 lines · Deepak Saxena
On May 11 2004, at 14:33, Andrew Morton was caught saying:
(quoted diff) +8
-0+#ifdef CONFIG_WATCHDOG_NOWAYOUT +static int nowayout = 1; +#else +static int nowayout = 0; +#endif +static int heartbeat = 60; /* (secs) Default is 1 minute */ +static unsigned long wdt_status = 0; +static unsigned long boot_status = 0;Wastes bss(?) space to explicitly zero statics.
My mother always told me to initialize my variables but this is easily fixed.
Quoted 11 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+static void +wdt_disable(void) +{ + *IXP4XX_OSWK = IXP4XX_WDT_KEY; + *IXP4XX_OSWE = 0; + *IXP4XX_OSWK = 0; +}Are these magic CPU addresses you're writing to? Normally one uses bus macros to read/write to devices. But SoC and embedded stuff is often special...
Yep, these are mapped in in platform setup code in arch/arm/mach-ixp4xx.
Quoted 14 lines
Review
Quoted 11 lines
(quoted diff) +11
-0+ixp4xx_wdt_open(struct inode *inode, struct file *file) +{ + if (test_and_set_bit(WDT_IN_USE, &wdt_status)) + return -EBUSY; + + clear_bit(WDT_OK_TO_CLOSE, &wdt_status); + + wdt_enable(); + + return 0; +}You typically want semaphores rather than bit tests.
Quoted 1 line
And what about blocking on multiple openers?
I'm just copying other wdt driver in the tree do, so I'm assuming it's "standard practice" to just -EBUSY on multiple users. Documentation/watchdog/watchdog-api.txt doesn't really specify the behavior, so this may just be from historical reasons. In practice you will only have one application managing the watchdog.
Quoted 20 lines
Quoted 20 lines
(quoted diff) +2
-0+static ssize_t +ixp4xx_wdt_write(struct file *file, const char *data, size_t len, loff_t*ppos)
(quoted diff) +17
-0+{ + /* Can't seek (pwrite) on this device */ + if (ppos != &file->f_pos) + return -ESPIPE; + + if (len) { + if (!nowayout) { + size_t i; + + clear_bit(WDT_OK_TO_CLOSE, &wdt_status); + + for (i = 0; i != len; i++) { + char c; + + if (get_user(c, data + i)) + return -EFAULT; + if (c == 'V')
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +1
-0+ set_bit(WDT_OK_TO_CLOSE,
&wdt_status);(quoted diff) +4
-0+ } + } + wdt_enable(); + }do other watchdog drivers really twiddle a bit like this on each write?
They use a separate variable but it amounts to the same behavior. Clear it then reset it after we read the user command. (see i8xx_tco.c) I decided to twiddle a bit instead of using another static.
Updated patch attached with variable init removed.
~Deepak
Signature · 4 lines
Deepak Saxena - dsaxena at plexity dot net - http://www.plexity.net/ "Unlike me, many of you have accepted the situation of your imprisonment and will die here like rotten cabbages." - Number 6
drivers/char/watchdog/Kconfig +11 -0
@@ -84,6 +84,17 @@ Not sure? It's safe to say N. +config IXP4XX_WATCHDOG + tristate "IXP4xx Watchdog" + depends on WATCHDOG && ARCH_IXP4XX + help + Say Y here if to include support for the watchdog timer + in the Intel IXP4xx network processors. This driver can + be built as a module by choosing M. The module will + be called ixp4xx_wdt. + + Say N if you are unsure. + config SA1100_WATCHDOG tristate "SA1100 watchdog" depends on WATCHDOG && ARCH_SA1100
drivers/char/watchdog/Makefile +1 -0
@@ -35,3 +35,4 @@ obj-$(CONFIG_INDYDOG) += indydog.o obj-$(CONFIG_PCIPCWATCHDOG) += pcwd_pci.o obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o +obj-$(CONFIG_IXP4XX_WATCHDOG) += ixp4xx_wdt.o
drivers/char/watchdog/ixp4xx_wdt.c +233 -0
233 lines not on this page · raw message