September 2021 - flashrom-gerrit

Change in flashrom[master]: ich_descriptors: Add check for ISL == 139 for LBG
by David Hendricks (Code Review) 14 Sep '21

14 Sep '21

David Hendricks has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/57581 ) Change subject: ich_descriptors: Add check for ISL == 139 for LBG ...................................................................... Abandoned Superseded by CB:57635 -- To view, visit https://review.coreboot.org/c/flashrom/+/57581 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Icb8d19fd6771ed787b488044aa5ed5516812b27c Gerrit-Change-Number: 57581 Gerrit-PatchSet: 3 Gerrit-Owner: David Hendricks Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> 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-MessageType: abandon

1 0

Change in flashrom[master]: ich_descriptors: Explicitly check for Ibex Peak ISL value
by David Hendricks (Code Review) 14 Sep '21

14 Sep '21

David Hendricks has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/57580 ) Change subject: ich_descriptors: Explicitly check for Ibex Peak ISL value ...................................................................... Abandoned Superceded by CB:57633 -- To view, visit https://review.coreboot.org/c/flashrom/+/57580 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8a880251a393666757f3879629a165b7fe3533ef Gerrit-Change-Number: 57580 Gerrit-PatchSet: 1 Gerrit-Owner: David Hendricks Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> 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: David Hendricks <david.hendricks(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: abandon

1 0

Change in flashrom[master]: tests: Revise mock chip definition and usage
by Anastasia Klimchuk (Code Review) 14 Sep '21

14 Sep '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57437 ) Change subject: tests: Revise mock chip definition and usage ...................................................................... Patch Set 4: (5 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/57437/comment/4a98c823_50bdcd8d PS3, Line 7: tests: Make chip definitions static global > The summary line should somehow cover all the changes. If you […] Done, I also upgraded commit message to mention everything that is going on here File tests/chip.c: https://review.coreboot.org/c/flashrom/+/57437/comment/d78479c2_7112e9c8 PS3, Line 24: #define CHIP_TOTAL_SIZE 8388608 > Please make use of the KiB/MiB macros. This should be `(8*MiB)` (usually […] Wow useful macros. I didn't know of them, probably because I was looking into flashchips.c which doesn't use those macros. One thing I wanted to ask: I didn't use macros for chip_W25Q128_V, I wanted it to look closer to flashchips.c definition, am I right? https://review.coreboot.org/c/flashrom/+/57437/comment/406feaff_0ef53cf2 PS3, Line 24: CHIP_TOTAL_SIZE > There's another chip below with a different size. Should we name it […] Done, I only made it a little bit shorter, MOCK_CHIP_SIZE. https://review.coreboot.org/c/flashrom/+/57437/comment/e0a8b26d_0a6aa52f PS3, Line 128: static struct flashchip chip_8MiB = { > Should we make them `const` and then work on a copy on the stack? […] I followed your advice exactly (made them const, and fresh copy for every test), but please tell me if this implementation is what you had in mind. For example, it is not a deep copy. https://review.coreboot.org/c/flashrom/+/57437/comment/9d8d982d_e130299a PS3, Line 130: .total_size = 8 * 1024, > CHIP_TOTAL_SIZE / KiB Done -- To view, visit https://review.coreboot.org/c/flashrom/+/57437 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia9b5fc71e30610684e68e9aca9fb1970da8f840a Gerrit-Change-Number: 57437 Gerrit-PatchSet: 4 Gerrit-Owner: 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-Comment-Date: Tue, 14 Sep 2021 05:53:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add tests to read from chip
by Anastasia Klimchuk (Code Review) 14 Sep '21

14 Sep '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57326 ) Change subject: tests: Add tests to read from chip ...................................................................... Patch Set 4: (2 comments) File tests/tests.c: https://review.coreboot.org/c/flashrom/+/57326/comment/ad5068e5_ded42cc4 PS3, Line 185: len > This is `nmemb` (according to manpage). `len` is easy to confuse with […] Done https://review.coreboot.org/c/flashrom/+/57326/comment/b8d17273_fe4e306a PS3, Line 188: size * len > This should return just `len` (actually `nmemb`). It only works because […] Thank you! I would probably discover this at some point later for some other future test, and it would take much longer time to debug/understand where the mistake is. -- To view, visit https://review.coreboot.org/c/flashrom/+/57326 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia57781ebc670c7bd6197e56fe8a20651a425c756 Gerrit-Change-Number: 57326 Gerrit-PatchSet: 4 Gerrit-Owner: 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-Comment-Date: Tue, 14 Sep 2021 05:41:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add tests to read from chip
by Anastasia Klimchuk (Code Review) 14 Sep '21

14 Sep '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/57326 to look at the new patch set (#4). Change subject: tests: Add tests to read from chip ...................................................................... tests: Add tests to read from chip Two tests cover the code which performs do_read operation. First one works with fake chip and dummy programmer. Fake chip has all operations defined, and a buffer to emulate chip memory. Second one uses the chip which is closer to the real one, because read/write/unlock/erase operations are real. The tests takes the advantage of dummyflasher's capability of emulating a W25Q128.V chip. BUG=b:181803212 TEST=builds and ninja test Change-Id: Ia57781ebc670c7bd6197e56fe8a20651a425c756 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M tests/chip.c M tests/io_mock.h M tests/meson.build M tests/tests.c M tests/tests.h 5 files changed, 146 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/26/57326/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/57326 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia57781ebc670c7bd6197e56fe8a20651a425c756 Gerrit-Change-Number: 57326 Gerrit-PatchSet: 4 Gerrit-Owner: 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: tests: Revise mock chip definition and usage
by Anastasia Klimchuk (Code Review) 14 Sep '21

14 Sep '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/57437 to look at the new patch set (#4). Change subject: tests: Revise mock chip definition and usage ...................................................................... tests: Revise mock chip definition and usage This patch is doing few things: 1) Makes chip definitions static global so that they can be reused between test functions. 2) Promotes existing mock chip from 8KiB to 8MiB and eraseblocks are expanded accordingly. Old value of 8KiB was very small and it was confusing. Mock chip looks more realistic now. 3) Uses KiB and MiB macros from flash.h for mock chip definition 4) Renames CHIP_TOTAL_SIZE to MOCK_CHIP_SIZE to avoid confusion (there is also a W25Q128.V chip in the tests) 5) Makes chip definitions const so that every test can work on a fresh copy on the stack. BUG=b:181803212 TEST=builds and ninja test Change-Id: Ia9b5fc71e30610684e68e9aca9fb1970da8f840a Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M tests/chip.c 1 file changed, 60 insertions(+), 117 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/37/57437/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/57437 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia9b5fc71e30610684e68e9aca9fb1970da8f840a Gerrit-Change-Number: 57437 Gerrit-PatchSet: 4 Gerrit-Owner: 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: ich_descriptors: Explicitly check for Ibex Peak ISL value
by David Hendricks (Code Review) 14 Sep '21

14 Sep '21

Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/57633 to look at the new patch set (#2). Change subject: ich_descriptors: Explicitly check for Ibex Peak ISL value ...................................................................... ich_descriptors: Explicitly check for Ibex Peak ISL value This patch does the following: - Checks for Ibex Peak more explicitly, as ifdtool currently does. - Unconditionally print the "peculiar flash descriptor" warning if none of the chipsets are detected explicitly, since checking `!= 16` is redundant with the above checks. Change-Id: I2b6301c6cd08f6822340d61a3454b31fddc3be52 Signed-off-by: David Hendricks <ddaveh(a)amazon.com> --- M ich_descriptors.c 1 file changed, 7 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/33/57633/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/57633 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2b6301c6cd08f6822340d61a3454b31fddc3be52 Gerrit-Change-Number: 57633 Gerrit-PatchSet: 2 Gerrit-Owner: David Hendricks Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: Add support for Intel Emmitsburg PCH
by David Hendricks (Code Review) 14 Sep '21

14 Sep '21

Attention is currently required from: Jonathan Zhang. Hello Jonathan Zhang, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/57635 to review the following change. Change subject: Add support for Intel Emmitsburg PCH ...................................................................... Add support for Intel Emmitsburg PCH This patch does the following: - Add PCIe ID for Intel Emmitsburg PCH - Based on ICH descriptor content, choose CHIPSET_C620_SERIES_LEWISBURG. Although EBG only uses 80 for its PSL value, LBG on Purley and Whitley use 139. So to cover both cases the latter is used. TESTED=tried on a server with Intel Emmitsburg PCH, flash update was successful. This server, however, does not have flash chip installed, it instead has em100 emulator connected. Change-Id: I1edae9163b910c3831adc82c90840fa965479451 Signed-off-by: Jonathan Zhang <jonzhang(a)fb.com> Signed-off-by: David Hendricks <ddaveh(a)amazon.com> --- M chipset_enable.c M ich_descriptors.c 2 files changed, 4 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/35/57635/1 diff --git a/chipset_enable.c b/chipset_enable.c index 638c05f..cb0fbf1 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -2073,6 +2073,7 @@ {0x8086, 0xa247, B_S, NT, "Intel", "C620 Series Chipset Supersku", enable_flash_c620}, {0x8086, 0xa248, B_S, NT, "Intel", "C620 Series Chipset Supersku", enable_flash_c620}, {0x8086, 0xa249, B_S, NT, "Intel", "C620 Series Chipset Supersku", enable_flash_c620}, + {0x8086, 0x1bca, B_S, NT, "Intel", "Emmitsburg Chipset SKU", enable_flash_c620}, {0x8086, 0xa2c4, B_S, NT, "Intel", "H270", enable_flash_pch100}, {0x8086, 0xa2c5, B_S, NT, "Intel", "Z270", enable_flash_pch100}, {0x8086, 0xa2c6, B_S, NT, "Intel", "Q270", enable_flash_pch100}, diff --git a/ich_descriptors.c b/ich_descriptors.c index c3b30a7..652e6e3 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -941,6 +941,9 @@ warn_peculiar_desc(content->ISL != 23, "Gemini Lake"); return CHIPSET_GEMINI_LAKE; } + /* Emmitsburg has 80, Lewisburg on Purley and Whitley has 139 */ + if (content->ISL <= 139) + return CHIPSET_C620_SERIES_LEWISBURG; /* * Ibex Peak is historically used as the default when other * chipsets are not identified. -- To view, visit https://review.coreboot.org/c/flashrom/+/57635 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1edae9163b910c3831adc82c90840fa965479451 Gerrit-Change-Number: 57635 Gerrit-PatchSet: 1 Gerrit-Owner: David Hendricks Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: tests: Extract setup and teardown for chip tests
by Edward O'Callaghan (Code Review) 14 Sep '21

14 Sep '21

Attention is currently required from: Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57436 ) Change subject: tests: Extract setup and teardown for chip tests ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/57436 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If59315646f06344664df08b145866d9ce846d751 Gerrit-Change-Number: 57436 Gerrit-PatchSet: 2 Gerrit-Owner: 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 14 Sep 2021 04:31:49 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Revert "Add support for Intel Emmitsburg PCH"
by David Hendricks (Code Review) 14 Sep '21

14 Sep '21

Attention is currently required from: Arthur Heymans, Nico Huber, Jonathan Zhang, Johnny Lin, David Hendricks, Christian Walter, Stefan Reinauer, Deomid "rojer" Ryabkov, Tim Chu. David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57589 ) Change subject: Revert "Add support for Intel Emmitsburg PCH" ...................................................................... Patch Set 2: (1 comment) Patchset: PS1: > I think there is a bit of cross talking here. Indeed, thanks for cutting through it :-) > Let's just let the revert go though please, please remember commits are just hashes and are free we don't need to fight over this and we can re-land this once you addressed all the comments in a follow up patch that fixes the concerns. Sure, I've created a follow-up that should address the concerns, incorporating various feedback from this and other patches. LMK what you think. > Sorry all for the double post, one thing that I forgot to mention. Is there room here to perhaps write a unit-test as well as `guess_ich_chipset()` seems to be a pure function without side-effects? Seems tedious but doable. AFAICT we'll basically give it one set of constants (ISLs, etc) and expect corresponding constants (CHIPSET_*) as outputs. Is that really valuable, or am I thinking of it incorrectly? Do we have an example that tests a similar function? -- To view, visit https://review.coreboot.org/c/flashrom/+/57589 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4ebb7a931cfa66276df2f762c63e6d092d6b3d5a Gerrit-Change-Number: 57589 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: David Hendricks Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Comment-Date: Tue, 14 Sep 2021 04:18:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit September 2021