April 2022 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: tests: assert pathname and flags when calling open()
by Anastasia Klimchuk (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: Sam McNally, Daniel Campello. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63227 ) Change subject: tests: assert pathname and flags when calling open() ...................................................................... Patch Set 7: (10 comments) Patchset: PS7: Thanks! File tests/io_mock.h: https://review.coreboot.org/c/flashrom/+/63227/comment/31c81a69_f2ccfdf0 PS7, Line 63: /* Maximum number of open to mock. */ The number is probably arbitrary? If yes, you can add this clarification into a comment, "the number is arbitrary". This way, if in future we need more calls, it will be easier to increase the number (no need to search history for the reasons why the number is this). https://review.coreboot.org/c/flashrom/+/63227/comment/4466f4c5_bc827d07 PS7, Line 64: 3 If you are in a good mood, let's make this number power of two :) like 4 for example. https://review.coreboot.org/c/flashrom/+/63227/comment/8fe149e6_7e375ab7 PS7, Line 118: open_state Another thing: could you please add a comment above this line, to explain what that is, something like "An alternative to custom open mock, a test can register one of: either its own mock open function or fallback_open". https://review.coreboot.org/c/flashrom/+/63227/comment/52561be1_1ce4126d PS7, Line 118: struct io_mock_open_state *open_state; This is, as I understand from the code, an alternative to custom open mocking, and mutually exclusive (a test can either mock open function or setup open state). Let's have more descriptive naming for this. For example: struct fallback_open_state *fallback_open I remember we agreed with Edward on "fallback" name in some earlier patch, let's use this name. File tests/io_mock.c: https://review.coreboot.org/c/flashrom/+/63227/comment/bf96023f_0039c312 PS7, Line 24: assert_true(io == NULL || io->open == NULL || io->open_state == NULL); I would also add a comment above this line, to make it crystal clear: "A test can register only one of: either custom mock for open function, or fallback open state". File tests/tests.c: https://review.coreboot.org/c/flashrom/+/63227/comment/d90d19f8_415e9955 PS7, Line 77: wrap_open_and_friends Lets just call it "mock_open" https://review.coreboot.org/c/flashrom/+/63227/comment/5e81318a_fdf67e4f PS7, Line 81: Could you please add new line between two conditionals? It would be easier to read. https://review.coreboot.org/c/flashrom/+/63227/comment/5e1008fe_a5743e9c PS7, Line 84: Also here, please add new line between declaration of flags and asserts https://review.coreboot.org/c/flashrom/+/63227/comment/f1e7592e_4acdbec9 PS7, Line 91: Also here, please add new line before return statement. -- To view, visit https://review.coreboot.org/c/flashrom/+/63227 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib46ca5b854c8453ec02ae09f3151cd4d25f988eb Gerrit-Change-Number: 63227 Gerrit-PatchSet: 7 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Comment-Date: Tue, 05 Apr 2022 03:58:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: spi25_statusreg: delete spi_read_status_register()
by Edward O'Callaghan (Code Review) 05 Apr '22

05 Apr '22

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/59529 ) Change subject: spi25_statusreg: delete spi_read_status_register() ...................................................................... spi25_statusreg: delete spi_read_status_register() Delete the spi_read_status_register() function because the generic spi_read_register() function can be used instead. This patch also converts all call sites over to spi_read_register(). A side effect is that error codes are now properly propagated and checked. 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: I146b4b5439872e66c5d33e156451a729d248c7da Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/59529 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M chipdrivers.h M it87spi.c M s25f.c M spi25.c M spi25_statusreg.c 5 files changed, 141 insertions(+), 46 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved Anastasia Klimchuk: Looks good to me, approved diff --git a/chipdrivers.h b/chipdrivers.h index c223534..0695993 100644 --- a/chipdrivers.h +++ b/chipdrivers.h @@ -62,8 +62,6 @@ /* spi25_statusreg.c */ -/* FIXME: replace spi_read_status_register() calls with spi_read_register() */ -uint8_t spi_read_status_register(const struct flashctx *flash); int spi_read_register(const struct flashctx *flash, enum flash_reg reg, uint8_t *value); int spi_write_register(const struct flashctx *flash, enum flash_reg reg, uint8_t value); void spi_prettyprint_status_register_bit(uint8_t status, int bit); diff --git a/it87spi.c b/it87spi.c index 728ec70..204816a 100644 --- a/it87spi.c +++ b/it87spi.c @@ -133,9 +133,20 @@ OUTB(0, data->flashport); /* Wait until the Write-In-Progress bit is cleared. * This usually takes 1-10 ms, so wait in 1 ms steps. + * + * FIXME: This should timeout after some number of retries. */ - while (spi_read_status_register(flash) & SPI_SR_WIP) + while (true) { + uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; + + if((status & SPI_SR_WIP) == 0) + return 0; + programmer_delay(1000); + } return 0; } @@ -277,7 +288,9 @@ } while (len >= chip->page_size) { - it8716f_spi_page_program(flash, buf, start); + int ret = it8716f_spi_page_program(flash, buf, start); + if (ret) + return ret; start += chip->page_size; len -= chip->page_size; buf += chip->page_size; diff --git a/s25f.c b/s25f.c index 0bd4a7c..82ca610 100644 --- a/s25f.c +++ b/s25f.c @@ -133,9 +133,14 @@ static int s25f_poll_status(const struct flashctx *flash) { - uint8_t tmp = spi_read_status_register(flash); + while (true) { + uint8_t tmp; + if (spi_read_register(flash, STATUS1, &tmp)) + return -1; - while (tmp & SPI_SR_WIP) { + if ((tmp & SPI_SR_WIP) == 0) + break; + /* * The WIP bit on S25F chips remains set to 1 if erase or * programming errors occur, so we must check for those @@ -156,7 +161,6 @@ } programmer_delay(1000 * 10); - tmp = spi_read_status_register(flash); } return 0; diff --git a/spi25.c b/spi25.c index 213273f..e96a38d 100644 --- a/spi25.c +++ b/spi25.c @@ -304,12 +304,17 @@ static int spi_poll_wip(struct flashctx *const flash, const unsigned int poll_delay) { - /* FIXME: We can't tell if spi_read_status_register() failed. */ /* FIXME: We don't time out. */ - while (spi_read_status_register(flash) & SPI_SR_WIP) + while (true) { + uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; + if (!(status & SPI_SR_WIP)) + return 0; + programmer_delay(poll_delay); - /* FIXME: Check the status register for errors. */ - return 0; + } } /** diff --git a/spi25_statusreg.c b/spi25_statusreg.c index 249ab9a..6b16839 100644 --- a/spi25_statusreg.c +++ b/spi25_statusreg.c @@ -123,7 +123,11 @@ } for (; delay_ms > 0; delay_ms -= 10) { - if ((spi_read_status_register(flash) & SPI_SR_WIP) == 0) + uint8_t status; + result = spi_read_register(flash, STATUS1, &status); + if (result) + return result; + if ((status & SPI_SR_WIP) == 0) return 0; programmer_delay(10 * 1000); } @@ -168,14 +172,6 @@ return 0; } -uint8_t spi_read_status_register(const struct flashctx *flash) -{ - uint8_t status = 0; - /* FIXME: We should propagate the error. */ - spi_read_register(flash, STATUS1, &status); - return status; -} - static int spi_restore_status(struct flashctx *flash, uint8_t status) { msg_cdbg("restoring chip status (0x%02x)\n", status); @@ -207,7 +203,10 @@ uint8_t status; int result; - status = spi_read_status_register(flash); + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; + if ((status & bp_mask) == 0) { msg_cdbg2("Block protection is disabled.\n"); return 0; @@ -229,7 +228,11 @@ msg_cerr("Could not write status register 1.\n"); return result; } - status = spi_read_status_register(flash); + + ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; + if ((status & lock_mask) != 0) { msg_cerr("Unsetting lock bit(s) failed.\n"); return 1; @@ -242,7 +245,11 @@ msg_cerr("Could not write status register 1.\n"); return result; } - status = spi_read_status_register(flash); + + ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; + if ((status & bp_mask) != 0) { msg_cerr("Block protection could not be disabled!\n"); if (flash->chip->printlock) @@ -363,7 +370,10 @@ int spi_prettyprint_status_register_plain(struct flashctx *flash) { - uint8_t status = spi_read_status_register(flash); + uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; spi_prettyprint_status_register_hex(status); return 0; } @@ -371,7 +381,10 @@ /* Print the plain hex value and the welwip bits only. */ int spi_prettyprint_status_register_default_welwip(struct flashctx *flash) { - uint8_t status = spi_read_status_register(flash); + uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; spi_prettyprint_status_register_hex(status); spi_prettyprint_status_register_welwip(status); @@ -384,7 +397,10 @@ */ int spi_prettyprint_status_register_bp1_srwd(struct flashctx *flash) { - uint8_t status = spi_read_status_register(flash); + uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; spi_prettyprint_status_register_hex(status); spi_prettyprint_status_register_srwd(status); @@ -402,7 +418,10 @@ */ int spi_prettyprint_status_register_bp2_srwd(struct flashctx *flash) { - uint8_t status = spi_read_status_register(flash); + uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; spi_prettyprint_status_register_hex(status); spi_prettyprint_status_register_srwd(status); @@ -419,7 +438,10 @@ */ int spi_prettyprint_status_register_bp3_srwd(struct flashctx *flash) { - uint8_t status = spi_read_status_register(flash); + uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; spi_prettyprint_status_register_hex(status); spi_prettyprint_status_register_srwd(status); @@ -431,7 +453,10 @@ int spi_prettyprint_status_register_bp4_srwd(struct flashctx *flash) { - uint8_t status = spi_read_status_register(flash); + uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; spi_prettyprint_status_register_hex(status); spi_prettyprint_status_register_srwd(status); @@ -442,7 +467,10 @@ int spi_prettyprint_status_register_bp2_bpl(struct flashctx *flash) { - uint8_t status = spi_read_status_register(flash); + uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; spi_prettyprint_status_register_hex(status); spi_prettyprint_status_register_bpl(status); @@ -455,7 +483,10 @@ int spi_prettyprint_status_register_bp2_tb_bpl(struct flashctx *flash) { - uint8_t status = spi_read_status_register(flash); + uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; spi_prettyprint_status_register_hex(status); spi_prettyprint_status_register_bpl(status); @@ -477,7 +508,10 @@ int spi_prettyprint_status_register_amic_a25l032(struct flashctx *flash) { - uint8_t status = spi_read_status_register(flash); + uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; spi_prettyprint_status_register_hex(status); spi_prettyprint_status_register_srwd(status); @@ -533,7 +567,11 @@ int spi_prettyprint_status_register_at25df(struct flashctx *flash) { - uint8_t status = spi_read_status_register(flash); + uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; + spi_prettyprint_status_register_hex(status); spi_prettyprint_status_register_atmel_at25_srpl(status); @@ -556,8 +594,10 @@ int spi_prettyprint_status_register_at25f(struct flashctx *flash) { uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; - status = spi_read_status_register(flash); spi_prettyprint_status_register_hex(status); spi_prettyprint_status_register_atmel_at25_wpen(status); @@ -572,8 +612,10 @@ int spi_prettyprint_status_register_at25f512a(struct flashctx *flash) { uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; - status = spi_read_status_register(flash); spi_prettyprint_status_register_hex(status); spi_prettyprint_status_register_atmel_at25_wpen(status); @@ -588,7 +630,10 @@ int spi_prettyprint_status_register_at25f512b(struct flashctx *flash) { - uint8_t status = spi_read_status_register(flash); + uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; spi_prettyprint_status_register_hex(status); spi_prettyprint_status_register_atmel_at25_srpl(status); @@ -604,7 +649,10 @@ { uint8_t status; - status = spi_read_status_register(flash); + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; + spi_prettyprint_status_register_hex(status); spi_prettyprint_status_register_atmel_at25_wpen(status); @@ -617,7 +665,10 @@ int spi_prettyprint_status_register_at25fs010(struct flashctx *flash) { - uint8_t status = spi_read_status_register(flash); + uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; spi_prettyprint_status_register_hex(status); spi_prettyprint_status_register_atmel_at25_wpen(status); @@ -637,7 +688,10 @@ int spi_prettyprint_status_register_at25fs040(struct flashctx *flash) { - uint8_t status = spi_read_status_register(flash); + uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; spi_prettyprint_status_register_hex(status); spi_prettyprint_status_register_atmel_at25_wpen(status); @@ -649,7 +703,10 @@ int spi_prettyprint_status_register_at26df081a(struct flashctx *flash) { - uint8_t status = spi_read_status_register(flash); + uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; spi_prettyprint_status_register_hex(status); spi_prettyprint_status_register_atmel_at25_srpl(status); @@ -707,7 +764,10 @@ int spi_prettyprint_status_register_en25s_wp(struct flashctx *flash) { - uint8_t status = spi_read_status_register(flash); + uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; spi_prettyprint_status_register_hex(status); spi_prettyprint_status_register_srwd(status); @@ -726,7 +786,10 @@ int spi_prettyprint_status_register_n25q(struct flashctx *flash) { - uint8_t status = spi_read_status_register(flash); + uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; spi_prettyprint_status_register_hex(status); spi_prettyprint_status_register_srwd(status); @@ -751,7 +814,10 @@ /* Used by Intel/Numonyx S33 and Spansion S25FL-S chips */ int spi_prettyprint_status_register_bp2_ep_srwd(struct flashctx *flash) { - uint8_t status = spi_read_status_register(flash); + uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; spi_prettyprint_status_register_hex(status); spi_prettyprint_status_register_srwd(status); @@ -779,7 +845,10 @@ int spi_prettyprint_status_register_sst25(struct flashctx *flash) { - uint8_t status = spi_read_status_register(flash); + uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; spi_prettyprint_status_register_sst25_common(status); return 0; } @@ -795,7 +864,10 @@ "100000H-1FFFFFH", "all", "all" }; - uint8_t status = spi_read_status_register(flash); + uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; spi_prettyprint_status_register_sst25_common(status); msg_cdbg("Resulting block protection : %s\n", bpt[(status & 0x1c) >> 2]); return 0; @@ -810,7 +882,10 @@ "0x40000-0x7ffff", "all blocks", "all blocks", "all blocks", "all blocks" }; - uint8_t status = spi_read_status_register(flash); + uint8_t status; + int ret = spi_read_register(flash, STATUS1, &status); + if (ret) + return ret; spi_prettyprint_status_register_sst25_common(status); msg_cdbg("Resulting block protection : %s\n", bpt[(status & 0x1c) >> 2]); return 0; 65 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. -- To view, visit https://review.coreboot.org/c/flashrom/+/59529 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I146b4b5439872e66c5d33e156451a729d248c7da Gerrit-Change-Number: 59529 Gerrit-PatchSet: 67 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-MessageType: merged

1 0

Change in flashrom[master]: spi25_statusreg: inline spi_write_register_flag()
by Edward O'Callaghan (Code Review) 05 Apr '22

05 Apr '22

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

1 0

Change in flashrom[master]: libflashrom,linux_mtd: add linux_mtd writeprotect support
by Edward O'Callaghan (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: Nico Huber, Angel Pons, Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61897 ) Change subject: libflashrom,linux_mtd: add linux_mtd writeprotect support ...................................................................... Patch Set 38: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/61897 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5c86e28cdec44bec49ba1d36f8ab62241b9b01da Gerrit-Change-Number: 61897 Gerrit-PatchSet: 38 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-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Tue, 05 Apr 2022 02:21:10 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dmi.c: Hide has_dmi_support global behind method
by Sam McNally (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: Edward O'Callaghan, Angel Pons. Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59283 ) Change subject: dmi.c: Hide has_dmi_support global behind method ...................................................................... Patch Set 4: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/59283 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibded9714998ea6f2e5d4e0512fa7c6b105f9638a Gerrit-Change-Number: 59283 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 05 Apr 2022 01:51:41 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashchips: enable write-protection for W25Q{64,128}.V
by Anastasia Klimchuk (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: Nico Huber, Angel Pons, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59071 ) Change subject: flashchips: enable write-protection for W25Q{64,128}.V ...................................................................... Patch Set 25: Code-Review+2 (1 comment) Patchset: PS15: > See CB:60230 and CB:60231. This seems resolved, just checking, does everyone agree this is resolved? -- To view, visit https://review.coreboot.org/c/flashrom/+/59071 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iccb69a8d3a0dd2192e2c938caddaf07b1889ed35 Gerrit-Change-Number: 59071 Gerrit-PatchSet: 25 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.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: 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Tue, 05 Apr 2022 00:59:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: writeprotect: add WPS bit and always set it to zero
by Anastasia Klimchuk (Code Review) 05 Apr '22

05 Apr '22

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

1 0

Change in flashrom[master]: writeprotect: add WPS bit and always set it to zero
by Anastasia Klimchuk (Code Review) 05 Apr '22

05 Apr '22

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

1 0

Change in flashrom[master]: spi25_statusreg.c: add SR3 read/write support
by Anastasia Klimchuk (Code Review) 05 Apr '22

05 Apr '22

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

1 0

Change in flashrom[master]: writeprotect.c: refactor and fix wp_mode functions
by Anastasia Klimchuk (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: Nico Huber, Angel Pons, Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62643 ) Change subject: writeprotect.c: refactor and fix wp_mode functions ...................................................................... Patch Set 12: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/62643 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib6c347453f9216e5816e4ed35bf9783fd3c720e0 Gerrit-Change-Number: 62643 Gerrit-PatchSet: 12 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-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Mon, 04 Apr 2022 23:27:51 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2022