Attention is currently required from: Nico Huber, Thomas Heijligen, Anastasia Klimchuk.
Hello build bot (Jenkins), Nico Huber, Angel Pons, Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/55387
to look at the new patch set (#2).
Change subject: programmer.h: remove compile guard from programmer drivers
......................................................................
programmer.h: remove compile guard from programmer drivers
The definition of external structs doesn't have to be guarded.
See discussion under review.coreboot.org/52946.
Change-Id: I01e6a785269c3e0bd648eeaee217a7a855ab0853
Signed-off-by: Thomas Heijligen <thomas.heijligen(a)secunet.de>
---
M programmer.h
1 file changed, 43 insertions(+), 187 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/55387/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/55387
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I01e6a785269c3e0bd648eeaee217a7a855ab0853
Gerrit-Change-Number: 55387
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
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: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: David Bartley, Nico Huber.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54968 )
Change subject: flashchips: add support for Winbond W25Q01JV
......................................................................
Patch Set 4:
(1 comment)
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/54968/comment/aa2ae2e3_20f2886d
PS1, Line 17757: /* Full chip erase is fastest, typically takes 200s */
> The problem is that there is no common use-case to tune the list for.
> Placing smaller blocks first is better for smaller changes, placing
> bigger blocks first is better for bigger changes. We don't know in
> advance what to expect.
>
> My idea how to re-arrange things:
> * Add an optional function to the programmer drivers that can check if a command is expected to succeed (or rather fail, the most obvious example is Intel's internal programmer that can be configured to only allow a specific set of SPI commands). With such a function, the list of block erasers to consider can be filtered at runtime.
> * Once we know the set of block erasers that is expected to work, we can apply a heuristic that chooses the biggest block that won't unnecessarily erase unchanged data. Or maybe with a threshold, e.g. prefer 64K over 4K if >80% of the 4K blocks would get erased.
I don't know how hard implementing this would be, but I'd try to decide which block erasers to use depending on what needs to be erased. For example, a 96K contiguous region that starts/ends at a 64K boundary could be erased using a 64K erase command and a 32K erase command.
> Well, maybe there is one low-hanging fruit: For the erase commandline
> option (-E), we could iterate over the list in reverse order?
Sounds like a very good idea. Do we have to account for region erases, though?
--
To view, visit https://review.coreboot.org/c/flashrom/+/54968
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If369409419332070c1fed96ce0e96b7cfe42c169
Gerrit-Change-Number: 54968
Gerrit-PatchSet: 4
Gerrit-Owner: David Bartley <andareed(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: David Bartley <andareed(a)gmail.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Comment-Date: Fri, 11 Jun 2021 11:27:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: David Bartley <andareed(a)gmail.com>
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Anastasia Klimchuk.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55387 )
Change subject: programmer.h: remove compile guard from programmer drivers
......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/55387/comment/9e0e6e64_170644e7
PS1, Line 9: musst
> must
actually, "doesn't have to be". "must not" would imply that it
didn't work before, cf. e.g. https://portlandenglish.edu/blog/musnt-neednt/
--
To view, visit https://review.coreboot.org/c/flashrom/+/55387
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I01e6a785269c3e0bd648eeaee217a7a855ab0853
Gerrit-Change-Number: 55387
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Fri, 11 Jun 2021 11:22:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55408 )
Change subject: tests: Move test environment header files into tests directory
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/55408/comment/04047176_13b5a1ab
PS1, Line 8:
I agree with the change, but I'd appreciate if you could add a sentence to explain why this is a good idea.
--
To view, visit https://review.coreboot.org/c/flashrom/+/55408
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ia42773b98b1eb6c65241aa559c0c8b4926bd0814
Gerrit-Change-Number: 55408
Gerrit-PatchSet: 1
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-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Fri, 11 Jun 2021 11:19:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: David Bartley, Angel Pons.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54968 )
Change subject: flashchips: add support for Winbond W25Q01JV
......................................................................
Patch Set 4:
(1 comment)
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/54968/comment/e3c4e52b_071bba5d
PS1, Line 17757: /* Full chip erase is fastest, typically takes 200s */
> Fair enough, I can start a thread on the mailing list exploring options. […]
The problem is that there is no common use-case to tune the list for.
Placing smaller blocks first is better for smaller changes, placing
bigger blocks first is better for bigger changes. We don't know in
advance what to expect.
My idea how to re-arrange things:
* Add an optional function to the programmer drivers that can check if a command is expected to succeed (or rather fail, the most obvious example is Intel's internal programmer that can be configured to only allow a specific set of SPI commands). With such a function, the list of block erasers to consider can be filtered at runtime.
* Once we know the set of block erasers that is expected to work, we can apply a heuristic that chooses the biggest block that won't unnecessarily erase unchanged data. Or maybe with a threshold, e.g. prefer 64K over 4K if >80% of the 4K blocks would get erased.
Well, maybe there is one low-hanging fruit: For the erase commandline
option (-E), we could iterate over the list in reverse order?
--
To view, visit https://review.coreboot.org/c/flashrom/+/54968
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If369409419332070c1fed96ce0e96b7cfe42c169
Gerrit-Change-Number: 54968
Gerrit-PatchSet: 4
Gerrit-Owner: David Bartley <andareed(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: David Bartley <andareed(a)gmail.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Fri, 11 Jun 2021 10:51:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: David Bartley <andareed(a)gmail.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55358 )
Change subject: dummyflasher: Replace another case of `sizeof(struct ...)`
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> I meant to say, thank you, it is an honour for me!
You're welcome :) And let's say it was overdue anyway, the requirements
for the reviewers group[1] are not very high:
People who can +2 / -1 changes.
Contributors with more activity than a one-off commit.
My personal expectations may be higher, but you have exceeded those
too anyway ;) You have shown that you are able and willing to read.
A trait not common among people in that group Oo
[1] https://review.coreboot.org/admin/groups
--
To view, visit https://review.coreboot.org/c/flashrom/+/55358
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If88ba394095f86c598dcc5cf1751e1c23b132d04
Gerrit-Change-Number: 55358
Gerrit-PatchSet: 1
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Fri, 11 Jun 2021 10:20:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment