Change in flashrom[master]: spi: Allow cached ID bytes to be cleared

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/41399 )

Change subject: spi: Allow cached ID bytes to be cleared
......................................................................

spi: Allow cached ID bytes to be cleared

This does two things:
1. Removes the static variables from various ID functions and moves
   them to a single struct.
2. Adds a function to clear the struct.

The idea of the original caching mechanism introduced years ago was
simply to speed up probe_flash() by not issuing the same read ID
commands dozens or hundreds of times for a single chip.

That implementation assumed that we would only call probe_flash()
once. However, there are cases when we want to call probe_flash()
multiple times, for example, if using an external programmer and
using different voltages (for probing the chip).

This patch is extremely similar to the original works of:
 `commit 57b7524b1448189d3630d6c4735e60dbbdf14d51`.
 Author: David Hendricks 
&&
 `commit 7f7c711ee1a41649607f81f1533e5135fc1361fc`.
 Author: David Hendricks

BUG=b:15656443
BRANCH=none
TEST=none

Change-Id: I879cb08dbe66db9ab0c3b8a7f93b04fe1c5980f4
Signed-off-by: Edward O'Callaghan 
---
M spi.h
M spi25.c
2 files changed, 50 insertions(+), 26 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/99/41399/1

diff --git a/spi.h b/spi.h
index 3f45038..c1ca46a 100644
--- a/spi.h
+++ b/spi.h
@@ -192,4 +192,6 @@
 #define SPI_FLASHROM_BUG	-5
 #define SPI_PROGRAMMER_ERROR	-6

+void clear_spi_id_cache(void);
+
 #endif		/* !__SPI_H__ */
diff --git a/spi25.c b/spi25.c
index fb91903..b4fbe6d 100644
--- a/spi25.c
+++ b/spi25.c
@@ -27,6 +27,26 @@
 #include "programmer.h"
 #include "spi.h"

+enum id_type {
+	RDID,
+	RDID4,
+	REMS,
+	RES2,
+	RES3,
+	NUM_ID_TYPES,
+};
+
+static struct {
+	int is_cached;
+	unsigned char bytes[4];		/* enough to hold largest ID type */
+} id_cache[NUM_ID_TYPES];
+
+void clear_spi_id_cache(void)
+{
+	memset(id_cache, 0, sizeof(id_cache));
+	return;
+}
+
 static int spi_rdid(struct flashctx *flash, unsigned char *readarr, int bytes)
 {
    static const unsigned char cmd[JEDEC_RDID_OUTSIZE] = { JEDEC_RDID };
@@ -138,18 +158,20 @@
 static int probe_spi_rdid_generic(struct flashctx *flash, int bytes)
 {
    const struct flashchip *chip = flash->chip;
-	unsigned char readarr[4];
    uint32_t id1;
    uint32_t id2;
+	enum id_type idty = bytes == 3 ? RDID : RDID4;

-	const int ret = spi_rdid(flash, readarr, bytes);
-	if (ret == SPI_INVALID_LENGTH)
-		msg_cinfo("%d byte RDID not supported on this SPI controller\n", bytes);
-	if (ret)
-		return 0;
+	if (!id_cache[idty].is_cached) {
+		const int ret = spi_rdid(flash, id_cache[idty].bytes, bytes);
+		if (ret == SPI_INVALID_LENGTH)
+			msg_cinfo("%d byte RDID not supported on this SPI controller\n", bytes);
+		if (ret)
+			return 0;
+		id_cache[idty].is_cached = 1;
+	}

-	rdid_get_ids(readarr, bytes, &id1, &id2);
-
+	rdid_get_ids(id_cache[idty].bytes, bytes, &id1, &id2);
    return compare_id(chip, id1, id2);
 }

@@ -166,16 +188,16 @@
 int probe_spi_rems(struct flashctx *flash)
 {
    const struct flashchip *chip = flash->chip;
-	unsigned char readarr[JEDEC_REMS_INSIZE];
    uint32_t id1, id2;

-	if (spi_rems(flash, readarr)) {
-		return 0;
+	if (!id_cache[REMS].is_cached) {
+		if (spi_rems(flash, id_cache[REMS].bytes))
+			return 0;
+		id_cache[REMS].is_cached = 1;
    }

-	id1 = readarr[0];
-	id2 = readarr[1];
-
+	id1 = id_cache[REMS].bytes[0];
+	id2 = id_cache[REMS].bytes[1];
    return compare_id(chip, id1, id2);
 }

@@ -222,16 +244,16 @@

int probe_spi_res2(struct flashctx *flash)
 {
-	unsigned char readarr[2];
    uint32_t id1, id2;

-	if (spi_res(flash, readarr, 2)) {
-		return 0;
+	if (!id_cache[RES2].is_cached) {
+		if (spi_res(flash, id_cache[RES2].bytes, 2))
+			return 0;
+		id_cache[RES2].is_cached = 1;
    }

-	id1 = readarr[0];
-	id2 = readarr[1];
-
+	id1 = id_cache[RES2].bytes[0];
+	id2 = id_cache[RES2].bytes[1];
    msg_cdbg("%s: id1 0x%x, id2 0x%x\n", __func__, id1, id2);

if (id1 != flash->chip->manufacture_id || id2 != flash->chip->model_id)
@@ -242,16 +264,16 @@

int probe_spi_res3(struct flashctx *flash)
 {
-	unsigned char readarr[3];
    uint32_t id1, id2;

-	if (spi_res(flash, readarr, 3)) {
-		return 0;
+	if (!id_cache[RES3].is_cached) {
+		if (spi_res(flash, id_cache[RES3].bytes, 3))
+			return 0;
+		id_cache[RES3].is_cached = 1;
    }

-	id1 = (readarr[0] << 8) | readarr[1];
-	id2 = readarr[2];
-
+	id1 = (id_cache[RES3].bytes[0] << 8) | id_cache[RES3].bytes[1];
+	id2 = id_cache[RES3].bytes[3];
    msg_cdbg("%s: id1 0x%x, id2 0x%x\n", __func__, id1, id2);

if (id1 != flash->chip->manufacture_id || id2 != flash->chip->model_id)

[Edward O'Callaghan (Code Review)]

Hello build bot (Jenkins), David Hendricks, Angel Pons,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/41399

to look at the new patch set (#2).

Change subject: spi: Allow cached ID bytes to be cleared
......................................................................

spi: Allow cached ID bytes to be cleared

This does two things:
1. Removes the static variables from various ID functions and moves
   them to a single struct.
2. Adds a function to clear the struct.

The idea of the original caching mechanism introduced years ago was
simply to speed up probe_flash() by not issuing the same read ID
commands dozens or hundreds of times for a single chip.

That implementation assumed that we would only call probe_flash()
once. However, there are cases when we want to call probe_flash()
multiple times, for example, if using an external programmer and
using different voltages (for probing the chip).

This patch is extremely similar to the original works of:
 `commit 57b7524b1448189d3630d6c4735e60dbbdf14d51`.
 Author: David Hendricks 
&&
 `commit 7f7c711ee1a41649607f81f1533e5135fc1361fc`.
 Author: David Hendricks

BUG=b:15656443
BRANCH=none
TEST=none

Change-Id: I879cb08dbe66db9ab0c3b8a7f93b04fe1c5980f4
Signed-off-by: Edward O'Callaghan 
---
M spi.h
M spi25.c
2 files changed, 51 insertions(+), 28 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/99/41399/2

[Edward O'Callaghan (Code Review)]

Hello build bot (Jenkins), David Hendricks, Angel Pons,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/41399

to look at the new patch set (#3).

Change subject: spi: Allow cached ID bytes to be cleared
......................................................................

spi: Allow cached ID bytes to be cleared

This does two things:
1. Removes the static variables from various ID functions and moves
   them to a single struct.
2. Adds a function to clear the struct.

The idea of the original caching mechanism introduced years ago was
simply to speed up probe_flash() by not issuing the same read ID
commands dozens or hundreds of times for a single chip.

That implementation assumed that we would only call probe_flash()
once. However, there are cases when we want to call probe_flash()
multiple times, for example, if using an external programmer and
using different voltages (for probing the chip).

This patch is extremely similar to the original works of:
 `commit 57b7524b1448189d3630d6c4735e60dbbdf14d51`.
 Author: David Hendricks 
&&
 `commit 7f7c711ee1a41649607f81f1533e5135fc1361fc`.
 Author: David Hendricks

BUG=b:15656443
BRANCH=none
TEST=none

Change-Id: I879cb08dbe66db9ab0c3b8a7f93b04fe1c5980f4
Signed-off-by: Edward O'Callaghan 
---
M spi.h
M spi25.c
2 files changed, 56 insertions(+), 34 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/99/41399/3

[Stefan Reinauer (Code Review)]

Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41399 )

Change subject: spi: Allow cached ID bytes to be cleared
......................................................................

Patch Set 3: Code-Review+2

[Edward O'Callaghan (Code Review)]

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

Change subject: spi: Allow cached ID bytes to be cleared
......................................................................

spi: Allow cached ID bytes to be cleared

This does two things:
1. Removes the static variables from various ID functions and moves
   them to a single struct.
2. Adds a function to clear the struct.

The idea of the original caching mechanism introduced years ago was
simply to speed up probe_flash() by not issuing the same read ID
commands dozens or hundreds of times for a single chip.

That implementation assumed that we would only call probe_flash()
once. However, there are cases when we want to call probe_flash()
multiple times, for example, if using an external programmer and
using different voltages (for probing the chip).

This patch is extremely similar to the original works of:
 `commit 57b7524b1448189d3630d6c4735e60dbbdf14d51`.
 Author: David Hendricks 
&&
 `commit 7f7c711ee1a41649607f81f1533e5135fc1361fc`.
 Author: David Hendricks

BUG=b:15656443
BRANCH=none
TEST=none

Change-Id: I879cb08dbe66db9ab0c3b8a7f93b04fe1c5980f4
Signed-off-by: Edward O'Callaghan 
Reviewed-on: https://review.coreboot.org/c/flashrom/+/41399
Tested-by: build bot (Jenkins) 
Reviewed-by: Stefan Reinauer 
---
M spi.h
M spi25.c
2 files changed, 56 insertions(+), 34 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Stefan Reinauer: Looks good to me, approved

diff --git a/spi.h b/spi.h
index 3f45038..c1ca46a 100644
--- a/spi.h
+++ b/spi.h
@@ -192,4 +192,6 @@
 #define SPI_FLASHROM_BUG	-5
 #define SPI_PROGRAMMER_ERROR	-6

+void clear_spi_id_cache(void);
+
 #endif		/* !__SPI_H__ */
diff --git a/spi25.c b/spi25.c
index fb91903..a044e02 100644
--- a/spi25.c
+++ b/spi25.c
@@ -27,6 +27,26 @@
 #include "programmer.h"
 #include "spi.h"

+enum id_type {
+	RDID,
+	RDID4,
+	REMS,
+	RES2,
+	RES3,
+	NUM_ID_TYPES,
+};
+
+static struct {
+	int is_cached;
+	unsigned char bytes[4];		/* enough to hold largest ID type */
+} id_cache[NUM_ID_TYPES];
+
+void clear_spi_id_cache(void)
+{
+	memset(id_cache, 0, sizeof(id_cache));
+	return;
+}
+
 static int spi_rdid(struct flashctx *flash, unsigned char *readarr, int bytes)
 {
    static const unsigned char cmd[JEDEC_RDID_OUTSIZE] = { JEDEC_RDID };
@@ -117,10 +137,11 @@
    }
 }

-static int compare_id(const struct flashchip *chip, uint32_t id1, uint32_t id2)
+static int compare_id(const struct flashctx *flash, uint32_t id1, uint32_t id2)
 {
-	msg_cdbg("%s: id1 0x%02x, id2 0x%02x\n", __func__, id1, id2);
+	const struct flashchip *chip = flash->chip;

+	msg_cdbg("%s: id1 0x%02x, id2 0x%02x\n", __func__, id1, id2);
    if (id1 == chip->manufacture_id && id2 == chip->model_id)
    	return 1;

@@ -137,20 +158,20 @@

static int probe_spi_rdid_generic(struct flashctx *flash, int bytes)
 {
-	const struct flashchip *chip = flash->chip;
-	unsigned char readarr[4];
-	uint32_t id1;
-	uint32_t id2;
+	uint32_t id1, id2;
+	enum id_type idty = bytes == 3 ? RDID : RDID4;

-	const int ret = spi_rdid(flash, readarr, bytes);
-	if (ret == SPI_INVALID_LENGTH)
-		msg_cinfo("%d byte RDID not supported on this SPI controller\n", bytes);
-	if (ret)
-		return 0;
+	if (!id_cache[idty].is_cached) {
+		const int ret = spi_rdid(flash, id_cache[idty].bytes, bytes);
+		if (ret == SPI_INVALID_LENGTH)
+			msg_cinfo("%d byte RDID not supported on this SPI controller\n", bytes);
+		if (ret)
+			return 0;
+		id_cache[idty].is_cached = 1;
+	}

-	rdid_get_ids(readarr, bytes, &id1, &id2);
-
-	return compare_id(chip, id1, id2);
+	rdid_get_ids(id_cache[idty].bytes, bytes, &id1, &id2);
+	return compare_id(flash, id1, id2);
 }

int probe_spi_rdid(struct flashctx *flash)
@@ -165,18 +186,17 @@

int probe_spi_rems(struct flashctx *flash)
 {
-	const struct flashchip *chip = flash->chip;
-	unsigned char readarr[JEDEC_REMS_INSIZE];
    uint32_t id1, id2;

-	if (spi_rems(flash, readarr)) {
-		return 0;
+	if (!id_cache[REMS].is_cached) {
+		if (spi_rems(flash, id_cache[REMS].bytes))
+			return 0;
+		id_cache[REMS].is_cached = 1;
    }

-	id1 = readarr[0];
-	id2 = readarr[1];
-
-	return compare_id(chip, id1, id2);
+	id1 = id_cache[REMS].bytes[0];
+	id2 = id_cache[REMS].bytes[1];
+	return compare_id(flash, id1, id2);
 }

int probe_spi_res1(struct flashctx *flash)
@@ -222,16 +242,16 @@

int probe_spi_res2(struct flashctx *flash)
 {
-	unsigned char readarr[2];
    uint32_t id1, id2;

-	if (spi_res(flash, readarr, 2)) {
-		return 0;
+	if (!id_cache[RES2].is_cached) {
+		if (spi_res(flash, id_cache[RES2].bytes, 2))
+			return 0;
+		id_cache[RES2].is_cached = 1;
    }

-	id1 = readarr[0];
-	id2 = readarr[1];
-
+	id1 = id_cache[RES2].bytes[0];
+	id2 = id_cache[RES2].bytes[1];
    msg_cdbg("%s: id1 0x%x, id2 0x%x\n", __func__, id1, id2);

if (id1 != flash->chip->manufacture_id || id2 != flash->chip->model_id)
@@ -242,16 +262,16 @@

int probe_spi_res3(struct flashctx *flash)
 {
-	unsigned char readarr[3];
    uint32_t id1, id2;

-	if (spi_res(flash, readarr, 3)) {
-		return 0;
+	if (!id_cache[RES3].is_cached) {
+		if (spi_res(flash, id_cache[RES3].bytes, 3))
+			return 0;
+		id_cache[RES3].is_cached = 1;
    }

-	id1 = (readarr[0] << 8) | readarr[1];
-	id2 = readarr[2];
-
+	id1 = (id_cache[RES3].bytes[0] << 8) | id_cache[RES3].bytes[1];
+	id2 = id_cache[RES3].bytes[3];
    msg_cdbg("%s: id1 0x%x, id2 0x%x\n", __func__, id1, id2);

if (id1 != flash->chip->manufacture_id || id2 != flash->chip->model_id)