[PATCH 0/2] Add support for NXP LPC18xx Watchdog timer

[PATCH 0/2] Add support for NXP LPC18xx Watchdog timer

[Ariel D'Alessandro]

Hi,

This patch series adds support for the watchdog timer found in NXP LPC
SoCs family which includes LPC18xx/LPC43xx. Other SoCs in that family may
share the same watchdog hardware.

Patchset is based on tag next-20150624 of the linux-next repository.
It has been successfully tested on a Hitex LPC4350 Evaluation Board and
on a CIAA NXP LPC4337 Board.

Ariel D'Alessandro (2):
  watchdog: NXP LPC18XX Windowed Watchdog Timer Driver
  DT: watchdog: Add NXP LPC18XX Watchdog Timer binding documentation

 .../devicetree/bindings/watchdog/lpc18xx-wdt.txt   |  19 ++
 drivers/watchdog/Kconfig                           |  11 +
 drivers/watchdog/Makefile                          |   1 +
 drivers/watchdog/lpc18xx_wdt.c                     | 340 +++++++++++++++++++++
 4 files changed, 371 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/lpc18xx-wdt.txt
 create mode 100644 drivers/watchdog/lpc18xx_wdt.c

-- 
1.9.1

[PATCH 1/2] watchdog: NXP LPC18XX Windowed Watchdog Timer Driver

[Ariel D'Alessandro]

This commit adds support for the watchdog timer found in NXP LPC SoCs
family, which includes LPC18xx/LPC43xx. Other SoCs in that family may
share the same watchdog hardware.

Watchdog driver registers a restart handler that will restart the system
by performing an incorrect feed after ensuring the watchdog is enabled in
reset mode.

As watchdog cannot be disabled in hardware, driver's stop routine will
regularly send a keepalive ping using a timer.

Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
---
 drivers/watchdog/Kconfig       |  11 ++
 drivers/watchdog/Makefile      |   1 +
 drivers/watchdog/lpc18xx_wdt.c | 340 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 352 insertions(+)
 create mode 100644 drivers/watchdog/lpc18xx_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 742fbbc..31100c2 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -547,6 +547,17 @@ config DIGICOLOR_WATCHDOG
 	  To compile this driver as a module, choose M here: the
 	  module will be called digicolor_wdt.
 
+config LPC18XX_WATCHDOG
+	tristate "LCP18XX Watchdog"
+	depends on ARCH_LPC18XX
+	select WATCHDOG_CORE
+	help
+	  Say Y here if to include support for the watchdog timer
+	  in NXP LPC SoCs family, which includes LPC18xx/LPC43xx
+	  processors.
+	  To compile this driver as a module, choose M here: the
+	  module will be called lpc18xx_wdt.
+
 # AVR32 Architecture
 
 config AT32AP700X_WDT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 59ea9a1..1b0ef48 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
 obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o
 obj-$(CONFIG_MEDIATEK_WATCHDOG) += mtk_wdt.o
 obj-$(CONFIG_DIGICOLOR_WATCHDOG) += digicolor_wdt.o
+obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
 
 # AVR32 Architecture
 obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
diff --git a/drivers/watchdog/lpc18xx_wdt.c b/drivers/watchdog/lpc18xx_wdt.c
new file mode 100644
index 0000000..80d68eb
--- /dev/null
+++ b/drivers/watchdog/lpc18xx_wdt.c
@@ -0,0 +1,340 @@
+/*
+ * NXP LPC18xx Windowed Watchdog Timer (WWDT)
+ *
+ * Copyright (c) 2015 Ariel D'Alessandro <ariel@vanguardiasur.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * Notes
+ * -----
+ * The Watchdog consists of a fixed divide-by-4 clock pre-scaler and a 24-bit counter
+ * which decrements on every clock cycle.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reboot.h>
+#include <linux/watchdog.h>
+
+/* Registers */
+#define LPC_WDT_MOD			0x00
+#define LPC_WDT_MOD_WDEN		BIT(0)
+#define LPC_WDT_MOD_WDRESET		BIT(1)
+#define LPC_WDT_MOD_WDTOF_MASK		0x04
+
+#define LPC_WDT_TC			0x04
+#define LPC_WDT_TC_MIN			0xff
+#define LPC_WDT_TC_MAX			0xffffff
+
+#define LPC_WDT_FEED			0x08
+#define LPC_WDT_FEED_MAGIC1		0xaa
+#define LPC_WDT_FEED_MAGIC2		0x55
+
+#define LPC_WDT_TV			0x0c
+
+/* Clock pre-scaler */
+#define LPC_WDT_CLK_DIV			4
+
+/* Timeout values in seconds */
+#define LPC_WDT_DEF_TIMEOUT		1
+
+static int heartbeat;
+module_param(heartbeat, int, 0);
+MODULE_PARM_DESC(heartbeat, "Watchdog heartbeats in seconds "
+	"(default=" __MODULE_STRING(LPC_WDT_DEF_TIMEOUT) ")");
+
+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) ")");
+
+struct lpc_wdt_dev {
+	struct watchdog_device wdt_dev;
+	struct clk *wdt_clk;
+	struct clk *sys_clk;
+	void __iomem *base;
+	struct timer_list timer;
+	struct notifier_block restart_handler;
+};
+
+static int lpc_wdt_feed(struct watchdog_device *wdt_dev)
+{
+	unsigned long flags;
+	struct lpc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
+
+	raw_local_irq_save(flags);
+	writel(LPC_WDT_FEED_MAGIC1, wdt->base + LPC_WDT_FEED);
+	writel(LPC_WDT_FEED_MAGIC2, wdt->base + LPC_WDT_FEED);
+	raw_local_irq_restore(flags);
+
+	return 0;
+}
+
+static void lpc_wdt_timer_feed(unsigned long data)
+{
+	unsigned long flags;
+	struct watchdog_device *wdt_dev = (struct watchdog_device *) data;
+	struct lpc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
+
+	/*
+	 * An abort condition will occur if an interrupt happens during the feed
+	 * sequence.
+	 */
+	raw_local_irq_save(flags);
+	writel(LPC_WDT_FEED_MAGIC1, wdt->base + LPC_WDT_FEED);
+	writel(LPC_WDT_FEED_MAGIC2, wdt->base + LPC_WDT_FEED);
+	raw_local_irq_restore(flags);
+
+	/* Use safe value (1/2 of real timeout) */
+	mod_timer(&wdt->timer, jiffies + msecs_to_jiffies((wdt_dev->timeout *
+							   MSEC_PER_SEC) / 2));
+}
+
+/*
+ * Since LPC18xx Watchdog cannot be disabled in hardware, we must keep feeding
+ * it with a timer until userspace watchdog software takes over.
+ */
+static int lpc_wdt_stop(struct watchdog_device *wdt_dev)
+{
+	lpc_wdt_timer_feed((unsigned long) wdt_dev);
+
+	return 0;
+}
+
+static void __lpc_wdt_set_timeout(struct lpc_wdt_dev *wdt)
+{
+	unsigned long clk_rate = clk_get_rate(wdt->wdt_clk);
+	unsigned int val;
+
+	val = DIV_ROUND_UP(wdt->wdt_dev.timeout * clk_rate, LPC_WDT_CLK_DIV);
+	writel(val, wdt->base + LPC_WDT_TC);
+}
+
+static int lpc_wdt_set_timeout(struct watchdog_device *wdt_dev,
+			       unsigned int new_timeout)
+{
+	struct lpc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
+
+	wdt->wdt_dev.timeout = new_timeout;
+	__lpc_wdt_set_timeout(wdt);
+
+	return 0;
+}
+
+
+unsigned int lpc_wdt_get_timeleft(struct watchdog_device *wdt_dev)
+{
+	struct lpc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
+	unsigned long clk_rate = clk_get_rate(wdt->wdt_clk);
+	unsigned int val;
+
+	val = readl(wdt->base + LPC_WDT_TV);
+	return ((val * LPC_WDT_CLK_DIV) / clk_rate);
+}
+
+static int lpc_wdt_start(struct watchdog_device *wdt_dev)
+{
+	unsigned int val;
+	struct lpc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
+
+	if (timer_pending(&wdt->timer))
+		del_timer(&wdt->timer);
+
+	val = readl(wdt->base + LPC_WDT_MOD);
+	val |= LPC_WDT_MOD_WDEN;
+	val |= LPC_WDT_MOD_WDRESET;
+	writel(val, wdt->base + LPC_WDT_MOD);
+
+	/*
+	 * Setting the WDEN bit in the WDMOD register is not sufficient to
+	 * enable the Watchdog. A valid feed sequence must be completed after
+	 * setting WDEN before the Watchdog is capable of generating a reset.
+	 */
+	lpc_wdt_feed(wdt_dev);
+
+	return 0;
+}
+
+static struct watchdog_info lpc_wdt_info = {
+	.identity	= "LPC 18XX Watchdog",
+	.options	= WDIOF_SETTIMEOUT |
+			  WDIOF_KEEPALIVEPING |
+			  WDIOF_MAGICCLOSE,
+};
+
+static const struct watchdog_ops lpc_wdt_ops = {
+	.owner		= THIS_MODULE,
+	.start		= lpc_wdt_start,
+	.stop		= lpc_wdt_stop,
+	.ping		= lpc_wdt_feed,
+	.set_timeout	= lpc_wdt_set_timeout,
+	.get_timeleft	= lpc_wdt_get_timeleft,
+};
+
+static int lpc_wdt_restart(struct notifier_block *this, unsigned long mode,
+			   void *cmd)
+{
+	int val;
+	unsigned long flags;
+	struct lpc_wdt_dev *wdt = container_of(this, struct lpc_wdt_dev,
+					       restart_handler);
+
+	/*
+	 * Incorrect feed sequence causes immediate watchdog reset if enabled.
+	 */
+	raw_local_irq_save(flags);
+
+	val = readl(wdt->base + LPC_WDT_MOD);
+	val |= LPC_WDT_MOD_WDEN;
+	val |= LPC_WDT_MOD_WDRESET;
+	writel(val, wdt->base + LPC_WDT_MOD);
+
+	writel(LPC_WDT_FEED_MAGIC1, wdt->base + LPC_WDT_FEED);
+	writel(LPC_WDT_FEED_MAGIC2, wdt->base + LPC_WDT_FEED);
+
+	writel(LPC_WDT_FEED_MAGIC1, wdt->base + LPC_WDT_FEED);
+	writel(LPC_WDT_FEED_MAGIC1, wdt->base + LPC_WDT_FEED);
+
+	raw_local_irq_restore(flags);
+
+	return NOTIFY_OK;
+}
+
+static int lpc_wdt_probe(struct platform_device *pdev)
+{
+	int ret;
+	unsigned long clk_rate;
+	struct resource *res;
+	struct lpc_wdt_dev *lpc_wdt;
+
+	lpc_wdt = devm_kzalloc(&pdev->dev, sizeof(*lpc_wdt), GFP_KERNEL);
+	if (!lpc_wdt)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	lpc_wdt->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(lpc_wdt->base))
+		return PTR_ERR(lpc_wdt->base);
+
+	lpc_wdt->sys_clk = devm_clk_get(&pdev->dev, "sys");
+	if (IS_ERR(lpc_wdt->sys_clk)) {
+		dev_err(&pdev->dev, "failed to get the sys clock\n");
+		return PTR_ERR(lpc_wdt->sys_clk);
+	}
+
+	lpc_wdt->wdt_clk = devm_clk_get(&pdev->dev, "wdt");
+	if (IS_ERR(lpc_wdt->wdt_clk)) {
+		dev_err(&pdev->dev, "failed to get the wdt clock\n");
+		return PTR_ERR(lpc_wdt->wdt_clk);
+	}
+
+	ret = clk_prepare_enable(lpc_wdt->sys_clk);
+	if (ret) {
+		dev_err(&pdev->dev, "could not prepare or enable sys clock\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(lpc_wdt->wdt_clk);
+	if (ret) {
+		dev_err(&pdev->dev, "could not prepare or enable wdt clock\n");
+		goto disable_sys_clk;
+	}
+
+	/* We use the clock rate to calculate timeouts */
+	clk_rate = clk_get_rate(lpc_wdt->wdt_clk);
+	if (clk_rate == 0) {
+		dev_err(&pdev->dev, "failed to get clock rate\n");
+		ret = -EINVAL;
+		goto disable_wdt_clk;
+	}
+
+	lpc_wdt->wdt_dev.info = &lpc_wdt_info;
+	lpc_wdt->wdt_dev.ops = &lpc_wdt_ops;
+
+	lpc_wdt->wdt_dev.min_timeout =
+		DIV_ROUND_UP(LPC_WDT_TC_MIN * LPC_WDT_CLK_DIV, clk_rate);
+
+	lpc_wdt->wdt_dev.max_timeout =
+		(LPC_WDT_TC_MAX * LPC_WDT_CLK_DIV) / clk_rate;
+
+	lpc_wdt->wdt_dev.parent = &pdev->dev;
+	watchdog_set_drvdata(&lpc_wdt->wdt_dev, lpc_wdt);
+
+	ret = watchdog_init_timeout(&lpc_wdt->wdt_dev, heartbeat, &pdev->dev);
+	if (ret < 0)
+		lpc_wdt->wdt_dev.timeout = LPC_WDT_DEF_TIMEOUT;
+
+	__lpc_wdt_set_timeout(lpc_wdt);
+
+	setup_timer(&lpc_wdt->timer, lpc_wdt_timer_feed,
+		    (unsigned long)&lpc_wdt->wdt_dev);
+
+	watchdog_set_nowayout(&lpc_wdt->wdt_dev, nowayout);
+
+	platform_set_drvdata(pdev, lpc_wdt);
+
+	ret = watchdog_register_device(&lpc_wdt->wdt_dev);
+	if (ret)
+		goto disable_wdt_clk;
+
+	lpc_wdt->restart_handler.notifier_call = lpc_wdt_restart;
+	lpc_wdt->restart_handler.priority = 128;
+	ret = register_restart_handler(&lpc_wdt->restart_handler);
+	if (ret)
+		dev_warn(&pdev->dev, "failed to register restart handler: %d\n",
+			 ret);
+
+	return 0;
+
+disable_wdt_clk:
+	clk_disable_unprepare(lpc_wdt->wdt_clk);
+disable_sys_clk:
+	clk_disable_unprepare(lpc_wdt->sys_clk);
+	return ret;
+}
+
+static void lpc_wdt_shutdown(struct platform_device *pdev)
+{
+	struct lpc_wdt_dev *lpc_wdt = platform_get_drvdata(pdev);
+
+	lpc_wdt_stop(&lpc_wdt->wdt_dev);
+}
+
+static int lpc_wdt_remove(struct platform_device *pdev)
+{
+	struct lpc_wdt_dev *lpc_wdt = platform_get_drvdata(pdev);
+
+	lpc_wdt_stop(&lpc_wdt->wdt_dev);
+	watchdog_unregister_device(&lpc_wdt->wdt_dev);
+	clk_disable_unprepare(lpc_wdt->wdt_clk);
+	clk_disable_unprepare(lpc_wdt->sys_clk);
+
+	return 0;
+}
+
+static const struct of_device_id lpc_wdt_match[] = {
+	{ .compatible = "nxp,lpc18xx-wdt" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, lpc_wdt_match);
+
+static struct platform_driver lpc_wdt_driver = {
+	.driver = {
+		.name = "lpc18xx-wdt",
+		.of_match_table	= lpc_wdt_match,
+	},
+	.probe = lpc_wdt_probe,
+	.remove = lpc_wdt_remove,
+	.shutdown = lpc_wdt_shutdown,
+};
+module_platform_driver(lpc_wdt_driver);
+
+MODULE_AUTHOR("Ariel D'Alessandro <ariel@vanguardiasur.com.ar>");
+MODULE_DESCRIPTION("NXP LPC18xx Windowed Watchdog Timer Driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH 2/2] DT: watchdog: Add NXP LPC18XX Watchdog Timer binding documentation

[Ariel D'Alessandro]

Add the devicetree binding document for NXP LPC18XX Watchdog Timer.

Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
---
 .../devicetree/bindings/watchdog/lpc18xx-wdt.txt      | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/lpc18xx-wdt.txt

diff --git a/Documentation/devicetree/bindings/watchdog/lpc18xx-wdt.txt b/Documentation/devicetree/bindings/watchdog/lpc18xx-wdt.txt
new file mode 100644
index 0000000..12f1861
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/lpc18xx-wdt.txt
@@ -0,0 +1,19 @@
+* NXP LPC18xx Windowed Watchdog timer (WWDT)
+
+Required properties:
+- compatible: Should be "nxp,lpc18xx-wdt"
+- reg: Should contain WDT registers location and length
+- clocks: Must contain an entry for each entry in clock-names.
+- clock-names: Should contain "wdt" and "sys"; the watchdog counter
+               clock and register interface clock respectively.
+- interrupts: Should contain WDT interrupt
+
+Examples:
+
+watchdog@40080000 {
+	compatible = "nxp,lpc18xx-wdt";
+	reg = <0x40080000 0x24>;
+	clocks = <&xtal>, <&pll1>;
+	clock-names = "wdt", "sys";
+	interrupts = <49>;
+};
-- 
1.9.1

Re: [PATCH 1/2] watchdog: NXP LPC18XX Windowed Watchdog Timer Driver

[Guenter Roeck]

Hi Ariel,

On Fri, Jun 26, 2015 at 03:24:31PM -0300, Ariel D'Alessandro wrote:
> This commit adds support for the watchdog timer found in NXP LPC SoCs
> family, which includes LPC18xx/LPC43xx. Other SoCs in that family may
> share the same watchdog hardware.
> 
> Watchdog driver registers a restart handler that will restart the system
> by performing an incorrect feed after ensuring the watchdog is enabled in
> reset mode.
> 
> As watchdog cannot be disabled in hardware, driver's stop routine will
> regularly send a keepalive ping using a timer.
> 
> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
> ---
>  drivers/watchdog/Kconfig       |  11 ++
>  drivers/watchdog/Makefile      |   1 +
>  drivers/watchdog/lpc18xx_wdt.c | 340 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 352 insertions(+)
>  create mode 100644 drivers/watchdog/lpc18xx_wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 742fbbc..31100c2 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -547,6 +547,17 @@ config DIGICOLOR_WATCHDOG
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called digicolor_wdt.
>  
> +config LPC18XX_WATCHDOG
> +	tristate "LCP18XX Watchdog"
> +	depends on ARCH_LPC18XX
> +	select WATCHDOG_CORE
> +	help
> +	  Say Y here if to include support for the watchdog timer
> +	  in NXP LPC SoCs family, which includes LPC18xx/LPC43xx
> +	  processors.
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called lpc18xx_wdt.
> +
>  # AVR32 Architecture
>  
>  config AT32AP700X_WDT
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 59ea9a1..1b0ef48 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -66,6 +66,7 @@ obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
>  obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o
>  obj-$(CONFIG_MEDIATEK_WATCHDOG) += mtk_wdt.o
>  obj-$(CONFIG_DIGICOLOR_WATCHDOG) += digicolor_wdt.o
> +obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
>  
>  # AVR32 Architecture
>  obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
> diff --git a/drivers/watchdog/lpc18xx_wdt.c b/drivers/watchdog/lpc18xx_wdt.c
> new file mode 100644
> index 0000000..80d68eb
> --- /dev/null
> +++ b/drivers/watchdog/lpc18xx_wdt.c
> @@ -0,0 +1,340 @@
> +/*
> + * NXP LPC18xx Windowed Watchdog Timer (WWDT)
> + *

What does "Windowed" stand for ? That term doesn't really mean anything
to me. How does it differ from a non-windowed watchdog driver ?

> + * Copyright (c) 2015 Ariel D'Alessandro <ariel@vanguardiasur.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * Notes
> + * -----
> + * The Watchdog consists of a fixed divide-by-4 clock pre-scaler and a 24-bit counter
> + * which decrements on every clock cycle.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reboot.h>
> +#include <linux/watchdog.h>
> +
> +/* Registers */
> +#define LPC_WDT_MOD			0x00
> +#define LPC_WDT_MOD_WDEN		BIT(0)
> +#define LPC_WDT_MOD_WDRESET		BIT(1)
> +#define LPC_WDT_MOD_WDTOF_MASK		0x04
> +
> +#define LPC_WDT_TC			0x04
> +#define LPC_WDT_TC_MIN			0xff
> +#define LPC_WDT_TC_MAX			0xffffff
> +
> +#define LPC_WDT_FEED			0x08
> +#define LPC_WDT_FEED_MAGIC1		0xaa
> +#define LPC_WDT_FEED_MAGIC2		0x55
> +
> +#define LPC_WDT_TV			0x0c
> +
> +/* Clock pre-scaler */
> +#define LPC_WDT_CLK_DIV			4
> +
> +/* Timeout values in seconds */
> +#define LPC_WDT_DEF_TIMEOUT		1
> +

One second ? This is highly unusual. 30 or 60 seconds is more common,
and one second would be very challenging for user space.

Any special reason for using such a tight default ?

> +static int heartbeat;
> +module_param(heartbeat, int, 0);
> +MODULE_PARM_DESC(heartbeat, "Watchdog heartbeats in seconds "
> +	"(default=" __MODULE_STRING(LPC_WDT_DEF_TIMEOUT) ")");
> +
> +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) ")");
> +
> +struct lpc_wdt_dev {
> +	struct watchdog_device wdt_dev;
> +	struct clk *wdt_clk;
> +	struct clk *sys_clk;
> +	void __iomem *base;
> +	struct timer_list timer;
> +	struct notifier_block restart_handler;
> +};
> +
> +static int lpc_wdt_feed(struct watchdog_device *wdt_dev)
> +{
> +	unsigned long flags;
> +	struct lpc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> +
Please order variables length wise where possible.

> +	raw_local_irq_save(flags);
> +	writel(LPC_WDT_FEED_MAGIC1, wdt->base + LPC_WDT_FEED);
> +	writel(LPC_WDT_FEED_MAGIC2, wdt->base + LPC_WDT_FEED);
> +	raw_local_irq_restore(flags);
> +
> +	return 0;
> +}
> +
> +static void lpc_wdt_timer_feed(unsigned long data)
> +{
> +	unsigned long flags;
> +	struct watchdog_device *wdt_dev = (struct watchdog_device *) data;
> +	struct lpc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> +
> +	/*
> +	 * An abort condition will occur if an interrupt happens during the feed
> +	 * sequence.
> +	 */
> +	raw_local_irq_save(flags);
> +	writel(LPC_WDT_FEED_MAGIC1, wdt->base + LPC_WDT_FEED);
> +	writel(LPC_WDT_FEED_MAGIC2, wdt->base + LPC_WDT_FEED);
> +	raw_local_irq_restore(flags);
> +
Unless I am missing something, 
	lpc_wdt_feed(wdt_dev);
should do the same and reduce code duplication.

> +	/* Use safe value (1/2 of real timeout) */
> +	mod_timer(&wdt->timer, jiffies + msecs_to_jiffies((wdt_dev->timeout *
> +							   MSEC_PER_SEC) / 2));
> +}
> +
> +/*
> + * Since LPC18xx Watchdog cannot be disabled in hardware, we must keep feeding
> + * it with a timer until userspace watchdog software takes over.
> + */
> +static int lpc_wdt_stop(struct watchdog_device *wdt_dev)
> +{
> +	lpc_wdt_timer_feed((unsigned long) wdt_dev);
> +
> +	return 0;
> +}
> +
> +static void __lpc_wdt_set_timeout(struct lpc_wdt_dev *wdt)
> +{
> +	unsigned long clk_rate = clk_get_rate(wdt->wdt_clk);
> +	unsigned int val;
> +
> +	val = DIV_ROUND_UP(wdt->wdt_dev.timeout * clk_rate, LPC_WDT_CLK_DIV);
> +	writel(val, wdt->base + LPC_WDT_TC);
> +}
> +
> +static int lpc_wdt_set_timeout(struct watchdog_device *wdt_dev,
> +			       unsigned int new_timeout)
> +{
> +	struct lpc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> +
> +	wdt->wdt_dev.timeout = new_timeout;
> +	__lpc_wdt_set_timeout(wdt);
> +
> +	return 0;
> +}
> +
> +
> +unsigned int lpc_wdt_get_timeleft(struct watchdog_device *wdt_dev)
> +{
> +	struct lpc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> +	unsigned long clk_rate = clk_get_rate(wdt->wdt_clk);
> +	unsigned int val;
> +
> +	val = readl(wdt->base + LPC_WDT_TV);
> +	return ((val * LPC_WDT_CLK_DIV) / clk_rate);
> +}
> +
> +static int lpc_wdt_start(struct watchdog_device *wdt_dev)
> +{
> +	unsigned int val;
> +	struct lpc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> +
> +	if (timer_pending(&wdt->timer))
> +		del_timer(&wdt->timer);
> +
> +	val = readl(wdt->base + LPC_WDT_MOD);
> +	val |= LPC_WDT_MOD_WDEN;
> +	val |= LPC_WDT_MOD_WDRESET;
> +	writel(val, wdt->base + LPC_WDT_MOD);
> +
> +	/*
> +	 * Setting the WDEN bit in the WDMOD register is not sufficient to
> +	 * enable the Watchdog. A valid feed sequence must be completed after
> +	 * setting WDEN before the Watchdog is capable of generating a reset.
> +	 */
> +	lpc_wdt_feed(wdt_dev);
> +
> +	return 0;
> +}
> +
> +static struct watchdog_info lpc_wdt_info = {
> +	.identity	= "LPC 18XX Watchdog",
> +	.options	= WDIOF_SETTIMEOUT |
> +			  WDIOF_KEEPALIVEPING |
> +			  WDIOF_MAGICCLOSE,
> +};
> +
> +static const struct watchdog_ops lpc_wdt_ops = {
> +	.owner		= THIS_MODULE,
> +	.start		= lpc_wdt_start,
> +	.stop		= lpc_wdt_stop,
> +	.ping		= lpc_wdt_feed,
> +	.set_timeout	= lpc_wdt_set_timeout,
> +	.get_timeleft	= lpc_wdt_get_timeleft,
> +};
> +
> +static int lpc_wdt_restart(struct notifier_block *this, unsigned long mode,
> +			   void *cmd)
> +{
> +	int val;
> +	unsigned long flags;
> +	struct lpc_wdt_dev *wdt = container_of(this, struct lpc_wdt_dev,
> +					       restart_handler);
> +
> +	/*
> +	 * Incorrect feed sequence causes immediate watchdog reset if enabled.
> +	 */
> +	raw_local_irq_save(flags);
> +
> +	val = readl(wdt->base + LPC_WDT_MOD);
> +	val |= LPC_WDT_MOD_WDEN;
> +	val |= LPC_WDT_MOD_WDRESET;
> +	writel(val, wdt->base + LPC_WDT_MOD);
> +
> +	writel(LPC_WDT_FEED_MAGIC1, wdt->base + LPC_WDT_FEED);
> +	writel(LPC_WDT_FEED_MAGIC2, wdt->base + LPC_WDT_FEED);
> +
> +	writel(LPC_WDT_FEED_MAGIC1, wdt->base + LPC_WDT_FEED);
> +	writel(LPC_WDT_FEED_MAGIC1, wdt->base + LPC_WDT_FEED);
> +
> +	raw_local_irq_restore(flags);
> +
> +	return NOTIFY_OK;
> +}
> +
> +static int lpc_wdt_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	unsigned long clk_rate;
> +	struct resource *res;
> +	struct lpc_wdt_dev *lpc_wdt;
> +
> +	lpc_wdt = devm_kzalloc(&pdev->dev, sizeof(*lpc_wdt), GFP_KERNEL);
> +	if (!lpc_wdt)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	lpc_wdt->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(lpc_wdt->base))
> +		return PTR_ERR(lpc_wdt->base);
> +
> +	lpc_wdt->sys_clk = devm_clk_get(&pdev->dev, "sys");
> +	if (IS_ERR(lpc_wdt->sys_clk)) {
> +		dev_err(&pdev->dev, "failed to get the sys clock\n");
> +		return PTR_ERR(lpc_wdt->sys_clk);
> +	}
> +
> +	lpc_wdt->wdt_clk = devm_clk_get(&pdev->dev, "wdt");
> +	if (IS_ERR(lpc_wdt->wdt_clk)) {
> +		dev_err(&pdev->dev, "failed to get the wdt clock\n");
> +		return PTR_ERR(lpc_wdt->wdt_clk);
> +	}
> +
> +	ret = clk_prepare_enable(lpc_wdt->sys_clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "could not prepare or enable sys clock\n");
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(lpc_wdt->wdt_clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "could not prepare or enable wdt clock\n");
> +		goto disable_sys_clk;
> +	}
> +
> +	/* We use the clock rate to calculate timeouts */
> +	clk_rate = clk_get_rate(lpc_wdt->wdt_clk);
> +	if (clk_rate == 0) {
> +		dev_err(&pdev->dev, "failed to get clock rate\n");
> +		ret = -EINVAL;
> +		goto disable_wdt_clk;
> +	}
> +
> +	lpc_wdt->wdt_dev.info = &lpc_wdt_info;
> +	lpc_wdt->wdt_dev.ops = &lpc_wdt_ops;
> +
> +	lpc_wdt->wdt_dev.min_timeout =
> +		DIV_ROUND_UP(LPC_WDT_TC_MIN * LPC_WDT_CLK_DIV, clk_rate);
> +
> +	lpc_wdt->wdt_dev.max_timeout =
> +		(LPC_WDT_TC_MAX * LPC_WDT_CLK_DIV) / clk_rate;
> +
> +	lpc_wdt->wdt_dev.parent = &pdev->dev;
> +	watchdog_set_drvdata(&lpc_wdt->wdt_dev, lpc_wdt);
> +
> +	ret = watchdog_init_timeout(&lpc_wdt->wdt_dev, heartbeat, &pdev->dev);
> +	if (ret < 0)
> +		lpc_wdt->wdt_dev.timeout = LPC_WDT_DEF_TIMEOUT;
> +
> +	__lpc_wdt_set_timeout(lpc_wdt);
> +
> +	setup_timer(&lpc_wdt->timer, lpc_wdt_timer_feed,
> +		    (unsigned long)&lpc_wdt->wdt_dev);
> +
> +	watchdog_set_nowayout(&lpc_wdt->wdt_dev, nowayout);
> +
> +	platform_set_drvdata(pdev, lpc_wdt);
> +
> +	ret = watchdog_register_device(&lpc_wdt->wdt_dev);
> +	if (ret)
> +		goto disable_wdt_clk;
> +
> +	lpc_wdt->restart_handler.notifier_call = lpc_wdt_restart;
> +	lpc_wdt->restart_handler.priority = 128;
> +	ret = register_restart_handler(&lpc_wdt->restart_handler);
> +	if (ret)
> +		dev_warn(&pdev->dev, "failed to register restart handler: %d\n",
> +			 ret);
> +
> +	return 0;
> +
> +disable_wdt_clk:
> +	clk_disable_unprepare(lpc_wdt->wdt_clk);
> +disable_sys_clk:
> +	clk_disable_unprepare(lpc_wdt->sys_clk);
> +	return ret;
> +}
> +
> +static void lpc_wdt_shutdown(struct platform_device *pdev)
> +{
> +	struct lpc_wdt_dev *lpc_wdt = platform_get_drvdata(pdev);
> +
> +	lpc_wdt_stop(&lpc_wdt->wdt_dev);
> +}
> +
> +static int lpc_wdt_remove(struct platform_device *pdev)
> +{
> +	struct lpc_wdt_dev *lpc_wdt = platform_get_drvdata(pdev);
> +
> +	lpc_wdt_stop(&lpc_wdt->wdt_dev);

This will keep the timer enabled. It would be interesting to see what
happens if you build the driver as module and unload it. I think
it will crash. Can you try ?

> +	watchdog_unregister_device(&lpc_wdt->wdt_dev);
> +	clk_disable_unprepare(lpc_wdt->wdt_clk);
> +	clk_disable_unprepare(lpc_wdt->sys_clk);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id lpc_wdt_match[] = {
> +	{ .compatible = "nxp,lpc18xx-wdt" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, lpc_wdt_match);
> +
> +static struct platform_driver lpc_wdt_driver = {
> +	.driver = {
> +		.name = "lpc18xx-wdt",
> +		.of_match_table	= lpc_wdt_match,
> +	},
> +	.probe = lpc_wdt_probe,
> +	.remove = lpc_wdt_remove,
> +	.shutdown = lpc_wdt_shutdown,
> +};
> +module_platform_driver(lpc_wdt_driver);
> +
> +MODULE_AUTHOR("Ariel D'Alessandro <ariel@vanguardiasur.com.ar>");
> +MODULE_DESCRIPTION("NXP LPC18xx Windowed Watchdog Timer Driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 1.9.1
> 

Re: [PATCH 1/2] watchdog: NXP LPC18XX Windowed Watchdog Timer Driver

[Ariel D'Alessandro]

Hi Guenter,
Thanks for your quick reply.

El 26/06/15 a las 16:07, Guenter Roeck escibió:
> Hi Ariel,
> 
> On Fri, Jun 26, 2015 at 03:24:31PM -0300, Ariel D'Alessandro wrote:
>> This commit adds support for the watchdog timer found in NXP LPC SoCs
>> family, which includes LPC18xx/LPC43xx. Other SoCs in that family may
>> share the same watchdog hardware.
>>
>> Watchdog driver registers a restart handler that will restart the system
>> by performing an incorrect feed after ensuring the watchdog is enabled in
>> reset mode.
>>
>> As watchdog cannot be disabled in hardware, driver's stop routine will
>> regularly send a keepalive ping using a timer.
>>
>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>> ---
>>  drivers/watchdog/Kconfig       |  11 ++
>>  drivers/watchdog/Makefile      |   1 +
>>  drivers/watchdog/lpc18xx_wdt.c | 340 +++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 352 insertions(+)
>>  create mode 100644 drivers/watchdog/lpc18xx_wdt.c
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 742fbbc..31100c2 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -547,6 +547,17 @@ config DIGICOLOR_WATCHDOG
>>  	  To compile this driver as a module, choose M here: the
>>  	  module will be called digicolor_wdt.
>>  
>> +config LPC18XX_WATCHDOG
>> +	tristate "LCP18XX Watchdog"
>> +	depends on ARCH_LPC18XX
>> +	select WATCHDOG_CORE
>> +	help
>> +	  Say Y here if to include support for the watchdog timer
>> +	  in NXP LPC SoCs family, which includes LPC18xx/LPC43xx
>> +	  processors.
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called lpc18xx_wdt.
>> +
>>  # AVR32 Architecture
>>  
>>  config AT32AP700X_WDT
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index 59ea9a1..1b0ef48 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -66,6 +66,7 @@ obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
>>  obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o
>>  obj-$(CONFIG_MEDIATEK_WATCHDOG) += mtk_wdt.o
>>  obj-$(CONFIG_DIGICOLOR_WATCHDOG) += digicolor_wdt.o
>> +obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
>>  
>>  # AVR32 Architecture
>>  obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
>> diff --git a/drivers/watchdog/lpc18xx_wdt.c b/drivers/watchdog/lpc18xx_wdt.c
>> new file mode 100644
>> index 0000000..80d68eb
>> --- /dev/null
>> +++ b/drivers/watchdog/lpc18xx_wdt.c
>> @@ -0,0 +1,340 @@
>> +/*
>> + * NXP LPC18xx Windowed Watchdog Timer (WWDT)
>> + *
> 
> What does "Windowed" stand for ? That term doesn't really mean anything
> to me. How does it differ from a non-windowed watchdog driver ?

"NXP LPC18xx Windowed Watchdog Timer" is the full specific name of the
Watchdog, as it's written in the LPC18xx User Manual (UM10430).

"Windowed" means that, besides the maximum, a minimum time-out period
can be set, requiring reload operation (feed) to occur between these two
limits.

Linux watchdog framework doesn't take this property into account, so
"Windowed" term may not have much significance here. Do you think it
should be removed?

> 
>> + * Copyright (c) 2015 Ariel D'Alessandro <ariel@vanguardiasur.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + *
>> + * Notes
>> + * -----
>> + * The Watchdog consists of a fixed divide-by-4 clock pre-scaler and a 24-bit counter
>> + * which decrements on every clock cycle.
>> + *
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reboot.h>
>> +#include <linux/watchdog.h>
>> +
>> +/* Registers */
>> +#define LPC_WDT_MOD			0x00
>> +#define LPC_WDT_MOD_WDEN		BIT(0)
>> +#define LPC_WDT_MOD_WDRESET		BIT(1)
>> +#define LPC_WDT_MOD_WDTOF_MASK		0x04
>> +
>> +#define LPC_WDT_TC			0x04
>> +#define LPC_WDT_TC_MIN			0xff
>> +#define LPC_WDT_TC_MAX			0xffffff
>> +
>> +#define LPC_WDT_FEED			0x08
>> +#define LPC_WDT_FEED_MAGIC1		0xaa
>> +#define LPC_WDT_FEED_MAGIC2		0x55
>> +
>> +#define LPC_WDT_TV			0x0c
>> +
>> +/* Clock pre-scaler */
>> +#define LPC_WDT_CLK_DIV			4
>> +
>> +/* Timeout values in seconds */
>> +#define LPC_WDT_DEF_TIMEOUT		1
>> +
> 
> One second ? This is highly unusual. 30 or 60 seconds is more common,
> and one second would be very challenging for user space.
> 
> Any special reason for using such a tight default ?

Considering that LPC18xx Watchdog has a fixed divide-by-4 clock
pre-scaler and a 24-bit counter and that Watchdog clock runs at a fixed
frequency of 12MHz, timeout range goes from 1 to 5 seconds.

I think you're right, 1 sec is very challenging, so it's 5 secs then.

> 
>> +static int heartbeat;
>> +module_param(heartbeat, int, 0);
>> +MODULE_PARM_DESC(heartbeat, "Watchdog heartbeats in seconds "
>> +	"(default=" __MODULE_STRING(LPC_WDT_DEF_TIMEOUT) ")");
>> +
>> +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) ")");
>> +
>> +struct lpc_wdt_dev {
>> +	struct watchdog_device wdt_dev;
>> +	struct clk *wdt_clk;
>> +	struct clk *sys_clk;
>> +	void __iomem *base;
>> +	struct timer_list timer;
>> +	struct notifier_block restart_handler;
>> +};
>> +
>> +static int lpc_wdt_feed(struct watchdog_device *wdt_dev)
>> +{
>> +	unsigned long flags;
>> +	struct lpc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
>> +
> Please order variables length wise where possible.

Okay, I'll do it in next version.

> 
>> +	raw_local_irq_save(flags);
>> +	writel(LPC_WDT_FEED_MAGIC1, wdt->base + LPC_WDT_FEED);
>> +	writel(LPC_WDT_FEED_MAGIC2, wdt->base + LPC_WDT_FEED);
>> +	raw_local_irq_restore(flags);
>> +
>> +	return 0;
>> +}
>> +
>> +static void lpc_wdt_timer_feed(unsigned long data)
>> +{
>> +	unsigned long flags;
>> +	struct watchdog_device *wdt_dev = (struct watchdog_device *) data;
>> +	struct lpc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
>> +
>> +	/*
>> +	 * An abort condition will occur if an interrupt happens during the feed
>> +	 * sequence.
>> +	 */
>> +	raw_local_irq_save(flags);
>> +	writel(LPC_WDT_FEED_MAGIC1, wdt->base + LPC_WDT_FEED);
>> +	writel(LPC_WDT_FEED_MAGIC2, wdt->base + LPC_WDT_FEED);
>> +	raw_local_irq_restore(flags);
>> +
> Unless I am missing something, 
> 	lpc_wdt_feed(wdt_dev);
> should do the same and reduce code duplication.

Totally right. I'll do it in next version.

> 
>> +	/* Use safe value (1/2 of real timeout) */
>> +	mod_timer(&wdt->timer, jiffies + msecs_to_jiffies((wdt_dev->timeout *
>> +							   MSEC_PER_SEC) / 2));
>> +}
>> +
>> +/*
>> + * Since LPC18xx Watchdog cannot be disabled in hardware, we must keep feeding
>> + * it with a timer until userspace watchdog software takes over.
>> + */
>> +static int lpc_wdt_stop(struct watchdog_device *wdt_dev)
>> +{
>> +	lpc_wdt_timer_feed((unsigned long) wdt_dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static void __lpc_wdt_set_timeout(struct lpc_wdt_dev *wdt)
>> +{
>> +	unsigned long clk_rate = clk_get_rate(wdt->wdt_clk);
>> +	unsigned int val;
>> +
>> +	val = DIV_ROUND_UP(wdt->wdt_dev.timeout * clk_rate, LPC_WDT_CLK_DIV);
>> +	writel(val, wdt->base + LPC_WDT_TC);
>> +}
>> +
>> +static int lpc_wdt_set_timeout(struct watchdog_device *wdt_dev,
>> +			       unsigned int new_timeout)
>> +{
>> +	struct lpc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
>> +
>> +	wdt->wdt_dev.timeout = new_timeout;
>> +	__lpc_wdt_set_timeout(wdt);
>> +
>> +	return 0;
>> +}
>> +
>> +
>> +unsigned int lpc_wdt_get_timeleft(struct watchdog_device *wdt_dev)
>> +{
>> +	struct lpc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
>> +	unsigned long clk_rate = clk_get_rate(wdt->wdt_clk);
>> +	unsigned int val;
>> +
>> +	val = readl(wdt->base + LPC_WDT_TV);
>> +	return ((val * LPC_WDT_CLK_DIV) / clk_rate);
>> +}
>> +
>> +static int lpc_wdt_start(struct watchdog_device *wdt_dev)
>> +{
>> +	unsigned int val;
>> +	struct lpc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
>> +
>> +	if (timer_pending(&wdt->timer))
>> +		del_timer(&wdt->timer);
>> +
>> +	val = readl(wdt->base + LPC_WDT_MOD);
>> +	val |= LPC_WDT_MOD_WDEN;
>> +	val |= LPC_WDT_MOD_WDRESET;
>> +	writel(val, wdt->base + LPC_WDT_MOD);
>> +
>> +	/*
>> +	 * Setting the WDEN bit in the WDMOD register is not sufficient to
>> +	 * enable the Watchdog. A valid feed sequence must be completed after
>> +	 * setting WDEN before the Watchdog is capable of generating a reset.
>> +	 */
>> +	lpc_wdt_feed(wdt_dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct watchdog_info lpc_wdt_info = {
>> +	.identity	= "LPC 18XX Watchdog",
>> +	.options	= WDIOF_SETTIMEOUT |
>> +			  WDIOF_KEEPALIVEPING |
>> +			  WDIOF_MAGICCLOSE,
>> +};
>> +
>> +static const struct watchdog_ops lpc_wdt_ops = {
>> +	.owner		= THIS_MODULE,
>> +	.start		= lpc_wdt_start,
>> +	.stop		= lpc_wdt_stop,
>> +	.ping		= lpc_wdt_feed,
>> +	.set_timeout	= lpc_wdt_set_timeout,
>> +	.get_timeleft	= lpc_wdt_get_timeleft,
>> +};
>> +
>> +static int lpc_wdt_restart(struct notifier_block *this, unsigned long mode,
>> +			   void *cmd)
>> +{
>> +	int val;
>> +	unsigned long flags;
>> +	struct lpc_wdt_dev *wdt = container_of(this, struct lpc_wdt_dev,
>> +					       restart_handler);
>> +
>> +	/*
>> +	 * Incorrect feed sequence causes immediate watchdog reset if enabled.
>> +	 */
>> +	raw_local_irq_save(flags);
>> +
>> +	val = readl(wdt->base + LPC_WDT_MOD);
>> +	val |= LPC_WDT_MOD_WDEN;
>> +	val |= LPC_WDT_MOD_WDRESET;
>> +	writel(val, wdt->base + LPC_WDT_MOD);
>> +
>> +	writel(LPC_WDT_FEED_MAGIC1, wdt->base + LPC_WDT_FEED);
>> +	writel(LPC_WDT_FEED_MAGIC2, wdt->base + LPC_WDT_FEED);
>> +
>> +	writel(LPC_WDT_FEED_MAGIC1, wdt->base + LPC_WDT_FEED);
>> +	writel(LPC_WDT_FEED_MAGIC1, wdt->base + LPC_WDT_FEED);
>> +
>> +	raw_local_irq_restore(flags);
>> +
>> +	return NOTIFY_OK;
>> +}
>> +
>> +static int lpc_wdt_probe(struct platform_device *pdev)
>> +{
>> +	int ret;
>> +	unsigned long clk_rate;
>> +	struct resource *res;
>> +	struct lpc_wdt_dev *lpc_wdt;
>> +
>> +	lpc_wdt = devm_kzalloc(&pdev->dev, sizeof(*lpc_wdt), GFP_KERNEL);
>> +	if (!lpc_wdt)
>> +		return -ENOMEM;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	lpc_wdt->base = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(lpc_wdt->base))
>> +		return PTR_ERR(lpc_wdt->base);
>> +
>> +	lpc_wdt->sys_clk = devm_clk_get(&pdev->dev, "sys");
>> +	if (IS_ERR(lpc_wdt->sys_clk)) {
>> +		dev_err(&pdev->dev, "failed to get the sys clock\n");
>> +		return PTR_ERR(lpc_wdt->sys_clk);
>> +	}
>> +
>> +	lpc_wdt->wdt_clk = devm_clk_get(&pdev->dev, "wdt");
>> +	if (IS_ERR(lpc_wdt->wdt_clk)) {
>> +		dev_err(&pdev->dev, "failed to get the wdt clock\n");
>> +		return PTR_ERR(lpc_wdt->wdt_clk);
>> +	}
>> +
>> +	ret = clk_prepare_enable(lpc_wdt->sys_clk);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "could not prepare or enable sys clock\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = clk_prepare_enable(lpc_wdt->wdt_clk);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "could not prepare or enable wdt clock\n");
>> +		goto disable_sys_clk;
>> +	}
>> +
>> +	/* We use the clock rate to calculate timeouts */
>> +	clk_rate = clk_get_rate(lpc_wdt->wdt_clk);
>> +	if (clk_rate == 0) {
>> +		dev_err(&pdev->dev, "failed to get clock rate\n");
>> +		ret = -EINVAL;
>> +		goto disable_wdt_clk;
>> +	}
>> +
>> +	lpc_wdt->wdt_dev.info = &lpc_wdt_info;
>> +	lpc_wdt->wdt_dev.ops = &lpc_wdt_ops;
>> +
>> +	lpc_wdt->wdt_dev.min_timeout =
>> +		DIV_ROUND_UP(LPC_WDT_TC_MIN * LPC_WDT_CLK_DIV, clk_rate);
>> +
>> +	lpc_wdt->wdt_dev.max_timeout =
>> +		(LPC_WDT_TC_MAX * LPC_WDT_CLK_DIV) / clk_rate;
>> +
>> +	lpc_wdt->wdt_dev.parent = &pdev->dev;
>> +	watchdog_set_drvdata(&lpc_wdt->wdt_dev, lpc_wdt);
>> +
>> +	ret = watchdog_init_timeout(&lpc_wdt->wdt_dev, heartbeat, &pdev->dev);
>> +	if (ret < 0)
>> +		lpc_wdt->wdt_dev.timeout = LPC_WDT_DEF_TIMEOUT;
>> +
>> +	__lpc_wdt_set_timeout(lpc_wdt);
>> +
>> +	setup_timer(&lpc_wdt->timer, lpc_wdt_timer_feed,
>> +		    (unsigned long)&lpc_wdt->wdt_dev);
>> +
>> +	watchdog_set_nowayout(&lpc_wdt->wdt_dev, nowayout);
>> +
>> +	platform_set_drvdata(pdev, lpc_wdt);
>> +
>> +	ret = watchdog_register_device(&lpc_wdt->wdt_dev);
>> +	if (ret)
>> +		goto disable_wdt_clk;
>> +
>> +	lpc_wdt->restart_handler.notifier_call = lpc_wdt_restart;
>> +	lpc_wdt->restart_handler.priority = 128;
>> +	ret = register_restart_handler(&lpc_wdt->restart_handler);
>> +	if (ret)
>> +		dev_warn(&pdev->dev, "failed to register restart handler: %d\n",
>> +			 ret);
>> +
>> +	return 0;
>> +
>> +disable_wdt_clk:
>> +	clk_disable_unprepare(lpc_wdt->wdt_clk);
>> +disable_sys_clk:
>> +	clk_disable_unprepare(lpc_wdt->sys_clk);
>> +	return ret;
>> +}
>> +
>> +static void lpc_wdt_shutdown(struct platform_device *pdev)
>> +{
>> +	struct lpc_wdt_dev *lpc_wdt = platform_get_drvdata(pdev);
>> +
>> +	lpc_wdt_stop(&lpc_wdt->wdt_dev);
>> +}
>> +
>> +static int lpc_wdt_remove(struct platform_device *pdev)
>> +{
>> +	struct lpc_wdt_dev *lpc_wdt = platform_get_drvdata(pdev);
>> +
>> +	lpc_wdt_stop(&lpc_wdt->wdt_dev);
> 
> This will keep the timer enabled. It would be interesting to see what
> happens if you build the driver as module and unload it. I think
> it will crash. Can you try ?

Yes, you're right. After module gets unloaded, timer keeps enabled with
a callback that was deallocated from memory.

Since Watchdog cannot be disabled in hardware, it's not going to be
possible to remove the driver without resetting the system. Unless we
could keep a timer set and running outside the module, it won't be
removable.

What do you think?

> 
>> +	watchdog_unregister_device(&lpc_wdt->wdt_dev);
>> +	clk_disable_unprepare(lpc_wdt->wdt_clk);
>> +	clk_disable_unprepare(lpc_wdt->sys_clk);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id lpc_wdt_match[] = {
>> +	{ .compatible = "nxp,lpc18xx-wdt" },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, lpc_wdt_match);
>> +
>> +static struct platform_driver lpc_wdt_driver = {
>> +	.driver = {
>> +		.name = "lpc18xx-wdt",
>> +		.of_match_table	= lpc_wdt_match,
>> +	},
>> +	.probe = lpc_wdt_probe,
>> +	.remove = lpc_wdt_remove,
>> +	.shutdown = lpc_wdt_shutdown,
>> +};
>> +module_platform_driver(lpc_wdt_driver);
>> +
>> +MODULE_AUTHOR("Ariel D'Alessandro <ariel@vanguardiasur.com.ar>");
>> +MODULE_DESCRIPTION("NXP LPC18xx Windowed Watchdog Timer Driver");
>> +MODULE_LICENSE("GPL v2");
>> -- 
>> 1.9.1
>>

Re: [PATCH 1/2] watchdog: NXP LPC18XX Windowed Watchdog Timer Driver

[Guenter Roeck]

On 06/28/2015 11:13 AM, Ariel D'Alessandro wrote:
> Hi Guenter,
> Thanks for your quick reply.
>
My pleasure ....

>>> +/*
>>> + * NXP LPC18xx Windowed Watchdog Timer (WWDT)
>>> + *
>>
>> What does "Windowed" stand for ? That term doesn't really mean anything
>> to me. How does it differ from a non-windowed watchdog driver ?
>
> "NXP LPC18xx Windowed Watchdog Timer" is the full specific name of the
> Watchdog, as it's written in the LPC18xx User Manual (UM10430).
>
> "Windowed" means that, besides the maximum, a minimum time-out period
> can be set, requiring reload operation (feed) to occur between these two
> limits.
>
> Linux watchdog framework doesn't take this property into account, so
> "Windowed" term may not have much significance here. Do you think it
> should be removed?
>
I would suggest to drop the term - without explanation it is just confusing.

>>> +/* Timeout values in seconds */
>>> +#define LPC_WDT_DEF_TIMEOUT		1
>>> +
>>
>> One second ? This is highly unusual. 30 or 60 seconds is more common,
>> and one second would be very challenging for user space.
>>
>> Any special reason for using such a tight default ?
>
> Considering that LPC18xx Watchdog has a fixed divide-by-4 clock
> pre-scaler and a 24-bit counter and that Watchdog clock runs at a fixed
> frequency of 12MHz, timeout range goes from 1 to 5 seconds.
>
> I think you're right, 1 sec is very challenging, so it's 5 secs then.
>
Ultimately you might want to consider a soft timer as backup to the system
timeout. But that can be done later if/when needed.

>>> +
>>> +static int lpc_wdt_remove(struct platform_device *pdev)
>>> +{
>>> +	struct lpc_wdt_dev *lpc_wdt = platform_get_drvdata(pdev);
>>> +
>>> +	lpc_wdt_stop(&lpc_wdt->wdt_dev);
>>
>> This will keep the timer enabled. It would be interesting to see what
>> happens if you build the driver as module and unload it. I think
>> it will crash. Can you try ?
>
> Yes, you're right. After module gets unloaded, timer keeps enabled with
> a callback that was deallocated from memory.
>
> Since Watchdog cannot be disabled in hardware, it's not going to be
> possible to remove the driver without resetting the system. Unless we
> could keep a timer set and running outside the module, it won't be
> removable.
>
> What do you think?
>

My take is that the driver should be loadable as module, which implies
that it must be removable. Whoever actually does remove the driver
is really asking for trouble and doesn't deserve better.

On the other side, there are a few other watchdogs with similar properties.
Maybe we can just take guidance from there. Can you have a look ?
I can look myself, but it may take a few days.

Thanks,
Guenter

Re: [PATCH 1/2] watchdog: NXP LPC18XX Windowed Watchdog Timer Driver

[adalessandro]

El 29/06/15 a las 01:47, Guenter Roeck escibió:
> On 06/28/2015 11:13 AM, Ariel D'Alessandro wrote:
>>>> +/* Timeout values in seconds */
>>>> +#define LPC_WDT_DEF_TIMEOUT        1
>>>> +
>>>
>>> One second ? This is highly unusual. 30 or 60 seconds is more common,
>>> and one second would be very challenging for user space.
>>>
>>> Any special reason for using such a tight default ?
>>
>> Considering that LPC18xx Watchdog has a fixed divide-by-4 clock
>> pre-scaler and a 24-bit counter and that Watchdog clock runs at a fixed
>> frequency of 12MHz, timeout range goes from 1 to 5 seconds.
>>
>> I think you're right, 1 sec is very challenging, so it's 5 secs then.
>>
> Ultimately you might want to consider a soft timer as backup to the system
> timeout. But that can be done later if/when needed.

I understand your point, but just to be sure, what do mean by soft timer?

> 
>>>> +
>>>> +static int lpc_wdt_remove(struct platform_device *pdev)
>>>> +{
>>>> +    struct lpc_wdt_dev *lpc_wdt = platform_get_drvdata(pdev);
>>>> +
>>>> +    lpc_wdt_stop(&lpc_wdt->wdt_dev);
>>>
>>> This will keep the timer enabled. It would be interesting to see what
>>> happens if you build the driver as module and unload it. I think
>>> it will crash. Can you try ?
>>
>> Yes, you're right. After module gets unloaded, timer keeps enabled with
>> a callback that was deallocated from memory.
>>
>> Since Watchdog cannot be disabled in hardware, it's not going to be
>> possible to remove the driver without resetting the system. Unless we
>> could keep a timer set and running outside the module, it won't be
>> removable.
>>
>> What do you think?
>>
> 
> My take is that the driver should be loadable as module, which implies
> that it must be removable. Whoever actually does remove the driver
> is really asking for trouble and doesn't deserve better.
> 
> On the other side, there are a few other watchdogs with similar properties.
> Maybe we can just take guidance from there. Can you have a look ?
> I can look myself, but it may take a few days.

Ok. I'm sending patchset v2 with all these modifications.

> 
> Thanks,
> Guenter
> 

Re: [PATCH 1/2] watchdog: NXP LPC18XX Windowed Watchdog Timer Driver

[Ariel D'Alessandro]

(Sorry, I sent the last mail with an incorrect mail account)

El 01/07/15 a las 08:30, adalessandro escibió:
> 
> El 29/06/15 a las 01:47, Guenter Roeck escibió:
>> On 06/28/2015 11:13 AM, Ariel D'Alessandro wrote:
>>>>> +/* Timeout values in seconds */
>>>>> +#define LPC_WDT_DEF_TIMEOUT        1
>>>>> +
>>>>
>>>> One second ? This is highly unusual. 30 or 60 seconds is more common,
>>>> and one second would be very challenging for user space.
>>>>
>>>> Any special reason for using such a tight default ?
>>>
>>> Considering that LPC18xx Watchdog has a fixed divide-by-4 clock
>>> pre-scaler and a 24-bit counter and that Watchdog clock runs at a fixed
>>> frequency of 12MHz, timeout range goes from 1 to 5 seconds.
>>>
>>> I think you're right, 1 sec is very challenging, so it's 5 secs then.
>>>
>> Ultimately you might want to consider a soft timer as backup to the system
>> timeout. But that can be done later if/when needed.
> 
> I understand your point, but just to be sure, what do mean by soft timer?
> 
>>
>>>>> +
>>>>> +static int lpc_wdt_remove(struct platform_device *pdev)
>>>>> +{
>>>>> +    struct lpc_wdt_dev *lpc_wdt = platform_get_drvdata(pdev);
>>>>> +
>>>>> +    lpc_wdt_stop(&lpc_wdt->wdt_dev);
>>>>
>>>> This will keep the timer enabled. It would be interesting to see what
>>>> happens if you build the driver as module and unload it. I think
>>>> it will crash. Can you try ?
>>>
>>> Yes, you're right. After module gets unloaded, timer keeps enabled with
>>> a callback that was deallocated from memory.
>>>
>>> Since Watchdog cannot be disabled in hardware, it's not going to be
>>> possible to remove the driver without resetting the system. Unless we
>>> could keep a timer set and running outside the module, it won't be
>>> removable.
>>>
>>> What do you think?
>>>
>>
>> My take is that the driver should be loadable as module, which implies
>> that it must be removable. Whoever actually does remove the driver
>> is really asking for trouble and doesn't deserve better.
>>
>> On the other side, there are a few other watchdogs with similar properties.
>> Maybe we can just take guidance from there. Can you have a look ?
>> I can look myself, but it may take a few days.
> 
> Ok. I'm sending patchset v2 with all these modifications.
> 
>>
>> Thanks,
>> Guenter
>>
> --

Re: [PATCH 1/2] watchdog: NXP LPC18XX Windowed Watchdog Timer Driver

[Guenter Roeck]

On 07/01/2015 05:02 AM, Ariel D'Alessandro wrote:
> (Sorry, I sent the last mail with an incorrect mail account)
>
> El 01/07/15 a las 08:30, adalessandro escibió:
>>
>> El 29/06/15 a las 01:47, Guenter Roeck escibió:
>>> On 06/28/2015 11:13 AM, Ariel D'Alessandro wrote:
>>>>>> +/* Timeout values in seconds */
>>>>>> +#define LPC_WDT_DEF_TIMEOUT        1
>>>>>> +
>>>>>
>>>>> One second ? This is highly unusual. 30 or 60 seconds is more common,
>>>>> and one second would be very challenging for user space.
>>>>>
>>>>> Any special reason for using such a tight default ?
>>>>
>>>> Considering that LPC18xx Watchdog has a fixed divide-by-4 clock
>>>> pre-scaler and a 24-bit counter and that Watchdog clock runs at a fixed
>>>> frequency of 12MHz, timeout range goes from 1 to 5 seconds.
>>>>
>>>> I think you're right, 1 sec is very challenging, so it's 5 secs then.
>>>>
>>> Ultimately you might want to consider a soft timer as backup to the system
>>> timeout. But that can be done later if/when needed.
>>
>> I understand your point, but just to be sure, what do mean by soft timer?
>>

A kernel function which pings the watchdog periodically even if the
watchdog is open.

Example: Timeout is set to 30 seconds. Since the HW watchdog times out
earlier than that, it needs to be pinged regularly (eg every 2.5 seconds).
The kernel does that with a timer unless user space does not ping the
watchdog within the configured interval of 30 seconds.

Guenter

Re: [PATCH 1/2] watchdog: NXP LPC18XX Windowed Watchdog Timer Driver

[Wim Van Sebroeck]

Hi Ariel,

> On 07/01/2015 05:02 AM, Ariel D'Alessandro wrote:
> >(Sorry, I sent the last mail with an incorrect mail account)
> >
> >El 01/07/15 a las 08:30, adalessandro escibió:
> >>
> >>El 29/06/15 a las 01:47, Guenter Roeck escibió:
> >>>On 06/28/2015 11:13 AM, Ariel D'Alessandro wrote:
> >>>>>>+/* Timeout values in seconds */
> >>>>>>+#define LPC_WDT_DEF_TIMEOUT        1
> >>>>>>+
> >>>>>
> >>>>>One second ? This is highly unusual. 30 or 60 seconds is more common,
> >>>>>and one second would be very challenging for user space.
> >>>>>
> >>>>>Any special reason for using such a tight default ?
> >>>>
> >>>>Considering that LPC18xx Watchdog has a fixed divide-by-4 clock
> >>>>pre-scaler and a 24-bit counter and that Watchdog clock runs at a fixed
> >>>>frequency of 12MHz, timeout range goes from 1 to 5 seconds.
> >>>>
> >>>>I think you're right, 1 sec is very challenging, so it's 5 secs then.
> >>>>
> >>>Ultimately you might want to consider a soft timer as backup to the 
> >>>system
> >>>timeout. But that can be done later if/when needed.
> >>
> >>I understand your point, but just to be sure, what do mean by soft timer?
> >>
> 
> A kernel function which pings the watchdog periodically even if the
> watchdog is open.
> 
> Example: Timeout is set to 30 seconds. Since the HW watchdog times out
> earlier than that, it needs to be pinged regularly (eg every 2.5 seconds).
> The kernel does that with a timer unless user space does not ping the
> watchdog within the configured interval of 30 seconds.

See at91sam9_wdt.c as an example. I'll add some sample code to linux-watchdog-next also in the very near future.

Kind regards,
Wim.

Re: [PATCH 1/2] watchdog: NXP LPC18XX Windowed Watchdog Timer Driver

[Ezequiel Garcia]

Hi Guenter,

First of all, thanks a lot for your feedback.

On 1 July 2015 at 10:54, Guenter Roeck <linux@roeck-us.net> wrote:
> On 07/01/2015 05:02 AM, Ariel D'Alessandro wrote:
>>
>> (Sorry, I sent the last mail with an incorrect mail account)
>>
>> El 01/07/15 a las 08:30, adalessandro escibió:
>>>
>>>
>>> El 29/06/15 a las 01:47, Guenter Roeck escibió:
>>>>
>>>> On 06/28/2015 11:13 AM, Ariel D'Alessandro wrote:
>>>>>>>
>>>>>>> +/* Timeout values in seconds */
>>>>>>> +#define LPC_WDT_DEF_TIMEOUT        1
>>>>>>> +
>>>>>>
>>>>>>
>>>>>> One second ? This is highly unusual. 30 or 60 seconds is more common,
>>>>>> and one second would be very challenging for user space.
>>>>>>
>>>>>> Any special reason for using such a tight default ?
>>>>>
>>>>>
>>>>> Considering that LPC18xx Watchdog has a fixed divide-by-4 clock
>>>>> pre-scaler and a 24-bit counter and that Watchdog clock runs at a fixed
>>>>> frequency of 12MHz, timeout range goes from 1 to 5 seconds.
>>>>>
>>>>> I think you're right, 1 sec is very challenging, so it's 5 secs then.
>>>>>
>>>> Ultimately you might want to consider a soft timer as backup to the
>>>> system
>>>> timeout. But that can be done later if/when needed.
>>>
>>>
>>> I understand your point, but just to be sure, what do mean by soft timer?
>>>
>
> A kernel function which pings the watchdog periodically even if the
> watchdog is open.
>
> Example: Timeout is set to 30 seconds. Since the HW watchdog times out
> earlier than that, it needs to be pinged regularly (eg every 2.5 seconds).
> The kernel does that with a timer unless user space does not ping the
> watchdog within the configured interval of 30 seconds.
>

Do we really need this? It sounds like bloat to me. Considering this watchdog
controller is included in cortex-M MCUs, you wouldn't expect the
scheduler to be under so much pressure.

I realize that 1-5 seconds is challenging for userspace, but having a soft timer
in the kernel side sounds like making the system heavier instead of lighter.

Not sure I'm making sense here, but I would think twice before adding bloat.

Maybe Ariel can submit his v2 (with basic watchdog support, which BTW also
provides a reset handler) now and we can pospone this discussion.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

[PATCH 1/2] watchdog: NXP LPC18XX Windowed Watchdog Timer Driver

[Ezequiel Garcia]

Hi Guenter,

First of all, thanks a lot for your feedback.

On 1 July 2015 at 10:54, Guenter Roeck <linux@roeck-us.net> wrote:
> On 07/01/2015 05:02 AM, Ariel D'Alessandro wrote:
>>
>> (Sorry, I sent the last mail with an incorrect mail account)
>>
>> El 01/07/15 a las 08:30, adalessandro escibi?:
>>>
>>>
>>> El 29/06/15 a las 01:47, Guenter Roeck escibi?:
>>>>
>>>> On 06/28/2015 11:13 AM, Ariel D'Alessandro wrote:
>>>>>>>
>>>>>>> +/* Timeout values in seconds */
>>>>>>> +#define LPC_WDT_DEF_TIMEOUT        1
>>>>>>> +
>>>>>>
>>>>>>
>>>>>> One second ? This is highly unusual. 30 or 60 seconds is more common,
>>>>>> and one second would be very challenging for user space.
>>>>>>
>>>>>> Any special reason for using such a tight default ?
>>>>>
>>>>>
>>>>> Considering that LPC18xx Watchdog has a fixed divide-by-4 clock
>>>>> pre-scaler and a 24-bit counter and that Watchdog clock runs at a fixed
>>>>> frequency of 12MHz, timeout range goes from 1 to 5 seconds.
>>>>>
>>>>> I think you're right, 1 sec is very challenging, so it's 5 secs then.
>>>>>
>>>> Ultimately you might want to consider a soft timer as backup to the
>>>> system
>>>> timeout. But that can be done later if/when needed.
>>>
>>>
>>> I understand your point, but just to be sure, what do mean by soft timer?
>>>
>
> A kernel function which pings the watchdog periodically even if the
> watchdog is open.
>
> Example: Timeout is set to 30 seconds. Since the HW watchdog times out
> earlier than that, it needs to be pinged regularly (eg every 2.5 seconds).
> The kernel does that with a timer unless user space does not ping the
> watchdog within the configured interval of 30 seconds.
>

Do we really need this? It sounds like bloat to me. Considering this watchdog
controller is included in cortex-M MCUs, you wouldn't expect the
scheduler to be under so much pressure.

I realize that 1-5 seconds is challenging for userspace, but having a soft timer
in the kernel side sounds like making the system heavier instead of lighter.

Not sure I'm making sense here, but I would think twice before adding bloat.

Maybe Ariel can submit his v2 (with basic watchdog support, which BTW also
provides a reset handler) now and we can pospone this discussion.
-- 
Ezequiel Garc?a, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 1/2] watchdog: NXP LPC18XX Windowed Watchdog Timer Driver

[Guenter Roeck]

On Wed, Jul 01, 2015 at 12:22:12PM -0300, Ezequiel Garcia wrote:
> Hi Guenter,
> 
> First of all, thanks a lot for your feedback.
> 
> On 1 July 2015 at 10:54, Guenter Roeck <linux@roeck-us.net> wrote:
> > On 07/01/2015 05:02 AM, Ariel D'Alessandro wrote:
> >>
> >> (Sorry, I sent the last mail with an incorrect mail account)
> >>
> >> El 01/07/15 a las 08:30, adalessandro escibió:
> >>>
> >>>
> >>> El 29/06/15 a las 01:47, Guenter Roeck escibió:
> >>>>
> >>>> On 06/28/2015 11:13 AM, Ariel D'Alessandro wrote:
> >>>>>>>
> >>>>>>> +/* Timeout values in seconds */
> >>>>>>> +#define LPC_WDT_DEF_TIMEOUT        1
> >>>>>>> +
> >>>>>>
> >>>>>>
> >>>>>> One second ? This is highly unusual. 30 or 60 seconds is more common,
> >>>>>> and one second would be very challenging for user space.
> >>>>>>
> >>>>>> Any special reason for using such a tight default ?
> >>>>>
> >>>>>
> >>>>> Considering that LPC18xx Watchdog has a fixed divide-by-4 clock
> >>>>> pre-scaler and a 24-bit counter and that Watchdog clock runs at a fixed
> >>>>> frequency of 12MHz, timeout range goes from 1 to 5 seconds.
> >>>>>
> >>>>> I think you're right, 1 sec is very challenging, so it's 5 secs then.
> >>>>>
> >>>> Ultimately you might want to consider a soft timer as backup to the
> >>>> system
> >>>> timeout. But that can be done later if/when needed.
> >>>
> >>>
> >>> I understand your point, but just to be sure, what do mean by soft timer?
> >>>
> >
> > A kernel function which pings the watchdog periodically even if the
> > watchdog is open.
> >
> > Example: Timeout is set to 30 seconds. Since the HW watchdog times out
> > earlier than that, it needs to be pinged regularly (eg every 2.5 seconds).
> > The kernel does that with a timer unless user space does not ping the
> > watchdog within the configured interval of 30 seconds.
> >
> 
> Do we really need this? It sounds like bloat to me. Considering this watchdog
> controller is included in cortex-M MCUs, you wouldn't expect the
> scheduler to be under so much pressure.
> 
No, this would be your call. There are also plans to add this into the watchdog
core, which is really where it should be.

Guenter

[PATCH 1/2] watchdog: NXP LPC18XX Windowed Watchdog Timer Driver

[Guenter Roeck]

On Wed, Jul 01, 2015 at 12:22:12PM -0300, Ezequiel Garcia wrote:
> Hi Guenter,
> 
> First of all, thanks a lot for your feedback.
> 
> On 1 July 2015 at 10:54, Guenter Roeck <linux@roeck-us.net> wrote:
> > On 07/01/2015 05:02 AM, Ariel D'Alessandro wrote:
> >>
> >> (Sorry, I sent the last mail with an incorrect mail account)
> >>
> >> El 01/07/15 a las 08:30, adalessandro escibi?:
> >>>
> >>>
> >>> El 29/06/15 a las 01:47, Guenter Roeck escibi?:
> >>>>
> >>>> On 06/28/2015 11:13 AM, Ariel D'Alessandro wrote:
> >>>>>>>
> >>>>>>> +/* Timeout values in seconds */
> >>>>>>> +#define LPC_WDT_DEF_TIMEOUT        1
> >>>>>>> +
> >>>>>>
> >>>>>>
> >>>>>> One second ? This is highly unusual. 30 or 60 seconds is more common,
> >>>>>> and one second would be very challenging for user space.
> >>>>>>
> >>>>>> Any special reason for using such a tight default ?
> >>>>>
> >>>>>
> >>>>> Considering that LPC18xx Watchdog has a fixed divide-by-4 clock
> >>>>> pre-scaler and a 24-bit counter and that Watchdog clock runs at a fixed
> >>>>> frequency of 12MHz, timeout range goes from 1 to 5 seconds.
> >>>>>
> >>>>> I think you're right, 1 sec is very challenging, so it's 5 secs then.
> >>>>>
> >>>> Ultimately you might want to consider a soft timer as backup to the
> >>>> system
> >>>> timeout. But that can be done later if/when needed.
> >>>
> >>>
> >>> I understand your point, but just to be sure, what do mean by soft timer?
> >>>
> >
> > A kernel function which pings the watchdog periodically even if the
> > watchdog is open.
> >
> > Example: Timeout is set to 30 seconds. Since the HW watchdog times out
> > earlier than that, it needs to be pinged regularly (eg every 2.5 seconds).
> > The kernel does that with a timer unless user space does not ping the
> > watchdog within the configured interval of 30 seconds.
> >
> 
> Do we really need this? It sounds like bloat to me. Considering this watchdog
> controller is included in cortex-M MCUs, you wouldn't expect the
> scheduler to be under so much pressure.
> 
No, this would be your call. There are also plans to add this into the watchdog
core, which is really where it should be.

Guenter