April 2022 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: flashchips.c: add writeprotect support for more chips
by Anastasia Klimchuk (Code Review) April 4, 2022

April 4, 2022

Attention is currently required from: Nico Huber, Angel Pons, Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62214 ) Change subject: flashchips.c: add writeprotect support for more chips ...................................................................... Patch Set 26: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/62214/comment/9ba5ca29_af337d1e PS26, Line 17: GD25LQ128C/GD25LQ128D/GD25LQ128E This ones seem to be added earlier, not in this patch but before. The table is a good idea, did you mean it to cover everything added in this patch, or everything that supported up to this patch? It is at the moment the latter (unless I am missing something). Which is also fine, just maybe add the title of the table "Summary: everything that supported up to this patch" or something like that. -- To view, visit https://review.coreboot.org/c/flashrom/+/62214 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7f3d4c4148056098a845b5c64308b0333ebda395 Gerrit-Change-Number: 62214 Gerrit-PatchSet: 26 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Mon, 04 Apr 2022 00:12:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: spi25_statusreg: delete spi_read_status_register()
by Anastasia Klimchuk (Code Review) April 3, 2022

April 3, 2022

Attention is currently required from: Nico Huber, Angel Pons, Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59529 ) Change subject: spi25_statusreg: delete spi_read_status_register() ...................................................................... Patch Set 65: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/59529 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I146b4b5439872e66c5d33e156451a729d248c7da Gerrit-Change-Number: 59529 Gerrit-PatchSet: 65 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sun, 03 Apr 2022 23:55:50 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Introduce HSFC CYCLE READ/WRITE/ERASE macros
by Angel Pons (Code Review) April 3, 2022

April 3, 2022

Attention is currently required from: Subrata Banik, Nico Huber, Edward O'Callaghan, Anastasia Klimchuk. Angel Pons 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/7e897cc5_79e453d7 PS12, Line 163: #define HSFC_FCYCLE(cyc) ((cyc) << HSFC_FCYCLE_OFF) Isn't this equivalent to `HSFC_FCYCLE_MASK`? https://review.coreboot.org/c/flashrom/+/62868/comment/b4d4e309_48e7f2f6 PS12, Line 166: 4K Is it always 4K? -- 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: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 03 Apr 2022 21:56:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Introduce HSFC FDBC(n) macro
by Angel Pons (Code Review) April 3, 2022

April 3, 2022

Attention is currently required from: Subrata Banik, Nico Huber. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62892 ) Change subject: ichspi: Introduce HSFC FDBC(n) macro ...................................................................... Patch Set 6: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/62892 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie2512c85de9fc21286234b97f5842ecef1729787 Gerrit-Change-Number: 62892 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: Subrata Banik <subratabanik(a)google.com> Gerrit-Comment-Date: Sun, 03 Apr 2022 21:54:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Rename HSFC_FDBC -> HSFC_FDBC_MASK
by Angel Pons (Code Review) April 3, 2022

April 3, 2022

Attention is currently required from: Subrata Banik, Nico Huber, Anastasia Klimchuk. Angel Pons 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: (1 comment) File ichspi.c: https://review.coreboot.org/c/flashrom/+/62894/comment/7d966a87_1c802c92 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. -- 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: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 03 Apr 2022 21:53:46 +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

1 0

Change in flashrom[master]: ichspi: Rename HSFC_FDBC -> HSFC_FDBC_MASK
by Angel Pons (Code Review) April 3, 2022

April 3, 2022

Attention is currently required from: Subrata Banik, Nico Huber, Anastasia Klimchuk. Angel Pons 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/11965152_783e220e 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. File ichspi.c: https://review.coreboot.org/c/flashrom/+/62894/comment/5f18dd70_e8035cf0 PS4, Line 508: FDBC > > I have one more question :) […] Hmmm, if the new macro name doesn't work nicely with the existing code, maybe it wasn't meant to be changed? -- 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: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 03 Apr 2022 21:50:59 +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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Introduce FCYCLE_MASK(n) macro
by Angel Pons (Code Review) April 3, 2022

April 3, 2022

Attention is currently required from: Subrata Banik, Nico Huber, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62891 ) Change subject: ichspi: Introduce FCYCLE_MASK(n) macro ...................................................................... Patch Set 6: (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. File ichspi.c: https://review.coreboot.org/c/flashrom/+/62891/comment/ed7a7ecf_b98d8a2d PS6, Line 162: n Please add parentheses around macro parameters -- 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: 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: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 03 Apr 2022 21:47:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Define `Write Enable Type (WET)` register under HSFC
by Angel Pons (Code Review) April 3, 2022

April 3, 2022

Attention is currently required from: Subrata Banik, Nico Huber. Angel Pons 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/6423acef_a3f881ff 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. -- 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: Subrata Banik <subratabanik(a)google.com> Gerrit-Comment-Date: Sun, 03 Apr 2022 21:45:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Unify timeouts across all SPI operations to 30s
by Angel Pons (Code Review) April 3, 2022

April 3, 2022

Attention is currently required from: Subrata Banik, Nico Huber, Paul Menzel, Anastasia Klimchuk. Angel Pons 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: > > Oh I was referring to previous code, […] I think the reason to use smaller timeout values in some cases is to avoid unnecessary delays. 30 seconds is the recommended timeout for the worst-case scenario. Personally, I don't think using the 30-sec timeout everywhere is beneficial. -- 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: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 03 Apr 2022 21:44:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ich_descriptors.c: Ensure unsigned types >=0 on to prevent underflow
by Angel Pons (Code Review) April 3, 2022

April 3, 2022

Attention is currently required from: Edward O'Callaghan, Light, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62764 ) Change subject: ich_descriptors.c: Ensure unsigned types >=0 on to prevent underflow ...................................................................... Patch Set 16: (1 comment) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/62764/comment/98f0a539_5620f3a8 PS14, Line 501: for (j = 0; j < (size_t)min(num_regions, 12); j++) > > Not sure why you don't believe it is the max bounds in this case? > > Because the second loop (the one which is modified in the patch) starts with 12, and runs until num_regions. So 12 is a starting point of the loop, not the maximum. We can't use "max" in the name. > > I want to understand what exactly this 12 means. It probably means that Intel initially thought that having 12 regions in the IFD would be more than enough for any future needs. Later, they realized that they needed more regions, so they added a new set of bitfields to have more regions. I imagine they preserved the field position for the original 12 regions for backwards compatibility purposes. You can see the 12 here: /* From Skylake on */ struct { uint32_t ext_read :4, ext_write :4, read :12, write :12; } mstr[MAX_NUM_MASTERS]; -- To view, visit https://review.coreboot.org/c/flashrom/+/62764 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5921cc571f3dca5188ca1973dba6ececbcbe2f39 Gerrit-Change-Number: 62764 Gerrit-PatchSet: 16 Gerrit-Owner: Light <aarya.chaumal(a)gmail.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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Light <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 03 Apr 2022 21:36:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Light <aarya.chaumal(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0