Attention is currently required from: Nico Huber, Thomas Heijligen, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60110 )
Change subject: hwaccess: move x86 port I/O related code into own files
......................................................................
Patch Set 4:
(2 comments)
File Makefile:
https://review.coreboot.org/c/flashrom/+/60110/comment/35ad35aa_e532052c
PS4, Line 783: ifneq ($(NEED_RAW_ACCESS), )
: # Raw memory, MSR or PCI port I/O access.
: FEATURE_CFLAGS += -D'NEED_RAW_ACCESS=1' -D'__FLASHROM_HAVE_OUTB__=1'
: PROGRAMMER_OBJS += physmap.o hwaccess.o hwaccess_x86_io.o
Mostly curiosity: what is the way to build with NEED_RAW_ACCESS empty? Would that be disabling all the drivers that need raw access?
Running `make` by default will have drivers that need raw access, which gives one (positive) branch of ifneq condition, that's what I am thinking.
File tests/hwaccess_x86_io_unittest.h:
https://review.coreboot.org/c/flashrom/+/60110/comment/6e74bef6_329c2b20
PS4, Line 33: int rget_io_perms(void);
If you could move it below all defines and includes (right above iopl)? thanks!
--
To view, visit https://review.coreboot.org/c/flashrom/+/60110
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I372b4a409f036da766c42bc406b596bc41b0f75a
Gerrit-Change-Number: 60110
Gerrit-PatchSet: 4
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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 16 Dec 2021 01:34:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk, Peter Marheine.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59275 )
Change subject: pcidev: Avoid internal programmer relying on pacc global
......................................................................
Patch Set 3:
(2 comments)
File internal.c:
https://review.coreboot.org/c/flashrom/+/59275/comment/c1ffc7c1_5b3bd2ca
PS3, Line 41: temp = pcidev_scandev(&filter, NULL);
: for (; temp; temp = pcidev_scandev(&filter, temp)) {
> I guess this could also be written as […]
That's a more natural construct, fixed the other loop accordingly too. Done.
https://review.coreboot.org/c/flashrom/+/59275/comment/c8e7630b_3d49bbc5
PS3, Line 43: if (pci_filter_match(&filter, temp)) {
> That's what pcidev_scandev() does, isn't it?
Yes, fat fingered the diff. Done.
--
To view, visit https://review.coreboot.org/c/flashrom/+/59275
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id83bfd41f785f907e52a65a6689e8c7016fc1b77
Gerrit-Change-Number: 59275
Gerrit-PatchSet: 3
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
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: Peter Marheine <pmarheine(a)chromium.org>
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-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Thu, 16 Dec 2021 00:30:32 +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, Angel Pons, Anastasia Klimchuk, Peter Marheine.
Hello build bot (Jenkins), Nico Huber, Angel Pons, Anastasia Klimchuk, Peter Marheine,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/59275
to look at the new patch set (#4).
Change subject: pcidev: Avoid internal programmer relying on pacc global
......................................................................
pcidev: Avoid internal programmer relying on pacc global
Make progress towards the goal of removing pacc from global
state as noted in the FIXME of programmer.h
BUG=none
TEST=builds
Change-Id: Id83bfd41f785f907e52a65a6689e8c7016fc1b77
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M internal.c
M pcidev.c
M programmer.h
3 files changed, 25 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/75/59275/4
--
To view, visit https://review.coreboot.org/c/flashrom/+/59275
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id83bfd41f785f907e52a65a6689e8c7016fc1b77
Gerrit-Change-Number: 59275
Gerrit-PatchSet: 4
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
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: Peter Marheine <pmarheine(a)chromium.org>
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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: newpatchset
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