[PATCH v2 1/2] spi: call WATCHDOG_RESET() in spi_nor_wait_till_ready_with_timeout()

[Vignesh Raghavendra]

Dear Wolfgang,

On 20/03/20 4:48 pm, Wolfgang Denk wrote:
> Dear Vignesh,
> 
> In message <20200320101448.10714-1-rasmus.villemoes at prevas.dk> Rasmus Villemoes wrote:
>> I have a board for which doing "sf erase 0x100000 0x80000"
>> consistently causes the external watchdog circuit to reset the
>> board. Make sure to pet the watchdog during slow operations such as
>> erasing or writing large areas of a spi nor flash.
> ...
> 
>>  drivers/mtd/spi/spi-nor-core.c | 2 ++
>>  drivers/mtd/spi/spi-nor-tiny.c | 2 ++
> 
> Rasmus' patch triggers a few interesting questions about the SPI NOR
> code:
> 
> 
> Is there any specific reason why spi-nor-core.c and spi-nor-tiny.c
> contain large parts of absolutely identical code?
> 

Aim of spi-nor-tiny.c is to have a tiny stack that can be used in
SPL/TPL or on resource constraint boards to only support _reading_ from
the flash. So tiny stack would be subset of spi-nor-core.

There are few options here:
One is to have single driver and hide things that are not required for
tiny stack under #ifdef. But this makes code harder to read and modify

Second option, is to factor out common functions into a separate file as
a library. This would avoid ifdef'ry. But whenever a new feature is
added that would add to the size of these common functions, it would be
probably mean moving it out of common library and into individual
stacks. This may need to unnecessary code churn whenever a new feature
is added.

So, suggestion was to add a parallel tiny stack (which was supposed to
plug into tiny read only MTD stack) that only supports reading from
flash. This would mean that new features can be freely added to
spi-nor-core.c without worrying about bloating SPL for older devices.

If the opinion is to switch to second option now, then I can rework the
framework. But note that this would make adding new features bit harder
due to need to maintain size of spi-nor-tiny.c.
Please, let me know?

> All the functions
> 
> 	spi_nor_read_write_reg()
> 	spi_nor_read_reg()
> 	spi_nor_write_reg()
> 	spi_nor_read_data()
> 	mtd_to_spi_nor()
> 	spi_nor_convert_opcode()
> 	spi_nor_ready()
> 	spi_nor_wait_till_ready_with_timeout()
> 	spi_nor_wait_till_ready()
> 	macronix_quad_enable()
> 	spansion_read_cr_quad_enable()
> 	spi_nor_set_read_settings()
> 
> 
> are absolutely identical;
> 
> functions
> 
> 	read_cr()
> 	write_sr()
> 	write_disable()
> 	set_4byte()
> 	spi_nor_read()
> 	write_sr_cr()
> 
> are mostly identical, but I wonder if the differences (like
> nor->write_reg() versus spi_nor_write_reg()) is indeed intended to
> save memory footprint or lack an update to later code?
> 

I am in the process of dropping nor->*() functions altogether as I don't
see any users outside of spi-nor-core.c

Note that some of these will no longer be same with 8D-8D-8D support[1]
thus further reducing the similarities.

[1] https://patchwork.ozlabs.org/project/uboot/list/?series=166175

> Function
> 
> 	spi_nor_convert_3to4_read()
> 	spi_nor_set_4byte_opcodes()
> 
> looks like it has not been updated/synced for some time.

Thats intentional... Adding Octal mode support to tiny stack would add
to code size and possibly break few boards.
Any addition to spi-nor-tiny.c should be debated whether or not that
change is absolutely needed for low footprint boards.

> 
> Function
> 
> 	spi_nor_sr_ready()
> 	spi_nor_fsr_ready()
> 
> is lacking error handling in spi-nor-tiny.c; and the rror handling
> code in spi-nor-core.c is also mostly duplicated a couple or times.
> 

Error handling is not required, as tiny stack does not support writing
to flash and these errors are raised when writing or erasing flash.

> 
> Function
> 
> 	spi_nor_read_id()
> 
> differs in "interesting" ways, i. e. we have 
> 
> 370         info = spi_nor_ids;
> 371         for (; info->sector_size != 0; info++) {
> 372                 if (info->id_len) {
> 
> here, and


In case of tiny stack, we save space by not storing flash names in
spi_nor_ids[] table (its a significant saving) and hence have to rely on
another field to detect EOL.

> 
> 894         info = spi_nor_ids;
> 895         for (; info->name; info++) {
> 896                 if (info->id_len) {
> 
> there, while all the rest is idential.  Lack of synchronization?
> 
> 
> Also the differences in
> 
> 	spi_nor_select_read()
> 
> make we wonder...
> 
> 
> 
> This extensive code duplication looks really painful and error prone
> to me.

Duplication is to avoid feature creep leading to increase in code size.
But I can factor out common code if there is a wider agreement.

> 
> Do you have any intentions to clean this up any time soon?
> 
> Best regards,
> 
> Wolfgang Denk
>