Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33388
Change subject: sb/intel/spi: Check for the SPI lock bit during runtime ......................................................................
sb/intel/spi: Check for the SPI lock bit during runtime
The SPI swseq controller can be locked in other parts of the code, for instance when it's locked down in the finalize section. The driver has to be made aware of that. The simpler solution is to not keep track of the state and simply read out the lock bit on each SPI transfer.
Change-Id: Ifcd5121b89d6f80fc1c1368786982d0d9fa1bf61 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/southbridge/intel/common/spi.c 1 file changed, 25 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/33388/1
diff --git a/src/southbridge/intel/common/spi.c b/src/southbridge/intel/common/spi.c index bf2a44c..3cecd4f 100644 --- a/src/southbridge/intel/common/spi.c +++ b/src/southbridge/intel/common/spi.c @@ -40,8 +40,6 @@
typedef struct spi_slave ich_spi_slave;
-static int g_ichspi_lock = 0; - typedef struct ich7_spi_regs { uint16_t spis; uint16_t spic; @@ -292,7 +290,6 @@ cntlr->data = (uint8_t *)ich7_spi->spid; cntlr->databytes = sizeof(ich7_spi->spid); cntlr->status = (uint8_t *)&ich7_spi->spis; - g_ichspi_lock = readw_(&ich7_spi->spis) & HSFS_FLOCKDN; cntlr->control = &ich7_spi->spic; cntlr->bbar = &ich7_spi->bbar; cntlr->preop = &ich7_spi->preop; @@ -302,7 +299,6 @@ ich9_spi = (ich9_spi_regs *)(rcrb + 0x3800); cntlr->ich9_spi = ich9_spi; hsfs = readw_(&ich9_spi->hsfs); - g_ichspi_lock = hsfs & HSFS_FLOCKDN; cntlr->hsfs = hsfs; cntlr->opmenu = ich9_spi->opmenu; cntlr->menubytes = sizeof(ich9_spi->opmenu); @@ -334,6 +330,29 @@ pci_write_config8(dev, 0xdc, bios_cntl | 0x1); }
+static int spi_locked(void) +{ + uint8_t *rcrb; /* Root Complex Register Block */ + uint32_t rcba; /* Root Complex Base Address */ + ich9_spi_regs *ich9_spi; + ich7_spi_regs *ich7_spi; +#ifdef __SIMPLE_DEVICE__ + pci_devfn_t dev = PCI_DEV(0, 31, 0); +#else + struct device *dev = pcidev_on_root(31, 0); +#endif + rcba = pci_read_config32(dev, 0xf0); + /* Bits 31-14 are the base address, 13-1 are reserved, 0 is enable. */ + rcrb = (uint8_t *)(rcba & 0xffffc000); + if (CONFIG(SOUTHBRIDGE_INTEL_I82801GX)) { + ich7_spi = (ich7_spi_regs *)(rcrb + 0x3020); + return !!(readw_(&ich7_spi->spis) & HSFS_FLOCKDN); + } else { + ich9_spi = (ich9_spi_regs *)(rcrb + 0x3800); + return !!(readw_(&ich9_spi->hsfs) | HSFS_FLOCKDN) ; + } +} + static void spi_init_cb(void *unused) { spi_init(); @@ -405,7 +424,7 @@
trans->opcode = trans->out[0]; spi_use_out(trans, 1); - if (!g_ichspi_lock) { + if (!spi_locked()) { /* The lock is off, so just use index 0. */ writeb_(trans->opcode, cntlr->opmenu); optypes = readw_(cntlr->optype); @@ -550,7 +569,7 @@ * in order to prevent the Management Engine from * issuing a transaction between WREN and DATA. */ - if (!g_ichspi_lock) + if (!spi_locked()) writew_(trans.opcode, cntlr->preop); return 0; }
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33388 )
Change subject: sb/intel/spi: Check for the SPI lock bit during runtime ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/#/c/33388/1/src/southbridge/intel/common/spi.c File src/southbridge/intel/common/spi.c:
https://review.coreboot.org/#/c/33388/1/src/southbridge/intel/common/spi.c@3... PS1, Line 349: return !!(readw_(&ich7_spi->spis) & HSFS_FLOCKDN); code indent should use tabs where possible
https://review.coreboot.org/#/c/33388/1/src/southbridge/intel/common/spi.c@3... PS1, Line 352: return !!(readw_(&ich9_spi->hsfs) | HSFS_FLOCKDN) ; code indent should use tabs where possible
https://review.coreboot.org/#/c/33388/1/src/southbridge/intel/common/spi.c@3... PS1, Line 352: return !!(readw_(&ich9_spi->hsfs) | HSFS_FLOCKDN) ; space prohibited before semicolon
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33388 )
Change subject: sb/intel/spi: Check for the SPI lock bit during runtime ......................................................................
Patch Set 1:
Maybe fetching the SPIBAR could be placed in a separate function.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33388 )
Change subject: sb/intel/spi: Check for the SPI lock bit during runtime ......................................................................
Patch Set 1:
(3 comments)
Maybe fetching the SPIBAR could be placed in a separate function.
There seems to be an easier, alternative approach. See inline comments.
https://review.coreboot.org/#/c/33388/1/src/southbridge/intel/common/spi.c File src/southbridge/intel/common/spi.c:
https://review.coreboot.org/#/c/33388/1/src/southbridge/intel/common/spi.c@9... PS1, Line 95: ich9_spi_regs *ich9_spi; Could make it a union with `ich7_spi_regs`.
https://review.coreboot.org/#/c/33388/1/src/southbridge/intel/common/spi.c@3... PS1, Line 353: } With the pointer to the MMIO regs already cached (needs to be added for ICH7, see above), this could be boiled down to:
if (CONFIG(SOUTHBRIDGE_INTEL_I82801GX)) return !!(read16(&cntlr->ich7_regs->spis) & HSFS_FLOCKDN); else return !!(read16(&cntlr->ich9_regs->hsfs) & HSFS_FLOCKDN);
https://review.coreboot.org/#/c/33388/1/src/southbridge/intel/common/spi.c@4... PS1, Line 427: if (!spi_locked()) { Pass `cntlr` here?
Hello build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33388
to look at the new patch set (#2).
Change subject: sb/intel/spi: Check for the SPI lock bit during runtime ......................................................................
sb/intel/spi: Check for the SPI lock bit during runtime
The SPI swseq controller can be locked in other parts of the code, for instance when it's locked down in the finalize section. The driver has to be made aware of that. The simpler solution is to not keep track of the state and simply read out the lock bit on each SPI transfer.
Change-Id: Ifcd5121b89d6f80fc1c1368786982d0d9fa1bf61 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/southbridge/intel/common/spi.c 1 file changed, 12 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/33388/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33388 )
Change subject: sb/intel/spi: Check for the SPI lock bit during runtime ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/#/c/33388/2/src/southbridge/intel/common/spi.c File src/southbridge/intel/common/spi.c:
https://review.coreboot.org/#/c/33388/2/src/southbridge/intel/common/spi.c@3... PS2, Line 339: return !!(readw_(&cntlr->ich7_spi->spis) & HSFS_FLOCKDN); code indent should use tabs where possible
https://review.coreboot.org/#/c/33388/2/src/southbridge/intel/common/spi.c@3... PS2, Line 341: return !!(readw_(&cntlr->ich9_spi->hsfs) | HSFS_FLOCKDN) ; code indent should use tabs where possible
https://review.coreboot.org/#/c/33388/2/src/southbridge/intel/common/spi.c@3... PS2, Line 341: return !!(readw_(&cntlr->ich9_spi->hsfs) | HSFS_FLOCKDN) ; space prohibited before semicolon
Hello build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33388
to look at the new patch set (#3).
Change subject: sb/intel/spi: Check for the SPI lock bit during runtime ......................................................................
sb/intel/spi: Check for the SPI lock bit during runtime
The SPI swseq controller can be locked in other parts of the code, for instance when it's locked down in the finalize section. The driver has to be made aware of that. The simpler solution is to not keep track of the state and simply read out the lock bit on each SPI transfer.
Change-Id: Ifcd5121b89d6f80fc1c1368786982d0d9fa1bf61 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/southbridge/intel/common/spi.c 1 file changed, 12 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/33388/3
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33388 )
Change subject: sb/intel/spi: Check for the SPI lock bit during runtime ......................................................................
Patch Set 3: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33388 )
Change subject: sb/intel/spi: Check for the SPI lock bit during runtime ......................................................................
Patch Set 3: Code-Review+1
Arthur Heymans has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33388 )
Change subject: sb/intel/spi: Check for the SPI lock bit during runtime ......................................................................
sb/intel/spi: Check for the SPI lock bit during runtime
The SPI swseq controller can be locked in other parts of the code, for instance when it's locked down in the finalize section. The driver has to be made aware of that. The simpler solution is to not keep track of the state and simply read out the lock bit on each SPI transfer.
Change-Id: Ifcd5121b89d6f80fc1c1368786982d0d9fa1bf61 Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/33388 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/southbridge/intel/common/spi.c 1 file changed, 12 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve
diff --git a/src/southbridge/intel/common/spi.c b/src/southbridge/intel/common/spi.c index f447515..d9a77c3 100644 --- a/src/southbridge/intel/common/spi.c +++ b/src/southbridge/intel/common/spi.c @@ -38,8 +38,6 @@
static int spi_is_multichip(void);
-static int g_ichspi_lock = 0; - struct ich7_spi_regs { uint16_t spis; uint16_t spic; @@ -294,7 +292,6 @@ cntlr->data = (uint8_t *)ich7_spi->spid; cntlr->databytes = sizeof(ich7_spi->spid); cntlr->status = (uint8_t *)&ich7_spi->spis; - g_ichspi_lock = readw_(&ich7_spi->spis) & HSFS_FLOCKDN; cntlr->control = &ich7_spi->spic; cntlr->bbar = &ich7_spi->bbar; cntlr->preop = &ich7_spi->preop; @@ -304,7 +301,6 @@ ich9_spi = (struct ich9_spi_regs *)(rcrb + 0x3800); cntlr->ich9_spi = ich9_spi; hsfs = readw_(&ich9_spi->hsfs); - g_ichspi_lock = hsfs & HSFS_FLOCKDN; cntlr->hsfs = hsfs; cntlr->opmenu = ich9_spi->opmenu; cntlr->menubytes = sizeof(ich9_spi->opmenu); @@ -336,6 +332,16 @@ pci_write_config8(dev, 0xdc, bios_cntl | 0x1); }
+static int spi_locked(void) +{ + struct ich_spi_controller *cntlr = &g_cntlr; + if (CONFIG(SOUTHBRIDGE_INTEL_I82801GX)) { + return !!(readw_(&cntlr->ich7_spi->spis) & HSFS_FLOCKDN); + } else { + return !!(readw_(&cntlr->ich9_spi->hsfs) | HSFS_FLOCKDN); + } +} + static void spi_init_cb(void *unused) { spi_init(); @@ -407,7 +413,7 @@
trans->opcode = trans->out[0]; spi_use_out(trans, 1); - if (!g_ichspi_lock) { + if (!spi_locked()) { /* The lock is off, so just use index 0. */ writeb_(trans->opcode, cntlr->opmenu); optypes = readw_(cntlr->optype); @@ -552,7 +558,7 @@ * in order to prevent the Management Engine from * issuing a transaction between WREN and DATA. */ - if (!g_ichspi_lock) + if (!spi_locked()) writew_(trans.opcode, cntlr->preop); return 0; }
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Nico Huber (Code Review)