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 nico.h@gmx.de --- 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; }
/*
Hello Carl-Daniel Hailfinger, ron minnich, David Hendricks, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/33613
to look at the new patch set (#2).
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 nico.h@gmx.de --- M flash.h M spi.c M spi25.c 3 files changed, 24 insertions(+), 33 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/33613/2
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33613 )
Change subject: spi: Move 16MiB partitioning up into spi_chip_read() ......................................................................
Patch Set 2: Code-Review+2
It's worth pointing out that this makes the loop look a lot less like the one in spi_write_chunked(). spi_write_chunked() should probably also be simplified with alignment done at a higher-level such that the page and eraseblock alignment are consolidated (since we may need to write additional pages within an eraseblock to restore old contents).
But that's for another patch...
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33613 )
Change subject: spi: Move 16MiB partitioning up into spi_chip_read() ......................................................................
Patch Set 2:
Patch Set 2: Code-Review+2
It's worth pointing out that this makes the loop look a lot less like the one in spi_write_chunked(). spi_write_chunked() should probably also be simplified with alignment done at a higher-level such that the page and eraseblock alignment are consolidated (since we may need to write additional pages within an eraseblock to restore old contents).
But that's for another patch...
Thanks for the review. Did you have time to test it?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33613 )
Change subject: spi: Move 16MiB partitioning up into spi_chip_read() ......................................................................
Patch Set 2: Code-Review-2
argh, I was too tired when I wrote this. The original code also ensured that the 16MiB borders are never crossed in a single read. Need to fix that.
Hello Carl-Daniel Hailfinger, ron minnich, David Hendricks, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/33613
to look at the new patch set (#3).
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 nico.h@gmx.de --- M flash.h M spi.c M spi25.c 3 files changed, 28 insertions(+), 33 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/33613/3
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33613 )
Change subject: spi: Move 16MiB partitioning up into spi_chip_read() ......................................................................
Patch Set 3: -Code-Review
sorry for the noise ._.
Ryan O'Leary has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33613 )
Change subject: spi: Move 16MiB partitioning up into spi_chip_read() ......................................................................
Patch Set 3: Code-Review+1
This works perfectly now with dediprog + MX66L51235F!
Before I had the error:
$ flashrom -p dediprog -r test.bios flashrom v1.1-rc1-12-ga724602-dirty on Linux 4.19.37-2rodete1-amd64 (x86_64) flashrom is free software, get the source code at https://flashrom.org
Using clock_gettime for delay loops (clk_id: 1, resolution: 1ns). Incomplete/failed Command Receive Device String! Found Macronix flash chip "MX66L51235F" (65536 kB, SPI) on dediprog. Reading flash... SPI bulk read failed! SPI bulk read failed! SPI bulk read failed! SPI bulk read failed! SPI bulk read failed! SPI bulk read failed! SPI bulk read failed! SPI bulk read failed! Read operation failed! FAILED.
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33613 )
Change subject: spi: Move 16MiB partitioning up into spi_chip_read() ......................................................................
Patch Set 3: Code-Review+2
tested and resolves our problems.
Thanks again nico.
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33613 )
Change subject: spi: Move 16MiB partitioning up into spi_chip_read() ......................................................................
Patch Set 3: Code-Review+2
Looks good to me.
Hello Carl-Daniel Hailfinger, ron minnich, Ryan O'Leary, ron minnich, David Hendricks, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/33613
to look at the new patch set (#4).
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.
Change-Id: Iab1fd5b2ea550b4b3ef3e8402e0b6ca218485a51 Signed-off-by: Nico Huber nico.h@gmx.de --- M flash.h M spi.c M spi25.c 3 files changed, 28 insertions(+), 33 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/33613/4
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33613 )
Change subject: spi: Move 16MiB partitioning up into spi_chip_read() ......................................................................
Patch Set 4: Verified+1
Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/flashrom/+/33613 )
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.
Change-Id: Iab1fd5b2ea550b4b3ef3e8402e0b6ca218485a51 Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/flashrom/+/33613 Reviewed-by: Ryan O'Leary Reviewed-by: ron minnich rminnich@gmail.com Reviewed-by: David Hendricks david.hendricks@gmail.com --- M flash.h M spi.c M spi25.c 3 files changed, 28 insertions(+), 33 deletions(-)
Approvals: Nico Huber: Verified David Hendricks: Looks good to me, approved ron minnich: Looks good to me, approved Ryan O'Leary: Looks good to me, but someone else must approve
diff --git a/flash.h b/flash.h index 5bbfa0a..b60a980 100644 --- a/flash.h +++ b/flash.h @@ -37,6 +37,12 @@ #include "libflashrom.h" #include "layout.h"
+#define KiB (1024) +#define MiB (1024 * KiB) + +/* Assumes `n` and `a` are at most 64-bit wide (to avoid typeof() operator). */ +#define ALIGN_DOWN(n, a) ((n) & ~((uint64_t)(a) - 1)) + #define ERROR_PTR ((void*)-1)
/* Error codes */ diff --git a/spi.c b/spi.c index a4dba19..489baf3 100644 --- a/spi.c +++ b/spi.c @@ -99,7 +99,19 @@ 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) { + /* Do not cross 16MiB boundaries in a single transfer. + This helps with + o multi-die 4-byte-addressing chips, + o dediprog that has a protocol limit of 32MiB-512B. */ + to_read = min(ALIGN_DOWN(start + 16*MiB, 16*MiB) - start, 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; }
/*
-
David Hendricks (Code Review) -
Nico Huber (Code Review)
-
ron minnich (Code Review)
-
Ryan O'Leary (Code Review)