Attention is currently required from: Nico Huber, Edward O'Callaghan, 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 4:
(1 comment)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/62894/comment/67c70ca4_b43a76f8
PS4, Line 508: FDBC
> Why this is replaced? Commit title says "Rename HSFC_FDBC -> HSFC_FDBC_MASK" and it doesn't mention FDBC.
see this macro definition
./ichspi.c:454:#define pprint_reg(reg, bit, val, sep) _pprint_reg(bit, reg##_##bit, reg##_##bit##_OFF, val, sep)
Before this CL:
reg -> HSFC
bit -> FDBC
hence, reg##_##bit => HSFC_FDBC and reg##_##bit##_OFF => HSFC_FDBC_OFF
both macros exist
With this CL:
reg -> HSFC
bit -> FDBC_MASK
hence, reg##_##bit => HSFC_FDBC_MASK and reg##_##bit##_OFF => HSFC_FDBC_MASK_OFF
`HSFC_FDBC_MASK` exists but there is no macro to resolve this symbol `HSFC_FDBC_MASK_OFF`, hence, had to use `_pprint_reg` which is very direct, instead those string addition.
./ichspi.c:453:#define _pprint_reg(bit, mask, off, val, sep) msg_pdbg("%s=%d" sep, #bit, (val & mask) >> off)
Hope this clarifies.
--
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: 4
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: 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: Fri, 18 Mar 2022 05:10:03 +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, Subrata Banik, Edward O'Callaghan, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62891 )
Change subject: ichspi: Introduce HSFC FCYCLE(cyc)/FCYCLE_MASK(n) macros
......................................................................
Patch Set 3:
(5 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/62891/comment/8b4c7ba6_6554ba6e
PS3, Line 12: Also, drops unused macros (PCH100_HSFC_FCYCLE_OFF and
: PCH100_HSFC_FCYCLE).
Can you extract this into a separate patch? Seems like these are two different things (corresponding to 1st and 2nd paragraph in commit message).
https://review.coreboot.org/c/flashrom/+/62891/comment/be76535d_aca982ec
PS3, Line 16: brya
I see you are using brya to test this chain, if you could add info what chipset brya has, it would be very useful.
(and the same for all other patches in the chain ;)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/62891/comment/83cc1fbc_05b6fd8d
PS3, Line 80: /* Changed HSFC Control bits */
Should the comment be removed as well?
https://review.coreboot.org/c/flashrom/+/62891/comment/b1b7f36d_17bef426
PS3, Line 500: reg_val, ", ");
No new line needed, this print statement can be on one line
https://www.flashrom.org/Development_Guidelines#Coding_stylehttps://review.coreboot.org/c/flashrom/+/62891/comment/5d89a1b5_cdb247d9
PS3, Line 505: reg_val, ", ");
Same as previous
--
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: 3
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: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Fri, 18 Mar 2022 04:52:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Subrata Banik, Edward O'Callaghan, Angel Pons.
Anastasia Klimchuk 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 3:
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/62888/comment/95da038f_1a38a628
PS3, Line 13: brya
What chipset brya has? It would be useful to have this info in commit message.
Patchset:
PS3:
This change should not change the binary, is it right? (please tell me if I am wrong)
You can build the binary before and after the patch and verify there are no diffs
make clean && make CONFIG_EVERYTHING=yes VERSION=none`
diff binary_before binary_after
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/62888/comment/79929f64_2d479b8a
PS3, Line 83: /* New HSFC Control bit */
This comment should be moved too?
--
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: 3
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: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Fri, 18 Mar 2022 04:34:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Subrata Banik, Paul Menzel, Edward O'Callaghan, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62867 )
Change subject: ichspi: Unify timeout (5sec) across all SPI operations
......................................................................
Patch Set 5:
(2 comments)
Patchset:
PS5:
Just curious, what was the reason for using two different timeouts?
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/62867/comment/459b2404_3534df0b
PS5, Line 33: /* Sunrise Point */
This "Sunrise Point" comment used to be above HSFS status bits macros, maybe it should be moved back to its place, if those macros are platform-specific?
Is ICH_HWSEQ_XFER_TIMEOUT_US Sunrise Point specific? (I don't think so).
--
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: 5
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: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Fri, 18 Mar 2022 04:25:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59280 )
Change subject: pcidev: Move pci_dev_find() from internal to canonical place
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/59280/comment/140d2fbd_9ba0e0dc
PS5, Line 10: sudo ./flashrom -p internal -r /tmp/bios
The same comment as in the previous patch: if you could add info what environment the command was performed, would be great.
--
To view, visit https://review.coreboot.org/c/flashrom/+/59280
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ie21f87699481a84398ca4450b3f03548f0528191
Gerrit-Change-Number: 59280
Gerrit-PatchSet: 5
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(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-Comment-Date: Fri, 18 Mar 2022 03:46:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59279 )
Change subject: pcidev: Move pci_card_find() from internal to canonical place
......................................................................
Patch Set 5:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/59279/comment/f491dd7b_11536db2
PS5, Line 10: sudo ./flashrom -p internal -r /tmp/bios
> Well this is just moving the function between files, there is no functional change. […]
Yes, so my point was, TEST info does not mention what environment the command was performed. You mentioned "controller chipset on Intel" but commit message does not say anything about Intel.
--
To view, visit https://review.coreboot.org/c/flashrom/+/59279
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I026bfbecba114411728d4ad1ed8969b469fa7d2d
Gerrit-Change-Number: 59279
Gerrit-PatchSet: 5
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
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: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Fri, 18 Mar 2022 03:41:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment