[PATCH] mtd: spi-nor-core: call WATCHDOG_RESET() in spi_nor_ready()

[Rasmus Villemoes]

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