Attention is currently required from: Daniel Campello.
Hello build bot (Jenkins), Edward O'Callaghan, Peter Marheine,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/52876
to look at the new patch set (#2).
Change subject: flashrom_tester: update syntax for --wp-range
......................................................................
flashrom_tester: update syntax for --wp-range
TEST=cargo test
Signed-off-by: Daniel Campello <campello(a)chromium.org>
Change-Id: If17a40fba1f7d41e09e0163b353d1602c215c8db
---
M util/flashrom_tester/flashrom/src/cmd.rs
1 file changed, 4 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/76/52876/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/52876
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If17a40fba1f7d41e09e0163b353d1602c215c8db
Gerrit-Change-Number: 52876
Gerrit-PatchSet: 2
Gerrit-Owner: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk.
Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 )
Change subject: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c
......................................................................
Patch Set 10:
(1 comment)
File tests/init_shutdown.c:
https://review.coreboot.org/c/flashrom/+/51487/comment/9b9d3675_6c5fbf9c
PS10, Line 53: MEC1308_SIO_PORT1
> Yes, although that would mean including more header files here ... but maybe this is fine? […]
Just to add one comment, adding tests to code that has not been written with testing in mind is often going to involve refactoring, I think. In general, I suggest doing a 'refactoring' commit to get things in the right place then another commit to add your test (i.e. in that order). I don't think we should be afraid to modify the code as needed for tests.
--
To view, visit https://review.coreboot.org/c/flashrom/+/51487
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc
Gerrit-Change-Number: 51487
Gerrit-PatchSet: 10
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Simon Glass <sjg(a)chromium.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Tue, 04 May 2021 15:43:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Simon Glass <sjg(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Richard Hughes, Angel Pons, Anastasia Klimchuk, Patrick Rudolph.
Hello build bot (Jenkins), Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/49643
to look at the new patch set (#6).
Change subject: libflashrom: Return progress state to the library user
......................................................................
libflashrom: Return progress state to the library user
Projects using libflashrom like fwupd expect the user to wait for the operation
to complete. To avoid the user thinking the process has "hung" or "got stuck"
report back the progress complete of the erase, write and read operations.
Include a test for the dummy spi25 device.
Change-Id: I7197572bb7f19e3bdb2bde855d70a0f50fd3854c
Signed-off-by: Richard Hughes <richard(a)hughsie.com>
---
M 82802ab.c
M at45db.c
M cli_classic.c
M cli_output.c
M dediprog.c
M en29lv640b.c
M flash.h
M it87spi.c
M jedec.c
M libflashrom.c
M libflashrom.h
M linux_mtd.c
M lspcon_i2c_spi.c
M realtek_mst_i2c_spi.c
M spi.c
M spi25.c
M sst28sf040.c
M tests/spi25.c
M tests/tests.c
M tests/tests.h
20 files changed, 145 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/43/49643/6
--
To view, visit https://review.coreboot.org/c/flashrom/+/49643
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7197572bb7f19e3bdb2bde855d70a0f50fd3854c
Gerrit-Change-Number: 49643
Gerrit-PatchSet: 6
Gerrit-Owner: Richard Hughes <richard(a)hughsie.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Richard Hughes <hughsient(a)gmail.com>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Richard Hughes <richard(a)hughsie.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Daniel Campello.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52876 )
Change subject: flashrom_tester: update syntax for --wp-range
......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/52876/comment/ee305479_596bcdaa
PS1, Line 12:
Nit: Please remove the blank line.
--
To view, visit https://review.coreboot.org/c/flashrom/+/52876
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If17a40fba1f7d41e09e0163b353d1602c215c8db
Gerrit-Change-Number: 52876
Gerrit-PatchSet: 1
Gerrit-Owner: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Comment-Date: Tue, 04 May 2021 15:27:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Richard Hughes, Angel Pons, Anastasia Klimchuk, Patrick Rudolph.
Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/49643 )
Change subject: libflashrom: Return progress state to the library user
......................................................................
Patch Set 5:
(1 comment)
File lspcon_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/49643/comment/6b1e91c5_070d715f
PS5, Line 355: PAGE_SIZE
LSPCON_PAGE_SIZE ? commit cbe8a39ece4c228ffb23736da3f477ef3b82358c
--
To view, visit https://review.coreboot.org/c/flashrom/+/49643
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7197572bb7f19e3bdb2bde855d70a0f50fd3854c
Gerrit-Change-Number: 49643
Gerrit-PatchSet: 5
Gerrit-Owner: Richard Hughes <richard(a)hughsie.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Richard Hughes <hughsient(a)gmail.com>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Richard Hughes <richard(a)hughsie.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Tue, 04 May 2021 14:33:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Richard Hughes, Angel Pons, Anastasia Klimchuk, Patrick Rudolph.
Hello build bot (Jenkins), Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/49643
to look at the new patch set (#5).
Change subject: libflashrom: Return progress state to the library user
......................................................................
libflashrom: Return progress state to the library user
Projects using libflashrom like fwupd expect the user to wait for the operation
to complete. To avoid the user thinking the process has "hung" or "got stuck"
report back the progress complete of the erase, write and read operations.
Include a test for the dummy spi25 device.
Change-Id: I7197572bb7f19e3bdb2bde855d70a0f50fd3854c
Signed-off-by: Richard Hughes <richard(a)hughsie.com>
---
M 82802ab.c
M at45db.c
M cli_classic.c
M cli_output.c
M dediprog.c
M en29lv640b.c
M flash.h
M it87spi.c
M jedec.c
M libflashrom.c
M libflashrom.h
M linux_mtd.c
M lspcon_i2c_spi.c
M realtek_mst_i2c_spi.c
M spi.c
M spi25.c
M sst28sf040.c
M tests/spi25.c
M tests/tests.c
M tests/tests.h
20 files changed, 145 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/43/49643/5
--
To view, visit https://review.coreboot.org/c/flashrom/+/49643
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7197572bb7f19e3bdb2bde855d70a0f50fd3854c
Gerrit-Change-Number: 49643
Gerrit-PatchSet: 5
Gerrit-Owner: Richard Hughes <richard(a)hughsie.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Richard Hughes <hughsient(a)gmail.com>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Richard Hughes <richard(a)hughsie.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Nico Huber, Nikolai Artemiev.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52883 )
Change subject: RFC: flashchips.c: merge GD25B128B/GD25Q128B and GD25Q127C/GD25Q128C
......................................................................
Patch Set 1: Code-Review-1
(1 comment)
Patchset:
PS1:
> Further explanation of the reasons for this change: […]
Sorry, I'm not convinced. This looks like a workaround for something only downstream uses.
However, I agree that having to deal with multiple flash chip definitions is annoying. If what downstream needs is to ignore one of the flashchip definitions, why not add a command-line argument to specify flashchip entries to ignore? It would have the same semantics as `-C`. This feature would be useful for everyone, not just downstream.
--
To view, visit https://review.coreboot.org/c/flashrom/+/52883
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I688f2e15aef61afbec728a9a81094bee56d6fbfa
Gerrit-Change-Number: 52883
Gerrit-PatchSet: 1
Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Tue, 04 May 2021 10:09:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Nikolai Artemiev.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52883 )
Change subject: RFC: flashchips.c: merge GD25B128B/GD25Q128B and GD25Q127C/GD25Q128C
......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/52883/comment/1f113254_fa6eac11
PS1, Line 13: FEATAURE_QPI
what did the datasheets say between 'C' and 'B' ?
--
To view, visit https://review.coreboot.org/c/flashrom/+/52883
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I688f2e15aef61afbec728a9a81094bee56d6fbfa
Gerrit-Change-Number: 52883
Gerrit-PatchSet: 1
Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Tue, 04 May 2021 10:06:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Simon Buhrow, Edward O'Callaghan, Samir Ibradžić.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 )
Change subject: ft2232_spi.c: Implement spi_send_multicommand()
......................................................................
Patch Set 33:
(1 comment)
Patchset:
PS32:
> OK, I understand (I have/had got no idea of VM stuff).
>
> So in case of not using VM, the speed we gain because of less USB communication is consumed by the increased processing time needed to pack the packages?
The processing time is negligible. Just prefix your flashrom
command with `time`, you'll see that it actually does nothing
but waiting in either case.
If you could provide a total number how long it takes to fully
read/erase/write a chip, that could give us a clue (sorry if
this was already given and I missed it). I suspect the VM case
to be slower in general, so the speed increase of this patch
would just be time re-gained that was lost due to the virtuali-
zation in the first place. So far my theory.
I expect, but don't know, the individual packets to reach the FTDI
with some delay. I have not looked into libftdi deeply enough. Maybe
it waits for confirmation that the write reached the device before
ftdi_write_data() returns. If this is the case, we'd get a lot syn-
chronization overhead. Consider this:
send WREN packet -->
send WP packet -->
wait for acks <--
vs.
send WREN packet -->
wait for ack <--
send WP packet -->
wait for ack <--
Ignoring bandwidth, in the second example, it would take 2 round-
trip times instead of one. And the round-trip is influenced by the
virtualization. It depends a lot how the lower layers are implemen-
ted. What flashrom leaves to libftdi.
>
> Feel free to comment if you think this patch is worth to get merged.
I think it's nice work and even if it wouldn't ever increase the speed,
it would still be a nice example how to implement spi_send_multicommand 😊
But that it helps your use case without affecting others would be enough
reason to get it in anyway.
From my side, I'll just have to find the time for a final good look at
it, then it could be merged.
--
To view, visit https://review.coreboot.org/c/flashrom/+/40477
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a
Gerrit-Change-Number: 40477
Gerrit-PatchSet: 33
Gerrit-Owner: Simon Buhrow
Gerrit-Reviewer: Alan Green <avg(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Samir Ibradžić <sibradzic(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Simon Buhrow
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com>
Gerrit-Comment-Date: Tue, 04 May 2021 10:03:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Simon Buhrow
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Angel Pons.
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52883 )
Change subject: RFC: flashchips.c: merge GD25B128B/GD25Q128B and GD25Q127C/GD25Q128C
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
Further explanation of the reasons for this change:
These chip entries are some of the last differences between cros and upstream flashrom - resolving them will get us extremely close to fully deforking the flashchips.{c,h} files.
We also tried bringing the duplicate chip into cros flashrom, but that caused issues with flashchip identification, and resolving it would be likely to significantly delay resyncing flashchips.c.
This patch does remove the FEATURE_QPI feature bit (since only 'C' chips had it), however (AFAIK) that feature flag is currently unused, and the chip's capabilities are still recorded in the comments. An additional advantage of this approach is that it generally makes flashrom easier to use.
--
To view, visit https://review.coreboot.org/c/flashrom/+/52883
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I688f2e15aef61afbec728a9a81094bee56d6fbfa
Gerrit-Change-Number: 52883
Gerrit-PatchSet: 1
Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Tue, 04 May 2021 09:24:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment