On 19/03/2020 13.28, Vignesh Raghavendra wrote:
On 19/03/20 5:09 pm, Rasmus Villemoes wrote:
On 19/03/2020 12.25, Vignesh Raghavendra wrote:
Hi,
[...]
@@ -399,6 +400,7 @@ static int spi_nor_ready(struct spi_nor *nor) { int sr, fsr;
- WATCHDOG_RESET();
Is it necessary to reset watchdog for every status register read? How small is the timeout? Resetting too often will impact performance depending on overhead of this call.
Is it not sufficient to reset watchdog once per page write (in spi_nor_write()) and once per sector erase (in spi_nor_erase())?
Probably, yes. I was a bit torn between putting the call here or in spi_nor_wait_till_ready(). That should do it once per erase/page write which should be fine (well, if the busy-looping for spi_nor_ready takes more than the watchdog timeout, the board will reset, but I don't think the flash is that bad).> I'll test that, but I just found out I'll need something in the read path as well. Reading 1MB works fine, reading 2MB resets:
[2020-03-19 12:31:11.923] => echo a ; sf read $loadaddr 0 0x100000 ; echo b [2020-03-19 12:31:32.724] a [2020-03-19 12:31:32.724] device 0 offset 0x0, size 0x100000 [2020-03-19 12:31:33.586] SF: 1048576 bytes @ 0x0 Read: OK [2020-03-19 12:31:33.586] b [2020-03-19 12:31:33.586] => echo a ; sf read $loadaddr 0 0x200000 ; echo b [2020-03-19 12:31:40.771] a [2020-03-19 12:31:40.771] device 0 offset 0x0, size 0x200000 [2020-03-19 12:31:42.666] [2020-03-19 12:31:42.666] U-Boot SPL 2020.01-00078-g058da1a-dirty (Mar 17 2020 - 16:27:58 +0000)
Hmm, Watchdog seems to be set to trigger after ~2s of inactivity. Isn't that too small? WATCHDOG_RESET() resets only once per second, so 2 second timeout is too close IMO.
Typical watchdog timeouts are usually in tens of seconds
Nope, not with this external gpio-triggered one. The data sheet says
Watchdog timeout period 1.12/1.60/2.24 (min/typ/max)
so ~2 seconds sounds about right - and I have to account for other instances of the board that may be a lot closer to the minimum. The timeout is fixed, nothing in software can affect it. So you see why I cannot afford to let spi flash operations take several seconds to complete without a WATCHDOG_RESET().
And yes, I'm very well aware of the rate-limiting imposed by the wdt-provided watchdog_reset() function - that's mostly a solved problem: https://patchwork.ozlabs.org/project/uboot/list/?series=164254
For the read side, it seems that just replacing the UINT_MAX in the two copies of spi_nor_read_data with some smaller constant should suffice. But I don't know if I should make that smaller constant a CONFIG_* option or just pick something like 256K. Thoughts?
Rasmus