Attention is currently required from: Subrata Banik, Paul Menzel, Angel Pons, Anastasia Klimchuk.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62867 )
Change subject: ichspi: Unify timeouts across all SPI operations to 30s
......................................................................
Patch Set 8:
(2 comments)
Patchset:
PS5:
> > I think the reason to use smaller timeout values in some cases is to avoid unnecessary delays. […]
Angel, the background is that there is a hardware component that arbitrates
between the multiple integrated masters (host, me, gbe, ec, ...). When the bus
is kept busy by one master, execution of a transaction of another master, e.g.
ours, will be delayed. There is no way for us to know for how long so we need
to wait for the worst case (every master happens to erase a block at the same
time). This arbitration was never documented AFAIK but if one thinks about it,
it's the only reasonable way to explain how it works with the given register
interfaces.
We could still have distinct timeouts, but the difference would seem marginal
because it would be something like
<worst-case-other-masters> + 100ms
vs.
<worst-case-other-masters> + 5s
If that worst case time is high, it makes no difference.
(the above information is roughly what I would expect in the commit message)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/62867/comment/27513014_0473af01
PS8, Line 68: */
This comment is much too long, starts by describing the past state of the
code and references a specific flash chip which it shouldn't. Also, first
calculating 2.065s and then concluding 30s probably raises more questions
than are answered.
It should be possible to boil it down to 2 or 3 sentences, preferably in
the commit message and not as a code comment. If Intel would update the
public datasheets, we could also reference those. If not, we could also
add it to Documentation/.
--
To view, visit https://review.coreboot.org/c/flashrom/+/62867
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b
Gerrit-Change-Number: 62867
Gerrit-PatchSet: 8
Gerrit-Owner: Subrata Banik <subratabanik(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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Subrata Banik <subi.banik(a)gmail.com>
Gerrit-Attention: Subrata Banik <subi.banik(a)gmail.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Mon, 04 Apr 2022 10:25:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Nikolai Artemiev, Anastasia Klimchuk, Sergii Dmytruk.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60231 )
Change subject: writeprotect: add WPS bit and set it to zero
......................................................................
Patch Set 8:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/60231/comment/71b4cf1c_df82fd08
PS8, Line 9: proper support
Can you elaborate about this and why it perhaps cannot just be done in the immediate sense here in this patch?
--
To view, visit https://review.coreboot.org/c/flashrom/+/60231
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2c26ec65d64a3b6fb1f1a73690bc771415db2744
Gerrit-Change-Number: 60231
Gerrit-PatchSet: 8
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Mon, 04 Apr 2022 05:57:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Sam McNally, Edward O'Callaghan.
Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63227 )
Change subject: tests: assert pathname and flags when calling open()
......................................................................
Patch Set 7:
(1 comment)
File tests/tests.c:
https://review.coreboot.org/c/flashrom/+/63227/comment/08938044_939b1e82
PS6, Line 77: wrap_open_and_friends
> That is what I mean by "unexpected behavior" as `open_state` being non-NULL implies a builtin versio […]
Added assertion for that case on io_mock_register()
--
To view, visit https://review.coreboot.org/c/flashrom/+/63227
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib46ca5b854c8453ec02ae09f3151cd4d25f988eb
Gerrit-Change-Number: 63227
Gerrit-PatchSet: 7
Gerrit-Owner: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Mon, 04 Apr 2022 05:36:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Daniel Campello <campello(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Sam McNally, Daniel Campello.
Hello Sam McNally, build bot (Jenkins), Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/63227
to look at the new patch set (#7).
Change subject: tests: assert pathname and flags when calling open()
......................................................................
tests: assert pathname and flags when calling open()
With this change the wrappers for mock and friends are able to take an
optional io_mock_open_state struct to assert expected pathnames and
flags whenever an open operation is called.
Based partially on https://review.coreboot.org/c/flashrom/+/62319/5
BUG=b:227404721,b:217629892,b:215255210
TEST=./test_build.sh; FEATURES=test emerge-amd64-generic flashrom
BRANCH=none
Signed-off-by: Daniel Campello <campello(a)chromium.org>
Co-Author: Edward O'Callaghan <quasisec(a)google.com>
Change-Id: Ib46ca5b854c8453ec02ae09f3151cd4d25f988eb
---
M tests/io_mock.c
M tests/io_mock.h
M tests/lifecycle.c
M tests/tests.c
4 files changed, 39 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/27/63227/7
--
To view, visit https://review.coreboot.org/c/flashrom/+/63227
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib46ca5b854c8453ec02ae09f3151cd4d25f988eb
Gerrit-Change-Number: 63227
Gerrit-PatchSet: 7
Gerrit-Owner: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Subrata Banik, Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62868 )
Change subject: ichspi: Introduce HSFC CYCLE READ/WRITE/ERASE macros
......................................................................
Patch Set 12:
(2 comments)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/62868/comment/6447fb57_2f4aa9cf
PS12, Line 163: #define HSFC_FCYCLE(cyc) ((cyc) << HSFC_FCYCLE_OFF)
> Isn't this equivalent to `HSFC_FCYCLE_MASK`?
Apparently it look same as its used for bit shift
HSFC_FCYCLE_MASK: to represent the FCYCLE bit shift between ICH and PCH.
HSFC_FCYCLE: takes SPI operation as argument and generate the the FCYCLE value for the same.
Do you have suggestion to unifies both in meaningful way ?
https://review.coreboot.org/c/flashrom/+/62868/comment/92ef7b7d_8ad08b43
PS12, Line 166: 4K
> Is it always 4K?
atleast on PCH chipsets I could see it's since SPT PCH and Nico has mentioned some history here https://review.coreboot.org/c/flashrom/+/62870/6..8//COMMIT_MSG#b10
--
To view, visit https://review.coreboot.org/c/flashrom/+/62868
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3ea74b668e5f8d8e4b3da2a8ad8b81f1813e1e80
Gerrit-Change-Number: 62868
Gerrit-PatchSet: 12
Gerrit-Owner: Subrata Banik <subratabanik(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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Subrata Banik <subi.banik(a)gmail.com>
Gerrit-Attention: Subrata Banik <subi.banik(a)gmail.com>
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-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Mon, 04 Apr 2022 04:49:15 +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: Subrata Banik, Nico Huber, Angel Pons, Anastasia Klimchuk.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62894 )
Change subject: ichspi: Rename HSFC_FDBC -> HSFC_FDBC_MASK
......................................................................
Patch Set 6:
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/62894/comment/53cbb9b2_4b6fce3e
PS6, Line 10: during the data portion of the SPI cycle.
> What's the reasoning for this change? The commit message doesn't explain *why* this change would be desired.
without this CL, what HSFC_FDBC represents is a mask to perform the byte shift, hence rename to reflect the same.
HSFC_FDBC_MASK macro represents the number of bytes to shift in or out
during the data portion of the SPI cycle.
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/62894/comment/4003b24e_4cffa435
PS4, Line 508: FDBC
> > Hmmm, if the new macro name doesn't work nicely with the existing code, maybe it wasn't meant to be changed?
>
> Or maybe it's worth updating all the mask macros along with the `pprint_reg` macro to avoid having to use `_pprint_reg` directly.
It's not that, this CL first introduces such direct usage of _pprint_reg instead pprint_reg. Refer below:
https://github.com/flashrom/flashrom/blob/master/ichspi.c#L466
This was the case when HSFC macros names are not compatible to get used with pprint_reg macro, folks uses _pprint_reg directly. won't expect any performance impact and debug log impact with this changes.
--
To view, visit https://review.coreboot.org/c/flashrom/+/62894
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ia0ae9a586b5c12f0229334898426175ec246a70c
Gerrit-Change-Number: 62894
Gerrit-PatchSet: 6
Gerrit-Owner: Subrata Banik <subratabanik(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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Subrata Banik <subi.banik(a)gmail.com>
Gerrit-Attention: Subrata Banik <subi.banik(a)gmail.com>
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-Comment-Date: Mon, 04 Apr 2022 04:43:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subi.banik(a)gmail.com>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Subrata Banik, Nico Huber, Angel Pons, Anastasia Klimchuk.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62891 )
Change subject: ichspi: Introduce FCYCLE_MASK(n) macro
......................................................................
Patch Set 7:
(2 comments)
Patchset:
PS6:
> What's the benefit of these changes? To me, the `FCYCLE_MASK` macro makes things more noisy. Also, note that the definitions are organized so that PCH100 differences appear first.
I have added the motivation here:
This patch introduces HSFC_FCYCLE_MASK(n) macro to cover both ICH and
PCH hardware sequencing FCYCLE Bit width.
Also without this CL, the Flash Cycle bit field width is wrong for latest chipset. It's 4 bit width on PCH as per EDS but without this CL, we are still considering it 2 bit.
Having a macro that unifies the FYCLE mask is the intention and once we rectified the same, we need to add the corresponding changes. May be that is what you call as *noisy*.
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/62891/comment/69633dc7_b4b7564c
PS6, Line 162: n
> Please add parentheses around macro parameters
Ack
--
To view, visit https://review.coreboot.org/c/flashrom/+/62891
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id222304165610c7ae48e365d72ec8fdeea51c51d
Gerrit-Change-Number: 62891
Gerrit-PatchSet: 7
Gerrit-Owner: Subrata Banik <subratabanik(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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Subrata Banik <subi.banik(a)gmail.com>
Gerrit-Attention: Subrata Banik <subi.banik(a)gmail.com>
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-Comment-Date: Mon, 04 Apr 2022 04:33:08 +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: Subrata Banik, Nico Huber, Angel Pons.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62888 )
Change subject: ichspi: Define `Write Enable Type (WET)` register under HSFC
......................................................................
Patch Set 6:
(1 comment)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/62888/comment/37900d66_23e49430
PS6, Line 91: /* New HSFC Control bit */
> I think this was defined here because it's new with PCH100. Older platforms don't seem to have this bit.
In older chipsets, I could see this field is mark reserved, not sure if that is documentation issue, but having a name won't impact the older platform IMO.
--
To view, visit https://review.coreboot.org/c/flashrom/+/62888
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id32cb4ccb83dd08e9b0b1ab30cc8e041dd059f5f
Gerrit-Change-Number: 62888
Gerrit-PatchSet: 6
Gerrit-Owner: Subrata Banik <subratabanik(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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Subrata Banik <subi.banik(a)gmail.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Mon, 04 Apr 2022 04:28:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment