Attention is currently required from: Anastasia Klimchuk, DZ, Stefan Reinauer.
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79633?usp=email )
Change subject: flashchips: Remove Macronix MX25U25635F from chip list
......................................................................
Patch Set 1:
(1 comment)
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/79633/comment/4b20278f_2c9dc05b :
PS1, Line 10310: .wps = {SECURITY, 7, OTP}, /* This bit is set by WPSEL command */
> Thanks for details, now I understand the situation. […]
Deleting the chip entry isn't a good solution as flashrom should still support the older model.
If it is possible for flashrom to distinguish the chips some how (e.g. custom opcode), then you can use a custom probe function to do that.
Otherwise, if the customer builds their own version of flashrom, they can use a local blocklist to ignore certain chips with duplicated IDs. This is what we did for ChromeOS.
--
To view, visit https://review.coreboot.org/c/flashrom/+/79633?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: Ief3fd7641bed817066692c4abffff6d3b0df16b9
Gerrit-Change-Number: 79633
Gerrit-PatchSet: 1
Gerrit-Owner: DZ
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
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Tue, 23 Jan 2024 23:34:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: DZ
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Edward O'Callaghan, Hsuan-ting Chen, Paul Menzel.
Brian Norris has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79060?usp=email )
Change subject: fmap: Update major/minor version check
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
any chance someone could submit this for me? I don't have permissions
--
To view, visit https://review.coreboot.org/c/flashrom/+/79060?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: I984835579d3b257a2462906f1f5091b179891bd0
Gerrit-Change-Number: 79060
Gerrit-PatchSet: 2
Gerrit-Owner: Brian Norris <briannorris(a)chromium.org>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Hsuan Ting Chen <roccochen(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Mon, 22 Jan 2024 20:19:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: DZ, Nikolai Artemiev, Stefan Reinauer.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79633?usp=email )
Change subject: flashchips: Remove Macronix MX25U25635F from chip list
......................................................................
Patch Set 1:
(1 comment)
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/79633/comment/38044a19_320984cc :
PS1, Line 10310: .wps = {SECURITY, 7, OTP}, /* This bit is set by WPSEL command */
> Thank you for your review. […]
Thanks for details, now I understand the situation.
I looked through the code which handles `.wps` bits: wps bits are read and written by WPSEL command.
WPSEL command is described in datasheet for MX25U25643G (newer model), but the command is not mentioned in MX25U25635F datasheet (older model).
What exactly happens if WPSEL command is issued for MX25U25635F (older chip)? Is it ignored, or everything crashes with error?
Can you maybe find out the answer for this specific question?
Thanks!
--
To view, visit https://review.coreboot.org/c/flashrom/+/79633?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: Ief3fd7641bed817066692c4abffff6d3b0df16b9
Gerrit-Change-Number: 79633
Gerrit-PatchSet: 1
Gerrit-Owner: DZ
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
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Sat, 20 Jan 2024 07:38:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: DZ
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk, Anton Samsonov, Thomas Heijligen.
Anton Samsonov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/77778?usp=email )
Change subject: Makefile,meson.build: Add support for Sphinx 3.x
......................................................................
Patch Set 4:
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/77778/comment/22b65aba_b8312a3a :
PS3, Line 7: 3
> 3 or 4? From the patch it seems like 4. […]
The patch adds support for older 3.x versions, since 4.x and newer exhibit the [naturally] expected behavior.
https://review.coreboot.org/c/flashrom/+/77778/comment/4d17918d_7b6f01d5 :
PS3, Line 10: 7996 and 9217
> This is very useful info, but you would need to add full links to GitHub issues (so that it's easier […]
Acknowledged
https://review.coreboot.org/c/flashrom/+/77778/comment/faeb4a66_7ccfd2a3 :
PS3, Line 13: but not on `sphinx-build` own level
: upon which `meson` build relies
> I don't fully understand this last part of the sentence, what does it mean "not on sphinx-build own […]
doc/meson.build contains the following:
`build_always_stale : true, # sphinx handles rebuilds`
It means that `sphinx-build` is always run by `meson`, and that utility itself decides whether a man page should be rebuild — not the `meson` build system. When we rename the "8" directory that is created by `sphinx-build` 3.x to "man8" directory that is expected by doc/meson.build, we effectively break `sphinx-build` ability to avoid future rebuilds. (This is unlike with `make` that handles dependencies by itself, as specified in Makefile.)
Alternatively, a symlink "8" may be created pointing at "man8" after the directory renaming occurs. Or "man8" may be created as a hardlink to "8". In that case `sphinx-build` 3.x would be able to avoid unnecessary rebuilds. I just didn't think it might be necessary, given the small build time and a single man page only; but should you consider it worth the efforts, I will try to implement that trick (not totally sure whether it will have the desired effect, though).
--
To view, visit https://review.coreboot.org/c/flashrom/+/77778?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: I9cd280551a1ba4d17edb2e857d56f80431b61e1b
Gerrit-Change-Number: 77778
Gerrit-PatchSet: 4
Gerrit-Owner: Anton Samsonov <devel(a)zxlab.ru>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anton Samsonov <avscomputing(a)gmail.com>
Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Anton Samsonov <devel(a)zxlab.ru>
Gerrit-Comment-Date: Thu, 18 Jan 2024 18:32:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Anton Samsonov, Thomas Heijligen.
Anton Samsonov has uploaded a new patch set (#4) to the change originally created by Anton Samsonov. ( https://review.coreboot.org/c/flashrom/+/77778?usp=email )
Change subject: Makefile,meson.build: Add support for Sphinx 3.x
......................................................................
Makefile,meson.build: Add support for Sphinx 3.x
Prior to version 4.0, `sphinx-build` outputs man pages to "8" directory
instead of "man8" expected by Makefile and doc/meson.build. See:
https://github.com/sphinx-doc/sphinx/issues/7996https://github.com/sphinx-doc/sphinx/issues/9217
Current solution is to rename "8" to "man8" after documentation build.
That enables successful build and installation, as well as dependency
tracking at build-system level, but not on `sphinx-build` own level
upon which `meson` build relies.
Change-Id: I9cd280551a1ba4d17edb2e857d56f80431b61e1b
Signed-off-by: Anton Samsonov <devel(a)zxlab.ru>
---
M Makefile
M doc/meson.build
A doc/sphinx-wrapper.sh
3 files changed, 40 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/78/77778/4
--
To view, visit https://review.coreboot.org/c/flashrom/+/77778?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: I9cd280551a1ba4d17edb2e857d56f80431b61e1b
Gerrit-Change-Number: 77778
Gerrit-PatchSet: 4
Gerrit-Owner: Anton Samsonov <devel(a)zxlab.ru>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anton Samsonov <devel(a)zxlab.ru>
Gerrit-MessageType: newpatchset
Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev, Stefan Reinauer.
DZ has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79633?usp=email )
Change subject: flashchips: Remove Macronix MX25U25635F from chip list
......................................................................
Patch Set 1:
(1 comment)
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/79633/comment/816a1ee1_6c3c8bb3 :
PS1, Line 10310: .wps = {SECURITY, 7, OTP}, /* This bit is set by WPSEL command */
> I looked through both datasheets, and found there is unfortunately this one difference! […]
Thank you for your review. Now we have a customer want to use Macronix MX25U25643G that has same model id with MX25U25635F, therefore flashrom need one more operation to choose the specific Flash EPN name in probe stage. This will push this customer to change their pre-defined automatic test flow. So, we want to remove MX25U25635F from chip list to fullfill this customer's request. Do you have some suggestions on this issue.
--
To view, visit https://review.coreboot.org/c/flashrom/+/79633?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: Ief3fd7641bed817066692c4abffff6d3b0df16b9
Gerrit-Change-Number: 79633
Gerrit-PatchSet: 1
Gerrit-Owner: DZ
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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Thu, 18 Jan 2024 01:52:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk, Edward O'Callaghan, Evan Benn.
Hsuan-ting Chen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79304?usp=email )
Change subject: flashrom_tester: Fix partial_lock_test on libflashrom
......................................................................
Patch Set 4:
(2 comments)
File util/flashrom_tester/Cargo.toml:
https://review.coreboot.org/c/flashrom/+/79304/comment/feca2f69_92a9220f :
PS3, Line 25: libflashrom = { path = "../../bindings/rust/libflashrom" }
> I am curious: why it was working before, without this line?
This is for the `use libflashrom::FlashromFlags;` in `util/flashrom_tester/src/tester.rs`, where we didn't need `libflashrom` in that file before.
File util/flashrom_tester/flashrom/src/cmd.rs:
https://review.coreboot.org/c/flashrom/+/79304/comment/ea645ee9_7ba6d13a :
PS3, Line 301: // TODO
> Did you mean to do something here?
The flashrom CLI sets its own default flags, and we currently have no need for custom flags, so this set_flags function is intentionally a no-op.
(Change the comments)
--
To view, visit https://review.coreboot.org/c/flashrom/+/79304?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: I7a8ac0c0984fef3cd9e73ed8d8097ddf429e54b2
Gerrit-Change-Number: 79304
Gerrit-PatchSet: 4
Gerrit-Owner: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: David Wu <david_wu(a)quanta.corp-partner.google.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Evan Benn <evanbenn(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Hsuan Ting Chen <roccochen(a)chromium.org>
Gerrit-CC: Nikolai Artemiev <nartemiev(a)chromium.org>
Gerrit-Attention: Evan Benn <evanbenn(a)gmail.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Mon, 15 Jan 2024 11:04:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: David Wu, Edward O'Callaghan, Evan Benn, Hsuan-ting Chen.
Hello Anastasia Klimchuk, David Wu, Edward O'Callaghan, Evan Benn, Hsuan Ting Chen, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/79304?usp=email
to look at the new patch set (#4).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: flashrom_tester: Fix partial_lock_test on libflashrom
......................................................................
flashrom_tester: Fix partial_lock_test on libflashrom
partial_lock_test (Lock_top_quad, Lock_bottom_quad, Lock_bottom_half,
and Lock_top_half) tries to:
1. Disable HWWP
2. Lock partial
3. Enable HWWP
4. Try to write the locked partial and expect a failure
...
The 4th step only works on flashrom binary since the binary will set
FLASHROM_FLAG_VERIFY_AFTER_WRITE=1 by default and it will error out
while verifying.
But libflashrom does not set any flag beforehand, so it has
FLASHROM_FLAG_VERIFY_AFTER_WRITE=0 and thus it will think the write
command works normally and raise no error. This causes the issue that
flashrom_tester with libflashrom has been failed until today.
To solve this issue, there are two solutions:
1. Take care of the default flags in libflashrom
2. Always pass --noverify to flashrom binary and verify it afterwards.
To make both methods more consistent, I fix it with approach 1.
BUG=b:304439294
BRANCH=none
TEST=flashrom_tester internal --libflashrom Lock_top_quad
Change-Id: I7a8ac0c0984fef3cd9e73ed8d8097ddf429e54b2
Signed-off-by: roccochen(a)chromium.com <roccochen(a)chromium.org>
---
M bindings/rust/libflashrom/src/lib.rs
M util/flashrom_tester/Cargo.toml
M util/flashrom_tester/flashrom/src/cmd.rs
M util/flashrom_tester/flashrom/src/flashromlib.rs
M util/flashrom_tester/flashrom/src/lib.rs
M util/flashrom_tester/src/tester.rs
6 files changed, 125 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/04/79304/4
--
To view, visit https://review.coreboot.org/c/flashrom/+/79304?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: I7a8ac0c0984fef3cd9e73ed8d8097ddf429e54b2
Gerrit-Change-Number: 79304
Gerrit-PatchSet: 4
Gerrit-Owner: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: David Wu <david_wu(a)quanta.corp-partner.google.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Evan Benn <evanbenn(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Hsuan Ting Chen <roccochen(a)chromium.org>
Gerrit-CC: Nikolai Artemiev <nartemiev(a)chromium.org>
Gerrit-Attention: David Wu <david_wu(a)quanta.corp-partner.google.com>
Gerrit-Attention: Evan Benn <evanbenn(a)gmail.com>
Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Anton Samsonov, Thomas Heijligen.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79152?usp=email )
Change subject: Makefile: Fix version string for non-Git builds
......................................................................
Patch Set 5: Code-Review+1
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/79152/comment/db59f4a7_f2c0488a :
PS5, Line 7: non-Git builds
What is an example of non-Git build? I am probably missing something, how do you get the repo without Git?
(The answer would probably be a good addition to commit description)
--
To view, visit https://review.coreboot.org/c/flashrom/+/79152?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: I8694e618878823a9e96b1f2bcfa63f6c71d3c2ed
Gerrit-Change-Number: 79152
Gerrit-PatchSet: 5
Gerrit-Owner: Anton Samsonov <devel(a)zxlab.ru>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Anton Samsonov <contact-launchpad(a)zxlab.ru>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anton Samsonov <devel(a)zxlab.ru>
Gerrit-Comment-Date: Mon, 15 Jan 2024 08:55:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment