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

Change in flashrom[master]: libflashrom: Return progress state to the library user
by Edward O'Callaghan (Code Review) 25 May '22

25 May '22

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/49643 ) Change subject: libflashrom: Return progress state to the library user ...................................................................... libflashrom: Return progress state to the library user Projects using libflashrom like fwupd expect the user to wait for the operation to complete. To avoid the user thinking the process has "hung" or "got stuck" report back the progress complete of the erase, write and read operations. Add a new --progress flag to the CLI to report progress of operations. Include a test for the dummy spi25 device. TEST=./test_build.sh; ./flashrom -p lspcon_i2c_spi:bus=7 -r /dev/null --progress Change-Id: I7197572bb7f19e3bdb2bde855d70a0f50fd3854c Signed-off-by: Richard Hughes <richard(a)hughsie.com> Signed-off-by: Daniel Campello <campello(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/49643 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> Reviewed-by: Thomas Heijligen <src(a)posteo.de> --- M 82802ab.c M at45db.c M cli_classic.c M cli_output.c M dediprog.c M en29lv640b.c M flashrom.8.tmpl M include/flash.h M include/libflashrom.h M it87spi.c M jedec.c M libflashrom.c M libflashrom.map M linux_mtd.c M lspcon_i2c_spi.c M realtek_mst_i2c_spi.c M spi.c M spi25.c M sst28sf040.c M tests/spi25.c M tests/tests.c M tests/tests.h 22 files changed, 184 insertions(+), 3 deletions(-) Approvals: build bot (Jenkins): Verified Thomas Heijligen: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved Anastasia Klimchuk: Looks good to me, approved diff --git a/82802ab.c b/82802ab.c index b485c11..1db69e6 100644 --- a/82802ab.c +++ b/82802ab.c @@ -134,6 +134,7 @@ chip_writeb(flash, 0x40, dst); chip_writeb(flash, *src++, dst++); wait_82802ab(flash); + update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 1, len); } /* FIXME: Ignore errors for now. */ diff --git a/at45db.c b/at45db.c index 5f949bb..3f586a2 100644 --- a/at45db.c +++ b/at45db.c @@ -550,6 +550,7 @@ msg_cerr("Writing page %u failed!\n", i); return 1; } + update_progress(flash, FLASHROM_PROGRESS_WRITE, i + page_size, len); } return 0; } diff --git a/cli_classic.c b/cli_classic.c index 83ac538..212534c 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -79,6 +79,7 @@ #if CONFIG_PRINT_WIKI == 1 " -z | --list-supported-wiki print supported devices in wiki syntax\n" #endif + " --progress show progress percentage on the standard output\n" " -p | --programmer <name>[:<param>] specify the programmer device. One of\n"); list_programmers_linebreak(4, 80, 0); printf(".\n\nYou can specify one of -h, -R, -L, " @@ -472,6 +473,7 @@ uint32_t wp_start = 0, wp_len = 0; int read_it = 0, extract_it = 0, write_it = 0, erase_it = 0, verify_it = 0; int dont_verify_it = 0, dont_verify_all = 0, list_supported = 0, operation_specified = 0; + int show_progress = 0; struct flashrom_layout *layout = NULL; static const struct programmer_entry *prog = NULL; enum { @@ -487,6 +489,7 @@ OPTION_WP_ENABLE, OPTION_WP_DISABLE, OPTION_WP_LIST, + OPTION_PROGRESS, }; int ret = 0; @@ -523,6 +526,7 @@ {"help", 0, NULL, 'h'}, {"version", 0, NULL, 'R'}, {"output", 1, NULL, 'o'}, + {"progress", 0, NULL, OPTION_PROGRESS}, {NULL, 0, NULL, 0}, }; @@ -763,6 +767,9 @@ cli_classic_abort_usage("No log filename specified.\n"); } break; + case OPTION_PROGRESS: + show_progress = 1; + break; default: cli_classic_abort_usage(NULL); break; @@ -934,6 +941,13 @@ fill_flash = &flashes[0]; + unsigned int progress_user_data[FLASHROM_PROGRESS_NR]; + struct flashrom_progress progress_state = { + .user_data = progress_user_data + }; + if (show_progress) + flashrom_set_progress_callback(fill_flash, &flashrom_progress_cb, &progress_state); + print_chip_support_status(fill_flash->chip); unsigned int limitexceeded = count_max_decode_exceedings(fill_flash); diff --git a/cli_output.c b/cli_output.c index 629db67..e5b829a 100644 --- a/cli_output.c +++ b/cli_output.c @@ -64,6 +64,31 @@ verbose_screen = oldverbose_screen; } +static const char *flashrom_progress_stage_to_string(enum flashrom_progress_stage stage) +{ + if (stage == FLASHROM_PROGRESS_READ) + return "READ"; + if (stage == FLASHROM_PROGRESS_WRITE) + return "WRITE"; + if (stage == FLASHROM_PROGRESS_ERASE) + return "ERASE"; + return "UNKNOWN"; +} + +void flashrom_progress_cb(struct flashrom_flashctx *flashctx) +{ + struct flashrom_progress *progress_state = flashctx->progress_state; + unsigned int pc = 0; + unsigned int *percentages = progress_state->user_data; + if (progress_state->current > 0 && progress_state->total > 0) + pc = ((unsigned long long) progress_state->current * 10000llu) / + ((unsigned long long) progress_state->total * 100llu); + if (percentages[progress_state->stage] != pc) { + percentages[progress_state->stage] = pc; + msg_ginfo("[%s] %u%% complete... ", flashrom_progress_stage_to_string(progress_state->stage), pc); + } +} + /* Please note that level is the verbosity, not the importance of the message. */ int flashrom_print_cb(enum flashrom_log_level level, const char *fmt, va_list ap) { diff --git a/dediprog.c b/dediprog.c index 8a05a7c..3bd61a0 100644 --- a/dediprog.c +++ b/dediprog.c @@ -658,6 +658,7 @@ msg_perr("SPI bulk write failed, expected %i, got %s!\n", 512, libusb_error_name(ret)); return 1; } + update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 1, count); } return 0; diff --git a/en29lv640b.c b/en29lv640b.c index 5b01904..6c0a565 100644 --- a/en29lv640b.c +++ b/en29lv640b.c @@ -48,6 +48,7 @@ #endif dst += 2; src += 2; + update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 2, len); } /* FIXME: Ignore errors for now. */ diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl index 58f5ec0..3dac4b9 100644 --- a/flashrom.8.tmpl +++ b/flashrom.8.tmpl @@ -53,7 +53,8 @@ [\fB\-\-wp\-status\fR] [\fB\-\-wp\-list\fR] [\fB\-\-wp\-enable\fR|\fB\-\-wp\-disable\fR] [\fB\-\-wp\-range\fR <start>,<length>|\fB\-\-wp\-region\fR <region>] [\fB\-n\fR] [\fB\-N\fR] [\fB\-f\fR])] - [\fB\-V\fR[\fBV\fR[\fBV\fR]]] [\fB-o\fR <logfile>] + [\fB\-V\fR[\fBV\fR[\fBV\fR]]] [\fB-o\fR <logfile>] [\fB\-\-progress\fR] + .SH DESCRIPTION .B flashrom is a utility for detecting, reading, writing, verifying and erasing flash @@ -445,6 +446,9 @@ way to gather logs from flashrom because they will be verbose even if the on-screen messages are not verbose and don't require output redirection. .TP +.B "\-\-progress" +Show progress percentage of operations on the standard output. +.TP .B "\-R, \-\-version" Show version information and exit. .SH PROGRAMMER-SPECIFIC INFORMATION diff --git a/include/flash.h b/include/flash.h index 27ade09..45533ba 100644 --- a/include/flash.h +++ b/include/flash.h @@ -350,6 +350,9 @@ chip_restore_fn_cb_t func; uint8_t status; } chip_restore_fn[MAX_CHIP_RESTORE_FUNCTIONS]; + /* Progress reporting */ + flashrom_progress_callback *progress_callback; + struct flashrom_progress *progress_state; }; /* Timing used in probe routines. ZERO is -2 to differentiate between an unset @@ -446,6 +449,7 @@ int close_logfile(void); void start_logging(void); int flashrom_print_cb(enum flashrom_log_level level, const char *fmt, va_list ap); +void flashrom_progress_cb(struct flashrom_flashctx *flashctx); /* Let gcc and clang check for correct printf-style format strings. */ int print(enum flashrom_log_level level, const char *fmt, ...) #ifdef __MINGW32__ @@ -474,6 +478,7 @@ #define msg_gspew(...) print(FLASHROM_MSG_SPEW, __VA_ARGS__) /* general debug spew */ #define msg_pspew(...) print(FLASHROM_MSG_SPEW, __VA_ARGS__) /* programmer debug spew */ #define msg_cspew(...) print(FLASHROM_MSG_SPEW, __VA_ARGS__) /* chip debug spew */ +void update_progress(struct flashctx *flash, enum flashrom_progress_stage stage, size_t current, size_t total); /* spi.c */ struct spi_command { diff --git a/include/libflashrom.h b/include/libflashrom.h index cf1f462..21a5785 100644 --- a/include/libflashrom.h +++ b/include/libflashrom.h @@ -69,6 +69,34 @@ */ void flashrom_set_log_callback(flashrom_log_callback *log_callback); +enum flashrom_progress_stage { + FLASHROM_PROGRESS_READ, + FLASHROM_PROGRESS_WRITE, + FLASHROM_PROGRESS_ERASE, + FLASHROM_PROGRESS_NR, +}; +struct flashrom_progress { + enum flashrom_progress_stage stage; + size_t current; + size_t total; + void *user_data; +}; +struct flashrom_flashctx; +typedef void(flashrom_progress_callback)(struct flashrom_flashctx *flashctx); +/** + * @brief Set the progress callback function. + * + * Set a callback function which will be invoked whenever libflashrom wants + * to indicate the progress has changed. This allows frontends to do whatever + * they see fit with such values, e.g. update a progress bar in a GUI tool. + * + * @param progress_callback Pointer to the new progress callback function. + * @param progress_state Pointer to progress state to include with the progress + * callback. + */ +void flashrom_set_progress_callback(struct flashrom_flashctx *const flashctx, + flashrom_progress_callback *progress_callback, struct flashrom_progress *progress_state); + /** @} */ /* end flashrom-general */ /** @@ -174,7 +202,6 @@ * @{ */ -struct flashrom_flashctx; /** * @brief Probe for a flash chip. * diff --git a/it87spi.c b/it87spi.c index 204816a..a24a172 100644 --- a/it87spi.c +++ b/it87spi.c @@ -291,6 +291,7 @@ int ret = it8716f_spi_page_program(flash, buf, start); if (ret) return ret; + update_progress(flash, FLASHROM_PROGRESS_WRITE, chip->page_size - len, chip->page_size); start += chip->page_size; len -= chip->page_size; buf += chip->page_size; diff --git a/jedec.c b/jedec.c index 0709efa..2542f3e 100644 --- a/jedec.c +++ b/jedec.c @@ -421,6 +421,7 @@ if (write_byte_program_jedec_common(flash, src, dst, mask)) failed = 1; dst++, src++; + update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 1, len); } if (failed) msg_cerr(" writing sector at 0x%" PRIxPTR " failed!\n", olddst); @@ -487,6 +488,7 @@ * we're OK for now. */ unsigned int page_size = flash->chip->page_size; + unsigned int nwrites = (start + len - 1) / page_size; /* Warning: This loop has a very unusual condition and body. * The loop needs to go through each page with at least one affected @@ -497,7 +499,7 @@ * (start + len - 1) / page_size. Since we want to include that last * page as well, the loop condition uses <=. */ - for (i = start / page_size; i <= (start + len - 1) / page_size; i++) { + for (i = start / page_size; i <= nwrites; i++) { /* Byte position of the first byte in the range in this page. */ /* starthere is an offset to the base address of the chip. */ starthere = max(start, i * page_size); @@ -506,6 +508,7 @@ if (write_page_write_jedec_common(flash, buf + starthere - start, starthere, lenhere)) return 1; + update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 1, nwrites + 1); } return 0; diff --git a/libflashrom.c b/libflashrom.c index e70c1d1..0c6613e 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -65,6 +65,25 @@ return 0; } +void flashrom_set_progress_callback(struct flashrom_flashctx *flashctx, flashrom_progress_callback *progress_callback, struct flashrom_progress *progress_state) +{ + flashctx->progress_callback = progress_callback; + flashctx->progress_state = progress_state; +} +/** @private */ +void update_progress(struct flashrom_flashctx *flashctx, enum flashrom_progress_stage stage, size_t current, size_t total) +{ + if (flashctx->progress_callback == NULL) + return; + if (current > total) + current = total; + + flashctx->progress_state->stage = stage; + flashctx->progress_state->current = current; + flashctx->progress_state->total = total; + flashctx->progress_callback(flashctx); +} + const char *flashrom_version_info(void) { return flashrom_version; diff --git a/libflashrom.map b/libflashrom.map index 2249d35..b3af70b 100644 --- a/libflashrom.map +++ b/libflashrom.map @@ -24,6 +24,7 @@ flashrom_programmer_init; flashrom_programmer_shutdown; flashrom_set_log_callback; + flashrom_set_progress_callback; flashrom_shutdown; flashrom_version_info; local: *; diff --git a/linux_mtd.c b/linux_mtd.c index 445a903..88488c0 100644 --- a/linux_mtd.c +++ b/linux_mtd.c @@ -214,6 +214,7 @@ } i += step; + update_progress(flash, FLASHROM_PROGRESS_READ, i, len); } return 0; @@ -256,6 +257,7 @@ } i += step; + update_progress(flash, FLASHROM_PROGRESS_WRITE, i, len); } return 0; @@ -292,6 +294,7 @@ __func__, ret, strerror(errno)); return 1; } + update_progress(flash, FLASHROM_PROGRESS_ERASE, u + data->erasesize, len); } return 0; diff --git a/lspcon_i2c_spi.c b/lspcon_i2c_spi.c index cabe21f..f3c11d6 100644 --- a/lspcon_i2c_spi.c +++ b/lspcon_i2c_spi.c @@ -354,6 +354,7 @@ for (i = 0; i < len; i += LSPCON_PAGE_SIZE) { ret |= lspcon_i2c_spi_map_page(fd, start + i); ret |= lspcon_i2c_spi_read_data(fd, PAGE_ADDRESS, buf + i, min(len - i, LSPCON_PAGE_SIZE)); + update_progress(flash, FLASHROM_PROGRESS_READ, i + LSPCON_PAGE_SIZE, len); } return ret; @@ -394,6 +395,7 @@ for (unsigned int i = 0; i < len; i += LSPCON_PAGE_SIZE) { ret |= lspcon_i2c_spi_map_page(fd, start + i); ret |= lspcon_i2c_spi_write_page(fd, buf + i, min(len - i, LSPCON_PAGE_SIZE)); + update_progress(flash, FLASHROM_PROGRESS_WRITE, i + LSPCON_PAGE_SIZE, len); } ret |= lspcon_i2c_spi_enable_write_protection(fd); diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c index 954150d..c7de2fe 100644 --- a/realtek_mst_i2c_spi.c +++ b/realtek_mst_i2c_spi.c @@ -398,6 +398,7 @@ ret |= realtek_mst_i2c_execute_write(fd); if (ret) break; + update_progress(flash, FLASHROM_PROGRESS_WRITE, i + RTK_PAGE_SIZE, len); } return ret; diff --git a/spi.c b/spi.c index f2e91c4..9f820d7 100644 --- a/spi.c +++ b/spi.c @@ -101,6 +101,8 @@ { int ret; size_t to_read; + size_t start_address = start; + size_t end_address = len - start; for (; len; len -= to_read, buf += to_read, start += to_read) { /* Do not cross 16MiB boundaries in a single transfer. This helps with @@ -110,6 +112,7 @@ ret = flash->mst->spi.read(flash, buf, start, to_read); if (ret) return ret; + update_progress(flash, FLASHROM_PROGRESS_READ, start - start_address + to_read, end_address); } return 0; } diff --git a/spi25.c b/spi25.c index e96a38d..39a8876 100644 --- a/spi25.c +++ b/spi25.c @@ -674,11 +674,14 @@ { int ret; size_t to_read; + size_t start_address = start; + size_t end_address = len - start; for (; len; len -= to_read, buf += to_read, start += to_read) { to_read = min(chunksize, len); ret = spi_nbyte_read(flash, start, buf, to_read); if (ret) return ret; + update_progress(flash, FLASHROM_PROGRESS_READ, start - start_address + to_read, end_address); } return 0; } @@ -698,6 +701,8 @@ * we're OK for now. */ unsigned int page_size = flash->chip->page_size; + size_t start_address = start; + size_t end_address = len - start; /* Warning: This loop has a very unusual condition and body. * The loop needs to go through each page with at least one affected @@ -722,6 +727,7 @@ if (rc) return rc; } + update_progress(flash, FLASHROM_PROGRESS_WRITE, start - start_address + lenhere, end_address); } return 0; @@ -741,6 +747,7 @@ for (i = start; i < start + len; i++) { if (spi_nbyte_program(flash, i, buf + i - start, 1)) return 1; + update_progress(flash, FLASHROM_PROGRESS_WRITE, i - start, len - start); } return 0; } diff --git a/sst28sf040.c b/sst28sf040.c index 3da25f1..9a08d6b 100644 --- a/sst28sf040.c +++ b/sst28sf040.c @@ -92,6 +92,7 @@ /* wait for Toggle bit ready */ toggle_ready_jedec(flash, bios); + update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 1, len); } return 0; diff --git a/tests/spi25.c b/tests/spi25.c index f57251e..f101910 100644 --- a/tests/spi25.c +++ b/tests/spi25.c @@ -67,6 +67,65 @@ return 0; } +static void spi_read_progress_cb(struct flashrom_flashctx *flashctx) +{ + struct flashrom_progress *progress_state = flashctx->progress_state; + uint32_t *cnt = (uint32_t *) progress_state->user_data; + assert_int_equal(0x300, progress_state->total); + switch (*cnt) { + case 0: + assert_int_equal(0x100, progress_state->current); + break; + case 1: + assert_int_equal(0x200, progress_state->current); + break; + case 2: + assert_int_equal(0x300, progress_state->current); + break; + case 3: + assert_int_equal(0x300, progress_state->current); + break; + case 4: + assert_int_equal(0x300, progress_state->current); + break; + default: + fail(); + } + (*cnt)++; +} + +void spi_read_chunked_test_success(void **state) +{ + (void) state; /* unused */ + uint8_t buf[0x400] = { 0x0 }; + uint32_t cnt = 0; + const unsigned int max_data_read = 0x100; + const unsigned int offset = 0x100; + struct registered_master mst = { + .spi.read = default_spi_read, + .spi.max_data_read = max_data_read + }; + + /* setup initial test state */ + struct flashctx flashctx = { + .chip = &mock_chip, + .mst = &mst + }; + struct flashrom_progress progress_state = { + .user_data = (void *) &cnt, + }; + flashrom_set_progress_callback(&flashctx, spi_read_progress_cb, &progress_state); + for (int i = 0; i < 4; i++) { + expect_memory(__wrap_spi_send_command, flash, + &flashctx, sizeof(flashctx)); + will_return(__wrap_spi_send_command, JEDEC_WRDI); + will_return(__wrap_spi_send_command, JEDEC_READ); + will_return(__wrap_spi_send_command, max_data_read); + } + assert_int_equal(0, spi_chip_read(&flashctx, buf, offset, sizeof(buf))); + assert_int_equal(5, cnt); +} + void spi_write_enable_test_success(void **state) { (void) state; /* unused */ diff --git a/tests/tests.c b/tests/tests.c index 2f955ff..a719a8f 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -391,6 +391,7 @@ const struct CMUnitTest spi25_tests[] = { cmocka_unit_test(spi_write_enable_test_success), cmocka_unit_test(spi_write_disable_test_success), + cmocka_unit_test(spi_read_chunked_test_success), cmocka_unit_test(probe_spi_rdid_test_success), cmocka_unit_test(probe_spi_rdid4_test_success), cmocka_unit_test(probe_spi_rems_test_success), diff --git a/tests/tests.h b/tests/tests.h index e2b2211..5ffc145 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -31,6 +31,7 @@ /* spi25.c */ void spi_write_enable_test_success(void **state); void spi_write_disable_test_success(void **state); +void spi_read_chunked_test_success(void **state); void probe_spi_rdid_test_success(void **state); void probe_spi_rdid4_test_success(void **state); void probe_spi_rems_test_success(void **state); -- To view, visit https://review.coreboot.org/c/flashrom/+/49643 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7197572bb7f19e3bdb2bde855d70a0f50fd3854c Gerrit-Change-Number: 49643 Gerrit-PatchSet: 23 Gerrit-Owner: Richard Hughes <richard(a)hughsie.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org> Gerrit-CC: Richard Hughes <hughsient(a)gmail.com> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: dummyflasher: Wire variable size feature via opaque infra
by Edward O'Callaghan (Code Review) 25 May '22

25 May '22

Attention is currently required from: Nico Huber, Thomas Heijligen, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64488 ) Change subject: dummyflasher: Wire variable size feature via opaque infra ...................................................................... Patch Set 3: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/64488 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I76402bfdf8b1a75489e4509fec92c9a777d0cf58 Gerrit-Change-Number: 64488 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Namyoon Woo <namyoon(a)google.com> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 25 May 2022 01:02:22 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dummyflasher: Wire variable size feature via opaque infra
by Anastasia Klimchuk (Code Review) 25 May '22

25 May '22

Attention is currently required from: Nico Huber, Thomas Heijligen, Edward O'Callaghan, Angel Pons, Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64488 ) Change subject: dummyflasher: Wire variable size feature via opaque infra ...................................................................... Patch Set 3: (1 comment) File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/64488/comment/ddc854ac_a9dba977 PS1, Line 1269: rmst.spi = *s_mst; : rmst.opaque = *o_mst; > > So... do you think it's too bad? :) I don't have another ideas at the moment :\ […] Good news: I got rid of double master! Now everything is done as opaque master. I will leave another comment unresolved to verify with the users of the feature, but at the moment I am optimistic :) I noticed something: > when I tried to port things from chromium myself Wow I didn't know that you have experience porting things from chromium! -- To view, visit https://review.coreboot.org/c/flashrom/+/64488 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I76402bfdf8b1a75489e4509fec92c9a777d0cf58 Gerrit-Change-Number: 64488 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Namyoon Woo <namyoon(a)google.com> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 25 May 2022 00:47:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dummyflasher: Wire variable size feature via opaque infra
by Anastasia Klimchuk (Code Review) 25 May '22

25 May '22

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev. Hello build bot (Jenkins), Thomas Heijligen, Edward O'Callaghan, Nikolai Artemiev, Peter Marheine, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/64488 to look at the new patch set (#3). Change subject: dummyflasher: Wire variable size feature via opaque infra ...................................................................... dummyflasher: Wire variable size feature via opaque infra Wire "variable size" feature in dummy programmer via opaque infra. This patch fixes the broken build with CONFIG_DUMMY=no. Dummyflasher registers opaque master for the case when it is initialised with EMULATE_VARIABLE_SIZE. Dummy opaque master emulates read/write/erase as simple memory operations over `data->flashchip_contents`. Fixes: https://ticket.coreboot.org/issues/365 TEST=the following scenarious run successfully Testing build $ make clean && make CONFIG_DUMMY=no $ flashrom -h : dummy is not in the list $ make clean && make CONFIG_EVERYTHING=yes $ flashrom -h : dummy is in the list Testing "variable size" feature $ flashrom -p dummy:size=8388608,emulate=VARIABLE_SIZE -V $ flashrom -p dummy:size=8388608,emulate=VARIABLE_SIZE -r /tmp/dump.bin -V $ head -c 8388608 </dev/urandom >/tmp/image.bin $ flashrom -p dummy:image=/tmp/image.bin,size=8388608,emulate=VARIABLE_SIZE -w /tmp/dump.bin -V Testing standard flow $ flashrom -p dummy:emulate=W25Q128FV -V $ flashrom -p dummy:emulate=W25Q128FV -r /tmp/dump.bin -V $ head -c 16777216 </dev/urandom >/tmp/image.bin $ flashrom -p dummy:image=/tmp/image.bin,emulate=W25Q128FV -w /tmp/dump.bin -V Change-Id: I76402bfdf8b1a75489e4509fec92c9a777d0cf58 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M dummyflasher.c M flashchips.c M include/chipdrivers.h 3 files changed, 77 insertions(+), 73 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/88/64488/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/64488 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I76402bfdf8b1a75489e4509fec92c9a777d0cf58 Gerrit-Change-Number: 64488 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Namyoon Woo <namyoon(a)google.com> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: flashchips: Add missing block eraser for S25FL512S
by Edward O'Callaghan (Code Review) 24 May '22

24 May '22

Attention is currently required from: Nico Huber, Angel Pons. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64637 ) Change subject: flashchips: Add missing block eraser for S25FL512S ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/64637 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8708294d42f5da80c0ca07ccdae627f13fd5c645 Gerrit-Change-Number: 64637 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 24 May 2022 23:54:03 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashchips.c: add CMP bit entry to W25Q256.V
by Edward O'Callaghan (Code Review) 24 May '22

24 May '22

Attention is currently required from: Nikolai Artemiev, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64607 ) Change subject: flashchips.c: add CMP bit entry to W25Q256.V ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/64607 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1cb400f6b8542a9054875b8f2557db1cc06292e2 Gerrit-Change-Number: 64607 Gerrit-PatchSet: 2 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 24 May 2022 23:49:23 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: meson: use files() for srcs list
by Edward O'Callaghan (Code Review) 24 May '22

24 May '22

Attention is currently required from: Thomas Heijligen. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64384 ) Change subject: meson: use files() for srcs list ...................................................................... Patch Set 1: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/64384/comment/10519b5a_2623fbe9 PS1, Line 8: > Meson looks up if the file xyz.c exists when calling file("xyz.c"). […] Sorry I was too brief; yes please include the first part in the commit message that is all. -- To view, visit https://review.coreboot.org/c/flashrom/+/64384 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I346b5468b4203f1521ec73a90f93ff3b13ebf43c Gerrit-Change-Number: 64384 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Tue, 24 May 2022 23:46:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: spi25_statusreg.c: Allow opaque masters to hook spi_{rw}_register()
by Edward O'Callaghan (Code Review) 24 May '22

24 May '22

Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev, Sergii Dmytruk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64375 ) Change subject: spi25_statusreg.c: Allow opaque masters to hook spi_{rw}_register() ...................................................................... Patch Set 4: (1 comment) File spi25_statusreg.c: https://review.coreboot.org/c/flashrom/+/64375/comment/df68aca4_bbd605a6 PS3, Line 35: } > Well, it says "Do not unnecessarily use braces where a single statement will do.". […] I think once we get clang-format decided on we can just make this sort of thing automatic and make the whole code-base consistent. This sort of thing comes up from time to time in code review unnecessarily due to lack of tooling usage. -- To view, visit https://review.coreboot.org/c/flashrom/+/64375 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3ab0d7f5f48338c8ecb118a69651c203fbc516ac Gerrit-Change-Number: 64375 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Subrata Banik <subratabanik(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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Tue, 24 May 2022 23:43:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> 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]: flashchips.c: Fix 4BA for S25FL512S
by Nico Huber (Code Review) 24 May '22

24 May '22

Attention is currently required from: Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64425 ) Change subject: flashchips.c: Fix 4BA for S25FL512S ...................................................................... Patch Set 1: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/64425/comment/2fd2e31d_42b18674 PS1, Line 12: so select them through `FEATURE_4BA_EAR7`. As it is not known how this > It looks like they use different opcodes, though (16/17 instead of […] Turned out we actually supported different opcodes already with the `.wrea_override` field. As it's hard to understand, I dropped it in CB:64636 and replaced it with a proper feature flag. If my commit chain lands, I guess this change here becomes obsolete. -- To view, visit https://review.coreboot.org/c/flashrom/+/64425 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I08c5a70351d945130183fe01c328dc4d30cfe0ef Gerrit-Change-Number: 64425 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 24 May 2022 13:49:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashchips: Add missing block eraser for S25FL512S
by Nico Huber (Code Review) 24 May '22

24 May '22

Attention is currently required from: Angel Pons. Hello Angel Pons, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/64637 to review the following change. Change subject: flashchips: Add missing block eraser for S25FL512S ...................................................................... flashchips: Add missing block eraser for S25FL512S Now that we can make use of the extended-address register, we can also advertise the `d8` eraser that can take 3- or 4-byte addresses. Signed-off-by: Nico Huber <nico.h(a)gmx.de> Ticket: https://ticket.coreboot.org/issues/357 Change-Id: I8708294d42f5da80c0ca07ccdae627f13fd5c645 --- M flashchips.c 1 file changed, 3 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/37/64637/1 diff --git a/flashchips.c b/flashchips.c index cc80159..46a6017 100644 --- a/flashchips.c +++ b/flashchips.c @@ -16731,6 +16731,9 @@ .eraseblocks = { { 256 * 1024, 256} }, .block_erase = spi_block_erase_dc, }, { + .eraseblocks = { { 256 * 1024, 256} }, + .block_erase = spi_block_erase_d8, + }, { .eraseblocks = { { 65536 * 1024, 1} }, .block_erase = spi_block_erase_60, }, { -- To view, visit https://review.coreboot.org/c/flashrom/+/64637 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8708294d42f5da80c0ca07ccdae627f13fd5c645 Gerrit-Change-Number: 64637 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit May 2022