Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1591353 > unrolled thread

[PATCH 0/4] watchdog: sama5d4: fix issues

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] watchdog: sama5d4: fix issues Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-03-02 19:00 +0100
    [PATCH 3/4] watchodg: sama5d4: simplify probe Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-03-02 19:00 +0100
      Re: [PATCH 3/4] watchodg: sama5d4: simplify probe Alexander Dahl <post@lespocky.de> - 2017-03-02 20:40 +0100
      Re: [PATCH 3/4] watchodg: sama5d4: simplify probe Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2017-03-03 12:20 +0100
        Re: [PATCH 3/4] watchodg: sama5d4: simplify probe Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-03-03 12:30 +0100
          Re: [PATCH 3/4] watchodg: sama5d4: simplify probe Guenter Roeck <linux@roeck-us.net> - 2017-03-03 16:10 +0100
      Re: [PATCH 3/4] watchodg: sama5d4: simplify probe Guenter Roeck <linux@roeck-us.net> - 2017-03-04 16:10 +0100
    Re: [PATCH 2/4] watchdog: sama5d4: fix race condition Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-03-02 19:00 +0100
    Re: [PATCH 2/4] watchdog: sama5d4: fix race condition Guenter Roeck <linux@roeck-us.net> - 2017-03-02 20:10 +0100
    [PATCH 1/4] watchdog: sama5d4: fix WDDIS handling Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-03-03 09:30 +0100
      Re: [PATCH 1/4] watchdog: sama5d4: fix WDDIS handling Guenter Roeck <linux@roeck-us.net> - 2017-03-04 17:10 +0100
      RE: [PATCH 1/4] watchdog: sama5d4: fix WDDIS handling <Wenyou.Yang@microchip.com> - 2017-03-07 03:20 +0100
    [PATCH 4/4] watchdog: sama5d4: Add comment explaining what happens on resume Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-03-03 09:30 +0100
      Re: [PATCH 4/4] watchdog: sama5d4: Add comment explaining what  happens on resume Guenter Roeck <linux@roeck-us.net> - 2017-03-04 16:10 +0100
    [PATCH 2/4] watchdog: sama5d4: fix race condition Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-03-03 09:30 +0100
      Re: [PATCH 2/4] watchdog: sama5d4: fix race condition Guenter Roeck <linux@roeck-us.net> - 2017-03-04 16:10 +0100
      RE: [PATCH 2/4] watchdog: sama5d4: fix race condition <Wenyou.Yang@microchip.com> - 2017-03-07 03:10 +0100

#1591353 — [PATCH 0/4] watchdog: sama5d4: fix issues

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

[toc] | [next] | [standalone]

#1591359 — [PATCH 3/4] watchodg: sama5d4: simplify probe

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 <alexandre.belloni@free-electrons.com>
---
 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

[toc] | [prev] | [next] | [standalone]

#1591417 — Re: [PATCH 3/4] watchodg: sama5d4: simplify probe

[Multipart message — attachments visible in raw view] — view raw

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 ***

[toc] | [prev] | [next] | [standalone]

#1591865 — Re: [PATCH 3/4] watchodg: sama5d4: simplify probe

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

[toc] | [prev] | [next] | [standalone]

#1591867 — Re: [PATCH 3/4] watchodg: sama5d4: simplify probe

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

[toc] | [prev] | [next] | [standalone]

#1592028 — Re: [PATCH 3/4] watchodg: sama5d4: simplify probe

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

[toc] | [prev] | [next] | [standalone]

#1592508 — Re: [PATCH 3/4] watchodg: sama5d4: simplify probe

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 <alexandre.belloni@free-electrons.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  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,
>

[toc] | [prev] | [next] | [standalone]

#1591360 — Re: [PATCH 2/4] watchdog: sama5d4: fix race condition

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 <alexandre.belloni@free-electrons.com>
> > ---
> >  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

[toc] | [prev] | [next] | [standalone]

#1591404 — Re: [PATCH 2/4] watchdog: sama5d4: fix race condition

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 <alexandre.belloni@free-electrons.com>
> ---
>  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
> 

[toc] | [prev] | [next] | [standalone]

#1591739 — [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.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 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

[toc] | [prev] | [next] | [standalone]

#1592553 — Re: [PATCH 1/4] watchdog: sama5d4: fix WDDIS handling

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 <alexandre.belloni@free-electrons.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  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;
>  }
>

[toc] | [prev] | [next] | [standalone]

#1593862 — RE: [PATCH 1/4] watchdog: sama5d4: fix WDDIS handling

> -----Original Message-----
> From: Alexandre Belloni [mailto:alexandre.belloni@free-electrons.com]
> Sent: 2017年3月3日 1:31
> To: Guenter Roeck <linux@roeck-us.net>
> Cc: Wim Van Sebroeck <wim@iguana.be>; Nicolas Ferre - M43238
> <Nicolas.Ferre@microchip.com>; Wenyou Yang - A41535
> <Wenyou.Yang@microchip.com>; linux-watchdog@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Alexandre Belloni
> <alexandre.belloni@free-electrons.com>
> 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 <wenyou.yang@microchip.com>

> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>  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

[toc] | [prev] | [next] | [standalone]

#1591740 — [PATCH 4/4] watchdog: sama5d4: Add comment explaining what happens on resume

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 <alexandre.belloni@free-electrons.com>
---
 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

[toc] | [prev] | [next] | [standalone]

#1592510 — Re: [PATCH 4/4] watchdog: sama5d4: Add comment explaining what happens on resume

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 <alexandre.belloni@free-electrons.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  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;
>

[toc] | [prev] | [next] | [standalone]

#1591742 — [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.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 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

[toc] | [prev] | [next] | [standalone]

#1592507 — Re: [PATCH 2/4] watchdog: sama5d4: fix race condition

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 <alexandre.belloni@free-electrons.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  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);
>
>

[toc] | [prev] | [next] | [standalone]

#1593859 — RE: [PATCH 2/4] watchdog: sama5d4: fix race condition

> -----Original Message-----
> From: Alexandre Belloni [mailto:alexandre.belloni@free-electrons.com]
> Sent: 2017年3月3日 1:31
> To: Guenter Roeck <linux@roeck-us.net>
> Cc: Wim Van Sebroeck <wim@iguana.be>; Nicolas Ferre - M43238
> <Nicolas.Ferre@microchip.com>; Wenyou Yang - A41535
> <Wenyou.Yang@microchip.com>; linux-watchdog@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Alexandre Belloni
> <alexandre.belloni@free-electrons.com>
> 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 <wenyou.yang@microchip.com>

Thanks

> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>  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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web