Change in flashrom[master]: spi25_statusreg: delete spi_read_status_register()

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/59529 )

Change subject: spi25_statusreg: delete spi_read_status_register()
......................................................................

spi25_statusreg: delete spi_read_status_register()

Delete the spi_read_status_register() function because the generic
spi_read_register() function can be used instead.

This patch also converts all call sites over to spi_read_register().
A side effect is that error codes are now properly propagated and
checked.

BUG=b:195381327,b:153800563
BRANCH=none
TEST=flashrom -{r,w,E}
TEST=Tested with a W25Q128.W flash on a kasumi (AMD) dut.
     Read SR1/SR2 with --wp-status and activated various WP ranges
     that toggled bits in both SR1 and SR2.

Change-Id: I146b4b5439872e66c5d33e156451a729d248c7da
Signed-off-by: Nikolai Artemiev 
Reviewed-on: https://review.coreboot.org/c/flashrom/+/59529
Tested-by: build bot (Jenkins) 
Reviewed-by: Edward O'Callaghan 
Reviewed-by: Anastasia Klimchuk 
---
M chipdrivers.h
M it87spi.c
M s25f.c
M spi25.c
M spi25_statusreg.c
5 files changed, 141 insertions(+), 46 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Edward O'Callaghan: Looks good to me, approved
  Anastasia Klimchuk: Looks good to me, approved

diff --git a/chipdrivers.h b/chipdrivers.h
index c223534..0695993 100644
--- a/chipdrivers.h
+++ b/chipdrivers.h
@@ -62,8 +62,6 @@

/* 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_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);
diff --git a/it87spi.c b/it87spi.c
index 728ec70..204816a 100644
--- a/it87spi.c
+++ b/it87spi.c
@@ -133,9 +133,20 @@
    OUTB(0, data->flashport);
    /* Wait until the Write-In-Progress bit is cleared.
     * This usually takes 1-10 ms, so wait in 1 ms steps.
+	 *
+	 * FIXME: This should timeout after some number of retries.
     */
-	while (spi_read_status_register(flash) & SPI_SR_WIP)
+	while (true) {
+		uint8_t status;
+		int ret = spi_read_register(flash, STATUS1, &status);
+		if (ret)
+		       return ret;
+
+		if((status & SPI_SR_WIP) == 0)
+			return 0;
+
    	programmer_delay(1000);
+	}
    return 0;
 }

@@ -277,7 +288,9 @@
    	}

while (len >= chip->page_size) {
-			it8716f_spi_page_program(flash, buf, start);
+			int ret = it8716f_spi_page_program(flash, buf, start);
+			if (ret)
+				return ret;
    		start += chip->page_size;
    		len -= chip->page_size;
    		buf += chip->page_size;
diff --git a/s25f.c b/s25f.c
index 0bd4a7c..82ca610 100644
--- a/s25f.c
+++ b/s25f.c
@@ -133,9 +133,14 @@

static int s25f_poll_status(const struct flashctx *flash)
 {
-	uint8_t tmp = spi_read_status_register(flash);
+	while (true) {
+		uint8_t tmp;
+		if (spi_read_register(flash, STATUS1, &tmp))
+			return -1;

-	while (tmp & SPI_SR_WIP) {
+		if ((tmp & SPI_SR_WIP) == 0)
+			break;
+
    	/*
    	 * The WIP bit on S25F chips remains set to 1 if erase or
    	 * programming errors occur, so we must check for those
@@ -156,7 +161,6 @@
    	}

programmer_delay(1000 * 10);
-		tmp = spi_read_status_register(flash);
    }

return 0;
diff --git a/spi25.c b/spi25.c
index 213273f..e96a38d 100644
--- a/spi25.c
+++ b/spi25.c
@@ -304,12 +304,17 @@

static int spi_poll_wip(struct flashctx *const flash, const unsigned int poll_delay)
 {
-	/* FIXME: We can't tell if spi_read_status_register() failed. */
    /* FIXME: We don't time out. */
-	while (spi_read_status_register(flash) & SPI_SR_WIP)
+	while (true) {
+		uint8_t status;
+		int ret = spi_read_register(flash, STATUS1, &status);
+		if (ret)
+			return ret;
+		if (!(status & SPI_SR_WIP))
+			return 0;
+
    	programmer_delay(poll_delay);
-	/* FIXME: Check the status register for errors. */
-	return 0;
+	}
 }

/**
diff --git a/spi25_statusreg.c b/spi25_statusreg.c
index 249ab9a..6b16839 100644
--- a/spi25_statusreg.c
+++ b/spi25_statusreg.c
@@ -123,7 +123,11 @@
    }

for (; delay_ms > 0; delay_ms -= 10) {
-		if ((spi_read_status_register(flash) & SPI_SR_WIP) == 0)
+		uint8_t status;
+		result = spi_read_register(flash, STATUS1, &status);
+		if (result)
+			return result;
+		if ((status & SPI_SR_WIP) == 0)
    		return 0;
    	programmer_delay(10 * 1000);
    }
@@ -168,14 +172,6 @@
    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);
@@ -207,7 +203,10 @@
    uint8_t status;
    int result;

-	status = spi_read_status_register(flash);
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
+
    if ((status & bp_mask) == 0) {
    	msg_cdbg2("Block protection is disabled.\n");
    	return 0;
@@ -229,7 +228,11 @@
    		msg_cerr("Could not write status register 1.\n");
    		return result;
    	}
-		status = spi_read_status_register(flash);
+
+		ret = spi_read_register(flash, STATUS1, &status);
+		if (ret)
+			return ret;
+
    	if ((status & lock_mask) != 0) {
    		msg_cerr("Unsetting lock bit(s) failed.\n");
    		return 1;
@@ -242,7 +245,11 @@
    	msg_cerr("Could not write status register 1.\n");
    	return result;
    }
-	status = spi_read_status_register(flash);
+
+	ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
+
    if ((status & bp_mask) != 0) {
    	msg_cerr("Block protection could not be disabled!\n");
    	if (flash->chip->printlock)
@@ -363,7 +370,10 @@

int spi_prettyprint_status_register_plain(struct flashctx *flash)
 {
-	uint8_t status = spi_read_status_register(flash);
+	uint8_t status;
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
    spi_prettyprint_status_register_hex(status);
    return 0;
 }
@@ -371,7 +381,10 @@
 /* Print the plain hex value and the welwip bits only. */
 int spi_prettyprint_status_register_default_welwip(struct flashctx *flash)
 {
-	uint8_t status = spi_read_status_register(flash);
+	uint8_t status;
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
    spi_prettyprint_status_register_hex(status);

spi_prettyprint_status_register_welwip(status);
@@ -384,7 +397,10 @@
  */
 int spi_prettyprint_status_register_bp1_srwd(struct flashctx *flash)
 {
-	uint8_t status = spi_read_status_register(flash);
+	uint8_t status;
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
    spi_prettyprint_status_register_hex(status);

spi_prettyprint_status_register_srwd(status);
@@ -402,7 +418,10 @@
  */
 int spi_prettyprint_status_register_bp2_srwd(struct flashctx *flash)
 {
-	uint8_t status = spi_read_status_register(flash);
+	uint8_t status;
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
    spi_prettyprint_status_register_hex(status);

spi_prettyprint_status_register_srwd(status);
@@ -419,7 +438,10 @@
  */
 int spi_prettyprint_status_register_bp3_srwd(struct flashctx *flash)
 {
-	uint8_t status = spi_read_status_register(flash);
+	uint8_t status;
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
    spi_prettyprint_status_register_hex(status);

spi_prettyprint_status_register_srwd(status);
@@ -431,7 +453,10 @@

int spi_prettyprint_status_register_bp4_srwd(struct flashctx *flash)
 {
-	uint8_t status = spi_read_status_register(flash);
+	uint8_t status;
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
    spi_prettyprint_status_register_hex(status);

spi_prettyprint_status_register_srwd(status);
@@ -442,7 +467,10 @@

int spi_prettyprint_status_register_bp2_bpl(struct flashctx *flash)
 {
-	uint8_t status = spi_read_status_register(flash);
+	uint8_t status;
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
    spi_prettyprint_status_register_hex(status);

spi_prettyprint_status_register_bpl(status);
@@ -455,7 +483,10 @@

int spi_prettyprint_status_register_bp2_tb_bpl(struct flashctx *flash)
 {
-	uint8_t status = spi_read_status_register(flash);
+	uint8_t status;
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
    spi_prettyprint_status_register_hex(status);

spi_prettyprint_status_register_bpl(status);
@@ -477,7 +508,10 @@

int spi_prettyprint_status_register_amic_a25l032(struct flashctx *flash)
 {
-	uint8_t status = spi_read_status_register(flash);
+	uint8_t status;
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
    spi_prettyprint_status_register_hex(status);

spi_prettyprint_status_register_srwd(status);
@@ -533,7 +567,11 @@

int spi_prettyprint_status_register_at25df(struct flashctx *flash)
 {
-	uint8_t status = spi_read_status_register(flash);
+	uint8_t status;
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
+
    spi_prettyprint_status_register_hex(status);

spi_prettyprint_status_register_atmel_at25_srpl(status);
@@ -556,8 +594,10 @@
 int spi_prettyprint_status_register_at25f(struct flashctx *flash)
 {
    uint8_t status;
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;

-	status = spi_read_status_register(flash);
    spi_prettyprint_status_register_hex(status);

spi_prettyprint_status_register_atmel_at25_wpen(status);
@@ -572,8 +612,10 @@
 int spi_prettyprint_status_register_at25f512a(struct flashctx *flash)
 {
    uint8_t status;
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;

-	status = spi_read_status_register(flash);
    spi_prettyprint_status_register_hex(status);

spi_prettyprint_status_register_atmel_at25_wpen(status);
@@ -588,7 +630,10 @@

int spi_prettyprint_status_register_at25f512b(struct flashctx *flash)
 {
-	uint8_t status = spi_read_status_register(flash);
+	uint8_t status;
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
    spi_prettyprint_status_register_hex(status);

spi_prettyprint_status_register_atmel_at25_srpl(status);
@@ -604,7 +649,10 @@
 {
    uint8_t status;

-	status = spi_read_status_register(flash);
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
+
    spi_prettyprint_status_register_hex(status);

spi_prettyprint_status_register_atmel_at25_wpen(status);
@@ -617,7 +665,10 @@

int spi_prettyprint_status_register_at25fs010(struct flashctx *flash)
 {
-	uint8_t status = spi_read_status_register(flash);
+	uint8_t status;
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
    spi_prettyprint_status_register_hex(status);

spi_prettyprint_status_register_atmel_at25_wpen(status);
@@ -637,7 +688,10 @@

int spi_prettyprint_status_register_at25fs040(struct flashctx *flash)
 {
-	uint8_t status = spi_read_status_register(flash);
+	uint8_t status;
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
    spi_prettyprint_status_register_hex(status);

spi_prettyprint_status_register_atmel_at25_wpen(status);
@@ -649,7 +703,10 @@

int spi_prettyprint_status_register_at26df081a(struct flashctx *flash)
 {
-	uint8_t status = spi_read_status_register(flash);
+	uint8_t status;
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
    spi_prettyprint_status_register_hex(status);

spi_prettyprint_status_register_atmel_at25_srpl(status);
@@ -707,7 +764,10 @@

int spi_prettyprint_status_register_en25s_wp(struct flashctx *flash)
 {
-	uint8_t status = spi_read_status_register(flash);
+	uint8_t status;
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
    spi_prettyprint_status_register_hex(status);

spi_prettyprint_status_register_srwd(status);
@@ -726,7 +786,10 @@

int spi_prettyprint_status_register_n25q(struct flashctx *flash)
 {
-	uint8_t status = spi_read_status_register(flash);
+	uint8_t status;
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
    spi_prettyprint_status_register_hex(status);

spi_prettyprint_status_register_srwd(status);
@@ -751,7 +814,10 @@
 /* Used by Intel/Numonyx S33 and Spansion S25FL-S chips */
 int spi_prettyprint_status_register_bp2_ep_srwd(struct flashctx *flash)
 {
-	uint8_t status = spi_read_status_register(flash);
+	uint8_t status;
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
    spi_prettyprint_status_register_hex(status);

spi_prettyprint_status_register_srwd(status);
@@ -779,7 +845,10 @@

int spi_prettyprint_status_register_sst25(struct flashctx *flash)
 {
-	uint8_t status = spi_read_status_register(flash);
+	uint8_t status;
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
    spi_prettyprint_status_register_sst25_common(status);
    return 0;
 }
@@ -795,7 +864,10 @@
    	"100000H-1FFFFFH",
    	"all", "all"
    };
-	uint8_t status = spi_read_status_register(flash);
+	uint8_t status;
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
    spi_prettyprint_status_register_sst25_common(status);
    msg_cdbg("Resulting block protection : %s\n", bpt[(status & 0x1c) >> 2]);
    return 0;
@@ -810,7 +882,10 @@
    	"0x40000-0x7ffff",
    	"all blocks", "all blocks", "all blocks", "all blocks"
    };
-	uint8_t status = spi_read_status_register(flash);
+	uint8_t status;
+	int ret = spi_read_register(flash, STATUS1, &status);
+	if (ret)
+		return ret;
    spi_prettyprint_status_register_sst25_common(status);
    msg_cdbg("Resulting block protection : %s\n", bpt[(status & 0x1c) >> 2]);
    return 0;

65 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.