October 2022 - flashrom-gerrit

[M] Change in flashrom[master]: flashrom.c: Move count_max_decode_exceeding() to cli
by Anastasia Klimchuk (Code Review) Oct. 31, 2022

Oct. 31, 2022

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/68438 ) ( 1 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: flashrom.c: Move count_max_decode_exceeding() to cli ...................................................................... flashrom.c: Move count_max_decode_exceeding() to cli The count_max_decode_exceeding() function is only ever called within the cli_classic logic so move it there and make it static. This further cleans up the flashrom.c symbol namespace. Change-Id: If050eab7db8560676c03d5005a2b391313a0d642 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/68438 Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M cli_classic.c M flashrom.c M include/programmer.h 3 files changed, 60 insertions(+), 44 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved diff --git a/cli_classic.c b/cli_classic.c index d258cb4..b66094c 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -516,6 +516,49 @@ return ret; } +/* Returns the number of buses commonly supported by the current programmer and flash chip where the latter + * can not be completely accessed due to size/address limits of the programmer. */ +static unsigned int count_max_decode_exceedings(const struct flashctx *flash) +{ + unsigned int limitexceeded = 0; + uint32_t size = flash->chip->total_size * 1024; + enum chipbustype buses = flash->mst->buses_supported & flash->chip->bustype; + + if ((buses & BUS_PARALLEL) && (max_rom_decode.parallel < size)) { + limitexceeded++; + msg_pdbg("Chip size %u kB is bigger than supported " + "size %u kB of chipset/board/programmer " + "for %s interface, " + "probe/read/erase/write may fail. ", size / 1024, + max_rom_decode.parallel / 1024, "Parallel"); + } + if ((buses & BUS_LPC) && (max_rom_decode.lpc < size)) { + limitexceeded++; + msg_pdbg("Chip size %u kB is bigger than supported " + "size %u kB of chipset/board/programmer " + "for %s interface, " + "probe/read/erase/write may fail. ", size / 1024, + max_rom_decode.lpc / 1024, "LPC"); + } + if ((buses & BUS_FWH) && (max_rom_decode.fwh < size)) { + limitexceeded++; + msg_pdbg("Chip size %u kB is bigger than supported " + "size %u kB of chipset/board/programmer " + "for %s interface, " + "probe/read/erase/write may fail. ", size / 1024, + max_rom_decode.fwh / 1024, "FWH"); + } + if ((buses & BUS_SPI) && (max_rom_decode.spi < size)) { + limitexceeded++; + msg_pdbg("Chip size %u kB is bigger than supported " + "size %u kB of chipset/board/programmer " + "for %s interface, " + "probe/read/erase/write may fail. ", size / 1024, + max_rom_decode.spi / 1024, "SPI"); + } + return limitexceeded; +} + int main(int argc, char *argv[]) { const struct flashchip *chip = NULL; diff --git a/flashrom.c b/flashrom.c index 624c463..0f982c9 100644 --- a/flashrom.c +++ b/flashrom.c @@ -630,49 +630,6 @@ return first_len; } -/* Returns the number of buses commonly supported by the current programmer and flash chip where the latter - * can not be completely accessed due to size/address limits of the programmer. */ -unsigned int count_max_decode_exceedings(const struct flashctx *flash) -{ - unsigned int limitexceeded = 0; - uint32_t size = flash->chip->total_size * 1024; - enum chipbustype buses = flash->mst->buses_supported & flash->chip->bustype; - - if ((buses & BUS_PARALLEL) && (max_rom_decode.parallel < size)) { - limitexceeded++; - msg_pdbg("Chip size %u kB is bigger than supported " - "size %u kB of chipset/board/programmer " - "for %s interface, " - "probe/read/erase/write may fail. ", size / 1024, - max_rom_decode.parallel / 1024, "Parallel"); - } - if ((buses & BUS_LPC) && (max_rom_decode.lpc < size)) { - limitexceeded++; - msg_pdbg("Chip size %u kB is bigger than supported " - "size %u kB of chipset/board/programmer " - "for %s interface, " - "probe/read/erase/write may fail. ", size / 1024, - max_rom_decode.lpc / 1024, "LPC"); - } - if ((buses & BUS_FWH) && (max_rom_decode.fwh < size)) { - limitexceeded++; - msg_pdbg("Chip size %u kB is bigger than supported " - "size %u kB of chipset/board/programmer " - "for %s interface, " - "probe/read/erase/write may fail. ", size / 1024, - max_rom_decode.fwh / 1024, "FWH"); - } - if ((buses & BUS_SPI) && (max_rom_decode.spi < size)) { - limitexceeded++; - msg_pdbg("Chip size %u kB is bigger than supported " - "size %u kB of chipset/board/programmer " - "for %s interface, " - "probe/read/erase/write may fail. ", size / 1024, - max_rom_decode.spi / 1024, "SPI"); - } - return limitexceeded; -} - void unmap_flash(struct flashctx *flash) { if (flash->virtual_registers != (chipaddr)ERROR_PTR) { diff --git a/include/programmer.h b/include/programmer.h index 9603443..386e6ec 100644 --- a/include/programmer.h +++ b/include/programmer.h @@ -286,7 +286,6 @@ extern struct decode_sizes max_rom_decode; extern bool programmer_may_write; extern unsigned long flashbase; -unsigned int count_max_decode_exceedings(const struct flashctx *flash); char *extract_programmer_param_str(const struct programmer_cfg *cfg, const char *param_name); /* spi.c */ -- To view, visit https://review.coreboot.org/c/flashrom/+/68438 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If050eab7db8560676c03d5005a2b391313a0d642 Gerrit-Change-Number: 68438 Gerrit-PatchSet: 3 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-MessageType: merged

1 0

[XL] Change in flashrom[master]: tree/: Convert flashchip probe func ptr to enumerate
by Edward O'Callaghan (Code Review) Oct. 31, 2022

Oct. 31, 2022

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

1 0

[XL] Change in flashrom[master]: tree/: Convert flashchip write func ptr to enumerate
by Edward O'Callaghan (Code Review) Oct. 31, 2022

Oct. 31, 2022

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

1 0

[S] Change in flashrom[master]: ichspi.c: derive hsfc_fcycle from ctx in ich_start_hwseq_xfer()
by Subrata Banik (Code Review) Oct. 30, 2022

Oct. 30, 2022

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/+/68773 ) Change subject: ichspi.c: derive hsfc_fcycle from ctx in ich_start_hwseq_xfer() ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/68773 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib0ccf4b32fd1e2be2ecc3a4a4c6e397c8e901a0a Gerrit-Change-Number: 68773 Gerrit-PatchSet: 1 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: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> 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:58:06 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: ichspi.c: plumb flashctx through hwseq xfer helper
by Subrata Banik (Code Review) Oct. 30, 2022

Oct. 30, 2022

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

1 0

[S] Change in flashrom[master]: ichspi.c: plumb flashctx through hwseq xfer helper
by Subrata Banik (Code Review) Oct. 30, 2022

Oct. 30, 2022

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

1 0

[M] Change in flashrom[1.2.x]: ichspi: Unify timeouts across all SPI operations to 30s
by Felix Singer (Code Review) Oct. 30, 2022

Oct. 30, 2022

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

1 0

[M] Change in flashrom[1.1.x]: ichspi: Unify timeouts across all SPI operations to 30s
by Felix Singer (Code Review) Oct. 30, 2022

Oct. 30, 2022

Felix Singer has submitted this change. ( https://review.coreboot.org/c/flashrom/+/68690 ) 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.1.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/+/68690 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 0f1470d..6cf6b5f 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1265,20 +1265,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", @@ -1361,7 +1365,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) { @@ -1401,7 +1404,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; } @@ -1410,7 +1413,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) { @@ -1438,7 +1440,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; @@ -1451,7 +1453,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) { @@ -1480,7 +1481,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/+/68690 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: 1.1.x Gerrit-Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b Gerrit-Change-Number: 68690 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

1 0

[M] Change in flashrom[1.0.x]: ichspi: Unify timeouts across all SPI operations to 30s
by Felix Singer (Code Review) Oct. 30, 2022

Oct. 30, 2022

Felix Singer has submitted this change. ( https://review.coreboot.org/c/flashrom/+/68689 ) 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.0.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/+/68689 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 c7bda92..af201a4 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1251,20 +1251,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", @@ -1347,7 +1351,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) { @@ -1387,7 +1390,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; } @@ -1396,7 +1399,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) { @@ -1424,7 +1426,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; @@ -1437,7 +1439,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) { @@ -1466,7 +1467,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/+/68689 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: 1.0.x Gerrit-Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b Gerrit-Change-Number: 68689 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

1 0

[M] Change in flashrom[1.1.x]: ichspi: Unify timeouts across all SPI operations to 30s
by Michael Niewöhner (Code Review) Oct. 30, 2022

Oct. 30, 2022

Attention is currently required from: Felix Singer, Nico Huber, Angel Pons. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68690 ) Change subject: ichspi: Unify timeouts across all SPI operations to 30s ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/68690 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: 1.1.x Gerrit-Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b Gerrit-Change-Number: 68690 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> 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-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sun, 30 Oct 2022 08:27:42 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0