Peter Marheine has submitted this change. ( https://review.coreboot.org/c/flashrom/+/81608?usp=email )
Change subject: reduce DELAY_MINIMUM_SLEEP_US to 100 by default
......................................................................
reduce DELAY_MINIMUM_SLEEP_US to 100 by default
This makes flashrom sleep more eagerly rather than busy-waiting,
observing that most delays in flashrom are either less than 100
microseconds (barely enough time to get any work done, even on a fast
machine) or much more than 1 millisecond (very wasteful to busy-loop).
Since we believe most systems offer good timer resolution that should
provide sleep latency on the order of 100 microseconds, this is a
reasonable default.
For DOS, the default is set to 50ms because the best available timing
source on DOS only ticks at about 20 Hz.
Signed-off-by: Peter Marheine <pmarheine(a)chromium.org>
Change-Id: I0f431d240c670446218b14811ef62a34e4c83da2
Reviewed-on: https://review.coreboot.org/c/flashrom/+/81608
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M meson_cross/i586_djgpp_dos.txt
M meson_options.txt
2 files changed, 3 insertions(+), 1 deletion(-)
Approvals:
build bot (Jenkins): Verified
Anastasia Klimchuk: Looks good to me, approved
diff --git a/meson_cross/i586_djgpp_dos.txt b/meson_cross/i586_djgpp_dos.txt
index 3d97aab..a1f2401 100644
--- a/meson_cross/i586_djgpp_dos.txt
+++ b/meson_cross/i586_djgpp_dos.txt
@@ -30,6 +30,8 @@
[project options]
tests = 'disabled'
ich_descriptors_tool = 'disabled'
+# DOS time resolution is only about 50ms
+delay_minimum_sleep_us = 50000
[properties]
sys_root = '/usr/local/djgpp'
diff --git a/meson_options.txt b/meson_options.txt
index 0f56e26..6df95ba 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -21,6 +21,6 @@
option('documentation', type : 'feature', value : 'auto', description : 'build the html documentation')
option('ni845x_search_path', type : 'string', value : 'C:\Program Files (x86)\National Instruments\Ni-845x\MS Visual C',
description : 'Path to search for the proprietary ni845x library and header (32-bit Windows only)')
-option('delay_minimum_sleep_us', type : 'integer', min : 0, value : 100000,
+option('delay_minimum_sleep_us', type : 'integer', min : 0, value : 100,
description : 'Minimum time in microseconds to suspend execution for (rather than polling) when a delay is required.'
+ ' Larger values may perform better on machines with low timer resolution, at the cost of increased power.')
--
To view, visit https://review.coreboot.org/c/flashrom/+/81608?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: I0f431d240c670446218b14811ef62a34e4c83da2
Gerrit-Change-Number: 81608
Gerrit-PatchSet: 3
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
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>
Attention is currently required from: Aarya, Anastasia Klimchuk, Peter Marheine.
Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84078?usp=email )
Change subject: tests: Check verify op completed in full if chip memory modified
......................................................................
Patch Set 2:
(2 comments)
Patchset:
PS2:
There are a few changes in the new patchset:
1) Stronger requirement on verify: verify records cleared after each modification. In cases when the memory area is erased, and then written, we want to ensure verification happens after write, not just after erase.
2) FLASHROM_FLAG_VERIFY_AFTER_WRITE turned ON for all usual cases
3) FLASHROM_FLAG_VERIFY_AFTER_WRITE turned OFF for protected region test cases, verify op called manually after write. (because protected region not supposed to be modified)
4) protected region in the test changed to be only write-protected , read allowed, to be able to do verification
5) fixed a bug in post-processing of unaligned region when the end region needs to be extended. newcontents are supposed to be restored, but there was 1-off error.
How do 3 and 4 work in real life? Is the test close enough to real life?
Commit Message:
https://review.coreboot.org/c/flashrom/+/84078/comment/e7c9094a_a0bf0fe5?us… :
PS1, Line 18: Note: at the moment the test is not fully working, the following
: write test cases fail (all erase test cases pass):
> Looks like it might be a fencepost error (possibly treating the upper bound of a region as exclusive […]
This specific error was because verify_after_write flag was off! It is turned on in cli_classic, but tests are calling libflashrom directly, so the flag needs to be explicitly turned on.
The only verification that happened without the flag, was coming from after the erase operation, because erase ignores the flags :)
I found a fencepost bug in the other place though! (in erasure_layout.c)
--
To view, visit https://review.coreboot.org/c/flashrom/+/84078?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: I3c5d55a0deb20f23f4072caac8c0dce04cc98fd4
Gerrit-Change-Number: 84078
Gerrit-PatchSet: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Mon, 02 Sep 2024 12:39:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Attention is currently required from: Aarya, Peter Marheine.
Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84078?usp=email )
Change subject: tests: Check verify op completed in full if chip memory modified
......................................................................
Patch Set 2:
(2 comments)
File tests/erase_func_algo.c:
https://review.coreboot.org/c/flashrom/+/84078/comment/e8a9b973_fbce8b31?us… :
PS1, Line 60: uint8_t
> Since this is true/false, just use `bool`.
Done
https://review.coreboot.org/c/flashrom/+/84078/comment/2c96901a_682f053a?us… :
PS1, Line 77: == 0x1
> I'd rather use implicit boolean conversion throughout (`if (g_state.was_modified[start])`).
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/84078?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: I3c5d55a0deb20f23f4072caac8c0dce04cc98fd4
Gerrit-Change-Number: 84078
Gerrit-PatchSet: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Mon, 02 Sep 2024 12:21:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Attention is currently required from: Anastasia Klimchuk.
Hello Peter Marheine, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/84078?usp=email
to look at the new patch set (#2).
The following approvals got outdated and were removed:
Verified-1 by build bot (Jenkins)
Change subject: tests: Check verify op completed in full if chip memory modified
......................................................................
tests: Check verify op completed in full if chip memory modified
The patch adds new functionality to the test: tracking the areas of
chip memory that were modified (i.e. by erase or write operation),
and then checking those areas were completely covered by verify
operation.
The test operates over the mock chip memory of 16 bytes, so it is
possible to track each byte which was modified, and assert that is
has been verified afterwards.
Adding the test found a bug which is fixed in this commit.
Post-cleanup after processing unaligned region for the case when end
region needs to be extended to align with erase block. Writing was
done correctly, but post-processing of newcontents could go wrong,
which would make verification appear false-negative.
Note: at the moment the test is not fully working, test cases #19
are both failing, for the reason they are very edge cases with
logical layout covering only a part of the chip, and also this
layout being unaligned and smaller than the smallest erase block.
Probably needs some clever hack for testing verification.
[ FAILED ] 2 test(s), listed below:
[ FAILED ] Erase test case #19
[ FAILED ] Write test case #19
Change-Id: I3c5d55a0deb20f23f4072caac8c0dce04cc98fd4
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
---
M erasure_layout.c
M tests/erase_func_algo.c
2 files changed, 119 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/78/84078/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/84078?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I3c5d55a0deb20f23f4072caac8c0dce04cc98fd4
Gerrit-Change-Number: 84078
Gerrit-PatchSet: 2
Gerrit-Owner: 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: Anastasia Klimchuk <aklm(a)chromium.org>
Attention is currently required from: Nikolai Artemiev, Stefan Reinauer, Victor Lim.
Anastasia Klimchuk has posted comments on this change by Victor Lim. ( https://review.coreboot.org/c/flashrom/+/84090?usp=email )
Change subject: flashchips: add GD25F256F
......................................................................
Patch Set 1:
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/84090/comment/f12ac706_6445cc8d?us… :
PS1, Line 10: Will send the datasheet.
Got it by email, you can remove this line, thank you!
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/84090/comment/3540b503_f38d7516?us… :
PS1, Line 8058: STATUS1, 7
But this bit (S7 on page 14) is marked as Reserved?
--
To view, visit https://review.coreboot.org/c/flashrom/+/84090?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: Ibbbbb8a55adbcbc2ee1785782c4eb3771d50c167
Gerrit-Change-Number: 84090
Gerrit-PatchSet: 1
Gerrit-Owner: Victor Lim <vlim(a)gigadevice.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Victor Lim <vlim(a)gigadevice.com>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Mon, 02 Sep 2024 04:02:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No