June 2021 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: nicintel_spi.c: Implement `set_sck_set_mosi` and `set_sck_get_miso`
by Angel Pons (Code Review) 10 Jun '21

10 Jun '21

Angel Pons has restored this change. ( https://review.coreboot.org/c/flashrom/+/49267 ) Change subject: nicintel_spi.c: Implement `set_sck_set_mosi` and `set_sck_get_miso` ...................................................................... Restored -- To view, visit https://review.coreboot.org/c/flashrom/+/49267 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I52a0ae5ff331ee2ed41652eb5c2ed7ebe7253d74 Gerrit-Change-Number: 49267 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Shawn C <citypw(a)hardenedlinux.org> Gerrit-CC: Xiang W <wxjstz(a)126.com> Gerrit-MessageType: restore

1 0

Change in flashrom[master]: mcp6x_spi.c: Implement `set_sck_set_mosi` and `set_sck_get_miso`
by Angel Pons (Code Review) 10 Jun '21

10 Jun '21

Angel Pons has restored this change. ( https://review.coreboot.org/c/flashrom/+/49266 ) Change subject: mcp6x_spi.c: Implement `set_sck_set_mosi` and `set_sck_get_miso` ...................................................................... Restored -- To view, visit https://review.coreboot.org/c/flashrom/+/49266 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie86cc9d4df6c7f0609c30ff00d6f2d77fbb8b836 Gerrit-Change-Number: 49266 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Shawn C <citypw(a)hardenedlinux.org> Gerrit-CC: Xiang W <wxjstz(a)126.com> Gerrit-MessageType: restore

1 0

Change in flashrom[master]: rayer_spi.c: Implement `set_sck_set_mosi` operation
by Angel Pons (Code Review) 10 Jun '21

10 Jun '21

Angel Pons has restored this change. ( https://review.coreboot.org/c/flashrom/+/49265 ) Change subject: rayer_spi.c: Implement `set_sck_set_mosi` operation ...................................................................... Restored -- To view, visit https://review.coreboot.org/c/flashrom/+/49265 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ieb05a35967685c2444d064cb61cec98e2a00f590 Gerrit-Change-Number: 49265 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Shawn C <citypw(a)hardenedlinux.org> Gerrit-CC: Xiang W <wxjstz(a)126.com> Gerrit-MessageType: restore

1 0

Change in flashrom[master]: bitbang-spi.c: support clock polarity and phase
by Nico Huber (Code Review) 10 Jun '21

10 Jun '21

Attention is currently required from: Xiang W, Edward O'Callaghan, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/49255 ) Change subject: bitbang-spi.c: support clock polarity and phase ...................................................................... Patch Set 26: (1 comment) File bitbang_spi.c: https://review.coreboot.org/c/flashrom/+/49255/comment/93f32fe3_53aa1fb6 PS26, Line 121: programmer_delay(master->half_period); : bitbang_spi_set_sck_set_mosi(master, !cpol, (val >> i) & 1); : programmer_delay(master->half_period); : bitbang_spi_set_sck(master, cpol); This does not look right and is probably what led to the follow up commit. With cpha=0, the receiver is supposed to sample the data at the first edge. So the sequence should be: * set mosi * delay * first edge * delay * second edge * (followed without delay by the next mosi setting) -- To view, visit https://review.coreboot.org/c/flashrom/+/49255 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I04c1dfe132d756119229b27c3cd611d1be1abc8d Gerrit-Change-Number: 49255 Gerrit-PatchSet: 26 Gerrit-Owner: Xiang W <wxjstz(a)126.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: Stefan Reinauer <stefan.reinauer(a)coreboot.org> 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-CC: Shawn C <citypw(a)hardenedlinux.org> Gerrit-Attention: Xiang W <wxjstz(a)126.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Thu, 10 Jun 2021 20:21:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: bitbang-spi.c: support clock polarity and phase
by Nico Huber (Code Review) 10 Jun '21

10 Jun '21

Attention is currently required from: Xiang W, Edward O'Callaghan, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/49255 ) Change subject: bitbang-spi.c: support clock polarity and phase ...................................................................... Patch Set 26: (8 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/49255/comment/fba2547c_d605eaa9 PS1, Line 8: > Sorry I misunderstood. The change does two things: 1. implements a better mode 0, 2. adds the option of other modes. The former is most welcome. I also like to get the whole patch merged, under one condition: The commit message has to give a reasonable incentive to add the optional modes. For instance, what chip needs it to be supported? and what other changes are in the queue to make flashrom support it? Currently all the common code assumes that the programmer either implements mode 0 or 3 and that all flash chips support both. If we need to support other chips, the common code would have to change too, right? Commit Message: https://review.coreboot.org/c/flashrom/+/49255/comment/af59a92c_a15486b2 PS26, Line 11: may not be This "may not be" is rather vague. Do we know a real-world use case? Patchset: PS26: Sorry, I'm very late to this review. There is still the early question about the reason for the change. Basically, do we have a real-world use case? File bitbang_spi.c: https://review.coreboot.org/c/flashrom/+/49255/comment/147d1383_28ee3080 PS26, Line 171: _set_mosi Removing the spurious MOSI settings seems fine, but it should be mentioned in the commit message (or happen in a preceding commit). https://review.coreboot.org/c/flashrom/+/49255/comment/cad0abf5_d7823283 PS26, Line 34: data transmission Technically, it's the data sampling (or capturing, as Wikipedia puts it) that happens, not the transmission. https://review.coreboot.org/c/flashrom/+/49255/comment/b4c52ed0_ba3e8580 PS26, Line 96: bitbang_spi_set_sck(master, !cpol); The first clock change always needs to happen with a delay after asserting CS#. Both cpha cases should start with a delay. https://review.coreboot.org/c/flashrom/+/49255/comment/c2ff835a_2f5e6f13 PS26, Line 184: if (strlen(mode)) { So an empty string is not reported as error? https://review.coreboot.org/c/flashrom/+/49255/comment/e5719916_50b36152 PS26, Line 187: mode == endptr This does not account for trailing garbage. It's better to check for the null termination, e.g. if (*mode == '\0' || *endptr != '\0' || v > 3 || v < 0) { (this includes checking for an empty string) -- To view, visit https://review.coreboot.org/c/flashrom/+/49255 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I04c1dfe132d756119229b27c3cd611d1be1abc8d Gerrit-Change-Number: 49255 Gerrit-PatchSet: 26 Gerrit-Owner: Xiang W <wxjstz(a)126.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: Stefan Reinauer <stefan.reinauer(a)coreboot.org> 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-CC: Shawn C <citypw(a)hardenedlinux.org> Gerrit-Attention: Xiang W <wxjstz(a)126.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Thu, 10 Jun 2021 20:13:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Xiang W <wxjstz(a)126.com> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: programmer_table: move each entry to the associated programmer source
by Angel Pons (Code Review) 10 Jun '21

10 Jun '21

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52946 ) Change subject: programmer_table: move each entry to the associated programmer source ...................................................................... Patch Set 12: (1 comment) File programmer.h: https://review.coreboot.org/c/flashrom/+/52946/comment/73431de3_961b68e3 PS4, Line 402: #if CONFIG_DUMMY == 1 > Please just drop all these `#if`. The only difference it makes is > whether we get a compile-time or link-time error if something goes > wrong. I don't think it matters or even justifies the clutter. > > Mentioning the file name is also a bit redundant. Maybe just a single > comment /* programmer drivers */ ? > > Doesn't have to be in this patch, though CB:55387 > Thank you so much! Your long message is very helpful, I think now I actually understand: extern declaration by itself is fine, as long as all the usages are guarded. I made CB:55386 on coreboot earlier today. It introduces a helper function to transform a compile-time error into a link-time error, which allows dropping some preprocessor usage in code. I just forced build errors by not enabling `DEBUG_BOOT_STATE` and manually inverting the condition in src/lib/hardwaremain.c in order to show you the difference between a compile-time error and a link-time error. Compile-time error excerpt: src/lib/hardwaremain.c: In function 'bs_call_callbacks': src/lib/hardwaremain.c:279:15: error: 'struct boot_state_callback' has no member named 'location' bscb, bscb->location); The `location` member is inside a text block guarded by a conditional preprocessor directive. Since the `CONFIG(DEBUG_BOOT_STATE)` condition is false, the preprocessor does not pass the contents of this text block to the compiler, and the compiler does not "see" the `location` member in the struct definition. When it reaches the `bscb->location` part, the compiler complains about the non-existent `location` member. Link-time error excerpt: coreboot-builds/HP_280_G2/generated/ramstage.o: in function `bscb_location': src/include/bootstate.h:114: undefined reference to `_dead_code_assertion_failed' The `dead_code_t` macro inside the `bscb_location` function evaluates to some expression that calls the `_dead_code_assertion_failed` function, which is intentionally never defined (only declared). Compilation succeeds: the source code can be translated into a `hardwaremain.o` object file, which contains external definitions and references for linking. However, when the linker tries to resolve the external references, it cannot find a definition for the `_dead_code_assertion_failed` function, and bails out. The advantage of link-time errors over compile-time errors (i.e. what CB:55386 leverages) is that the compiler can automatically optimise out the code that would cause a link-time error. CB:55386 works because `bscb_location` is never reached when the `CONFIG(DEBUG_BOOT_STATE)` condition is false, and the compiler can thus optimise `bscb_location` away along with the `_dead_code_assertion_failed` function call. -- To view, visit https://review.coreboot.org/c/flashrom/+/52946 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3d02bd789f0299e936eb86819b3b15b5ea2bb921 Gerrit-Change-Number: 52946 Gerrit-PatchSet: 12 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Xiang W <wxjstz(a)126.com> Gerrit-Comment-Date: Thu, 10 Jun 2021 14:14:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No 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]: programmer.h: remove compile guard from programmer drivers
by Angel Pons (Code Review) 10 Jun '21

10 Jun '21

Attention is currently required from: Nico Huber, Thomas Heijligen, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55387 ) Change subject: programmer.h: remove compile guard from programmer drivers ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/55387 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I01e6a785269c3e0bd648eeaee217a7a855ab0853 Gerrit-Change-Number: 55387 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 10 Jun 2021 13:27:52 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashchips.c: Mark MT25QL256 as tested
by Nico Huber (Code Review) 10 Jun '21

10 Jun '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/50720 ) Change subject: flashchips.c: Mark MT25QL256 as tested ...................................................................... flashchips.c: Mark MT25QL256 as tested As mentioned in mail from Bernd.Stoeferle(a)elbitsystems-de.com on 22.12.2020. Change-Id: Ie49332333f49a40f7bd8f3b5e42a8e2ad6995618 Signed-off-by: Simon Buhrow <simon.buhrow(a)posteo.de> Reviewed-on: https://review.coreboot.org/c/flashrom/+/50720 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> --- M flashchips.c 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved diff --git a/flashchips.c b/flashchips.c index ef5ea5e..950f09b 100644 --- a/flashchips.c +++ b/flashchips.c @@ -11644,7 +11644,7 @@ /* supports SFDP */ /* OTP: 64B total; read 0x4B, write 0x42 */ .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_4BA_WREN, - .tested = TEST_UNTESTED, + .tested = TEST_OK_PREW, .probe = probe_spi_rdid, .probe_timing = TIMING_ZERO, .block_erasers = -- To view, visit https://review.coreboot.org/c/flashrom/+/50720 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie49332333f49a40f7bd8f3b5e42a8e2ad6995618 Gerrit-Change-Number: 50720 Gerrit-PatchSet: 4 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: buspirate_spi.c: Separate shutdown from failed init cleanup
by Angel Pons (Code Review) 10 Jun '21

10 Jun '21

Attention is currently required from: Nico Huber, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52877 ) Change subject: buspirate_spi.c: Separate shutdown from failed init cleanup ...................................................................... Patch Set 4: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/52877 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I04d57e2552d23a1a4a906c68539f0ccefc13e8a0 Gerrit-Change-Number: 52877 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: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 10 Jun 2021 13:25:15 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: buspirate_spi.c: Separate shutdown from failed init cleanup
by Angel Pons (Code Review) 10 Jun '21

10 Jun '21

Attention is currently required from: Nico Huber, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52877 ) Change subject: buspirate_spi.c: Separate shutdown from failed init cleanup ...................................................................... Patch Set 4: Code-Review+1 (1 comment) File buspirate_spi.c: https://review.coreboot.org/c/flashrom/+/52877/comment/3038fc96_58ceddfa PS4, Line 620: ret = 1; For another patch: We could get rid of these `ret = 1;` statements and just use the existing non-zero value. -- To view, visit https://review.coreboot.org/c/flashrom/+/52877 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I04d57e2552d23a1a4a906c68539f0ccefc13e8a0 Gerrit-Change-Number: 52877 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: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 10 Jun 2021 13:25:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit June 2021