Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59741 )
Change subject: tests: Add run_probe_lifecycle and add dummyflasher probe test
......................................................................
Patch Set 2:
(2 comments)
File tests/lifecycle.c:
https://review.coreboot.org/c/flashrom/+/59741/comment/2787629a_9b69cc73
PS1, Line 53: printf("Testing flashrom_flash_probe for programmer=%s, chip=%s ... \n", prog->name, chip_name);
: assert_int_equal(0, flashrom_flash_probe(&flashctx, flashprog, chip_name));
: printf("... flashrom_flash_probe for programmer=%s successful\n", prog->name);
> technically this block is the real meaning of the test and the rest is mostly almost exact the same […]
I like your idea! Please have a look at my attempt to implement it :)
https://review.coreboot.org/c/flashrom/+/59741/comment/7b48667b_47c3e9b1
PS1, Line 68: "bus=spi,emulate=W25Q128FV", "W25Q128.V"
> this is changing the behavior of the test in a rename commit.
This is not a rename commit, but you are right, behavior is changing because it's a different test now (it's a new probe test). However on the second thought I realised there is no reason to delete existing test, so I returned it back. Now dummy is officially The Most Tested Programmer!
Given that programmer params differ, two dummy tests are not a duplication. So all good (I think).
--
To view, visit https://review.coreboot.org/c/flashrom/+/59741
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9eb7fe3a436fbba5e70db957139fd26e00efec36
Gerrit-Change-Number: 59741
Gerrit-PatchSet: 2
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-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-Comment-Date: Wed, 15 Dec 2021 22:45:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59740 )
Change subject: tests: Rename run_lifecycle into run_basic_lifecycle
......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/59740/comment/5a2c9862_f8a172bb
PS1, Line 7: basic
It means simple. I have an explanation in commit message:
> Existing lifecycle, being the simplest possible one becomes "basic lifecycle"
I thought it is okay name :) but if you have a better word in mind, please tell me!
--
To view, visit https://review.coreboot.org/c/flashrom/+/59740
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2771921ae2bd37f4b3f49342e03d9abb5ee36ea0
Gerrit-Change-Number: 59740
Gerrit-PatchSet: 2
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-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-Comment-Date: Wed, 15 Dec 2021 22:20:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk.
Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/59741
to look at the new patch set (#2).
Change subject: tests: Add run_probe_lifecycle and add dummyflasher probe test
......................................................................
tests: Add run_probe_lifecycle and add dummyflasher probe test
This patch implements run_probe_lifecycle and adds dummyflasher
test to run probing lifecycle.
A lifecycle consists of 3 steps: 1) init programmer 2) do some action
3) shutdown programmer. Step 2 can be "do nothing", and this is
named "basic lifecycle", i.e. the simplest. This patch implements
"probe lifecycle" which probes a chip as Step 2.
Internally there is one run_lifecycle function which performs steps
1, 2, 3. run_lifecycle is operating via libflashrom API. Long term
goal for cli_classic is to operate via libflashrom API, so the test
aligns with this approach.
BUG=b:181803212
TEST=ninja test
Change-Id: I9eb7fe3a436fbba5e70db957139fd26e00efec36
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M tests/lifecycle.c
M tests/tests.c
M tests/tests.h
3 files changed, 53 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/41/59741/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/59741
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9eb7fe3a436fbba5e70db957139fd26e00efec36
Gerrit-Change-Number: 59741
Gerrit-PatchSet: 2
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-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Angel Pons, Nikolai Artemiev, Sergii Dmytruk.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58478 )
Change subject: flash: add structure to represent chip wp configuration
......................................................................
Patch Set 22:
(3 comments)
File flash.h:
https://review.coreboot.org/c/flashrom/+/58478/comment/fd05d9a1_d789bbbb
PS22, Line 201: Complete
`Complete` seems a bit exaggerated. Given the many different
implementations, I'm not sure if we can ever have that. Actually,
I'm missing one thing already, `WPS`.
https://review.coreboot.org/c/flashrom/+/58478/comment/69db359b_595f3746
PS22, Line 203: srp
Please add comments what all these acronyms mean.
File flash.h:
https://review.coreboot.org/c/flashrom/+/58478/comment/b3f30cd6_74048bf8
PS21, Line 201: /* Complete description of a chip's write protection configuration */
: struct flashrom_wp_chip_config {
: size_t srp_bit_count;
: uint8_t srp[MAX_SRP_BITS];
:
: bool cmp_bit_present;
: uint8_t cmp;
:
: bool sec_bit_present;
: uint8_t sec;
:
: bool tb_bit_present;
: uint8_t tb;
:
: size_t bp_bit_count;
: uint8_t bp[MAX_BP_BITS];
: };
> This is specific to SPI flash chips.
Well, not by nature, probably by coincidence. I don't see why one couldn't
make a flash chip with such registers but without SPI interface.
However, I also see that it doesn't seem to belong here. As the commit
message suggests, this is the representation used by the writeprotect
code (not the API?), it should be private to chip drivers and
`writeprotect*.c` I guess.
--
To view, visit https://review.coreboot.org/c/flashrom/+/58478
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I17dee630248ce7b51e624a6e46d7097d5d0de809
Gerrit-Change-Number: 58478
Gerrit-PatchSet: 22
Gerrit-Owner: Nikolai Artemiev <nartemiev(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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Wed, 15 Dec 2021 22:12:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev.
Sergii Dmytruk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59072 )
Change subject: dummyflasher: add SR2 emulation harness
......................................................................
Patch Set 16:
(1 comment)
File dummyflasher.c:
https://review.coreboot.org/c/flashrom/+/59072/comment/ab62f3b2_d4abf81c
PS14, Line 346: if (writecnt == 3)
> Oh great! Also maybe I am missing something, but why two ifs are not one if? it can do the job and t […]
I think merging statements would make code structure look odd:
work for both cases (1);
if (wrsr_ext) {
work for one case (2);
debug print about (1) and (2);
} else {
debug print about (1);
}
I'd keep actual work and messages separate.
--
To view, visit https://review.coreboot.org/c/flashrom/+/59072
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I177ae3f068f03380f5b3941d9996a07205672e59
Gerrit-Change-Number: 59072
Gerrit-PatchSet: 16
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
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-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com>
Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Wed, 15 Dec 2021 21:34:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Angel Pons, Nikolai Artemiev, Sergii Dmytruk.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59072 )
Change subject: dummyflasher: add SR2 emulation harness
......................................................................
Patch Set 16:
(1 comment)
File dummyflasher.c:
https://review.coreboot.org/c/flashrom/+/59072/comment/d2c33ec4_ea8f4057
PS14, Line 346: if (writecnt == 3)
> Thanks, the two conditions should be in sync.
Oh great! Also maybe I am missing something, but why two ifs are not one if? it can do the job and then print a message?
--
To view, visit https://review.coreboot.org/c/flashrom/+/59072
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I177ae3f068f03380f5b3941d9996a07205672e59
Gerrit-Change-Number: 59072
Gerrit-PatchSet: 16
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
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-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com>
Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Wed, 15 Dec 2021 20:54:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Nikolai Artemiev.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58735 )
Change subject: ichspi: Split very long init function into two
......................................................................
Patch Set 1:
(1 comment)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/58735/comment/07c2e4e3_5364da8e
PS1, Line 2049: else {
: register_spi_master(&spi_master_ich9, NULL);
: }
> Not in this commit, please.
I fully agree
--
To view, visit https://review.coreboot.org/c/flashrom/+/58735
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6789bc456a4878e6555831ae0b80ecbdbf62938b
Gerrit-Change-Number: 58735
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: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
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: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Wed, 15 Dec 2021 19:19:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk, Nikolai Artemiev, Sergii Dmytruk.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58477 )
Change subject: flashchips: add writeprotect bit layout map to chips
......................................................................
Patch Set 21:
(4 comments)
File flash.h:
https://review.coreboot.org/c/flashrom/+/58477/comment/3db77e9e_1ce7f9ca
PS21, Line 192: 0
Why give these numbers? Is there anything to sync with?
https://review.coreboot.org/c/flashrom/+/58477/comment/64a5e9cb_76f52562
PS21, Line 196: };
If we'd ignore the enum types, i.e. use `uint8_t` for all members, we could
squeeze everything in 1B, saving 92% of the space.
https://review.coreboot.org/c/flashrom/+/58477/comment/614d3a06_c9c09d6e
PS21, Line 283: */
We should generally try to find a better representation for the flash chips.
The database is already too big for some use cases. With the current `struct
reg_bit_map` definition, this bloats the database by 23%. Fairly compressable
though.
You don't have to use common definitions with a pointer btw. I played
with this once for the block erasers. The only diff in the database was
- .block_erasers =
+ .block_erasers = (const struct block_eraser[NUM_ERASEFUNCTIONS])
all over it. Together with `-fmerge-all-constants` GCC could join the
common definitions.
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/58477/comment/d963df53_9cc6cd65
PS9, Line 6760: {STATUS1, 5, RW}
> I got the bits mixed up, thanks for noticing!
You could, maybe should, extend selfcheck(), e.g. add something similar to
selfcheck_eraseblocks(). Or add a unit test (but there were always plans to
make the chip database more runtime flexible). We could definitely catch
such issues.
--
To view, visit https://review.coreboot.org/c/flashrom/+/58477
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id08d77e6d4ca5109c0d698271146d026dbc21284
Gerrit-Change-Number: 58477
Gerrit-PatchSet: 21
Gerrit-Owner: Nikolai Artemiev <nartemiev(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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Wed, 15 Dec 2021 17:36:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment