[PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
Ariel D'Alessandro
ariel at vanguardiasur.com.ar
Thu Jul 23 13:38:08 PDT 2015
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 at 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 at 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 at vanguardiasur.com.ar>");
>> +MODULE_DESCRIPTION("NXP LPC18xx Watchdog Timer Driver");
>> +MODULE_LICENSE("GPL v2");
>>
>
More information about the linux-arm-kernel
mailing list