December 2023 - flashrom-gerrit

[S] Change in flashrom[main]: flash: move nested struct to top level
by Anastasia Klimchuk (Code Review) 03 Dec '23

03 Dec '23

Anastasia Klimchuk has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/62999?usp=email ) Change subject: flash: move nested struct to top level ...................................................................... Abandoned The patch looks abandoned with unresolved comments and no responses for >1yr. Patch can be restored later if needed. -- To view, visit https://review.coreboot.org/c/flashrom/+/62999?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I89988e4a6a7ca65866ba019320e480e491389d56 Gerrit-Change-Number: 62999 Gerrit-PatchSet: 1 Gerrit-Owner: Tom Hughes <tomhughes(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-MessageType: abandon

1 0

[XS] Change in flashrom[main]: dummyflasher: Fix JEDEC_WRSR2 and JEDEC_WRSR3 emulation
by Anastasia Klimchuk (Code Review) 03 Dec '23

03 Dec '23

Attention is currently required from: Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79390?usp=email ) Change subject: dummyflasher: Fix JEDEC_WRSR2 and JEDEC_WRSR3 emulation ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: I am wondering, is it useful to have a test for this scenario? -- To view, visit https://review.coreboot.org/c/flashrom/+/79390?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ia0e51a9b8dbd1f67cb947e3ec73f64b6d30cbb97 Gerrit-Change-Number: 79390 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Sun, 03 Dec 2023 09:19:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[main]: dummyflasher: make RO status of SPI_SR_WEL explicit
by Anastasia Klimchuk (Code Review) 03 Dec '23

03 Dec '23

Attention is currently required from: Angel Pons, Nico Huber, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60224?usp=email ) Change subject: dummyflasher: make RO status of SPI_SR_WEL explicit ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: Sergii, I found this old patch and I am not sure it's needed anymore, because the code has changed a lot since then. The only thing that might be need is test, although if yes it should go to a different file, `chip_wp.c` with other wp tests. If you think this patch is not needed anymore, them maybe mark it abandoned. Also while I was looking at the current code in the dummyflasher I made another patch CB:79390 maybe you can have a look. Thank you! -- To view, visit https://review.coreboot.org/c/flashrom/+/60224?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I5dce77e22a1f9cd29f40ec0881d2b1238a985e97 Gerrit-Change-Number: 60224 Gerrit-PatchSet: 1 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> 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-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Sun, 03 Dec 2023 09:19:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: dummyflasher: Fix JEDEC_WRSR2 and JEDEC_WRSR3 emulation
by Anastasia Klimchuk (Code Review) 03 Dec '23

03 Dec '23

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/79390?usp=email ) Change subject: dummyflasher: Fix JEDEC_WRSR2 and JEDEC_WRSR3 emulation ...................................................................... dummyflasher: Fix JEDEC_WRSR2 and JEDEC_WRSR3 emulation JEDEC_WRSR already emulated STATUS1 and STATUS2 correctly, but JEDEC_WRSR2 and JEDEC_WRSR3 were handled by a different case branch. Change-Id: Ia0e51a9b8dbd1f67cb947e3ec73f64b6d30cbb97 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M dummyflasher.c 1 file changed, 2 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/90/79390/1 diff --git a/dummyflasher.c b/dummyflasher.c index cf4ca03..efe5720 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -605,7 +605,7 @@ ro_bits = get_reg_ro_bit_mask(data, STATUS2); data->emu_status[1] &= ro_bits; - data->emu_status[1] |= (writearr[1] & ~ro_bits); + data->emu_status[1] |= (writearr[2] & ~ro_bits); msg_pdbg2("WRSR2 wrote 0x%02x.\n", data->emu_status[1]); @@ -621,7 +621,7 @@ ro_bits = get_reg_ro_bit_mask(data, STATUS3); data->emu_status[2] &= ro_bits; - data->emu_status[2] |= (writearr[1] & ~ro_bits); + data->emu_status[2] |= (writearr[3] & ~ro_bits); msg_pdbg2("WRSR3 wrote 0x%02x.\n", data->emu_status[2]); break; -- To view, visit https://review.coreboot.org/c/flashrom/+/79390?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ia0e51a9b8dbd1f67cb947e3ec73f64b6d30cbb97 Gerrit-Change-Number: 79390 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newchange

1 0

[XS] Change in flashrom[main]: cli_output.c: Format progress output to show in single line
by Anastasia Klimchuk (Code Review) 03 Dec '23

03 Dec '23

Anastasia Klimchuk has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/64668?usp=email ) Change subject: cli_output.c: Format progress output to show in single line ...................................................................... Abandoned -- To view, visit https://review.coreboot.org/c/flashrom/+/64668?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: If982193f9857f0da47cd71706ce6cebaa60d4323 Gerrit-Change-Number: 64668 Gerrit-PatchSet: 3 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Evan Benn <evanbenn(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Richard Hughes <richard(a)hughsie.com> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: abandon

1 0

[XS] Change in flashrom[main]: cli_output.c: Format progress output to show in single line
by Anastasia Klimchuk (Code Review) 03 Dec '23

03 Dec '23

Attention is currently required from: Aarya, Daniel Campello. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64668?usp=email ) Change subject: cli_output.c: Format progress output to show in single line ...................................................................... Patch Set 3: (1 comment) Patchset: PS3: Hello Aarya and everyone, I am looking at this patch right now and I think to mark it abandoned. Firstly because it seems like it is indeed, but also because I am planning to change the progress indicator soon-ish (next year). I understand the idea of the patch, but I want to start from different side: for example I don't think the word "completed" needs to be repeated with every % , and same for operation name (it is enough to say READ started... READ completed, without repeating it with every %). After that maybe the issue this patch is trying to solve, will go away naturally. And in any case, thanks everyone for the effort <3 -- To view, visit https://review.coreboot.org/c/flashrom/+/64668?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: If982193f9857f0da47cd71706ce6cebaa60d4323 Gerrit-Change-Number: 64668 Gerrit-PatchSet: 3 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Evan Benn <evanbenn(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Richard Hughes <richard(a)hughsie.com> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Sun, 03 Dec 2023 07:48:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: buspirate_spi: Add support for hiz output with pullups=off
by Dreg (Code Review) 01 Dec '23

01 Dec '23

Attention is currently required from: Anastasia Klimchuk, Thomas Heijligen. Dreg has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79299?usp=email ) Change subject: buspirate_spi: Add support for hiz output with pullups=off ...................................................................... Patch Set 6: Code-Review+1 (1 comment) Patchset: PS6: > David, thank you for the patch! I have a question. […] Hello! good points, When someone enables 'pullups=on', the internal pull-ups are activated (powered from a VPU input pin), making the MOSI, MISO, etc., pins function as an open collector (hi-z) using these pull-ups. Therefore, 'pullups=on' inherently works as an open collector. Thus, setting both 'pullups=on' and 'hiz=on' is redundant but not incorrect. However, considering human error, I'm unsure if we should flag this as an error. What do you think? Would you like me to commit this change? BUT! A common and dangerous mistake is when people incorrectly use 'pullups' arg: 'pullups=1' or 'pullups=enable'. In such cases, flashrom continues to run without using the pull-ups or open collector, sending 3.3V through the SPI pins, which can damage 1.8V or 2.5V chips. Therefore, I believe if we detect a syntax error in 'pullups=' or 'hiz=', we should flag an error and not continue running flashrom. Would you like me to commit this change? -- To view, visit https://review.coreboot.org/c/flashrom/+/79299?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6d9909b23782ef3fcb1428fd127dae0d6b211855 Gerrit-Change-Number: 79299 Gerrit-PatchSet: 6 Gerrit-Owner: Dreg <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Dreg <dreg(a)rootkit.es> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 01 Dec 2023 18:35:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: erasure_layout: Remove redundant `verify_range` call from `erase_write`
by Vincent Fazio (Code Review) 01 Dec '23

01 Dec '23

Attention is currently required from: Aarya, Anastasia Klimchuk, Carly Zlabek. Vincent Fazio has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79354?usp=email ) Change subject: erasure_layout: Remove redundant `verify_range` call from `erase_write` ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Carly, thank you so much for the patch! You are right, there is a redundant verification. […] Anastasia, Thanks for taking the time to look this over. I will discuss this with Carly later today and try to schedule time to collect this data. Our company leverages flashrom pretty heavily and we're grateful for the project so are happy to contribute patches as we can. I think I agree that it makes sense to drop that `check_erased_range` call as it should be duplicating the smaller range checks in 341-344 but I would like us to make sure that we're not missing verification coverage there due to some weird loop quirk. The other option is to just call `check_erased_range` once for the entire range instead of multiple times on smaller chunks to avoid the overhead of the function calls, the multiple mallocs, and the flash reads.. though it does require more contiguous memory to do it just once which may put pressure on memory constrained systems. We can try benchmarking both. -- That said, I would like to make a case for why these proposed changes should still occur independent of the orphaned `check_erased_range` change. When programming an empty chip (scenario 3 in the testing info), it's not the orphaned `check_erased_range` that causes problems as the chip is blank and we never enter that loop, it's the `verity_range` over the entire layout after the writes are performed. In our case, this causes a ~40% reduction in programming performance (140sec -> 200sec) which would affect our board yields compared to flashrom 1.2. As mentioned in the commit message, `flashrom_flash_write` [already verifies writes when instructed](https://review.coreboot.org/plugins/gitiles/flashrom/+/refs/hea… and does so based on flags set in `flashctx` so the call here is both unavoidable (because it doesn't respect caller set flags) and redundant (because it duplicates what's already being done by `flashrom_flash_write`). While it's possible to keep the `verify_range` check in `erase_write`, it doesn't seem ideal as `flashrom_flash_write` already has a more "global" view and appropriately handles both full chip verification and verification by layout. If we kept it here in `erase_write`, it would need to be guarded by `flashctx` flags and not overlap with any checks in `flashrom_flash_write`. It would also make the verification logic disjointed and more difficult to maintain as they would no longer be colocated in `flashrom_flash_write`. -- As a side note, it could be argued that the erase path should also respect the verify flag(s), but I think that's a larger discussion to happen for a separate patchset and would be a change in behavior. [This FIXME here](https://review.coreboot.org/plugins/gitiles/flashrom/+/refs/heads/mai… seems to imply there's not much benefit in announcing erase failures, so performing verification seems like additional overhead for that operation, but I think it make senses to make any potentially destructive APIs, such as erase and write, consistent with respect to verification. So either they respect the verify flags in `flashctx` or API users, including the CLI, should independently invoke verification after calling erase/write. Currently (without this patch), for testing scenario 1 (overwriting an image with a new image) where we're erasing _and_ writing, I think we're verifying data 4 times: 1) `check_erased_range` unconditionally on small chunks 2) `check_erased_range` unconditionally on the entire erase block 3) `verify_range` unconditionally in `erase_write` after writing data 4) the verification flagged to be performed in `flashrom_flash_write` -- To view, visit https://review.coreboot.org/c/flashrom/+/79354?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I638835facd9311979c4991cc4ca41a4b9e174bd5 Gerrit-Change-Number: 79354 Gerrit-PatchSet: 1 Gerrit-Owner: Carly Zlabek <carlyzlabek(a)gmail.com> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Vincent Fazio <vfazio(a)gmail.com> Gerrit-Attention: Carly Zlabek <carlyzlabek(a)gmail.com> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 01 Dec 2023 13:05:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[main]: Add Idaville platform into chipset enable list and add update IRC reg...
by Anastasia Klimchuk (Code Review) 01 Dec '23

01 Dec '23

Attention is currently required from: Kevin Yu. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79287?usp=email ) Change subject: Add Idaville platform into chipset enable list and add update IRC region support ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: I also found there is an earlier patch that you sent CB:67438 which very sadly got missed :\ I apologise for this. Let's pick that up, I don't want your work to be lost. The both patches are about Idaville support, are they compatible, do you need both? -- To view, visit https://review.coreboot.org/c/flashrom/+/79287?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I15bb3bb368156b078547ce0bdea556c7ab87f729 Gerrit-Change-Number: 79287 Gerrit-PatchSet: 1 Gerrit-Owner: Kevin Yu <ykevin(a)celestica.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Kevin Yu <ykevin(a)celestica.com> Gerrit-Comment-Date: Fri, 01 Dec 2023 12:16:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: ati: add polaris22
by Anastasia Klimchuk (Code Review) 01 Dec '23

01 Dec '23

Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/29222?usp=email ) Change subject: ati: add polaris22 ...................................................................... Patch Set 5: (1 comment) Patchset: PS3: > I have rebased the patches, but I sadly do not have old enough devices to test. Jiajie, thank you for your effort of rebasing! Not much that we can do without the devices to test. Original author abandoned the chain. On the other hand, from the comments someone else in Aug 2020 tested this, but it's a while ago, don't know if they return back. Let's keep it as is for now. Abandoned patches are not deleted, and can be restored. -- To view, visit https://review.coreboot.org/c/flashrom/+/29222?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ia4950bb098a2bb8dba7b10bb9b2b6012c79365ea Gerrit-Change-Number: 29222 Gerrit-PatchSet: 5 Gerrit-Owner: Luc Verhaegen <libv(a)skynet.be> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Chen <c(a)jia.je> Gerrit-Reviewer: Name of user not set #1003051 Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Comment-Date: Fri, 01 Dec 2023 11:02:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Chen <c(a)jia.je> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit December 2023