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
Attention is currently required from: Subrata Banik, Nico Huber, Paul Menzel, Angel Pons, Anastasia Klimchuk.
Subrata Banik 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:
(1 comment)
Patchset:
PS5:
> I think the reason to use smaller timeout values in some cases is to avoid unnecessary delays.
I don't understand the comment here about *delay*, 30sec is the timeout value for the worst case scenario to avoid the SPI contention issue among multi-master scenario, it's not *forced delay* that would impact every chipsets. You could see more of the operations are still getting over within few ms of timeout.
> 30 seconds is the recommended timeout for the worst-case scenario.
>
> Personally, I don't think using the 30-sec timeout everywhere is beneficial.
Unless increased the timeout value in general, how would we know if it is applicable or not? I guess you are referring about SPI read operation which might not get impacted due to multi-master worst case operation as chipset has *fast-read* feature.
--
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: Nico Huber <nico.h(a)gmx.de>
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 04:26:54 +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: Daniel Campello.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63193 )
Change subject: tests: use MOCK_FD instead of NON_ZERO
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/63193/comment/093c924a_e1ec6c37
PS3, Line 9: return a non-negative value for
: the file descriptor expected from open operations.
> to fail if there is no `open()` mocked in a test
Oh definitely not intentional.
NON_ZERO was intended to represent a valid (successful) return value. What I think has happened, the value of NON_ZERO was taken too large (not intentionally) and it overflows to negative value.
It is possible those too (NON_ZERO and MOCK_FD) can be converged into one macro, but I would better do it later in a separate patch. Realtek test was using its own mock fd anyway (although I forgot why).
--
To view, visit https://review.coreboot.org/c/flashrom/+/63193
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib6bac051852aea2465665a6fd669b7f5e3772985
Gerrit-Change-Number: 63193
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Comment-Date: Mon, 04 Apr 2022 03:36:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
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: Nico Huber, Angel Pons, Anastasia Klimchuk, Sergii Dmytruk.
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59071 )
Change subject: flashchips: enable write-protection for W25Q{64,128}.V
......................................................................
Patch Set 24:
(1 comment)
Patchset:
PS24:
Hi Sergii,
Thanks again for extending writeprotect support with these patches! If you have some time, could you please rebase them on the latest patchset of the original WP patch chain? I'm also happy to have a go at it if you're busy right now.
- Nik
--
To view, visit https://review.coreboot.org/c/flashrom/+/59071
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Iccb69a8d3a0dd2192e2c938caddaf07b1889ed35
Gerrit-Change-Number: 59071
Gerrit-PatchSet: 24
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: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Mon, 04 Apr 2022 03:14:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk.
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62643 )
Change subject: writeprotect.c: refactor and fix wp_mode functions
......................................................................
Patch Set 11:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/62643/comment/73b58b21_6c260208
PS10, Line 12: since a chip without SRP is just FLASHROM_WP_MODE_DISABLED
> Just to double-check, chip without SRP/SRL will have […]
Yep :)
--
To view, visit https://review.coreboot.org/c/flashrom/+/62643
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib6c347453f9216e5816e4ed35bf9783fd3c720e0
Gerrit-Change-Number: 62643
Gerrit-PatchSet: 11
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: 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-Comment-Date: Mon, 04 Apr 2022 03:10:10 +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: Nico Huber, Angel Pons, Anastasia Klimchuk.
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61897 )
Change subject: libflashrom,linux_mtd: add linux_mtd writeprotect support
......................................................................
Patch Set 36:
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/61897/comment/6e1a4461_b01e7fab
PS36, Line 10: and uses it support
> Maybe instead "and uses it support" -> "and uses the framework to support".
Done
File linux_mtd.c:
https://review.coreboot.org/c/flashrom/+/61897/comment/8ac26c96_ac518582
PS36, Line 380: if ((cfg->range.len == 0) != (cfg->mode != FLASHROM_WP_MODE_DISABLED)) {
: return FLASHROM_WP_ERR_OTHER;
: }
> Please help me, I spent some time staring at the code, also reading the comment above and commit mes […]
Good catch! I think this was a last minute rewrite from `cfg->mode != FLASHROM_WP_MODE_HARDWARE` where I didn't change the operator.
--
To view, visit https://review.coreboot.org/c/flashrom/+/61897
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I5c86e28cdec44bec49ba1d36f8ab62241b9b01da
Gerrit-Change-Number: 61897
Gerrit-PatchSet: 36
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: 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-Comment-Date: Mon, 04 Apr 2022 03:08:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment