December 2022 - flashrom-gerrit

[M] Change in flashrom[master]: ichspi: Expose flash descriptor regions through get_region()
by Nikolai Artemiev (Code Review) 07 Dec '22

07 Dec '22

Attention is currently required from: Edward O'Callaghan. Hello build bot (Jenkins), Edward O'Callaghan, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/70127 to look at the new patch set (#9). Change subject: ichspi: Expose flash descriptor regions through get_region() ...................................................................... ichspi: Expose flash descriptor regions through get_region() Region attributes are now stored in a `fd_regions` array after being decoded in ich9_handle_frap() and used by ich_get_region(). A special cases is handled in ich_get_region(): if there is a gap between two flash regions, an artificial region is created to fill the gap. I.e. any address inside the gap will return a region that spans the gap between the end the of the previous region and the start of the next region. This allows ich_get_region() to be used to iterate the entire flash region-by-region. Read and write operations are assumed to be allowed inside gaps between regions. BUG=b:260440773 BRANCH=none TEST=flashrom -{r,w,E,v} on dedede (JSL) Change-Id: I019f3f407f6a2a82f686a168457e0e32961ff483 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M ichspi.c 1 file changed, 99 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/27/70127/9 -- To view, visit https://review.coreboot.org/c/flashrom/+/70127 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I019f3f407f6a2a82f686a168457e0e32961ff483 Gerrit-Change-Number: 70127 Gerrit-PatchSet: 9 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[master]: flashchips.c: remove WREN from GD25Q256D enter 4BA sequence
by Nikolai Artemiev (Code Review) 07 Dec '22

07 Dec '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Eric Lai. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70342 ) Change subject: flashchips.c: remove WREN from GD25Q256D enter 4BA sequence ...................................................................... Patch Set 3: (4 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/70342/comment/33bf1f6c_00b366e9 PS1, Line 9: CB:35480 > please use commit hash. Done https://review.coreboot.org/c/flashrom/+/70342/comment/a81c539c_958bf2d4 PS1, Line 15: Coreboot issue > Ticket […] Done https://review.coreboot.org/c/flashrom/+/70342/comment/cf221c18_bc8a6aa5 PS1, Line 19: FT2232H > This programmer also supports native 4BA commands which are probably […] Ok interesting, I wrote a little test case: ``` static void print_sr(struct flashctx *flash) { uint8_t value; int ret = spi_read_register(flash, STATUS1, &value); printf("SR1=0x%02x ret=%d\n", value, ret); ret = spi_read_register(flash, STATUS2, &value); printf("SR2=0x%02x ret=%d\n", value, ret); ret = spi_read_register(flash, STATUS3, &value); printf("SR3=0x%02x ret=%d\n\n", value, ret); } static void test(struct flashctx *flash) { printf("Power-up state:\n"); print_sr(flash); int ret = spi_enter_exit_4ba(flash, false); printf("Exit 4BA ret=%d\n", ret); print_sr(flash); ret = spi_enter_exit_4ba(flash, true); printf("Enter 4BA ret=%d\n", ret); print_sr(flash); ret = spi_enter_exit_4ba(flash, false); printf("Exit 4BA ret=%d\n", ret); print_sr(flash); } ``` Here are the results: ``` # FEATURE_4BA: Power-up state: SR1=0x00 ret=0 SR2=0x00 ret=0 SR3=0x20 ret=0 Exit 4BA ret=0 SR1=0x00 ret=0 SR2=0x00 ret=0 SR3=0x20 ret=0 Enter 4BA ret=0 SR1=0x00 ret=0 SR2=0x01 ret=0 SR3=0x20 ret=0 Exit 4BA ret=0 SR1=0x00 ret=0 SR2=0x00 ret=0 SR3=0x20 ret=0 ``` ``` # FEATURE_4BA_WREN: Power-up state: SR1=0x00 ret=0 SR2=0x00 ret=0 SR3=0x20 ret=0 Exit 4BA ret=0 SR1=0x02 ret=0 SR2=0x00 ret=0 SR3=0x20 ret=0 Enter 4BA ret=0 SR1=0x02 ret=0 SR2=0x01 ret=0 SR3=0x20 ret=0 Exit 4BA ret=0 SR1=0x02 ret=0 SR2=0x00 ret=0 SR3=0x20 ret=0 ``` So it is able to set the ADS bit with or without WREN. The bit that's always set in SR3 is DRV0, which defaults to 1 according to the datasheet. Just to be sure I set it to 0 (i.e. `spi_write_register(flash, STATUS3, 0)` and reran the test: ``` # FEATURE_4BA, DRV0 cleared: Power-up state: SR1=0x00 ret=0 SR2=0x00 ret=0 SR3=0x00 ret=0 Exit 4BA ret=0 SR1=0x00 ret=0 SR2=0x00 ret=0 SR3=0x00 ret=0 Enter 4BA ret=0 SR1=0x00 ret=0 SR2=0x01 ret=0 SR3=0x00 ret=0 Exit 4BA ret=0 SR1=0x00 ret=0 SR2=0x00 ret=0 SR3=0x00 ret=0 ``` Patchset: PS1: > You forgot to add the topic! I am marking this unresolved so that the comment is more visible. Done -- To view, visit https://review.coreboot.org/c/flashrom/+/70342 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I96e48933f33c52c0d10a0d4cb7f7e07c1fceab99 Gerrit-Change-Number: 70342 Gerrit-PatchSet: 3 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> 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: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Comment-Date: Wed, 07 Dec 2022 02:28:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: flashchips.c: remove WREN from GD25Q256D enter 4BA sequence
by Nikolai Artemiev (Code Review) 07 Dec '22

07 Dec '22

Attention is currently required from: Edward O'Callaghan, Eric Lai, Nikolai Artemiev. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/70342 to look at the new patch set (#3). Change subject: flashchips.c: remove WREN from GD25Q256D enter 4BA sequence ...................................................................... flashchips.c: remove WREN from GD25Q256D enter 4BA sequence As noted in a comment on `commit 86fc9cf7ab221bc54ef6f10252e296fc2d7a22d2`, the GD25Q256D datasheet indicates that the chip does not require a WREN command to enter 4BA mode. Testing has confirmed that a WREN command is not required, so change the flashchip feature flags from FEATURE_4BA_WREN to FEATURE_4BA. Ticket: https://ticket.coreboot.org/issues/356 BUG=none BRANCH=none TEST=read/write/erase/verify GD25Q256D flash with FT2232H programmer TEST=called spi_enter_exit_4ba(true), dumped registers, checked ADS=1. Change-Id: I96e48933f33c52c0d10a0d4cb7f7e07c1fceab99 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M flashchips.c 1 file changed, 26 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/42/70342/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/70342 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I96e48933f33c52c0d10a0d4cb7f7e07c1fceab99 Gerrit-Change-Number: 70342 Gerrit-PatchSet: 3 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> 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: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: tests: Add llvm-cov option and run target for code coverage
by Anastasia Klimchuk (Code Review) 07 Dec '22

07 Dec '22

Attention is currently required from: Thomas Heijligen, Evan Benn. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69268 ) Change subject: tests: Add llvm-cov option and run target for code coverage ...................................................................... Patch Set 14: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/69268/comment/504779e2_6690f6b8 PS11, Line 14: TEST=meson test; ninja llvm-cov-tests > I ran it for all the test_build configs with coverage enabled, anad with and without tests. I meant to say, test scenario == "build flashrom and run tests with coverage disabled" (from your comment & commit message it sounds like you always run with coverage enabled) File meson.build: https://review.coreboot.org/c/flashrom/+/69268/comment/0050fe45_d70391e3 PS14, Line 625: run_target('llvm-cov-cli', command : ['scripts/llvm-cov', classic_cli]) But why this is always run? It should only run when feature is enabled? -- To view, visit https://review.coreboot.org/c/flashrom/+/69268 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id6c73bff46e7b88d425956a80def97082b201f56 Gerrit-Change-Number: 69268 Gerrit-PatchSet: 14 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-Comment-Date: Wed, 07 Dec 2022 02:09:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: ichspi.c: Read chip ID and use it to populate `flash->chip`
by Edward O'Callaghan (Code Review) 07 Dec '22

07 Dec '22

Attention is currently required from: Subrata Banik, Angel Pons, Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69195 ) Change subject: ichspi.c: Read chip ID and use it to populate `flash->chip` ...................................................................... Patch Set 12: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/69195/comment/f77e52e6_e081f504 PS2, Line 7: ichspi.c: Read chip ID and use it to identify chip > I've made flash identification bail out and print a message if there's more than one chip now. […] Subrata can you take a look too please. -- To view, visit https://review.coreboot.org/c/flashrom/+/69195 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia408e1e45dc6f53c0934afd6558e301abfa48ee6 Gerrit-Change-Number: 69195 Gerrit-PatchSet: 12 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 07 Dec 2022 02:04:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: ichspi.c: Read chip ID and use it to populate `flash->chip`
by Edward O'Callaghan (Code Review) 07 Dec '22

07 Dec '22

Attention is currently required from: Subrata Banik, Angel Pons, Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69195 ) Change subject: ichspi.c: Read chip ID and use it to populate `flash->chip` ...................................................................... Patch Set 12: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/69195 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia408e1e45dc6f53c0934afd6558e301abfa48ee6 Gerrit-Change-Number: 69195 Gerrit-PatchSet: 12 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 07 Dec 2022 02:03:14 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom: Skip read/write/erase/verify ops on inaccessable flash regions
by Edward O'Callaghan (Code Review) 07 Dec '22

07 Dec '22

Attention is currently required from: Angel Pons, Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70128 ) Change subject: flashrom: Skip read/write/erase/verify ops on inaccessable flash regions ...................................................................... Patch Set 13: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/70128/comment/7adb2614_e3456aa0 PS6, Line 11: platforms with active an CSME coprocessor. > I agree, silently skipping regions could easily go unnoticed by users and probably isn't what most u […] Would it help to split this up into read/write/erase/verify ops? I feel like 'erase' and 'read' are reasonably safe to skip on as the former is expected to be destructive and the latter is specifically non-destructive. -- To view, visit https://review.coreboot.org/c/flashrom/+/70128 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9b96fb04b863625d2c9f9a00b97c35b3ddb0871b Gerrit-Change-Number: 70128 Gerrit-PatchSet: 13 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 07 Dec 2022 02:01:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom: Skip read/write/erase/verify ops on inaccessable flash regions
by Edward O'Callaghan (Code Review) 07 Dec '22

07 Dec '22

Attention is currently required from: Angel Pons, Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70128 ) Change subject: flashrom: Skip read/write/erase/verify ops on inaccessable flash regions ...................................................................... Patch Set 13: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/70128/comment/584b28be_48dbf1e5 PS13, Line 579: i use `addr` not `i`, it is more descriptive, index is prone to error. -- To view, visit https://review.coreboot.org/c/flashrom/+/70128 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9b96fb04b863625d2c9f9a00b97c35b3ddb0871b Gerrit-Change-Number: 70128 Gerrit-PatchSet: 13 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 07 Dec 2022 01:58:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom: Skip read/write/erase/verify ops on inaccessable flash regions
by Edward O'Callaghan (Code Review) 07 Dec '22

07 Dec '22

Attention is currently required from: Angel Pons, Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70128 ) Change subject: flashrom: Skip read/write/erase/verify ops on inaccessable flash regions ...................................................................... Patch Set 13: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/70128/comment/d5a34e70_edbc0005 PS13, Line 348: region->name = strdup(""); > This (and also strdup in ichspi) can leak memory. […] can `region->name` not be NULL and treat that as the empty string? -- To view, visit https://review.coreboot.org/c/flashrom/+/70128 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9b96fb04b863625d2c9f9a00b97c35b3ddb0871b Gerrit-Change-Number: 70128 Gerrit-PatchSet: 13 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 07 Dec 2022 01:56:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tests: Detect llvm coverage run and redirect to real I/O functions
by Anastasia Klimchuk (Code Review) 07 Dec '22

07 Dec '22

Attention is currently required from: Evan Benn. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69267 ) Change subject: tests: Detect llvm coverage run and redirect to real I/O functions ...................................................................... Patch Set 13: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/69267 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I21cc1d631e92fa19006b967e85676f108e80b307 Gerrit-Change-Number: 69267 Gerrit-PatchSet: 13 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-Comment-Date: Wed, 07 Dec 2022 01:53:15 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit December 2022