[PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Romain Izard]

If the internal counter is not refreshed when the watchdog is started
for the first time, the watchdog will trigger very rapidly. For example,
opening /dev/watchdog without writing in it will immediately trigger a
reboot, instead of waiting for the delay to expire.

To avoid this problem, reload the timer on opening the watchdog device.

Command: "while sleep 5; do echo 1; done > /dev/watchdog"
Before: system reset
After: the watchdog runs correctly

Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
---
 drivers/watchdog/sama5d4_wdt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
index a49634cdc1cc..e162fe140ae1 100644
--- a/drivers/watchdog/sama5d4_wdt.c
+++ b/drivers/watchdog/sama5d4_wdt.c
@@ -15,6 +15,7 @@
 #include <linux/platform_device.h>
 #include <linux/reboot.h>
 #include <linux/watchdog.h>
+#include <linux/delay.h>
 
 #include "at91sam9_wdt.h"
 
@@ -58,6 +59,8 @@ static int sama5d4_wdt_start(struct watchdog_device *wdd)
 	reg = wdt_read(wdt, AT91_WDT_MR);
 	reg &= ~AT91_WDT_WDDIS;
 	wdt_write(wdt, AT91_WDT_MR, reg);
+	udelay(125); /* > 4 cycles at 32,768 Hz */
+	wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
 
 	return 0;
 }
-- 
2.5.0

Re: [PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Guenter Roeck]

On 03/03/2016 02:29 AM, Romain Izard wrote:
> If the internal counter is not refreshed when the watchdog is started
> for the first time, the watchdog will trigger very rapidly. For example,
> opening /dev/watchdog without writing in it will immediately trigger a
> reboot, instead of waiting for the delay to expire.
>
> To avoid this problem, reload the timer on opening the watchdog device.
>
> Command: "while sleep 5; do echo 1; done > /dev/watchdog"
> Before: system reset
> After: the watchdog runs correctly
>
> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>

Subject might better read "ping watchdog on start" or similar.

Does the watchdog have to be pinged before it is enabled ?
I am a bit concerned that there may still be a 125 uS window
during which the system could restart.

Thanks,
Guenter

> ---
>   drivers/watchdog/sama5d4_wdt.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index a49634cdc1cc..e162fe140ae1 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -15,6 +15,7 @@
>   #include <linux/platform_device.h>
>   #include <linux/reboot.h>
>   #include <linux/watchdog.h>
> +#include <linux/delay.h>
>
>   #include "at91sam9_wdt.h"
>
> @@ -58,6 +59,8 @@ static int sama5d4_wdt_start(struct watchdog_device *wdd)
>   	reg = wdt_read(wdt, AT91_WDT_MR);
>   	reg &= ~AT91_WDT_WDDIS;
>   	wdt_write(wdt, AT91_WDT_MR, reg);
> +	udelay(125); /* > 4 cycles at 32,768 Hz */
> +	wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
>
>   	return 0;
>   }
>

Re: [PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Romain Izard]

Hi Guenter,

2016-03-03 13:10 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
> On 03/03/2016 02:29 AM, Romain Izard wrote:
>>
>> If the internal counter is not refreshed when the watchdog is started
>> for the first time, the watchdog will trigger very rapidly. For
>> example, opening /dev/watchdog without writing in it will immediately
>> trigger a reboot, instead of waiting for the delay to expire.
>>
>> To avoid this problem, reload the timer on opening the watchdog
>> device.
>>
>> Command: "while sleep 5; do echo 1; done > /dev/watchdog"
>> Before: system reset
>> After: the watchdog runs correctly
>>
>> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
>
>
> Subject might better read "ping watchdog on start" or similar.
>
OK. I'll change it for a v2.

> Does the watchdog have to be pinged before it is enabled ?  I am a bit
> concerned that there may still be a 125 uS window during which the
> system could restart.
>

According to the SAMA5D2 & SAMA5D4 datasheets, the timer ought to be
reloaded when the watchdog is enabled by a write in the MR register.
Unfortunately, it does not work as described, as I encountered the
problem on a SAMA5D2 Xplained board.

The 4 clock delay is not in the datasheet either, but without any delay
the timer is clearly not reloaded, as my issue stays the same. As there
is a required delay before writing to MR after writing to CR, I applied
the same type of delay in the reverse case.

Perhaps Nicolas or Wenyou have more information on this.

Best regards,
-- 
Romain Izard

Re: [PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Guenter Roeck]

On Thu, Mar 03, 2016 at 01:53:47PM +0100, Romain Izard wrote:
> Hi Guenter,
> 
> 2016-03-03 13:10 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
> > On 03/03/2016 02:29 AM, Romain Izard wrote:
> >>
> >> If the internal counter is not refreshed when the watchdog is started
> >> for the first time, the watchdog will trigger very rapidly. For
> >> example, opening /dev/watchdog without writing in it will immediately
> >> trigger a reboot, instead of waiting for the delay to expire.
> >>
> >> To avoid this problem, reload the timer on opening the watchdog
> >> device.
> >>
> >> Command: "while sleep 5; do echo 1; done > /dev/watchdog"
> >> Before: system reset
> >> After: the watchdog runs correctly
> >>
> >> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> >
> >
> > Subject might better read "ping watchdog on start" or similar.
> >
> OK. I'll change it for a v2.
> 
> > Does the watchdog have to be pinged before it is enabled ?  I am a bit
> > concerned that there may still be a 125 uS window during which the
> > system could restart.
> >
> 
> According to the SAMA5D2 & SAMA5D4 datasheets, the timer ought to be
> reloaded when the watchdog is enabled by a write in the MR register.
> Unfortunately, it does not work as described, as I encountered the
> problem on a SAMA5D2 Xplained board.
> 
> The 4 clock delay is not in the datasheet either, but without any delay
> the timer is clearly not reloaded, as my issue stays the same. As there
> is a required delay before writing to MR after writing to CR, I applied
> the same type of delay in the reverse case.
> 
Question is if there is now a 4 clock window where the watchdog can
still reset the system. Can you reload the timer before enabling
the watchdog ?

Thanks,
Guenter

> Perhaps Nicolas or Wenyou have more information on this.
> 
> Best regards,
> -- 
> Romain Izard

Re: [PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Yang, Wenyou]

Hi Romain,

On 2016/3/3 18:29, Romain Izard wrote:
> If the internal counter is not refreshed when the watchdog is started
> for the first time, the watchdog will trigger very rapidly. For example,
> opening /dev/watchdog without writing in it will immediately trigger a
> reboot, instead of waiting for the delay to expire.
>
> To avoid this problem, reload the timer on opening the watchdog device.
>
> Command: "while sleep 5; do echo 1; done > /dev/watchdog"
> Before: system reset
> After: the watchdog runs correctly
I didn't reproduce your issue on my side,

run the your commands as follows, it works fine,  the system reset 
doesn't happen.
---8<----
#!/bin/sh

while [ 1 ]
do
         sleep 5;
         echo 1 > /dev/watchdog
done
--->8----

I also check the WDT_MR register before and after enabling watchdog, the 
WDV and WDD fields are correct.

Can you check it again? thank you.

>
> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> ---
>   drivers/watchdog/sama5d4_wdt.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index a49634cdc1cc..e162fe140ae1 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -15,6 +15,7 @@
>   #include <linux/platform_device.h>
>   #include <linux/reboot.h>
>   #include <linux/watchdog.h>
> +#include <linux/delay.h>
>   
>   #include "at91sam9_wdt.h"
>   
> @@ -58,6 +59,8 @@ static int sama5d4_wdt_start(struct watchdog_device *wdd)
>   	reg = wdt_read(wdt, AT91_WDT_MR);
>   	reg &= ~AT91_WDT_WDDIS;
>   	wdt_write(wdt, AT91_WDT_MR, reg);
> +	udelay(125); /* > 4 cycles at 32,768 Hz */
> +	wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
>   
>   	return 0;
>   }

Best Regards,
Wenyou Yang

Re: [PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Guenter Roeck]

On 03/03/2016 05:35 PM, Yang, Wenyou wrote:
> Hi Romain,
>
> On 2016/3/3 18:29, Romain Izard wrote:
>> If the internal counter is not refreshed when the watchdog is started
>> for the first time, the watchdog will trigger very rapidly. For example,
>> opening /dev/watchdog without writing in it will immediately trigger a
>> reboot, instead of waiting for the delay to expire.
>>
>> To avoid this problem, reload the timer on opening the watchdog device.
>>
>> Command: "while sleep 5; do echo 1; done > /dev/watchdog"
>> Before: system reset
>> After: the watchdog runs correctly
> I didn't reproduce your issue on my side,
>
> run the your commands as follows, it works fine,  the system reset doesn't happen.

Different chip revision ? Different chip type ? Different chip initialization by ROMMON ?

Can we get exact chip revisions and types for both cases (working and not working),
and (if it might be relevant) a dump of all associated chip registers ?

Thanks,
Guenter

> ---8<----
> #!/bin/sh
>
> while [ 1 ]
> do
>          sleep 5;
>          echo 1 > /dev/watchdog
> done
> --->8----
>
> I also check the WDT_MR register before and after enabling watchdog, the WDV and WDD fields are correct.
>
> Can you check it again? thank you.
>
>>
>> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
>> ---
>>   drivers/watchdog/sama5d4_wdt.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
>> index a49634cdc1cc..e162fe140ae1 100644
>> --- a/drivers/watchdog/sama5d4_wdt.c
>> +++ b/drivers/watchdog/sama5d4_wdt.c
>> @@ -15,6 +15,7 @@
>>   #include <linux/platform_device.h>
>>   #include <linux/reboot.h>
>>   #include <linux/watchdog.h>
>> +#include <linux/delay.h>
>>   #include "at91sam9_wdt.h"
>> @@ -58,6 +59,8 @@ static int sama5d4_wdt_start(struct watchdog_device *wdd)
>>       reg = wdt_read(wdt, AT91_WDT_MR);
>>       reg &= ~AT91_WDT_WDDIS;
>>       wdt_write(wdt, AT91_WDT_MR, reg);
>> +    udelay(125); /* > 4 cycles at 32,768 Hz */
>> +    wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
>>       return 0;
>>   }
>
> Best Regards,
> Wenyou Yang
>

Re: [PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Romain Izard]

Hi Wenyou, Guenter,

2016-03-04 6:23 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
> On 03/03/2016 05:35 PM, Yang, Wenyou wrote:
>> On 2016/3/3 18:29, Romain Izard wrote:
>>>
>>> If the internal counter is not refreshed when the watchdog is
>>> started for the first time, the watchdog will trigger very rapidly.
>>> For example, opening /dev/watchdog without writing in it will
>>> immediately trigger a reboot, instead of waiting for the delay to
>>> expire.
>>>
>>> To avoid this problem, reload the timer on opening the watchdog
>>> device.
>>>
>>> Command: "while sleep 5; do echo 1; done > /dev/watchdog" Before:
>>> system reset After: the watchdog runs correctly
>>
>> I didn't reproduce your issue on my side,
>>
>> run the your commands as follows, it works fine,  the system reset
>> doesn't happen.

I've just verified with the factory image provided on the SAMA5D2
Xplained board. It does not display this behaviour.

But the difference is that in the case without the issue, I'm using the
AT91bootstrap SPL, U-Boot, and the kernel from the QSPI chip. When I
have the issue, I have a U-Boot based SPL, U-Boot itself and the kernel
that come from the FAT partition of an SD-Card.

Userspace does not seem to be involved in the issue, as I can reproduce
it both with my buildroot environment, and the Yocto environment from
the factory image.

> Different chip revision ? Different chip type ? Different chip
> initialization by ROMMON ?
>
> Can we get exact chip revisions and types for both cases (working and
> not working), and (if it might be relevant) a dump of all associated
> chip registers ?


>> I also check the WDT_MR register before and after enabling watchdog,
>> the WDV and WDD fields are correct.
>>
>> Can you check it again? thank you.

Working case:
MR on kernel startup:   0x3fffafff
MR after watchdog init: 0x0fffafff
MR after start:         0x0fff2fff

Problem case:
MR on kernel startup:   0x00008000
MR after watchdog init: 0x0fffafff
MR after start:         0x0fff2fff

So this means that the counter reload does not seem to work very well if
WDD/WDV have been set to 0 in the past. The other question is why does
U-Boot (from the Atmel branch based on 2015.1) put this stange value in
this register.

Best regards,
-- 
Romain Izard

Re: [PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Guenter Roeck]

On 03/04/2016 01:06 AM, Romain Izard wrote:
> Hi Wenyou, Guenter,
>
> 2016-03-04 6:23 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
>> On 03/03/2016 05:35 PM, Yang, Wenyou wrote:
>>> On 2016/3/3 18:29, Romain Izard wrote:
>>>>
>>>> If the internal counter is not refreshed when the watchdog is
>>>> started for the first time, the watchdog will trigger very rapidly.
>>>> For example, opening /dev/watchdog without writing in it will
>>>> immediately trigger a reboot, instead of waiting for the delay to
>>>> expire.
>>>>
>>>> To avoid this problem, reload the timer on opening the watchdog
>>>> device.
>>>>
>>>> Command: "while sleep 5; do echo 1; done > /dev/watchdog" Before:
>>>> system reset After: the watchdog runs correctly
>>>
>>> I didn't reproduce your issue on my side,
>>>
>>> run the your commands as follows, it works fine,  the system reset
>>> doesn't happen.
>
> I've just verified with the factory image provided on the SAMA5D2
> Xplained board. It does not display this behaviour.
>
> But the difference is that in the case without the issue, I'm using the
> AT91bootstrap SPL, U-Boot, and the kernel from the QSPI chip. When I
> have the issue, I have a U-Boot based SPL, U-Boot itself and the kernel
> that come from the FAT partition of an SD-Card.
>
> Userspace does not seem to be involved in the issue, as I can reproduce
> it both with my buildroot environment, and the Yocto environment from
> the factory image.
>
>> Different chip revision ? Different chip type ? Different chip
>> initialization by ROMMON ?
>>
>> Can we get exact chip revisions and types for both cases (working and
>> not working), and (if it might be relevant) a dump of all associated
>> chip registers ?
>
>
>>> I also check the WDT_MR register before and after enabling watchdog,
>>> the WDV and WDD fields are correct.
>>>
>>> Can you check it again? thank you.
>
> Working case:
> MR on kernel startup:   0x3fffafff
> MR after watchdog init: 0x0fffafff
> MR after start:         0x0fff2fff
>
> Problem case:
> MR on kernel startup:   0x00008000
> MR after watchdog init: 0x0fffafff
> MR after start:         0x0fff2fff
>
> So this means that the counter reload does not seem to work very well if
> WDD/WDV have been set to 0 in the past. The other question is why does
> U-Boot (from the Atmel branch based on 2015.1) put this stange value in
> this register.
>

Can you check the value of AT91_WDT_SR ? Maybe it tells us something.

Also, in the error case, can you check if the watchdog times out at all
after you applied your patch ?

Thanks,
Guenter

> Best regards,
>

Re: [PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Romain Izard]

2016-03-04 14:09 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
> On 03/04/2016 01:06 AM, Romain Izard wrote:
>> 2016-03-04 6:23 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
>>> On 03/03/2016 05:35 PM, Yang, Wenyou wrote:
>>>> On 2016/3/3 18:29, Romain Izard wrote:
>>>>>
>>>>> If the internal counter is not refreshed when the watchdog is
>>>>> started for the first time, the watchdog will trigger very
>>>>> rapidly.  For example, opening /dev/watchdog without writing in it
>>>>> will immediately trigger a reboot, instead of waiting for the
>>>>> delay to expire.
>>>>>
>>>>> To avoid this problem, reload the timer on opening the watchdog
>>>>> device.
>>>>>
>>>>> Command: "while sleep 5; do echo 1; done > /dev/watchdog" Before:
>>>>> system reset After: the watchdog runs correctly
>>>>
>>>>
>>>> I didn't reproduce your issue on my side,
>>>>
>>>> run the your commands as follows, it works fine,  the system reset
>>>> doesn't happen.
>>
>>
>> I've just verified with the factory image provided on the SAMA5D2
>> Xplained board. It does not display this behaviour.
>>
>> But the difference is that in the case without the issue, I'm using
>> the AT91bootstrap SPL, U-Boot, and the kernel from the QSPI chip.
>> When I have the issue, I have a U-Boot based SPL, U-Boot itself and
>> the kernel that come from the FAT partition of an SD-Card.
>>
>> Userspace does not seem to be involved in the issue, as I can
>> reproduce it both with my buildroot environment, and the Yocto
>> environment from the factory image.
>>
>>> Different chip revision ? Different chip type ? Different chip
>>> initialization by ROMMON ?
>>>
>>> Can we get exact chip revisions and types for both cases (working
>>> and not working), and (if it might be relevant) a dump of all
>>> associated chip registers ?
>>
>>
>>
>>>> I also check the WDT_MR register before and after enabling
>>>> watchdog, the WDV and WDD fields are correct.
>>>>
>>>> Can you check it again? thank you.
>>
>>
>> Working case:
>> MR on kernel startup:   0x3fffafff
>> MR after watchdog init: 0x0fffafff
>> MR after start:         0x0fff2fff
>>
>> Problem case:
>> MR on kernel startup:   0x00008000
>> MR after watchdog init: 0x0fffafff
>> MR after start:         0x0fff2fff
>>
>> So this means that the counter reload does not seem to work very well
>> if WDD/WDV have been set to 0 in the past. The other question is why
>> does U-Boot (from the Atmel branch based on 2015.1) put this stange
>> value in this register.
>>
>
> Can you check the value of AT91_WDT_SR ? Maybe it tells us something.
>
I didn't report it because it contained 0 at all times. So no information.

> Also, in the error case, can you check if the watchdog times out at all
> after you applied your patch ?

It times out after 16s as expected, and reboot occurs correctly.

-- 
Romain Izard

Re: [PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Guenter Roeck]

On 03/04/2016 05:26 AM, Romain Izard wrote:
> 2016-03-04 14:09 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
>> On 03/04/2016 01:06 AM, Romain Izard wrote:
>>> 2016-03-04 6:23 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
>>>> On 03/03/2016 05:35 PM, Yang, Wenyou wrote:
>>>>> On 2016/3/3 18:29, Romain Izard wrote:
>>>>>>
>>>>>> If the internal counter is not refreshed when the watchdog is
>>>>>> started for the first time, the watchdog will trigger very
>>>>>> rapidly.  For example, opening /dev/watchdog without writing in it
>>>>>> will immediately trigger a reboot, instead of waiting for the
>>>>>> delay to expire.
>>>>>>
>>>>>> To avoid this problem, reload the timer on opening the watchdog
>>>>>> device.
>>>>>>
>>>>>> Command: "while sleep 5; do echo 1; done > /dev/watchdog" Before:
>>>>>> system reset After: the watchdog runs correctly
>>>>>
>>>>>
>>>>> I didn't reproduce your issue on my side,
>>>>>
>>>>> run the your commands as follows, it works fine,  the system reset
>>>>> doesn't happen.
>>>
>>>
>>> I've just verified with the factory image provided on the SAMA5D2
>>> Xplained board. It does not display this behaviour.
>>>
>>> But the difference is that in the case without the issue, I'm using
>>> the AT91bootstrap SPL, U-Boot, and the kernel from the QSPI chip.
>>> When I have the issue, I have a U-Boot based SPL, U-Boot itself and
>>> the kernel that come from the FAT partition of an SD-Card.
>>>
>>> Userspace does not seem to be involved in the issue, as I can
>>> reproduce it both with my buildroot environment, and the Yocto
>>> environment from the factory image.
>>>
>>>> Different chip revision ? Different chip type ? Different chip
>>>> initialization by ROMMON ?
>>>>
>>>> Can we get exact chip revisions and types for both cases (working
>>>> and not working), and (if it might be relevant) a dump of all
>>>> associated chip registers ?
>>>
>>>
>>>
>>>>> I also check the WDT_MR register before and after enabling
>>>>> watchdog, the WDV and WDD fields are correct.
>>>>>
>>>>> Can you check it again? thank you.
>>>
>>>
>>> Working case:
>>> MR on kernel startup:   0x3fffafff
>>> MR after watchdog init: 0x0fffafff
>>> MR after start:         0x0fff2fff
>>>
>>> Problem case:
>>> MR on kernel startup:   0x00008000
>>> MR after watchdog init: 0x0fffafff
>>> MR after start:         0x0fff2fff
>>>
>>> So this means that the counter reload does not seem to work very well
>>> if WDD/WDV have been set to 0 in the past. The other question is why
>>> does U-Boot (from the Atmel branch based on 2015.1) put this stange
>>> value in this register.
>>>
>>
>> Can you check the value of AT91_WDT_SR ? Maybe it tells us something.
>>
> I didn't report it because it contained 0 at all times. So no information.
>
>> Also, in the error case, can you check if the watchdog times out at all
>> after you applied your patch ?
>
> It times out after 16s as expected, and reboot occurs correctly.
>

Interesting. So it looks like AT91_WDT_WDRSTT has to be set if the timer
values in MR are changed from 0 to another value, or maybe after each
timer value change. Wonder if that should be done in the init function,
after MR is set (with the watchdog disabled).

Thoughts, anyone ?

Thanks,
Guenter

Re: [PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Lothar Waßmann]

Hi,

On Fri, 4 Mar 2016 05:56:20 -0800 Guenter Roeck wrote:
> On 03/04/2016 05:26 AM, Romain Izard wrote:
> > 2016-03-04 14:09 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
> >> On 03/04/2016 01:06 AM, Romain Izard wrote:
> >>> 2016-03-04 6:23 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
> >>>> On 03/03/2016 05:35 PM, Yang, Wenyou wrote:
> >>>>> On 2016/3/3 18:29, Romain Izard wrote:
> >>>>>>
> >>>>>> If the internal counter is not refreshed when the watchdog is
> >>>>>> started for the first time, the watchdog will trigger very
> >>>>>> rapidly.  For example, opening /dev/watchdog without writing in it
> >>>>>> will immediately trigger a reboot, instead of waiting for the
> >>>>>> delay to expire.
> >>>>>>
> >>>>>> To avoid this problem, reload the timer on opening the watchdog
> >>>>>> device.
> >>>>>>
> >>>>>> Command: "while sleep 5; do echo 1; done > /dev/watchdog" Before:
> >>>>>> system reset After: the watchdog runs correctly
> >>>>>
> >>>>>
> >>>>> I didn't reproduce your issue on my side,
> >>>>>
> >>>>> run the your commands as follows, it works fine,  the system reset
> >>>>> doesn't happen.
> >>>
> >>>
> >>> I've just verified with the factory image provided on the SAMA5D2
> >>> Xplained board. It does not display this behaviour.
> >>>
> >>> But the difference is that in the case without the issue, I'm using
> >>> the AT91bootstrap SPL, U-Boot, and the kernel from the QSPI chip.
> >>> When I have the issue, I have a U-Boot based SPL, U-Boot itself and
> >>> the kernel that come from the FAT partition of an SD-Card.
> >>>
> >>> Userspace does not seem to be involved in the issue, as I can
> >>> reproduce it both with my buildroot environment, and the Yocto
> >>> environment from the factory image.
> >>>
> >>>> Different chip revision ? Different chip type ? Different chip
> >>>> initialization by ROMMON ?
> >>>>
> >>>> Can we get exact chip revisions and types for both cases (working
> >>>> and not working), and (if it might be relevant) a dump of all
> >>>> associated chip registers ?
> >>>
> >>>
> >>>
> >>>>> I also check the WDT_MR register before and after enabling
> >>>>> watchdog, the WDV and WDD fields are correct.
> >>>>>
> >>>>> Can you check it again? thank you.
> >>>
> >>>
> >>> Working case:
> >>> MR on kernel startup:   0x3fffafff
> >>> MR after watchdog init: 0x0fffafff
> >>> MR after start:         0x0fff2fff
> >>>
> >>> Problem case:
> >>> MR on kernel startup:   0x00008000
> >>> MR after watchdog init: 0x0fffafff
> >>> MR after start:         0x0fff2fff
> >>>
> >>> So this means that the counter reload does not seem to work very well
> >>> if WDD/WDV have been set to 0 in the past. The other question is why
> >>> does U-Boot (from the Atmel branch based on 2015.1) put this stange
> >>> value in this register.
> >>>
> >>
> >> Can you check the value of AT91_WDT_SR ? Maybe it tells us something.
> >>
> > I didn't report it because it contained 0 at all times. So no information.
> >
> >> Also, in the error case, can you check if the watchdog times out at all
> >> after you applied your patch ?
> >
> > It times out after 16s as expected, and reboot occurs correctly.
> >
> 
> Interesting. So it looks like AT91_WDT_WDRSTT has to be set if the timer
> values in MR are changed from 0 to another value, or maybe after each
> timer value change. Wonder if that should be done in the init function,
> after MR is set (with the watchdog disabled).
> 
> Thoughts, anyone ?
> 
Are you aware of the Notes in the SAMA5D4 Reference Manual (Chapter
19.5.2 Watchdog Timer Mode Register):
|Note: The first write access prevents any further modification of
|      the value of this register. Read accesses remain possible.
|Note: The WDD and WDV values must not be modified within three slow
|      clock periods following a restart of the watchdog performed by
|      a write access in WDT_CR. Any modification will cause the watchdog to trigger an end of period earlier than expected.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Romain Izard]

Hi Lothar,

2016-03-04 15:59 GMT+01:00 Lothar Waßmann <LW@karo-electronics.de>:
>> >>>>> I also check the WDT_MR register before and after enabling
>> >>>>> watchdog, the WDV and WDD fields are correct.
>> >>>>>
>> >>>>> Can you check it again? thank you.
>> >>>
>> >>>
>> >>> Working case:
>> >>> MR on kernel startup:   0x3fffafff
>> >>> MR after watchdog init: 0x0fffafff
>> >>> MR after start:         0x0fff2fff
>> >>>
>> >>> Problem case:
>> >>> MR on kernel startup:   0x00008000
>> >>> MR after watchdog init: 0x0fffafff
>> >>> MR after start:         0x0fff2fff
>> >>>
>> >>> So this means that the counter reload does not seem to work very well
>> >>> if WDD/WDV have been set to 0 in the past. The other question is why
>> >>> does U-Boot (from the Atmel branch based on 2015.1) put this stange
>> >>> value in this register.
>> >>>
>> >>
>> >> Can you check the value of AT91_WDT_SR ? Maybe it tells us something.
>> >>
>> > I didn't report it because it contained 0 at all times. So no information.
>> >
>> >> Also, in the error case, can you check if the watchdog times out at all
>> >> after you applied your patch ?
>> >
>> > It times out after 16s as expected, and reboot occurs correctly.
>> >
>>
>> Interesting. So it looks like AT91_WDT_WDRSTT has to be set if the timer
>> values in MR are changed from 0 to another value, or maybe after each
>> timer value change. Wonder if that should be done in the init function,
>> after MR is set (with the watchdog disabled).
>>
>> Thoughts, anyone ?
>>

> Are you aware of the Notes in the SAMA5D4 Reference Manual (Chapter
> 19.5.2 Watchdog Timer Mode Register):
>
> |Note: The first write access prevents any further modification of
> |      the value of this register. Read accesses remain possible.
> |Note: The WDD and WDV values must not be modified within three slow
> |      clock periods following a restart of the watchdog performed by
> |      a write access in WDT_CR. Any modification will cause the watchdog
> |      to trigger an end of period earlier than expected.

This text is valid for older versions of the Watchdog controller, found
in AT91SAM9 and SAMA5D3 chips. But SAMA5D4 & SAMA5D2 have a newer
revision, which supports multiple writes to the MR register.

Are you sure about your datasheet? I have this in the latest version
found on Atmel's site.

> Atmel-11238B-ATARM-SAMA5D4-Datasheet_24-Aug-15
> Section 18.5.2
>
> Note: Write access to this register has no effect if the LOCKMR
>       command is issued in WDT_CR (unlocked on hardware reset).
> Note: The WDT_MR register values must not be modified within three slow
>       clock periods following a restart of the watchdog performed by
>       a write access in WDT_CR. Any modification will cause the watchdog
>       to trigger an end of period earlier than expected.
>

It matches the comments from Wenyou when he committed the sama5d4
watchdog driver to replace the existing at91sam9 watchdog.

Best regards,
-- 
Romain Izard

Re: [PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Lothar Waßmann]

Hi,

On Fri, 4 Mar 2016 16:26:59 +0100 Romain Izard wrote:
> Hi Lothar,
> 
> 2016-03-04 15:59 GMT+01:00 Lothar Waßmann <LW@karo-electronics.de>:
> >> >>>>> I also check the WDT_MR register before and after enabling
> >> >>>>> watchdog, the WDV and WDD fields are correct.
> >> >>>>>
> >> >>>>> Can you check it again? thank you.
> >> >>>
> >> >>>
> >> >>> Working case:
> >> >>> MR on kernel startup:   0x3fffafff
> >> >>> MR after watchdog init: 0x0fffafff
> >> >>> MR after start:         0x0fff2fff
> >> >>>
> >> >>> Problem case:
> >> >>> MR on kernel startup:   0x00008000
> >> >>> MR after watchdog init: 0x0fffafff
> >> >>> MR after start:         0x0fff2fff
> >> >>>
> >> >>> So this means that the counter reload does not seem to work very well
> >> >>> if WDD/WDV have been set to 0 in the past. The other question is why
> >> >>> does U-Boot (from the Atmel branch based on 2015.1) put this stange
> >> >>> value in this register.
> >> >>>
> >> >>
> >> >> Can you check the value of AT91_WDT_SR ? Maybe it tells us something.
> >> >>
> >> > I didn't report it because it contained 0 at all times. So no information.
> >> >
> >> >> Also, in the error case, can you check if the watchdog times out at all
> >> >> after you applied your patch ?
> >> >
> >> > It times out after 16s as expected, and reboot occurs correctly.
> >> >
> >>
> >> Interesting. So it looks like AT91_WDT_WDRSTT has to be set if the timer
> >> values in MR are changed from 0 to another value, or maybe after each
> >> timer value change. Wonder if that should be done in the init function,
> >> after MR is set (with the watchdog disabled).
> >>
> >> Thoughts, anyone ?
> >>
> 
> > Are you aware of the Notes in the SAMA5D4 Reference Manual (Chapter
> > 19.5.2 Watchdog Timer Mode Register):
> >
> > |Note: The first write access prevents any further modification of
> > |      the value of this register. Read accesses remain possible.
> > |Note: The WDD and WDV values must not be modified within three slow
> > |      clock periods following a restart of the watchdog performed by
> > |      a write access in WDT_CR. Any modification will cause the watchdog
> > |      to trigger an end of period earlier than expected.
> 
> This text is valid for older versions of the Watchdog controller, found
> in AT91SAM9 and SAMA5D3 chips. But SAMA5D4 & SAMA5D2 have a newer
> revision, which supports multiple writes to the MR register.
> 
> Are you sure about your datasheet? I have this in the latest version
> found on Atmel's site.
> 
> > Atmel-11238B-ATARM-SAMA5D4-Datasheet_24-Aug-15
> > Section 18.5.2
> >
OK, I obviously had an outdated Manual.


Lothar Waßmann