* [PATCH 12/16] watchdog: add Atheros AR2315 watchdog driver
[not found] <1411929195-23775-1-git-send-email-ryazanov.s.a@gmail.com>
@ 2014-09-28 18:33 ` Sergey Ryazanov
2014-09-28 21:35 ` Guenter Roeck
0 siblings, 1 reply; 5+ messages in thread
From: Sergey Ryazanov @ 2014-09-28 18:33 UTC (permalink / raw)
To: Ralf Baechle; +Cc: Linux MIPS, Wim Van Sebroeck, linux-watchdog
Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
Cc: Wim Van Sebroeck <wim@iguana.be>
Cc: linux-watchdog@vger.kernel.org
---
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@openwrt.org>
+ * 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 */
+
+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;
+ 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);
+ }
+ 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);
+ 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@openwrt.org>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" DRIVER_NAME);
--
1.8.5.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 12/16] watchdog: add Atheros AR2315 watchdog driver
2014-09-28 18:33 ` [PATCH 12/16] watchdog: add Atheros AR2315 watchdog driver Sergey Ryazanov
@ 2014-09-28 21:35 ` Guenter Roeck
2014-09-29 20:14 ` Sergey Ryazanov
0 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2014-09-28 21:35 UTC (permalink / raw)
To: Sergey Ryazanov, Ralf Baechle
Cc: Linux MIPS, Wim Van Sebroeck, linux-watchdog
On 09/28/2014 11:33 AM, Sergey Ryazanov wrote:
> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Cc: linux-watchdog@vger.kernel.org
> ---
>
> 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@openwrt.org>
> + * 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.
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 ?
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).
> +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.
> + 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.
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 ?
> + 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 ?
> + 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@openwrt.org>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRIVER_NAME);
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 12/16] watchdog: add Atheros AR2315 watchdog driver
2014-09-28 21:35 ` Guenter Roeck
@ 2014-09-29 20:14 ` Sergey Ryazanov
2014-09-29 20:46 ` Guenter Roeck
0 siblings, 1 reply; 5+ messages in thread
From: Sergey Ryazanov @ 2014-09-29 20:14 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Ralf Baechle, Linux MIPS, Wim Van Sebroeck, linux-watchdog
2014-09-29 1:35 GMT+04:00 Guenter Roeck <linux@roeck-us.net>:
> On 09/28/2014 11:33 AM, Sergey Ryazanov wrote:
>>
>> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
>> Cc: Wim Van Sebroeck <wim@iguana.be>
>> Cc: linux-watchdog@vger.kernel.org
>> ---
>>
>> 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@openwrt.org>
>> + * 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@openwrt.org>");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:" DRIVER_NAME);
>>
>
--
BR,
Sergey
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 12/16] watchdog: add Atheros AR2315 watchdog driver
2014-09-29 20:14 ` Sergey Ryazanov
@ 2014-09-29 20:46 ` Guenter Roeck
2014-09-29 21:01 ` Sergey Ryazanov
0 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2014-09-29 20:46 UTC (permalink / raw)
To: Sergey Ryazanov
Cc: Ralf Baechle, Linux MIPS, Wim Van Sebroeck, linux-watchdog
On Tue, Sep 30, 2014 at 12:14:58AM +0400, Sergey Ryazanov wrote:
> 2014-09-29 1:35 GMT+04:00 Guenter Roeck <linux@roeck-us.net>:
> > On 09/28/2014 11:33 AM, Sergey Ryazanov wrote:
> >>
> >> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> >> Cc: Wim Van Sebroeck <wim@iguana.be>
> >> Cc: linux-watchdog@vger.kernel.org
> >> ---
> >>
> >> 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@openwrt.org>
> >> + * 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 they are not used, please drop those defines. They only create unnecessary
confusion if unused.
> > 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.
>
Then it would be appropriate to stop it in the probe function.
> >
> >> + 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?
>
Only you never stop the watchdog nor disable the chip interrupt,
even on close (the stop function above does nothing).
Guenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 12/16] watchdog: add Atheros AR2315 watchdog driver
2014-09-29 20:46 ` Guenter Roeck
@ 2014-09-29 21:01 ` Sergey Ryazanov
0 siblings, 0 replies; 5+ messages in thread
From: Sergey Ryazanov @ 2014-09-29 21:01 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Ralf Baechle, Linux MIPS, Wim Van Sebroeck, linux-watchdog
2014-09-30 0:46 GMT+04:00 Guenter Roeck <linux@roeck-us.net>:
> On Tue, Sep 30, 2014 at 12:14:58AM +0400, Sergey Ryazanov wrote:
>> 2014-09-29 1:35 GMT+04:00 Guenter Roeck <linux@roeck-us.net>:
>> > On 09/28/2014 11:33 AM, Sergey Ryazanov wrote:
>> >>
>> >> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
>> >> Cc: Wim Van Sebroeck <wim@iguana.be>
>> >> Cc: linux-watchdog@vger.kernel.org
>> >> ---
>> >>
>> >> 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@openwrt.org>
>> >> + * 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 they are not used, please drop those defines. They only create unnecessary
> confusion if unused.
>
Ok.
>> > 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.
>>
> Then it would be appropriate to stop it in the probe function.
>
Yes sure.
>> >
>> >> + 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?
>>
> Only you never stop the watchdog nor disable the chip interrupt,
> even on close (the stop function above does nothing).
>
Oops. I really forget to disable interrupt. Thank you!
--
BR,
Sergey
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-09-29 21:01 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1411929195-23775-1-git-send-email-ryazanov.s.a@gmail.com>
2014-09-28 18:33 ` [PATCH 12/16] watchdog: add Atheros AR2315 watchdog driver Sergey Ryazanov
2014-09-28 21:35 ` Guenter Roeck
2014-09-29 20:14 ` Sergey Ryazanov
2014-09-29 20:46 ` Guenter Roeck
2014-09-29 21:01 ` Sergey Ryazanov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox