Hi,
This is a rework of how the watchdog is getting programmed. Currently,
there are multiple issue that have the same symptoms: the watchdog is
unexpectidly resetting the SoCs when configuring it.
The first issue was how WDDIS was handled. To sum it up, the watchdog
has to be configured when removing WDDIS instead of first removing it
then configuring it. It is solved by only configuring the IP when
enabling the watchdog.
Note that there were no issue updating the timeout with the watchdog
running.
The second issue is how the write are synchronized inside the IP. The
datasheet state that iit is necessary to wait 3 slow clock periods
between a write to CR and subsequents writes to CR and MR. This was not
done in the driver. Also, it apears it is necessary to wait the same
amount of time between a write to MR and a write to CR or MR.
Finally, a simplification of the probe is done and a comment is added in
the resume function to explain why the reset may be delayed after a
suspend/resume cycle (but it will still happen).
Before the series, the watchdog would reset the SoC after a few
configurations. Now, I've lett a test progam run that managed to do more
than 1 000 000 configurations with the watchdog enabled and the same
while disabled.
Alexandre Belloni (4):
watchdog: sama5d4: fix WDDIS handling
watchdog: sama5d4: fix race condition
watchodg: sama5d4: simplify probe
watchdog: sama5d4: Add comment explaining what happens on resume
drivers/watchdog/sama5d4_wdt.c | 96 ++++++++++++++++++++++++++++++------------
1 file changed, 68 insertions(+), 28 deletions(-)
--
2.11.0
On 02/03/2017 at 09:42:24 -0800, Guenter Roeck wrote:
> On Thu, Mar 02, 2017 at 06:31:12PM +0100, Alexandre Belloni wrote:
> > WDT_MR and WDT_CR must not updated within three slow clock periods after
> > the last ping (write to WDT_CR or WDT_MR). Ensure enough time has elapsed
> > before writing those registers.
> > wdt_write() waits for 4 periods to ensure at least 3 edges are seen by the
> > IP.
> >
>
> Would it be possible to use min_hw_heartbeat_ms for this purpose ?
>
I don't think so unless you want the core to also use that delay between
__watchdog_ping, watchdog_start, watchdog_set_timeout and watchdog_stop.
Currently, it is only between two calls of __watchdog_ping,
> Thanks,
> Guenter
>
> > Signed-off-by: Alexandre Belloni <[email protected]>
> > ---
> > drivers/watchdog/sama5d4_wdt.c | 33 +++++++++++++++++++++++++++++----
> > 1 file changed, 29 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> > index 5cee20caca78..362fd229786d 100644
> > --- a/drivers/watchdog/sama5d4_wdt.c
> > +++ b/drivers/watchdog/sama5d4_wdt.c
> > @@ -6,6 +6,7 @@
> > * Licensed under GPLv2.
> > */
> >
> > +#include <linux/delay.h>
> > #include <linux/interrupt.h>
> > #include <linux/io.h>
> > #include <linux/kernel.h>
> > @@ -29,6 +30,7 @@ struct sama5d4_wdt {
> > struct watchdog_device wdd;
> > void __iomem *reg_base;
> > u32 mr;
> > + unsigned long last_ping;
> > };
> >
> > static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
> > @@ -49,8 +51,29 @@ MODULE_PARM_DESC(nowayout,
> > #define wdt_read(wdt, field) \
> > readl_relaxed((wdt)->reg_base + (field))
> >
> > -#define wdt_write(wtd, field, val) \
> > - writel_relaxed((val), (wdt)->reg_base + (field))
> > +/* 4 slow clock periods is 4/32768 = 122.07?s*/
> > +#define WDT_DELAY usecs_to_jiffies(123)
> > +
> > +static void wdt_write(struct sama5d4_wdt *wdt, u32 field, u32 val)
> > +{
> > + /*
> > + * WDT_CR and WDT_MR must not be modified within three slow clock
> > + * periods following a restart of the watchdog performed by a write
> > + * access in WDT_CR.
> > + */
> > + while (time_before(jiffies, wdt->last_ping + WDT_DELAY))
> > + usleep_range(30, 125);
> > + writel_relaxed(val, wdt->reg_base + field);
> > + wdt->last_ping = jiffies;
> > +}
> > +
> > +static void wdt_write_nosleep(struct sama5d4_wdt *wdt, u32 field, u32 val)
> > +{
> > + if (time_before(jiffies, wdt->last_ping + WDT_DELAY))
> > + udelay(123);
> > + writel_relaxed(val, wdt->reg_base + field);
> > + wdt->last_ping = jiffies;
> > +}
> >
> > static int sama5d4_wdt_start(struct watchdog_device *wdd)
> > {
> > @@ -164,11 +187,12 @@ static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
> > * Else, we have to disable it properly.
> > */
> > if (wdt_enabled) {
> > - wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> > + wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
> > } else {
> > reg = wdt_read(wdt, AT91_WDT_MR);
> > if (!(reg & AT91_WDT_WDDIS))
> > - wdt_write(wdt, AT91_WDT_MR, reg | AT91_WDT_WDDIS);
> > + wdt_write_nosleep(wdt, AT91_WDT_MR,
> > + reg | AT91_WDT_WDDIS);
> > }
> > return 0;
> > }
> > @@ -193,6 +217,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
> > wdd->ops = &sama5d4_wdt_ops;
> > wdd->min_timeout = MIN_WDT_TIMEOUT;
> > wdd->max_timeout = MAX_WDT_TIMEOUT;
> > + wdt->last_ping = jiffies;
> >
> > watchdog_set_drvdata(wdd, wdt);
> >
> > --
> > 2.11.0
> >
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Because the only way to use the driver is to have a device tree enabling
it, pdev->dev.of_node will never be NULL. Remove the unnecessary check.
Signed-off-by: Alexandre Belloni <[email protected]>
---
drivers/watchdog/sama5d4_wdt.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
index 362fd229786d..d710014f3b7d 100644
--- a/drivers/watchdog/sama5d4_wdt.c
+++ b/drivers/watchdog/sama5d4_wdt.c
@@ -228,15 +228,13 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
wdt->reg_base = regs;
- if (pdev->dev.of_node) {
- irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
- if (!irq)
- dev_warn(&pdev->dev, "failed to get IRQ from DT\n");
+ irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
+ if (!irq)
+ dev_warn(&pdev->dev, "failed to get IRQ from DT\n");
- ret = of_sama5d4_wdt_init(pdev->dev.of_node, wdt);
- if (ret)
- return ret;
- }
+ ret = of_sama5d4_wdt_init(pdev->dev.of_node, wdt);
+ if (ret)
+ return ret;
if ((wdt->mr & AT91_WDT_WDFIEN) && irq) {
ret = devm_request_irq(&pdev->dev, irq, sama5d4_wdt_irq_handler,
--
2.11.0
On Thu, Mar 02, 2017 at 06:31:12PM +0100, Alexandre Belloni wrote:
> WDT_MR and WDT_CR must not updated within three slow clock periods after
> the last ping (write to WDT_CR or WDT_MR). Ensure enough time has elapsed
> before writing those registers.
> wdt_write() waits for 4 periods to ensure at least 3 edges are seen by the
> IP.
>
Would it be possible to use min_hw_heartbeat_ms for this purpose ?
Thanks,
Guenter
> Signed-off-by: Alexandre Belloni <[email protected]>
> ---
> drivers/watchdog/sama5d4_wdt.c | 33 +++++++++++++++++++++++++++++----
> 1 file changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 5cee20caca78..362fd229786d 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -6,6 +6,7 @@
> * Licensed under GPLv2.
> */
>
> +#include <linux/delay.h>
> #include <linux/interrupt.h>
> #include <linux/io.h>
> #include <linux/kernel.h>
> @@ -29,6 +30,7 @@ struct sama5d4_wdt {
> struct watchdog_device wdd;
> void __iomem *reg_base;
> u32 mr;
> + unsigned long last_ping;
> };
>
> static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
> @@ -49,8 +51,29 @@ MODULE_PARM_DESC(nowayout,
> #define wdt_read(wdt, field) \
> readl_relaxed((wdt)->reg_base + (field))
>
> -#define wdt_write(wtd, field, val) \
> - writel_relaxed((val), (wdt)->reg_base + (field))
> +/* 4 slow clock periods is 4/32768 = 122.07?s*/
> +#define WDT_DELAY usecs_to_jiffies(123)
> +
> +static void wdt_write(struct sama5d4_wdt *wdt, u32 field, u32 val)
> +{
> + /*
> + * WDT_CR and WDT_MR must not be modified within three slow clock
> + * periods following a restart of the watchdog performed by a write
> + * access in WDT_CR.
> + */
> + while (time_before(jiffies, wdt->last_ping + WDT_DELAY))
> + usleep_range(30, 125);
> + writel_relaxed(val, wdt->reg_base + field);
> + wdt->last_ping = jiffies;
> +}
> +
> +static void wdt_write_nosleep(struct sama5d4_wdt *wdt, u32 field, u32 val)
> +{
> + if (time_before(jiffies, wdt->last_ping + WDT_DELAY))
> + udelay(123);
> + writel_relaxed(val, wdt->reg_base + field);
> + wdt->last_ping = jiffies;
> +}
>
> static int sama5d4_wdt_start(struct watchdog_device *wdd)
> {
> @@ -164,11 +187,12 @@ static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
> * Else, we have to disable it properly.
> */
> if (wdt_enabled) {
> - wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> + wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
> } else {
> reg = wdt_read(wdt, AT91_WDT_MR);
> if (!(reg & AT91_WDT_WDDIS))
> - wdt_write(wdt, AT91_WDT_MR, reg | AT91_WDT_WDDIS);
> + wdt_write_nosleep(wdt, AT91_WDT_MR,
> + reg | AT91_WDT_WDDIS);
> }
> return 0;
> }
> @@ -193,6 +217,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
> wdd->ops = &sama5d4_wdt_ops;
> wdd->min_timeout = MIN_WDT_TIMEOUT;
> wdd->max_timeout = MAX_WDT_TIMEOUT;
> + wdt->last_ping = jiffies;
>
> watchdog_set_drvdata(wdd, wdt);
>
> --
> 2.11.0
>
Hei hei,
there's a typo in the subject you may want to fix in a v2:
watchodg → watchdog
Greets
Alex
--
»With the first link, the chain is forged. The first speech censured,
the first thought forbidden, the first freedom denied, chains us all
irrevocably.« (Jean-Luc Picard, quoting Judge Aaron Satie)
*** GnuPG-FP: C28E E6B9 0263 95CF 8FAF 08FA 34AD CD00 7221 5CC6 ***
The datasheet states: "When setting the WDDIS bit, and while it is set, the
fields WDV and WDD must not be modified."
Because the whole configuration is already cached inside .mr, wait for the
user to enable the watchdog to configure it so it is enabled and configured
at the same time (what the IP is actually expecting).
When the watchdog is already enabled, it is not an issue to reconfigure it.
Signed-off-by: Alexandre Belloni <[email protected]>
---
drivers/watchdog/sama5d4_wdt.c | 48 ++++++++++++++++++++++++++----------------
1 file changed, 30 insertions(+), 18 deletions(-)
diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
index f709962018ac..5cee20caca78 100644
--- a/drivers/watchdog/sama5d4_wdt.c
+++ b/drivers/watchdog/sama5d4_wdt.c
@@ -44,6 +44,8 @@ MODULE_PARM_DESC(nowayout,
"Watchdog cannot be stopped once started (default="
__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+#define wdt_enabled (!(wdt->mr & AT91_WDT_WDDIS))
+
#define wdt_read(wdt, field) \
readl_relaxed((wdt)->reg_base + (field))
@@ -89,7 +91,16 @@ static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd,
wdt->mr &= ~AT91_WDT_WDD;
wdt->mr |= AT91_WDT_SET_WDV(value);
wdt->mr |= AT91_WDT_SET_WDD(value);
- wdt_write(wdt, AT91_WDT_MR, wdt->mr);
+
+ /*
+ * WDDIS has to be 0 when updating WDD/WDV. The datasheet states: When
+ * setting the WDDIS bit, and while it is set, the fields WDV and WDD
+ * must not be modified.
+ * If the watchdog is enabled, then the timeout can be updated. Else,
+ * wait that the user enables it.
+ */
+ if (wdt_enabled)
+ wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
wdd->timeout = timeout;
@@ -145,23 +156,20 @@ static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt)
static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
{
- struct watchdog_device *wdd = &wdt->wdd;
- u32 value = WDT_SEC2TICKS(wdd->timeout);
u32 reg;
-
/*
- * Because the fields WDV and WDD must not be modified when the WDDIS
- * bit is set, so clear the WDDIS bit before writing the WDT_MR.
+ * When booting and resuming, the bootloader may have changed the
+ * watchdog configuration.
+ * If the watchdog is already running, we can safely update it.
+ * Else, we have to disable it properly.
*/
- reg = wdt_read(wdt, AT91_WDT_MR);
- reg &= ~AT91_WDT_WDDIS;
- wdt_write(wdt, AT91_WDT_MR, reg);
-
- wdt->mr |= AT91_WDT_SET_WDD(value);
- wdt->mr |= AT91_WDT_SET_WDV(value);
-
- wdt_write(wdt, AT91_WDT_MR, wdt->mr);
-
+ if (wdt_enabled) {
+ wdt_write(wdt, AT91_WDT_MR, wdt->mr);
+ } else {
+ reg = wdt_read(wdt, AT91_WDT_MR);
+ if (!(reg & AT91_WDT_WDDIS))
+ wdt_write(wdt, AT91_WDT_MR, reg | AT91_WDT_WDDIS);
+ }
return 0;
}
@@ -172,6 +180,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
struct resource *res;
void __iomem *regs;
u32 irq = 0;
+ u32 timeout;
int ret;
wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
@@ -221,6 +230,11 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
return ret;
}
+ timeout = WDT_SEC2TICKS(wdd->timeout);
+
+ wdt->mr |= AT91_WDT_SET_WDD(timeout);
+ wdt->mr |= AT91_WDT_SET_WDV(timeout);
+
ret = sama5d4_wdt_init(wdt);
if (ret)
return ret;
@@ -263,9 +277,7 @@ static int sama5d4_wdt_resume(struct device *dev)
{
struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
- wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
- if (wdt->mr & AT91_WDT_WDDIS)
- wdt_write(wdt, AT91_WDT_MR, wdt->mr);
+ sama5d4_wdt_init(wdt);
return 0;
}
--
2.11.0
WDT_MR and WDT_CR must not updated within three slow clock periods after
the last ping (write to WDT_CR or WDT_MR). Ensure enough time has elapsed
before writing those registers.
wdt_write() waits for 4 periods to ensure at least 3 edges are seen by the
IP.
Signed-off-by: Alexandre Belloni <[email protected]>
---
drivers/watchdog/sama5d4_wdt.c | 33 +++++++++++++++++++++++++++++----
1 file changed, 29 insertions(+), 4 deletions(-)
diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
index 5cee20caca78..362fd229786d 100644
--- a/drivers/watchdog/sama5d4_wdt.c
+++ b/drivers/watchdog/sama5d4_wdt.c
@@ -6,6 +6,7 @@
* Licensed under GPLv2.
*/
+#include <linux/delay.h>
#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/kernel.h>
@@ -29,6 +30,7 @@ struct sama5d4_wdt {
struct watchdog_device wdd;
void __iomem *reg_base;
u32 mr;
+ unsigned long last_ping;
};
static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
@@ -49,8 +51,29 @@ MODULE_PARM_DESC(nowayout,
#define wdt_read(wdt, field) \
readl_relaxed((wdt)->reg_base + (field))
-#define wdt_write(wtd, field, val) \
- writel_relaxed((val), (wdt)->reg_base + (field))
+/* 4 slow clock periods is 4/32768 = 122.07µs*/
+#define WDT_DELAY usecs_to_jiffies(123)
+
+static void wdt_write(struct sama5d4_wdt *wdt, u32 field, u32 val)
+{
+ /*
+ * WDT_CR and WDT_MR must not be modified within three slow clock
+ * periods following a restart of the watchdog performed by a write
+ * access in WDT_CR.
+ */
+ while (time_before(jiffies, wdt->last_ping + WDT_DELAY))
+ usleep_range(30, 125);
+ writel_relaxed(val, wdt->reg_base + field);
+ wdt->last_ping = jiffies;
+}
+
+static void wdt_write_nosleep(struct sama5d4_wdt *wdt, u32 field, u32 val)
+{
+ if (time_before(jiffies, wdt->last_ping + WDT_DELAY))
+ udelay(123);
+ writel_relaxed(val, wdt->reg_base + field);
+ wdt->last_ping = jiffies;
+}
static int sama5d4_wdt_start(struct watchdog_device *wdd)
{
@@ -164,11 +187,12 @@ static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
* Else, we have to disable it properly.
*/
if (wdt_enabled) {
- wdt_write(wdt, AT91_WDT_MR, wdt->mr);
+ wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
} else {
reg = wdt_read(wdt, AT91_WDT_MR);
if (!(reg & AT91_WDT_WDDIS))
- wdt_write(wdt, AT91_WDT_MR, reg | AT91_WDT_WDDIS);
+ wdt_write_nosleep(wdt, AT91_WDT_MR,
+ reg | AT91_WDT_WDDIS);
}
return 0;
}
@@ -193,6 +217,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
wdd->ops = &sama5d4_wdt_ops;
wdd->min_timeout = MIN_WDT_TIMEOUT;
wdd->max_timeout = MAX_WDT_TIMEOUT;
+ wdt->last_ping = jiffies;
watchdog_set_drvdata(wdd, wdt);
--
2.11.0
Because suspending to RAM may lose the register values, they are restored
on resume. This is currently done unconditionally because there is
currently no way to know (from the driver) whether they have really been
lost or are still valid. Writing MR also pings the watchdog and this may
not be what is expected so add a comment explaining why it happens.
Signed-off-by: Alexandre Belloni <[email protected]>
---
drivers/watchdog/sama5d4_wdt.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
index d710014f3b7d..0ae947c3d7bc 100644
--- a/drivers/watchdog/sama5d4_wdt.c
+++ b/drivers/watchdog/sama5d4_wdt.c
@@ -300,6 +300,11 @@ static int sama5d4_wdt_resume(struct device *dev)
{
struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
+ /*
+ * FIXME: writing MR also pings the watchdog which may not be desired.
+ * This should only be done when the registers are lost on suspend but
+ * there is no way to get this information right now.
+ */
sama5d4_wdt_init(wdt);
return 0;
--
2.11.0
Hello,
On Thu, 2 Mar 2017 18:31:13 +0100, Alexandre Belloni wrote:
> + irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
Any reason to use irq_of_parse_and_map() over the more conventional
platform_get_irq() ?
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On 03/03/2017 at 09:03:25 +0100, Thomas Petazzoni wrote:
> On Thu, 2 Mar 2017 18:31:13 +0100, Alexandre Belloni wrote:
>
> > + irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
>
> Any reason to use irq_of_parse_and_map() over the more conventional
> platform_get_irq() ?
>
No particular reason but I'm just removing the if (pdev->dev.of_node)
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On 03/03/2017 01:29 AM, Alexandre Belloni wrote:
> On 03/03/2017 at 09:03:25 +0100, Thomas Petazzoni wrote:
>> On Thu, 2 Mar 2017 18:31:13 +0100, Alexandre Belloni wrote:
>>
>>> + irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
>>
>> Any reason to use irq_of_parse_and_map() over the more conventional
>> platform_get_irq() ?
>>
>
> No particular reason but I'm just removing the if (pdev->dev.of_node)
>
A function call change would (should) be a separate patch.
Guenter
On 03/02/2017 09:31 AM, Alexandre Belloni wrote:
> WDT_MR and WDT_CR must not updated within three slow clock periods after
> the last ping (write to WDT_CR or WDT_MR). Ensure enough time has elapsed
> before writing those registers.
> wdt_write() waits for 4 periods to ensure at least 3 edges are seen by the
> IP.
>
> Signed-off-by: Alexandre Belloni <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> drivers/watchdog/sama5d4_wdt.c | 33 +++++++++++++++++++++++++++++----
> 1 file changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 5cee20caca78..362fd229786d 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -6,6 +6,7 @@
> * Licensed under GPLv2.
> */
>
> +#include <linux/delay.h>
> #include <linux/interrupt.h>
> #include <linux/io.h>
> #include <linux/kernel.h>
> @@ -29,6 +30,7 @@ struct sama5d4_wdt {
> struct watchdog_device wdd;
> void __iomem *reg_base;
> u32 mr;
> + unsigned long last_ping;
> };
>
> static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
> @@ -49,8 +51,29 @@ MODULE_PARM_DESC(nowayout,
> #define wdt_read(wdt, field) \
> readl_relaxed((wdt)->reg_base + (field))
>
> -#define wdt_write(wtd, field, val) \
> - writel_relaxed((val), (wdt)->reg_base + (field))
> +/* 4 slow clock periods is 4/32768 = 122.07µs*/
> +#define WDT_DELAY usecs_to_jiffies(123)
> +
> +static void wdt_write(struct sama5d4_wdt *wdt, u32 field, u32 val)
> +{
> + /*
> + * WDT_CR and WDT_MR must not be modified within three slow clock
> + * periods following a restart of the watchdog performed by a write
> + * access in WDT_CR.
> + */
> + while (time_before(jiffies, wdt->last_ping + WDT_DELAY))
> + usleep_range(30, 125);
> + writel_relaxed(val, wdt->reg_base + field);
> + wdt->last_ping = jiffies;
> +}
> +
> +static void wdt_write_nosleep(struct sama5d4_wdt *wdt, u32 field, u32 val)
> +{
> + if (time_before(jiffies, wdt->last_ping + WDT_DELAY))
> + udelay(123);
> + writel_relaxed(val, wdt->reg_base + field);
> + wdt->last_ping = jiffies;
> +}
>
> static int sama5d4_wdt_start(struct watchdog_device *wdd)
> {
> @@ -164,11 +187,12 @@ static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
> * Else, we have to disable it properly.
> */
> if (wdt_enabled) {
> - wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> + wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
> } else {
> reg = wdt_read(wdt, AT91_WDT_MR);
> if (!(reg & AT91_WDT_WDDIS))
> - wdt_write(wdt, AT91_WDT_MR, reg | AT91_WDT_WDDIS);
> + wdt_write_nosleep(wdt, AT91_WDT_MR,
> + reg | AT91_WDT_WDDIS);
> }
> return 0;
> }
> @@ -193,6 +217,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
> wdd->ops = &sama5d4_wdt_ops;
> wdd->min_timeout = MIN_WDT_TIMEOUT;
> wdd->max_timeout = MAX_WDT_TIMEOUT;
> + wdt->last_ping = jiffies;
>
> watchdog_set_drvdata(wdd, wdt);
>
>
On 03/02/2017 09:31 AM, Alexandre Belloni wrote:
> Because the only way to use the driver is to have a device tree enabling
> it, pdev->dev.of_node will never be NULL. Remove the unnecessary check.
>
> Signed-off-by: Alexandre Belloni <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> drivers/watchdog/sama5d4_wdt.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 362fd229786d..d710014f3b7d 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -228,15 +228,13 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
>
> wdt->reg_base = regs;
>
> - if (pdev->dev.of_node) {
> - irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> - if (!irq)
> - dev_warn(&pdev->dev, "failed to get IRQ from DT\n");
> + irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> + if (!irq)
> + dev_warn(&pdev->dev, "failed to get IRQ from DT\n");
>
> - ret = of_sama5d4_wdt_init(pdev->dev.of_node, wdt);
> - if (ret)
> - return ret;
> - }
> + ret = of_sama5d4_wdt_init(pdev->dev.of_node, wdt);
> + if (ret)
> + return ret;
>
> if ((wdt->mr & AT91_WDT_WDFIEN) && irq) {
> ret = devm_request_irq(&pdev->dev, irq, sama5d4_wdt_irq_handler,
>
On 03/02/2017 09:31 AM, Alexandre Belloni wrote:
> Because suspending to RAM may lose the register values, they are restored
> on resume. This is currently done unconditionally because there is
> currently no way to know (from the driver) whether they have really been
> lost or are still valid. Writing MR also pings the watchdog and this may
> not be what is expected so add a comment explaining why it happens.
>
> Signed-off-by: Alexandre Belloni <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> drivers/watchdog/sama5d4_wdt.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index d710014f3b7d..0ae947c3d7bc 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -300,6 +300,11 @@ static int sama5d4_wdt_resume(struct device *dev)
> {
> struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
>
> + /*
> + * FIXME: writing MR also pings the watchdog which may not be desired.
> + * This should only be done when the registers are lost on suspend but
> + * there is no way to get this information right now.
> + */
> sama5d4_wdt_init(wdt);
>
> return 0;
>
On 03/02/2017 09:31 AM, Alexandre Belloni wrote:
> The datasheet states: "When setting the WDDIS bit, and while it is set, the
> fields WDV and WDD must not be modified."
>
> Because the whole configuration is already cached inside .mr, wait for the
> user to enable the watchdog to configure it so it is enabled and configured
> at the same time (what the IP is actually expecting).
>
> When the watchdog is already enabled, it is not an issue to reconfigure it.
>
> Signed-off-by: Alexandre Belloni <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> drivers/watchdog/sama5d4_wdt.c | 48 ++++++++++++++++++++++++++----------------
> 1 file changed, 30 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index f709962018ac..5cee20caca78 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -44,6 +44,8 @@ MODULE_PARM_DESC(nowayout,
> "Watchdog cannot be stopped once started (default="
> __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>
> +#define wdt_enabled (!(wdt->mr & AT91_WDT_WDDIS))
> +
> #define wdt_read(wdt, field) \
> readl_relaxed((wdt)->reg_base + (field))
>
> @@ -89,7 +91,16 @@ static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd,
> wdt->mr &= ~AT91_WDT_WDD;
> wdt->mr |= AT91_WDT_SET_WDV(value);
> wdt->mr |= AT91_WDT_SET_WDD(value);
> - wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> +
> + /*
> + * WDDIS has to be 0 when updating WDD/WDV. The datasheet states: When
> + * setting the WDDIS bit, and while it is set, the fields WDV and WDD
> + * must not be modified.
> + * If the watchdog is enabled, then the timeout can be updated. Else,
> + * wait that the user enables it.
> + */
> + if (wdt_enabled)
> + wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
>
> wdd->timeout = timeout;
>
> @@ -145,23 +156,20 @@ static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt)
>
> static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
> {
> - struct watchdog_device *wdd = &wdt->wdd;
> - u32 value = WDT_SEC2TICKS(wdd->timeout);
> u32 reg;
> -
> /*
> - * Because the fields WDV and WDD must not be modified when the WDDIS
> - * bit is set, so clear the WDDIS bit before writing the WDT_MR.
> + * When booting and resuming, the bootloader may have changed the
> + * watchdog configuration.
> + * If the watchdog is already running, we can safely update it.
> + * Else, we have to disable it properly.
> */
> - reg = wdt_read(wdt, AT91_WDT_MR);
> - reg &= ~AT91_WDT_WDDIS;
> - wdt_write(wdt, AT91_WDT_MR, reg);
> -
> - wdt->mr |= AT91_WDT_SET_WDD(value);
> - wdt->mr |= AT91_WDT_SET_WDV(value);
> -
> - wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> -
> + if (wdt_enabled) {
> + wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> + } else {
> + reg = wdt_read(wdt, AT91_WDT_MR);
> + if (!(reg & AT91_WDT_WDDIS))
> + wdt_write(wdt, AT91_WDT_MR, reg | AT91_WDT_WDDIS);
> + }
> return 0;
> }
>
> @@ -172,6 +180,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
> struct resource *res;
> void __iomem *regs;
> u32 irq = 0;
> + u32 timeout;
> int ret;
>
> wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> @@ -221,6 +230,11 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
> return ret;
> }
>
> + timeout = WDT_SEC2TICKS(wdd->timeout);
> +
> + wdt->mr |= AT91_WDT_SET_WDD(timeout);
> + wdt->mr |= AT91_WDT_SET_WDV(timeout);
> +
> ret = sama5d4_wdt_init(wdt);
> if (ret)
> return ret;
> @@ -263,9 +277,7 @@ static int sama5d4_wdt_resume(struct device *dev)
> {
> struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
>
> - wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
> - if (wdt->mr & AT91_WDT_WDDIS)
> - wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> + sama5d4_wdt_init(wdt);
>
> return 0;
> }
>
> -----Original Message-----
> From: Alexandre Belloni [mailto:[email protected]]
> Sent: 2017年3月3日 1:31
> To: Guenter Roeck <[email protected]>
> Cc: Wim Van Sebroeck <[email protected]>; Nicolas Ferre - M43238
> <[email protected]>; Wenyou Yang - A41535
> <[email protected]>; [email protected]; linux-arm-
> [email protected]; [email protected]; Alexandre Belloni
> <[email protected]>
> Subject: [PATCH 2/4] watchdog: sama5d4: fix race condition
>
> WDT_MR and WDT_CR must not updated within three slow clock periods after
> the last ping (write to WDT_CR or WDT_MR). Ensure enough time has elapsed
> before writing those registers.
> wdt_write() waits for 4 periods to ensure at least 3 edges are seen by the IP.
Indeed.
Acked-by: Wenyou.Yang <[email protected]>
Thanks
>
> Signed-off-by: Alexandre Belloni <[email protected]>
> ---
> drivers/watchdog/sama5d4_wdt.c | 33 +++++++++++++++++++++++++++++----
> 1 file changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 5cee20caca78..362fd229786d 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -6,6 +6,7 @@
> * Licensed under GPLv2.
> */
>
> +#include <linux/delay.h>
> #include <linux/interrupt.h>
> #include <linux/io.h>
> #include <linux/kernel.h>
> @@ -29,6 +30,7 @@ struct sama5d4_wdt {
> struct watchdog_device wdd;
> void __iomem *reg_base;
> u32 mr;
> + unsigned long last_ping;
> };
>
> static int wdt_timeout = WDT_DEFAULT_TIMEOUT; @@ -49,8 +51,29 @@
> MODULE_PARM_DESC(nowayout, #define wdt_read(wdt, field) \
> readl_relaxed((wdt)->reg_base + (field))
>
> -#define wdt_write(wtd, field, val) \
> - writel_relaxed((val), (wdt)->reg_base + (field))
> +/* 4 slow clock periods is 4/32768 = 122.07µs*/
> +#define WDT_DELAY usecs_to_jiffies(123)
> +
> +static void wdt_write(struct sama5d4_wdt *wdt, u32 field, u32 val) {
> + /*
> + * WDT_CR and WDT_MR must not be modified within three slow clock
> + * periods following a restart of the watchdog performed by a write
> + * access in WDT_CR.
> + */
> + while (time_before(jiffies, wdt->last_ping + WDT_DELAY))
> + usleep_range(30, 125);
> + writel_relaxed(val, wdt->reg_base + field);
> + wdt->last_ping = jiffies;
> +}
> +
> +static void wdt_write_nosleep(struct sama5d4_wdt *wdt, u32 field, u32
> +val) {
> + if (time_before(jiffies, wdt->last_ping + WDT_DELAY))
> + udelay(123);
> + writel_relaxed(val, wdt->reg_base + field);
> + wdt->last_ping = jiffies;
> +}
>
> static int sama5d4_wdt_start(struct watchdog_device *wdd) { @@ -164,11
> +187,12 @@ static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
> * Else, we have to disable it properly.
> */
> if (wdt_enabled) {
> - wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> + wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
> } else {
> reg = wdt_read(wdt, AT91_WDT_MR);
> if (!(reg & AT91_WDT_WDDIS))
> - wdt_write(wdt, AT91_WDT_MR, reg |
> AT91_WDT_WDDIS);
> + wdt_write_nosleep(wdt, AT91_WDT_MR,
> + reg | AT91_WDT_WDDIS);
> }
> return 0;
> }
> @@ -193,6 +217,7 @@ static int sama5d4_wdt_probe(struct platform_device
> *pdev)
> wdd->ops = &sama5d4_wdt_ops;
> wdd->min_timeout = MIN_WDT_TIMEOUT;
> wdd->max_timeout = MAX_WDT_TIMEOUT;
> + wdt->last_ping = jiffies;
>
> watchdog_set_drvdata(wdd, wdt);
>
> --
> 2.11.0
Best Regards,
Wenyou Yang
> -----Original Message-----
> From: Alexandre Belloni [mailto:[email protected]]
> Sent: 2017??3??3?? 1:31
> To: Guenter Roeck <[email protected]>
> Cc: Wim Van Sebroeck <[email protected]>; Nicolas Ferre - M43238
> <[email protected]>; Wenyou Yang - A41535
> <[email protected]>; [email protected]; linux-arm-
> [email protected]; [email protected]; Alexandre Belloni
> <[email protected]>
> Subject: [PATCH 1/4] watchdog: sama5d4: fix WDDIS handling
>
> The datasheet states: "When setting the WDDIS bit, and while it is set, the fields
> WDV and WDD must not be modified."
>
> Because the whole configuration is already cached inside .mr, wait for the user to
> enable the watchdog to configure it so it is enabled and configured at the same
> time (what the IP is actually expecting).
>
> When the watchdog is already enabled, it is not an issue to reconfigure it.
Indeed, thanks.
Acked-by: Wenyou.Yang <[email protected]>
>
> Signed-off-by: Alexandre Belloni <[email protected]>
> ---
> drivers/watchdog/sama5d4_wdt.c | 48 ++++++++++++++++++++++++++----------
> ------
> 1 file changed, 30 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index f709962018ac..5cee20caca78 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -44,6 +44,8 @@ MODULE_PARM_DESC(nowayout,
> "Watchdog cannot be stopped once started (default="
> __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>
> +#define wdt_enabled (!(wdt->mr & AT91_WDT_WDDIS))
> +
> #define wdt_read(wdt, field) \
> readl_relaxed((wdt)->reg_base + (field))
>
> @@ -89,7 +91,16 @@ static int sama5d4_wdt_set_timeout(struct
> watchdog_device *wdd,
> wdt->mr &= ~AT91_WDT_WDD;
> wdt->mr |= AT91_WDT_SET_WDV(value);
> wdt->mr |= AT91_WDT_SET_WDD(value);
> - wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> +
> + /*
> + * WDDIS has to be 0 when updating WDD/WDV. The datasheet states:
> When
> + * setting the WDDIS bit, and while it is set, the fields WDV and WDD
> + * must not be modified.
> + * If the watchdog is enabled, then the timeout can be updated. Else,
> + * wait that the user enables it.
> + */
> + if (wdt_enabled)
> + wdt_write(wdt, AT91_WDT_MR, wdt->mr &
> ~AT91_WDT_WDDIS);
>
> wdd->timeout = timeout;
>
> @@ -145,23 +156,20 @@ static int of_sama5d4_wdt_init(struct device_node *np,
> struct sama5d4_wdt *wdt)
>
> static int sama5d4_wdt_init(struct sama5d4_wdt *wdt) {
> - struct watchdog_device *wdd = &wdt->wdd;
> - u32 value = WDT_SEC2TICKS(wdd->timeout);
> u32 reg;
> -
> /*
> - * Because the fields WDV and WDD must not be modified when the
> WDDIS
> - * bit is set, so clear the WDDIS bit before writing the WDT_MR.
> + * When booting and resuming, the bootloader may have changed the
> + * watchdog configuration.
> + * If the watchdog is already running, we can safely update it.
> + * Else, we have to disable it properly.
> */
> - reg = wdt_read(wdt, AT91_WDT_MR);
> - reg &= ~AT91_WDT_WDDIS;
> - wdt_write(wdt, AT91_WDT_MR, reg);
> -
> - wdt->mr |= AT91_WDT_SET_WDD(value);
> - wdt->mr |= AT91_WDT_SET_WDV(value);
> -
> - wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> -
> + if (wdt_enabled) {
> + wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> + } else {
> + reg = wdt_read(wdt, AT91_WDT_MR);
> + if (!(reg & AT91_WDT_WDDIS))
> + wdt_write(wdt, AT91_WDT_MR, reg |
> AT91_WDT_WDDIS);
> + }
> return 0;
> }
>
> @@ -172,6 +180,7 @@ static int sama5d4_wdt_probe(struct platform_device
> *pdev)
> struct resource *res;
> void __iomem *regs;
> u32 irq = 0;
> + u32 timeout;
> int ret;
>
> wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); @@ -
> 221,6 +230,11 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
> return ret;
> }
>
> + timeout = WDT_SEC2TICKS(wdd->timeout);
> +
> + wdt->mr |= AT91_WDT_SET_WDD(timeout);
> + wdt->mr |= AT91_WDT_SET_WDV(timeout);
> +
> ret = sama5d4_wdt_init(wdt);
> if (ret)
> return ret;
> @@ -263,9 +277,7 @@ static int sama5d4_wdt_resume(struct device *dev) {
> struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
>
> - wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
> - if (wdt->mr & AT91_WDT_WDDIS)
> - wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> + sama5d4_wdt_init(wdt);
>
> return 0;
> }
> --
> 2.11.0
Best Regards,
Wenyou Yang