[PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

[PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

[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 <vilhelm.gray@gmail.com>
---
Changes in v3:
  - Remove unnecessary explicit initialization of the timeout parameter
  - Move dmi_match call to beginning of init function
  - Create WATCHDOG_MAX_TIMEOUT define and explain its magic number
  - Use platform_set_drvdata to match later call to platform_get_drvdata
  - Use MODULE_NAME as argument for the platform_device_alloc call
  - Remove duplicate return statement for the init function

 MAINTAINERS                     |   6 ++
 drivers/watchdog/Kconfig        |   9 ++
 drivers/watchdog/Makefile       |   1 +
 drivers/watchdog/ebc-c384_wdt.c | 187 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 203 insertions(+)
 create mode 100644 drivers/watchdog/ebc-c384_wdt.c

diff --git a/MAINTAINERS b/MAINTAINERS
index b1e3da7..c058abf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11629,6 +11629,12 @@ M:	David Härdeman <david@hardeman.nu>
 S:	Maintained
 F:	drivers/media/rc/winbond-cir.c
 
+WINSYSTEMS EBC-C384 WATCHDOG DRIVER
+M:	William Breathitt Gray <vilhelm.gray@gmail.com>
+L:	linux-watchdog@vger.kernel.org
+S:	Maintained
+F:	drivers/watchdog/ebc-c384_wdt.c
+
 WIMAX STACK
 M:	Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
 M:	linux-wimax@intel.com
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 4f0e7be..b5b1353 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -711,6 +711,15 @@ config ALIM7101_WDT
 
 	  Most people will say N.
 
+config EBC_C386_WDT
+	tristate "WinSystems EBC-C384 Watchdog Timer"
+	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
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index f566753..1522316 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -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
diff --git a/drivers/watchdog/ebc-c384_wdt.c b/drivers/watchdog/ebc-c384_wdt.c
new file mode 100644
index 0000000..9166b02
--- /dev/null
+++ b/drivers/watchdog/ebc-c384_wdt.c
@@ -0,0 +1,187 @@
+/*
+ * 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/dmi.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/types.h>
+#include <linux/watchdog.h>
+
+#define MODULE_NAME "ebc-c384_wdt"
+#define WATCHDOG_TIMEOUT 60
+/* Since the timeout value in minutes must fit in a single byte when sent to the
+ * watchdog timer, the maximum timeout possible is 15300 (255 * 60) seconds
+ */
+#define WATCHDOG_MAX_TIMEOUT 15300
+#define BASE_ADDR 0x564
+#define ADDR_EXTENT 5
+#define CFG_ADDR (BASE_ADDR + 1)
+#define PET_ADDR (BASE_ADDR + 2)
+
+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;
+module_param(timeout, uint, 0);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default="
+	__MODULE_STRING(WATCHDOG_TIMEOUT) ")");
+
+static int ebc_c384_wdt_start(struct watchdog_device *wdev)
+{
+	unsigned t = wdev->timeout;
+
+	/* resolution is in minutes for timeouts greater than 255 seconds */
+	if (t > 255)
+		t /= 60;
+
+	outb(t, PET_ADDR);
+
+	return 0;
+}
+
+static int ebc_c384_wdt_stop(struct watchdog_device *wdev)
+{
+	outb(0x00, PET_ADDR);
+
+	return 0;
+}
+
+static int ebc_c384_wdt_set_timeout(struct watchdog_device *wdev, unsigned t)
+{
+	/* resolution is in minutes for timeouts greater than 255 seconds */
+	if (t > 255) {
+		/* truncate second resolution to minute resolution */
+		t /= 60;
+		wdev->timeout = t * 60;
+
+		/* set watchdog timer for minutes */
+		outb(0x00, CFG_ADDR);
+	} else {
+		wdev->timeout = t;
+
+		/* set watchdog timer for seconds */
+		outb(0x80, CFG_ADDR);
+	}
+
+	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,
+	.identity = MODULE_NAME
+};
+
+static int __init ebc_c384_wdt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct watchdog_device *wdd;
+
+	if (!devm_request_region(dev, BASE_ADDR, ADDR_EXTENT, dev_name(dev))) {
+		dev_err(dev, "Unable to lock port addresses (0x%X-0x%X)\n",
+			BASE_ADDR, BASE_ADDR + ADDR_EXTENT);
+		return -EBUSY;
+	}
+
+	wdd = devm_kzalloc(dev, sizeof(*wdd), GFP_KERNEL);
+	if (!wdd)
+		return -ENOMEM;
+
+	wdd->info = &ebc_c384_wdt_info;
+	wdd->ops = &ebc_c384_wdt_ops;
+	wdd->timeout = WATCHDOG_TIMEOUT;
+	wdd->min_timeout = 1;
+	wdd->max_timeout = WATCHDOG_MAX_TIMEOUT;
+
+	watchdog_set_nowayout(wdd, nowayout);
+
+	if (watchdog_init_timeout(wdd, timeout, dev))
+		dev_warn(dev, "Invalid timeout (%u seconds), using default (%u seconds)\n",
+			timeout, WATCHDOG_TIMEOUT);
+
+	platform_set_drvdata(pdev, wdd);
+
+	return watchdog_register_device(wdd);
+}
+
+static int ebc_c384_wdt_remove(struct platform_device *pdev)
+{
+	struct watchdog_device *wdd = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(wdd);
+
+	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;
+
+	if (!dmi_match(DMI_BOARD_NAME, "EBC-C384 SBC"))
+		return -ENODEV;
+
+	ebc_c384_wdt_device = platform_device_alloc(MODULE_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;
+}
+
+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);
+
+MODULE_AUTHOR("William Breathitt Gray <vilhelm.gray@gmail.com>");
+MODULE_DESCRIPTION("WinSystems EBC-C384 watchdog timer driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" MODULE_NAME);
-- 
2.4.10

Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

[One Thousand Gnomes]

> +static int ebc_c384_wdt_set_timeout(struct watchdog_device *wdev, unsigned t)
> +{
> +	/* resolution is in minutes for timeouts greater than 255 seconds */
> +	if (t > 255) {
> +		/* truncate second resolution to minute resolution */
> +		t /= 60;
> +		wdev->timeout = t * 60;
> +
> +		/* set watchdog timer for minutes */
> +		outb(0x00, CFG_ADDR);

If ask for 299 seconds surely I should get 300 not 240 ?
(Whether to round off or round up is an interesting question for the
middle range - does it go off early or late - I'd have said late but...)

> +static int __init ebc_c384_wdt_init(void)
> +{
> +	int err;
> +
> +	if (!dmi_match(DMI_BOARD_NAME, "EBC-C384 SBC"))
> +		return -ENODEV;
> +

Is there no ACPI entry for it ?

Alan

Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

[Guenter Roeck]

On 01/25/2016 11:28 AM, One Thousand Gnomes wrote:
>> +static int ebc_c384_wdt_set_timeout(struct watchdog_device *wdev, unsigned t)
>> +{
>> +	/* resolution is in minutes for timeouts greater than 255 seconds */
>> +	if (t > 255) {
>> +		/* truncate second resolution to minute resolution */
>> +		t /= 60;
>> +		wdev->timeout = t * 60;
>> +
>> +		/* set watchdog timer for minutes */
>> +		outb(0x00, CFG_ADDR);
>
> If ask for 299 seconds surely I should get 300 not 240 ?
> (Whether to round off or round up is an interesting question for the
> middle range - does it go off early or late - I'd have said late but...)
>

Matter of endless discussion. Some argue that the value should be rounded
up, some argue that it should be rounded down, some argue that it should
be rounded to the closest match. Each camp has its own valid arguments.
I usually leave it up to the driver's author to decide, with a slight
preference to never select a value larger than requested.

>> +static int __init ebc_c384_wdt_init(void)
>> +{
>> +	int err;
>> +
>> +	if (!dmi_match(DMI_BOARD_NAME, "EBC-C384 SBC"))
>> +		return -ENODEV;
>> +
>
> Is there no ACPI entry for it ?
>
Same here. As long as the board is identified, I tend to leave it up
to the driver author to decide _how_ to identify it.

Only question for me would be if the watchdog timer is implemented
in a Super-IO chip, and if so, if it would be possible to use the chip
identification instead of a DMI (or ACPI) entry to instantiate the driver.

Thanks,
Guenter

Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

[William Breathitt Gray]

On 01/25/2016 03:42 PM, Guenter Roeck wrote:
> On 01/25/2016 11:28 AM, One Thousand Gnomes wrote:
>> If ask for 299 seconds surely I should get 300 not 240 ?
>> (Whether to round off or round up is an interesting question for the
>> middle range - does it go off early or late - I'd have said late but...)
>>
> 
> Matter of endless discussion. Some argue that the value should be rounded
> up, some argue that it should be rounded down, some argue that it should
> be rounded to the closest match. Each camp has its own valid arguments.
> I usually leave it up to the driver's author to decide, with a slight
> preference to never select a value larger than requested.

I implemented it to round down simply because it was the simplest        
solution (i.e. integer truncation). Although I see merit in an           
implementation that rounds to the closest valid value, I'll keep the     
current implementation for now due to its simplicity; if enough users of 
the driver prefer a different implementation, then I'll add it in a      
later patch.

>> Is there no ACPI entry for it ?
>>
> Same here. As long as the board is identified, I tend to leave it up
> to the driver author to decide _how_ to identify it.
> 
> Only question for me would be if the watchdog timer is implemented
> in a Super-IO chip, and if so, if it would be possible to use the chip
> identification instead of a DMI (or ACPI) entry to instantiate the driver.

I do not believe there is an ACPI entry for it. Interestingly, the       
watchdog timer BIOS configuration option for this motherboard is listed  
under the Super I/O menu; perhaps this watchdog timer is implemented in  
the Super I/O chip.                                                      
                                                                         
The manual for this motherboard does not provide much information about  
the Super I/O chip (no model number, etc.), and neither sensors-detect    
nor superiotool was able to detect it. I've sent an email to the         
motherboard company (WinSystems) requesting further information about    
the Super I/O chip and whether the watchdog timer is built-in to the     
Super I/O chip.                                                          
                                                                         
William Breathitt Gray

Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

[Guenter Roeck]

On 01/25/2016 11:09 AM, William Breathitt Gray wrote:
> 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 <vilhelm.gray@gmail.com>

Nitpicks only this time. Please fix and resubmit, and feel free to add

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

Thanks,
Guenter

> ---
> Changes in v3:
>    - Remove unnecessary explicit initialization of the timeout parameter
>    - Move dmi_match call to beginning of init function
>    - Create WATCHDOG_MAX_TIMEOUT define and explain its magic number
>    - Use platform_set_drvdata to match later call to platform_get_drvdata
>    - Use MODULE_NAME as argument for the platform_device_alloc call
>    - Remove duplicate return statement for the init function
>
>   MAINTAINERS                     |   6 ++
>   drivers/watchdog/Kconfig        |   9 ++
>   drivers/watchdog/Makefile       |   1 +
>   drivers/watchdog/ebc-c384_wdt.c | 187 ++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 203 insertions(+)
>   create mode 100644 drivers/watchdog/ebc-c384_wdt.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b1e3da7..c058abf 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11629,6 +11629,12 @@ M:	David Härdeman <david@hardeman.nu>
>   S:	Maintained
>   F:	drivers/media/rc/winbond-cir.c
>
> +WINSYSTEMS EBC-C384 WATCHDOG DRIVER
> +M:	William Breathitt Gray <vilhelm.gray@gmail.com>
> +L:	linux-watchdog@vger.kernel.org
> +S:	Maintained
> +F:	drivers/watchdog/ebc-c384_wdt.c
> +
>   WIMAX STACK
>   M:	Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
>   M:	linux-wimax@intel.com
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 4f0e7be..b5b1353 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -711,6 +711,15 @@ config ALIM7101_WDT
>
>   	  Most people will say N.
>
> +config EBC_C386_WDT
> +	tristate "WinSystems EBC-C384 Watchdog Timer"
> +	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
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index f566753..1522316 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -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
> diff --git a/drivers/watchdog/ebc-c384_wdt.c b/drivers/watchdog/ebc-c384_wdt.c
> new file mode 100644
> index 0000000..9166b02
> --- /dev/null
> +++ b/drivers/watchdog/ebc-c384_wdt.c
> @@ -0,0 +1,187 @@
> +/*
> + * 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/dmi.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/types.h>
> +#include <linux/watchdog.h>
> +
> +#define MODULE_NAME "ebc-c384_wdt"
> +#define WATCHDOG_TIMEOUT 60
> +/* Since the timeout value in minutes must fit in a single byte when sent to the
> + * watchdog timer, the maximum timeout possible is 15300 (255 * 60) seconds
> + */

/*
  * Please use standard multi-line comments.
  */

> +#define WATCHDOG_MAX_TIMEOUT 15300
> +#define BASE_ADDR 0x564
> +#define ADDR_EXTENT 5
> +#define CFG_ADDR (BASE_ADDR + 1)
> +#define PET_ADDR (BASE_ADDR + 2)

Please use tabs before the values for alignment.

> +
> +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;
> +module_param(timeout, uint, 0);
> +MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default="
> +	__MODULE_STRING(WATCHDOG_TIMEOUT) ")");
> +
> +static int ebc_c384_wdt_start(struct watchdog_device *wdev)
> +{
> +	unsigned t = wdev->timeout;
> +
> +	/* resolution is in minutes for timeouts greater than 255 seconds */
> +	if (t > 255)
> +		t /= 60;
> +
> +	outb(t, PET_ADDR);
> +
> +	return 0;
> +}
> +
> +static int ebc_c384_wdt_stop(struct watchdog_device *wdev)
> +{
> +	outb(0x00, PET_ADDR);
> +
> +	return 0;
> +}
> +
> +static int ebc_c384_wdt_set_timeout(struct watchdog_device *wdev, unsigned t)
> +{
> +	/* resolution is in minutes for timeouts greater than 255 seconds */
> +	if (t > 255) {
> +		/* truncate second resolution to minute resolution */
> +		t /= 60;
> +		wdev->timeout = t * 60;
> +
> +		/* set watchdog timer for minutes */
> +		outb(0x00, CFG_ADDR);
> +	} else {
> +		wdev->timeout = t;
> +
> +		/* set watchdog timer for seconds */
> +		outb(0x80, CFG_ADDR);
> +	}
> +
> +	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,
> +	.identity = MODULE_NAME
> +};
> +
> +static int __init ebc_c384_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct watchdog_device *wdd;
> +
> +	if (!devm_request_region(dev, BASE_ADDR, ADDR_EXTENT, dev_name(dev))) {
> +		dev_err(dev, "Unable to lock port addresses (0x%X-0x%X)\n",
> +			BASE_ADDR, BASE_ADDR + ADDR_EXTENT);
> +		return -EBUSY;
> +	}
> +
> +	wdd = devm_kzalloc(dev, sizeof(*wdd), GFP_KERNEL);
> +	if (!wdd)
> +		return -ENOMEM;
> +
> +	wdd->info = &ebc_c384_wdt_info;
> +	wdd->ops = &ebc_c384_wdt_ops;
> +	wdd->timeout = WATCHDOG_TIMEOUT;
> +	wdd->min_timeout = 1;
> +	wdd->max_timeout = WATCHDOG_MAX_TIMEOUT;
> +
> +	watchdog_set_nowayout(wdd, nowayout);
> +
> +	if (watchdog_init_timeout(wdd, timeout, dev))
> +		dev_warn(dev, "Invalid timeout (%u seconds), using default (%u seconds)\n",
> +			timeout, WATCHDOG_TIMEOUT);

Multi-line alignment is off by one character.

> +
> +	platform_set_drvdata(pdev, wdd);
> +
> +	return watchdog_register_device(wdd);
> +}
> +
> +static int ebc_c384_wdt_remove(struct platform_device *pdev)
> +{
> +	struct watchdog_device *wdd = platform_get_drvdata(pdev);
> +
> +	watchdog_unregister_device(wdd);
> +
> +	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;
> +
> +	if (!dmi_match(DMI_BOARD_NAME, "EBC-C384 SBC"))
> +		return -ENODEV;
> +
> +	ebc_c384_wdt_device = platform_device_alloc(MODULE_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;
> +}
> +
> +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);
> +
> +MODULE_AUTHOR("William Breathitt Gray <vilhelm.gray@gmail.com>");
> +MODULE_DESCRIPTION("WinSystems EBC-C384 watchdog timer driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:" MODULE_NAME);
>

Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

[Guenter Roeck]

On 01/25/2016 03:36 PM, William Breathitt Gray wrote:
> On 01/25/2016 03:42 PM, Guenter Roeck wrote:
>> On 01/25/2016 11:28 AM, One Thousand Gnomes wrote:
>>> If ask for 299 seconds surely I should get 300 not 240 ?
>>> (Whether to round off or round up is an interesting question for the
>>> middle range - does it go off early or late - I'd have said late but...)
>>>
>>
>> Matter of endless discussion. Some argue that the value should be rounded
>> up, some argue that it should be rounded down, some argue that it should
>> be rounded to the closest match. Each camp has its own valid arguments.
>> I usually leave it up to the driver's author to decide, with a slight
>> preference to never select a value larger than requested.
>
> I implemented it to round down simply because it was the simplest
> solution (i.e. integer truncation). Although I see merit in an
> implementation that rounds to the closest valid value, I'll keep the
> current implementation for now due to its simplicity; if enough users of
> the driver prefer a different implementation, then I'll add it in a
> later patch.
>
>>> Is there no ACPI entry for it ?
>>>
>> Same here. As long as the board is identified, I tend to leave it up
>> to the driver author to decide _how_ to identify it.
>>
>> Only question for me would be if the watchdog timer is implemented
>> in a Super-IO chip, and if so, if it would be possible to use the chip
>> identification instead of a DMI (or ACPI) entry to instantiate the driver.
>
> I do not believe there is an ACPI entry for it. Interestingly, the
> watchdog timer BIOS configuration option for this motherboard is listed
> under the Super I/O menu; perhaps this watchdog timer is implemented in
> the Super I/O chip.
>
Normally it is. Question is which one.

> The manual for this motherboard does not provide much information about
> the Super I/O chip (no model number, etc.), and neither sensors-detect
> nor superiotool was able to detect it. I've sent an email to the
> motherboard company (WinSystems) requesting further information about
> the Super I/O chip and whether the watchdog timer is built-in to the
> Super I/O chip.
>

Ah, I somehow thought you were associated with WinSystems, since you know
how to configure the chip.

Did you get any useful output from sensors-detect or superiotool
(like 'unknown chip xxxx'), or did those tools find nothing ?

Thanks,
Guenter

Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

[Guenter Roeck]

On 01/25/2016 03:36 PM, William Breathitt Gray wrote:
> On 01/25/2016 03:42 PM, Guenter Roeck wrote:
>> On 01/25/2016 11:28 AM, One Thousand Gnomes wrote:
>>> If ask for 299 seconds surely I should get 300 not 240 ?
>>> (Whether to round off or round up is an interesting question for the
>>> middle range - does it go off early or late - I'd have said late but...)
>>>
>>
>> Matter of endless discussion. Some argue that the value should be rounded
>> up, some argue that it should be rounded down, some argue that it should
>> be rounded to the closest match. Each camp has its own valid arguments.
>> I usually leave it up to the driver's author to decide, with a slight
>> preference to never select a value larger than requested.
>
> I implemented it to round down simply because it was the simplest
> solution (i.e. integer truncation). Although I see merit in an
> implementation that rounds to the closest valid value, I'll keep the
> current implementation for now due to its simplicity; if enough users of
> the driver prefer a different implementation, then I'll add it in a
> later patch.
>
>>> Is there no ACPI entry for it ?
>>>
>> Same here. As long as the board is identified, I tend to leave it up
>> to the driver author to decide _how_ to identify it.
>>
>> Only question for me would be if the watchdog timer is implemented
>> in a Super-IO chip, and if so, if it would be possible to use the chip
>> identification instead of a DMI (or ACPI) entry to instantiate the driver.
>
> I do not believe there is an ACPI entry for it. Interestingly, the
> watchdog timer BIOS configuration option for this motherboard is listed
> under the Super I/O menu; perhaps this watchdog timer is implemented in
> the Super I/O chip.
>
> The manual for this motherboard does not provide much information about
> the Super I/O chip (no model number, etc.), and neither sensors-detect
> nor superiotool was able to detect it. I've sent an email to the
> motherboard company (WinSystems) requesting further information about
> the Super I/O chip and whether the watchdog timer is built-in to the
> Super I/O chip.
>

I had a close look at the board pictures in the manual. Looks like there
is a NCT7802Y hardware monitoring chip on the board. This suggests that the
Super-IO chip does probably not implement hardware monitoring. Not that
it helps much to know that, but it reduces the possibilities.

There is also a pretty large Lattice FPGA on the board, which makes it at
least somewhat likely that the functionality is implemented in that FPGA.

Guenter

Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

[Paul Bolle]

On ma, 2016-01-25 at 14:09 -0500, William Breathitt Gray wrote:
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig

> +config EBC_C386_WDT
> +	tristate "WinSystems EBC-C384 Watchdog Timer"
> +	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.

It's utterly trivial, but I couldn't help noticing that this supports
the "EBC-C384" motherboard but the Kconfig symbol is EBC_C386_WDT (note
the 6). Any particular reason?


Thanks,


Paul Bolle

Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

[William Breathitt Gray]

On 01/26/2016 04:09 AM, Paul Bolle wrote:
> On ma, 2016-01-25 at 14:09 -0500, William Breathitt Gray wrote:
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
> 
>> +config EBC_C386_WDT
>> +	tristate "WinSystems EBC-C384 Watchdog Timer"
>> +	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.
> 
> It's utterly trivial, but I couldn't help noticing that this supports
> the "EBC-C384" motherboard but the Kconfig symbol is EBC_C386_WDT (note
> the 6). Any particular reason?

Looks like you caught a typo of mine! I'll fix it in my next patch. Many 
thanks Paul for your good eye.                                           
                                                                         
William Breathitt Gray 

Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

[William Breathitt Gray]

On 01/25/2016 08:26 PM, Guenter Roeck wrote:
>> The manual for this motherboard does not provide much information about
>> the Super I/O chip (no model number, etc.), and neither sensors-detect
>> nor superiotool was able to detect it. I've sent an email to the
>> motherboard company (WinSystems) requesting further information about
>> the Super I/O chip and whether the watchdog timer is built-in to the
>> Super I/O chip.
>>
> 
> Ah, I somehow thought you were associated with WinSystems, since you know
> how to configure the chip.
> 
> Did you get any useful output from sensors-detect or superiotool
> (like 'unknown chip xxxx'), or did those tools find nothing ?

Unfortunately, the sensors-detect only reported "No" for each Super I/O  
chip test, while the superiotool gave an unhelpful "No Super I/O chip    
detected" message.                                                       
                                                                         
I haven't heard a response yet from WinSystems, but I'll give them a     
couple days before sending another email to their engineering            
department. For now, I'll submit a version 4 of this patch to get the    
minor updates I made out for review; for what its worth, I believe the   
dmi_match method will be sufficient until I get an update from           
WinSystems helping me get a proper check to identify the Super I/O chip. 
                                                                         
William Breathitt Gray

Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

[Guenter Roeck]

On 01/26/2016 03:38 PM, William Breathitt Gray wrote:
> On 01/25/2016 08:26 PM, Guenter Roeck wrote:
>>> The manual for this motherboard does not provide much information about
>>> the Super I/O chip (no model number, etc.), and neither sensors-detect
>>> nor superiotool was able to detect it. I've sent an email to the
>>> motherboard company (WinSystems) requesting further information about
>>> the Super I/O chip and whether the watchdog timer is built-in to the
>>> Super I/O chip.
>>>
>>
>> Ah, I somehow thought you were associated with WinSystems, since you know
>> how to configure the chip.
>>
>> Did you get any useful output from sensors-detect or superiotool
>> (like 'unknown chip xxxx'), or did those tools find nothing ?
>
> Unfortunately, the sensors-detect only reported "No" for each Super I/O
> chip test, while the superiotool gave an unhelpful "No Super I/O chip
> detected" message.
>

Too bad. That suggests that the watchdog may in fact be implemented in the fpga.

> I haven't heard a response yet from WinSystems, but I'll give them a
> couple days before sending another email to their engineering
> department. For now, I'll submit a version 4 of this patch to get the
> minor updates I made out for review; for what its worth, I believe the
> dmi_match method will be sufficient until I get an update from
> WinSystems helping me get a proper check to identify the Super I/O chip.
>

I added v4 to my watchdog-next branch and will include it in my pull request
to Wim later in the release cycle. If there are enhancements (eg if WinSystems
comes back with an actual specification), we can implement those as incremental
improvements.

Thanks,
Guenter

Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

[William Breathitt Gray]

On Tue, Jan 26, 2016 at 09:02:45PM -0800, Guenter Roeck wrote:
>> Unfortunately, the sensors-detect only reported "No" for each Super I/O
>> chip test, while the superiotool gave an unhelpful "No Super I/O chip
>> detected" message.
>>
>
>Too bad. That suggests that the watchdog may in fact be implemented in the fpga.

I received a response from WinSystems: the watchdog timer is implemented
in the Lattice FPGA (base address 0x298), along with other WinSystems
firmware. I was offered two methods of identifying the chip.

The first method is to use a 16-bit read of the register at port address
0x29E to get the version number of the watchdog timer; my machine
reported a value of 0x0009. Unfortunately, I don't believe this method
is very reliable since the version number may not be consistent across
these motherboards, and the same value could easily happen to be
returned by an unknown hardware.

The second method is slightly more involved so I'll quote WinSystems:

> 8-bit read of 299h – save this value
> 8-bit write of 60h to 299h
> 16-bit read of 29ah should return the base address of the WDT which is 564h
> 8-bit write of saved value to 299h - don’t want t accidentally change the WDT base address

If the system does return a value of 0x564, then it's pretty safe to say
that the watchdog timer is implemented on the chip. However, I'm not
sure it would be safe to send write commands to a port address until the
hardware has been identified; this second method may not be the best
route either.

What do you think?

William Breathitt Gray

Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

[Guenter Roeck]

On 01/27/2016 04:18 PM, William Breathitt Gray wrote:
> On Tue, Jan 26, 2016 at 09:02:45PM -0800, Guenter Roeck wrote:
>>> Unfortunately, the sensors-detect only reported "No" for each Super I/O
>>> chip test, while the superiotool gave an unhelpful "No Super I/O chip
>>> detected" message.
>>>
>>
>> Too bad. That suggests that the watchdog may in fact be implemented in the fpga.
>
> I received a response from WinSystems: the watchdog timer is implemented
> in the Lattice FPGA (base address 0x298), along with other WinSystems
> firmware. I was offered two methods of identifying the chip.
>
> The first method is to use a 16-bit read of the register at port address
> 0x29E to get the version number of the watchdog timer; my machine
> reported a value of 0x0009. Unfortunately, I don't believe this method
> is very reliable since the version number may not be consistent across
> these motherboards, and the same value could easily happen to be
> returned by an unknown hardware.
>
> The second method is slightly more involved so I'll quote WinSystems:
>
>> 8-bit read of 299h – save this value
>> 8-bit write of 60h to 299h
>> 16-bit read of 29ah should return the base address of the WDT which is 564h
>> 8-bit write of saved value to 299h - don’t want t accidentally change the WDT base address
>
> If the system does return a value of 0x564, then it's pretty safe to say
> that the watchdog timer is implemented on the chip. However, I'm not
> sure it would be safe to send write commands to a port address until the
> hardware has been identified; this second method may not be the best
> route either.
>
> What do you think?
>
Let's stick with DMI.

Thanks,
Guenter

Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

[One Thousand Gnomes]

> > 8-bit read of 299h – save this value
> > 8-bit write of 60h to 299h
> > 16-bit read of 29ah should return the base address of the WDT which is 564h
> > 8-bit write of saved value to 299h - don’t want t accidentally change the WDT base address  
> 
> If the system does return a value of 0x564, then it's pretty safe to say
> that the watchdog timer is implemented on the chip. However, I'm not
> sure it would be safe to send write commands to a port address until the
> hardware has been identified; this second method may not be the best
> route either.
> 
> What do you think?

Pokng at 299/29a could hang other systems, so DMI is definitely
preferable if it's not described in ACPI as it perhaps ought to have
been. If all boards with that DMI string have the device then sorted, if
not then perhaps check 299/29a after the DMI match.

Alan

Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

[One Thousand Gnomes]

> > 8-bit read of 299h – save this value
> > 8-bit write of 60h to 299h
> > 16-bit read of 29ah should return the base address of the WDT which is 564h
> > 8-bit write of saved value to 299h - don’t want t accidentally change the WDT base address  
> 
> If the system does return a value of 0x564, then it's pretty safe to say
> that the watchdog timer is implemented on the chip. However, I'm not
> sure it would be safe to send write commands to a port address until the
> hardware has been identified; this second method may not be the best
> route either.
> 
> What do you think?

Pokng at 299/29a could hang other systems, so DMI is definitely
preferable if it's not described in ACPI as it perhaps ought to have
been. If all boards with that DMI string have the device then sorted, if
not then perhaps check 299/29a after the DMI match.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

[Wim Van Sebroeck]

> > +static int ebc_c384_wdt_set_timeout(struct watchdog_device *wdev, unsigned t)
> > +{
> > +	/* resolution is in minutes for timeouts greater than 255 seconds */
> > +	if (t > 255) {
> > +		/* truncate second resolution to minute resolution */
> > +		t /= 60;
> > +		wdev->timeout = t * 60;
> > +
> > +		/* set watchdog timer for minutes */
> > +		outb(0x00, CFG_ADDR);
> 
> If ask for 299 seconds surely I should get 300 not 240 ?
> (Whether to round off or round up is an interesting question for the
> middle range - does it go off early or late - I'd have said late but...)

This is my preference:
	if (t > 255)
		t = (((t - 1) / 60) + 1) * 60;

Which basically is a round-up to minutes
t = 256 -> gives 300
t = 299 -> gives 300 
t = 300 -> gives 300
t = 301 -> gives 360
t = 15299 -> gives 15300
t = 15300 -> gives 15300
t = 15301 -> gives 15360

So I am also for going late.

Kind regards,
Wim.

Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

[William Breathitt Gray]

On Sun, Feb 28, 2016 at 03:07:39PM +0100, Wim Van Sebroeck wrote:
>> > +static int ebc_c384_wdt_set_timeout(struct watchdog_device *wdev, unsigned t)
>> > +{
>> > +	/* resolution is in minutes for timeouts greater than 255 seconds */
>> > +	if (t > 255) {
>> > +		/* truncate second resolution to minute resolution */
>> > +		t /= 60;
>> > +		wdev->timeout = t * 60;
>> > +
>> > +		/* set watchdog timer for minutes */
>> > +		outb(0x00, CFG_ADDR);
>> 
>> If ask for 299 seconds surely I should get 300 not 240 ?
>> (Whether to round off or round up is an interesting question for the
>> middle range - does it go off early or late - I'd have said late but...)
>
>This is my preference:
>	if (t > 255)
>		t = (((t - 1) / 60) + 1) * 60;
>
>Which basically is a round-up to minutes
>t = 256 -> gives 300
>t = 299 -> gives 300 
>t = 300 -> gives 300
>t = 301 -> gives 360
>t = 15299 -> gives 15300
>t = 15300 -> gives 15300
>t = 15301 -> gives 15360
>
>So I am also for going late.
>
>Kind regards,
>Wim.

I decided to give this another consideration: perhaps we should allow it
to go late rather than early. I figure that when a user configures the
watchdog timer for a certain value in seconds they will have an
expectation that the watchdog timer will wait at least that amount of
time (with the understanding that it may be somewhat late due to the
granularity of the timer).

This behavior follows a lot of other timer standards (e.g. nanosleep)
so it's what the user expects and what we should follow for now. I'll
submit a version 5 patch with this change.

William Breathitt Gray

Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

[Guenter Roeck]

On 02/28/2016 06:07 AM, Wim Van Sebroeck wrote:
>>> +static int ebc_c384_wdt_set_timeout(struct watchdog_device *wdev, unsigned t)
>>> +{
>>> +	/* resolution is in minutes for timeouts greater than 255 seconds */
>>> +	if (t > 255) {
>>> +		/* truncate second resolution to minute resolution */
>>> +		t /= 60;
>>> +		wdev->timeout = t * 60;
>>> +
>>> +		/* set watchdog timer for minutes */
>>> +		outb(0x00, CFG_ADDR);
>>
>> If ask for 299 seconds surely I should get 300 not 240 ?
>> (Whether to round off or round up is an interesting question for the
>> middle range - does it go off early or late - I'd have said late but...)
>
> This is my preference:
> 	if (t > 255)
> 		t = (((t - 1) / 60) + 1) * 60;
>

In case I am missing something: Why not just use DIV_ROUND_UP() ?

Thanks,
Guenter

Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

[Wim Van Sebroeck]

Hi Guenter,

> On 02/28/2016 06:07 AM, Wim Van Sebroeck wrote:
> >>>+static int ebc_c384_wdt_set_timeout(struct watchdog_device *wdev, 
> >>>unsigned t)
> >>>+{
> >>>+	/* resolution is in minutes for timeouts greater than 255 seconds */
> >>>+	if (t > 255) {
> >>>+		/* truncate second resolution to minute resolution */
> >>>+		t /= 60;
> >>>+		wdev->timeout = t * 60;
> >>>+
> >>>+		/* set watchdog timer for minutes */
> >>>+		outb(0x00, CFG_ADDR);
> >>
> >>If ask for 299 seconds surely I should get 300 not 240 ?
> >>(Whether to round off or round up is an interesting question for the
> >>middle range - does it go off early or late - I'd have said late but...)
> >
> >This is my preference:
> >	if (t > 255)
> >		t = (((t - 1) / 60) + 1) * 60;
> >
> 
> In case I am missing something: Why not just use DIV_ROUND_UP() ?

That's indeed also usable (from kernel.h: #define DIV_ROUND_UP(n,d) (((n) + (d) -1) / (d)) which is the same) and indeed preferable.

Kind regards,
Wim.

Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

[Guenter Roeck]

On 01/26/2016 06:31 AM, William Breathitt Gray wrote:
> On 01/25/2016 5:11 PM, Guenter Roeck wrote:
>>> +		dev_warn(dev, "Invalid timeout (%u seconds), using default (%u seconds)\n",
>>> +			timeout, WATCHDOG_TIMEOUT);
>>
>> Multi-line alignment is off by one character.
>
> I used tabs to align the lines to 8-character boundaries. Should I add
> an additional space to align the "timeout" argument with the "dev"
> argument?
>

Continuation line alignment should be with "(", so, yes, on the same column as "dev".

Thanks,
Guenter

Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

[William Breathitt Gray]

On 01/25/2016 5:11 PM, Guenter Roeck wrote:
>> +		dev_warn(dev, "Invalid timeout (%u seconds), using default (%u seconds)\n",
>> +			timeout, WATCHDOG_TIMEOUT);
>
> Multi-line alignment is off by one character.

I used tabs to align the lines to 8-character boundaries. Should I add
an additional space to align the "timeout" argument with the "dev"
argument?

William Breathitt Gray