Hello,
This patch series is a porposal to enhance the sam9 watchdog timer support.
The at91sam9 watchdog timer can only be configured once, and the current
implementation tries to configure it in a static way:
- 2 seconds timeout
- wdt restart every 500ms
If the timer has already been configured with different values, it returns an
error and do not create any watchdog device.
This is not critical if the watchdog is disabled, but if it has been enabled
with different timeout values it will lead to a SoC reset.
This patch series tries to address this issue by adapting the heartbeat value
according the WDT timer config:
- it first tries to configure the timer as requested.
- if it fails it fallbacks to the current config, adapting its heartbeat timer
to the needs
This patch series also move to a dynamically allocated at91wdt device instead
of the static instance. I'm not sure this is the best solution, so please tell
me if you prefer to keep static instance of watchdog.
It adds a new at91 wdt type: software. This new type make use of the at91 wdt
interrupt to trigger a software reboot.
Finally it adds several properties to the device tree bindings.
Best Regards,
Boris
Change since v1:
- fix typo in documentaion
- fix irq dt definition for sama5d3 SoC
Boris BREZILLON (4):
watchdog: at91sam9_wdt: better watchdog support
watchdog: at91sam9_wdt: update device tree doc
ARM: at91/dt: add sam9 watchdog default options to SoCs
ARM: at91/dt: add watchdog properties to kizbox board
.../devicetree/bindings/watchdog/atmel-wdt.txt | 30 +-
arch/arm/boot/dts/at91sam9260.dtsi | 5 +
arch/arm/boot/dts/at91sam9263.dtsi | 5 +
arch/arm/boot/dts/at91sam9g45.dtsi | 5 +
arch/arm/boot/dts/at91sam9n12.dtsi | 5 +
arch/arm/boot/dts/at91sam9x5.dtsi | 5 +
arch/arm/boot/dts/kizbox.dts | 6 +
arch/arm/boot/dts/sama5d3.dtsi | 5 +
drivers/watchdog/at91sam9_wdt.c | 319 +++++++++++++++-----
9 files changed, 300 insertions(+), 85 deletions(-)
--
1.7.9.5
The at91sam9 watchdog timer can only be configured once, and the current
implementation tries to configure it in a static way:
- 2 seconds timeout
- wdt restart every 500ms
If the timer has already been configured with different values, it returns an
error and do not create any watchdog device.
This is not critical if the watchdog is disabled, but if it has been enabled with
different timeout values it will lead to a SoC reset.
This patch series tries to address this issue by adapting the heartbeat value
according the WDT timer config:
- it first tries to configure the timer as requested.
- if it fails it fallbacks to the current config, adapting its heartbeat timer
to the needs
This patch series also move to a dynamically allocated at91wdt device instead
of the static instance.
It adds a new at91 wdt type: software. This new type make use of the at91 wdt
interrupt to trigger a software reboot.
Finally it adds several properties to the device tree bindings.
Signed-off-by: Boris BREZILLON <[email protected]>
---
drivers/watchdog/at91sam9_wdt.c | 319 +++++++++++++++++++++++++++++----------
1 file changed, 236 insertions(+), 83 deletions(-)
diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index be37dde..fb47ec5 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -19,11 +19,13 @@
#include <linux/errno.h>
#include <linux/init.h>
+#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/moduleparam.h>
#include <linux/platform_device.h>
+#include <linux/reboot.h>
#include <linux/types.h>
#include <linux/watchdog.h>
#include <linux/jiffies.h>
@@ -31,23 +33,32 @@
#include <linux/bitops.h>
#include <linux/uaccess.h>
#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
#include "at91sam9_wdt.h"
#define DRV_NAME "AT91SAM9 Watchdog"
-#define wdt_read(field) \
- __raw_readl(at91wdt_private.base + field)
-#define wdt_write(field, val) \
- __raw_writel((val), at91wdt_private.base + field)
+#define wdt_read(wdt, field) \
+ __raw_readl((wdt)->base + field)
+#define wdt_write(wtd, field, val) \
+ __raw_writel((val), (wdt)->base + field)
/* AT91SAM9 watchdog runs a 12bit counter @ 256Hz,
* use this to convert a watchdog
* value from/to milliseconds.
*/
+#define hz_to_ticks(h) (((h << 8) / HZ) - 1)
+#define ticks_to_hz(t) (((t + 1) * HZ) >> 8)
#define ms_to_ticks(t) (((t << 8) / 1000) - 1)
#define ticks_to_ms(t) (((t + 1) * 1000) >> 8)
+#define WDT_MR_RESET 0x3FFF2FFF
+
+#define WDT_WDD_MAX 0xFFF
+#define WDT_WDV_MAX 0xFFF
+
/* Hardware timeout in seconds */
#define WDT_HW_TIMEOUT 2
@@ -66,23 +77,41 @@ module_param(nowayout, bool, 0);
MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
"(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
-static struct watchdog_device at91_wdt_dev;
-static void at91_ping(unsigned long data);
-
-static struct {
+#define to_wdt(wdd) container_of(wdd, struct at91wdt, wdd)
+struct at91wdt {
+ struct watchdog_device wdd;
void __iomem *base;
unsigned long next_heartbeat; /* the next_heartbeat for the timer */
struct timer_list timer; /* The timer that pings the watchdog */
-} at91wdt_private;
+ u32 mr;
+ u32 mr_mask;
+ unsigned long heartbeat; /* WDT heartbeat in jiffies */
+ bool nowayout;
+ unsigned int irq;
+};
/* ......................................................................... */
+static irqreturn_t wdt_interrupt(int irq, void *dev_id)
+{
+ struct at91wdt *wdt = (struct at91wdt *)dev_id;
+
+ if (wdt_read(wdt, AT91_WDT_SR)) {
+ /* Reboot */
+ pr_crit("at91sam9 WDT software reset\n");
+ emergency_restart();
+ pr_crit("Reboot didn't ?????\n");
+ }
+
+ return IRQ_HANDLED;
+}
+
/*
* Reload the watchdog timer. (ie, pat the watchdog)
*/
-static inline void at91_wdt_reset(void)
+static inline void at91_wdt_reset(struct at91wdt *wdt)
{
- wdt_write(AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
+ wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
}
/*
@@ -90,26 +119,20 @@ static inline void at91_wdt_reset(void)
*/
static void at91_ping(unsigned long data)
{
- if (time_before(jiffies, at91wdt_private.next_heartbeat) ||
- (!watchdog_active(&at91_wdt_dev))) {
- at91_wdt_reset();
- mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
+ struct at91wdt *wdt = (struct at91wdt *)data;
+ if (time_before(jiffies, wdt->next_heartbeat) ||
+ (!watchdog_active(&wdt->wdd))) {
+ at91_wdt_reset(wdt);
+ mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
} else
pr_crit("I will reset your machine !\n");
}
static int at91_wdt_ping(struct watchdog_device *wdd)
{
+ struct at91wdt *wdt = to_wdt(wdd);
/* calculate when the next userspace timeout will be */
- at91wdt_private.next_heartbeat = jiffies + wdd->timeout * HZ;
- return 0;
-}
-
-static int at91_wdt_start(struct watchdog_device *wdd)
-{
- /* calculate the next userspace timeout and modify the timer */
- at91_wdt_ping(wdd);
- mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
+ wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
return 0;
}
@@ -125,36 +148,82 @@ static int at91_wdt_set_timeout(struct watchdog_device *wdd, unsigned int new_ti
return 0;
}
-/*
- * Set the watchdog time interval in 1/256Hz (write-once)
- * Counter is 12 bit.
- */
-static int at91_wdt_settimeout(unsigned int timeout)
+static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
{
- unsigned int reg;
- unsigned int mr;
-
- /* Check if disabled */
- mr = wdt_read(AT91_WDT_MR);
- if (mr & AT91_WDT_WDDIS) {
- pr_err("sorry, watchdog is disabled\n");
- return -EIO;
+ u32 tmp;
+ u32 delta;
+ u32 value;
+ int err;
+ u32 mask = wdt->mr_mask;
+
+ tmp = wdt_read(wdt, AT91_WDT_MR);
+ if ((tmp & mask) != (wdt->mr & mask)) {
+ if (tmp == WDT_MR_RESET) {
+ wdt_write(wdt, AT91_WDT_MR, wdt->mr);
+ tmp = wdt_read(wdt, AT91_WDT_MR);
+ }
+ }
+
+ if (tmp & AT91_WDT_WDDIS) {
+ if (wdt->mr & AT91_WDT_WDDIS)
+ return 0;
+ dev_err(wdt->wdd.parent,
+ ": sorry, watchdog is disabled\n");
+ return -EINVAL;
+ }
+
+ value = tmp & AT91_WDT_WDV;
+ delta = (tmp & AT91_WDT_WDD) >> 16;
+
+ if (delta < value) {
+ if ((value / (value - delta)) < 4) {
+ dev_err(wdt->wdd.parent,
+ ": sorry, max-heartbeat should be at least 4 times min-heartbeat (max-heartbeat = %d x min-heartbeat)\n",
+ (value / (value - delta)));
+ return -EINVAL;
+ }
+ }
+
+ wdt->heartbeat = ticks_to_hz(value / 4);
+
+ if (!wdt->heartbeat) {
+ dev_err(wdt->wdd.parent,
+ ": sorry, linux timer (%i Hz) cannot handle watchdog timeout (%i ms)\n",
+ HZ, ticks_to_ms(value));
+ return -EINVAL;
+ }
+
+ if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
+ err = request_irq(wdt->irq, wdt_interrupt,
+ IRQF_SHARED | IRQF_IRQPOLL,
+ pdev->name, wdt);
+ if (err)
+ return err;
}
- /*
- * All counting occurs at SLOW_CLOCK / 128 = 256 Hz
- *
- * Since WDV is a 12-bit counter, the maximum period is
- * 4096 / 256 = 16 seconds.
- */
- reg = AT91_WDT_WDRSTEN /* causes watchdog reset */
- /* | AT91_WDT_WDRPROC causes processor reset only */
- | AT91_WDT_WDDBGHLT /* disabled in debug mode */
- | AT91_WDT_WDD /* restart at any time */
- | (timeout & AT91_WDT_WDV); /* timer value */
- wdt_write(AT91_WDT_MR, reg);
+ if ((tmp & wdt->mr_mask) != (wdt->mr & wdt->mr_mask))
+ dev_warn(wdt->wdd.parent,
+ ": watchdog already configured differently (mr = %x expecting %x)\n",
+ tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
+
+ setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
+ mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
+
+ /* Try to set timeout from device tree first */
+ if (watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev))
+ watchdog_init_timeout(&wdt->wdd, heartbeat, &pdev->dev);
+ watchdog_set_nowayout(&wdt->wdd, wdt->nowayout);
+ err = watchdog_register_device(&wdt->wdd);
+ if (err)
+ goto out_stop_timer;
+
+ wdt->next_heartbeat = jiffies + wdt->wdd.timeout * HZ;
return 0;
+
+out_stop_timer:
+ del_timer(&wdt->timer);
+ return err;
}
/* ......................................................................... */
@@ -167,63 +236,147 @@ static const struct watchdog_info at91_wdt_info = {
static const struct watchdog_ops at91_wdt_ops = {
.owner = THIS_MODULE,
- .start = at91_wdt_start,
+ .start = at91_wdt_ping,
.stop = at91_wdt_stop,
.ping = at91_wdt_ping,
.set_timeout = at91_wdt_set_timeout,
};
-static struct watchdog_device at91_wdt_dev = {
- .info = &at91_wdt_info,
- .ops = &at91_wdt_ops,
- .timeout = WDT_HEARTBEAT,
- .min_timeout = 1,
- .max_timeout = 0xFFFF,
-};
+#if defined(CONFIG_OF)
+static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
+{
+ u32 min = 0;
+ u32 max = 0xFFF;
+ const char *tmp;
+
+ wdt->base = of_iomap(np, 0);
+ if (!wdt->base) {
+ dev_err(wdt->wdd.parent, "failed to map registers, aborting.\n");
+ return -EINVAL;
+ }
+
+ /* Get the interrupts property */
+ wdt->irq = irq_of_parse_and_map(np, 0);
+ if (!wdt->irq)
+ dev_warn(wdt->wdd.parent, "failed to get IRQ from DT.\n");
+
+ if (!of_property_read_u32_index(np, "atmel,max-heartbeat-sec", 0,
+ &max)) {
+ max *= 256;
+ if (max > WDT_WDV_MAX)
+ max = WDT_WDV_MAX;
+
+ if (!of_property_read_u32_index(np, "atmel,min-heartbeat-sec",
+ 0, &min)) {
+ min *= 256;
+ if (4 * min > max)
+ min = max / 4;
+ }
+ }
+
+ wdt->mr_mask = 0x3FFFFFFF;
+ wdt->mr = 0;
+ if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
+ !strcmp(tmp, "software")) {
+ wdt->mr |= AT91_WDT_WDFIEN;
+ wdt->mr_mask &= ~AT91_WDT_WDRPROC;
+ } else
+ wdt->mr |= AT91_WDT_WDRSTEN;
+
+ if (!of_property_read_string(np, "atmel,reset-type", &tmp) &&
+ !strcmp(tmp, "proc"))
+ wdt->mr |= AT91_WDT_WDRPROC;
+
+ if (of_property_read_bool(np, "atmel,disable")) {
+ wdt->mr |= AT91_WDT_WDDIS;
+ wdt->mr_mask &= AT91_WDT_WDDIS;
+ }
+
+ if (of_property_read_bool(np, "atmel,idle-halt"))
+ wdt->mr |= AT91_WDT_WDIDLEHLT;
+
+ if (of_property_read_bool(np, "atmel,dbg-halt"))
+ wdt->mr |= AT91_WDT_WDDBGHLT;
+
+ wdt->mr |= max | ((max - min) << 16);
+
+ return 0;
+}
+#else
+static inline int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
+{
+ return 0;
+}
+#endif
static int __init at91wdt_probe(struct platform_device *pdev)
{
struct resource *r;
- int res;
-
- r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!r)
- return -ENODEV;
- at91wdt_private.base = ioremap(r->start, resource_size(r));
- if (!at91wdt_private.base) {
- dev_err(&pdev->dev, "failed to map registers, aborting.\n");
- return -ENOMEM;
- }
+ int err;
+ struct at91wdt *wdt;
- at91_wdt_dev.parent = &pdev->dev;
- watchdog_init_timeout(&at91_wdt_dev, heartbeat, &pdev->dev);
- watchdog_set_nowayout(&at91_wdt_dev, nowayout);
+ wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+ if (!wdt)
+ return -ENOMEM;
- /* Set watchdog */
- res = at91_wdt_settimeout(ms_to_ticks(WDT_HW_TIMEOUT * 1000));
- if (res)
- return res;
+ wdt->mr = (WDT_HW_TIMEOUT * 256) | AT91_WDT_WDRSTEN | AT91_WDT_WDD |
+ AT91_WDT_WDDBGHLT | AT91_WDT_WDIDLEHLT;
+ wdt->mr_mask = 0x3FFFFFFF;
+ wdt->nowayout = nowayout;
+ wdt->wdd.parent = &pdev->dev;
+ wdt->wdd.info = &at91_wdt_info;
+ wdt->wdd.ops = &at91_wdt_ops;
+ wdt->wdd.timeout = WDT_HEARTBEAT;
+ wdt->wdd.min_timeout = 1;
+ wdt->wdd.max_timeout = 0xFFFF;
+
+ if (pdev->dev.of_node) {
+ err = of_at91wdt_init(pdev->dev.of_node, wdt);
+ if (err)
+ goto out_free_watchdog;
+ } else {
+ r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!r) {
+ err = -ENODEV;
+ goto out_free_watchdog;
+ }
+
+ wdt->base = ioremap(r->start, resource_size(r));
+ if (!wdt->base) {
+ dev_err(&pdev->dev, "failed to map registers, aborting.\n");
+ err = -ENOMEM;
+ goto out_free_watchdog;
+ }
+ }
- res = watchdog_register_device(&at91_wdt_dev);
- if (res)
- return res;
+ err = at91_wdt_init(pdev, wdt);
+ if (err)
+ goto out_iounmap;
- at91wdt_private.next_heartbeat = jiffies + at91_wdt_dev.timeout * HZ;
- setup_timer(&at91wdt_private.timer, at91_ping, 0);
- mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
+ platform_set_drvdata(pdev, wdt);
pr_info("enabled (heartbeat=%d sec, nowayout=%d)\n",
- at91_wdt_dev.timeout, nowayout);
+ wdt->wdd.timeout, wdt->nowayout);
return 0;
+
+out_iounmap:
+ iounmap(wdt->base);
+out_free_watchdog:
+ devm_kfree(&pdev->dev, wdt);
+ return err;
}
static int __exit at91wdt_remove(struct platform_device *pdev)
{
- watchdog_unregister_device(&at91_wdt_dev);
+ struct at91wdt *wdt = platform_get_drvdata(pdev);
+ watchdog_unregister_device(&wdt->wdd);
pr_warn("I quit now, hardware will probably reboot!\n");
- del_timer(&at91wdt_private.timer);
+ del_timer(&wdt->timer);
+
+ iounmap(wdt->base);
+ devm_kfree(&pdev->dev, wdt);
return 0;
}
--
1.7.9.5
Add new at91sam9 watchdog properties to the documentation.
Signed-off-by: Boris BREZILLON <[email protected]>
---
.../devicetree/bindings/watchdog/atmel-wdt.txt | 30 ++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
index fcdd48f..e043106 100644
--- a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
@@ -9,11 +9,37 @@ Required properties:
Optional properties:
- timeout-sec: contains the watchdog timeout in seconds.
+- interrupts : Should contain WDT interrupt.
+- atmel,max-heartbeat-sec : Should contain the maximum heartbeat value in
+ seconds. This value should be less than 16. It is used to compute the
+ WDV field.
+- atmel,min-heartbeat-sec : Should contain the minimum heartbeat value in
+ seconds. This value should be less than 4 times the max-heartbeat-sec
+ value. It is used to compute the WDD field.
+- atmel,watchdog-type : Should be "hardware" or "software". Hardware watchdog
+ use the at91 watchdog reset. Software watchdog use the watchdog
+ interrupt to trigger a software reset.
+- atmel,reset-type : Should be "proc" or "all".
+ "all" : assert peripherals and processor reset signals
+ "proc" : assert the processor reset signal
+ This is valid only when using "hardware" watchdog.
+- atmel,disable : Should be present if you want to disable the watchdog.
+- atmel,idle-halt : Should be present if you want to stop the watchdog when
+ entering idle state.
+- atmel,dbg-halt : Should be present if you want to stop the watchdog when
+ entering debug state.
Example:
-
watchdog@fffffd40 {
compatible = "atmel,at91sam9260-wdt";
reg = <0xfffffd40 0x10>;
- timeout-sec = <10>;
+ interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+ timeout-sec = <15>;
+ atmel,watchdog-type = "hardware";
+ atmel,reset-type = "all";
+ atmel,dbg-halt;
+ atmel,idle-halt;
+ atmel,max-heartbeat-sec = <16>;
+ atmel,min-heartbeat-sec = <0>;
+ status = "okay";
};
--
1.7.9.5
Set default watchdog options in every SoC compatible with the sam9 watchdog.
Signed-off-by: Boris BREZILLON <[email protected]>
---
arch/arm/boot/dts/at91sam9260.dtsi | 5 +++++
arch/arm/boot/dts/at91sam9263.dtsi | 5 +++++
arch/arm/boot/dts/at91sam9g45.dtsi | 5 +++++
arch/arm/boot/dts/at91sam9n12.dtsi | 5 +++++
arch/arm/boot/dts/at91sam9x5.dtsi | 5 +++++
arch/arm/boot/dts/sama5d3.dtsi | 5 +++++
6 files changed, 30 insertions(+)
diff --git a/arch/arm/boot/dts/at91sam9260.dtsi b/arch/arm/boot/dts/at91sam9260.dtsi
index 899a7cf..6c44c35 100644
--- a/arch/arm/boot/dts/at91sam9260.dtsi
+++ b/arch/arm/boot/dts/at91sam9260.dtsi
@@ -643,6 +643,11 @@
watchdog@fffffd40 {
compatible = "atmel,at91sam9260-wdt";
reg = <0xfffffd40 0x10>;
+ interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+ atmel,watchdog-type = "hardware";
+ atmel,reset-type = "all";
+ atmel,dbg-halt;
+ atmel,idle-halt;
status = "disabled";
};
};
diff --git a/arch/arm/boot/dts/at91sam9263.dtsi b/arch/arm/boot/dts/at91sam9263.dtsi
index ff63811..6b31ee0 100644
--- a/arch/arm/boot/dts/at91sam9263.dtsi
+++ b/arch/arm/boot/dts/at91sam9263.dtsi
@@ -519,6 +519,11 @@
watchdog@fffffd40 {
compatible = "atmel,at91sam9260-wdt";
reg = <0xfffffd40 0x10>;
+ interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+ atmel,watchdog-type = "hardware";
+ atmel,reset-type = "all";
+ atmel,dbg-halt;
+ atmel,idle-halt;
status = "disabled";
};
diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi
index faec17d..ca1a643 100644
--- a/arch/arm/boot/dts/at91sam9g45.dtsi
+++ b/arch/arm/boot/dts/at91sam9g45.dtsi
@@ -634,6 +634,11 @@
watchdog@fffffd40 {
compatible = "atmel,at91sam9260-wdt";
reg = <0xfffffd40 0x10>;
+ interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+ atmel,watchdog-type = "hardware";
+ atmel,reset-type = "all";
+ atmel,dbg-halt;
+ atmel,idle-halt;
status = "disabled";
};
diff --git a/arch/arm/boot/dts/at91sam9n12.dtsi b/arch/arm/boot/dts/at91sam9n12.dtsi
index 3166e1d..7c8a5ea 100644
--- a/arch/arm/boot/dts/at91sam9n12.dtsi
+++ b/arch/arm/boot/dts/at91sam9n12.dtsi
@@ -505,6 +505,11 @@
watchdog@fffffe40 {
compatible = "atmel,at91sam9260-wdt";
reg = <0xfffffe40 0x10>;
+ interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+ atmel,watchdog-type = "hardware";
+ atmel,reset-type = "all";
+ atmel,dbg-halt;
+ atmel,idle-halt;
status = "disabled";
};
};
diff --git a/arch/arm/boot/dts/at91sam9x5.dtsi b/arch/arm/boot/dts/at91sam9x5.dtsi
index e77106e..48eb1cc 100644
--- a/arch/arm/boot/dts/at91sam9x5.dtsi
+++ b/arch/arm/boot/dts/at91sam9x5.dtsi
@@ -742,6 +742,11 @@
watchdog@fffffe40 {
compatible = "atmel,at91sam9260-wdt";
reg = <0xfffffe40 0x10>;
+ interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+ atmel,watchdog-type = "hardware";
+ atmel,reset-type = "all";
+ atmel,dbg-halt;
+ atmel,idle-halt;
status = "disabled";
};
diff --git a/arch/arm/boot/dts/sama5d3.dtsi b/arch/arm/boot/dts/sama5d3.dtsi
index c00e158..7974723 100644
--- a/arch/arm/boot/dts/sama5d3.dtsi
+++ b/arch/arm/boot/dts/sama5d3.dtsi
@@ -885,6 +885,11 @@
watchdog@fffffe40 {
compatible = "atmel,at91sam9260-wdt";
reg = <0xfffffe40 0x10>;
+ interrupts = <4 IRQ_TYPE_LEVEL_HIGH 7>;
+ atmel,watchdog-type = "hardware";
+ atmel,reset-type = "all";
+ atmel,dbg-halt;
+ atmel,idle-halt;
status = "disabled";
};
--
1.7.9.5
Set default watchdog options in every SoC compatible with the sam9 watchdog.
Signed-off-by: Boris BREZILLON <[email protected]>
---
arch/arm/boot/dts/at91sam9260.dtsi | 5 +++++
arch/arm/boot/dts/at91sam9263.dtsi | 5 +++++
arch/arm/boot/dts/at91sam9g45.dtsi | 5 +++++
arch/arm/boot/dts/at91sam9n12.dtsi | 5 +++++
arch/arm/boot/dts/at91sam9x5.dtsi | 5 +++++
arch/arm/boot/dts/sama5d3.dtsi | 5 +++++
6 files changed, 30 insertions(+)
diff --git a/arch/arm/boot/dts/at91sam9260.dtsi b/arch/arm/boot/dts/at91sam9260.dtsi
index 899a7cf..6c44c35 100644
--- a/arch/arm/boot/dts/at91sam9260.dtsi
+++ b/arch/arm/boot/dts/at91sam9260.dtsi
@@ -643,6 +643,11 @@
watchdog@fffffd40 {
compatible = "atmel,at91sam9260-wdt";
reg = <0xfffffd40 0x10>;
+ interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+ atmel,watchdog-type = "hardware";
+ atmel,reset-type = "all";
+ atmel,dbg-halt;
+ atmel,idle-halt;
status = "disabled";
};
};
diff --git a/arch/arm/boot/dts/at91sam9263.dtsi b/arch/arm/boot/dts/at91sam9263.dtsi
index ff63811..6b31ee0 100644
--- a/arch/arm/boot/dts/at91sam9263.dtsi
+++ b/arch/arm/boot/dts/at91sam9263.dtsi
@@ -519,6 +519,11 @@
watchdog@fffffd40 {
compatible = "atmel,at91sam9260-wdt";
reg = <0xfffffd40 0x10>;
+ interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+ atmel,watchdog-type = "hardware";
+ atmel,reset-type = "all";
+ atmel,dbg-halt;
+ atmel,idle-halt;
status = "disabled";
};
diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi
index faec17d..ca1a643 100644
--- a/arch/arm/boot/dts/at91sam9g45.dtsi
+++ b/arch/arm/boot/dts/at91sam9g45.dtsi
@@ -634,6 +634,11 @@
watchdog@fffffd40 {
compatible = "atmel,at91sam9260-wdt";
reg = <0xfffffd40 0x10>;
+ interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+ atmel,watchdog-type = "hardware";
+ atmel,reset-type = "all";
+ atmel,dbg-halt;
+ atmel,idle-halt;
status = "disabled";
};
diff --git a/arch/arm/boot/dts/at91sam9n12.dtsi b/arch/arm/boot/dts/at91sam9n12.dtsi
index 3166e1d..7c8a5ea 100644
--- a/arch/arm/boot/dts/at91sam9n12.dtsi
+++ b/arch/arm/boot/dts/at91sam9n12.dtsi
@@ -505,6 +505,11 @@
watchdog@fffffe40 {
compatible = "atmel,at91sam9260-wdt";
reg = <0xfffffe40 0x10>;
+ interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+ atmel,watchdog-type = "hardware";
+ atmel,reset-type = "all";
+ atmel,dbg-halt;
+ atmel,idle-halt;
status = "disabled";
};
};
diff --git a/arch/arm/boot/dts/at91sam9x5.dtsi b/arch/arm/boot/dts/at91sam9x5.dtsi
index e77106e..48eb1cc 100644
--- a/arch/arm/boot/dts/at91sam9x5.dtsi
+++ b/arch/arm/boot/dts/at91sam9x5.dtsi
@@ -742,6 +742,11 @@
watchdog@fffffe40 {
compatible = "atmel,at91sam9260-wdt";
reg = <0xfffffe40 0x10>;
+ interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+ atmel,watchdog-type = "hardware";
+ atmel,reset-type = "all";
+ atmel,dbg-halt;
+ atmel,idle-halt;
status = "disabled";
};
diff --git a/arch/arm/boot/dts/sama5d3.dtsi b/arch/arm/boot/dts/sama5d3.dtsi
index c00e158..7974723 100644
--- a/arch/arm/boot/dts/sama5d3.dtsi
+++ b/arch/arm/boot/dts/sama5d3.dtsi
@@ -885,6 +885,11 @@
watchdog@fffffe40 {
compatible = "atmel,at91sam9260-wdt";
reg = <0xfffffe40 0x10>;
+ interrupts = <4 IRQ_TYPE_LEVEL_HIGH 7>;
+ atmel,watchdog-type = "hardware";
+ atmel,reset-type = "all";
+ atmel,dbg-halt;
+ atmel,idle-halt;
status = "disabled";
};
--
1.7.9.5
Add watchdog specific config for kizbox board.
Signed-off-by: Boris BREZILLON <[email protected]>
---
arch/arm/boot/dts/kizbox.dts | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/arm/boot/dts/kizbox.dts b/arch/arm/boot/dts/kizbox.dts
index 02df191..928f6ee 100644
--- a/arch/arm/boot/dts/kizbox.dts
+++ b/arch/arm/boot/dts/kizbox.dts
@@ -53,6 +53,12 @@
status = "okay";
};
+ watchdog@fffffd40 {
+ timeout-sec = <15>;
+ atmel,max-heartbeat-sec = <16>;
+ atmel,min-heartbeat-sec = <0>;
+ status = "okay";
+ };
};
nand0: nand@40000000 {
--
1.7.9.5
Hello,
I missed Grant's ack on this patch:
Acked-by: Grant Likely<[email protected]>
Best Regards,
Boris
On 21/06/2013 09:23, Boris BREZILLON wrote:
> Add new at91sam9 watchdog properties to the documentation.
>
> Signed-off-by: Boris BREZILLON <[email protected]>
> ---
> .../devicetree/bindings/watchdog/atmel-wdt.txt | 30 ++++++++++++++++++--
> 1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
> index fcdd48f..e043106 100644
> --- a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
> @@ -9,11 +9,37 @@ Required properties:
>
> Optional properties:
> - timeout-sec: contains the watchdog timeout in seconds.
> +- interrupts : Should contain WDT interrupt.
> +- atmel,max-heartbeat-sec : Should contain the maximum heartbeat value in
> + seconds. This value should be less than 16. It is used to compute the
> + WDV field.
> +- atmel,min-heartbeat-sec : Should contain the minimum heartbeat value in
> + seconds. This value should be less than 4 times the max-heartbeat-sec
> + value. It is used to compute the WDD field.
> +- atmel,watchdog-type : Should be "hardware" or "software". Hardware watchdog
> + use the at91 watchdog reset. Software watchdog use the watchdog
> + interrupt to trigger a software reset.
> +- atmel,reset-type : Should be "proc" or "all".
> + "all" : assert peripherals and processor reset signals
> + "proc" : assert the processor reset signal
> + This is valid only when using "hardware" watchdog.
> +- atmel,disable : Should be present if you want to disable the watchdog.
> +- atmel,idle-halt : Should be present if you want to stop the watchdog when
> + entering idle state.
> +- atmel,dbg-halt : Should be present if you want to stop the watchdog when
> + entering debug state.
>
> Example:
> -
> watchdog@fffffd40 {
> compatible = "atmel,at91sam9260-wdt";
> reg = <0xfffffd40 0x10>;
> - timeout-sec = <10>;
> + interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
> + timeout-sec = <15>;
> + atmel,watchdog-type = "hardware";
> + atmel,reset-type = "all";
> + atmel,dbg-halt;
> + atmel,idle-halt;
> + atmel,max-heartbeat-sec = <16>;
> + atmel,min-heartbeat-sec = <0>;
> + status = "okay";
> };
Hello Yang,
Did you test/review this series ?
If you did, could you add your Acked, Reviewed and/or Tested-by.
I'd really like to get this series merged to mainline.
Thanks.
Best Regards,
Boris
On 21/06/2013 09:19, Boris BREZILLON wrote:
> Hello,
>
> This patch series is a porposal to enhance the sam9 watchdog timer support.
>
> The at91sam9 watchdog timer can only be configured once, and the current
> implementation tries to configure it in a static way:
> - 2 seconds timeout
> - wdt restart every 500ms
>
> If the timer has already been configured with different values, it returns an
> error and do not create any watchdog device.
>
> This is not critical if the watchdog is disabled, but if it has been enabled
> with different timeout values it will lead to a SoC reset.
>
> This patch series tries to address this issue by adapting the heartbeat value
> according the WDT timer config:
> - it first tries to configure the timer as requested.
> - if it fails it fallbacks to the current config, adapting its heartbeat timer
> to the needs
>
> This patch series also move to a dynamically allocated at91wdt device instead
> of the static instance. I'm not sure this is the best solution, so please tell
> me if you prefer to keep static instance of watchdog.
>
> It adds a new at91 wdt type: software. This new type make use of the at91 wdt
> interrupt to trigger a software reboot.
>
> Finally it adds several properties to the device tree bindings.
>
> Best Regards,
> Boris
>
> Change since v1:
> - fix typo in documentaion
> - fix irq dt definition for sama5d3 SoC
>
> Boris BREZILLON (4):
> watchdog: at91sam9_wdt: better watchdog support
> watchdog: at91sam9_wdt: update device tree doc
> ARM: at91/dt: add sam9 watchdog default options to SoCs
> ARM: at91/dt: add watchdog properties to kizbox board
>
> .../devicetree/bindings/watchdog/atmel-wdt.txt | 30 +-
> arch/arm/boot/dts/at91sam9260.dtsi | 5 +
> arch/arm/boot/dts/at91sam9263.dtsi | 5 +
> arch/arm/boot/dts/at91sam9g45.dtsi | 5 +
> arch/arm/boot/dts/at91sam9n12.dtsi | 5 +
> arch/arm/boot/dts/at91sam9x5.dtsi | 5 +
> arch/arm/boot/dts/kizbox.dts | 6 +
> arch/arm/boot/dts/sama5d3.dtsi | 5 +
> drivers/watchdog/at91sam9_wdt.c | 319 +++++++++++++++-----
> 9 files changed, 300 insertions(+), 85 deletions(-)
>
> -----Original Message-----
> From: boris brezillon [mailto:[email protected]]
> Sent: 2013??8??26?? 21:26
> To: Boris BREZILLON; Yang, Wenyou
> Cc: Wim Van Sebroeck; Ferre, Nicolas; Jean-Christophe PLAGNIOL-VILLARD;
> Desroches, Ludovic; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH v2 0/4] watchdog: at91sam9_wdt: handle already
> configured wdt
>
> Hello Yang,
>
> Did you test/review this series ?
>
> If you did, could you add your Acked, Reviewed and/or Tested-by.
> I'd really like to get this series merged to mainline.
>
> Thanks.
>
> Best Regards,
> Boris
>
> On 21/06/2013 09:19, Boris BREZILLON wrote:
> > Hello,
> >
> > This patch series is a porposal to enhance the sam9 watchdog timer support.
> >
> > The at91sam9 watchdog timer can only be configured once, and the current
> > implementation tries to configure it in a static way:
> > - 2 seconds timeout
> > - wdt restart every 500ms
> >
> > If the timer has already been configured with different values, it returns an
> > error and do not create any watchdog device.
> >
> > This is not critical if the watchdog is disabled, but if it has been enabled
> > with different timeout values it will lead to a SoC reset.
> >
> > This patch series tries to address this issue by adapting the heartbeat value
> > according the WDT timer config:
> > - it first tries to configure the timer as requested.
> > - if it fails it fallbacks to the current config, adapting its heartbeat timer
> > to the needs
> >
> > This patch series also move to a dynamically allocated at91wdt device instead
> > of the static instance. I'm not sure this is the best solution, so please tell
> > me if you prefer to keep static instance of watchdog.
> >
> > It adds a new at91 wdt type: software. This new type make use of the at91
> wdt
> > interrupt to trigger a software reboot.
> >
> > Finally it adds several properties to the device tree bindings.
> >
> > Best Regards,
> > Boris
> >
> > Change since v1:
> > - fix typo in documentaion
> > - fix irq dt definition for sama5d3 SoC
> >
> > Boris BREZILLON (4):
> > watchdog: at91sam9_wdt: better watchdog support
> > watchdog: at91sam9_wdt: update device tree doc
> > ARM: at91/dt: add sam9 watchdog default options to SoCs
> > ARM: at91/dt: add watchdog properties to kizbox board
> >
> > .../devicetree/bindings/watchdog/atmel-wdt.txt | 30 +-
> > arch/arm/boot/dts/at91sam9260.dtsi | 5 +
> > arch/arm/boot/dts/at91sam9263.dtsi | 5 +
> > arch/arm/boot/dts/at91sam9g45.dtsi | 5 +
> > arch/arm/boot/dts/at91sam9n12.dtsi | 5 +
> > arch/arm/boot/dts/at91sam9x5.dtsi | 5 +
> > arch/arm/boot/dts/kizbox.dts | 6 +
> > arch/arm/boot/dts/sama5d3.dtsi | 5 +
> > drivers/watchdog/at91sam9_wdt.c | 319
> +++++++++++++++-----
> > 9 files changed, 300 insertions(+), 85 deletions(-)
> >
Tested on sama5d34ek, at91sam9g25ek, linux-next_20130620
Tested-by Wenyou Yang <Wenyou.yang at atmel.com>
Best Regards,
Wenyou Yang
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Fri, Jun 21, 2013 at 03:23:34PM -0000, Boris BREZILLON wrote:
> Add new at91sam9 watchdog properties to the documentation.
>
> Signed-off-by: Boris BREZILLON <[email protected]>
> Acked-by: Grant Likely<[email protected]>
>
> ---
> .../devicetree/bindings/watchdog/atmel-wdt.txt | 30 ++++++++++++++++++--
> 1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
> index fcdd48f..e043106 100644
> --- a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
> @@ -9,11 +9,37 @@ Required properties:
>
> Optional properties:
> - timeout-sec: contains the watchdog timeout in seconds.
> +- interrupts : Should contain WDT interrupt.
> +- atmel,max-heartbeat-sec : Should contain the maximum heartbeat value in
> + seconds. This value should be less than 16. It is used to compute the
> + WDV field.
> +- atmel,min-heartbeat-sec : Should contain the minimum heartbeat value in
> + seconds. This value should be less than 4 times the max-heartbeat-sec
> + value. It is used to compute the WDD field.
I am a bit at loss about "less than 4 times the max-heartbeat-sec value".
Why would a min-timeout larger than the max-timeout make sense under any
condition ? Do you mean to say that max-heartbeat should be at least four
times the value of min-heartbeat (or that min-heartbeat should be less or
equal to one-forth of max-heartbeat) ?
Guenter
> +- atmel,watchdog-type : Should be "hardware" or "software". Hardware watchdog
> + use the at91 watchdog reset. Software watchdog use the watchdog
> + interrupt to trigger a software reset.
> +- atmel,reset-type : Should be "proc" or "all".
> + "all" : assert peripherals and processor reset signals
> + "proc" : assert the processor reset signal
> + This is valid only when using "hardware" watchdog.
> +- atmel,disable : Should be present if you want to disable the watchdog.
> +- atmel,idle-halt : Should be present if you want to stop the watchdog when
> + entering idle state.
> +- atmel,dbg-halt : Should be present if you want to stop the watchdog when
> + entering debug state.
>
> Example:
> -
> watchdog@fffffd40 {
> compatible = "atmel,at91sam9260-wdt";
> reg = <0xfffffd40 0x10>;
> - timeout-sec = <10>;
> + interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
> + timeout-sec = <15>;
> + atmel,watchdog-type = "hardware";
> + atmel,reset-type = "all";
> + atmel,dbg-halt;
> + atmel,idle-halt;
> + atmel,max-heartbeat-sec = <16>;
> + atmel,min-heartbeat-sec = <0>;
> + status = "okay";
> };
On Fri, Jun 21, 2013 at 03:21:28PM -0000, Boris BREZILLON wrote:
> The at91sam9 watchdog timer can only be configured once, and the current
> implementation tries to configure it in a static way:
> - 2 seconds timeout
> - wdt restart every 500ms
>
> If the timer has already been configured with different values, it returns an
> error and do not create any watchdog device.
>
> This is not critical if the watchdog is disabled, but if it has been enabled with
> different timeout values it will lead to a SoC reset.
>
> This patch series tries to address this issue by adapting the heartbeat value
> according the WDT timer config:
> - it first tries to configure the timer as requested.
> - if it fails it fallbacks to the current config, adapting its heartbeat timer
> to the needs
>
> This patch series also move to a dynamically allocated at91wdt device instead
> of the static instance.
>
> It adds a new at91 wdt type: software. This new type make use of the at91 wdt
> interrupt to trigger a software reboot.
>
> Finally it adds several properties to the device tree bindings.
>
> Signed-off-by: Boris BREZILLON <[email protected]>
>
> ---
> drivers/watchdog/at91sam9_wdt.c | 319 +++++++++++++++++++++++++++++----------
> 1 file changed, 236 insertions(+), 83 deletions(-)
>
> diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
> index be37dde..fb47ec5 100644
> --- a/drivers/watchdog/at91sam9_wdt.c
> +++ b/drivers/watchdog/at91sam9_wdt.c
> @@ -19,11 +19,13 @@
>
> #include <linux/errno.h>
> #include <linux/init.h>
> +#include <linux/interrupt.h>
> #include <linux/io.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/moduleparam.h>
> #include <linux/platform_device.h>
> +#include <linux/reboot.h>
> #include <linux/types.h>
> #include <linux/watchdog.h>
> #include <linux/jiffies.h>
> @@ -31,23 +33,32 @@
> #include <linux/bitops.h>
> #include <linux/uaccess.h>
> #include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
>
> #include "at91sam9_wdt.h"
>
> #define DRV_NAME "AT91SAM9 Watchdog"
>
> -#define wdt_read(field) \
> - __raw_readl(at91wdt_private.base + field)
> -#define wdt_write(field, val) \
> - __raw_writel((val), at91wdt_private.base + field)
> +#define wdt_read(wdt, field) \
> + __raw_readl((wdt)->base + field)
> +#define wdt_write(wtd, field, val) \
> + __raw_writel((val), (wdt)->base + field)
>
> /* AT91SAM9 watchdog runs a 12bit counter @ 256Hz,
> * use this to convert a watchdog
> * value from/to milliseconds.
> */
> +#define hz_to_ticks(h) (((h << 8) / HZ) - 1)
Unused.
> +#define ticks_to_hz(t) (((t + 1) * HZ) >> 8)
> #define ms_to_ticks(t) (((t << 8) / 1000) - 1)
> #define ticks_to_ms(t) (((t + 1) * 1000) >> 8)
You should put macro parameters in () to avoid undesired side effects.
(h), (t), (field)
Sure, some of them are old. No reason not to fix it, though.
>
> +#define WDT_MR_RESET 0x3FFF2FFF
> +
> +#define WDT_WDD_MAX 0xFFF
> +#define WDT_WDV_MAX 0xFFF
What are the units ?
> +
> /* Hardware timeout in seconds */
> #define WDT_HW_TIMEOUT 2
>
> @@ -66,23 +77,41 @@ module_param(nowayout, bool, 0);
> MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
> "(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>
> -static struct watchdog_device at91_wdt_dev;
> -static void at91_ping(unsigned long data);
> -
> -static struct {
> +#define to_wdt(wdd) container_of(wdd, struct at91wdt, wdd)
> +struct at91wdt {
> + struct watchdog_device wdd;
> void __iomem *base;
> unsigned long next_heartbeat; /* the next_heartbeat for the timer */
> struct timer_list timer; /* The timer that pings the watchdog */
> -} at91wdt_private;
> + u32 mr;
> + u32 mr_mask;
> + unsigned long heartbeat; /* WDT heartbeat in jiffies */
> + bool nowayout;
> + unsigned int irq;
> +};
>
> /* ......................................................................... */
>
> +static irqreturn_t wdt_interrupt(int irq, void *dev_id)
> +{
> + struct at91wdt *wdt = (struct at91wdt *)dev_id;
> +
> + if (wdt_read(wdt, AT91_WDT_SR)) {
> + /* Reboot */
That is quite obvious, isn't it ?
> + pr_crit("at91sam9 WDT software reset\n");
> + emergency_restart();
> + pr_crit("Reboot didn't ?????\n");
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> /*
> * Reload the watchdog timer. (ie, pat the watchdog)
> */
> -static inline void at91_wdt_reset(void)
> +static inline void at91_wdt_reset(struct at91wdt *wdt)
> {
> - wdt_write(AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
> + wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
> }
>
> /*
> @@ -90,26 +119,20 @@ static inline void at91_wdt_reset(void)
> */
> static void at91_ping(unsigned long data)
> {
> - if (time_before(jiffies, at91wdt_private.next_heartbeat) ||
> - (!watchdog_active(&at91_wdt_dev))) {
> - at91_wdt_reset();
> - mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
> + struct at91wdt *wdt = (struct at91wdt *)data;
> + if (time_before(jiffies, wdt->next_heartbeat) ||
> + (!watchdog_active(&wdt->wdd))) {
Unnecessary () around !watchdog_active()
[ Sure, old code. Not a reason not to clean it up ]
> + at91_wdt_reset(wdt);
> + mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
> } else
> pr_crit("I will reset your machine !\n");
> }
>
> static int at91_wdt_ping(struct watchdog_device *wdd)
> {
> + struct at91wdt *wdt = to_wdt(wdd);
> /* calculate when the next userspace timeout will be */
> - at91wdt_private.next_heartbeat = jiffies + wdd->timeout * HZ;
> - return 0;
> -}
> -
> -static int at91_wdt_start(struct watchdog_device *wdd)
> -{
> - /* calculate the next userspace timeout and modify the timer */
> - at91_wdt_ping(wdd);
> - mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
> + wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
> return 0;
> }
>
> @@ -125,36 +148,82 @@ static int at91_wdt_set_timeout(struct watchdog_device *wdd, unsigned int new_ti
> return 0;
> }
>
> -/*
> - * Set the watchdog time interval in 1/256Hz (write-once)
> - * Counter is 12 bit.
> - */
> -static int at91_wdt_settimeout(unsigned int timeout)
> +static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> {
> - unsigned int reg;
> - unsigned int mr;
> -
> - /* Check if disabled */
> - mr = wdt_read(AT91_WDT_MR);
> - if (mr & AT91_WDT_WDDIS) {
> - pr_err("sorry, watchdog is disabled\n");
> - return -EIO;
> + u32 tmp;
> + u32 delta;
> + u32 value;
> + int err;
> + u32 mask = wdt->mr_mask;
> +
> + tmp = wdt_read(wdt, AT91_WDT_MR);
> + if ((tmp & mask) != (wdt->mr & mask)) {
> + if (tmp == WDT_MR_RESET) {
> + wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> + tmp = wdt_read(wdt, AT91_WDT_MR);
> + }
> + }
> +
> + if (tmp & AT91_WDT_WDDIS) {
> + if (wdt->mr & AT91_WDT_WDDIS)
> + return 0;
> + dev_err(wdt->wdd.parent,
> + ": sorry, watchdog is disabled\n");
No need to be sorry all the time. The ':' is unnecessary.
> + return -EINVAL;
> + }
> +
> + value = tmp & AT91_WDT_WDV;
> + delta = (tmp & AT91_WDT_WDD) >> 16;
> +
> + if (delta < value) {
> + if ((value / (value - delta)) < 4) {
Unnecessary ( ) around left side of equation.
> + dev_err(wdt->wdd.parent,
> + ": sorry, max-heartbeat should be at least 4 times min-heartbeat (max-heartbeat = %d x min-heartbeat)\n",
> + (value / (value - delta)));
Unnecessary ( ) around the equation.
> + return -EINVAL;
> + }
> + }
> +
> + wdt->heartbeat = ticks_to_hz(value / 4);
> +
> + if (!wdt->heartbeat) {
> + dev_err(wdt->wdd.parent,
> + ": sorry, linux timer (%i Hz) cannot handle watchdog timeout (%i ms)\n",
> + HZ, ticks_to_ms(value));
> + return -EINVAL;
Isn't that a bit rude ? Why not set it to the minimum ?
> + }
> +
> + if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
> + err = request_irq(wdt->irq, wdt_interrupt,
> + IRQF_SHARED | IRQF_IRQPOLL,
> + pdev->name, wdt);
> + if (err)
> + return err;
> }
>
> - /*
> - * All counting occurs at SLOW_CLOCK / 128 = 256 Hz
> - *
> - * Since WDV is a 12-bit counter, the maximum period is
> - * 4096 / 256 = 16 seconds.
> - */
> - reg = AT91_WDT_WDRSTEN /* causes watchdog reset */
> - /* | AT91_WDT_WDRPROC causes processor reset only */
> - | AT91_WDT_WDDBGHLT /* disabled in debug mode */
> - | AT91_WDT_WDD /* restart at any time */
> - | (timeout & AT91_WDT_WDV); /* timer value */
> - wdt_write(AT91_WDT_MR, reg);
> + if ((tmp & wdt->mr_mask) != (wdt->mr & wdt->mr_mask))
> + dev_warn(wdt->wdd.parent,
> + ": watchdog already configured differently (mr = %x expecting %x)\n",
> + tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
> +
> + setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
> + mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
> +
> + /* Try to set timeout from device tree first */
> + if (watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev))
> + watchdog_init_timeout(&wdt->wdd, heartbeat, &pdev->dev);
> + watchdog_set_nowayout(&wdt->wdd, wdt->nowayout);
> + err = watchdog_register_device(&wdt->wdd);
> + if (err)
> + goto out_stop_timer;
> +
> + wdt->next_heartbeat = jiffies + wdt->wdd.timeout * HZ;
>
> return 0;
> +
> +out_stop_timer:
> + del_timer(&wdt->timer);
> + return err;
> }
>
> /* ......................................................................... */
> @@ -167,63 +236,147 @@ static const struct watchdog_info at91_wdt_info = {
>
> static const struct watchdog_ops at91_wdt_ops = {
> .owner = THIS_MODULE,
> - .start = at91_wdt_start,
> + .start = at91_wdt_ping,
> .stop = at91_wdt_stop,
> .ping = at91_wdt_ping,
> .set_timeout = at91_wdt_set_timeout,
> };
>
> -static struct watchdog_device at91_wdt_dev = {
> - .info = &at91_wdt_info,
> - .ops = &at91_wdt_ops,
> - .timeout = WDT_HEARTBEAT,
> - .min_timeout = 1,
> - .max_timeout = 0xFFFF,
> -};
> +#if defined(CONFIG_OF)
> +static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
> +{
> + u32 min = 0;
> + u32 max = 0xFFF;
Are those default values ? If so might use defines (there are actually defines
for 0xfff already).
> + const char *tmp;
> +
> + wdt->base = of_iomap(np, 0);
I am a bit at loss why you need to use use this function here.
I think it would be more beneficial to use devm_ioremap() or even better
devm_ioremap_resource() in both of and non-dt cases; then you don't have
to call iounmap().
> + if (!wdt->base) {
> + dev_err(wdt->wdd.parent, "failed to map registers, aborting.\n");
> + return -EINVAL;
> + }
> +
> + /* Get the interrupts property */
> + wdt->irq = irq_of_parse_and_map(np, 0);
> + if (!wdt->irq)
> + dev_warn(wdt->wdd.parent, "failed to get IRQ from DT.\n");
> +
> + if (!of_property_read_u32_index(np, "atmel,max-heartbeat-sec", 0,
> + &max)) {
> + max *= 256;
> + if (max > WDT_WDV_MAX)
> + max = WDT_WDV_MAX;
> +
> + if (!of_property_read_u32_index(np, "atmel,min-heartbeat-sec",
> + 0, &min)) {
> + min *= 256;
> + if (4 * min > max)
> + min = max / 4;
> + }
> + }
I am a bit confused about those parameters. How do they match min_timeout and
max_timeout ?
> +
> + wdt->mr_mask = 0x3FFFFFFF;
> + wdt->mr = 0;
> + if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
> + !strcmp(tmp, "software")) {
> + wdt->mr |= AT91_WDT_WDFIEN;
> + wdt->mr_mask &= ~AT91_WDT_WDRPROC;
> + } else
> + wdt->mr |= AT91_WDT_WDRSTEN;
> +
> + if (!of_property_read_string(np, "atmel,reset-type", &tmp) &&
> + !strcmp(tmp, "proc"))
> + wdt->mr |= AT91_WDT_WDRPROC;
> +
> + if (of_property_read_bool(np, "atmel,disable")) {
> + wdt->mr |= AT91_WDT_WDDIS;
> + wdt->mr_mask &= AT91_WDT_WDDIS;
> + }
> +
> + if (of_property_read_bool(np, "atmel,idle-halt"))
> + wdt->mr |= AT91_WDT_WDIDLEHLT;
> +
> + if (of_property_read_bool(np, "atmel,dbg-halt"))
> + wdt->mr |= AT91_WDT_WDDBGHLT;
> +
> + wdt->mr |= max | ((max - min) << 16);
> +
> + return 0;
> +}
> +#else
> +static inline int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
> +{
> + return 0;
> +}
> +#endif
>
> static int __init at91wdt_probe(struct platform_device *pdev)
> {
> struct resource *r;
> - int res;
> -
> - r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (!r)
> - return -ENODEV;
> - at91wdt_private.base = ioremap(r->start, resource_size(r));
> - if (!at91wdt_private.base) {
> - dev_err(&pdev->dev, "failed to map registers, aborting.\n");
> - return -ENOMEM;
> - }
> + int err;
> + struct at91wdt *wdt;
>
> - at91_wdt_dev.parent = &pdev->dev;
> - watchdog_init_timeout(&at91_wdt_dev, heartbeat, &pdev->dev);
> - watchdog_set_nowayout(&at91_wdt_dev, nowayout);
> + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> + if (!wdt)
> + return -ENOMEM;
>
> - /* Set watchdog */
> - res = at91_wdt_settimeout(ms_to_ticks(WDT_HW_TIMEOUT * 1000));
> - if (res)
> - return res;
> + wdt->mr = (WDT_HW_TIMEOUT * 256) | AT91_WDT_WDRSTEN | AT91_WDT_WDD |
> + AT91_WDT_WDDBGHLT | AT91_WDT_WDIDLEHLT;
> + wdt->mr_mask = 0x3FFFFFFF;
> + wdt->nowayout = nowayout;
> + wdt->wdd.parent = &pdev->dev;
> + wdt->wdd.info = &at91_wdt_info;
> + wdt->wdd.ops = &at91_wdt_ops;
> + wdt->wdd.timeout = WDT_HEARTBEAT;
> + wdt->wdd.min_timeout = 1;
> + wdt->wdd.max_timeout = 0xFFFF;
> +
> + if (pdev->dev.of_node) {
> + err = of_at91wdt_init(pdev->dev.of_node, wdt);
> + if (err)
> + goto out_free_watchdog;
> + } else {
> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!r) {
> + err = -ENODEV;
> + goto out_free_watchdog;
> + }
> +
> + wdt->base = ioremap(r->start, resource_size(r));
> + if (!wdt->base) {
> + dev_err(&pdev->dev, "failed to map registers, aborting.\n");
> + err = -ENOMEM;
> + goto out_free_watchdog;
> + }
> + }
>
> - res = watchdog_register_device(&at91_wdt_dev);
> - if (res)
> - return res;
> + err = at91_wdt_init(pdev, wdt);
> + if (err)
> + goto out_iounmap;
>
> - at91wdt_private.next_heartbeat = jiffies + at91_wdt_dev.timeout * HZ;
> - setup_timer(&at91wdt_private.timer, at91_ping, 0);
> - mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
> + platform_set_drvdata(pdev, wdt);
>
> pr_info("enabled (heartbeat=%d sec, nowayout=%d)\n",
> - at91_wdt_dev.timeout, nowayout);
> + wdt->wdd.timeout, wdt->nowayout);
>
> return 0;
> +
> +out_iounmap:
> + iounmap(wdt->base);
> +out_free_watchdog:
> + devm_kfree(&pdev->dev, wdt);
The whole point of using devm_kzalloc() is that you don't have to call kfree()
or devm_kfree(). So this call (and with it the label) is unnecessary.
> + return err;
> }
>
> static int __exit at91wdt_remove(struct platform_device *pdev)
> {
> - watchdog_unregister_device(&at91_wdt_dev);
> + struct at91wdt *wdt = platform_get_drvdata(pdev);
> + watchdog_unregister_device(&wdt->wdd);
>
> pr_warn("I quit now, hardware will probably reboot!\n");
> - del_timer(&at91wdt_private.timer);
> + del_timer(&wdt->timer);
> +
> + iounmap(wdt->base);
> + devm_kfree(&pdev->dev, wdt);
>
The call to devm_kfree() is unnecessary.
> return 0;
> }
On 02/10/2013 17:51, Guenter Roeck wrote:
> On Fri, Jun 21, 2013 at 03:23:34PM -0000, Boris BREZILLON wrote:
>> Add new at91sam9 watchdog properties to the documentation.
>>
>> Signed-off-by: Boris BREZILLON <[email protected]>
>> Acked-by: Grant Likely<[email protected]>
>>
>> ---
>> .../devicetree/bindings/watchdog/atmel-wdt.txt | 30 ++++++++++++++++++--
>> 1 file changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
>> index fcdd48f..e043106 100644
>> --- a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
>> +++ b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
>> @@ -9,11 +9,37 @@ Required properties:
>>
>> Optional properties:
>> - timeout-sec: contains the watchdog timeout in seconds.
>> +- interrupts : Should contain WDT interrupt.
>> +- atmel,max-heartbeat-sec : Should contain the maximum heartbeat value in
>> + seconds. This value should be less than 16. It is used to compute the
>> + WDV field.
>> +- atmel,min-heartbeat-sec : Should contain the minimum heartbeat value in
>> + seconds. This value should be less than 4 times the max-heartbeat-sec
>> + value. It is used to compute the WDD field.
> I am a bit at loss about "less than 4 times the max-heartbeat-sec value".
> Why would a min-timeout larger than the max-timeout make sense under any
> condition ? Do you mean to say that max-heartbeat should be at least four
> times the value of min-heartbeat (or that min-heartbeat should be less or
> equal to one-forth of max-heartbeat) ?
That's exactely what I meant (should be less or equal to one-forth of
max-heartbeat).
I'll fix it.
> Guenter
>
>> +- atmel,watchdog-type : Should be "hardware" or "software". Hardware watchdog
>> + use the at91 watchdog reset. Software watchdog use the watchdog
>> + interrupt to trigger a software reset.
>> +- atmel,reset-type : Should be "proc" or "all".
>> + "all" : assert peripherals and processor reset signals
>> + "proc" : assert the processor reset signal
>> + This is valid only when using "hardware" watchdog.
>> +- atmel,disable : Should be present if you want to disable the watchdog.
>> +- atmel,idle-halt : Should be present if you want to stop the watchdog when
>> + entering idle state.
>> +- atmel,dbg-halt : Should be present if you want to stop the watchdog when
>> + entering debug state.
>>
>> Example:
>> -
>> watchdog@fffffd40 {
>> compatible = "atmel,at91sam9260-wdt";
>> reg = <0xfffffd40 0x10>;
>> - timeout-sec = <10>;
>> + interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
>> + timeout-sec = <15>;
>> + atmel,watchdog-type = "hardware";
>> + atmel,reset-type = "all";
>> + atmel,dbg-halt;
>> + atmel,idle-halt;
>> + atmel,max-heartbeat-sec = <16>;
>> + atmel,min-heartbeat-sec = <0>;
>> + status = "okay";
>> };
Hello Guenter,
Thanks for reviewing this patch.
On 02/10/2013 18:12, Guenter Roeck wrote:
> On Fri, Jun 21, 2013 at 03:21:28PM -0000, Boris BREZILLON wrote:
>> The at91sam9 watchdog timer can only be configured once, and the current
>> implementation tries to configure it in a static way:
>> - 2 seconds timeout
>> - wdt restart every 500ms
>>
>> If the timer has already been configured with different values, it returns an
>> error and do not create any watchdog device.
>>
>> This is not critical if the watchdog is disabled, but if it has been enabled with
>> different timeout values it will lead to a SoC reset.
>>
>> This patch series tries to address this issue by adapting the heartbeat value
>> according the WDT timer config:
>> - it first tries to configure the timer as requested.
>> - if it fails it fallbacks to the current config, adapting its heartbeat timer
>> to the needs
>>
>> This patch series also move to a dynamically allocated at91wdt device instead
>> of the static instance.
>>
>> It adds a new at91 wdt type: software. This new type make use of the at91 wdt
>> interrupt to trigger a software reboot.
>>
>> Finally it adds several properties to the device tree bindings.
>>
>> Signed-off-by: Boris BREZILLON <[email protected]>
>>
>> ---
>> drivers/watchdog/at91sam9_wdt.c | 319 +++++++++++++++++++++++++++++----------
>> 1 file changed, 236 insertions(+), 83 deletions(-)
>>
>> diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
>> index be37dde..fb47ec5 100644
>> --- a/drivers/watchdog/at91sam9_wdt.c
>> +++ b/drivers/watchdog/at91sam9_wdt.c
>> @@ -19,11 +19,13 @@
>>
>> #include <linux/errno.h>
>> #include <linux/init.h>
>> +#include <linux/interrupt.h>
>> #include <linux/io.h>
>> #include <linux/kernel.h>
>> #include <linux/module.h>
>> #include <linux/moduleparam.h>
>> #include <linux/platform_device.h>
>> +#include <linux/reboot.h>
>> #include <linux/types.h>
>> #include <linux/watchdog.h>
>> #include <linux/jiffies.h>
>> @@ -31,23 +33,32 @@
>> #include <linux/bitops.h>
>> #include <linux/uaccess.h>
>> #include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>>
>> #include "at91sam9_wdt.h"
>>
>> #define DRV_NAME "AT91SAM9 Watchdog"
>>
>> -#define wdt_read(field) \
>> - __raw_readl(at91wdt_private.base + field)
>> -#define wdt_write(field, val) \
>> - __raw_writel((val), at91wdt_private.base + field)
>> +#define wdt_read(wdt, field) \
>> + __raw_readl((wdt)->base + field)
>> +#define wdt_write(wtd, field, val) \
>> + __raw_writel((val), (wdt)->base + field)
>>
>> /* AT91SAM9 watchdog runs a 12bit counter @ 256Hz,
>> * use this to convert a watchdog
>> * value from/to milliseconds.
>> */
>> +#define hz_to_ticks(h) (((h << 8) / HZ) - 1)
> Unused.
I'll drop it.
>
>> +#define ticks_to_hz(t) (((t + 1) * HZ) >> 8)
>> #define ms_to_ticks(t) (((t << 8) / 1000) - 1)
>> #define ticks_to_ms(t) (((t + 1) * 1000) >> 8)
> You should put macro parameters in () to avoid undesired side effects.
>
> (h), (t), (field)
>
> Sure, some of them are old. No reason not to fix it, though.
I'll fix it (I'll fix old macros in a separate patch)
>
>>
>> +#define WDT_MR_RESET 0x3FFF2FFF
>> +
>> +#define WDT_WDD_MAX 0xFFF
>> +#define WDT_WDV_MAX 0xFFF
> What are the units ?
ticks (slow clk cycles).
I'll rename these macros:
#define WDT_XXX_MAX_TICKS 0xFFF
>
>> +
>> /* Hardware timeout in seconds */
>> #define WDT_HW_TIMEOUT 2
>>
>> @@ -66,23 +77,41 @@ module_param(nowayout, bool, 0);
>> MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
>> "(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>>
>> -static struct watchdog_device at91_wdt_dev;
>> -static void at91_ping(unsigned long data);
>> -
>> -static struct {
>> +#define to_wdt(wdd) container_of(wdd, struct at91wdt, wdd)
>> +struct at91wdt {
>> + struct watchdog_device wdd;
>> void __iomem *base;
>> unsigned long next_heartbeat; /* the next_heartbeat for the timer */
>> struct timer_list timer; /* The timer that pings the watchdog */
>> -} at91wdt_private;
>> + u32 mr;
>> + u32 mr_mask;
>> + unsigned long heartbeat; /* WDT heartbeat in jiffies */
>> + bool nowayout;
>> + unsigned int irq;
>> +};
>>
>> /* ......................................................................... */
>>
>> +static irqreturn_t wdt_interrupt(int irq, void *dev_id)
>> +{
>> + struct at91wdt *wdt = (struct at91wdt *)dev_id;
>> +
>> + if (wdt_read(wdt, AT91_WDT_SR)) {
>> + /* Reboot */
> That is quite obvious, isn't it ?
I'll drop the comment line.
>
>> + pr_crit("at91sam9 WDT software reset\n");
>> + emergency_restart();
>> + pr_crit("Reboot didn't ?????\n");
>> + }
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> /*
>> * Reload the watchdog timer. (ie, pat the watchdog)
>> */
>> -static inline void at91_wdt_reset(void)
>> +static inline void at91_wdt_reset(struct at91wdt *wdt)
>> {
>> - wdt_write(AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
>> + wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
>> }
>>
>> /*
>> @@ -90,26 +119,20 @@ static inline void at91_wdt_reset(void)
>> */
>> static void at91_ping(unsigned long data)
>> {
>> - if (time_before(jiffies, at91wdt_private.next_heartbeat) ||
>> - (!watchdog_active(&at91_wdt_dev))) {
>> - at91_wdt_reset();
>> - mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
>> + struct at91wdt *wdt = (struct at91wdt *)data;
>> + if (time_before(jiffies, wdt->next_heartbeat) ||
>> + (!watchdog_active(&wdt->wdd))) {
>
> Unnecessary () around !watchdog_active()
>
> [ Sure, old code. Not a reason not to clean it up ]
Okay, I'll remove all unnecessary "()" (even the old ones).
>
>> + at91_wdt_reset(wdt);
>> + mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
>> } else
>> pr_crit("I will reset your machine !\n");
>> }
>>
>> static int at91_wdt_ping(struct watchdog_device *wdd)
>> {
>> + struct at91wdt *wdt = to_wdt(wdd);
>> /* calculate when the next userspace timeout will be */
>> - at91wdt_private.next_heartbeat = jiffies + wdd->timeout * HZ;
>> - return 0;
>> -}
>> -
>> -static int at91_wdt_start(struct watchdog_device *wdd)
>> -{
>> - /* calculate the next userspace timeout and modify the timer */
>> - at91_wdt_ping(wdd);
>> - mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
>> + wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
>> return 0;
>> }
>>
>> @@ -125,36 +148,82 @@ static int at91_wdt_set_timeout(struct watchdog_device *wdd, unsigned int new_ti
>> return 0;
>> }
>>
>> -/*
>> - * Set the watchdog time interval in 1/256Hz (write-once)
>> - * Counter is 12 bit.
>> - */
>> -static int at91_wdt_settimeout(unsigned int timeout)
>> +static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>> {
>> - unsigned int reg;
>> - unsigned int mr;
>> -
>> - /* Check if disabled */
>> - mr = wdt_read(AT91_WDT_MR);
>> - if (mr & AT91_WDT_WDDIS) {
>> - pr_err("sorry, watchdog is disabled\n");
>> - return -EIO;
>> + u32 tmp;
>> + u32 delta;
>> + u32 value;
>> + int err;
>> + u32 mask = wdt->mr_mask;
>> +
>> + tmp = wdt_read(wdt, AT91_WDT_MR);
>> + if ((tmp & mask) != (wdt->mr & mask)) {
>> + if (tmp == WDT_MR_RESET) {
>> + wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>> + tmp = wdt_read(wdt, AT91_WDT_MR);
>> + }
>> + }
>> +
>> + if (tmp & AT91_WDT_WDDIS) {
>> + if (wdt->mr & AT91_WDT_WDDIS)
>> + return 0;
>> + dev_err(wdt->wdd.parent,
>> + ": sorry, watchdog is disabled\n");
> No need to be sorry all the time. The ':' is unnecessary.
Sure, I'll remove the ":" and rework old and new messages (remove the
"sorry" word).
>
>> + return -EINVAL;
>> + }
>> +
>> + value = tmp & AT91_WDT_WDV;
>> + delta = (tmp & AT91_WDT_WDD) >> 16;
>> +
>> + if (delta < value) {
>> + if ((value / (value - delta)) < 4) {
> Unnecessary ( ) around left side of equation.
>
>> + dev_err(wdt->wdd.parent,
>> + ": sorry, max-heartbeat should be at least 4 times min-heartbeat (max-heartbeat = %d x min-heartbeat)\n",
>> + (value / (value - delta)));
> Unnecessary ( ) around the equation.
>
>> + return -EINVAL;
>> + }
>> + }
>> +
>> + wdt->heartbeat = ticks_to_hz(value / 4);
>> +
>> + if (!wdt->heartbeat) {
>> + dev_err(wdt->wdd.parent,
>> + ": sorry, linux timer (%i Hz) cannot handle watchdog timeout (%i ms)\n",
>> + HZ, ticks_to_ms(value));
>> + return -EINVAL;
> Isn't that a bit rude ? Why not set it to the minimum ?
We're using a normal timer (not an hrtimer), and IIRC normal timers'
precision depends on the
configured HZ option. As a result the minimum heartbeat value may
trigger the timer function
after the hardware watchdog counter expires.
An example where this problem might occur:
HZ = 10 <=> normal timer precision = 100 ms
min hearbeat = 180 ms
max hearbeat = 200 ms
The div by 4 operation is here to take a safety margin by refreshing the
hardware counter at least
4 times more often than needed.
I'll think of something more flexible...
If you have some ideas, please feel free to share it.
>> + }
>> +
>> + if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
>> + err = request_irq(wdt->irq, wdt_interrupt,
>> + IRQF_SHARED | IRQF_IRQPOLL,
>> + pdev->name, wdt);
>> + if (err)
>> + return err;
>> }
>>
>> - /*
>> - * All counting occurs at SLOW_CLOCK / 128 = 256 Hz
>> - *
>> - * Since WDV is a 12-bit counter, the maximum period is
>> - * 4096 / 256 = 16 seconds.
>> - */
>> - reg = AT91_WDT_WDRSTEN /* causes watchdog reset */
>> - /* | AT91_WDT_WDRPROC causes processor reset only */
>> - | AT91_WDT_WDDBGHLT /* disabled in debug mode */
>> - | AT91_WDT_WDD /* restart at any time */
>> - | (timeout & AT91_WDT_WDV); /* timer value */
>> - wdt_write(AT91_WDT_MR, reg);
>> + if ((tmp & wdt->mr_mask) != (wdt->mr & wdt->mr_mask))
>> + dev_warn(wdt->wdd.parent,
>> + ": watchdog already configured differently (mr = %x expecting %x)\n",
>> + tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
>> +
>> + setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
>> + mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
>> +
>> + /* Try to set timeout from device tree first */
>> + if (watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev))
>> + watchdog_init_timeout(&wdt->wdd, heartbeat, &pdev->dev);
>> + watchdog_set_nowayout(&wdt->wdd, wdt->nowayout);
>> + err = watchdog_register_device(&wdt->wdd);
>> + if (err)
>> + goto out_stop_timer;
>> +
>> + wdt->next_heartbeat = jiffies + wdt->wdd.timeout * HZ;
>>
>> return 0;
>> +
>> +out_stop_timer:
>> + del_timer(&wdt->timer);
>> + return err;
>> }
>>
>> /* ......................................................................... */
>> @@ -167,63 +236,147 @@ static const struct watchdog_info at91_wdt_info = {
>>
>> static const struct watchdog_ops at91_wdt_ops = {
>> .owner = THIS_MODULE,
>> - .start = at91_wdt_start,
>> + .start = at91_wdt_ping,
>> .stop = at91_wdt_stop,
>> .ping = at91_wdt_ping,
>> .set_timeout = at91_wdt_set_timeout,
>> };
>>
>> -static struct watchdog_device at91_wdt_dev = {
>> - .info = &at91_wdt_info,
>> - .ops = &at91_wdt_ops,
>> - .timeout = WDT_HEARTBEAT,
>> - .min_timeout = 1,
>> - .max_timeout = 0xFFFF,
>> -};
>> +#if defined(CONFIG_OF)
>> +static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
>> +{
>> + u32 min = 0;
>> + u32 max = 0xFFF;
> Are those default values ? If so might use defines (there are actually defines
> for 0xfff already).
I'll replace the 0xfff value by the XX_MAX macro.
Should I create a new XX_MIN macro ?
>> + const char *tmp;
>> +
>> + wdt->base = of_iomap(np, 0);
> I am a bit at loss why you need to use use this function here.
>
> I think it would be more beneficial to use devm_ioremap() or even better
> devm_ioremap_resource() in both of and non-dt cases; then you don't have
> to call iounmap().
Sure. This was one of my first driver using the devm and of_iomap
functions, and now I realize
I've been misusing them.
I'll fix this too.
>> + if (!wdt->base) {
>> + dev_err(wdt->wdd.parent, "failed to map registers, aborting.\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* Get the interrupts property */
>> + wdt->irq = irq_of_parse_and_map(np, 0);
>> + if (!wdt->irq)
>> + dev_warn(wdt->wdd.parent, "failed to get IRQ from DT.\n");
>> +
>> + if (!of_property_read_u32_index(np, "atmel,max-heartbeat-sec", 0,
>> + &max)) {
>> + max *= 256;
>> + if (max > WDT_WDV_MAX)
>> + max = WDT_WDV_MAX;
>> +
>> + if (!of_property_read_u32_index(np, "atmel,min-heartbeat-sec",
>> + 0, &min)) {
>> + min *= 256;
>> + if (4 * min > max)
>> + min = max / 4;
>> + }
>> + }
> I am a bit confused about those parameters. How do they match min_timeout and
> max_timeout ?
AT91 wdt block does not provide a reconfigurable timeout.
To bypass this limitation the driver periodically reset the hw watchdog
(based on the previous config,
the device tree config or the static config) using a timer.
The ping callback (used by watchdog core) only compute the
next_heartbeat (expressed in system ticks)
based on the current jiffies value (number of ticks elapsed since system
startup) and the configured
timeout value (configured using the set_timeout callback).
To summarize:
- min/max heartbeat timeout represent the hw timeout properties (those
directly linked to WDD WDV
fields).
- min/max timeout (in watchdog_device struct) represent the software
timeout properties (those
exposed to the user space API).
I could rework the driver to directly use the hw properties and remove
the set_timeout functionality,
but this would break the user space API.
>> +
>> + wdt->mr_mask = 0x3FFFFFFF;
>> + wdt->mr = 0;
>> + if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
>> + !strcmp(tmp, "software")) {
>> + wdt->mr |= AT91_WDT_WDFIEN;
>> + wdt->mr_mask &= ~AT91_WDT_WDRPROC;
>> + } else
>> + wdt->mr |= AT91_WDT_WDRSTEN;
>> +
>> + if (!of_property_read_string(np, "atmel,reset-type", &tmp) &&
>> + !strcmp(tmp, "proc"))
>> + wdt->mr |= AT91_WDT_WDRPROC;
>> +
>> + if (of_property_read_bool(np, "atmel,disable")) {
>> + wdt->mr |= AT91_WDT_WDDIS;
>> + wdt->mr_mask &= AT91_WDT_WDDIS;
>> + }
>> +
>> + if (of_property_read_bool(np, "atmel,idle-halt"))
>> + wdt->mr |= AT91_WDT_WDIDLEHLT;
>> +
>> + if (of_property_read_bool(np, "atmel,dbg-halt"))
>> + wdt->mr |= AT91_WDT_WDDBGHLT;
>> +
>> + wdt->mr |= max | ((max - min) << 16);
>> +
>> + return 0;
>> +}
>> +#else
>> +static inline int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
>> +{
>> + return 0;
>> +}
>> +#endif
>>
>> static int __init at91wdt_probe(struct platform_device *pdev)
>> {
>> struct resource *r;
>> - int res;
>> -
>> - r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> - if (!r)
>> - return -ENODEV;
>> - at91wdt_private.base = ioremap(r->start, resource_size(r));
>> - if (!at91wdt_private.base) {
>> - dev_err(&pdev->dev, "failed to map registers, aborting.\n");
>> - return -ENOMEM;
>> - }
>> + int err;
>> + struct at91wdt *wdt;
>>
>> - at91_wdt_dev.parent = &pdev->dev;
>> - watchdog_init_timeout(&at91_wdt_dev, heartbeat, &pdev->dev);
>> - watchdog_set_nowayout(&at91_wdt_dev, nowayout);
>> + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
>> + if (!wdt)
>> + return -ENOMEM;
>>
>> - /* Set watchdog */
>> - res = at91_wdt_settimeout(ms_to_ticks(WDT_HW_TIMEOUT * 1000));
>> - if (res)
>> - return res;
>> + wdt->mr = (WDT_HW_TIMEOUT * 256) | AT91_WDT_WDRSTEN | AT91_WDT_WDD |
>> + AT91_WDT_WDDBGHLT | AT91_WDT_WDIDLEHLT;
>> + wdt->mr_mask = 0x3FFFFFFF;
>> + wdt->nowayout = nowayout;
>> + wdt->wdd.parent = &pdev->dev;
>> + wdt->wdd.info = &at91_wdt_info;
>> + wdt->wdd.ops = &at91_wdt_ops;
>> + wdt->wdd.timeout = WDT_HEARTBEAT;
>> + wdt->wdd.min_timeout = 1;
>> + wdt->wdd.max_timeout = 0xFFFF;
>> +
>> + if (pdev->dev.of_node) {
>> + err = of_at91wdt_init(pdev->dev.of_node, wdt);
>> + if (err)
>> + goto out_free_watchdog;
>> + } else {
>> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!r) {
>> + err = -ENODEV;
>> + goto out_free_watchdog;
>> + }
>> +
>> + wdt->base = ioremap(r->start, resource_size(r));
>> + if (!wdt->base) {
>> + dev_err(&pdev->dev, "failed to map registers, aborting.\n");
>> + err = -ENOMEM;
>> + goto out_free_watchdog;
>> + }
>> + }
>>
>> - res = watchdog_register_device(&at91_wdt_dev);
>> - if (res)
>> - return res;
>> + err = at91_wdt_init(pdev, wdt);
>> + if (err)
>> + goto out_iounmap;
>>
>> - at91wdt_private.next_heartbeat = jiffies + at91_wdt_dev.timeout * HZ;
>> - setup_timer(&at91wdt_private.timer, at91_ping, 0);
>> - mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
>> + platform_set_drvdata(pdev, wdt);
>>
>> pr_info("enabled (heartbeat=%d sec, nowayout=%d)\n",
>> - at91_wdt_dev.timeout, nowayout);
>> + wdt->wdd.timeout, wdt->nowayout);
>>
>> return 0;
>> +
>> +out_iounmap:
>> + iounmap(wdt->base);
>> +out_free_watchdog:
>> + devm_kfree(&pdev->dev, wdt);
> The whole point of using devm_kzalloc() is that you don't have to call kfree()
> or devm_kfree(). So this call (and with it the label) is unnecessary.
>
>> + return err;
>> }
>>
>> static int __exit at91wdt_remove(struct platform_device *pdev)
>> {
>> - watchdog_unregister_device(&at91_wdt_dev);
>> + struct at91wdt *wdt = platform_get_drvdata(pdev);
>> + watchdog_unregister_device(&wdt->wdd);
>>
>> pr_warn("I quit now, hardware will probably reboot!\n");
>> - del_timer(&at91wdt_private.timer);
>> + del_timer(&wdt->timer);
>> +
>> + iounmap(wdt->base);
>> + devm_kfree(&pdev->dev, wdt);
>>
> The call to devm_kfree() is unnecessary.
>
>> return 0;
>> }
On Wed, Oct 02, 2013 at 09:27:03PM +0200, boris brezillon wrote:
> Hello Guenter,
>
> Thanks for reviewing this patch.
>
> On 02/10/2013 18:12, Guenter Roeck wrote:
> >On Fri, Jun 21, 2013 at 03:21:28PM -0000, Boris BREZILLON wrote:
> >>The at91sam9 watchdog timer can only be configured once, and the current
> >>implementation tries to configure it in a static way:
> >>- 2 seconds timeout
> >>- wdt restart every 500ms
> >>
> >>If the timer has already been configured with different values, it returns an
> >>error and do not create any watchdog device.
> >>
> >>This is not critical if the watchdog is disabled, but if it has been enabled with
> >>different timeout values it will lead to a SoC reset.
> >>
> >>This patch series tries to address this issue by adapting the heartbeat value
> >>according the WDT timer config:
> >>- it first tries to configure the timer as requested.
> >>- if it fails it fallbacks to the current config, adapting its heartbeat timer
> >>to the needs
> >>
> >>This patch series also move to a dynamically allocated at91wdt device instead
> >>of the static instance.
> >>
> >>It adds a new at91 wdt type: software. This new type make use of the at91 wdt
> >>interrupt to trigger a software reboot.
> >>
> >>Finally it adds several properties to the device tree bindings.
> >>
> >>Signed-off-by: Boris BREZILLON <[email protected]>
> >>
> >>---
> >>drivers/watchdog/at91sam9_wdt.c | 319 +++++++++++++++++++++++++++++----------
> >> 1 file changed, 236 insertions(+), 83 deletions(-)
> >>
> >>diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
> >>index be37dde..fb47ec5 100644
> >>--- a/drivers/watchdog/at91sam9_wdt.c
> >>+++ b/drivers/watchdog/at91sam9_wdt.c
> >>@@ -19,11 +19,13 @@
> >> #include <linux/errno.h>
> >> #include <linux/init.h>
> >>+#include <linux/interrupt.h>
> >> #include <linux/io.h>
> >> #include <linux/kernel.h>
> >> #include <linux/module.h>
> >> #include <linux/moduleparam.h>
> >> #include <linux/platform_device.h>
> >>+#include <linux/reboot.h>
> >> #include <linux/types.h>
> >> #include <linux/watchdog.h>
> >> #include <linux/jiffies.h>
> >>@@ -31,23 +33,32 @@
> >> #include <linux/bitops.h>
> >> #include <linux/uaccess.h>
> >> #include <linux/of.h>
> >>+#include <linux/of_address.h>
> >>+#include <linux/of_irq.h>
> >> #include "at91sam9_wdt.h"
> >> #define DRV_NAME "AT91SAM9 Watchdog"
> >>-#define wdt_read(field) \
> >>- __raw_readl(at91wdt_private.base + field)
> >>-#define wdt_write(field, val) \
> >>- __raw_writel((val), at91wdt_private.base + field)
> >>+#define wdt_read(wdt, field) \
> >>+ __raw_readl((wdt)->base + field)
> >>+#define wdt_write(wtd, field, val) \
> >>+ __raw_writel((val), (wdt)->base + field)
> >> /* AT91SAM9 watchdog runs a 12bit counter @ 256Hz,
> >> * use this to convert a watchdog
> >> * value from/to milliseconds.
> >> */
> >>+#define hz_to_ticks(h) (((h << 8) / HZ) - 1)
> >Unused.
>
> I'll drop it.
>
> >
> >>+#define ticks_to_hz(t) (((t + 1) * HZ) >> 8)
> >> #define ms_to_ticks(t) (((t << 8) / 1000) - 1)
> >> #define ticks_to_ms(t) (((t + 1) * 1000) >> 8)
> >You should put macro parameters in () to avoid undesired side effects.
> >
> > (h), (t), (field)
> >
> >Sure, some of them are old. No reason not to fix it, though.
>
> I'll fix it (I'll fix old macros in a separate patch)
>
> >
> >>+#define WDT_MR_RESET 0x3FFF2FFF
> >>+
> >>+#define WDT_WDD_MAX 0xFFF
> >>+#define WDT_WDV_MAX 0xFFF
> >What are the units ?
> ticks (slow clk cycles).
> I'll rename these macros:
> #define WDT_XXX_MAX_TICKS 0xFFF
> >
> >>+
> >> /* Hardware timeout in seconds */
> >> #define WDT_HW_TIMEOUT 2
> >>@@ -66,23 +77,41 @@ module_param(nowayout, bool, 0);
> >> MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
> >> "(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> >>-static struct watchdog_device at91_wdt_dev;
> >>-static void at91_ping(unsigned long data);
> >>-
> >>-static struct {
> >>+#define to_wdt(wdd) container_of(wdd, struct at91wdt, wdd)
> >>+struct at91wdt {
> >>+ struct watchdog_device wdd;
> >> void __iomem *base;
> >> unsigned long next_heartbeat; /* the next_heartbeat for the timer */
> >> struct timer_list timer; /* The timer that pings the watchdog */
> >>-} at91wdt_private;
> >>+ u32 mr;
> >>+ u32 mr_mask;
> >>+ unsigned long heartbeat; /* WDT heartbeat in jiffies */
> >>+ bool nowayout;
> >>+ unsigned int irq;
> >>+};
> >> /* ......................................................................... */
> >>+static irqreturn_t wdt_interrupt(int irq, void *dev_id)
> >>+{
> >>+ struct at91wdt *wdt = (struct at91wdt *)dev_id;
> >>+
> >>+ if (wdt_read(wdt, AT91_WDT_SR)) {
> >>+ /* Reboot */
> >That is quite obvious, isn't it ?
> I'll drop the comment line.
> >
> >>+ pr_crit("at91sam9 WDT software reset\n");
> >>+ emergency_restart();
> >>+ pr_crit("Reboot didn't ?????\n");
> >>+ }
> >>+
> >>+ return IRQ_HANDLED;
> >>+}
> >>+
> >> /*
> >> * Reload the watchdog timer. (ie, pat the watchdog)
> >> */
> >>-static inline void at91_wdt_reset(void)
> >>+static inline void at91_wdt_reset(struct at91wdt *wdt)
> >> {
> >>- wdt_write(AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
> >>+ wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
> >> }
> >> /*
> >>@@ -90,26 +119,20 @@ static inline void at91_wdt_reset(void)
> >> */
> >> static void at91_ping(unsigned long data)
> >> {
> >>- if (time_before(jiffies, at91wdt_private.next_heartbeat) ||
> >>- (!watchdog_active(&at91_wdt_dev))) {
> >>- at91_wdt_reset();
> >>- mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
> >>+ struct at91wdt *wdt = (struct at91wdt *)data;
> >>+ if (time_before(jiffies, wdt->next_heartbeat) ||
> >>+ (!watchdog_active(&wdt->wdd))) {
> >
> > Unnecessary () around !watchdog_active()
> >
> >[ Sure, old code. Not a reason not to clean it up ]
> Okay, I'll remove all unnecessary "()" (even the old ones).
> >
> >>+ at91_wdt_reset(wdt);
> >>+ mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
> >> } else
> >> pr_crit("I will reset your machine !\n");
> >> }
> >> static int at91_wdt_ping(struct watchdog_device *wdd)
> >> {
> >>+ struct at91wdt *wdt = to_wdt(wdd);
> >> /* calculate when the next userspace timeout will be */
> >>- at91wdt_private.next_heartbeat = jiffies + wdd->timeout * HZ;
> >>- return 0;
> >>-}
> >>-
> >>-static int at91_wdt_start(struct watchdog_device *wdd)
> >>-{
> >>- /* calculate the next userspace timeout and modify the timer */
> >>- at91_wdt_ping(wdd);
> >>- mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
> >>+ wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
> >> return 0;
> >> }
> >>@@ -125,36 +148,82 @@ static int at91_wdt_set_timeout(struct watchdog_device *wdd, unsigned int new_ti
> >> return 0;
> >> }
> >>-/*
> >>- * Set the watchdog time interval in 1/256Hz (write-once)
> >>- * Counter is 12 bit.
> >>- */
> >>-static int at91_wdt_settimeout(unsigned int timeout)
> >>+static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> >> {
> >>- unsigned int reg;
> >>- unsigned int mr;
> >>-
> >>- /* Check if disabled */
> >>- mr = wdt_read(AT91_WDT_MR);
> >>- if (mr & AT91_WDT_WDDIS) {
> >>- pr_err("sorry, watchdog is disabled\n");
> >>- return -EIO;
> >>+ u32 tmp;
> >>+ u32 delta;
> >>+ u32 value;
> >>+ int err;
> >>+ u32 mask = wdt->mr_mask;
> >>+
> >>+ tmp = wdt_read(wdt, AT91_WDT_MR);
> >>+ if ((tmp & mask) != (wdt->mr & mask)) {
> >>+ if (tmp == WDT_MR_RESET) {
> >>+ wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> >>+ tmp = wdt_read(wdt, AT91_WDT_MR);
> >>+ }
> >>+ }
> >>+
> >>+ if (tmp & AT91_WDT_WDDIS) {
> >>+ if (wdt->mr & AT91_WDT_WDDIS)
> >>+ return 0;
> >>+ dev_err(wdt->wdd.parent,
> >>+ ": sorry, watchdog is disabled\n");
> >No need to be sorry all the time. The ':' is unnecessary.
>
> Sure, I'll remove the ":" and rework old and new messages (remove
> the "sorry" word).
> >
> >>+ return -EINVAL;
> >>+ }
> >>+
> >>+ value = tmp & AT91_WDT_WDV;
> >>+ delta = (tmp & AT91_WDT_WDD) >> 16;
> >>+
> >>+ if (delta < value) {
> >>+ if ((value / (value - delta)) < 4) {
> > Unnecessary ( ) around left side of equation.
> >
> >>+ dev_err(wdt->wdd.parent,
> >>+ ": sorry, max-heartbeat should be at least 4 times min-heartbeat (max-heartbeat = %d x min-heartbeat)\n",
> >>+ (value / (value - delta)));
> > Unnecessary ( ) around the equation.
> >
> >>+ return -EINVAL;
> >>+ }
> >>+ }
> >>+
> >>+ wdt->heartbeat = ticks_to_hz(value / 4);
> >>+
> >>+ if (!wdt->heartbeat) {
> >>+ dev_err(wdt->wdd.parent,
> >>+ ": sorry, linux timer (%i Hz) cannot handle watchdog timeout (%i ms)\n",
> >>+ HZ, ticks_to_ms(value));
> >>+ return -EINVAL;
> >Isn't that a bit rude ? Why not set it to the minimum ?
> We're using a normal timer (not an hrtimer), and IIRC normal timers'
> precision depends on the
> configured HZ option. As a result the minimum heartbeat value may
> trigger the timer function
> after the hardware watchdog counter expires.
>
> An example where this problem might occur:
>
> HZ = 10 <=> normal timer precision = 100 ms
> min hearbeat = 180 ms
> max hearbeat = 200 ms
>
Seems to me those values are not really reasonable to start with. From a Linux
perspective, the minimum timeout will never be less than 1 second.
In the given context (HZ=10, as unlikely it might be), you might consider
setting accepted minimum to 400ms. The maximum should then be at least 1,600ms
anyway, correct ?
Thanks,
Guenter
> The div by 4 operation is here to take a safety margin by refreshing
> the hardware counter at least
> 4 times more often than needed.
>
> I'll think of something more flexible...
> If you have some ideas, please feel free to share it.
>
> >>+ }
> >>+
> >>+ if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
> >>+ err = request_irq(wdt->irq, wdt_interrupt,
> >>+ IRQF_SHARED | IRQF_IRQPOLL,
> >>+ pdev->name, wdt);
> >>+ if (err)
> >>+ return err;
> >> }
> >>- /*
> >>- * All counting occurs at SLOW_CLOCK / 128 = 256 Hz
> >>- *
> >>- * Since WDV is a 12-bit counter, the maximum period is
> >>- * 4096 / 256 = 16 seconds.
> >>- */
> >>- reg = AT91_WDT_WDRSTEN /* causes watchdog reset */
> >>- /* | AT91_WDT_WDRPROC causes processor reset only */
> >>- | AT91_WDT_WDDBGHLT /* disabled in debug mode */
> >>- | AT91_WDT_WDD /* restart at any time */
> >>- | (timeout & AT91_WDT_WDV); /* timer value */
> >>- wdt_write(AT91_WDT_MR, reg);
> >>+ if ((tmp & wdt->mr_mask) != (wdt->mr & wdt->mr_mask))
> >>+ dev_warn(wdt->wdd.parent,
> >>+ ": watchdog already configured differently (mr = %x expecting %x)\n",
> >>+ tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
> >>+
> >>+ setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
> >>+ mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
> >>+
> >>+ /* Try to set timeout from device tree first */
> >>+ if (watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev))
> >>+ watchdog_init_timeout(&wdt->wdd, heartbeat, &pdev->dev);
> >>+ watchdog_set_nowayout(&wdt->wdd, wdt->nowayout);
> >>+ err = watchdog_register_device(&wdt->wdd);
> >>+ if (err)
> >>+ goto out_stop_timer;
> >>+
> >>+ wdt->next_heartbeat = jiffies + wdt->wdd.timeout * HZ;
> >> return 0;
> >>+
> >>+out_stop_timer:
> >>+ del_timer(&wdt->timer);
> >>+ return err;
> >> }
> >> /* ......................................................................... */
> >>@@ -167,63 +236,147 @@ static const struct watchdog_info at91_wdt_info = {
> >> static const struct watchdog_ops at91_wdt_ops = {
> >> .owner = THIS_MODULE,
> >>- .start = at91_wdt_start,
> >>+ .start = at91_wdt_ping,
> >> .stop = at91_wdt_stop,
> >> .ping = at91_wdt_ping,
> >> .set_timeout = at91_wdt_set_timeout,
> >> };
> >>-static struct watchdog_device at91_wdt_dev = {
> >>- .info = &at91_wdt_info,
> >>- .ops = &at91_wdt_ops,
> >>- .timeout = WDT_HEARTBEAT,
> >>- .min_timeout = 1,
> >>- .max_timeout = 0xFFFF,
> >>-};
> >>+#if defined(CONFIG_OF)
> >>+static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
> >>+{
> >>+ u32 min = 0;
> >>+ u32 max = 0xFFF;
> >Are those default values ? If so might use defines (there are actually defines
> >for 0xfff already).
> I'll replace the 0xfff value by the XX_MAX macro.
> Should I create a new XX_MIN macro ?
> >>+ const char *tmp;
> >>+
> >>+ wdt->base = of_iomap(np, 0);
> >I am a bit at loss why you need to use use this function here.
> >
> >I think it would be more beneficial to use devm_ioremap() or even better
> >devm_ioremap_resource() in both of and non-dt cases; then you don't have
> >to call iounmap().
>
> Sure. This was one of my first driver using the devm and of_iomap
> functions, and now I realize
> I've been misusing them.
>
> I'll fix this too.
>
> >>+ if (!wdt->base) {
> >>+ dev_err(wdt->wdd.parent, "failed to map registers, aborting.\n");
> >>+ return -EINVAL;
> >>+ }
> >>+
> >>+ /* Get the interrupts property */
> >>+ wdt->irq = irq_of_parse_and_map(np, 0);
> >>+ if (!wdt->irq)
> >>+ dev_warn(wdt->wdd.parent, "failed to get IRQ from DT.\n");
> >>+
> >>+ if (!of_property_read_u32_index(np, "atmel,max-heartbeat-sec", 0,
> >>+ &max)) {
> >>+ max *= 256;
> >>+ if (max > WDT_WDV_MAX)
> >>+ max = WDT_WDV_MAX;
> >>+
> >>+ if (!of_property_read_u32_index(np, "atmel,min-heartbeat-sec",
> >>+ 0, &min)) {
> >>+ min *= 256;
> >>+ if (4 * min > max)
> >>+ min = max / 4;
> >>+ }
> >>+ }
> >I am a bit confused about those parameters. How do they match min_timeout and
> >max_timeout ?
>
> AT91 wdt block does not provide a reconfigurable timeout.
> To bypass this limitation the driver periodically reset the hw
> watchdog (based on the previous config,
> the device tree config or the static config) using a timer.
>
> The ping callback (used by watchdog core) only compute the
> next_heartbeat (expressed in system ticks)
> based on the current jiffies value (number of ticks elapsed since
> system startup) and the configured
> timeout value (configured using the set_timeout callback).
>
> To summarize:
> - min/max heartbeat timeout represent the hw timeout properties
> (those directly linked to WDD WDV
> fields).
> - min/max timeout (in watchdog_device struct) represent the
> software timeout properties (those
> exposed to the user space API).
>
> I could rework the driver to directly use the hw properties and
> remove the set_timeout functionality,
> but this would break the user space API.
>
> >>+
> >>+ wdt->mr_mask = 0x3FFFFFFF;
> >>+ wdt->mr = 0;
> >>+ if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
> >>+ !strcmp(tmp, "software")) {
> >>+ wdt->mr |= AT91_WDT_WDFIEN;
> >>+ wdt->mr_mask &= ~AT91_WDT_WDRPROC;
> >>+ } else
> >>+ wdt->mr |= AT91_WDT_WDRSTEN;
> >>+
> >>+ if (!of_property_read_string(np, "atmel,reset-type", &tmp) &&
> >>+ !strcmp(tmp, "proc"))
> >>+ wdt->mr |= AT91_WDT_WDRPROC;
> >>+
> >>+ if (of_property_read_bool(np, "atmel,disable")) {
> >>+ wdt->mr |= AT91_WDT_WDDIS;
> >>+ wdt->mr_mask &= AT91_WDT_WDDIS;
> >>+ }
> >>+
> >>+ if (of_property_read_bool(np, "atmel,idle-halt"))
> >>+ wdt->mr |= AT91_WDT_WDIDLEHLT;
> >>+
> >>+ if (of_property_read_bool(np, "atmel,dbg-halt"))
> >>+ wdt->mr |= AT91_WDT_WDDBGHLT;
> >>+
> >>+ wdt->mr |= max | ((max - min) << 16);
> >>+
> >>+ return 0;
> >>+}
> >>+#else
> >>+static inline int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
> >>+{
> >>+ return 0;
> >>+}
> >>+#endif
> >> static int __init at91wdt_probe(struct platform_device *pdev)
> >> {
> >> struct resource *r;
> >>- int res;
> >>-
> >>- r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>- if (!r)
> >>- return -ENODEV;
> >>- at91wdt_private.base = ioremap(r->start, resource_size(r));
> >>- if (!at91wdt_private.base) {
> >>- dev_err(&pdev->dev, "failed to map registers, aborting.\n");
> >>- return -ENOMEM;
> >>- }
> >>+ int err;
> >>+ struct at91wdt *wdt;
> >>- at91_wdt_dev.parent = &pdev->dev;
> >>- watchdog_init_timeout(&at91_wdt_dev, heartbeat, &pdev->dev);
> >>- watchdog_set_nowayout(&at91_wdt_dev, nowayout);
> >>+ wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> >>+ if (!wdt)
> >>+ return -ENOMEM;
> >>- /* Set watchdog */
> >>- res = at91_wdt_settimeout(ms_to_ticks(WDT_HW_TIMEOUT * 1000));
> >>- if (res)
> >>- return res;
> >>+ wdt->mr = (WDT_HW_TIMEOUT * 256) | AT91_WDT_WDRSTEN | AT91_WDT_WDD |
> >>+ AT91_WDT_WDDBGHLT | AT91_WDT_WDIDLEHLT;
> >>+ wdt->mr_mask = 0x3FFFFFFF;
> >>+ wdt->nowayout = nowayout;
> >>+ wdt->wdd.parent = &pdev->dev;
> >>+ wdt->wdd.info = &at91_wdt_info;
> >>+ wdt->wdd.ops = &at91_wdt_ops;
> >>+ wdt->wdd.timeout = WDT_HEARTBEAT;
> >>+ wdt->wdd.min_timeout = 1;
> >>+ wdt->wdd.max_timeout = 0xFFFF;
> >>+
> >>+ if (pdev->dev.of_node) {
> >>+ err = of_at91wdt_init(pdev->dev.of_node, wdt);
> >>+ if (err)
> >>+ goto out_free_watchdog;
> >>+ } else {
> >>+ r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>+ if (!r) {
> >>+ err = -ENODEV;
> >>+ goto out_free_watchdog;
> >>+ }
> >>+
> >>+ wdt->base = ioremap(r->start, resource_size(r));
> >>+ if (!wdt->base) {
> >>+ dev_err(&pdev->dev, "failed to map registers, aborting.\n");
> >>+ err = -ENOMEM;
> >>+ goto out_free_watchdog;
> >>+ }
> >>+ }
> >>- res = watchdog_register_device(&at91_wdt_dev);
> >>- if (res)
> >>- return res;
> >>+ err = at91_wdt_init(pdev, wdt);
> >>+ if (err)
> >>+ goto out_iounmap;
> >>- at91wdt_private.next_heartbeat = jiffies + at91_wdt_dev.timeout * HZ;
> >>- setup_timer(&at91wdt_private.timer, at91_ping, 0);
> >>- mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
> >>+ platform_set_drvdata(pdev, wdt);
> >> pr_info("enabled (heartbeat=%d sec, nowayout=%d)\n",
> >>- at91_wdt_dev.timeout, nowayout);
> >>+ wdt->wdd.timeout, wdt->nowayout);
> >> return 0;
> >>+
> >>+out_iounmap:
> >>+ iounmap(wdt->base);
> >>+out_free_watchdog:
> >>+ devm_kfree(&pdev->dev, wdt);
> >The whole point of using devm_kzalloc() is that you don't have to call kfree()
> >or devm_kfree(). So this call (and with it the label) is unnecessary.
> >
> >>+ return err;
> >> }
> >> static int __exit at91wdt_remove(struct platform_device *pdev)
> >> {
> >>- watchdog_unregister_device(&at91_wdt_dev);
> >>+ struct at91wdt *wdt = platform_get_drvdata(pdev);
> >>+ watchdog_unregister_device(&wdt->wdd);
> >> pr_warn("I quit now, hardware will probably reboot!\n");
> >>- del_timer(&at91wdt_private.timer);
> >>+ del_timer(&wdt->timer);
> >>+
> >>+ iounmap(wdt->base);
> >>+ devm_kfree(&pdev->dev, wdt);
> >The call to devm_kfree() is unnecessary.
> >
> >> return 0;
> >> }
>
>
On 02/10/2013 21:34, Guenter Roeck wrote:
> On Wed, Oct 02, 2013 at 09:27:03PM +0200, boris brezillon wrote:
>> Hello Guenter,
>>
>> Thanks for reviewing this patch.
>>
>> On 02/10/2013 18:12, Guenter Roeck wrote:
>>> On Fri, Jun 21, 2013 at 03:21:28PM -0000, Boris BREZILLON wrote:
>>>> The at91sam9 watchdog timer can only be configured once, and the current
>>>> implementation tries to configure it in a static way:
>>>> - 2 seconds timeout
>>>> - wdt restart every 500ms
>>>>
>>>> If the timer has already been configured with different values, it returns an
>>>> error and do not create any watchdog device.
>>>>
>>>> This is not critical if the watchdog is disabled, but if it has been enabled with
>>>> different timeout values it will lead to a SoC reset.
>>>>
>>>> This patch series tries to address this issue by adapting the heartbeat value
>>>> according the WDT timer config:
>>>> - it first tries to configure the timer as requested.
>>>> - if it fails it fallbacks to the current config, adapting its heartbeat timer
>>>> to the needs
>>>>
>>>> This patch series also move to a dynamically allocated at91wdt device instead
>>>> of the static instance.
>>>>
>>>> It adds a new at91 wdt type: software. This new type make use of the at91 wdt
>>>> interrupt to trigger a software reboot.
>>>>
>>>> Finally it adds several properties to the device tree bindings.
>>>>
>>>> Signed-off-by: Boris BREZILLON <[email protected]>
>>>>
>>>> ---
>>>> drivers/watchdog/at91sam9_wdt.c | 319 +++++++++++++++++++++++++++++----------
>>>> 1 file changed, 236 insertions(+), 83 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
>>>> index be37dde..fb47ec5 100644
>>>> --- a/drivers/watchdog/at91sam9_wdt.c
>>>> +++ b/drivers/watchdog/at91sam9_wdt.c
>>>> @@ -19,11 +19,13 @@
>>>> #include <linux/errno.h>
>>>> #include <linux/init.h>
>>>> +#include <linux/interrupt.h>
>>>> #include <linux/io.h>
>>>> #include <linux/kernel.h>
>>>> #include <linux/module.h>
>>>> #include <linux/moduleparam.h>
>>>> #include <linux/platform_device.h>
>>>> +#include <linux/reboot.h>
>>>> #include <linux/types.h>
>>>> #include <linux/watchdog.h>
>>>> #include <linux/jiffies.h>
>>>> @@ -31,23 +33,32 @@
>>>> #include <linux/bitops.h>
>>>> #include <linux/uaccess.h>
>>>> #include <linux/of.h>
>>>> +#include <linux/of_address.h>
>>>> +#include <linux/of_irq.h>
>>>> #include "at91sam9_wdt.h"
>>>> #define DRV_NAME "AT91SAM9 Watchdog"
>>>> -#define wdt_read(field) \
>>>> - __raw_readl(at91wdt_private.base + field)
>>>> -#define wdt_write(field, val) \
>>>> - __raw_writel((val), at91wdt_private.base + field)
>>>> +#define wdt_read(wdt, field) \
>>>> + __raw_readl((wdt)->base + field)
>>>> +#define wdt_write(wtd, field, val) \
>>>> + __raw_writel((val), (wdt)->base + field)
>>>> /* AT91SAM9 watchdog runs a 12bit counter @ 256Hz,
>>>> * use this to convert a watchdog
>>>> * value from/to milliseconds.
>>>> */
>>>> +#define hz_to_ticks(h) (((h << 8) / HZ) - 1)
>>> Unused.
>> I'll drop it.
>>
>>>> +#define ticks_to_hz(t) (((t + 1) * HZ) >> 8)
>>>> #define ms_to_ticks(t) (((t << 8) / 1000) - 1)
>>>> #define ticks_to_ms(t) (((t + 1) * 1000) >> 8)
>>> You should put macro parameters in () to avoid undesired side effects.
>>>
>>> (h), (t), (field)
>>>
>>> Sure, some of them are old. No reason not to fix it, though.
>> I'll fix it (I'll fix old macros in a separate patch)
>>
>>>> +#define WDT_MR_RESET 0x3FFF2FFF
>>>> +
>>>> +#define WDT_WDD_MAX 0xFFF
>>>> +#define WDT_WDV_MAX 0xFFF
>>> What are the units ?
>> ticks (slow clk cycles).
>> I'll rename these macros:
>> #define WDT_XXX_MAX_TICKS 0xFFF
>>>> +
>>>> /* Hardware timeout in seconds */
>>>> #define WDT_HW_TIMEOUT 2
>>>> @@ -66,23 +77,41 @@ module_param(nowayout, bool, 0);
>>>> MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
>>>> "(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>>>> -static struct watchdog_device at91_wdt_dev;
>>>> -static void at91_ping(unsigned long data);
>>>> -
>>>> -static struct {
>>>> +#define to_wdt(wdd) container_of(wdd, struct at91wdt, wdd)
>>>> +struct at91wdt {
>>>> + struct watchdog_device wdd;
>>>> void __iomem *base;
>>>> unsigned long next_heartbeat; /* the next_heartbeat for the timer */
>>>> struct timer_list timer; /* The timer that pings the watchdog */
>>>> -} at91wdt_private;
>>>> + u32 mr;
>>>> + u32 mr_mask;
>>>> + unsigned long heartbeat; /* WDT heartbeat in jiffies */
>>>> + bool nowayout;
>>>> + unsigned int irq;
>>>> +};
>>>> /* ......................................................................... */
>>>> +static irqreturn_t wdt_interrupt(int irq, void *dev_id)
>>>> +{
>>>> + struct at91wdt *wdt = (struct at91wdt *)dev_id;
>>>> +
>>>> + if (wdt_read(wdt, AT91_WDT_SR)) {
>>>> + /* Reboot */
>>> That is quite obvious, isn't it ?
>> I'll drop the comment line.
>>>> + pr_crit("at91sam9 WDT software reset\n");
>>>> + emergency_restart();
>>>> + pr_crit("Reboot didn't ?????\n");
>>>> + }
>>>> +
>>>> + return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> /*
>>>> * Reload the watchdog timer. (ie, pat the watchdog)
>>>> */
>>>> -static inline void at91_wdt_reset(void)
>>>> +static inline void at91_wdt_reset(struct at91wdt *wdt)
>>>> {
>>>> - wdt_write(AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
>>>> + wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
>>>> }
>>>> /*
>>>> @@ -90,26 +119,20 @@ static inline void at91_wdt_reset(void)
>>>> */
>>>> static void at91_ping(unsigned long data)
>>>> {
>>>> - if (time_before(jiffies, at91wdt_private.next_heartbeat) ||
>>>> - (!watchdog_active(&at91_wdt_dev))) {
>>>> - at91_wdt_reset();
>>>> - mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
>>>> + struct at91wdt *wdt = (struct at91wdt *)data;
>>>> + if (time_before(jiffies, wdt->next_heartbeat) ||
>>>> + (!watchdog_active(&wdt->wdd))) {
>>> Unnecessary () around !watchdog_active()
>>>
>>> [ Sure, old code. Not a reason not to clean it up ]
>> Okay, I'll remove all unnecessary "()" (even the old ones).
>>>> + at91_wdt_reset(wdt);
>>>> + mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
>>>> } else
>>>> pr_crit("I will reset your machine !\n");
>>>> }
>>>> static int at91_wdt_ping(struct watchdog_device *wdd)
>>>> {
>>>> + struct at91wdt *wdt = to_wdt(wdd);
>>>> /* calculate when the next userspace timeout will be */
>>>> - at91wdt_private.next_heartbeat = jiffies + wdd->timeout * HZ;
>>>> - return 0;
>>>> -}
>>>> -
>>>> -static int at91_wdt_start(struct watchdog_device *wdd)
>>>> -{
>>>> - /* calculate the next userspace timeout and modify the timer */
>>>> - at91_wdt_ping(wdd);
>>>> - mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
>>>> + wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
>>>> return 0;
>>>> }
>>>> @@ -125,36 +148,82 @@ static int at91_wdt_set_timeout(struct watchdog_device *wdd, unsigned int new_ti
>>>> return 0;
>>>> }
>>>> -/*
>>>> - * Set the watchdog time interval in 1/256Hz (write-once)
>>>> - * Counter is 12 bit.
>>>> - */
>>>> -static int at91_wdt_settimeout(unsigned int timeout)
>>>> +static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>>>> {
>>>> - unsigned int reg;
>>>> - unsigned int mr;
>>>> -
>>>> - /* Check if disabled */
>>>> - mr = wdt_read(AT91_WDT_MR);
>>>> - if (mr & AT91_WDT_WDDIS) {
>>>> - pr_err("sorry, watchdog is disabled\n");
>>>> - return -EIO;
>>>> + u32 tmp;
>>>> + u32 delta;
>>>> + u32 value;
>>>> + int err;
>>>> + u32 mask = wdt->mr_mask;
>>>> +
>>>> + tmp = wdt_read(wdt, AT91_WDT_MR);
>>>> + if ((tmp & mask) != (wdt->mr & mask)) {
>>>> + if (tmp == WDT_MR_RESET) {
>>>> + wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>>>> + tmp = wdt_read(wdt, AT91_WDT_MR);
>>>> + }
>>>> + }
>>>> +
>>>> + if (tmp & AT91_WDT_WDDIS) {
>>>> + if (wdt->mr & AT91_WDT_WDDIS)
>>>> + return 0;
>>>> + dev_err(wdt->wdd.parent,
>>>> + ": sorry, watchdog is disabled\n");
>>> No need to be sorry all the time. The ':' is unnecessary.
>> Sure, I'll remove the ":" and rework old and new messages (remove
>> the "sorry" word).
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + value = tmp & AT91_WDT_WDV;
>>>> + delta = (tmp & AT91_WDT_WDD) >> 16;
>>>> +
>>>> + if (delta < value) {
>>>> + if ((value / (value - delta)) < 4) {
>>> Unnecessary ( ) around left side of equation.
>>>
>>>> + dev_err(wdt->wdd.parent,
>>>> + ": sorry, max-heartbeat should be at least 4 times min-heartbeat (max-heartbeat = %d x min-heartbeat)\n",
>>>> + (value / (value - delta)));
>>> Unnecessary ( ) around the equation.
>>>
>>>> + return -EINVAL;
>>>> + }
>>>> + }
>>>> +
>>>> + wdt->heartbeat = ticks_to_hz(value / 4);
>>>> +
>>>> + if (!wdt->heartbeat) {
>>>> + dev_err(wdt->wdd.parent,
>>>> + ": sorry, linux timer (%i Hz) cannot handle watchdog timeout (%i ms)\n",
>>>> + HZ, ticks_to_ms(value));
>>>> + return -EINVAL;
>>> Isn't that a bit rude ? Why not set it to the minimum ?
I might have misunderstood your point.
What is a bit rude ?
- the fact that the minimum heartbeat timeout has to be at less or
equal to one-forth of
max heartbeat timeout
- the fact that heartbeat expressed in ticks has to be more than 0
- something else
>> We're using a normal timer (not an hrtimer), and IIRC normal timers'
>> precision depends on the
>> configured HZ option. As a result the minimum heartbeat value may
>> trigger the timer function
>> after the hardware watchdog counter expires.
>>
>> An example where this problem might occur:
>>
>> HZ = 10 <=> normal timer precision = 100 ms
>> min hearbeat = 180 ms
>> max hearbeat = 200 ms
>>
> Seems to me those values are not really reasonable to start with. From a Linux
> perspective, the minimum timeout will never be less than 1 second.
>
> In the given context (HZ=10, as unlikely it might be), you might consider
> setting accepted minimum to 400ms. The maximum should then be at least 1,600ms
> anyway, correct ?
This is correct.
> Thanks,
> Guenter
>
>> The div by 4 operation is here to take a safety margin by refreshing
>> the hardware counter at least
>> 4 times more often than needed.
>>
>> I'll think of something more flexible...
>> If you have some ideas, please feel free to share it.
>>
>>>> + }
>>>> +
>>>> + if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
>>>> + err = request_irq(wdt->irq, wdt_interrupt,
>>>> + IRQF_SHARED | IRQF_IRQPOLL,
>>>> + pdev->name, wdt);
>>>> + if (err)
>>>> + return err;
>>>> }
>>>> - /*
>>>> - * All counting occurs at SLOW_CLOCK / 128 = 256 Hz
>>>> - *
>>>> - * Since WDV is a 12-bit counter, the maximum period is
>>>> - * 4096 / 256 = 16 seconds.
>>>> - */
>>>> - reg = AT91_WDT_WDRSTEN /* causes watchdog reset */
>>>> - /* | AT91_WDT_WDRPROC causes processor reset only */
>>>> - | AT91_WDT_WDDBGHLT /* disabled in debug mode */
>>>> - | AT91_WDT_WDD /* restart at any time */
>>>> - | (timeout & AT91_WDT_WDV); /* timer value */
>>>> - wdt_write(AT91_WDT_MR, reg);
>>>> + if ((tmp & wdt->mr_mask) != (wdt->mr & wdt->mr_mask))
>>>> + dev_warn(wdt->wdd.parent,
>>>> + ": watchdog already configured differently (mr = %x expecting %x)\n",
>>>> + tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
>>>> +
>>>> + setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
>>>> + mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
>>>> +
>>>> + /* Try to set timeout from device tree first */
>>>> + if (watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev))
>>>> + watchdog_init_timeout(&wdt->wdd, heartbeat, &pdev->dev);
>>>> + watchdog_set_nowayout(&wdt->wdd, wdt->nowayout);
>>>> + err = watchdog_register_device(&wdt->wdd);
>>>> + if (err)
>>>> + goto out_stop_timer;
>>>> +
>>>> + wdt->next_heartbeat = jiffies + wdt->wdd.timeout * HZ;
>>>> return 0;
>>>> +
>>>> +out_stop_timer:
>>>> + del_timer(&wdt->timer);
>>>> + return err;
>>>> }
>>>> /* ......................................................................... */
>>>> @@ -167,63 +236,147 @@ static const struct watchdog_info at91_wdt_info = {
>>>> static const struct watchdog_ops at91_wdt_ops = {
>>>> .owner = THIS_MODULE,
>>>> - .start = at91_wdt_start,
>>>> + .start = at91_wdt_ping,
>>>> .stop = at91_wdt_stop,
>>>> .ping = at91_wdt_ping,
>>>> .set_timeout = at91_wdt_set_timeout,
>>>> };
>>>> -static struct watchdog_device at91_wdt_dev = {
>>>> - .info = &at91_wdt_info,
>>>> - .ops = &at91_wdt_ops,
>>>> - .timeout = WDT_HEARTBEAT,
>>>> - .min_timeout = 1,
>>>> - .max_timeout = 0xFFFF,
>>>> -};
>>>> +#if defined(CONFIG_OF)
>>>> +static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
>>>> +{
>>>> + u32 min = 0;
>>>> + u32 max = 0xFFF;
>>> Are those default values ? If so might use defines (there are actually defines
>>> for 0xfff already).
>> I'll replace the 0xfff value by the XX_MAX macro.
>> Should I create a new XX_MIN macro ?
>>>> + const char *tmp;
>>>> +
>>>> + wdt->base = of_iomap(np, 0);
>>> I am a bit at loss why you need to use use this function here.
>>>
>>> I think it would be more beneficial to use devm_ioremap() or even better
>>> devm_ioremap_resource() in both of and non-dt cases; then you don't have
>>> to call iounmap().
>> Sure. This was one of my first driver using the devm and of_iomap
>> functions, and now I realize
>> I've been misusing them.
>>
>> I'll fix this too.
>>
>>>> + if (!wdt->base) {
>>>> + dev_err(wdt->wdd.parent, "failed to map registers, aborting.\n");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + /* Get the interrupts property */
>>>> + wdt->irq = irq_of_parse_and_map(np, 0);
>>>> + if (!wdt->irq)
>>>> + dev_warn(wdt->wdd.parent, "failed to get IRQ from DT.\n");
>>>> +
>>>> + if (!of_property_read_u32_index(np, "atmel,max-heartbeat-sec", 0,
>>>> + &max)) {
>>>> + max *= 256;
>>>> + if (max > WDT_WDV_MAX)
>>>> + max = WDT_WDV_MAX;
>>>> +
>>>> + if (!of_property_read_u32_index(np, "atmel,min-heartbeat-sec",
>>>> + 0, &min)) {
>>>> + min *= 256;
>>>> + if (4 * min > max)
>>>> + min = max / 4;
>>>> + }
>>>> + }
>>> I am a bit confused about those parameters. How do they match min_timeout and
>>> max_timeout ?
>> AT91 wdt block does not provide a reconfigurable timeout.
>> To bypass this limitation the driver periodically reset the hw
>> watchdog (based on the previous config,
>> the device tree config or the static config) using a timer.
>>
>> The ping callback (used by watchdog core) only compute the
>> next_heartbeat (expressed in system ticks)
>> based on the current jiffies value (number of ticks elapsed since
>> system startup) and the configured
>> timeout value (configured using the set_timeout callback).
>>
>> To summarize:
>> - min/max heartbeat timeout represent the hw timeout properties
>> (those directly linked to WDD WDV
>> fields).
>> - min/max timeout (in watchdog_device struct) represent the
>> software timeout properties (those
>> exposed to the user space API).
>>
>> I could rework the driver to directly use the hw properties and
>> remove the set_timeout functionality,
>> but this would break the user space API.
>>
>>>> +
>>>> + wdt->mr_mask = 0x3FFFFFFF;
>>>> + wdt->mr = 0;
>>>> + if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
>>>> + !strcmp(tmp, "software")) {
>>>> + wdt->mr |= AT91_WDT_WDFIEN;
>>>> + wdt->mr_mask &= ~AT91_WDT_WDRPROC;
>>>> + } else
>>>> + wdt->mr |= AT91_WDT_WDRSTEN;
>>>> +
>>>> + if (!of_property_read_string(np, "atmel,reset-type", &tmp) &&
>>>> + !strcmp(tmp, "proc"))
>>>> + wdt->mr |= AT91_WDT_WDRPROC;
>>>> +
>>>> + if (of_property_read_bool(np, "atmel,disable")) {
>>>> + wdt->mr |= AT91_WDT_WDDIS;
>>>> + wdt->mr_mask &= AT91_WDT_WDDIS;
>>>> + }
>>>> +
>>>> + if (of_property_read_bool(np, "atmel,idle-halt"))
>>>> + wdt->mr |= AT91_WDT_WDIDLEHLT;
>>>> +
>>>> + if (of_property_read_bool(np, "atmel,dbg-halt"))
>>>> + wdt->mr |= AT91_WDT_WDDBGHLT;
>>>> +
>>>> + wdt->mr |= max | ((max - min) << 16);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +#else
>>>> +static inline int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +#endif
>>>> static int __init at91wdt_probe(struct platform_device *pdev)
>>>> {
>>>> struct resource *r;
>>>> - int res;
>>>> -
>>>> - r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> - if (!r)
>>>> - return -ENODEV;
>>>> - at91wdt_private.base = ioremap(r->start, resource_size(r));
>>>> - if (!at91wdt_private.base) {
>>>> - dev_err(&pdev->dev, "failed to map registers, aborting.\n");
>>>> - return -ENOMEM;
>>>> - }
>>>> + int err;
>>>> + struct at91wdt *wdt;
>>>> - at91_wdt_dev.parent = &pdev->dev;
>>>> - watchdog_init_timeout(&at91_wdt_dev, heartbeat, &pdev->dev);
>>>> - watchdog_set_nowayout(&at91_wdt_dev, nowayout);
>>>> + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
>>>> + if (!wdt)
>>>> + return -ENOMEM;
>>>> - /* Set watchdog */
>>>> - res = at91_wdt_settimeout(ms_to_ticks(WDT_HW_TIMEOUT * 1000));
>>>> - if (res)
>>>> - return res;
>>>> + wdt->mr = (WDT_HW_TIMEOUT * 256) | AT91_WDT_WDRSTEN | AT91_WDT_WDD |
>>>> + AT91_WDT_WDDBGHLT | AT91_WDT_WDIDLEHLT;
>>>> + wdt->mr_mask = 0x3FFFFFFF;
>>>> + wdt->nowayout = nowayout;
>>>> + wdt->wdd.parent = &pdev->dev;
>>>> + wdt->wdd.info = &at91_wdt_info;
>>>> + wdt->wdd.ops = &at91_wdt_ops;
>>>> + wdt->wdd.timeout = WDT_HEARTBEAT;
>>>> + wdt->wdd.min_timeout = 1;
>>>> + wdt->wdd.max_timeout = 0xFFFF;
>>>> +
>>>> + if (pdev->dev.of_node) {
>>>> + err = of_at91wdt_init(pdev->dev.of_node, wdt);
>>>> + if (err)
>>>> + goto out_free_watchdog;
>>>> + } else {
>>>> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> + if (!r) {
>>>> + err = -ENODEV;
>>>> + goto out_free_watchdog;
>>>> + }
>>>> +
>>>> + wdt->base = ioremap(r->start, resource_size(r));
>>>> + if (!wdt->base) {
>>>> + dev_err(&pdev->dev, "failed to map registers, aborting.\n");
>>>> + err = -ENOMEM;
>>>> + goto out_free_watchdog;
>>>> + }
>>>> + }
>>>> - res = watchdog_register_device(&at91_wdt_dev);
>>>> - if (res)
>>>> - return res;
>>>> + err = at91_wdt_init(pdev, wdt);
>>>> + if (err)
>>>> + goto out_iounmap;
>>>> - at91wdt_private.next_heartbeat = jiffies + at91_wdt_dev.timeout * HZ;
>>>> - setup_timer(&at91wdt_private.timer, at91_ping, 0);
>>>> - mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
>>>> + platform_set_drvdata(pdev, wdt);
>>>> pr_info("enabled (heartbeat=%d sec, nowayout=%d)\n",
>>>> - at91_wdt_dev.timeout, nowayout);
>>>> + wdt->wdd.timeout, wdt->nowayout);
>>>> return 0;
>>>> +
>>>> +out_iounmap:
>>>> + iounmap(wdt->base);
>>>> +out_free_watchdog:
>>>> + devm_kfree(&pdev->dev, wdt);
>>> The whole point of using devm_kzalloc() is that you don't have to call kfree()
>>> or devm_kfree(). So this call (and with it the label) is unnecessary.
>>>
>>>> + return err;
>>>> }
>>>> static int __exit at91wdt_remove(struct platform_device *pdev)
>>>> {
>>>> - watchdog_unregister_device(&at91_wdt_dev);
>>>> + struct at91wdt *wdt = platform_get_drvdata(pdev);
>>>> + watchdog_unregister_device(&wdt->wdd);
>>>> pr_warn("I quit now, hardware will probably reboot!\n");
>>>> - del_timer(&at91wdt_private.timer);
>>>> + del_timer(&wdt->timer);
>>>> +
>>>> + iounmap(wdt->base);
>>>> + devm_kfree(&pdev->dev, wdt);
>>> The call to devm_kfree() is unnecessary.
>>>
>>>> return 0;
>>>> }
>>
On Wed, Oct 02, 2013 at 09:53:54PM +0200, boris brezillon wrote:
[ ... ]
> >>>>+
> >>>>+ if (!wdt->heartbeat) {
> >>>>+ dev_err(wdt->wdd.parent,
> >>>>+ ": sorry, linux timer (%i Hz) cannot handle watchdog timeout (%i ms)\n",
> >>>>+ HZ, ticks_to_ms(value));
> >>>>+ return -EINVAL;
> >>>Isn't that a bit rude ? Why not set it to the minimum ?
>
> I might have misunderstood your point.
> What is a bit rude ?
> - the fact that the minimum heartbeat timeout has to be at less or
> equal to one-forth of
> max heartbeat timeout
> - the fact that heartbeat expressed in ticks has to be more than 0
> - something else
That you don't auto-correct the heatbeat to the minimum but return -EINVAL instead.
I prefer to be user-friendly, which in this case would be to accept and handle
the <min, max> timeout values provided to the infrastructure and handle any
deviations / limitaions internally.
Or, in other words, I don't like it if the user ends up having to guess
valid parameter ranges.
Thanks,
Guenter
On Fri, Jun 21, 2013 at 9:23 AM, Boris BREZILLON
<[email protected]> wrote:
> Add new at91sam9 watchdog properties to the documentation.
>
> Signed-off-by: Boris BREZILLON <[email protected]>
> ---
> .../devicetree/bindings/watchdog/atmel-wdt.txt | 30 ++++++++++++++++++--
> 1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
> index fcdd48f..e043106 100644
> --- a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
> @@ -9,11 +9,37 @@ Required properties:
>
> Optional properties:
> - timeout-sec: contains the watchdog timeout in seconds.
Why are you removing the documentation about this property?
Regards
--
Fabio Porcedda
On Thu, Oct 3, 2013 at 10:23 AM, boris brezillon
<[email protected]> wrote:
> Hello Fabio,
>
>
> On 03/10/2013 10:08, Fabio Porcedda wrote:
>>
>> On Fri, Jun 21, 2013 at 9:23 AM, Boris BREZILLON
>> <[email protected]> wrote:
>>>
>>> Add new at91sam9 watchdog properties to the documentation.
>>>
>>> Signed-off-by: Boris BREZILLON <[email protected]>
>>> ---
>>> .../devicetree/bindings/watchdog/atmel-wdt.txt | 30
>>> ++++++++++++++++++--
>>> 1 file changed, 28 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
>>> b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
>>> index fcdd48f..e043106 100644
>>> --- a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
>>> +++ b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
>>> @@ -9,11 +9,37 @@ Required properties:
>>>
>>> Optional properties:
>>> - timeout-sec: contains the watchdog timeout in seconds.
>>
>> Why are you removing the documentation about this property?
>
>
> I'm not removing the property: this is an enumeration minus not a diff
> minus.
Good, sorry for the noisy.
Best regards
--
Fabio Porcedda
Hello Fabio,
On 03/10/2013 10:08, Fabio Porcedda wrote:
> On Fri, Jun 21, 2013 at 9:23 AM, Boris BREZILLON
> <[email protected]> wrote:
>> Add new at91sam9 watchdog properties to the documentation.
>>
>> Signed-off-by: Boris BREZILLON <[email protected]>
>> ---
>> .../devicetree/bindings/watchdog/atmel-wdt.txt | 30 ++++++++++++++++++--
>> 1 file changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
>> index fcdd48f..e043106 100644
>> --- a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
>> +++ b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
>> @@ -9,11 +9,37 @@ Required properties:
>>
>> Optional properties:
>> - timeout-sec: contains the watchdog timeout in seconds.
> Why are you removing the documentation about this property?
I'm not removing the property: this is an enumeration minus not a diff
minus.
> Regards
Regards,
Boris