Attention is currently required from: Sergii Dmytruk.
Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84423?usp=email )
Change subject: dummyflasher: Enable higher frequency emulation, add docs and tests
......................................................................
Patch Set 3:
(5 comments)
Patchset:
PS3:
Thanks for suggestions for documentation, that's so useful.
I also added tests to cover freq initialisation code.
File doc/classic_cli_manpage.rst:
https://review.coreboot.org/c/flashrom/+/84423/comment/7e1d67f3_6ba3ebdc?us… :
PS2, Line 619: **Frequency**
> I'd document that the range is [1Hz, 8000Mhz] and there is no delay by default.
Done
https://review.coreboot.org/c/flashrom/+/84423/comment/2f589e96_22e6980e?us… :
PS2, Line 620: emulated
> nit: […]
Done
https://review.coreboot.org/c/flashrom/+/84423/comment/6533d600_90cf7cc6?us… :
PS2, Line 624: The delay is calculated based on the assumption that we only work with bytes and transfer at 1 bit/Hz::
> ```suggestion […]
Done
File dummyflasher.c:
https://review.coreboot.org/c/flashrom/+/84423/comment/76a14afb_857ab910?us… :
PS2, Line 1169: if (freq == 0) {
> A similar check could be added for an upper bound. […]
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/84423?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: Iaca5d95f8f977bf0c2283c6458d8977e6ce70251
Gerrit-Change-Number: 84423
Gerrit-PatchSet: 3
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Sat, 21 Sep 2024 09:49:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Attention is currently required from: Anastasia Klimchuk.
Hello Sergii Dmytruk, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/84423?usp=email
to look at the new patch set (#3).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: dummyflasher: Enable higher frequency emulation, add docs and tests
......................................................................
dummyflasher: Enable higher frequency emulation, add docs and tests
The patch adds a section on a manpage to explain the freq
parameter in dummyflasher, and tests for various valid and invalid
values of freq parameter.
Co-developed-by: Anastasia Klimchuk <aklm(a)flashrom.org>
Co-developed-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Change-Id: Iaca5d95f8f977bf0c2283c6458d8977e6ce70251
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
---
M doc/classic_cli_manpage.rst
M dummyflasher.c
M tests/dummyflasher.c
M tests/tests.c
M tests/tests.h
5 files changed, 41 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/23/84423/3
--
To view, visit https://review.coreboot.org/c/flashrom/+/84423?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: Iaca5d95f8f977bf0c2283c6458d8977e6ce70251
Gerrit-Change-Number: 84423
Gerrit-PatchSet: 3
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Attention is currently required from: Anastasia Klimchuk.
Sergii Dmytruk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84423?usp=email )
Change subject: dummyflasher: Enable higher frequency emulation
......................................................................
Patch Set 2:
(4 comments)
File doc/classic_cli_manpage.rst:
https://review.coreboot.org/c/flashrom/+/84423/comment/68b97887_d8199de5?us… :
PS2, Line 619: **Frequency**
I'd document that the range is [1Hz, 8000Mhz] and there is no delay by default.
https://review.coreboot.org/c/flashrom/+/84423/comment/354aea8a_2372dd49?us… :
PS2, Line 620: emulated
nit:
```suggestion
Frequency can be specified in ``Hz`` (default), ``KHz``, or ``MHz`` (not case sensitive).
```
https://review.coreboot.org/c/flashrom/+/84423/comment/04be9687_e2e02135?us… :
PS2, Line 624: The delay is calculated based on the assumption that we only work with bytes and transfer at 1 bit/Hz::
```suggestion
The delay of an SPI command is proportional to the number of bits send over SPI bus in both directions and is calculated based on the assumption that we transfer at 1 bit/Hz::
```
File dummyflasher.c:
https://review.coreboot.org/c/flashrom/+/84423/comment/3274ac80_ff09a223?us… :
PS2, Line 1169: if (freq == 0) {
A similar check could be added for an upper bound. It's quite confusing to get a zero delay all of a sudden.
--
To view, visit https://review.coreboot.org/c/flashrom/+/84423?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: Iaca5d95f8f977bf0c2283c6458d8977e6ce70251
Gerrit-Change-Number: 84423
Gerrit-PatchSet: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Fri, 20 Sep 2024 16:33:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Aarya, Nikolai Artemiev, Sergii Dmytruk.
Anastasia Klimchuk has posted comments on this change by Sergii Dmytruk. ( https://review.coreboot.org/c/flashrom/+/84102?usp=email )
Change subject: Complete and fix progress feature implementation for all operations
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS1:
> A comment to fix the tests
I fixed the failing test, `spi_read_chunked_test_success` and also renamed it to `default_spi_read_test_success` because a higher level was needed to invoke progress
Then, it turns out that `setup_progress_from_layout_and_diff` is causing tests in tests/chip.c to timeout. So I decided to split the commit and introduce `setup_progress_from_layout_and_diff` in the next commit (which is not ready yet because tests timeout).
I think `setup_progress_from_layout` is already enough to display progress, it's not optimal but it works. It can be improved in the next commit.
--
To view, visit https://review.coreboot.org/c/flashrom/+/84102?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: If1e40fc97f443c4f0c0501cef11cff1f3f84c051
Gerrit-Change-Number: 84102
Gerrit-PatchSet: 3
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.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: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Fri, 20 Sep 2024 13:59:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Attention is currently required from: Sergii Dmytruk.
Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84423?usp=email )
Change subject: dummyflasher: Enable higher frequency emulation
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS1:
> Note: need to ensure this change works together with CB:84102 and all tests pass at the end of a cha […]
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/84423?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: Iaca5d95f8f977bf0c2283c6458d8977e6ce70251
Gerrit-Change-Number: 84423
Gerrit-PatchSet: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Fri, 20 Sep 2024 13:52:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Attention is currently required from: Aarya, Nikolai Artemiev, Sergii Dmytruk.
Anastasia Klimchuk has uploaded a new patch set (#3) to the change originally created by Sergii Dmytruk. ( https://review.coreboot.org/c/flashrom/+/84102?usp=email )
The following approvals got outdated and were removed:
Verified-1 by build bot (Jenkins)
Change subject: Complete and fix progress feature implementation for all operations
......................................................................
Complete and fix progress feature implementation for all operations
Original progress reporting implemented in CB:49643 and it has some
issues, for example:
size_t start_address = start;
size_t end_address = len - start;
End address is anything but length minus start address.
update_progress(flash,
FLASHROM_PROGRESS_READ,
/*current*/ start - start_address + to_read,
/*total*/ end_address);
Total should just be length if that's how current value is computed.
---
libflashrom needs to know total size ahead of time.
That's init_progress() and changed update_progress().
It also needs to store the last current value to be able to update it.
That's stage_progress in flashrom_flashctx.
Measuring accurately amount of data which will be read/erased/written
isn't easy because things can be skipped as optimizations. Current
values are probably off, there are TODO/FIXME comments.
---
CLI shares terminal with the rest of the code and has to maintain more
state to handle that reasonably well.
The progress doesn't just dump lots of stuff on the screen which
probably won't fly because of CB:64668, but it's not hard to adjust
this.
---
A script to test the CLI:
\#!/bin/bash
t=${1:-rewW}
shift
if [[ $t =~ r ]]; then
echo ">>> READ"
./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz -r dump.rom --progress "$@"
echo
fi
if [[ $t =~ e ]]; then
echo ">>> ERASE"
./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz -E --progress "$@"
echo
fi
if [[ $t =~ w ]]; then
echo ">>> WRITE (without erase)"
dd if=/dev/zero of=zero.rom bs=1M count=16 2> /dev/null
./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz -w zero.rom --progress "$@"
echo
fi
if [[ $t =~ W ]]; then
echo ">>> WRITE (with erase)"
dd if=/dev/zero of=zero.rom bs=1M count=16 2> /dev/null
dd if=/dev/random of=random.rom bs=1M count=16 2> /dev/null
./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz,image=random.rom -w zero.rom --progress "$@"
echo
fi
Co-developed-by: Anastasia Klimchuk <aklm(a)flashrom.org>
Co-developed-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Change-Id: If1e40fc97f443c4f0c0501cef11cff1f3f84c051
Signed-off-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
---
M 82802ab.c
M at45db.c
M cli_classic.c
M cli_output.c
M dediprog.c
M doc/release_notes/devel.rst
M en29lv640b.c
M erasure_layout.c
M flashrom.c
M include/flash.h
M it87spi.c
M jedec.c
M libflashrom.c
M linux_mtd.c
M parade_lspcon.c
M realtek_mst_i2c_spi.c
M spi.c
M spi25.c
M sst28sf040.c
M tests/chip.c
M tests/spi25.c
M tests/tests.c
M tests/tests.h
23 files changed, 285 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/02/84102/3
--
To view, visit https://review.coreboot.org/c/flashrom/+/84102?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: If1e40fc97f443c4f0c0501cef11cff1f3f84c051
Gerrit-Change-Number: 84102
Gerrit-PatchSet: 3
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.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: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/84423?usp=email
to look at the new patch set (#2).
Change subject: dummyflasher: Enable higher frequency emulation
......................................................................
dummyflasher: Enable higher frequency emulation
The patch also adds a section on a manpage to explain the freq
parameter in dummyflasher.
Co-developed-by: Anastasia Klimchuk <aklm(a)flashrom.org>
Co-developed-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Change-Id: Iaca5d95f8f977bf0c2283c6458d8977e6ce70251
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
---
M doc/classic_cli_manpage.rst
M dummyflasher.c
2 files changed, 14 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/23/84423/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/84423?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: Iaca5d95f8f977bf0c2283c6458d8977e6ce70251
Gerrit-Change-Number: 84423
Gerrit-PatchSet: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Attention is currently required from: Anastasia Klimchuk.
Hello Sergii Dmytruk, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/84421?usp=email
to look at the new patch set (#2).
Change subject: dediprog: Fix comment about usb transfer size
......................................................................
dediprog: Fix comment about usb transfer size
Co-developed-by: Anastasia Klimchuk <aklm(a)flashrom.org>
Co-developed-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Change-Id: I1ad7f89b0a6c91907440e3897ac262bd82f846d5
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
---
M dediprog.c
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/84421/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/84421?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: I1ad7f89b0a6c91907440e3897ac262bd82f846d5
Gerrit-Change-Number: 84421
Gerrit-PatchSet: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>