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 <[e-mail]>
&&
 `commit 7f7c711ee1a41649607f81f1533e5135fc1361fc`.
 Author: David Hendricks <[e-mail]>

BUG=b:15656443
BRANCH=none
TEST=none

Change-Id: I879cb08dbe66db9ab0c3b8a7f93b04fe1c5980f4
Signed-off-by: Edward O'Callaghan <[e-mail]>
---
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)

-- 
To view, visit https://review.coreboot.org/c/flashrom/+/41399
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I879cb08dbe66db9ab0c3b8a7f93b04fe1c5980f4
Gerrit-Change-Number: 41399
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <[e-mail]>
Gerrit-MessageType: newchange

[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 <[e-mail]>
&&
 `commit 7f7c711ee1a41649607f81f1533e5135fc1361fc`.
 Author: David Hendricks <[e-mail]>

BUG=b:15656443
BRANCH=none
TEST=none

Change-Id: I879cb08dbe66db9ab0c3b8a7f93b04fe1c5980f4
Signed-off-by: Edward O'Callaghan <[e-mail]>
---
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
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/41399
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I879cb08dbe66db9ab0c3b8a7f93b04fe1c5980f4
Gerrit-Change-Number: 41399
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-MessageType: newpatchset

[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 <[e-mail]>
&&
 `commit 7f7c711ee1a41649607f81f1533e5135fc1361fc`.
 Author: David Hendricks <[e-mail]>

BUG=b:15656443
BRANCH=none
TEST=none

Change-Id: I879cb08dbe66db9ab0c3b8a7f93b04fe1c5980f4
Signed-off-by: Edward O'Callaghan <[e-mail]>
---
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
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/41399
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I879cb08dbe66db9ab0c3b8a7f93b04fe1c5980f4
Gerrit-Change-Number: 41399
Gerrit-PatchSet: 3
Gerrit-Owner: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-MessageType: newpatchset

[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


-- 
To view, visit https://review.coreboot.org/c/flashrom/+/41399
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I879cb08dbe66db9ab0c3b8a7f93b04fe1c5980f4
Gerrit-Change-Number: 41399
Gerrit-PatchSet: 3
Gerrit-Owner: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: Stefan Reinauer <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Wed, 26 Aug 2020 04:43:56 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[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 <[e-mail]>
&&
 `commit 7f7c711ee1a41649607f81f1533e5135fc1361fc`.
 Author: David Hendricks <[e-mail]>

BUG=b:15656443
BRANCH=none
TEST=none

Change-Id: I879cb08dbe66db9ab0c3b8a7f93b04fe1c5980f4
Signed-off-by: Edward O'Callaghan <[e-mail]>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/41399
Tested-by: build bot (Jenkins) <[e-mail]>
Reviewed-by: Stefan Reinauer <[e-mail]>
---
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)

-- 
To view, visit https://review.coreboot.org/c/flashrom/+/41399
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I879cb08dbe66db9ab0c3b8a7f93b04fe1c5980f4
Gerrit-Change-Number: 41399
Gerrit-PatchSet: 5
Gerrit-Owner: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Stefan Reinauer <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-MessageType: merged