Attention is currently required from: Edward O'Callaghan.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59276 )
Change subject: pcidev: Move pci_get_dev() logic into canonical place
......................................................................
Patch Set 4:
(1 comment)
File board_enable.c:
https://review.coreboot.org/c/flashrom/+/59276/comment/63c8f76a_99505f4f
PS3, Line 1097: dev = pcidev_clonedev(dev);
> Yes `pacc` is a handle scoped to libpci and so lexically scope to pcidev.c which is what this change is doing.
I see what your code is doing, but I don't know your reasoning. Are you just
following design patterns you have learned or did you think it through? how
should I know? That's why we have a mailing list where we can discuss such
design choices. If we don't discuss such things and the reasons are not 100%
obvious, another developer with a different opinion might accidently work
on reverting your changes in the future. I've seen this a lot in coreboot
btw. it's not just a figment.
Currently `pacc` is global yes. But as you well know we want to get rid of
global state. So eventually, `pacc` might just become another, locally known
parameter, like `dev` is to pci_read_long() etc. And I don't see (yet), that
we'd be wrapping every single libpci function in `pcidev.c`. That's not saying
that it's wrong to do so for some functions, I just think our reasoning should
be more subtle.
Also, after reading above paragraphs together again: Can we tell that this
change makes replacing the global `pacc` easier or harder? That's all things
a reviewer must consider when the direction is unclear.
--
To view, visit https://review.coreboot.org/c/flashrom/+/59276
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id9ce055d5e5d347520ec5002b8c6548e60eaa0a7
Gerrit-Change-Number: 59276
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-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Sun, 27 Feb 2022 14:22:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59276 )
Change subject: pcidev: Move pci_get_dev() logic into canonical place
......................................................................
Patch Set 4:
(1 comment)
File board_enable.c:
https://review.coreboot.org/c/flashrom/+/59276/comment/baa6fcc1_6154cef6
PS3, Line 1097: dev = pcidev_clonedev(dev);
> The direction should be fairly clear, I rebased the follow on patches to give clarity. You realise every time you change your mind here I need to keep wasting time rebasing.
How you organize your patch trains is up to you. If constant rebasing is an
issue, then don't rebase constantly. Are you really making reviewers responsible
for necessary changes during review here? Can you please tell me in your
own words what is the point of reviews?
> Give actionable feedback, you wanted it renamed, then you changed your mind but agreed that 1 in the function name will do it now your comment is sort of high-Z.
Sorry, your patches look like you are spending little time to write them. So
please don't expect me to take much more time to write and repeat endless
explanations. I changed my mind because my original suggestion was based on
the idea that your original naming choice (pcidev_get_dev()) made sense which
it made little. I'll try to do better in the future and not to run into that
pitfall again.
> The change here is not particularly controversial, it is just isolating libpci stuff into pcidev.c which is pretty logical - look in pcidev.c for libpci stuff. So I don't know what you mean by "I have to look in another file", do you want the whole project in one file?
I fear, we are talking past each other. What I wanted to say is that before
this patch, when reading the code in `board_status.c` I could see what the
code does: it queries function 1 of the same PCI device. Now, after this
change, when reading the code in `board_status.c` I can't see that anymore
because the fact that it's about function 1 is hidden in another file. Putting
`function` in the API name could fix that. Other languages than C might not
have that problem, AFAIR you know Ada: There you could just write that parameter
name in the call e.g. `pcidev_getdev(dev, function => 1)`. But in C all we can do
is to choose good function and variable names. (Or add code comments, but I
kind of hate those (not checked by the compiler.))
--
To view, visit https://review.coreboot.org/c/flashrom/+/59276
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id9ce055d5e5d347520ec5002b8c6548e60eaa0a7
Gerrit-Change-Number: 59276
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-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Sun, 27 Feb 2022 14:08:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Patrick Georgi, Rizwan Qureshi, Stefan Reinauer, Angel Pons, Sridhar Siricilla, Alex Levin, YH Lin, Martin Roth - Personal, Caveh Jalali, David Hendricks, Tim Wawrzynczak, Nick Vaccaro, Boris Mittelberg.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61854 )
Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
......................................................................
Patch Set 12:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/61854/comment/4eeb4c35_0620f182
PS11, Line 18: AU
> nit: What does this mean? I wouldn't use an acronym here.
Ack
--
To view, visit https://review.coreboot.org/c/flashrom/+/61854
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484
Gerrit-Change-Number: 61854
Gerrit-PatchSet: 12
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Martin Roth - Personal <martinroth(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Alex Levin <levinale(a)chromium.org>
Gerrit-Attention: YH Lin <yueherngl(a)chromium.org>
Gerrit-Attention: Martin Roth - Personal <martinroth(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Comment-Date: Sun, 27 Feb 2022 13:03:30 +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: Patrick Georgi, Rizwan Qureshi, Stefan Reinauer, Sridhar Siricilla, Alex Levin, YH Lin, Martin Roth - Personal, Subrata Banik, Caveh Jalali, David Hendricks, Tim Wawrzynczak, Nick Vaccaro, Boris Mittelberg.
Hello build bot (Jenkins), Patrick Georgi, Stefan Reinauer, Rizwan Qureshi, Angel Pons, Sridhar Siricilla, Alex Levin, YH Lin, Nico Huber, Martin Roth - Personal, Caveh Jalali, Tim Wawrzynczak, Edward O'Callaghan, Nick Vaccaro, Boris Mittelberg,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/61854
to look at the new patch set (#12).
Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
......................................................................
ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
As per EDS, SPI controller sets the HSFSTS.bit5 (SCIP) when software
sets the Flash Cycle Go (FGO) bit in the Hardware Sequencing Flash
Control register.
This bit remains set until the cycle completes on the SPI interface.
Hardware automatically sets and clears this bit. Software must initiate
the next SPI transaction when this bit is 0.
Problem Statement:
Evidencing AU (Auto Update) failure while performing firmware update on
the Alder Lake based ChromeOS devices.
Observation:
Based on the initial understanding from the failure log/pattern, it
seems like the platform is evidencing multiple `flashrom` access from
different source, for example: `futility` accesses flashrom for erase,
write and read operation, `crossystem` uses flashrom for updating VBNV,
additionally, `set_fw_good` script also uses `crossystem` to update the
fw status.
Solution:
Without this SCIP check being implemented in flashrom, there is no
way to ensure multiple instances of flashrom performing different SPI
operations are not cancelling each other and running into below error:
Erasing and writing flash chip... Timeout error between offset
0x0061c000 and 0x0061c03f (= 0x0061c000 + 63)! FAILED!
Uh oh. Erase/write failed. Checking if anything has changed.
TEST=Able to flash coreboot image on Alder Lake Brya variants, Tiger
Lake Volteer variants and Comet Lake Hatch variants without any failure.
Signed-off-by: Subrata Banik <subratabanik(a)google.com>
Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484
---
M ichspi.c
1 file changed, 15 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/54/61854/12
--
To view, visit https://review.coreboot.org/c/flashrom/+/61854
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484
Gerrit-Change-Number: 61854
Gerrit-PatchSet: 12
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Martin Roth - Personal <martinroth(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Alex Levin <levinale(a)chromium.org>
Gerrit-Attention: YH Lin <yueherngl(a)chromium.org>
Gerrit-Attention: Martin Roth - Personal <martinroth(a)google.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Edward O'Callaghan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59281 )
Change subject: pcidev.c: Simplify by consolidating common logic
......................................................................
Patch Set 2:
(1 comment)
File pcidev.c:
https://review.coreboot.org/c/flashrom/+/59281/comment/b523c625_908d10c3
PS2, Line 175: pci_dev_find(vendor, 0)
> isn't that current behavior? `struct pci_filter filter;` has a uninitialised member of `. […]
Looks like `pci_filter_init()` initialises the device ID to -1: https://github.com/pciutils/pciutils/blob/master/lib/filter.c#L23
--
To view, visit https://review.coreboot.org/c/flashrom/+/59281
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I0048fc6ab816d230ff48c84bc17122431753d55d
Gerrit-Change-Number: 59281
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.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-Comment-Date: Sun, 27 Feb 2022 12:08:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Nikolai Artemiev.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51097 )
Change subject: Add support for Adesto AT25SF128A
......................................................................
Patch Set 2:
(1 comment)
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/51097/comment/c6022a33_3ff2f2de
PS2, Line 2329: .voltage = {1700, 2000},
> Adesto and Dialog both say it would be 2.7 to 3.6V. Can somebody confirm? […]
Hmmm, as per https://www.dialog-semiconductor.com/products/memory/dual-quad-spi-memory the 1.8V model would be `AT25SL128A` (s/F/L)
--
To view, visit https://review.coreboot.org/c/flashrom/+/51097
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I1ce2a6699a1f0116306f668123673a1ba9c932d2
Gerrit-Change-Number: 51097
Gerrit-PatchSet: 2
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-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Sun, 27 Feb 2022 11:47:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment