From mboxrd@z Thu Jan 1 00:00:00 1970 From: Guenter Roeck Subject: Re: [PATCH 2/2] watchdog: digicolor: driver for Conexant Digicolor CX92755 SoC Date: Sun, 22 Mar 2015 09:59:06 -0700 Message-ID: <550EF4DA.6040407@roeck-us.net> References: <6bc6e867b500cd3cfc629b96f90442f5c0aced3a.1427028036.git.baruch@tkos.co.il> <409ece75204d3911dc526a9bc0c908fc30893b26.1427028036.git.baruch@tkos.co.il> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <409ece75204d3911dc526a9bc0c908fc30893b26.1427028036.git.baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org> Sender: linux-watchdog-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Baruch Siach , Wim Van Sebroeck Cc: linux-watchdog-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: devicetree@vger.kernel.org On 03/22/2015 05:40 AM, Baruch Siach wrote: > This commit add a driver for the watchdog functionality of the Conexant CX92755 > SoC, from the Digicolor series of SoCs. Of 8 system timers provided by the > CX92755, the first one, timer A, can reset the chip when its counter reaches > zero. This driver uses this capability to provide userspace with a standard > watchdog, using the watchdog timer driver core framework. This driver also > implements a reboot handler for the reboot(2) system call. > > The watchdog driver shares the timer registers with the CX92755 timer driver > (drivers/clocksource/timer-digicolor.c). The timer driver, however, uses only > timers other than A, so both drivers should coexist. > > Signed-off-by: Baruch Siach Looks pretty good. Couple of comments below. Thanks, Guenter > --- > drivers/watchdog/Kconfig | 10 ++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/digicolor_wdt.c | 203 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 214 insertions(+) > create mode 100644 drivers/watchdog/digicolor_wdt.c > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 16f202350997..7d73d6c78cf6 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -515,6 +515,16 @@ config MEDIATEK_WATCHDOG > To compile this driver as a module, choose M here: the > module will be called mtk_wdt. > > +config DIGICOLOR_WATCHDOG > + tristate "Conexant Digicolor SoCs watchdog support" > + depends on ARCH_DIGICOLOR > + select WATCHDOG_CORE This doesn't (directly) depend on HAVE_CLK, meaning clk_get can return NULL if HAVE_CLK is not configured. Can that happen ? > + help > + Say Y here to include support for the watchdog timer > + in Conexant Digicolor SoCs. > + To compile this driver as a module, choose M here: the > + module will be called digicolor_wdt. > + > # AVR32 Architecture > > config AT32AP700X_WDT > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 5c19294d1c30..0721f10e8d13 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -64,6 +64,7 @@ obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o > obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o > obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o > obj-$(CONFIG_MEDIATEK_WATCHDOG) += mtk_wdt.o > +obj-$(CONFIG_DIGICOLOR_WATCHDOG) += digicolor_wdt.o > > # AVR32 Architecture > obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o > diff --git a/drivers/watchdog/digicolor_wdt.c b/drivers/watchdog/digicolor_wdt.c > new file mode 100644 > index 000000000000..260cc4495370 > --- /dev/null > +++ b/drivers/watchdog/digicolor_wdt.c > @@ -0,0 +1,203 @@ > +/* > + * Watchdog driver for Conexant Digicolor > + * > + * Copyright (C) 2015 Paradox Innovation Ltd. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; either version 2 of the License, or (at your > + * option) any later version. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define TIMER_A_CONTROL 0 > +#define TIMER_A_COUNT 4 > + > +#define TIMER_A_ENABLE_COUNT BIT(0) > +#define TIMER_A_ENABLE_WATCHDOG BIT(1) > + > +#define DC_WDT_DEFAULT_TIME 5 > + Five seconds is an extremely low default timeout. More common would be 30 or 60 seconds. Any special reason for selecting this default ? > +struct dc_wdt { > + void __iomem *base; > + struct clk *clk; > + struct notifier_block restart_handler; > + spinlock_t lock; > +}; > + > +static unsigned timeout; > +module_param(timeout, uint, 0); > +MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds"); > + > +static int dc_restart_handler(struct notifier_block *this, unsigned long mode, > + void *cmd) > +{ > + struct dc_wdt *wdt = container_of(this, struct dc_wdt, restart_handler); > + unsigned long flags; > + > + spin_lock_irqsave(&wdt->lock, flags); > + > + writel_relaxed(0, wdt->base + TIMER_A_CONTROL); > + writel_relaxed(1, wdt->base + TIMER_A_COUNT); > + writel_relaxed(TIMER_A_ENABLE_COUNT | TIMER_A_ENABLE_WATCHDOG, > + wdt->base + TIMER_A_CONTROL); > + > + spin_unlock_irqrestore(&wdt->lock, flags); > + > + /* wait for reset to assert... */ > + mdelay(500); > + > + return NOTIFY_DONE; > +} > + > +static int dc_wdt_start(struct watchdog_device *wdog) > +{ > + struct dc_wdt *wdt = watchdog_get_drvdata(wdog); > + unsigned long flags; > + > + spin_lock_irqsave(&wdt->lock, flags); > + > + writel_relaxed(0, wdt->base + TIMER_A_CONTROL); > + writel_relaxed(wdog->timeout * clk_get_rate(wdt->clk), > + wdt->base + TIMER_A_COUNT); > + writel_relaxed(TIMER_A_ENABLE_COUNT | TIMER_A_ENABLE_WATCHDOG, > + wdt->base + TIMER_A_CONTROL); > + > + spin_unlock_irqrestore(&wdt->lock, flags); > + You have this sequence twice, so a little helper function taking wdt and the timeout as parameters might be useful. > + return 0; > +} > + > +static int dc_wdt_stop(struct watchdog_device *wdog) > +{ > + struct dc_wdt *wdt = watchdog_get_drvdata(wdog); > + > + writel_relaxed(0, wdt->base + TIMER_A_CONTROL); > + > + return 0; > +} > + > +static int dc_wdt_set_timeout(struct watchdog_device *wdog, unsigned int t) > +{ > + wdog->timeout = t; > + No updating the timer count register ? If you increase the timeout significantly, the next ping may come too late. > + return 0; > +} > + > +static unsigned int dc_wdt_get_timeleft(struct watchdog_device *wdog) > +{ > + struct dc_wdt *wdt = watchdog_get_drvdata(wdog); > + uint32_t count = readl_relaxed(wdt->base + TIMER_A_COUNT); > + > + return (count / clk_get_rate(wdt->clk)); > +} > + > +static struct watchdog_ops dc_wdt_ops = { > + .owner = THIS_MODULE, > + .start = dc_wdt_start, > + .stop = dc_wdt_stop, Since you don't have a ping function, each ping will stop the watchdog, update the timer, and restart it. Is this intentional ? > + .set_timeout = dc_wdt_set_timeout, > + .get_timeleft = dc_wdt_get_timeleft, > +}; > + > +static struct watchdog_info dc_wdt_info = { > + .options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE > + | WDIOF_KEEPALIVEPING, > + .identity = "Conexant Digicolor Watchdog", > +}; > + > +static struct watchdog_device dc_wdt_wdd = { > + .info = &dc_wdt_info, > + .ops = &dc_wdt_ops, > + .min_timeout = 1, > + .timeout = DC_WDT_DEFAULT_TIME, > +}; > + > +static int dc_wdt_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + struct dc_wdt *wdt; > + int ret; > + > + wdt = devm_kzalloc(dev, sizeof(struct dc_wdt), GFP_KERNEL); > + if (!wdt) > + return -ENOMEM; > + platform_set_drvdata(pdev, wdt); > + > + wdt->base = of_iomap(np, 0); > + if (!wdt->base) { > + dev_err(dev, "Failed to remap watchdog regs"); > + return -ENODEV; > + } > + > + wdt->clk = clk_get(&pdev->dev, NULL); > + if (IS_ERR(wdt->clk)) > + return PTR_ERR(wdt->clk); iounmap missing. > + dc_wdt_wdd.max_timeout = U32_MAX / clk_get_rate(wdt->clk); > + > + spin_lock_init(&wdt->lock); > + > + watchdog_set_drvdata(&dc_wdt_wdd, wdt); > + watchdog_init_timeout(&dc_wdt_wdd, timeout, dev); > + ret = watchdog_register_device(&dc_wdt_wdd); > + if (ret) { > + dev_err(dev, "Failed to register watchdog device"); > + iounmap(wdt->base); I don't see clk_put in any of the error cases. How about using devm_clk_get above ? > + return ret; > + } > + > + wdt->restart_handler.notifier_call = dc_restart_handler; > + wdt->restart_handler.priority = 128; Is 128 intentional, ie is this the expected reset method for this platform ? If not, it may be better to select a lower priority (eg 64). Using a watchdog to reset a system should in general be a method of last resort. > + ret = register_restart_handler(&wdt->restart_handler); > + if (ret) > + dev_err(&pdev->dev, "cannot register restart handler\n"); dev_warn would be better here, since this is non-fatal. > + > + return 0; > +} > + > +static int dc_wdt_remove(struct platform_device *pdev) > +{ > + struct dc_wdt *wdt = platform_get_drvdata(pdev); > + > + unregister_restart_handler(&wdt->restart_handler); > + watchdog_unregister_device(&dc_wdt_wdd); > + iounmap(wdt->base); clk_put missing (or use devm_clk_get). > + > + return 0; > +} > + > +static void dc_wdt_shutdown(struct platform_device *pdev) > +{ > + dc_wdt_stop(&dc_wdt_wdd); > +} > + > +static const struct of_device_id dc_wdt_of_match[] = { > + { .compatible = "cnxt,cx92755-wdt", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, dc_wdt_of_match); > + > +static struct platform_driver dc_wdt_driver = { > + .probe = dc_wdt_probe, > + .remove = dc_wdt_remove, > + .shutdown = dc_wdt_shutdown, > + .driver = { > + .name = "digicolor-wdt", > + .of_match_table = dc_wdt_of_match, > + }, > +}; > +module_platform_driver(dc_wdt_driver); > + > +MODULE_AUTHOR("Baruch Siach "); > +MODULE_DESCRIPTION("Driver for Conexant Digicolor watchdog timer"); > +MODULE_LICENSE("GPL"); > -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html