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 quasisec@google.com --- 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).
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
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
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.
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 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.
-
Angel Pons (Code Review) -
Edward O'Callaghan (Code Review)
-
Sam McNally (Code Review)