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
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
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
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
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
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
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
Kevin Yu has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81258?usp=email )
Change subject: Add support for Idaville HCC platform and iRC regions
......................................................................
Patch Set 2:
This change is ready for review.
--
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-Comment-Date: Fri, 15 Mar 2024 07:36:26 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Riku Viitanen, Stefan Reinauer.
Hello Riku Viitanen, Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/81032?usp=email
to look at the new patch set (#3).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: serprog: Fix scan-build warning of resource leak
......................................................................
serprog: Fix scan-build warning of resource leak
Warning found by the latest scan-build run:
*** CID 1534883: (RESOURCE_LEAK)
/serprog.c: 853 in serprog_init()
847 "by programmer!\n", cs_num8);
848 goto init_err_cleanup_exit;
849 }
850 }
851 bt = serprog_buses_supported;
852 if (sp_docommand(S_CMD_S_BUSTYPE, 1, &bt, 0, NULL))
>>>CID 1534883: (RESOURCE_LEAK)
>>>Variable "cs" going out of scope leaks the storage it points to.
853 goto init_err_cleanup_exit;
854 }
Follow up on
commit e8c350f55e596aae3ab2bbc210b68389e2301a6c
Change-Id: Id9cf211de3c482f702adebfcfa274a183c83a33f
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
---
M serprog.c
1 file changed, 4 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/81032/3
--
To view, visit https://review.coreboot.org/c/flashrom/+/81032?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: Id9cf211de3c482f702adebfcfa274a183c83a33f
Gerrit-Change-Number: 81032
Gerrit-PatchSet: 3
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Riku Viitanen <riku.viitanen(a)protonmail.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Riku Viitanen <riku.viitanen(a)protonmail.com>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-MessageType: newpatchset