2014-09-29 1:35 GMT+04:00 Guenter Roeck <linux@xxxxxxxxxxxx>: > On 09/28/2014 11:33 AM, Sergey Ryazanov wrote: >> >> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@xxxxxxxxx> >> Cc: Wim Van Sebroeck <wim@xxxxxxxxx> >> Cc: linux-watchdog@xxxxxxxxxxxxxxx >> --- >> >> Changes since RFC: >> - use watchdog infrastructure >> - remove deprecated IRQF_DISABLED flag >> - move device registration to separate patch >> >> drivers/watchdog/Kconfig | 8 ++ >> drivers/watchdog/Makefile | 1 + >> drivers/watchdog/ar2315-wtd.c | 167 >> ++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 176 insertions(+) >> create mode 100644 drivers/watchdog/ar2315-wtd.c >> >> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >> index f57312f..dbace99 100644 >> --- a/drivers/watchdog/Kconfig >> +++ b/drivers/watchdog/Kconfig >> @@ -1186,6 +1186,14 @@ config RALINK_WDT >> help >> Hardware driver for the Ralink SoC Watchdog Timer. >> >> +config AR2315_WDT >> + tristate "Atheros AR2315+ WiSoCs Watchdog Timer" >> + select WATCHDOG_CORE >> + depends on SOC_AR2315 >> + help >> + Hardware driver for the built-in watchdog timer on the Atheros >> + AR2315/AR2316 WiSoCs. >> + >> # PARISC Architecture >> >> # POWERPC Architecture >> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile >> index 468c320..ef7f83b 100644 >> --- a/drivers/watchdog/Makefile >> +++ b/drivers/watchdog/Makefile >> @@ -133,6 +133,7 @@ obj-$(CONFIG_WDT_MTX1) += mtx-1_wdt.o >> obj-$(CONFIG_PNX833X_WDT) += pnx833x_wdt.o >> obj-$(CONFIG_SIBYTE_WDOG) += sb_wdog.o >> obj-$(CONFIG_AR7_WDT) += ar7_wdt.o >> +obj-$(CONFIG_AR2315_WDT) += ar2315-wtd.o >> obj-$(CONFIG_TXX9_WDT) += txx9wdt.o >> obj-$(CONFIG_OCTEON_WDT) += octeon-wdt.o >> octeon-wdt-y := octeon-wdt-main.o octeon-wdt-nmi.o >> diff --git a/drivers/watchdog/ar2315-wtd.c b/drivers/watchdog/ar2315-wtd.c >> new file mode 100644 >> index 0000000..4fd34d2 >> --- /dev/null >> +++ b/drivers/watchdog/ar2315-wtd.c >> @@ -0,0 +1,167 @@ >> +/* >> + * 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. >> + * >> + * 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. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, see <http://www.gnu.org/licenses/>. >> + * >> + * Copyright (C) 2008 John Crispin <blogic@xxxxxxxxxxx> >> + * Based on EP93xx and ifxmips wdt driver >> + */ >> + >> +#include <linux/interrupt.h> >> +#include <linux/module.h> >> +#include <linux/watchdog.h> >> +#include <linux/reboot.h> >> +#include <linux/init.h> >> +#include <linux/platform_device.h> >> +#include <linux/io.h> >> + >> +#define DRIVER_NAME "ar2315-wdt" >> + >> +#define CLOCK_RATE 40000000 >> + >> +#define WDT_REG_TIMER 0x00 >> +#define WDT_REG_CTRL 0x04 >> + >> +#define WDT_CTRL_ACT_NONE 0x00000000 /* No action */ >> +#define WDT_CTRL_ACT_NMI 0x00000001 /* NMI on watchdog */ >> +#define WDT_CTRL_ACT_RESET 0x00000002 /* reset on watchdog */ >> + > > What are those defines for ? They don't seem to be used. > This defines for reference. There no documentation for this chips, so I left this defines as some kind of documentation. > If the watchdog can result in an immediate restart, as > this define suggests, why don't you use it but rely on > the interrupt handler instead ? > AFAIK some of chips have a HW bug in restarting unit, so chip specific restart routine (in arch code) use a lot of hacks to reset chip. So we use interrupt to call reset function, which should reliably reset chip. > This means the watchdog won't really fire if it times out, but depend > on the interrupt handler to work. Which it won't if there is a real > problem and interrupts are disabled (or if the system hangs entirely). > Sure. But without reset function call from the interrupt handler we can not reliable reset chip (see above). >> +static int started; >> +static void __iomem *wdt_base; >> + >> +static inline void ar2315_wdt_wr(unsigned reg, u32 val) >> +{ >> + iowrite32(val, wdt_base + reg); >> +} >> + >> +static void ar2315_wdt_enable(struct watchdog_device *wdd) >> +{ >> + ar2315_wdt_wr(WDT_REG_TIMER, wdd->timeout * CLOCK_RATE); >> +} >> + >> +static int ar2315_wdt_start(struct watchdog_device *wdd) >> +{ >> + ar2315_wdt_enable(wdd); >> + started = 1; > > > I don't really see why you would need this variable. > To protect against spurious interrupts, since the watchdog timer could be started by bootloader. > >> + return 0; >> +} >> + >> +static int ar2315_wdt_stop(struct watchdog_device *wdd) >> +{ >> + return 0; >> +} >> + >> +static int ar2315_wdt_ping(struct watchdog_device *wdd) >> +{ >> + ar2315_wdt_enable(wdd); >> + return 0; >> +} >> + >> +static int ar2315_wdt_set_timeout(struct watchdog_device *wdd, unsigned >> val) >> +{ >> + wdd->timeout = val; >> + return 0; >> +} >> + >> +static irqreturn_t ar2315_wdt_interrupt(int irq, void *dev) >> +{ >> + struct platform_device *pdev = (struct platform_device *)dev; >> + >> + if (started) { >> + dev_crit(&pdev->dev, "watchdog expired, rebooting >> system\n"); >> + emergency_restart(); >> + } else { >> + ar2315_wdt_wr(WDT_REG_CTRL, 0); >> + ar2315_wdt_wr(WDT_REG_TIMER, 0); >> + } > > > This is quite unusual. > Why not stop the watchdog in the stop function ? Quite apparently > it can be stopped, or at least this is what it looks like. > The started variable is set to true inside the watchdog start routine, but it never reset to false. This code only disable the watchdog when it was started by bootloader. > When do you expect this function to be called in the first place > with started == 1 ? > > If the idea is to stop the watchdog if it was already enabled > when probing the driver, why don't you stop it there ? > Sure, I will try to do that. >> + return IRQ_HANDLED; >> +} >> + >> +static const struct watchdog_info ar2315_wdt_info = { >> + .identity = "ar2315 Watchdog", >> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING, >> +}; >> + >> +static const struct watchdog_ops ar2315_wdt_ops = { >> + .owner = THIS_MODULE, >> + .start = ar2315_wdt_start, >> + .stop = ar2315_wdt_stop, >> + .ping = ar2315_wdt_ping, >> + .set_timeout = ar2315_wdt_set_timeout, >> +}; >> + >> +static struct watchdog_device ar2315_wdt_dev = { >> + .info = &ar2315_wdt_info, >> + .ops = &ar2315_wdt_ops, >> + .min_timeout = 1, >> + .max_timeout = 90, >> + .timeout = 20, >> +}; >> + >> +static int ar2315_wdt_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct resource *res; >> + int ret = 0; >> + >> + if (wdt_base) >> + return -EBUSY; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + wdt_base = devm_ioremap_resource(dev, res); >> + if (IS_ERR(wdt_base)) >> + return PTR_ERR(wdt_base); >> + >> + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); >> + if (!res) { >> + dev_err(dev, "no IRQ resource\n"); >> + return -ENOENT; >> + } >> + >> + ret = devm_request_irq(dev, res->start, ar2315_wdt_interrupt, 0, >> + DRIVER_NAME, pdev); >> + if (ret) { >> + dev_err(dev, "failed to register inetrrupt\n"); >> + return ret; >> + } >> + >> + ar2315_wdt_dev.parent = dev; >> + ret = watchdog_register_device(&ar2315_wdt_dev); >> + if (ret) { >> + dev_err(dev, "failed to register watchdog device\n"); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static int ar2315_wdt_remove(struct platform_device *pdev) >> +{ >> + watchdog_unregister_device(&ar2315_wdt_dev); > > > Why don't you stop the watchdog on remove ? > While the watchdog is running, the watchdog core prevents the module unloading, so this routine could not be called while the watchdog is running. Isn't it? > >> + return 0; >> +} >> + >> +static struct platform_driver ar2315_wdt_driver = { >> + .probe = ar2315_wdt_probe, >> + .remove = ar2315_wdt_remove, >> + .driver = { >> + .name = DRIVER_NAME, >> + .owner = THIS_MODULE, >> + }, >> +}; >> + >> +module_platform_driver(ar2315_wdt_driver); >> + >> +MODULE_DESCRIPTION("Atheros AR2315 hardware watchdog driver"); >> +MODULE_AUTHOR("John Crispin <blogic@xxxxxxxxxxx>"); >> +MODULE_LICENSE("GPL"); >> +MODULE_ALIAS("platform:" DRIVER_NAME); >> > -- BR, Sergey
