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

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

[Ariel D'Alessandro]

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.

Changes since v1:
* Dropped "Windowed" term in Watchdog's name.
* Changed default timeout value from 1 to 5 because it was too tight.
* Fixed module remove function, since it was keeping the timer enabled. Warning
  message was added.
* Changed dt compatible string to "nxp,lpc1850-wdt".
* Renamed clocks matching most of the other lpc18xx drivers.

Ariel D'Alessandro (2):
  watchdog: NXP LPC18xx 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                     | 342 +++++++++++++++++++++
 4 files changed, 373 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/lpc18xx-wdt.txt
 create mode 100644 drivers/watchdog/lpc18xx_wdt.c

-- 
1.9.1

[PATCH v2 1/2] watchdog: NXP LPC18xx 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 | 342 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 354 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..ddc5403
--- /dev/null
+++ b/drivers/watchdog/lpc18xx_wdt.c
@@ -0,0 +1,342 @@
+/*
+ * NXP LPC18xx Watchdog Timer (WDT)
+ *
+ * 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 LPC18XX_WDT_MOD			0x00
+#define LPC18XX_WDT_MOD_WDEN		BIT(0)
+#define LPC18XX_WDT_MOD_WDRESET		BIT(1)
+#define LPC18XX_WDT_MOD_WDTOF_MASK	0x04
+
+#define LPC18XX_WDT_TC			0x04
+#define LPC18XX_WDT_TC_MIN		0xff
+#define LPC18XX_WDT_TC_MAX		0xffffff
+
+#define LPC18XX_WDT_FEED		0x08
+#define LPC18XX_WDT_FEED_MAGIC1		0xaa
+#define LPC18XX_WDT_FEED_MAGIC2		0x55
+
+#define LPC18XX_WDT_TV			0x0c
+
+/* Clock pre-scaler */
+#define LPC18XX_WDT_CLK_DIV		4
+
+/* Timeout values in seconds */
+#define LPC18XX_WDT_DEF_TIMEOUT		5
+
+static int heartbeat;
+module_param(heartbeat, int, 0);
+MODULE_PARM_DESC(heartbeat, "Watchdog heartbeats in seconds "
+	"(default=" __MODULE_STRING(LPC18XX_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 lpc18xx_wdt_dev {
+	struct watchdog_device wdt_dev;
+	struct clk *wdt_clk;
+	struct clk *reg_clk;
+	void __iomem *base;
+	struct timer_list timer;
+	struct notifier_block restart_handler;
+};
+
+static int lpc18xx_wdt_feed(struct watchdog_device *wdt_dev)
+{
+	struct lpc18xx_wdt_dev *lpc18xx_wdt = watchdog_get_drvdata(wdt_dev);
+	unsigned long flags;
+
+	/*
+	 * An abort condition will occur if an interrupt happens during the feed
+	 * sequence.
+	 */
+	raw_local_irq_save(flags);
+	writel(LPC18XX_WDT_FEED_MAGIC1, lpc18xx_wdt->base + LPC18XX_WDT_FEED);
+	writel(LPC18XX_WDT_FEED_MAGIC2, lpc18xx_wdt->base + LPC18XX_WDT_FEED);
+	raw_local_irq_restore(flags);
+
+	return 0;
+}
+
+static void lpc18xx_wdt_timer_feed(unsigned long data)
+{
+	struct watchdog_device *wdt_dev = (struct watchdog_device *) data;
+	struct lpc18xx_wdt_dev *lpc18xx_wdt = watchdog_get_drvdata(wdt_dev);
+
+	lpc18xx_wdt_feed(wdt_dev);
+
+	/* Use safe value (1/2 of real timeout) */
+	mod_timer(&lpc18xx_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 lpc18xx_wdt_stop(struct watchdog_device *wdt_dev)
+{
+	lpc18xx_wdt_timer_feed((unsigned long) wdt_dev);
+
+	return 0;
+}
+
+static void __lpc18xx_wdt_set_timeout(struct lpc18xx_wdt_dev *lpc18xx_wdt)
+{
+	unsigned long clk_rate = clk_get_rate(lpc18xx_wdt->wdt_clk);
+	unsigned int val;
+
+	val = DIV_ROUND_UP(lpc18xx_wdt->wdt_dev.timeout * clk_rate,
+			   LPC18XX_WDT_CLK_DIV);
+	writel(val, lpc18xx_wdt->base + LPC18XX_WDT_TC);
+}
+
+static int lpc18xx_wdt_set_timeout(struct watchdog_device *wdt_dev,
+				   unsigned int new_timeout)
+{
+	struct lpc18xx_wdt_dev *lpc18xx_wdt = watchdog_get_drvdata(wdt_dev);
+
+	lpc18xx_wdt->wdt_dev.timeout = new_timeout;
+	__lpc18xx_wdt_set_timeout(lpc18xx_wdt);
+
+	return 0;
+}
+
+
+unsigned int lpc18xx_wdt_get_timeleft(struct watchdog_device *wdt_dev)
+{
+	struct lpc18xx_wdt_dev *lpc18xx_wdt = watchdog_get_drvdata(wdt_dev);
+	unsigned long clk_rate = clk_get_rate(lpc18xx_wdt->wdt_clk);
+	unsigned int val;
+
+	val = readl(lpc18xx_wdt->base + LPC18XX_WDT_TV);
+	return ((val * LPC18XX_WDT_CLK_DIV) / clk_rate);
+}
+
+static int lpc18xx_wdt_start(struct watchdog_device *wdt_dev)
+{
+	struct lpc18xx_wdt_dev *lpc18xx_wdt = watchdog_get_drvdata(wdt_dev);
+	unsigned int val;
+
+	if (timer_pending(&lpc18xx_wdt->timer))
+		del_timer(&lpc18xx_wdt->timer);
+
+	val = readl(lpc18xx_wdt->base + LPC18XX_WDT_MOD);
+	val |= LPC18XX_WDT_MOD_WDEN;
+	val |= LPC18XX_WDT_MOD_WDRESET;
+	writel(val, lpc18xx_wdt->base + LPC18XX_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.
+	 */
+	lpc18xx_wdt_feed(wdt_dev);
+
+	return 0;
+}
+
+static struct watchdog_info lpc18xx_wdt_info = {
+	.identity	= "NXP LPC18xx Watchdog",
+	.options	= WDIOF_SETTIMEOUT |
+			  WDIOF_KEEPALIVEPING |
+			  WDIOF_MAGICCLOSE,
+};
+
+static const struct watchdog_ops lpc18xx_wdt_ops = {
+	.owner		= THIS_MODULE,
+	.start		= lpc18xx_wdt_start,
+	.stop		= lpc18xx_wdt_stop,
+	.ping		= lpc18xx_wdt_feed,
+	.set_timeout	= lpc18xx_wdt_set_timeout,
+	.get_timeleft	= lpc18xx_wdt_get_timeleft,
+};
+
+static int lpc18xx_wdt_restart(struct notifier_block *this, unsigned long mode,
+			       void *cmd)
+{
+	struct lpc18xx_wdt_dev *lpc18xx_wdt = container_of(this,
+				struct lpc18xx_wdt_dev, restart_handler);
+	unsigned long flags;
+	int val;
+
+	/*
+	 * Incorrect feed sequence causes immediate watchdog reset if enabled.
+	 */
+	raw_local_irq_save(flags);
+
+	val = readl(lpc18xx_wdt->base + LPC18XX_WDT_MOD);
+	val |= LPC18XX_WDT_MOD_WDEN;
+	val |= LPC18XX_WDT_MOD_WDRESET;
+	writel(val, lpc18xx_wdt->base + LPC18XX_WDT_MOD);
+
+	writel(LPC18XX_WDT_FEED_MAGIC1, lpc18xx_wdt->base + LPC18XX_WDT_FEED);
+	writel(LPC18XX_WDT_FEED_MAGIC2, lpc18xx_wdt->base + LPC18XX_WDT_FEED);
+
+	writel(LPC18XX_WDT_FEED_MAGIC1, lpc18xx_wdt->base + LPC18XX_WDT_FEED);
+	writel(LPC18XX_WDT_FEED_MAGIC1, lpc18xx_wdt->base + LPC18XX_WDT_FEED);
+
+	raw_local_irq_restore(flags);
+
+	return NOTIFY_OK;
+}
+
+static int lpc18xx_wdt_probe(struct platform_device *pdev)
+{
+	struct lpc18xx_wdt_dev *lpc18xx_wdt;
+	unsigned long clk_rate;
+	struct resource *res;
+	int ret;
+
+	lpc18xx_wdt = devm_kzalloc(&pdev->dev, sizeof(*lpc18xx_wdt),
+				   GFP_KERNEL);
+	if (!lpc18xx_wdt)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	lpc18xx_wdt->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(lpc18xx_wdt->base))
+		return PTR_ERR(lpc18xx_wdt->base);
+
+	lpc18xx_wdt->reg_clk = devm_clk_get(&pdev->dev, "reg");
+	if (IS_ERR(lpc18xx_wdt->reg_clk)) {
+		dev_err(&pdev->dev, "failed to get the reg clock\n");
+		return PTR_ERR(lpc18xx_wdt->reg_clk);
+	}
+
+	lpc18xx_wdt->wdt_clk = devm_clk_get(&pdev->dev, "wdtclk");
+	if (IS_ERR(lpc18xx_wdt->wdt_clk)) {
+		dev_err(&pdev->dev, "failed to get the wdt clock\n");
+		return PTR_ERR(lpc18xx_wdt->wdt_clk);
+	}
+
+	ret = clk_prepare_enable(lpc18xx_wdt->reg_clk);
+	if (ret) {
+		dev_err(&pdev->dev, "could not prepare or enable sys clock\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(lpc18xx_wdt->wdt_clk);
+	if (ret) {
+		dev_err(&pdev->dev, "could not prepare or enable wdt clock\n");
+		goto disable_reg_clk;
+	}
+
+	/* We use the clock rate to calculate timeouts */
+	clk_rate = clk_get_rate(lpc18xx_wdt->wdt_clk);
+	if (clk_rate == 0) {
+		dev_err(&pdev->dev, "failed to get clock rate\n");
+		ret = -EINVAL;
+		goto disable_wdt_clk;
+	}
+
+	lpc18xx_wdt->wdt_dev.info = &lpc18xx_wdt_info;
+	lpc18xx_wdt->wdt_dev.ops = &lpc18xx_wdt_ops;
+	lpc18xx_wdt->wdt_dev.timeout = LPC18XX_WDT_DEF_TIMEOUT;
+
+	lpc18xx_wdt->wdt_dev.min_timeout = DIV_ROUND_UP(LPC18XX_WDT_TC_MIN *
+						LPC18XX_WDT_CLK_DIV, clk_rate);
+
+	lpc18xx_wdt->wdt_dev.max_timeout = (LPC18XX_WDT_TC_MAX *
+					    LPC18XX_WDT_CLK_DIV) / clk_rate;
+
+	lpc18xx_wdt->wdt_dev.parent = &pdev->dev;
+	watchdog_set_drvdata(&lpc18xx_wdt->wdt_dev, lpc18xx_wdt);
+
+	ret = watchdog_init_timeout(&lpc18xx_wdt->wdt_dev, heartbeat,
+				    &pdev->dev);
+
+	__lpc18xx_wdt_set_timeout(lpc18xx_wdt);
+
+	setup_timer(&lpc18xx_wdt->timer, lpc18xx_wdt_timer_feed,
+		    (unsigned long)&lpc18xx_wdt->wdt_dev);
+
+	watchdog_set_nowayout(&lpc18xx_wdt->wdt_dev, nowayout);
+
+	platform_set_drvdata(pdev, lpc18xx_wdt);
+
+	ret = watchdog_register_device(&lpc18xx_wdt->wdt_dev);
+	if (ret)
+		goto disable_wdt_clk;
+
+	lpc18xx_wdt->restart_handler.notifier_call = lpc18xx_wdt_restart;
+	lpc18xx_wdt->restart_handler.priority = 128;
+	ret = register_restart_handler(&lpc18xx_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(lpc18xx_wdt->wdt_clk);
+disable_reg_clk:
+	clk_disable_unprepare(lpc18xx_wdt->reg_clk);
+	return ret;
+}
+
+static void lpc18xx_wdt_shutdown(struct platform_device *pdev)
+{
+	struct lpc18xx_wdt_dev *lpc18xx_wdt = platform_get_drvdata(pdev);
+
+	lpc18xx_wdt_stop(&lpc18xx_wdt->wdt_dev);
+}
+
+static int lpc18xx_wdt_remove(struct platform_device *pdev)
+{
+	struct lpc18xx_wdt_dev *lpc18xx_wdt = platform_get_drvdata(pdev);
+
+	unregister_restart_handler(&lpc18xx_wdt->restart_handler);
+
+	dev_warn(&pdev->dev, "I quit now, hardware will probably reboot!\n");
+	del_timer(&lpc18xx_wdt->timer);
+
+	watchdog_unregister_device(&lpc18xx_wdt->wdt_dev);
+	clk_disable_unprepare(lpc18xx_wdt->wdt_clk);
+	clk_disable_unprepare(lpc18xx_wdt->reg_clk);
+
+	return 0;
+}
+
+static const struct of_device_id lpc18xx_wdt_match[] = {
+	{ .compatible = "nxp,lpc1850-wdt" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, lpc18xx_wdt_match);
+
+static struct platform_driver lpc18xx_wdt_driver = {
+	.driver = {
+		.name = "lpc18xx-wdt",
+		.of_match_table	= lpc18xx_wdt_match,
+	},
+	.probe = lpc18xx_wdt_probe,
+	.remove = lpc18xx_wdt_remove,
+	.shutdown = lpc18xx_wdt_shutdown,
+};
+module_platform_driver(lpc18xx_wdt_driver);
+
+MODULE_AUTHOR("Ariel D'Alessandro <ariel@vanguardiasur.com.ar>");
+MODULE_DESCRIPTION("NXP LPC18xx Watchdog Timer Driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH v2 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver

[Joachim Eastwood]

Hi Ariel,

On 1 July 2015 at 22:52, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> 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 | 342 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 354 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"

LPC18xx/43xx in tristate to indicate that is usable for both families.

> +       depends on ARCH_LPC18XX
> +       select WATCHDOG_CORE

Maybe add COMPILE_TEST. At least a couple of other watchdog drivers does that.

> +       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.
> +
[...]
> +++ b/drivers/watchdog/lpc18xx_wdt.c
> @@ -0,0 +1,342 @@
> +/*
> + * NXP LPC18xx Watchdog Timer (WDT)

I think you should use "Windowed Watchdog Timer (WWDT)" here since
that is name from the user manual, even though the driver doesn't have
window functionality.
At least use wwdt for the compatible string, ie: "nxp,lpc1850-wwdt".

I don't think Guenter asked you to remove the "Windowed" part last
time, but only asked what it meant.
http://www.maximintegrated.com/en/glossary/definitions.mvp/term/Window%20Watchdog/gpk/337

There seem to be one other window type wdt in drivers/watchdog: da9062_wdt.

> +static int lpc18xx_wdt_feed(struct watchdog_device *wdt_dev)
> +{
> +       struct lpc18xx_wdt_dev *lpc18xx_wdt = watchdog_get_drvdata(wdt_dev);
> +       unsigned long flags;
> +
> +       /*
> +        * An abort condition will occur if an interrupt happens during the feed
> +        * sequence.
> +        */
> +       raw_local_irq_save(flags);

Any particular reason why you are using the raw variant of
local_irq_save/restore?
raw_local_irq_save doesn't seem to be a common function for drivers to
call at all.

If you need a lock to protect register access as well you could use a
spin lock with irq save.

> +       writel(LPC18XX_WDT_FEED_MAGIC1, lpc18xx_wdt->base + LPC18XX_WDT_FEED);
> +       writel(LPC18XX_WDT_FEED_MAGIC2, lpc18xx_wdt->base + LPC18XX_WDT_FEED);
> +       raw_local_irq_restore(flags);
> +
> +       return 0;
> +}
[...]
> +unsigned int lpc18xx_wdt_get_timeleft(struct watchdog_device *wdt_dev)
> +{
> +       struct lpc18xx_wdt_dev *lpc18xx_wdt = watchdog_get_drvdata(wdt_dev);
> +       unsigned long clk_rate = clk_get_rate(lpc18xx_wdt->wdt_clk);
> +       unsigned int val;
> +
> +       val = readl(lpc18xx_wdt->base + LPC18XX_WDT_TV);
> +       return ((val * LPC18XX_WDT_CLK_DIV) / clk_rate);

Outer parentheses on return is unnecessary.

> +}
> +
> +static int lpc18xx_wdt_start(struct watchdog_device *wdt_dev)
> +{
> +       struct lpc18xx_wdt_dev *lpc18xx_wdt = watchdog_get_drvdata(wdt_dev);
> +       unsigned int val;
> +
> +       if (timer_pending(&lpc18xx_wdt->timer))
> +               del_timer(&lpc18xx_wdt->timer);
> +
> +       val = readl(lpc18xx_wdt->base + LPC18XX_WDT_MOD);
> +       val |= LPC18XX_WDT_MOD_WDEN;
> +       val |= LPC18XX_WDT_MOD_WDRESET;
> +       writel(val, lpc18xx_wdt->base + LPC18XX_WDT_MOD);

R-M-W sequence should probably be protect with a lock.

> +       /*
> +        * 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.
> +        */
> +       lpc18xx_wdt_feed(wdt_dev);
> +
> +       return 0;
> +}


regards,
Joachim Eastwood

[PATCH v2 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver

[Guenter Roeck]

On 07/01/2015 04:04 PM, Joachim Eastwood wrote:
> Hi Ariel,
>

>> +/*
>> + * NXP LPC18xx Watchdog Timer (WDT)
>
> I think you should use "Windowed Watchdog Timer (WWDT)" here since
> that is name from the user manual, even though the driver doesn't have
> window functionality.
> At least use wwdt for the compatible string, ie: "nxp,lpc1850-wwdt".
>
> I don't think Guenter asked you to remove the "Windowed" part last
> time, but only asked what it meant.

I did, separately, but I don't want to start a war of words either.
No one will know what that term means, and I am personally not too hot
having to explain it to others.

If you insist using the term, please add a comment into the code
explaining what it is and that the driver does not support it.

Guenter

[PATCH v2 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver

[Joachim Eastwood]

On 2 July 2015 at 02:14, Guenter Roeck <linux@roeck-us.net> wrote:
> On 07/01/2015 04:04 PM, Joachim Eastwood wrote:
>>
>> Hi Ariel,
>>
>
>>> +/*
>>> + * NXP LPC18xx Watchdog Timer (WDT)
>>
>>
>> I think you should use "Windowed Watchdog Timer (WWDT)" here since
>> that is name from the user manual, even though the driver doesn't have
>> window functionality.
>> At least use wwdt for the compatible string, ie: "nxp,lpc1850-wwdt".
>>
>> I don't think Guenter asked you to remove the "Windowed" part last
>> time, but only asked what it meant.
>
>
> I did, separately, but I don't want to start a war of words either.
> No one will know what that term means, and I am personally not too hot
> having to explain it to others.

Fine.

My only request then is to make the compatible string
"nxp,lpc1850-wwdt", to leave the WWDT name somewhere. As this is the
name you will find through out the user manual for this hw block.


regards,
Joachim Eastwood


> If you insist using the term, please add a comment into the code
> explaining what it is and that the driver does not support it.
>
> Guenter
>

[PATCH v2 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver

[Ariel D'Alessandro]

El 02/07/15 a las 06:03, Joachim Eastwood escibi?:
> On 2 July 2015 at 02:14, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 07/01/2015 04:04 PM, Joachim Eastwood wrote:
>>>
>>> Hi Ariel,
>>>
>>
>>>> +/*
>>>> + * NXP LPC18xx Watchdog Timer (WDT)
>>>
>>>
>>> I think you should use "Windowed Watchdog Timer (WWDT)" here since
>>> that is name from the user manual, even though the driver doesn't have
>>> window functionality.
>>> At least use wwdt for the compatible string, ie: "nxp,lpc1850-wwdt".
>>>
>>> I don't think Guenter asked you to remove the "Windowed" part last
>>> time, but only asked what it meant.
>>
>>
>> I did, separately, but I don't want to start a war of words either.
>> No one will know what that term means, and I am personally not too hot
>> having to explain it to others.
> 
> Fine.
> 
> My only request then is to make the compatible string
> "nxp,lpc1850-wwdt", to leave the WWDT name somewhere. As this is the
> name you will find through out the user manual for this hw block.

Well, then I'll just change the compatible string to "nxp,lpc1850-wwdt".

[PATCH v2 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver

[Ezequiel Garcia]

Hi Joachim,

On 1 July 2015 at 20:04, Joachim  Eastwood <manabian@gmail.com> wrote:
[..]
>> +static int lpc18xx_wdt_start(struct watchdog_device *wdt_dev)
>> +{
>> +       struct lpc18xx_wdt_dev *lpc18xx_wdt = watchdog_get_drvdata(wdt_dev);
>> +       unsigned int val;
>> +
>> +       if (timer_pending(&lpc18xx_wdt->timer))
>> +               del_timer(&lpc18xx_wdt->timer);
>> +
>> +       val = readl(lpc18xx_wdt->base + LPC18XX_WDT_MOD);
>> +       val |= LPC18XX_WDT_MOD_WDEN;
>> +       val |= LPC18XX_WDT_MOD_WDRESET;
>> +       writel(val, lpc18xx_wdt->base + LPC18XX_WDT_MOD);
>
> R-M-W sequence should probably be protect with a lock.
>

Notice the watchdog framework serializes this access (see the mutex in
watchdog_dev.c). So we only need protection for the registers
accessed in the timer and the restart handler.

-- 
Ezequiel Garc?a, VanguardiaSur
www.vanguardiasur.com.ar

[PATCH v2 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver

[Ariel D'Alessandro]

Hi Joachim,

El 01/07/15 a las 20:04, Joachim Eastwood escibi?:
> Hi Ariel,
> 
> On 1 July 2015 at 22:52, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
>> 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"
> 
> LPC18xx/43xx in tristate to indicate that is usable for both families.

Ok, I'll do that.

> 
>> +       depends on ARCH_LPC18XX
>> +       select WATCHDOG_CORE
> 
> Maybe add COMPILE_TEST. At least a couple of other watchdog drivers does that.

Yes, that's correct. I'll add it in the next version.

> 
>> +       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.
>> +
> [...]
>> +static int lpc18xx_wdt_feed(struct watchdog_device *wdt_dev)
>> +{
>> +       struct lpc18xx_wdt_dev *lpc18xx_wdt = watchdog_get_drvdata(wdt_dev);
>> +       unsigned long flags;
>> +
>> +       /*
>> +        * An abort condition will occur if an interrupt happens during the feed
>> +        * sequence.
>> +        */
>> +       raw_local_irq_save(flags);
> 
> Any particular reason why you are using the raw variant of
> local_irq_save/restore?
> raw_local_irq_save doesn't seem to be a common function for drivers to
> call at all.

No, you're right, raw variants don't seem to be necessary.

> 
> If you need a lock to protect register access as well you could use a
> spin lock with irq save.

Yes but, as Ezequiel said in his response, the watchdog framework
serializes this access. So I think a lock isn't necessary.

> 
>> +       writel(LPC18XX_WDT_FEED_MAGIC1, lpc18xx_wdt->base + LPC18XX_WDT_FEED);
>> +       writel(LPC18XX_WDT_FEED_MAGIC2, lpc18xx_wdt->base + LPC18XX_WDT_FEED);
>> +       raw_local_irq_restore(flags);
>> +
>> +       return 0;
>> +}
> [...]
>> +unsigned int lpc18xx_wdt_get_timeleft(struct watchdog_device *wdt_dev)
>> +{
>> +       struct lpc18xx_wdt_dev *lpc18xx_wdt = watchdog_get_drvdata(wdt_dev);
>> +       unsigned long clk_rate = clk_get_rate(lpc18xx_wdt->wdt_clk);
>> +       unsigned int val;
>> +
>> +       val = readl(lpc18xx_wdt->base + LPC18XX_WDT_TV);
>> +       return ((val * LPC18XX_WDT_CLK_DIV) / clk_rate);
> 
> Outer parentheses on return is unnecessary.

You're right, I'll fix it.

> 
>> +}
>> +
>> +static int lpc18xx_wdt_start(struct watchdog_device *wdt_dev)
>> +{
>> +       struct lpc18xx_wdt_dev *lpc18xx_wdt = watchdog_get_drvdata(wdt_dev);
>> +       unsigned int val;
>> +
>> +       if (timer_pending(&lpc18xx_wdt->timer))
>> +               del_timer(&lpc18xx_wdt->timer);
>> +
>> +       val = readl(lpc18xx_wdt->base + LPC18XX_WDT_MOD);
>> +       val |= LPC18XX_WDT_MOD_WDEN;
>> +       val |= LPC18XX_WDT_MOD_WDRESET;
>> +       writel(val, lpc18xx_wdt->base + LPC18XX_WDT_MOD);
> 
> R-M-W sequence should probably be protect with a lock.

We can continue discussing this point on Ezequiel's response.

> 
>> +       /*
>> +        * 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.
>> +        */
>> +       lpc18xx_wdt_feed(wdt_dev);
>> +
>> +       return 0;
>> +}
> 
> 
> regards,
> Joachim Eastwood
> 

[PATCH v2 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver

[Guenter Roeck]

On 07/04/2015 07:06 PM, Ariel D'Alessandro wrote:
> Hi Joachim,
>
[ ... ]
>>> +
>>> +static int lpc18xx_wdt_start(struct watchdog_device *wdt_dev)
>>> +{
>>> +       struct lpc18xx_wdt_dev *lpc18xx_wdt = watchdog_get_drvdata(wdt_dev);
>>> +       unsigned int val;
>>> +
>>> +       if (timer_pending(&lpc18xx_wdt->timer))
>>> +               del_timer(&lpc18xx_wdt->timer);
>>> +
>>> +       val = readl(lpc18xx_wdt->base + LPC18XX_WDT_MOD);
>>> +       val |= LPC18XX_WDT_MOD_WDEN;
>>> +       val |= LPC18XX_WDT_MOD_WDRESET;
>>> +       writel(val, lpc18xx_wdt->base + LPC18XX_WDT_MOD);
>>
>> R-M-W sequence should probably be protect with a lock.
>
> We can continue discussing this point on Ezequiel's response.
>

Please avoid unnecessary locks.

Thanks,
Guenter

[PATCH v2 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..e7a09eb
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/lpc18xx-wdt.txt
@@ -0,0 +1,19 @@
+* NXP LPC18xx Watchdog Timer (WDT)
+
+Required properties:
+- compatible: Should be "nxp,lpc1850-wdt"
+- reg: Should contain WDT registers location and length
+- clocks: Must contain an entry for each entry in clock-names.
+- clock-names: Should contain "wdtclk" and "reg"; the watchdog counter
+               clock and register interface clock respectively.
+- interrupts: Should contain WDT interrupt
+
+Examples:
+
+watchdog at 40080000 {
+	compatible = "nxp,lpc1850-wdt";
+	reg = <0x40080000 0x24>;
+	clocks = <&cgu BASE_SAFE_CLK>, <&ccu1 CLK_CPU_WWDT>;
+	clock-names = "wdtclk", "reg";
+	interrupts = <49>;
+};
-- 
1.9.1