March 2024 - flashrom-gerrit - mail.coreboot.org

[S] Change in flashrom[main]: ich: Add names for region 5, 9, 10, 11, 12, 13, 15
by Anastasia Klimchuk (Code Review) 23 Mar '24

23 Mar '24

Attention is currently required from: Hsuan Ting Chen, Hsuan-ting Chen, Stefan Reinauer. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81356?usp=email ) Change subject: ich: Add names for region 5, 9, 10, 11, 12, 13, 15 ...................................................................... Patch Set 1: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/81356/comment/c037f319_9bad52f5 : PS1, Line 12: * Incorporate missing region names from https://github.com/coreboot/coreboot/blob/main/util/ifdtool/ifdtool.c for completeness. Thank you for syncing region names in two places! I am curious, is there any official doc from Intel on the region names? I see that the names are from coreboot code, which is good. But I assume coreboot code was written based on some doc? Do you happen to know? File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/81356/comment/fde7f702_a542e10d : PS1, Line 521: msg_pdbg2(" FD BIOS ME GbE Pltf DE BIOS2 Reg7 EC DE2 "); For this line (and diffs below in this file) you have changed the number of spaces between names? So now the whole line is shown as diff, even though the first 5 names haven't changed. If there a reason to change the number of spaces? -- To view, visit https://review.coreboot.org/c/flashrom/+/81356?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: I3b164ce4ae84bfd523fcd8be416c5d13183ed632 Gerrit-Change-Number: 81356 Gerrit-PatchSet: 1 Gerrit-Owner: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Hsuan Ting Chen <roccochen(a)chromium.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Hsuan Ting Chen <roccochen(a)chromium.org> Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Comment-Date: Sat, 23 Mar 2024 07:45:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: serprog: Add SPI Mode and CS Mode commands
by Funkeleinhorn (Code Review) 23 Mar '24

23 Mar '24

Funkeleinhorn has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/81428?usp=email ) Change subject: serprog: Add SPI Mode and CS Mode commands ...................................................................... serprog: Add SPI Mode and CS Mode commands This commit adds two new commands to the serprog protocol which allow more fine grained control over the SPI bus. This enables more applications over serprog like e.g. flashing AVR microcontrollers. This can be tried with my forks of pico-serprog: https://github.com/funkeleinhorn/pico-serprog/tree/spimode and avrdude: https://github.com/funkeleinhorn/avrgirl/tree/serprog-programmer I announced this change in flashrom and flashprog IRC channels and got overall positive feedback in the flashprog channel. The same changes will be sent to flashprog to prevent diverging specs. Change-Id: Idb5a9a3710fede322def5191d68b7fba0e135292 Signed-off-by: Funkeleinhorn <git(a)funkeleinhorn.com> --- M Documentation/serprog-protocol.txt 1 file changed, 17 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/28/81428/1 diff --git a/Documentation/serprog-protocol.txt b/Documentation/serprog-protocol.txt index 3d4aa1a..d001ceb 100644 --- a/Documentation/serprog-protocol.txt +++ b/Documentation/serprog-protocol.txt @@ -36,6 +36,8 @@ 0x14 Set SPI clock frequency in Hz 32-bit requested frequency ACK + 32-bit set frequency / NAK 0x15 Toggle flash chip pin drivers 8-bit (0 disable, else enable) ACK / NAK 0x16 Set SPI Chip Select 8-bit ACK / NAK +0x17 Set SPI Mode 8-bit ACK / NAK +0x18 Set CS Mode 8-bit ACK / NAK 0x?? unimplemented command - invalid. @@ -93,6 +95,21 @@ 0x16 (S_SPI_CS): Set which SPI Chip Select pin to use. This operation is immediate, meaning it doesn't use the operation buffer. + 0x17 (S_SPI_MODE): + Set which SPI Mode to use for 0x13 O_SPIOP commands. + This operation is immediate, meaning it doesn't use the operation buffer. + The current defined modes are: + 0x00: SPI Half Duplex (default) + 0x01: SPI Full Duplex + 0x18 (S_CS_MODE): + Set which CS Mode to use. The CS Mode determines the CS behaviour. + This allows manual control over the CS. + This operation is immediate, meaning it doesn't use the operation buffer. + The current defined modes are: + 0x00: CS Auto Mode. The CS gets selected before 0x13 O_SPIOP commands and + deselected afterwards. (default) + 0x01: CS Selected. The CS will be selected until another mode is set. + 0x02: CS Deselected. The CS will be deselected until another mode is set. About mandatory commands: The only truly mandatory commands for any device are 0x00, 0x01, 0x02 and 0x10, but one can't really do anything with these commands. -- To view, visit https://review.coreboot.org/c/flashrom/+/81428?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: Idb5a9a3710fede322def5191d68b7fba0e135292 Gerrit-Change-Number: 81428 Gerrit-PatchSet: 1 Gerrit-Owner: Funkeleinhorn <git(a)funkeleinhorn.com> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[main]: flashchips: Add Zetta Device ZD25LQ128
by Anastasia Klimchuk (Code Review) 22 Mar '24

22 Mar '24

Attention is currently required from: Anton Samsonov, Nikolai Artemiev, Stefan Reinauer. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81392?usp=email ) Change subject: flashchips: Add Zetta Device ZD25LQ128 ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/81392?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: I5cb20158e81ab109f16958285b8787858efb4831 Gerrit-Change-Number: 81392 Gerrit-PatchSet: 1 Gerrit-Owner: Anton Samsonov <devel(a)zxlab.ru> 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: Anton Samsonov <devel(a)zxlab.ru> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Fri, 22 Mar 2024 09:51:13 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: flashchips: Add support for MXIC MX25L12850F
by Anastasia Klimchuk (Code Review) 22 Mar '24

22 Mar '24

Attention is currently required from: Alexander Goncharov, DZ, Nikolai Artemiev, Stefan Reinauer. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81350?usp=email ) Change subject: flashchips: Add support for MXIC MX25L12850F ...................................................................... Patch Set 1: Code-Review+1 (4 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/81350/comment/f4711261_b90961e8 : PS1, Line 10: wp Could you please mention the commands you ran? Since there are multiple commands for WP, if you list the ones you ran that would be great. Patchset: PS1: > MX25L12845E and MX25L12865E have no configuration register. Yes right, the difference is in configuration register which changes the way `.reg_bits` need to be defined for the chips. Existing entry for `MX25L12833F/MX25L12835F/MX25L12845E/MX25L12865E/MX25L12873F` has no reg_bits currently, but if there were to be added at some point that would be different than what MX25L12850F has. Meanwhile the newer model MX25L12850F can have all set of operations defined, so users can use all this and not be constrained by older models. For complete context, existing monolithic entry `MX25L12833F/MX25L12835F/MX25L12845E/MX25L12865E/MX25L12873F` can be split further because out of the five models, they have would slightly different reg_bits config. But they can be split later, if at some point write-protect support would be added to any of them (which means you would have to define exact reg_bits). Also Joursoir, I am so happy to see you here again !! :) PS1: Daniel thank you! I think it's much better to update one chip at a time, one per commit. So you did the right thing. File flashchips.c: https://review.coreboot.org/c/flashrom/+/81350/comment/581c3430_73fc5317 : PS1, Line 9051: MACRONIX_MX25L12805D Could you please append the model name (MX25L12850F) to a comment in `include/flashchips.h` for the macro `MACRONIX_MX25L12805D` ? I know it's a long comment already, but let's add one more id at the end of it. -- To view, visit https://review.coreboot.org/c/flashrom/+/81350?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: I71ac70d273904f94d015401f9d8df587084efad0 Gerrit-Change-Number: 81350 Gerrit-PatchSet: 1 Gerrit-Owner: DZ <danielzhang(a)mxic.com.cn> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> 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: DZ <danielzhang(a)mxic.com.cn> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Fri, 22 Mar 2024 07:50:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: DZ <danielzhang(a)mxic.com.cn> Comment-In-Reply-To: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: doc/dev_guide: Add section about Jenkins build, and scan-build
by Anastasia Klimchuk (Code Review) 22 Mar '24

22 Mar '24

Attention is currently required from: Peter Marheine, Stefan Reinauer, Thomas Heijligen. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81261?usp=email ) Change subject: doc/dev_guide: Add section about Jenkins build, and scan-build ...................................................................... Patch Set 2: (1 comment) File doc/dev_guide/development_guide.rst: https://review.coreboot.org/c/flashrom/+/81261/comment/5c5f90fc_2fc7a900 : PS1, Line 226: need to > "should" seems better, since you're saying authors should review the report but aren't required to. Done -- To view, visit https://review.coreboot.org/c/flashrom/+/81261?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: I416b632c55d1ceb925456ac8c8947dfbcef2e888 Gerrit-Change-Number: 81261 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Fri, 22 Mar 2024 06:48:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: doc/dev_guide: Add section about Jenkins build, and scan-build
by Anastasia Klimchuk (Code Review) 22 Mar '24

22 Mar '24

Attention is currently required from: Anastasia Klimchuk, Stefan Reinauer, Thomas Heijligen. Hello Peter Marheine, Stefan Reinauer, Thomas Heijligen, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/81261?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Code-Review+2 by Stefan Reinauer, Verified+1 by build bot (Jenkins) Change subject: doc/dev_guide: Add section about Jenkins build, and scan-build ...................................................................... doc/dev_guide: Add section about Jenkins build, and scan-build Change-Id: I416b632c55d1ceb925456ac8c8947dfbcef2e888 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M doc/dev_guide/development_guide.rst 1 file changed, 17 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/61/81261/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/81261?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: I416b632c55d1ceb925456ac8c8947dfbcef2e888 Gerrit-Change-Number: 81261 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[main]: udelay: clock_getres() does not provide clock_gettime() resolution
by Peter Marheine (Code Review) 22 Mar '24

22 Mar '24

Attention is currently required from: Anastasia Klimchuk, Brian Norris, Chen, Nikolai Artemiev, Stefan Reinauer. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80806?usp=email ) Change subject: udelay: clock_getres() does not provide clock_gettime() resolution ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/80806/comment/66216798_cba81099 : PS2, Line 10: Linux makes this clearer > I think `#ifdef __linux__` is a good plan B , but before we come to it let me try invite few more pe […] I looked at what FreeBSD does: 1. `sys_clock_getres` calls `kern_clock_getres` 2. `kern_clock_geres(CLOCK_MONOTONIC)` computes the clock period using `tc_getfrequency` 3. `tc_getfrequency` uses the declared frequency of the first `struct timehands` in the system 4. The primary time handler is chosen in `tc_init` based on each counter's declared quality, where larger values are better The RISC-V timecounter driver (`sys/riscv/riscv/timer.c`) declares a constant high quality (1000) and its frequency is defined by the `timebase-frequency` shared by all CPUs in the system. Among device trees in `sys/contrib/device-tree/src/riscv/`, it looks like this varies between 1 MHz and 24 MHz. This traces back to `clock_gettime` through `nanouptime` and eventually `tc_delta` on the active timecounter, which calls its `tc_get_timecount`. In the RISC-V driver, that eventually executes a `rdtime` instruction which reads the hardware counter. --- So at least FreeBSD on RISC-V reports accurate resolution for `CLOCK_MONOTONIC`, and it looks like other platforms are similar. It looks like the x86 PIIX timer driver has a known fixed frequency of ~3.57 MHz, for instance. I think Linux has an unusual interpretation here. Another side of this is that Linux does this because it can't depend on a stable high-resolution timer existing, so it's conservative. On some systems it might end up needing to use an i8253 timer which has exactly the same period as one tick (== the reported resolution in low-resolution mode) so it's useful to use the calibrated delay loop. I wonder if it might be more helpful to make this a user-visible option maybe with a compile-time default? There's no harm in using OS timers if they're slower than desired, just chip delays may tend to be longer than they need to be so programming might be slower than it needs to be. If it's exposed as an option, users can judge whether it saves time to skip calibration and distributors can choose a default according to expected typical use. -- To view, visit https://review.coreboot.org/c/flashrom/+/80806?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: Icdc6e184eacb14d3bd27029e3fc75be4431d82b4 Gerrit-Change-Number: 80806 Gerrit-PatchSet: 3 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Chen <c(a)jia.je> Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Brian Norris <briannorris(a)chromium.org> Gerrit-Attention: Chen <c(a)jia.je> 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: Fri, 22 Mar 2024 06:17:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Brian Norris <briannorris(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[main]: WIP: erasure_layout: Fix get_flash_region bug
by Nikolai Artemiev (Code Review) 22 Mar '24

22 Mar '24

Attention is currently required from: Aarya, Anastasia Klimchuk. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81385?usp=email ) Change subject: WIP: erasure_layout: Fix get_flash_region bug ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: Very rough draft of fix. Haven't tested -- To view, visit https://review.coreboot.org/c/flashrom/+/81385?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: I5d8492351a1ac5832227cbdbe40dea7b9ac3abd1 Gerrit-Change-Number: 81385 Gerrit-PatchSet: 2 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.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-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 22 Mar 2024 03:56:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[main]: WIP: erasure_layout: Fix get_flash_region bug
by Nikolai Artemiev (Code Review) 22 Mar '24

22 Mar '24

Attention is currently required from: Aarya. Hello Aarya, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/81385?usp=email to look at the new patch set (#2). Change subject: WIP: erasure_layout: Fix get_flash_region bug ...................................................................... WIP: erasure_layout: Fix get_flash_region bug This effectively exchanges the nesting of the loops over erase blocks and flash regions. Old: - Select erasefns - Loop over blocks to erase for each selected erasefn - Loop over programmer flash regions within erase block - Erase regions (may fail since selected erasefn will be too big if flash region is smaller than erase block) New: - Loop over programmer flash regions within erase block - Select erasefns within programer flash region - Loop over blocks to erase for each selected erasefn - Erase regions Eraser selection and erasing has also been factored out into a helper function to manage nesting depth. TEST=builds BUG=none BRANCH=none Change-Id: I5d8492351a1ac5832227cbdbe40dea7b9ac3abd1 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M erasure_layout.c 1 file changed, 102 insertions(+), 92 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/85/81385/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/81385?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: I5d8492351a1ac5832227cbdbe40dea7b9ac3abd1 Gerrit-Change-Number: 81385 Gerrit-PatchSet: 2 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[main]: WIP: erasure_layout: Fix get_flash_region bug
by Nikolai Artemiev (Code Review) 22 Mar '24

22 Mar '24

Nikolai Artemiev has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/81385?usp=email ) Change subject: WIP: erasure_layout: Fix get_flash_region bug ...................................................................... WIP: erasure_layout: Fix get_flash_region bug This effectively exchanges the nesting of the loops over erase blocks and flash regions. Old: - Select erasefns - Loop over blocks to erase for each selected erasefn - Loop over programmer flash regions within erase block - Erase regions (may fail since selected erasefn will be too big if flash region is smaller than erase block) New: - Loop over programmer flash regions within erase block - Select erasefns within programer flash region - Loop over blocks to erase for each selected erasefn - Erase regions Eraser selection and erasing has also been factored out into a helper function to manage nesting depth. TEST=builds BUG=none BRANCH=none Change-Id: I5d8492351a1ac5832227cbdbe40dea7b9ac3abd1 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M erasure_layout.c 1 file changed, 102 insertions(+), 92 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/85/81385/1 diff --git a/erasure_layout.c b/erasure_layout.c index 1dd6b60..3fe0e4f 100644 --- a/erasure_layout.c +++ b/erasure_layout.c @@ -238,6 +238,80 @@ } } +static int erase_write_helper(struct flashctx *const flashctx, chipoff_t region_start, chipoff_t region_end, + uint8_t *curcontents, uint8_t *newcontents, + struct erase_layout *erase_layout, bool *all_skipped) +{ + const size_t erasefn_count = count_usable_erasers(flashctx); + + // select erase functions + for (size_t i = 0; i < erase_layout[erasefn_count - 1].block_count; i++) { + if (erase_layout[erasefn_count - 1].layout_list[i].start_addr <= region_end && + region_start <= erase_layout[erasefn_count - 1].layout_list[i].end_addr) + select_erase_functions(flashctx, erase_layout, + erasefn_count - 1, i, + curcontents, newcontents, + region_start, region_end); + } + + // erase + for (size_t i = 0; i < erasefn_count; i++) { + for (size_t j = 0; j < erase_layout[i].block_count; j++) { + if (!erase_layout[i].layout_list[j].selected) + continue; + + chipoff_t start_addr = erase_layout[i].layout_list[j].start_addr; + unsigned int block_len = erase_layout[i].layout_list[j].end_addr - start_addr + 1; + const uint8_t erased_value = ERASED_VALUE(flashctx); + // execute erase + erasefunc_t *erasefn = lookup_erase_func_ptr(erase_layout[i].eraser); + + + if (erasefn(flashctx, start_addr, block_len)) { + return -1; + } + if (check_erased_range(flashctx, start_addr, block_len)) { + return -1; + msg_cerr("ERASE FAILED!\n"); + } + + // adjust curcontents + memset(curcontents+start_addr, erased_value, block_len); + // after erase make it unselected again + erase_layout[i].layout_list[j].selected = false; + msg_cdbg("E(%"PRIx32":%"PRIx32")", start_addr, start_addr + block_len - 1); + + *all_skipped = false; + } + } + + // write + unsigned int start_here = 0, len_here = 0, erase_len = region_end - region_start + 1; + while ((len_here = get_next_write(curcontents + region_start + start_here, + newcontents + region_start + start_here, + erase_len - start_here, &start_here, + flashctx->chip->gran))) { + // execute write + int ret = write_flash(flashctx, + newcontents + region_start + start_here, + region_start + start_here, len_here); + if (ret) { + msg_cerr("Write failed at %#x, Abort.\n", region_start + start_here); + return -1; + } + + // adjust curcontents + memcpy(curcontents + region_start + start_here, + newcontents + region_start + start_here, len_here); + msg_cdbg("W(%"PRIx32":%"PRIx32")", region_start + start_here, region_start + start_here + len_here - 1); + + *all_skipped = false; + } + + return 0; +} + + /* * @brief wrapper to use the erase algorithm * @@ -253,12 +327,15 @@ uint8_t *curcontents, uint8_t *newcontents, struct erase_layout *erase_layout, bool *all_skipped) { - const size_t erasefn_count = count_usable_erasers(flashctx); int ret = 0; - size_t i; chipoff_t old_start = region_start, old_end = region_end; align_region(erase_layout, flashctx, &region_start, &region_end); + if (!flashctx->flags.skip_unwritable_regions) { + if (check_for_unwritable_regions(flashctx, region_start, region_end - region_start)) + return -1; + } + uint8_t *old_start_buf = NULL, *old_end_buf = NULL; old_start_buf = (uint8_t *)malloc(old_start - region_start); if (!old_start_buf) { @@ -285,101 +362,34 @@ memcpy(newcontents + end_offset, curcontents + end_offset, region_end - old_end); } - // select erase functions - for (i = 0; i < erase_layout[erasefn_count - 1].block_count; i++) { - if (erase_layout[erasefn_count - 1].layout_list[i].start_addr <= region_end && - region_start <= erase_layout[erasefn_count - 1].layout_list[i].end_addr) - select_erase_functions(flashctx, erase_layout, - erasefn_count - 1, i, - curcontents, newcontents, - region_start, region_end); - } + unsigned int len; + for (unsigned int addr = region_start; addr < region_end; addr += len) { + struct flash_region region; + get_flash_region(flashctx, addr, &region); + len = min(region_end, region.end) - addr; - for (i = 0; i < erasefn_count; i++) { - for (size_t j = 0; j < erase_layout[i].block_count; j++) { - if (!erase_layout[i].layout_list[j].selected) continue; - - // erase - chipoff_t start_addr = erase_layout[i].layout_list[j].start_addr; - unsigned int block_len = erase_layout[i].layout_list[j].end_addr - start_addr + 1; - const uint8_t erased_value = ERASED_VALUE(flashctx); - // execute erase - erasefunc_t *erasefn = lookup_erase_func_ptr(erase_layout[i].eraser); - - if (!flashctx->flags.skip_unwritable_regions) { - if (check_for_unwritable_regions(flashctx, start_addr, block_len)) - goto _end; - } - - unsigned int len; - for (unsigned int addr = start_addr; addr < start_addr + block_len; addr += len) { - struct flash_region region; - get_flash_region(flashctx, addr, &region); - - len = min(start_addr + block_len, region.end) - addr; - - if (region.write_prot) { - msg_gdbg("%s: cannot erase inside %s " - "region (%#08"PRIx32"..%#08"PRIx32"), skipping range (%#08x..%#08x).\n", - __func__, region.name, - region.start, region.end - 1, - addr, addr + len - 1); - free(region.name); - continue; - } - - msg_gdbg("%s: %s region (%#08"PRIx32"..%#08"PRIx32") is " - "writable, erasing range (%#08x..%#08x).\n", - __func__, region.name, - region.start, region.end - 1, - addr, addr + len - 1); - free(region.name); - - if (erasefn(flashctx, addr, len)) { - ret = -1; - goto _end; - } - if (check_erased_range(flashctx, addr, len)) { - ret = - 1; - msg_cerr("ERASE FAILED!\n"); - goto _end; - } - } - - // adjust curcontents - memset(curcontents+start_addr, erased_value, block_len); - // after erase make it unselected again - erase_layout[i].layout_list[j].selected = false; - msg_cdbg("E(%"PRIx32":%"PRIx32")", start_addr, start_addr + block_len - 1); - - *all_skipped = false; + if (region.write_prot) { + msg_gdbg("%s: cannot erase inside %s " + "region (%#08"PRIx32"..%#08"PRIx32"), skipping range (%#08x..%#08x).\n", + __func__, region.name, + region.start, region.end - 1, + addr, addr + len - 1); + free(region.name); + continue; } - } - // write - unsigned int start_here = 0, len_here = 0, erase_len = region_end - region_start + 1; - while ((len_here = get_next_write(curcontents + region_start + start_here, - newcontents + region_start + start_here, - erase_len - start_here, &start_here, - flashctx->chip->gran))) { - // execute write - ret = write_flash(flashctx, - newcontents + region_start + start_here, - region_start + start_here, len_here); - if (ret) { - msg_cerr("Write failed at %#zx, Abort.\n", i); - ret = -1; + msg_gdbg("%s: %s region (%#08"PRIx32"..%#08"PRIx32") is " + "writable, erasing range (%#08x..%#08x).\n", + __func__, region.name, + region.start, region.end - 1, + addr, addr + len - 1); + free(region.name); + + + ret = erase_write_helper(flashctx, addr, len, curcontents, newcontents, erase_layout, all_skipped); + if (ret) goto _end; - } - - // adjust curcontents - memcpy(curcontents + region_start + start_here, - newcontents + region_start + start_here, len_here); - msg_cdbg("W(%"PRIx32":%"PRIx32")", region_start + start_here, region_start + start_here + len_here - 1); - - *all_skipped = false; } - _end: memcpy(newcontents + region_start, old_start_buf, old_start - region_start); memcpy(newcontents + old_end, old_end_buf, region_end - old_end); -- To view, visit https://review.coreboot.org/c/flashrom/+/81385?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: I5d8492351a1ac5832227cbdbe40dea7b9ac3abd1 Gerrit-Change-Number: 81385 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newchange

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit March 2024