[U-Boot] [PATCH] xyz-modem: Change getc timeout loop waiting

[U-Boot] [PATCH] xyz-modem: Change getc timeout loop waiting

[Tomas Melin]

This fixes the loop delay when using a hw watchdog.

In case a watchdog is used that accesses CPU registers,
the defined delay of 20us in a tight loop will cause a
huge delay in the actual timeout seen. This is caused
by the fact that udelay will inheritantly call WATCHDOG_RESET.
Together with the omap wdt implementation, the seen timeout increases up to
around 30s. This makes the loop very slow and causes long
delays when using the modem.

Instead, implement the 2 sec loop by using the timer interface to know
when to break out of the timeout loop. Watchdog kicking is taken care of
by getc().

Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
---
 common/xyzModem.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/common/xyzModem.c b/common/xyzModem.c
index 5656aac..e0d87db 100644
--- a/common/xyzModem.c
+++ b/common/xyzModem.c
@@ -71,12 +71,12 @@ typedef int cyg_int32;
 static int
 CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c)
 {
-#define DELAY 20
-  unsigned long counter = 0;
-  while (!tstc () && (counter < xyzModem_CHAR_TIMEOUT * 1000 / DELAY))
+
+  ulong now = get_timer(0);
+  while (!tstc ())
     {
-      udelay (DELAY);
-      counter++;
+      if (get_timer(now) > xyzModem_CHAR_TIMEOUT)
+        break;
     }
   if (tstc ())
     {
-- 
2.1.4

[U-Boot] xyz-modem: Change getc timeout loop waiting

[Tom Rini]

On Mon, Nov 21, 2016 at 10:18:51AM +0200, tomas.melin at vaisala.com wrote:

> This fixes the loop delay when using a hw watchdog.
> 
> In case a watchdog is used that accesses CPU registers,
> the defined delay of 20us in a tight loop will cause a
> huge delay in the actual timeout seen. This is caused
> by the fact that udelay will inheritantly call WATCHDOG_RESET.
> Together with the omap wdt implementation, the seen timeout increases up to
> around 30s. This makes the loop very slow and causes long
> delays when using the modem.
> 
> Instead, implement the 2 sec loop by using the timer interface to know
> when to break out of the timeout loop. Watchdog kicking is taken care of
> by getc().
> 
> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>

Applied to u-boot/master, 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/20161128/1bd15f84/attachment.sig>

[U-Boot] xyz-modem: Change getc timeout loop waiting

[Alexander Sverdlin]

Hello!

On Mon, 21 Nov 2016 10:18:51 +0200
Tomas Melin <tomas.melin@vaisala.com> wrote:

> This fixes the loop delay when using a hw watchdog.
> 
> In case a watchdog is used that accesses CPU registers,
> the defined delay of 20us in a tight loop will cause a
> huge delay in the actual timeout seen. This is caused
> by the fact that udelay will inheritantly call WATCHDOG_RESET.
> Together with the omap wdt implementation, the seen timeout increases up to
> around 30s. This makes the loop very slow and causes long
> delays when using the modem.
> 
> Instead, implement the 2 sec loop by using the timer interface to know
> when to break out of the timeout loop. Watchdog kicking is taken care of
> by getc().
> 
> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>

This commit breaks YMODEM SPL->U-Boot boot on Beagle Bone,
transfer is aborted (because of timeout) after 497kb
(u-boot.img is around 570kb).
Reverting the commit repairs YMODEM boot.

> ---
>  common/xyzModem.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/common/xyzModem.c b/common/xyzModem.c
> index 5656aac..e0d87db 100644
> --- a/common/xyzModem.c
> +++ b/common/xyzModem.c
> @@ -71,12 +71,12 @@ typedef int cyg_int32;
>  static int
>  CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c)
>  {
> -#define DELAY 20
> -  unsigned long counter = 0;
> -  while (!tstc () && (counter < xyzModem_CHAR_TIMEOUT * 1000 / DELAY))
> +
> +  ulong now = get_timer(0);
> +  while (!tstc ())
>      {
> -      udelay (DELAY);
> -      counter++;
> +      if (get_timer(now) > xyzModem_CHAR_TIMEOUT)
> +        break;
>      }
>    if (tstc ())
>      {


-- 
Alexander Sverdlin.

[U-Boot] xyz-modem: Change getc timeout loop waiting

[Tomas Melin]

Hi,

On 08/01/2018 03:15 AM, Alexander Sverdlin wrote:

> This commit breaks YMODEM SPL->U-Boot boot on Beagle Bone,
> transfer is aborted (because of timeout) after 497kb
> (u-boot.img is around 570kb).
> Reverting the commit repairs YMODEM boot.

Is the timeout a watchdog timeout or some communication freeze?
In case watchdog is correctly configured, kicking should still
happen.

Tomas

>> ---
>>   common/xyzModem.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/common/xyzModem.c b/common/xyzModem.c
>> index 5656aac..e0d87db 100644
>> --- a/common/xyzModem.c
>> +++ b/common/xyzModem.c
>> @@ -71,12 +71,12 @@ typedef int cyg_int32;
>>   static int
>>   CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c)
>>   {
>> -#define DELAY 20
>> -  unsigned long counter = 0;
>> -  while (!tstc () && (counter < xyzModem_CHAR_TIMEOUT * 1000 / DELAY))
>> +
>> +  ulong now = get_timer(0);
>> +  while (!tstc ())
>>       {
>> -      udelay (DELAY);
>> -      counter++;
>> +      if (get_timer(now) > xyzModem_CHAR_TIMEOUT)
>> +        break;
>>       }
>>     if (tstc ())
>>       {
>

[U-Boot] xyz-modem: Change getc timeout loop waiting

[Alexander Sverdlin]

Hi!

On Wed, 1 Aug 2018 08:44:13 +0300
Tomas Melin <tomas.melin@vaisala.com> wrote:

> > This commit breaks YMODEM SPL->U-Boot boot on Beagle Bone,
> > transfer is aborted (because of timeout) after 497kb
> > (u-boot.img is around 570kb).
> > Reverting the commit repairs YMODEM boot.
> 
> Is the timeout a watchdog timeout or some communication freeze?

I suppose, it reports false-positive timeout back to ymodem code.
Because all it does is terminating communication gracefully (with 'C'
and a bunch of CANs).
I need to check if timer overflow is possible on Beaglebone,
because the code is obviously not overflow-proof, but on the
other hand it happens within minutes and at least the timer variable
is 32 bit...

> In case watchdog is correctly configured, kicking should still
> happen.

-- 
Alexander Sverdlin.

[U-Boot] xyz-modem: Change getc timeout loop waiting

[Tomas Melin]

Hi,

On 08/01/2018 08:54 AM, Alexander Sverdlin wrote:
>> Is the timeout a watchdog timeout or some communication freeze?
> I suppose, it reports false-positive timeout back to ymodem code.
> Because all it does is terminating communication gracefully (with 'C'
> and a bunch of CANs).
If you are using omap_wdt then check the configured value of the timeout 
(WDT_HW_TIMEOUT).
Time the y-modem loading, and if the reset happens just after that 
configured value, it would indicate that the watchdog is not kicked, for 
one reason or another.

Tomas

[U-Boot] xyz-modem: Change getc timeout loop waiting

[Alexander Sverdlin]

Hello Tomas,

On Wed, 1 Aug 2018 09:16:38 +0300
Tomas Melin <tomas.melin@vaisala.com> wrote:

> >> Is the timeout a watchdog timeout or some communication freeze?
> > I suppose, it reports false-positive timeout back to ymodem code.
> > Because all it does is terminating communication gracefully (with 'C'
> > and a bunch of CANs).
> If you are using omap_wdt then check the configured value of the timeout 
> (WDT_HW_TIMEOUT).
> Time the y-modem loading, and if the reset happens just after that 
> configured value, it would indicate that the watchdog is not kicked, for 
> one reason or another.

Yes, I'm using omap_wdt with default 60 seconds timeout, but as I mentioned
already, it's not WDT kicking in, but your patch breaking YMODEM protocol
reporting false timeout back to YMODEM code.

-- 
Alexander Sverdlin.