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

Change in flashrom[master]: ichspi: Unify timeout (5sec) across all SPI operations
by Subrata Banik (Code Review) 16 Mar '22

16 Mar '22

Attention is currently required from: Nico Huber, Paul Menzel, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62867 ) Change subject: ichspi: Unify timeout (5sec) across all SPI operations ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/62867/comment/8cc49c6f_81522a02 PS1, Line 7: Unified > Unify Ack -- 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: 2 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-CC: Paul Menzel <paulepanter(a)mailbox.org> 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 16 Mar 2022 16:20:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Unify timeout (5sec) across all SPI operations
by Subrata Banik (Code Review) 16 Mar '22

16 Mar '22

Attention is currently required from: Nico Huber, Subrata Banik, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Hello 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 (#2). Change subject: ichspi: Unify timeout (5sec) across all SPI operations ...................................................................... ichspi: Unify timeout (5sec) across all SPI operations This patch removes taking `timeout` argument for different operations using `ich_hwseq_wait_for_cycle_complete()`. Make use of 5sec unified timeout for all SPI operations. Document Number: TBD (supporting document for multi-master accessing the SPI Flash device is awaited from Intel.) BUG=b:223630977 TEST=Able to perform read/write/erase/status operation on brya. Signed-off-by: Subrata Banik <subratabanik(a)google.com> Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b --- M ichspi.c 1 file changed, 37 insertions(+), 7 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/67/62867/2 -- 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: 2 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-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: 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-MessageType: newpatchset

1 0

Change in flashrom[master]: ichspi: Unified timeout (5sec) across all SPI operations
by Paul Menzel (Code Review) 16 Mar '22

16 Mar '22

Attention is currently required from: Nico Huber, Subrata Banik, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62867 ) Change subject: ichspi: Unified timeout (5sec) across all SPI operations ...................................................................... Patch Set 1: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/62867/comment/5aced51b_7ded4cfd PS1, Line 7: Unified Unify -- 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: 1 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-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: 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: Wed, 16 Mar 2022 16:07:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Add support for 64KB SPI erase operation
by Subrata Banik (Code Review) 16 Mar '22

16 Mar '22

Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/62870 ) Change subject: ichspi: Add support for 64KB SPI erase operation ...................................................................... ichspi: Add support for 64KB SPI erase operation This patch add support for SPI HW sequencing 64KB erase operation based on erase_block size and `only_4k` flag is not set. BUG=b:223630977 TEST=Able to perform SPI erase operation on brya. Signed-off-by: Subrata Banik <subratabanik(a)google.com> Change-Id: I534471dc8eeeed66e5cbc44baadcd2448d21d34d --- M ichspi.c 1 file changed, 6 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/70/62870/1 diff --git a/ichspi.c b/ichspi.c index d0692d0..fe2f78f 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1451,7 +1451,7 @@ static int ich_hwseq_block_erase(struct flashctx *flash, unsigned int addr, unsigned int len) { - uint32_t erase_block; + uint32_t erase_block, erase_cycle; erase_block = ich_hwseq_get_erase_block_size(addr); if (len != erase_block) { @@ -1477,7 +1477,11 @@ } msg_pdbg("Erasing %d bytes starting at 0x%06x.\n", len, addr); - if (ich_exec_sync_hwseq_xfer(HSFC_CYCLE_4K_ERASE, addr, 0)) + if (erase_block == 64 * KiB) + erase_cycle = HSFC_CYCLE_64K_ERASE; + else + erase_cycle = HSFC_CYCLE_4K_ERASE; + if (ich_exec_sync_hwseq_xfer(erase_cycle, addr, 0)) return -1; return 0; -- To view, visit https://review.coreboot.org/c/flashrom/+/62870 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I534471dc8eeeed66e5cbc44baadcd2448d21d34d Gerrit-Change-Number: 62870 Gerrit-PatchSet: 1 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: ichspi: Refactor Flashrom HW Sequencing Operation Part II
by Subrata Banik (Code Review) 16 Mar '22

16 Mar '22

Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/62869 ) Change subject: ichspi: Refactor Flashrom HW Sequencing Operation Part II ...................................................................... ichspi: Refactor Flashrom HW Sequencing Operation Part II List of changes: 1. Add a unified `execute SPI flash transfer` function that does: - Check the SCIP bit prior initiate new operation. - Start the transfer by setting address and length for transfer, finally set FGO bit. - Wait for the transaction to get completed/failed/timed out. 2. All HW Sequencing SPI operation uses `execute SPI flash transfer` function BUG=b:223630977 TEST=Able to perform flash read/write/erase operation on brya without any failure. Signed-off-by: Subrata Banik <subratabanik(a)google.com> Change-Id: Ic9fd50841449e02f476a8834f4642d6ecad36dc3 --- M ichspi.c 1 file changed, 41 insertions(+), 52 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/69/62869/1 diff --git a/ichspi.c b/ichspi.c index 3aaf751..d0692d0 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1355,6 +1355,44 @@ return 0; } +/* Fire up a transfer using the hardware sequencer. */ +static void ich_start_hwseq_xfer(uint32_t hsfc_cycle, uint32_t flash_addr, size_t len) +{ + /* make sure FDONE, FCERR, AEL are cleared by writing 1 to them */ + REGWRITE16(ICH9_REG_HSFS, REGREAD16(ICH9_REG_HSFS)); + + /* Set up transaction parameters. */ + uint16_t hsfc = 0x00; /* Make sure all bits get cleared. */ + hsfc |= hsfc_cycle & hwseq_data.hsfc_fcycle; + hsfc |= HSFC_FDBC(len - 1); + + ich_hwseq_set_addr(flash_addr); + hsfc |= HSFC_FGO; /* start */ + prettyprint_ich9_reg_hsfc(hsfc, ich_generation); + REGWRITE16(ICH9_REG_HSFC, hsfc); +} + +static int ich_wait_for_hwseq_spi_cycle_complete(void) +{ + if ((REGREAD16(ICH9_REG_HSFS) & HSFS_SCIP) == 1) { + msg_perr("Error: SCIP bit is unexpectedly set.\n"); + return 1; + } + return 0; +} + +/* Execute SPI flash transfer */ +static int ich_exec_sync_hwseq_xfer(uint32_t hsfc_cycle, uint32_t flash_addr, + size_t len) +{ + if (ich_wait_for_hwseq_spi_cycle_complete()) { + msg_perr("SPI Transaction Timeout due to previous operation in process!\n"); + return 1; + } + + ich_start_hwseq_xfer(hsfc_cycle, flash_addr, len); + return ich_hwseq_wait_for_cycle_complete(len); +} static int ich_hwseq_probe(struct flashctx *flash) { uint32_t total_size, boundary; @@ -1414,7 +1452,6 @@ unsigned int len) { uint32_t erase_block; - uint16_t hsfc; erase_block = ich_hwseq_get_erase_block_size(addr); if (len != erase_block) { @@ -1440,33 +1477,15 @@ } msg_pdbg("Erasing %d bytes starting at 0x%06x.\n", len, addr); - ich_hwseq_set_addr(addr); - - /* make sure FDONE, FCERR, AEL are cleared by writing 1 to them */ - REGWRITE16(ICH9_REG_HSFS, REGREAD16(ICH9_REG_HSFS)); - - if (REGREAD8(ICH9_REG_HSFS) & HSFS_SCIP) { - msg_perr("Error: SCIP bit is unexpectedly set.\n"); + if (ich_exec_sync_hwseq_xfer(HSFC_CYCLE_4K_ERASE, addr, 0)) return -1; - } - hsfc = REGREAD16(ICH9_REG_HSFC); - hsfc &= ~hwseq_data.hsfc_fcycle; /* clear operation */ - hsfc |= HSFC_CYCLE_4K_ERASE; /* set erase operation */ - hsfc |= HSFC_FGO; /* start */ - msg_pdbg("HSFC used for block erasing: "); - prettyprint_ich9_reg_hsfc(hsfc, ich_generation); - REGWRITE16(ICH9_REG_HSFC, hsfc); - - if (ich_hwseq_wait_for_cycle_complete(len)) - return -1; return 0; } static int ich_hwseq_read(struct flashctx *flash, uint8_t *buf, unsigned int addr, unsigned int len) { - uint16_t hsfc; uint8_t block_len; if (addr + len > flash->chip->total_size * 1024) { @@ -1485,24 +1504,9 @@ /* as well as flash chip page borders as demanded in the Intel datasheets. */ block_len = min(block_len, 256 - (addr & 0xFF)); - ich_hwseq_set_addr(addr); - - if (REGREAD8(ICH9_REG_HSFS) & HSFS_SCIP) { - msg_perr("Error: SCIP bit is unexpectedly set.\n"); + if (ich_exec_sync_hwseq_xfer(HSFC_CYCLE_READ, addr, block_len)) return -1; - } - hsfc = REGREAD16(ICH9_REG_HSFC); - hsfc &= ~hwseq_data.hsfc_fcycle; /* set read operation */ - hsfc &= ~HSFC_FDBC_MASK; /* clear byte count */ - hsfc |= HSFC_CYCLE_READ; /* set erase operation */ - /* set byte count */ - hsfc |= HSFC_FDBC(block_len - 1); - hsfc |= HSFC_FGO; /* start */ - REGWRITE16(ICH9_REG_HSFC, hsfc); - - if (ich_hwseq_wait_for_cycle_complete(block_len)) - return 1; ich_read_data(buf, block_len, ICH9_REG_FDATA0); addr += block_len; buf += block_len; @@ -1513,7 +1517,6 @@ static int ich_hwseq_write(struct flashctx *flash, const uint8_t *buf, unsigned int addr, unsigned int len) { - uint16_t hsfc; uint8_t block_len; if (addr + len > flash->chip->total_size * 1024) { @@ -1527,29 +1530,15 @@ REGWRITE16(ICH9_REG_HSFS, REGREAD16(ICH9_REG_HSFS)); while (len > 0) { - ich_hwseq_set_addr(addr); /* Obey programmer limit... */ block_len = min(len, flash->mst->opaque.max_data_write); /* as well as flash chip page borders as demanded in the Intel datasheets. */ block_len = min(block_len, 256 - (addr & 0xFF)); ich_fill_data(buf, block_len, ICH9_REG_FDATA0); - if (REGREAD8(ICH9_REG_HSFS) & HSFS_SCIP) { - msg_perr("Error: SCIP bit is unexpectedly set.\n"); + if (ich_exec_sync_hwseq_xfer(HSFC_CYCLE_WRITE, addr, block_len)) return -1; - } - hsfc = REGREAD16(ICH9_REG_HSFC); - hsfc &= ~hwseq_data.hsfc_fcycle; /* clear operation */ - hsfc |= HSFC_CYCLE_WRITE; /* set write operation */ - hsfc &= ~HSFC_FDBC_MASK; /* clear byte count */ - /* set byte count */ - hsfc |= HSFC_FDBC(block_len - 1); - hsfc |= HSFC_FGO; /* start */ - REGWRITE16(ICH9_REG_HSFC, hsfc); - - if (ich_hwseq_wait_for_cycle_complete(block_len)) - return -1; addr += block_len; buf += block_len; len -= block_len; -- To view, visit https://review.coreboot.org/c/flashrom/+/62869 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic9fd50841449e02f476a8834f4642d6ecad36dc3 Gerrit-Change-Number: 62869 Gerrit-PatchSet: 1 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: ichspi: Refactor Flashrom HW Sequencing Operation Part I
by Subrata Banik (Code Review) 16 Mar '22

16 Mar '22

Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/62868 ) Change subject: ichspi: Refactor Flashrom HW Sequencing Operation Part I ...................................................................... ichspi: Refactor Flashrom HW Sequencing Operation Part I List of changes: 1. Introduced useful macros (read/write/erase/status etc.) and used throughout the SPI operations. 2. Drop unused macros. 3. SPI operations are using `control register (offset 0x6)` hence converted 32-bit reads into 16-bit read to cover the only hardware sequencing control register alone. BUG=b:223630977 TEST=Able to perform read/write/erase operation on brya. Signed-off-by: Subrata Banik <subratabanik(a)google.com> Change-Id: I3ea74b668e5f8d8e4b3da2a8ad8b81f1813e1e80 --- M ichspi.c 1 file changed, 29 insertions(+), 17 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/68/62868/1 diff --git a/ichspi.c b/ichspi.c index dc03422..3aaf751 100644 --- a/ichspi.c +++ b/ichspi.c @@ -78,11 +78,7 @@ */ /* Changed HSFC Control bits */ -#define PCH100_HSFC_FCYCLE_OFF (17 - 16) /* 1-4: FLASH Cycle */ -#define PCH100_HSFC_FCYCLE (0xf << PCH100_HSFC_FCYCLE_OFF) /* New HSFC Control bit */ -#define HSFC_WET_OFF (21 - 16) /* 5: Write Enable Type */ -#define HSFC_WET (0x1 << HSFC_WET_OFF) #define PCH100_FADDR_FLA 0x07ffffff @@ -140,10 +136,23 @@ #define HSFC_FGO_OFF 0 /* 0: Flash Cycle Go */ #define HSFC_FGO (0x1 << HSFC_FGO_OFF) #define HSFC_FCYCLE_OFF 1 /* 1-2: FLASH Cycle */ -#define HSFC_FCYCLE (0x3 << HSFC_FCYCLE_OFF) +#define HSFC_FCYCLE_BIT_WIDTH_ICH9 3 /* 2 bits */ +#define HSFC_FCYCLE_BIT_WIDTH_PCH 0xf /* 4 bits */ +#define HSFC_FCYCLE_MASK(n) (n << HSFC_FCYCLE_OFF) +#define HSFC_FCYCLE(cyc) ((cyc) << HSFC_FCYCLE_OFF) +#define HSFC_CYCLE_READ HSFC_FCYCLE(0) +#define HSFC_CYCLE_WRITE HSFC_FCYCLE(2) +#define HSFC_CYCLE_4K_ERASE HSFC_FCYCLE(3) +#define HSFC_CYCLE_64K_ERASE HSFC_FCYCLE(4) +#define HSFC_CYCLE_RDID HSFC_FCYCLE(6) +#define HSFC_CYCLE_WR_STATUS HSFC_FCYCLE(7) +#define HSFC_CYCLE_RD_STATUS HSFC_FCYCLE(8) +#define HSFC_WET_OFF 5 +#define HSFC_WET (0x1 << HSFC_WET_OFF) /* 3-7: reserved */ #define HSFC_FDBC_OFF 8 /* 8-13: Flash Data Byte Count */ -#define HSFC_FDBC (0x3f << HSFC_FDBC_OFF) +#define HSFC_FDBC_MASK (0x3f << HSFC_FDBC_OFF) +#define HSFC_FDBC(n) (((n) << HSFC_FDBC_OFF) & HSFC_FDBC_MASK) /* 14: reserved */ #define HSFC_SME_OFF 15 /* 15: SPI SMI# Enable */ #define HSFC_SME (0x1 << HSFC_SME_OFF) @@ -495,14 +504,16 @@ case CHIPSET_400_SERIES_COMET_POINT: case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_ELKHART_LAKE: - _pprint_reg(HSFC, PCH100_HSFC_FCYCLE, PCH100_HSFC_FCYCLE_OFF, reg_val, ", "); + _pprint_reg(HSFC, HSFC_FCYCLE_MASK(HSFC_FCYCLE_BIT_WIDTH_PCH), HSFC_FCYCLE_OFF, + reg_val, ", "); pprint_reg(HSFC, WET, reg_val, ", "); break; default: - pprint_reg(HSFC, FCYCLE, reg_val, ", "); + _pprint_reg(HSFC, HSFC_FCYCLE_MASK(HSFC_FCYCLE_BIT_WIDTH_ICH9), HSFC_FCYCLE_OFF, + reg_val, ", "); break; } - pprint_reg(HSFC, FDBC, reg_val, ", "); + _pprint_reg(HSFC, HSFC_FDBC_MASK, HSFC_FDBC_OFF, reg_val, ", "); pprint_reg(HSFC, SME, reg_val, "\n"); } @@ -1441,7 +1452,7 @@ hsfc = REGREAD16(ICH9_REG_HSFC); hsfc &= ~hwseq_data.hsfc_fcycle; /* clear operation */ - hsfc |= (0x3 << HSFC_FCYCLE_OFF); /* set erase operation */ + hsfc |= HSFC_CYCLE_4K_ERASE; /* set erase operation */ hsfc |= HSFC_FGO; /* start */ msg_pdbg("HSFC used for block erasing: "); prettyprint_ich9_reg_hsfc(hsfc, ich_generation); @@ -1483,9 +1494,10 @@ hsfc = REGREAD16(ICH9_REG_HSFC); hsfc &= ~hwseq_data.hsfc_fcycle; /* set read operation */ - hsfc &= ~HSFC_FDBC; /* clear byte count */ + hsfc &= ~HSFC_FDBC_MASK; /* clear byte count */ + hsfc |= HSFC_CYCLE_READ; /* set erase operation */ /* set byte count */ - hsfc |= (((block_len - 1) << HSFC_FDBC_OFF) & HSFC_FDBC); + hsfc |= HSFC_FDBC(block_len - 1); hsfc |= HSFC_FGO; /* start */ REGWRITE16(ICH9_REG_HSFC, hsfc); @@ -1529,10 +1541,10 @@ hsfc = REGREAD16(ICH9_REG_HSFC); hsfc &= ~hwseq_data.hsfc_fcycle; /* clear operation */ - hsfc |= (0x2 << HSFC_FCYCLE_OFF); /* set write operation */ - hsfc &= ~HSFC_FDBC; /* clear byte count */ + hsfc |= HSFC_CYCLE_WRITE; /* set write operation */ + hsfc &= ~HSFC_FDBC_MASK; /* clear byte count */ /* set byte count */ - hsfc |= (((block_len - 1) << HSFC_FDBC_OFF) & HSFC_FDBC); + hsfc |= HSFC_FDBC(block_len - 1); hsfc |= HSFC_FGO; /* start */ REGWRITE16(ICH9_REG_HSFC, hsfc); @@ -1840,7 +1852,7 @@ swseq->reg_opmenu = PCH100_REG_OPMENU; hwseq->addr_mask = PCH100_FADDR_FLA; hwseq->only_4k = true; - hwseq->hsfc_fcycle = PCH100_HSFC_FCYCLE; + hwseq->hsfc_fcycle = HSFC_FCYCLE_MASK(HSFC_FCYCLE_BIT_WIDTH_PCH); break; default: *num_pr = 5; @@ -1851,7 +1863,7 @@ swseq->reg_opmenu = ICH9_REG_OPMENU; hwseq->addr_mask = ICH9_FADDR_FLA; hwseq->only_4k = false; - hwseq->hsfc_fcycle = HSFC_FCYCLE; + hwseq->hsfc_fcycle = HSFC_FCYCLE_MASK(HSFC_FCYCLE_BIT_WIDTH_ICH9); break; } -- To view, visit https://review.coreboot.org/c/flashrom/+/62868 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3ea74b668e5f8d8e4b3da2a8ad8b81f1813e1e80 Gerrit-Change-Number: 62868 Gerrit-PatchSet: 1 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: ichspi: Unified timeout (5sec) across all SPI operations
by Subrata Banik (Code Review) 16 Mar '22

16 Mar '22

Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/62867 ) Change subject: ichspi: Unified timeout (5sec) across all SPI operations ...................................................................... ichspi: Unified timeout (5sec) across all SPI operations This patch removes taking `timeout` argument for different operations using `ich_hwseq_wait_for_cycle_complete()`. Make use of 5sec unified timeout for all SPI operations. Document Number: TBD (supporting document for multi-master accessing the SPI Flash device is awaited from Intel.) BUG=b:223630977 TEST=Able to perform read/write/erase/status operation on brya. Signed-off-by: Subrata Banik <subratabanik(a)google.com> Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b --- M ichspi.c 1 file changed, 37 insertions(+), 7 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/67/62867/1 diff --git a/ichspi.c b/ichspi.c index 08ca038..dc03422 100644 --- a/ichspi.c +++ b/ichspi.c @@ -32,6 +32,38 @@ /* Sunrise Point */ +/* + * On Intel Chipsets that support multi-master accessing the SPI flash might run + * into a timeout failure when the operation initiated from a single master just + * following the SPI operational timeout recommendation by the vendor datasheet (example: + * winbond spiflash W25Q256JV-DTR specification, table 9.7). + * + * At the underlying level of hardware sequencing operation, it's impossible to know + * the actual status of the SPI bus from a master prior to starting the operation. + * 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: + * + * 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 + */ +#define ICH_HWSEQ_XFER_TIMEOUT_US 5000000 /* 5 seconds */ /* Added HSFS Status bits */ #define HSFS_WRSDIS_OFF 11 /* 11: Flash Configuration Lock-Down */ #define HSFS_WRSDIS (0x1 << HSFS_WRSDIS_OFF) @@ -1275,8 +1307,9 @@ 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) { + unsigned int timeout = ICH_HWSEQ_XFER_TIMEOUT_US; uint16_t hsfs; uint32_t addr; @@ -1371,7 +1404,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) { @@ -1415,7 +1447,7 @@ prettyprint_ich9_reg_hsfc(hsfc, ich_generation); 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; } @@ -1424,7 +1456,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) { @@ -1458,7 +1489,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; @@ -1471,7 +1502,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) { @@ -1506,7 +1536,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/+/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: 1 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: ichspi: Maximize usage of `ich_generation` global variable
by Subrata Banik (Code Review) 16 Mar '22

16 Mar '22

Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/62866 ) Change subject: ichspi: Maximize usage of `ich_generation` global variable ...................................................................... ichspi: Maximize usage of `ich_generation` global variable This patch drop local variable `ich_gen` from `ich_hwseq_wait_for_cycle_complete()` to maintain symmetry between all hardware sequencing function in ichspi.c. BUG=b:223630977 TEST=Able to perform flashrom read/write/erase operation on brya. Signed-off-by: Subrata Banik <subratabanik(a)google.com> Change-Id: I7f37894b683196e741b6a95e290a68e83a550205 --- M ichspi.c 1 file changed, 10 insertions(+), 10 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/66/62866/1 diff --git a/ichspi.c b/ichspi.c index 9f45ec2..08ca038 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1275,9 +1275,7 @@ 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, - enum ich_chipset ich_gen) +static int ich_hwseq_wait_for_cycle_complete(unsigned int timeout, unsigned int len) { uint16_t hsfs; uint32_t addr; @@ -1289,13 +1287,14 @@ programmer_delay(8); } REGWRITE16(ICH9_REG_HSFS, REGREAD16(ICH9_REG_HSFS)); + if (!timeout) { addr = REGREAD32(ICH9_REG_FADDR) & hwseq_data.addr_mask; msg_perr("Timeout error between offset 0x%08x and " "0x%08x (= 0x%08x + %d)!\n", addr, addr + len - 1, addr, len - 1); - prettyprint_ich9_reg_hsfs(hsfs, ich_gen); - prettyprint_ich9_reg_hsfc(REGREAD16(ICH9_REG_HSFC), ich_gen); + prettyprint_ich9_reg_hsfs(hsfs, ich_generation); + prettyprint_ich9_reg_hsfc(REGREAD16(ICH9_REG_HSFC), ich_generation); return 1; } @@ -1304,10 +1303,11 @@ msg_perr("Transaction error between offset 0x%08x and " "0x%08x (= 0x%08x + %d)!\n", addr, addr + len - 1, addr, len - 1); - prettyprint_ich9_reg_hsfs(hsfs, ich_gen); - prettyprint_ich9_reg_hsfc(REGREAD16(ICH9_REG_HSFC), ich_gen); + prettyprint_ich9_reg_hsfs(hsfs, ich_generation); + prettyprint_ich9_reg_hsfc(REGREAD16(ICH9_REG_HSFC), ich_generation); return 1; } + return 0; } @@ -1415,7 +1415,7 @@ prettyprint_ich9_reg_hsfc(hsfc, ich_generation); REGWRITE16(ICH9_REG_HSFC, hsfc); - if (ich_hwseq_wait_for_cycle_complete(timeout, len, ich_generation)) + if (ich_hwseq_wait_for_cycle_complete(timeout, len)) return -1; return 0; } @@ -1458,7 +1458,7 @@ hsfc |= HSFC_FGO; /* start */ REGWRITE16(ICH9_REG_HSFC, hsfc); - if (ich_hwseq_wait_for_cycle_complete(timeout, block_len, ich_generation)) + if (ich_hwseq_wait_for_cycle_complete(timeout, block_len)) return 1; ich_read_data(buf, block_len, ICH9_REG_FDATA0); addr += block_len; @@ -1506,7 +1506,7 @@ hsfc |= HSFC_FGO; /* start */ REGWRITE16(ICH9_REG_HSFC, hsfc); - if (ich_hwseq_wait_for_cycle_complete(timeout, block_len, ich_generation)) + if (ich_hwseq_wait_for_cycle_complete(timeout, block_len)) return -1; addr += block_len; buf += block_len; -- To view, visit https://review.coreboot.org/c/flashrom/+/62866 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7f37894b683196e741b6a95e290a68e83a550205 Gerrit-Change-Number: 62866 Gerrit-PatchSet: 1 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: ichspi: Add support for Meteor Lake
by Angel Pons (Code Review) 16 Mar '22

16 Mar '22

Attention is currently required from: Wonkyu Kim, Subrata Banik, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62783 ) Change subject: ichspi: Add support for Meteor Lake ...................................................................... Patch Set 6: (1 comment) File programmer.h: https://review.coreboot.org/c/flashrom/+/62783/comment/f514bb7b_257128c4 PS5, Line 357: CHIPSET_METEOR_LAKE, > > Any particular reason to put this here? I'd expect this to come after `CHIPSET_ELKHART_LAKE` to pr […] Ah, I see. Thanks for the clarification. -- To view, visit https://review.coreboot.org/c/flashrom/+/62783 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0a2ffe2ba8d96c90d89b77e0d8583d179ff02a75 Gerrit-Change-Number: 62783 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: Wonkyu Kim Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Wonkyu Kim Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 16 Mar 2022 15:21:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No 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: Add support for Meteor Lake
by Subrata Banik (Code Review) 16 Mar '22

16 Mar '22

Attention is currently required from: Wonkyu Kim, Angel Pons, Anastasia Klimchuk. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62783 ) Change subject: ichspi: Add support for Meteor Lake ...................................................................... Patch Set 6: (3 comments) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/62783/comment/7da34ab6_5c0100c4 PS5, Line 310: case CHIPSET_METEOR_LAKE: > Is this correct? Yes, I have verified with MTL SPI programming guide. File ichspi.c: https://review.coreboot.org/c/flashrom/+/62783/comment/628caea6_a0881d21 PS5, Line 2062: ich_gen == CHIPSET_METEOR_LAKE)) { > Please keep this aligned with the previous conditions. […] Ack File programmer.h: https://review.coreboot.org/c/flashrom/+/62783/comment/e8e0611a_ef464ce8 PS5, Line 357: CHIPSET_METEOR_LAKE, > Any particular reason to put this here? I'd expect this to come after `CHIPSET_ELKHART_LAKE` to preserve chronological ordering and PCH/non-PCH grouping. > > Or is this placed after `CHIPSET_600_SERIES_ALDER_POINT`? You got it right, it's the successor of ADL hence, put as per the order. -- To view, visit https://review.coreboot.org/c/flashrom/+/62783 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0a2ffe2ba8d96c90d89b77e0d8583d179ff02a75 Gerrit-Change-Number: 62783 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: Wonkyu Kim Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Wonkyu Kim Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 16 Mar 2022 15:04:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit March 2022