Attention is currently required from: Shelley Chen, Furquan Shaikh. Hello Shelley Chen, Furquan Shaikh,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/56287
to review the following change.
Change subject: drivers: spi_flash: Add Fast Read Dual I/O support ......................................................................
drivers: spi_flash: Add Fast Read Dual I/O support
The Fast Read Dual Output and Fast Read Dual I/O commands are practically identical, the only difference being how the read address is transferred (saving a whooping 2 bytes which is totally irrelevant for the amounts of data coreboot tends to read). We originally implemented Fast Read Dual Output since it's the older command and some older Winbond chips only supported that one... but it seems that some older Macronix parts for whatever reason chose to only support Fast Read Dual I/O instead. So in order to make this work for as many parts as possible, I guess we'll have to implement both. (Also, the Macronix device ID situation is utter madness with different chips with different capabilities often having the same ID, so we basically have to make a best-effort guess to strike a trade-off between fast speeds and best chance at supporting all chips. If this turns out to be a problem later, we may have to add Kconfig overrides for this or resort to SFDP parsing, although that would defeat the whole point of trying to be fast.)
BUG=b:193486682 TEST=Booted CoachZ (with Dual I/O)
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: Ia1a20581f251615127f132eadea367b7b66c4709 --- M src/drivers/spi/gigadevice.c M src/drivers/spi/macronix.c M src/drivers/spi/spi_flash.c M src/drivers/spi/spi_flash_internal.h M src/drivers/spi/winbond.c M src/include/spi_flash.h 6 files changed, 100 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/56287/1
diff --git a/src/drivers/spi/gigadevice.c b/src/drivers/spi/gigadevice.c index 15ad907..fc2a405 100644 --- a/src/drivers/spi/gigadevice.c +++ b/src/drivers/spi/gigadevice.c @@ -31,72 +31,84 @@ .id[0] = 0x4014, .nr_sectors_shift = 8, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, }, /* also GD25Q80B */ { /* GD25Q16 */ .id[0] = 0x4015, .nr_sectors_shift = 9, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, }, /* also GD25Q16B */ { /* GD25Q32B */ .id[0] = 0x4016, .nr_sectors_shift = 10, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, }, /* also GD25Q32B */ { /* GD25Q64 */ .id[0] = 0x4017, .nr_sectors_shift = 11, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, }, /* also GD25Q64B, GD25B64C */ { /* GD25Q128 */ .id[0] = 0x4018, .nr_sectors_shift = 12, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, }, /* also GD25Q128B */ { /* GD25VQ80C */ .id[0] = 0x4214, .nr_sectors_shift = 8, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, }, { /* GD25VQ16C */ .id[0] = 0x4215, .nr_sectors_shift = 9, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, }, { /* GD25LQ80 */ .id[0] = 0x6014, .nr_sectors_shift = 8, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, }, { /* GD25LQ16 */ .id[0] = 0x6015, .nr_sectors_shift = 9, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, }, { /* GD25LQ32 */ .id[0] = 0x6016, .nr_sectors_shift = 10, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, }, { /* GD25LQ64C */ .id[0] = 0x6017, .nr_sectors_shift = 11, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, }, /* also GD25LB64C */ { /* GD25LQ128 */ .id[0] = 0x6018, .nr_sectors_shift = 12, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, }, };
diff --git a/src/drivers/spi/macronix.c b/src/drivers/spi/macronix.c index 69e7a9f..b6981e0 100644 --- a/src/drivers/spi/macronix.c +++ b/src/drivers/spi/macronix.c @@ -63,55 +63,75 @@ .id[0] = 0x2415, .nr_sectors_shift = 9, }, + /* + * NOTE: C225xx JEDEC IDs are basically useless because Macronix keeps + * reusing the same IDs for vastly different chips. 35E versions always + * seem to support Dual I/O but not Dual Output, while 35F versions seem + * to support both, so we only set Dual I/O here to improve our chances + * of compatibility. Since Macronix makes it impossible to search all + * different parts that it recklessly assigned the same IDs to, it's + * hard to know if there may be parts that don't even support Dual I/O + * with these IDs, though (or what we should do if there are). + */ { /* MX25L1635E */ .id[0] = 0x2515, .nr_sectors_shift = 9, + .fast_read_dual_io_support = 1, }, { /* MX25U8032E */ .id[0] = 0x2534, .nr_sectors_shift = 8, + .fast_read_dual_io_support = 1, }, { - /* MX25U1635E */ + /* MX25U1635E/MX25U1635F */ .id[0] = 0x2535, .nr_sectors_shift = 9, + .fast_read_dual_io_support = 1, }, { - /* MX25U3235E */ + /* MX25U3235E/MX25U3235F */ .id[0] = 0x2536, .nr_sectors_shift = 10, + .fast_read_dual_io_support = 1, }, { - /* MX25U6435F */ + /* MX25U6435E/MX25U6435F */ .id[0] = 0x2537, .nr_sectors_shift = 11, + .fast_read_dual_io_support = 1, }, { /* MX25U12835F */ .id[0] = 0x2538, .nr_sectors_shift = 12, + .fast_read_dual_io_support = 1, }, { /* MX25U25635F */ .id[0] = 0x2539, .nr_sectors_shift = 13, + .fast_read_dual_io_support = 1, }, { - /* MX25U51245G */ + /* MX25U51235F */ .id[0] = 0x253a, .nr_sectors_shift = 14, + .fast_read_dual_io_support = 1, }, { /* MX25L12855E */ .id[0] = 0x2618, .nr_sectors_shift = 12, + .fast_read_dual_io_support = 1, }, { /* MX25L3235D/MX25L3225D/MX25L3236D/MX25L3237D */ .id[0] = 0x5e16, .nr_sectors_shift = 10, + .fast_read_dual_io_support = 1, }, { /* MX25L6495F */ diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c index b674749..02fe4de 100644 --- a/src/drivers/spi/spi_flash.c +++ b/src/drivers/spi/spi_flash.c @@ -20,7 +20,7 @@ cmd[3] = addr >> 0; }
-static int do_spi_flash_cmd(const struct spi_slave *spi, const void *dout, +static int do_spi_flash_cmd(const struct spi_slave *spi, const u8 *dout, size_t bytes_out, void *din, size_t bytes_in) { int ret; @@ -51,8 +51,8 @@ return ret; }
-static int do_dual_read_cmd(const struct spi_slave *spi, const void *dout, - size_t bytes_out, void *din, size_t bytes_in) +static int do_dual_output_cmd(const struct spi_slave *spi, const u8 *dout, + size_t bytes_out, void *din, size_t bytes_in) { int ret;
@@ -79,6 +79,31 @@ return ret; }
+static int do_dual_io_cmd(const struct spi_slave *spi, const u8 *dout, + size_t bytes_out, void *din, size_t bytes_in) +{ + int ret; + + /* Only the very first byte (opcode) is transfered in "single" mode. */ + struct spi_op vector = { .dout = dout, .bytesout = 1, + .din = NULL, .bytesin = 0 }; + + ret = spi_claim_bus(spi); + if (ret) + return ret; + + ret = spi_xfer_vector(spi, &vector, 1); + + if (!ret) + ret = spi->ctrlr->xfer_dual(spi, &dout[1], bytes_out - 1, NULL, 0); + + if (!ret) + ret = spi->ctrlr->xfer_dual(spi, NULL, 0, din, bytes_in); + + spi_release_bus(spi); + return ret; +} + int spi_flash_cmd(const struct spi_slave *spi, u8 cmd, void *response, size_t len) { int ret = do_spi_flash_cmd(spi, &cmd, sizeof(cmd), response, len); @@ -119,18 +144,23 @@ { u8 cmd[5]; int ret, cmd_len; - int (*do_cmd)(const struct spi_slave *spi, const void *din, + int (*do_cmd)(const struct spi_slave *spi, const u8 *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 if (flash->flags.dual_spi && flash->spi.ctrlr->xfer_dual) { + } else if (flash->flags.dual_io && flash->spi.ctrlr->xfer_dual) { + cmd_len = 5; + cmd[0] = CMD_READ_FAST_DUAL_IO; + cmd[4] = 0; + do_cmd = do_dual_io_cmd; + } else if (flash->flags.dual_output && flash->spi.ctrlr->xfer_dual) { cmd_len = 5; cmd[0] = CMD_READ_FAST_DUAL_OUTPUT; cmd[4] = 0; - do_cmd = do_dual_read_cmd; + do_cmd = do_dual_output_cmd; } else { cmd_len = 5; cmd[0] = CMD_READ_ARRAY_FAST; @@ -352,7 +382,8 @@ flash->pp_cmd = vi->desc->pp_cmd; flash->wren_cmd = vi->desc->wren_cmd;
- flash->flags.dual_spi = part->fast_read_dual_output_support; + flash->flags.dual_output = part->fast_read_dual_output_support; + flash->flags.dual_io = part->fast_read_dual_io_support;
flash->ops = &vi->desc->ops; flash->prot_ops = vi->prot_ops; @@ -471,8 +502,10 @@ }
const char *mode_string = ""; - if (flash->flags.dual_spi && spi.ctrlr->xfer_dual) - mode_string = " (Dual SPI mode)"; + if (flash->flags.dual_io && spi.ctrlr->xfer_dual) + mode_string = " (Dual I/O mode)"; + else if (flash->flags.dual_output && spi.ctrlr->xfer_dual) + mode_string = " (Dual Output mode)"; printk(BIOS_INFO, "SF: Detected %02x %04x with sector size 0x%x, total 0x%x%s\n", flash->vendor, flash->model, flash->sector_size, flash->size, mode_string); diff --git a/src/drivers/spi/spi_flash_internal.h b/src/drivers/spi/spi_flash_internal.h index b4d39b3..4a7beea 100644 --- a/src/drivers/spi/spi_flash_internal.h +++ b/src/drivers/spi/spi_flash_internal.h @@ -15,6 +15,7 @@ #define CMD_READ_ARRAY_LEGACY 0xe8
#define CMD_READ_FAST_DUAL_OUTPUT 0x3b +#define CMD_READ_FAST_DUAL_IO 0xbb
#define CMD_READ_STATUS 0x05 #define CMD_WRITE_ENABLE 0x06 @@ -69,8 +70,9 @@ uint16_t id[2]; /* Log based 2 total number of sectors. */ uint16_t nr_sectors_shift: 4; - uint16_t fast_read_dual_output_support : 1; - uint16_t _reserved_for_flags: 3; + uint16_t fast_read_dual_output_support : 1; /* 1-1-2 read */ + uint16_t fast_read_dual_io_support : 1; /* 1-2-2 read */ + uint16_t _reserved_for_flags: 2; /* Block protection. Currently used by Winbond. */ uint16_t protection_granularity_shift : 5; uint16_t bp_bits : 3; diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index 9fe0492..8f93e3b 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -101,12 +101,14 @@ .id[0] = 0x4014, .nr_sectors_shift = 8, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, }, { /* W25Q16_V */ .id[0] = 0x4015, .nr_sectors_shift = 9, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, .protection_granularity_shift = 16, .bp_bits = 3, }, @@ -115,6 +117,7 @@ .id[0] = 0x6015, .nr_sectors_shift = 9, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, .protection_granularity_shift = 16, .bp_bits = 3, }, @@ -123,6 +126,7 @@ .id[0] = 0x4016, .nr_sectors_shift = 10, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, .protection_granularity_shift = 16, .bp_bits = 3, }, @@ -131,6 +135,7 @@ .id[0] = 0x6016, .nr_sectors_shift = 10, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, .protection_granularity_shift = 16, .bp_bits = 3, }, @@ -139,6 +144,7 @@ .id[0] = 0x4017, .nr_sectors_shift = 11, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, .protection_granularity_shift = 17, .bp_bits = 3, }, @@ -147,6 +153,7 @@ .id[0] = 0x6017, .nr_sectors_shift = 11, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, .protection_granularity_shift = 17, .bp_bits = 3, }, @@ -155,6 +162,7 @@ .id[0] = 0x8017, .nr_sectors_shift = 11, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, .protection_granularity_shift = 17, .bp_bits = 3, }, @@ -163,6 +171,7 @@ .id[0] = 0x4018, .nr_sectors_shift = 12, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, .protection_granularity_shift = 18, .bp_bits = 3, }, @@ -171,6 +180,7 @@ .id[0] = 0x6018, .nr_sectors_shift = 12, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, .protection_granularity_shift = 18, .bp_bits = 3, }, @@ -179,6 +189,7 @@ .id[0] = 0x7018, .nr_sectors_shift = 12, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, .protection_granularity_shift = 18, .bp_bits = 3, }, @@ -187,6 +198,7 @@ .id[0] = 0x8018, .nr_sectors_shift = 12, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, .protection_granularity_shift = 18, .bp_bits = 3, }, @@ -195,6 +207,7 @@ .id[0] = 0x8020, .nr_sectors_shift = 11, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, .protection_granularity_shift = 16, .bp_bits = 4, }, @@ -203,6 +216,7 @@ .id[0] = 0x4019, .nr_sectors_shift = 13, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, .protection_granularity_shift = 16, .bp_bits = 4, }, @@ -211,6 +225,7 @@ .id[0] = 0x7019, .nr_sectors_shift = 13, .fast_read_dual_output_support = 1, + .fast_read_dual_io_support = 1, .protection_granularity_shift = 16, .bp_bits = 4, }, diff --git a/src/include/spi_flash.h b/src/include/spi_flash.h index a7f707e..fe77e3e 100644 --- a/src/include/spi_flash.h +++ b/src/include/spi_flash.h @@ -86,8 +86,9 @@ union { u8 raw; struct { - u8 dual_spi : 1; - u8 _reserved : 7; + u8 dual_output : 1; + u8 dual_io : 1; + u8 _reserved : 6; }; } flags; u16 model;
-
Julius Werner (Code Review)