April 2022 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: sb600spi.c: Add rev 0x71 as promontory
by Robert Zieba (Code Review) 06 Apr '22

06 Apr '22

Attention is currently required from: Raul Rangel, Jon Murphy, Edward O'Callaghan, Karthik Ramasubramanian, Felix Held. Robert Zieba has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63423 ) Change subject: sb600spi.c: Add rev 0x71 as promontory ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/63423 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2408959fbf1c105508f0a12f38418c9606280ab9 Gerrit-Change-Number: 63423 Gerrit-PatchSet: 1 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jon Murphy <jpmurphy(a)google.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Robert Zieba <robertzieba(a)google.com> Gerrit-CC: Karthikeyan Ramasubramanian <kramasub(a)chromium.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Jon Murphy <jpmurphy(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Wed, 06 Apr 2022 22:27:23 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: sb600spi.c: Add rev 0x71 as promontory
by Karthik Ramasubramanian (Code Review) 06 Apr '22

06 Apr '22

Karthik Ramasubramanian has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/63423 ) Change subject: sb600spi.c: Add rev 0x71 as promontory ...................................................................... sb600spi.c: Add rev 0x71 as promontory Sabrina SoC uses SMBUS revision code 0x71 which behaves exactly as the promontory chip. Hence add 0x71 as promontory. BUG=b:228238107 TEST=Build and deploy flashrom in Skyrim. Use flashrom to read and write from SPI ROM successfully. Change-Id: I2408959fbf1c105508f0a12f38418c9606280ab9 Signed-off-by: Karthikeyan Ramasubramanian <kramasub(a)google.com> --- M sb600spi.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/23/63423/1 diff --git a/sb600spi.c b/sb600spi.c index b1e140b..70d7c65 100644 --- a/sb600spi.c +++ b/sb600spi.c @@ -132,7 +132,7 @@ * The revisions I have found by searching various lspci * outputs are as follows: 0x4b, 0x59 & 0x61. */ - } else if (rev == 0x4b || rev == 0x51 || rev == 0x59 || rev == 0x61) { + } else if (rev == 0x4b || rev == 0x51 || rev == 0x59 || rev == 0x61 || rev == 0x71) { msg_pdbg("Promontory (rev 0x%02x) detected.\n", rev); return CHIPSET_PROMONTORY; } else { -- To view, visit https://review.coreboot.org/c/flashrom/+/63423 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2408959fbf1c105508f0a12f38418c9606280ab9 Gerrit-Change-Number: 63423 Gerrit-PatchSet: 1 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-CC: Karthikeyan Ramasubramanian <kramasub(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: sb600spi.c: Use SPI100 bit mappings
by Karthik Ramasubramanian (Code Review) 06 Apr '22

06 Apr '22

Karthik Ramasubramanian has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/63422 ) Change subject: sb600spi.c: Use SPI100 bit mappings ...................................................................... sb600spi.c: Use SPI100 bit mappings Flashrom has been using legacy spi100 bit mappings when programming the opcode and triggering the command. These legacy bit mappings are deprecated in upcoming generation of AMD SoCs. Stop using the legacy spi100 registers and use the correct ones. BUG=b:228238107 TEST=Build and deploy flashrom in Skyrim. Ensure that flashrom is able to detect the SPI ROM chip, read from it and write to it successfully. Change-Id: If42130757331f4294b5a42c848557d3287f24fc3 Signed-off-by: Karthikeyan Ramasubramanian <kramasub(a)google.com> --- M sb600spi.c 1 file changed, 16 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/22/63422/1 diff --git a/sb600spi.c b/sb600spi.c index 6f8cc40..b1e140b 100644 --- a/sb600spi.c +++ b/sb600spi.c @@ -53,6 +53,10 @@ #define FIFO_SIZE_OLD 8 #define FIFO_SIZE_YANGTZE 71 +#define SPI100_CMD_CODE_REG 0x45 +#define SPI100_CMD_TRIGGER_REG 0x47 +#define SPI100_EXECUTE_CMD (1 << 7) + struct sb600spi_data { struct flashctx *flash; uint8_t *spibar; @@ -200,6 +204,16 @@ msg_pspew("done\n"); } +static void execute_spi100_command(uint8_t *sb600_spibar) +{ + msg_pspew("Executing... "); + mmio_writeb(mmio_readb(sb600_spibar + SPI100_CMD_TRIGGER_REG) | SPI100_EXECUTE_CMD, + sb600_spibar + SPI100_CMD_TRIGGER_REG); + while (mmio_readb(sb600_spibar + SPI100_CMD_TRIGGER_REG) & SPI100_CMD_TRIGGER_REG) + ; + msg_pspew("done\n"); +} + static int sb600_spi_send_command(const struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, @@ -299,7 +313,7 @@ unsigned char cmd = *writearr++; writecnt--; msg_pspew("%s, cmd=0x%02x, writecnt=%d, readcnt=%d\n", __func__, cmd, writecnt, readcnt); - mmio_writeb(cmd, sb600_spibar + 0); + mmio_writeb(cmd, sb600_spibar + SPI100_CMD_CODE_REG); int ret = check_readwritecnt(flash, writecnt, readcnt); if (ret != 0) @@ -317,7 +331,7 @@ } msg_pspew("\n"); - execute_command(sb600_spibar); + execute_spi100_command(sb600_spibar); msg_pspew("Reading buffer: "); for (count = 0; count < readcnt; count++) { -- To view, visit https://review.coreboot.org/c/flashrom/+/63422 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If42130757331f4294b5a42c848557d3287f24fc3 Gerrit-Change-Number: 63422 Gerrit-PatchSet: 1 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-CC: Karthikeyan Ramasubramanian <kramasub(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: ichspi: Define `Write Enable Type (WET)` register under HSFC
by Subrata Banik (Code Review) 06 Apr '22

06 Apr '22

Attention is currently required from: Subrata Banik, Nico Huber, Angel Pons. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62888 ) Change subject: ichspi: Define `Write Enable Type (WET)` register under HSFC ...................................................................... Patch Set 6: (1 comment) File ichspi.c: https://review.coreboot.org/c/flashrom/+/62888/comment/342c320b_90b7ec42 PS6, Line 91: /* New HSFC Control bit */ > If you have doubts that the bit was introduced with PCH100, you can always > test the hardware. Do we have any action item here regarding the open comment or we are good here ? Ref to Intel doc 355845 section 22.1.3. Bit 7:3 are reserved, so, WET bit does exist there. Although there is no use of this bit hence, I will keep this CL aside. -- To view, visit https://review.coreboot.org/c/flashrom/+/62888 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id32cb4ccb83dd08e9b0b1ab30cc8e041dd059f5f Gerrit-Change-Number: 62888 Gerrit-PatchSet: 6 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 06 Apr 2022 20:03:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Unify timeouts across all SPI operations to 30s
by Subrata Banik (Code Review) 06 Apr '22

06 Apr '22

Attention is currently required from: Subrata Banik, Nico Huber, Paul Menzel, Edward O'Callaghan, Anastasia Klimchuk. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62867 ) Change subject: ichspi: Unify timeouts across all SPI operations to 30s ...................................................................... Patch Set 12: (1 comment) File ichspi.c: https://review.coreboot.org/c/flashrom/+/62867/comment/8c56ca75_5622db86 PS8, Line 68: */ > > > However, I think Documentation/ or the commit message is a better place. > > > > Can this comment go into commit message as it is? Appending to existing 2 paragraphs, between "Document" and "BUG"? I know you mentioned potentially shrinking to 2-3 sentences, but maybe we can keep it long? It was useful for me to read long information, it would be harder to understand short version of 2-3 sentences. > > > > Another thing you mentioned: > > > Also, first calculating 2.065s and then concluding 30s probably raises more questions than are answered. > > > > I found where 30s comes from the last paragraph: it says "defines 5 master sections ... The worst case operation ... might take ~5 seconds" > > so that's 5*5=25 and taking a bit larger value 30 just in case. I hope my understanding is correct :) > > > > Do you think it's fine to move this comment to commit message? And then, as you said, one line comment in the code? > > Thanks all for your suggestion. I have moved the detailed description into the commit section and kept just one liner to define the 30sec timeout. marking resolved, please feel free to open if you think that the comment is not addressed. -- To view, visit https://review.coreboot.org/c/flashrom/+/62867 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b Gerrit-Change-Number: 62867 Gerrit-PatchSet: 12 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 06 Apr 2022 19:43:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Unify timeouts across all SPI operations to 30s
by Subrata Banik (Code Review) 06 Apr '22

06 Apr '22

Attention is currently required from: Subrata Banik, Nico Huber, Paul Menzel, Edward O'Callaghan, Anastasia Klimchuk. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/62867 to look at the new patch set (#12). Change subject: ichspi: Unify timeouts across all SPI operations to 30s ...................................................................... ichspi: Unify timeouts across all SPI operations to 30s `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 2 SPI master like, Host CPU and CSE, the Timeout Calculation for SPI Write Operation would look like as below: Maximum Time consumed by a SPI Operation = 2 seconds (for SPI Erase Operation as per Winbond Data Sheet) Worst Case Operational Delay = (2 seconds + 50 milliseconds) = 2 seconds 50 milliseconds Timeout Recommendation for Hardware Seq Operation = (2 seconds 50 milliseconds) * (2 - 1) + 15 milliseconds (for SPI Write Operation as per Winbond Data Sheet) = 2 seconds 65 milliseconds The timeout value for SPI Hardware Sequencing is defined here based on the maximum number of master with SPI flash access and worst case SPI operation time. For example: SPI programming guide on PCH 600 series defines 5 master sections as HOST CPU/BIOS, CSE, GBE, EC and one reserved. A rough calculation for the worst case operation a.k.a SPI erase might take ~5 seconds, hence, the safest timeout value might be (5 * ~5sec) + 5sec (keeping some buffer) = 30 seconds. BUG=b:223630977 TEST=Able to perform read/write/erase operation on PCH 600 series chipset (board name: Brya). Signed-off-by: Subrata Banik <subratabanik(a)google.com> Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b --- M ichspi.c 1 file changed, 12 insertions(+), 12 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/67/62867/12 -- To view, visit https://review.coreboot.org/c/flashrom/+/62867 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b Gerrit-Change-Number: 62867 Gerrit-PatchSet: 12 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: ichspi: Unify timeouts across all SPI operations to 30s
by Subrata Banik (Code Review) 06 Apr '22

06 Apr '22

Attention is currently required from: Subrata Banik, Nico Huber, Paul Menzel, Edward O'Callaghan, Anastasia Klimchuk. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/62867 to look at the new patch set (#11). Change subject: ichspi: Unify timeouts across all SPI operations to 30s ...................................................................... ichspi: Unify timeouts across all SPI operations to 30s `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 2 SPI master like, Host CPU and CSE, the Timeout Calculation for SPI Write Operation would look like as below: Maximum Time consumed by a SPI Operation = 2 seconds (for SPI Erase Operation as per Winbond Data Sheet) Worst Case Operational Delay = (2 seconds + 50 milliseconds) = 2 seconds 50 milliseconds Timeout Recommendation for Hardware Seq Operation = (2 seconds 50 milliseconds) * (2 - 1) + 15 milliseconds (for SPI Write Operation as per Winbond Data Sheet) = 2 seconds 65 milliseconds The timeout value for SPI Hardware Sequencing is defined here based on the maximum number of master with SPI flash access and worst case SPI operation time. For example: SPI programming guide on PCH 600 series defines 5 master sections as HOST CPU/BIOS, CSE, GBE, EC and one reserved. A rough calculation for the worst case operation a.k.a SPI erase might take ~5 seconds, hence, the safest timeout value might be (5 * ~5sec) + 5sec (keeping some buffer) = 30 seconds. BUG=b:223630977 TEST=Able to perform read/write/erase operation on PCH 600 series chipset (board name: Brya). Signed-off-by: Subrata Banik <subratabanik(a)google.com> Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b --- M ichspi.c 1 file changed, 12 insertions(+), 12 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/67/62867/11 -- To view, visit https://review.coreboot.org/c/flashrom/+/62867 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b Gerrit-Change-Number: 62867 Gerrit-PatchSet: 11 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: ichspi: Unify timeouts across all SPI operations to 30s
by Subrata Banik (Code Review) 06 Apr '22

06 Apr '22

Attention is currently required from: Subrata Banik, Nico Huber, Paul Menzel, Edward O'Callaghan, Anastasia Klimchuk. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/62867 to look at the new patch set (#10). Change subject: ichspi: Unify timeouts across all SPI operations to 30s ...................................................................... ichspi: Unify timeouts across all SPI operations to 30s `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 2 SPI master like, Host CPU and CSE, the Timeout Calculation for SPI Write Operation would look like as below: Maximum Time consumed by a SPI Operation = 2 seconds (for SPI Erase Operation as per Winbond Data Sheet) Worst Case Operational Delay = (2 seconds + 50 milliseconds) = 2 seconds 50 milliseconds Timeout Recommendation for Hardware Seq Operation = (2 seconds 50 milliseconds) * (2 - 1) + 15 milliseconds (for SPI Write Operation as per Winbond Data Sheet) = 2 seconds 65 milliseconds The timeout value for SPI Hardware Sequencing is defined here based on the maximum number of master with SPI flash access and worst case SPI operation time. For example: SPI programming guide on PCH 600 series defines 5 master sections as HOST CPU/BIOS, CSE, GBE, EC and one reserved. A rough calculation for the worst case operation a.k.a SPI erase might take ~5 seconds, hence, the safest timeout value might be (5 * ~5sec) + 5sec (keeping some buffer) = 30 seconds. BUG=b:223630977 TEST=Able to perform read/write/erase operation on PCH 600 series chipset (board name: Brya). Signed-off-by: Subrata Banik <subratabanik(a)google.com> Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b --- M ichspi.c 1 file changed, 9 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/67/62867/10 -- To view, visit https://review.coreboot.org/c/flashrom/+/62867 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b Gerrit-Change-Number: 62867 Gerrit-PatchSet: 10 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: ichspi: Unify timeouts across all SPI operations to 30s
by Subrata Banik (Code Review) 06 Apr '22

06 Apr '22

Attention is currently required from: Subrata Banik, Nico Huber, Paul Menzel, Edward O'Callaghan, Anastasia Klimchuk. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62867 ) Change subject: ichspi: Unify timeouts across all SPI operations to 30s ...................................................................... Patch Set 9: (1 comment) File ichspi.c: https://review.coreboot.org/c/flashrom/+/62867/comment/996b29b6_dbff3f30 PS8, Line 68: */ > > However, I think Documentation/ or the commit message is a better place. > > Can this comment go into commit message as it is? Appending to existing 2 paragraphs, between "Document" and "BUG"? I know you mentioned potentially shrinking to 2-3 sentences, but maybe we can keep it long? It was useful for me to read long information, it would be harder to understand short version of 2-3 sentences. > > Another thing you mentioned: > > Also, first calculating 2.065s and then concluding 30s probably raises more questions than are answered. > > I found where 30s comes from the last paragraph: it says "defines 5 master sections ... The worst case operation ... might take ~5 seconds" > so that's 5*5=25 and taking a bit larger value 30 just in case. I hope my understanding is correct :) > > Do you think it's fine to move this comment to commit message? And then, as you said, one line comment in the code? Thanks all for your suggestion. I have moved the detailed description into the commit section and kept just one liner to define the 30sec timeout. -- To view, visit https://review.coreboot.org/c/flashrom/+/62867 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b Gerrit-Change-Number: 62867 Gerrit-PatchSet: 9 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 06 Apr 2022 19:34:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Unify timeouts across all SPI operations to 30s
by Subrata Banik (Code Review) 06 Apr '22

06 Apr '22

Attention is currently required from: Subrata Banik, Nico Huber, Paul Menzel, Edward O'Callaghan. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/62867 to look at the new patch set (#9). Change subject: ichspi: Unify timeouts across all SPI operations to 30s ...................................................................... ichspi: Unify timeouts across all SPI operations to 30s `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 2 SPI master like, Host CPU and CSE, the Timeout Calculation for SPI Write Operation would look like as below: Maximum Time consumed by a SPI Operation = 2 seconds (for SPI Erase Operation as per Winbond Data Sheet) Worst Case Operational Delay = (2 seconds + 50 milliseconds) = 2 seconds 50 milliseconds Timeout Recommendation for Hardware Seq Operation = (2 seconds 50 milliseconds) * (2 - 1) + 15 milliseconds (for SPI Write Operation as per Winbond Data Sheet) = 2 seconds 65 milliseconds The timeout value for SPI Hardware Sequencing is defined here based on the maximum number of master with SPI flash access and worst case SPI operation time. For example: SPI programming guide on PCH 600 series defines 5 master sections as HOST CPU/BIOS, CSE, GBE, EC and one reserved. A rough calculation for the worst case operation a.k.a SPI erase might take ~5 seconds, hence, the safest timeout value might be (5 * ~5sec) + 5sec (keeping some buffer) = 30 seconds. BUG=b:223630977 TEST=Able to perform read/write/erase operation on PCH 600 series chipset (board name: Brya). Signed-off-by: Subrata Banik <subratabanik(a)google.com> Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b --- M ichspi.c 1 file changed, 9 insertions(+), 10 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/67/62867/9 -- To view, visit https://review.coreboot.org/c/flashrom/+/62867 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b Gerrit-Change-Number: 62867 Gerrit-PatchSet: 9 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2022