Change in flashrom[master]: Add support for continuous SPI reads

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/34584 )

Change subject: Add support for continuous SPI reads
......................................................................

Add support for continuous SPI reads

Forward port the original patch from Duncan Laurie 
in commit 06ffd5263892061c5ebf46e52b7878786cf2cece so that it fits for
upstream consumption.

This patch adds support for "unbounded reads" which are for continous
reads to a SPI flash chip instead of reading in page size blocks.

This speeds up the flash process over FTDI by quite a bit, testing
the read of a 16MB part it goes from 2m11s to 0m15s.

Change-Id: I804545aeb1b827ffdd41b21024fd618475e8263a
Signed-off-by: Edward O'Callaghan 
---
M chipdrivers.h
M flash.h
M spi.c
M spi25.c
4 files changed, 40 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/84/34584/1

diff --git a/chipdrivers.h b/chipdrivers.h
index e380878..77e1943 100644
--- a/chipdrivers.h
+++ b/chipdrivers.h
@@ -55,6 +55,7 @@
 int spi_chip_write_1(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len);
 int spi_nbyte_read(struct flashctx *flash, unsigned int addr, uint8_t *bytes, unsigned int len);
 int spi_read_chunked(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len, unsigned int chunksize);
+int spi_read_unbound(struct flashchip *flash, uint8_t *buf, unsigned int start, unsigned int len, unsigned int chunksize);
 int spi_write_chunked(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len, unsigned int chunksize);
 int spi_enter_4ba(struct flashctx *flash);
 int spi_exit_4ba(struct flashctx *flash);
diff --git a/flash.h b/flash.h
index b60a980..981f88b 100644
--- a/flash.h
+++ b/flash.h
@@ -143,6 +143,8 @@

#define ERASED_VALUE(flash)	(((flash)->chip->feature_bits & FEATURE_ERASED_ZERO) ? 0x00 : 0xff)

+#define FEATURE_UNBOUND_READ   (1 << 19)
+
 enum test_state {
    OK = 0,
    NT = 1,	/* Not tested */
diff --git a/spi.c b/spi.c
index 489baf3..23f4cfe 100644
--- a/spi.c
+++ b/spi.c
@@ -75,13 +75,18 @@
    	     unsigned int len)
 {
    unsigned int max_data = flash->mst->spi.max_data_read;
+       int rc;
    if (max_data == MAX_DATA_UNSPECIFIED) {
    	msg_perr("%s called, but SPI read chunk size not defined "
    		 "on this hardware. Please report a bug at "
    		 "[e-mail]\n", __func__);
    	return 1;
    }
-	return spi_read_chunked(flash, buf, start, len, max_data);
+       if (flash->feature_bits & FEATURE_UNBOUND_READ)
+               rc = spi_read_unbound(flash, buf, start, len, max_data);
+       else
+               rc = spi_read_chunked(flash, buf, start, len, max_data);
+       return rc;
 }

int default_spi_write_256(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len)
diff --git a/spi25.c b/spi25.c
index 2a1d492..3327110 100644
--- a/spi25.c
+++ b/spi25.c
@@ -659,6 +659,37 @@
 }

/*
+ * Read a part of the flash chip.
+ * Ignore pages and read the data continuously, the only bound is the chunksize.
+ */
+int spi_read_unbound(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len, unsigned int chunksize)
+{
+	int rc = 0;
+	unsigned int i;
+
+	for (i = start; i < (start + len); i += chunksize) {
+		int chunk_status = 0;
+		unsigned int toread = min(chunksize, start + len - i);
+
+		chunk_status = spi_nbyte_read(flash, i, buf + (i - start), toread);
+		if (chunk_status) {
+			if (ignore_error(chunk_status)) {
+				/* fill this chunk with 0xff bytes and
+				   let caller know about the error */
+				memset(buf + (i - start), 0xff, toread);
+				rc = chunk_status;
+				continue;
+			} else {
+				rc = chunk_status;
+				break;
+			}
+		}
+	}
+
+	return rc;
+}
+
+/*
  * Write a part of the flash chip.
  * FIXME: Use the chunk code from Michael Karcher instead.
  * Each page is written separately in chunks with a maximum size of chunksize.

[Edward O'Callaghan (Code Review)]

Hello Alan Green, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/34584

to look at the new patch set (#2).

Change subject: Add support for continuous SPI reads
......................................................................

Add support for continuous SPI reads

Forward port the original patch from Duncan Laurie 
in commit 06ffd5263892061c5ebf46e52b7878786cf2cece so that it fits for
upstream consumption.

This patch adds support for "unbounded reads" which are for continous
reads to a SPI flash chip instead of reading in page size blocks.

This speeds up the flash process over FTDI by quite a bit, testing
the read of a 16MB part it goes from 2m11s to 0m15s.

Change-Id: I804545aeb1b827ffdd41b21024fd618475e8263a
Signed-off-by: Edward O'Callaghan 
---
M chipdrivers.h
M flash.h
M spi.c
M spi25.c
4 files changed, 32 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/84/34584/2

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34584 )

Change subject: Add support for continuous SPI reads
......................................................................

Patch Set 2: Code-Review-1

This is obsolete. We changed the spi_read_chunked() implementation
long ago, because nobody knew any SPI flash that would have a page-
size limitation for reads. So far nobody complained.

Nice to see, though, that my patches get around :)

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34584 )

Change subject: Add support for continuous SPI reads
......................................................................

Patch Set 2:
> Patch Set 2: Code-Review-1
>
> This is obsolete. We changed the spi_read_chunked() implementation
> long ago, because nobody knew any SPI flash that would have a page-
> size limitation for reads. So far nobody complained.
>
> Nice to see, though, that my patches get around :)

Thanks for the review Nico, do you mind pointing me to the commit you are referring to so I can get my head around reconciling the differences between the two trees.

We have a feature flag for chips called UNBOUND_READ that builds on top of this, are you suggesting that is completely obsolete and not needed for each chip definition now?

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34584 )

Change subject: Add support for continuous SPI reads
......................................................................

Patch Set 2:
> > This is obsolete. We changed the spi_read_chunked() implementation
> > long ago, because nobody knew any SPI flash that would have a page-
> > size limitation for reads. So far nobody complained.
> >
> > Nice to see, though, that my patches get around :)
>
> Thanks for the review Nico, do you mind pointing me to the commit you are referring to so I can get my head around reconciling the differences between the two trees.
>
> We have a feature flag for chips called UNBOUND_READ that builds on top of this, are you suggesting that is completely obsolete and not needed for each chip definition now?

Just for thoroughness, what was roughly mentioned on IRC:
* Support was added upstream with 731316a9 (Enable continuous SPI reads)
* A flag doesn't seem to be needed, all SPI flashes should support it.

And my personal opinion on the `ignore_error()` feature downstream:
* Doesn't seem useful: why tell flashrom to do something that can't be done in the first place?
* spi_read_unbound(), downstream, looks broken: if the `chunksize` doesn't align with a protected region, it would fake data outside of that region.