September 2021 - flashrom-gerrit

Change in flashrom[master]: Add support for Intel Emmitsburg PCH
by Nico Huber (Code Review) 12 Sep '21

12 Sep '21

Attention is currently required from: Jonathan Zhang. Nico Huber has created a revert of this change. ( https://review.coreboot.org/c/flashrom/+/54965 ) Change subject: Add support for Intel Emmitsburg PCH ...................................................................... -- To view, visit https://review.coreboot.org/c/flashrom/+/54965 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2a1bb7467e693d1583aa885fa0e277075edd4a3e Gerrit-Change-Number: 54965 Gerrit-PatchSet: 3 Gerrit-Owner: Jonathan Zhang <jonzhang(a)fb.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: Nico Huber <nico.h(a)gmx.de> 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: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-MessageType: revert

1 0

Change in flashrom[master]: Revert "Add support for Intel Emmitsburg PCH"
by Nico Huber (Code Review) 12 Sep '21

12 Sep '21

Attention is currently required from: Arthur Heymans, Jonathan Zhang, Johnny Lin, Christian Walter, David Hendricks, Stefan Reinauer, Edward O'Callaghan, Deomid "rojer" Ryabkov, Tim Chu. Hello Arthur Heymans, build bot (Jenkins), Jonathan Zhang, Johnny Lin, David Hendricks, Christian Walter, Stefan Reinauer, Edward O'Callaghan, David Hendricks, Deomid "rojer" Ryabkov, Tim Chu, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/57589 to review the following change. Change subject: Revert "Add support for Intel Emmitsburg PCH" ...................................................................... Revert "Add support for Intel Emmitsburg PCH" This reverts commit 51e1d0e4b7670e5822560acc724a6a8dd00b6af4. Breaks Ibex Peak descriptor detection. Change-Id: I4ebb7a931cfa66276df2f762c63e6d092d6b3d5a Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- M chipset_enable.c M ich_descriptors.c 2 files changed, 0 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/89/57589/1 diff --git a/chipset_enable.c b/chipset_enable.c index 110d7a9..cdd51ae 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -2073,7 +2073,6 @@ {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 d56ade7..aba7528 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -939,8 +939,6 @@ warn_peculiar_desc(content->ISL != 23, "Gemini Lake"); return CHIPSET_GEMINI_LAKE; } - if (content->ISL <= 80) - return CHIPSET_C620_SERIES_LEWISBURG; warn_peculiar_desc(content->ISL != 16, "Ibex Peak"); return CHIPSET_5_SERIES_IBEX_PEAK; } else if (upper->MDTBA == 0x00) { -- 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: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> 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: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: David Hendricks Gerrit-Attention: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: Add support for Intel Emmitsburg PCH
by Nico Huber (Code Review) 12 Sep '21

12 Sep '21

Attention is currently required from: Jonathan Zhang. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54965 ) Change subject: Add support for Intel Emmitsburg PCH ...................................................................... Patch Set 3: (2 comments) Patchset: PS1: > Yes, and IMO we should find a better heuristic or require the user to specify a ICH/PCH generation o […] I'm not talking about the detection. I mean if anybody checked that all the code activated by this patch matches the hardware and descriptor. IOW if all the paths chosen for CHIPSET_C620_SERIES_LEWISBURG are suitable or just work by chance for rudimentary flashing. Now that I looked at the merged change, it seems obvious that it breaks Ibex Peak detection. Its SPI guide is 403598. I'm not sure if Intel still keeps these files. File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/54965/comment/7e85bdc5_740ff3f3 PS3, Line 943: return CHIPSET_C620_SERIES_LEWISBURG; Was this reviewed at all? It assumes that CHIPSET_5_SERIES_IBEX_PEAK has ISL > 80. Which seems unlikely given the low numbers around here. In the one descriptor file I have around ISL is 16. -- To view, visit https://review.coreboot.org/c/flashrom/+/54965 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2a1bb7467e693d1583aa885fa0e277075edd4a3e Gerrit-Change-Number: 54965 Gerrit-PatchSet: 3 Gerrit-Owner: Jonathan Zhang <jonzhang(a)fb.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: Nico Huber <nico.h(a)gmx.de> 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: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Comment-Date: Sun, 12 Sep 2021 16:26:04 +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> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dediprog.c: Drop dediprog_ prefix for spi data struct members
by Nico Huber (Code Review) 12 Sep '21

12 Sep '21

Attention is currently required from: Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56415 ) Change subject: dediprog.c: Drop dediprog_ prefix for spi data struct members ...................................................................... Patch Set 4: Code-Review+2 (1 comment) File dediprog.c: https://review.coreboot.org/c/flashrom/+/56415/comment/101a9f51_e5272551 PS4, Line 1058: Looks like one tab too much now, maybe it would even look better w/o a line break or with an earlier one. -- To view, visit https://review.coreboot.org/c/flashrom/+/56415 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I688d50926b78a6c3f1c5a8ba4ef88a0d5b495bd0 Gerrit-Change-Number: 56415 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 12 Sep 2021 14:52:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dediprog.c: Refactor singleton states into reentrant pattern
by Nico Huber (Code Review) 12 Sep '21

12 Sep '21

Attention is currently required from: Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56414 ) Change subject: dediprog.c: Refactor singleton states into reentrant pattern ...................................................................... Patch Set 4: Code-Review+2 (1 comment) File dediprog.c: https://review.coreboot.org/c/flashrom/+/56414/comment/5610a75d_898c9e2e PS3, Line 221: const int finish, > I used "at least after the opening parenthesis" option. […] I don't think there is a written rule. There are three common ways to indent parameter lists, I think, by decreasing popularity (by my own perception): /* Aligned right after the opening parenthesis */ void some_function(int some_param, int another_param) /* All aligned to two tabs */ void function_with_a_long_name_that_leaves_little_space( int some_param, int another_param) /* Aligned after the opening parenthesis, with tabs only */ void some_function(int some_param, int another_param) I'd say for review, it just shouldn't look obviously wrong. To me, dediprog_read() still does. If I had to extract a rule from the most common practice it would be this: All parameter declarations should be aligned to the same column, but never with the function body (hence the two tabs in the second example and not just one). That would apply to the first two examples above. -- To view, visit https://review.coreboot.org/c/flashrom/+/56414 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I72085e750af97b94dfa94f2ebf2a134e41a2ec8d Gerrit-Change-Number: 56414 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 12 Sep 2021 14:48:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Make chip definitions static global
by Nico Huber (Code Review) 12 Sep '21

12 Sep '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57437 ) Change subject: tests: Make chip definitions static global ...................................................................... Patch Set 3: (6 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/57437/comment/a07f7a08_8db02933 PS3, Line 7: tests: Make chip definitions static global The summary line should somehow cover all the changes. If you do much in a single commit, it might simply end up as a vague "tests: Revise chip definitions". https://review.coreboot.org/c/flashrom/+/57437/comment/f839994d_210c4d9d PS3, Line 13: was confusing. Mock chip looks more realistic now. Generally, moving and changing code in a single commit often makes the review unnecessarily harder. Not too bad in this case. File tests/chip.c: https://review.coreboot.org/c/flashrom/+/57437/comment/c4df88a7_c453873f PS3, Line 24: CHIP_TOTAL_SIZE There's another chip below with a different size. Should we name it `MOCKED_CHIP_SIZE` maybe? https://review.coreboot.org/c/flashrom/+/57437/comment/e3e24329_c22654e1 PS3, Line 24: #define CHIP_TOTAL_SIZE 8388608 Please make use of the KiB/MiB macros. This should be `(8*MiB)` (usually written without spaces around the asterisk). https://review.coreboot.org/c/flashrom/+/57437/comment/d733ef15_1b8a06fa PS3, Line 128: static struct flashchip chip_8MiB = { Should we make them `const` and then work on a copy on the stack? That way we'd have a fresh copy for every test. https://review.coreboot.org/c/flashrom/+/57437/comment/70b14686_488e1b5a PS3, Line 130: .total_size = 8 * 1024, CHIP_TOTAL_SIZE / KiB -- 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: 3 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-Comment-Date: Sun, 12 Sep 2021 12:29:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add tests to read from chip
by Nico Huber (Code Review) 12 Sep '21

12 Sep '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57326 ) Change subject: tests: Add tests to read from chip ...................................................................... Patch Set 3: Code-Review+1 (3 comments) File tests/chip.c: https://review.coreboot.org/c/flashrom/+/57326/comment/198dbcb9_4301177b PS1, Line 246: }; > Yes right, they started to repeat once more tests have been added. I created CB:57437 for this. Thanks. Often it's better to clean up first, though. Doesn't matter for this chain anymore ;) My rule of thumb: if you have to touch many lines again later in the chain, the order is probably wrong. File tests/tests.c: https://review.coreboot.org/c/flashrom/+/57326/comment/53985f31_e98f9d1f PS3, Line 185: len This is `nmemb` (according to manpage). `len` is easy to confuse with a length in bytes. https://review.coreboot.org/c/flashrom/+/57326/comment/3c8d2e0e_cfa0d517 PS3, Line 188: size * len This should return just `len` (actually `nmemb`). It only works because flashrom passes 1 as `size`. -- 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: 3 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-Comment-Date: Sun, 12 Sep 2021 12:17:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Extract setup and teardown for chip tests
by Nico Huber (Code Review) 12 Sep '21

12 Sep '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nico Huber 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 (1 comment) File tests/chip.c: https://review.coreboot.org/c/flashrom/+/57436/comment/de80f98a_0fc8f33c PS1, Line 29: g_chip_state > Good idea, totally makes sense! I was going to do it quickly, but then I realised I don't know how t […] Alas, there is no proper abstraction yet. Some of the field of struct flashrom_flashctx look chip specific (e.g. those about SPI 4BA), so looks like there is a reason for another data pointer even without the tests. -- 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: 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: Sun, 12 Sep 2021 12:02:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: board_enable.c: Add ME unlock function for TUXEDO laptops
by Nico Huber (Code Review) 12 Sep '21

12 Sep '21

Attention is currently required from: Felix Singer, Michał Żygowski, Paul Menzel, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56023 ) Change subject: board_enable.c: Add ME unlock function for TUXEDO laptops ...................................................................... Patch Set 8: (1 comment) Patchset: PS7: > Some feedback about the current state of the code vs open discussions would be appreciated. […] I'm still confused why this is done deep in flashrom on the programmer path and not in a separate binary (which could share any code necessary ofc). If you could provide some details on the overall workflow, that could get the discussion started I guess. -- To view, visit https://review.coreboot.org/c/flashrom/+/56023 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6bf1c40900aa674e3ea4f6add12dae8b73759fbb Gerrit-Change-Number: 56023 Gerrit-PatchSet: 8 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sun, 12 Sep 2021 11:31:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: acpi_ec: Implement basic ACPI embedded controller API
by Nico Huber (Code Review) 12 Sep '21

12 Sep '21

Attention is currently required from: Michał Żygowski, Angel Pons, Michael Niewöhner. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55714 ) Change subject: acpi_ec: Implement basic ACPI embedded controller API ...................................................................... Patch Set 8: (2 comments) File acpi_ec.h: https://review.coreboot.org/c/flashrom/+/55714/comment/5cbc35a9_b182327e PS4, Line 41: bool ec_wait_for_ibuf(uint8_t control_port, unsigned int max_checks); : bool ec_wait_for_obuf(uint8_t control_port, unsigned int max_checks); > Done Still there. File acpi_ec.h: https://review.coreboot.org/c/flashrom/+/55714/comment/867b2d82_c7ca7902 PS1, Line 41: bool ec_wait_for_ibuf(uint8_t control_port); : bool ec_wait_for_obuf(uint8_t control_port, unsigned int max_checks); > The erase command required longer timeout on output buffer flag. […] So, I've been looking through the code of the follow ups. There is indeed only this one case where a non-default value is passed. That you need a higher timeout there is clear. But why do you need a lower one for every- thing else? The calls would look much cleaner without this parameter, or at least by making use of the default by passing 0. And we wouldn't have to expose EC_MAX_STATUS_CHECKS in any case. -- To view, visit https://review.coreboot.org/c/flashrom/+/55714 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie3bae8d81c80ae2f286b619e974869e3f2f4545d Gerrit-Change-Number: 55714 Gerrit-PatchSet: 8 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Comment-Date: Sun, 12 Sep 2021 11:21:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit September 2021