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@xxxxxxxxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxxxx> >> + * >> + * 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@xxxxxxxxxxxxxxxxxxxx>"); >> +MODULE_DESCRIPTION("NXP LPC18xx Watchdog Timer Driver"); >> +MODULE_LICENSE("GPL v2"); >> > -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
