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