[U-Boot] [PATCH] CFI Driver: Reset watchdog timer after each flash operation
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; }
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@denx.de
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
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
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
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
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
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
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
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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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
participants (6)
-
Graeme Russ -
Ingo van Lil -
J. William Campbell -
Mike Frysinger -
Stefan Roese -
Wolfgang Denk