From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from bh-25.webhostbox.net ([208.91.199.152]:41180 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934008AbbGVAvP (ORCPT ); Tue, 21 Jul 2015 20:51:15 -0400 Message-ID: <55AEE8F5.3020700@roeck-us.net> Date: Tue, 21 Jul 2015 17:51:01 -0700 From: Guenter Roeck MIME-Version: 1.0 To: Ariel D'Alessandro , linux-watchdog@vger.kernel.org CC: wim@iguana.be, ezequiel@vanguardiasur.com.ar, linux-arm-kernel@lists.infradead.org, manabian@gmail.com Subject: Re: [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver References: <1436921445-11138-1-git-send-email-ariel@vanguardiasur.com.ar> <1436921445-11138-2-git-send-email-ariel@vanguardiasur.com.ar> In-Reply-To: <1436921445-11138-2-git-send-email-ariel@vanguardiasur.com.ar> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-watchdog-owner@vger.kernel.org List-Id: linux-watchdog@vger.kernel.org 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 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 > + * > + * 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 > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* 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 "); > +MODULE_DESCRIPTION("NXP LPC18xx Watchdog Timer Driver"); > +MODULE_LICENSE("GPL v2"); >