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 Hsuan-ting Chen (Code Review) 19 Mar '24

19 Mar '24

Attention is currently required from: Hsuan Ting Chen. Hello Hsuan Ting Chen, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/81356?usp=email to review the following change. Change subject: ich: Add names for region 5, 9, 10, 11, 12, 13, 15 ...................................................................... ich: Add names for region 5, 9, 10, 11, 12, 13, 15 Add Region 9 for Intel Meteor Lake; update missing regions. * Include Region 9 as officially required for Intel Meteor Lake platform. * Incorporate missing region names from https://github.com/coreboot/coreboot/blob/main/util/ifdtool/ifdtool.c for completeness. Region 5: Device Expansion (DE or DevExp) Region 9: Device Expansion 2 (DE2 or DevExp2) Region 10: Innovation Engine (IE) Region 11: 10 GbE 0 Region 12: 10 GbE 1 Region 15: PTT BUG=b:319773700 TEST=none Change-Id: I3b164ce4ae84bfd523fcd8be416c5d13183ed632 Signed-off-by: roccochen(a)chromium.com <roccochen(a)chromium.org> --- M ich_descriptors.c M ichspi.c M util/ich_descriptors_tool/ich_descriptors_tool.c 3 files changed, 9 insertions(+), 8 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/56/81356/1 diff --git a/ich_descriptors.c b/ich_descriptors.c index 7bdca61..d1b1d9e 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -518,24 +518,24 @@ } size_t num_regions; - msg_pdbg2(" FD BIOS ME GbE Pltf Reg5 Reg6 Reg7 EC Reg9"); + msg_pdbg2(" FD BIOS ME GbE Pltf DE BIOS2 Reg7 EC DE2 "); if (cs == CHIPSET_100_SERIES_SUNRISE_POINT) { num_regions = 10; msg_pdbg2("\n"); } else { num_regions = 16; - msg_pdbg2(" RegA RegB RegC RegD RegE RegF\n"); + msg_pdbg2(" IE 10GbE0 10GbE1 RegD RegE PTT \n"); } for (i = 0; i < nm; i++) { const unsigned int ext_region_start = 12; size_t j; msg_pdbg2("%-4s", master_names[i]); for (j = 0; j < (size_t)min(num_regions, ext_region_start); j++) - msg_pdbg2(" %c%c ", + msg_pdbg2(" %c%c ", desc->master.mstr[i].read & (1 << j) ? 'r' : ' ', desc->master.mstr[i].write & (1 << j) ? 'w' : ' '); for (j = ext_region_start; j < num_regions; j++) - msg_pdbg2(" %c%c ", + msg_pdbg2(" %c%c ", desc->master.mstr[i].ext_read & (1 << (j - ext_region_start)) ? 'r' : ' ', desc->master.mstr[i].ext_write & (1 << (j - ext_region_start)) ? 'w' : ' '); msg_pdbg2("\n"); diff --git a/ichspi.c b/ichspi.c index 36a7b3b..bee5ec9 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1840,7 +1840,8 @@ static const char *const region_names[] = { "Flash Descriptor", "BIOS", "Management Engine", "Gigabit Ethernet", "Platform Data", "Device Expansion", - "BIOS2", "unknown", "EC/BMC", + "BIOS2", "unknown", "EC/BMC", "Device Expansion 2", + "Innovation Engine", "10GbE0", "10GbE1", "unknown", "unknown", "PTT", }; const char *const region_name = i < ARRAY_SIZE(region_names) ? region_names[i] : "unknown"; diff --git a/util/ich_descriptors_tool/ich_descriptors_tool.c b/util/ich_descriptors_tool/ich_descriptors_tool.c index a5a59ad..844d8e9 100644 --- a/util/ich_descriptors_tool/ich_descriptors_tool.c +++ b/util/ich_descriptors_tool/ich_descriptors_tool.c @@ -38,9 +38,9 @@ static const char *const region_names[] = { "Descriptor", "BIOS", "ME", "GbE", "Platform", - "Region5", "BIOS2", "Region7", "EC/BMC", "Region9", - "IE", "10GbE", "Region12", "Region13", "Region14", - "Region15" + "DevExp", "BIOS2", "Region7", "EC/BMC", "DevExp2", + "IE", "10GbE0", "10GbE1", "Region13", "Region14", + "PTT" }; static void dump_file(const char *prefix, const uint32_t *dump, unsigned int len, -- 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: Hsuan Ting Chen <roccochen(a)chromium.org> Gerrit-Attention: Hsuan Ting Chen <roccochen(a)chromium.org> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[main]: Add support for Idaville HCC platform and iRC regions
by Kevin Yu (Code Review) 18 Mar '24

18 Mar '24

Attention is currently required from: Paul Menzel. Kevin Yu has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81327?usp=email ) Change subject: Add support for Idaville HCC platform and iRC regions ...................................................................... Patch Set 1: This change is ready for review. -- To view, visit https://review.coreboot.org/c/flashrom/+/81327?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: I39e7fa56b1b1de429f413ce32e69c8d8c769b6a9 Gerrit-Change-Number: 81327 Gerrit-PatchSet: 1 Gerrit-Owner: Kevin Yu <ykevin(a)celestica.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Mon, 18 Mar 2024 06:02:18 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[L] Change in flashrom[main]: flashchips: Add Macronix Flash EPN support
by DZ (Code Review) 18 Mar '24

18 Mar '24

DZ has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/81096?usp=email ) Change subject: flashchips: Add Macronix Flash EPN support ...................................................................... Abandoned need modify the source code -- To view, visit https://review.coreboot.org/c/flashrom/+/81096?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: Ia07032c1a3168bd595cea6b24e4875843e20190c Gerrit-Change-Number: 81096 Gerrit-PatchSet: 1 Gerrit-Owner: DZ <danielzhang(a)mxic.com.cn> 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-MessageType: abandon

1 0

[XS] Change in flashrom[main]: cli_common: Add link to the documentation how to mark chip tested
by Sergii Dmytruk (Code Review) 17 Mar '24

17 Mar '24

Attention is currently required from: Anastasia Klimchuk, Peter Marheine. Sergii Dmytruk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81266?usp=email ) Change subject: cli_common: Add link to the documentation how to mark chip tested ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/81266?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: I36105725058f2fecb82408c369b70b3324502ece Gerrit-Change-Number: 81266 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Sun, 17 Mar 2024 16:58:07 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: cli_common: Add link to the documentation how to mark chip tested
by Anastasia Klimchuk (Code Review) 16 Mar '24

16 Mar '24

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/81266?usp=email ) Change subject: cli_common: Add link to the documentation how to mark chip tested ...................................................................... cli_common: Add link to the documentation how to mark chip tested Perhaps some of the users will be able to follow the instructions and send a patch to mark chip as tested. The option to send report to the mailing list remains available as before. Change-Id: I36105725058f2fecb82408c369b70b3324502ece Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M cli_common.c 1 file changed, 2 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/66/81266/1 diff --git a/cli_common.c b/cli_common.c index 5373585..afcd1f3 100644 --- a/cli_common.c +++ b/cli_common.c @@ -80,6 +80,8 @@ "work correctly for you with this flash chip. Please include the flashrom log\n" "file for all operations you tested (see the man page for details), and mention\n" "which mainboard or programmer you tested in the subject line.\n" + "You can also try to follow the instructions here:\n" + "https://www.flashrom.org/contrib_howtos/how_to_mark_chip_tested.html\n" "Thanks for your help!\n"); } } -- To view, visit https://review.coreboot.org/c/flashrom/+/81266?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: I36105725058f2fecb82408c369b70b3324502ece Gerrit-Change-Number: 81266 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newchange

1 0

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

15 Mar '24

Attention is currently required from: Anastasia Klimchuk, Peter Marheine, Thomas Heijligen. Stefan Reinauer 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 1: Code-Review+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: 1 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: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Fri, 15 Mar 2024 15:10:18 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes 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) 15 Mar '24

15 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 1: (1 comment) Patchset: PS1: This was inspired by CB:81032 Somehow I know what I want to say, but the hardest part was to give a name to the new section in the guide. I can see the name of section is not ideal, really appreciate ideas how to improve it! -- 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: 1 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-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, 15 Mar 2024 12:40:33 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No 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) 15 Mar '24

15 Mar '24

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/81261?usp=email ) 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/1 diff --git a/doc/dev_guide/development_guide.rst b/doc/dev_guide/development_guide.rst index 0b64325..1ef1fc9 100644 --- a/doc/dev_guide/development_guide.rst +++ b/doc/dev_guide/development_guide.rst @@ -191,6 +191,8 @@ Pushing a patch --------------- +Before pushing a patch, make sure it builds on your environment and all unit tests pass (see :doc:`building_from_source`). + To push patch to Gerrit, use the follow command: :code:`git push upstream HEAD:refs/for/main`. * If using HTTPS you will be prompted for the username and password you @@ -209,6 +211,21 @@ Alternatively, you can add a topic from a Gerrit UI after the patch in pushed (on the top-left section) of patch UI. +Checking the CI +--------------- + +Every patch needs to get a ``Verified +1`` label, typically from Jenkins. Once the patch is pushed +to Gerrit, Jenkins is added automatically and runs its build script. The script builds the patch with +various config options, and runs unit tests (for more details see source code of ``test_build.sh``). +Then, Jenkins gives the patch ``+1`` or ``-1`` vote, indicating success or fail. + +In case of failure, follow Jenkins link (which it adds as a comment to the patch), open Console output, +find the error and try to fix it. + +In addition to building and running unit tests, Jenkins also runs a scan-build over the patch. Ideally +you need to check that your patch does not introduce new warnings. To see scan-build report, follow +Jenkins link -> Build artifacts -> scan build link for the given run. + Adding reviewers to the patch ----------------------------- -- 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: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newchange

1 0

[L] Change in flashrom[main]: flashchips: Add Macronix Flash EPN support
by Anastasia Klimchuk (Code Review) 15 Mar '24

15 Mar '24

Attention is currently required from: DZ, Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81096?usp=email ) Change subject: flashchips: Add Macronix Flash EPN support ...................................................................... Patch Set 1: (13 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/81096/comment/a35d804a_90ead2e8 : PS1, Line 9: We have tested read/write/erase and write protection function for following Macronix Flash. This is great, thank you so much! Could you please add info to commit message: which programmer you were using, and which operations you ran. Thanks! Patchset: PS1: Daniel thank you the patch! I appreciate you are updating so many chip definitions! I have a suggestion, if you want, you can split this commit as one chip per commit. This way we can gradually review and submit them one by one. But you can keep it in one large commit, however in this case none of the chips will be submitted until all of them are ready. I added comments, and then I will have another round of review after these ones are fixed. File flashchips.c: https://review.coreboot.org/c/flashrom/+/81096/comment/f5276d8a_c964e083 : PS1, Line 6201: .tested = TEST_UNTESTED, This diff seems to be by mistake? That's Fudan chip, but also if the chip has been tested, you can't untest it :) https://review.coreboot.org/c/flashrom/+/81096/comment/233b82cb_58f936a1 : PS1, Line 9008: MX25L12845E/MX25L12865E These two models (MX25L12845E and MX25L12865E) have no configuration register, not in datasheets, which means: In the patch you update the current definition with `FEATURE_CFGR` and `reg_bits`, and now MX25L12845E and MX25L12865E do not match this new definition anymore. The easiest option is to add MX25L12850F as a new definition (and it will be reusing the same model ID, which is fine), and add `FEATURE_CFGR` and `reg_bits` into it. Old definition remains the same. Another option (harder, but more comprehensive) is to have old definition left for MX25L12845E and MX25L12865E, and create new definition with write-protect support for the remaining chips (including new one). https://review.coreboot.org/c/flashrom/+/81096/comment/aa5ebaf1_6c51b52d : PS1, Line 9275: .tb = {CONFIG, 3, OTP} I didn't find configuration register in datasheets? https://review.coreboot.org/c/flashrom/+/81096/comment/20b69955_7136025c : PS1, Line 9547: .eraseblocks = { {64 * 1024, 64} }, : .block_erase = SPI_BLOCK_ERASE_52, this command is used for 32K block https://review.coreboot.org/c/flashrom/+/81096/comment/c2e74bfd_93878745 : PS1, Line 9564: {1650, 3600} datasheet says 2.65V-3.6V https://review.coreboot.org/c/flashrom/+/81096/comment/9ece37cf_8bf93e73 : PS1, Line 9567: .srp = {STATUS1, 7, RW}, This bit is "Reserved" in datasheet https://review.coreboot.org/c/flashrom/+/81096/comment/f69f8af8_6e243245 : PS1, Line 9594: .eraseblocks = { {64 * 1024, 64} }, : .block_erase = SPI_BLOCK_ERASE_52, this command is for 32K block https://review.coreboot.org/c/flashrom/+/81096/comment/8bd19c68_f25e6897 : PS1, Line 9611: {1650, 3600}, datasheet says 2.7V-3.6V https://review.coreboot.org/c/flashrom/+/81096/comment/e9da62a5_3d78bec9 : PS1, Line 10285: .name = "MX25R1635F", It seems like you moved MX25R1635F entry, and now it's out of alphabetical order. You need to return in back into its position in the list File include/flashchips.h: https://review.coreboot.org/c/flashrom/+/81096/comment/6535e38c_a2511122 : PS1, Line 538: #define MACRONIX_MX25L3239E 0x2536 for this and the other new lines: you need to keep the indentation between macro name and values, to keep value aligned https://review.coreboot.org/c/flashrom/+/81096/comment/6860b225_eb3fe60b : PS1, Line 546: #define MACRONIX_MX25R4035F 0x2813 : #define MACRONIX_MX25R1635F 0x2815 it's a duplicate, the same macro exists already -- To view, visit https://review.coreboot.org/c/flashrom/+/81096?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: Ia07032c1a3168bd595cea6b24e4875843e20190c Gerrit-Change-Number: 81096 Gerrit-PatchSet: 1 Gerrit-Owner: DZ <danielzhang(a)mxic.com.cn> 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Fri, 15 Mar 2024 08:50:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[main]: Add support for Idaville HCC platform and iRC regions
by Kevin Yu (Code Review) 15 Mar '24

15 Mar '24

Kevin Yu has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/81258?usp=email ) Change subject: Add support for Idaville HCC platform and iRC regions ...................................................................... Abandoned -- To view, visit https://review.coreboot.org/c/flashrom/+/81258?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: I61b810e4fa2aba8d92483bb4744561e67f46b959 Gerrit-Change-Number: 81258 Gerrit-PatchSet: 2 Gerrit-Owner: Kevin Yu <ykevin(a)celestica.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: abandon

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit March 2024