watchdog: Add watchdog timer support for the WinSystems EBC-C384
Subsystems (2)
PATCH 1/1 watchdog: Add watchdog timer support for the WinSystems EBC-C384
The WinSystems EBC-C384 has an onboard watchdog timer. The timeout range supported by the watchdog timer is 1 second to 255 minutes. Timeouts under 256 seconds have a 1 second resolution, while the rest have a 1 minute resolution.
This driver adds watchdog timer support for this onboard watchdog timer. The timeout may be configured via the timeout module parameter.
--- MAINTAINERS | 6 ++ drivers/watchdog/Kconfig | 9 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/ebc-c384_wdt.c | 226 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 242 insertions(+) create mode 100644 drivers/watchdog/ebc-c384_wdt.c
MAINTAINERS +6 -0
@@ -11629,6 +11629,12 @@ M: David Härdeman <[email protected]> S: Maintained F: drivers/media/rc/winbond-cir.c +WINSYSTEMS EBC-C384 WATCHDOG DRIVER +M: William Breathitt Gray <[email protected]> +L: [email protected] +S: Maintained +F: drivers/watchdog/ebc-c384_wdt.c + WIMAX STACK M: Inaky Perez-Gonzalez <[email protected]> M: [email protected]
drivers/watchdog/Kconfig +9 -0
@@ -711,6 +711,15 @@ config ALIM7101_WDT Most people will say N. +config EBC_C386_WDT + tristate "WinSystems EBC-C384 watchdog timer support" + depends on X86 + select WATCHDOG_CORE + help + Enables watchdog timer support for the watchdog timer on the + WinSystems EBC-C384 motherboard. The timeout may be configured via + the timeout module parameter. + config F71808E_WDT tristate "Fintek F71808E, F71862FG, F71869, F71882FG and F71889FG Watchdog" depends on X86
drivers/watchdog/Makefile +1 -0
@@ -88,6 +88,7 @@ obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o obj-$(CONFIG_ADVANTECH_WDT) += advantechwdt.o obj-$(CONFIG_ALIM1535_WDT) += alim1535_wdt.o obj-$(CONFIG_ALIM7101_WDT) += alim7101_wdt.o +obj-$(CONFIG_EBC_C386_WDT) += ebc-c384_wdt.o obj-$(CONFIG_F71808E_WDT) += f71808e_wdt.o obj-$(CONFIG_SP5100_TCO) += sp5100_tco.o obj-$(CONFIG_GEODE_WDT) += geodewdt.o
drivers/watchdog/ebc-c384_wdt.c new file +226 -0
226 lines not on this page · raw message
Signature · 1 line
2.4.10
Guenter Roeck
maintainerQuoted 118 lines · William Breathitt Gray
The WinSystems EBC-C384 has an onboard watchdog timer. The timeout range supported by the watchdog timer is 1 second to 255 minutes. Timeouts under 256 seconds have a 1 second resolution, while the rest have a 1 minute resolution.
This driver adds watchdog timer support for this onboard watchdog timer. The timeout may be configured via the timeout module parameter.
Signed-off-by William Breathitt Gray <[email protected]>--- MAINTAINERS | 6 ++ drivers/watchdog/Kconfig | 9 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/ebc-c384_wdt.c | 226 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 242 insertions(+)create mode 100644
drivers/watchdog/ebc-c384_wdt.cMAINTAINERS +6
-0@@ -11629,6 +11629,12 @@ M: David Härdeman <[email protected]> S: Maintained F: drivers/media/rc/winbond-cir.c +WINSYSTEMS EBC-C384 WATCHDOG DRIVER +M: William Breathitt Gray <[email protected]> +L: [email protected] +S: Maintained +F: drivers/watchdog/ebc-c384_wdt.c + WIMAX STACK M: Inaky Perez-Gonzalez <[email protected]> M: [email protected]drivers/watchdog/Kconfig +9
-0@@ -711,6 +711,15 @@ config ALIM7101_WDT Most people will say N. +config EBC_C386_WDT + tristate "WinSystems EBC-C384 watchdog timer support" + depends on X86 + select WATCHDOG_CORE + help + Enables watchdog timer support for the watchdog timer on the + WinSystems EBC-C384 motherboard. The timeout may be configured via + the timeout module parameter. + config F71808E_WDT tristate "Fintek F71808E, F71862FG, F71869, F71882FG and F71889FG Watchdog" depends on X86drivers/watchdog/Makefile +1
-0@@ -88,6 +88,7 @@ obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o obj-$(CONFIG_ADVANTECH_WDT) += advantechwdt.o obj-$(CONFIG_ALIM1535_WDT) += alim1535_wdt.o obj-$(CONFIG_ALIM7101_WDT) += alim7101_wdt.o +obj-$(CONFIG_EBC_C386_WDT) += ebc-c384_wdt.o obj-$(CONFIG_F71808E_WDT) += f71808e_wdt.o obj-$(CONFIG_SP5100_TCO) += sp5100_tco.o obj-$(CONFIG_GEODE_WDT) += geodewdt.odrivers/watchdog/ebc-c384_wdt.c new file +46
-0@@ -0,0 +1,226 @@ +/* +* Watchdog timer driver for the WinSystems EBC-C384 +* Copyright (C) 2016 William Breathitt Gray +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License, version 2, as +* published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, but +* WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +* General Public License for more details. +*/ +#include <linux/device.h> +#include <linux/errno.h> +#include <linux/io.h> +#include <linux/ioport.h> +#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/platform_device.h> +#include <linux/spinlock.h> +#include <linux/types.h> +#include <linux/watchdog.h> + +#define MODULE_NAME "ebc-c384_wdt" +#define WATCHDOG_TIMEOUT 60 + +static bool nowayout =WATCHDOG_NOWAYOUT ; +module_param (nowayout, bool,0 ); +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=" + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); + +static unsigned timeout =WATCHDOG_TIMEOUT ; +module_param (timeout, uint,0 ); +MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default=" + __MODULE_STRING(WATCHDOG_TIMEOUT) ")"); + +/** +* struct ebc_c384_wdt - Watchdog timer device private data structure +* @wdd: instance of struct watchdog_device +* @lock: synchronization lock to prevent race conditions +* @base: base port address of the device +* @extent: extent of port address region of the device +*/ +struct ebc_c384_wdt { +struct watchdog_device wdd;
drivers/watchdog/ebc-c384_wdt.c line 56
Quoted 10 lines
drivers/watchdog/ebc-c384_wdt.c new file +10
-0+spinlock_t lock; +unsigned base; +unsigned extent; +}; + +static int ebc_c384_wdt_start (struct watchdog_device *wdev) +{ +struct ebc_c384_wdt *wdt =watchdog_get_drvdata (wdev); + +return wdt->wdd.ops->set_timeout (wdev, wdt->wdd.timeout);
This implies that setting the timeout would start the watchdog, which is inappropriate (the timeout can be set while the watchdog is stopped).
Also, setting the timeout sets both the resolution _and_ the timeout, which is probably unnecessary when starting or pinging the watchdog.
drivers/watchdog/ebc-c384_wdt.c line 69
Quoted 13 lines
drivers/watchdog/ebc-c384_wdt.c new file +13
-0+} + +static int ebc_c384_wdt_stop (struct watchdog_device *wdev) +{ +struct ebc_c384_wdt *wdt =watchdog_get_drvdata (wdev); +unsigned long flags; + +spin_lock_irqsave (&wdt->lock, flags); + +outb (0x00 , wdt->base +2 ); + +spin_unlock_irqrestore (&wdt->lock, flags); +
Is there a reason to believe that the locking provided by the watchdog core is insufficient ?
drivers/watchdog/ebc-c384_wdt.c line 77
Quoted 8 lines
drivers/watchdog/ebc-c384_wdt.c new file +8
-0+return 0 ; +} + +static int ebc_c384_wdt_set_timeout (struct watchdog_device *wdev,unsigned t) +{ +struct ebc_c384_wdt *wdt =watchdog_get_drvdata (wdev); +unsigned long flags; +unsigned minutes =0 ;
Unnecessary initialization.
drivers/watchdog/ebc-c384_wdt.c line 80
Quoted 3 lines
drivers/watchdog/ebc-c384_wdt.c new file +3
-0+ +if (!t || t > wdt->wdd.max_timeout) +return -EINVAL ;
This won't happen.
drivers/watchdog/ebc-c384_wdt.c line 87
Quoted 7 lines
drivers/watchdog/ebc-c384_wdt.c new file +7
-0+ +spin_lock_irqsave (&wdt->lock, flags); + +/* resolution is in minutes for timeouts greater than 255 seconds */ +if (t >255 ) { +/* truncate seconds to minute resolution */ + minutes = t/60 ;
t / 60;
drivers/watchdog/ebc-c384_wdt.c line 98
Quoted 11 lines
drivers/watchdog/ebc-c384_wdt.c new file +11
-0+ +/* set watchdog timer for minutes */ +outb (0x80 , wdt->base +1 ); +outb (minutes, wdt->base +2 ); + + wdt->wdd.timeout = minutes *60 ; + }else { +/* set watchdog timer for seconds */ +outb (0x00 , wdt->base +1 ); +outb (t, wdt->base +2 ); +
It may be better to set the number of seconds first before setting the scale.
Quoted 16 lines
drivers/watchdog/ebc-c384_wdt.c new file +16
-0+ wdt->wdd.timeout = t; + } + +spin_unlock_irqrestore (&wdt->lock, flags); + +return 0 ; +} + +static const struct watchdog_ops ebc_c384_wdt_ops = { + .start = ebc_c384_wdt_start, + .stop = ebc_c384_wdt_stop, + .set_timeout = ebc_c384_wdt_set_timeout +}; + +static const struct watchdog_info ebc_c384_wdt_info = { + .options =WDIOF_KEEPALIVEPING |WDIOF_MAGICCLOSE |WDIOF_SETTIMEOUT ,
drivers/watchdog/ebc-c384_wdt.c line 124
Quoted 10 lines
drivers/watchdog/ebc-c384_wdt.c new file +10
-0+ .identity = MODULE_NAME +}; + +static int __init ebc_c384_wdt_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct ebc_c384_wdt *wdt; +const unsigned base =0x564 ; +const unsigned extent =5 ; +const char *const name =dev_name (dev);
What is the value of those const variables ? Why not just use dev_name() and defines ?
drivers/watchdog/ebc-c384_wdt.c line 126
Quoted 2 lines
drivers/watchdog/ebc-c384_wdt.c new file +2
-0+int err; +
Is there a means to detect if this is the correct system ? DMI, maybe ? Blindly instantiating the driver seems to be a bit risky and should be avoided if possible.
drivers/watchdog/ebc-c384_wdt.c line 131
Quoted 5 lines
drivers/watchdog/ebc-c384_wdt.c new file +5
-0+ wdt =devm_kzalloc (dev,sizeof (*wdt),GFP_KERNEL ); +if (!wdt) +return -ENOMEM ; + +if (!request_region (base, extent, name)) {
Please use devm_request_region(), or explain why it is not feasible.
drivers/watchdog/ebc-c384_wdt.c line 133
Quoted 2 lines
drivers/watchdog/ebc-c384_wdt.c new file +2
-0+dev_err (dev,"Unable to lock %s port addresses (0x%X-0x%X)\n" , + name, base, base + extent);
name is redundant and already provided by dev_err().
drivers/watchdog/ebc-c384_wdt.c line 135
Quoted 2 lines
drivers/watchdog/ebc-c384_wdt.c new file +2
-0+ err = -EBUSY ; + goto err_lock_io_port;
return -EBUSY;
drivers/watchdog/ebc-c384_wdt.c line 145
Quoted 10 lines
drivers/watchdog/ebc-c384_wdt.c new file +10
-0+ } + +dev_set_drvdata (dev, wdt); + + wdt->wdd.info = &ebc_c384_wdt_info; + wdt->wdd.ops = &ebc_c384_wdt_ops; + wdt->wdd.min_timeout =1 ; + wdt->wdd.max_timeout =15300 ; + wdt->base = base; + wdt->extent = extent;
Both base and extent are constants. Why keep them in the data structure ?
drivers/watchdog/ebc-c384_wdt.c line 154
Quoted 9 lines
drivers/watchdog/ebc-c384_wdt.c new file +9
-0+ +spin_lock_init (&wdt->lock); + +watchdog_set_nowayout (&wdt->wdd, nowayout); +watchdog_set_drvdata (&wdt->wdd, wdt); + + err =watchdog_init_timeout (&wdt->wdd, timeout, dev); +if (err) + goto err_init_timeout;
A more tolerant implementation would set the default timeout.
drivers/watchdog/ebc-c384_wdt.c line 163
Quoted 9 lines
drivers/watchdog/ebc-c384_wdt.c new file +9
-0+ + err =watchdog_register_device (&wdt->wdd); +if (err) + goto err_register_wdt; + +return 0 ; + +err_register_wdt: +err_init_timeout:
Two labels pointing to the same location is unnecessary.
drivers/watchdog/ebc-c384_wdt.c line 165
Quoted 2 lines
drivers/watchdog/ebc-c384_wdt.c new file +2
-0+release_region (base, extent); +err_lock_io_port:
Unnecessary label. When you use devm_request_region(), the error handling should no longer be needed at all.
drivers/watchdog/ebc-c384_wdt.c line 173
Quoted 8 lines
drivers/watchdog/ebc-c384_wdt.c new file +8
-0+return err; +} + +static int ebc_c384_wdt_remove (struct platform_device *pdev) +{ +struct ebc_c384_wdt *wdt =platform_get_drvdata (pdev); + +release_region (wdt->base, wdt->extent);
No watchdog_unregister_device() ?
Quoted 40 lines
drivers/watchdog/ebc-c384_wdt.c new file +40
-0+ +return 0 ; +} + +static struct platform_driver ebc_c384_wdt_driver = { + .driver = { + .name =MODULE_NAME + }, + .remove = ebc_c384_wdt_remove +}; + +static struct platform_device *ebc_c384_wdt_device; + +static int __init ebc_c384_wdt_init (void ) +{ +int err; + + ebc_c384_wdt_device =platform_device_alloc ( + ebc_c384_wdt_driver.driver.name,-1 ); +if (!ebc_c384_wdt_device) +return -ENOMEM ; + + err =platform_device_add (ebc_c384_wdt_device); +if (err) + goto err_platform_device; + + err =platform_driver_probe (&ebc_c384_wdt_driver, ebc_c384_wdt_probe); +if (err) + goto err_platform_driver; + +return 0 ; + +err_platform_driver: +platform_device_del (ebc_c384_wdt_device); +err_platform_device: +platform_device_put (ebc_c384_wdt_device); +return err; + +return 0 ; +}
drivers/watchdog/ebc-c384_wdt.c line 223
Quoted 10 lines
drivers/watchdog/ebc-c384_wdt.c new file +10
-0+ +static void __exit ebc_c384_wdt_exit(void) +{ +platform_device_unregister (ebc_c384_wdt_device); +platform_driver_unregister (&ebc_c384_wdt_driver); +} + +module_init (ebc_c384_wdt_init); +module_exit (ebc_c384_wdt_exit); +
Have you considered using module_platform_driver_probe() ?
drivers/watchdog/ebc-c384_wdt.c line 226
Quoted 3 lines
drivers/watchdog/ebc-c384_wdt.c new file +3
-0+MODULE_AUTHOR ("William Breathitt Gray <[email protected]>" ); +MODULE_DESCRIPTION ("WinSystems EBC-C384 watchdog timer driver" ); +MODULE_LICENSE ("GPL" );
GPL v2 as stated above.
Quoted 1 line
No MODULE_ALIAS ?
Quoted 6 lines · Guenter Roeck
This implies that setting the timeout would start the watchdog, which is inappropriate (the timeout can be set while the watchdog is stopped).
Also, setting the timeout sets both the resolution _and_ the timeout, which is probably unnecessary when starting or pinging the watchdog.
Help me understand the functionality of the watchdog operations briefly since I'm relatively new to the interface. Is it proper to say that the start callback starts (and in my case also pings) the watchdog based on the value of the previously configured timeout member, while the set_timeout callback merely sets the timeout member itself to the correct value in seconds accordingly to the watchdog timer's resolution?
Quoted 4 lines
drivers/watchdog/ebc-c384_wdt.c line 124
Quoted 3 lines
drivers/watchdog/ebc-c384_wdt.c new file +3
-0+const unsigned base =0x564 ; +const unsigned extent =5 ; +const char *const name =dev_name (dev);
Quoted 8 lines
What is the value of those const variables ? Why not just use
dev_name()and defines ?Review
Quoted 2 lines
(quoted diff) +2
-0+ int err; +Is there a means to detect if this is the correct system ? DMI, maybe ? Blindly instantiating the driver seems to be a bit risky and should be avoided if possible.
Unfortunately, the watchdog timer hardware lacks probing capabilities; the documentation for the motherboard indicates that the watchdog timer is exposed over an ISA-style I/O-mapped port address. In other words, the watchdog timer is non-hotpluggable.
I agree that carrying around the constant values in the private data structure is somewhat unnecessary, so I'll give them global scope over the file. I'm hesitant to lose the type-safety of C variables; is there a reason to prefer preprocessor defines over const-qualified variables?
Quoted 5 lines
drivers/watchdog/ebc-c384_wdt.c line 154
Quoted 3 lines
drivers/watchdog/ebc-c384_wdt.c new file +3
-0+ err =watchdog_init_timeout (&wdt->wdd, timeout, dev); +if (err) + goto err_init_timeout;A more tolerant implementation would set the default timeout.
Should I remove the timeout module parameter entirely, and simply initialize the watchdog_device timeout member to the default timeout I want (e.g. wdt->wdd.timeout = 60)? Would I still need to call watchdog_init_timeout in that case?
Quoted 1 line
Have you considered using
module_platform_driver_probe()?
For some reason, I was under the impression that I must allocate a platform_device before calling platform_driver_probe. I'll try module_platform_driver_probe since that would indeed be far simpler if the platform_device setup code is unnecessary.
Quoted 1 line
No
MODULE_ALIAS?
Since the watchdog timer hardware is non-hotpluggable, I'm not sure I should add a MODULE_ALIAS for autoloading the module.
William Breathitt Gray
Guenter Roeck
maintainerQuoted 8 lines · William Breathitt Gray
Quoted 6 lines · Guenter Roeck
This implies that setting the timeout would start the watchdog, which is inappropriate (the timeout can be set while the watchdog is stopped).
Also, setting the timeout sets both the resolution _and_ the timeout, which is probably unnecessary when starting or pinging the watchdog.
Quoted 7 lines
Help me understand the functionality of the watchdog operations briefly since I'm relatively new to the interface. Is it proper to say that the start callback starts (and in my case also pings) the watchdog based on the value of the previously configured timeout member, while the
set_timeoutcallback merely sets the timeout member itself to the correct value in seconds accordingly to the watchdog timer's resolution?
Correct. In your case it may be sufficient to just set the 'timeout' variable to a valid timeout (ie one supported by the hardware).
Quoted 9 lines
Quoted 9 lines
drivers/watchdog/ebc-c384_wdt.c line 124
Quoted 3 lines
drivers/watchdog/ebc-c384_wdt.c new file +3
-0+const unsigned base =0x564 ; +const unsigned extent =5 ; +const char *const name =dev_name (dev);What is the value of those const variables ? Why not just use
dev_name()and defines ?Quoted 2 lines
(quoted diff) +2
-0+ int err; +
Quoted 9 lines
Quoted 3 lines
Is there a means to detect if this is the correct system ? DMI, maybe ? Blindly instantiating the driver seems to be a bit risky and should be avoided if possible.
Unfortunately, the watchdog timer hardware lacks probing capabilities; the documentation for the motherboard indicates that the watchdog timer is exposed over an ISA-style I/O-mapped port address. In other words, the watchdog timer is non-hotpluggable.
So how about DMI ? This is a PC, after all, so it should be possible to identify the hardware with DMI. We should have _something_ available to identify the hardware.
Quoted 5 lines
I agree that carrying around the constant values in the private data structure is somewhat unnecessary, so I'll give them global scope over the file. I'm hesitant to lose the type-safety of C variables; is there a reason to prefer preprocessor defines over const-qualified variables?
It is the common and established approach to use defines (you use a define for WATCHDOG_TIMEOUT as well). The type-safety argument doesn't really apply; to me it is similar to the argument for Yoda programming. The compiler will happily complain if you use an integer define as pointer, or a string as integer.
Defines are used all over the kernel, and work perfectly fine. I don't see a need to change that. Worse, it makes life more difficult for reviewers.
Quoted 11 lines
Quoted 5 lines
drivers/watchdog/ebc-c384_wdt.c line 154
Quoted 3 lines
drivers/watchdog/ebc-c384_wdt.c new file +3
-0+ err =watchdog_init_timeout (&wdt->wdd, timeout, dev); +if (err) + goto err_init_timeout;A more tolerant implementation would set the default timeout.
Should I remove the timeout module parameter entirely, and simply initialize the
watchdog_devicetimeout member to the default timeout I want (e.g.wdt->wdd.timeout= 60)? Would I still need to callwatchdog_init_timeoutin that case?
I didn't want to suggest that. One option would be to set wdt->wdd.timeout = 60 and ignore the return value from watchdog_init_timeout, like most watchdog drivers. Another would be something like
wdt->wdd.timeout = WATCHDOG_TIMEOUT; if (watchdog_init_timeout(&wdt->wdd, timeout, dev)) dev_warn(dev, "Invalid timeout %d, using default\n", timeout);
You can also abort, as you currently do, I just think it is a bit strict.
Quoted 7 lines
Quoted 1 line
Have you considered using
module_platform_driver_probe()?For some reason, I was under the impression that I must allocate a
platform_devicebefore callingplatform_driver_probe. I'll trymodule_platform_driver_probesince that would indeed be far simpler if theplatform_devicesetup code is unnecessary.
Ah yes, my mistake. The idea is that another driver (usually a platform driver, or architecture initialization code) would instantiate the device. Sorry for the noise.
Quoted 5 lines
Quoted 1 line
No
MODULE_ALIAS?Since the watchdog timer hardware is non-hotpluggable, I'm not sure I should add a
MODULE_ALIASfor autoloading the module.
Not sure I understand what that has to do with hotplug. Almost all of the 39 watchdog drivers defining MODULE_ALIAS are not hot-pluggable. Ok, let's see if it comes back to bite us ;-).
Guenter