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>

12 files changed, 35 insertions, 103 deletions

diff --git a/src/drivers/spi/adesto.c b/src/drivers/spi/adesto.c
index a805609c5e..4e1043edb7 100644
--- a/src/drivers/spi/adesto.c
+++ b/src/drivers/spi/adesto.c
@@ -201,11 +201,6 @@ out:
 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 64c91cc79f..b580dc3df0 100644
--- a/src/drivers/spi/amic.c
+++ b/src/drivers/spi/amic.c
@@ -176,11 +176,6 @@ out:
 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 7d6e172755..58a2862eeb 100644
--- a/src/drivers/spi/atmel.c
+++ b/src/drivers/spi/atmel.c
@@ -157,11 +157,6 @@ out:
 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 33e12a000f..f3cf70ef1b 100644
--- a/src/drivers/spi/eon.c
+++ b/src/drivers/spi/eon.c
@@ -292,7 +292,6 @@ static const struct spi_flash_ops spi_flash_ops = {
 	.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 714ed894f7..cc4cf1dddb 100644
--- a/src/drivers/spi/gigadevice.c
+++ b/src/drivers/spi/gigadevice.c
@@ -221,11 +221,6 @@ static const struct spi_flash_ops spi_flash_ops = {
 	.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 1610ca18c8..5a97b8f794 100644
--- a/src/drivers/spi/macronix.c
+++ b/src/drivers/spi/macronix.c
@@ -268,11 +268,6 @@ static const struct spi_flash_ops spi_flash_ops = {
 	.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 e687bf8471..4a241baf09 100644
--- a/src/drivers/spi/spansion.c
+++ b/src/drivers/spi/spansion.c
@@ -282,7 +282,6 @@ static int spansion_write(const struct spi_flash *flash, u32 offset, size_t len,
 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 ae1d2efb22..5e42a375f6 100644
--- a/src/drivers/spi/spi_flash.c
+++ b/src/drivers/spi/spi_flash.c
@@ -31,7 +31,7 @@ static void spi_flash_addr(u32 addr, u8 *cmd)
 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 @@ static int do_spi_flash_cmd(const struct spi_slave *spi, const void *dout,
 	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 @@ int spi_flash_cmd(const struct spi_slave *spi, u8 cmd, void *response, size_t le
 	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 @@ int spi_flash_cmd_write(const struct spi_slave *spi, const u8 *cmd,
 }
 #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;
-	uint8_t *data = buf;
+	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 @@ static int spi_flash_cmd_read_array_wrapped(const struct spi_slave *spi,
 	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 @@ int spi_flash_cmd_poll_bit(const struct spi_flash *flash, unsigned long timeout,
 	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_probe(unsigned int bus, unsigned int cs, struct spi_flash *flash)
 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 a89610a7bc..f15c737461 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 e4ea7805dd..abe3f2ace6 100644
--- a/src/drivers/spi/sst.c
+++ b/src/drivers/spi/sst.c
@@ -55,14 +55,12 @@ static const struct spi_flash_ops spi_flash_ops_write_ai = {
 	.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 fb24b27266..6625764268 100644
--- a/src/drivers/spi/stmicro.c
+++ b/src/drivers/spi/stmicro.c
@@ -351,7 +351,6 @@ out:
 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 11a518746c..d0ef3cd6e0 100644
--- a/src/drivers/spi/winbond.c
+++ b/src/drivers/spi/winbond.c
@@ -646,11 +646,6 @@ static const struct spi_flash_ops spi_flash_ops = {
 	.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,
 };