[U-Boot] [PATCH] xyz-modem: Fix timeout loop waiting with WATCHDOG

[U-Boot] [PATCH] xyz-modem: Fix timeout loop waiting with WATCHDOG

[Lokesh Vutla]

Commit 2c77c0d6524eb ("xyz-modem: Change getc timeout loop waiting")
fixes the loop delay when using a hw watchdog. But assuming that Watchdog
kicking is taken care of by getc(). This is not true in case of DM_SERIAL.
So, kick the watchdog before loop waiting, instead of relying on other
functions.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
- This fixes UART boot on AM335x-evm.

 common/xyzModem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/common/xyzModem.c b/common/xyzModem.c
index a0c5dfeece..8c4679473f 100644
--- a/common/xyzModem.c
+++ b/common/xyzModem.c
@@ -26,6 +26,7 @@
 #include <xyzModem.h>
 #include <stdarg.h>
 #include <crc.h>
+#include <watchdog.h>
 
 /* Assumption - run xyzModem protocol over the console port */
 
@@ -64,6 +65,7 @@ CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c)
 {
 
   ulong now = get_timer(0);
+  WATCHDOG_RESET();
   while (!tstc ())
     {
       if (get_timer(now) > xyzModem_CHAR_TIMEOUT)
-- 
2.14.1

[U-Boot] [PATCH] xyz-modem: Fix timeout loop waiting with WATCHDOG

[Tom Rini]

On Sat, Sep 16, 2017 at 05:14:31PM +0530, Lokesh Vutla wrote:
> Commit 2c77c0d6524eb ("xyz-modem: Change getc timeout loop waiting")
> fixes the loop delay when using a hw watchdog. But assuming that Watchdog
> kicking is taken care of by getc(). This is not true in case of DM_SERIAL.
> So, kick the watchdog before loop waiting, instead of relying on other
> functions.
> 
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
> - This fixes UART boot on AM335x-evm.
> 
>  common/xyzModem.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/common/xyzModem.c b/common/xyzModem.c
> index a0c5dfeece..8c4679473f 100644
> --- a/common/xyzModem.c
> +++ b/common/xyzModem.c
> @@ -26,6 +26,7 @@
>  #include <xyzModem.h>
>  #include <stdarg.h>
>  #include <crc.h>
> +#include <watchdog.h>
>  
>  /* Assumption - run xyzModem protocol over the console port */
>  
> @@ -64,6 +65,7 @@ CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c)
>  {
>  
>    ulong now = get_timer(0);
> +  WATCHDOG_RESET();
>    while (!tstc ())
>      {
>        if (get_timer(now) > xyzModem_CHAR_TIMEOUT)

My worry is that other places also assume watchdog petted by getc().
Simon, is there a reason DM_SERIAL isn't doing what was done before?
Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170916/476f9285/attachment.sig>

[U-Boot] [PATCH] xyz-modem: Fix timeout loop waiting with WATCHDOG

[Simon Glass]

Hi,

On 16 September 2017 at 07:43, Tom Rini <trini@konsulko.com> wrote:
> On Sat, Sep 16, 2017 at 05:14:31PM +0530, Lokesh Vutla wrote:
>> Commit 2c77c0d6524eb ("xyz-modem: Change getc timeout loop waiting")
>> fixes the loop delay when using a hw watchdog. But assuming that Watchdog
>> kicking is taken care of by getc(). This is not true in case of DM_SERIAL.
>> So, kick the watchdog before loop waiting, instead of relying on other
>> functions.
>>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> ---
>> - This fixes UART boot on AM335x-evm.
>>
>>  common/xyzModem.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/common/xyzModem.c b/common/xyzModem.c
>> index a0c5dfeece..8c4679473f 100644
>> --- a/common/xyzModem.c
>> +++ b/common/xyzModem.c
>> @@ -26,6 +26,7 @@
>>  #include <xyzModem.h>
>>  #include <stdarg.h>
>>  #include <crc.h>
>> +#include <watchdog.h>
>>
>>  /* Assumption - run xyzModem protocol over the console port */
>>
>> @@ -64,6 +65,7 @@ CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c)
>>  {
>>
>>    ulong now = get_timer(0);
>> +  WATCHDOG_RESET();
>>    while (!tstc ())
>>      {
>>        if (get_timer(now) > xyzModem_CHAR_TIMEOUT)
>
> My worry is that other places also assume watchdog petted by getc().
> Simon, is there a reason DM_SERIAL isn't doing what was done before?
> Thanks!

This should really be fixed at source. Is the problems in
_serial_tstc()? The watchdog is reset with _serial_getc().

Regards,
Simon

[U-Boot] [PATCH] xyz-modem: Fix timeout loop waiting with WATCHDOG

[Lokesh Vutla]

Simon,

On 9/16/2017 9:43 PM, Simon Glass wrote:
> Hi,
> 
> On 16 September 2017 at 07:43, Tom Rini <trini@konsulko.com> wrote:
>> On Sat, Sep 16, 2017 at 05:14:31PM +0530, Lokesh Vutla wrote:
>>> Commit 2c77c0d6524eb ("xyz-modem: Change getc timeout loop waiting")
>>> fixes the loop delay when using a hw watchdog. But assuming that Watchdog
>>> kicking is taken care of by getc(). This is not true in case of DM_SERIAL.
>>> So, kick the watchdog before loop waiting, instead of relying on other
>>> functions.
>>>
>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>> ---
>>> - This fixes UART boot on AM335x-evm.
>>>
>>>  common/xyzModem.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/common/xyzModem.c b/common/xyzModem.c
>>> index a0c5dfeece..8c4679473f 100644
>>> --- a/common/xyzModem.c
>>> +++ b/common/xyzModem.c
>>> @@ -26,6 +26,7 @@
>>>  #include <xyzModem.h>
>>>  #include <stdarg.h>
>>>  #include <crc.h>
>>> +#include <watchdog.h>
>>>
>>>  /* Assumption - run xyzModem protocol over the console port */
>>>
>>> @@ -64,6 +65,7 @@ CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c)
>>>  {
>>>
>>>    ulong now = get_timer(0);
>>> +  WATCHDOG_RESET();
>>>    while (!tstc ())
>>>      {
>>>        if (get_timer(now) > xyzModem_CHAR_TIMEOUT)
>>
>> My worry is that other places also assume watchdog petted by getc().
>> Simon, is there a reason DM_SERIAL isn't doing what was done before?
>> Thanks!
> 
> This should really be fixed at source. Is the problems in
> _serial_tstc()? The watchdog is reset with _serial_getc().

Sorry, my bad. WATCHDOG_RESET() is being called in __serial_getc() when
err is -EAGAIN. But looks like it is never hitting this case. This way
ymodem fails as watchdog gets reset when downloading large files.

Thanks and regards,
Lokesh

> 
> Regards,
> Simon
> 

[U-Boot] [PATCH] xyz-modem: Fix timeout loop waiting with WATCHDOG

[Simon Glass]

Hi,

On 16 September 2017 at 23:12, Lokesh Vutla <lokeshvutla@ti.com> wrote:
> Simon,
>
> On 9/16/2017 9:43 PM, Simon Glass wrote:
>> Hi,
>>
>> On 16 September 2017 at 07:43, Tom Rini <trini@konsulko.com> wrote:
>>> On Sat, Sep 16, 2017 at 05:14:31PM +0530, Lokesh Vutla wrote:
>>>> Commit 2c77c0d6524eb ("xyz-modem: Change getc timeout loop waiting")
>>>> fixes the loop delay when using a hw watchdog. But assuming that Watchdog
>>>> kicking is taken care of by getc(). This is not true in case of DM_SERIAL.
>>>> So, kick the watchdog before loop waiting, instead of relying on other
>>>> functions.
>>>>
>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>>> ---
>>>> - This fixes UART boot on AM335x-evm.
>>>>
>>>>  common/xyzModem.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/common/xyzModem.c b/common/xyzModem.c
>>>> index a0c5dfeece..8c4679473f 100644
>>>> --- a/common/xyzModem.c
>>>> +++ b/common/xyzModem.c
>>>> @@ -26,6 +26,7 @@
>>>>  #include <xyzModem.h>
>>>>  #include <stdarg.h>
>>>>  #include <crc.h>
>>>> +#include <watchdog.h>
>>>>
>>>>  /* Assumption - run xyzModem protocol over the console port */
>>>>
>>>> @@ -64,6 +65,7 @@ CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c)
>>>>  {
>>>>
>>>>    ulong now = get_timer(0);
>>>> +  WATCHDOG_RESET();
>>>>    while (!tstc ())
>>>>      {
>>>>        if (get_timer(now) > xyzModem_CHAR_TIMEOUT)
>>>
>>> My worry is that other places also assume watchdog petted by getc().
>>> Simon, is there a reason DM_SERIAL isn't doing what was done before?
>>> Thanks!
>>
>> This should really be fixed at source. Is the problems in
>> _serial_tstc()? The watchdog is reset with _serial_getc().
>
> Sorry, my bad. WATCHDOG_RESET() is being called in __serial_getc() when
> err is -EAGAIN. But looks like it is never hitting this case. This way
> ymodem fails as watchdog gets reset when downloading large files.

So every time we read a character there is one available? How can that
happen? Surely the CPU can run faster than the serial port?

Regards,
Simon

[U-Boot] [PATCH] xyz-modem: Fix timeout loop waiting with WATCHDOG

[Lokesh Vutla]

On Monday 25 September 2017 07:45 AM, Simon Glass wrote:
> Hi,
> 
> On 16 September 2017 at 23:12, Lokesh Vutla <lokeshvutla@ti.com> wrote:
>> Simon,
>>
>> On 9/16/2017 9:43 PM, Simon Glass wrote:
>>> Hi,
>>>
>>> On 16 September 2017 at 07:43, Tom Rini <trini@konsulko.com> wrote:
>>>> On Sat, Sep 16, 2017 at 05:14:31PM +0530, Lokesh Vutla wrote:
>>>>> Commit 2c77c0d6524eb ("xyz-modem: Change getc timeout loop waiting")
>>>>> fixes the loop delay when using a hw watchdog. But assuming that Watchdog
>>>>> kicking is taken care of by getc(). This is not true in case of DM_SERIAL.
>>>>> So, kick the watchdog before loop waiting, instead of relying on other
>>>>> functions.
>>>>>
>>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>>>> ---
>>>>> - This fixes UART boot on AM335x-evm.
>>>>>
>>>>>  common/xyzModem.c | 2 ++
>>>>>  1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/common/xyzModem.c b/common/xyzModem.c
>>>>> index a0c5dfeece..8c4679473f 100644
>>>>> --- a/common/xyzModem.c
>>>>> +++ b/common/xyzModem.c
>>>>> @@ -26,6 +26,7 @@
>>>>>  #include <xyzModem.h>
>>>>>  #include <stdarg.h>
>>>>>  #include <crc.h>
>>>>> +#include <watchdog.h>
>>>>>
>>>>>  /* Assumption - run xyzModem protocol over the console port */
>>>>>
>>>>> @@ -64,6 +65,7 @@ CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c)
>>>>>  {
>>>>>
>>>>>    ulong now = get_timer(0);
>>>>> +  WATCHDOG_RESET();
>>>>>    while (!tstc ())
>>>>>      {
>>>>>        if (get_timer(now) > xyzModem_CHAR_TIMEOUT)
>>>>
>>>> My worry is that other places also assume watchdog petted by getc().
>>>> Simon, is there a reason DM_SERIAL isn't doing what was done before?
>>>> Thanks!
>>>
>>> This should really be fixed at source. Is the problems in
>>> _serial_tstc()? The watchdog is reset with _serial_getc().
>>
>> Sorry, my bad. WATCHDOG_RESET() is being called in __serial_getc() when
>> err is -EAGAIN. But looks like it is never hitting this case. This way
>> ymodem fails as watchdog gets reset when downloading large files.
> 
> So every time we read a character there is one available? How can that
> happen? Surely the CPU can run faster than the serial port?

For a quick experiment, I added a infinite while loop[1] instead of
-EAGAIN. I never hit this case when downloading with ymodem. File
downloaded successfully with $patch even with the infinite while loop.
Am I missing something?

[1] http://pastebin.ubuntu.com/25625299/

Thanks and regards,
Lokesh

[U-Boot] [PATCH] xyz-modem: Fix timeout loop waiting with WATCHDOG

[Simon Glass]

Hi Lokesh,

On 27 September 2017 at 00:27, Lokesh Vutla <lokeshvutla@ti.com> wrote:
>
>
> On Monday 25 September 2017 07:45 AM, Simon Glass wrote:
>> Hi,
>>
>> On 16 September 2017 at 23:12, Lokesh Vutla <lokeshvutla@ti.com> wrote:
>>> Simon,
>>>
>>> On 9/16/2017 9:43 PM, Simon Glass wrote:
>>>> Hi,
>>>>
>>>> On 16 September 2017 at 07:43, Tom Rini <trini@konsulko.com> wrote:
>>>>> On Sat, Sep 16, 2017 at 05:14:31PM +0530, Lokesh Vutla wrote:
>>>>>> Commit 2c77c0d6524eb ("xyz-modem: Change getc timeout loop waiting")
>>>>>> fixes the loop delay when using a hw watchdog. But assuming that Watchdog
>>>>>> kicking is taken care of by getc(). This is not true in case of DM_SERIAL.
>>>>>> So, kick the watchdog before loop waiting, instead of relying on other
>>>>>> functions.
>>>>>>
>>>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>>>>> ---
>>>>>> - This fixes UART boot on AM335x-evm.
>>>>>>
>>>>>>  common/xyzModem.c | 2 ++
>>>>>>  1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/common/xyzModem.c b/common/xyzModem.c
>>>>>> index a0c5dfeece..8c4679473f 100644
>>>>>> --- a/common/xyzModem.c
>>>>>> +++ b/common/xyzModem.c
>>>>>> @@ -26,6 +26,7 @@
>>>>>>  #include <xyzModem.h>
>>>>>>  #include <stdarg.h>
>>>>>>  #include <crc.h>
>>>>>> +#include <watchdog.h>
>>>>>>
>>>>>>  /* Assumption - run xyzModem protocol over the console port */
>>>>>>
>>>>>> @@ -64,6 +65,7 @@ CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c)
>>>>>>  {
>>>>>>
>>>>>>    ulong now = get_timer(0);
>>>>>> +  WATCHDOG_RESET();
>>>>>>    while (!tstc ())
>>>>>>      {
>>>>>>        if (get_timer(now) > xyzModem_CHAR_TIMEOUT)
>>>>>
>>>>> My worry is that other places also assume watchdog petted by getc().
>>>>> Simon, is there a reason DM_SERIAL isn't doing what was done before?
>>>>> Thanks!
>>>>
>>>> This should really be fixed at source. Is the problems in
>>>> _serial_tstc()? The watchdog is reset with _serial_getc().
>>>
>>> Sorry, my bad. WATCHDOG_RESET() is being called in __serial_getc() when
>>> err is -EAGAIN. But looks like it is never hitting this case. This way
>>> ymodem fails as watchdog gets reset when downloading large files.
>>
>> So every time we read a character there is one available? How can that
>> happen? Surely the CPU can run faster than the serial port?
>
> For a quick experiment, I added a infinite while loop[1] instead of
> -EAGAIN. I never hit this case when downloading with ymodem. File
> downloaded successfully with $patch even with the infinite while loop.
> Am I missing something?
>
> [1] http://pastebin.ubuntu.com/25625299/

Well it is very strange. I cannot imagine that a serial line could
keep up with the CPU requesting bytes. I think this needs more
investigation.

Regards,
Simon