* [PATCH 2/2] watchdog: sama5d4: write the mode register in two steps
@ 2018-09-14 10:13 ` Romain Izard
0 siblings, 0 replies; 18+ messages in thread
From: Romain Izard @ 2018-09-14 10:13 UTC (permalink / raw)
To: linux-arm-kernel
The specification for SAMA5D2 and SAMA5D4 chips, that use this IP for
their watchdog timer, has the following advice regarding the Mode Register:
"When setting the WDDIS bit, and while it is set, the fields WDV and WDD
must not be modified."
I have observed on a board based on a SAMA5D2 chip that using any other
timeout duration than the default 16s in the device tree will reset the
board when the watchdog device is opened; this is probably due to ignoring
the aforementioned constraint.
To fix this, read the content of the Mode Register before writing it,
and split the access into two parts if WDV or WDD need to be changed.
Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
---
drivers/watchdog/sama5d4_wdt.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
index 1e93c1b0e3cf..1e05268ad94b 100644
--- a/drivers/watchdog/sama5d4_wdt.c
+++ b/drivers/watchdog/sama5d4_wdt.c
@@ -46,7 +46,10 @@ 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_enabled(reg) (!((reg) & AT91_WDT_WDDIS))
+
+#define wdt_different_counters(reg_a, reg_b) \
+ (((reg_a) ^ (reg_b)) & (AT91_WDT_WDV | AT91_WDT_WDD))
#define wdt_read(wdt, field) \
readl_relaxed((wdt)->reg_base + (field))
@@ -78,8 +81,11 @@ static void wdt_write_nosleep(struct sama5d4_wdt *wdt, u32 field, u32 val)
static int sama5d4_wdt_start(struct watchdog_device *wdd)
{
struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
+ u32 reg = wdt_read(wdt, AT91_WDT_MR);
wdt->mr &= ~AT91_WDT_WDDIS;
+ if (!wdt_enabled(reg) && wdt_different_counters(reg, wdt->mr))
+ wdt_write(wdt, AT91_WDT_MR, reg & ~AT91_WDT_WDDIS);
wdt_write(wdt, AT91_WDT_MR, wdt->mr);
return 0;
@@ -88,8 +94,11 @@ static int sama5d4_wdt_start(struct watchdog_device *wdd)
static int sama5d4_wdt_stop(struct watchdog_device *wdd)
{
struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
+ u32 reg = wdt_read(wdt, AT91_WDT_MR);
wdt->mr |= AT91_WDT_WDDIS;
+ if (wdt_enabled(reg) && wdt_different_counters(reg, wdt->mr))
+ wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
wdt_write(wdt, AT91_WDT_MR, wdt->mr);
return 0;
@@ -122,7 +131,7 @@ static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd,
* If the watchdog is enabled, then the timeout can be updated. Else,
* wait that the user enables it.
*/
- if (wdt_enabled)
+ if (wdt_enabled(wdt->mr))
wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
wdd->timeout = timeout;
@@ -186,13 +195,17 @@ static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
* If the watchdog is already running, we can safely update it.
* Else, we have to disable it properly.
*/
- if (wdt_enabled) {
+ reg = wdt_read(wdt, AT91_WDT_MR);
+ if (wdt_enabled(reg)) {
+ if (!wdt_enabled(wdt->mr))
+ wdt_write_nosleep(wdt, AT91_WDT_MR,
+ wdt->mr & ~AT91_WDT_WDDIS);
wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
- } else {
- reg = wdt_read(wdt, AT91_WDT_MR);
- if (!(reg & AT91_WDT_WDDIS))
+ } else if (wdt_enabled(wdt->mr)) {
+ if (wdt_different_counters(reg, wdt->mr))
wdt_write_nosleep(wdt, AT91_WDT_MR,
- reg | AT91_WDT_WDDIS);
+ reg & ~AT91_WDT_WDDIS);
+ wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
}
return 0;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] watchdog: sama5d4: write the mode register in two steps
2018-09-14 10:13 ` Romain Izard
@ 2018-09-14 10:27 ` Alexandre Belloni
-1 siblings, 0 replies; 18+ messages in thread
From: Alexandre Belloni @ 2018-09-14 10:27 UTC (permalink / raw)
To: Romain Izard
Cc: Nicolas Ferre, Wim Van Sebroeck, Guenter Roeck, Marcus Folkesson,
linux-arm-kernel, linux-watchdog, linux-kernel
On 14/09/2018 12:13:39+0200, Romain Izard wrote:
> The specification for SAMA5D2 and SAMA5D4 chips, that use this IP for
> their watchdog timer, has the following advice regarding the Mode Register:
>
> "When setting the WDDIS bit, and while it is set, the fields WDV and WDD
> must not be modified."
>
> I have observed on a board based on a SAMA5D2 chip that using any other
> timeout duration than the default 16s in the device tree will reset the
> board when the watchdog device is opened; this is probably due to ignoring
> the aforementioned constraint.
>
> To fix this, read the content of the Mode Register before writing it,
> and split the access into two parts if WDV or WDD need to be changed.
>
Hum, that is really weird because when I developed
015b528644a84b0018d3286ecd6ea5f82dce0180, I tested with a program doing:
flags = WDIOS_DISABLECARD;
ioctl(fd, WDIOC_SETOPTIONS, &flags);
for (i = 16; i > 2; i--) {
ioctl(fd, WDIOC_SETTIMEOUT, &i);
}
ioctl(fd, WDIOC_KEEPALIVE, &dummy);
flags = WDIOS_ENABLECARD;
ioctl(fd, WDIOC_SETOPTIONS, &flags);
for (i = 16; i > 2; i--) {
ioctl(fd, WDIOC_SETTIMEOUT, &i);
}
This would immediately reproduce the reset when changing WDV/WDD with
WDDIS set.
I'll test again.
> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> ---
> drivers/watchdog/sama5d4_wdt.c | 27 ++++++++++++++++++++-------
> 1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 1e93c1b0e3cf..1e05268ad94b 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -46,7 +46,10 @@ 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_enabled(reg) (!((reg) & AT91_WDT_WDDIS))
> +
> +#define wdt_different_counters(reg_a, reg_b) \
> + (((reg_a) ^ (reg_b)) & (AT91_WDT_WDV | AT91_WDT_WDD))
>
> #define wdt_read(wdt, field) \
> readl_relaxed((wdt)->reg_base + (field))
> @@ -78,8 +81,11 @@ static void wdt_write_nosleep(struct sama5d4_wdt *wdt, u32 field, u32 val)
> static int sama5d4_wdt_start(struct watchdog_device *wdd)
> {
> struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
> + u32 reg = wdt_read(wdt, AT91_WDT_MR);
>
> wdt->mr &= ~AT91_WDT_WDDIS;
> + if (!wdt_enabled(reg) && wdt_different_counters(reg, wdt->mr))
> + wdt_write(wdt, AT91_WDT_MR, reg & ~AT91_WDT_WDDIS);
> wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>
> return 0;
> @@ -88,8 +94,11 @@ static int sama5d4_wdt_start(struct watchdog_device *wdd)
> static int sama5d4_wdt_stop(struct watchdog_device *wdd)
> {
> struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
> + u32 reg = wdt_read(wdt, AT91_WDT_MR);
>
> wdt->mr |= AT91_WDT_WDDIS;
> + if (wdt_enabled(reg) && wdt_different_counters(reg, wdt->mr))
> + wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
> wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>
> return 0;
> @@ -122,7 +131,7 @@ static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd,
> * If the watchdog is enabled, then the timeout can be updated. Else,
> * wait that the user enables it.
> */
> - if (wdt_enabled)
> + if (wdt_enabled(wdt->mr))
> wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
>
> wdd->timeout = timeout;
> @@ -186,13 +195,17 @@ static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
> * If the watchdog is already running, we can safely update it.
> * Else, we have to disable it properly.
> */
> - if (wdt_enabled) {
> + reg = wdt_read(wdt, AT91_WDT_MR);
> + if (wdt_enabled(reg)) {
> + if (!wdt_enabled(wdt->mr))
> + wdt_write_nosleep(wdt, AT91_WDT_MR,
> + wdt->mr & ~AT91_WDT_WDDIS);
> wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
> - } else {
> - reg = wdt_read(wdt, AT91_WDT_MR);
> - if (!(reg & AT91_WDT_WDDIS))
> + } else if (wdt_enabled(wdt->mr)) {
> + if (wdt_different_counters(reg, wdt->mr))
> wdt_write_nosleep(wdt, AT91_WDT_MR,
> - reg | AT91_WDT_WDDIS);
> + reg & ~AT91_WDT_WDDIS);
> + wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
> }
> return 0;
> }
> --
> 2.17.1
>
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 2/2] watchdog: sama5d4: write the mode register in two steps
@ 2018-09-14 10:27 ` Alexandre Belloni
0 siblings, 0 replies; 18+ messages in thread
From: Alexandre Belloni @ 2018-09-14 10:27 UTC (permalink / raw)
To: linux-arm-kernel
On 14/09/2018 12:13:39+0200, Romain Izard wrote:
> The specification for SAMA5D2 and SAMA5D4 chips, that use this IP for
> their watchdog timer, has the following advice regarding the Mode Register:
>
> "When setting the WDDIS bit, and while it is set, the fields WDV and WDD
> must not be modified."
>
> I have observed on a board based on a SAMA5D2 chip that using any other
> timeout duration than the default 16s in the device tree will reset the
> board when the watchdog device is opened; this is probably due to ignoring
> the aforementioned constraint.
>
> To fix this, read the content of the Mode Register before writing it,
> and split the access into two parts if WDV or WDD need to be changed.
>
Hum, that is really weird because when I developed
015b528644a84b0018d3286ecd6ea5f82dce0180, I tested with a program doing:
flags = WDIOS_DISABLECARD;
ioctl(fd, WDIOC_SETOPTIONS, &flags);
for (i = 16; i > 2; i--) {
ioctl(fd, WDIOC_SETTIMEOUT, &i);
}
ioctl(fd, WDIOC_KEEPALIVE, &dummy);
flags = WDIOS_ENABLECARD;
ioctl(fd, WDIOC_SETOPTIONS, &flags);
for (i = 16; i > 2; i--) {
ioctl(fd, WDIOC_SETTIMEOUT, &i);
}
This would immediately reproduce the reset when changing WDV/WDD with
WDDIS set.
I'll test again.
> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> ---
> drivers/watchdog/sama5d4_wdt.c | 27 ++++++++++++++++++++-------
> 1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 1e93c1b0e3cf..1e05268ad94b 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -46,7 +46,10 @@ 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_enabled(reg) (!((reg) & AT91_WDT_WDDIS))
> +
> +#define wdt_different_counters(reg_a, reg_b) \
> + (((reg_a) ^ (reg_b)) & (AT91_WDT_WDV | AT91_WDT_WDD))
>
> #define wdt_read(wdt, field) \
> readl_relaxed((wdt)->reg_base + (field))
> @@ -78,8 +81,11 @@ static void wdt_write_nosleep(struct sama5d4_wdt *wdt, u32 field, u32 val)
> static int sama5d4_wdt_start(struct watchdog_device *wdd)
> {
> struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
> + u32 reg = wdt_read(wdt, AT91_WDT_MR);
>
> wdt->mr &= ~AT91_WDT_WDDIS;
> + if (!wdt_enabled(reg) && wdt_different_counters(reg, wdt->mr))
> + wdt_write(wdt, AT91_WDT_MR, reg & ~AT91_WDT_WDDIS);
> wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>
> return 0;
> @@ -88,8 +94,11 @@ static int sama5d4_wdt_start(struct watchdog_device *wdd)
> static int sama5d4_wdt_stop(struct watchdog_device *wdd)
> {
> struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
> + u32 reg = wdt_read(wdt, AT91_WDT_MR);
>
> wdt->mr |= AT91_WDT_WDDIS;
> + if (wdt_enabled(reg) && wdt_different_counters(reg, wdt->mr))
> + wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
> wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>
> return 0;
> @@ -122,7 +131,7 @@ static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd,
> * If the watchdog is enabled, then the timeout can be updated. Else,
> * wait that the user enables it.
> */
> - if (wdt_enabled)
> + if (wdt_enabled(wdt->mr))
> wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
>
> wdd->timeout = timeout;
> @@ -186,13 +195,17 @@ static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
> * If the watchdog is already running, we can safely update it.
> * Else, we have to disable it properly.
> */
> - if (wdt_enabled) {
> + reg = wdt_read(wdt, AT91_WDT_MR);
> + if (wdt_enabled(reg)) {
> + if (!wdt_enabled(wdt->mr))
> + wdt_write_nosleep(wdt, AT91_WDT_MR,
> + wdt->mr & ~AT91_WDT_WDDIS);
> wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
> - } else {
> - reg = wdt_read(wdt, AT91_WDT_MR);
> - if (!(reg & AT91_WDT_WDDIS))
> + } else if (wdt_enabled(wdt->mr)) {
> + if (wdt_different_counters(reg, wdt->mr))
> wdt_write_nosleep(wdt, AT91_WDT_MR,
> - reg | AT91_WDT_WDDIS);
> + reg & ~AT91_WDT_WDDIS);
> + wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
> }
> return 0;
> }
> --
> 2.17.1
>
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] watchdog: sama5d4: write the mode register in two steps
2018-09-14 10:27 ` Alexandre Belloni
@ 2018-09-17 13:58 ` Romain Izard
-1 siblings, 0 replies; 18+ messages in thread
From: Romain Izard @ 2018-09-17 13:58 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Nicolas Ferre, Wim Van Sebroeck, Guenter Roeck, Marcus Folkesson,
linux-arm-kernel, linux-watchdog, LKML
2018-09-14 12:27 GMT+02:00 Alexandre Belloni <alexandre.belloni@bootlin.com>:
> On 14/09/2018 12:13:39+0200, Romain Izard wrote:
>> The specification for SAMA5D2 and SAMA5D4 chips, that use this IP for
>> their watchdog timer, has the following advice regarding the Mode Register:
>>
>> "When setting the WDDIS bit, and while it is set, the fields WDV and WDD
>> must not be modified."
>>
>> I have observed on a board based on a SAMA5D2 chip that using any other
>> timeout duration than the default 16s in the device tree will reset the
>> board when the watchdog device is opened; this is probably due to ignoring
>> the aforementioned constraint.
>>
>> To fix this, read the content of the Mode Register before writing it,
>> and split the access into two parts if WDV or WDD need to be changed.
>>
>
> Hum, that is really weird because when I developed
> 015b528644a84b0018d3286ecd6ea5f82dce0180, I tested with a program doing:
>
> flags = WDIOS_DISABLECARD;
> ioctl(fd, WDIOC_SETOPTIONS, &flags);
> for (i = 16; i > 2; i--) {
> ioctl(fd, WDIOC_SETTIMEOUT, &i);
> }
>
> ioctl(fd, WDIOC_KEEPALIVE, &dummy);
>
> flags = WDIOS_ENABLECARD;
> ioctl(fd, WDIOC_SETOPTIONS, &flags);
>
> for (i = 16; i > 2; i--) {
> ioctl(fd, WDIOC_SETTIMEOUT, &i);
> }
>
> This would immediately reproduce the reset when changing WDV/WDD with
> WDDIS set.
>
> I'll test again.
>
The issue is visible when setting a custom value for the timeout on startup.
In the past it was only possible to do so with a module parameter, and the
previous patch in the series makes it possible to do with the device tree.
When using the Linux4SAM 5.7 release, it is sufficient to set the timeout on
the command line to reproduce the issue:
In the bootloader:
# setenv bootargs $bootargs sama5d4_wdt.wdt_timeout=10
To trigger an immediate reset (with some code that should work):
# (echo 1; while sleep 3; do echo 1; done) > /dev/watchdog
Best regards,
--
Romain Izard
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 2/2] watchdog: sama5d4: write the mode register in two steps
@ 2018-09-17 13:58 ` Romain Izard
0 siblings, 0 replies; 18+ messages in thread
From: Romain Izard @ 2018-09-17 13:58 UTC (permalink / raw)
To: linux-arm-kernel
2018-09-14 12:27 GMT+02:00 Alexandre Belloni <alexandre.belloni@bootlin.com>:
> On 14/09/2018 12:13:39+0200, Romain Izard wrote:
>> The specification for SAMA5D2 and SAMA5D4 chips, that use this IP for
>> their watchdog timer, has the following advice regarding the Mode Register:
>>
>> "When setting the WDDIS bit, and while it is set, the fields WDV and WDD
>> must not be modified."
>>
>> I have observed on a board based on a SAMA5D2 chip that using any other
>> timeout duration than the default 16s in the device tree will reset the
>> board when the watchdog device is opened; this is probably due to ignoring
>> the aforementioned constraint.
>>
>> To fix this, read the content of the Mode Register before writing it,
>> and split the access into two parts if WDV or WDD need to be changed.
>>
>
> Hum, that is really weird because when I developed
> 015b528644a84b0018d3286ecd6ea5f82dce0180, I tested with a program doing:
>
> flags = WDIOS_DISABLECARD;
> ioctl(fd, WDIOC_SETOPTIONS, &flags);
> for (i = 16; i > 2; i--) {
> ioctl(fd, WDIOC_SETTIMEOUT, &i);
> }
>
> ioctl(fd, WDIOC_KEEPALIVE, &dummy);
>
> flags = WDIOS_ENABLECARD;
> ioctl(fd, WDIOC_SETOPTIONS, &flags);
>
> for (i = 16; i > 2; i--) {
> ioctl(fd, WDIOC_SETTIMEOUT, &i);
> }
>
> This would immediately reproduce the reset when changing WDV/WDD with
> WDDIS set.
>
> I'll test again.
>
The issue is visible when setting a custom value for the timeout on startup.
In the past it was only possible to do so with a module parameter, and the
previous patch in the series makes it possible to do with the device tree.
When using the Linux4SAM 5.7 release, it is sufficient to set the timeout on
the command line to reproduce the issue:
In the bootloader:
# setenv bootargs $bootargs sama5d4_wdt.wdt_timeout=10
To trigger an immediate reset (with some code that should work):
# (echo 1; while sleep 3; do echo 1; done) > /dev/watchdog
Best regards,
--
Romain Izard
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] watchdog: sama5d4: write the mode register in two steps
2018-09-17 13:58 ` Romain Izard
@ 2018-10-01 15:52 ` Alexandre Belloni
-1 siblings, 0 replies; 18+ messages in thread
From: Alexandre Belloni @ 2018-10-01 15:52 UTC (permalink / raw)
To: Romain Izard
Cc: Nicolas Ferre, Wim Van Sebroeck, Guenter Roeck, Marcus Folkesson,
linux-arm-kernel, linux-watchdog, LKML
On 17/09/2018 15:58:45+0200, Romain Izard wrote:
> 2018-09-14 12:27 GMT+02:00 Alexandre Belloni <alexandre.belloni@bootlin.com>:
> > On 14/09/2018 12:13:39+0200, Romain Izard wrote:
> >> The specification for SAMA5D2 and SAMA5D4 chips, that use this IP for
> >> their watchdog timer, has the following advice regarding the Mode Register:
> >>
> >> "When setting the WDDIS bit, and while it is set, the fields WDV and WDD
> >> must not be modified."
> >>
> >> I have observed on a board based on a SAMA5D2 chip that using any other
> >> timeout duration than the default 16s in the device tree will reset the
> >> board when the watchdog device is opened; this is probably due to ignoring
> >> the aforementioned constraint.
> >>
> >> To fix this, read the content of the Mode Register before writing it,
> >> and split the access into two parts if WDV or WDD need to be changed.
> >>
> >
> > Hum, that is really weird because when I developed
> > 015b528644a84b0018d3286ecd6ea5f82dce0180, I tested with a program doing:
> >
> > flags = WDIOS_DISABLECARD;
> > ioctl(fd, WDIOC_SETOPTIONS, &flags);
> > for (i = 16; i > 2; i--) {
> > ioctl(fd, WDIOC_SETTIMEOUT, &i);
> > }
> >
> > ioctl(fd, WDIOC_KEEPALIVE, &dummy);
> >
> > flags = WDIOS_ENABLECARD;
> > ioctl(fd, WDIOC_SETOPTIONS, &flags);
> >
> > for (i = 16; i > 2; i--) {
> > ioctl(fd, WDIOC_SETTIMEOUT, &i);
> > }
> >
> > This would immediately reproduce the reset when changing WDV/WDD with
> > WDDIS set.
> >
> > I'll test again.
> >
>
> The issue is visible when setting a custom value for the timeout on startup.
> In the past it was only possible to do so with a module parameter, and the
> previous patch in the series makes it possible to do with the device tree.
>
> When using the Linux4SAM 5.7 release, it is sufficient to set the timeout on
> the command line to reproduce the issue:
>
> In the bootloader:
> # setenv bootargs $bootargs sama5d4_wdt.wdt_timeout=10
>
> To trigger an immediate reset (with some code that should work):
> # (echo 1; while sleep 3; do echo 1; done) > /dev/watchdog
>
Ok, I've tested and seen the issue. I agree with Guenter that it is
probably worth having a function to update MR that takes the timeout
value and whether the watchdog needs to be enabled. It would probably
make caching mr unnecessary.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 2/2] watchdog: sama5d4: write the mode register in two steps
@ 2018-10-01 15:52 ` Alexandre Belloni
0 siblings, 0 replies; 18+ messages in thread
From: Alexandre Belloni @ 2018-10-01 15:52 UTC (permalink / raw)
To: linux-arm-kernel
On 17/09/2018 15:58:45+0200, Romain Izard wrote:
> 2018-09-14 12:27 GMT+02:00 Alexandre Belloni <alexandre.belloni@bootlin.com>:
> > On 14/09/2018 12:13:39+0200, Romain Izard wrote:
> >> The specification for SAMA5D2 and SAMA5D4 chips, that use this IP for
> >> their watchdog timer, has the following advice regarding the Mode Register:
> >>
> >> "When setting the WDDIS bit, and while it is set, the fields WDV and WDD
> >> must not be modified."
> >>
> >> I have observed on a board based on a SAMA5D2 chip that using any other
> >> timeout duration than the default 16s in the device tree will reset the
> >> board when the watchdog device is opened; this is probably due to ignoring
> >> the aforementioned constraint.
> >>
> >> To fix this, read the content of the Mode Register before writing it,
> >> and split the access into two parts if WDV or WDD need to be changed.
> >>
> >
> > Hum, that is really weird because when I developed
> > 015b528644a84b0018d3286ecd6ea5f82dce0180, I tested with a program doing:
> >
> > flags = WDIOS_DISABLECARD;
> > ioctl(fd, WDIOC_SETOPTIONS, &flags);
> > for (i = 16; i > 2; i--) {
> > ioctl(fd, WDIOC_SETTIMEOUT, &i);
> > }
> >
> > ioctl(fd, WDIOC_KEEPALIVE, &dummy);
> >
> > flags = WDIOS_ENABLECARD;
> > ioctl(fd, WDIOC_SETOPTIONS, &flags);
> >
> > for (i = 16; i > 2; i--) {
> > ioctl(fd, WDIOC_SETTIMEOUT, &i);
> > }
> >
> > This would immediately reproduce the reset when changing WDV/WDD with
> > WDDIS set.
> >
> > I'll test again.
> >
>
> The issue is visible when setting a custom value for the timeout on startup.
> In the past it was only possible to do so with a module parameter, and the
> previous patch in the series makes it possible to do with the device tree.
>
> When using the Linux4SAM 5.7 release, it is sufficient to set the timeout on
> the command line to reproduce the issue:
>
> In the bootloader:
> # setenv bootargs $bootargs sama5d4_wdt.wdt_timeout=10
>
> To trigger an immediate reset (with some code that should work):
> # (echo 1; while sleep 3; do echo 1; done) > /dev/watchdog
>
Ok, I've tested and seen the issue. I agree with Guenter that it is
probably worth having a function to update MR that takes the timeout
value and whether the watchdog needs to be enabled. It would probably
make caching mr unnecessary.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] watchdog: sama5d4: write the mode register in two steps
2018-09-14 10:13 ` Romain Izard
@ 2018-09-14 21:29 ` Guenter Roeck
-1 siblings, 0 replies; 18+ messages in thread
From: Guenter Roeck @ 2018-09-14 21:29 UTC (permalink / raw)
To: Romain Izard
Cc: Nicolas Ferre, Alexandre Belloni, Wim Van Sebroeck,
Marcus Folkesson, linux-arm-kernel, linux-watchdog, linux-kernel
On Fri, Sep 14, 2018 at 12:13:39PM +0200, Romain Izard wrote:
> The specification for SAMA5D2 and SAMA5D4 chips, that use this IP for
> their watchdog timer, has the following advice regarding the Mode Register:
>
> "When setting the WDDIS bit, and while it is set, the fields WDV and WDD
> must not be modified."
>
> I have observed on a board based on a SAMA5D2 chip that using any other
> timeout duration than the default 16s in the device tree will reset the
> board when the watchdog device is opened; this is probably due to ignoring
> the aforementioned constraint.
>
> To fix this, read the content of the Mode Register before writing it,
> and split the access into two parts if WDV or WDD need to be changed.
>
> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
That code now looks terribly complicated. Assuming it is indeed necessary,
would it be possible to update the affected register in its own function ?
Guenter
> ---
> drivers/watchdog/sama5d4_wdt.c | 27 ++++++++++++++++++++-------
> 1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 1e93c1b0e3cf..1e05268ad94b 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -46,7 +46,10 @@ 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_enabled(reg) (!((reg) & AT91_WDT_WDDIS))
> +
> +#define wdt_different_counters(reg_a, reg_b) \
> + (((reg_a) ^ (reg_b)) & (AT91_WDT_WDV | AT91_WDT_WDD))
>
> #define wdt_read(wdt, field) \
> readl_relaxed((wdt)->reg_base + (field))
> @@ -78,8 +81,11 @@ static void wdt_write_nosleep(struct sama5d4_wdt *wdt, u32 field, u32 val)
> static int sama5d4_wdt_start(struct watchdog_device *wdd)
> {
> struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
> + u32 reg = wdt_read(wdt, AT91_WDT_MR);
>
> wdt->mr &= ~AT91_WDT_WDDIS;
> + if (!wdt_enabled(reg) && wdt_different_counters(reg, wdt->mr))
> + wdt_write(wdt, AT91_WDT_MR, reg & ~AT91_WDT_WDDIS);
> wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>
> return 0;
> @@ -88,8 +94,11 @@ static int sama5d4_wdt_start(struct watchdog_device *wdd)
> static int sama5d4_wdt_stop(struct watchdog_device *wdd)
> {
> struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
> + u32 reg = wdt_read(wdt, AT91_WDT_MR);
>
> wdt->mr |= AT91_WDT_WDDIS;
> + if (wdt_enabled(reg) && wdt_different_counters(reg, wdt->mr))
> + wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
> wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>
> return 0;
> @@ -122,7 +131,7 @@ static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd,
> * If the watchdog is enabled, then the timeout can be updated. Else,
> * wait that the user enables it.
> */
> - if (wdt_enabled)
> + if (wdt_enabled(wdt->mr))
> wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
>
> wdd->timeout = timeout;
> @@ -186,13 +195,17 @@ static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
> * If the watchdog is already running, we can safely update it.
> * Else, we have to disable it properly.
> */
> - if (wdt_enabled) {
> + reg = wdt_read(wdt, AT91_WDT_MR);
> + if (wdt_enabled(reg)) {
> + if (!wdt_enabled(wdt->mr))
> + wdt_write_nosleep(wdt, AT91_WDT_MR,
> + wdt->mr & ~AT91_WDT_WDDIS);
> wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
> - } else {
> - reg = wdt_read(wdt, AT91_WDT_MR);
> - if (!(reg & AT91_WDT_WDDIS))
> + } else if (wdt_enabled(wdt->mr)) {
> + if (wdt_different_counters(reg, wdt->mr))
> wdt_write_nosleep(wdt, AT91_WDT_MR,
> - reg | AT91_WDT_WDDIS);
> + reg & ~AT91_WDT_WDDIS);
> + wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
> }
> return 0;
> }
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 2/2] watchdog: sama5d4: write the mode register in two steps
@ 2018-09-14 21:29 ` Guenter Roeck
0 siblings, 0 replies; 18+ messages in thread
From: Guenter Roeck @ 2018-09-14 21:29 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Sep 14, 2018 at 12:13:39PM +0200, Romain Izard wrote:
> The specification for SAMA5D2 and SAMA5D4 chips, that use this IP for
> their watchdog timer, has the following advice regarding the Mode Register:
>
> "When setting the WDDIS bit, and while it is set, the fields WDV and WDD
> must not be modified."
>
> I have observed on a board based on a SAMA5D2 chip that using any other
> timeout duration than the default 16s in the device tree will reset the
> board when the watchdog device is opened; this is probably due to ignoring
> the aforementioned constraint.
>
> To fix this, read the content of the Mode Register before writing it,
> and split the access into two parts if WDV or WDD need to be changed.
>
> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
That code now looks terribly complicated. Assuming it is indeed necessary,
would it be possible to update the affected register in its own function ?
Guenter
> ---
> drivers/watchdog/sama5d4_wdt.c | 27 ++++++++++++++++++++-------
> 1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 1e93c1b0e3cf..1e05268ad94b 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -46,7 +46,10 @@ 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_enabled(reg) (!((reg) & AT91_WDT_WDDIS))
> +
> +#define wdt_different_counters(reg_a, reg_b) \
> + (((reg_a) ^ (reg_b)) & (AT91_WDT_WDV | AT91_WDT_WDD))
>
> #define wdt_read(wdt, field) \
> readl_relaxed((wdt)->reg_base + (field))
> @@ -78,8 +81,11 @@ static void wdt_write_nosleep(struct sama5d4_wdt *wdt, u32 field, u32 val)
> static int sama5d4_wdt_start(struct watchdog_device *wdd)
> {
> struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
> + u32 reg = wdt_read(wdt, AT91_WDT_MR);
>
> wdt->mr &= ~AT91_WDT_WDDIS;
> + if (!wdt_enabled(reg) && wdt_different_counters(reg, wdt->mr))
> + wdt_write(wdt, AT91_WDT_MR, reg & ~AT91_WDT_WDDIS);
> wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>
> return 0;
> @@ -88,8 +94,11 @@ static int sama5d4_wdt_start(struct watchdog_device *wdd)
> static int sama5d4_wdt_stop(struct watchdog_device *wdd)
> {
> struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
> + u32 reg = wdt_read(wdt, AT91_WDT_MR);
>
> wdt->mr |= AT91_WDT_WDDIS;
> + if (wdt_enabled(reg) && wdt_different_counters(reg, wdt->mr))
> + wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
> wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>
> return 0;
> @@ -122,7 +131,7 @@ static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd,
> * If the watchdog is enabled, then the timeout can be updated. Else,
> * wait that the user enables it.
> */
> - if (wdt_enabled)
> + if (wdt_enabled(wdt->mr))
> wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
>
> wdd->timeout = timeout;
> @@ -186,13 +195,17 @@ static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
> * If the watchdog is already running, we can safely update it.
> * Else, we have to disable it properly.
> */
> - if (wdt_enabled) {
> + reg = wdt_read(wdt, AT91_WDT_MR);
> + if (wdt_enabled(reg)) {
> + if (!wdt_enabled(wdt->mr))
> + wdt_write_nosleep(wdt, AT91_WDT_MR,
> + wdt->mr & ~AT91_WDT_WDDIS);
> wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
> - } else {
> - reg = wdt_read(wdt, AT91_WDT_MR);
> - if (!(reg & AT91_WDT_WDDIS))
> + } else if (wdt_enabled(wdt->mr)) {
> + if (wdt_different_counters(reg, wdt->mr))
> wdt_write_nosleep(wdt, AT91_WDT_MR,
> - reg | AT91_WDT_WDDIS);
> + reg & ~AT91_WDT_WDDIS);
> + wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
> }
> return 0;
> }
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread