Attention is currently required from: Daniel Campello, Light.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64668 )
Change subject: cli_output.c: Format progress output to show in single line
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
Patchset:
PS1:
Makes sense to me.
Daniel, Richard, does this look good for you as authors of progress API?
--
To view, visit https://review.coreboot.org/c/flashrom/+/64668
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If982193f9857f0da47cd71706ce6cebaa60d4323
Gerrit-Change-Number: 64668
Gerrit-PatchSet: 1
Gerrit-Owner: Light <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Richard Hughes <richard(a)hughsie.com>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Attention: Light <aarya.chaumal(a)gmail.com>
Gerrit-Comment-Date: Mon, 30 May 2022 00:37:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Nico Huber, Richard Hughes.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64663 )
Change subject: libflashrom: Allow getting the progress_state from the flashctx
......................................................................
Patch Set 2:
(2 comments)
Patchset:
PS2:
I think I can answer some of these, let me try! :)
> have you tested if the current implementation works?
There is a testing information in commit message, I asked to add earlier.
> I'm a bit confused about when update_progress() should get called.
Previous patch CB:49643 introduces update_progress() calls into many programmers. For me, it was easier to understand when looking at CB:49643
> Does it do anything on a modern Intel system at all?
This question, maybe someone can confirm, but what I see is that CB:49643 is not changing ichspi.c at all, so I guess Intel systems are not touched?
> There are odd paths like spi_chip_read() calls into the master, which may
> call spi_read_chunked() which calls update_progress(). But spi_chip_read()
> calls it as well? I wonder what was the reason to do it at the chip and
> programmer levels.
This is interesting, I had a closer look into `spi_chip_read()` and `spi_read_chunked()` and I see now what you mean. It is possible that update_progress is called by both functions, however not always, but only in some cases.
I looked into flashrom_progress_cb in cli_output.c and it calculates percent progress, and then it produces output only of percent has changed. So it won't produce the same repeated lines like
READ 66% complete...
READ 66% complete...
It will be only line in such case.
What would be a better way to wire update_progress into spi infra, what do you think?
File include/libflashrom.h:
https://review.coreboot.org/c/flashrom/+/64663/comment/d262150c_4a0fb9d9
PS2, Line 85: typedef void(flashrom_progress_callback)(struct flashrom_flashctx *flashctx);
> Why doesn't it get the progress passed directly?
I think I can answer this one. There is a progress_state in flashctx, so no need to pass another one.
The earlier versions in CB:49643 had two arguments: flashctx and progress_state and it was confusing on which exactly state is used. The patch owner was asked to remove the second argument during review. I can understand because there is a progress_state in flashctx already.
I hope I answered your question! At least, to the extent how I understand the question.
--
To view, visit https://review.coreboot.org/c/flashrom/+/64663
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I322bf56ff92f7b4d0ffc92768e9f0cdf7cb82010
Gerrit-Change-Number: 64663
Gerrit-PatchSet: 2
Gerrit-Owner: Richard Hughes <richard(a)hughsie.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Richard Hughes <richard(a)hughsie.com>
Gerrit-Comment-Date: Mon, 30 May 2022 00:23:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev.
Sergii Dmytruk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64375 )
Change subject: writeprotect.c: Allow opaque masters to hook spi_{rw}_register()
......................................................................
Patch Set 6:
(1 comment)
File include/programmer.h:
https://review.coreboot.org/c/flashrom/+/64375/comment/b2805ef9_e950968d
PS6, Line 405: int (*read_register)(const struct flashctx *flash, enum flash_reg reg, uint8_t *value);
: int (*write_register)(const struct flashctx *flash, enum flash_reg reg, uint8_t value);
If these are meant to stay WP only, they should have "wp_" prefix.
--
To view, visit https://review.coreboot.org/c/flashrom/+/64375
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3ab0d7f5f48338c8ecb118a69651c203fbc516ac
Gerrit-Change-Number: 64375
Gerrit-PatchSet: 6
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Sun, 29 May 2022 13:18:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Nico Huber, Martin L Roth.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64499 )
Change subject: Add reviewers group to allow +1, -1 changes.
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
Patchset:
PS2:
Looks reasonable.
--
To view, visit https://review.coreboot.org/c/flashrom/+/64499
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: refs/meta/config
Gerrit-Change-Id: I6af85c58d9a8c37c8bf17181c83720332a474cc8
Gerrit-Change-Number: 64499
Gerrit-PatchSet: 2
Gerrit-Owner: Martin L Roth <gaumless(a)tutanota.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Martin L Roth <gaumless(a)tutanota.com>
Gerrit-Comment-Date: Sun, 29 May 2022 11:44:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Nikolai Artemiev, Sergii Dmytruk.
Hello build bot (Jenkins), Subrata Banik, Nikolai Artemiev, Sergii Dmytruk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/64375
to look at the new patch set (#6).
Change subject: writeprotect.c: Allow opaque masters to hook spi_{rw}_register()
......................................................................
writeprotect.c: Allow opaque masters to hook spi_{rw}_register()
Allow specialisation in opaque masters, such as ichspi hwseq, to
write to status registers.
Change-Id: I3ab0d7f5f48338c8ecb118a69651c203fbc516ac
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M include/programmer.h
M writeprotect.c
2 files changed, 26 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/75/64375/6
--
To view, visit https://review.coreboot.org/c/flashrom/+/64375
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3ab0d7f5f48338c8ecb118a69651c203fbc516ac
Gerrit-Change-Number: 64375
Gerrit-PatchSet: 6
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Nikolai Artemiev, Sergii Dmytruk.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64375 )
Change subject: spi25_statusreg.c: Allow opaque masters to hook spi_{rw}_register()
......................................................................
Patch Set 5: Code-Review-1
(1 comment)
File spi25_statusreg.c:
https://review.coreboot.org/c/flashrom/+/64375/comment/e19c8ce1_3afee909
PS5, Line 33: if ((flash->mst->buses_supported & BUS_PROG) && flash->mst->opaque.write_register) {
: return flash->mst->opaque.write_register(flash, reg, value);
: }
I am inclined to move this specalisation to actually within writeprotect.c I feel this layer violates as-is and that flashrom probably should be refactored some to wrap up the spi command issuing semantics and master selection behind some kind of internal API.
--
To view, visit https://review.coreboot.org/c/flashrom/+/64375
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3ab0d7f5f48338c8ecb118a69651c203fbc516ac
Gerrit-Change-Number: 64375
Gerrit-PatchSet: 5
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Sun, 29 May 2022 02:05:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev.
Hello build bot (Jenkins), Subrata Banik, Nikolai Artemiev, Sergii Dmytruk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/64540
to look at the new patch set (#5).
Change subject: ichspi.c: Implement read_write_status for wp
......................................................................
ichspi.c: Implement read_write_status for wp
The ichspi hwseq path has a opaque master specialisation that
allows for reading and writing STATUS1 registers. Hook the callbacks
with a implementation to allow for this so that writeprotect maybe
supported though this path.
Change-Id: I7ecbe8491ecea3697922c91af26ca62276e86317
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M ichspi.c
1 file changed, 75 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/40/64540/5
--
To view, visit https://review.coreboot.org/c/flashrom/+/64540
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7ecbe8491ecea3697922c91af26ca62276e86317
Gerrit-Change-Number: 64540
Gerrit-PatchSet: 5
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Nico Huber, Thomas Heijligen, Nikolai Artemiev, Sergii Dmytruk.
Hello build bot (Jenkins), Thomas Heijligen, Nikolai Artemiev, Sergii Dmytruk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/64746
to look at the new patch set (#2).
Change subject: spi25_statusreg: Allow WRSR_EXT for Status Register 3
......................................................................
spi25_statusreg: Allow WRSR_EXT for Status Register 3
Spansion flash chips S25FL128L and S25FL256L use the WRSR instruction to
write more than 2 registers. So align SR2 and SR3 support: The current
FEATURE_WRSR_EXT is renamed to FEATURE_WRSR_EXT2 and FEATURE_WRSR_EXT3
is added. Also, WRSR3 needs a separate flag now.
Verified that FEATURE_WRSR_EXT2 still works using the `dummy_flasher`.
Signed-off-by: Nico Huber <nico.h(a)gmx.de>
Change-Id: Ibdfc6eb3d2cfecbf8da0493d067031ddb079a094
---
M dummyflasher.c
M flashchips.c
M include/flash.h
M include/spi.h
M spi25_statusreg.c
M tests/chip_wp.c
6 files changed, 83 insertions(+), 38 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/46/64746/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/64746
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ibdfc6eb3d2cfecbf8da0493d067031ddb079a094
Gerrit-Change-Number: 64746
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Nico Huber, Thomas Heijligen, Anastasia Klimchuk, Nikolai Artemiev.
Sergii Dmytruk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64748 )
Change subject: dummyflasher: Add emulation for S25FL128L
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
The new chip wasn't added to the list of emulated chips in flashrom.8.tmpl
--
To view, visit https://review.coreboot.org/c/flashrom/+/64748
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ic3cbea87218c973331b9b83e809e7d438407bc13
Gerrit-Change-Number: 64748
Gerrit-PatchSet: 1
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Sat, 28 May 2022 16:17:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment