Attention is currently required from: Nikolai Artemiev, Stefan Reinauer, Victor Lim.
Anastasia Klimchuk has posted comments on this change by Victor Lim. ( https://review.coreboot.org/c/flashrom/+/84090?usp=email )
Change subject: flashchips: add GD25F256F
......................................................................
Patch Set 2:
(1 comment)
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/84090/comment/c8cb6954_ef898103?us… :
PS2, Line 8056: reg_bits
Nikolai, I wanted to check with you, if srp is not defined in datasheet , is it fine to not define it in reg_bits?
Thank you!
--
To view, visit https://review.coreboot.org/c/flashrom/+/84090?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: Ibbbbb8a55adbcbc2ee1785782c4eb3771d50c167
Gerrit-Change-Number: 84090
Gerrit-PatchSet: 2
Gerrit-Owner: Victor Lim <vlim(a)gigadevice.com>
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: Victor Lim <vlim(a)gigadevice.com>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Wed, 09 Oct 2024 10:28:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/84253?usp=email )
Change subject: ichspi: Probe opcode in POSSIBLE_OPCODES[] as well
......................................................................
ichspi: Probe opcode in POSSIBLE_OPCODES[] as well
ich_spi_send_command() and ich_spi_send_multicommand() will overwrite
the "Sector erase" opcode with the opcode for command via
reprogram_opcode_on_the_fly(), but not restore it, causing the "Sector
erase" opcode may get lost after sending commands, leaving only "Bulk
erase" opcode which erase the whole chip available.
In the mean time, ich_spi_probe_opcode() used not to report opcodes in
POSSIBLE_OPCODES[] but not in curopcodes->opcode[] as supported.
Now, if the opcode being probed is not in curopcodes->opcode[] but in
POSSIBLE_OPCODES[], it will be reported as supported, and programmed
later by ich_spi_send_(multi)command().
Fix:https://ticket.coreboot.org/issues/556
Change-Id: I3fc831fc072e2af9265835cb2f71bf8c222c6a64
Signed-off-by: persmule <persmule(a)hardenedlinux.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/84253
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org>
Reviewed-by: Nikolai Artemiev <nartemiev(a)google.com>
---
M ichspi.c
1 file changed, 5 insertions(+), 1 deletion(-)
Approvals:
Anastasia Klimchuk: Looks good to me, approved
build bot (Jenkins): Verified
Nikolai Artemiev: Looks good to me, approved
diff --git a/ichspi.c b/ichspi.c
index 3388657..fc994db 100644
--- a/ichspi.c
+++ b/ichspi.c
@@ -1822,7 +1822,11 @@
static bool ich_spi_probe_opcode(const struct flashctx *flash, uint8_t opcode)
{
- return find_opcode(curopcodes, opcode) >= 0;
+ int ret = find_opcode(curopcodes, opcode);
+ if ((ret == -1) && (lookup_spi_type(opcode) <= 3))
+ /* opcode is in POSSIBLE_OPCODES, report supported. */
+ return true;
+ return ret >= 0;
}
#define ICH_BMWAG(x) ((x >> 24) & 0xff)
--
To view, visit https://review.coreboot.org/c/flashrom/+/84253?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: merged
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I3fc831fc072e2af9265835cb2f71bf8c222c6a64
Gerrit-Change-Number: 84253
Gerrit-PatchSet: 6
Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/84595?usp=email )
Change subject: ichspi: const-correct POSSIBLE_OPCODES[]
......................................................................
ichspi: const-correct POSSIBLE_OPCODES[]
POSSIBLE_OPCODES[] is never modified, so mark it as read-only.
Change-Id: I217f8a9e50b9e2e9f2731adec89a46780874c754
Signed-off-by: persmule <persmule(a)hardenedlinux.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/84595
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org>
Reviewed-by: Nikolai Artemiev <nartemiev(a)google.com>
---
M ichspi.c
1 file changed, 1 insertion(+), 1 deletion(-)
Approvals:
Nikolai Artemiev: Looks good to me, approved
build bot (Jenkins): Verified
Anastasia Klimchuk: Looks good to me, approved
diff --git a/ichspi.c b/ichspi.c
index 358d9f4..d53e671 100644
--- a/ichspi.c
+++ b/ichspi.c
@@ -399,7 +399,7 @@
* It is used to reprogram the chipset OPCODE table on-the-fly if an opcode
* is needed which is currently not in the chipset OPCODE table
*/
-static OPCODE POSSIBLE_OPCODES[] = {
+static const OPCODE POSSIBLE_OPCODES[] = {
{JEDEC_BYTE_PROGRAM, SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS, 0}, // Write Byte
{JEDEC_READ, SPI_OPCODE_TYPE_READ_WITH_ADDRESS, 0}, // Read Data
{JEDEC_BE_D8, SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS, 0}, // Erase Sector
--
To view, visit https://review.coreboot.org/c/flashrom/+/84595?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: merged
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I217f8a9e50b9e2e9f2731adec89a46780874c754
Gerrit-Change-Number: 84595
Gerrit-PatchSet: 2
Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/84567?usp=email )
Change subject: ichspi: Change the opcode position for reprogramming on the fly 2->4
......................................................................
ichspi: Change the opcode position for reprogramming on the fly 2->4
The function for reprogramming on the fly was using the default
configuration O_ST_M25P as a base and the position 2 as the position
to reprogram. Position 2 corresponds to JEDEC_SE which is often useful
for chip erase (when less than whole chip needs to be erased).
This patch changes the default position to reprogram to 4, which
corresponds to JEDEC_REMS. It is used less often, but if it needs to
be used, it will be discovered missing and reprogrammed back.
For erase opcodes, there is usually several of them available. So if
one is missing, erase still can be performed with the remaining ones.
However, this hides the fact that one of available erase opcodes is
missing (it won't be reprogrammed back), and also it gives non-optimal
erase layout.
Context: https://ticket.coreboot.org/issues/556
Change-Id: I6bc855daedf0af2e8de191f23a3512de3ebc3fef
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/84567
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Nikolai Artemiev <nartemiev(a)google.com>
---
M ichspi.c
1 file changed, 1 insertion(+), 1 deletion(-)
Approvals:
Nikolai Artemiev: Looks good to me, approved
build bot (Jenkins): Verified
diff --git a/ichspi.c b/ichspi.c
index d01f2f3..358d9f4 100644
--- a/ichspi.c
+++ b/ichspi.c
@@ -661,7 +661,7 @@
else // we have an invalid case
return SPI_INVALID_LENGTH;
}
- int oppos = 2; // use original JEDEC_BE_D8 offset
+ int oppos = 4; // use the original position of JEDEC_REMS
curopcodes->opcode[oppos].opcode = opcode;
curopcodes->opcode[oppos].spi_type = spi_type;
program_opcodes(curopcodes, 0, ich_generation);
--
To view, visit https://review.coreboot.org/c/flashrom/+/84567?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: merged
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I6bc855daedf0af2e8de191f23a3512de3ebc3fef
Gerrit-Change-Number: 84567
Gerrit-PatchSet: 3
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Attention is currently required from: Aarya, Anastasia Klimchuk, Bill XIE.
Peter Marheine has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84686?usp=email )
Change subject: erase/write: Deselect all smaller blocks when large block is selected
......................................................................
Patch Set 2:
(1 comment)
File erasure_layout.c:
https://review.coreboot.org/c/flashrom/+/84686/comment/0939c99a_00ddcc2f?us… :
PS2, Line 254: We are selecting one large block instead, to send opcode once
: instead of sending many smaller once. */
typos
```suggestion
We are selecting one large block instead, so send opcode once
instead of sending many smaller ones. */
```
--
To view, visit https://review.coreboot.org/c/flashrom/+/84686?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: Icfc18d5c090b1dcb92ab157e2c139be71af59300
Gerrit-Change-Number: 84686
Gerrit-PatchSet: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Wed, 09 Oct 2024 03:59:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Aarya, Anastasia Klimchuk, Bill XIE.
Peter Marheine has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84614?usp=email )
Change subject: erasure_layout: Erase larger block only when all sub-block need erase
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS1:
> Thank you for context! This was very educational, I understand now. […]
I think minimizing the number of bytes erased ("safe") is the correct default behavior, regardless. A command-line option to instead reduce the number of erases ("quick-n-dangerous", assumed to operate faster but wear flash faster) might be nice, though.
Making that a compile-time option seems like it's easy for a user to configure it to be quick-n-dangerous then unintentionally use that mode later, whereas requiring quick-n-dangerous mode to be opt-in for each invocation (default to safe operation) should prevent any surprises. The choice could also be easily exposed to library users then, too.
--
To view, visit https://review.coreboot.org/c/flashrom/+/84614?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: I9e10749186e395da67ec80e296119f33c3f83122
Gerrit-Change-Number: 84614
Gerrit-PatchSet: 2
Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Wed, 09 Oct 2024 03:56:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Bill XIE <persmule(a)hardenedlinux.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Attention is currently required from: Aarya, Bill XIE.
Anastasia Klimchuk has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84614?usp=email )
Change subject: erasure_layout: Erase larger block only when all sub-block need erase
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS1:
> > I will never consider to exchange programming speed with chip lifetime. […]
Thank you for context! This was very educational, I understand now.
It seems like yes, can be an option. Which most of users won't touch and just use default, but still, there can be an option.
However, maybe a build-time option? as is, what we have in `meson_options.txt`
So that you can build flashrom with your preference (chip lifetime), and then run it like that.
We have recently added `delay_minimum_sleep_us` this can be an example.
The option that comes to my mind would be the threshold of how much area covered in small blocks when we decide to switch to large block instead (and you will set it 100). I think it fits naturally into the existing condition...
The biggest challenge is the naming! %) I am making my first attempt as
option name: larger_block_erase_ratio
description: The percent threshold of how much area to erase is covered by smaller-size blocks, at this percent coverage we switch to a larger block to cover the area
default can stay the same as it is now, 50, and then if decided, it can be later changed in a separate 1-line patch.
what do you think?
--
To view, visit https://review.coreboot.org/c/flashrom/+/84614?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: I9e10749186e395da67ec80e296119f33c3f83122
Gerrit-Change-Number: 84614
Gerrit-PatchSet: 2
Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Comment-Date: Tue, 08 Oct 2024 12:33:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Bill XIE <persmule(a)hardenedlinux.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Attention is currently required from: Aarya, Bill XIE.
Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84686?usp=email )
Change subject: erase/write: Deselect all smaller blocks when large block is selected
......................................................................
Patch Set 2:
(1 comment)
File erasure_layout.c:
https://review.coreboot.org/c/flashrom/+/84686/comment/746d8238_aedf7b2d?us… :
PS1, Line 242: while (true)
> I want to re-write this as recursive fn `deselect_erase_functions`
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/84686?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: Icfc18d5c090b1dcb92ab157e2c139be71af59300
Gerrit-Change-Number: 84686
Gerrit-PatchSet: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Comment-Date: Tue, 08 Oct 2024 11:01:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>