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
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>
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
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
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>
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 = ®istered_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>
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
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
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