Message ID | d1efd94bfbc92b89e246b0b745d976d547b145b6.1509070855.git.eric.long@spreadtrum.com |
---|---|
State | Not Applicable, archived |
Headers | show |
Series | [v3,1/2] dt-bindings: watchdog: Add Spreadtrum watchdog documentation | expand |
On 10/26/2017 07:28 PM, Eric Long wrote: > This patch adds the watchdog driver for Spreadtrum SC9860 platform. > > Signed-off-by: Eric Long <eric.long@spreadtrum.com> > --- > Change since v2: > - Rename all the macors, add SPRD tag at the head of the macro names. > - Rename SPRD_WDT_CLK as SPRD_WTC_CNT_STEP. > - Remove the code which check timeout value at the wrong place. > - Add min/max timeout value limit. > - Remove set WDOG_HW_RUNNING status at sprd_wdt_enable(). > - Add timeout/pretimeout judgment when set them. > - Support WATCHDOG_NOWAYOUT status. > > Changes since v1: > - Use pretimeout instead of own implementation. > - Fix timeout loop when loading timeout values. > - use the infrastructure to read and set "timeout-sec" property. > - Add conditions when start or stop watchdog. > - Change the position of enabling watchdog. > - Other optimization. > --- > drivers/watchdog/Kconfig | 8 + > drivers/watchdog/Makefile | 1 + > drivers/watchdog/sprd_wdt.c | 415 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 424 insertions(+) > create mode 100644 drivers/watchdog/sprd_wdt.c > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index c722cbf..4a77e17 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -787,6 +787,14 @@ config UNIPHIER_WATCHDOG > To compile this driver as a module, choose M here: the > module will be called uniphier_wdt. > > +config SPRD_WATCHDOG > + tristate "Spreadtrum watchdog support" > + depends on ARCH_SPRD || COMPILE_TEST > + select WATCHDOG_CORE > + help > + Say Y here to include support watchdog timer embedded > + into the Spreadtrum system. Maybe better "supported by" instead of "embedded into". Or at least "embedded in". > + > # AVR32 Architecture > > config AT32AP700X_WDT > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 56adf9f..187cca2 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -87,6 +87,7 @@ obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o > obj-$(CONFIG_ZX2967_WATCHDOG) += zx2967_wdt.o > obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o > obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o > +obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o > > # AVR32 Architecture > obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o > diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c > new file mode 100644 > index 0000000..5fc3dde > --- /dev/null > +++ b/drivers/watchdog/sprd_wdt.c > @@ -0,0 +1,415 @@ > +/* > + * Spreadtrum watchdog driver > + * Copyright (C) 2017 Spreadtrum - http://www.spreadtrum.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. > + * > + * This program is distributed in the hope that it will be useful, but > + * WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * General Public License for more details. > + */ > + > +#include <linux/bitops.h> > +#include <linux/clk.h> > +#include <linux/err.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/platform_device.h> > +#include <linux/watchdog.h> > + > +#define SPRD_WDT_LOAD_LOW 0x0 > +#define SPRD_WDT_LOAD_HIGH 0x4 > +#define SPRD_WDT_CTRL 0x8 > +#define SPRD_WDT_INT_CLR 0xc > +#define SPRD_WDT_INT_RAW 0x10 > +#define SPRD_WDT_INT_MSK 0x14 > +#define SPRD_WDT_CNT_LOW 0x18 > +#define SPRD_WDT_CNT_HIGH 0x1c > +#define SPRD_WDT_LOCK 0x20 > +#define SPRD_WDT_IRQ_LOAD_LOW 0x2c > +#define SPRD_WDT_IRQ_LOAD_HIGH 0x30 > + > +/* WDT_CTRL */ > +#define SPRD_WDT_INT_EN_BIT BIT(0) > +#define SPRD_WDT_CNT_EN_BIT BIT(1) > +#define SPRD_WDT_NEW_VER_EN BIT(2) > +#define SPRD_WDT_RST_EN_BIT BIT(3) > + > +/* WDT_INT_CLR */ > +#define SPRD_WDT_INT_CLEAR_BIT BIT(0) > +#define SPRD_WDT_RST_CLEAR_BIT BIT(3) > + > +/* WDT_INT_RAW */ > +#define SPRD_WDT_INT_RAW_BIT BIT(0) > +#define SPRD_WDT_RST_RAW_BIT BIT(3) > +#define SPRD_WDT_LD_BUSY_BIT BIT(4) > + > +/* 1s equale to 32768 counter steps */ equal ? > +#define SPRD_WDT_CNT_STEP 32768 > + > +#define SPRD_WDT_UNLOCK_KEY 0xe551 > +#define SPRD_WDT_MIN_TIMROUT 3 TIMROUT ot TIMEOUT ? > +#define SPRD_WDT_MAX_TIMEOUT 60 Is that really the maximum supported timeout ? Seems a bit low. Shouldn't it be something like (U32_MAX / SPRD_WDT_CNT_STEP) ? > + > +#define SPRD_WDT_CNT_HIGH_VALUE 16 Maybe name it "SPRD_WDT_CNT_HIGH_SHIFT". It is not really a value, it is a shift. > +#define SPRD_WDT_LOW_VALUE_MASK GENMASK(15, 0) > +#define SPRD_WDT_CNT_VALUE_MAX GENMASK(31, 0) Does this mask serve a useful purpose ? > +#define SPRD_WDT_LOAD_TIMEOUT 1000 > + > +struct sprd_wdt { > + void __iomem *base; > + struct watchdog_device wdd; > + struct clk *enable; > + struct clk *rtc_enable; > + unsigned int irq; > +}; > + > +static inline struct sprd_wdt *to_sprd_wdt(struct watchdog_device *wdd) > +{ > + return container_of(wdd, struct sprd_wdt, wdd); > +} > + > +static inline void sprd_wdt_lock(void __iomem *addr) > +{ > + writel_relaxed(0x0, addr + SPRD_WDT_LOCK); > +} > + > +static inline void sprd_wdt_unlock(void __iomem *addr) > +{ > + writel_relaxed(SPRD_WDT_UNLOCK_KEY, addr + SPRD_WDT_LOCK); > +} > + > +static inline bool sprd_wdt_is_running(struct sprd_wdt *wdt) > +{ > + u32 val; > + > + val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); > + return val & SPRD_WDT_NEW_VER_EN; > +} > + > +static irqreturn_t sprd_wdt_isr(int irq, void *dev_id) > +{ > + struct sprd_wdt *wdt = (struct sprd_wdt *)dev_id; > + > + sprd_wdt_unlock(wdt->base); > + writel_relaxed(SPRD_WDT_INT_CLEAR_BIT, wdt->base + SPRD_WDT_INT_CLR); > + sprd_wdt_lock(wdt->base); > + watchdog_notify_pretimeout(&wdt->wdd); > + return IRQ_HANDLED; > +} > + > +static u32 sprd_wdt_get_cnt_value(struct sprd_wdt *wdt) > +{ > + u32 val; > + > + val = readl_relaxed(wdt->base + SPRD_WDT_CNT_HIGH) << > + SPRD_WDT_CNT_HIGH_VALUE; > + val |= readl_relaxed(wdt->base + SPRD_WDT_CNT_LOW) & > + SPRD_WDT_LOW_VALUE_MASK; > + > + return val; > +} > + > +static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout, > + u32 pretimeout) > +{ > + u32 val, delay_cnt = 0; > + u32 tmr_step = timeout * SPRD_WDT_CNT_STEP; > + u32 prtmr_step = pretimeout * SPRD_WDT_CNT_STEP; > + > + sprd_wdt_unlock(wdt->base); > + writel_relaxed((tmr_step >> SPRD_WDT_CNT_HIGH_VALUE) & > + SPRD_WDT_LOW_VALUE_MASK, wdt->base + SPRD_WDT_LOAD_HIGH); > + writel_relaxed((tmr_step & SPRD_WDT_LOW_VALUE_MASK), > + wdt->base + SPRD_WDT_LOAD_LOW); > + writel_relaxed((prtmr_step >> SPRD_WDT_CNT_HIGH_VALUE) & > + SPRD_WDT_LOW_VALUE_MASK, > + wdt->base + SPRD_WDT_IRQ_LOAD_HIGH); > + writel_relaxed(prtmr_step & SPRD_WDT_LOW_VALUE_MASK, > + wdt->base + SPRD_WDT_IRQ_LOAD_LOW); > + sprd_wdt_lock(wdt->base); > + > + /* > + * Waiting the load value operation done, > + * it needs two or three RTC clock cycles. > + */ > + do { > + val = readl_relaxed(wdt->base + SPRD_WDT_INT_RAW); > + if (!(val & SPRD_WDT_LD_BUSY_BIT)) > + break; > + > + cpu_relax(); > + } while (delay_cnt++ < SPRD_WDT_LOAD_TIMEOUT); > + > + if (delay_cnt >= SPRD_WDT_LOAD_TIMEOUT) > + return -EBUSY; > + return 0; > +} > + > +static int sprd_wdt_enable(struct sprd_wdt *wdt) > +{ > + u32 val; > + int ret; > + > + ret = clk_prepare_enable(wdt->enable); > + if (ret) > + return ret; > + ret = clk_prepare_enable(wdt->rtc_enable); > + if (ret) > + return ret; > + > + sprd_wdt_unlock(wdt->base); > + val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); > + val |= SPRD_WDT_NEW_VER_EN; > + writel_relaxed(val, wdt->base + SPRD_WDT_CTRL); > + sprd_wdt_lock(wdt->base); > + return 0; > +} > + > +static void sprd_wdt_disable(struct sprd_wdt *wdt) > +{ > + sprd_wdt_unlock(wdt->base); > + writel_relaxed(0x0, wdt->base + SPRD_WDT_CTRL); > + sprd_wdt_lock(wdt->base); > + > + clk_disable_unprepare(wdt->rtc_enable); > + clk_disable_unprepare(wdt->enable); > +} > + > +static int sprd_wdt_start(struct watchdog_device *wdd) > +{ > + struct sprd_wdt *wdt = to_sprd_wdt(wdd); > + u32 val; > + int ret; > + > + ret = sprd_wdt_load_value(wdt, wdd->timeout, wdd->pretimeout); > + if (ret) > + return ret; > + > + sprd_wdt_unlock(wdt->base); > + val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); > + val |= SPRD_WDT_CNT_EN_BIT | SPRD_WDT_INT_EN_BIT | SPRD_WDT_RST_EN_BIT; > + writel_relaxed(val, wdt->base + SPRD_WDT_CTRL); > + sprd_wdt_lock(wdt->base); > + set_bit(WDOG_HW_RUNNING, &wdd->status); > + > + return 0; > +} > + > +static int sprd_wdt_stop(struct watchdog_device *wdd) > +{ > + struct sprd_wdt *wdt = to_sprd_wdt(wdd); > + u32 val; > + > + sprd_wdt_unlock(wdt->base); > + val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); > + val &= ~(SPRD_WDT_CNT_EN_BIT | SPRD_WDT_RST_EN_BIT | > + SPRD_WDT_INT_EN_BIT); > + writel_relaxed(val, wdt->base + SPRD_WDT_CTRL); > + sprd_wdt_lock(wdt->base); > + return 0; > +} > + > +static int sprd_wdt_set_timeout(struct watchdog_device *wdd, > + u32 timeout) > +{ > + struct sprd_wdt *wdt = to_sprd_wdt(wdd); > + > + if (timeout == wdd->timeout) > + return 0; > + > + wdd->timeout = timeout; > + > + return sprd_wdt_load_value(wdt, timeout, wdd->pretimeout); > +} > + > +static int sprd_wdt_set_pretimeout(struct watchdog_device *wdd, > + u32 new_pretimeout) > +{ > + struct sprd_wdt *wdt = to_sprd_wdt(wdd); > + > + if (new_pretimeout == wdd->pretimeout) > + return 0; This is inconsistent. pretimeout == 0 is accepted until it is set to a non-zero value once, then 0 is no longer accepted. pretimeout==0 should reflect "pretimeout disabled". If that is not possible you need to set some non-0 default value in the probe function. > + if (new_pretimeout <= wdd->min_timeout) > + return -EINVAL; Why is pretimeout == wdd->min_timeout invalid ? > + > + wdd->pretimeout = new_pretimeout; > + > + return sprd_wdt_load_value(wdt, wdd->timeout, new_pretimeout); > +} > + > +static u32 sprd_wdt_get_timeleft(struct watchdog_device *wdd) > +{ > + struct sprd_wdt *wdt = to_sprd_wdt(wdd); > + u32 val; > + > + val = sprd_wdt_get_cnt_value(wdt); > + val = val / SPRD_WDT_CNT_STEP; > + > + return val; > +} > + > +static const struct watchdog_ops sprd_wdt_ops = { > + .owner = THIS_MODULE, > + .start = sprd_wdt_start, > + .stop = sprd_wdt_stop, > + .set_timeout = sprd_wdt_set_timeout, > + .set_pretimeout = sprd_wdt_set_pretimeout, > + .get_timeleft = sprd_wdt_get_timeleft, Just wondering - no heartbeat function ? Having to load the timer values for each heartbeat is expensive. > +}; > + > +static const struct watchdog_info sprd_wdt_info = { > + .options = WDIOF_SETTIMEOUT | > + WDIOF_PRETIMEOUT | > + WDIOF_MAGICCLOSE | > + WDIOF_KEEPALIVEPING, > + .identity = "Spreadtrum Watchdog Timer", > +}; > + > +static int sprd_wdt_probe(struct platform_device *pdev) > +{ > + struct resource *wdt_res; > + struct sprd_wdt *wdt; > + int ret; > + > + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); > + if (!wdt) > + return -ENOMEM; > + > + wdt_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!wdt_res) { > + dev_err(&pdev->dev, "failed to memory resource\n"); > + return -ENOMEM; > + } Unnecessary error check and message. devm_ioremap_resource() returns an error if res is NULL. > + > + wdt->base = devm_ioremap_resource(&pdev->dev, wdt_res); > + if (IS_ERR(wdt->base)) Move the error message to here. > + return PTR_ERR(wdt->base); > + > + wdt->enable = devm_clk_get(&pdev->dev, "enable"); > + if (IS_ERR(wdt->enable)) { > + dev_err(&pdev->dev, "can't get the enable clock\n"); > + return PTR_ERR(wdt->enable); > + } > + > + wdt->rtc_enable = devm_clk_get(&pdev->dev, "rtc_enable"); > + if (IS_ERR(wdt->rtc_enable)) { > + dev_err(&pdev->dev, "can't get the rtc enable clock\n"); > + return PTR_ERR(wdt->rtc_enable); > + } > + > + wdt->irq = platform_get_irq(pdev, 0); > + if (wdt->irq < 0) { > + dev_err(&pdev->dev, "failed to get IRQ resource\n"); > + return wdt->irq; > + } > + > + ret = devm_request_irq(&pdev->dev, wdt->irq, sprd_wdt_isr, > + IRQF_NO_SUSPEND, "sprd-wdt", (void *)wdt); > + if (ret) { > + dev_err(&pdev->dev, "failed to register irq\n"); > + return ret; > + } > + > + wdt->wdd.info = &sprd_wdt_info; > + wdt->wdd.ops = &sprd_wdt_ops; > + wdt->wdd.parent = &pdev->dev; > + wdt->wdd.min_timeout = SPRD_WDT_MIN_TIMROUT; > + wdt->wdd.max_timeout = SPRD_WDT_MAX_TIMEOUT; > + You might want to set wdt->wdd.timeout to a default value. > + ret = sprd_wdt_enable(wdt); > + if (ret) { > + dev_err(&pdev->dev, "failed to enable wdt\n"); > + return ret; > + } > + This needs a matching sprd_wdt_disable() call in the remove function. Best way to handle this would be to add devm_add_action() here. > + watchdog_set_nowayout(&wdt->wdd, WATCHDOG_NOWAYOUT); > + watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev); > + > + ret = watchdog_register_device(&wdt->wdd); Unfortunately this doesn't work. It leaves interrupts enabled after the watchdog device is removed in sprd_wdt_remove(), but the driver will be gone. I would suggest to use devm_watchdog_register_device() and reorder calls to request the interrupt after registering the watchdog device. Then you can drop the remove function entirely. > + if (ret) { > + sprd_wdt_disable(wdt); > + dev_err(&pdev->dev, "failed to register watchdog\n"); > + return ret; > + } > + platform_set_drvdata(pdev, wdt); > + > + return 0; > +} > + > +static int sprd_wdt_remove(struct platform_device *pdev) > +{ > + struct sprd_wdt *wdt = platform_get_drvdata(pdev); > + > + watchdog_unregister_device(&wdt->wdd); > + > + if (sprd_wdt_is_running(wdt)) > + dev_crit(&pdev->dev, "Device removed: Expect reboot!\n"); > + return 0; > +} > + > +static int __maybe_unused sprd_wdt_pm_suspend(struct device *dev) > +{ > + struct watchdog_device *wdd = dev_get_drvdata(dev); > + struct sprd_wdt *wdt = dev_get_drvdata(dev); > + > + if (watchdog_active(wdd)) { > + sprd_wdt_stop(&wdt->wdd); > + sprd_wdt_disable(wdt); Are you sure you only want to disable the clocks if the watchdog was active ? That seems to be a bit inconsistent. > + } > + > + return 0; > +} > + > +static int __maybe_unused sprd_wdt_pm_resume(struct device *dev) > +{ > + struct watchdog_device *wdd = dev_get_drvdata(dev); > + struct sprd_wdt *wdt = dev_get_drvdata(dev); > + int ret; > + > + if (watchdog_active(wdd)) { > + ret = sprd_wdt_enable(wdt); > + if (ret) > + return ret; > + ret = sprd_wdt_start(&wdt->wdd); > + if (ret) { > + sprd_wdt_disable(wdt); > + return ret; > + } This can leave the system in an inconsistent state. Sometimes the wdt may be disabled, sometimes not. > + } > + > + return 0; > +} > + > +static const struct dev_pm_ops sprd_wdt_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(sprd_wdt_pm_suspend, > + sprd_wdt_pm_resume) > +}; > + > +static const struct of_device_id sprd_wdt_match_table[] = { > + { .compatible = "sprd,sp9860-wdt", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, sprd_wdt_match_table); > + > +static struct platform_driver sprd_watchdog_driver = { > + .probe = sprd_wdt_probe, > + .remove = sprd_wdt_remove, > + .driver = { > + .name = "sprd-wdt", > + .of_match_table = sprd_wdt_match_table, > + .pm = &sprd_wdt_pm_ops, > + }, > +}; > +module_platform_driver(sprd_watchdog_driver); > + > +MODULE_AUTHOR("Eric Long <eric.long@spreadtrum.com>"); > +MODULE_DESCRIPTION("Spreadtrum Watchdog Timer Controller Driver"); > +MODULE_LICENSE("GPL v2"); > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guenter, Thanks for you commonts. On Thu, Oct 26, 2017 at 10:23:40PM -0700, Guenter Roeck wrote: > On 10/26/2017 07:28 PM, Eric Long wrote: > >This patch adds the watchdog driver for Spreadtrum SC9860 platform. > > > >Signed-off-by: Eric Long <eric.long@spreadtrum.com> > >--- > >Change since v2: > > - Rename all the macors, add SPRD tag at the head of the macro names. > > - Rename SPRD_WDT_CLK as SPRD_WTC_CNT_STEP. > > - Remove the code which check timeout value at the wrong place. > > - Add min/max timeout value limit. > > - Remove set WDOG_HW_RUNNING status at sprd_wdt_enable(). > > - Add timeout/pretimeout judgment when set them. > > - Support WATCHDOG_NOWAYOUT status. > > > >Changes since v1: > > - Use pretimeout instead of own implementation. > > - Fix timeout loop when loading timeout values. > > - use the infrastructure to read and set "timeout-sec" property. > > - Add conditions when start or stop watchdog. > > - Change the position of enabling watchdog. > > - Other optimization. > >--- > > drivers/watchdog/Kconfig | 8 + > > drivers/watchdog/Makefile | 1 + > > drivers/watchdog/sprd_wdt.c | 415 ++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 424 insertions(+) > > create mode 100644 drivers/watchdog/sprd_wdt.c > > > >diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > >index c722cbf..4a77e17 100644 > >--- a/drivers/watchdog/Kconfig > >+++ b/drivers/watchdog/Kconfig > >@@ -787,6 +787,14 @@ config UNIPHIER_WATCHDOG > > To compile this driver as a module, choose M here: the > > module will be called uniphier_wdt. > >+config SPRD_WATCHDOG > >+ tristate "Spreadtrum watchdog support" > >+ depends on ARCH_SPRD || COMPILE_TEST > >+ select WATCHDOG_CORE > >+ help > >+ Say Y here to include support watchdog timer embedded > >+ into the Spreadtrum system. > > Maybe better "supported by" instead of "embedded into". Or at least > "embedded in". OK, I will fix it. > >+ > > # AVR32 Architecture > > config AT32AP700X_WDT > >diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > >index 56adf9f..187cca2 100644 > >--- a/drivers/watchdog/Makefile > >+++ b/drivers/watchdog/Makefile > >@@ -87,6 +87,7 @@ obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o > > obj-$(CONFIG_ZX2967_WATCHDOG) += zx2967_wdt.o > > obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o > > obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o > >+obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o > > # AVR32 Architecture > > obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o > >diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c > >new file mode 100644 > >index 0000000..5fc3dde > >--- /dev/null > >+++ b/drivers/watchdog/sprd_wdt.c > >@@ -0,0 +1,415 @@ > >+/* > >+ * Spreadtrum watchdog driver > >+ * Copyright (C) 2017 Spreadtrum - http://www.spreadtrum.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. > >+ * > >+ * This program is distributed in the hope that it will be useful, but > >+ * WITHOUT ANY WARRANTY; without even the implied warranty of > >+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > >+ * General Public License for more details. > >+ */ > >+ > >+#include <linux/bitops.h> > >+#include <linux/clk.h> > >+#include <linux/err.h> > >+#include <linux/interrupt.h> > >+#include <linux/io.h> > >+#include <linux/kernel.h> > >+#include <linux/module.h> > >+#include <linux/of.h> > >+#include <linux/of_address.h> > >+#include <linux/platform_device.h> > >+#include <linux/watchdog.h> > >+ > >+#define SPRD_WDT_LOAD_LOW 0x0 > >+#define SPRD_WDT_LOAD_HIGH 0x4 > >+#define SPRD_WDT_CTRL 0x8 > >+#define SPRD_WDT_INT_CLR 0xc > >+#define SPRD_WDT_INT_RAW 0x10 > >+#define SPRD_WDT_INT_MSK 0x14 > >+#define SPRD_WDT_CNT_LOW 0x18 > >+#define SPRD_WDT_CNT_HIGH 0x1c > >+#define SPRD_WDT_LOCK 0x20 > >+#define SPRD_WDT_IRQ_LOAD_LOW 0x2c > >+#define SPRD_WDT_IRQ_LOAD_HIGH 0x30 > >+ > >+/* WDT_CTRL */ > >+#define SPRD_WDT_INT_EN_BIT BIT(0) > >+#define SPRD_WDT_CNT_EN_BIT BIT(1) > >+#define SPRD_WDT_NEW_VER_EN BIT(2) > >+#define SPRD_WDT_RST_EN_BIT BIT(3) > >+ > >+/* WDT_INT_CLR */ > >+#define SPRD_WDT_INT_CLEAR_BIT BIT(0) > >+#define SPRD_WDT_RST_CLEAR_BIT BIT(3) > >+ > >+/* WDT_INT_RAW */ > >+#define SPRD_WDT_INT_RAW_BIT BIT(0) > >+#define SPRD_WDT_RST_RAW_BIT BIT(3) > >+#define SPRD_WDT_LD_BUSY_BIT BIT(4) > >+ > >+/* 1s equale to 32768 counter steps */ > > equal ? > OK, I will fix it. > >+#define SPRD_WDT_CNT_STEP 32768 > >+ > >+#define SPRD_WDT_UNLOCK_KEY 0xe551 > >+#define SPRD_WDT_MIN_TIMROUT 3 > > TIMROUT ot TIMEOUT ? > OK, I will fix it. > >+#define SPRD_WDT_MAX_TIMEOUT 60 > > Is that really the maximum supported timeout ? Seems a bit low. > Shouldn't it be something like (U32_MAX / SPRD_WDT_CNT_STEP) ? > It supports the max value like as U32_MAX/SPRD_WDT_CNT_STEP, but it doesn't unnecessary to support so big value, if the system can not response it in 60s, the watchdog could trigger timeout. > >+ > >+#define SPRD_WDT_CNT_HIGH_VALUE 16 > > Maybe name it "SPRD_WDT_CNT_HIGH_SHIFT". It is not really a value, > it is a shift. > Yes, you are right, _SHIFT will be better. > >+#define SPRD_WDT_LOW_VALUE_MASK GENMASK(15, 0) > >+#define SPRD_WDT_CNT_VALUE_MAX GENMASK(31, 0) > > Does this mask serve a useful purpose ? > OK, I will remove it, thanks! > >+#define SPRD_WDT_LOAD_TIMEOUT 1000 > >+ > >+struct sprd_wdt { > >+ void __iomem *base; > >+ struct watchdog_device wdd; > >+ struct clk *enable; > >+ struct clk *rtc_enable; > >+ unsigned int irq; > >+}; > >+ > >+static inline struct sprd_wdt *to_sprd_wdt(struct watchdog_device *wdd) > >+{ > >+ return container_of(wdd, struct sprd_wdt, wdd); > >+} > >+ > >+static inline void sprd_wdt_lock(void __iomem *addr) > >+{ > >+ writel_relaxed(0x0, addr + SPRD_WDT_LOCK); > >+} > >+ > >+static inline void sprd_wdt_unlock(void __iomem *addr) > >+{ > >+ writel_relaxed(SPRD_WDT_UNLOCK_KEY, addr + SPRD_WDT_LOCK); > >+} > >+ > >+static inline bool sprd_wdt_is_running(struct sprd_wdt *wdt) > >+{ > >+ u32 val; > >+ > >+ val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); > >+ return val & SPRD_WDT_NEW_VER_EN; > >+} > >+ > >+static irqreturn_t sprd_wdt_isr(int irq, void *dev_id) > >+{ > >+ struct sprd_wdt *wdt = (struct sprd_wdt *)dev_id; > >+ > >+ sprd_wdt_unlock(wdt->base); > >+ writel_relaxed(SPRD_WDT_INT_CLEAR_BIT, wdt->base + SPRD_WDT_INT_CLR); > >+ sprd_wdt_lock(wdt->base); > >+ watchdog_notify_pretimeout(&wdt->wdd); > >+ return IRQ_HANDLED; > >+} > >+ > >+static u32 sprd_wdt_get_cnt_value(struct sprd_wdt *wdt) > >+{ > >+ u32 val; > >+ > >+ val = readl_relaxed(wdt->base + SPRD_WDT_CNT_HIGH) << > >+ SPRD_WDT_CNT_HIGH_VALUE; > >+ val |= readl_relaxed(wdt->base + SPRD_WDT_CNT_LOW) & > >+ SPRD_WDT_LOW_VALUE_MASK; > >+ > >+ return val; > >+} > >+ > >+static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout, > >+ u32 pretimeout) > >+{ > >+ u32 val, delay_cnt = 0; > >+ u32 tmr_step = timeout * SPRD_WDT_CNT_STEP; > >+ u32 prtmr_step = pretimeout * SPRD_WDT_CNT_STEP; > >+ > >+ sprd_wdt_unlock(wdt->base); > >+ writel_relaxed((tmr_step >> SPRD_WDT_CNT_HIGH_VALUE) & > >+ SPRD_WDT_LOW_VALUE_MASK, wdt->base + SPRD_WDT_LOAD_HIGH); > >+ writel_relaxed((tmr_step & SPRD_WDT_LOW_VALUE_MASK), > >+ wdt->base + SPRD_WDT_LOAD_LOW); > >+ writel_relaxed((prtmr_step >> SPRD_WDT_CNT_HIGH_VALUE) & > >+ SPRD_WDT_LOW_VALUE_MASK, > >+ wdt->base + SPRD_WDT_IRQ_LOAD_HIGH); > >+ writel_relaxed(prtmr_step & SPRD_WDT_LOW_VALUE_MASK, > >+ wdt->base + SPRD_WDT_IRQ_LOAD_LOW); > >+ sprd_wdt_lock(wdt->base); > >+ > >+ /* > >+ * Waiting the load value operation done, > >+ * it needs two or three RTC clock cycles. > >+ */ > >+ do { > >+ val = readl_relaxed(wdt->base + SPRD_WDT_INT_RAW); > >+ if (!(val & SPRD_WDT_LD_BUSY_BIT)) > >+ break; > >+ > >+ cpu_relax(); > >+ } while (delay_cnt++ < SPRD_WDT_LOAD_TIMEOUT); > >+ > >+ if (delay_cnt >= SPRD_WDT_LOAD_TIMEOUT) > >+ return -EBUSY; > >+ return 0; > >+} > >+ > >+static int sprd_wdt_enable(struct sprd_wdt *wdt) > >+{ > >+ u32 val; > >+ int ret; > >+ > >+ ret = clk_prepare_enable(wdt->enable); > >+ if (ret) > >+ return ret; > >+ ret = clk_prepare_enable(wdt->rtc_enable); > >+ if (ret) > >+ return ret; > >+ > >+ sprd_wdt_unlock(wdt->base); > >+ val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); > >+ val |= SPRD_WDT_NEW_VER_EN; > >+ writel_relaxed(val, wdt->base + SPRD_WDT_CTRL); > >+ sprd_wdt_lock(wdt->base); > >+ return 0; > >+} > >+ > >+static void sprd_wdt_disable(struct sprd_wdt *wdt) > >+{ > >+ sprd_wdt_unlock(wdt->base); > >+ writel_relaxed(0x0, wdt->base + SPRD_WDT_CTRL); > >+ sprd_wdt_lock(wdt->base); > >+ > >+ clk_disable_unprepare(wdt->rtc_enable); > >+ clk_disable_unprepare(wdt->enable); > >+} > >+ > >+static int sprd_wdt_start(struct watchdog_device *wdd) > >+{ > >+ struct sprd_wdt *wdt = to_sprd_wdt(wdd); > >+ u32 val; > >+ int ret; > >+ > >+ ret = sprd_wdt_load_value(wdt, wdd->timeout, wdd->pretimeout); > >+ if (ret) > >+ return ret; > >+ > >+ sprd_wdt_unlock(wdt->base); > >+ val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); > >+ val |= SPRD_WDT_CNT_EN_BIT | SPRD_WDT_INT_EN_BIT | SPRD_WDT_RST_EN_BIT; > >+ writel_relaxed(val, wdt->base + SPRD_WDT_CTRL); > >+ sprd_wdt_lock(wdt->base); > >+ set_bit(WDOG_HW_RUNNING, &wdd->status); > >+ > >+ return 0; > >+} > >+ > >+static int sprd_wdt_stop(struct watchdog_device *wdd) > >+{ > >+ struct sprd_wdt *wdt = to_sprd_wdt(wdd); > >+ u32 val; > >+ > >+ sprd_wdt_unlock(wdt->base); > >+ val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); > >+ val &= ~(SPRD_WDT_CNT_EN_BIT | SPRD_WDT_RST_EN_BIT | > >+ SPRD_WDT_INT_EN_BIT); > >+ writel_relaxed(val, wdt->base + SPRD_WDT_CTRL); > >+ sprd_wdt_lock(wdt->base); > >+ return 0; > >+} > >+ > >+static int sprd_wdt_set_timeout(struct watchdog_device *wdd, > >+ u32 timeout) > >+{ > >+ struct sprd_wdt *wdt = to_sprd_wdt(wdd); > >+ > >+ if (timeout == wdd->timeout) > >+ return 0; > >+ > >+ wdd->timeout = timeout; > >+ > >+ return sprd_wdt_load_value(wdt, timeout, wdd->pretimeout); > >+} > >+ > >+static int sprd_wdt_set_pretimeout(struct watchdog_device *wdd, > >+ u32 new_pretimeout) > >+{ > >+ struct sprd_wdt *wdt = to_sprd_wdt(wdd); > >+ > >+ if (new_pretimeout == wdd->pretimeout) > >+ return 0; > > This is inconsistent. pretimeout == 0 is accepted until it is set > to a non-zero value once, then 0 is no longer accepted. pretimeout==0 > should reflect "pretimeout disabled". If that is not possible you > need to set some non-0 default value in the probe function. > I understand your opinion, I will fix it. > >+ if (new_pretimeout <= wdd->min_timeout) > >+ return -EINVAL; > > Why is pretimeout == wdd->min_timeout invalid ? > OK, you are right, it should be pretimeout < min_timeout. > >+ > >+ wdd->pretimeout = new_pretimeout; > >+ > >+ return sprd_wdt_load_value(wdt, wdd->timeout, new_pretimeout); > >+} > >+ > >+static u32 sprd_wdt_get_timeleft(struct watchdog_device *wdd) > >+{ > >+ struct sprd_wdt *wdt = to_sprd_wdt(wdd); > >+ u32 val; > >+ > >+ val = sprd_wdt_get_cnt_value(wdt); > >+ val = val / SPRD_WDT_CNT_STEP; > >+ > >+ return val; > >+} > >+ > >+static const struct watchdog_ops sprd_wdt_ops = { > >+ .owner = THIS_MODULE, > >+ .start = sprd_wdt_start, > >+ .stop = sprd_wdt_stop, > >+ .set_timeout = sprd_wdt_set_timeout, > >+ .set_pretimeout = sprd_wdt_set_pretimeout, > >+ .get_timeleft = sprd_wdt_get_timeleft, > > Just wondering - no heartbeat function ? Having to load the timer > values for each heartbeat is expensive. > Unfortunately, this watchdog has not RELOAD register. > >+}; > >+ > >+static const struct watchdog_info sprd_wdt_info = { > >+ .options = WDIOF_SETTIMEOUT | > >+ WDIOF_PRETIMEOUT | > >+ WDIOF_MAGICCLOSE | > >+ WDIOF_KEEPALIVEPING, > >+ .identity = "Spreadtrum Watchdog Timer", > >+}; > >+ > >+static int sprd_wdt_probe(struct platform_device *pdev) > >+{ > >+ struct resource *wdt_res; > >+ struct sprd_wdt *wdt; > >+ int ret; > >+ > >+ wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); > >+ if (!wdt) > >+ return -ENOMEM; > >+ > >+ wdt_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >+ if (!wdt_res) { > >+ dev_err(&pdev->dev, "failed to memory resource\n"); > >+ return -ENOMEM; > >+ } > > Unnecessary error check and message. devm_ioremap_resource() > returns an error if res is NULL. > OK, I will fix it. > >+ > >+ wdt->base = devm_ioremap_resource(&pdev->dev, wdt_res); > >+ if (IS_ERR(wdt->base)) > > Move the error message to here. > > >+ return PTR_ERR(wdt->base); > >+ > >+ wdt->enable = devm_clk_get(&pdev->dev, "enable"); > >+ if (IS_ERR(wdt->enable)) { > >+ dev_err(&pdev->dev, "can't get the enable clock\n"); > >+ return PTR_ERR(wdt->enable); > >+ } > >+ > >+ wdt->rtc_enable = devm_clk_get(&pdev->dev, "rtc_enable"); > >+ if (IS_ERR(wdt->rtc_enable)) { > >+ dev_err(&pdev->dev, "can't get the rtc enable clock\n"); > >+ return PTR_ERR(wdt->rtc_enable); > >+ } > >+ > >+ wdt->irq = platform_get_irq(pdev, 0); > >+ if (wdt->irq < 0) { > >+ dev_err(&pdev->dev, "failed to get IRQ resource\n"); > >+ return wdt->irq; > >+ } > >+ > >+ ret = devm_request_irq(&pdev->dev, wdt->irq, sprd_wdt_isr, > >+ IRQF_NO_SUSPEND, "sprd-wdt", (void *)wdt); > >+ if (ret) { > >+ dev_err(&pdev->dev, "failed to register irq\n"); > >+ return ret; > >+ } > >+ > >+ wdt->wdd.info = &sprd_wdt_info; > >+ wdt->wdd.ops = &sprd_wdt_ops; > >+ wdt->wdd.parent = &pdev->dev; > >+ wdt->wdd.min_timeout = SPRD_WDT_MIN_TIMROUT; > >+ wdt->wdd.max_timeout = SPRD_WDT_MAX_TIMEOUT; > >+ > You might want to set wdt->wdd.timeout to a default value. > OK, I will set a default timeout value in case of no timeout-sec in device-tree. > >+ ret = sprd_wdt_enable(wdt); > >+ if (ret) { > >+ dev_err(&pdev->dev, "failed to enable wdt\n"); > >+ return ret; > >+ } > >+ > > This needs a matching sprd_wdt_disable() call in the remove function. > Best way to handle this would be to add devm_add_action() here. > If add devm_add_action(), it will be called in the remove function, but this is not our expect, if someone remove this module, we want it just timeout. > >+ watchdog_set_nowayout(&wdt->wdd, WATCHDOG_NOWAYOUT); > >+ watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev); > >+ > >+ ret = watchdog_register_device(&wdt->wdd); > > Unfortunately this doesn't work. It leaves interrupts enabled > after the watchdog device is removed in sprd_wdt_remove(), > but the driver will be gone. I would suggest to use > devm_watchdog_register_device() and reorder calls to request > the interrupt after registering the watchdog device. Then you > can drop the remove function entirely. > Yes, you are right, I understand your opinion, and it is very important, devm_watchdog_register_device() is better. Thanks. > >+ if (ret) { > >+ sprd_wdt_disable(wdt); > >+ dev_err(&pdev->dev, "failed to register watchdog\n"); > >+ return ret; > >+ } > >+ platform_set_drvdata(pdev, wdt); > >+ > >+ return 0; > >+} > >+ > >+static int sprd_wdt_remove(struct platform_device *pdev) > >+{ > >+ struct sprd_wdt *wdt = platform_get_drvdata(pdev); > >+ > >+ watchdog_unregister_device(&wdt->wdd); > >+ > >+ if (sprd_wdt_is_running(wdt)) > >+ dev_crit(&pdev->dev, "Device removed: Expect reboot!\n"); > >+ return 0; > >+} > >+ > >+static int __maybe_unused sprd_wdt_pm_suspend(struct device *dev) > >+{ > >+ struct watchdog_device *wdd = dev_get_drvdata(dev); > >+ struct sprd_wdt *wdt = dev_get_drvdata(dev); > >+ > >+ if (watchdog_active(wdd)) { > >+ sprd_wdt_stop(&wdt->wdd); > >+ sprd_wdt_disable(wdt); > > Are you sure you only want to disable the clocks if the watchdog was active ? > That seems to be a bit inconsistent. > This watchdog is in always on power domain, if system suspend, it needs to disable it, or it will timeout. > >+ } > >+ > >+ return 0; > >+} > >+ > >+static int __maybe_unused sprd_wdt_pm_resume(struct device *dev) > >+{ > >+ struct watchdog_device *wdd = dev_get_drvdata(dev); > >+ struct sprd_wdt *wdt = dev_get_drvdata(dev); > >+ int ret; > >+ > >+ if (watchdog_active(wdd)) { > >+ ret = sprd_wdt_enable(wdt); > >+ if (ret) > >+ return ret; > >+ ret = sprd_wdt_start(&wdt->wdd); > >+ if (ret) { > >+ sprd_wdt_disable(wdt); > >+ return ret; > >+ } > > This can leave the system in an inconsistent state. Sometimes the wdt may be > disabled, sometimes not. > If watchdog enable failed, it means there is something wrong in this system. > >+ } > >+ > >+ return 0; > >+} > >+ > >+static const struct dev_pm_ops sprd_wdt_pm_ops = { > >+ SET_SYSTEM_SLEEP_PM_OPS(sprd_wdt_pm_suspend, > >+ sprd_wdt_pm_resume) > >+}; > >+ > >+static const struct of_device_id sprd_wdt_match_table[] = { > >+ { .compatible = "sprd,sp9860-wdt", }, > >+ {}, > >+}; > >+MODULE_DEVICE_TABLE(of, sprd_wdt_match_table); > >+ > >+static struct platform_driver sprd_watchdog_driver = { > >+ .probe = sprd_wdt_probe, > >+ .remove = sprd_wdt_remove, > >+ .driver = { > >+ .name = "sprd-wdt", > >+ .of_match_table = sprd_wdt_match_table, > >+ .pm = &sprd_wdt_pm_ops, > >+ }, > >+}; > >+module_platform_driver(sprd_watchdog_driver); > >+ > >+MODULE_AUTHOR("Eric Long <eric.long@spreadtrum.com>"); > >+MODULE_DESCRIPTION("Spreadtrum Watchdog Timer Controller Driver"); > >+MODULE_LICENSE("GPL v2"); > > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/27/2017 12:54 AM, Eric Long wrote: > Hi Guenter, > > Thanks for you commonts. > > On Thu, Oct 26, 2017 at 10:23:40PM -0700, Guenter Roeck wrote: >> On 10/26/2017 07:28 PM, Eric Long wrote: >>> This patch adds the watchdog driver for Spreadtrum SC9860 platform. >>> >>> Signed-off-by: Eric Long <eric.long@spreadtrum.com> >>> --- >>> Change since v2: >>> - Rename all the macors, add SPRD tag at the head of the macro names. >>> - Rename SPRD_WDT_CLK as SPRD_WTC_CNT_STEP. >>> - Remove the code which check timeout value at the wrong place. >>> - Add min/max timeout value limit. >>> - Remove set WDOG_HW_RUNNING status at sprd_wdt_enable(). >>> - Add timeout/pretimeout judgment when set them. >>> - Support WATCHDOG_NOWAYOUT status. >>> >>> Changes since v1: >>> - Use pretimeout instead of own implementation. >>> - Fix timeout loop when loading timeout values. >>> - use the infrastructure to read and set "timeout-sec" property. >>> - Add conditions when start or stop watchdog. >>> - Change the position of enabling watchdog. >>> - Other optimization. >>> --- >>> drivers/watchdog/Kconfig | 8 + >>> drivers/watchdog/Makefile | 1 + >>> drivers/watchdog/sprd_wdt.c | 415 ++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 424 insertions(+) >>> create mode 100644 drivers/watchdog/sprd_wdt.c >>> >>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >>> index c722cbf..4a77e17 100644 >>> --- a/drivers/watchdog/Kconfig >>> +++ b/drivers/watchdog/Kconfig >>> @@ -787,6 +787,14 @@ config UNIPHIER_WATCHDOG >>> To compile this driver as a module, choose M here: the >>> module will be called uniphier_wdt. >>> +config SPRD_WATCHDOG >>> + tristate "Spreadtrum watchdog support" >>> + depends on ARCH_SPRD || COMPILE_TEST >>> + select WATCHDOG_CORE >>> + help >>> + Say Y here to include support watchdog timer embedded >>> + into the Spreadtrum system. >> >> Maybe better "supported by" instead of "embedded into". Or at least >> "embedded in". > > OK, I will fix it. > >>> + >>> # AVR32 Architecture >>> config AT32AP700X_WDT >>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile >>> index 56adf9f..187cca2 100644 >>> --- a/drivers/watchdog/Makefile >>> +++ b/drivers/watchdog/Makefile >>> @@ -87,6 +87,7 @@ obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o >>> obj-$(CONFIG_ZX2967_WATCHDOG) += zx2967_wdt.o >>> obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o >>> obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o >>> +obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o >>> # AVR32 Architecture >>> obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o >>> diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c >>> new file mode 100644 >>> index 0000000..5fc3dde >>> --- /dev/null >>> +++ b/drivers/watchdog/sprd_wdt.c >>> @@ -0,0 +1,415 @@ >>> +/* >>> + * Spreadtrum watchdog driver >>> + * Copyright (C) 2017 Spreadtrum - http://www.spreadtrum.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. >>> + * >>> + * This program is distributed in the hope that it will be useful, but >>> + * WITHOUT ANY WARRANTY; without even the implied warranty of >>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >>> + * General Public License for more details. >>> + */ >>> + >>> +#include <linux/bitops.h> >>> +#include <linux/clk.h> >>> +#include <linux/err.h> >>> +#include <linux/interrupt.h> >>> +#include <linux/io.h> >>> +#include <linux/kernel.h> >>> +#include <linux/module.h> >>> +#include <linux/of.h> >>> +#include <linux/of_address.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/watchdog.h> >>> + >>> +#define SPRD_WDT_LOAD_LOW 0x0 >>> +#define SPRD_WDT_LOAD_HIGH 0x4 >>> +#define SPRD_WDT_CTRL 0x8 >>> +#define SPRD_WDT_INT_CLR 0xc >>> +#define SPRD_WDT_INT_RAW 0x10 >>> +#define SPRD_WDT_INT_MSK 0x14 >>> +#define SPRD_WDT_CNT_LOW 0x18 >>> +#define SPRD_WDT_CNT_HIGH 0x1c >>> +#define SPRD_WDT_LOCK 0x20 >>> +#define SPRD_WDT_IRQ_LOAD_LOW 0x2c >>> +#define SPRD_WDT_IRQ_LOAD_HIGH 0x30 >>> + >>> +/* WDT_CTRL */ >>> +#define SPRD_WDT_INT_EN_BIT BIT(0) >>> +#define SPRD_WDT_CNT_EN_BIT BIT(1) >>> +#define SPRD_WDT_NEW_VER_EN BIT(2) >>> +#define SPRD_WDT_RST_EN_BIT BIT(3) >>> + >>> +/* WDT_INT_CLR */ >>> +#define SPRD_WDT_INT_CLEAR_BIT BIT(0) >>> +#define SPRD_WDT_RST_CLEAR_BIT BIT(3) >>> + >>> +/* WDT_INT_RAW */ >>> +#define SPRD_WDT_INT_RAW_BIT BIT(0) >>> +#define SPRD_WDT_RST_RAW_BIT BIT(3) >>> +#define SPRD_WDT_LD_BUSY_BIT BIT(4) >>> + >>> +/* 1s equale to 32768 counter steps */ >> >> equal ? >> > > OK, I will fix it. > >>> +#define SPRD_WDT_CNT_STEP 32768 >>> + >>> +#define SPRD_WDT_UNLOCK_KEY 0xe551 >>> +#define SPRD_WDT_MIN_TIMROUT 3 >> >> TIMROUT ot TIMEOUT ? >> > > OK, I will fix it. > >>> +#define SPRD_WDT_MAX_TIMEOUT 60 >> >> Is that really the maximum supported timeout ? Seems a bit low. >> Shouldn't it be something like (U32_MAX / SPRD_WDT_CNT_STEP) ? >> > > It supports the max value like as U32_MAX/SPRD_WDT_CNT_STEP, > but it doesn't unnecessary to support so big value, if the system can not > response it in 60s, the watchdog could trigger timeout. > It is customary to provide the highest possible value here. No one is forced to set such high values. You are making a choice for others here. But, sure, if you insist; not worth arguing about. >>> + >>> +#define SPRD_WDT_CNT_HIGH_VALUE 16 >> >> Maybe name it "SPRD_WDT_CNT_HIGH_SHIFT". It is not really a value, >> it is a shift. >> > > Yes, you are right, _SHIFT will be better. > >>> +#define SPRD_WDT_LOW_VALUE_MASK GENMASK(15, 0) >>> +#define SPRD_WDT_CNT_VALUE_MAX GENMASK(31, 0) >> >> Does this mask serve a useful purpose ? >> > > OK, I will remove it, thanks! > >>> +#define SPRD_WDT_LOAD_TIMEOUT 1000 >>> + >>> +struct sprd_wdt { >>> + void __iomem *base; >>> + struct watchdog_device wdd; >>> + struct clk *enable; >>> + struct clk *rtc_enable; >>> + unsigned int irq; >>> +}; >>> + >>> +static inline struct sprd_wdt *to_sprd_wdt(struct watchdog_device *wdd) >>> +{ >>> + return container_of(wdd, struct sprd_wdt, wdd); >>> +} >>> + >>> +static inline void sprd_wdt_lock(void __iomem *addr) >>> +{ >>> + writel_relaxed(0x0, addr + SPRD_WDT_LOCK); >>> +} >>> + >>> +static inline void sprd_wdt_unlock(void __iomem *addr) >>> +{ >>> + writel_relaxed(SPRD_WDT_UNLOCK_KEY, addr + SPRD_WDT_LOCK); >>> +} >>> + >>> +static inline bool sprd_wdt_is_running(struct sprd_wdt *wdt) >>> +{ >>> + u32 val; >>> + >>> + val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); >>> + return val & SPRD_WDT_NEW_VER_EN; >>> +} >>> + >>> +static irqreturn_t sprd_wdt_isr(int irq, void *dev_id) >>> +{ >>> + struct sprd_wdt *wdt = (struct sprd_wdt *)dev_id; >>> + >>> + sprd_wdt_unlock(wdt->base); >>> + writel_relaxed(SPRD_WDT_INT_CLEAR_BIT, wdt->base + SPRD_WDT_INT_CLR); >>> + sprd_wdt_lock(wdt->base); >>> + watchdog_notify_pretimeout(&wdt->wdd); >>> + return IRQ_HANDLED; >>> +} >>> + >>> +static u32 sprd_wdt_get_cnt_value(struct sprd_wdt *wdt) >>> +{ >>> + u32 val; >>> + >>> + val = readl_relaxed(wdt->base + SPRD_WDT_CNT_HIGH) << >>> + SPRD_WDT_CNT_HIGH_VALUE; >>> + val |= readl_relaxed(wdt->base + SPRD_WDT_CNT_LOW) & >>> + SPRD_WDT_LOW_VALUE_MASK; >>> + >>> + return val; >>> +} >>> + >>> +static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout, >>> + u32 pretimeout) >>> +{ >>> + u32 val, delay_cnt = 0; >>> + u32 tmr_step = timeout * SPRD_WDT_CNT_STEP; >>> + u32 prtmr_step = pretimeout * SPRD_WDT_CNT_STEP; >>> + >>> + sprd_wdt_unlock(wdt->base); >>> + writel_relaxed((tmr_step >> SPRD_WDT_CNT_HIGH_VALUE) & >>> + SPRD_WDT_LOW_VALUE_MASK, wdt->base + SPRD_WDT_LOAD_HIGH); >>> + writel_relaxed((tmr_step & SPRD_WDT_LOW_VALUE_MASK), >>> + wdt->base + SPRD_WDT_LOAD_LOW); >>> + writel_relaxed((prtmr_step >> SPRD_WDT_CNT_HIGH_VALUE) & >>> + SPRD_WDT_LOW_VALUE_MASK, >>> + wdt->base + SPRD_WDT_IRQ_LOAD_HIGH); >>> + writel_relaxed(prtmr_step & SPRD_WDT_LOW_VALUE_MASK, >>> + wdt->base + SPRD_WDT_IRQ_LOAD_LOW); >>> + sprd_wdt_lock(wdt->base); >>> + >>> + /* >>> + * Waiting the load value operation done, >>> + * it needs two or three RTC clock cycles. >>> + */ >>> + do { >>> + val = readl_relaxed(wdt->base + SPRD_WDT_INT_RAW); >>> + if (!(val & SPRD_WDT_LD_BUSY_BIT)) >>> + break; >>> + >>> + cpu_relax(); >>> + } while (delay_cnt++ < SPRD_WDT_LOAD_TIMEOUT); >>> + >>> + if (delay_cnt >= SPRD_WDT_LOAD_TIMEOUT) >>> + return -EBUSY; >>> + return 0; >>> +} >>> + >>> +static int sprd_wdt_enable(struct sprd_wdt *wdt) >>> +{ >>> + u32 val; >>> + int ret; >>> + >>> + ret = clk_prepare_enable(wdt->enable); >>> + if (ret) >>> + return ret; >>> + ret = clk_prepare_enable(wdt->rtc_enable); >>> + if (ret) >>> + return ret; >>> + >>> + sprd_wdt_unlock(wdt->base); >>> + val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); >>> + val |= SPRD_WDT_NEW_VER_EN; >>> + writel_relaxed(val, wdt->base + SPRD_WDT_CTRL); >>> + sprd_wdt_lock(wdt->base); >>> + return 0; >>> +} >>> + >>> +static void sprd_wdt_disable(struct sprd_wdt *wdt) >>> +{ >>> + sprd_wdt_unlock(wdt->base); >>> + writel_relaxed(0x0, wdt->base + SPRD_WDT_CTRL); >>> + sprd_wdt_lock(wdt->base); >>> + >>> + clk_disable_unprepare(wdt->rtc_enable); >>> + clk_disable_unprepare(wdt->enable); >>> +} >>> + >>> +static int sprd_wdt_start(struct watchdog_device *wdd) >>> +{ >>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd); >>> + u32 val; >>> + int ret; >>> + >>> + ret = sprd_wdt_load_value(wdt, wdd->timeout, wdd->pretimeout); >>> + if (ret) >>> + return ret; >>> + >>> + sprd_wdt_unlock(wdt->base); >>> + val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); >>> + val |= SPRD_WDT_CNT_EN_BIT | SPRD_WDT_INT_EN_BIT | SPRD_WDT_RST_EN_BIT; >>> + writel_relaxed(val, wdt->base + SPRD_WDT_CTRL); >>> + sprd_wdt_lock(wdt->base); >>> + set_bit(WDOG_HW_RUNNING, &wdd->status); >>> + >>> + return 0; >>> +} >>> + >>> +static int sprd_wdt_stop(struct watchdog_device *wdd) >>> +{ >>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd); >>> + u32 val; >>> + >>> + sprd_wdt_unlock(wdt->base); >>> + val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); >>> + val &= ~(SPRD_WDT_CNT_EN_BIT | SPRD_WDT_RST_EN_BIT | >>> + SPRD_WDT_INT_EN_BIT); >>> + writel_relaxed(val, wdt->base + SPRD_WDT_CTRL); >>> + sprd_wdt_lock(wdt->base); >>> + return 0; >>> +} >>> + >>> +static int sprd_wdt_set_timeout(struct watchdog_device *wdd, >>> + u32 timeout) >>> +{ >>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd); >>> + >>> + if (timeout == wdd->timeout) >>> + return 0; >>> + >>> + wdd->timeout = timeout; >>> + >>> + return sprd_wdt_load_value(wdt, timeout, wdd->pretimeout); >>> +} >>> + >>> +static int sprd_wdt_set_pretimeout(struct watchdog_device *wdd, >>> + u32 new_pretimeout) >>> +{ >>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd); >>> + >>> + if (new_pretimeout == wdd->pretimeout) >>> + return 0; >> >> This is inconsistent. pretimeout == 0 is accepted until it is set >> to a non-zero value once, then 0 is no longer accepted. pretimeout==0 >> should reflect "pretimeout disabled". If that is not possible you >> need to set some non-0 default value in the probe function. >> > > I understand your opinion, I will fix it. > >>> + if (new_pretimeout <= wdd->min_timeout) >>> + return -EINVAL; >> >> Why is pretimeout == wdd->min_timeout invalid ? >> > > OK, you are right, it should be pretimeout < min_timeout. > >>> + >>> + wdd->pretimeout = new_pretimeout; >>> + >>> + return sprd_wdt_load_value(wdt, wdd->timeout, new_pretimeout); >>> +} >>> + >>> +static u32 sprd_wdt_get_timeleft(struct watchdog_device *wdd) >>> +{ >>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd); >>> + u32 val; >>> + >>> + val = sprd_wdt_get_cnt_value(wdt); >>> + val = val / SPRD_WDT_CNT_STEP; >>> + >>> + return val; >>> +} >>> + >>> +static const struct watchdog_ops sprd_wdt_ops = { >>> + .owner = THIS_MODULE, >>> + .start = sprd_wdt_start, >>> + .stop = sprd_wdt_stop, >>> + .set_timeout = sprd_wdt_set_timeout, >>> + .set_pretimeout = sprd_wdt_set_pretimeout, >>> + .get_timeleft = sprd_wdt_get_timeleft, >> >> Just wondering - no heartbeat function ? Having to load the timer >> values for each heartbeat is expensive. >> > > Unfortunately, this watchdog has not RELOAD register. > >>> +}; >>> + >>> +static const struct watchdog_info sprd_wdt_info = { >>> + .options = WDIOF_SETTIMEOUT | >>> + WDIOF_PRETIMEOUT | >>> + WDIOF_MAGICCLOSE | >>> + WDIOF_KEEPALIVEPING, >>> + .identity = "Spreadtrum Watchdog Timer", >>> +}; >>> + >>> +static int sprd_wdt_probe(struct platform_device *pdev) >>> +{ >>> + struct resource *wdt_res; >>> + struct sprd_wdt *wdt; >>> + int ret; >>> + >>> + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); >>> + if (!wdt) >>> + return -ENOMEM; >>> + >>> + wdt_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> + if (!wdt_res) { >>> + dev_err(&pdev->dev, "failed to memory resource\n"); >>> + return -ENOMEM; >>> + } >> >> Unnecessary error check and message. devm_ioremap_resource() >> returns an error if res is NULL. >> > > OK, I will fix it. > >>> + >>> + wdt->base = devm_ioremap_resource(&pdev->dev, wdt_res); >>> + if (IS_ERR(wdt->base)) >> >> Move the error message to here. >> >>> + return PTR_ERR(wdt->base); >>> + >>> + wdt->enable = devm_clk_get(&pdev->dev, "enable"); >>> + if (IS_ERR(wdt->enable)) { >>> + dev_err(&pdev->dev, "can't get the enable clock\n"); >>> + return PTR_ERR(wdt->enable); >>> + } >>> + >>> + wdt->rtc_enable = devm_clk_get(&pdev->dev, "rtc_enable"); >>> + if (IS_ERR(wdt->rtc_enable)) { >>> + dev_err(&pdev->dev, "can't get the rtc enable clock\n"); >>> + return PTR_ERR(wdt->rtc_enable); >>> + } >>> + >>> + wdt->irq = platform_get_irq(pdev, 0); >>> + if (wdt->irq < 0) { >>> + dev_err(&pdev->dev, "failed to get IRQ resource\n"); >>> + return wdt->irq; >>> + } >>> + >>> + ret = devm_request_irq(&pdev->dev, wdt->irq, sprd_wdt_isr, >>> + IRQF_NO_SUSPEND, "sprd-wdt", (void *)wdt); >>> + if (ret) { >>> + dev_err(&pdev->dev, "failed to register irq\n"); >>> + return ret; >>> + } >>> + >>> + wdt->wdd.info = &sprd_wdt_info; >>> + wdt->wdd.ops = &sprd_wdt_ops; >>> + wdt->wdd.parent = &pdev->dev; >>> + wdt->wdd.min_timeout = SPRD_WDT_MIN_TIMROUT; >>> + wdt->wdd.max_timeout = SPRD_WDT_MAX_TIMEOUT; >>> + >> You might want to set wdt->wdd.timeout to a default value. >> > > OK, I will set a default timeout value in case of no timeout-sec in device-tree. > >>> + ret = sprd_wdt_enable(wdt); >>> + if (ret) { >>> + dev_err(&pdev->dev, "failed to enable wdt\n"); >>> + return ret; >>> + } >>> + >> >> This needs a matching sprd_wdt_disable() call in the remove function. >> Best way to handle this would be to add devm_add_action() here. >> > > If add devm_add_action(), it will be called in the remove function, > but this is not our expect, if someone remove this module, we want it just timeout. > But that leaves the watchdog enabled even if it was stopped (no call to sprd_wdt_disable()). Relying on NOWAYOUT would be a better option here. Again, you are making a choice for the user. >>> + watchdog_set_nowayout(&wdt->wdd, WATCHDOG_NOWAYOUT); >>> + watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev); >>> + >>> + ret = watchdog_register_device(&wdt->wdd); >> >> Unfortunately this doesn't work. It leaves interrupts enabled >> after the watchdog device is removed in sprd_wdt_remove(), >> but the driver will be gone. I would suggest to use >> devm_watchdog_register_device() and reorder calls to request >> the interrupt after registering the watchdog device. Then you >> can drop the remove function entirely. >> > > Yes, you are right, I understand your opinion, and it is very important, > devm_watchdog_register_device() is better. Thanks. > >>> + if (ret) { >>> + sprd_wdt_disable(wdt); >>> + dev_err(&pdev->dev, "failed to register watchdog\n"); >>> + return ret; >>> + } >>> + platform_set_drvdata(pdev, wdt); >>> + >>> + return 0; >>> +} >>> + >>> +static int sprd_wdt_remove(struct platform_device *pdev) >>> +{ >>> + struct sprd_wdt *wdt = platform_get_drvdata(pdev); >>> + >>> + watchdog_unregister_device(&wdt->wdd); >>> + >>> + if (sprd_wdt_is_running(wdt)) >>> + dev_crit(&pdev->dev, "Device removed: Expect reboot!\n"); >>> + return 0; >>> +} >>> + >>> +static int __maybe_unused sprd_wdt_pm_suspend(struct device *dev) >>> +{ >>> + struct watchdog_device *wdd = dev_get_drvdata(dev); >>> + struct sprd_wdt *wdt = dev_get_drvdata(dev); >>> + >>> + if (watchdog_active(wdd)) { >>> + sprd_wdt_stop(&wdt->wdd); >>> + sprd_wdt_disable(wdt); >> >> Are you sure you only want to disable the clocks if the watchdog was active ? >> That seems to be a bit inconsistent. >> > > This watchdog is in always on power domain, if system suspend, it needs to > disable it, or it will timeout. > That is not what I asked. I asked why it isn't if (watchdog_active(wdd)) sprd_wdt_stop(&wdt->wdd); sprd_wdt_disable(wdt); instead. >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static int __maybe_unused sprd_wdt_pm_resume(struct device *dev) >>> +{ >>> + struct watchdog_device *wdd = dev_get_drvdata(dev); >>> + struct sprd_wdt *wdt = dev_get_drvdata(dev); >>> + int ret; >>> + >>> + if (watchdog_active(wdd)) { >>> + ret = sprd_wdt_enable(wdt); >>> + if (ret) >>> + return ret; >>> + ret = sprd_wdt_start(&wdt->wdd); >>> + if (ret) { >>> + sprd_wdt_disable(wdt); >>> + return ret; >>> + } >> >> This can leave the system in an inconsistent state. Sometimes the wdt may be >> disabled, sometimes not. >> > > If watchdog enable failed, it means there is something wrong in this system. > True. Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guenter, (There are some problem with Eric's email, he can not receive this email, so I help to reply his comments following yours. sorry for troubles.) >>>> +#define SPRD_WDT_MAX_TIMEOUT 60 >>> >>> >>> Is that really the maximum supported timeout ? Seems a bit low. >>> Shouldn't it be something like (U32_MAX / SPRD_WDT_CNT_STEP) ? >>> >> >> It supports the max value like as U32_MAX/SPRD_WDT_CNT_STEP, >> but it doesn't unnecessary to support so big value, if the system can not >> response it in 60s, the watchdog could trigger timeout. >> > It is customary to provide the highest possible value here. > No one is forced to set such high values. You are making a choice for > others here. But, sure, if you insist; not worth arguing about. Eric said 60s is better for us, thanks for your patient explanation. > >>>> + >>>> +#define SPRD_WDT_CNT_HIGH_VALUE 16 >>> >>> >>> Maybe name it "SPRD_WDT_CNT_HIGH_SHIFT". It is not really a value, >>> it is a shift. >>> >> >> Yes, you are right, _SHIFT will be better. >> >>>> >>>> +#define SPRD_WDT_LOW_VALUE_MASK GENMASK(15, 0) >>>> +#define SPRD_WDT_CNT_VALUE_MAX GENMASK(31, 0) >>> >>> >>> Does this mask serve a useful purpose ? >>> >> >> OK, I will remove it, thanks! >> >>>> +#define SPRD_WDT_LOAD_TIMEOUT 1000 >>>> + >>>> +struct sprd_wdt { >>>> + void __iomem *base; >>>> + struct watchdog_device wdd; >>>> + struct clk *enable; >>>> + struct clk *rtc_enable; >>>> + unsigned int irq; >>>> +}; >>>> + >>>> +static inline struct sprd_wdt *to_sprd_wdt(struct watchdog_device *wdd) >>>> +{ >>>> + return container_of(wdd, struct sprd_wdt, wdd); >>>> +} >>>> + >>>> +static inline void sprd_wdt_lock(void __iomem *addr) >>>> +{ >>>> + writel_relaxed(0x0, addr + SPRD_WDT_LOCK); >>>> +} >>>> + >>>> +static inline void sprd_wdt_unlock(void __iomem *addr) >>>> +{ >>>> + writel_relaxed(SPRD_WDT_UNLOCK_KEY, addr + SPRD_WDT_LOCK); >>>> +} >>>> + >>>> +static inline bool sprd_wdt_is_running(struct sprd_wdt *wdt) >>>> +{ >>>> + u32 val; >>>> + >>>> + val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); >>>> + return val & SPRD_WDT_NEW_VER_EN; >>>> +} >>>> + >>>> +static irqreturn_t sprd_wdt_isr(int irq, void *dev_id) >>>> +{ >>>> + struct sprd_wdt *wdt = (struct sprd_wdt *)dev_id; >>>> + >>>> + sprd_wdt_unlock(wdt->base); >>>> + writel_relaxed(SPRD_WDT_INT_CLEAR_BIT, wdt->base + >>>> SPRD_WDT_INT_CLR); >>>> + sprd_wdt_lock(wdt->base); >>>> + watchdog_notify_pretimeout(&wdt->wdd); >>>> + return IRQ_HANDLED; >>>> +} >>>> + >>>> +static u32 sprd_wdt_get_cnt_value(struct sprd_wdt *wdt) >>>> +{ >>>> + u32 val; >>>> + >>>> + val = readl_relaxed(wdt->base + SPRD_WDT_CNT_HIGH) << >>>> + SPRD_WDT_CNT_HIGH_VALUE; >>>> + val |= readl_relaxed(wdt->base + SPRD_WDT_CNT_LOW) & >>>> + SPRD_WDT_LOW_VALUE_MASK; >>>> + >>>> + return val; >>>> +} >>>> + >>>> +static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout, >>>> + u32 pretimeout) >>>> +{ >>>> + u32 val, delay_cnt = 0; >>>> + u32 tmr_step = timeout * SPRD_WDT_CNT_STEP; >>>> + u32 prtmr_step = pretimeout * SPRD_WDT_CNT_STEP; >>>> + >>>> + sprd_wdt_unlock(wdt->base); >>>> + writel_relaxed((tmr_step >> SPRD_WDT_CNT_HIGH_VALUE) & >>>> + SPRD_WDT_LOW_VALUE_MASK, wdt->base + >>>> SPRD_WDT_LOAD_HIGH); >>>> + writel_relaxed((tmr_step & SPRD_WDT_LOW_VALUE_MASK), >>>> + wdt->base + SPRD_WDT_LOAD_LOW); >>>> + writel_relaxed((prtmr_step >> SPRD_WDT_CNT_HIGH_VALUE) & >>>> + SPRD_WDT_LOW_VALUE_MASK, >>>> + wdt->base + SPRD_WDT_IRQ_LOAD_HIGH); >>>> + writel_relaxed(prtmr_step & SPRD_WDT_LOW_VALUE_MASK, >>>> + wdt->base + SPRD_WDT_IRQ_LOAD_LOW); >>>> + sprd_wdt_lock(wdt->base); >>>> + >>>> + /* >>>> + * Waiting the load value operation done, >>>> + * it needs two or three RTC clock cycles. >>>> + */ >>>> + do { >>>> + val = readl_relaxed(wdt->base + SPRD_WDT_INT_RAW); >>>> + if (!(val & SPRD_WDT_LD_BUSY_BIT)) >>>> + break; >>>> + >>>> + cpu_relax(); >>>> + } while (delay_cnt++ < SPRD_WDT_LOAD_TIMEOUT); >>>> + >>>> + if (delay_cnt >= SPRD_WDT_LOAD_TIMEOUT) >>>> + return -EBUSY; >>>> + return 0; >>>> +} >>>> + >>>> +static int sprd_wdt_enable(struct sprd_wdt *wdt) >>>> +{ >>>> + u32 val; >>>> + int ret; >>>> + >>>> + ret = clk_prepare_enable(wdt->enable); >>>> + if (ret) >>>> + return ret; >>>> + ret = clk_prepare_enable(wdt->rtc_enable); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + sprd_wdt_unlock(wdt->base); >>>> + val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); >>>> + val |= SPRD_WDT_NEW_VER_EN; >>>> + writel_relaxed(val, wdt->base + SPRD_WDT_CTRL); >>>> + sprd_wdt_lock(wdt->base); >>>> + return 0; >>>> +} >>>> + >>>> +static void sprd_wdt_disable(struct sprd_wdt *wdt) >>>> +{ >>>> + sprd_wdt_unlock(wdt->base); >>>> + writel_relaxed(0x0, wdt->base + SPRD_WDT_CTRL); >>>> + sprd_wdt_lock(wdt->base); >>>> + >>>> + clk_disable_unprepare(wdt->rtc_enable); >>>> + clk_disable_unprepare(wdt->enable); >>>> +} >>>> + >>>> +static int sprd_wdt_start(struct watchdog_device *wdd) >>>> +{ >>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd); >>>> + u32 val; >>>> + int ret; >>>> + >>>> + ret = sprd_wdt_load_value(wdt, wdd->timeout, wdd->pretimeout); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + sprd_wdt_unlock(wdt->base); >>>> + val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); >>>> + val |= SPRD_WDT_CNT_EN_BIT | SPRD_WDT_INT_EN_BIT | >>>> SPRD_WDT_RST_EN_BIT; >>>> + writel_relaxed(val, wdt->base + SPRD_WDT_CTRL); >>>> + sprd_wdt_lock(wdt->base); >>>> + set_bit(WDOG_HW_RUNNING, &wdd->status); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int sprd_wdt_stop(struct watchdog_device *wdd) >>>> +{ >>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd); >>>> + u32 val; >>>> + >>>> + sprd_wdt_unlock(wdt->base); >>>> + val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); >>>> + val &= ~(SPRD_WDT_CNT_EN_BIT | SPRD_WDT_RST_EN_BIT | >>>> + SPRD_WDT_INT_EN_BIT); >>>> + writel_relaxed(val, wdt->base + SPRD_WDT_CTRL); >>>> + sprd_wdt_lock(wdt->base); >>>> + return 0; >>>> +} >>>> + >>>> +static int sprd_wdt_set_timeout(struct watchdog_device *wdd, >>>> + u32 timeout) >>>> +{ >>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd); >>>> + >>>> + if (timeout == wdd->timeout) >>>> + return 0; >>>> + >>>> + wdd->timeout = timeout; >>>> + >>>> + return sprd_wdt_load_value(wdt, timeout, wdd->pretimeout); >>>> +} >>>> + >>>> +static int sprd_wdt_set_pretimeout(struct watchdog_device *wdd, >>>> + u32 new_pretimeout) >>>> +{ >>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd); >>>> + >>>> + if (new_pretimeout == wdd->pretimeout) >>>> + return 0; >>> >>> >>> This is inconsistent. pretimeout == 0 is accepted until it is set >>> to a non-zero value once, then 0 is no longer accepted. pretimeout==0 >>> should reflect "pretimeout disabled". If that is not possible you >>> need to set some non-0 default value in the probe function. >>> >> >> I understand your opinion, I will fix it. >> >>>> + if (new_pretimeout <= wdd->min_timeout) >>>> + return -EINVAL; >>> >>> >>> Why is pretimeout == wdd->min_timeout invalid ? >>> >> >> OK, you are right, it should be pretimeout < min_timeout. >> >>>> + >>>> + wdd->pretimeout = new_pretimeout; >>>> + >>>> + return sprd_wdt_load_value(wdt, wdd->timeout, new_pretimeout); >>>> +} >>>> + >>>> +static u32 sprd_wdt_get_timeleft(struct watchdog_device *wdd) >>>> +{ >>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd); >>>> + u32 val; >>>> + >>>> + val = sprd_wdt_get_cnt_value(wdt); >>>> + val = val / SPRD_WDT_CNT_STEP; >>>> + >>>> + return val; >>>> +} >>>> + >>>> +static const struct watchdog_ops sprd_wdt_ops = { >>>> + .owner = THIS_MODULE, >>>> + .start = sprd_wdt_start, >>>> + .stop = sprd_wdt_stop, >>>> + .set_timeout = sprd_wdt_set_timeout, >>>> + .set_pretimeout = sprd_wdt_set_pretimeout, >>>> + .get_timeleft = sprd_wdt_get_timeleft, >>> >>> >>> Just wondering - no heartbeat function ? Having to load the timer >>> values for each heartbeat is expensive. >>> >> >> Unfortunately, this watchdog has not RELOAD register. >> >>>> >>>> +}; >>>> + >>>> +static const struct watchdog_info sprd_wdt_info = { >>>> + .options = WDIOF_SETTIMEOUT | >>>> + WDIOF_PRETIMEOUT | >>>> + WDIOF_MAGICCLOSE | >>>> + WDIOF_KEEPALIVEPING, >>>> + .identity = "Spreadtrum Watchdog Timer", >>>> +}; >>>> + >>>> +static int sprd_wdt_probe(struct platform_device *pdev) >>>> +{ >>>> + struct resource *wdt_res; >>>> + struct sprd_wdt *wdt; >>>> + int ret; >>>> + >>>> + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); >>>> + if (!wdt) >>>> + return -ENOMEM; >>>> + >>>> + wdt_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>> + if (!wdt_res) { >>>> + dev_err(&pdev->dev, "failed to memory resource\n"); >>>> + return -ENOMEM; >>>> + } >>> >>> >>> Unnecessary error check and message. devm_ioremap_resource() >>> returns an error if res is NULL. >>> >> >> OK, I will fix it. >> >>>> >>>> + >>>> + wdt->base = devm_ioremap_resource(&pdev->dev, wdt_res); >>>> + if (IS_ERR(wdt->base)) >>> >>> >>> Move the error message to here. >>> >>>> + return PTR_ERR(wdt->base); >>>> + >>>> + wdt->enable = devm_clk_get(&pdev->dev, "enable"); >>>> + if (IS_ERR(wdt->enable)) { >>>> + dev_err(&pdev->dev, "can't get the enable clock\n"); >>>> + return PTR_ERR(wdt->enable); >>>> + } >>>> + >>>> + wdt->rtc_enable = devm_clk_get(&pdev->dev, "rtc_enable"); >>>> + if (IS_ERR(wdt->rtc_enable)) { >>>> + dev_err(&pdev->dev, "can't get the rtc enable clock\n"); >>>> + return PTR_ERR(wdt->rtc_enable); >>>> + } >>>> + >>>> + wdt->irq = platform_get_irq(pdev, 0); >>>> + if (wdt->irq < 0) { >>>> + dev_err(&pdev->dev, "failed to get IRQ resource\n"); >>>> + return wdt->irq; >>>> + } >>>> + >>>> + ret = devm_request_irq(&pdev->dev, wdt->irq, sprd_wdt_isr, >>>> + IRQF_NO_SUSPEND, "sprd-wdt", (void >>>> *)wdt); >>>> + if (ret) { >>>> + dev_err(&pdev->dev, "failed to register irq\n"); >>>> + return ret; >>>> + } >>>> + >>>> + wdt->wdd.info = &sprd_wdt_info; >>>> + wdt->wdd.ops = &sprd_wdt_ops; >>>> + wdt->wdd.parent = &pdev->dev; >>>> + wdt->wdd.min_timeout = SPRD_WDT_MIN_TIMROUT; >>>> + wdt->wdd.max_timeout = SPRD_WDT_MAX_TIMEOUT; >>>> + >>> >>> You might want to set wdt->wdd.timeout to a default value. >>> >> >> OK, I will set a default timeout value in case of no timeout-sec in >> device-tree. >> >>>> >>>> + ret = sprd_wdt_enable(wdt); >>>> + if (ret) { >>>> + dev_err(&pdev->dev, "failed to enable wdt\n"); >>>> + return ret; >>>> + } >>>> + >>> >>> >>> This needs a matching sprd_wdt_disable() call in the remove function. >>> Best way to handle this would be to add devm_add_action() here. >>> >> >> If add devm_add_action(), it will be called in the remove function, >> but this is not our expect, if someone remove this module, we want it just >> timeout. >> > > But that leaves the watchdog enabled even if it was stopped (no call to > sprd_wdt_disable()). Eric said It can not be stopped since we have set WATCHDOG_NOWAYOUT, which means it will reboot the system when removing the watchdog. > Relying on NOWAYOUT would be a better option here. Again, you are > making a choice for the user. we have set WATCHDOG_NOWAYOUT. > > >>>> + watchdog_set_nowayout(&wdt->wdd, WATCHDOG_NOWAYOUT); >>>> + watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev); >>>> + >>>> + ret = watchdog_register_device(&wdt->wdd); >>> >>> >>> Unfortunately this doesn't work. It leaves interrupts enabled >>> after the watchdog device is removed in sprd_wdt_remove(), >>> but the driver will be gone. I would suggest to use >>> devm_watchdog_register_device() and reorder calls to request >>> the interrupt after registering the watchdog device. Then you >>> can drop the remove function entirely. >>> >> >> Yes, you are right, I understand your opinion, and it is very important, >> devm_watchdog_register_device() is better. Thanks. >> >>>> + if (ret) { >>>> + sprd_wdt_disable(wdt); >>>> + dev_err(&pdev->dev, "failed to register watchdog\n"); >>>> + return ret; >>>> + } >>>> + platform_set_drvdata(pdev, wdt); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int sprd_wdt_remove(struct platform_device *pdev) >>>> +{ >>>> + struct sprd_wdt *wdt = platform_get_drvdata(pdev); >>>> + >>>> + watchdog_unregister_device(&wdt->wdd); >>>> + >>>> + if (sprd_wdt_is_running(wdt)) >>>> + dev_crit(&pdev->dev, "Device removed: Expect >>>> reboot!\n"); >>>> + return 0; >>>> +} >>>> + >>>> +static int __maybe_unused sprd_wdt_pm_suspend(struct device *dev) >>>> +{ >>>> + struct watchdog_device *wdd = dev_get_drvdata(dev); >>>> + struct sprd_wdt *wdt = dev_get_drvdata(dev); >>>> + >>>> + if (watchdog_active(wdd)) { >>>> + sprd_wdt_stop(&wdt->wdd); >>>> + sprd_wdt_disable(wdt); >>> >>> >>> Are you sure you only want to disable the clocks if the watchdog was >>> active ? >>> That seems to be a bit inconsistent. >>> >> >> This watchdog is in always on power domain, if system suspend, it needs to >> disable it, or it will timeout. >> > That is not what I asked. I asked why it isn't > > if (watchdog_active(wdd)) > sprd_wdt_stop(&wdt->wdd); > > sprd_wdt_disable(wdt); > > instead. Yes, Eric will fix this in next version. > >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int __maybe_unused sprd_wdt_pm_resume(struct device *dev) >>>> +{ >>>> + struct watchdog_device *wdd = dev_get_drvdata(dev); >>>> + struct sprd_wdt *wdt = dev_get_drvdata(dev); >>>> + int ret; >>>> + >>>> + if (watchdog_active(wdd)) { >>>> + ret = sprd_wdt_enable(wdt); >>>> + if (ret) >>>> + return ret; >>>> + ret = sprd_wdt_start(&wdt->wdd); >>>> + if (ret) { >>>> + sprd_wdt_disable(wdt); >>>> + return ret; >>>> + } >>> >>> >>> This can leave the system in an inconsistent state. Sometimes the wdt may >>> be >>> disabled, sometimes not. >>> >> >> If watchdog enable failed, it means there is something wrong in this >> system. >> > > > True. > > Guenter
On 10/30/2017 02:18 AM, Baolin Wang wrote: > Hi Guenter, > > (There are some problem with Eric's email, he can not receive this > email, so I help to reply his comments following yours. sorry for > troubles.) > >>>>> +#define SPRD_WDT_MAX_TIMEOUT 60 >>>> >>>> >>>> Is that really the maximum supported timeout ? Seems a bit low. >>>> Shouldn't it be something like (U32_MAX / SPRD_WDT_CNT_STEP) ? >>>> >>> >>> It supports the max value like as U32_MAX/SPRD_WDT_CNT_STEP, >>> but it doesn't unnecessary to support so big value, if the system can not >>> response it in 60s, the watchdog could trigger timeout. >>> >> It is customary to provide the highest possible value here. >> No one is forced to set such high values. You are making a choice for >> others here. But, sure, if you insist; not worth arguing about. > > Eric said 60s is better for us, thanks for your patient explanation. > >> >>>>> + >>>>> +#define SPRD_WDT_CNT_HIGH_VALUE 16 >>>> >>>> >>>> Maybe name it "SPRD_WDT_CNT_HIGH_SHIFT". It is not really a value, >>>> it is a shift. >>>> >>> >>> Yes, you are right, _SHIFT will be better. >>> >>>>> >>>>> +#define SPRD_WDT_LOW_VALUE_MASK GENMASK(15, 0) >>>>> +#define SPRD_WDT_CNT_VALUE_MAX GENMASK(31, 0) >>>> >>>> >>>> Does this mask serve a useful purpose ? >>>> >>> >>> OK, I will remove it, thanks! >>> >>>>> +#define SPRD_WDT_LOAD_TIMEOUT 1000 >>>>> + >>>>> +struct sprd_wdt { >>>>> + void __iomem *base; >>>>> + struct watchdog_device wdd; >>>>> + struct clk *enable; >>>>> + struct clk *rtc_enable; >>>>> + unsigned int irq; >>>>> +}; >>>>> + >>>>> +static inline struct sprd_wdt *to_sprd_wdt(struct watchdog_device *wdd) >>>>> +{ >>>>> + return container_of(wdd, struct sprd_wdt, wdd); >>>>> +} >>>>> + >>>>> +static inline void sprd_wdt_lock(void __iomem *addr) >>>>> +{ >>>>> + writel_relaxed(0x0, addr + SPRD_WDT_LOCK); >>>>> +} >>>>> + >>>>> +static inline void sprd_wdt_unlock(void __iomem *addr) >>>>> +{ >>>>> + writel_relaxed(SPRD_WDT_UNLOCK_KEY, addr + SPRD_WDT_LOCK); >>>>> +} >>>>> + >>>>> +static inline bool sprd_wdt_is_running(struct sprd_wdt *wdt) >>>>> +{ >>>>> + u32 val; >>>>> + >>>>> + val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); >>>>> + return val & SPRD_WDT_NEW_VER_EN; >>>>> +} >>>>> + >>>>> +static irqreturn_t sprd_wdt_isr(int irq, void *dev_id) >>>>> +{ >>>>> + struct sprd_wdt *wdt = (struct sprd_wdt *)dev_id; >>>>> + >>>>> + sprd_wdt_unlock(wdt->base); >>>>> + writel_relaxed(SPRD_WDT_INT_CLEAR_BIT, wdt->base + >>>>> SPRD_WDT_INT_CLR); >>>>> + sprd_wdt_lock(wdt->base); >>>>> + watchdog_notify_pretimeout(&wdt->wdd); >>>>> + return IRQ_HANDLED; >>>>> +} >>>>> + >>>>> +static u32 sprd_wdt_get_cnt_value(struct sprd_wdt *wdt) >>>>> +{ >>>>> + u32 val; >>>>> + >>>>> + val = readl_relaxed(wdt->base + SPRD_WDT_CNT_HIGH) << >>>>> + SPRD_WDT_CNT_HIGH_VALUE; >>>>> + val |= readl_relaxed(wdt->base + SPRD_WDT_CNT_LOW) & >>>>> + SPRD_WDT_LOW_VALUE_MASK; >>>>> + >>>>> + return val; >>>>> +} >>>>> + >>>>> +static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout, >>>>> + u32 pretimeout) >>>>> +{ >>>>> + u32 val, delay_cnt = 0; >>>>> + u32 tmr_step = timeout * SPRD_WDT_CNT_STEP; >>>>> + u32 prtmr_step = pretimeout * SPRD_WDT_CNT_STEP; >>>>> + >>>>> + sprd_wdt_unlock(wdt->base); >>>>> + writel_relaxed((tmr_step >> SPRD_WDT_CNT_HIGH_VALUE) & >>>>> + SPRD_WDT_LOW_VALUE_MASK, wdt->base + >>>>> SPRD_WDT_LOAD_HIGH); >>>>> + writel_relaxed((tmr_step & SPRD_WDT_LOW_VALUE_MASK), >>>>> + wdt->base + SPRD_WDT_LOAD_LOW); >>>>> + writel_relaxed((prtmr_step >> SPRD_WDT_CNT_HIGH_VALUE) & >>>>> + SPRD_WDT_LOW_VALUE_MASK, >>>>> + wdt->base + SPRD_WDT_IRQ_LOAD_HIGH); >>>>> + writel_relaxed(prtmr_step & SPRD_WDT_LOW_VALUE_MASK, >>>>> + wdt->base + SPRD_WDT_IRQ_LOAD_LOW); >>>>> + sprd_wdt_lock(wdt->base); >>>>> + >>>>> + /* >>>>> + * Waiting the load value operation done, >>>>> + * it needs two or three RTC clock cycles. >>>>> + */ >>>>> + do { >>>>> + val = readl_relaxed(wdt->base + SPRD_WDT_INT_RAW); >>>>> + if (!(val & SPRD_WDT_LD_BUSY_BIT)) >>>>> + break; >>>>> + >>>>> + cpu_relax(); >>>>> + } while (delay_cnt++ < SPRD_WDT_LOAD_TIMEOUT); >>>>> + >>>>> + if (delay_cnt >= SPRD_WDT_LOAD_TIMEOUT) >>>>> + return -EBUSY; >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int sprd_wdt_enable(struct sprd_wdt *wdt) >>>>> +{ >>>>> + u32 val; >>>>> + int ret; >>>>> + >>>>> + ret = clk_prepare_enable(wdt->enable); >>>>> + if (ret) >>>>> + return ret; >>>>> + ret = clk_prepare_enable(wdt->rtc_enable); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + sprd_wdt_unlock(wdt->base); >>>>> + val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); >>>>> + val |= SPRD_WDT_NEW_VER_EN; >>>>> + writel_relaxed(val, wdt->base + SPRD_WDT_CTRL); >>>>> + sprd_wdt_lock(wdt->base); >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static void sprd_wdt_disable(struct sprd_wdt *wdt) >>>>> +{ >>>>> + sprd_wdt_unlock(wdt->base); >>>>> + writel_relaxed(0x0, wdt->base + SPRD_WDT_CTRL); >>>>> + sprd_wdt_lock(wdt->base); >>>>> + >>>>> + clk_disable_unprepare(wdt->rtc_enable); >>>>> + clk_disable_unprepare(wdt->enable); >>>>> +} >>>>> + >>>>> +static int sprd_wdt_start(struct watchdog_device *wdd) >>>>> +{ >>>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd); >>>>> + u32 val; >>>>> + int ret; >>>>> + >>>>> + ret = sprd_wdt_load_value(wdt, wdd->timeout, wdd->pretimeout); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + sprd_wdt_unlock(wdt->base); >>>>> + val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); >>>>> + val |= SPRD_WDT_CNT_EN_BIT | SPRD_WDT_INT_EN_BIT | >>>>> SPRD_WDT_RST_EN_BIT; >>>>> + writel_relaxed(val, wdt->base + SPRD_WDT_CTRL); >>>>> + sprd_wdt_lock(wdt->base); >>>>> + set_bit(WDOG_HW_RUNNING, &wdd->status); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int sprd_wdt_stop(struct watchdog_device *wdd) >>>>> +{ >>>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd); >>>>> + u32 val; >>>>> + >>>>> + sprd_wdt_unlock(wdt->base); >>>>> + val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); >>>>> + val &= ~(SPRD_WDT_CNT_EN_BIT | SPRD_WDT_RST_EN_BIT | >>>>> + SPRD_WDT_INT_EN_BIT); >>>>> + writel_relaxed(val, wdt->base + SPRD_WDT_CTRL); >>>>> + sprd_wdt_lock(wdt->base); >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int sprd_wdt_set_timeout(struct watchdog_device *wdd, >>>>> + u32 timeout) >>>>> +{ >>>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd); >>>>> + >>>>> + if (timeout == wdd->timeout) >>>>> + return 0; >>>>> + >>>>> + wdd->timeout = timeout; >>>>> + >>>>> + return sprd_wdt_load_value(wdt, timeout, wdd->pretimeout); >>>>> +} >>>>> + >>>>> +static int sprd_wdt_set_pretimeout(struct watchdog_device *wdd, >>>>> + u32 new_pretimeout) >>>>> +{ >>>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd); >>>>> + >>>>> + if (new_pretimeout == wdd->pretimeout) >>>>> + return 0; >>>> >>>> >>>> This is inconsistent. pretimeout == 0 is accepted until it is set >>>> to a non-zero value once, then 0 is no longer accepted. pretimeout==0 >>>> should reflect "pretimeout disabled". If that is not possible you >>>> need to set some non-0 default value in the probe function. >>>> >>> >>> I understand your opinion, I will fix it. >>> >>>>> + if (new_pretimeout <= wdd->min_timeout) >>>>> + return -EINVAL; >>>> >>>> >>>> Why is pretimeout == wdd->min_timeout invalid ? >>>> >>> >>> OK, you are right, it should be pretimeout < min_timeout. >>> >>>>> + >>>>> + wdd->pretimeout = new_pretimeout; >>>>> + >>>>> + return sprd_wdt_load_value(wdt, wdd->timeout, new_pretimeout); >>>>> +} >>>>> + >>>>> +static u32 sprd_wdt_get_timeleft(struct watchdog_device *wdd) >>>>> +{ >>>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd); >>>>> + u32 val; >>>>> + >>>>> + val = sprd_wdt_get_cnt_value(wdt); >>>>> + val = val / SPRD_WDT_CNT_STEP; >>>>> + >>>>> + return val; >>>>> +} >>>>> + >>>>> +static const struct watchdog_ops sprd_wdt_ops = { >>>>> + .owner = THIS_MODULE, >>>>> + .start = sprd_wdt_start, >>>>> + .stop = sprd_wdt_stop, >>>>> + .set_timeout = sprd_wdt_set_timeout, >>>>> + .set_pretimeout = sprd_wdt_set_pretimeout, >>>>> + .get_timeleft = sprd_wdt_get_timeleft, >>>> >>>> >>>> Just wondering - no heartbeat function ? Having to load the timer >>>> values for each heartbeat is expensive. >>>> >>> >>> Unfortunately, this watchdog has not RELOAD register. >>> >>>>> >>>>> +}; >>>>> + >>>>> +static const struct watchdog_info sprd_wdt_info = { >>>>> + .options = WDIOF_SETTIMEOUT | >>>>> + WDIOF_PRETIMEOUT | >>>>> + WDIOF_MAGICCLOSE | >>>>> + WDIOF_KEEPALIVEPING, >>>>> + .identity = "Spreadtrum Watchdog Timer", >>>>> +}; >>>>> + >>>>> +static int sprd_wdt_probe(struct platform_device *pdev) >>>>> +{ >>>>> + struct resource *wdt_res; >>>>> + struct sprd_wdt *wdt; >>>>> + int ret; >>>>> + >>>>> + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); >>>>> + if (!wdt) >>>>> + return -ENOMEM; >>>>> + >>>>> + wdt_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>> + if (!wdt_res) { >>>>> + dev_err(&pdev->dev, "failed to memory resource\n"); >>>>> + return -ENOMEM; >>>>> + } >>>> >>>> >>>> Unnecessary error check and message. devm_ioremap_resource() >>>> returns an error if res is NULL. >>>> >>> >>> OK, I will fix it. >>> >>>>> >>>>> + >>>>> + wdt->base = devm_ioremap_resource(&pdev->dev, wdt_res); >>>>> + if (IS_ERR(wdt->base)) >>>> >>>> >>>> Move the error message to here. >>>> >>>>> + return PTR_ERR(wdt->base); >>>>> + >>>>> + wdt->enable = devm_clk_get(&pdev->dev, "enable"); >>>>> + if (IS_ERR(wdt->enable)) { >>>>> + dev_err(&pdev->dev, "can't get the enable clock\n"); >>>>> + return PTR_ERR(wdt->enable); >>>>> + } >>>>> + >>>>> + wdt->rtc_enable = devm_clk_get(&pdev->dev, "rtc_enable"); >>>>> + if (IS_ERR(wdt->rtc_enable)) { >>>>> + dev_err(&pdev->dev, "can't get the rtc enable clock\n"); >>>>> + return PTR_ERR(wdt->rtc_enable); >>>>> + } >>>>> + >>>>> + wdt->irq = platform_get_irq(pdev, 0); >>>>> + if (wdt->irq < 0) { >>>>> + dev_err(&pdev->dev, "failed to get IRQ resource\n"); >>>>> + return wdt->irq; >>>>> + } >>>>> + >>>>> + ret = devm_request_irq(&pdev->dev, wdt->irq, sprd_wdt_isr, >>>>> + IRQF_NO_SUSPEND, "sprd-wdt", (void >>>>> *)wdt); >>>>> + if (ret) { >>>>> + dev_err(&pdev->dev, "failed to register irq\n"); >>>>> + return ret; >>>>> + } >>>>> + >>>>> + wdt->wdd.info = &sprd_wdt_info; >>>>> + wdt->wdd.ops = &sprd_wdt_ops; >>>>> + wdt->wdd.parent = &pdev->dev; >>>>> + wdt->wdd.min_timeout = SPRD_WDT_MIN_TIMROUT; >>>>> + wdt->wdd.max_timeout = SPRD_WDT_MAX_TIMEOUT; >>>>> + >>>> >>>> You might want to set wdt->wdd.timeout to a default value. >>>> >>> >>> OK, I will set a default timeout value in case of no timeout-sec in >>> device-tree. >>> >>>>> >>>>> + ret = sprd_wdt_enable(wdt); >>>>> + if (ret) { >>>>> + dev_err(&pdev->dev, "failed to enable wdt\n"); >>>>> + return ret; >>>>> + } >>>>> + >>>> >>>> >>>> This needs a matching sprd_wdt_disable() call in the remove function. >>>> Best way to handle this would be to add devm_add_action() here. >>>> >>> >>> If add devm_add_action(), it will be called in the remove function, >>> but this is not our expect, if someone remove this module, we want it just >>> timeout. >>> >> >> But that leaves the watchdog enabled even if it was stopped (no call to >> sprd_wdt_disable()). > > Eric said It can not be stopped since we have set WATCHDOG_NOWAYOUT, > which means it will reboot the system when removing the watchdog. > >> Relying on NOWAYOUT would be a better option here. Again, you are >> making a choice for the user. > > we have set WATCHDOG_NOWAYOUT. > Yes, I understand, but that is configurable. Guenter >> >> >>>>> + watchdog_set_nowayout(&wdt->wdd, WATCHDOG_NOWAYOUT); >>>>> + watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev); >>>>> + >>>>> + ret = watchdog_register_device(&wdt->wdd); >>>> >>>> >>>> Unfortunately this doesn't work. It leaves interrupts enabled >>>> after the watchdog device is removed in sprd_wdt_remove(), >>>> but the driver will be gone. I would suggest to use >>>> devm_watchdog_register_device() and reorder calls to request >>>> the interrupt after registering the watchdog device. Then you >>>> can drop the remove function entirely. >>>> >>> >>> Yes, you are right, I understand your opinion, and it is very important, >>> devm_watchdog_register_device() is better. Thanks. >>> >>>>> + if (ret) { >>>>> + sprd_wdt_disable(wdt); >>>>> + dev_err(&pdev->dev, "failed to register watchdog\n"); >>>>> + return ret; >>>>> + } >>>>> + platform_set_drvdata(pdev, wdt); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int sprd_wdt_remove(struct platform_device *pdev) >>>>> +{ >>>>> + struct sprd_wdt *wdt = platform_get_drvdata(pdev); >>>>> + >>>>> + watchdog_unregister_device(&wdt->wdd); >>>>> + >>>>> + if (sprd_wdt_is_running(wdt)) >>>>> + dev_crit(&pdev->dev, "Device removed: Expect >>>>> reboot!\n"); >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int __maybe_unused sprd_wdt_pm_suspend(struct device *dev) >>>>> +{ >>>>> + struct watchdog_device *wdd = dev_get_drvdata(dev); >>>>> + struct sprd_wdt *wdt = dev_get_drvdata(dev); >>>>> + >>>>> + if (watchdog_active(wdd)) { >>>>> + sprd_wdt_stop(&wdt->wdd); >>>>> + sprd_wdt_disable(wdt); >>>> >>>> >>>> Are you sure you only want to disable the clocks if the watchdog was >>>> active ? >>>> That seems to be a bit inconsistent. >>>> >>> >>> This watchdog is in always on power domain, if system suspend, it needs to >>> disable it, or it will timeout. >>> >> That is not what I asked. I asked why it isn't >> >> if (watchdog_active(wdd)) >> sprd_wdt_stop(&wdt->wdd); >> >> sprd_wdt_disable(wdt); >> >> instead. > > Yes, Eric will fix this in next version. > >> >>>>> + } >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int __maybe_unused sprd_wdt_pm_resume(struct device *dev) >>>>> +{ >>>>> + struct watchdog_device *wdd = dev_get_drvdata(dev); >>>>> + struct sprd_wdt *wdt = dev_get_drvdata(dev); >>>>> + int ret; >>>>> + >>>>> + if (watchdog_active(wdd)) { >>>>> + ret = sprd_wdt_enable(wdt); >>>>> + if (ret) >>>>> + return ret; >>>>> + ret = sprd_wdt_start(&wdt->wdd); >>>>> + if (ret) { >>>>> + sprd_wdt_disable(wdt); >>>>> + return ret; >>>>> + } >>>> >>>> >>>> This can leave the system in an inconsistent state. Sometimes the wdt may >>>> be >>>> disabled, sometimes not. >>>> >>> >>> If watchdog enable failed, it means there is something wrong in this >>> system. >>> >> >> >> True. >> >> Guenter > > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 30 October 2017 at 17:32, Guenter Roeck <linux@roeck-us.net> wrote: > On 10/30/2017 02:18 AM, Baolin Wang wrote: >> >> Hi Guenter, >> >> (There are some problem with Eric's email, he can not receive this >> email, so I help to reply his comments following yours. sorry for >> troubles.) >> >>>>>> +#define SPRD_WDT_MAX_TIMEOUT 60 >>>>> >>>>> >>>>> >>>>> Is that really the maximum supported timeout ? Seems a bit low. >>>>> Shouldn't it be something like (U32_MAX / SPRD_WDT_CNT_STEP) ? >>>>> >>>> >>>> It supports the max value like as U32_MAX/SPRD_WDT_CNT_STEP, >>>> but it doesn't unnecessary to support so big value, if the system can >>>> not >>>> response it in 60s, the watchdog could trigger timeout. >>>> >>> It is customary to provide the highest possible value here. >>> No one is forced to set such high values. You are making a choice for >>> others here. But, sure, if you insist; not worth arguing about. >> >> >> Eric said 60s is better for us, thanks for your patient explanation. >> >>> >>>>>> + >>>>>> +#define SPRD_WDT_CNT_HIGH_VALUE 16 >>>>> >>>>> >>>>> >>>>> Maybe name it "SPRD_WDT_CNT_HIGH_SHIFT". It is not really a value, >>>>> it is a shift. >>>>> >>>> >>>> Yes, you are right, _SHIFT will be better. >>>> >>>>>> >>>>>> +#define SPRD_WDT_LOW_VALUE_MASK GENMASK(15, 0) >>>>>> +#define SPRD_WDT_CNT_VALUE_MAX GENMASK(31, 0) >>>>> >>>>> >>>>> >>>>> Does this mask serve a useful purpose ? >>>>> >>>> >>>> OK, I will remove it, thanks! >>>> >>>>>> +#define SPRD_WDT_LOAD_TIMEOUT 1000 >>>>>> + >>>>>> +struct sprd_wdt { >>>>>> + void __iomem *base; >>>>>> + struct watchdog_device wdd; >>>>>> + struct clk *enable; >>>>>> + struct clk *rtc_enable; >>>>>> + unsigned int irq; >>>>>> +}; >>>>>> + >>>>>> +static inline struct sprd_wdt *to_sprd_wdt(struct watchdog_device >>>>>> *wdd) >>>>>> +{ >>>>>> + return container_of(wdd, struct sprd_wdt, wdd); >>>>>> +} >>>>>> + >>>>>> +static inline void sprd_wdt_lock(void __iomem *addr) >>>>>> +{ >>>>>> + writel_relaxed(0x0, addr + SPRD_WDT_LOCK); >>>>>> +} >>>>>> + >>>>>> +static inline void sprd_wdt_unlock(void __iomem *addr) >>>>>> +{ >>>>>> + writel_relaxed(SPRD_WDT_UNLOCK_KEY, addr + SPRD_WDT_LOCK); >>>>>> +} >>>>>> + >>>>>> +static inline bool sprd_wdt_is_running(struct sprd_wdt *wdt) >>>>>> +{ >>>>>> + u32 val; >>>>>> + >>>>>> + val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); >>>>>> + return val & SPRD_WDT_NEW_VER_EN; >>>>>> +} >>>>>> + >>>>>> +static irqreturn_t sprd_wdt_isr(int irq, void *dev_id) >>>>>> +{ >>>>>> + struct sprd_wdt *wdt = (struct sprd_wdt *)dev_id; >>>>>> + >>>>>> + sprd_wdt_unlock(wdt->base); >>>>>> + writel_relaxed(SPRD_WDT_INT_CLEAR_BIT, wdt->base + >>>>>> SPRD_WDT_INT_CLR); >>>>>> + sprd_wdt_lock(wdt->base); >>>>>> + watchdog_notify_pretimeout(&wdt->wdd); >>>>>> + return IRQ_HANDLED; >>>>>> +} >>>>>> + >>>>>> +static u32 sprd_wdt_get_cnt_value(struct sprd_wdt *wdt) >>>>>> +{ >>>>>> + u32 val; >>>>>> + >>>>>> + val = readl_relaxed(wdt->base + SPRD_WDT_CNT_HIGH) << >>>>>> + SPRD_WDT_CNT_HIGH_VALUE; >>>>>> + val |= readl_relaxed(wdt->base + SPRD_WDT_CNT_LOW) & >>>>>> + SPRD_WDT_LOW_VALUE_MASK; >>>>>> + >>>>>> + return val; >>>>>> +} >>>>>> + >>>>>> +static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout, >>>>>> + u32 pretimeout) >>>>>> +{ >>>>>> + u32 val, delay_cnt = 0; >>>>>> + u32 tmr_step = timeout * SPRD_WDT_CNT_STEP; >>>>>> + u32 prtmr_step = pretimeout * SPRD_WDT_CNT_STEP; >>>>>> + >>>>>> + sprd_wdt_unlock(wdt->base); >>>>>> + writel_relaxed((tmr_step >> SPRD_WDT_CNT_HIGH_VALUE) & >>>>>> + SPRD_WDT_LOW_VALUE_MASK, wdt->base + >>>>>> SPRD_WDT_LOAD_HIGH); >>>>>> + writel_relaxed((tmr_step & SPRD_WDT_LOW_VALUE_MASK), >>>>>> + wdt->base + SPRD_WDT_LOAD_LOW); >>>>>> + writel_relaxed((prtmr_step >> SPRD_WDT_CNT_HIGH_VALUE) & >>>>>> + SPRD_WDT_LOW_VALUE_MASK, >>>>>> + wdt->base + SPRD_WDT_IRQ_LOAD_HIGH); >>>>>> + writel_relaxed(prtmr_step & SPRD_WDT_LOW_VALUE_MASK, >>>>>> + wdt->base + SPRD_WDT_IRQ_LOAD_LOW); >>>>>> + sprd_wdt_lock(wdt->base); >>>>>> + >>>>>> + /* >>>>>> + * Waiting the load value operation done, >>>>>> + * it needs two or three RTC clock cycles. >>>>>> + */ >>>>>> + do { >>>>>> + val = readl_relaxed(wdt->base + SPRD_WDT_INT_RAW); >>>>>> + if (!(val & SPRD_WDT_LD_BUSY_BIT)) >>>>>> + break; >>>>>> + >>>>>> + cpu_relax(); >>>>>> + } while (delay_cnt++ < SPRD_WDT_LOAD_TIMEOUT); >>>>>> + >>>>>> + if (delay_cnt >= SPRD_WDT_LOAD_TIMEOUT) >>>>>> + return -EBUSY; >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int sprd_wdt_enable(struct sprd_wdt *wdt) >>>>>> +{ >>>>>> + u32 val; >>>>>> + int ret; >>>>>> + >>>>>> + ret = clk_prepare_enable(wdt->enable); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + ret = clk_prepare_enable(wdt->rtc_enable); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + >>>>>> + sprd_wdt_unlock(wdt->base); >>>>>> + val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); >>>>>> + val |= SPRD_WDT_NEW_VER_EN; >>>>>> + writel_relaxed(val, wdt->base + SPRD_WDT_CTRL); >>>>>> + sprd_wdt_lock(wdt->base); >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static void sprd_wdt_disable(struct sprd_wdt *wdt) >>>>>> +{ >>>>>> + sprd_wdt_unlock(wdt->base); >>>>>> + writel_relaxed(0x0, wdt->base + SPRD_WDT_CTRL); >>>>>> + sprd_wdt_lock(wdt->base); >>>>>> + >>>>>> + clk_disable_unprepare(wdt->rtc_enable); >>>>>> + clk_disable_unprepare(wdt->enable); >>>>>> +} >>>>>> + >>>>>> +static int sprd_wdt_start(struct watchdog_device *wdd) >>>>>> +{ >>>>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd); >>>>>> + u32 val; >>>>>> + int ret; >>>>>> + >>>>>> + ret = sprd_wdt_load_value(wdt, wdd->timeout, wdd->pretimeout); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + >>>>>> + sprd_wdt_unlock(wdt->base); >>>>>> + val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); >>>>>> + val |= SPRD_WDT_CNT_EN_BIT | SPRD_WDT_INT_EN_BIT | >>>>>> SPRD_WDT_RST_EN_BIT; >>>>>> + writel_relaxed(val, wdt->base + SPRD_WDT_CTRL); >>>>>> + sprd_wdt_lock(wdt->base); >>>>>> + set_bit(WDOG_HW_RUNNING, &wdd->status); >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int sprd_wdt_stop(struct watchdog_device *wdd) >>>>>> +{ >>>>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd); >>>>>> + u32 val; >>>>>> + >>>>>> + sprd_wdt_unlock(wdt->base); >>>>>> + val = readl_relaxed(wdt->base + SPRD_WDT_CTRL); >>>>>> + val &= ~(SPRD_WDT_CNT_EN_BIT | SPRD_WDT_RST_EN_BIT | >>>>>> + SPRD_WDT_INT_EN_BIT); >>>>>> + writel_relaxed(val, wdt->base + SPRD_WDT_CTRL); >>>>>> + sprd_wdt_lock(wdt->base); >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int sprd_wdt_set_timeout(struct watchdog_device *wdd, >>>>>> + u32 timeout) >>>>>> +{ >>>>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd); >>>>>> + >>>>>> + if (timeout == wdd->timeout) >>>>>> + return 0; >>>>>> + >>>>>> + wdd->timeout = timeout; >>>>>> + >>>>>> + return sprd_wdt_load_value(wdt, timeout, wdd->pretimeout); >>>>>> +} >>>>>> + >>>>>> +static int sprd_wdt_set_pretimeout(struct watchdog_device *wdd, >>>>>> + u32 new_pretimeout) >>>>>> +{ >>>>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd); >>>>>> + >>>>>> + if (new_pretimeout == wdd->pretimeout) >>>>>> + return 0; >>>>> >>>>> >>>>> >>>>> This is inconsistent. pretimeout == 0 is accepted until it is set >>>>> to a non-zero value once, then 0 is no longer accepted. pretimeout==0 >>>>> should reflect "pretimeout disabled". If that is not possible you >>>>> need to set some non-0 default value in the probe function. >>>>> >>>> >>>> I understand your opinion, I will fix it. >>>> >>>>>> + if (new_pretimeout <= wdd->min_timeout) >>>>>> + return -EINVAL; >>>>> >>>>> >>>>> >>>>> Why is pretimeout == wdd->min_timeout invalid ? >>>>> >>>> >>>> OK, you are right, it should be pretimeout < min_timeout. >>>> >>>>>> + >>>>>> + wdd->pretimeout = new_pretimeout; >>>>>> + >>>>>> + return sprd_wdt_load_value(wdt, wdd->timeout, new_pretimeout); >>>>>> +} >>>>>> + >>>>>> +static u32 sprd_wdt_get_timeleft(struct watchdog_device *wdd) >>>>>> +{ >>>>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd); >>>>>> + u32 val; >>>>>> + >>>>>> + val = sprd_wdt_get_cnt_value(wdt); >>>>>> + val = val / SPRD_WDT_CNT_STEP; >>>>>> + >>>>>> + return val; >>>>>> +} >>>>>> + >>>>>> +static const struct watchdog_ops sprd_wdt_ops = { >>>>>> + .owner = THIS_MODULE, >>>>>> + .start = sprd_wdt_start, >>>>>> + .stop = sprd_wdt_stop, >>>>>> + .set_timeout = sprd_wdt_set_timeout, >>>>>> + .set_pretimeout = sprd_wdt_set_pretimeout, >>>>>> + .get_timeleft = sprd_wdt_get_timeleft, >>>>> >>>>> >>>>> >>>>> Just wondering - no heartbeat function ? Having to load the timer >>>>> values for each heartbeat is expensive. >>>>> >>>> >>>> Unfortunately, this watchdog has not RELOAD register. >>>> >>>>>> >>>>>> +}; >>>>>> + >>>>>> +static const struct watchdog_info sprd_wdt_info = { >>>>>> + .options = WDIOF_SETTIMEOUT | >>>>>> + WDIOF_PRETIMEOUT | >>>>>> + WDIOF_MAGICCLOSE | >>>>>> + WDIOF_KEEPALIVEPING, >>>>>> + .identity = "Spreadtrum Watchdog Timer", >>>>>> +}; >>>>>> + >>>>>> +static int sprd_wdt_probe(struct platform_device *pdev) >>>>>> +{ >>>>>> + struct resource *wdt_res; >>>>>> + struct sprd_wdt *wdt; >>>>>> + int ret; >>>>>> + >>>>>> + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); >>>>>> + if (!wdt) >>>>>> + return -ENOMEM; >>>>>> + >>>>>> + wdt_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>>> + if (!wdt_res) { >>>>>> + dev_err(&pdev->dev, "failed to memory resource\n"); >>>>>> + return -ENOMEM; >>>>>> + } >>>>> >>>>> >>>>> >>>>> Unnecessary error check and message. devm_ioremap_resource() >>>>> returns an error if res is NULL. >>>>> >>>> >>>> OK, I will fix it. >>>> >>>>>> >>>>>> + >>>>>> + wdt->base = devm_ioremap_resource(&pdev->dev, wdt_res); >>>>>> + if (IS_ERR(wdt->base)) >>>>> >>>>> >>>>> >>>>> Move the error message to here. >>>>> >>>>>> + return PTR_ERR(wdt->base); >>>>>> + >>>>>> + wdt->enable = devm_clk_get(&pdev->dev, "enable"); >>>>>> + if (IS_ERR(wdt->enable)) { >>>>>> + dev_err(&pdev->dev, "can't get the enable clock\n"); >>>>>> + return PTR_ERR(wdt->enable); >>>>>> + } >>>>>> + >>>>>> + wdt->rtc_enable = devm_clk_get(&pdev->dev, "rtc_enable"); >>>>>> + if (IS_ERR(wdt->rtc_enable)) { >>>>>> + dev_err(&pdev->dev, "can't get the rtc enable >>>>>> clock\n"); >>>>>> + return PTR_ERR(wdt->rtc_enable); >>>>>> + } >>>>>> + >>>>>> + wdt->irq = platform_get_irq(pdev, 0); >>>>>> + if (wdt->irq < 0) { >>>>>> + dev_err(&pdev->dev, "failed to get IRQ resource\n"); >>>>>> + return wdt->irq; >>>>>> + } >>>>>> + >>>>>> + ret = devm_request_irq(&pdev->dev, wdt->irq, sprd_wdt_isr, >>>>>> + IRQF_NO_SUSPEND, "sprd-wdt", (void >>>>>> *)wdt); >>>>>> + if (ret) { >>>>>> + dev_err(&pdev->dev, "failed to register irq\n"); >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + wdt->wdd.info = &sprd_wdt_info; >>>>>> + wdt->wdd.ops = &sprd_wdt_ops; >>>>>> + wdt->wdd.parent = &pdev->dev; >>>>>> + wdt->wdd.min_timeout = SPRD_WDT_MIN_TIMROUT; >>>>>> + wdt->wdd.max_timeout = SPRD_WDT_MAX_TIMEOUT; >>>>>> + >>>>> >>>>> >>>>> You might want to set wdt->wdd.timeout to a default value. >>>>> >>>> >>>> OK, I will set a default timeout value in case of no timeout-sec in >>>> device-tree. >>>> >>>>>> >>>>>> + ret = sprd_wdt_enable(wdt); >>>>>> + if (ret) { >>>>>> + dev_err(&pdev->dev, "failed to enable wdt\n"); >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>> >>>>> >>>>> >>>>> This needs a matching sprd_wdt_disable() call in the remove function. >>>>> Best way to handle this would be to add devm_add_action() here. >>>>> >>>> >>>> If add devm_add_action(), it will be called in the remove function, >>>> but this is not our expect, if someone remove this module, we want it >>>> just >>>> timeout. >>>> >>> >>> But that leaves the watchdog enabled even if it was stopped (no call to >>> sprd_wdt_disable()). >> >> >> Eric said It can not be stopped since we have set WATCHDOG_NOWAYOUT, >> which means it will reboot the system when removing the watchdog. >> >>> Relying on NOWAYOUT would be a better option here. Again, you are >>> making a choice for the user. >> >> >> we have set WATCHDOG_NOWAYOUT. >> > Yes, I understand, but that is configurable. Yes, you are right. So I think we need add test_bit(WDOG_NO_WAY_OUT, &wdd->status) in remove function to decide if we should issue sprd_wdt_disable(). >>> >>> >>>>>> + watchdog_set_nowayout(&wdt->wdd, WATCHDOG_NOWAYOUT); >>>>>> + watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev); >>>>>> + >>>>>> + ret = watchdog_register_device(&wdt->wdd); >>>>> >>>>> >>>>> >>>>> Unfortunately this doesn't work. It leaves interrupts enabled >>>>> after the watchdog device is removed in sprd_wdt_remove(), >>>>> but the driver will be gone. I would suggest to use >>>>> devm_watchdog_register_device() and reorder calls to request >>>>> the interrupt after registering the watchdog device. Then you >>>>> can drop the remove function entirely. >>>>> >>>> >>>> Yes, you are right, I understand your opinion, and it is very important, >>>> devm_watchdog_register_device() is better. Thanks. >>>> >>>>>> + if (ret) { >>>>>> + sprd_wdt_disable(wdt); >>>>>> + dev_err(&pdev->dev, "failed to register watchdog\n"); >>>>>> + return ret; >>>>>> + } >>>>>> + platform_set_drvdata(pdev, wdt); >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int sprd_wdt_remove(struct platform_device *pdev) >>>>>> +{ >>>>>> + struct sprd_wdt *wdt = platform_get_drvdata(pdev); >>>>>> + >>>>>> + watchdog_unregister_device(&wdt->wdd); >>>>>> + >>>>>> + if (sprd_wdt_is_running(wdt)) >>>>>> + dev_crit(&pdev->dev, "Device removed: Expect >>>>>> reboot!\n"); >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int __maybe_unused sprd_wdt_pm_suspend(struct device *dev) >>>>>> +{ >>>>>> + struct watchdog_device *wdd = dev_get_drvdata(dev); >>>>>> + struct sprd_wdt *wdt = dev_get_drvdata(dev); >>>>>> + >>>>>> + if (watchdog_active(wdd)) { >>>>>> + sprd_wdt_stop(&wdt->wdd); >>>>>> + sprd_wdt_disable(wdt); >>>>> >>>>> >>>>> >>>>> Are you sure you only want to disable the clocks if the watchdog was >>>>> active ? >>>>> That seems to be a bit inconsistent. >>>>> >>>> >>>> This watchdog is in always on power domain, if system suspend, it needs >>>> to >>>> disable it, or it will timeout. >>>> >>> That is not what I asked. I asked why it isn't >>> >>> if (watchdog_active(wdd)) >>> sprd_wdt_stop(&wdt->wdd); >>> >>> sprd_wdt_disable(wdt); >>> >>> instead. >> >> >> Yes, Eric will fix this in next version. >> >>> >>>>>> + } >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int __maybe_unused sprd_wdt_pm_resume(struct device *dev) >>>>>> +{ >>>>>> + struct watchdog_device *wdd = dev_get_drvdata(dev); >>>>>> + struct sprd_wdt *wdt = dev_get_drvdata(dev); >>>>>> + int ret; >>>>>> + >>>>>> + if (watchdog_active(wdd)) { >>>>>> + ret = sprd_wdt_enable(wdt); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + ret = sprd_wdt_start(&wdt->wdd); >>>>>> + if (ret) { >>>>>> + sprd_wdt_disable(wdt); >>>>>> + return ret; >>>>>> + } >>>>> >>>>> >>>>> >>>>> This can leave the system in an inconsistent state. Sometimes the wdt >>>>> may >>>>> be >>>>> disabled, sometimes not. >>>>> >>>> >>>> If watchdog enable failed, it means there is something wrong in this >>>> system. >>>> >>> >>> >>> True. >>> >>> Guenter >> >> >> >> >
diff --git a/Documentation/devicetree/bindings/watchdog/sprd-wdt.txt b/Documentation/devicetree/bindings/watchdog/sprd-wdt.txt new file mode 100644 index 0000000..aeaf3e0 --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/sprd-wdt.txt @@ -0,0 +1,19 @@ +Spreadtrum SoCs Watchdog timer + +Required properties: +- compatible : Should be "sprd,sp9860-wdt". +- reg : Specifies base physical address and size of the registers. +- interrupts : Exactly one interrupt specifier. +- timeout-sec : Contain the default watchdog timeout in seconds. +- clock-names : Contain the input clock names. +- clocks : Phandles to input clocks. + +Example: + watchdog: watchdog@40310000 { + compatible = "sprd,sp9860-wdt"; + reg = <0 0x40310000 0 0x1000>; + interrupts = <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>; + timeout-sec = <12>; + clock-names = "enable", "rtc_enable"; + clocks = <&clk_aon_apb_gates1 8>, <&clk_aon_apb_rtc_gates 9>; + };