Just add the driver author Eric. On 25 October 2017 at 19:13, Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > On 10/25/2017 03:29 AM, Eric Long wrote: >> >> Hi Guenter, >> >> Sorry for late reply, and thanks for your detail comments. >> >> On Sun, Oct 22, 2017 at 09:07:29AM -0700, Guenter Roeck wrote: >>> >>> On Tue, Sep 12, 2017 at 07:40:09PM +0800, Eric Long wrote: >>>> >>>> This patch adds the watchdog driver for Spreadtrum SC9860 platform. >>>> >>>> Signed-off-by: Eric Long <eric.long@xxxxxxxxxxxxxx> >>>> --- >>>> 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 | 384 >>>> ++++++++++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 393 insertions(+) >>>> create mode 100644 drivers/watchdog/sprd_wdt.c >>>> >>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >>>> index c722cbf..ea07718 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 >>>> + select WATCHDOG_CORE >>>> + help >>>> + Say Y here to include support watchdog timer embedded >>>> + into the Spreadtrum system. >>>> + >>>> # 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..dedbca6fd >>>> --- /dev/null >>>> +++ b/drivers/watchdog/sprd_wdt.c >>>> @@ -0,0 +1,384 @@ >>>> +/* >>>> + * 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/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 WDT_LOAD_LOW 0x0 >>>> +#define WDT_LOAD_HIGH 0x4 >>>> +#define WDT_CTRL 0x8 >>>> +#define WDT_INT_CLR 0xc >>>> +#define WDT_INT_RAW 0x10 >>>> +#define WDT_INT_MSK 0x14 >>>> +#define WDT_CNT_LOW 0x18 >>>> +#define WDT_CNT_HIGH 0x1c >>>> +#define WDT_LOCK 0x20 >>>> +#define WDT_IRQ_LOAD_LOW 0x2c >>>> +#define WDT_IRQ_LOAD_HIGH 0x30 >>>> + >>>> +/* WDT_CTRL */ >>>> +#define WDT_INT_EN_BIT BIT(0) >>>> +#define WDT_CNT_EN_BIT BIT(1) >>>> +#define WDT_NEW_VER_EN BIT(2) >>>> +#define WDT_RST_EN_BIT BIT(3) >>>> + >>>> +/* WDT_INT_CLR */ >>>> +#define WDT_INT_CLEAR_BIT BIT(0) >>>> +#define WDT_RST_CLEAR_BIT BIT(3) >>>> + >>> >>> Requires include of bitops.h. >> >> >> OK, I will fix it. >> >>>> +/* WDT_INT_RAW */ >>>> +#define WDT_INT_RAW_BIT BIT(0) >>>> +#define WDT_RST_RAW_BIT BIT(3) >>>> +#define WDT_LD_BUSY_BIT BIT(4) >>>> + >>>> +#define WDT_CLK 32768 >>> >>> >>> Would it make sense to use clk_get_rate() instead ? >> >> >> This wdt works at 153.6MHz, but its count step is 32768, >> so it cannot get the count step value by clk_get_rate(). >> > > Please add a comment, and maybe rename the define to avoid the assumption > that it is related to the clock rate. > > >>>> +#define WDT_UNLOCK_KEY 0xe551 >>>> +#define WDT_DEFAULT_PRETMROUT 3 >>>> + >>>> +#define WDT_CNT_VALUE_SIZE 16 >>>> +#define WDT_CNT_VALUE_MASK GENMASK(15, 0) >>>> +#define WDT_LOAD_TIMEOUT_NUM 10000 >>>> + >>>> +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 + WDT_LOCK); >>>> +} >>>> + >>>> +static inline void sprd_wdt_unlock(void __iomem *addr) >>>> +{ >>>> + writel_relaxed(WDT_UNLOCK_KEY, addr + WDT_LOCK); >>>> +} >>>> + >>>> +static inline bool sprd_wdt_is_running(struct sprd_wdt *wdt) >>>> +{ >>>> + u32 val; >>>> + >>>> + val = readl_relaxed(wdt->base + WDT_CTRL); >>>> + return val & 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(WDT_INT_CLEAR_BIT, wdt->base + 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 + WDT_CNT_HIGH) << >>>> WDT_CNT_VALUE_SIZE; >>>> + val |= readl_relaxed(wdt->base + WDT_CNT_LOW) & >>>> WDT_CNT_VALUE_MASK; >>>> + >>>> + return val; >>>> +} >>>> + >>>> +static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout, >>>> + u32 pretimeout) >>>> +{ >>>> + u32 val, cnt = 0; >>>> + >>>> + if (timeout < pretimeout) >>>> + return -EINVAL; >>>> + >>> >>> >>> This is the wrong place to check if the timeout is valid. >>> The core should know about limits and perform the checks. >> >> >> OK, I will fix it. >> >>>> >>>> + if (!pretimeout) >>>> + pretimeout = WDT_DEFAULT_PRETMROUT; >>>> + >>> >>> >>> If pretimeout was 0 and timeout < 3, this will accept the timeout. If the >>> pretimeout is mandatory, it should be enforced, and the minimum timeout >>> should be larger than the miniumum pretimeout. >> >> >> OK, I will add min/max timeout at probe function. >> >>>> + sprd_wdt_unlock(wdt->base); >>>> + writel_relaxed(((timeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) & >>>> + WDT_CNT_VALUE_MASK, wdt->base + WDT_LOAD_HIGH); >>> >>> >>> This can overflow. The maximum timeout must be <= 0xffffffff / WDT_CLK. >> >> >> Yes, you are right, I will fix it, and the timeout value will be limited >> by min/max timeout, so there is no need to judge this timeout value after >> I add min/max timeout at probe function. >> >>>> >>>> + writel_relaxed(((timeout * WDT_CLK) & WDT_CNT_VALUE_MASK), >>>> + wdt->base + WDT_LOAD_LOW); >>>> + writel_relaxed(((pretimeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) & >>>> + WDT_CNT_VALUE_MASK, wdt->base + >>>> WDT_IRQ_LOAD_HIGH); >>> >>> >>> Same for pretimeout. >>> >>>> + writel_relaxed((pretimeout * WDT_CLK) & WDT_CNT_VALUE_MASK, >>>> + wdt->base + 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 + WDT_INT_RAW); >>>> + if (!(val & WDT_LD_BUSY_BIT)) >>>> + break; >>>> + >>>> + cpu_relax(); >>>> + } while (cnt++ < WDT_LOAD_TIMEOUT_NUM); >>>> + >>>> + if (cnt >= WDT_LOAD_TIMEOUT_NUM) >>>> + return -EBUSY; >>>> + return 0; >>>> +} >>>> + >>>> +static void sprd_wdt_enable(struct sprd_wdt *wdt) >>>> +{ >>>> + u32 val; >>>> + >>>> + clk_prepare_enable(wdt->enable); >>>> + clk_prepare_enable(wdt->rtc_enable); >>> >>> >>> Both functions can fail. >> >> >> OK, I will fix it. >> >>>> >>>> + >>>> + sprd_wdt_unlock(wdt->base); >>>> + val = readl_relaxed(wdt->base + WDT_CTRL); >>>> + val |= WDT_NEW_VER_EN; >>>> + writel_relaxed(val, wdt->base + WDT_CTRL); >>>> + sprd_wdt_lock(wdt->base); >>>> + set_bit(WDOG_HW_RUNNING, &wdt->wdd.status); >>> >>> >>> Why ? The watchdog isn't started here. >> >> >> OK, I will delete it. >> >>>> +} >>>> + >>>> +static void sprd_wdt_disable(struct sprd_wdt *wdt) >>>> +{ >>>> + sprd_wdt_unlock(wdt->base); >>>> + writel_relaxed(0x0, wdt->base + WDT_CTRL); >>>> + sprd_wdt_lock(wdt->base); >>>> + >>>> + clk_disable(wdt->enable); >>>> + clk_disable(wdt->rtc_enable); >>> >>> >>> clk_prepare_enable but no matching clk_disable_unprepare ? >> >> >> Yes, thanks, you are right, it should use clk_disable_unprepare to >> instead. >> >>>> +} >>>> + >>>> +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 + WDT_CTRL); >>>> + val |= WDT_CNT_EN_BIT | WDT_INT_EN_BIT | WDT_RST_EN_BIT; >>>> + writel_relaxed(val, wdt->base + 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 + WDT_CTRL); >>>> + val &= ~(WDT_CNT_EN_BIT | WDT_RST_EN_BIT | WDT_INT_EN_BIT); >>>> + writel_relaxed(val, wdt->base + 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); >>>> + >>>> + wdd->timeout = timeout; >>>> + >>>> + return sprd_wdt_load_value(wdt, timeout, wdd->pretimeout); >>> >>> >>> Even on error, this accepts the new (bad) timeout. >> >> >> OK, I should add judgment api here. >> > > No, you should provide limits to the core and let the core handle it. > > >>>> +} >>>> + >>>> +static int sprd_wdt_set_pretimeout(struct watchdog_device *wdd, >>>> + u32 new_pretimeout) >>>> +{ >>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd); >>>> + >>>> + wdd->pretimeout = new_pretimeout; >>>> + >>>> + return sprd_wdt_load_value(wdt, wdd->timeout, new_pretimeout); >>> >>> >>> Even on error, this accepts the new (bad) pretimeout. >> >> >> OK, I should add judgment api here. >> >>>> >>>> +} >>>> + >>>> +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 / WDT_CLK; >>>> + >>>> + 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, >>>> +}; >>>> + >>>> +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; >>>> + } >>>> + >>>> + wdt->base = devm_ioremap_nocache(&pdev->dev, wdt_res->start, >>>> + resource_size(wdt_res)); >>> >>> >>> Consider using devm_ioremap_resource(). >> >> >> Yes, thanks, devm_ioremap_resource() will be better. >> >>>> >>>> + if (!wdt->base) >>>> + return -ENOMEM; >>>> + >>>> + 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; >>>> + >>> >>> >>> This should also set limits for min/max to let the core validate ranges. >>> If the minimum pretimeout is 3 seconds, the lower limit for timeout >>> should >>> be set accordingly. >> >> >> Yes, you are right, I should add min/max timeout to limit the validate >> ranges, thanks. >> >>>> >>>> + sprd_wdt_enable(wdt); >>>> + >>>> + watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev); >>>> + >>>> + ret = watchdog_register_device(&wdt->wdd); >>>> + if (ret) { >>>> + dev_err(&pdev->dev, "failed to register watchdog\n"); >>> >>> >>> No wdt disable on error ? >> >> >> Yes, it should call sprd_wdt_disable(). >> >>>> + 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); >>>> + >>>> + if (sprd_wdt_is_running(wdt)) { >>>> + sprd_wdt_stop(&wdt->wdd); >>>> + sprd_wdt_disable(wdt); >>>> + } >>> >>> >>> I assume you understand that this defeats NOWAYOUT. >> >> >> In my understand, if NOWAYOUT is set, this watchdog cannot be stopped, >> but we hope the watchdog can be stopped when someone want to debug the >> kernel. >> > > I would suggest that maybe NOWAYOUT should not be enabled in that case. > > Anyway, looks like the driver doesn't support NOWAYOUT anyway (why ?). > So what this defeats is MAGICCLOSE, and I still don't understand the logic > behind it. If you don't want to support it, fine, then just don't set the > flag, > and the core will stop the watchdog for you. > > Please add a short note to the driver explaining why you don't want those > flags to be supported, to prevent others from adding it later. > > Thanks, > Guenter > > >>>> + watchdog_unregister_device(&wdt->wdd); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int __maybe_unused sprd_wdt_pm_suspend(struct device *dev) >>>> +{ >>>> + struct sprd_wdt *wdt = dev_get_drvdata(dev); >>>> + >>>> + if (sprd_wdt_is_running(wdt)) { >>> >>> >>> if (watchdog_active()) should work here. >> >> >> Yes, you are right, I will fix it, thanks. >> >>>> >>>> + sprd_wdt_stop(&wdt->wdd); >>>> + sprd_wdt_disable(wdt); >>>> + } >>>> + >>>> + 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); >>>> + >>>> + if (watchdog_active(wdd) && !sprd_wdt_is_running(wdt)) { >>> >>> >>> sprd_wdt_is_running() should not be needed. >> >> >> OK, I will fix it, thanks. >> >>>> + sprd_wdt_enable(wdt); >>>> + sprd_wdt_start(&wdt->wdd); >>>> + } >>>> + >>>> + 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@xxxxxxxxxxxxxx>"); >>>> +MODULE_DESCRIPTION("Spreadtrum Watchdog Timer Controller Driver"); >>>> +MODULE_LICENSE("GPL v2"); >> >> > -- Baolin.wang Best Regards -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
