spi25_statusreg: make register read/write functions generic

This patch adds new spi_{read,write}_register() functions that take the
source/destination register as an argument. Currently they can only
access SR1, support for other registers will be added in another patch.

Since we're refactoring things, this commit also makes
spi_read_register() return an error code, making it possible to identify
error conditions that spi_read_status_register() concealed.

This also removes the initial 100ms delay between writing a register and
the first attempt to check the chip's status. An initial delay was added
to avoid needing to read the status register multiple times, but that is
unlikely to cause problems on modern flash chips.

BUG=b:195381327,b:153800563
BRANCH=none
TEST=flashrom -{r,w,E}
TEST=flashrom --wp-{enable,disable,range,list,status} at end of patch series

Change-Id: I0a3951bbf993f2d8d830143b29d3ce16cc6901d7
Signed-off-by: Nikolai Artemiev <nartemiev@google.com>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/58475
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Nico Huber <nico.h@gmx.de>
Reviewed-by: Anastasia Klimchuk <aklm@chromium.org>
Reviewed-by: Edward O'Callaghan <quasisec@chromium.org>

3 files changed, 95 insertions, 39 deletions

diff --git a/chipdrivers.h b/chipdrivers.h
index e1d6aa9e7..ea8d480d5 100644
--- a/chipdrivers.h
+++ b/chipdrivers.h
@@ -62,8 +62,10 @@ int spi_set_extended_address(struct flashctx *, uint8_t addr_high);
 
 
 /* spi25_statusreg.c */
+/* FIXME: replace spi_read_status_register() calls with spi_read_register() */
 uint8_t spi_read_status_register(const struct flashctx *flash);
-int spi_write_status_register(const struct flashctx *flash, int status);
+int spi_read_register(const struct flashctx *flash, enum flash_reg reg, uint8_t *value);
+int spi_write_register(const struct flashctx *flash, enum flash_reg reg, uint8_t value);
 void spi_prettyprint_status_register_bit(uint8_t status, int bit);
 int spi_prettyprint_status_register_plain(struct flashctx *flash);
 int spi_prettyprint_status_register_default_welwip(struct flashctx *flash);
diff --git a/flash.h b/flash.h
index 654cdee6d..f1a8b340e 100644
--- a/flash.h
+++ b/flash.h
@@ -166,6 +166,13 @@ struct flashrom_flashctx;
 #define flashctx flashrom_flashctx /* TODO: Agree on a name and convert all occurences. */
 typedef int (erasefunc_t)(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
 
+enum flash_reg {
+	INVALID_REG = 0,
+	STATUS1,
+	STATUS2,
+	MAX_REGISTERS
+};
+
 struct flashchip {
 	const char *vendor;
 	const char *name;
diff --git a/spi25_statusreg.c b/spi25_statusreg.c
index a0b0fcf5e..0d7bc25b9 100644
--- a/spi25_statusreg.c
+++ b/spi25_statusreg.c
@@ -22,24 +22,23 @@
 #include "spi.h"
 
 /* === Generic functions === */
-static int spi_write_status_register_flag(const struct flashctx *flash, int status, const unsigned char enable_opcode)
+static int spi_write_register_flag(const struct flashctx *flash, uint8_t enable_opcode, uint8_t *write_cmd, size_t write_cmd_len, enum flash_reg reg)
 {
-	int result;
-	int i = 0;
 	/*
-	 * WRSR requires either EWSR or WREN depending on chip type.
-	 * The code below relies on the fact hat EWSR and WREN have the same
-	 * INSIZE and OUTSIZE.
+	 * Enabling register writes requires either EWSR or WREN depending on
+	 * chip type. The code below relies on the fact hat EWSR and WREN have
+	 * the same INSIZE and OUTSIZE.
 	 */
+
 	struct spi_command cmds[] = {
 	{
 		.writecnt	= JEDEC_WREN_OUTSIZE,
-		.writearr	= (const unsigned char[]){ enable_opcode },
+		.writearr	= &enable_opcode,
 		.readcnt	= 0,
 		.readarr	= NULL,
 	}, {
-		.writecnt	= JEDEC_WRSR_OUTSIZE,
-		.writearr	= (const unsigned char[]){ JEDEC_WRSR, (unsigned char) status },
+		.writecnt	= write_cmd_len,
+		.writearr	= write_cmd,
 		.readcnt	= 0,
 		.readarr	= NULL,
 	}, {
@@ -49,69 +48,117 @@ static int spi_write_status_register_flag(const struct flashctx *flash, int stat
 		.readarr	= NULL,
 	}};
 
-	result = spi_send_multicommand(flash, cmds);
+	int result = spi_send_multicommand(flash, cmds);
 	if (result) {
 		msg_cerr("%s failed during command execution\n", __func__);
-		/* No point in waiting for the command to complete if execution
+		/*
+		 * No point in waiting for the command to complete if execution
 		 * failed.
 		 */
 		return result;
 	}
-	/* WRSR performs a self-timed erase before the changes take effect.
+
+	/*
+	 * WRSR performs a self-timed erase before the changes take effect.
 	 * This may take 50-85 ms in most cases, and some chips apparently
 	 * allow running RDSR only once. Therefore pick an initial delay of
 	 * 100 ms, then wait in 10 ms steps until a total of 5 s have elapsed.
+	 *
+	 * Newer chips with multiple status registers (SR2 etc.) are unlikely
+	 * to have problems with multiple RDSR commands, so only wait for the
+	 * initial 100 ms if the register we wrote to was SR1.
 	 */
-	programmer_delay(100 * 1000);
-	while (spi_read_status_register(flash) & SPI_SR_WIP) {
-		if (++i > 490) {
-			msg_cerr("Error: WIP bit after WRSR never cleared\n");
-			return TIMEOUT_ERROR;
-		}
+	int delay_ms = 5000;
+	if (reg == STATUS1) {
+		programmer_delay(100 * 1000);
+		delay_ms -= 100;
+	}
+
+	for (; delay_ms > 0; delay_ms -= 10) {
+		if ((spi_read_status_register(flash) & SPI_SR_WIP) == 0)
+			return 0;
 		programmer_delay(10 * 1000);
 	}
-	return 0;
+
+	msg_cerr("Error: WIP bit after WRSR never cleared\n");
+	return TIMEOUT_ERROR;
 }
 
-int spi_write_status_register(const struct flashctx *flash, int status)
+int spi_write_register(const struct flashctx *flash, enum flash_reg reg, uint8_t value)
 {
 	int feature_bits = flash->chip->feature_bits;
-	int ret = 1;
+
+	uint8_t write_cmd[3];
+	size_t write_cmd_len = 0;
+
+	/*
+	 * Create SPI write command sequence based on the destination register
+	 * and the chip's supported command set.
+	 */
+	switch (reg) {
+	case STATUS1:
+		write_cmd[0] = JEDEC_WRSR;
+		write_cmd[1] = value;
+		write_cmd_len = JEDEC_WRSR_OUTSIZE;
+		break;
+	default:
+		msg_cerr("Cannot write register: unknown register\n");
+		return 1;
+	}
 
 	if (!(feature_bits & (FEATURE_WRSR_WREN | FEATURE_WRSR_EWSR))) {
 		msg_cdbg("Missing status register write definition, assuming "
 			 "EWSR is needed\n");
 		feature_bits |= FEATURE_WRSR_EWSR;
 	}
+
+	int ret = 1;
 	if (feature_bits & FEATURE_WRSR_WREN)
-		ret = spi_write_status_register_flag(flash, status, JEDEC_WREN);
+		ret = spi_write_register_flag(flash, JEDEC_WREN, write_cmd, write_cmd_len, reg);
 	if (ret && (feature_bits & FEATURE_WRSR_EWSR))
-		ret = spi_write_status_register_flag(flash, status, JEDEC_EWSR);
+		ret = spi_write_register_flag(flash, JEDEC_EWSR, write_cmd, write_cmd_len, reg);
 	return ret;
 }
 
-uint8_t spi_read_status_register(const struct flashctx *flash)
+int spi_read_register(const struct flashctx *flash, enum flash_reg reg, uint8_t *value)
 {
-	static const unsigned char cmd[JEDEC_RDSR_OUTSIZE] = { JEDEC_RDSR };
+	uint8_t read_cmd;
+
+	switch (reg) {
+	case STATUS1:
+		read_cmd = JEDEC_RDSR;
+		break;
+	default:
+		msg_cerr("Cannot read register: unknown register\n");
+		return 1;
+	}
+
 	/* FIXME: No workarounds for driver/hardware bugs in generic code. */
-	unsigned char readarr[2]; /* JEDEC_RDSR_INSIZE=1 but wbsio needs 2 */
-	int ret;
+	/* JEDEC_RDSR_INSIZE=1 but wbsio needs 2 */
+	uint8_t readarr[2];
 
-	/* Read Status Register */
-	ret = spi_send_command(flash, sizeof(cmd), sizeof(readarr), cmd, readarr);
+	int ret = spi_send_command(flash, sizeof(read_cmd), sizeof(readarr), &read_cmd, readarr);
 	if (ret) {
-		msg_cerr("RDSR failed!\n");
-		/* FIXME: We should propagate the error. */
-		return 0;
+		msg_cerr("Register read failed!\n");
+		return ret;
 	}
 
-	return readarr[0];
+	*value = readarr[0];
+	return 0;
+}
+
+uint8_t spi_read_status_register(const struct flashctx *flash)
+{
+	uint8_t status = 0;
+	/* FIXME: We should propagate the error. */
+	spi_read_register(flash, STATUS1, &status);
+	return status;
 }
 
 static int spi_restore_status(struct flashctx *flash, uint8_t status)
 {
 	msg_cdbg("restoring chip status (0x%02x)\n", status);
-	return spi_write_status_register(flash, status);
+	return spi_write_register(flash, STATUS1, status);
 }
 
 /* A generic block protection disable.
@@ -156,9 +203,9 @@ static int spi_disable_blockprotect_generic(struct flashctx *flash, uint8_t bp_m
 			return 1;
 		}
 		/* All bits except the register lock bit (often called SPRL, SRWD, WPEN) are readonly. */
-		result = spi_write_status_register(flash, status & ~lock_mask);
+		result = spi_write_register(flash, STATUS1, status & ~lock_mask);
 		if (result) {
-			msg_cerr("spi_write_status_register failed.\n");
+			msg_cerr("Could not write status register 1.\n");
 			return result;
 		}
 		status = spi_read_status_register(flash);
@@ -169,9 +216,9 @@ static int spi_disable_blockprotect_generic(struct flashctx *flash, uint8_t bp_m
 		msg_cdbg("done.\n");
 	}
 	/* Global unprotect. Make sure to mask the register lock bit as well. */
-	result = spi_write_status_register(flash, status & ~(bp_mask | lock_mask) & unprotect_mask);
+	result = spi_write_register(flash, STATUS1, status & ~(bp_mask | lock_mask) & unprotect_mask);
 	if (result) {
-		msg_cerr("spi_write_status_register failed.\n");
+		msg_cerr("Could not write status register 1.\n");
 		return result;
 	}
 	status = spi_read_status_register(flash);