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

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). -- 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 <quasisec@chromium.org> Gerrit-MessageType: newchange

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 <quasisec@chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Sam McNally <sammc@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Mon, 28 Sep 2020 05:48:56 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

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 <quasisec@chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Sam McNally <sammc@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Mon, 28 Sep 2020 09:29:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

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 <quasisec@chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Sam McNally <sammc@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Mon, 28 Sep 2020 11:51:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Gerrit-MessageType: comment

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.
-- 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 <quasisec@chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Sam McNally <sammc@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Mon, 28 Sep 2020 18:54:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec@chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Gerrit-MessageType: comment

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 <quasisec@chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Sam McNally <sammc@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-MessageType: abandon
participants (3)
-
Angel Pons (Code Review)
-
Edward O'Callaghan (Code Review)
-
Sam McNally (Code Review)