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

Vignesh Raghavendra vigneshr at ti.com
Fri Mar 20 08:43:07 CET 2020



On 19/03/20 7:20 pm, Rasmus Villemoes wrote:
> On 19/03/2020 14.23, Vignesh Raghavendra wrote:
>>
> 
>>> 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?
>>
>> Breaking reads into smaller units unconditionally will cause performance
>> regressions. But I would like to avoid adding new CONFIG option as well.
> 
> Hm, but how much are we talking about? I can't imagine WATCHDOG_RESET()
> taking much more than 10us - especially the rate-limited one that has an
> early return just based on reading the current time will be practically
> free to call. For me, reading 256K takes about 200ms, which I assume is
> a rather typical value. Even if I'm off by an order of magnitude on both
> numbers, we're talking about adding an extra 100us for every 20ms, i.e.
> 0.5%. And that's extremely pessimistic.
> 

256K for 200ms is <2 MB/s which is pretty slow IMO.

QSPI and OSPIs flashes are capable of up to 400MB/s read throughput.
Some of the drivers that support such faster flashes use DMA to achieve
this and therefore have higher setup overhead per read request. Most SPI
drivers are not optimized and reconfigure registers for every read
requests which adds to the overhead.
Splitting of transfers into 256K unconditionally would degrade
performance for such platforms (irrespective of whether or not watchdog
is present).


>> How about resetting the watchdog in the SPI driver's read
>> implementation? 
> 
> I'd prefer something done in the generic layer, not something specific
> to this SOC (because next month I'll have another customer with another
> board based on some ARM SOC that also decided to put on an aggressive
> gpio watchdog, and next month yet another...).
> 

So, there should at least be a config option at the minimum. OR
determine the length of transfer possible before watchdog timeout
happens by looking at bus frequency and watchdog timeout value.

> Or setting max_read_size (in struct spi_slave) to
>> smaller value in the SPI controller driver to force fragmentation of reads?
> 
> Even if one forces fragmentation in that way, the generic layer would
> still need to grow a WATCHDOG_RESET() call in the read loop, no? It also
> seems to be an abuse of max_read_size.
> 

I agree that read loop should call WATCHDOG_RESET()

Regards
Vignesh


More information about the U-Boot mailing list