From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from bh-25.webhostbox.net ([208.91.199.152]:33826 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755924AbbLAJnN (ORCPT ); Tue, 1 Dec 2015 04:43:13 -0500 Subject: Re: [PATCH v3] watchdog: atlas7: add watchdog driver of CSRatlas7 To: Barry Song <21cnbao@gmail.com>, wim@iguana.be, linux-watchdog@vger.kernel.org References: <1448955825-17115-1-git-send-email-21cnbao@gmail.com> Cc: workgroup.linux@csr.com, Guo Zeng , William Wang , Barry Song From: Guenter Roeck Message-ID: <565D6BAD.6070609@roeck-us.net> Date: Tue, 1 Dec 2015 01:43:09 -0800 MIME-Version: 1.0 In-Reply-To: <1448955825-17115-1-git-send-email-21cnbao@gmail.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-watchdog-owner@vger.kernel.org List-Id: linux-watchdog@vger.kernel.org On 11/30/2015 11:43 PM, Barry Song wrote: > From: Guo Zeng > > This patch adds watchdog driver for CSRatlas7 platform. > On CSRatlas7, the 6th timer can act as a watchdog timer > when the Watchdog mode is enabled. > > Cc: Guenter Roeck > Signed-off-by: Guo Zeng > Signed-off-by: William Wang > Signed-off-by: Barry Song Are those people all in the sign-off path, or just somehow involved in writing the code ? > --- > -v3: fix issues according to Guenter's comments > 1. rename updatetimeout to ping > 2. drop redundant calling ping in set_timeout > 3. fix the error handler in probe > 4. move to __maybe_unused for suspend/resume entries > 5. fix the min and max timeout > 6. clean up clear ugly codes for timeout set > > drivers/watchdog/Kconfig | 10 ++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/atlas7_wdt.c | 249 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 260 insertions(+) > create mode 100644 drivers/watchdog/atlas7_wdt.c > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 1c427be..52b308f 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -578,6 +578,16 @@ config LPC18XX_WATCHDOG > To compile this driver as a module, choose M here: the > module will be called lpc18xx_wdt. > > +config ATLAS7_WATCHDOG > + tristate "CSRatlas7 watchdog" > + depends on ARCH_ATLAS7 > + help > + Say Y here to include Watchdog timer support for the watchdog > + existing on the CSRatlas7 series platforms. > + > + To compile this driver as a module, choose M here: the > + module will be called atlas7_wdt. > + > # AVR32 Architecture > > config AT32AP700X_WDT > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 53d4827..e2bc52c 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -69,6 +69,7 @@ obj-$(CONFIG_MEDIATEK_WATCHDOG) += mtk_wdt.o > obj-$(CONFIG_DIGICOLOR_WATCHDOG) += digicolor_wdt.o > obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o > obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o > +obj-$(CONFIG_ATLAS7_WATCHDOG) += atlas7_wdt.o > > # AVR32 Architecture > obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o > diff --git a/drivers/watchdog/atlas7_wdt.c b/drivers/watchdog/atlas7_wdt.c > new file mode 100644 > index 0000000..4f191fe > --- /dev/null > +++ b/drivers/watchdog/atlas7_wdt.c > @@ -0,0 +1,249 @@ > +/* > + * Watchdog driver for CSRatlas7 > + * > + * Copyright (c) 2015 Cambridge Silicon Radio Limited, a CSR plc group company. > + * > + * Licensed under GPLv2 or later. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include Not required. > +#include > + Please order include files alphabetically. > +#define ATLAS7_TIMER_WDT_INDEX 5 > +#define ATLAS7_WDT_DEFAULT_TIMEOUT 20 /* 20 secs */ > + > +#define ATLAS7_WDT_CNT_CTRL 0 > +#define ATLAS7_WDT_CNT_MATCH 0x18 > +#define ATLAS7_WDT_CNT 0x48 Wondering - below you always add 4 * ATLAS7_TIMER_WDT_INDEX to the WDT_CNT_CTRL, WDT_CNT_MATCH, and WDT_CNT registers. Wouldn't it be easier to specify the final register offsets directly ? #define ATLAS7_WDT_CNT_CTRL (0 + 4 * ATLAS7_TIMER_WDT_INDEX) or even #define ATLAS7_WDT_CNT_CTRL 0x14 The long expressions below just make the code more difficult to read. > +#define ATLAS7_WDT_CNT_EN (BIT(0) | BIT(1)) > +#define ATLAS7_WDT_EN 0x64 > + > +static unsigned int timeout = ATLAS7_WDT_DEFAULT_TIMEOUT; This will cause a devicetree based initialization to be ignored. It would be better to set this to 0 and initialize .timeout with ATLAS7_WDT_DEFAULT_TIMEOUT prior to calling watchdog_init_timeout(). > +static bool nowayout = WATCHDOG_NOWAYOUT; > + > +module_param(timeout, uint, 0); > +module_param(nowayout, bool, 0); > + > +MODULE_PARM_DESC(timeout, "Default watchdog timeout (in seconds)"); > +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=" > + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); > + > +struct atlas7_wdog { > + struct device *dev; > + void __iomem *base; > + unsigned long tick_rate; > + struct clk *clk; > +}; > + > +static unsigned int atlas7_wdt_gettimeleft(struct watchdog_device *wdd) > +{ > + struct atlas7_wdog *wdt = watchdog_get_drvdata(wdd); > + u32 counter, match, delta; > + > + counter = readl(wdt->base + ATLAS7_WDT_CNT + > + 4 * ATLAS7_TIMER_WDT_INDEX); > + match = readl(wdt->base + ATLAS7_WDT_CNT_MATCH + > + 4 * ATLAS7_TIMER_WDT_INDEX); In general, continuation lines should be aligned with '('. On the other side, macros such as #define WDT_CNT_REG(wdt) ((wdt)->base + ATLAS7_WDT_CNT + 4 * ATLAS7_TIMER_WDT_INDEX) #define WDT_CNT_MATCH_REG(wdt) ((wdt)->base + ATLAS7_WDT_CNT_MATCH + 4 * ATLAS7_TIMER_WDT_INDEX) #define WDT_CNT_CTRL_REG(wdt) ((wdt)->base + ATLAS7_WDT_CNT_CTRL + 4 * ATLAS7_TIMER_WDT_INDEX) might be quite useful to avoid the repeated use of long expressions. Or at least simplify the register definitions as suggested above. > + delta = match - counter; > + > + return delta / wdt->tick_rate; > +} > + > +static int atlas7_wdt_ping(struct watchdog_device *wdd) > +{ > + struct atlas7_wdog *wdt = watchdog_get_drvdata(wdd); > + u32 counter, match, delta; > + > + counter = readl(wdt->base + ATLAS7_WDT_CNT + > + 4 * ATLAS7_TIMER_WDT_INDEX); > + delta = wdd->timeout * wdt->tick_rate; > + match = counter + delta; > + > + writel(match, wdt->base + ATLAS7_WDT_CNT_MATCH + > + 4 * ATLAS7_TIMER_WDT_INDEX); > + > + return 0; > +} > + > +static int atlas7_wdt_enable(struct watchdog_device *wdd) > +{ > + struct atlas7_wdog *wdt = watchdog_get_drvdata(wdd); > + void __iomem *ctrl_reg = wdt->base + ATLAS7_WDT_CNT_CTRL + > + 4 * ATLAS7_TIMER_WDT_INDEX; > + > + atlas7_wdt_ping(wdd); > + > + writel(readl(ctrl_reg) | ATLAS7_WDT_CNT_EN, ctrl_reg); > + writel(1, wdt->base + ATLAS7_WDT_EN); > + > + return 0; > +} > + > +static int atlas7_wdt_disable(struct watchdog_device *wdd) > +{ > + struct atlas7_wdog *wdt = watchdog_get_drvdata(wdd); > + void __iomem *ctrl_reg = wdt->base + ATLAS7_WDT_CNT_CTRL + > + 4 * ATLAS7_TIMER_WDT_INDEX; > + > + writel(0, wdt->base + ATLAS7_WDT_EN); > + writel(readl(ctrl_reg) & ~ATLAS7_WDT_CNT_EN, ctrl_reg); > + > + return 0; > +} > + > +static int atlas7_wdt_settimeout(struct watchdog_device *wdd, unsigned int to) > +{ > + wdd->timeout = to; > + > + return 0; > +} > + > +#define OPTIONS (WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE) > + > +static const struct watchdog_info atlas7_wdt_ident = { > + .options = OPTIONS, > + .firmware_version = 0, > + .identity = "atlas7 Watchdog", The tab after '=' is really unnecessary. I would suggest to replace it with a space. > +}; > + > +static struct watchdog_ops atlas7_wdt_ops = { > + .owner = THIS_MODULE, > + .start = atlas7_wdt_enable, > + .stop = atlas7_wdt_disable, > + .get_timeleft = atlas7_wdt_gettimeleft, > + .ping = atlas7_wdt_ping, > + .set_timeout = atlas7_wdt_settimeout, > +}; > + > +static struct watchdog_device atlas7_wdd = { > + .info = &atlas7_wdt_ident, > + .ops = &atlas7_wdt_ops, > + .timeout = ATLAS7_WDT_DEFAULT_TIMEOUT, > +}; > + > +static const struct of_device_id atlas7_wdt_ids[] = { > + { .compatible = "sirf,atlas7-tick"}, > + {} > +}; > + > +static int atlas7_wdt_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct atlas7_wdog *wdt; > + struct resource *res; > + struct clk *clk; > + int ret; > + > + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); > + if (!wdt) > + return -ENOMEM; > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + wdt->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(wdt->base)) > + return PTR_ERR(wdt->base); > + > + clk = of_clk_get(np, 0); > + if (IS_ERR(clk)) { > + ret = PTR_ERR(clk); > + goto err; You can just return PTR_ERR(clk) here. The "err:" label, just to return, is unnecessary. > + } > + ret = clk_prepare_enable(clk); > + if (ret) { > + pr_err("wdt clk enable failed\n"); > + goto err1; wdt->clk is not yet set here, meaning the clk_put() below won't work. > + } > + > + /* disable watchdog hardware */ > + writel(0, wdt->base + ATLAS7_WDT_CNT_CTRL + > + 4 * ATLAS7_TIMER_WDT_INDEX); > + > + wdt->tick_rate = clk_get_rate(clk); > + wdt->clk = clk; > + atlas7_wdd.min_timeout = 1; > + atlas7_wdd.max_timeout = UINT_MAX / wdt->tick_rate; > + > + watchdog_init_timeout(&atlas7_wdd, timeout, &pdev->dev); > + watchdog_set_nowayout(&atlas7_wdd, nowayout); > + > + watchdog_set_drvdata(&atlas7_wdd, wdt); > + platform_set_drvdata(pdev, &atlas7_wdd); > + > + ret = watchdog_register_device(&atlas7_wdd); > + if (ret) > + goto err2; > + > + return 0; > + > +err2: > + clk_disable_unprepare(wdt->clk); > +err1: > + clk_put(wdt->clk); > +err: > + return ret; > +} > + > +static void atlas7_wdt_shutdown(struct platform_device *pdev) > +{ > + struct watchdog_device *wdd = platform_get_drvdata(pdev); > + struct atlas7_wdog *wdt = watchdog_get_drvdata(wdd); > + > + atlas7_wdt_disable(wdd); > + clk_disable_unprepare(wdt->clk); > +} > + > +static int atlas7_wdt_remove(struct platform_device *pdev) > +{ > + struct watchdog_device *wdd = platform_get_drvdata(pdev); > + struct atlas7_wdog *wdt = watchdog_get_drvdata(wdd); > + > + atlas7_wdt_shutdown(pdev); > + clk_put(wdt->clk); > + return 0; > +} > + > +static int __maybe_unused atlas7_wdt_suspend(struct device *dev) > +{ > + return 0; Is it not necessary to stop the watchdog ? If the watchdog is disabled elsewhere, a similar comment to the one below would help. > +} > + > +static int __maybe_unused atlas7_wdt_resume(struct device *dev) > +{ > + struct watchdog_device *wdd = dev_get_drvdata(dev); > + > + /* > + * NOTE: Since timer controller registers settings are saved > + * and restored back by the timer-atlas7.c, so we need not > + * update WD settings except refreshing timeout. > + */ > + atlas7_wdt_ping(wdd); > + > + return 0; > +} > + > +static SIMPLE_DEV_PM_OPS(atlas7_wdt_pm_ops, > + atlas7_wdt_suspend, atlas7_wdt_resume); > + > +MODULE_DEVICE_TABLE(of, atlas7_wdt_ids); > + > +static struct platform_driver atlas7_wdt_driver = { > + .driver = { > + .name = "atlas7-wdt", > + .pm = &atlas7_wdt_pm_ops, > + .of_match_table = atlas7_wdt_ids, > + }, > + .probe = atlas7_wdt_probe, > + .remove = atlas7_wdt_remove, > + .shutdown = atlas7_wdt_shutdown, > +}; > +module_platform_driver(atlas7_wdt_driver); > + > +MODULE_DESCRIPTION("CSRatlas7 watchdog driver"); > +MODULE_AUTHOR("Guo Zeng "); > +MODULE_LICENSE("GPL v2"); This contradicts "GPL v2 or later" in the comments at the beginning of the file. > +MODULE_ALIAS("platform:atlas7-wdt"); >