Attention is currently required from: Anastasia Klimchuk.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59291 )
Change subject: flashrom: Convert do_read() into a libflashrom user
......................................................................
Patch Set 4:
This change is ready for review.
--
To view, visit https://review.coreboot.org/c/flashrom/+/59291
To unsubscribe, or for help writing mail filters, visit https://review.…
[View More]coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I4b690b688acf9d5deb46e8642a252a2132ea8c73
Gerrit-Change-Number: 59291
Gerrit-PatchSet: 4
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Mon, 29 Nov 2021 02:45:57 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
[View Less]
Attention is currently required from: Edward O'Callaghan, Daniel Campello, Anastasia Klimchuk.
Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59532 )
Change subject: [FIX ME] tests: Add test for extract operation
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS1:
> Daniel, Edward I added you because I found your names in commit ce983bccaab450d358854494f15c2d8a1846 […]
Failing …
[View More]looks like correct behavior to me, since `read_flash_to_file` will never have a meaningful result beyond the side-effect of writing to a file; failing to tell it what file you want is an error.
The confusion seems to have come from the Chromium fork of flashrom checking if the filename is NULL in `read_flash_to_file` and then skipping `write_buf_to_file` if no filename is given. That behavior seems wrong because a user should simply not call `read_flash_to_file` if they don't have a file, otherwise it would be doing the work to read the flash then throwing away the results.
--
To view, visit https://review.coreboot.org/c/flashrom/+/59532
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I13107c0e095d53184e32a41fa72227cf7dc1d449
Gerrit-Change-Number: 59532
Gerrit-PatchSet: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Mon, 29 Nov 2021 02:17:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
[View Less]
Sergii Dmytruk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/59711 )
Change subject: [RFC][OTP] otp: support 2-bit locks
......................................................................
[RFC][OTP] otp: support 2-bit locks
Change-Id: I0593724901a0e354f1016405577f60ac82d4b14c
Signed-off-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
---
M otp.c
M otp.h
2 files changed, 24 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:…
[View More]29418/flashrom refs/changes/11/59711/1
diff --git a/otp.c b/otp.c
index f340244..1db5639 100644
--- a/otp.c
+++ b/otp.c
@@ -33,11 +33,6 @@
* GD25Q80(B), GD25Q128B
*/
-/*
- * TODO: some chips have a separate Security Register which must be modified
- * with special opcodes (see Adesto AT25SF641 chip for example).
- */
-
static bool bp_is_stashed;
static uint8_t stashed_bp_value;
@@ -244,12 +239,26 @@
return 1;
}
- uint8_t status;
int result = 0;
- if (read_reg_bit(flash, otp->regions[region].user_lock, &status)) {
- msg_cerr("Error: Failed to query status of OTP region %d.\n",
- region + 1);
+ *locked = false;
+
+ uint8_t user_bit = 0;
+ if (read_reg_bit(flash, otp->regions[region].user_lock, &user_bit)) {
+ msg_cerr("Error: Failed to query user lock of OTP region %d.\n", region + 1);
result = 1;
+ } else {
+ *locked = user_bit;
+ }
+
+ const struct reg_bit_info factory_lock = otp->regions[region].factory_lock;
+ if (!*locked && factory_lock.reg != INVALID_REG) {
+ uint8_t factory_bit = 0;
+ if (read_reg_bit(flash, factory_lock, &factory_bit)) {
+ msg_cerr("Error: Failed to query factory lock of OTP region %d.\n", region + 1);
+ result = 1;
+ } else {
+ *locked = factory_bit;
+ }
}
if (exit_otp_mode(flash)) {
@@ -257,7 +266,6 @@
return 1;
}
- *locked = status;
return result;
}
diff --git a/otp.h b/otp.h
index 9a1a53b..80a1e31 100644
--- a/otp.h
+++ b/otp.h
@@ -54,12 +54,17 @@
uint32_t size; /* in bytes */
/*
+ * Lock-down bits. Setting at least one of them makes OTP
+ * regions read-only. Once set, they can't be unset.
+ *
* All chips have at least user lock bit, which is mandatory for
* OTP configuration to count as valid. User bit might actually
* be set by a manufacturer after writing ESN. Some chips have
- * a designated factory lock-down bit instead.
+ * a designated factory lock-down bit instead and defining it here is
+ * optional.
*/
struct reg_bit_info user_lock;
+ struct reg_bit_info factory_lock;
} regions[FLASHROM_OTP_MAX_REGIONS + 1]; /* We need one more than maximum */
};
--
To view, visit https://review.coreboot.org/c/flashrom/+/59711
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I0593724901a0e354f1016405577f60ac82d4b14c
Gerrit-Change-Number: 59711
Gerrit-PatchSet: 1
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-MessageType: newchange
[View Less]
Sergii Dmytruk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/59709 )
Change subject: [RFC][OTP] spi25_statusreg: support reading/writing security register
......................................................................
[RFC][OTP] spi25_statusreg: support reading/writing security register
Not to be confused with "secure registers" of OTP.
Security register is a dedicated status register for security-related
bits. You don't write its value directly, …
[View More]issuing corresponding write
command with no data just sets OTP bit to 1 automatically.
No WREN is necessary, but at least some datasheets indicate BUSY state
after write command.
Unlike cases where OTP bit is part of SR and can only be written while
in OTP mode, security register can only be written outside of the mode.
Change-Id: Iae1753ca4cb051127a5bcbeba7f064053adb8dae
Signed-off-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
---
M flash.h
M spi.h
M spi25_statusreg.c
3 files changed, 54 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/09/59709/1
diff --git a/flash.h b/flash.h
index a277e43..1074d09 100644
--- a/flash.h
+++ b/flash.h
@@ -144,6 +144,8 @@
#define FEATURE_WRSR2 (1 << 20)
#define FEATURE_RDSR2 (1 << 21)
+/* Whether chip has security register (RDSCUR/WRSCUR commands) */
+#define FEATURE_SCUR (1 << 22)
#define ERASED_VALUE(flash) (((flash)->chip->feature_bits & FEATURE_ERASED_ZERO) ? 0x00 : 0xff)
@@ -177,8 +179,9 @@
STATUS1,
STATUS2,
CONFIG1,
+ SECREG, /* security register */
};
-#define MAX_REGISTERS 4
+#define MAX_REGISTERS 5
struct reg_bit_info {
/* Register containing the bit */
diff --git a/spi.h b/spi.h
index 36d83f5..07d131d 100644
--- a/spi.h
+++ b/spi.h
@@ -158,6 +158,16 @@
#define JEDEC_WRSR2_OUTSIZE 0x02
#define JEDEC_WRSR2_INSIZE 0x00
+/* Read Security Register */
+#define JEDEC_RDSCUR 0x2b
+#define JEDEC_RDSCUR_OUTSIZE 0x01
+#define JEDEC_RDSCUR_INSIZE 0x01
+
+/* Write Security Register */
+#define JEDEC_WRSCUR 0x2f
+#define JEDEC_WRSCUR_OUTSIZE 0x01
+#define JEDEC_WRSCUR_INSIZE 0x00
+
/* Enter 4-byte Address Mode */
#define JEDEC_ENTER_4_BYTE_ADDR_MODE 0xB7
diff --git a/spi25_statusreg.c b/spi25_statusreg.c
index f549d10..6ca2117 100644
--- a/spi25_statusreg.c
+++ b/spi25_statusreg.c
@@ -22,6 +22,25 @@
#include "spi.h"
/* === Generic functions === */
+static int spi_write_register_wait(const struct flashctx *flash)
+{
+ /* 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.
+ */
+ int i = 0;
+ programmer_delay(100 * 1000);
+ while (spi_read_status_register(flash) & SPI_SR_WIP) {
+ if (++i > 490) {
+ msg_cerr("Error: WIP bit after WRSR never cleared\n");
+ return TIMEOUT_ERROR;
+ }
+ programmer_delay(10 * 1000);
+ }
+ return 0;
+}
+
static int spi_write_register_flag(const struct flashctx *flash, uint8_t enable_opcode, uint8_t *write_cmd, size_t write_cmd_len)
{
/*
@@ -56,21 +75,8 @@
*/
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.
- */
- int i = 0;
- programmer_delay(100 * 1000);
- while (spi_read_status_register(flash) & SPI_SR_WIP) {
- if (++i > 490) {
- msg_cerr("Error: WIP bit after WRSR never cleared\n");
- return TIMEOUT_ERROR;
- }
- programmer_delay(10 * 1000);
- }
- return 0;
+
+ return spi_write_register_wait(flash);
}
static const char *reg_name(enum flash_reg reg)
@@ -79,6 +85,7 @@
case STATUS1: return "SR1";
case STATUS2: return "SR2";
case CONFIG1: return "CR1";
+ case SECREG: return "SCR";
default: return "Unknown register";
}
}
@@ -116,12 +123,28 @@
write_cmd[1] = value;
write_cmd_len = JEDEC_WRSR2_OUTSIZE;
}
+ } else if (reg == SECREG) {
+ if (feature_bits & FEATURE_SCUR) {
+ /* This command accepts no value, it just sets OTP
+ * bit. */
+ write_cmd[0] = JEDEC_WRSCUR;
+ write_cmd_len = JEDEC_WRSCUR_OUTSIZE;
+ }
}
if (write_cmd_len == 0) {
msg_cerr("Chip does not support writing register %s.\n", reg_name(reg));
return 1;
}
+ /* No WREN is necessary for writing security register (at least for
+ * Macronix or Adesto), but might need to wait for WIP == 0. */
+ if (reg == SECREG) {
+ int ret = spi_send_command(flash, write_cmd_len, 0, write_cmd, NULL);
+ if (ret)
+ return ret;
+ return spi_write_register_wait(flash);
+ }
+
if (!(feature_bits & (FEATURE_WRSR_WREN | FEATURE_WRSR_EWSR))) {
msg_cdbg("Missing status register write definition, assuming "
"EWSR is needed\n");
@@ -145,6 +168,8 @@
read_cmd = JEDEC_RDSR;
} else if (reg == STATUS2 && (feature_bits & FEATURE_RDSR2)) {
read_cmd = JEDEC_RDSR2;
+ } else if (reg == SECREG && (feature_bits & FEATURE_SCUR)) {
+ read_cmd = JEDEC_RDSCUR;
} else {
msg_cerr("Chip does not support reading register %s.\n", reg_name(reg));
return 1;
--
To view, visit https://review.coreboot.org/c/flashrom/+/59709
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Iae1753ca4cb051127a5bcbeba7f064053adb8dae
Gerrit-Change-Number: 59709
Gerrit-PatchSet: 1
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-MessageType: newchange
[View Less]