Hello Aaron Durbin, Furquan Shaikh,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/33282
to review the following change.
Change subject: spi_flash: Make .read() callback optional ......................................................................
spi_flash: Make .read() callback optional
All SPI flash chip drivers currently in coreboot use the generic read functions (spi_flash_cmd_read_fast()/_slow()) as their read callback. The only use case for specialized read callbacks we have left is with specialized flash controllers like Intel fast_spi (which sort of impersonate the flash chip driver by implementing their own probe function).
This patch unifies the behavior for all normal flash drivers by making the read callback optional and letting them all fall back to a default read implementation that handles normal fast/slow reading. Most of the drivers used to install the respective callback after checking CONFIG_SPI_FLASH_NO_FAST_READ, but some hardcoded either slow or fast writes. I have found no indications for why this is and spot-checked datasheets for affected vendors to make sure they all support both commands, so I assume this is just some old inaccuracy rather than important differences that need preserving. (Please yell if you disagree.)
Also take the opportunity to refactor some of the common spi_flash.c code a bit because I felt there are too many nested functions that don't really do enough on their own, and centralizing stuff a bit should make it easier to follow the code flow. (Some of this is in preparation for the next patch.)
Change-Id: I2096a3ce619767b41b1b0c0c2b8e95b2bd90a419 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/drivers/spi/adesto.c M src/drivers/spi/amic.c M src/drivers/spi/atmel.c M src/drivers/spi/eon.c M src/drivers/spi/gigadevice.c M src/drivers/spi/macronix.c M src/drivers/spi/spansion.c M src/drivers/spi/spi_flash.c M src/drivers/spi/spi_flash_internal.h M src/drivers/spi/sst.c M src/drivers/spi/stmicro.c M src/drivers/spi/winbond.c 12 files changed, 34 insertions(+), 103 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/33282/1
diff --git a/src/drivers/spi/adesto.c b/src/drivers/spi/adesto.c index a805609..4e1043e 100644 --- a/src/drivers/spi/adesto.c +++ b/src/drivers/spi/adesto.c @@ -201,11 +201,6 @@ static const struct spi_flash_ops spi_flash_ops = { .write = adesto_write, .erase = spi_flash_cmd_erase, -#if CONFIG(SPI_FLASH_NO_FAST_READ) - .read = spi_flash_cmd_read_slow, -#else - .read = spi_flash_cmd_read_fast, -#endif };
int spi_flash_probe_adesto(const struct spi_slave *spi, u8 *idcode, diff --git a/src/drivers/spi/amic.c b/src/drivers/spi/amic.c index 64c91cc..b580dc3 100644 --- a/src/drivers/spi/amic.c +++ b/src/drivers/spi/amic.c @@ -176,11 +176,6 @@ static const struct spi_flash_ops spi_flash_ops = { .write = amic_write, .erase = spi_flash_cmd_erase, -#if CONFIG(SPI_FLASH_NO_FAST_READ) - .read = spi_flash_cmd_read_slow, -#else - .read = spi_flash_cmd_read_fast, -#endif };
int spi_flash_probe_amic(const struct spi_slave *spi, u8 *idcode, diff --git a/src/drivers/spi/atmel.c b/src/drivers/spi/atmel.c index 7d6e172..58a2862 100644 --- a/src/drivers/spi/atmel.c +++ b/src/drivers/spi/atmel.c @@ -157,11 +157,6 @@ static const struct spi_flash_ops spi_flash_ops = { .write = atmel_write, .erase = spi_flash_cmd_erase, -#if CONFIG(SPI_FLASH_NO_FAST_READ) - .read = spi_flash_cmd_read_slow, -#else - .read = spi_flash_cmd_read_fast, -#endif };
int spi_flash_probe_atmel(const struct spi_slave *spi, u8 *idcode, diff --git a/src/drivers/spi/eon.c b/src/drivers/spi/eon.c index 33e12a0..f3cf70e 100644 --- a/src/drivers/spi/eon.c +++ b/src/drivers/spi/eon.c @@ -292,7 +292,6 @@ .write = eon_write, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, - .read = spi_flash_cmd_read_fast, };
int spi_flash_probe_eon(const struct spi_slave *spi, u8 *idcode, diff --git a/src/drivers/spi/gigadevice.c b/src/drivers/spi/gigadevice.c index 83216c0..194c5ec 100644 --- a/src/drivers/spi/gigadevice.c +++ b/src/drivers/spi/gigadevice.c @@ -222,11 +222,6 @@ .write = gigadevice_write, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, -#if CONFIG(SPI_FLASH_NO_FAST_READ) - .read = spi_flash_cmd_read_slow, -#else - .read = spi_flash_cmd_read_fast, -#endif };
int spi_flash_probe_gigadevice(const struct spi_slave *spi, u8 *idcode, diff --git a/src/drivers/spi/macronix.c b/src/drivers/spi/macronix.c index 1610ca1..5a97b8f 100644 --- a/src/drivers/spi/macronix.c +++ b/src/drivers/spi/macronix.c @@ -268,11 +268,6 @@ .write = macronix_write, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, -#if CONFIG(SPI_FLASH_NO_FAST_READ) - .read = spi_flash_cmd_read_slow, -#else - .read = spi_flash_cmd_read_fast, -#endif };
int spi_flash_probe_macronix(const struct spi_slave *spi, u8 *idcode, diff --git a/src/drivers/spi/spansion.c b/src/drivers/spi/spansion.c index e687bf8..4a241ba 100644 --- a/src/drivers/spi/spansion.c +++ b/src/drivers/spi/spansion.c @@ -282,7 +282,6 @@ static const struct spi_flash_ops spi_flash_ops = { .write = spansion_write, .erase = spi_flash_cmd_erase, - .read = spi_flash_cmd_read_slow, .status = spi_flash_cmd_status, };
diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c index ae1d2ef..d152027 100644 --- a/src/drivers/spi/spi_flash.c +++ b/src/drivers/spi/spi_flash.c @@ -31,7 +31,7 @@ static int do_spi_flash_cmd(const struct spi_slave *spi, const void *dout, size_t bytes_out, void *din, size_t bytes_in) { - int ret = 1; + int ret; /* * SPI flash requires command-response kind of behavior. Thus, two * separate SPI vectors are required -- first to transmit dout and other @@ -49,11 +49,10 @@ if (!bytes_in) count = 1;
- if (spi_claim_bus(spi)) + if ((ret = spi_claim_bus(spi))) return ret;
- if (spi_xfer_vector(spi, vectors, count) == 0) - ret = 0; + ret = spi_xfer_vector(spi, vectors, count);
spi_release_bus(spi); return ret; @@ -68,18 +67,6 @@ return ret; }
-static int spi_flash_cmd_read(const struct spi_slave *spi, const u8 *cmd, - size_t cmd_len, void *data, size_t data_len) -{ - int ret = do_spi_flash_cmd(spi, cmd, cmd_len, data, data_len); - if (ret) { - printk(BIOS_WARNING, "SF: Failed to send read command (%zu bytes): %d\n", - data_len, ret); - } - - return ret; -} - /* TODO: This code is quite possibly broken and overflowing stacks. Fix ASAP! */ #pragma GCC diagnostic push #if defined(__GNUC__) && !defined(__clang__) @@ -103,34 +90,38 @@ } #pragma GCC diagnostic pop
-static int spi_flash_cmd_read_array(const struct spi_slave *spi, u8 *cmd, - size_t cmd_len, u32 offset, - size_t len, void *data) -{ - spi_flash_addr(offset, cmd); - return spi_flash_cmd_read(spi, cmd, cmd_len, data, len); -} - /* Perform the read operation honoring spi controller fifo size, reissuing * the read command until the full request completed. */ -static int spi_flash_cmd_read_array_wrapped(const struct spi_slave *spi, - u8 *cmd, size_t cmd_len, u32 offset, - size_t len, void *buf) +static int spi_flash_read_chunked(const struct spi_flash *flash, u32 offset, + size_t len, void *buf) { - int ret; - size_t xfer_len; + u8 cmd[5]; + int ret, cmd_len; + int (*do_cmd)(const struct spi_slave *, const void *, size_t, + void *, size_t); + + if (CONFIG(SPI_FLASH_NO_FAST_READ)) { + cmd_len = 4; + cmd[0] = CMD_READ_ARRAY_SLOW; + do_cmd = do_spi_flash_cmd; + } else { + cmd_len = 5; + cmd[0] = CMD_READ_ARRAY_FAST; + cmd[4] = 0; + do_cmd = do_spi_flash_cmd; + } + uint8_t *data = buf; - while (len) { - xfer_len = spi_crop_chunk(spi, cmd_len, len); - - /* Perform the read. */ - ret = spi_flash_cmd_read_array(spi, cmd, cmd_len, - offset, xfer_len, data); - - if (ret) + size_t xfer_len = spi_crop_chunk(&flash->spi, sizeof(cmd), len); + spi_flash_addr(offset, cmd); + ret = do_cmd(&flash->spi, cmd, cmd_len, data, xfer_len); + if (ret) { + printk(BIOS_WARNING, + "SF: Failed to send read command %#.2x(%#x, %#zx): %d\n", + cmd[0], offset, xfer_len, ret); return ret; - + } offset += xfer_len; data += xfer_len; len -= xfer_len; @@ -139,28 +130,6 @@ return 0; }
-int spi_flash_cmd_read_fast(const struct spi_flash *flash, u32 offset, - size_t len, void *data) -{ - u8 cmd[5]; - - cmd[0] = CMD_READ_ARRAY_FAST; - cmd[4] = 0x00; - - return spi_flash_cmd_read_array_wrapped(&flash->spi, cmd, sizeof(cmd), - offset, len, data); -} - -int spi_flash_cmd_read_slow(const struct spi_flash *flash, u32 offset, - size_t len, void *data) -{ - u8 cmd[4]; - - cmd[0] = CMD_READ_ARRAY_SLOW; - return spi_flash_cmd_read_array_wrapped(&flash->spi, cmd, sizeof(cmd), - offset, len, data); -} - int spi_flash_cmd_poll_bit(const struct spi_flash *flash, unsigned long timeout, u8 cmd, u8 poll_bit) { @@ -174,7 +143,7 @@ mono_time_add_msecs(&end, timeout);
do { - ret = spi_flash_cmd_read(spi, &cmd, 1, &status, 1); + ret = do_spi_flash_cmd(spi, &cmd, 1, &status, 1); if (ret) return -1; if ((status & poll_bit) == 0) @@ -391,7 +360,10 @@ int spi_flash_read(const struct spi_flash *flash, u32 offset, size_t len, void *buf) { - return flash->ops->read(flash, offset, len, buf); + if (flash->ops->read) + return flash->ops->read(flash, offset, len, buf); + + return spi_flash_read_chunked(flash, offset, len, buf); }
int spi_flash_write(const struct spi_flash *flash, u32 offset, size_t len, diff --git a/src/drivers/spi/spi_flash_internal.h b/src/drivers/spi/spi_flash_internal.h index a89610a..f15c737 100644 --- a/src/drivers/spi/spi_flash_internal.h +++ b/src/drivers/spi/spi_flash_internal.h @@ -34,12 +34,6 @@ /* Send a single-byte command to the device and read the response */ int spi_flash_cmd(const struct spi_slave *spi, u8 cmd, void *response, size_t len);
-int spi_flash_cmd_read_fast(const struct spi_flash *flash, u32 offset, - size_t len, void *data); - -int spi_flash_cmd_read_slow(const struct spi_flash *flash, u32 offset, - size_t len, void *data); - /* * Send a multi-byte command to the device followed by (optional) * data. Used for programming the flash array, etc. diff --git a/src/drivers/spi/sst.c b/src/drivers/spi/sst.c index e4ea7805..abe3f2a 100644 --- a/src/drivers/spi/sst.c +++ b/src/drivers/spi/sst.c @@ -55,14 +55,12 @@ .write = sst_write_ai, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, - .read = spi_flash_cmd_read_fast, };
static const struct spi_flash_ops spi_flash_ops_write_256 = { .write = sst_write_256, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, - .read = spi_flash_cmd_read_fast, };
#define SST_SECTOR_SIZE (4 * 1024) diff --git a/src/drivers/spi/stmicro.c b/src/drivers/spi/stmicro.c index fb24b27..6625764 100644 --- a/src/drivers/spi/stmicro.c +++ b/src/drivers/spi/stmicro.c @@ -351,7 +351,6 @@ static const struct spi_flash_ops spi_flash_ops = { .write = stmicro_write, .erase = spi_flash_cmd_erase, - .read = spi_flash_cmd_read_fast, };
int spi_flash_probe_stmicro(const struct spi_slave *spi, u8 *idcode, diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index 11a5187..d0ef3cd 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -646,11 +646,6 @@ .write = winbond_write, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, -#if CONFIG(SPI_FLASH_NO_FAST_READ) - .read = spi_flash_cmd_read_slow, -#else - .read = spi_flash_cmd_read_fast, -#endif .get_write_protection = winbond_get_write_protection, .set_write_protection = winbond_set_write_protection, };
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33282 )
Change subject: spi_flash: Make .read() callback optional ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/#/c/33282/1/src/drivers/spi/spi_flash.c File src/drivers/spi/spi_flash.c:
https://review.coreboot.org/#/c/33282/1/src/drivers/spi/spi_flash.c@52 PS1, Line 52: if ((ret = spi_claim_bus(spi))) do not use assignment in if condition
https://review.coreboot.org/#/c/33282/1/src/drivers/spi/spi_flash.c@100 PS1, Line 100: int (*do_cmd)(const struct spi_slave *, const void *, size_t, function definition argument 'const struct spi_slave *' should also have an identifier name
https://review.coreboot.org/#/c/33282/1/src/drivers/spi/spi_flash.c@100 PS1, Line 100: int (*do_cmd)(const struct spi_slave *, const void *, size_t, function definition argument 'const void *' should also have an identifier name
https://review.coreboot.org/#/c/33282/1/src/drivers/spi/spi_flash.c@100 PS1, Line 100: int (*do_cmd)(const struct spi_slave *, const void *, size_t, function definition argument 'size_t' should also have an identifier name
https://review.coreboot.org/#/c/33282/1/src/drivers/spi/spi_flash.c@100 PS1, Line 100: int (*do_cmd)(const struct spi_slave *, const void *, size_t, function definition argument 'void *' should also have an identifier name
https://review.coreboot.org/#/c/33282/1/src/drivers/spi/spi_flash.c@100 PS1, Line 100: int (*do_cmd)(const struct spi_slave *, const void *, size_t, function definition argument 'size_t' should also have an identifier name
Hello Aaron Durbin, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33282
to look at the new patch set (#2).
Change subject: spi_flash: Make .read() callback optional ......................................................................
spi_flash: Make .read() callback optional
All SPI flash chip drivers currently in coreboot use the generic read functions (spi_flash_cmd_read_fast()/_slow()) as their read callback. The only use case for specialized read callbacks we have left is with specialized flash controllers like Intel fast_spi (which sort of impersonate the flash chip driver by implementing their own probe function).
This patch unifies the behavior for all normal flash drivers by making the read callback optional and letting them all fall back to a default read implementation that handles normal fast/slow reading. Most of the drivers used to install the respective callback after checking CONFIG_SPI_FLASH_NO_FAST_READ, but some hardcoded either slow or fast writes. I have found no indications for why this is and spot-checked datasheets for affected vendors to make sure they all support both commands, so I assume this is just some old inaccuracy rather than important differences that need preserving. (Please yell if you disagree.)
Also take the opportunity to refactor some of the common spi_flash.c code a bit because I felt there are too many nested functions that don't really do enough on their own, and centralizing stuff a bit should make it easier to follow the code flow. (Some of this is in preparation for the next patch.)
Change-Id: I2096a3ce619767b41b1b0c0c2b8e95b2bd90a419 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/drivers/spi/adesto.c M src/drivers/spi/amic.c M src/drivers/spi/atmel.c M src/drivers/spi/eon.c M src/drivers/spi/gigadevice.c M src/drivers/spi/macronix.c M src/drivers/spi/spansion.c M src/drivers/spi/spi_flash.c M src/drivers/spi/spi_flash_internal.h M src/drivers/spi/sst.c M src/drivers/spi/stmicro.c M src/drivers/spi/winbond.c 12 files changed, 35 insertions(+), 103 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/33282/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33282 )
Change subject: spi_flash: Make .read() callback optional ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/#/c/33282/2/src/drivers/spi/spi_flash.c File src/drivers/spi/spi_flash.c:
https://review.coreboot.org/#/c/33282/2/src/drivers/spi/spi_flash.c@101 PS2, Line 101: int (*do_cmd)(const struct spi_slave * spi, const void * din, "foo * bar" should be "foo *bar"
https://review.coreboot.org/#/c/33282/2/src/drivers/spi/spi_flash.c@101 PS2, Line 101: int (*do_cmd)(const struct spi_slave * spi, const void * din, "foo * bar" should be "foo *bar"
https://review.coreboot.org/#/c/33282/2/src/drivers/spi/spi_flash.c@102 PS2, Line 102: size_t in_bytes, void * out, size_t out_bytes); "foo * bar" should be "foo *bar"
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33282 )
Change subject: spi_flash: Make .read() callback optional ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/33282/1/src/drivers/spi/spi_flash.c File src/drivers/spi/spi_flash.c:
https://review.coreboot.org/#/c/33282/1/src/drivers/spi/spi_flash.c@116 PS1, Line 116: sizeof(cmd) shouldn't this be cmd_len?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33282 )
Change subject: spi_flash: Make .read() callback optional ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/33282/1/src/drivers/spi/spi_flash.c File src/drivers/spi/spi_flash.c:
https://review.coreboot.org/#/c/33282/1/src/drivers/spi/spi_flash.c@116 PS1, Line 116: sizeof(cmd)
shouldn't this be cmd_len?
Whoops yes, thanks, that snuck in from an earlier draft.
Hello Aaron Durbin, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33282
to look at the new patch set (#3).
Change subject: spi_flash: Make .read() callback optional ......................................................................
spi_flash: Make .read() callback optional
All SPI flash chip drivers currently in coreboot use the generic read functions (spi_flash_cmd_read_fast()/_slow()) as their read callback. The only use case for specialized read callbacks we have left is with specialized flash controllers like Intel fast_spi (which sort of impersonate the flash chip driver by implementing their own probe function).
This patch unifies the behavior for all normal flash drivers by making the read callback optional and letting them all fall back to a default read implementation that handles normal fast/slow reading. Most of the drivers used to install the respective callback after checking CONFIG_SPI_FLASH_NO_FAST_READ, but some hardcoded either slow or fast writes. I have found no indications for why this is and spot-checked datasheets for affected vendors to make sure they all support both commands, so I assume this is just some old inaccuracy rather than important differences that need preserving. (Please yell if you disagree.)
Also take the opportunity to refactor some of the common spi_flash.c code a bit because I felt there are too many nested functions that don't really do enough on their own, and centralizing stuff a bit should make it easier to follow the code flow. (Some of this is in preparation for the next patch.)
Change-Id: I2096a3ce619767b41b1b0c0c2b8e95b2bd90a419 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/drivers/spi/adesto.c M src/drivers/spi/amic.c M src/drivers/spi/atmel.c M src/drivers/spi/eon.c M src/drivers/spi/gigadevice.c M src/drivers/spi/macronix.c M src/drivers/spi/spansion.c M src/drivers/spi/spi_flash.c M src/drivers/spi/spi_flash_internal.h M src/drivers/spi/sst.c M src/drivers/spi/stmicro.c M src/drivers/spi/winbond.c 12 files changed, 35 insertions(+), 103 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/33282/3
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33282 )
Change subject: spi_flash: Make .read() callback optional ......................................................................
Patch Set 3: Code-Review+2
We've goot b
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33282 )
Change subject: spi_flash: Make .read() callback optional ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/33282/4/src/drivers/spi/spi_flash.c File src/drivers/spi/spi_flash.c:
https://review.coreboot.org/#/c/33282/4/src/drivers/spi/spi_flash.c@104 PS4, Line 104: CONFIG(SPI_FLASH_NO_FAST_READ) Outside the scope of this patch: Is the only use case for this Intel SPI controllers that are locked down and cannot use fast_read? If so a callback that bases the decision on spispeed and in the Intel case the opmenu + lockdown state could be better?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33282 )
Change subject: spi_flash: Make .read() callback optional ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/33282/4/src/drivers/spi/spi_flash.c File src/drivers/spi/spi_flash.c:
https://review.coreboot.org/#/c/33282/4/src/drivers/spi/spi_flash.c@104 PS4, Line 104: CONFIG(SPI_FLASH_NO_FAST_READ)
Outside the scope of this patch: Is the only use case for this Intel SPI controllers that are locked […]
I don't know tbh, I just didn't want to kill the existing functionality in case anyone needed it. It looks like nothing in the tree is selecting this, so I don't think any platform really depends on it. It's probably just meant as an out for cases where you have to work around some weird bug.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33282 )
Change subject: spi_flash: Make .read() callback optional ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/33282/4/src/drivers/spi/spi_flash.c File src/drivers/spi/spi_flash.c:
https://review.coreboot.org/#/c/33282/4/src/drivers/spi/spi_flash.c@101 PS4, Line 101: do_cmd Is this required at all since it is always set to do_spi_flash_cmd?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33282 )
Change subject: spi_flash: Make .read() callback optional ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/33282/4/src/drivers/spi/spi_flash.c File src/drivers/spi/spi_flash.c:
https://review.coreboot.org/#/c/33282/4/src/drivers/spi/spi_flash.c@101 PS4, Line 101: do_cmd
Is this required at all since it is always set to do_spi_flash_cmd?
I see its used in a follow up patch :).
Julius Werner has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33282 )
Change subject: spi_flash: Make .read() callback optional ......................................................................
spi_flash: Make .read() callback optional
All SPI flash chip drivers currently in coreboot use the generic read functions (spi_flash_cmd_read_fast()/_slow()) as their read callback. The only use case for specialized read callbacks we have left is with specialized flash controllers like Intel fast_spi (which sort of impersonate the flash chip driver by implementing their own probe function).
This patch unifies the behavior for all normal flash drivers by making the read callback optional and letting them all fall back to a default read implementation that handles normal fast/slow reading. Most of the drivers used to install the respective callback after checking CONFIG_SPI_FLASH_NO_FAST_READ, but some hardcoded either slow or fast writes. I have found no indications for why this is and spot-checked datasheets for affected vendors to make sure they all support both commands, so I assume this is just some old inaccuracy rather than important differences that need preserving. (Please yell if you disagree.)
Also take the opportunity to refactor some of the common spi_flash.c code a bit because I felt there are too many nested functions that don't really do enough on their own, and centralizing stuff a bit should make it easier to follow the code flow. (Some of this is in preparation for the next patch.)
Change-Id: I2096a3ce619767b41b1b0c0c2b8e95b2bd90a419 Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/33282 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/drivers/spi/adesto.c M src/drivers/spi/amic.c M src/drivers/spi/atmel.c M src/drivers/spi/eon.c M src/drivers/spi/gigadevice.c M src/drivers/spi/macronix.c M src/drivers/spi/spansion.c M src/drivers/spi/spi_flash.c M src/drivers/spi/spi_flash_internal.h M src/drivers/spi/sst.c M src/drivers/spi/stmicro.c M src/drivers/spi/winbond.c 12 files changed, 35 insertions(+), 103 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Furquan Shaikh: Looks good to me, approved Arthur Heymans: Looks good to me, approved
diff --git a/src/drivers/spi/adesto.c b/src/drivers/spi/adesto.c index a805609..4e1043e 100644 --- a/src/drivers/spi/adesto.c +++ b/src/drivers/spi/adesto.c @@ -201,11 +201,6 @@ static const struct spi_flash_ops spi_flash_ops = { .write = adesto_write, .erase = spi_flash_cmd_erase, -#if CONFIG(SPI_FLASH_NO_FAST_READ) - .read = spi_flash_cmd_read_slow, -#else - .read = spi_flash_cmd_read_fast, -#endif };
int spi_flash_probe_adesto(const struct spi_slave *spi, u8 *idcode, diff --git a/src/drivers/spi/amic.c b/src/drivers/spi/amic.c index 64c91cc..b580dc3 100644 --- a/src/drivers/spi/amic.c +++ b/src/drivers/spi/amic.c @@ -176,11 +176,6 @@ static const struct spi_flash_ops spi_flash_ops = { .write = amic_write, .erase = spi_flash_cmd_erase, -#if CONFIG(SPI_FLASH_NO_FAST_READ) - .read = spi_flash_cmd_read_slow, -#else - .read = spi_flash_cmd_read_fast, -#endif };
int spi_flash_probe_amic(const struct spi_slave *spi, u8 *idcode, diff --git a/src/drivers/spi/atmel.c b/src/drivers/spi/atmel.c index 7d6e172..58a2862 100644 --- a/src/drivers/spi/atmel.c +++ b/src/drivers/spi/atmel.c @@ -157,11 +157,6 @@ static const struct spi_flash_ops spi_flash_ops = { .write = atmel_write, .erase = spi_flash_cmd_erase, -#if CONFIG(SPI_FLASH_NO_FAST_READ) - .read = spi_flash_cmd_read_slow, -#else - .read = spi_flash_cmd_read_fast, -#endif };
int spi_flash_probe_atmel(const struct spi_slave *spi, u8 *idcode, diff --git a/src/drivers/spi/eon.c b/src/drivers/spi/eon.c index 33e12a0..f3cf70e 100644 --- a/src/drivers/spi/eon.c +++ b/src/drivers/spi/eon.c @@ -292,7 +292,6 @@ .write = eon_write, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, - .read = spi_flash_cmd_read_fast, };
int spi_flash_probe_eon(const struct spi_slave *spi, u8 *idcode, diff --git a/src/drivers/spi/gigadevice.c b/src/drivers/spi/gigadevice.c index 714ed89..cc4cf1d 100644 --- a/src/drivers/spi/gigadevice.c +++ b/src/drivers/spi/gigadevice.c @@ -221,11 +221,6 @@ .write = gigadevice_write, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, -#if CONFIG(SPI_FLASH_NO_FAST_READ) - .read = spi_flash_cmd_read_slow, -#else - .read = spi_flash_cmd_read_fast, -#endif };
int spi_flash_probe_gigadevice(const struct spi_slave *spi, u8 *idcode, diff --git a/src/drivers/spi/macronix.c b/src/drivers/spi/macronix.c index 1610ca1..5a97b8f 100644 --- a/src/drivers/spi/macronix.c +++ b/src/drivers/spi/macronix.c @@ -268,11 +268,6 @@ .write = macronix_write, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, -#if CONFIG(SPI_FLASH_NO_FAST_READ) - .read = spi_flash_cmd_read_slow, -#else - .read = spi_flash_cmd_read_fast, -#endif };
int spi_flash_probe_macronix(const struct spi_slave *spi, u8 *idcode, diff --git a/src/drivers/spi/spansion.c b/src/drivers/spi/spansion.c index e687bf8..4a241ba 100644 --- a/src/drivers/spi/spansion.c +++ b/src/drivers/spi/spansion.c @@ -282,7 +282,6 @@ static const struct spi_flash_ops spi_flash_ops = { .write = spansion_write, .erase = spi_flash_cmd_erase, - .read = spi_flash_cmd_read_slow, .status = spi_flash_cmd_status, };
diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c index ae1d2ef..5e42a37 100644 --- a/src/drivers/spi/spi_flash.c +++ b/src/drivers/spi/spi_flash.c @@ -31,7 +31,7 @@ static int do_spi_flash_cmd(const struct spi_slave *spi, const void *dout, size_t bytes_out, void *din, size_t bytes_in) { - int ret = 1; + int ret; /* * SPI flash requires command-response kind of behavior. Thus, two * separate SPI vectors are required -- first to transmit dout and other @@ -49,11 +49,11 @@ if (!bytes_in) count = 1;
- if (spi_claim_bus(spi)) + ret = spi_claim_bus(spi); + if (ret) return ret;
- if (spi_xfer_vector(spi, vectors, count) == 0) - ret = 0; + ret = spi_xfer_vector(spi, vectors, count);
spi_release_bus(spi); return ret; @@ -68,18 +68,6 @@ return ret; }
-static int spi_flash_cmd_read(const struct spi_slave *spi, const u8 *cmd, - size_t cmd_len, void *data, size_t data_len) -{ - int ret = do_spi_flash_cmd(spi, cmd, cmd_len, data, data_len); - if (ret) { - printk(BIOS_WARNING, "SF: Failed to send read command (%zu bytes): %d\n", - data_len, ret); - } - - return ret; -} - /* TODO: This code is quite possibly broken and overflowing stacks. Fix ASAP! */ #pragma GCC diagnostic push #if defined(__GNUC__) && !defined(__clang__) @@ -103,34 +91,38 @@ } #pragma GCC diagnostic pop
-static int spi_flash_cmd_read_array(const struct spi_slave *spi, u8 *cmd, - size_t cmd_len, u32 offset, - size_t len, void *data) -{ - spi_flash_addr(offset, cmd); - return spi_flash_cmd_read(spi, cmd, cmd_len, data, len); -} - /* Perform the read operation honoring spi controller fifo size, reissuing * the read command until the full request completed. */ -static int spi_flash_cmd_read_array_wrapped(const struct spi_slave *spi, - u8 *cmd, size_t cmd_len, u32 offset, - size_t len, void *buf) +static int spi_flash_read_chunked(const struct spi_flash *flash, u32 offset, + size_t len, void *buf) { - int ret; - size_t xfer_len; + u8 cmd[5]; + int ret, cmd_len; + int (*do_cmd)(const struct spi_slave *spi, const void *din, + size_t in_bytes, void *out, size_t out_bytes); + + if (CONFIG(SPI_FLASH_NO_FAST_READ)) { + cmd_len = 4; + cmd[0] = CMD_READ_ARRAY_SLOW; + do_cmd = do_spi_flash_cmd; + } else { + cmd_len = 5; + cmd[0] = CMD_READ_ARRAY_FAST; + cmd[4] = 0; + do_cmd = do_spi_flash_cmd; + } + uint8_t *data = buf; - while (len) { - xfer_len = spi_crop_chunk(spi, cmd_len, len); - - /* Perform the read. */ - ret = spi_flash_cmd_read_array(spi, cmd, cmd_len, - offset, xfer_len, data); - - if (ret) + size_t xfer_len = spi_crop_chunk(&flash->spi, cmd_len, len); + spi_flash_addr(offset, cmd); + ret = do_cmd(&flash->spi, cmd, cmd_len, data, xfer_len); + if (ret) { + printk(BIOS_WARNING, + "SF: Failed to send read command %#.2x(%#x, %#zx): %d\n", + cmd[0], offset, xfer_len, ret); return ret; - + } offset += xfer_len; data += xfer_len; len -= xfer_len; @@ -139,28 +131,6 @@ return 0; }
-int spi_flash_cmd_read_fast(const struct spi_flash *flash, u32 offset, - size_t len, void *data) -{ - u8 cmd[5]; - - cmd[0] = CMD_READ_ARRAY_FAST; - cmd[4] = 0x00; - - return spi_flash_cmd_read_array_wrapped(&flash->spi, cmd, sizeof(cmd), - offset, len, data); -} - -int spi_flash_cmd_read_slow(const struct spi_flash *flash, u32 offset, - size_t len, void *data) -{ - u8 cmd[4]; - - cmd[0] = CMD_READ_ARRAY_SLOW; - return spi_flash_cmd_read_array_wrapped(&flash->spi, cmd, sizeof(cmd), - offset, len, data); -} - int spi_flash_cmd_poll_bit(const struct spi_flash *flash, unsigned long timeout, u8 cmd, u8 poll_bit) { @@ -174,7 +144,7 @@ mono_time_add_msecs(&end, timeout);
do { - ret = spi_flash_cmd_read(spi, &cmd, 1, &status, 1); + ret = do_spi_flash_cmd(spi, &cmd, 1, &status, 1); if (ret) return -1; if ((status & poll_bit) == 0) @@ -391,7 +361,10 @@ int spi_flash_read(const struct spi_flash *flash, u32 offset, size_t len, void *buf) { - return flash->ops->read(flash, offset, len, buf); + if (flash->ops->read) + return flash->ops->read(flash, offset, len, buf); + + return spi_flash_read_chunked(flash, offset, len, buf); }
int spi_flash_write(const struct spi_flash *flash, u32 offset, size_t len, diff --git a/src/drivers/spi/spi_flash_internal.h b/src/drivers/spi/spi_flash_internal.h index a89610a..f15c737 100644 --- a/src/drivers/spi/spi_flash_internal.h +++ b/src/drivers/spi/spi_flash_internal.h @@ -34,12 +34,6 @@ /* Send a single-byte command to the device and read the response */ int spi_flash_cmd(const struct spi_slave *spi, u8 cmd, void *response, size_t len);
-int spi_flash_cmd_read_fast(const struct spi_flash *flash, u32 offset, - size_t len, void *data); - -int spi_flash_cmd_read_slow(const struct spi_flash *flash, u32 offset, - size_t len, void *data); - /* * Send a multi-byte command to the device followed by (optional) * data. Used for programming the flash array, etc. diff --git a/src/drivers/spi/sst.c b/src/drivers/spi/sst.c index e4ea7805..abe3f2a 100644 --- a/src/drivers/spi/sst.c +++ b/src/drivers/spi/sst.c @@ -55,14 +55,12 @@ .write = sst_write_ai, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, - .read = spi_flash_cmd_read_fast, };
static const struct spi_flash_ops spi_flash_ops_write_256 = { .write = sst_write_256, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, - .read = spi_flash_cmd_read_fast, };
#define SST_SECTOR_SIZE (4 * 1024) diff --git a/src/drivers/spi/stmicro.c b/src/drivers/spi/stmicro.c index fb24b27..6625764 100644 --- a/src/drivers/spi/stmicro.c +++ b/src/drivers/spi/stmicro.c @@ -351,7 +351,6 @@ static const struct spi_flash_ops spi_flash_ops = { .write = stmicro_write, .erase = spi_flash_cmd_erase, - .read = spi_flash_cmd_read_fast, };
int spi_flash_probe_stmicro(const struct spi_slave *spi, u8 *idcode, diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index 11a5187..d0ef3cd 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -646,11 +646,6 @@ .write = winbond_write, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, -#if CONFIG(SPI_FLASH_NO_FAST_READ) - .read = spi_flash_cmd_read_slow, -#else - .read = spi_flash_cmd_read_fast, -#endif .get_write_protection = winbond_get_write_protection, .set_write_protection = winbond_set_write_protection, };
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Furquan Shaikh (Code Review)
-
Julius Werner (Code Review)