Attention is currently required from: Anastasia Klimchuk, David Hendricks, Matt DeVillier.
Anton Samsonov has posted comments on this change by Matt DeVillier. ( https://review.coreboot.org/c/flashrom/+/85160?usp=email )
Change subject: cli_classic: Add option to use first detected chip if multiple found
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
> There are certainly cases where there is no harm in selecting the first chip of multiple.
There may be a couple of options here.
1. Automatic disambiguation through advanced fingerprinting
Did you examined Device ID combinations from different identification methods, such as RDID + RES/REMS in cases when no extended RDID is available? (I did not dug deep, but noticed that 8-bit Device ID from REMS not necessarily copies the least significant 8-bit Device ID from RDID, creating a possibility of distinct 24-bit combinations compared to regular 16-bit RDID duplicates.)
As proposed by Anastasia, some parts of SFDP data may become another source of identification, when compared among similar models relevant to your case.
2. Automatic parameter tuning
If you are absolutely sure that several chip models that share single Device ID constitute a mutually interchangeable group of device profiles, then why not create "compatibility domains" in flashchips database? Or, better yet, why not create them automatically on-the-fly? That is, create a "meta-device" which acts like greatest common denominator for all compatible chips in question. Like, first AND all feature bits, then OR them, and compare those after masking relevant bits.
For example, the aforementioned Spansion "twins" have these flags:
S25FL512S = FEATURE_WRSR_WREN | FEATURE_OTP |
FEATURE_4BA_NATIVE | FEATURE_4BA_ENTER_EAR7 | FEATURE_4BA_EAR_1716
S25FS512S = FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_QPI |
FEATURE_4BA_NATIVE | FEATURE_4BA_ENTER
Of our interest, we see FEATURE_WRSR_WREN and FEATURE_4BA_NATIVE in common,
but no universally supported method of entering 4-byte addressing. So, those
chips cannot be actually treated interchangeably, which is proven by practice.
But there may be plenty of other chips that possibly can, if you are right.
--
To view, visit https://review.coreboot.org/c/flashrom/+/85160?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: I0427b1ef80e4eca16623f0fc9119d79f7dd62551
Gerrit-Change-Number: 85160
Gerrit-PatchSet: 2
Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anton Samsonov <avscomputing(a)gmail.com>
Gerrit-CC: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Tue, 17 Dec 2024 18:52:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anton Samsonov <avscomputing(a)gmail.com>
Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)gmail.com>
Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Attention is currently required from: Anastasia Klimchuk, Bill XIE, Stefan Reinauer.
Matt DeVillier has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/85612?usp=email )
Change subject: ichspi: Check whether chipset is locked when probing opcode
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit https://review.coreboot.org/c/flashrom/+/85612?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: 1.5.x
Gerrit-Change-Id: I701a86f030cfef43a1158bf075287ade569254e6
Gerrit-Change-Number: 85612
Gerrit-PatchSet: 1
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Tue, 17 Dec 2024 16:13:39 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Anastasia Klimchuk, Bill XIE, Stefan Reinauer.
Matt DeVillier has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/85612?usp=email )
Change subject: ichspi: Check whether chipset is locked when probing opcode
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> This is a cherry-pick of CB:85592 into branch 1.5.x […]
yep I tested against main and 1.5.x
--
To view, visit https://review.coreboot.org/c/flashrom/+/85612?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: 1.5.x
Gerrit-Change-Id: I701a86f030cfef43a1158bf075287ade569254e6
Gerrit-Change-Number: 85612
Gerrit-PatchSet: 1
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Tue, 17 Dec 2024 16:13:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Attention is currently required from: Bill XIE, Matt DeVillier, Stefan Reinauer.
Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/85612?usp=email )
Change subject: ichspi: Check whether chipset is locked when probing opcode
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
Bill, if you are reading this, I am hoping maybe you can join and test that https://ticket.coreboot.org/issues/556 is still working fine with this patch? Thank you so much!
--
To view, visit https://review.coreboot.org/c/flashrom/+/85612?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: 1.5.x
Gerrit-Change-Id: I701a86f030cfef43a1158bf075287ade569254e6
Gerrit-Change-Number: 85612
Gerrit-PatchSet: 1
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Comment-Date: Tue, 17 Dec 2024 06:09:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Bill XIE, Matt DeVillier, Stefan Reinauer.
Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/85612?usp=email )
Change subject: ichspi: Check whether chipset is locked when probing opcode
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
This is a cherry-pick of CB:85592 into branch 1.5.x
The branch starts from v1.5.0 and has this one patch only.
Matt, is it possible for you to test the ticket https://ticket.coreboot.org/issues/573 in the branch too? I know you tested already, but just in case, it would be great to run it in the branch too. Thank you so much!
--
To view, visit https://review.coreboot.org/c/flashrom/+/85612?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: 1.5.x
Gerrit-Change-Id: I701a86f030cfef43a1158bf075287ade569254e6
Gerrit-Change-Number: 85612
Gerrit-PatchSet: 1
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Comment-Date: Tue, 17 Dec 2024 06:07:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/85612?usp=email )
Change subject: ichspi: Check whether chipset is locked when probing opcode
......................................................................
ichspi: Check whether chipset is locked when probing opcode
This is follow up (or fix) for
commit 26a1eb514ccefc61b110068cf0eea73c397ba045
When probing opcode, all opcodes in POSSIBLE_OPCODES are reported as
supported, even if the opcode is not in curopcodes. This is relying
on reprogramming on-the-fly to handle the gap between POSSIBLE_OPCODES
and curopcodes. However, for locked chipsets on-the-fly is not
happening (is not possible, since list of opcodes is locked), so we
can't rely on it.
So, we need to check whether chipset is locked.
Ticket: https://ticket.coreboot.org/issues/573
Change-Id: I701a86f030cfef43a1158bf075287ade569254e6
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
Tested-by: Matt DeVillier <matt.devillier(a)gmail.com>
---
M ichspi.c
1 file changed, 6 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/12/85612/1
diff --git a/ichspi.c b/ichspi.c
index fc994db..94227de 100644
--- a/ichspi.c
+++ b/ichspi.c
@@ -1823,8 +1823,12 @@
static bool ich_spi_probe_opcode(const struct flashctx *flash, uint8_t opcode)
{
int ret = find_opcode(curopcodes, opcode);
- if ((ret == -1) && (lookup_spi_type(opcode) <= 3))
- /* opcode is in POSSIBLE_OPCODES, report supported. */
+ if ((ret == -1) && (lookup_spi_type(opcode) <= 3) && (!ichspi_lock))
+ /* opcode is in POSSIBLE_OPCODES, report supported.
+ * Relying on reprogramming on-the-fly
+ * when opcode is not in curopcodes, but is in POSSIBLE_OPCODES.
+ *
+ * on-the-fly does not work for locked chipsets, therefore checking ichspi_lock. */
return true;
return ret >= 0;
}
--
To view, visit https://review.coreboot.org/c/flashrom/+/85612?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: 1.5.x
Gerrit-Change-Id: I701a86f030cfef43a1158bf075287ade569254e6
Gerrit-Change-Number: 85612
Gerrit-PatchSet: 1
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Attention is currently required from: Anastasia Klimchuk, Anton Samsonov, David Hendricks.
Matt DeVillier has posted comments on this change by Matt DeVillier. ( https://review.coreboot.org/c/flashrom/+/85160?usp=email )
Change subject: cli_classic: Add option to use first detected chip if multiple found
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
> My point is: do whatever you can to identify chips programmatically, rather than relying on people and their "first choice", unless you plan to provoke massive bricking.
I agree completely. However, there are certainly cases where there is no harm in selecting the first chip of multiple, and where having the user disassemble the device to determine the correct chip simply isn't feasible. If the consensus is that this option isn't a good fit for upstream, then I'll just keep it in my fork
--
To view, visit https://review.coreboot.org/c/flashrom/+/85160?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: I0427b1ef80e4eca16623f0fc9119d79f7dd62551
Gerrit-Change-Number: 85160
Gerrit-PatchSet: 2
Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anton Samsonov <avscomputing(a)gmail.com>
Gerrit-CC: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Anton Samsonov <avscomputing(a)gmail.com>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Mon, 16 Dec 2024 17:56:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anton Samsonov <avscomputing(a)gmail.com>
Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Attention is currently required from: Antonio Vázquez Blanco, David Reguera Garcia (Dreg), Matti Finder, Miklós Márton, Nikolai Artemiev.
Sergii Dmytruk has posted comments on this change by Antonio Vázquez Blanco. ( https://review.coreboot.org/c/flashrom/+/85539?usp=email )
Change subject: Extract SPI declarations to the correct header
......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/85539/comment/93cdb036_fbdba7e1?us… :
PS2, Line 7: Extract SPI declarations to the correct header
Nit: "extract" usually means creating something new in the process of move (header in this case), so I think "move" explains what happens better. I'd also mention at least target header because I had to look through the changes to understand what is being done.
--
To view, visit https://review.coreboot.org/c/flashrom/+/85539?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: I794a71536a3b85fde39f83c802fa0f5dd8d428e0
Gerrit-Change-Number: 85539
Gerrit-PatchSet: 2
Gerrit-Owner: Antonio Vázquez Blanco <antoniovazquezblanco(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: David Reguera Garcia (Dreg) <regueragarciadavid(a)gmail.com>
Gerrit-Reviewer: Matti Finder <matti.finder(a)gmail.com>
Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
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: Antonio Vázquez Blanco <antoniovazquezblanco(a)gmail.com>
Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: David Reguera Garcia (Dreg) <regueragarciadavid(a)gmail.com>
Gerrit-Attention: Matti Finder <matti.finder(a)gmail.com>
Gerrit-Comment-Date: Mon, 16 Dec 2024 15:02:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Anastasia Klimchuk, David Hendricks, Matt DeVillier.
Anton Samsonov has posted comments on this change by Matt DeVillier. ( https://review.coreboot.org/c/flashrom/+/85160?usp=email )
Change subject: cli_classic: Add option to use first detected chip if multiple found
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
> Thank you David, this is useful!
My two cents.
I recently submitted a patch for adding Spansion S25FS512S, because several — not just one! — of my colleagues happily relied on the auto-detected S25FL512S (its FS vs FL, with different command set and register handling, and different voltage as well) and scratched their head why it results in bricking. They were using both internal linux_spi and external dediprog, including with original DediProg Engineering software, but no one of them bothered with visually expecting the motherboard for identifying the chip, which would be pretty easy. People are lazy; even the engineers are, too. Luckily, Spansion provides additional Device ID info for disambiguating the chips programmatically.
Another recent example was Spansion S25FL128S......0, which shares the same 2-byte ID with 7 another models, including S25FL128S......1 (with different sector size), S25FL128P......0 (with different command behavior), S25FS128S (see "FS-vs-FL" above) and so on. Not surprisingly, when people are not instructed which chip to select, they happily select the first one, or a random one, and end up with a bricked device, even when they also have the opportunity to open the chassis and have a look at the motherboard near the SPI header. Since those chips are all Spansion, too, I'll try to write a patch what would disambiguate them all, if it's acceptable to submit patches that affect multiple chips based on their datasheets only, with testing on a single chip only.
My point is: do whatever you can to identify chips programmatically, rather than relying on people and their "first choice", unless you plan to provoke massive bricking.
--
To view, visit https://review.coreboot.org/c/flashrom/+/85160?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: I0427b1ef80e4eca16623f0fc9119d79f7dd62551
Gerrit-Change-Number: 85160
Gerrit-PatchSet: 2
Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anton Samsonov <avscomputing(a)gmail.com>
Gerrit-CC: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Mon, 16 Dec 2024 12:33:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Attention is currently required from: Anastasia Klimchuk, Bill XIE.
Hello Bill XIE, Matt DeVillier, Nikolai Artemiev, Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/85592?usp=email
to look at the new patch set (#4).
Change subject: ichspi: Check whether chipset is locked when probing opcode
......................................................................
ichspi: Check whether chipset is locked when probing opcode
This is follow up (or fix) for
commit 26a1eb514ccefc61b110068cf0eea73c397ba045
When probing opcode, all opcodes in POSSIBLE_OPCODES are reported as
supported, even if the opcode is not in curopcodes. This is relying
on reprogramming on-the-fly to handle the gap between POSSIBLE_OPCODES
and curopcodes. However, for locked chipsets on-the-fly is not
happening (is not possible, since list of opcodes is locked), so we
can't rely on it.
So, we need to check whether chipset is locked.
Ticket: https://ticket.coreboot.org/issues/573
Change-Id: I701a86f030cfef43a1158bf075287ade569254e6
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
Tested-by: Matt DeVillier <matt.devillier(a)gmail.com>
---
M ichspi.c
1 file changed, 6 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/92/85592/4
--
To view, visit https://review.coreboot.org/c/flashrom/+/85592?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I701a86f030cfef43a1158bf075287ade569254e6
Gerrit-Change-Number: 85592
Gerrit-PatchSet: 4
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
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: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>