Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/59528 )
Change subject: spi25_statusreg: inline spi_write_register_flag()
......................................................................
spi25_statusreg: inline spi_write_register_flag()
Creating the entire SPI command that should be sent to the chip in
spi_write_register() is simpler than splitting it across two functions
that have to pass multiple parameters between them.
Additionally, having separate spi_write_register_flag() function
provided little benefit, as it was only ever called from
spi_write_register().
BUG=b:195381327,b:153800563
BRANCH=none
TEST=flashrom -{r,w,E}
TEST=Tested with a W25Q128.W flash on a kasumi (AMD) dut.
Read SR1/SR2 with --wp-status and activated various WP ranges
that toggled bits in both SR1 and SR2.
Change-Id: I4996b0848d0ed09032bad2ab13ab1f40bbfc0304
Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/59528
Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org>
Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
---
M spi25_statusreg.c
1 file changed, 56 insertions(+), 70 deletions(-)
Approvals:
build bot (Jenkins): Verified
Edward O'Callaghan: Looks good to me, approved
Anastasia Klimchuk: Looks good to me, approved
diff --git a/spi25_statusreg.c b/spi25_statusreg.c
index 31d6c76..249ab9a 100644
--- a/spi25_statusreg.c
+++ b/spi25_statusreg.c
@@ -22,68 +22,6 @@
#include "spi.h"
/* === Generic functions === */
-static int spi_write_register_flag(const struct flashctx *flash, uint8_t enable_opcode, uint8_t *write_cmd, size_t write_cmd_len, enum flash_reg reg)
-{
- /*
- * Enabling register writes requires either EWSR or WREN depending on
- * chip type. The code below relies on the fact hat EWSR and WREN have
- * the same INSIZE and OUTSIZE.
- */
-
- struct spi_command cmds[] = {
- {
- .writecnt = JEDEC_WREN_OUTSIZE,
- .writearr = &enable_opcode,
- .readcnt = 0,
- .readarr = NULL,
- }, {
- .writecnt = write_cmd_len,
- .writearr = write_cmd,
- .readcnt = 0,
- .readarr = NULL,
- }, {
- .writecnt = 0,
- .writearr = NULL,
- .readcnt = 0,
- .readarr = NULL,
- }};
-
- int result = spi_send_multicommand(flash, cmds);
- if (result) {
- msg_cerr("%s failed during command execution\n", __func__);
- /*
- * No point in waiting for the command to complete if execution
- * failed.
- */
- return result;
- }
-
- /*
- * WRSR performs a self-timed erase before the changes take effect.
- * This may take 50-85 ms in most cases, and some chips apparently
- * allow running RDSR only once. Therefore pick an initial delay of
- * 100 ms, then wait in 10 ms steps until a total of 5 s have elapsed.
- *
- * Newer chips with multiple status registers (SR2 etc.) are unlikely
- * to have problems with multiple RDSR commands, so only wait for the
- * initial 100 ms if the register we wrote to was SR1.
- */
- int delay_ms = 5000;
- if (reg == STATUS1) {
- programmer_delay(100 * 1000);
- delay_ms -= 100;
- }
-
- for (; delay_ms > 0; delay_ms -= 10) {
- if ((spi_read_status_register(flash) & SPI_SR_WIP) == 0)
- return 0;
- programmer_delay(10 * 1000);
- }
-
- msg_cerr("Error: WIP bit after WRSR never cleared\n");
- return TIMEOUT_ERROR;
-}
-
int spi_write_register(const struct flashctx *flash, enum flash_reg reg, uint8_t value)
{
int feature_bits = flash->chip->feature_bits;
@@ -133,18 +71,66 @@
return 1;
}
- if (!(feature_bits & (FEATURE_WRSR_WREN | FEATURE_WRSR_EWSR))) {
+ uint8_t enable_cmd;
+ if (feature_bits & FEATURE_WRSR_WREN) {
+ enable_cmd = JEDEC_WREN;
+ } else if (feature_bits & FEATURE_WRSR_EWSR) {
+ enable_cmd = JEDEC_EWSR;
+ } else {
msg_cdbg("Missing status register write definition, assuming "
"EWSR is needed\n");
- feature_bits |= FEATURE_WRSR_EWSR;
+ enable_cmd = JEDEC_EWSR;
}
- int ret = 1;
- if (feature_bits & FEATURE_WRSR_WREN)
- ret = spi_write_register_flag(flash, JEDEC_WREN, write_cmd, write_cmd_len, reg);
- if (ret && (feature_bits & FEATURE_WRSR_EWSR))
- ret = spi_write_register_flag(flash, JEDEC_EWSR, write_cmd, write_cmd_len, reg);
- return ret;
+ struct spi_command cmds[] = {
+ {
+ .writecnt = JEDEC_WREN_OUTSIZE,
+ .writearr = &enable_cmd,
+ .readcnt = 0,
+ .readarr = NULL,
+ }, {
+ .writecnt = write_cmd_len,
+ .writearr = write_cmd,
+ .readcnt = 0,
+ .readarr = NULL,
+ }, {
+ .writecnt = 0,
+ .writearr = NULL,
+ .readcnt = 0,
+ .readarr = NULL,
+ }};
+
+ int result = spi_send_multicommand(flash, cmds);
+ if (result) {
+ msg_cerr("%s failed during command execution\n", __func__);
+ return result;
+ }
+
+ /*
+ * WRSR performs a self-timed erase before the changes take effect.
+ * This may take 50-85 ms in most cases, and some chips apparently
+ * allow running RDSR only once. Therefore pick an initial delay of
+ * 100 ms, then wait in 10 ms steps until a total of 5 s have elapsed.
+ *
+ * Newer chips with multiple status registers (SR2 etc.) are unlikely
+ * to have problems with multiple RDSR commands, so only wait for the
+ * initial 100 ms if the register we wrote to was SR1.
+ */
+ int delay_ms = 5000;
+ if (reg == STATUS1) {
+ programmer_delay(100 * 1000);
+ delay_ms -= 100;
+ }
+
+ for (; delay_ms > 0; delay_ms -= 10) {
+ if ((spi_read_status_register(flash) & SPI_SR_WIP) == 0)
+ return 0;
+ programmer_delay(10 * 1000);
+ }
+
+
+ msg_cerr("Error: WIP bit after WRSR never cleared\n");
+ return TIMEOUT_ERROR;
}
int spi_read_register(const struct flashctx *flash, enum flash_reg reg, uint8_t *value)
--
To view, visit https://review.coreboot.org/c/flashrom/+/59528
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I4996b0848d0ed09032bad2ab13ab1f40bbfc0304
Gerrit-Change-Number: 59528
Gerrit-PatchSet: 64
Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-MessageType: merged
Attention is currently required from: Nico Huber, Nikolai Artemiev, Edward O'Callaghan, Sergii Dmytruk.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60231 )
Change subject: writeprotect: add WPS bit and always set it to zero
......................................................................
Patch Set 9: Code-Review+2
(1 comment)
Patchset:
PS9:
> I have meta-question, please help me understand where the new WPS bit is used? This patch is adding […]
I found immediate answer to my question in the comment thread in the next patch :)
However I always like to hear about longer plans, if you have any I am interested!
--
To view, visit https://review.coreboot.org/c/flashrom/+/60231
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2c26ec65d64a3b6fb1f1a73690bc771415db2744
Gerrit-Change-Number: 60231
Gerrit-PatchSet: 9
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)chromium.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Tue, 05 Apr 2022 00:29:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Nikolai Artemiev, Edward O'Callaghan, Sergii Dmytruk.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60231 )
Change subject: writeprotect: add WPS bit and always set it to zero
......................................................................
Patch Set 9: Code-Review+1
(1 comment)
Patchset:
PS9:
I have meta-question, please help me understand where the new WPS bit is used? This patch is adding it, but it is always zero, so it's not used yet. Commit message saying "proper support requires extension of the API as well as implementation", is this a plan, or is it happening later in the chain? Thanks!
Otherwise, looks good.
--
To view, visit https://review.coreboot.org/c/flashrom/+/60231
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2c26ec65d64a3b6fb1f1a73690bc771415db2744
Gerrit-Change-Number: 60231
Gerrit-PatchSet: 9
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)chromium.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Tue, 05 Apr 2022 00:24:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Sergii Dmytruk.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60230 )
Change subject: spi25_statusreg.c: add SR3 read/write support
......................................................................
Patch Set 9: Code-Review+2
(1 comment)
File flash.h:
https://review.coreboot.org/c/flashrom/+/60230/comment/36fdd93b_659c6cc9
PS2, Line 148: FEATURE_SR3
Thank for explaining, I understand why there is one flag.
> Single flag or no flag at all?
Single flag, I am thinking: the code would look consistent with SR2 code (as Sergii mentioned). The cases in spi_write_register and spi_read_register would look similar between status2 and 3, I think it's good?
What do you think Nico? (since you asked about no flag at all?)
--
To view, visit https://review.coreboot.org/c/flashrom/+/60230
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id987c544c02da2b956e6ad2c525265cac8f15be1
Gerrit-Change-Number: 60230
Gerrit-PatchSet: 9
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Tue, 05 Apr 2022 00:01:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-MessageType: comment