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

[XS] Change in flashrom[main]: Add clarification to struct flash_region on chipoff_t
by Anastasia Klimchuk (Code Review) May 28, 2024

May 28, 2024

Attention is currently required from: Peter Marheine. Anastasia Klimchuk has posted comments on this change by Peter Marheine. ( https://review.coreboot.org/c/flashrom/+/82677?usp=email ) Change subject: Add clarification to struct flash_region on chipoff_t ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/82677?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: I80b61a87ca31bd5a116224aadb4e211ee6841e1f Gerrit-Change-Number: 82677 Gerrit-PatchSet: 1 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Tue, 28 May 2024 04:29:27 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[S] Change in flashrom[main]: Correct get_flash_region() to use inclusive upper bounds
by Peter Marheine (Code Review) May 28, 2024

May 28, 2024

Attention is currently required from: Aarya, Anastasia Klimchuk, Hsuan-ting Chen, Nikolai Artemiev. Peter Marheine has posted comments on this change by Peter Marheine. ( https://review.coreboot.org/c/flashrom/+/82496?usp=email ) Change subject: Correct get_flash_region() to use inclusive upper bounds ...................................................................... Patch Set 6: (2 comments) File flashrom.c: https://review.coreboot.org/c/flashrom/+/82496/comment/28a45641_d25f224b?us… : PS6, Line 2240: start = region->end + 1; > Just want to double-check: This and all other layout-related occurrences in `layout. […] I think that's right. I only identified `get_flash_region` as a problem, and that was added along with all the code that uses it at about the same time. File include/layout.h: -- To view, visit https://review.coreboot.org/c/flashrom/+/82496?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: Ia0ca867aef33b598c2697fc264807fa5e29683ec Gerrit-Change-Number: 82496 Gerrit-PatchSet: 6 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Tue, 28 May 2024 02:58:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Hsuan-ting Chen <roccochen(a)google.com>

1 0

[XS] Change in flashrom[main]: Add clarification to struct flash_region on chipoff_t
by Peter Marheine (Code Review) May 28, 2024

May 28, 2024

Peter Marheine has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/82677?usp=email ) Change subject: Add clarification to struct flash_region on chipoff_t ...................................................................... Add clarification to struct flash_region on chipoff_t Although chipoff_t is fairly clearly documented on its own, it seems fairly frequent that developers will treat the end address of a flash region as an exclusive upper bound rather than the inclusive one it should be; for example CB:82496 fixes an incorrect use that affected multiple sites, and CB:73571 stemmed from a similar cause. Add a clarifying comment to call attention to this, to help programmers avoid making similar mistakes in the future. Change-Id: I80b61a87ca31bd5a116224aadb4e211ee6841e1f Signed-off-by: Peter Marheine <pmarheine(a)chromium.org> --- M include/layout.h 1 file changed, 5 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/77/82677/1 diff --git a/include/layout.h b/include/layout.h index 5614bea..d03a15b 100644 --- a/include/layout.h +++ b/include/layout.h @@ -37,6 +37,11 @@ struct flash_region { char *name; + /* + * Note that because start and end are chipoff_t, end is an inclusive upper + * bound: the length of a region is (end - start + 1) bytes and it is + * impossible to represent a region with zero length. + */ chipoff_t start; chipoff_t end; bool read_prot; -- To view, visit https://review.coreboot.org/c/flashrom/+/82677?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I80b61a87ca31bd5a116224aadb4e211ee6841e1f Gerrit-Change-Number: 82677 Gerrit-PatchSet: 1 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>

1 0

[S] Change in flashrom[main]: Correct get_flash_region() to use inclusive upper bounds
by Anastasia Klimchuk (Code Review) May 28, 2024

May 28, 2024

Attention is currently required from: Aarya, Hsuan-ting Chen, Nikolai Artemiev, Peter Marheine. Anastasia Klimchuk has posted comments on this change by Peter Marheine. ( https://review.coreboot.org/c/flashrom/+/82496?usp=email ) Change subject: Correct get_flash_region() to use inclusive upper bounds ...................................................................... Patch Set 6: (1 comment) File include/layout.h: -- To view, visit https://review.coreboot.org/c/flashrom/+/82496?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: Ia0ca867aef33b598c2697fc264807fa5e29683ec Gerrit-Change-Number: 82496 Gerrit-PatchSet: 6 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Tue, 28 May 2024 01:55:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Hsuan-ting Chen <roccochen(a)google.com>

1 0

[XS] Change in flashrom[main]: add gcc-14 -Werror=calloc-transposed-args compatibility
by Robert Marko (Code Review) May 27, 2024

May 27, 2024

Attention is currently required from: Alexander Goncharov. Robert Marko has posted comments on this change by Alexander Goncharov. ( https://review.coreboot.org/c/flashrom/+/82657?usp=email ) Change subject: add gcc-14 -Werror=calloc-transposed-args compatibility ...................................................................... Patch Set 1: Code-Review+1 (1 comment) Patchset: PS1: Fixes compilation with GCC14 on Fedora 40 for me. -- To view, visit https://review.coreboot.org/c/flashrom/+/82657?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: Icb9842fbc2fa6ad4cd9dc9384c19fd3741eadb2e Gerrit-Change-Number: 82657 Gerrit-PatchSet: 1 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Robert Marko <robimarko(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Mon, 27 May 2024 19:34:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes

1 0

[S] Change in flashrom[main]: flashchips: Add XTX XT25F128B
by Robert Marko (Code Review) May 27, 2024

May 27, 2024

Robert Marko has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/82676?usp=email ) Change subject: flashchips: Add XTX XT25F128B ...................................................................... flashchips: Add XTX XT25F128B XTX XT25F128B is 128M-bit version of XT25F64B. Tested probe, read, erase and write with FT232H. Datasheet: https://wmsc.lcsc.com/wmsc/upload/file/pdf/v2/lcsc/2304140030_XTX-XT25F128B… Change-Id: I37084bd66bc7a8f93d6533ab0d67aa2528786299 Signed-off-by: Robert Marko <robert.marko(a)sartura.hr> --- M flashchips.c M include/flashchips.h 2 files changed, 41 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/76/82676/1 diff --git a/flashchips.c b/flashchips.c index 776b3a8..fb28712 100644 --- a/flashchips.c +++ b/flashchips.c @@ -22178,6 +22178,46 @@ }, { + .vendor = "XTX Technology Limited", + .name = "XT25F128B", + .bustype = BUS_SPI, + .manufacture_id = XTX_ID, + .model_id = XTX_XT25F128B, + .total_size = 16 * 1024, + .page_size = 256, + /* Supports SFDP */ + /* OTP: 4 x 256 bytes */ + .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_QPI, + .tested = TEST_OK_PREW, + .probe = PROBE_SPI_RDID, + .probe_timing = TIMING_ZERO, + .block_erasers = + { + { + .eraseblocks = { {4 * 1024, 4096} }, + .block_erase = SPI_BLOCK_ERASE_20, + }, { + .eraseblocks = { {32 * 1024, 512} }, + .block_erase = SPI_BLOCK_ERASE_52, + }, { + .eraseblocks = { {64 * 1024, 256} }, + .block_erase = SPI_BLOCK_ERASE_D8, + }, { + .eraseblocks = { {16 * 1024 * 1024, 1} }, + .block_erase = SPI_BLOCK_ERASE_60, + }, { + .eraseblocks = { {16 * 1024 * 1024, 1} }, + .block_erase = SPI_BLOCK_ERASE_C7, + } + }, + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_BP4_SRWD, + .unlock = SPI_DISABLE_BLOCKPROTECT_BP4_SRWD, + .write = SPI_CHIP_WRITE256, + .read = SPI_CHIP_READ, + .voltage = {2700, 3600}, + }, + + { .vendor = "Zetta Device", .name = "ZD25D20", .bustype = BUS_SPI, diff --git a/include/flashchips.h b/include/flashchips.h index b8a58b2..d8e8bf1 100644 --- a/include/flashchips.h +++ b/include/flashchips.h @@ -1061,6 +1061,7 @@ #define XTX_ID 0x0B /* XTX Technology Limited */ #define XTX_XT25F02E 0x4012 #define XTX_XT25F64B 0x4017 +#define XTX_XT25F128B 0x4018 #define ZETTA_ID 0xBA /* Zetta Device */ #define ZETTA_ZD25D20 0x2012 -- To view, visit https://review.coreboot.org/c/flashrom/+/82676?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I37084bd66bc7a8f93d6533ab0d67aa2528786299 Gerrit-Change-Number: 82676 Gerrit-PatchSet: 1 Gerrit-Owner: Robert Marko <robimarko(a)gmail.com>

1 0

[L] Change in flashrom[main]: erasure_layout: Fix get_flash_region bug
by Anastasia Klimchuk (Code Review) May 27, 2024

May 27, 2024

Attention is currently required from: Aarya, Alexander Goncharov, Nikolai Artemiev, Peter Marheine. Anastasia Klimchuk has posted comments on this change by Peter Marheine. ( https://review.coreboot.org/c/flashrom/+/82393?usp=email ) Change subject: erasure_layout: Fix get_flash_region bug ...................................................................... Patch Set 13: (4 comments) Patchset: PS13: I got some last moment thoughts... File erasure_layout.c: https://review.coreboot.org/c/flashrom/+/82393/comment/afce5561_615cb6d6?us… : PS13, Line 368: for (unsigned int addr = region_start; addr <= region_end; addr += len) { I looked at this loop and got a bit suspicious. Specifically, will it properly get to the other side of protected region and continue after the end of protected region? And I really only now thought about it. And now I am realising that current test cases do not cover this case, when protected region somewhere in the middle of the chip (which is always the case in real life). The unit test has protected region which ends right at the end of chip memory :\ To represent what I am thinking about now, it could be e.g. between bytes 6-12, and we want to make sure bytes 13-15 are also processed. What do you think? I can try modify the test cases, maybe at the end of this week. Or if you want you can? File tests/erase_func_algo.c: https://review.coreboot.org/c/flashrom/+/82393/comment/8fa90395_825e150d?us… : PS9, Line 953: const size_t num_unparameterized = 1; > I don't understand what you're suggesting as an alternative. […] I forgot what exactly was on my mind when I was asking , sorry :( Probably I was confused because parameterized tests are initialized in a loop, and non-param with a memcpy. File tests/erase_func_algo.c: https://review.coreboot.org/c/flashrom/+/82393/comment/4e231d09_e567a183?us… : PS13, Line 766: : #define START_PROTECTED_REGION 8 : #define END_PROTECTED_REGION 15 This was what I thought in the other comment: have this both in the middle of a chip (5-12, something like that). Which might also need to adjust layout regions. It's easier to have the same protected region, and variations of layout regions in every test case. -- To view, visit https://review.coreboot.org/c/flashrom/+/82393?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: Ic5bf9d0f0e4a94c48d6f6e74e3cb9cccdc7adec9 Gerrit-Change-Number: 82393 Gerrit-PatchSet: 13 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> 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: 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: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Mon, 27 May 2024 12:59:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>

1 0

[S] Change in flashrom[main]: Correct get_flash_region() to use inclusive upper bounds
by Hsuan-ting Chen (Code Review) May 27, 2024

May 27, 2024

Attention is currently required from: Aarya, Nikolai Artemiev, Peter Marheine. Hsuan-ting Chen has posted comments on this change by Peter Marheine. ( https://review.coreboot.org/c/flashrom/+/82496?usp=email ) Change subject: Correct get_flash_region() to use inclusive upper bounds ...................................................................... Patch Set 6: (2 comments) File flashrom.c: https://review.coreboot.org/c/flashrom/+/82496/comment/aa48d700_675c6ee4?us… : PS6, Line 2240: start = region->end + 1; Just want to double-check: This and all other layout-related occurrences in `layout.c` (like `included_regions_overlap` and `layout_sanity_checks`) don't need to be changed. Is my understanding correct? File include/layout.h: -- To view, visit https://review.coreboot.org/c/flashrom/+/82496?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: Ia0ca867aef33b598c2697fc264807fa5e29683ec Gerrit-Change-Number: 82496 Gerrit-PatchSet: 6 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Mon, 27 May 2024 10:59:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[S] Change in flashrom[main]: Correct get_flash_region() to use inclusive upper bounds
by Anastasia Klimchuk (Code Review) May 27, 2024

May 27, 2024

Attention is currently required from: Aarya, Nikolai Artemiev, Peter Marheine. Anastasia Klimchuk has posted comments on this change by Peter Marheine. ( https://review.coreboot.org/c/flashrom/+/82496?usp=email ) Change subject: Correct get_flash_region() to use inclusive upper bounds ...................................................................... Patch Set 6: (1 comment) Patchset: PS6: I am wondering, this path in ichspi (ich_get_region) is the same in Chrome OS, is it possible to do a few test runs on a device? There are unit tests in the following patch, but they are implementing test version of get_flash_region and this changes the ich one. Otherwise I have no other comments! -- To view, visit https://review.coreboot.org/c/flashrom/+/82496?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: Ia0ca867aef33b598c2697fc264807fa5e29683ec Gerrit-Change-Number: 82496 Gerrit-PatchSet: 6 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Mon, 27 May 2024 10:41:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[M] Change in flashrom[main]: tests/erase: record the opcode for each erase
by Anastasia Klimchuk (Code Review) May 27, 2024

May 27, 2024

Attention is currently required from: Nikolai Artemiev, Peter Marheine, Thomas Heijligen. Anastasia Klimchuk has posted comments on this change by Peter Marheine. ( https://review.coreboot.org/c/flashrom/+/82251?usp=email ) Change subject: tests/erase: record the opcode for each erase ...................................................................... Patch Set 8: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/82251?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: I3983fe42c2e7f06668a1bd20d2db7fafa93b8043 Gerrit-Change-Number: 82251 Gerrit-PatchSet: 8 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Mon, 27 May 2024 09:36:29 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0