LinuxLists.cc - [PATCH] watchdog: New watchdog driver for MEN A21 watchdogs

[permalink] [raw]

Subject: Re: [PATCH] watchdog: New watchdog driver for MEN A21 watchdogs

On Mon, May 13, 2013 at 10:58:29PM +0200, [email protected] wrote:
> From: Johannes Thumshirn <[email protected]>
>
> This patch adds the driver for the watchdog devices found on MEN Mikro
> Elektronik A21 VMEbus CPU Carrier Boards. It has DT-support and uses the
> watchdog framework.
>
> Signed-off-by: Johannes Thumshirn <[email protected]>

Besides all of its problems, seems to me that could (or should) be a generic gpio
watchdog driver, possibly with device tree bindings describing how the various
gpio pins are used.

> ---
> MAINTAINERS | 6 +
> drivers/watchdog/Kconfig | 8 +
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/mena21_wdt.c | 351 +++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 366 insertions(+)
> create mode 100644 drivers/watchdog/mena21_wdt.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3d7782b..3e5b3f1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5291,6 +5291,12 @@ F: drivers/mtd/
> F: include/linux/mtd/
> F: include/uapi/mtd/
>
> +MEN A21 WATCHDOG DRIVER
> +M: Johannes Thumshirn <[email protected]>
> +L: [email protected]
> +S: Supported
> +F: drivers/watchdog/mena21_wdt.c
> +
> METAG ARCHITECTURE
> M: James Hogan <[email protected]>
> S: Supported
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index e89fc31..192b84d 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1172,6 +1172,14 @@ config BOOKE_WDT_DEFAULT_TIMEOUT
>
> The value can be overridden by the wdt_period command-line parameter.
>
> +config MEN_A21_WDT
> + tristate "MEN A21 VME CPU Carrier Board Watchdog Timer"
> + select WATCHDOG_CORE
> + help
> + Watchdog driver for MEN A21 VMEbus CPU Carrier Boards.
> +
> + If unsure select N here.
> +
> # PPC64 Architecture
>
> config WATCHDOG_RTAS
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index a300b94..bffdcb1 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -143,6 +143,7 @@ obj-$(CONFIG_8xxx_WDT) += mpc8xxx_wdt.o
> obj-$(CONFIG_MV64X60_WDT) += mv64x60_wdt.o
> obj-$(CONFIG_PIKA_WDT) += pika_wdt.o
> obj-$(CONFIG_BOOKE_WDT) += booke_wdt.o
> +obj-$(CONFIG_MEN_A21_WDT) += mena21_wdt.o
>
> # PPC64 Architecture
> obj-$(CONFIG_WATCHDOG_RTAS) += wdrtas.o
> diff --git a/drivers/watchdog/mena21_wdt.c b/drivers/watchdog/mena21_wdt.c
> new file mode 100644
> index 0000000..d534f81
> --- /dev/null
> +++ b/drivers/watchdog/mena21_wdt.c
> @@ -0,0 +1,351 @@
> +/*
> + * Watchdog driver for the A21 VME CPU Boards
> + *
> + * Copyright (C) 2013 MEN Mikro Elektronik Nuernberg GmbH
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation
> + */
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/types.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +#include <linux/uaccess.h>
> +#include <linux/gpio.h>
> +#include <linux/delay.h>
> +#include <linux/reboot.h>
> +#include <linux/bitops.h>
> +
> +static char *reset_causes[] = {
> + "Power On Reset",
> + "CPU Reset Request",
> + "Push Button",
> + "FPGA Reset Request",
> + "Watchdog",
> + "Local Power Bad",
> + "Invalid",
> + "BDI",
> +};
> +
> +#define GPIO_WD_ENAB 169
> +#define GPIO_WD_FAST 170
> +#define GPIO_WD_TRIG 171
> +
> +#define GPIO_RST_CAUSE_BASE 166
> +
> +struct a21_wdt_drv {
> + struct watchdog_device wdt;
> + struct mutex lock;
> + bool open;
> +};
> +
> +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 ssize_t rebootcause_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + unsigned int reset = 0;
> + int i;
> +
> + for (i = 0; i < 3; i++)
> + reset |= !!gpio_get_value(GPIO_RST_CAUSE_BASE + i);

The result of !! is 0 or 1, so reset will never be larger than 1.

Not sure if using fixed numbers for gpio pins is a good idea or even
correct. You might want to pass that by the gpio maintainers.

> +
> + if (reset >= 8)
> + return -EIO;
> +
Meaning this will never happen.

> + return sprintf(buf, "%s\n", reset_causes[reset]);
> +}
> +static DEVICE_ATTR(rebootcause, S_IRUGO, rebootcause_show, NULL);
> +
> +static ssize_t active_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + return sprintf(buf, "%d\n", !!gpio_get_value(GPIO_WD_ENAB));
> +}
> +static DEVICE_ATTR(active, S_IRUGO, active_show, NULL);
> +
> +static ssize_t allow_disable_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + return sprintf(buf, "%d\n", !nowayout);
> +}
> +static DEVICE_ATTR(allow_disable, S_IRUGO, allow_disable_show, NULL);
> +
> +static ssize_t fastmode_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + return sprintf(buf, "%d\n", !!gpio_get_value(GPIO_WD_FAST));
> +}
> +static DEVICE_ATTR(fastmode, S_IRUGO, fastmode_show, NULL);
> +
> +static int a21_wdt_create_files(struct device *dev)
> +{
> + int ret;
> +
> + ret = device_create_file(dev, &dev_attr_rebootcause);
> + if (ret)
> + return ret;
> + ret = device_create_file(dev, &dev_attr_active);
> + if (ret)
> + return ret;
> + ret = device_create_file(dev, &dev_attr_allow_disable);
> + if (ret)
> + return ret;
> + ret = device_create_file(dev, &dev_attr_fastmode);

Wonder if the maintainer will let you bypass the infrastructure with those
attributes. I wouldn't.

> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static void a21_wdt_remove_files(struct device *dev)
> +{
> + device_remove_file(dev, &dev_attr_rebootcause);
> + device_remove_file(dev, &dev_attr_active);
> + device_remove_file(dev, &dev_attr_allow_disable);
> + device_remove_file(dev, &dev_attr_fastmode);
> +}
> +
> +static int a21_wdt_start(struct watchdog_device *wdt)
> +{
> + struct a21_wdt_drv *drv = watchdog_get_drvdata(wdt);
> +
> + mutex_lock(&drv->lock);
> +
> + drv->open = true;

What is this flag for ?

> + gpio_set_value(GPIO_WD_ENAB, 1);
> +
> + mutex_unlock(&drv->lock);
> +
> + return 0;
> +}
> +
> +static int a21_wdt_stop(struct watchdog_device *wdt)
> +{
> + struct a21_wdt_drv *drv = watchdog_get_drvdata(wdt);
> +
> + /* We don't stop if WDOG_NO_WAY_OUT is set */
> + if (test_bit(WDOG_NO_WAY_OUT, &wdt->status))
> + return -EINVAL;
> +
> + mutex_lock(&drv->lock);
> +
> + gpio_set_value(GPIO_WD_ENAB, 0);
> + drv->open = false;
> +
> + mutex_unlock(&drv->lock);
> +
> + return 0;
> +}
> +
> +static int a21_wdt_ping(struct watchdog_device *wdt)
> +{
> + struct a21_wdt_drv *drv = watchdog_get_drvdata(wdt);
> +
> + mutex_lock(&drv->lock);
> +
> + gpio_set_value(GPIO_WD_TRIG, 0);
> + msleep(400);

Pretty long sleep. You really want to put applications to sleep for that long
when pinging the watchdog ? I'd be more than a bit concerned that this might be
problematic for applications expecting to do something else besides sleeping in
a watchdog ping call.

> + gpio_set_value(GPIO_WD_TRIG, 1);
> +
> + mutex_unlock(&drv->lock);
> +
> + return 0;
> +}
> +
> +static int a21_wdt_set_timeout(struct watchdog_device *wdt,
> + unsigned int timeout)
> +{
> + struct a21_wdt_drv *drv = watchdog_get_drvdata(wdt);
> +
> + if (timeout != 1 || timeout != 30) {

Hmmm ... did you ever try to set the timeout ?

> + pr_err("Only 1 and 30 allowed as timeout\n");

dev_err is preferred for messages, but I wonder what value you expect
from this message (and other similar messages) in the error log.

> + return -EINVAL;
> + }
> +
> + mutex_lock(&drv->lock);
> +
> + if (timeout == 30)
> + gpio_set_value(GPIO_WD_FAST, 1);
> + else
> + gpio_set_value(GPIO_WD_FAST, 0);
> +
> + wdt->timeout = timeout;
> +
> + mutex_unlock(&drv->lock);
> +
> + return 0;
> +}
> +
> +static long a21_wdt_ioctl(struct watchdog_device *wdt, unsigned int cmd,
> + unsigned long arg)
> +{
> + unsigned int timeout;
> + int ret;
> + int __user *argp = (void __user *) arg;
> +
> + switch (cmd) {
> + case WDIOC_GETSUPPORT:
> + return copy_to_user(argp,
> + wdt->info, sizeof(struct watchdog_info));
> + case WDIOC_KEEPALIVE:
> + return a21_wdt_ping(wdt);
> + case WDIOC_SETTIMEOUT:
> + ret = get_user(timeout, argp);
> + if (ret)
> + return ret;
> + a21_wdt_set_timeout(wdt, timeout);
> + /* Fallthrough */
> + case WDIOC_GETTIMEOUT:
> + return put_user(wdt->timeout, argp);
> + case WDIOC_SETOPTIONS: {
> + int options;
> +
> + ret = get_user(options, argp);
> + if (ret)
> + return ret;
> + if (options & WDIOS_DISABLECARD)
> + a21_wdt_stop(wdt);
> + if (options & WDIOS_ENABLECARD)
> + a21_wdt_start(wdt);
> + break;
> + }
> + default:
> + pr_err("IOCTL 0x%x not understood\n", cmd);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
What does this function offer that is not in the infrastructure ?

> +
> +static int a21_wdt_notify_sys(struct notifier_block *notify, unsigned long code,
> + void *unused)
> +{
> + if (code == SYS_DOWN || code == SYS_HALT)
> + gpio_set_value(GPIO_WD_ENAB, 0);
> +
> + return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block a21_wdt_notifier = {
> + .notifier_call = a21_wdt_notify_sys,
> +};
> +
> +static const struct watchdog_info a21_wdt_info = {
> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> + .identity = "MEN A21 Watchdog",
> +};
> +
> +static const struct watchdog_ops a21_wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = a21_wdt_start,
> + .stop = a21_wdt_stop,
> + .ping = a21_wdt_ping,
> + .set_timeout = a21_wdt_set_timeout,
> + .ioctl = a21_wdt_ioctl,
> +};
> +
> +static struct watchdog_device a21_wdt = {
> + .info = &a21_wdt_info,
> + .ops = &a21_wdt_ops,
> + .min_timeout = 1,
> + .max_timeout = 30,
> +};
> +
> +static int a21_wdt_probe(struct platform_device *pdev)
> +{
> + struct a21_wdt_drv *drv;
> + int ret;
> +
> + pr_info("MEN A21 watchdog timer driver enabled\n");
> +
pr_ -> dev_

> + drv = devm_kzalloc(&pdev->dev, sizeof(struct a21_wdt_drv), GFP_KERNEL);
> + if (!drv) {
> + dev_err(&pdev->dev,
> + "Unable to allocate memory for watchdog device\n");
> + return -ENOMEM;
> + }
> +
> + mutex_init(&drv->lock);
> + watchdog_set_nowayout(&a21_wdt, nowayout);
> + watchdog_set_drvdata(&a21_wdt, drv);
> +
> + ret = watchdog_register_device(&a21_wdt);
> + if (ret) {
> + pr_err("Cannot register watchdog device\n");
> + goto err_register_wd;
> + }
> +
> + ret = register_reboot_notifier(&a21_wdt_notifier);
> + if (ret) {
> + pr_err("Cannot register reboot notifier\n");
> + goto err_register_notif;
> + }
> +
> + ret = a21_wdt_create_files(&pdev->dev);
> + if (ret) {
> + dev_err(&pdev->dev, "Cannot create sysfs entries\n");
> + goto err_create_sysfs;
> + }
> +
> + dev_set_drvdata(&pdev->dev, drv);
> +
> + return 0;
> +
> +err_create_sysfs:
> + unregister_reboot_notifier(&a21_wdt_notifier);
> +err_register_notif:
> + watchdog_unregister_device(&drv->wdt);
> +err_register_wd:
> + mutex_destroy(&drv->lock);
> + devm_kfree(&pdev->dev, drv);

You might want to look into the reasons for using devm_ functions.

> +
> + return ret;
> +}
> +
> +static int a21_wdt_remove(struct platform_device *pdev)
> +{
> + struct a21_wdt_drv *drv = dev_get_drvdata(&pdev->dev);
> +
> + pr_warn("Unregistering A21 watchdog driver, board may reboot\n");
> +
> + a21_wdt_remove_files(&pdev->dev);
> +
> + watchdog_unregister_device(&drv->wdt);
> + unregister_reboot_notifier(&a21_wdt_notifier);
> +
> + mutex_destroy(&drv->lock);
> + devm_kfree(&pdev->dev, drv);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id a21_wdt_ids[] = {
> + { .compatible = "men,a021-wdt" },
> + { },
> +};
> +
> +static struct platform_driver a21_wdt_driver = {
> + .probe = a21_wdt_probe,
> + .remove = a21_wdt_remove,
> + .driver = {
> + .name = "a21-watchdog",
> + .of_match_table = a21_wdt_ids,
> + },
> +};
> +
> +module_platform_driver(a21_wdt_driver);
> +
> +MODULE_AUTHOR("MEN Mikro Elektronik");
> +MODULE_DESCRIPTION("MEN A21 Watchdog");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:a21-watchdog");
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

2013-05-14 19:07:44

[permalink] [raw]

Subject: [PATCH v2] watchdog: New watchdog driver for MEN A21 watchdogs

From: Johannes Thumshirn <[email protected]>

This patch adds the driver for the watchdog devices found on MEN Mikro
Elektronik A21 VMEbus CPU Carrier Boards. It has DT-support and uses the
watchdog framework.

Revision 2:
* Removed unneeded open flag in struct a21_wdt_drv
* Corrected 3bit reason code from gpio
* Additional sysfs files are now part of watchdog sysfs
* Changed OFF/ON delay in ping from 400ms to 10ns
* Reworked timeout setting
* Removed a21_wdt_ioctl(...)

Signed-off-by: Johannes Thumshirn <[email protected]>
---
MAINTAINERS | 6 +
drivers/watchdog/Kconfig | 8 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/mena21_wdt.c | 305 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 320 insertions(+)
create mode 100644 drivers/watchdog/mena21_wdt.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 3d7782b..3e5b3f1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5291,6 +5291,12 @@ F: drivers/mtd/
F: include/linux/mtd/
F: include/uapi/mtd/

+MEN A21 WATCHDOG DRIVER
+M: Johannes Thumshirn <[email protected]>
+L: [email protected]
+S: Supported
+F: drivers/watchdog/mena21_wdt.c
+
METAG ARCHITECTURE
M: James Hogan <[email protected]>
S: Supported
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index e89fc31..192b84d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1172,6 +1172,14 @@ config BOOKE_WDT_DEFAULT_TIMEOUT

The value can be overridden by the wdt_period command-line parameter.

+config MEN_A21_WDT
+ tristate "MEN A21 VME CPU Carrier Board Watchdog Timer"
+ select WATCHDOG_CORE
+ help
+ Watchdog driver for MEN A21 VMEbus CPU Carrier Boards.
+
+ If unsure select N here.
+
# PPC64 Architecture

config WATCHDOG_RTAS
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index a300b94..bffdcb1 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -143,6 +143,7 @@ obj-$(CONFIG_8xxx_WDT) += mpc8xxx_wdt.o
obj-$(CONFIG_MV64X60_WDT) += mv64x60_wdt.o
obj-$(CONFIG_PIKA_WDT) += pika_wdt.o
obj-$(CONFIG_BOOKE_WDT) += booke_wdt.o
+obj-$(CONFIG_MEN_A21_WDT) += mena21_wdt.o

# PPC64 Architecture
obj-$(CONFIG_WATCHDOG_RTAS) += wdrtas.o
diff --git a/drivers/watchdog/mena21_wdt.c b/drivers/watchdog/mena21_wdt.c
new file mode 100644
index 0000000..9c5bf32
--- /dev/null
+++ b/drivers/watchdog/mena21_wdt.c
@@ -0,0 +1,305 @@
+/*
+ * Watchdog driver for the A21 VME CPU Boards
+ *
+ * Copyright (C) 2013 MEN Mikro Elektronik Nuernberg GmbH
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation
+ */
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+#include <linux/uaccess.h>
+#include <linux/gpio.h>
+#include <linux/delay.h>
+#include <linux/reboot.h>
+#include <linux/bitops.h>
+
+static char *reset_causes[] = {
+ "Power On Reset",
+ "CPU Reset Request",
+ "Push Button",
+ "FPGA Reset Request",
+ "Watchdog",
+ "Local Power Bad",
+ "Invalid",
+ "BDI",
+};
+
+#define GPIO_WD_ENAB 169
+#define GPIO_WD_FAST 170
+#define GPIO_WD_TRIG 171
+
+#define GPIO_RST_CAUSE_BASE 166
+
+struct a21_wdt_drv {
+ struct watchdog_device wdt;
+ struct mutex lock;
+};
+
+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 ssize_t rebootcause_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ unsigned int reset = 0;
+ int i;
+
+ for (i = 0; i < 3; i++)
+ reset |= gpio_get_value(GPIO_RST_CAUSE_BASE + i)
+ ? (1 << i) : 0;
+
+ if (reset >= 8)
+ return -EIO;
+
+ return sprintf(buf, "%s\n", reset_causes[reset]);
+}
+static DEVICE_ATTR(rebootcause, S_IRUGO, rebootcause_show, NULL);
+
+static ssize_t active_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ return sprintf(buf, "%d\n", !!gpio_get_value(GPIO_WD_ENAB));
+}
+static DEVICE_ATTR(active, S_IRUGO, active_show, NULL);
+
+static ssize_t allow_disable_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ return sprintf(buf, "%d\n", !nowayout);
+}
+static DEVICE_ATTR(allow_disable, S_IRUGO, allow_disable_show, NULL);
+
+static ssize_t fastmode_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ return sprintf(buf, "%d\n", !!gpio_get_value(GPIO_WD_FAST));
+}
+static DEVICE_ATTR(fastmode, S_IRUGO, fastmode_show, NULL);
+
+static int a21_wdt_create_files(struct watchdog_device *wdev)
+{
+ int ret;
+
+ ret = device_create_file(wdev->dev, &dev_attr_rebootcause);
+ if (ret)
+ return ret;
+ ret = device_create_file(wdev->dev, &dev_attr_active);
+ if (ret)
+ return ret;
+ ret = device_create_file(wdev->dev, &dev_attr_allow_disable);
+ if (ret)
+ return ret;
+ ret = device_create_file(wdev->dev, &dev_attr_fastmode);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static void a21_wdt_remove_files(struct watchdog_device *wdev)
+{
+ device_remove_file(wdev->dev, &dev_attr_rebootcause);
+ device_remove_file(wdev->dev, &dev_attr_active);
+ device_remove_file(wdev->dev, &dev_attr_allow_disable);
+ device_remove_file(wdev->dev, &dev_attr_fastmode);
+}
+
+static int a21_wdt_start(struct watchdog_device *wdt)
+{
+ struct a21_wdt_drv *drv = watchdog_get_drvdata(wdt);
+
+ mutex_lock(&drv->lock);
+
+ gpio_set_value(GPIO_WD_ENAB, 1);
+
+ mutex_unlock(&drv->lock);
+
+ return 0;
+}
+
+static int a21_wdt_stop(struct watchdog_device *wdt)
+{
+ struct a21_wdt_drv *drv = watchdog_get_drvdata(wdt);
+
+ /* We don't stop if WDOG_NO_WAY_OUT is set */
+ if (test_bit(WDOG_NO_WAY_OUT, &wdt->status))
+ return -EINVAL;
+
+ mutex_lock(&drv->lock);
+
+ gpio_set_value(GPIO_WD_ENAB, 0);
+
+ mutex_unlock(&drv->lock);
+
+ return 0;
+}
+
+static int a21_wdt_ping(struct watchdog_device *wdt)
+{
+ struct a21_wdt_drv *drv = watchdog_get_drvdata(wdt);
+
+ mutex_lock(&drv->lock);
+
+ gpio_set_value(GPIO_WD_TRIG, 0);
+ ndelay(10);
+ gpio_set_value(GPIO_WD_TRIG, 1);
+
+ mutex_unlock(&drv->lock);
+
+ return 0;
+}
+
+static int a21_wdt_set_timeout(struct watchdog_device *wdt,
+ unsigned int timeout)
+{
+ struct a21_wdt_drv *drv = watchdog_get_drvdata(wdt);
+
+ if (timeout != 1 && timeout != 30) {
+ dev_err(wdt->dev, "Only 1 and 30 allowed as timeout\n");
+ return -EINVAL;
+ }
+
+ mutex_lock(&drv->lock);
+
+ if (timeout == 30)
+ gpio_set_value(GPIO_WD_FAST, 1);
+ else
+ gpio_set_value(GPIO_WD_FAST, 0);
+
+ wdt->timeout = timeout;
+
+ mutex_unlock(&drv->lock);
+
+ return 0;
+}
+
+static int a21_wdt_notify_sys(struct notifier_block *notify, unsigned long code,
+ void *unused)
+{
+ if (code == SYS_DOWN || code == SYS_HALT)
+ gpio_set_value(GPIO_WD_ENAB, 0);
+
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block a21_wdt_notifier = {
+ .notifier_call = a21_wdt_notify_sys,
+};
+
+static const struct watchdog_info a21_wdt_info = {
+ .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+ .identity = "MEN A21 Watchdog",
+};
+
+static const struct watchdog_ops a21_wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = a21_wdt_start,
+ .stop = a21_wdt_stop,
+ .ping = a21_wdt_ping,
+ .set_timeout = a21_wdt_set_timeout,
+};
+
+static struct watchdog_device a21_wdt = {
+ .info = &a21_wdt_info,
+ .ops = &a21_wdt_ops,
+ .min_timeout = 1,
+ .max_timeout = 30,
+};
+
+static int a21_wdt_probe(struct platform_device *pdev)
+{
+ struct a21_wdt_drv *drv;
+ int ret;
+
+ dev_info(&pdev->dev, "MEN A21 watchdog timer driver enabled\n");
+
+ drv = devm_kzalloc(&pdev->dev, sizeof(struct a21_wdt_drv), GFP_KERNEL);
+ if (!drv) {
+ dev_err(&pdev->dev,
+ "Unable to allocate memory for watchdog device\n");
+ return -ENOMEM;
+ }
+
+ mutex_init(&drv->lock);
+ watchdog_set_nowayout(&a21_wdt, nowayout);
+ watchdog_set_drvdata(&a21_wdt, drv);
+
+ ret = watchdog_register_device(&a21_wdt);
+ if (ret) {
+ pr_err("Cannot register watchdog device\n");
+ goto err_register_wd;
+ }
+
+ ret = register_reboot_notifier(&a21_wdt_notifier);
+ if (ret) {
+ pr_err("Cannot register reboot notifier\n");
+ goto err_register_notif;
+ }
+
+ ret = a21_wdt_create_files(&a21_wdt);
+ if (ret) {
+ dev_err(&pdev->dev, "Cannot create sysfs entries\n");
+ goto err_create_sysfs;
+ }
+
+ dev_set_drvdata(&pdev->dev, drv);
+
+ return 0;
+
+err_create_sysfs:
+ unregister_reboot_notifier(&a21_wdt_notifier);
+err_register_notif:
+ watchdog_unregister_device(&drv->wdt);
+err_register_wd:
+ mutex_destroy(&drv->lock);
+
+ return ret;
+}
+
+static int a21_wdt_remove(struct platform_device *pdev)
+{
+ struct a21_wdt_drv *drv = dev_get_drvdata(&pdev->dev);
+
+ pr_warn("Unregistering A21 watchdog driver, board may reboot\n");
+
+ a21_wdt_remove_files(&drv->wdt);
+
+ watchdog_unregister_device(&drv->wdt);
+ unregister_reboot_notifier(&a21_wdt_notifier);
+
+ mutex_destroy(&drv->lock);
+
+ return 0;
+}
+
+static const struct of_device_id a21_wdt_ids[] = {
+ { .compatible = "men,a021-wdt" },
+ { },
+};
+
+static struct platform_driver a21_wdt_driver = {
+ .probe = a21_wdt_probe,
+ .remove = a21_wdt_remove,
+ .driver = {
+ .name = "a21-watchdog",
+ .of_match_table = a21_wdt_ids,
+ },
+};
+
+module_platform_driver(a21_wdt_driver);
+
+MODULE_AUTHOR("MEN Mikro Elektronik");
+MODULE_DESCRIPTION("MEN A21 Watchdog");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:a21-watchdog");
--
1.7.9.5

2013-05-14 19:33:57

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH v2] watchdog: New watchdog driver for MEN A21 watchdogs

On Tue, 2013-05-14 at 21:09 +0200, [email protected] wrote:
> From: Johannes Thumshirn <[email protected]>
>
> This patch adds the driver for the watchdog devices found on MEN Mikro
> Elektronik A21 VMEbus CPU Carrier Boards. It has DT-support and uses the
> watchdog framework.

trivia:

> diff --git a/drivers/watchdog/mena21_wdt.c b/drivers/watchdog/mena21_wdt.c

add:

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

before any #include to prefix pr_<level> messages.

> +#include <linux/module.h>
[]
> +static int a21_wdt_probe(struct platform_device *pdev)
> +{
> + struct a21_wdt_drv *drv;
> + int ret;
> +
> + dev_info(&pdev->dev, "MEN A21 watchdog timer driver enabled\n");
> +
> + drv = devm_kzalloc(&pdev->dev, sizeof(struct a21_wdt_drv), GFP_KERNEL);
> + if (!drv) {
> + dev_err(&pdev->dev,
> + "Unable to allocate memory for watchdog device\n");

OOM messages aren't particularly useful as all
failed memory allocations (without __GFP_NOWARN)
get a generic dump_stack().

> + ret = watchdog_register_device(&a21_wdt);
> + if (ret) {
> + pr_err("Cannot register watchdog device\n");
> + goto err_register_wd;
> + }
> +
> + ret = register_reboot_notifier(&a21_wdt_notifier);
> + if (ret) {
> + pr_err("Cannot register reboot notifier\n");
> + goto err_register_notif;
> + }
> +
> + ret = a21_wdt_create_files(&a21_wdt);
> + if (ret) {
> + dev_err(&pdev->dev, "Cannot create sysfs entries\n");
> + goto err_create_sysfs;
> + }

Mixing pr_err and dev_err may not be that useful.

Why not all dev_err? Then there'd be no pr_<level>
uses at all and you wouldn't need the pr_fmt either.

2013-05-15 18:47:35

[permalink] [raw]

Subject: [PATCH v3] watchdog: New watchdog driver for MEN A21 watchdogs

From: Johannes Thumshirn <[email protected]>

This patch adds the driver for the watchdog devices found on MEN Mikro
Elektronik A21 VMEbus CPU Carrier Boards. It has DT-support and uses the
watchdog framework.

Revision 2:
* Removed unneeded open flag in struct a21_wdt_drv
* Corrected 3bit reason code from gpio
* Additional sysfs files are now part of watchdog sysfs
* Changed OFF/ON delay in ping from 400ms to 10ns
* Reworked timeout setting
* Removed a21_wdt_ioctl(...)

Revision 3:
* Changed pr_{err,info} to dev_{err,info}
* Removed out of memory error print
* Transition from "fast" to "slow" mode not allowed by chip

Signed-off-by: Johannes Thumshirn <[email protected]>
---
MAINTAINERS | 6 +
drivers/watchdog/Kconfig | 8 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/mena21_wdt.c | 309 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 324 insertions(+)
create mode 100644 drivers/watchdog/mena21_wdt.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 3d7782b..3e5b3f1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5291,6 +5291,12 @@ F: drivers/mtd/
F: include/linux/mtd/
F: include/uapi/mtd/

+MEN A21 WATCHDOG DRIVER
+M: Johannes Thumshirn <[email protected]>
+L: [email protected]
+S: Supported
+F: drivers/watchdog/mena21_wdt.c
+
METAG ARCHITECTURE
M: James Hogan <[email protected]>
S: Supported
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index e89fc31..192b84d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1172,6 +1172,14 @@ config BOOKE_WDT_DEFAULT_TIMEOUT

The value can be overridden by the wdt_period command-line parameter.

+config MEN_A21_WDT
+ tristate "MEN A21 VME CPU Carrier Board Watchdog Timer"
+ select WATCHDOG_CORE
+ help
+ Watchdog driver for MEN A21 VMEbus CPU Carrier Boards.
+
+ If unsure select N here.
+
# PPC64 Architecture

config WATCHDOG_RTAS
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index a300b94..bffdcb1 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -143,6 +143,7 @@ obj-$(CONFIG_8xxx_WDT) += mpc8xxx_wdt.o
obj-$(CONFIG_MV64X60_WDT) += mv64x60_wdt.o
obj-$(CONFIG_PIKA_WDT) += pika_wdt.o
obj-$(CONFIG_BOOKE_WDT) += booke_wdt.o
+obj-$(CONFIG_MEN_A21_WDT) += mena21_wdt.o

# PPC64 Architecture
obj-$(CONFIG_WATCHDOG_RTAS) += wdrtas.o
diff --git a/drivers/watchdog/mena21_wdt.c b/drivers/watchdog/mena21_wdt.c
new file mode 100644
index 0000000..389bd59
--- /dev/null
+++ b/drivers/watchdog/mena21_wdt.c
@@ -0,0 +1,309 @@
+/*
+ * Watchdog driver for the A21 VME CPU Boards
+ *
+ * Copyright (C) 2013 MEN Mikro Elektronik Nuernberg GmbH
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation
+ */
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+#include <linux/uaccess.h>
+#include <linux/gpio.h>
+#include <linux/delay.h>
+#include <linux/reboot.h>
+#include <linux/bitops.h>
+
+static char *reset_causes[] = {
+ "Power On Reset",
+ "CPU Reset Request",
+ "Push Button",
+ "FPGA Reset Request",
+ "Watchdog",
+ "Local Power Bad",
+ "Invalid",
+ "BDI",
+};
+
+#define GPIO_WD_ENAB 169
+#define GPIO_WD_FAST 170
+#define GPIO_WD_TRIG 171
+
+#define GPIO_RST_CAUSE_BASE 166
+
+struct a21_wdt_drv {
+ struct watchdog_device wdt;
+ struct mutex lock;
+};
+
+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 ssize_t rebootcause_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ unsigned int reset = 0;
+ int i;
+
+ for (i = 0; i < 3; i++)
+ reset |= gpio_get_value(GPIO_RST_CAUSE_BASE + i)
+ ? (1 << i) : 0;
+
+ if (reset >= 8)
+ return -EIO;
+
+ return sprintf(buf, "%s\n", reset_causes[reset]);
+}
+static DEVICE_ATTR(rebootcause, S_IRUGO, rebootcause_show, NULL);
+
+static ssize_t active_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ return sprintf(buf, "%d\n", !!gpio_get_value(GPIO_WD_ENAB));
+}
+static DEVICE_ATTR(active, S_IRUGO, active_show, NULL);
+
+static ssize_t allow_disable_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ return sprintf(buf, "%d\n", !nowayout);
+}
+static DEVICE_ATTR(allow_disable, S_IRUGO, allow_disable_show, NULL);
+
+static ssize_t fastmode_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ return sprintf(buf, "%d\n", !!gpio_get_value(GPIO_WD_FAST));
+}
+static DEVICE_ATTR(fastmode, S_IRUGO, fastmode_show, NULL);
+
+static int a21_wdt_create_files(struct watchdog_device *wdev)
+{
+ int ret;
+
+ ret = device_create_file(wdev->dev, &dev_attr_rebootcause);
+ if (ret)
+ return ret;
+ ret = device_create_file(wdev->dev, &dev_attr_active);
+ if (ret)
+ return ret;
+ ret = device_create_file(wdev->dev, &dev_attr_allow_disable);
+ if (ret)
+ return ret;
+ ret = device_create_file(wdev->dev, &dev_attr_fastmode);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static void a21_wdt_remove_files(struct watchdog_device *wdev)
+{
+ device_remove_file(wdev->dev, &dev_attr_rebootcause);
+ device_remove_file(wdev->dev, &dev_attr_active);
+ device_remove_file(wdev->dev, &dev_attr_allow_disable);
+ device_remove_file(wdev->dev, &dev_attr_fastmode);
+}
+
+static int a21_wdt_start(struct watchdog_device *wdt)
+{
+ struct a21_wdt_drv *drv = watchdog_get_drvdata(wdt);
+
+ mutex_lock(&drv->lock);
+
+ gpio_set_value(GPIO_WD_ENAB, 1);
+
+ mutex_unlock(&drv->lock);
+
+ return 0;
+}
+
+static int a21_wdt_stop(struct watchdog_device *wdt)
+{
+ struct a21_wdt_drv *drv = watchdog_get_drvdata(wdt);
+
+ /* We don't stop if WDOG_NO_WAY_OUT is set */
+ if (test_bit(WDOG_NO_WAY_OUT, &wdt->status))
+ return -EINVAL;
+
+ mutex_lock(&drv->lock);
+
+ gpio_set_value(GPIO_WD_ENAB, 0);
+
+ mutex_unlock(&drv->lock);
+
+ return 0;
+}
+
+static int a21_wdt_ping(struct watchdog_device *wdt)
+{
+ struct a21_wdt_drv *drv = watchdog_get_drvdata(wdt);
+
+ mutex_lock(&drv->lock);
+
+ gpio_set_value(GPIO_WD_TRIG, 0);
+ ndelay(10);
+ gpio_set_value(GPIO_WD_TRIG, 1);
+
+ mutex_unlock(&drv->lock);
+
+ return 0;
+}
+
+static int a21_wdt_set_timeout(struct watchdog_device *wdt,
+ unsigned int timeout)
+{
+ struct a21_wdt_drv *drv = watchdog_get_drvdata(wdt);
+
+ if (timeout != 1 && timeout != 30) {
+ dev_err(wdt->dev, "Only 1 and 30 allowed as timeout\n");
+ return -EINVAL;
+ }
+
+ if (timeout == 30 && wdt->timeout == 1) {
+ dev_err(wdt->dev,
+ "Transition from fast to slow mode not allowed\n");
+ return -EINVAL;
+ }
+
+ mutex_lock(&drv->lock);
+
+ if (timeout == 1)
+ gpio_set_value(GPIO_WD_FAST, 1);
+ else
+ gpio_set_value(GPIO_WD_FAST, 0);
+
+ wdt->timeout = timeout;
+
+ mutex_unlock(&drv->lock);
+
+ return 0;
+}
+
+static int a21_wdt_notify_sys(struct notifier_block *notify, unsigned long code,
+ void *unused)
+{
+ if (code == SYS_DOWN || code == SYS_HALT)
+ gpio_set_value(GPIO_WD_ENAB, 0);
+
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block a21_wdt_notifier = {
+ .notifier_call = a21_wdt_notify_sys,
+};
+
+static const struct watchdog_info a21_wdt_info = {
+ .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+ .identity = "MEN A21 Watchdog",
+};
+
+static const struct watchdog_ops a21_wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = a21_wdt_start,
+ .stop = a21_wdt_stop,
+ .ping = a21_wdt_ping,
+ .set_timeout = a21_wdt_set_timeout,
+};
+
+static struct watchdog_device a21_wdt = {
+ .info = &a21_wdt_info,
+ .ops = &a21_wdt_ops,
+ .min_timeout = 1,
+ .max_timeout = 30,
+};
+
+static int a21_wdt_probe(struct platform_device *pdev)
+{
+ struct a21_wdt_drv *drv;
+ int ret;
+
+ dev_info(&pdev->dev, "MEN A21 watchdog timer driver enabled\n");
+
+ drv = devm_kzalloc(&pdev->dev, sizeof(struct a21_wdt_drv), GFP_KERNEL);
+ if (!drv)
+ return -ENOMEM;
+
+ mutex_init(&drv->lock);
+ watchdog_set_nowayout(&a21_wdt, nowayout);
+ watchdog_set_drvdata(&a21_wdt, drv);
+
+ ret = watchdog_register_device(&a21_wdt);
+ if (ret) {
+ dev_err(&pdev->dev, "Cannot register watchdog device\n");
+ goto err_register_wd;
+ }
+
+ ret = register_reboot_notifier(&a21_wdt_notifier);
+ if (ret) {
+ dev_err(&pdev->dev, "Cannot register reboot notifier\n");
+ goto err_register_notif;
+ }
+
+ ret = a21_wdt_create_files(&a21_wdt);
+ if (ret) {
+ dev_err(&pdev->dev, "Cannot create sysfs entries\n");
+ goto err_create_sysfs;
+ }
+
+ dev_set_drvdata(&pdev->dev, drv);
+
+ return 0;
+
+err_create_sysfs:
+ unregister_reboot_notifier(&a21_wdt_notifier);
+err_register_notif:
+ watchdog_unregister_device(&drv->wdt);
+err_register_wd:
+ mutex_destroy(&drv->lock);
+
+ return ret;
+}
+
+static int a21_wdt_remove(struct platform_device *pdev)
+{
+ struct a21_wdt_drv *drv = dev_get_drvdata(&pdev->dev);
+
+ dev_warn(&pdev->dev,
+ "Unregistering A21 watchdog driver, board may reboot\n");
+
+ a21_wdt_remove_files(&drv->wdt);
+
+ watchdog_unregister_device(&drv->wdt);
+ unregister_reboot_notifier(&a21_wdt_notifier);
+
+ mutex_destroy(&drv->lock);
+
+ return 0;
+}
+
+static const struct of_device_id a21_wdt_ids[] = {
+ { .compatible = "men,a021-wdt" },
+ { },
+};
+
+static struct platform_driver a21_wdt_driver = {
+ .probe = a21_wdt_probe,
+ .remove = a21_wdt_remove,
+ .driver = {
+ .name = "a21-watchdog",
+ .of_match_table = a21_wdt_ids,
+ },
+};
+
+module_platform_driver(a21_wdt_driver);
+
+MODULE_AUTHOR("MEN Mikro Elektronik");
+MODULE_DESCRIPTION("MEN A21 Watchdog");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:a21-watchdog");
--
1.7.9.5

2013-05-26 14:34:07

[permalink] [raw]

Subject: Re: [PATCH v3] watchdog: New watchdog driver for MEN A21 watchdogs

Hi Johannes,

> +static int a21_wdt_notify_sys(struct notifier_block *notify, unsigned long code,
> + void *unused)
> +{
> + if (code == SYS_DOWN || code == SYS_HALT)
> + gpio_set_value(GPIO_WD_ENAB, 0);
> +
> + return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block a21_wdt_notifier = {
> + .notifier_call = a21_wdt_notify_sys,
> +};
...
> + ret = register_reboot_notifier(&a21_wdt_notifier);
> + if (ret) {
> + dev_err(&pdev->dev, "Cannot register reboot notifier\n");
> + goto err_register_notif;
> + unregister_reboot_notifier(&a21_wdt_notifier);
...
> + unregister_reboot_notifier(&a21_wdt_notifier);
...
> +static struct platform_driver a21_wdt_driver = {
> + .probe = a21_wdt_probe,
> + .remove = a21_wdt_remove,
> + .driver = {
> + .name = "a21-watchdog",
> + .of_match_table = a21_wdt_ids,
> + },
> +};

I prefer to have the reboot_notifier is being replaced by a platform .shutdown function.
Can you also split the sysfs stuff as a second patch so that I can review that seperately?

Kind regards,
Wim.

2013-05-27 06:45:46

[permalink] [raw]

Subject: AW: [PATCH v3] watchdog: New watchdog driver for MEN A21 watchdogs

Hi Johannes,

> +static int a21_wdt_notify_sys(struct notifier_block *notify, unsigned long code,
> + void *unused)
> +{
> + if (code == SYS_DOWN || code == SYS_HALT)
> + gpio_set_value(GPIO_WD_ENAB, 0);
> +
> + return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block a21_wdt_notifier = {
> + .notifier_call = a21_wdt_notify_sys, };
...
> + ret = register_reboot_notifier(&a21_wdt_notifier);
> + if (ret) {
> + dev_err(&pdev->dev, "Cannot register reboot notifier\n");
> + goto err_register_notif;
> + unregister_reboot_notifier(&a21_wdt_notifier);
...
> + unregister_reboot_notifier(&a21_wdt_notifier);
...
> +static struct platform_driver a21_wdt_driver = {
> + .probe = a21_wdt_probe,
> + .remove = a21_wdt_remove,
> + .driver = {
> + .name = "a21-watchdog",
> + .of_match_table = a21_wdt_ids,
> + },
> +};

I prefer to have the reboot_notifier is being replaced by a platform .shutdown function.
Can you also split the sysfs stuff as a second patch so that I can review that seperately?

Kind regards,
Wim.

Hi Wim,

Of cause I'll do, but unfortunately I probably won't be able to do it today, mind if you get the patch tomorrow?

Regards,
Johannes.

2013-05-27 09:07:59

[permalink] [raw]

Subject: Re: [PATCH v3] watchdog: New watchdog driver for MEN A21 watchdogs

Hi Johannes,

> Of cause I'll do, but unfortunately I probably won't be able to do it today, mind if you get the patch tomorrow?

I don't mind. There is no rush. Just take the time you need to change the code and do your testing.

Kind regards,
wim.

2013-05-27 18:11:52

[permalink] [raw]

Subject: [PATCH v4 1/2] watchdog: New watchdog driver for MEN A21 watchdogs

From: Johannes Thumshirn <[email protected]>

This patch adds the driver for the watchdog devices found on MEN Mikro
Elektronik A21 VMEbus CPU Carrier Boards. It has DT-support and uses the
watchdog framework.

Revision 2:
* Removed unneeded open flag in struct a21_wdt_drv
* Corrected 3bit reason code from gpio
* Additional sysfs files are now part of watchdog sysfs
* Changed OFF/ON delay in ping from 400ms to 10ns
* Reworked timeout setting
* Removed a21_wdt_ioctl(...)

Revision 3:
* Changed pr_{err,info} to dev_{err,info}
* Removed out of memory error print
* Transition from "fast" to "slow" mode not allowed by chip

Revision 4:
* Remove reboot_notifier and place disable code into platform_device's shutdown function
* Removed sysfs interface

Signed-off-by: Johannes Thumshirn <[email protected]>
---
MAINTAINERS | 6 ++
drivers/watchdog/Kconfig | 8 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/mena21_wdt.c | 204 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 219 insertions(+)
create mode 100644 drivers/watchdog/mena21_wdt.c

diff --git a/MAINTAINERS b/MAINTAINERS
index fd3a495..1ca147a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5306,6 +5306,12 @@ F: drivers/mtd/
F: include/linux/mtd/
F: include/uapi/mtd/

+MEN A21 WATCHDOG DRIVER
+M: Johannes Thumshirn <[email protected]>
+L: [email protected]
+S: Supported
+F: drivers/watchdog/mena21_wdt.c
+
METAG ARCHITECTURE
M: James Hogan <[email protected]>
S: Supported
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index e89fc31..192b84d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1172,6 +1172,14 @@ config BOOKE_WDT_DEFAULT_TIMEOUT

The value can be overridden by the wdt_period command-line parameter.

+config MEN_A21_WDT
+ tristate "MEN A21 VME CPU Carrier Board Watchdog Timer"
+ select WATCHDOG_CORE
+ help
+ Watchdog driver for MEN A21 VMEbus CPU Carrier Boards.
+
+ If unsure select N here.
+
# PPC64 Architecture

config WATCHDOG_RTAS
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index a300b94..bffdcb1 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -143,6 +143,7 @@ obj-$(CONFIG_8xxx_WDT) += mpc8xxx_wdt.o
obj-$(CONFIG_MV64X60_WDT) += mv64x60_wdt.o
obj-$(CONFIG_PIKA_WDT) += pika_wdt.o
obj-$(CONFIG_BOOKE_WDT) += booke_wdt.o
+obj-$(CONFIG_MEN_A21_WDT) += mena21_wdt.o

# PPC64 Architecture
obj-$(CONFIG_WATCHDOG_RTAS) += wdrtas.o
diff --git a/drivers/watchdog/mena21_wdt.c b/drivers/watchdog/mena21_wdt.c
new file mode 100644
index 0000000..4fe65ec
--- /dev/null
+++ b/drivers/watchdog/mena21_wdt.c
@@ -0,0 +1,204 @@
+/*
+ * Watchdog driver for the A21 VME CPU Boards
+ *
+ * Copyright (C) 2013 MEN Mikro Elektronik Nuernberg GmbH
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation
+ */
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+#include <linux/uaccess.h>
+#include <linux/gpio.h>
+#include <linux/delay.h>
+#include <linux/bitops.h>
+
+#define GPIO_WD_ENAB 169
+#define GPIO_WD_FAST 170
+#define GPIO_WD_TRIG 171
+
+#define GPIO_RST_CAUSE_BASE 166
+
+struct a21_wdt_drv {
+ struct watchdog_device wdt;
+ struct mutex lock;
+};
+
+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 int a21_wdt_start(struct watchdog_device *wdt)
+{
+ struct a21_wdt_drv *drv = watchdog_get_drvdata(wdt);
+
+ mutex_lock(&drv->lock);
+
+ gpio_set_value(GPIO_WD_ENAB, 1);
+
+ mutex_unlock(&drv->lock);
+
+ return 0;
+}
+
+static int a21_wdt_stop(struct watchdog_device *wdt)
+{
+ struct a21_wdt_drv *drv = watchdog_get_drvdata(wdt);
+
+ /* We don't stop if WDOG_NO_WAY_OUT is set */
+ if (test_bit(WDOG_NO_WAY_OUT, &wdt->status))
+ return -EINVAL;
+
+ mutex_lock(&drv->lock);
+
+ gpio_set_value(GPIO_WD_ENAB, 0);
+
+ mutex_unlock(&drv->lock);
+
+ return 0;
+}
+
+static int a21_wdt_ping(struct watchdog_device *wdt)
+{
+ struct a21_wdt_drv *drv = watchdog_get_drvdata(wdt);
+
+ mutex_lock(&drv->lock);
+
+ gpio_set_value(GPIO_WD_TRIG, 0);
+ ndelay(10);
+ gpio_set_value(GPIO_WD_TRIG, 1);
+
+ mutex_unlock(&drv->lock);
+
+ return 0;
+}
+
+static int a21_wdt_set_timeout(struct watchdog_device *wdt,
+ unsigned int timeout)
+{
+ struct a21_wdt_drv *drv = watchdog_get_drvdata(wdt);
+
+ if (timeout != 1 && timeout != 30) {
+ dev_err(wdt->dev, "Only 1 and 30 allowed as timeout\n");
+ return -EINVAL;
+ }
+
+ if (timeout == 30 && wdt->timeout == 1) {
+ dev_err(wdt->dev,
+ "Transition from fast to slow mode not allowed\n");
+ return -EINVAL;
+ }
+
+ mutex_lock(&drv->lock);
+
+ if (timeout == 1)
+ gpio_set_value(GPIO_WD_FAST, 1);
+ else
+ gpio_set_value(GPIO_WD_FAST, 0);
+
+ wdt->timeout = timeout;
+
+ mutex_unlock(&drv->lock);
+
+ return 0;
+}
+
+static const struct watchdog_info a21_wdt_info = {
+ .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+ .identity = "MEN A21 Watchdog",
+};
+
+static const struct watchdog_ops a21_wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = a21_wdt_start,
+ .stop = a21_wdt_stop,
+ .ping = a21_wdt_ping,
+ .set_timeout = a21_wdt_set_timeout,
+};
+
+static struct watchdog_device a21_wdt = {
+ .info = &a21_wdt_info,
+ .ops = &a21_wdt_ops,
+ .min_timeout = 1,
+ .max_timeout = 30,
+};
+
+static int a21_wdt_probe(struct platform_device *pdev)
+{
+ struct a21_wdt_drv *drv;
+ int ret;
+
+ dev_info(&pdev->dev, "MEN A21 watchdog timer driver enabled\n");
+
+ drv = devm_kzalloc(&pdev->dev, sizeof(struct a21_wdt_drv), GFP_KERNEL);
+ if (!drv)
+ return -ENOMEM;
+
+ mutex_init(&drv->lock);
+ watchdog_set_nowayout(&a21_wdt, nowayout);
+ watchdog_set_drvdata(&a21_wdt, drv);
+
+ ret = watchdog_register_device(&a21_wdt);
+ if (ret) {
+ dev_err(&pdev->dev, "Cannot register watchdog device\n");
+ goto err_register_wd;
+ }
+
+ dev_set_drvdata(&pdev->dev, drv);
+
+ return 0;
+
+err_register_wd:
+ mutex_destroy(&drv->lock);
+
+ return ret;
+}
+
+static int a21_wdt_remove(struct platform_device *pdev)
+{
+ struct a21_wdt_drv *drv = dev_get_drvdata(&pdev->dev);
+
+ dev_warn(&pdev->dev,
+ "Unregistering A21 watchdog driver, board may reboot\n");
+
+
+ watchdog_unregister_device(&drv->wdt);
+
+ mutex_destroy(&drv->lock);
+
+ return 0;
+}
+
+static void a21_wdt_shutdown(struct platform_device *pdev)
+{
+ gpio_set_value(GPIO_WD_ENAB, 0);
+}
+
+static const struct of_device_id a21_wdt_ids[] = {
+ { .compatible = "men,a021-wdt" },
+ { },
+};
+
+static struct platform_driver a21_wdt_driver = {
+ .probe = a21_wdt_probe,
+ .remove = a21_wdt_remove,
+ .shutdown = a21_wdt_shutdown,
+ .driver = {
+ .name = "a21-watchdog",
+ .of_match_table = a21_wdt_ids,
+ },
+};
+
+module_platform_driver(a21_wdt_driver);
+
+MODULE_AUTHOR("MEN Mikro Elektronik");
+MODULE_DESCRIPTION("MEN A21 Watchdog");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:a21-watchdog");
--
1.7.9.5

2013-05-27 18:12:03

[permalink] [raw]

Subject: [PATCH v4 2/2] watchdog: Add sysfs interface for MEN A21 watchdog

From: Johannes Thumshirn <[email protected]>

This patch adds a sysfs interface for the watchdog device found on MEN A21
Boards.

The newly generated files are:
* rebootcause:
Can be one of:
Power on Reset,
CPU Reset Request,
Push Button,
FPGA Reset Request,
Watchdog,
Local Power Bad,
Invalid or
BDI
and shows the reason of the boards last reboot.

* active:
Shows if the watchdog CPLD is actually running

* allow_disable:
Shows if the watchdog is allowed to be disabled (NOWAYOUT disabled)

* fastmode:
Shows if the CPLD is running in fast mode (1s timeout), once it is in
fastmode it can't be switched back to slow mode (30s timeout) until the
next reboot.

Signed-off-by: Johannes Thumshirn <[email protected]>
---
drivers/watchdog/mena21_wdt.c | 88 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 88 insertions(+)

diff --git a/drivers/watchdog/mena21_wdt.c b/drivers/watchdog/mena21_wdt.c
index 4fe65ec..c97c5f4 100644
--- a/drivers/watchdog/mena21_wdt.c
+++ b/drivers/watchdog/mena21_wdt.c
@@ -19,6 +19,17 @@
#include <linux/delay.h>
#include <linux/bitops.h>

+static char *reset_causes[] = {
+ "Power On Reset",
+ "CPU Reset Request",
+ "Push Button",
+ "FPGA Reset Request",
+ "Watchdog",
+ "Local Power Bad",
+ "Invalid",
+ "BDI",
+};
+
#define GPIO_WD_ENAB 169
#define GPIO_WD_FAST 170
#define GPIO_WD_TRIG 171
@@ -35,6 +46,74 @@ module_param(nowayout, bool, 0);
MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");

+static ssize_t rebootcause_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ unsigned int reset = 0;
+ int i;
+
+ for (i = 0; i < 3; i++)
+ reset |= gpio_get_value(GPIO_RST_CAUSE_BASE + i)
+ ? (1 << i) : 0;
+
+ if (reset >= 8)
+ return -EIO;
+
+ return sprintf(buf, "%s\n", reset_causes[reset]);
+}
+static DEVICE_ATTR(rebootcause, S_IRUGO, rebootcause_show, NULL);
+
+static ssize_t active_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ return sprintf(buf, "%d\n", !!gpio_get_value(GPIO_WD_ENAB));
+}
+static DEVICE_ATTR(active, S_IRUGO, active_show, NULL);
+
+static ssize_t allow_disable_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ return sprintf(buf, "%d\n", !nowayout);
+}
+static DEVICE_ATTR(allow_disable, S_IRUGO, allow_disable_show, NULL);
+
+static ssize_t fastmode_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ return sprintf(buf, "%d\n", !!gpio_get_value(GPIO_WD_FAST));
+}
+static DEVICE_ATTR(fastmode, S_IRUGO, fastmode_show, NULL);
+
+static int a21_wdt_create_files(struct watchdog_device *wdev)
+{
+ int ret;
+
+ ret = device_create_file(wdev->dev, &dev_attr_rebootcause);
+ if (ret)
+ return ret;
+ ret = device_create_file(wdev->dev, &dev_attr_active);
+ if (ret)
+ return ret;
+ ret = device_create_file(wdev->dev, &dev_attr_allow_disable);
+ if (ret)
+ return ret;
+ ret = device_create_file(wdev->dev, &dev_attr_fastmode);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static void a21_wdt_remove_files(struct watchdog_device *wdev)
+{
+ device_remove_file(wdev->dev, &dev_attr_rebootcause);
+ device_remove_file(wdev->dev, &dev_attr_active);
+ device_remove_file(wdev->dev, &dev_attr_allow_disable);
+ device_remove_file(wdev->dev, &dev_attr_fastmode);
+}
+
static int a21_wdt_start(struct watchdog_device *wdt)
{
struct a21_wdt_drv *drv = watchdog_get_drvdata(wdt);
@@ -151,10 +230,18 @@ static int a21_wdt_probe(struct platform_device *pdev)
goto err_register_wd;
}

+ ret = a21_wdt_create_files(&a21_wdt);
+ if (ret) {
+ dev_err(&pdev->dev, "Cannot create sysfs entries\n");
+ goto err_create_sysfs;
+ }
+
dev_set_drvdata(&pdev->dev, drv);

return 0;

+err_create_sysfs:
+ watchdog_unregister_device(&drv->wdt);
err_register_wd:
mutex_destroy(&drv->lock);

@@ -168,6 +255,7 @@ static int a21_wdt_remove(struct platform_device *pdev)
dev_warn(&pdev->dev,
"Unregistering A21 watchdog driver, board may reboot\n");

+ a21_wdt_remove_files(&drv->wdt);

watchdog_unregister_device(&drv->wdt);

--
1.7.9.5

2013-05-27 18:25:57

[permalink] [raw]

Subject: Re: [PATCH v4 1/2] watchdog: New watchdog driver for MEN A21 watchdogs

Hi Johannes,

> This patch adds the driver for the watchdog devices found on MEN Mikro
> Elektronik A21 VMEbus CPU Carrier Boards. It has DT-support and uses the
> watchdog framework.
>
> Revision 2:
> * Removed unneeded open flag in struct a21_wdt_drv
> * Corrected 3bit reason code from gpio
> * Additional sysfs files are now part of watchdog sysfs
> * Changed OFF/ON delay in ping from 400ms to 10ns
> * Reworked timeout setting
> * Removed a21_wdt_ioctl(...)
>
> Revision 3:
> * Changed pr_{err,info} to dev_{err,info}
> * Removed out of memory error print
> * Transition from "fast" to "slow" mode not allowed by chip
>
> Revision 4:
> * Remove reboot_notifier and place disable code into platform_device's shutdown function
> * Removed sysfs interface
>
> Signed-off-by: Johannes Thumshirn <[email protected]>
> ---
> MAINTAINERS | 6 ++
> drivers/watchdog/Kconfig | 8 ++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/mena21_wdt.c | 204 +++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 219 insertions(+)
> create mode 100644 drivers/watchdog/mena21_wdt.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fd3a495..1ca147a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5306,6 +5306,12 @@ F: drivers/mtd/
> F: include/linux/mtd/
> F: include/uapi/mtd/
>
> +MEN A21 WATCHDOG DRIVER
> +M: Johannes Thumshirn <[email protected]>
> +L: [email protected]
> +S: Supported
> +F: drivers/watchdog/mena21_wdt.c
> +
> METAG ARCHITECTURE
> M: James Hogan <[email protected]>
> S: Supported
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index e89fc31..192b84d 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1172,6 +1172,14 @@ config BOOKE_WDT_DEFAULT_TIMEOUT
>
> The value can be overridden by the wdt_period command-line parameter.
>
> +config MEN_A21_WDT
> + tristate "MEN A21 VME CPU Carrier Board Watchdog Timer"
> + select WATCHDOG_CORE
> + help
> + Watchdog driver for MEN A21 VMEbus CPU Carrier Boards.
> +
> + If unsure select N here.
> +
> # PPC64 Architecture
>
> config WATCHDOG_RTAS
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index a300b94..bffdcb1 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -143,6 +143,7 @@ obj-$(CONFIG_8xxx_WDT) += mpc8xxx_wdt.o
> obj-$(CONFIG_MV64X60_WDT) += mv64x60_wdt.o
> obj-$(CONFIG_PIKA_WDT) += pika_wdt.o
> obj-$(CONFIG_BOOKE_WDT) += booke_wdt.o
> +obj-$(CONFIG_MEN_A21_WDT) += mena21_wdt.o
>
> # PPC64 Architecture
> obj-$(CONFIG_WATCHDOG_RTAS) += wdrtas.o
> diff --git a/drivers/watchdog/mena21_wdt.c b/drivers/watchdog/mena21_wdt.c
> new file mode 100644
> index 0000000..4fe65ec
> --- /dev/null
> +++ b/drivers/watchdog/mena21_wdt.c
> @@ -0,0 +1,204 @@
> +/*
> + * Watchdog driver for the A21 VME CPU Boards
> + *
> + * Copyright (C) 2013 MEN Mikro Elektronik Nuernberg GmbH
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation
> + */
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/types.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +#include <linux/uaccess.h>
> +#include <linux/gpio.h>
> +#include <linux/delay.h>
> +#include <linux/bitops.h>
> +
> +#define GPIO_WD_ENAB 169
> +#define GPIO_WD_FAST 170
> +#define GPIO_WD_TRIG 171
> +
> +#define GPIO_RST_CAUSE_BASE 166
> +
> +struct a21_wdt_drv {
> + struct watchdog_device wdt;
> + struct mutex lock;
> +};
> +
> +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 int a21_wdt_start(struct watchdog_device *wdt)
> +{
> + struct a21_wdt_drv *drv = watchdog_get_drvdata(wdt);
> +
> + mutex_lock(&drv->lock);
> +
> + gpio_set_value(GPIO_WD_ENAB, 1);
> +
> + mutex_unlock(&drv->lock);
> +
> + return 0;
> +}
> +
> +static int a21_wdt_stop(struct watchdog_device *wdt)
> +{
> + struct a21_wdt_drv *drv = watchdog_get_drvdata(wdt);
> +
> + /* We don't stop if WDOG_NO_WAY_OUT is set */
> + if (test_bit(WDOG_NO_WAY_OUT, &wdt->status))
> + return -EINVAL;
> +
> + mutex_lock(&drv->lock);
> +
> + gpio_set_value(GPIO_WD_ENAB, 0);
> +
> + mutex_unlock(&drv->lock);
> +
> + return 0;
> +}
> +
> +static int a21_wdt_ping(struct watchdog_device *wdt)
> +{
> + struct a21_wdt_drv *drv = watchdog_get_drvdata(wdt);
> +
> + mutex_lock(&drv->lock);
> +
> + gpio_set_value(GPIO_WD_TRIG, 0);
> + ndelay(10);
> + gpio_set_value(GPIO_WD_TRIG, 1);
> +
> + mutex_unlock(&drv->lock);
> +
> + return 0;
> +}
> +
> +static int a21_wdt_set_timeout(struct watchdog_device *wdt,
> + unsigned int timeout)
> +{
> + struct a21_wdt_drv *drv = watchdog_get_drvdata(wdt);
> +
> + if (timeout != 1 && timeout != 30) {
> + dev_err(wdt->dev, "Only 1 and 30 allowed as timeout\n");
> + return -EINVAL;
> + }
> +
> + if (timeout == 30 && wdt->timeout == 1) {
> + dev_err(wdt->dev,
> + "Transition from fast to slow mode not allowed\n");
> + return -EINVAL;
> + }
> +
> + mutex_lock(&drv->lock);
> +
> + if (timeout == 1)
> + gpio_set_value(GPIO_WD_FAST, 1);
> + else
> + gpio_set_value(GPIO_WD_FAST, 0);
> +
> + wdt->timeout = timeout;
> +
> + mutex_unlock(&drv->lock);
> +
> + return 0;
> +}
> +
> +static const struct watchdog_info a21_wdt_info = {
> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> + .identity = "MEN A21 Watchdog",
> +};
> +
> +static const struct watchdog_ops a21_wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = a21_wdt_start,
> + .stop = a21_wdt_stop,
> + .ping = a21_wdt_ping,
> + .set_timeout = a21_wdt_set_timeout,
> +};
> +
> +static struct watchdog_device a21_wdt = {
> + .info = &a21_wdt_info,
> + .ops = &a21_wdt_ops,
> + .min_timeout = 1,
> + .max_timeout = 30,
> +};
> +
> +static int a21_wdt_probe(struct platform_device *pdev)
> +{
> + struct a21_wdt_drv *drv;
> + int ret;
> +
> + dev_info(&pdev->dev, "MEN A21 watchdog timer driver enabled\n");
> +
> + drv = devm_kzalloc(&pdev->dev, sizeof(struct a21_wdt_drv), GFP_KERNEL);
> + if (!drv)
> + return -ENOMEM;
> +
> + mutex_init(&drv->lock);
> + watchdog_set_nowayout(&a21_wdt, nowayout);
> + watchdog_set_drvdata(&a21_wdt, drv);

I am missing the initialisation of the watchdog's timeout value here...

> +
> + ret = watchdog_register_device(&a21_wdt);
> + if (ret) {
> + dev_err(&pdev->dev, "Cannot register watchdog device\n");
> + goto err_register_wd;
> + }
> +
> + dev_set_drvdata(&pdev->dev, drv);
> +
> + return 0;
> +
> +err_register_wd:
> + mutex_destroy(&drv->lock);
> +
> + return ret;
> +}
> +
> +static int a21_wdt_remove(struct platform_device *pdev)
> +{
> + struct a21_wdt_drv *drv = dev_get_drvdata(&pdev->dev);
> +
> + dev_warn(&pdev->dev,
> + "Unregistering A21 watchdog driver, board may reboot\n");
> +
> +
> + watchdog_unregister_device(&drv->wdt);
> +
> + mutex_destroy(&drv->lock);
> +
> + return 0;
> +}
> +
> +static void a21_wdt_shutdown(struct platform_device *pdev)
> +{
> + gpio_set_value(GPIO_WD_ENAB, 0);
> +}
> +
> +static const struct of_device_id a21_wdt_ids[] = {
> + { .compatible = "men,a021-wdt" },
> + { },
> +};
> +
> +static struct platform_driver a21_wdt_driver = {
> + .probe = a21_wdt_probe,
> + .remove = a21_wdt_remove,
> + .shutdown = a21_wdt_shutdown,
> + .driver = {
> + .name = "a21-watchdog",
> + .of_match_table = a21_wdt_ids,
> + },
> +};
> +
> +module_platform_driver(a21_wdt_driver);
> +
> +MODULE_AUTHOR("MEN Mikro Elektronik");
> +MODULE_DESCRIPTION("MEN A21 Watchdog");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:a21-watchdog");
> --
> 1.7.9.5
>

For the rest this looks OK.

Kind regards,
wim.

2013-05-27 19:41:56

[permalink] [raw]

Subject: Re: [PATCH v4 2/2] watchdog: Add sysfs interface for MEN A21 watchdog

Hi Johannes,

> This patch adds a sysfs interface for the watchdog device found on MEN A21
> Boards.
>
> The newly generated files are:
> * rebootcause:
> Can be one of:
> Power on Reset,
> CPU Reset Request,
> Push Button,
> FPGA Reset Request,
> Watchdog,
> Local Power Bad,
> Invalid or
> BDI
> and shows the reason of the boards last reboot.

Question: why don't you use the detection of the reboot cause in your driver
initialization so that you can:
1) report the reboot cause at the start of the driver (like we do in most
drivers that support these kind of features)
2) set the .bootstatus info in the watchdog_device struct data?
(See the WDIOF flags in #include <uapi/linux/watchdog.h>
#define WDIOF_OVERHEAT 0x0001 /* Reset due to CPU overheat */
#define WDIOF_FANFAULT 0x0002 /* Fan failed */
#define WDIOF_EXTERN1 0x0004 /* External relay 1 */
#define WDIOF_EXTERN2 0x0008 /* External relay 2 */
#define WDIOF_POWERUNDER 0x0010 /* Power bad/power fault */
#define WDIOF_CARDRESET 0x0020 /* Card previously reset the CPU */
#define WDIOF_POWEROVER 0x0040 /* Power over voltage */
#define WDIOF_SETTIMEOUT 0x0080 /* Set timeout (in seconds) */
#define WDIOF_MAGICCLOSE 0x0100 /* Supports magic close char */
#define WDIOF_PRETIMEOUT 0x0200 /* Pretimeout (in seconds), get/set */
#define WDIOF_ALARMONLY 0x0400 /* Watchdog triggers a management or
other external alarm not a reboot */
#define WDIOF_KEEPALIVEPING 0x8000 /* Keep alive ping reply */
(Example driver: wdt_pci.c )).

Kind regards,
Wim.

2013-05-28 08:09:08

[permalink] [raw]

Subject: Re: [PATCH v4 1/2] watchdog: New watchdog driver for MEN A21 watchdogs

On Mon, May 27, 2013 at 08:25:54PM +0200, Wim Van Sebroeck wrote:
[...]
> > + watchdog_set_drvdata(&a21_wdt, drv);
>
> I am missing the initialisation of the watchdog's timeout value here...

This watchdog only knows two timeout values, 1s and 30s with the constraint
that you can't go back to 30s once your in a 1s timeout without a reset of the
CPLD. I could initially set it to 30s but that would be redundant.

> For the rest this looks OK.
>
> Kind regards,
> wim.
>

This sounds pretty good.

2013-05-28 16:21:57

[permalink] [raw]

Subject: Re: [PATCH v4 1/2] watchdog: New watchdog driver for MEN A21 watchdogs

On Tue, May 28, 2013 at 10:10:53AM +0200, Johannes Thumshirn wrote:
> On Mon, May 27, 2013 at 08:25:54PM +0200, Wim Van Sebroeck wrote:
> [...]
> > > + watchdog_set_drvdata(&a21_wdt, drv);
> >
> > I am missing the initialisation of the watchdog's timeout value here...
>
> This watchdog only knows two timeout values, 1s and 30s with the constraint
> that you can't go back to 30s once your in a 1s timeout without a reset of the
> CPLD. I could initially set it to 30s but that would be redundant.
>
I wonder - why bother with supporting one-second timeouts ?

Is this realistic, ie can you guarantee that the watchdog will be pinged fast
enough to keep the system alive under all load conditions ? As far as I know
you can not even configure the watchdog application for less than 1 second
ping intervals.

Thanks,
Guenter

2013-05-29 08:19:50

[permalink] [raw]

Subject: Re: [PATCH v4 1/2] watchdog: New watchdog driver for MEN A21 watchdogs

On Tue, May 28, 2013 at 09:22:18AM -0700, Guenter Roeck wrote:
> On Tue, May 28, 2013 at 10:10:53AM +0200, Johannes Thumshirn wrote:
> > On Mon, May 27, 2013 at 08:25:54PM +0200, Wim Van Sebroeck wrote:
> > [...]
> > > > + watchdog_set_drvdata(&a21_wdt, drv);
> > >
> > > I am missing the initialisation of the watchdog's timeout value here...
> >
> > This watchdog only knows two timeout values, 1s and 30s with the constraint
> > that you can't go back to 30s once your in a 1s timeout without a reset of the
> > CPLD. I could initially set it to 30s but that would be redundant.
> >
> I wonder - why bother with supporting one-second timeouts ?
>
> Is this realistic, ie can you guarantee that the watchdog will be pinged fast
> enough to keep the system alive under all load conditions ? As far as I know
> you can not even configure the watchdog application for less than 1 second
> ping intervals.
>
> Thanks,
> Guenter

Well in my tests it was possible, maybe not under extremely high load. But
considering the application these boards (the VME CPU board the watchdogs chip
is on) are usually used by our customers, I hope they can react within a single
second. Another reason why I want to support the 1s timeout is, the VxWorks BSP
does it too and we don't want a Linux driver have less features than it's
VxWorks counterpart, do we ;-). Just kidding. My opinion is, the hardware
supports it and so should the driver, especially for the (safety critical)
industrial control and automation applications our customers usually do. Just
my point of view though.

Regards,
Johannes

2013-05-29 08:34:54

[permalink] [raw]

Subject: Re: [PATCH v4 2/2] watchdog: Add sysfs interface for MEN A21 watchdog

On Mon, May 27, 2013 at 09:41:54PM +0200, Wim Van Sebroeck wrote:
> Hi Johannes,
>
> > This patch adds a sysfs interface for the watchdog device found on MEN A21
> > Boards.
> >
> > The newly generated files are:
> > * rebootcause:
> > Can be one of:
> > Power on Reset,
> > CPU Reset Request,
> > Push Button,
> > FPGA Reset Request,
> > Watchdog,
> > Local Power Bad,
> > Invalid or
> > BDI
> > and shows the reason of the boards last reboot.
>
> Question: why don't you use the detection of the reboot cause in your driver
> initialization so that you can:
> 1) report the reboot cause at the start of the driver (like we do in most
> drivers that support these kind of features)
> 2) set the .bootstatus info in the watchdog_device struct data?
> (See the WDIOF flags in #include <uapi/linux/watchdog.h>
> #define WDIOF_OVERHEAT 0x0001 /* Reset due to CPU overheat */
> #define WDIOF_FANFAULT 0x0002 /* Fan failed */
> #define WDIOF_EXTERN1 0x0004 /* External relay 1 */
> #define WDIOF_EXTERN2 0x0008 /* External relay 2 */
> #define WDIOF_POWERUNDER 0x0010 /* Power bad/power fault */
> #define WDIOF_CARDRESET 0x0020 /* Card previously reset the CPU */
> #define WDIOF_POWEROVER 0x0040 /* Power over voltage */
> #define WDIOF_SETTIMEOUT 0x0080 /* Set timeout (in seconds) */
> #define WDIOF_MAGICCLOSE 0x0100 /* Supports magic close char */
> #define WDIOF_PRETIMEOUT 0x0200 /* Pretimeout (in seconds), get/set */
> #define WDIOF_ALARMONLY 0x0400 /* Watchdog triggers a management or
> other external alarm not a reboot */
> #define WDIOF_KEEPALIVEPING 0x8000 /* Keep alive ping reply */
> (Example driver: wdt_pci.c )).
>
> Kind regards,
> Wim.
>

Ok I'll do. Should I repost this as a change of this patch or the "add driver"
patch?

Regards,
Johannes

2013-05-30 21:52:13

[permalink] [raw]

Subject: Re: [PATCH v4 1/2] watchdog: New watchdog driver for MEN A21 watchdogs

Hi Johannes,

> On Mon, May 27, 2013 at 08:25:54PM +0200, Wim Van Sebroeck wrote:
> [...]
> > > + watchdog_set_drvdata(&a21_wdt, drv);
> >
> > I am missing the initialisation of the watchdog's timeout value here...
>
> This watchdog only knows two timeout values, 1s and 30s with the constraint
> that you can't go back to 30s once your in a 1s timeout without a reset of the
> CPLD. I could initially set it to 30s but that would be redundant.

Yes, but if I use a WDIOC_GETTIMEOUT ioctl call then I will get 0 because
you didn't initialize a21_wdt.timeout so you don't know wether or not
it's 30 or 1.

You can also set it default to 1 and use a timer to do the bridge between userspace
and an in kernel timer of 0.5msec. (see git/linux-watchdog/drivers/watchdog/pika_wdt.c
as an example).

Kind regards,
Wim.

2013-05-30 21:59:35

[permalink] [raw]

Subject: Re: [PATCH v4 1/2] watchdog: New watchdog driver for MEN A21 watchdogs

Hi Guenter,

> On Tue, May 28, 2013 at 10:10:53AM +0200, Johannes Thumshirn wrote:
> > On Mon, May 27, 2013 at 08:25:54PM +0200, Wim Van Sebroeck wrote:
> > [...]
> > > > + watchdog_set_drvdata(&a21_wdt, drv);
> > >
> > > I am missing the initialisation of the watchdog's timeout value here...
> >
> > This watchdog only knows two timeout values, 1s and 30s with the constraint
> > that you can't go back to 30s once your in a 1s timeout without a reset of the
> > CPLD. I could initially set it to 30s but that would be redundant.
> >
> I wonder - why bother with supporting one-second timeouts ?
>
> Is this realistic, ie can you guarantee that the watchdog will be pinged fast
> enough to keep the system alive under all load conditions ? As far as I know
> you can not even configure the watchdog application for less than 1 second
> ping intervals.

that's why certain drivers use a timer to keep kicking the watchdog at
a rate of 0.5s (do a grep on HZ/2) untill userspace times out.
(example: drivers/watchdog/pika_wdt.c or drivers/watchdog/pcwd.c).

Kind regards,
Wim.

2013-05-30 22:00:41

[permalink] [raw]

Subject: Re: [PATCH v4 2/2] watchdog: Add sysfs interface for MEN A21 watchdog

Hi Johannes,

> Ok I'll do. Should I repost this as a change of this patch or the "add driver"
> patch?

the add driver patch.

Kind regards,
Wim.

2013-05-30 22:02:25

[permalink] [raw]

Subject: Re: [PATCH v4 1/2] watchdog: New watchdog driver for MEN A21 watchdogs

On Thu, May 30, 2013 at 11:59:28PM +0200, Wim Van Sebroeck wrote:
> Hi Guenter,
>
> > On Tue, May 28, 2013 at 10:10:53AM +0200, Johannes Thumshirn wrote:
> > > On Mon, May 27, 2013 at 08:25:54PM +0200, Wim Van Sebroeck wrote:
> > > [...]
> > > > > + watchdog_set_drvdata(&a21_wdt, drv);
> > > >
> > > > I am missing the initialisation of the watchdog's timeout value here...
> > >
> > > This watchdog only knows two timeout values, 1s and 30s with the constraint
> > > that you can't go back to 30s once your in a 1s timeout without a reset of the
> > > CPLD. I could initially set it to 30s but that would be redundant.
> > >
> > I wonder - why bother with supporting one-second timeouts ?
> >
> > Is this realistic, ie can you guarantee that the watchdog will be pinged fast
> > enough to keep the system alive under all load conditions ? As far as I know
> > you can not even configure the watchdog application for less than 1 second
> > ping intervals.
>
> that's why certain drivers use a timer to keep kicking the watchdog at
> a rate of 0.5s (do a grep on HZ/2) untill userspace times out.
> (example: drivers/watchdog/pika_wdt.c or drivers/watchdog/pcwd.c).
>
Yes, I have used the same trick in a couple of my drivers. That isn't done here,
though.

I even thought about adding this capability to the infrastructure.

Thanks,
Guenter

2013-05-31 08:22:31

[permalink] [raw]

Subject: Re: [PATCH v4 1/2] watchdog: New watchdog driver for MEN A21 watchdogs

Hi Guenther,

On Thu, May 30, 2013 at 03:02:47PM -0700, Guenter Roeck wrote:
> On Thu, May 30, 2013 at 11:59:28PM +0200, Wim Van Sebroeck wrote:
> > Hi Guenter,
> >
[...]
>
> I even thought about adding this capability to the infrastructure.

Actually this would be pretty nice to have
>
> Thanks,
> Guenter

Johannes

2013-05-31 09:35:59

by anish singh

[permalink] [raw]

Subject: Re: [PATCH v4 1/2] watchdog: New watchdog driver for MEN A21 watchdogs

Hi Guenter,

On Fri, May 31, 2013 at 3:32 AM, Guenter Roeck <[email protected]> wrote:
> On Thu, May 30, 2013 at 11:59:28PM +0200, Wim Van Sebroeck wrote:
>> Hi Guenter,
>>
>> > On Tue, May 28, 2013 at 10:10:53AM +0200, Johannes Thumshirn wrote:
>> > > On Mon, May 27, 2013 at 08:25:54PM +0200, Wim Van Sebroeck wrote:
>> > > [...]
>> > > > > + watchdog_set_drvdata(&a21_wdt, drv);
>> > > >
>> > > > I am missing the initialisation of the watchdog's timeout value here...
>> > >
>> > > This watchdog only knows two timeout values, 1s and 30s with the constraint
>> > > that you can't go back to 30s once your in a 1s timeout without a reset of the
>> > > CPLD. I could initially set it to 30s but that would be redundant.
>> > >
>> > I wonder - why bother with supporting one-second timeouts ?
>> >
>> > Is this realistic, ie can you guarantee that the watchdog will be pinged fast
>> > enough to keep the system alive under all load conditions ? As far as I know
>> > you can not even configure the watchdog application for less than 1 second
>> > ping intervals.
>>
>> that's why certain drivers use a timer to keep kicking the watchdog at
>> a rate of 0.5s (do a grep on HZ/2) untill userspace times out.
>> (example: drivers/watchdog/pika_wdt.c or drivers/watchdog/pcwd.c).
>>
> Yes, I have used the same trick in a couple of my drivers. That isn't done here,
> though.
>
> I even thought about adding this capability to the infrastructure.

If you don't mind can I give it a try?
>
> Thanks,
> Guenter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

2013-05-31 10:09:20