Change in flashrom[master]: spi25: Add 'spi_read_status_register_2()' helper

[Edward O'Callaghan (Code Review)]

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


Change subject: spi25: Add 'spi_read_status_register_2()' helper
......................................................................

spi25: Add 'spi_read_status_register_2()' helper

Winbond chips have a WRSR opcode of 0x35 interestingly.
Add helper to allow for reading it. This is later used
by writeprotection support.

BUG=b:153800563
BRANCH=none
TEST=builds

Change-Id: I2d25f22ced17ab94d122e86b270357723b45458e
Signed-off-by: Edward O'Callaghan <[e-mail]>
---
M chipdrivers.h
M spi25_statusreg.c
2 files changed, 13 insertions(+), 2 deletions(-)



  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/48/45748/1

diff --git a/chipdrivers.h b/chipdrivers.h
index cf03811..4e76588 100644
--- a/chipdrivers.h
+++ b/chipdrivers.h
@@ -63,6 +63,7 @@
 
 /* spi25_statusreg.c */
 uint8_t spi_read_status_register(const struct flashctx *flash);
+uint8_t spi_read_status_register_2(const struct flashctx *flash);
 int spi_write_status_register(const struct flashctx *flash, int status);
 void spi_prettyprint_status_register_bit(uint8_t status, int bit);
 int spi_prettyprint_status_register_plain(struct flashctx *flash);
diff --git a/spi25_statusreg.c b/spi25_statusreg.c
index 34f9ad4..cad92e6 100644
--- a/spi25_statusreg.c
+++ b/spi25_statusreg.c
@@ -90,9 +90,9 @@
 	return ret;
 }
 
-uint8_t spi_read_status_register(const struct flashctx *flash)
+static uint8_t spi_read_status_register_generic(const struct flashctx *flash, uint8_t opcode)
 {
-	static const unsigned char cmd[JEDEC_RDSR_OUTSIZE] = { JEDEC_RDSR };
+	const unsigned char cmd[JEDEC_RDSR_OUTSIZE] = { opcode };
 	/* FIXME: No workarounds for driver/hardware bugs in generic code. */
 	unsigned char readarr[2]; /* JEDEC_RDSR_INSIZE=1 but wbsio needs 2 */
 	int ret;
@@ -108,6 +108,16 @@
 	return readarr[0];
 }
 
+uint8_t spi_read_status_register(const struct flashctx *flash)
+{
+	return spi_read_status_register_generic(flash, JEDEC_RDSR);
+}
+
+uint8_t spi_read_status_register_2(const struct flashctx *flash)
+{
+	return spi_read_status_register_generic(flash, 0x35); /* Unclear if this is a std JEDEC opcode. */
+}
+
 /* A generic block protection disable.
  * Tests if a protection is enabled with the block protection mask (bp_mask) and returns success otherwise.
  * Tests if the register bits are locked with the lock_mask (lock_mask).

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

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

[Sam McNally (Code Review)]

Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/45748 )

Change subject: spi25: Add 'spi_read_status_register_2()' helper
......................................................................


Patch Set 2: Code-Review+2


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

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2d25f22ced17ab94d122e86b270357723b45458e
Gerrit-Change-Number: 45748
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Patrick Georgi <[e-mail]>
Gerrit-Reviewer: Sam McNally <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Mon, 28 Sep 2020 05:48:56 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/45748 )

Change subject: spi25: Add 'spi_read_status_register_2()' helper
......................................................................


Patch Set 3: Code-Review+1

(1 comment)

https://review.coreboot.org/c/flashrom/+/45748/3/spi25_statusreg.c 
File spi25_statusreg.c:

https://review.coreboot.org/c/flashrom/+/45748/3/spi25_statusreg.c@118 
PS3, Line 118: 	return spi_read_status_register_generic(flash, 0x35); /* Unclear if this is a std JEDEC opcode. */
Looks like not, as other vendors use it to enter QPI:

Macronix: https://www.macronix.com/Lists/ApplicationNote/Attachments/1899/AN0251V1%20-...
ISSI: http://www.issi.com/WW/pdf/25WP064A.pdf

Others use it differently:

Cypress: https://docs.rs-online.com/e0e8/A700000006483477.pdf

Others use it for some status register:

SST: https://www.tme.eu/Document/84d3bb0d3f6d37c285de97bf579cab66/SST25VF020B.pdf
Adesto: https://www.adestotech.com/wp-content/uploads/doc8715.pdf
Zbit: https://datasheet.lcsc.com/szlcsc/2003141212_Zbit-ZB25VQ80ATIG_C495747.pdf



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

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2d25f22ced17ab94d122e86b270357723b45458e
Gerrit-Change-Number: 45748
Gerrit-PatchSet: 3
Gerrit-Owner: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Patrick Georgi <[e-mail]>
Gerrit-Reviewer: Sam McNally <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Mon, 28 Sep 2020 09:29:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/45748 )

Change subject: spi25: Add 'spi_read_status_register_2()' helper
......................................................................


Patch Set 3:

(1 comment)

https://review.coreboot.org/c/flashrom/+/45748/3/spi25_statusreg.c 
File spi25_statusreg.c:

https://review.coreboot.org/c/flashrom/+/45748/3/spi25_statusreg.c@118 
PS3, Line 118: 	return spi_read_status_register_generic(flash, 0x35); /* Unclear if this is a std JEDEC opcode. */
> Looks like not, as other vendors use it to enter QPI: […]
> That is some decent research thanks! Tricky ok.. I had a think and I have great proposals so open to suggestions? I agree the current function is less than ideal giving the situation as it could mislead a caller into perhaps something they never intended and it would be hard to debug.

We could call it 'spi_read_msr()` and just document in the header that this is intended as a sort of RDSR2 op on Winbond, SST, Adesto and Zbit however Cypress*, ISSI, Macronix and others could be exceptions.



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

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2d25f22ced17ab94d122e86b270357723b45458e
Gerrit-Change-Number: 45748
Gerrit-PatchSet: 3
Gerrit-Owner: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Patrick Georgi <[e-mail]>
Gerrit-Reviewer: Sam McNally <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Mon, 28 Sep 2020 11:51:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <[e-mail]>
Gerrit-MessageType: comment

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/45748 )

Change subject: spi25: Add 'spi_read_status_register_2()' helper
......................................................................


Patch Set 3:

(1 comment)

https://review.coreboot.org/c/flashrom/+/45748/3/spi25_statusreg.c 
File spi25_statusreg.c:

https://review.coreboot.org/c/flashrom/+/45748/3/spi25_statusreg.c@118 
PS3, Line 118: 	return spi_read_status_register_generic(flash, 0x35); /* Unclear if this is a std JEDEC opcode. */
> That is some decent research thanks! Tricky ok.. […]
> I'd prefer if there was an abstraction somewhere to cleanly map high-level operations (e.g. handle write protect stuff) to lower-level stuff (SPI commands). There's some sort of layering in flashrom, but it seems to have degraded over time.

[Edward O'Callaghan (Code Review)]

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

Change subject: spi25: Add 'spi_read_status_register_2()' helper
......................................................................


Abandoned

in fav of Nick's upstream work.
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/45748
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2d25f22ced17ab94d122e86b270357723b45458e
Gerrit-Change-Number: 45748
Gerrit-PatchSet: 3
Gerrit-Owner: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Sam McNally <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-MessageType: abandon