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

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

[Rasmus Villemoes]

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.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/mtd/spi/spi-nor-core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 4076646225..b7f7aa7b28 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -20,6 +20,7 @@
 #include <linux/mtd/spi-nor.h>
 #include <spi-mem.h>
 #include <spi.h>
+#include <watchdog.h>
 
 #include "sf_internal.h"
 
@@ -399,6 +400,7 @@ static int spi_nor_ready(struct spi_nor *nor)
 {
 	int sr, fsr;
 
+	WATCHDOG_RESET();
 	sr = spi_nor_sr_ready(nor);
 	if (sr < 0)
 		return sr;
-- 
2.23.0

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

[Vignesh Raghavendra]

Hi,

On 17/03/20 1:48 am, 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.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/mtd/spi/spi-nor-core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index 4076646225..b7f7aa7b28 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -20,6 +20,7 @@
>  #include <linux/mtd/spi-nor.h>
>  #include <spi-mem.h>
>  #include <spi.h>
> +#include <watchdog.h>
>  
>  #include "sf_internal.h"
>  
> @@ -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())?


>  	sr = spi_nor_sr_ready(nor);
>  	if (sr < 0)
>  		return sr;
> 

Regards
Vignesh

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

[Rasmus Villemoes]

On 19/03/2020 12.25, Vignesh Raghavendra wrote:
> Hi,
> 
> On 17/03/20 1:48 am, 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.
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>>  drivers/mtd/spi/spi-nor-core.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index 4076646225..b7f7aa7b28 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -20,6 +20,7 @@
>>  #include <linux/mtd/spi-nor.h>
>>  #include <spi-mem.h>
>>  #include <spi.h>
>> +#include <watchdog.h>
>>  
>>  #include "sf_internal.h"
>>  
>> @@ -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)

Rasmus

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

[Vignesh Raghavendra]

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

[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

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

[Vignesh Raghavendra]

On 19/03/20 6:14 pm, Rasmus Villemoes wrote:
>> 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
> 

Understood.

> 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.

How about resetting the watchdog in the SPI driver's read
implementation? Or setting max_read_size (in struct spi_slave) to
smaller value in the SPI controller driver to force fragmentation of reads?

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

[Rasmus Villemoes]

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.

> 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...).

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.

Rasmus

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

[Kuldeep Singh]

Hi Vignesh,

> -----Original Message-----
> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Rasmus
> Villemoes
> Sent: Tuesday, March 17, 2020 1:49 AM
> To: u-boot at lists.denx.de
> Cc: Jagan Teki <jagan@amarulasolutions.com>; Vignesh R
> <vigneshr@ti.com>; Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Subject: [EXT] [PATCH] mtd: spi-nor-core: call WATCHDOG_RESET() in
> spi_nor_ready()
> 
> Caution: EXT Email
> 
> 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.

I also stumbled with the same problem of board resetting but it was in order of MBs and not in KBs.
Board gets reset while performing burst erase/write(for 48M erase, resets at 16M~) and read operation on 16M+.
I provided fix [1] in driver itself(add 1us delay) assuming it to be soc specific which solves problem for me and able to perform complete flash size operations.

Thanks 
Kuldeep

[1] https://patchwork.ozlabs.org/patch/1243005/

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

[Rasmus Villemoes]

On 19/03/2020 15.52, Kuldeep Singh wrote:
> Hi Vignesh,
> 

>> 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.
> 
> I also stumbled with the same problem of board resetting but it was in order of MBs and not in KBs.
> Board gets reset while performing burst erase/write(for 48M erase, resets at 16M~) and read operation on 16M+.
> I provided fix [1] in driver itself(add 1us delay) assuming it to be soc specific which solves problem for me and able to perform complete flash size operations.

Eh, but is it the short delay that fixes the problem you saw, or the
implicit WATCHDOG_RESET() done inside the udelay() function?

Rasmus

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

[Vignesh Raghavendra]

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

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

[Rasmus Villemoes]

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.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/mtd/spi/spi-nor-core.c | 2 ++
 drivers/mtd/spi/spi-nor-tiny.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 7b6ad495ac..c5d98debf0 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -22,6 +22,7 @@
 #include <linux/mtd/spi-nor.h>
 #include <spi-mem.h>
 #include <spi.h>
+#include <watchdog.h>
 
 #include "sf_internal.h"
 
@@ -424,6 +425,7 @@ static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor,
 	unsigned long timebase;
 	int ret;
 
+	WATCHDOG_RESET();
 	timebase = get_timer(0);
 
 	while (get_timer(timebase) < timeout) {
diff --git a/drivers/mtd/spi/spi-nor-tiny.c b/drivers/mtd/spi/spi-nor-tiny.c
index ccc0ab07af..d91989567d 100644
--- a/drivers/mtd/spi/spi-nor-tiny.c
+++ b/drivers/mtd/spi/spi-nor-tiny.c
@@ -21,6 +21,7 @@
 #include <linux/mtd/spi-nor.h>
 #include <spi-mem.h>
 #include <spi.h>
+#include <watchdog.h>
 
 #include "sf_internal.h"
 
@@ -324,6 +325,7 @@ static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor,
 	unsigned long timebase;
 	int ret;
 
+	WATCHDOG_RESET();
 	timebase = get_timer(0);
 
 	while (get_timer(timebase) < timeout) {
-- 
2.23.0

[PATCH v2 2/2] spi: allow calling WATCHDOG_RESET during long reads

[Rasmus Villemoes]

Some boards have a watchdog with a short (~1s) timeout and a slowish
nor flash. For example, I'm currently working on a board where doing a
2MB read from the flash will cause the board to reset.

Similar to the various CHUNKSZ, CHUNKSZ_SHA1 etc. defines that are
used to chop hash digest and/or memmove operations into chunks, doing
a WATCHDOG_RESET for each, introduce a CONFIG_SPI_FLASH_READ_CHUNKSZ
config knob. We keep the default of doing the whole read in one go,
but the board config can set a suitable threshold.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/mtd/spi/Kconfig        | 12 ++++++++++++
 drivers/mtd/spi/spi-nor-core.c |  4 +++-
 drivers/mtd/spi/spi-nor-tiny.c |  4 +++-
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi/Kconfig b/drivers/mtd/spi/Kconfig
index 018e8c597e..9dda0047d2 100644
--- a/drivers/mtd/spi/Kconfig
+++ b/drivers/mtd/spi/Kconfig
@@ -169,6 +169,18 @@ config SPI_FLASH_USE_4K_SECTORS
 	  Please note that some tools/drivers/filesystems may not work with
 	  4096 B erase size (e.g. UBIFS requires 15 KiB as a minimum).
 
+config SPI_FLASH_READ_CHUNKSZ
+	int "Chunk size for reading from SPI flash"
+	depends on SPI_FLASH
+	default 0
+	help
+	  Some boards have a watchdog with a very short timeout
+	  period. Doing large reads from a SPI flash on such a board
+	  causes the watchdog to fire and reset the board. Setting
+	  this option to a non-zero value will ensure that
+	  watchdog_reset() gets called after each read of that many
+	  bytes.
+
 config SPI_FLASH_DATAFLASH
 	bool "AT45xxx DataFlash support"
 	depends on SPI_FLASH && DM_SPI_FLASH
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index c5d98debf0..8c846a4b42 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -82,6 +82,7 @@ static ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
 				   SPI_MEM_OP_DUMMY(nor->read_dummy, 1),
 				   SPI_MEM_OP_DATA_IN(len, buf, 1));
 	size_t remaining = len;
+	size_t chunksz = CONFIG_SPI_FLASH_READ_CHUNKSZ ?: UINT_MAX;
 	int ret;
 
 	/* get transfer protocols. */
@@ -94,7 +95,7 @@ static ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
 	op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
 
 	while (remaining) {
-		op.data.nbytes = remaining < UINT_MAX ? remaining : UINT_MAX;
+		op.data.nbytes = min(remaining, chunksz);
 		ret = spi_mem_adjust_op_size(nor->spi, &op);
 		if (ret)
 			return ret;
@@ -102,6 +103,7 @@ static ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
 		ret = spi_mem_exec_op(nor->spi, &op);
 		if (ret)
 			return ret;
+		WATCHDOG_RESET();
 
 		op.addr.val += op.data.nbytes;
 		remaining -= op.data.nbytes;
diff --git a/drivers/mtd/spi/spi-nor-tiny.c b/drivers/mtd/spi/spi-nor-tiny.c
index d91989567d..e9d490ba52 100644
--- a/drivers/mtd/spi/spi-nor-tiny.c
+++ b/drivers/mtd/spi/spi-nor-tiny.c
@@ -81,6 +81,7 @@ static ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
 				   SPI_MEM_OP_DUMMY(nor->read_dummy, 1),
 				   SPI_MEM_OP_DATA_IN(len, buf, 1));
 	size_t remaining = len;
+	size_t chunksz = CONFIG_SPI_FLASH_READ_CHUNKSZ ?: UINT_MAX;
 	int ret;
 
 	/* get transfer protocols. */
@@ -93,7 +94,7 @@ static ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
 	op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
 
 	while (remaining) {
-		op.data.nbytes = remaining < UINT_MAX ? remaining : UINT_MAX;
+		op.data.nbytes = min(remaining, chunksz);
 		ret = spi_mem_adjust_op_size(nor->spi, &op);
 		if (ret)
 			return ret;
@@ -101,6 +102,7 @@ static ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
 		ret = spi_mem_exec_op(nor->spi, &op);
 		if (ret)
 			return ret;
+		WATCHDOG_RESET();
 
 		op.addr.val += op.data.nbytes;
 		remaining -= op.data.nbytes;
-- 
2.23.0

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

[Wolfgang Denk]

Dear Vignesh,

In message <20200320101448.10714-1-rasmus.villemoes@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?

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?

Function

	spi_nor_convert_3to4_read()
	spi_nor_set_4byte_opcodes()

looks like it has not been updated/synced for some time.

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.


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

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.

Do you have any intentions to clean this up any time soon?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Common sense and a sense of humor  are  the  same  thing,  moving  at
different speeds.  A sense of humor is just common sense, dancing.
                                                        - Clive James

[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@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
> 

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

[Wolfgang Denk]

Dear Vignesh,

In message <05694b0e-50a1-de5d-25d8-0444a2caeff3@ti.com> you wrote:
>
> 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.

I fully understand this goal.

> 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

#ifdef is one way to implement conditioan code, plain C code like

	if (IS_ENABLED(CONFIG_<something)) {
		...
	}

is another, usually much cleaner.

> 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.

This is all speculation, and experience says that this risks and
disadvantages of duplicated code are much higher.

> 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.

Yes, and the result is that you have two different implementations
that are out of sync from day one after you created them, bugs get
fixed here but no there, support for new chips same, etc.

> 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.

I agree that the reorganization will take additional efforts, but in
the long term, maintenance efforts will be much smaller, as you have
to maintain one common code base only.  And if you add new features
and see that they have negative impact on the SPL configurations,
you can always encapsulate these parts in IS_ENABLED() code.

> Please, let me know?

Well, for large parts things are pretty easy:

> > 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;

For these functions there is absolutely no justification to have
them duplicated.

> > 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.

Well, maybe this rework should consider the idea of having common
code both for normal and size-limited use cases?

In the current form, the differences are so small they could easily
be handled by a few macro definitions so the code would be
indentical again.

Maybe this is also possible in your rework?

> > 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.

OK - I did not look deeply enough into the code if it was just new
features that will never be needed in the SPL, of if they might
actually be needed, or if they were actually bug fixes.

You are the expert here, so I trust your assessment.

> Any addition to spi-nor-tiny.c should be debated whether or not that
> change is absolutely needed for low footprint boards.

Agreed.

> > 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.

These differences are tricial to handle using IS_ENABLED() for code
parts that are needed only when erase/write support is configured.


> > 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.

You could still use the same method in both implementations, right?

> 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.

Code duplication never evermakes sense to me. It is just a cause of
errors and mental pain.

I would really appreciate if youc ould clean this up.

Thanks!

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Veni, Vidi, VISA:
        I came, I saw, I did a little shopping.

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

[Vignesh Raghavendra]

On 24/03/20 8:11 pm, Wolfgang Denk wrote:
> Dear Vignesh,
> 
> In message <05694b0e-50a1-de5d-25d8-0444a2caeff3@ti.com> you wrote:
>>
>> 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.
> 
> I fully understand this goal.
> 
>> 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
> 
> #ifdef is one way to implement conditioan code, plain C code like
> 
> 	if (IS_ENABLED(CONFIG_<something)) {
> 		...
> 	}
> 
> is another, usually much cleaner.
> 
>> 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.
> 
> This is all speculation, and experience says that this risks and
> disadvantages of duplicated code are much higher.
> 
>> 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.
> 
> Yes, and the result is that you have two different implementations
> that are out of sync from day one after you created them, bugs get
> fixed here but no there, support for new chips same, etc.
> 

I fully understand your concerns and will work on unifying the two
stacks with IS_ENABLED() macro so that there will still be a tiny stack
with same memory footprint.

But I want to state that the differences currently in spi-nor-tiny.c vs
spi-nor-core.c are intentional. I don't think there have been any fixes
in the main code missing from tiny stack.
Features such as Octal mode and other stuff have been intentionally kept
out of spi-nor-tiny to avoid code size increase.

Appreciate all the suggestions!

Regards
Vignesh

[...]

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

[Wolfgang Denk]

Dear Vignesh,

In message <9a1e75ac-135a-26aa-2ded-784fbe14bc14@ti.com> you wrote:
> 
> I fully understand your concerns and will work on unifying the two
> stacks with IS_ENABLED() macro so that there will still be a tiny stack
> with same memory footprint.

Thanks!!

> But I want to state that the differences currently in spi-nor-tiny.c vs
> spi-nor-core.c are intentional. I don't think there have been any fixes
> in the main code missing from tiny stack.
> Features such as Octal mode and other stuff have been intentionally kept
> out of spi-nor-tiny to avoid code size increase.

Understood.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
As long as we're going to reinvent the wheel again, we might as  well
try making it round this time.                        - Mike Dennison

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

[Rasmus Villemoes]

On 20/03/2020 11.14, 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.

Ping.

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

[Rasmus Villemoes]

On 20/05/2020 00.12, Rasmus Villemoes wrote:
> On 20/03/2020 11.14, 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.
> 
> Ping.
> 

Ping^2.