- flashrom-gerrit - mail.coreboot.org

[S] Change in flashrom[main]: doc/devel: Add info about new libflashrom APIs
by Peter Marheine (Code Review) 12 May '25

12 May '25

Attention is currently required from: Anastasia Klimchuk, Dmitry Zhadinets. Peter Marheine has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/87644?usp=email ) Change subject: doc/devel: Add info about new libflashrom APIs ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/87644?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I98524353b8bedff3364568636ed42a4bd6ce8b3d Gerrit-Change-Number: 87644 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Dmitry Zhadinets <dzhadinets(a)gmail.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Dmitry Zhadinets <dzhadinets(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 12 May 2025 23:50:18 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[S] Change in flashrom[main]: cli: Use flashrom_supported_programmers API in cli
by Anastasia Klimchuk (Code Review) 12 May '25

12 May '25

Attention is currently required from: Dmitry Zhadinets. Anastasia Klimchuk has posted comments on this change by Dmitry Zhadinets. ( https://review.coreboot.org/c/flashrom/+/87195?usp=email ) Change subject: cli: Use flashrom_supported_programmers API in cli ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > I just thought about something, it's for the separate commit. […] I made a patch and added all updated for libflashrom API in one patch CB:87644 (added you as reviewer) -- To view, visit https://review.coreboot.org/c/flashrom/+/87195?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I86ac83ee0ef4e850e69aa4e7b607e28ebab9d3d5 Gerrit-Change-Number: 87195 Gerrit-PatchSet: 2 Gerrit-Owner: Dmitry Zhadinets <dzhadinets(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Dmitry Zhadinets <dzhadinets(a)gmail.com> Gerrit-Comment-Date: Mon, 12 May 2025 12:17:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[XS] Change in flashrom[main]: Add more delay time for jlink power on feature to stabilize the LDO a...
by Anastasia Klimchuk (Code Review) 12 May '25

12 May '25

Attention is currently required from: attila-v. Anastasia Klimchuk has posted comments on this change by attila-v. ( https://review.coreboot.org/c/flashrom/+/86085?usp=email ) Change subject: Add more delay time for jlink power on feature to stabilize the LDO and the decoupling capacitors ...................................................................... Patch Set 3: (1 comment) Patchset: PS3: I rebased to make Jenkins re-run, it's successful now. -- To view, visit https://review.coreboot.org/c/flashrom/+/86085?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ic2dd94e99ac6ffa17a009b8488ce027698ae2c28 Gerrit-Change-Number: 86085 Gerrit-PatchSet: 3 Gerrit-Owner: attila-v Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: attila-v Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: attila-v Gerrit-Comment-Date: Mon, 12 May 2025 12:15:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[XS] Change in flashrom[main]: Add more delay time for jlink power on feature to stabilize the LDO a...
by Anastasia Klimchuk (Code Review) 12 May '25

12 May '25

Attention is currently required from: attila-v. Anastasia Klimchuk has posted comments on this change by attila-v. ( https://review.coreboot.org/c/flashrom/+/86085?usp=email ) Change subject: Add more delay time for jlink power on feature to stabilize the LDO and the decoupling capacitors ...................................................................... Patch Set 2: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/86085/comment/bf8a2ea6_131cc4d1?us… : PS2, Line 7: Add more delay time for jlink power on feature to stabilize the LDO and the decoupling capacitors For this, I can give a suggestion, you can use it or compose a different one (just make sure it wraps at 72 chars) title (first line) > jlink_spi: Increase delay on power on feature from 10 to 100 millisec and then a longer detailed description like this for example > More delay time is needed to to stabilize the LDO and the decoupling capacitors when power on feature is enabled. Specifically 100 ms was tested on HT7333 and it's sufficient. > I used the power supply from the J-Link device pin 19. this power is 5V, I put a 3.3V LDO (HT7333) after it, and this LDO (and decoupling capacitors) need more time to fix output voltage. File jlink_spi.c: https://review.coreboot.org/c/flashrom/+/86085/comment/e3b85a07_5c19de94?us… : PS2, Line 469: internal_sleep(100000); > I think it will be enoughwthis small delay patch for now, nobody will sense this little 0.1 seconds delay which I think is definitely enough. That's good! Do I understand correctly, you think we can just go ahead with this change? It's easier if parameter is not needed, then all the code you done already. For me, yes it seems 0.1s is small enough, we can have it. One thing to do is to update commit message: write a good commit title and message which explains the reason. I did my suggestion (in the other comment) but you can make corrections or add details of you want. -- To view, visit https://review.coreboot.org/c/flashrom/+/86085?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ic2dd94e99ac6ffa17a009b8488ce027698ae2c28 Gerrit-Change-Number: 86085 Gerrit-PatchSet: 2 Gerrit-Owner: attila-v Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: attila-v Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: attila-v Gerrit-Comment-Date: Mon, 12 May 2025 12:06:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: attila-v

1 0

[S] Change in flashrom[main]: doc/devel: Add info about new libflashrom APIs
by Anastasia Klimchuk (Code Review) 12 May '25

12 May '25

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/87644?usp=email ) Change subject: doc/devel: Add info about new libflashrom APIs ...................................................................... doc/devel: Add info about new libflashrom APIs Change-Id: I98524353b8bedff3364568636ed42a4bd6ce8b3d Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M doc/release_notes/devel.rst 1 file changed, 39 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/44/87644/1 diff --git a/doc/release_notes/devel.rst b/doc/release_notes/devel.rst index 2a2870d..ef8364d 100644 --- a/doc/release_notes/devel.rst +++ b/doc/release_notes/devel.rst @@ -52,7 +52,15 @@ to ``enabled`` in which case the build will fail if the requirements are not satisfied. -New libflashrom API for progress reporting +Programmer updates +------------------ + +* spidriver: Add support for the Excamera Labs SPIDriver + +libflashrom API updates +======================= + +New API for progress reporting ------------------------------------------ The old ``flashrom_set_progress_callback`` function for requesting progress updates @@ -65,7 +73,34 @@ define their own global state to track progress, and it was impossible to fix that issue while maintaining binary compatibility without adding a new API. -Programmer updates ------------------- +New API for logging and setting log level +----------------------------------------- -* spidriver: Add support for the Excamera Labs SPIDriver +New API ``flashrom_set_log_callback_v2`` is added which allows the users to provide +a pointer to user data, the latter is managed by the caller of the API. New API also +introduces a new signature for log callback function, ``flashrom_log_callback_v2``. + +New API supports setting the log level for messages from libflashrom, ``flashrom_set_log_level``. +The log callback is invoked only for the messages with log level less or equal than the one +requested (i.e. only for the messages at least as urgent as the level requested). + +Old API ``flashrom_set_log_callback`` without user data continues to work as before. + +New API for probing flashchips +------------------------------ + +New API for probing is added ``flashrom_flash_probe_v2`` which can (if requested) +go through all known flashchips and find all matches. v2 returns the number of matches +found (or 0 if none found) and the list of names of all matched entries. + +``flashrom_flash_probe_v2`` continues to support an optional parameter ``chip_name`` +if the caller want to probe for only one specific chip with given name. + +Old API ``flashrom_flash_probe`` which stops probing if more than one chip entry matches +continues to work as before. + +New API to get list of supported programmers +-------------------------------------------- + +New API ``flashrom_supported_programmers`` returns the list of all programmers that are +supported on a current run environment. -- To view, visit https://review.coreboot.org/c/flashrom/+/87644?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I98524353b8bedff3364568636ed42a4bd6ce8b3d Gerrit-Change-Number: 87644 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[M] Change in flashrom[main]: cli: Remove array of flash contexts, use one active context instead
by Anastasia Klimchuk (Code Review) 12 May '25

12 May '25

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/87533?usp=email ) Change subject: cli: Remove array of flash contexts, use one active context instead ...................................................................... cli: Remove array of flash contexts, use one active context instead The only purpose of array of 8 flash contexts was to facilitate probing of multiple chips. Probing is now done inside libflashrom, and cli as a client of libflashrom only need to have one active context. In addition to array of 8, cli also had one more flash context, `fill_flash` which is also now replaced by the same, one active context. Another detail is that array of 8 was effectively a limit of how many mathing chips could be found. While 8 seemed a lot at the time of initial implementation, at the moment we have an example of 6 matches already. (see `./flashrom -p dummy:emulate=MX25L6436`) Change-Id: Ia4284ae7aaa43fe59f0d3f57314ebc5cc93d2d9b Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/87533 Reviewed-by: Peter Marheine <pmarheine(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M cli_classic.c 1 file changed, 37 insertions(+), 43 deletions(-) Approvals: Peter Marheine: Looks good to me, approved build bot (Jenkins): Verified diff --git a/cli_classic.c b/cli_classic.c index f465785..8b6474b 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -1046,9 +1046,7 @@ int main(int argc, char *argv[]) { const struct flashchip *chip = NULL; - /* Probe for up to eight flash chips. */ - struct flashctx flashes[8] = {{0}}; - struct flashctx *fill_flash; + struct flashctx context = {0}; /* holds the active detected chip and other info */ char *tempstr = NULL; int i, j; int ret = 0; @@ -1210,7 +1208,7 @@ msg_pdbg("The following protocols are supported: %s.\n", tempstr ? tempstr : "?"); free(tempstr); - all_matched_count = flashrom_flash_probe_v2(&flashes[0], &all_matched_names, + all_matched_count = flashrom_flash_probe_v2(&context, &all_matched_names, NULL, options.chip_to_probe); if (all_matched_count == -1) { /* -1 is the ret code which means "something went wrong". @@ -1223,7 +1221,7 @@ if (all_matched_count > 1) { msg_cinfo("Multiple flash chip definitions match the detected chip(s): \"%s\"", - flashes[0].chip->name); + context.chip->name); for (int ind = 1; ind < all_matched_count; ind++) msg_cinfo(", \"%s\"", all_matched_names[ind]); msg_cinfo("\nPlease specify which chip definition to use with the -c <chipname> option.\n"); @@ -1258,7 +1256,7 @@ int startchip = -1; for (j = 0; j < registered_master_count; j++) { mst = &registered_masters[j]; - startchip = probe_flash(mst, 0, &flashes[0], 1, options.chip_to_probe); + startchip = probe_flash(mst, 0, &context, 1, options.chip_to_probe); if (startchip != -1) break; } @@ -1269,33 +1267,31 @@ goto out_shutdown; } msg_cinfo("Please note that forced reads most likely contain garbage.\n"); - flashrom_flag_set(&flashes[0], FLASHROM_FLAG_FORCE, options.force); - ret = do_read(&flashes[0], options.filename); - free(flashes[0].chip); + flashrom_flag_set(&context, FLASHROM_FLAG_FORCE, options.force); + ret = do_read(&context, options.filename); + free(context.chip); goto out_shutdown; } ret = 1; goto out_shutdown; } else if (!options.chip_to_probe) { /* repeat for convenience when looking at foreign logs */ - tempstr = flashbuses_to_text(flashes[0].chip->bustype); + tempstr = flashbuses_to_text(context.chip->bustype); msg_gdbg("Found %s flash chip \"%s\" (%d kB, %s).\n", - flashes[0].chip->vendor, flashes[0].chip->name, flashes[0].chip->total_size, + context.chip->vendor, context.chip->name, context.chip->total_size, tempstr ? tempstr : "?"); free(tempstr); } - fill_flash = &flashes[0]; - struct cli_progress cli_progress = {0}; if (options.show_progress) - flashrom_set_progress_callback_v2(fill_flash, &flashrom_progress_cb, &cli_progress); + flashrom_set_progress_callback_v2(&context, &flashrom_progress_cb, &cli_progress); - print_chip_support_status(fill_flash->chip); + print_chip_support_status(context.chip); - unsigned int limitexceeded = count_max_decode_exceedings(fill_flash, &max_rom_decode); + unsigned int limitexceeded = count_max_decode_exceedings(&context, &max_rom_decode); if (limitexceeded > 0 && !options.force) { - enum chipbustype commonbuses = fill_flash->mst->buses_supported & fill_flash->chip->bustype; + enum chipbustype commonbuses = context.mst->buses_supported & context.chip->bustype; /* Sometimes chip and programmer have more than one bus in common, * and the limit is not exceeded on all buses. Tell the user. */ @@ -1393,10 +1389,10 @@ } if (options.flash_name) { - if (fill_flash->chip->vendor && fill_flash->chip->name) { + if (context.chip->vendor && context.chip->name) { printf("vendor=\"%s\" name=\"%s\"\n", - fill_flash->chip->vendor, - fill_flash->chip->name); + context.chip->vendor, + context.chip->name); } else { ret = -1; } @@ -1404,7 +1400,7 @@ } if (options.flash_size) { - printf("%zu\n", flashrom_flash_getsize(fill_flash)); + printf("%zu\n", flashrom_flash_getsize(&context)); goto out_shutdown; } @@ -1413,10 +1409,10 @@ msg_ginfo("Invalid input of sacrifice ratio, valid 0-50. Fallback to default value 0.\n"); options.sacrifice_ratio = 0; } - fill_flash->sacrifice_ratio = options.sacrifice_ratio; + context.sacrifice_ratio = options.sacrifice_ratio; } - if (options.ifd && (flashrom_layout_read_from_ifd(&options.layout, fill_flash, NULL, 0) || + if (options.ifd && (flashrom_layout_read_from_ifd(&options.layout, &context, NULL, 0) || process_include_args(options.layout, options.include_args))) { ret = 1; goto out_shutdown; @@ -1441,20 +1437,20 @@ goto out_shutdown; } - if (flashrom_layout_read_fmap_from_buffer(&options.layout, fill_flash, fmapfile_buffer, fmapfile_size) || + if (flashrom_layout_read_fmap_from_buffer(&options.layout, &context, fmapfile_buffer, fmapfile_size) || process_include_args(options.layout, options.include_args)) { ret = 1; free(fmapfile_buffer); goto out_shutdown; } free(fmapfile_buffer); - } else if (options.fmap && (flashrom_layout_read_fmap_from_rom(&options.layout, fill_flash, 0, - flashrom_flash_getsize(fill_flash)) || + } else if (options.fmap && (flashrom_layout_read_fmap_from_rom(&options.layout, &context, 0, + flashrom_flash_getsize(&context)) || process_include_args(options.layout, options.include_args))) { ret = 1; goto out_shutdown; } - flashrom_layout_set(fill_flash, options.layout); + flashrom_layout_set(&context, options.layout); if (any_wp_op) { if (options.set_wp_region && options.wp_region) { @@ -1472,7 +1468,7 @@ options.set_wp_range = true; } ret = wp_cli( - fill_flash, + &context, options.enable_wp, options.disable_wp, options.print_wp_status, @@ -1485,24 +1481,24 @@ goto out_release; } - flashrom_flag_set(fill_flash, FLASHROM_FLAG_FORCE, options.force); + flashrom_flag_set(&context, FLASHROM_FLAG_FORCE, options.force); #if CONFIG_INTERNAL == 1 - flashrom_flag_set(fill_flash, FLASHROM_FLAG_FORCE_BOARDMISMATCH, force_boardmismatch); + flashrom_flag_set(&context, FLASHROM_FLAG_FORCE_BOARDMISMATCH, force_boardmismatch); #endif - flashrom_flag_set(fill_flash, FLASHROM_FLAG_VERIFY_AFTER_WRITE, !options.dont_verify_it); - flashrom_flag_set(fill_flash, FLASHROM_FLAG_VERIFY_WHOLE_CHIP, !options.dont_verify_all); + flashrom_flag_set(&context, FLASHROM_FLAG_VERIFY_AFTER_WRITE, !options.dont_verify_it); + flashrom_flag_set(&context, FLASHROM_FLAG_VERIFY_WHOLE_CHIP, !options.dont_verify_all); /* FIXME: We should issue an unconditional chip reset here. This can be * done once we have a .reset function in struct flashchip. * Give the chip time to settle. */ - programmer_delay(fill_flash, 100000); + programmer_delay(&context, 100000); if (options.read_it) - ret = do_read(fill_flash, options.filename); + ret = do_read(&context, options.filename); else if (options.extract_it) - ret = do_extract(fill_flash); + ret = do_extract(&context); else if (options.erase_it) { - ret = flashrom_flash_erase(fill_flash); + ret = flashrom_flash_erase(&context); /* * FIXME: Do we really want the scary warning if erase failed? * After all, after erase the chip is either blank or partially @@ -1514,13 +1510,13 @@ emergency_help_message(); } else if (options.write_it) - ret = do_write(fill_flash, options.filename, options.referencefile); + ret = do_write(&context, options.filename, options.referencefile); else if (options.verify_it) - ret = do_verify(fill_flash, options.filename); + ret = do_verify(&context, options.filename); #if CONFIG_RPMC_ENABLED == 1 if (any_rpmc_op && ret == 0) { - ret = rpmc_cli(fill_flash, + ret = rpmc_cli(&context, options.rpmc_root_key_file, options.rpmc_key_data, options.rpmc_counter_address, @@ -1538,10 +1534,8 @@ out_shutdown: flashrom_programmer_shutdown(NULL); out: - for (int ind = 0; ind < all_matched_count; ind++) { - flashrom_layout_release(flashes[ind].default_layout); - free(flashes[ind].chip); - } + flashrom_layout_release(context.default_layout); + free(context.chip); flashrom_data_free(all_matched_names); free_options(&options); -- To view, visit https://review.coreboot.org/c/flashrom/+/87533?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ia4284ae7aaa43fe59f0d3f57314ebc5cc93d2d9b Gerrit-Change-Number: 87533 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

[L] Change in flashrom[main]: libflashrom: Add probing v2 which can find all matching chips
by Anastasia Klimchuk (Code Review) 12 May '25

12 May '25

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/87341?usp=email ) Change subject: libflashrom: Add probing v2 which can find all matching chips ...................................................................... libflashrom: Add probing v2 which can find all matching chips Probing v2 can (if requested) go through all flashchips and find all the matching chip definitions. This is the way cli behaves, so cli becomes a client of probing v2. Previously cli and libflashrom had different probing logic, and different code in different source files. This patch also adds tests for probing v2. Testing from the cli: ./flashrom -p dummy:emulate=W25Q128FV -r dump.rom ./flashrom -p dummy:emulate=MX25L6436 -r dump.rom ./flashrom -p dummy:emulate=MX25L6436 -c "MX25L6473E" -r dump.rom ./flashrom -p dummy:emulate=SST25VF032B -E ./flashrom -p dummy:emulate=S25FL128L -r dump.rom ./flashrom -p dummy:emulate=INVALID -r dump.rom ./flashrom -p dummy:emulate=MX25L6436 -c "NONEXISTENT" -r dump.rom Change-Id: Idfcf377a8071e22028ba98515f08495ed2a6e9f0 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/87341 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Peter Marheine <pmarheine(a)chromium.org> --- M cli_classic.c M include/libflashrom.h M libflashrom.c M libflashrom.map M tests/dummyflasher.c M tests/lifecycle.c M tests/lifecycle.h M tests/tests.c M tests/tests.h 9 files changed, 235 insertions(+), 24 deletions(-) Approvals: build bot (Jenkins): Verified Peter Marheine: Looks good to me, approved diff --git a/cli_classic.c b/cli_classic.c index 3e5dab9..f465785 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -1050,9 +1050,10 @@ struct flashctx flashes[8] = {{0}}; struct flashctx *fill_flash; char *tempstr = NULL; - int startchip = -1, chipcount = 0; int i, j; int ret = 0; + int all_matched_count = 0; + const char **all_matched_names = NULL; struct cli_options options = { 0 }; static const char optstring[] = "r::Rw::v::nNVEfc:l:i:p:Lzho:x"; @@ -1209,26 +1210,26 @@ msg_pdbg("The following protocols are supported: %s.\n", tempstr ? tempstr : "?"); free(tempstr); - for (j = 0; j < registered_master_count; j++) { - startchip = 0; - while (chipcount < (int)ARRAY_SIZE(flashes)) { - startchip = probe_flash(&registered_masters[j], startchip, &flashes[chipcount], 0, options.chip_to_probe); - if (startchip == -1) - break; - chipcount++; - startchip++; - } + all_matched_count = flashrom_flash_probe_v2(&flashes[0], &all_matched_names, + NULL, options.chip_to_probe); + if (all_matched_count == -1) { + /* -1 is the ret code which means "something went wrong". + * Multiple match and no match are different ret codes. + * More details about the error were printed during actual probing. */ + msg_cerr("Error: probing failed.\n"); + ret = 1; + goto out_shutdown; } - if (chipcount > 1) { + if (all_matched_count > 1) { msg_cinfo("Multiple flash chip definitions match the detected chip(s): \"%s\"", flashes[0].chip->name); - for (i = 1; i < chipcount; i++) - msg_cinfo(", \"%s\"", flashes[i].chip->name); + for (int ind = 1; ind < all_matched_count; ind++) + msg_cinfo(", \"%s\"", all_matched_names[ind]); msg_cinfo("\nPlease specify which chip definition to use with the -c <chipname> option.\n"); ret = 1; goto out_shutdown; - } else if (!chipcount) { + } else if (!all_matched_count) { msg_cinfo("No EEPROM/flash device found.\n"); if (!options.force || !options.chip_to_probe) { msg_cinfo("Note: flashrom can never write if the flash chip isn't found " @@ -1253,6 +1254,8 @@ if (compatible_masters > 1) msg_cinfo("More than one compatible controller found for the requested flash " "chip, using the first one.\n"); + + int startchip = -1; for (j = 0; j < registered_master_count; j++) { mst = &registered_masters[j]; startchip = probe_flash(mst, 0, &flashes[0], 1, options.chip_to_probe); @@ -1535,10 +1538,11 @@ out_shutdown: flashrom_programmer_shutdown(NULL); out: - for (i = 0; i < chipcount; i++) { - flashrom_layout_release(flashes[i].default_layout); - free(flashes[i].chip); + for (int ind = 0; ind < all_matched_count; ind++) { + flashrom_layout_release(flashes[ind].default_layout); + free(flashes[ind].chip); } + flashrom_data_free(all_matched_names); free_options(&options); ret |= close_logfile(); diff --git a/include/libflashrom.h b/include/libflashrom.h index f30ad6b..ca9a5e3 100644 --- a/include/libflashrom.h +++ b/include/libflashrom.h @@ -279,6 +279,45 @@ * or 1 on any other error. */ int flashrom_flash_probe(struct flashrom_flashctx **flashctx, const struct flashrom_programmer *flashprog, const char *chip_name); + +/** + * @brief Probe for a flash chip, v2 + * + * Probes for a flash chip and returns a flash context, that can be used + * later with flash chip and @ref flashrom-ops "image operations", if + * exactly one matching chip is found. + * + * Returns the list of names for all chips that matched, and the count of + * how many chips matched. + * + * Memory for the list of chips is dynamically allocated according to the + * number of chips found, and always needs to be freed with flashrom_data_free + * afterwards (including when no matches found or error happened). + * + * Note that if chip_name param is set, then probing happens only once, only + * for this one requested chip name. So the number of matches that can be + * returned in this case will be either 1 or 0 (and -1 for error). + * + * @param[out] flashctx Points to a struct flashrom_flashctx + * that will be set if exactly one chip is found. *flashctx + * has to be freed by the caller with @ref flashrom_flash_release. + * @param[out] all_matched_names pointer to an array containing the names of all chips + * that were successfully probed, terminated with a NULL pointer. + * If no chips are found, the returned array contains + * a single NULL element. Callers must free the array once unused + * by calling `flashrom_data_free`. + * @param[in] flashprog The flash programmer used to access the chip, + * currently unused. + * @param[in] chip_name Name of a chip to probe for, or NULL to probe for + * all known chips. + * @return the number of matched chips (which can be 0) on success, + * -1 if error happened during probing. + */ +int flashrom_flash_probe_v2(struct flashrom_flashctx *flashctx, + const char *** const all_matched_names, + const struct flashrom_programmer *flashprog, + const char *chip_name); + /** * @brief Returns the size of the specified flash chip in bytes. * diff --git a/libflashrom.c b/libflashrom.c index d6bd098..a45e0d1 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -329,8 +329,6 @@ return programmer_shutdown(); } -/* TODO: flashrom_programmer_capabilities()? */ - int flashrom_flash_probe(struct flashrom_flashctx **const flashctx, const struct flashrom_programmer *const flashprog, const char *const chip_name) @@ -363,6 +361,56 @@ return ret; } +int flashrom_flash_probe_v2(struct flashrom_flashctx *flashctx, + const char *** const all_matched_names, + const struct flashrom_programmer *flashprog, + const char *chip_name) +{ + int startchip; + unsigned int all_matched_count = 0; // start with no match found + const char **matched_names = calloc(flashchips_size + 1, sizeof(char*)); + + for (int i = 0; i < registered_master_count; i++) { + startchip = 0; + while (all_matched_count < flashchips_size) { + struct flashrom_flashctx second_flashctx = { 0, }; // used for second and more matches + struct flashctx *context_for_probing = (all_matched_count > 0) ? &second_flashctx : flashctx; + startchip = probe_flash(&registered_masters[i], startchip, context_for_probing, 0, chip_name); + + if (startchip == -1) + break; + + matched_names[all_matched_count] = context_for_probing->chip->name; + all_matched_count++; + startchip++; + + if (all_matched_count > 1) { + /* It's used for the second and subsequent probing. */ + flashrom_layout_release(second_flashctx.default_layout); + free(second_flashctx.chip); + } + } + } + + matched_names[all_matched_count] = NULL; + matched_names = realloc(matched_names, (all_matched_count + 1) * sizeof(char*)); + *all_matched_names = matched_names; + + /* The return value should be the number of matched chips, or -1 on error. + * However, currently the error is never returned, because probe_flash return code + * is -1 for both cases of error and no match found, so there is no way + * to distinguish between probing error (e.g. error talking to hw, or out of memory) + * and no match. + * (no match can happen if chip is not in our database, even if hw work perfectly). + * + * TODO improve probe_flash return code to distinguish between + * probing error and no match found. */ + int ret = (int) all_matched_count; + + return ret; +} + + size_t flashrom_flash_getsize(const struct flashrom_flashctx *const flashctx) { return flashctx->chip->total_size * 1024; diff --git a/libflashrom.map b/libflashrom.map index fc6724a..2b757e5 100644 --- a/libflashrom.map +++ b/libflashrom.map @@ -6,6 +6,7 @@ flashrom_flash_erase; flashrom_flash_getsize; flashrom_flash_probe; + flashrom_flash_probe_v2; flashrom_flash_release; flashrom_image_read; flashrom_image_verify; diff --git a/tests/dummyflasher.c b/tests/dummyflasher.c index 9878bef..0faecb9 100644 --- a/tests/dummyflasher.c +++ b/tests/dummyflasher.c @@ -42,6 +42,57 @@ run_probe_lifecycle(state, &dummy_io, &programmer_dummy, "bus=spi,emulate=W25Q128FV", "W25Q128.V"); } +void dummy_probe_v2_one_match_for_W25Q128FV(void **state) +{ + struct io_mock_fallback_open_state dummy_fallback_open_state = { + .noc = 0, + .paths = { NULL }, + }; + const struct io_mock dummy_io = { + .fallback_open_state = &dummy_fallback_open_state, + }; + + const char *expected_matched_names[1] = {"W25Q128.V"}; + run_probe_v2_lifecycle(state, &dummy_io, &programmer_dummy, "bus=spi,emulate=W25Q128FV", + NULL, /* any chip name */ + expected_matched_names, 1); +} + +void dummy_probe_v2_six_matches_for_MX25L6436(void **state) +{ + struct io_mock_fallback_open_state dummy_fallback_open_state = { + .noc = 0, + .paths = { NULL }, + }; + const struct io_mock dummy_io = { + .fallback_open_state = &dummy_fallback_open_state, + }; + + const char *expected_matched_names[6] = {"MX25L6405", + "MX25L6405D", + "MX25L6406E/MX25L6408E", + "MX25L6436E/MX25L6445E/MX25L6465E", + "MX25L6473E", + "MX25L6473F"}; + run_probe_v2_lifecycle(state, &dummy_io, &programmer_dummy, "bus=spi,emulate=MX25L6436", + NULL, /* any chip name */ + expected_matched_names, 6); +} + +void dummy_probe_v2_no_matches_found(void **state) +{ + struct io_mock_fallback_open_state dummy_fallback_open_state = { + .noc = 0, + .paths = { NULL }, + }; + const struct io_mock dummy_io = { + .fallback_open_state = &dummy_fallback_open_state, + }; + + run_probe_v2_lifecycle(state, &dummy_io, &programmer_dummy, "bus=spi,emulate=MX25L6436", + "NONEXISTENT", NULL /* no matched names */, 0); +} + void dummy_probe_variable_size_test_success(void **state) { struct io_mock_fallback_open_state dummy_fallback_open_state = { @@ -163,6 +214,9 @@ #else SKIP_TEST(dummy_basic_lifecycle_test_success) SKIP_TEST(dummy_probe_lifecycle_test_success) + SKIP_TEST(dummy_probe_v2_one_match_for_W25Q128FV) + SKIP_TEST(dummy_probe_v2_six_matches_for_MX25L6436) + SKIP_TEST(dummy_probe_v2_no_matches_found) SKIP_TEST(dummy_probe_variable_size_test_success) SKIP_TEST(dummy_init_fails_unhandled_param_test_success) SKIP_TEST(dummy_init_success_invalid_param_test_success) diff --git a/tests/lifecycle.c b/tests/lifecycle.c index 95c4249..a92e93e 100644 --- a/tests/lifecycle.c +++ b/tests/lifecycle.c @@ -17,22 +17,61 @@ static void probe_chip(const struct programmer_entry *prog, struct flashrom_programmer *flashprog, - const char *const chip_name) + const char *const chip_name, + const char **expected_matched_names, /* unused in probe v1 */ + unsigned int expected_matched_count /* unused in probe v1 */) { struct flashrom_flashctx *flashctx; printf("Testing flashrom_flash_probe for programmer=%s, chip=%s ... \n", prog->name, chip_name); + assert_int_equal(0, flashrom_flash_probe(&flashctx, flashprog, chip_name)); + if (chip_name) + assert_int_equal(0, strcmp(chip_name, flashctx->chip->name)); + printf("... flashrom_flash_probe for programmer=%s successful\n", prog->name); flashrom_flash_release(flashctx); /* cleanup */ } +static void probe_chip_v2(const struct programmer_entry *prog, + struct flashrom_programmer *flashprog, + const char *const chip_name, + const char **expected_matched_names, + unsigned int expected_matched_count) +{ + struct flashrom_flashctx flashctx = { 0 }; + const char **all_matched_names = NULL; + + printf("Testing flashrom_flash_probe_v2 for programmer=%s, chip=%s ... \n", prog->name, chip_name); + assert_int_equal(expected_matched_count, flashrom_flash_probe_v2(&flashctx, &all_matched_names, + flashprog, chip_name)); + + for (unsigned int i = 0; i < expected_matched_count; i++) + assert_int_equal(0, strcmp(expected_matched_names[i], all_matched_names[i])); + + assert_null(all_matched_names[expected_matched_count]); + + if (chip_name && expected_matched_count > 0) + assert_int_equal(0, strcmp(chip_name, flashctx.chip->name)); + + printf("... flashrom_flash_probe_v2 for programmer=%s successful\n", prog->name); + + /* cleanup */ + flashrom_data_free(all_matched_names); + flashrom_layout_release(flashctx.default_layout); + free(flashctx.chip); +} + static void run_lifecycle(void **state, const struct io_mock *io, const struct programmer_entry *prog, const char *param, const char *const chip_name, + const char **expected_matched_names, + unsigned int expected_matched_count, void (*action)(const struct programmer_entry *prog, struct flashrom_programmer *flashprog, - const char *const chip_name)) + const char *const chip_name, + const char **expected_matched_names, + unsigned int expected_matched_count)) { (void) state; /* unused */ @@ -45,7 +84,7 @@ printf("... flashrom_programmer_init for programmer=%s successful\n", prog->name); if (action) - action(prog, flashprog, chip_name); + action(prog, flashprog, chip_name, expected_matched_names, expected_matched_count); printf("Testing flashrom_programmer_shutdown for programmer=%s ...\n", prog->name); assert_int_equal(0, flashrom_programmer_shutdown(flashprog)); @@ -59,7 +98,9 @@ { /* Basic lifecycle only does init and shutdown, * so neither chip name nor action is needed. */ - run_lifecycle(state, io, prog, param, NULL /* chip_name */, NULL /* action */); + run_lifecycle(state, io, prog, param, NULL /* chip_name */, + NULL /* expected_matched_names, */, 0 /* expected_matched_count, */, + NULL /* action */); } void run_probe_lifecycle(void **state, const struct io_mock *io, @@ -67,7 +108,20 @@ { /* Each probe lifecycle should run independently, without cache. */ clear_spi_id_cache(); - run_lifecycle(state, io, prog, param, chip_name, &probe_chip); + run_lifecycle(state, io, prog, param, chip_name, + NULL /* expected_matched_names, */, 0 /* expected_matched_count, */, + &probe_chip); +} + +void run_probe_v2_lifecycle(void **state, const struct io_mock *io, + const struct programmer_entry *prog, const char *param, + const char *const chip_name, + const char **expected_matched_names, unsigned int expected_matched_count) +{ + /* Each probe lifecycle should run independently, without cache. */ + clear_spi_id_cache(); + run_lifecycle(state, io, prog, param, chip_name, + expected_matched_names, expected_matched_count, &probe_chip_v2); } void run_init_error_path(void **state, const struct io_mock *io, const struct programmer_entry *prog, diff --git a/tests/lifecycle.h b/tests/lifecycle.h index b4c2fbe..1565a69 100644 --- a/tests/lifecycle.h +++ b/tests/lifecycle.h @@ -34,6 +34,11 @@ void run_probe_lifecycle(void **state, const struct io_mock *io, const struct programmer_entry *prog, const char *param, const char *const chip_name); +void run_probe_v2_lifecycle(void **state, const struct io_mock *io, + const struct programmer_entry *prog, const char *param, + const char *const chip_name, + const char **expected_matched_names, unsigned int expected_matched_count); + void run_init_error_path(void **state, const struct io_mock *io, const struct programmer_entry *prog, const char *param, const int error_code); #endif /* __LIFECYCLE_H__ */ diff --git a/tests/tests.c b/tests/tests.c index 2a79d32..c43af37 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -504,6 +504,9 @@ const struct CMUnitTest lifecycle_tests[] = { cmocka_unit_test(dummy_basic_lifecycle_test_success), cmocka_unit_test(dummy_probe_lifecycle_test_success), + cmocka_unit_test(dummy_probe_v2_one_match_for_W25Q128FV), + cmocka_unit_test(dummy_probe_v2_six_matches_for_MX25L6436), + cmocka_unit_test(dummy_probe_v2_no_matches_found), cmocka_unit_test(dummy_probe_variable_size_test_success), cmocka_unit_test(dummy_init_fails_unhandled_param_test_success), cmocka_unit_test(dummy_init_success_invalid_param_test_success), diff --git a/tests/tests.h b/tests/tests.h index e87c00c..6ec4056 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -53,6 +53,9 @@ /* lifecycle.c */ void dummy_basic_lifecycle_test_success(void **state); void dummy_probe_lifecycle_test_success(void **state); +void dummy_probe_v2_one_match_for_W25Q128FV(void **state); +void dummy_probe_v2_six_matches_for_MX25L6436(void **state); +void dummy_probe_v2_no_matches_found(void **state); void dummy_probe_variable_size_test_success(void **state); void dummy_init_fails_unhandled_param_test_success(void **state); void dummy_init_success_invalid_param_test_success(void **state); -- To view, visit https://review.coreboot.org/c/flashrom/+/87341?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Idfcf377a8071e22028ba98515f08495ed2a6e9f0 Gerrit-Change-Number: 87341 Gerrit-PatchSet: 9 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Dmitry Zhadinets <dzhadinets(a)gmail.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

[XS] Change in flashrom[main]: Add more delay time for jlink power on feature to stabilize the LDO a...
by attila-v (Code Review) 11 May '25

11 May '25

Attention is currently required from: Anastasia Klimchuk. attila-v has posted comments on this change by attila-v. ( https://review.coreboot.org/c/flashrom/+/86085?usp=email ) Change subject: Add more delay time for jlink power on feature to stabilize the LDO and the decoupling capacitors ...................................................................... Patch Set 2: (1 comment) File jlink_spi.c: https://review.coreboot.org/c/flashrom/+/86085/comment/a921dee2_8bbb55bf?us… : PS2, Line 469: internal_sleep(100000); > hm, my first thought was that this code patch would be a bit of an overhead. […] 3 I tested this delay many-many times, because it is necessary for my work now. -- To view, visit https://review.coreboot.org/c/flashrom/+/86085?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ic2dd94e99ac6ffa17a009b8488ce027698ae2c28 Gerrit-Change-Number: 86085 Gerrit-PatchSet: 2 Gerrit-Owner: attila-v Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: attila-v Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 11 May 2025 17:44:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: attila-v

1 0

[XS] Change in flashrom[main]: Add more delay time for jlink power on feature to stabilize the LDO a...
by attila-v (Code Review) 11 May '25

11 May '25

Attention is currently required from: Anastasia Klimchuk. attila-v has posted comments on this change by attila-v. ( https://review.coreboot.org/c/flashrom/+/86085?usp=email ) Change subject: Add more delay time for jlink power on feature to stabilize the LDO and the decoupling capacitors ...................................................................... Patch Set 2: (1 comment) File jlink_spi.c: https://review.coreboot.org/c/flashrom/+/86085/comment/6629bbed_419f6e9b?us… : PS2, Line 469: internal_sleep(100000); > Thank you for details! In this case, it can be another programmer param? […] hm, my first thought was that this code patch would be a bit of an overhead. there is set a limit of the voltage level in the code (1.2V), which is usually presents at the original delay time; your code shows this value at the starting point. of course, a little bit later this voltage will be at the nominal value, but your code will not inform me about it. I think if you want to expand the program with a new feature, it should not be an optional parameter for the delay, but wait for a value close to the nominal one (e.g. 3V for LDO 3.3V). I really respect your work, the flashrom is a great project, but I think it will be enoughwthis small delay patch for now, nobody will sense this little 0.1 seconds delay which I think is definitely enough. -- To view, visit https://review.coreboot.org/c/flashrom/+/86085?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ic2dd94e99ac6ffa17a009b8488ce027698ae2c28 Gerrit-Change-Number: 86085 Gerrit-PatchSet: 2 Gerrit-Owner: attila-v Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: attila-v Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 11 May 2025 17:39:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: attila-v

1 0

[XS] Change in flashrom[main]: Add more delay time for jlink power on feature to stabilize the LDO a...
by Anastasia Klimchuk (Code Review) 08 May '25

08 May '25

Attention is currently required from: attila-v. Anastasia Klimchuk has posted comments on this change by attila-v. ( https://review.coreboot.org/c/flashrom/+/86085?usp=email ) Change subject: Add more delay time for jlink power on feature to stabilize the LDO and the decoupling capacitors ...................................................................... Patch Set 2: (1 comment) File jlink_spi.c: https://review.coreboot.org/c/flashrom/+/86085/comment/8663c328_5770a1b2?us… : PS2, Line 469: internal_sleep(100000); Thank you for details! In this case, it can be another programmer param? What I mean is, for example currently jlink_spi has spispeed, serial etc (https://flashrom.org/classic_cli_manpage.html#jlink-spi-programmer) and it can be another param like that. Do you have interest to update the patch to implement new param? Another option if you not interested to write code, I think I can do it, but then I will need your help with few things. Again, that's only if you are interested. You can tell me what you think! including, if you don't want to do anything at all :) Concretely, I had a look today, few things that I will need help 1) naming the param - I produced `delay_after_power_on` in my head, but it feels long :( 2) info for manpage, I made this text, is it correct? or, any other suggestions > optional param: delay in milliseconds which is typically needed after enabling target power. If not set, default value is 10. Larger values can be needed for LDO-s and decoupling capacitors. Valid values between 10 and 1000. 3) testing - you currently have the setup to test the non-default value? -- To view, visit https://review.coreboot.org/c/flashrom/+/86085?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ic2dd94e99ac6ffa17a009b8488ce027698ae2c28 Gerrit-Change-Number: 86085 Gerrit-PatchSet: 2 Gerrit-Owner: attila-v Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: attila-v Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: attila-v Gerrit-Comment-Date: Thu, 08 May 2025 13:02:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: attila-v

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit