[U-Boot] [PATCH] CFI Driver: Reset watchdog timer after each flash operation

[U-Boot] [PATCH] CFI Driver: Reset watchdog timer after each flash operation

[Ingo van Lil]

The CFI driver does not reset the device's watchdog, so long-running
flash operations will cause the watchdog timer to expire. A comment in
flash_status_check() suggests that udelay() is expected to reset the
watchdog, but I can't find any architecture where it does.

Signed-off-by: Ingo van Lil <inguin@gmx.de>
---
 drivers/mtd/cfi_flash.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
index 6eea49a..5dcd62d 100644
--- a/drivers/mtd/cfi_flash.c
+++ b/drivers/mtd/cfi_flash.c
@@ -39,6 +39,7 @@
 #include <asm/io.h>
 #include <asm/byteorder.h>
 #include <environment.h>
+#include <watchdog.h>
 
 /*
  * This file implements a Common Flash Interface (CFI) driver for
@@ -675,7 +676,8 @@ static int flash_status_check (flash_info_t * info, flash_sect_t sector,
 			flash_write_cmd (info, sector, 0, info->cmd_reset);
 			return ERR_TIMOUT;
 		}
-		udelay (1);		/* also triggers watchdog */
+		udelay (1);
+		WATCHDOG_RESET();
 	}
 	return ERR_OK;
 }
-- 
1.6.2.5

[U-Boot] [PATCH] CFI Driver: Reset watchdog timer after each flash operation

[Stefan Roese]

Hi Ingo,

On Friday 02 October 2009 12:34:17 Ingo van Lil wrote:
> The CFI driver does not reset the device's watchdog, so long-running
> flash operations will cause the watchdog timer to expire. A comment in
> flash_status_check() suggests that udelay() is expected to reset the
> watchdog, but I can't find any architecture where it does.

PPC does it this way. udelay() in lib_ppc/time.c calls wait_ticks(). And here 
you will find WATCHDOG_RESET.

Which platform are you using? I support this needs to be fixed in your 
platform.
 
Cheers,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de

[U-Boot] [PATCH] CFI Driver: Reset watchdog timer after each flash operation

[Ingo van Lil]

On 10/02/2009 01:06 PM, Stefan Roese wrote:

>> The CFI driver does not reset the device's watchdog, so long-running
>> flash operations will cause the watchdog timer to expire. A comment in
>> flash_status_check() suggests that udelay() is expected to reset the
>> watchdog, but I can't find any architecture where it does.
>
> PPC does it this way. udelay() in lib_ppc/time.c calls wait_ticks(). And here
> you will find WATCHDOG_RESET.

You're right. Seems to be an exception, though: According to ctags there 
are 40 separate implementations of udelay(), and the ones in lib_ppc and 
lib_nios seem to be the only ones that actually do call WATCHDOG_RESET.

> Which platform are you using? I support this needs to be fixed in your
> platform.

I'm using an Atmel AT91-based custom board, and the udelay() function 
can be found in cpu/arm926ejs/at91/timer.c. Unfortunately there's no 
central udelay() implementation in lib_arm.

Regards,
Ingo

[U-Boot] [PATCH] CFI Driver: Reset watchdog timer after each flash operation

[Wolfgang Denk]

Dear Ingo van Lil,

In message <4AC5EE3B.5090200@gmx.de> you wrote:
>
> > PPC does it this way. udelay() in lib_ppc/time.c calls wait_ticks(). And here
> > you will find WATCHDOG_RESET.
> 
> You're right. Seems to be an exception, though: According to ctags there 

No, not an exception, but the reference implementation. I cannot help
it that most other architectures / SoCs don;t care much.

> I'm using an Atmel AT91-based custom board, and the udelay() function 
> can be found in cpu/arm926ejs/at91/timer.c. Unfortunately there's no 
> central udelay() implementation in lib_arm.

Guess nobody attempted to use a WD on such a system before.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Backed up the system lately?

[U-Boot] [PATCH] CFI Driver: Reset watchdog timer after each flash operation

[Ingo van Lil]

On 10/04/2009 01:29 AM, Wolfgang Denk wrote:

> No, not an exception, but the reference implementation. I cannot help
> it that most other architectures / SoCs don;t care much.

Well, if such an uncommon side-effect is expected of a function with a 
well-known name it should at least be prominently documented. I like 
Mike's suggestion to have a central udelay() implementation for that; 
I'm gonna try to whip up a patch tomorrow.


>> I'm using an Atmel AT91-based custom board, and the udelay() function
>> can be found in cpu/arm926ejs/at91/timer.c. Unfortunately there's no
>> central udelay() implementation in lib_arm.
>
> Guess nobody attempted to use a WD on such a system before.

You hardly have a choice: After reset the AT91's watchdog timer runs at 
maximum period (15 seconds), and the control register is one-time 
writable. If you disable the watchdog in u-boot there's no way to 
re-enable it later in the OS.

Regards,
Ingo

[U-Boot] [PATCH] CFI Driver: Reset watchdog timer after each flash operation

[Graeme Russ]

On Sun, Oct 4, 2009 at 10:15 PM, Ingo van Lil <inguin@gmx.de> wrote:
> On 10/04/2009 01:29 AM, Wolfgang Denk wrote:
>
>> No, not an exception, but the reference implementation. I cannot help
>> it that most other architectures / SoCs don;t care much.
>
> Well, if such an uncommon side-effect is expected of a function with a
> well-known name it should at least be prominently documented. I like
> Mike's suggestion to have a central udelay() implementation for that;
> I'm gonna try to whip up a patch tomorrow.
>
>
>>> I'm using an Atmel AT91-based custom board, and the udelay() function
>>> can be found in cpu/arm926ejs/at91/timer.c. Unfortunately there's no
>>> central udelay() implementation in lib_arm.
>>
>> Guess nobody attempted to use a WD on such a system before.
>
> You hardly have a choice: After reset the AT91's watchdog timer runs at
> maximum period (15 seconds), and the control register is one-time
> writable. If you disable the watchdog in u-boot there's no way to
> re-enable it later in the OS.
>
> Regards,
> Ingo

Naive question of the day - Is there any reason for the complexity of the
watchdog implementation with all the #defines to sort out software and
hardware resets?

Wouldn't it be easier to replace it with a weak function with an empty
default implementation over-ridden at the board/arch/cpu level as needed?

We can then call the watchdog reset in udelay() - will be a problem if the
watchdog needs udelay (for a timed reset pulse for example)

Just a thought

Regards,

Graeme

[U-Boot] [PATCH] CFI Driver: Reset watchdog timer after each flash operation

[Wolfgang Denk]

Dear Ingo van Lil,

In message <20091002103417.GA9767@zaphod.peppercon.de> you wrote:
> The CFI driver does not reset the device's watchdog, so long-running
> flash operations will cause the watchdog timer to expire. A comment in
> flash_status_check() suggests that udelay() is expected to reset the
> watchdog, but I can't find any architecture where it does.

Please have a closer look, then. On PowerPC, udelay()
["lib_ppc/time.c"] calls wait_ticks(), which in turn
["lib_ppc/ticks.S"] calls WATCHDOG_RESET

If this is missing in other architectures, it should be fixed at the
root cause, i. e. in udelay() or in the respective support routines.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
As long as we're going to reinvent the wheel again, we might as  well
try making it round this time.                        - Mike Dennison

[U-Boot] [PATCH] CFI Driver: Reset watchdog timer after each flash operation

[Mike Frysinger]

On Friday 02 October 2009 08:30:51 Wolfgang Denk wrote:
> Ingo van Lil wrote:
> > The CFI driver does not reset the device's watchdog, so long-running
> > flash operations will cause the watchdog timer to expire. A comment in
> > flash_status_check() suggests that udelay() is expected to reset the
> > watchdog, but I can't find any architecture where it does.
> 
> Please have a closer look, then. On PowerPC, udelay()
> ["lib_ppc/time.c"] calls wait_ticks(), which in turn
> ["lib_ppc/ticks.S"] calls WATCHDOG_RESET
> 
> If this is missing in other architectures, it should be fixed at the
> root cause, i. e. in udelay() or in the respective support routines.

Blackfin is missing it as well as i really had no idea it was supposed to be 
there.  certainly no doc states this requirement.  perhaps it'd make sense to 
break apart the common stuff to a common udelay() that does things like call 
the watchdog and then call the implementation __udelay().  there should be at 
least a doc/README.arch that includes these kind of details ...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20091002/35cc04ba/attachment.pgp 

[U-Boot] [PATCH] CFI Driver: Reset watchdog timer after each flash operation

[Wolfgang Denk]

Dear Mike Frysinger,

In message <200910021431.23079.vapier@gentoo.org> you wrote:
>
> Blackfin is missing it as well as i really had no idea it was supposed to be 
> there.  certainly no doc states this requirement.  perhaps it'd make sense to 
> break apart the common stuff to a common udelay() that does things like call 
> the watchdog and then call the implementation __udelay().  there should be at 
> least a doc/README.arch that includes these kind of details ...

Agreed - we should start and collect such documentation about
internal data structures, APIs, "expectations" of the code and the
like to the wiki.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The heart is not a logical organ.
	-- Dr. Janet Wallace, "The Deadly Years", stardate 3479.4

[U-Boot] [PATCH] CFI Driver: Reset watchdog timer after each flash operation

[J. William Campbell]

Mike Frysinger wrote:
> On Friday 02 October 2009 08:30:51 Wolfgang Denk wrote:
>   
>> Ingo van Lil wrote:
>>     
>>> The CFI driver does not reset the device's watchdog, so long-running
>>> flash operations will cause the watchdog timer to expire. A comment in
>>> flash_status_check() suggests that udelay() is expected to reset the
>>> watchdog, but I can't find any architecture where it does.
>>>       
>> Please have a closer look, then. On PowerPC, udelay()
>> ["lib_ppc/time.c"] calls wait_ticks(), which in turn
>> ["lib_ppc/ticks.S"] calls WATCHDOG_RESET
>>
>> If this is missing in other architectures, it should be fixed at the
>> root cause, i. e. in udelay() or in the respective support routines.
>>     
>
> Blackfin is missing it as well as i really had no idea it was supposed to be 
> there.  certainly no doc states this requirement.  perhaps it'd make sense to 
> break apart the common stuff to a common udelay() that does things like call 
> the watchdog and then call the implementation __udelay().  there should be at 
> least a doc/README.arch that includes these kind of details ...
> -mike
>   
>   
At this point it might be appropriate to ask if including such a reset 
in udelay() is a good idea. The way it is, no "infinite loop" in u-boot 
that contains a udelay() will ever allow the watchdog to time out. That 
restriction is somewhat non-obvious. I would argue that there are 
basically two classes of commands in u-boot, those that should execute 
so fast that there is no way the watchdog should be triggered, and 
commands that may take "a long time" during  which the watchdog may 
erroneously be triggered if there is no provision made to reset it. In 
the second case, the resetting of the watchdog must be placed where 
"measurable progress" towards command completion is being made, i.e. 
there is a certainty that we are getting closer to a command complete. 
Just because the program invokes udelay, there is no assurance that 
measurable progress is being made. The udelay may be part of a process 
that will never complete. Therefore, having a watchdog reset in udelay 
seems like a less than optimal idea in general. Maybe now would be a 
good time to look at removing it?

Bill Campbell
> ------------------------------------------------------------------------
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>   

[U-Boot] [PATCH] CFI Driver: Reset watchdog timer after each flash operation

[Wolfgang Denk]

Dear "J. William Campbell",

In message <4AC660E3.4010404@comcast.net> you wrote:
>
> At this point it might be appropriate to ask if including such a reset 
> in udelay() is a good idea. The way it is, no "infinite loop" in u-boot 
> that contains a udelay() will ever allow the watchdog to time out. That 

Indeed. We do not have any watchdog preotection for U-Boot itself.
U-Boot just makes sure to allow booting an OS with the watchdog
activated.

> Just because the program invokes udelay, there is no assurance that 
> measurable progress is being made. The udelay may be part of a process 
> that will never complete. Therefore, having a watchdog reset in udelay 
> seems like a less than optimal idea in general. Maybe now would be a 
> good time to look at removing it?

There are other places which have similar issues. For example, you
will probably find that most serial drivers (at least those being
used on systems with active watchdogs) will have WATCHDOG_RESET()
calls in their getc() implementations - which is needed to trigger
the WD while the board is in interactive mode, waiting for input.

WD support in U-Boot is just good enough to bridge the time until the
OS starts to do the right thing. We do not trry to WD-protect U-Boot
itself yet. Implementing such protection would definitely be a good
thing to have, but non-trivial effort, too. It would require much more
changes than removing the trigger from the udelay() code.

Please feel free to submit patches...

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A door is what a dog is perpetually on the wrong side of.
                                                        - Ogden Nash