Attention is currently required from: Felix Singer, Thomas Heijligen, Alexander Goncharov.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68247 )
Change subject: util: add bash completion script
......................................................................
Patch Set 7:
(1 comment)
Patchset:
PS7:
> The patch is merge-ready :)
I just looked, there are unresolved comments.
Especially one of them on the topic of "getting the path from a pkgconfig variable" , it does look like waiting for reply and/or action? am I right or not?
The second one about vertical list is less important.
--
To view, visit https://review.coreboot.org/c/flashrom/+/68247
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ie68bc91c3cea4de2ffdbeffd07e48edd8d5590e1
Gerrit-Change-Number: 68247
Gerrit-PatchSet: 7
Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Comment-Date: Mon, 31 Oct 2022 05:33:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Edward O'Callaghan, Arthur Heymans, Anastasia Klimchuk.
Hello Felix Singer, build bot (Jenkins), Angel Pons, Anastasia Klimchuk, Nikolai Artemiev,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/66781
to look at the new patch set (#12).
Change subject: tree/: Convert flashchip probe func ptr to enumerate
......................................................................
tree/: Convert flashchip probe func ptr to enumerate
This forges the way for flashchips.c to be pure declarative
data and lookup functions for dispatch to be pure. This
means that the flashchips data could be extracted out to
be agnostic data of the flashrom code and algorithms.
squash in,
flashchip.c: probe_jedec_29gl
flashchip.c: probe_82802ab
flashchip.c: probe_opaque
flashchip.c: probe_jedec
flashchip.c: probe_spi_res2
flashchip.c: probe_spi_res1
flashchip.c: probe_spi_rems
flashchip.c: probe_spi_at25f
flashchip.c: probe_spi_at45db
flashchip.c: probe_spi_st95
flashchip.c: probe_spi_sfdp
flashchip.c: probe_spi_rdid{4}
flashchip.c: probe_w29ee011
flashchip.c: probe_en29lv640b
flashchip.c: edi_probe_kb9012
flashchip.c: probe_spi_big_spansion
Change-Id: I00aaab9c83f305cd47e78c36d9c2867f2b73c396
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M flashchips.c
M flashrom.c
M include/flash.h
M tests/spi25.c
4 files changed, 684 insertions(+), 596 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/81/66781/12
--
To view, visit https://review.coreboot.org/c/flashrom/+/66781
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I00aaab9c83f305cd47e78c36d9c2867f2b73c396
Gerrit-Change-Number: 66781
Gerrit-PatchSet: 12
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Singer, Edward O'Callaghan, Angel Pons.
Hello Felix Singer, build bot (Jenkins), Angel Pons, Anastasia Klimchuk, Nikolai Artemiev,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/66782
to look at the new patch set (#12).
Change subject: tree/: Convert flashchip write func ptr to enumerate
......................................................................
tree/: Convert flashchip write func ptr to enumerate
This forges the way for flashchips.c to be pure declarative
data and lookup functions for dispatch to be pure. This
means that the flashchips data could be extracted out to
be agnostic data of the flashrom code and algorithms.
Squash in,
flashchips.c: convert spi_chip_write_256()
flashchips.c: convert write_jedec_1()
flashchips.c: convert write_jedec()
flashchips.c: convert write_opaque()
flashchips.c: convert spi_chip_write_1()
flashchips.c: convert spi_aai_write()
flashchips.c: convert edi_chip_write()
flashchips.c: convert write_en29lv640b()
flashchips.c: convert write_82802ab()
flashchips.c: convert spi_write_at45db()
flashchips.c: convert write_28sf040()
flashchips.c: convert NULL write
Change-Id: I80149de169464b204fb09f1424a86fc645b740fd
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M flashchips.c
M flashrom.c
M include/flash.h
M sfdp.c
M tests/chip.c
M tests/chip_wp.c
M tests/spi25.c
7 files changed, 684 insertions(+), 599 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/82/66782/12
--
To view, visit https://review.coreboot.org/c/flashrom/+/66782
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I80149de169464b204fb09f1424a86fc645b740fd
Gerrit-Change-Number: 66782
Gerrit-PatchSet: 12
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68772 )
Change subject: ichspi.c: plumb flashctx through hwseq xfer helper
......................................................................
Patch Set 2:
(1 comment)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/68772/comment/67dbf97f_e237efce
PS2, Line 1348: const struct flashctx *flash
> Wondering if we are planning to use it in future ? in current code, i don't see any usage of this ne […]
Looks like https://review.coreboot.org/c/flashrom/+/68773/1/ichspi.c#1353 is the purpose
--
To view, visit https://review.coreboot.org/c/flashrom/+/68772
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I67b5aa6350930d912e5036473ac3e792debac0bd
Gerrit-Change-Number: 68772
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Subrata Banik <subratabanik(a)google.com>
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-Comment-Date: Sun, 30 Oct 2022 17:57:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68772 )
Change subject: ichspi.c: plumb flashctx through hwseq xfer helper
......................................................................
Patch Set 2:
(1 comment)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/68772/comment/4d477e6a_ac90dd46
PS2, Line 1348: const struct flashctx *flash
Wondering if we are planning to use it in future ? in current code, i don't see any usage of this new argument ?
--
To view, visit https://review.coreboot.org/c/flashrom/+/68772
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I67b5aa6350930d912e5036473ac3e792debac0bd
Gerrit-Change-Number: 68772
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Subrata Banik <subratabanik(a)google.com>
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-Comment-Date: Sun, 30 Oct 2022 17:56:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Felix Singer has submitted this change. ( https://review.coreboot.org/c/flashrom/+/68691 )
Change subject: ichspi: Unify timeouts across all SPI operations to 30s
......................................................................
ichspi: Unify timeouts across all SPI operations to 30s
Note: This patch was backported from the master branch and it was
modified so that it can be applied on the 1.2.x branch.
`ich_hwseq_wait_for_cycle_complete()` drops taking `timeout` as argument
in favor of a fixed timeout of `30 seconds` for any given SPI operation
as recommended by the SPI programming guide.
Document: Alder Lake-P Client Platform SPI Programming Guide
Rev 1.30 (supporting document for multi-master accessing the
SPI Flash device.)
Refer to below section to understand the problem in more detail and SPI
operation timeout recommendation from Intel in multi-master
scenarios.
On Intel Chipsets that support multi-mastering access of the SPI flash
may run into a timeout failure when the operation initiated from a
single master just follows the SPI operational timeout recommendation
as per the vendor datasheet (example: winbond spiflash W25Q256JV-DTR
specification, table 9.7).
In the multi-master SPI accessing scenario using hardware sequencing
operation, it's impossible to know the actual status of the SPI bus
prior to individual master starting the operation (SPI Cycle In Progress
a.k.a SCIP bit represents the status of SPI operation on individual
master).
Thus, any SPI operation triggered in multi-master environment might need
to account a worst case scenario where the most time consuming operation
might have occupied the SPI bus from a master and an operation initiated
by another master just timed out.
Here is the timeout calculation for any hardware sequencing operation:
Worst Case Operational Delay =
(Maximum Time consumed by a SPI operation + Any marginal
adjustment)
Timeout Recommendation for Hardware Sequencing Operation =
((Worst Case Operational Delay) * (#No. Of SPI Master - 1) +
Current Operational latency)
Assume, on Intel platform with 6 SPI master like, Host CPU, CSE, EC,
GbE and other reserved etc, hence, the Timeout Calculation for SPI
erase Operation would look like as below:
Maximum Time consumed by a SPI Operation = 5 seconds
Worst Case Operational Delay = 5 seconds
Timeout Recommendation for Hardware Seq Operation =
5 seconds * (6 - 1) + 5 seconds = 30 seconds
BUG=b:223630977
TEST=Able to perform read/write/erase operation on PCH 600 series
chipset (board name: Brya).
Original-Signed-off-by: Subrata Banik <subratabanik(a)google.com>
Original-Reviewed-on: https://review.coreboot.org/c/flashrom/+/62867
Original-Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Original-Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org>
Original-Reviewed-by: Arthur Heymans <arthur(a)aheymans.xyz>
Original-Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org>
Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b
Signed-off-by: Felix Singer <felixsinger(a)posteo.net>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/68691
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Michael Niewöhner <foss(a)mniewoehner.de>
---
M ichspi.c
1 file changed, 87 insertions(+), 11 deletions(-)
Approvals:
build bot (Jenkins): Verified
Michael Niewöhner: Looks good to me, approved
diff --git a/ichspi.c b/ichspi.c
index 12ee126..725bfb3 100644
--- a/ichspi.c
+++ b/ichspi.c
@@ -1282,20 +1282,24 @@
Resets all error flags in HSFS.
Returns 0 if the cycle completes successfully without errors within
timeout us, 1 on errors. */
-static int ich_hwseq_wait_for_cycle_complete(unsigned int timeout,
- unsigned int len)
+static int ich_hwseq_wait_for_cycle_complete(unsigned int len)
{
+ /*
+ * The SPI bus may be busy due to performing operations from other masters, hence
+ * introduce the long timeout of 30s to cover the worst case scenarios as well.
+ */
+ unsigned int timeout_us = 30 * 1000 * 1000;
uint16_t hsfs;
uint32_t addr;
- timeout /= 8; /* scale timeout duration to counter */
+ timeout_us /= 8; /* scale timeout duration to counter */
while ((((hsfs = REGREAD16(ICH9_REG_HSFS)) &
(HSFS_FDONE | HSFS_FCERR)) == 0) &&
- --timeout) {
+ --timeout_us) {
programmer_delay(8);
}
REGWRITE16(ICH9_REG_HSFS, REGREAD16(ICH9_REG_HSFS));
- if (!timeout) {
+ if (!timeout_us) {
addr = REGREAD32(ICH9_REG_FADDR) & hwseq_data.addr_mask;
msg_perr("Timeout error between offset 0x%08x and "
"0x%08x (= 0x%08x + %d)!\n",
@@ -1378,7 +1382,6 @@
{
uint32_t erase_block;
uint16_t hsfc;
- uint32_t timeout = 5000 * 1000; /* 5 s for max 64 kB */
erase_block = ich_hwseq_get_erase_block_size(addr);
if (len != erase_block) {
@@ -1418,7 +1421,7 @@
prettyprint_ich9_reg_hsfc(hsfc);
REGWRITE16(ICH9_REG_HSFC, hsfc);
- if (ich_hwseq_wait_for_cycle_complete(timeout, len))
+ if (ich_hwseq_wait_for_cycle_complete(len))
return -1;
return 0;
}
@@ -1427,7 +1430,6 @@
unsigned int addr, unsigned int len)
{
uint16_t hsfc;
- uint16_t timeout = 100 * 60;
uint8_t block_len;
if (addr + len > flash->chip->total_size * 1024) {
@@ -1455,7 +1457,7 @@
hsfc |= HSFC_FGO; /* start */
REGWRITE16(ICH9_REG_HSFC, hsfc);
- if (ich_hwseq_wait_for_cycle_complete(timeout, block_len))
+ if (ich_hwseq_wait_for_cycle_complete(block_len))
return 1;
ich_read_data(buf, block_len, ICH9_REG_FDATA0);
addr += block_len;
@@ -1468,7 +1470,6 @@
static int ich_hwseq_write(struct flashctx *flash, const uint8_t *buf, unsigned int addr, unsigned int len)
{
uint16_t hsfc;
- uint16_t timeout = 100 * 60;
uint8_t block_len;
if (addr + len > flash->chip->total_size * 1024) {
@@ -1497,7 +1498,7 @@
hsfc |= HSFC_FGO; /* start */
REGWRITE16(ICH9_REG_HSFC, hsfc);
- if (ich_hwseq_wait_for_cycle_complete(timeout, block_len))
+ if (ich_hwseq_wait_for_cycle_complete(block_len))
return -1;
addr += block_len;
buf += block_len;
--
To view, visit https://review.coreboot.org/c/flashrom/+/68691
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: 1.2.x
Gerrit-Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b
Gerrit-Change-Number: 68691
Gerrit-PatchSet: 2
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Subrata Banik <subratabanik(a)google.com>
Gerrit-MessageType: merged