* [PATCH v3 0/2] Add support for NXP LPC18xx Watchdog timer
@ 2015-07-15 0:50 ` Ariel D'Alessandro
0 siblings, 0 replies; 24+ messages in thread
From: Ariel D'Alessandro @ 2015-07-15 0:50 UTC (permalink / raw)
To: linux-arm-kernel
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 v2:
* Changed config tristate to "LPC18xx/43xx".
* Added COMPILE_TEST as a dependency.
* Replaced raw_local_irq* calls by spinlock.
* Changed dt compatible string to "nxp,lpc1850-wwdt".
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 | 344 +++++++++++++++++++++
4 files changed, 375 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/lpc18xx-wdt.txt
create mode 100644 drivers/watchdog/lpc18xx_wdt.c
--
1.9.1
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 0/2] Add support for NXP LPC18xx Watchdog timer
@ 2015-07-15 0:50 ` Ariel D'Alessandro
0 siblings, 0 replies; 24+ messages in thread
From: Ariel D'Alessandro @ 2015-07-15 0:50 UTC (permalink / raw)
To: linux-watchdog
Cc: wim, linux, ezequiel, linux-arm-kernel, manabian,
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 v2:
* Changed config tristate to "LPC18xx/43xx".
* Added COMPILE_TEST as a dependency.
* Replaced raw_local_irq* calls by spinlock.
* Changed dt compatible string to "nxp,lpc1850-wwdt".
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 | 344 +++++++++++++++++++++
4 files changed, 375 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/lpc18xx-wdt.txt
create mode 100644 drivers/watchdog/lpc18xx_wdt.c
--
1.9.1
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
2015-07-15 0:50 ` Ariel D'Alessandro
@ 2015-07-15 0:50 ` Ariel D'Alessandro
-1 siblings, 0 replies; 24+ messages in thread
From: Ariel D'Alessandro @ 2015-07-15 0:50 UTC (permalink / raw)
To: linux-arm-kernel
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 | 344 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 356 insertions(+)
create mode 100644 drivers/watchdog/lpc18xx_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 742fbbc..af5e2e3 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 "LPC18xx/43xx Watchdog"
+ depends on ARCH_LPC18XX || COMPILE_TEST
+ 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..2b489fc
--- /dev/null
+++ b/drivers/watchdog/lpc18xx_wdt.c
@@ -0,0 +1,344 @@
+/*
+ * 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) ")");
+
+static DEFINE_SPINLOCK(wdt_lock);
+
+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.
+ */
+ spin_lock_irqsave(&wdt_lock, flags);
+ writel(LPC18XX_WDT_FEED_MAGIC1, lpc18xx_wdt->base + LPC18XX_WDT_FEED);
+ writel(LPC18XX_WDT_FEED_MAGIC2, lpc18xx_wdt->base + LPC18XX_WDT_FEED);
+ spin_unlock_irqrestore(&wdt_lock, 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.
+ */
+ spin_lock_irqsave(&wdt_lock, 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);
+
+ spin_unlock_irqrestore(&wdt_lock, 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-wwdt" },
+ {}
+};
+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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
@ 2015-07-15 0:50 ` Ariel D'Alessandro
0 siblings, 0 replies; 24+ messages in thread
From: Ariel D'Alessandro @ 2015-07-15 0:50 UTC (permalink / raw)
To: linux-watchdog
Cc: wim, linux, ezequiel, linux-arm-kernel, manabian,
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 | 344 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 356 insertions(+)
create mode 100644 drivers/watchdog/lpc18xx_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 742fbbc..af5e2e3 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 "LPC18xx/43xx Watchdog"
+ depends on ARCH_LPC18XX || COMPILE_TEST
+ 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..2b489fc
--- /dev/null
+++ b/drivers/watchdog/lpc18xx_wdt.c
@@ -0,0 +1,344 @@
+/*
+ * 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) ")");
+
+static DEFINE_SPINLOCK(wdt_lock);
+
+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.
+ */
+ spin_lock_irqsave(&wdt_lock, flags);
+ writel(LPC18XX_WDT_FEED_MAGIC1, lpc18xx_wdt->base + LPC18XX_WDT_FEED);
+ writel(LPC18XX_WDT_FEED_MAGIC2, lpc18xx_wdt->base + LPC18XX_WDT_FEED);
+ spin_unlock_irqrestore(&wdt_lock, 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.
+ */
+ spin_lock_irqsave(&wdt_lock, 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);
+
+ spin_unlock_irqrestore(&wdt_lock, 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-wwdt" },
+ {}
+};
+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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 2/2] DT: watchdog: Add NXP LPC18xx Watchdog Timer binding documentation
2015-07-15 0:50 ` Ariel D'Alessandro
@ 2015-07-15 0:50 ` Ariel D'Alessandro
-1 siblings, 0 replies; 24+ messages in thread
From: Ariel D'Alessandro @ 2015-07-15 0:50 UTC (permalink / raw)
To: linux-arm-kernel
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..09f6b24
--- /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-wwdt"
+- 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-wwdt";
+ reg = <0x40080000 0x24>;
+ clocks = <&cgu BASE_SAFE_CLK>, <&ccu1 CLK_CPU_WWDT>;
+ clock-names = "wdtclk", "reg";
+ interrupts = <49>;
+};
--
1.9.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 2/2] DT: watchdog: Add NXP LPC18xx Watchdog Timer binding documentation
@ 2015-07-15 0:50 ` Ariel D'Alessandro
0 siblings, 0 replies; 24+ messages in thread
From: Ariel D'Alessandro @ 2015-07-15 0:50 UTC (permalink / raw)
To: linux-watchdog
Cc: wim, linux, ezequiel, linux-arm-kernel, manabian,
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..09f6b24
--- /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-wwdt"
+- 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@40080000 {
+ compatible = "nxp,lpc1850-wwdt";
+ reg = <0x40080000 0x24>;
+ clocks = <&cgu BASE_SAFE_CLK>, <&ccu1 CLK_CPU_WWDT>;
+ clock-names = "wdtclk", "reg";
+ interrupts = <49>;
+};
--
1.9.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
2015-07-15 0:50 ` Ariel D'Alessandro
@ 2015-07-22 0:51 ` Guenter Roeck
-1 siblings, 0 replies; 24+ messages in thread
From: Guenter Roeck @ 2015-07-22 0:51 UTC (permalink / raw)
To: linux-arm-kernel
On 07/14/2015 05:50 PM, 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>
Hi Ariel,
sorry for the delayed response. Comments below.
Guenter
> ---
> drivers/watchdog/Kconfig | 11 ++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/lpc18xx_wdt.c | 344 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 356 insertions(+)
> create mode 100644 drivers/watchdog/lpc18xx_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 742fbbc..af5e2e3 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 "LPC18xx/43xx Watchdog"
> + depends on ARCH_LPC18XX || COMPILE_TEST
> + 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..2b489fc
> --- /dev/null
> +++ b/drivers/watchdog/lpc18xx_wdt.c
> @@ -0,0 +1,344 @@
> +/*
> + * 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
Not used.
> +
> +#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
> +
This is (still) really low for Linux. Something like min(30, max_timeout)
might make more sense.
> +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) ")");
> +
> +static DEFINE_SPINLOCK(wdt_lock);
> +
Why is this lock not defined as part of lpc18xx_wdt_dev ?
> +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.
> + */
> + spin_lock_irqsave(&wdt_lock, flags);
> + writel(LPC18XX_WDT_FEED_MAGIC1, lpc18xx_wdt->base + LPC18XX_WDT_FEED);
> + writel(LPC18XX_WDT_FEED_MAGIC2, lpc18xx_wdt->base + LPC18XX_WDT_FEED);
> + spin_unlock_irqrestore(&wdt_lock, 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.
> + */
> + spin_lock_irqsave(&wdt_lock, 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);
> +
> + spin_unlock_irqrestore(&wdt_lock, 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);
Given how often you use &pdev->dev, it might make sense to have a local variable
for it.
struct device *dev = &pdev->dev;
> + 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;
> + }
> +
It might make sense to store clk_rate locally so you don't have to read it
at runtime.
> + 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-wwdt" },
> + {}
> +};
> +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");
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
@ 2015-07-22 0:51 ` Guenter Roeck
0 siblings, 0 replies; 24+ messages in thread
From: Guenter Roeck @ 2015-07-22 0:51 UTC (permalink / raw)
To: Ariel D'Alessandro, linux-watchdog
Cc: wim, ezequiel, linux-arm-kernel, manabian
On 07/14/2015 05:50 PM, 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>
Hi Ariel,
sorry for the delayed response. Comments below.
Guenter
> ---
> drivers/watchdog/Kconfig | 11 ++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/lpc18xx_wdt.c | 344 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 356 insertions(+)
> create mode 100644 drivers/watchdog/lpc18xx_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 742fbbc..af5e2e3 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 "LPC18xx/43xx Watchdog"
> + depends on ARCH_LPC18XX || COMPILE_TEST
> + 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..2b489fc
> --- /dev/null
> +++ b/drivers/watchdog/lpc18xx_wdt.c
> @@ -0,0 +1,344 @@
> +/*
> + * 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
Not used.
> +
> +#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
> +
This is (still) really low for Linux. Something like min(30, max_timeout)
might make more sense.
> +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) ")");
> +
> +static DEFINE_SPINLOCK(wdt_lock);
> +
Why is this lock not defined as part of lpc18xx_wdt_dev ?
> +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.
> + */
> + spin_lock_irqsave(&wdt_lock, flags);
> + writel(LPC18XX_WDT_FEED_MAGIC1, lpc18xx_wdt->base + LPC18XX_WDT_FEED);
> + writel(LPC18XX_WDT_FEED_MAGIC2, lpc18xx_wdt->base + LPC18XX_WDT_FEED);
> + spin_unlock_irqrestore(&wdt_lock, 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.
> + */
> + spin_lock_irqsave(&wdt_lock, 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);
> +
> + spin_unlock_irqrestore(&wdt_lock, 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);
Given how often you use &pdev->dev, it might make sense to have a local variable
for it.
struct device *dev = &pdev->dev;
> + 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;
> + }
> +
It might make sense to store clk_rate locally so you don't have to read it
at runtime.
> + 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-wwdt" },
> + {}
> +};
> +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");
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
2015-07-22 0:51 ` Guenter Roeck
@ 2015-07-23 20:38 ` Ariel D'Alessandro
-1 siblings, 0 replies; 24+ messages in thread
From: Ariel D'Alessandro @ 2015-07-23 20:38 UTC (permalink / raw)
To: linux-arm-kernel
Hi Guenter,
El 21/07/15 a las 21:51, Guenter Roeck escibi?:
> On 07/14/2015 05:50 PM, 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>
>
> Hi Ariel,
>
> sorry for the delayed response. Comments below.
>
> Guenter
>
>> ---
>> drivers/watchdog/Kconfig | 11 ++
>> drivers/watchdog/Makefile | 1 +
>> drivers/watchdog/lpc18xx_wdt.c | 344
>> +++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 356 insertions(+)
>> create mode 100644 drivers/watchdog/lpc18xx_wdt.c
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 742fbbc..af5e2e3 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 "LPC18xx/43xx Watchdog"
>> + depends on ARCH_LPC18XX || COMPILE_TEST
>> + 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..2b489fc
>> --- /dev/null
>> +++ b/drivers/watchdog/lpc18xx_wdt.c
>> @@ -0,0 +1,344 @@
>> +/*
>> + * 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
>
> Not used.
That's right. I'll remove it.
>
>> +
>> +#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
>> +
>
> This is (still) really low for Linux. Something like min(30, max_timeout)
> might make more sense.
Remember that hardware limits timeout to a maximum value of 5.
As you said before we could use a soft timer, but I not sure that's a
good idea. As Ezequiel said [0], it might be adding bloat and we must
consider that this watchdog controller is included in cortex-M MCUs.
[0] http://www.spinics.net/lists/linux-watchdog/msg06904.html
>
>> +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) ")");
>> +
>> +static DEFINE_SPINLOCK(wdt_lock);
>> +
>
> Why is this lock not defined as part of lpc18xx_wdt_dev ?
There's no reason. I'll move it into lpc18xx_wdt_dev, where it should be.
>
>> +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.
>> + */
>> + spin_lock_irqsave(&wdt_lock, flags);
>> + writel(LPC18XX_WDT_FEED_MAGIC1, lpc18xx_wdt->base +
>> LPC18XX_WDT_FEED);
>> + writel(LPC18XX_WDT_FEED_MAGIC2, lpc18xx_wdt->base +
>> LPC18XX_WDT_FEED);
>> + spin_unlock_irqrestore(&wdt_lock, 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.
>> + */
>> + spin_lock_irqsave(&wdt_lock, 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);
>> +
>> + spin_unlock_irqrestore(&wdt_lock, 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);
>
> Given how often you use &pdev->dev, it might make sense to have a local
> variable
> for it.
>
> struct device *dev = &pdev->dev;
OK. Will do that.
>
>
>> + 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;
>> + }
>> +
> It might make sense to store clk_rate locally so you don't have to read it
> at runtime.
OK. Will do that.
>
>> + 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-wwdt" },
>> + {}
>> +};
>> +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");
>>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
@ 2015-07-23 20:38 ` Ariel D'Alessandro
0 siblings, 0 replies; 24+ messages in thread
From: Ariel D'Alessandro @ 2015-07-23 20:38 UTC (permalink / raw)
To: Guenter Roeck, linux-watchdog; +Cc: wim, ezequiel, linux-arm-kernel, manabian
Hi Guenter,
El 21/07/15 a las 21:51, Guenter Roeck escibió:
> On 07/14/2015 05:50 PM, 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>
>
> Hi Ariel,
>
> sorry for the delayed response. Comments below.
>
> Guenter
>
>> ---
>> drivers/watchdog/Kconfig | 11 ++
>> drivers/watchdog/Makefile | 1 +
>> drivers/watchdog/lpc18xx_wdt.c | 344
>> +++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 356 insertions(+)
>> create mode 100644 drivers/watchdog/lpc18xx_wdt.c
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 742fbbc..af5e2e3 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 "LPC18xx/43xx Watchdog"
>> + depends on ARCH_LPC18XX || COMPILE_TEST
>> + 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..2b489fc
>> --- /dev/null
>> +++ b/drivers/watchdog/lpc18xx_wdt.c
>> @@ -0,0 +1,344 @@
>> +/*
>> + * 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
>
> Not used.
That's right. I'll remove it.
>
>> +
>> +#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
>> +
>
> This is (still) really low for Linux. Something like min(30, max_timeout)
> might make more sense.
Remember that hardware limits timeout to a maximum value of 5.
As you said before we could use a soft timer, but I not sure that's a
good idea. As Ezequiel said [0], it might be adding bloat and we must
consider that this watchdog controller is included in cortex-M MCUs.
[0] http://www.spinics.net/lists/linux-watchdog/msg06904.html
>
>> +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) ")");
>> +
>> +static DEFINE_SPINLOCK(wdt_lock);
>> +
>
> Why is this lock not defined as part of lpc18xx_wdt_dev ?
There's no reason. I'll move it into lpc18xx_wdt_dev, where it should be.
>
>> +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.
>> + */
>> + spin_lock_irqsave(&wdt_lock, flags);
>> + writel(LPC18XX_WDT_FEED_MAGIC1, lpc18xx_wdt->base +
>> LPC18XX_WDT_FEED);
>> + writel(LPC18XX_WDT_FEED_MAGIC2, lpc18xx_wdt->base +
>> LPC18XX_WDT_FEED);
>> + spin_unlock_irqrestore(&wdt_lock, 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.
>> + */
>> + spin_lock_irqsave(&wdt_lock, 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);
>> +
>> + spin_unlock_irqrestore(&wdt_lock, 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);
>
> Given how often you use &pdev->dev, it might make sense to have a local
> variable
> for it.
>
> struct device *dev = &pdev->dev;
OK. Will do that.
>
>
>> + 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;
>> + }
>> +
> It might make sense to store clk_rate locally so you don't have to read it
> at runtime.
OK. Will do that.
>
>> + 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-wwdt" },
>> + {}
>> +};
>> +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");
>>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
2015-07-23 20:38 ` Ariel D'Alessandro
@ 2015-07-23 21:21 ` Guenter Roeck
-1 siblings, 0 replies; 24+ messages in thread
From: Guenter Roeck @ 2015-07-23 21:21 UTC (permalink / raw)
To: linux-arm-kernel
On 07/23/2015 01:38 PM, Ariel D'Alessandro wrote:
> Hi Guenter,
>
> El 21/07/15 a las 21:51, Guenter Roeck escibi?:
>> On 07/14/2015 05:50 PM, 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>
>>
>> Hi Ariel,
>>
>> sorry for the delayed response. Comments below.
>>
>> Guenter
>>
>>> ---
>>> drivers/watchdog/Kconfig | 11 ++
>>> drivers/watchdog/Makefile | 1 +
>>> drivers/watchdog/lpc18xx_wdt.c | 344
>>> +++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 356 insertions(+)
>>> create mode 100644 drivers/watchdog/lpc18xx_wdt.c
>>>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>> index 742fbbc..af5e2e3 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 "LPC18xx/43xx Watchdog"
>>> + depends on ARCH_LPC18XX || COMPILE_TEST
>>> + 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..2b489fc
>>> --- /dev/null
>>> +++ b/drivers/watchdog/lpc18xx_wdt.c
>>> @@ -0,0 +1,344 @@
>>> +/*
>>> + * 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
>>
>> Not used.
>
> That's right. I'll remove it.
>
>>
>>> +
>>> +#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
>>> +
>>
>> This is (still) really low for Linux. Something like min(30, max_timeout)
>> might make more sense.
>
> Remember that hardware limits timeout to a maximum value of 5.
>
> As you said before we could use a soft timer, but I not sure that's a
> good idea. As Ezequiel said [0], it might be adding bloat and we must
> consider that this watchdog controller is included in cortex-M MCUs.
>
> [0] http://www.spinics.net/lists/linux-watchdog/msg06904.html
>
Further down you have
lpc18xx_wdt->wdt_dev.max_timeout = (LPC18XX_WDT_TC_MAX *
LPC18XX_WDT_CLK_DIV) / clk_rate;
which seems to contradict this statement. If the maximum timeout is 5 seconds,
why don't you set max_timeout to a fixed value of 5 seconds ?
Thanks,
Guenter
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
@ 2015-07-23 21:21 ` Guenter Roeck
0 siblings, 0 replies; 24+ messages in thread
From: Guenter Roeck @ 2015-07-23 21:21 UTC (permalink / raw)
To: Ariel D'Alessandro, linux-watchdog
Cc: wim, ezequiel, linux-arm-kernel, manabian
On 07/23/2015 01:38 PM, Ariel D'Alessandro wrote:
> Hi Guenter,
>
> El 21/07/15 a las 21:51, Guenter Roeck escibió:
>> On 07/14/2015 05:50 PM, 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>
>>
>> Hi Ariel,
>>
>> sorry for the delayed response. Comments below.
>>
>> Guenter
>>
>>> ---
>>> drivers/watchdog/Kconfig | 11 ++
>>> drivers/watchdog/Makefile | 1 +
>>> drivers/watchdog/lpc18xx_wdt.c | 344
>>> +++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 356 insertions(+)
>>> create mode 100644 drivers/watchdog/lpc18xx_wdt.c
>>>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>> index 742fbbc..af5e2e3 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 "LPC18xx/43xx Watchdog"
>>> + depends on ARCH_LPC18XX || COMPILE_TEST
>>> + 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..2b489fc
>>> --- /dev/null
>>> +++ b/drivers/watchdog/lpc18xx_wdt.c
>>> @@ -0,0 +1,344 @@
>>> +/*
>>> + * 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
>>
>> Not used.
>
> That's right. I'll remove it.
>
>>
>>> +
>>> +#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
>>> +
>>
>> This is (still) really low for Linux. Something like min(30, max_timeout)
>> might make more sense.
>
> Remember that hardware limits timeout to a maximum value of 5.
>
> As you said before we could use a soft timer, but I not sure that's a
> good idea. As Ezequiel said [0], it might be adding bloat and we must
> consider that this watchdog controller is included in cortex-M MCUs.
>
> [0] http://www.spinics.net/lists/linux-watchdog/msg06904.html
>
Further down you have
lpc18xx_wdt->wdt_dev.max_timeout = (LPC18XX_WDT_TC_MAX *
LPC18XX_WDT_CLK_DIV) / clk_rate;
which seems to contradict this statement. If the maximum timeout is 5 seconds,
why don't you set max_timeout to a fixed value of 5 seconds ?
Thanks,
Guenter
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
2015-07-23 21:21 ` Guenter Roeck
@ 2015-07-24 13:54 ` Ariel D'Alessandro
-1 siblings, 0 replies; 24+ messages in thread
From: Ariel D'Alessandro @ 2015-07-24 13:54 UTC (permalink / raw)
To: linux-arm-kernel
El 23/07/15 a las 18:21, Guenter Roeck escibi?:
> On 07/23/2015 01:38 PM, Ariel D'Alessandro wrote:
>> Hi Guenter,
>>
>> El 21/07/15 a las 21:51, Guenter Roeck escibi?:
>>> On 07/14/2015 05:50 PM, 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>
>>>
>>> Hi Ariel,
>>>
>>> sorry for the delayed response. Comments below.
>>>
>>> Guenter
>>>
>>>> ---
>>>> drivers/watchdog/Kconfig | 11 ++
>>>> drivers/watchdog/Makefile | 1 +
>>>> drivers/watchdog/lpc18xx_wdt.c | 344
>>>> +++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 356 insertions(+)
>>>> create mode 100644 drivers/watchdog/lpc18xx_wdt.c
>>>>
>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>> index 742fbbc..af5e2e3 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 "LPC18xx/43xx Watchdog"
>>>> + depends on ARCH_LPC18XX || COMPILE_TEST
>>>> + 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..2b489fc
>>>> --- /dev/null
>>>> +++ b/drivers/watchdog/lpc18xx_wdt.c
>>>> @@ -0,0 +1,344 @@
>>>> +/*
>>>> + * 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
>>>
>>> Not used.
>>
>> That's right. I'll remove it.
>>
>>>
>>>> +
>>>> +#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
>>>> +
>>>
>>> This is (still) really low for Linux. Something like min(30,
>>> max_timeout)
>>> might make more sense.
>>
>> Remember that hardware limits timeout to a maximum value of 5.
>>
>> As you said before we could use a soft timer, but I not sure that's a
>> good idea. As Ezequiel said [0], it might be adding bloat and we must
>> consider that this watchdog controller is included in cortex-M MCUs.
>>
>> [0] http://www.spinics.net/lists/linux-watchdog/msg06904.html
>>
>
> Further down you have
> lpc18xx_wdt->wdt_dev.max_timeout = (LPC18XX_WDT_TC_MAX *
> LPC18XX_WDT_CLK_DIV) / clk_rate;
>
> which seems to contradict this statement. If the maximum timeout is 5
> seconds,
> why don't you set max_timeout to a fixed value of 5 seconds ?
Well, I think that's the less hardcoded way. It might allow the driver
to be extended to another HW variant that configures the wdt clk rate.
I see what's your point. As you proposed, using the same criterion I
should do this:
#define LPC18XX_WDT_DEF_TIMEOUT (unsigned int)30
and further down, in probe function:
lpc18xx_wdt->wdt_dev.timeout = min(LPC18XX_WDT_DEF_TIMEOUT,
lpc18xx_wdt->wdt_dev.max_timeout);
Is this close enough to your solution?
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
@ 2015-07-24 13:54 ` Ariel D'Alessandro
0 siblings, 0 replies; 24+ messages in thread
From: Ariel D'Alessandro @ 2015-07-24 13:54 UTC (permalink / raw)
To: Guenter Roeck, linux-watchdog; +Cc: wim, ezequiel, linux-arm-kernel, manabian
El 23/07/15 a las 18:21, Guenter Roeck escibió:
> On 07/23/2015 01:38 PM, Ariel D'Alessandro wrote:
>> Hi Guenter,
>>
>> El 21/07/15 a las 21:51, Guenter Roeck escibió:
>>> On 07/14/2015 05:50 PM, 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>
>>>
>>> Hi Ariel,
>>>
>>> sorry for the delayed response. Comments below.
>>>
>>> Guenter
>>>
>>>> ---
>>>> drivers/watchdog/Kconfig | 11 ++
>>>> drivers/watchdog/Makefile | 1 +
>>>> drivers/watchdog/lpc18xx_wdt.c | 344
>>>> +++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 356 insertions(+)
>>>> create mode 100644 drivers/watchdog/lpc18xx_wdt.c
>>>>
>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>> index 742fbbc..af5e2e3 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 "LPC18xx/43xx Watchdog"
>>>> + depends on ARCH_LPC18XX || COMPILE_TEST
>>>> + 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..2b489fc
>>>> --- /dev/null
>>>> +++ b/drivers/watchdog/lpc18xx_wdt.c
>>>> @@ -0,0 +1,344 @@
>>>> +/*
>>>> + * 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
>>>
>>> Not used.
>>
>> That's right. I'll remove it.
>>
>>>
>>>> +
>>>> +#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
>>>> +
>>>
>>> This is (still) really low for Linux. Something like min(30,
>>> max_timeout)
>>> might make more sense.
>>
>> Remember that hardware limits timeout to a maximum value of 5.
>>
>> As you said before we could use a soft timer, but I not sure that's a
>> good idea. As Ezequiel said [0], it might be adding bloat and we must
>> consider that this watchdog controller is included in cortex-M MCUs.
>>
>> [0] http://www.spinics.net/lists/linux-watchdog/msg06904.html
>>
>
> Further down you have
> lpc18xx_wdt->wdt_dev.max_timeout = (LPC18XX_WDT_TC_MAX *
> LPC18XX_WDT_CLK_DIV) / clk_rate;
>
> which seems to contradict this statement. If the maximum timeout is 5
> seconds,
> why don't you set max_timeout to a fixed value of 5 seconds ?
Well, I think that's the less hardcoded way. It might allow the driver
to be extended to another HW variant that configures the wdt clk rate.
I see what's your point. As you proposed, using the same criterion I
should do this:
#define LPC18XX_WDT_DEF_TIMEOUT (unsigned int)30
and further down, in probe function:
lpc18xx_wdt->wdt_dev.timeout = min(LPC18XX_WDT_DEF_TIMEOUT,
lpc18xx_wdt->wdt_dev.max_timeout);
Is this close enough to your solution?
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
2015-07-24 13:54 ` Ariel D'Alessandro
@ 2015-07-24 14:04 ` Guenter Roeck
-1 siblings, 0 replies; 24+ messages in thread
From: Guenter Roeck @ 2015-07-24 14:04 UTC (permalink / raw)
To: linux-arm-kernel
On 07/24/2015 06:54 AM, Ariel D'Alessandro wrote:
>
>
> El 23/07/15 a las 18:21, Guenter Roeck escibi?:
>> On 07/23/2015 01:38 PM, Ariel D'Alessandro wrote:
>>> Hi Guenter,
>>>
>>> El 21/07/15 a las 21:51, Guenter Roeck escibi?:
>>>> On 07/14/2015 05:50 PM, 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>
>>>>
>>>> Hi Ariel,
>>>>
>>>> sorry for the delayed response. Comments below.
>>>>
>>>> Guenter
>>>>
>>>>> ---
>>>>> drivers/watchdog/Kconfig | 11 ++
>>>>> drivers/watchdog/Makefile | 1 +
>>>>> drivers/watchdog/lpc18xx_wdt.c | 344
>>>>> +++++++++++++++++++++++++++++++++++++++++
>>>>> 3 files changed, 356 insertions(+)
>>>>> create mode 100644 drivers/watchdog/lpc18xx_wdt.c
>>>>>
>>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>>> index 742fbbc..af5e2e3 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 "LPC18xx/43xx Watchdog"
>>>>> + depends on ARCH_LPC18XX || COMPILE_TEST
>>>>> + 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..2b489fc
>>>>> --- /dev/null
>>>>> +++ b/drivers/watchdog/lpc18xx_wdt.c
>>>>> @@ -0,0 +1,344 @@
>>>>> +/*
>>>>> + * 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
>>>>
>>>> Not used.
>>>
>>> That's right. I'll remove it.
>>>
>>>>
>>>>> +
>>>>> +#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
>>>>> +
>>>>
>>>> This is (still) really low for Linux. Something like min(30,
>>>> max_timeout)
>>>> might make more sense.
>>>
>>> Remember that hardware limits timeout to a maximum value of 5.
>>>
>>> As you said before we could use a soft timer, but I not sure that's a
>>> good idea. As Ezequiel said [0], it might be adding bloat and we must
>>> consider that this watchdog controller is included in cortex-M MCUs.
>>>
>>> [0] http://www.spinics.net/lists/linux-watchdog/msg06904.html
>>>
>>
>> Further down you have
>> lpc18xx_wdt->wdt_dev.max_timeout = (LPC18XX_WDT_TC_MAX *
>> LPC18XX_WDT_CLK_DIV) / clk_rate;
>>
>> which seems to contradict this statement. If the maximum timeout is 5
>> seconds,
>> why don't you set max_timeout to a fixed value of 5 seconds ?
>
> Well, I think that's the less hardcoded way. It might allow the driver
> to be extended to another HW variant that configures the wdt clk rate.
>
> I see what's your point. As you proposed, using the same criterion I
> should do this:
>
> #define LPC18XX_WDT_DEF_TIMEOUT (unsigned int)30
>
> and further down, in probe function:
>
> lpc18xx_wdt->wdt_dev.timeout = min(LPC18XX_WDT_DEF_TIMEOUT,
> lpc18xx_wdt->wdt_dev.max_timeout);
>
> Is this close enough to your solution?
>
Yes, but why the typecast for LPC18XX_WDT_DEF_TIMEOUT ?
Thanks,
Guenter
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
@ 2015-07-24 14:04 ` Guenter Roeck
0 siblings, 0 replies; 24+ messages in thread
From: Guenter Roeck @ 2015-07-24 14:04 UTC (permalink / raw)
To: Ariel D'Alessandro, linux-watchdog
Cc: wim, ezequiel, linux-arm-kernel, manabian
On 07/24/2015 06:54 AM, Ariel D'Alessandro wrote:
>
>
> El 23/07/15 a las 18:21, Guenter Roeck escibió:
>> On 07/23/2015 01:38 PM, Ariel D'Alessandro wrote:
>>> Hi Guenter,
>>>
>>> El 21/07/15 a las 21:51, Guenter Roeck escibió:
>>>> On 07/14/2015 05:50 PM, 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>
>>>>
>>>> Hi Ariel,
>>>>
>>>> sorry for the delayed response. Comments below.
>>>>
>>>> Guenter
>>>>
>>>>> ---
>>>>> drivers/watchdog/Kconfig | 11 ++
>>>>> drivers/watchdog/Makefile | 1 +
>>>>> drivers/watchdog/lpc18xx_wdt.c | 344
>>>>> +++++++++++++++++++++++++++++++++++++++++
>>>>> 3 files changed, 356 insertions(+)
>>>>> create mode 100644 drivers/watchdog/lpc18xx_wdt.c
>>>>>
>>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>>> index 742fbbc..af5e2e3 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 "LPC18xx/43xx Watchdog"
>>>>> + depends on ARCH_LPC18XX || COMPILE_TEST
>>>>> + 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..2b489fc
>>>>> --- /dev/null
>>>>> +++ b/drivers/watchdog/lpc18xx_wdt.c
>>>>> @@ -0,0 +1,344 @@
>>>>> +/*
>>>>> + * 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
>>>>
>>>> Not used.
>>>
>>> That's right. I'll remove it.
>>>
>>>>
>>>>> +
>>>>> +#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
>>>>> +
>>>>
>>>> This is (still) really low for Linux. Something like min(30,
>>>> max_timeout)
>>>> might make more sense.
>>>
>>> Remember that hardware limits timeout to a maximum value of 5.
>>>
>>> As you said before we could use a soft timer, but I not sure that's a
>>> good idea. As Ezequiel said [0], it might be adding bloat and we must
>>> consider that this watchdog controller is included in cortex-M MCUs.
>>>
>>> [0] http://www.spinics.net/lists/linux-watchdog/msg06904.html
>>>
>>
>> Further down you have
>> lpc18xx_wdt->wdt_dev.max_timeout = (LPC18XX_WDT_TC_MAX *
>> LPC18XX_WDT_CLK_DIV) / clk_rate;
>>
>> which seems to contradict this statement. If the maximum timeout is 5
>> seconds,
>> why don't you set max_timeout to a fixed value of 5 seconds ?
>
> Well, I think that's the less hardcoded way. It might allow the driver
> to be extended to another HW variant that configures the wdt clk rate.
>
> I see what's your point. As you proposed, using the same criterion I
> should do this:
>
> #define LPC18XX_WDT_DEF_TIMEOUT (unsigned int)30
>
> and further down, in probe function:
>
> lpc18xx_wdt->wdt_dev.timeout = min(LPC18XX_WDT_DEF_TIMEOUT,
> lpc18xx_wdt->wdt_dev.max_timeout);
>
> Is this close enough to your solution?
>
Yes, but why the typecast for LPC18XX_WDT_DEF_TIMEOUT ?
Thanks,
Guenter
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
2015-07-24 14:04 ` Guenter Roeck
@ 2015-07-24 20:57 ` Ariel D'Alessandro
-1 siblings, 0 replies; 24+ messages in thread
From: Ariel D'Alessandro @ 2015-07-24 20:57 UTC (permalink / raw)
To: linux-arm-kernel
El 24/07/15 a las 11:04, Guenter Roeck escibi?:
> On 07/24/2015 06:54 AM, Ariel D'Alessandro wrote:
>>
>>
>> El 23/07/15 a las 18:21, Guenter Roeck escibi?:
>>> On 07/23/2015 01:38 PM, Ariel D'Alessandro wrote:
>>>> Hi Guenter,
>>>>
>>>> El 21/07/15 a las 21:51, Guenter Roeck escibi?:
>>>>> On 07/14/2015 05:50 PM, 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>
>>>>>
>>>>> Hi Ariel,
>>>>>
>>>>> sorry for the delayed response. Comments below.
>>>>>
>>>>> Guenter
>>>>>
>>>>>> ---
>>>>>> drivers/watchdog/Kconfig | 11 ++
>>>>>> drivers/watchdog/Makefile | 1 +
>>>>>> drivers/watchdog/lpc18xx_wdt.c | 344
>>>>>> +++++++++++++++++++++++++++++++++++++++++
>>>>>> 3 files changed, 356 insertions(+)
>>>>>> create mode 100644 drivers/watchdog/lpc18xx_wdt.c
>>>>>>
>>>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>>>> index 742fbbc..af5e2e3 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 "LPC18xx/43xx Watchdog"
>>>>>> + depends on ARCH_LPC18XX || COMPILE_TEST
>>>>>> + 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..2b489fc
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/watchdog/lpc18xx_wdt.c
>>>>>> @@ -0,0 +1,344 @@
>>>>>> +/*
>>>>>> + * 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
>>>>>
>>>>> Not used.
>>>>
>>>> That's right. I'll remove it.
>>>>
>>>>>
>>>>>> +
>>>>>> +#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
>>>>>> +
>>>>>
>>>>> This is (still) really low for Linux. Something like min(30,
>>>>> max_timeout)
>>>>> might make more sense.
>>>>
>>>> Remember that hardware limits timeout to a maximum value of 5.
>>>>
>>>> As you said before we could use a soft timer, but I not sure that's a
>>>> good idea. As Ezequiel said [0], it might be adding bloat and we must
>>>> consider that this watchdog controller is included in cortex-M MCUs.
>>>>
>>>> [0] http://www.spinics.net/lists/linux-watchdog/msg06904.html
>>>>
>>>
>>> Further down you have
>>> lpc18xx_wdt->wdt_dev.max_timeout = (LPC18XX_WDT_TC_MAX *
>>> LPC18XX_WDT_CLK_DIV) / clk_rate;
>>>
>>> which seems to contradict this statement. If the maximum timeout is 5
>>> seconds,
>>> why don't you set max_timeout to a fixed value of 5 seconds ?
>>
>> Well, I think that's the less hardcoded way. It might allow the driver
>> to be extended to another HW variant that configures the wdt clk rate.
>>
>> I see what's your point. As you proposed, using the same criterion I
>> should do this:
>>
>> #define LPC18XX_WDT_DEF_TIMEOUT (unsigned int)30
>>
>> and further down, in probe function:
>>
>> lpc18xx_wdt->wdt_dev.timeout = min(LPC18XX_WDT_DEF_TIMEOUT,
>> lpc18xx_wdt->wdt_dev.max_timeout);
>>
>> Is this close enough to your solution?
>>
> Yes, but why the typecast for LPC18XX_WDT_DEF_TIMEOUT ?
>
Otherwise the compiler will warn about a comparison of distinct pointer
types in expansion of the macro 'min'.
Should it be casted when using it better than in definition?
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
@ 2015-07-24 20:57 ` Ariel D'Alessandro
0 siblings, 0 replies; 24+ messages in thread
From: Ariel D'Alessandro @ 2015-07-24 20:57 UTC (permalink / raw)
To: Guenter Roeck, linux-watchdog; +Cc: wim, ezequiel, linux-arm-kernel, manabian
El 24/07/15 a las 11:04, Guenter Roeck escibió:
> On 07/24/2015 06:54 AM, Ariel D'Alessandro wrote:
>>
>>
>> El 23/07/15 a las 18:21, Guenter Roeck escibió:
>>> On 07/23/2015 01:38 PM, Ariel D'Alessandro wrote:
>>>> Hi Guenter,
>>>>
>>>> El 21/07/15 a las 21:51, Guenter Roeck escibió:
>>>>> On 07/14/2015 05:50 PM, 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>
>>>>>
>>>>> Hi Ariel,
>>>>>
>>>>> sorry for the delayed response. Comments below.
>>>>>
>>>>> Guenter
>>>>>
>>>>>> ---
>>>>>> drivers/watchdog/Kconfig | 11 ++
>>>>>> drivers/watchdog/Makefile | 1 +
>>>>>> drivers/watchdog/lpc18xx_wdt.c | 344
>>>>>> +++++++++++++++++++++++++++++++++++++++++
>>>>>> 3 files changed, 356 insertions(+)
>>>>>> create mode 100644 drivers/watchdog/lpc18xx_wdt.c
>>>>>>
>>>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>>>> index 742fbbc..af5e2e3 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 "LPC18xx/43xx Watchdog"
>>>>>> + depends on ARCH_LPC18XX || COMPILE_TEST
>>>>>> + 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..2b489fc
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/watchdog/lpc18xx_wdt.c
>>>>>> @@ -0,0 +1,344 @@
>>>>>> +/*
>>>>>> + * 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
>>>>>
>>>>> Not used.
>>>>
>>>> That's right. I'll remove it.
>>>>
>>>>>
>>>>>> +
>>>>>> +#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
>>>>>> +
>>>>>
>>>>> This is (still) really low for Linux. Something like min(30,
>>>>> max_timeout)
>>>>> might make more sense.
>>>>
>>>> Remember that hardware limits timeout to a maximum value of 5.
>>>>
>>>> As you said before we could use a soft timer, but I not sure that's a
>>>> good idea. As Ezequiel said [0], it might be adding bloat and we must
>>>> consider that this watchdog controller is included in cortex-M MCUs.
>>>>
>>>> [0] http://www.spinics.net/lists/linux-watchdog/msg06904.html
>>>>
>>>
>>> Further down you have
>>> lpc18xx_wdt->wdt_dev.max_timeout = (LPC18XX_WDT_TC_MAX *
>>> LPC18XX_WDT_CLK_DIV) / clk_rate;
>>>
>>> which seems to contradict this statement. If the maximum timeout is 5
>>> seconds,
>>> why don't you set max_timeout to a fixed value of 5 seconds ?
>>
>> Well, I think that's the less hardcoded way. It might allow the driver
>> to be extended to another HW variant that configures the wdt clk rate.
>>
>> I see what's your point. As you proposed, using the same criterion I
>> should do this:
>>
>> #define LPC18XX_WDT_DEF_TIMEOUT (unsigned int)30
>>
>> and further down, in probe function:
>>
>> lpc18xx_wdt->wdt_dev.timeout = min(LPC18XX_WDT_DEF_TIMEOUT,
>> lpc18xx_wdt->wdt_dev.max_timeout);
>>
>> Is this close enough to your solution?
>>
> Yes, but why the typecast for LPC18XX_WDT_DEF_TIMEOUT ?
>
Otherwise the compiler will warn about a comparison of distinct pointer
types in expansion of the macro 'min'.
Should it be casted when using it better than in definition?
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
2015-07-24 20:57 ` Ariel D'Alessandro
@ 2015-07-24 21:17 ` Ariel D'Alessandro
-1 siblings, 0 replies; 24+ messages in thread
From: Ariel D'Alessandro @ 2015-07-24 21:17 UTC (permalink / raw)
To: linux-arm-kernel
El 24/07/15 a las 17:57, Ariel D'Alessandro escibi?:
>
>
> El 24/07/15 a las 11:04, Guenter Roeck escibi?:
>> On 07/24/2015 06:54 AM, Ariel D'Alessandro wrote:
>>>
>>>
>>> El 23/07/15 a las 18:21, Guenter Roeck escibi?:
>>>> On 07/23/2015 01:38 PM, Ariel D'Alessandro wrote:
>>>>> Hi Guenter,
>>>>>
>>>>> El 21/07/15 a las 21:51, Guenter Roeck escibi?:
>>>>>> On 07/14/2015 05:50 PM, 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>
>>>>>>
>>>>>> Hi Ariel,
>>>>>>
>>>>>> sorry for the delayed response. Comments below.
>>>>>>
>>>>>> Guenter
>>>>>>
>>>>>>> ---
>>>>>>> drivers/watchdog/Kconfig | 11 ++
>>>>>>> drivers/watchdog/Makefile | 1 +
>>>>>>> drivers/watchdog/lpc18xx_wdt.c | 344
>>>>>>> +++++++++++++++++++++++++++++++++++++++++
>>>>>>> 3 files changed, 356 insertions(+)
>>>>>>> create mode 100644 drivers/watchdog/lpc18xx_wdt.c
>>>>>>>
>>>>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>>>>> index 742fbbc..af5e2e3 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 "LPC18xx/43xx Watchdog"
>>>>>>> + depends on ARCH_LPC18XX || COMPILE_TEST
>>>>>>> + 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..2b489fc
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/watchdog/lpc18xx_wdt.c
>>>>>>> @@ -0,0 +1,344 @@
>>>>>>> +/*
>>>>>>> + * 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
>>>>>>
>>>>>> Not used.
>>>>>
>>>>> That's right. I'll remove it.
>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +#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
>>>>>>> +
>>>>>>
>>>>>> This is (still) really low for Linux. Something like min(30,
>>>>>> max_timeout)
>>>>>> might make more sense.
>>>>>
>>>>> Remember that hardware limits timeout to a maximum value of 5.
>>>>>
>>>>> As you said before we could use a soft timer, but I not sure that's a
>>>>> good idea. As Ezequiel said [0], it might be adding bloat and we must
>>>>> consider that this watchdog controller is included in cortex-M MCUs.
>>>>>
>>>>> [0] http://www.spinics.net/lists/linux-watchdog/msg06904.html
>>>>>
>>>>
>>>> Further down you have
>>>> lpc18xx_wdt->wdt_dev.max_timeout = (LPC18XX_WDT_TC_MAX *
>>>> LPC18XX_WDT_CLK_DIV) / clk_rate;
>>>>
>>>> which seems to contradict this statement. If the maximum timeout is 5
>>>> seconds,
>>>> why don't you set max_timeout to a fixed value of 5 seconds ?
>>>
>>> Well, I think that's the less hardcoded way. It might allow the driver
>>> to be extended to another HW variant that configures the wdt clk rate.
>>>
>>> I see what's your point. As you proposed, using the same criterion I
>>> should do this:
>>>
>>> #define LPC18XX_WDT_DEF_TIMEOUT (unsigned int)30
>>>
>>> and further down, in probe function:
>>>
>>> lpc18xx_wdt->wdt_dev.timeout = min(LPC18XX_WDT_DEF_TIMEOUT,
>>> lpc18xx_wdt->wdt_dev.max_timeout);
>>>
>>> Is this close enough to your solution?
>>>
>> Yes, but why the typecast for LPC18XX_WDT_DEF_TIMEOUT ?
>>
>
> Otherwise the compiler will warn about a comparison of distinct pointer
> types in expansion of the macro 'min'.
>
> Should it be casted when using it better than in definition?
>
Just realized that a literal suffix would be better.
#define LPC18XX_WDT_DEF_TIMEOUT 30U
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
@ 2015-07-24 21:17 ` Ariel D'Alessandro
0 siblings, 0 replies; 24+ messages in thread
From: Ariel D'Alessandro @ 2015-07-24 21:17 UTC (permalink / raw)
To: Guenter Roeck, linux-watchdog; +Cc: wim, ezequiel, linux-arm-kernel, manabian
El 24/07/15 a las 17:57, Ariel D'Alessandro escibió:
>
>
> El 24/07/15 a las 11:04, Guenter Roeck escibió:
>> On 07/24/2015 06:54 AM, Ariel D'Alessandro wrote:
>>>
>>>
>>> El 23/07/15 a las 18:21, Guenter Roeck escibió:
>>>> On 07/23/2015 01:38 PM, Ariel D'Alessandro wrote:
>>>>> Hi Guenter,
>>>>>
>>>>> El 21/07/15 a las 21:51, Guenter Roeck escibió:
>>>>>> On 07/14/2015 05:50 PM, 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>
>>>>>>
>>>>>> Hi Ariel,
>>>>>>
>>>>>> sorry for the delayed response. Comments below.
>>>>>>
>>>>>> Guenter
>>>>>>
>>>>>>> ---
>>>>>>> drivers/watchdog/Kconfig | 11 ++
>>>>>>> drivers/watchdog/Makefile | 1 +
>>>>>>> drivers/watchdog/lpc18xx_wdt.c | 344
>>>>>>> +++++++++++++++++++++++++++++++++++++++++
>>>>>>> 3 files changed, 356 insertions(+)
>>>>>>> create mode 100644 drivers/watchdog/lpc18xx_wdt.c
>>>>>>>
>>>>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>>>>> index 742fbbc..af5e2e3 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 "LPC18xx/43xx Watchdog"
>>>>>>> + depends on ARCH_LPC18XX || COMPILE_TEST
>>>>>>> + 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..2b489fc
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/watchdog/lpc18xx_wdt.c
>>>>>>> @@ -0,0 +1,344 @@
>>>>>>> +/*
>>>>>>> + * 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
>>>>>>
>>>>>> Not used.
>>>>>
>>>>> That's right. I'll remove it.
>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +#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
>>>>>>> +
>>>>>>
>>>>>> This is (still) really low for Linux. Something like min(30,
>>>>>> max_timeout)
>>>>>> might make more sense.
>>>>>
>>>>> Remember that hardware limits timeout to a maximum value of 5.
>>>>>
>>>>> As you said before we could use a soft timer, but I not sure that's a
>>>>> good idea. As Ezequiel said [0], it might be adding bloat and we must
>>>>> consider that this watchdog controller is included in cortex-M MCUs.
>>>>>
>>>>> [0] http://www.spinics.net/lists/linux-watchdog/msg06904.html
>>>>>
>>>>
>>>> Further down you have
>>>> lpc18xx_wdt->wdt_dev.max_timeout = (LPC18XX_WDT_TC_MAX *
>>>> LPC18XX_WDT_CLK_DIV) / clk_rate;
>>>>
>>>> which seems to contradict this statement. If the maximum timeout is 5
>>>> seconds,
>>>> why don't you set max_timeout to a fixed value of 5 seconds ?
>>>
>>> Well, I think that's the less hardcoded way. It might allow the driver
>>> to be extended to another HW variant that configures the wdt clk rate.
>>>
>>> I see what's your point. As you proposed, using the same criterion I
>>> should do this:
>>>
>>> #define LPC18XX_WDT_DEF_TIMEOUT (unsigned int)30
>>>
>>> and further down, in probe function:
>>>
>>> lpc18xx_wdt->wdt_dev.timeout = min(LPC18XX_WDT_DEF_TIMEOUT,
>>> lpc18xx_wdt->wdt_dev.max_timeout);
>>>
>>> Is this close enough to your solution?
>>>
>> Yes, but why the typecast for LPC18XX_WDT_DEF_TIMEOUT ?
>>
>
> Otherwise the compiler will warn about a comparison of distinct pointer
> types in expansion of the macro 'min'.
>
> Should it be casted when using it better than in definition?
>
Just realized that a literal suffix would be better.
#define LPC18XX_WDT_DEF_TIMEOUT 30U
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
2015-07-24 20:57 ` Ariel D'Alessandro
@ 2015-07-24 22:45 ` Guenter Roeck
-1 siblings, 0 replies; 24+ messages in thread
From: Guenter Roeck @ 2015-07-24 22:45 UTC (permalink / raw)
To: linux-arm-kernel
On 07/24/2015 01:57 PM, Ariel D'Alessandro wrote:
>
>
> El 24/07/15 a las 11:04, Guenter Roeck escibi?:
>> On 07/24/2015 06:54 AM, Ariel D'Alessandro wrote:
>>>
>>>
>>> El 23/07/15 a las 18:21, Guenter Roeck escibi?:
>>>> On 07/23/2015 01:38 PM, Ariel D'Alessandro wrote:
>>>>> Hi Guenter,
>>>>>
>>>>> El 21/07/15 a las 21:51, Guenter Roeck escibi?:
>>>>>> On 07/14/2015 05:50 PM, 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>
>>>>>>
>>>>>> Hi Ariel,
>>>>>>
>>>>>> sorry for the delayed response. Comments below.
>>>>>>
>>>>>> Guenter
>>>>>>
>>>>>>> ---
>>>>>>> drivers/watchdog/Kconfig | 11 ++
>>>>>>> drivers/watchdog/Makefile | 1 +
>>>>>>> drivers/watchdog/lpc18xx_wdt.c | 344
>>>>>>> +++++++++++++++++++++++++++++++++++++++++
>>>>>>> 3 files changed, 356 insertions(+)
>>>>>>> create mode 100644 drivers/watchdog/lpc18xx_wdt.c
>>>>>>>
>>>>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>>>>> index 742fbbc..af5e2e3 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 "LPC18xx/43xx Watchdog"
>>>>>>> + depends on ARCH_LPC18XX || COMPILE_TEST
>>>>>>> + 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..2b489fc
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/watchdog/lpc18xx_wdt.c
>>>>>>> @@ -0,0 +1,344 @@
>>>>>>> +/*
>>>>>>> + * 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
>>>>>>
>>>>>> Not used.
>>>>>
>>>>> That's right. I'll remove it.
>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +#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
>>>>>>> +
>>>>>>
>>>>>> This is (still) really low for Linux. Something like min(30,
>>>>>> max_timeout)
>>>>>> might make more sense.
>>>>>
>>>>> Remember that hardware limits timeout to a maximum value of 5.
>>>>>
>>>>> As you said before we could use a soft timer, but I not sure that's a
>>>>> good idea. As Ezequiel said [0], it might be adding bloat and we must
>>>>> consider that this watchdog controller is included in cortex-M MCUs.
>>>>>
>>>>> [0] http://www.spinics.net/lists/linux-watchdog/msg06904.html
>>>>>
>>>>
>>>> Further down you have
>>>> lpc18xx_wdt->wdt_dev.max_timeout = (LPC18XX_WDT_TC_MAX *
>>>> LPC18XX_WDT_CLK_DIV) / clk_rate;
>>>>
>>>> which seems to contradict this statement. If the maximum timeout is 5
>>>> seconds,
>>>> why don't you set max_timeout to a fixed value of 5 seconds ?
>>>
>>> Well, I think that's the less hardcoded way. It might allow the driver
>>> to be extended to another HW variant that configures the wdt clk rate.
>>>
>>> I see what's your point. As you proposed, using the same criterion I
>>> should do this:
>>>
>>> #define LPC18XX_WDT_DEF_TIMEOUT (unsigned int)30
>>>
>>> and further down, in probe function:
>>>
>>> lpc18xx_wdt->wdt_dev.timeout = min(LPC18XX_WDT_DEF_TIMEOUT,
>>> lpc18xx_wdt->wdt_dev.max_timeout);
>>>
>>> Is this close enough to your solution?
>>>
>> Yes, but why the typecast for LPC18XX_WDT_DEF_TIMEOUT ?
>>
>
> Otherwise the compiler will warn about a comparison of distinct pointer
> types in expansion of the macro 'min'.
>
> Should it be casted when using it better than in definition?
>
Pointer types sounds odd. What exactly is the message ?
How about using the following ?
#define LPC18XX_WDT_DEF_TIMEOUT 30U
Guenter
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
@ 2015-07-24 22:45 ` Guenter Roeck
0 siblings, 0 replies; 24+ messages in thread
From: Guenter Roeck @ 2015-07-24 22:45 UTC (permalink / raw)
To: Ariel D'Alessandro, linux-watchdog
Cc: wim, ezequiel, linux-arm-kernel, manabian
On 07/24/2015 01:57 PM, Ariel D'Alessandro wrote:
>
>
> El 24/07/15 a las 11:04, Guenter Roeck escibió:
>> On 07/24/2015 06:54 AM, Ariel D'Alessandro wrote:
>>>
>>>
>>> El 23/07/15 a las 18:21, Guenter Roeck escibió:
>>>> On 07/23/2015 01:38 PM, Ariel D'Alessandro wrote:
>>>>> Hi Guenter,
>>>>>
>>>>> El 21/07/15 a las 21:51, Guenter Roeck escibió:
>>>>>> On 07/14/2015 05:50 PM, 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>
>>>>>>
>>>>>> Hi Ariel,
>>>>>>
>>>>>> sorry for the delayed response. Comments below.
>>>>>>
>>>>>> Guenter
>>>>>>
>>>>>>> ---
>>>>>>> drivers/watchdog/Kconfig | 11 ++
>>>>>>> drivers/watchdog/Makefile | 1 +
>>>>>>> drivers/watchdog/lpc18xx_wdt.c | 344
>>>>>>> +++++++++++++++++++++++++++++++++++++++++
>>>>>>> 3 files changed, 356 insertions(+)
>>>>>>> create mode 100644 drivers/watchdog/lpc18xx_wdt.c
>>>>>>>
>>>>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>>>>> index 742fbbc..af5e2e3 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 "LPC18xx/43xx Watchdog"
>>>>>>> + depends on ARCH_LPC18XX || COMPILE_TEST
>>>>>>> + 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..2b489fc
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/watchdog/lpc18xx_wdt.c
>>>>>>> @@ -0,0 +1,344 @@
>>>>>>> +/*
>>>>>>> + * 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
>>>>>>
>>>>>> Not used.
>>>>>
>>>>> That's right. I'll remove it.
>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +#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
>>>>>>> +
>>>>>>
>>>>>> This is (still) really low for Linux. Something like min(30,
>>>>>> max_timeout)
>>>>>> might make more sense.
>>>>>
>>>>> Remember that hardware limits timeout to a maximum value of 5.
>>>>>
>>>>> As you said before we could use a soft timer, but I not sure that's a
>>>>> good idea. As Ezequiel said [0], it might be adding bloat and we must
>>>>> consider that this watchdog controller is included in cortex-M MCUs.
>>>>>
>>>>> [0] http://www.spinics.net/lists/linux-watchdog/msg06904.html
>>>>>
>>>>
>>>> Further down you have
>>>> lpc18xx_wdt->wdt_dev.max_timeout = (LPC18XX_WDT_TC_MAX *
>>>> LPC18XX_WDT_CLK_DIV) / clk_rate;
>>>>
>>>> which seems to contradict this statement. If the maximum timeout is 5
>>>> seconds,
>>>> why don't you set max_timeout to a fixed value of 5 seconds ?
>>>
>>> Well, I think that's the less hardcoded way. It might allow the driver
>>> to be extended to another HW variant that configures the wdt clk rate.
>>>
>>> I see what's your point. As you proposed, using the same criterion I
>>> should do this:
>>>
>>> #define LPC18XX_WDT_DEF_TIMEOUT (unsigned int)30
>>>
>>> and further down, in probe function:
>>>
>>> lpc18xx_wdt->wdt_dev.timeout = min(LPC18XX_WDT_DEF_TIMEOUT,
>>> lpc18xx_wdt->wdt_dev.max_timeout);
>>>
>>> Is this close enough to your solution?
>>>
>> Yes, but why the typecast for LPC18XX_WDT_DEF_TIMEOUT ?
>>
>
> Otherwise the compiler will warn about a comparison of distinct pointer
> types in expansion of the macro 'min'.
>
> Should it be casted when using it better than in definition?
>
Pointer types sounds odd. What exactly is the message ?
How about using the following ?
#define LPC18XX_WDT_DEF_TIMEOUT 30U
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
2015-07-24 22:45 ` Guenter Roeck
@ 2015-07-26 20:49 ` Ariel D'Alessandro
-1 siblings, 0 replies; 24+ messages in thread
From: Ariel D'Alessandro @ 2015-07-26 20:49 UTC (permalink / raw)
To: linux-arm-kernel
Guenter,
El 24/07/15 a las 19:45, Guenter Roeck escibi?:
> On 07/24/2015 01:57 PM, Ariel D'Alessandro wrote:
>>
>>
>> El 24/07/15 a las 11:04, Guenter Roeck escibi?:
>>> On 07/24/2015 06:54 AM, Ariel D'Alessandro wrote:
>>>>
>>>>
>>>> El 23/07/15 a las 18:21, Guenter Roeck escibi?:
>>>>> On 07/23/2015 01:38 PM, Ariel D'Alessandro wrote:
>>>>>> Hi Guenter,
>>>>>>
>>>>>> El 21/07/15 a las 21:51, Guenter Roeck escibi?:
>>>>>>> On 07/14/2015 05:50 PM, 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>
>>>>>>>
>>>>>>> Hi Ariel,
>>>>>>>
>>>>>>> sorry for the delayed response. Comments below.
>>>>>>>
>>>>>>> Guenter
>>>>>>>
>>>>>>>> ---
>>>>>>>> drivers/watchdog/Kconfig | 11 ++
>>>>>>>> drivers/watchdog/Makefile | 1 +
>>>>>>>> drivers/watchdog/lpc18xx_wdt.c | 344
>>>>>>>> +++++++++++++++++++++++++++++++++++++++++
>>>>>>>> 3 files changed, 356 insertions(+)
>>>>>>>> create mode 100644 drivers/watchdog/lpc18xx_wdt.c
>>>>>>>>
>>>>>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>>>>>> index 742fbbc..af5e2e3 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 "LPC18xx/43xx Watchdog"
>>>>>>>> + depends on ARCH_LPC18XX || COMPILE_TEST
>>>>>>>> + 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..2b489fc
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/drivers/watchdog/lpc18xx_wdt.c
>>>>>>>> @@ -0,0 +1,344 @@
>>>>>>>> +/*
>>>>>>>> + * 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
>>>>>>>
>>>>>>> Not used.
>>>>>>
>>>>>> That's right. I'll remove it.
>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>> +#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
>>>>>>>> +
>>>>>>>
>>>>>>> This is (still) really low for Linux. Something like min(30,
>>>>>>> max_timeout)
>>>>>>> might make more sense.
>>>>>>
>>>>>> Remember that hardware limits timeout to a maximum value of 5.
>>>>>>
>>>>>> As you said before we could use a soft timer, but I not sure that's a
>>>>>> good idea. As Ezequiel said [0], it might be adding bloat and we must
>>>>>> consider that this watchdog controller is included in cortex-M MCUs.
>>>>>>
>>>>>> [0] http://www.spinics.net/lists/linux-watchdog/msg06904.html
>>>>>>
>>>>>
>>>>> Further down you have
>>>>> lpc18xx_wdt->wdt_dev.max_timeout = (LPC18XX_WDT_TC_MAX *
>>>>> LPC18XX_WDT_CLK_DIV) / clk_rate;
>>>>>
>>>>> which seems to contradict this statement. If the maximum timeout is 5
>>>>> seconds,
>>>>> why don't you set max_timeout to a fixed value of 5 seconds ?
>>>>
>>>> Well, I think that's the less hardcoded way. It might allow the driver
>>>> to be extended to another HW variant that configures the wdt clk rate.
>>>>
>>>> I see what's your point. As you proposed, using the same criterion I
>>>> should do this:
>>>>
>>>> #define LPC18XX_WDT_DEF_TIMEOUT (unsigned int)30
>>>>
>>>> and further down, in probe function:
>>>>
>>>> lpc18xx_wdt->wdt_dev.timeout = min(LPC18XX_WDT_DEF_TIMEOUT,
>>>> lpc18xx_wdt->wdt_dev.max_timeout);
>>>>
>>>> Is this close enough to your solution?
>>>>
>>> Yes, but why the typecast for LPC18XX_WDT_DEF_TIMEOUT ?
>>>
>>
>> Otherwise the compiler will warn about a comparison of distinct pointer
>> types in expansion of the macro 'min'.
>>
>> Should it be casted when using it better than in definition?
>>
> Pointer types sounds odd. What exactly is the message ?
drivers/watchdog/lpc18xx_wdt.c: In function 'lpc18xx_wdt_probe':
include/linux/kernel.h:721:17: warning: comparison of distinct pointer
types lacks a cast [enabled by default]
(void) (&_min1 == &_min2); \
^
drivers/watchdog/lpc18xx_wdt.c:262:33: note: in expansion of macro 'min'
lpc18xx_wdt->wdt_dev.timeout = min(lpc18xx_wdt->wdt_dev.max_timeout,
^
>
> How about using the following ?
>
> #define LPC18XX_WDT_DEF_TIMEOUT 30U
Yes, that's what I responded in another mail. Thanks for your
suggestions, I'm submitting v4 now with all the modifications you've
pointed out.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
@ 2015-07-26 20:49 ` Ariel D'Alessandro
0 siblings, 0 replies; 24+ messages in thread
From: Ariel D'Alessandro @ 2015-07-26 20:49 UTC (permalink / raw)
To: Guenter Roeck, linux-watchdog; +Cc: wim, ezequiel, linux-arm-kernel, manabian
Guenter,
El 24/07/15 a las 19:45, Guenter Roeck escibió:
> On 07/24/2015 01:57 PM, Ariel D'Alessandro wrote:
>>
>>
>> El 24/07/15 a las 11:04, Guenter Roeck escibió:
>>> On 07/24/2015 06:54 AM, Ariel D'Alessandro wrote:
>>>>
>>>>
>>>> El 23/07/15 a las 18:21, Guenter Roeck escibió:
>>>>> On 07/23/2015 01:38 PM, Ariel D'Alessandro wrote:
>>>>>> Hi Guenter,
>>>>>>
>>>>>> El 21/07/15 a las 21:51, Guenter Roeck escibió:
>>>>>>> On 07/14/2015 05:50 PM, 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>
>>>>>>>
>>>>>>> Hi Ariel,
>>>>>>>
>>>>>>> sorry for the delayed response. Comments below.
>>>>>>>
>>>>>>> Guenter
>>>>>>>
>>>>>>>> ---
>>>>>>>> drivers/watchdog/Kconfig | 11 ++
>>>>>>>> drivers/watchdog/Makefile | 1 +
>>>>>>>> drivers/watchdog/lpc18xx_wdt.c | 344
>>>>>>>> +++++++++++++++++++++++++++++++++++++++++
>>>>>>>> 3 files changed, 356 insertions(+)
>>>>>>>> create mode 100644 drivers/watchdog/lpc18xx_wdt.c
>>>>>>>>
>>>>>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>>>>>> index 742fbbc..af5e2e3 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 "LPC18xx/43xx Watchdog"
>>>>>>>> + depends on ARCH_LPC18XX || COMPILE_TEST
>>>>>>>> + 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..2b489fc
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/drivers/watchdog/lpc18xx_wdt.c
>>>>>>>> @@ -0,0 +1,344 @@
>>>>>>>> +/*
>>>>>>>> + * 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
>>>>>>>
>>>>>>> Not used.
>>>>>>
>>>>>> That's right. I'll remove it.
>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>> +#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
>>>>>>>> +
>>>>>>>
>>>>>>> This is (still) really low for Linux. Something like min(30,
>>>>>>> max_timeout)
>>>>>>> might make more sense.
>>>>>>
>>>>>> Remember that hardware limits timeout to a maximum value of 5.
>>>>>>
>>>>>> As you said before we could use a soft timer, but I not sure that's a
>>>>>> good idea. As Ezequiel said [0], it might be adding bloat and we must
>>>>>> consider that this watchdog controller is included in cortex-M MCUs.
>>>>>>
>>>>>> [0] http://www.spinics.net/lists/linux-watchdog/msg06904.html
>>>>>>
>>>>>
>>>>> Further down you have
>>>>> lpc18xx_wdt->wdt_dev.max_timeout = (LPC18XX_WDT_TC_MAX *
>>>>> LPC18XX_WDT_CLK_DIV) / clk_rate;
>>>>>
>>>>> which seems to contradict this statement. If the maximum timeout is 5
>>>>> seconds,
>>>>> why don't you set max_timeout to a fixed value of 5 seconds ?
>>>>
>>>> Well, I think that's the less hardcoded way. It might allow the driver
>>>> to be extended to another HW variant that configures the wdt clk rate.
>>>>
>>>> I see what's your point. As you proposed, using the same criterion I
>>>> should do this:
>>>>
>>>> #define LPC18XX_WDT_DEF_TIMEOUT (unsigned int)30
>>>>
>>>> and further down, in probe function:
>>>>
>>>> lpc18xx_wdt->wdt_dev.timeout = min(LPC18XX_WDT_DEF_TIMEOUT,
>>>> lpc18xx_wdt->wdt_dev.max_timeout);
>>>>
>>>> Is this close enough to your solution?
>>>>
>>> Yes, but why the typecast for LPC18XX_WDT_DEF_TIMEOUT ?
>>>
>>
>> Otherwise the compiler will warn about a comparison of distinct pointer
>> types in expansion of the macro 'min'.
>>
>> Should it be casted when using it better than in definition?
>>
> Pointer types sounds odd. What exactly is the message ?
drivers/watchdog/lpc18xx_wdt.c: In function 'lpc18xx_wdt_probe':
include/linux/kernel.h:721:17: warning: comparison of distinct pointer
types lacks a cast [enabled by default]
(void) (&_min1 == &_min2); \
^
drivers/watchdog/lpc18xx_wdt.c:262:33: note: in expansion of macro 'min'
lpc18xx_wdt->wdt_dev.timeout = min(lpc18xx_wdt->wdt_dev.max_timeout,
^
>
> How about using the following ?
>
> #define LPC18XX_WDT_DEF_TIMEOUT 30U
Yes, that's what I responded in another mail. Thanks for your
suggestions, I'm submitting v4 now with all the modifications you've
pointed out.
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2015-07-26 20:49 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-15 0:50 [PATCH v3 0/2] Add support for NXP LPC18xx Watchdog timer Ariel D'Alessandro
2015-07-15 0:50 ` Ariel D'Alessandro
2015-07-15 0:50 ` [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver Ariel D'Alessandro
2015-07-15 0:50 ` Ariel D'Alessandro
2015-07-22 0:51 ` Guenter Roeck
2015-07-22 0:51 ` Guenter Roeck
2015-07-23 20:38 ` Ariel D'Alessandro
2015-07-23 20:38 ` Ariel D'Alessandro
2015-07-23 21:21 ` Guenter Roeck
2015-07-23 21:21 ` Guenter Roeck
2015-07-24 13:54 ` Ariel D'Alessandro
2015-07-24 13:54 ` Ariel D'Alessandro
2015-07-24 14:04 ` Guenter Roeck
2015-07-24 14:04 ` Guenter Roeck
2015-07-24 20:57 ` Ariel D'Alessandro
2015-07-24 20:57 ` Ariel D'Alessandro
2015-07-24 21:17 ` Ariel D'Alessandro
2015-07-24 21:17 ` Ariel D'Alessandro
2015-07-24 22:45 ` Guenter Roeck
2015-07-24 22:45 ` Guenter Roeck
2015-07-26 20:49 ` Ariel D'Alessandro
2015-07-26 20:49 ` Ariel D'Alessandro
2015-07-15 0:50 ` [PATCH v3 2/2] DT: watchdog: Add NXP LPC18xx Watchdog Timer binding documentation Ariel D'Alessandro
2015-07-15 0:50 ` Ariel D'Alessandro
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.