[flashrom-gerrit] Change in ...flashrom[master]: spi: Move 16MiB partitioning up into spi_chip_read()

[Nico Huber (Code Review)]

Hello Carl-Daniel Hailfinger, ron minnich, David Hendricks,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: spi: Move 16MiB partitioning up into spi_chip_read()
......................................................................

spi: Move 16MiB partitioning up into spi_chip_read()

We enforced a 16MiB limit in spi_read_chunked() for multi-die flash
chips that can't be fully read at once. The same limit can be useful
for dediprog programmers. So move it into a more generic place.

Untested.

Change-Id: Iab1fd5b2ea550b4b3ef3e8402e0b6ca218485a51
Signed-off-by: Nico Huber 
---
M flash.h
M spi.c
M spi25.c
3 files changed, 21 insertions(+), 33 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/33613/1

diff --git a/flash.h b/flash.h
index 5bbfa0a..3a7713e 100644
--- a/flash.h
+++ b/flash.h
@@ -37,6 +37,9 @@
 #include "libflashrom.h"
 #include "layout.h"

+#define KiB (1024)
+#define MiB (1024 * KiB)
+
 #define ERROR_PTR ((void*)-1)

/* Error codes */
diff --git a/spi.c b/spi.c
index a4dba19..cdf43c6 100644
--- a/spi.c
+++ b/spi.c
@@ -99,7 +99,15 @@
 int spi_chip_read(struct flashctx *flash, uint8_t *buf, unsigned int start,
    	  unsigned int len)
 {
-	return flash->mst->spi.read(flash, buf, start, len);
+	int ret;
+	size_t to_read;
+	for (; len; len -= to_read, buf += to_read, start += to_read) {
+		to_read = min(16*MiB, len);
+		ret = flash->mst->spi.read(flash, buf, start, to_read);
+		if (ret)
+			return ret;
+	}
+	return 0;
 }

/*
diff --git a/spi25.c b/spi25.c
index fd87dc9..cca2a04 100644
--- a/spi25.c
+++ b/spi25.c
@@ -657,43 +657,20 @@

/*
  * Read a part of the flash chip.
- * FIXME: Use the chunk code from Michael Karcher instead.
- * Each naturally aligned area is read separately in chunks with a maximum size of chunksize.
+ * Data is read in chunks with a maximum size of chunksize.
  */
 int spi_read_chunked(struct flashctx *flash, uint8_t *buf, unsigned int start,
    	     unsigned int len, unsigned int chunksize)
 {
-	int rc = 0;
-	unsigned int i, j, starthere, lenhere, toread;
-	/* Limit for multi-die 4-byte-addressing chips. */
-	unsigned int area_size = min(flash->chip->total_size * 1024, 16 * 1024 * 1024);
-
-	/* Warning: This loop has a very unusual condition and body.
-	 * The loop needs to go through each area with at least one affected
-	 * byte. The lowest area number is (start / area_size) since that
-	 * division rounds down. The highest area number we want is the area
-	 * where the last byte of the range lives. That last byte has the
-	 * address (start + len - 1), thus the highest area number is
-	 * (start + len - 1) / area_size. Since we want to include that last
-	 * area as well, the loop condition uses <=.
-	 */
-	for (i = start / area_size; i <= (start + len - 1) / area_size; i++) {
-		/* Byte position of the first byte in the range in this area. */
-		/* starthere is an offset to the base address of the chip. */
-		starthere = max(start, i * area_size);
-		/* Length of bytes in the range in this area. */
-		lenhere = min(start + len, (i + 1) * area_size) - starthere;
-		for (j = 0; j < lenhere; j += chunksize) {
-			toread = min(chunksize, lenhere - j);
-			rc = spi_nbyte_read(flash, starthere + j, buf + starthere - start + j, toread);
-			if (rc)
-				break;
-		}
-		if (rc)
-			break;
+	int ret;
+	size_t to_read;
+	for (; len; len -= to_read, buf += to_read, start += to_read) {
+		to_read = min(chunksize, len);
+		ret = spi_nbyte_read(flash, start, buf, to_read);
+		if (ret)
+			return ret;
    }
-
-	return rc;
+	return 0;
 }

/*