June 2023 - flashrom-gerrit - mail.coreboot.org

[XS] Change in flashrom[master]: ichspi.c: Bug fix for ich_hwseq_get_flash_id
by Nikolai Artemiev (Code Review) 20 Jun '23

20 Jun '23

Attention is currently required from: Anastasia Klimchuk, Edward O'Callaghan, eshankelkar(a)galorithm.com. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71872?usp=email ) Change subject: ichspi.c: Bug fix for ich_hwseq_get_flash_id ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Nikolai, just to check, so is this normal if `ich_hwseq_get_flash_id` doesn't write anything into `f […] That's right, it is ok for this function to return without copying anything, -- To view, visit https://review.coreboot.org/c/flashrom/+/71872?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I35c112cd032e3b94e30c347766764392d5bbfe3d Gerrit-Change-Number: 71872 Gerrit-PatchSet: 1 Gerrit-Owner: eshankelkar(a)galorithm.com Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: eshankelkar(a)galorithm.com Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 20 Jun 2023 00:19:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[master]: ichspi.c: Bug fix for ich_hwseq_get_flash_id
by Anastasia Klimchuk (Code Review) 19 Jun '23

19 Jun '23

Attention is currently required from: Edward O'Callaghan, eshankelkar(a)galorithm.com. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71872?usp=email ) Change subject: ichspi.c: Bug fix for ich_hwseq_get_flash_id ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: Nikolai, just to check, so is this normal if `ich_hwseq_get_flash_id` doesn't write anything into `flash->chip` and just returns, this is not an error? -- To view, visit https://review.coreboot.org/c/flashrom/+/71872?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I35c112cd032e3b94e30c347766764392d5bbfe3d Gerrit-Change-Number: 71872 Gerrit-PatchSet: 1 Gerrit-Owner: eshankelkar(a)galorithm.com Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: eshankelkar(a)galorithm.com Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Mon, 19 Jun 2023 11:35:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[L] Change in flashrom[master]: dirtyjtag: Add DirtyJTAG programmer
by Anastasia Klimchuk (Code Review) 19 Jun '23

19 Jun '23

Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67878?usp=email ) Change subject: dirtyjtag: Add DirtyJTAG programmer ...................................................................... Patch Set 17: (1 comment) Patchset: PS17: > Taken care of in https://review.coreboot. […] David, do you have more details, maybe logs or error messages that you can give? It's hard to understand the issue otherwise. The programmer has been tested, but it usually details that matter. If you could give details, maybe you can post on the mailing list? Or if you have account, maybe you can create issue on https://ticket.coreboot.org/projects/flashrom ? Whatever you prefer. thank you! -- To view, visit https://review.coreboot.org/c/flashrom/+/67878?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic43e9a014ed7d04e429e73b30c9dcfdde1a78913 Gerrit-Change-Number: 67878 Gerrit-PatchSet: 17 Gerrit-Owner: Jean THOMAS <virgule(a)jeanthomas.me> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Alexander Goncharov <chat(a)joursoir.net> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: David Williams Gerrit-Comment-Date: Mon, 19 Jun 2023 11:29:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: David Williams Comment-In-Reply-To: Jean THOMAS <virgule(a)jeanthomas.me> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: flashchips: change chip lock status funcs for Winbond chips
by Alexander Goncharov (Code Review) 19 Jun '23

19 Jun '23

Alexander Goncharov has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/75877?usp=email ) Change subject: flashchips: change chip lock status funcs for Winbond chips ...................................................................... flashchips: change chip lock status funcs for Winbond chips Decode status register bits for user friendly output. Signed-off-by: Alexander Goncharov <chat(a)joursoir.net> Change-Id: I5066863b514825aee0dffe496492514ac99b6e49 --- M flashchips.c 1 file changed, 21 insertions(+), 21 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/77/75877/1 diff --git a/flashchips.c b/flashchips.c index 3f09687..8a99309 100644 --- a/flashchips.c +++ b/flashchips.c @@ -17874,7 +17874,7 @@ .block_erase = SPI_BLOCK_ERASE_C7, } }, - .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_PLAIN, /* TODO: improve */ + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_AMIC_A25L032, .unlock = SPI_DISABLE_BLOCKPROTECT, .write = SPI_CHIP_WRITE256, .read = SPI_CHIP_READ, @@ -17923,7 +17923,7 @@ .block_erase = SPI_BLOCK_ERASE_C7, } }, - .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_PLAIN, + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_AMIC_A25L032, .unlock = SPI_DISABLE_BLOCKPROTECT, .write = SPI_CHIP_WRITE256, .read = SPI_CHIP_READ, @@ -17973,7 +17973,7 @@ .block_erase = SPI_BLOCK_ERASE_C7, } }, - .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_PLAIN, /* TODO: improve */ + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_AMIC_A25L032, .unlock = SPI_DISABLE_BLOCKPROTECT, .write = SPI_CHIP_WRITE256, .read = SPI_CHIP_READ, @@ -18021,7 +18021,7 @@ .block_erase = SPI_BLOCK_ERASE_C7, } }, - .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_PLAIN, /* TODO: improve */ + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_AMIC_A25L032, .unlock = SPI_DISABLE_BLOCKPROTECT, .write = SPI_CHIP_WRITE256, .read = SPI_CHIP_READ, @@ -18071,7 +18071,7 @@ .block_erase = SPI_BLOCK_ERASE_C7, } }, - .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_PLAIN, /* TODO: improve */ + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_AMIC_A25L032, .unlock = SPI_DISABLE_BLOCKPROTECT, .write = SPI_CHIP_WRITE256, .read = SPI_CHIP_READ, @@ -18202,7 +18202,7 @@ .block_erase = SPI_BLOCK_ERASE_C7, } }, - .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_PLAIN, /* TODO: improve */ + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_BP3_SRWD, .unlock = SPI_DISABLE_BLOCKPROTECT, .write = SPI_CHIP_WRITE256, .read = SPI_CHIP_READ, @@ -18257,7 +18257,7 @@ .block_erase = SPI_BLOCK_ERASE_C7, } }, - .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_PLAIN, /* TODO: improve */ + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_BP3_SRWD, .unlock = SPI_DISABLE_BLOCKPROTECT, .write = SPI_CHIP_WRITE256, .read = SPI_CHIP_READ, @@ -18312,7 +18312,7 @@ .block_erase = SPI_BLOCK_ERASE_C7, } }, - .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_PLAIN, /* TODO: improve */ + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_BP3_SRWD, .unlock = SPI_DISABLE_BLOCKPROTECT, .write = SPI_CHIP_WRITE256, .read = SPI_CHIP_READ, @@ -18367,7 +18367,7 @@ .block_erase = SPI_BLOCK_ERASE_C7, } }, - .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_PLAIN, /* TODO: improve */ + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_BP3_SRWD, .unlock = SPI_DISABLE_BLOCKPROTECT, .write = SPI_CHIP_WRITE256, .read = SPI_CHIP_READ, @@ -18414,7 +18414,7 @@ .block_erase = SPI_BLOCK_ERASE_C7, } }, - .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_PLAIN, /* TODO: improve */ + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_BP3_SRWD, .unlock = SPI_DISABLE_BLOCKPROTECT, .write = SPI_CHIP_WRITE256, .read = SPI_CHIP_READ, @@ -18464,7 +18464,7 @@ .block_erase = SPI_BLOCK_ERASE_C7, } }, - .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_PLAIN, /* TODO: improve */ + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_AMIC_A25L032, .unlock = SPI_DISABLE_BLOCKPROTECT, .write = SPI_CHIP_WRITE256, .read = SPI_CHIP_READ, @@ -18515,7 +18515,7 @@ .block_erase = SPI_BLOCK_ERASE_C7, } }, - .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_PLAIN, /* TODO: improve */ + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_AMIC_A25L032, .unlock = SPI_DISABLE_BLOCKPROTECT, .write = SPI_CHIP_WRITE256, .read = SPI_CHIP_READ, @@ -18567,7 +18567,7 @@ .block_erase = SPI_BLOCK_ERASE_C7, } }, - .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_PLAIN, /* TODO: improve */ + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_AMIC_A25L032, .unlock = SPI_DISABLE_BLOCKPROTECT, .write = SPI_CHIP_WRITE256, .read = SPI_CHIP_READ, @@ -18618,7 +18618,7 @@ .block_erase = SPI_BLOCK_ERASE_C7, } }, - .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_PLAIN, /* TODO: improve */ + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_AMIC_A25L032, .unlock = SPI_DISABLE_BLOCKPROTECT, .write = SPI_CHIP_WRITE256, .read = SPI_CHIP_READ, @@ -18669,7 +18669,7 @@ .block_erase = SPI_BLOCK_ERASE_C7, } }, - .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_PLAIN, /* TODO: improve */ + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_AMIC_A25L032, .unlock = SPI_DISABLE_BLOCKPROTECT, .write = SPI_CHIP_WRITE256, .read = SPI_CHIP_READ, @@ -18721,7 +18721,7 @@ .block_erase = SPI_BLOCK_ERASE_C7, } }, - .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_PLAIN, /* TODO: improve */ + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_AMIC_A25L032, .unlock = SPI_DISABLE_BLOCKPROTECT, .write = SPI_CHIP_WRITE256, .read = SPI_CHIP_READ, @@ -18773,7 +18773,7 @@ .block_erase = SPI_BLOCK_ERASE_C7, } }, - .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_BP2_TB_BPL, + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_AMIC_A25L032, .unlock = SPI_DISABLE_BLOCKPROTECT_BP2_SRWD, .write = SPI_CHIP_WRITE256, .read = SPI_CHIP_READ, @@ -19040,7 +19040,7 @@ .block_erase = SPI_BLOCK_ERASE_C7, } }, - .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_PLAIN, /* TODO: improve */ + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_AMIC_A25L032, .unlock = SPI_DISABLE_BLOCKPROTECT, .write = SPI_CHIP_WRITE256, .read = SPI_CHIP_READ, @@ -19182,7 +19182,7 @@ .block_erase = SPI_BLOCK_ERASE_C7, } }, - .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_PLAIN, /* TODO: improve */ + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_AMIC_A25L032, .unlock = SPI_DISABLE_BLOCKPROTECT, .write = SPI_CHIP_WRITE256, .read = SPI_CHIP_READ, @@ -19232,7 +19232,7 @@ .block_erase = SPI_BLOCK_ERASE_C7, } }, - .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_PLAIN, /* TODO: improve */ + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_AMIC_A25L032, .unlock = SPI_DISABLE_BLOCKPROTECT, .write = SPI_CHIP_WRITE256, .read = SPI_CHIP_READ, @@ -20565,7 +20565,7 @@ .block_erase = SPI_BLOCK_ERASE_C7, } }, - .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_PLAIN, + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_AMIC_A25L032, .unlock = SPI_DISABLE_BLOCKPROTECT, .write = SPI_CHIP_WRITE256, .read = SPI_CHIP_READ, -- To view, visit https://review.coreboot.org/c/flashrom/+/75877?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5066863b514825aee0dffe496492514ac99b6e49 Gerrit-Change-Number: 75877 Gerrit-PatchSet: 1 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: newchange

1 0

[XS] Change in flashrom[master]: Makefile: Fix DirtyJTAG build
by Anastasia Klimchuk (Code Review) 19 Jun '23

19 Jun '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/75891?usp=email ) Change subject: Makefile: Fix DirtyJTAG build ...................................................................... Makefile: Fix DirtyJTAG build Fix typo where "FEATURE_FLAGS" is spelled "FEATURE_CFLAGS", preventing the DirtyJTAG driver from being used. Signed-off-by: Jean THOMAS <virgule(a)jeanthomas.me> Change-Id: I4b6bce24f34848d11731f4bc118a3052fb24e639 Reviewed-on: https://review.coreboot.org/c/flashrom/+/75891 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Peter Marheine <pmarheine(a)chromium.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M Makefile 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Anastasia Klimchuk: Looks good to me, approved build bot (Jenkins): Verified Peter Marheine: Looks good to me, approved diff --git a/Makefile b/Makefile index da5203e..5827cab 100644 --- a/Makefile +++ b/Makefile @@ -828,7 +828,7 @@ endif ifeq ($(CONFIG_DIRTYJTAG_SPI), yes) -FEATURE_CFLAGS += -D'CONFIG_DIRTYJTAG_SPI=1' +FEATURE_FLAGS += -D'CONFIG_DIRTYJTAG_SPI=1' PROGRAMMER_OBJS += dirtyjtag_spi.o ACTIVE_PROGRAMMERS += dirtyjtag_spi endif -- To view, visit https://review.coreboot.org/c/flashrom/+/75891?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4b6bce24f34848d11731f4bc118a3052fb24e639 Gerrit-Change-Number: 75891 Gerrit-PatchSet: 4 Gerrit-Owner: Jean THOMAS <virgule(a)jeanthomas.me> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Williams Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[XS] Change in flashrom[master]: Revert "MAINTAINERS: Add Arthur Heymans for sb600spi"
by Anastasia Klimchuk (Code Review) 19 Jun '23

19 Jun '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/75901?usp=email ) Change subject: Revert "MAINTAINERS: Add Arthur Heymans for sb600spi" ...................................................................... Revert "MAINTAINERS: Add Arthur Heymans for sb600spi" This reverts commit 0bc84bfe4a2b9bb8cbfa719f6221eef2d8f866ca. Change-Id: Ie538337a409f606b717d7104fad346b708f9e2f8 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> Reviewed-on: https://review.coreboot.org/c/flashrom/+/75901 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M MAINTAINERS 1 file changed, 0 insertions(+), 5 deletions(-) Approvals: build bot (Jenkins): Verified Anastasia Klimchuk: Looks good to me, approved diff --git a/MAINTAINERS b/MAINTAINERS index c8e4453..7699eca 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -129,11 +129,6 @@ # PROGRAMMERS ################################################################################ -AMD SB600SPI -M: Arthur Heymans <arthur(a)aheymans.xyz> -S: Maintained -F: sb600spi.c - CH347 M: Nicholas Chin <nic.c3.14(a)gmail.com> S: Maintained -- To view, visit https://review.coreboot.org/c/flashrom/+/75901?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie538337a409f606b717d7104fad346b708f9e2f8 Gerrit-Change-Number: 75901 Gerrit-PatchSet: 2 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[M] Change in flashrom[master]: flashchips.c: Adding support for ISSI IS25WP020/40/80
by Ao Zhong (Code Review) 19 Jun '23

19 Jun '23

Attention is currently required from: Ao Zhong. Hello Anastasia Klimchuk, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/75830?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.c: Adding support for ISSI IS25WP020/40/80 ...................................................................... flashchips.c: Adding support for ISSI IS25WP020/40/80 Hello, I added support for IS25WP020, IS25WP040, and IS25WP080 SPI flash chips. The datasheet for these chips can be found at: https://www.issi.com/WW/pdf/25WP016_080_040_020.pdf Tested read, write, and erase functions on IS25WP080. Here are test logs Write: https://paste.flashrom.org/view.php?id=3698 Write test 2: https://paste.flashrom.org/view.php?id=3699 Erase: https://paste.flashrom.org/view.php?id=3700 Change-Id: I8a786de5cf9ffefb2d57f89bbab71e289b5c2b28 Signed-off-by: Ao Zhong <hacc1225(a)gmail.com> --- M flashchips.c M include/flashchips.h 2 files changed, 129 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/30/75830/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/75830?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8a786de5cf9ffefb2d57f89bbab71e289b5c2b28 Gerrit-Change-Number: 75830 Gerrit-PatchSet: 2 Gerrit-Owner: Ao Zhong <hacc1225(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Ao Zhong <hacc1225(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

[L] Change in flashrom[master]: doc: Add new Development guidelines
by Angel Pons (Code Review) 19 Jun '23

19 Jun '23

Attention is currently required from: Alexander Goncharov, Anastasia Klimchuk, Edward O'Callaghan, Peter Marheine, Stefan Reinauer, Swift Geek (Sebastian Grzywna), Thomas Heijligen. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/75906?usp=email ) Change subject: doc: Add new Development guidelines ...................................................................... Patch Set 2: (3 comments) File doc/dev_guide/development_guidelines.rst: https://review.coreboot.org/c/flashrom/+/75906/comment/d498da49_ebf81d2e : PS1, Line 5: Intro, is there anything useful to say? No idea what to put in an introduction, maybe: > This document describes the rules and recommendations about the development, contribution and review processes. Note: it may be useful to visibly mark open questions in a patch with a `TODO` prefix or similar, so as to (hopefully) catch the reviewers' attention. File doc/dev_guide/development_guidelines.rst: https://review.coreboot.org/c/flashrom/+/75906/comment/5c59fb8a_2e32523a : PS2, Line 7: Set up the git repository and dev environment +1, having detailed instructions in a separate "how-to" document (or multiple) would make more sense. To us, a guideline would be something containing general rules, principles and/or pieces of advice about something (in this case, the development/contribution/review processes). One could link to the how-to's with something like this: > For instructions on how to clone the flashrom repository for development, please refer to the <link to "how-to clone" page>. https://review.coreboot.org/c/flashrom/+/75906/comment/4055a303_6dae5165 : PS2, Line 279: Except, if a commit The comma seems rather clunky, how about: > Exception: if a commit is ... -- To view, visit https://review.coreboot.org/c/flashrom/+/75906?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7fe9ab2e27fead8e795138294219b11240f15928 Gerrit-Change-Number: 75906 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(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-CC: Swift Geek (Sebastian Grzywna) <swiftgeek(a)gmail.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Attention: Swift Geek (Sebastian Grzywna) <swiftgeek(a)gmail.com> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Mon, 19 Jun 2023 09:27:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Swift Geek (Sebastian Grzywna) <swiftgeek(a)gmail.com> Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: comment

1 0

[L] Change in flashrom[master]: doc: Add new Development guidelines
by Peter Marheine (Code Review) 19 Jun '23

19 Jun '23

Attention is currently required from: Alexander Goncharov, Anastasia Klimchuk, Angel Pons, Edward O'Callaghan, Stefan Reinauer, Swift Geek (Sebastian Grzywna), Thomas Heijligen. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/75906?usp=email ) Change subject: doc: Add new Development guidelines ...................................................................... Patch Set 2: (10 comments) File doc/dev_guide/development_guidelines.rst: https://review.coreboot.org/c/flashrom/+/75906/comment/2bfd90d8_020fc00a : PS2, Line 7: Set up the git repository and dev environment This is more of a how-to than guideline, which seems a little weird given the document name. Maybe rename this file to the development guide instead? https://review.coreboot.org/c/flashrom/+/75906/comment/5c168b7c_ac17dff8 : PS2, Line 27: Create a commit and sign-off ``git commit -s``. This is a good concise summary, but it's kind of confusing with this wording. Perhaps: > In short, commit your changes with a descriptive message and remember to sign off > on the commit (``git commit -s``). https://review.coreboot.org/c/flashrom/+/75906/comment/aeae58d0_5d59fbba : PS2, Line 141: there Remove unnecessary word. https://review.coreboot.org/c/flashrom/+/75906/comment/acd3d905_467abb42 : PS2, Line 216: The only official repository is https://review.coreboot.org/flashrom and GitHub and GitLab are just mirrors. Slight modification for clarity: > The only official repository is https://review.coreboot.org/flashrom; GitHub and GitLab are only mirrors. **Reviewers will not look at pull requests** on mirrors. https://review.coreboot.org/c/flashrom/+/75906/comment/f4b14f76_eb028aaf : PS2, Line 221: sending : it to review.coreboot.org. Link this to the Working With Gerrit heading above? https://review.coreboot.org/c/flashrom/+/75906/comment/c7100ac8_ceb217b0 : PS2, Line 225: Code review process This heading seems redundant because other sections also discuss code review. I think the paragraphs here should be moved to other subsections where they're more relevant and this one deleted. https://review.coreboot.org/c/flashrom/+/75906/comment/9ae43246_32521577 : PS2, Line 228: Our guidelines borrow heavily from `coreboot coding style : <https://doc.coreboot.org/contributing/coding_style.html>`_ and `Gerrit : guidelines <https://doc.coreboot.org/contributing/gerrit_guidelines.html>`_, : and most of them apply to flashrom as well. The really important part is about : the :ref:`sign-off procedure <sign-off>`. : : We try to **reuse as much code as possible** and create new files only if : absolutely needed, so if you find a function somewhere in the tree which : already does what you want, please use it. These paragraphs seems like they belong in the Coding Style section above. https://review.coreboot.org/c/flashrom/+/75906/comment/dbc16486_a684e309 : PS2, Line 241: If you introduce new features (not flash chips, but stuff like partial : programming, support for new external programmers, voltage handling, etc) : please **discuss your plans** on the :ref:`mailing list` first. That way, we : can avoid duplicated work and know about how flashrom internals need to be : adjusted and you avoid frustration if there is some disagreement about the : design. This seems like it belongs near the top (before mechanics of how to write and share patches) since otherwise the document flow seems to suggest writing a patch but then this says you shouldn't necessarily start by just sending a patch. https://review.coreboot.org/c/flashrom/+/75906/comment/75fa5346_16c30d64 : PS2, Line 248: Initial review should include a broad indication of acceptance or rejection of : the idea/rationale/motivation or the implementation : : In general, reviews should focus on the architectural changes and things that : affect flashrom as a whole. This includes (but is by no means limited to) : changes in APIs and types, safety, portability, extensibility, and : maintainability. The purpose of reviews is not to create perfect patches, but : to steer development in the right direction and produce consensus within the : community. The goal of each patch should be to improve the state of the project : - it does not need to fix all problems of the respective field perfectly. : : .. note:: : : New contributors may need more detailed advices and should be told about : minor issues like formatting problems more precisely. The result of a review : should either be an accepted patch or a guideline how the existing code : should be changed to be eventually accepted. : : If you sent a patch and later lost interest or no longer have time to follow up on code review, : please add a comment saying so. Then, if any of our maintainers are interested in finishing the work, : they can take over the patch. Looks like part of "Going Through Code Reviews" https://review.coreboot.org/c/flashrom/+/75906/comment/5b055647_795506d6 : PS2, Line 270: Merging patches : =============== : : Merging to branches is limited to the "flashrom developers" group on Gerrit. : The following rules apply, some are already : enforced by Gerrit: : : * Every commit has to be reviewed and needs at least one +2 that was not given : by the commit's author. : * Except, if a commit is authored by more than one person, each author may +2 : the other author's changes. : * flashrom developers are people from literally all around the planet, and various : timezones, so we usually wait for 3 days after the patch is fully approved just in case : of last minute concerns from all timezones. : * In the case of emergency, merging should not take place within less than 24 hours after the review : started (i.e. the first message by a reviewer on Gerrit). : * Finally, before hitting Submit, one is reponsible to check that all comments : have been addressed, especially if there was a negative review (-1). Make this the final subsection of "Working With Gerrit". -- To view, visit https://review.coreboot.org/c/flashrom/+/75906?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7fe9ab2e27fead8e795138294219b11240f15928 Gerrit-Change-Number: 75906 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(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-CC: Swift Geek (Sebastian Grzywna) <swiftgeek(a)gmail.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Swift Geek (Sebastian Grzywna) <swiftgeek(a)gmail.com> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Mon, 19 Jun 2023 01:56:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: meson: Add support for ni845x_spi on Windows
by Peter Marheine (Code Review) 19 Jun '23

19 Jun '23

Attention is currently required from: Anastasia Klimchuk, Miklós Márton, Stefan Reinauer, Thomas Heijligen. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/75236?usp=email ) Change subject: meson: Add support for ni845x_spi on Windows ...................................................................... Patch Set 8: (1 comment) Patchset: PS4: > Thomas, do you plan any more testing? Which include path do you mean, is that the one hardcoded in m […] I attempted to test that the build worked as expected when the library path doesn't exist and found that the library handing was too clever and broke builds with `programmer=auto` when the library wasn't found: ``` ../meson.build:546:26: ERROR: Unknown method "name" in object <[ExternalLibraryHolder] holds [ExternalLibrary]: <ExternalLibrary ni845x: False>> of type ExternalLibraryHolder. ``` I fixed that by always setting `libni845x` to the real dependency on Windows (reintroducing the manual check for include directory existence which is required), and tested these cases: * Windows with `programmer=auto`: successful build without ni845x * Windows with programmer=auto and ni845x_search_path pointing to a directory that doesn't exist: successful build * Windows with ni845x and search path as default: successful build * Windows with `programmer=ni845x_spi` and `ni845x_search_path=C:\foobar\`: fails because library is not found * Linux with programmer=auto: successful build * Linux with programmer=ni845x_spi: error, programmer not supported on this platform -- To view, visit https://review.coreboot.org/c/flashrom/+/75236?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2d32f11852ac1a5184af8e8683ca1914a6e72973 Gerrit-Change-Number: 75236 Gerrit-PatchSet: 8 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.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-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 19 Jun 2023 01:41:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit June 2023