[PATCHv4] Add a simple watchdog 'framework' to Barebox

[PATCHv4] Add a simple watchdog 'framework' to Barebox

[Juergen Beisert]

This patch series add a less simple watchdog command and also a user of it
for the i.MX28 SoC. The watchdog driver might also work on i.MX23, but this
is not tested yet.

This version has reorganized additions. And it is more or less prepared to add
support for more than one watchdog at runtime at a time later on.

The following changes since commit 7eccba9c44dc69c9727a14b3e7a82c31b874327e:

  Merge branch 'for-next/sparse' into next (2012-06-26 21:30:22 +0200)

are available in the git repository at:

  git://git.pengutronix.de/git/jbe/barebox.git next_add_watchdog_frameworkV4

for you to fetch changes up to b75764cbf2bbdbc63c38f85690a39d647a5a7bda:

  ARM/MXS: add a watchdog driver for i.MX28 (2012-06-27 12:25:49 +0200)

----------------------------------------------------------------
Juergen Beisert (2):
      Add a simple watchdog framework
      ARM/MXS: add a watchdog driver for i.MX28

 commands/Kconfig           |   19 +++++++
 commands/Makefile          |    1 +
 commands/wd.c              |   68 ++++++++++++++++++++++++
 drivers/Kconfig            |    1 +
 drivers/Makefile           |    1 +
 drivers/watchdog/Kconfig   |   15 ++++++
 drivers/watchdog/Makefile  |    2 +
 drivers/watchdog/im28wd.c  |  124 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/watchdog/wd_core.c |   61 ++++++++++++++++++++++
 include/watchdog.h         |   24 +++++++++
 10 files changed, 316 insertions(+)
 create mode 100644 commands/wd.c
 create mode 100644 drivers/watchdog/Kconfig
 create mode 100644 drivers/watchdog/Makefile
 create mode 100644 drivers/watchdog/im28wd.c
 create mode 100644 drivers/watchdog/wd_core.c
 create mode 100644 include/watchdog.h

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/2] Add a simple watchdog framework

[Juergen Beisert]

This patch adds a simple wd command which can setup, trigger and stop a watchdog
on the platform.

Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
---
 commands/Kconfig           |   19 +++++++++++++
 commands/Makefile          |    1 +
 commands/wd.c              |   68 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/Kconfig            |    1 +
 drivers/Makefile           |    1 +
 drivers/watchdog/Kconfig   |    9 ++++++
 drivers/watchdog/Makefile  |    1 +
 drivers/watchdog/wd_core.c |   61 +++++++++++++++++++++++++++++++++++++++
 include/watchdog.h         |   24 ++++++++++++++++
 9 files changed, 185 insertions(+)
 create mode 100644 commands/wd.c
 create mode 100644 drivers/watchdog/Kconfig
 create mode 100644 drivers/watchdog/Makefile
 create mode 100644 drivers/watchdog/wd_core.c
 create mode 100644 include/watchdog.h

diff --git a/commands/Kconfig b/commands/Kconfig
index 9fe0c57..2478e3f 100644
--- a/commands/Kconfig
+++ b/commands/Kconfig
@@ -579,6 +579,25 @@ config CMD_USB
 	help
 	  The usb command allows to rescan for USB devices.
 
+menuconfig CMD_WD
+	bool
+	depends on WATCHDOG
+	prompt "wd command                    "
+	help
+	  The 'wd' command which allows to start, stop and trigger the onboard
+	  watchdog.
+
+if CMD_WD
+
+config CMD_WD_DEFAULT_TIMOUT
+	int
+	prompt "default timeout"
+	help
+	  Define the default timeout value in [seconds] if the first call of
+	  'wd' is done without a timeout value (which means the watchdog gets
+	  enabled and re-triggered with the default timeout value).
+endif
+
 endmenu
 
 endif
diff --git a/commands/Makefile b/commands/Makefile
index e9503da..54191b4 100644
--- a/commands/Makefile
+++ b/commands/Makefile
@@ -60,6 +60,7 @@ obj-$(CONFIG_CMD_MENU)		+= menu.o
 obj-$(CONFIG_CMD_PASSWD)	+= passwd.o
 obj-$(CONFIG_CMD_LOGIN)		+= login.o
 obj-$(CONFIG_CMD_LED)		+= led.o
+obj-$(CONFIG_CMD_WD)		+= wd.o
 obj-$(CONFIG_CMD_LED_TRIGGER)	+= trigger.o
 obj-$(CONFIG_CMD_USB)		+= usb.o
 obj-$(CONFIG_CMD_TIME)		+= time.o
diff --git a/commands/wd.c b/commands/wd.c
new file mode 100644
index 0000000..080bab9
--- /dev/null
+++ b/commands/wd.c
@@ -0,0 +1,68 @@
+/*
+ * (c) 2012 Juergen Beisert <kernel@pengutronix.de>
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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 <common.h>
+#include <command.h>
+#include <errno.h>
+#include <linux/ctype.h>
+#include <watchdog.h>
+
+/* default timeout in [sec] */
+static unsigned timeout = CONFIG_CMD_WD_DEFAULT_TIMOUT;
+
+static int do_wd(int argc, char *argv[])
+{
+	int rc;
+
+	if (argc > 1) {
+		if (isdigit(*argv[1])) {
+			timeout = simple_strtoul(argv[1], NULL, 0);
+		} else {
+			printf("numerical parameter expected\n");
+			return 1;
+		}
+	}
+
+	rc = watchdog_set_timeout(timeout);
+	if (rc < 0) {
+		switch (rc) {
+		case -EINVAL:
+			printf("Timeout value out of range\n");
+			break;
+		case -ENOSYS:
+			printf("Watchdog cannot be disabled\n");
+			break;
+		default:
+			printf("Watchdog fails: '%s'\n", strerror(-rc));
+			break;
+		}
+
+		return 1;
+	}
+
+	return 0;
+}
+
+BAREBOX_CMD_HELP_START(wd)
+BAREBOX_CMD_HELP_USAGE("wd [<time>]\n")
+BAREBOX_CMD_HELP_SHORT("enable the watchdog to bark in <time> seconds. "
+		"When <time> is 0, the watchdog gets disabled,\n"
+		"without a parameter the watchdog will be re-triggered\n")
+BAREBOX_CMD_HELP_END
+
+BAREBOX_CMD_START(wd)
+	.cmd = do_wd,
+	.usage = "enable/disable/trigger the watchdog",
+	BAREBOX_CMD_HELP(cmd_wd_help)
+BAREBOX_CMD_END
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 037b0d4..e193063 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -18,5 +18,6 @@ source "drivers/input/Kconfig"
 
 source "drivers/pwm/Kconfig"
 source "drivers/dma/Kconfig"
+source "drivers/watchdog/Kconfig"
 
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index f40b321..52a44c9 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -16,3 +16,4 @@ obj-y	+= eeprom/
 obj-$(CONFIG_PWM) += pwm/
 obj-y	+= input/
 obj-y	+= dma/
+obj-y	+= watchdog/
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
new file mode 100644
index 0000000..c734dfe
--- /dev/null
+++ b/drivers/watchdog/Kconfig
@@ -0,0 +1,9 @@
+menuconfig WATCHDOG
+	bool "Watchdog support              "
+	help
+	  Many platforms support a watchdog to keep track of a working machine.
+	  This framework provides routines to handle these watchdogs.
+
+if WATCHDOG
+
+endif
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
new file mode 100644
index 0000000..630f1b6
--- /dev/null
+++ b/drivers/watchdog/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_WATCHDOG) += wd_core.o
diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
new file mode 100644
index 0000000..8d22212
--- /dev/null
+++ b/drivers/watchdog/wd_core.c
@@ -0,0 +1,61 @@
+/*
+ * (c) 2012 Juergen Beisert <kernel@pengutronix.de>
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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 <common.h>
+#include <command.h>
+#include <errno.h>
+#include <linux/ctype.h>
+#include <watchdog.h>
+
+/*
+ * Note: this simple framework supports one watchdog only.
+ */
+static struct watchdog *watchdog;
+
+int watchdog_register(struct watchdog *wd)
+{
+	if (wd == NULL)
+		return -EINVAL;
+	if (watchdog != NULL)
+		return -EBUSY;
+
+	watchdog = wd;
+	return 0;
+}
+EXPORT_SYMBOL(watchdog_register);
+
+int watchdog_deregister(struct watchdog *wd)
+{
+	if (wd == NULL)
+		return -EINVAL;
+	if (watchdog == NULL || wd != watchdog)
+		return -ENODEV;
+
+	watchdog = NULL;
+	return 0;
+}
+EXPORT_SYMBOL(watchdog_deregister);
+
+/*
+ * start, stop or retrigger the watchdog
+ * timeout in [seconds]. timeout of '0' will disable the watchdog (if possible)
+ */
+int watchdog_set_timeout(unsigned timeout)
+{
+	if (watchdog == NULL)
+		return -ENODEV;
+
+	return watchdog->set_timeout(watchdog, timeout);
+}
+EXPORT_SYMBOL(watchdog_set_timeout);
diff --git a/include/watchdog.h b/include/watchdog.h
new file mode 100644
index 0000000..3e2d08e
--- /dev/null
+++ b/include/watchdog.h
@@ -0,0 +1,24 @@
+/*
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+
+#ifndef INCLUDE_WATCHDOG_H
+# define INCLUDE_WATCHDOG_H
+
+struct watchdog {
+	int (*set_timeout)(struct watchdog *, unsigned);
+};
+
+int watchdog_register(struct watchdog *);
+int watchdog_deregister(struct watchdog *);
+int watchdog_set_timeout(unsigned);
+
+#endif /* INCLUDE_WATCHDOG_H */
-- 
1.7.10


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/2] ARM/MXS: add a watchdog driver for i.MX28

[Juergen Beisert]

Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
---
 drivers/watchdog/Kconfig  |    6 +++
 drivers/watchdog/Makefile |    1 +
 drivers/watchdog/im28wd.c |  124 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 131 insertions(+)
 create mode 100644 drivers/watchdog/im28wd.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index c734dfe..8fdc7a5 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -6,4 +6,10 @@ menuconfig WATCHDOG
 
 if WATCHDOG
 
+config WATCHDOG_MXS28
+	bool "i.MX28"
+	depends on ARCH_IMX28
+	help
+	  Add support for watchdog management for the i.MX28 SoC.
+
 endif
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 630f1b6..b29103b 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_WATCHDOG) += wd_core.o
+obj-$(CONFIG_WATCHDOG_MXS28) += im28wd.o
diff --git a/drivers/watchdog/im28wd.c b/drivers/watchdog/im28wd.c
new file mode 100644
index 0000000..b016910
--- /dev/null
+++ b/drivers/watchdog/im28wd.c
@@ -0,0 +1,124 @@
+/*
+ * (c) 2012 Juergen Beisert <kernel@pengutronix.de>
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ *
+ * Note: this driver works for the i.MX28 SoC. It might work for the
+ * i.MX23 Soc as well, but is not tested yet.
+ */
+
+#include <common.h>
+#include <init.h>
+#include <io.h>
+#include <errno.h>
+#include <malloc.h>
+#include <watchdog.h>
+
+#define MXS_RTC_CTRL 0x0
+#define MXS_RTC_SET_ADDR 0x4
+#define MXS_RTC_CLR_ADDR 0x8
+# define MXS_RTC_CTRL_WATCHDOGEN (1 << 4)
+
+#define MXS_RTC_STAT 0x10
+# define MXS_RTC_STAT_WD_PRESENT (1 << 29)
+
+#define MXS_RTC_WATCHDOG 0x50
+
+#define MXS_RTC_PERSISTENT0 0x60
+/* dubious meaning from inside the SoC's firmware ROM */
+# define MXS_RTC_PERSISTENT0_EXT_RST (1 << 21)
+/* dubious meaning from inside the SoC's firmware ROM */
+# define MXS_RTC_PERSISTENT0_THM_RST (1 << 20)
+
+#define MXS_RTC_PERSISTENT1 0x70
+/* dubious meaning from inside the SoC's firmware ROM */
+# define MXS_RTC_PERSISTENT1_FORCE_UPDATER (1 << 31)
+
+#define MXS_RTC_DEBUG 0xc0
+
+#define WDOG_TICK_RATE 1000 /* the watchdog uses a 1 kHz clock rate */
+
+struct imx28_wd {
+	struct watchdog wd;
+	void __iomem *regs;
+};
+
+#define to_imx28_wd(h) container_of(h, struct imx28_wd, wd)
+
+static int imx28_watchdog_set_timeout(struct watchdog *wd, unsigned timeout)
+{
+	struct imx28_wd *pwd = (struct imx28_wd *)to_imx28_wd(wd);
+	void __iomem *base;
+
+	if (timeout > (ULONG_MAX / WDOG_TICK_RATE))
+		return -EINVAL;
+
+	if (timeout) {
+		writel(timeout * WDOG_TICK_RATE, pwd->regs + MXS_RTC_WATCHDOG);
+		base = pwd->regs + MXS_RTC_SET_ADDR;
+	} else {
+		base = pwd->regs + MXS_RTC_CLR_ADDR;
+	}
+	writel(MXS_RTC_CTRL_WATCHDOGEN, base + MXS_RTC_CTRL);
+	writel(MXS_RTC_PERSISTENT1_FORCE_UPDATER, base + MXS_RTC_PERSISTENT1);
+
+	return 0;
+}
+
+static int imx28_wd_probe(struct device_d *dev)
+{
+	struct imx28_wd *priv;
+	int rc;
+
+	priv = xzalloc(sizeof(struct imx28_wd));
+	priv->regs = dev_request_mem_region(dev, 0);
+	priv->wd.set_timeout = imx28_watchdog_set_timeout;
+
+	if (!(readl(priv->regs + MXS_RTC_STAT) & MXS_RTC_STAT_WD_PRESENT)) {
+		rc = -ENODEV;
+		goto on_error;
+	}
+
+	/* disable the debug feature to ensure a working WD */
+	writel(0x00000000, priv->regs + MXS_RTC_DEBUG);
+
+	rc = watchdog_register(&priv->wd);
+	if (rc != 0)
+		goto on_error;
+
+	dev->priv = priv;
+	return 0;
+
+on_error:
+	free(priv);
+	return rc;
+}
+
+static void imx28_wd_remove(struct device_d *dev)
+{
+	struct imx28_wd *priv= dev->priv;
+	watchdog_deregister(&priv->wd);
+	free(priv);
+}
+
+static struct driver_d imx28_wd_driver = {
+	.name   = "im28wd",
+	.probe  = imx28_wd_probe,
+	.remove = imx28_wd_remove,
+};
+
+static int imx28_wd_init(void)
+{
+	register_driver(&imx28_wd_driver);
+	return 0;
+}
+
+device_initcall(imx28_wd_init);
-- 
1.7.10


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/2] Add a simple watchdog framework

[Juergen Beisert]

This patch adds a simple wd command which can setup, trigger and stop a watchdog
on the platform.

Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
---
 drivers/Kconfig            |    1 +
 drivers/Makefile           |    1 +
 drivers/watchdog/Kconfig   |   10 +++++++
 drivers/watchdog/Makefile  |    1 +
 drivers/watchdog/wd_core.c |   67 ++++++++++++++++++++++++++++++++++++++++++++
 include/watchdog.h         |   18 ++++++++++++
 6 files changed, 98 insertions(+)
 create mode 100644 drivers/watchdog/Kconfig
 create mode 100644 drivers/watchdog/Makefile
 create mode 100644 drivers/watchdog/wd_core.c
 create mode 100644 include/watchdog.h

diff --git a/drivers/Kconfig b/drivers/Kconfig
index 037b0d4..e193063 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -18,5 +18,6 @@ source "drivers/input/Kconfig"
 
 source "drivers/pwm/Kconfig"
 source "drivers/dma/Kconfig"
+source "drivers/watchdog/Kconfig"
 
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index f40b321..52a44c9 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -16,3 +16,4 @@ obj-y	+= eeprom/
 obj-$(CONFIG_PWM) += pwm/
 obj-y	+= input/
 obj-y	+= dma/
+obj-y	+= watchdog/
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
new file mode 100644
index 0000000..b5970c9
--- /dev/null
+++ b/drivers/watchdog/Kconfig
@@ -0,0 +1,10 @@
+menuconfig WATCHDOG
+	bool "Watchdog support              "
+	help
+
+if WATCHDOG
+
+config WATCHDOG_CORE
+	bool
+
+endif
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
new file mode 100644
index 0000000..7a446d7
--- /dev/null
+++ b/drivers/watchdog/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_WATCHDOG_CORE) += wd_core.o
diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
new file mode 100644
index 0000000..7672529
--- /dev/null
+++ b/drivers/watchdog/wd_core.c
@@ -0,0 +1,67 @@
+/*
+ * (c) 2012 Juergen Beisert <kernel@pengutronix.de>
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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 <common.h>
+#include <command.h>
+#include <errno.h>
+#include <linux/ctype.h>
+#include <watchdog.h>
+
+static unsigned timeout = 20; /* default timeout in [sec] */
+
+static int do_wd(int argc, char *argv[])
+{
+	int rc;
+
+	if (argc > 1) {
+		if (isdigit(*argv[1])) {
+			timeout = simple_strtoul(argv[1], NULL, 0);
+		} else {
+			printf("numerical parameter expected\n");
+			return 1;
+		}
+	}
+
+	rc = watchdog_set_timeout(timeout);
+	if (rc < 0) {
+		switch (rc) {
+		case -EINVAL:
+			printf("Timeout value out of range\n");
+			break;
+		case -ENOSYS:
+			printf("Watchdog cannot be disabled\n");
+			break;
+		default:
+			printf("Watchdog fails: '%s'\n", strerror(-rc));
+			break;
+		}
+
+		return 1;
+	}
+
+	return 0;
+}
+
+BAREBOX_CMD_HELP_START(wd)
+BAREBOX_CMD_HELP_USAGE("wd [<seconds>]\n")
+BAREBOX_CMD_HELP_SHORT("enable the watchdog to bark in <seconds> seconds. "
+		"When <seconds> is 0, the watchdog gets disabled,\n"
+		"without a parameter the watchdog will be re-triggered\n")
+BAREBOX_CMD_HELP_END
+
+BAREBOX_CMD_START(wd)
+	.cmd = do_wd,
+	.usage = "enable/disable/trigger the watchdog",
+	BAREBOX_CMD_HELP(cmd_wd_help)
+BAREBOX_CMD_END
diff --git a/include/watchdog.h b/include/watchdog.h
new file mode 100644
index 0000000..721f806
--- /dev/null
+++ b/include/watchdog.h
@@ -0,0 +1,18 @@
+/*
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+
+#ifndef INCLUDE_WATCHDOG_H
+# define INCLUDE_WATCHDOG_H
+
+int watchdog_set_timeout(unsigned);
+
+#endif /* INCLUDE_WATCHDOG_H */
-- 
1.7.10


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] Add a simple watchdog framework

[Juergen Beisert]

Hi Sascha,

Sascha Hauer wrote:
> [...]
> > > > +
> > > > +	rc = watchdog_set_timeout(timeout);
> > > > +	if (rc == -EINVAL) {
> > >
> > > Why do you check for -EINVAL only? This way all other errors will be
> > > silently ignored.
> >
> > Are you sure we must handle other failure codes than -EINVAL here? The
> > called function is very simple and only must distinguish "timeout != 0"
> > and "timeout == 0". So, timeout can be "out of range" or - as you
> > mentioned - some platforms cannot disable the watchdog anymore once it is
> > enabled. Both results into -EINVAL, because the called function cannot
> > use the given value. What else can happen?
>
> Why should we only test for errors that we know and silently drop all
> others?

+1

> Somebody might think that -ENOSYS is the appropriate return value if the
> watchdog can't be disabled. When such a patch is added nobody will
> remember that our error handling only checks for a special error value.

If we accept every possible error value, then we also must move the readable 
error message into the called function (because only the called function 
knows the meaning of its returned value).
Otherwise we must force a specific return-value to give a correct error 
message to the user. But if we force a specific return-value (-EINVAL for 
example) then the -ENOSYS would violate the defined API.

jbe

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] Add a simple watchdog framework

[Sascha Hauer]

On Mon, Jun 25, 2012 at 02:45:43PM +0200, Juergen Beisert wrote:
> Sascha Hauer wrote:
> > On Fri, Jun 22, 2012 at 11:54:02AM +0200, Juergen Beisert wrote:
> > > This patch adds a simple wd command which can setup, trigger and stop a
> > > watchdog on the platform.
> > >
> > > +static int do_wd(int argc, char *argv[])
> > > +{
> > > +	int rc;
> > > +
> > > +	if (argc > 1) {
> > > +		if (isdigit(*argv[1])) {
> > > +			timeout = simple_strtoul(argv[1], NULL, 0);
> > > +		} else {
> > > +			printf("numerical parameter expected\n");
> > > +			return 1;
> > > +		}
> > > +	}
> > > +
> > > +	rc = watchdog_set_timeout(timeout);
> > > +	if (rc == -EINVAL) {
> >
> > Why do you check for -EINVAL only? This way all other errors will be
> > silently ignored.
> 
> Are you sure we must handle other failure codes than -EINVAL here? The called 
> function is very simple and only must distinguish "timeout != 0" and "timeout 
> == 0". So, timeout can be "out of range" or - as you mentioned - some 
> platforms cannot disable the watchdog anymore once it is enabled. Both 
> results into -EINVAL, because the called function cannot use the given value. 
> What else can happen?

Why should we only test for errors that we know and silently drop all others?

Somebody might think that -ENOSYS is the appropriate return value if the
watchdog can't be disabled. When such a patch is added nobody will
remember that our error handling only checks for a special error value.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] Add a simple watchdog framework

[Juergen Beisert]

Sascha Hauer wrote:
> On Fri, Jun 22, 2012 at 11:54:02AM +0200, Juergen Beisert wrote:
> > This patch adds a simple wd command which can setup, trigger and stop a
> > watchdog on the platform.
> >
> > +static int do_wd(int argc, char *argv[])
> > +{
> > +	int rc;
> > +
> > +	if (argc > 1) {
> > +		if (isdigit(*argv[1])) {
> > +			timeout = simple_strtoul(argv[1], NULL, 0);
> > +		} else {
> > +			printf("numerical parameter expected\n");
> > +			return 1;
> > +		}
> > +	}
> > +
> > +	rc = watchdog_set_timeout(timeout);
> > +	if (rc == -EINVAL) {
>
> Why do you check for -EINVAL only? This way all other errors will be
> silently ignored.

Are you sure we must handle other failure codes than -EINVAL here? The called 
function is very simple and only must distinguish "timeout != 0" and "timeout 
== 0". So, timeout can be "out of range" or - as you mentioned - some 
platforms cannot disable the watchdog anymore once it is enabled. Both 
results into -EINVAL, because the called function cannot use the given value. 
What else can happen?

> > +		if (timeout == 0)
> > +			printf("Watchdog cannot be disabled\n");
> > +		else
> > +			printf("Timeout value out of range\n");
> > +		return rc;
>
> Do not return negative error codes here. The shell will interpret
> negative numbers as 'exit'. You have to return 1 for failure.

Okay, missed that.

> > +	}
> > +	return 0;
> > +}
> > +
> > +
> > +#ifndef INCLUDE_WATCHDOG_H
> > +# define INCLUDE_WATCHDOG_H
> > +
> > +extern int watchdog_set_timeout(unsigned);
>
> extern is not needed here.

Okay.

jbe

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] Add a simple watchdog framework

[Sascha Hauer]

On Fri, Jun 22, 2012 at 11:54:02AM +0200, Juergen Beisert wrote:
> This patch adds a simple wd command which can setup, trigger and stop a watchdog
> on the platform.
> 
> +static int do_wd(int argc, char *argv[])
> +{
> +	int rc;
> +
> +	if (argc > 1) {
> +		if (isdigit(*argv[1])) {
> +			timeout = simple_strtoul(argv[1], NULL, 0);
> +		} else {
> +			printf("numerical parameter expected\n");
> +			return 1;
> +		}
> +	}
> +
> +	rc = watchdog_set_timeout(timeout);
> +	if (rc == -EINVAL) {

Why do you check for -EINVAL only? This way all other errors will be
silently ignored.

> +		if (timeout == 0)
> +			printf("Watchdog cannot be disabled\n");
> +		else
> +			printf("Timeout value out of range\n");
> +		return rc;

Do not return negative error codes here. The shell will interpret
negative numbers as 'exit'. You have to return 1 for failure.

> +	}
> +	return 0;
> +}
> +
> +
> +#ifndef INCLUDE_WATCHDOG_H
> +# define INCLUDE_WATCHDOG_H
> +
> +extern int watchdog_set_timeout(unsigned);

extern is not needed here.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/2] Add a simple watchdog framework

[Juergen Beisert]

This patch adds a simple wd command which can setup, trigger and stop a watchdog
on the platform.

Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
---
 drivers/Kconfig            |    1 +
 drivers/Makefile           |    1 +
 drivers/watchdog/Kconfig   |   10 ++++++++
 drivers/watchdog/Makefile  |    1 +
 drivers/watchdog/wd_core.c |   58 ++++++++++++++++++++++++++++++++++++++++++++
 include/watchdog.h         |   18 ++++++++++++++
 6 files changed, 89 insertions(+)
 create mode 100644 drivers/watchdog/Kconfig
 create mode 100644 drivers/watchdog/Makefile
 create mode 100644 drivers/watchdog/wd_core.c
 create mode 100644 include/watchdog.h

diff --git a/drivers/Kconfig b/drivers/Kconfig
index 037b0d4..e193063 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -18,5 +18,6 @@ source "drivers/input/Kconfig"
 
 source "drivers/pwm/Kconfig"
 source "drivers/dma/Kconfig"
+source "drivers/watchdog/Kconfig"
 
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index f40b321..52a44c9 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -16,3 +16,4 @@ obj-y	+= eeprom/
 obj-$(CONFIG_PWM) += pwm/
 obj-y	+= input/
 obj-y	+= dma/
+obj-y	+= watchdog/
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
new file mode 100644
index 0000000..b5970c9
--- /dev/null
+++ b/drivers/watchdog/Kconfig
@@ -0,0 +1,10 @@
+menuconfig WATCHDOG
+	bool "Watchdog support              "
+	help
+
+if WATCHDOG
+
+config WATCHDOG_CORE
+	bool
+
+endif
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
new file mode 100644
index 0000000..7a446d7
--- /dev/null
+++ b/drivers/watchdog/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_WATCHDOG_CORE) += wd_core.o
diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
new file mode 100644
index 0000000..50f1441
--- /dev/null
+++ b/drivers/watchdog/wd_core.c
@@ -0,0 +1,58 @@
+/*
+ * (c) 2012 Juergen Beisert <kernel@pengutronix.de>
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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 <common.h>
+#include <command.h>
+#include <errno.h>
+#include <linux/ctype.h>
+#include <watchdog.h>
+
+static unsigned timeout = 20; /* timeout in [sec] */
+
+static int do_wd(int argc, char *argv[])
+{
+	int rc;
+
+	if (argc > 1) {
+		if (isdigit(*argv[1])) {
+			timeout = simple_strtoul(argv[1], NULL, 0);
+		} else {
+			printf("numerical parameter expected\n");
+			return 1;
+		}
+	}
+
+	rc = watchdog_set_timeout(timeout);
+	if (rc == -EINVAL) {
+		if (timeout == 0)
+			printf("Watchdog cannot be disabled\n");
+		else
+			printf("Timeout value out of range\n");
+		return rc;
+	}
+	return 0;
+}
+
+BAREBOX_CMD_HELP_START(wd)
+BAREBOX_CMD_HELP_USAGE("wd <seconds>\n")
+BAREBOX_CMD_HELP_SHORT("enable the watchdog to bark in <seconds> seconds. "
+		"When <seconds> is 0, the watchdog gets disabled,\n"
+		"without a parameter the watchdog will be re-triggered\n")
+BAREBOX_CMD_HELP_END
+
+BAREBOX_CMD_START(wd)
+	.cmd = do_wd,
+	.usage = "enable/disable/trigger the watchdog",
+	BAREBOX_CMD_HELP(cmd_wd_help)
+BAREBOX_CMD_END
diff --git a/include/watchdog.h b/include/watchdog.h
new file mode 100644
index 0000000..d052a10
--- /dev/null
+++ b/include/watchdog.h
@@ -0,0 +1,18 @@
+/*
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+
+#ifndef INCLUDE_WATCHDOG_H
+# define INCLUDE_WATCHDOG_H
+
+extern int watchdog_set_timeout(unsigned);
+
+#endif /* INCLUDE_WATCHDOG_H */
-- 
1.7.10


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/2] Add a simple watchdog framework

[Juergen Beisert]

This patch adds a simple wd command which can setup, trigger and stop a watchdog
on the platform.

Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
---
 drivers/Kconfig            |    1 +
 drivers/Makefile           |    1 +
 drivers/watchdog/Kconfig   |   10 +++++++++
 drivers/watchdog/Makefile  |    1 +
 drivers/watchdog/wd_core.c |   48 ++++++++++++++++++++++++++++++++++++++++++++
 include/watchdog.h         |   18 +++++++++++++++++
 6 files changed, 79 insertions(+)
 create mode 100644 drivers/watchdog/Kconfig
 create mode 100644 drivers/watchdog/Makefile
 create mode 100644 drivers/watchdog/wd_core.c
 create mode 100644 include/watchdog.h

diff --git a/drivers/Kconfig b/drivers/Kconfig
index 037b0d4..e193063 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -18,5 +18,6 @@ source "drivers/input/Kconfig"
 
 source "drivers/pwm/Kconfig"
 source "drivers/dma/Kconfig"
+source "drivers/watchdog/Kconfig"
 
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index f40b321..52a44c9 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -16,3 +16,4 @@ obj-y	+= eeprom/
 obj-$(CONFIG_PWM) += pwm/
 obj-y	+= input/
 obj-y	+= dma/
+obj-y	+= watchdog/
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
new file mode 100644
index 0000000..b5970c9
--- /dev/null
+++ b/drivers/watchdog/Kconfig
@@ -0,0 +1,10 @@
+menuconfig WATCHDOG
+	bool "Watchdog support              "
+	help
+
+if WATCHDOG
+
+config WATCHDOG_CORE
+	bool
+
+endif
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
new file mode 100644
index 0000000..7a446d7
--- /dev/null
+++ b/drivers/watchdog/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_WATCHDOG_CORE) += wd_core.o
diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
new file mode 100644
index 0000000..6b88b31
--- /dev/null
+++ b/drivers/watchdog/wd_core.c
@@ -0,0 +1,48 @@
+/*
+ * (c) 2012 Juergen Beisert <kernel@pengutronix.de>
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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 <common.h>
+#include <command.h>
+#include <linux/ctype.h>
+#include <watchdog.h>
+
+static unsigned timeout = 20; /* timeout in [sec] */
+
+static int do_wd(int argc, char *argv[])
+{
+	if (argc > 1) {
+		if (isdigit(*argv[1])) {
+			timeout = simple_strtoul(argv[1], NULL, 0);
+		} else {
+			printf("numerical parameter expected\n");
+			return 1;
+		}
+	}
+
+	watchdog_set_timeout(timeout);
+	return 0;
+}
+
+BAREBOX_CMD_HELP_START(wd)
+BAREBOX_CMD_HELP_USAGE("wd <seconds>\n")
+BAREBOX_CMD_HELP_SHORT("enable the watchdog to bark in <seconds> seconds. "
+		"When <seconds> is 0, the watchdog gets disabled,\n"
+		"without a parameter the watchdog will be re-triggered\n")
+BAREBOX_CMD_HELP_END
+
+BAREBOX_CMD_START(wd)
+	.cmd = do_wd,
+	.usage = "enable/disable/trigger the watchdog",
+	BAREBOX_CMD_HELP(cmd_wd_help)
+BAREBOX_CMD_END
diff --git a/include/watchdog.h b/include/watchdog.h
new file mode 100644
index 0000000..d052a10
--- /dev/null
+++ b/include/watchdog.h
@@ -0,0 +1,18 @@
+/*
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+
+#ifndef INCLUDE_WATCHDOG_H
+# define INCLUDE_WATCHDOG_H
+
+extern int watchdog_set_timeout(unsigned);
+
+#endif /* INCLUDE_WATCHDOG_H */
-- 
1.7.10


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox