- flashrom-gerrit - mail.coreboot.org

[S] Change in flashrom[main]: flashchips: add GD25F128F
by Victor Lim (Code Review) 27 Oct '24

27 Oct '24

Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev, Stefan Reinauer. Hello Anastasia Klimchuk, Nikolai Artemiev, Stefan Reinauer, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/84827?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: flashchips: add GD25F128F ...................................................................... flashchips: add GD25F128F GD25F128F: 3V 128Mbit, high performance Tested on ch347 with erase, write, read, and protection Corrected the OTP statement. Change-Id: I14c0905f50e92492287d50f8377790b997c4acfe Signed-off-by: Victor <vlim(a)gigadevice.com> --- M flashchips.c M include/flashchips.h 2 files changed, 46 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/27/84827/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/84827?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I14c0905f50e92492287d50f8377790b997c4acfe Gerrit-Change-Number: 84827 Gerrit-PatchSet: 2 Gerrit-Owner: Victor Lim <vlim(a)gigadevice.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>

1 0

[S] Change in flashrom[main]: flashchips: add GD25F128F
by Victor Lim (Code Review) 27 Oct '24

27 Oct '24

Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev, Stefan Reinauer. Victor Lim has posted comments on this change by Victor Lim. ( https://review.coreboot.org/c/flashrom/+/84827?usp=email ) Change subject: flashchips: add GD25F128F ...................................................................... Patch Set 1: (1 comment) File flashchips.c: https://review.coreboot.org/c/flashrom/+/84827/comment/20ac5f55_2a70703e?us… : PS1, Line 8199: OTP: 1024B total, 256B reserved > I have a question, why it is 1024B total, and what does it mean "reserved"? […] Thanks. I need to make a correction here. It is 3x2048 bytes. Each can be erased and be programed. -- To view, visit https://review.coreboot.org/c/flashrom/+/84827?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I14c0905f50e92492287d50f8377790b997c4acfe Gerrit-Change-Number: 84827 Gerrit-PatchSet: 1 Gerrit-Owner: Victor Lim <vlim(a)gigadevice.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sun, 27 Oct 2024 02:04:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[XS] Change in flashrom[main]: linux_mtd: fix build with clang >= 19
by Anastasia Klimchuk (Code Review) 26 Oct '24

26 Oct '24

Attention is currently required from: Arnaud Ferraris, Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change by Arnaud Ferraris. ( https://review.coreboot.org/c/flashrom/+/84856?usp=email ) Change subject: linux_mtd: fix build with clang >= 19 ...................................................................... Patch Set 1: Code-Review+2 (1 comment) Patchset: PS1: Arnaud, thank you for the patch! I am interested, whether you ran the tests on your environment, and if yes did they pass? Since as I understand you can get more warnings on your environment in comparison with our CI, so I am wondering how the tests would run. -- To view, visit https://review.coreboot.org/c/flashrom/+/84856?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: If470a65702e9ae08e4303123a0014e53a1fee56e Gerrit-Change-Number: 84856 Gerrit-PatchSet: 1 Gerrit-Owner: Arnaud Ferraris <arnaud.ferraris(a)collabora.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Arnaud Ferraris <arnaud.ferraris(a)collabora.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sat, 26 Oct 2024 13:44:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes

1 0

[M] Change in flashrom[main]: erasure_layout: Add an option to sacrifice unchanged blocks for speed
by Anastasia Klimchuk (Code Review) 26 Oct '24

26 Oct '24

Attention is currently required from: Aarya, Bill XIE. Anastasia Klimchuk has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84721?usp=email ) Change subject: erasure_layout: Add an option to sacrifice unchanged blocks for speed ...................................................................... Patch Set 5: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/84721/comment/f57f2ed5_a9fec48d?us… : PS5, Line 9: Someone may prefer to sacrifice sub-blocks needless to change within a : larger erase block for programming speed. These people who are okay to : fry their chip sooner can set this option. If the percentage of : sub-blocks needless to change within a larger erase block is lower : than the given value, the whole larger erase block will be erased. I wrote another version of commit message, what do you think? I can update myself, but first I wanted to ask what do you think: (commit title is fine) === The patch adds command line option to handle the following situation: There is a region which is requested to be erased (or written, because the write operation uses erase too). Some of the areas inside this region don't need to be erased, because the bytes already have expected value. Such areas can be skipped. The logic selects eraseblocks that can cover the areas which need to be erased. Suppose there is a region which is partially covered by eraseblocks of size S (partially because remaining areas don't need to be erased). Now suppose we can cover the whole region with eraseblock of larger size, S+1, and erase it all at once. This will run faster: erase opcode will only be sent once instead of many smaller opcodes. However, this will run erase over some areas of the chip memory that didn't need to be erased. Which means, the chip, as a hardware, will wear faster. New command line option sets the maximum % memory that is allowed for redundant erase. Default is 0, S+1 size block only selected if all the area needs to be erased in full. 50 means that if more than a half of the area needs to be erased, a S+1 size block can be selected to cover all area with one block. The tradeoff is the speed of programming operation VS the longevity of the chip. Default is longevity. File cli_classic.c: https://review.coreboot.org/c/flashrom/+/84721/comment/2d1f60bb_1cd94751?us… : PS3, Line 124: " --sacrifice-ratio <ratio> The percentage of overall sub-blocks needless to\n" : " change within a larger erase block to be sacrificed\n" : " for programming speed, default 0, max 100\n" I have an idea, what do you think: > Maximum % memory that is allowed for redundant erase, valid range 0 to 50. Default is 0, S+1 size block only selected if all the area needs to be erased in full. 50 means that if more than a half of the area needs to be erased, a S+1 size block can be selected to cover all area with one block. The tradeoff is the speed of programming operation VS the longevity of the chip. Default is longevity. DANGEROUS! It wears your chip faster! -- To view, visit https://review.coreboot.org/c/flashrom/+/84721?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I154e8a713f626c37dbbe118db700055b96d24803 Gerrit-Change-Number: 84721 Gerrit-PatchSet: 5 Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org> 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-Attention: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Sat, 26 Oct 2024 13:04:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[L] Change in flashrom[main]: Complete and fix progress feature implementation for all operations
by Anastasia Klimchuk (Code Review) 26 Oct '24

26 Oct '24

Attention is currently required from: Aarya, Nikolai Artemiev, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change by Sergii Dmytruk. ( https://review.coreboot.org/c/flashrom/+/84102?usp=email ) Change subject: Complete and fix progress feature implementation for all operations ...................................................................... Patch Set 13: Code-Review+2 (1 comment) Patchset: PS13: I just remembered, from our guidelines if there are multiple authors, each should approve each other work. I approve! -- To view, visit https://review.coreboot.org/c/flashrom/+/84102?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: If1e40fc97f443c4f0c0501cef11cff1f3f84c051 Gerrit-Change-Number: 84102 Gerrit-PatchSet: 13 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Sat, 26 Oct 2024 10:19:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes

1 0

[M] Change in flashrom[main]: Display progress for what is actually erased/written
by Anastasia Klimchuk (Code Review) 26 Oct '24

26 Oct '24

Attention is currently required from: Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84439?usp=email ) Change subject: Display progress for what is actually erased/written ...................................................................... Patch Set 16: (4 comments) Patchset: PS16: Just out of curiosity I ran the test case from here https://review.coreboot.org/c/flashrom/+/84614/1..3//COMMIT_MSG#b19 but with added progress and freq param. That other patch is not about progress indicator, but it is a deep dive into erase/write logic, and in the test case there are various areas which need/need not to be written or erased. The progress is shown! Read and write eventually get to 100, erase did not (only got up to 50), I guess because only selective regions were needed to erase. Not blocking this patch, but it was interesting to check. It seems there is a room for improvement, but as I said I think not blocking this patch File flashrom.c: https://review.coreboot.org/c/flashrom/+/84439/comment/6a40e284_2f8ceb46?us… : PS13, Line 1270: // FIXME: round up to granularity? > I think `get_next_write` limits length to not try to suggest writing more data then there is. […] Okay so I will close this (and the other one). I removed FIXMEs, is this fine? https://review.coreboot.org/c/flashrom/+/84439/comment/b1635465_9cfb818c?us… : PS13, Line 1281: // FIXME: should be rounded up to page size? > It does take granularity into account so maybe it's fine as is, assuming that regions never cross bo […] Acknowledged https://review.coreboot.org/c/flashrom/+/84439/comment/c50461a4_b0e66f74?us… : PS13, Line 1463: // TODO: use erase_layout? > I want to try to do this, leaving this as comment Actually not, at this stage erase_layout is not useful: because it is only created but not initialised and more importantly, not processed for which erase blocks are selected for being run. There is no info for progress in erase_layout at this stage. -- To view, visit https://review.coreboot.org/c/flashrom/+/84439?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I88ac4d40f1b6ccc1636b1efb690d8d68bdebec08 Gerrit-Change-Number: 84439 Gerrit-PatchSet: 16 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: Sat, 26 Oct 2024 10:18:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>

1 0

[M] Change in flashrom[main]: Display progress for what is actually erased/written
by Anastasia Klimchuk (Code Review) 26 Oct '24

26 Oct '24

Attention is currently required from: Sergii Dmytruk. Hello Sergii Dmytruk, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/84439?usp=email to look at the new patch set (#16). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: Display progress for what is actually erased/written ...................................................................... Display progress for what is actually erased/written The patch updates calculation of total length for the operation which is displayed with progress. The reason is: even if, for example the whole chip erase or write was requested, the actual length of bytes modified can be less than whole chip size (areas which already have expected content, are skipped). Change-Id: I88ac4d40f1b6ccc1636b1efb690d8d68bdebec08 Co-developed-by: Anastasia Klimchuk <aklm(a)flashrom.org> Co-developed-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M doc/classic_cli_manpage.rst M flashrom.c M tests/chip.c M tests/tests.c M tests/tests.h 5 files changed, 113 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/39/84439/16 -- To view, visit https://review.coreboot.org/c/flashrom/+/84439?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I88ac4d40f1b6ccc1636b1efb690d8d68bdebec08 Gerrit-Change-Number: 84439 Gerrit-PatchSet: 16 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>

1 0

[M] Change in flashrom[main]: Display progress for what is actually erased/written
by Anastasia Klimchuk (Code Review) 26 Oct '24

26 Oct '24

Attention is currently required from: Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84439?usp=email ) Change subject: Display progress for what is actually erased/written ...................................................................... Patch Set 15: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/84439/comment/cdf62aec_7458f8f7?us… : PS7, Line 1265: // TODO: take FEATURE_NO_ERASE into account? > I like the second approach better too. […] Inspired by your suggestion, I made a solution which maybe not perfect, but at least it's simple (lines 1285-1290) :) Total gets doubled for writing for no_erase chips, and then the rest running as normal. -- To view, visit https://review.coreboot.org/c/flashrom/+/84439?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I88ac4d40f1b6ccc1636b1efb690d8d68bdebec08 Gerrit-Change-Number: 84439 Gerrit-PatchSet: 15 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: Sat, 26 Oct 2024 08:51:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>

1 0

[M] Change in flashrom[main]: Display progress for what is actually erased/written
by Anastasia Klimchuk (Code Review) 26 Oct '24

26 Oct '24

Attention is currently required from: Anastasia Klimchuk. Hello Sergii Dmytruk, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/84439?usp=email to look at the new patch set (#15). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: Display progress for what is actually erased/written ...................................................................... Display progress for what is actually erased/written The patch updates calculation of total length for the operation which is displayed with progress. The reason is: even if, for example the whole chip erase or write was requested, the actual length of bytes modified can be less than whole chip size (areas which already have expected content, are skipped). Change-Id: I88ac4d40f1b6ccc1636b1efb690d8d68bdebec08 Co-developed-by: Anastasia Klimchuk <aklm(a)flashrom.org> Co-developed-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M doc/classic_cli_manpage.rst M flashrom.c M tests/chip.c M tests/tests.c M tests/tests.h 5 files changed, 117 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/39/84439/15 -- To view, visit https://review.coreboot.org/c/flashrom/+/84439?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I88ac4d40f1b6ccc1636b1efb690d8d68bdebec08 Gerrit-Change-Number: 84439 Gerrit-PatchSet: 15 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: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[XS] Change in flashrom[main]: writeprotect: Fix inaccurate return code
by Anastasia Klimchuk (Code Review) 26 Oct '24

26 Oct '24

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/84826?usp=email ) Change subject: writeprotect: Fix inaccurate return code ...................................................................... writeprotect: Fix inaccurate return code If hardware protection is requested but not supported by the flash chip, return an error code indicating that the protection mode is unsupported, rather than indicating that all WP features are unsupported. TEST=ninja test Change-Id: I29e9069c7781fbb238f30aa9a9285b692b0c7200 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/84826 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M writeprotect.c 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Anastasia Klimchuk: Looks good to me, approved build bot (Jenkins): Verified diff --git a/writeprotect.c b/writeprotect.c index 411089d..964c311 100644 --- a/writeprotect.c +++ b/writeprotect.c @@ -482,7 +482,7 @@ case FLASHROM_WP_MODE_HARDWARE: if (!bits->srp_bit_present) - return FLASHROM_WP_ERR_CHIP_UNSUPPORTED; + return FLASHROM_WP_ERR_MODE_UNSUPPORTED; bits->srl = 0; bits->srp = 1; -- To view, visit https://review.coreboot.org/c/flashrom/+/84826?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I29e9069c7781fbb238f30aa9a9285b692b0c7200 Gerrit-Change-Number: 84826 Gerrit-PatchSet: 3 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: 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>

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit