Hello Richard Spiegel, Martin Roth, Furquan Shaikh,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/32652
to review the following change.
Change subject: soc/amd/stoneyridge: Rework SPI base address get/set ......................................................................
soc/amd/stoneyridge: Rework SPI base address get/set
A subsequent patch will move the soc//stoneyridge LPC functionality to a common directory. Prepare by reworking the SPI BAR configuration function in southbridge.h. The SPI BAR is not a typical PCI BAR, and is at D14F3xA0.
Change-Id: I73ddb4afaf9e67ca0522ecb6085b23c92fedc461 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/stoneyridge/include/soc/southbridge.h M src/soc/amd/stoneyridge/lpc.c M src/soc/amd/stoneyridge/southbridge.c 3 files changed, 41 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/32652/1
diff --git a/src/soc/amd/stoneyridge/include/soc/southbridge.h b/src/soc/amd/stoneyridge/include/soc/southbridge.h index 6734efb..4688ee3 100644 --- a/src/soc/amd/stoneyridge/include/soc/southbridge.h +++ b/src/soc/amd/stoneyridge/include/soc/southbridge.h @@ -357,12 +357,11 @@ #define LPC_WIDEIO2_GENERIC_PORT 0x90
#define SPIROM_BASE_ADDRESS_REGISTER 0xa0 -#define SPI_BASE_RESERVED (BIT(4) | BIT(5)) +#define SPI_BASE_ALIGNMENT 32 #define ROUTE_TPM_2_SPI BIT(3) #define SPI_ABORT_ENABLE BIT(2) #define SPI_ROM_ENABLE BIT(1) #define SPI_ROM_ALT_ENABLE BIT(0) -#define SPI_PRESERVE_BITS (BIT(0) | BIT(1) | BIT(2) | BIT(3))
/* LPC register 0xb8 is DWORD, here there are definitions for byte access. For example, bits 31-24 are accessed through byte access diff --git a/src/soc/amd/stoneyridge/lpc.c b/src/soc/amd/stoneyridge/lpc.c index 3ace1fd..1741e92 100644 --- a/src/soc/amd/stoneyridge/lpc.c +++ b/src/soc/amd/stoneyridge/lpc.c @@ -146,7 +146,7 @@ /* Special case. The SpiRomEnable and other enables should STAY set. */ res = find_resource(dev, 2); spi_enable_bits = pci_read_config32(dev, SPIROM_BASE_ADDRESS_REGISTER); - spi_enable_bits &= SPI_PRESERVE_BITS; + spi_enable_bits &= SPI_BASE_ALIGNMENT - 1; pci_write_config32(dev, SPIROM_BASE_ADDRESS_REGISTER, res->base | spi_enable_bits);
diff --git a/src/soc/amd/stoneyridge/southbridge.c b/src/soc/amd/stoneyridge/southbridge.c index 440f889..3b14a9a 100644 --- a/src/soc/amd/stoneyridge/southbridge.c +++ b/src/soc/amd/stoneyridge/southbridge.c @@ -398,27 +398,49 @@ misc_write32(MISC_CLK_CNTL1, ctrl); }
-static uintptr_t sb_spibase(void) +static uintptr_t sb_get_spibase(void) { - u32 base, enables; + u32 base;
- /* Make sure the base address is predictable */ base = pci_read_config32(SOC_LPC_DEV, SPIROM_BASE_ADDRESS_REGISTER); - enables = base & SPI_PRESERVE_BITS; - base &= ~(SPI_PRESERVE_BITS | SPI_BASE_RESERVED); - - if (!base) { - base = SPI_BASE_ADDRESS; - pci_write_config32(SOC_LPC_DEV, SPIROM_BASE_ADDRESS_REGISTER, - base | enables | SPI_ROM_ENABLE); - /* PCI_COMMAND_MEMORY is read-only and enabled. */ - } + base = ALIGN_DOWN(base, SPI_BASE_ALIGNMENT); return (uintptr_t)base; }
+static void sb_set_spibase(u32 base, u32 enable) +{ + u32 reg32; + + /* only two types of CS# enables are allowed */ + enable &= SPI_ROM_ENABLE | SPI_ROM_ALT_ENABLE; + + reg32 = pci_read_config32(SOC_LPC_DEV, SPIROM_BASE_ADDRESS_REGISTER); + + reg32 &= SPI_BASE_ALIGNMENT - 1; /* preserve only reserved, enables */ + reg32 &= ~(SPI_ROM_ENABLE | SPI_ROM_ALT_ENABLE); + reg32 |= enable; + reg32 |= ALIGN_DOWN(base, SPI_BASE_ALIGNMENT); + + pci_write_config32(SOC_LPC_DEV, SPIROM_BASE_ADDRESS_REGISTER, reg32); +} + +static uintptr_t sb_init_spi_base(void) +{ + u32 base; + + /* Make sure the base address is predictable */ + base = sb_get_spibase(); + + if (!base) + sb_set_spibase(SPI_BASE_ADDRESS, SPI_ROM_ENABLE); + + return (uintptr_t)base; +} + + void sb_set_spi100(u16 norm, u16 fast, u16 alt, u16 tpm) { - uintptr_t base = sb_spibase(); + uintptr_t base = sb_init_spi_base(); write16((void *)(base + SPI100_SPEED_CONFIG), (norm << SPI_NORM_SPEED_NEW_SH) | (fast << SPI_FAST_SPEED_NEW_SH) | @@ -429,7 +451,7 @@
void sb_disable_4dw_burst(void) { - uintptr_t base = sb_spibase(); + uintptr_t base = sb_init_spi_base(); write16((void *)(base + SPI100_HOST_PREF_CONFIG), read16((void *)(base + SPI100_HOST_PREF_CONFIG)) & ~SPI_RD4DW_EN_HOST); @@ -437,7 +459,7 @@
void sb_read_mode(u32 mode) { - uintptr_t base = sb_spibase(); + uintptr_t base = sb_init_spi_base(); write32((void *)(base + SPI_CNTRL0), (read32((void *)(base + SPI_CNTRL0)) & ~SPI_READ_MODE_MASK) | mode); @@ -467,7 +489,7 @@ * Enable FCH to decode TPM associated Memory and IO regions to SPI * * This should be used if TPM is connected to SPI bus. - * Assumes SPI address space is already configured via a call to sb_spibase(). + * Assumes SPI address space is already configured. */ void sb_tpm_decode_spi(void) { @@ -632,7 +654,7 @@ sb_lpc_port80(); sb_lpc_decode(); sb_lpc_early_setup(); - sb_spibase(); + sb_init_spi_base(); sb_disable_4dw_burst(); /* Must be disabled on CZ(ST) */ enable_acpimmio_decode(); fch_smbus_init();
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32652 )
Change subject: soc/amd/stoneyridge: Rework SPI base address get/set ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32652/1/src/soc/amd/stoneyridge/southbridge.... File src/soc/amd/stoneyridge/southbridge.c:
https://review.coreboot.org/#/c/32652/1/src/soc/amd/stoneyridge/southbridge.... PS1, Line 435: sb_set_spibase need to return SPI_BASE_ADDRESS;
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32652 )
Change subject: soc/amd/stoneyridge: Rework SPI base address get/set ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32652/1/src/soc/amd/stoneyridge/southbridge.... File src/soc/amd/stoneyridge/southbridge.c:
https://review.coreboot.org/#/c/32652/1/src/soc/amd/stoneyridge/southbridge.... PS1, Line 435: sb_set_spibase
need to return SPI_BASE_ADDRESS;
Done
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32652 )
Change subject: soc/amd/stoneyridge: Rework SPI base address get/set ......................................................................
Patch Set 2: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32652 )
Change subject: soc/amd/stoneyridge: Rework SPI base address get/set ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/32652/4/src/soc/amd/stoneyridge/include/soc/... File src/soc/amd/stoneyridge/include/soc/southbridge.h:
https://review.coreboot.org/#/c/32652/4/src/soc/amd/stoneyridge/include/soc/... PS4, Line 360: 32 Shouldn't this be 64 to ensure bit 5 is also preserved?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32652 )
Change subject: soc/amd/stoneyridge: Rework SPI base address get/set ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/#/c/32652/4/src/soc/amd/stoneyridge/southbridge.... File src/soc/amd/stoneyridge/southbridge.c:
https://review.coreboot.org/#/c/32652/4/src/soc/amd/stoneyridge/southbridge.... PS4, Line 410: sb_set_spibase I am guessing this is going to be called by more than just sb_init_spi_base() in the future?
https://review.coreboot.org/#/c/32652/4/src/soc/amd/stoneyridge/southbridge.... PS4, Line 414: /* only two types of CS# enables are allowed */ This is a difference in behavior than before. I am guessing that's intentional?
Hello Richard Spiegel, build bot (Jenkins), Furquan Shaikh, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32652
to look at the new patch set (#5).
Change subject: soc/amd/stoneyridge: Rework SPI base address get/set ......................................................................
soc/amd/stoneyridge: Rework SPI base address get/set
A subsequent patch will move the soc//stoneyridge LPC functionality to a common directory. Prepare by reworking the SPI BAR configuration function in southbridge.h. The SPI BAR is not a typical PCI BAR, and is at D14F3xA0.
Change-Id: I73ddb4afaf9e67ca0522ecb6085b23c92fedc461 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/stoneyridge/include/soc/southbridge.h M src/soc/amd/stoneyridge/lpc.c M src/soc/amd/stoneyridge/southbridge.c 3 files changed, 41 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/32652/5
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32652 )
Change subject: soc/amd/stoneyridge: Rework SPI base address get/set ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/#/c/32652/4/src/soc/amd/stoneyridge/include/soc/... File src/soc/amd/stoneyridge/include/soc/southbridge.h:
https://review.coreboot.org/#/c/32652/4/src/soc/amd/stoneyridge/include/soc/... PS4, Line 360: 32
Shouldn't this be 64 to ensure bit 5 is also preserved?
Done
https://review.coreboot.org/#/c/32652/4/src/soc/amd/stoneyridge/southbridge.... File src/soc/amd/stoneyridge/southbridge.c:
https://review.coreboot.org/#/c/32652/4/src/soc/amd/stoneyridge/southbridge.... PS4, Line 410: sb_set_spibase
I am guessing this is going to be called by more than just sb_init_spi_base() in the future?
While possible, I don't have any particular plans. Separating it was deliberate, though. The SPI controller's base address & enables are in the LPC controller's non-standard PCI config space. So, it's done with the intent of moving to a different file.
https://review.coreboot.org/#/c/32652/4/src/soc/amd/stoneyridge/southbridge.... PS4, Line 414: /* only two types of CS# enables are allowed */
This is a difference in behavior than before. […]
Unless I goofed it, it should produce an identical result as before, but be more flexible. (I ran a quick test on Grunt and it seems to be true.) Rather than always enabling only SPI_ROM_ENABLE (b1), it allows a caller to also use SPI_ROM_ALT_ENABLE (b0).
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32652 )
Change subject: soc/amd/stoneyridge: Rework SPI base address get/set ......................................................................
Patch Set 5: Code-Review+2
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32652 )
Change subject: soc/amd/stoneyridge: Rework SPI base address get/set ......................................................................
Patch Set 6: Code-Review+2
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32652 )
Change subject: soc/amd/stoneyridge: Rework SPI base address get/set ......................................................................
soc/amd/stoneyridge: Rework SPI base address get/set
A subsequent patch will move the soc//stoneyridge LPC functionality to a common directory. Prepare by reworking the SPI BAR configuration function in southbridge.h. The SPI BAR is not a typical PCI BAR, and is at D14F3xA0.
Change-Id: I73ddb4afaf9e67ca0522ecb6085b23c92fedc461 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32652 Reviewed-by: Martin Roth martinroth@google.com Reviewed-by: Richard Spiegel richard.spiegel@silverbackltd.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/stoneyridge/include/soc/southbridge.h M src/soc/amd/stoneyridge/lpc.c M src/soc/amd/stoneyridge/southbridge.c 3 files changed, 41 insertions(+), 20 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved Richard Spiegel: Looks good to me, approved
diff --git a/src/soc/amd/stoneyridge/include/soc/southbridge.h b/src/soc/amd/stoneyridge/include/soc/southbridge.h index 6734efb..0ac4385 100644 --- a/src/soc/amd/stoneyridge/include/soc/southbridge.h +++ b/src/soc/amd/stoneyridge/include/soc/southbridge.h @@ -357,12 +357,11 @@ #define LPC_WIDEIO2_GENERIC_PORT 0x90
#define SPIROM_BASE_ADDRESS_REGISTER 0xa0 -#define SPI_BASE_RESERVED (BIT(4) | BIT(5)) +#define SPI_BASE_ALIGNMENT BIT(6) #define ROUTE_TPM_2_SPI BIT(3) #define SPI_ABORT_ENABLE BIT(2) #define SPI_ROM_ENABLE BIT(1) #define SPI_ROM_ALT_ENABLE BIT(0) -#define SPI_PRESERVE_BITS (BIT(0) | BIT(1) | BIT(2) | BIT(3))
/* LPC register 0xb8 is DWORD, here there are definitions for byte access. For example, bits 31-24 are accessed through byte access diff --git a/src/soc/amd/stoneyridge/lpc.c b/src/soc/amd/stoneyridge/lpc.c index 3ace1fd..1741e92 100644 --- a/src/soc/amd/stoneyridge/lpc.c +++ b/src/soc/amd/stoneyridge/lpc.c @@ -146,7 +146,7 @@ /* Special case. The SpiRomEnable and other enables should STAY set. */ res = find_resource(dev, 2); spi_enable_bits = pci_read_config32(dev, SPIROM_BASE_ADDRESS_REGISTER); - spi_enable_bits &= SPI_PRESERVE_BITS; + spi_enable_bits &= SPI_BASE_ALIGNMENT - 1; pci_write_config32(dev, SPIROM_BASE_ADDRESS_REGISTER, res->base | spi_enable_bits);
diff --git a/src/soc/amd/stoneyridge/southbridge.c b/src/soc/amd/stoneyridge/southbridge.c index 84db3dd..b8d0595 100644 --- a/src/soc/amd/stoneyridge/southbridge.c +++ b/src/soc/amd/stoneyridge/southbridge.c @@ -397,27 +397,49 @@ misc_write32(MISC_CLK_CNTL1, ctrl); }
-static uintptr_t sb_spibase(void) +static uintptr_t sb_get_spibase(void) { - u32 base, enables; + u32 base; + + base = pci_read_config32(SOC_LPC_DEV, SPIROM_BASE_ADDRESS_REGISTER); + base = ALIGN_DOWN(base, SPI_BASE_ALIGNMENT); + return (uintptr_t)base; +} + +static void sb_set_spibase(u32 base, u32 enable) +{ + u32 reg32; + + /* only two types of CS# enables are allowed */ + enable &= SPI_ROM_ENABLE | SPI_ROM_ALT_ENABLE; + + reg32 = pci_read_config32(SOC_LPC_DEV, SPIROM_BASE_ADDRESS_REGISTER); + + reg32 &= SPI_BASE_ALIGNMENT - 1; /* preserve only reserved, enables */ + reg32 &= ~(SPI_ROM_ENABLE | SPI_ROM_ALT_ENABLE); + reg32 |= enable; + reg32 |= ALIGN_DOWN(base, SPI_BASE_ALIGNMENT); + + pci_write_config32(SOC_LPC_DEV, SPIROM_BASE_ADDRESS_REGISTER, reg32); +} + +static uintptr_t sb_init_spi_base(void) +{ + uintptr_t base;
/* Make sure the base address is predictable */ - base = pci_read_config32(SOC_LPC_DEV, SPIROM_BASE_ADDRESS_REGISTER); - enables = base & SPI_PRESERVE_BITS; - base &= ~(SPI_PRESERVE_BITS | SPI_BASE_RESERVED); + base = sb_get_spibase();
- if (!base) { - base = SPI_BASE_ADDRESS; - pci_write_config32(SOC_LPC_DEV, SPIROM_BASE_ADDRESS_REGISTER, - base | enables | SPI_ROM_ENABLE); - /* PCI_COMMAND_MEMORY is read-only and enabled. */ - } - return (uintptr_t)base; + if (base) + return base; + + sb_set_spibase(SPI_BASE_ADDRESS, SPI_ROM_ENABLE); + return SPI_BASE_ADDRESS; }
void sb_set_spi100(u16 norm, u16 fast, u16 alt, u16 tpm) { - uintptr_t base = sb_spibase(); + uintptr_t base = sb_init_spi_base(); write16((void *)(base + SPI100_SPEED_CONFIG), (norm << SPI_NORM_SPEED_NEW_SH) | (fast << SPI_FAST_SPEED_NEW_SH) | @@ -428,7 +450,7 @@
void sb_disable_4dw_burst(void) { - uintptr_t base = sb_spibase(); + uintptr_t base = sb_init_spi_base(); write16((void *)(base + SPI100_HOST_PREF_CONFIG), read16((void *)(base + SPI100_HOST_PREF_CONFIG)) & ~SPI_RD4DW_EN_HOST); @@ -436,7 +458,7 @@
void sb_read_mode(u32 mode) { - uintptr_t base = sb_spibase(); + uintptr_t base = sb_init_spi_base(); write32((void *)(base + SPI_CNTRL0), (read32((void *)(base + SPI_CNTRL0)) & ~SPI_READ_MODE_MASK) | mode); @@ -466,7 +488,7 @@ * Enable FCH to decode TPM associated Memory and IO regions to SPI * * This should be used if TPM is connected to SPI bus. - * Assumes SPI address space is already configured via a call to sb_spibase(). + * Assumes SPI address space is already configured. */ void sb_tpm_decode_spi(void) { @@ -631,7 +653,7 @@ sb_lpc_port80(); sb_lpc_decode(); sb_lpc_early_setup(); - sb_spibase(); + sb_init_spi_base(); sb_disable_4dw_burst(); /* Must be disabled on CZ(ST) */ enable_acpimmio_decode(); fch_smbus_init();
-
Furquan Shaikh (Code Review) -
Marshall Dawson (Code Review)
-
Martin Roth (Code Review)
-
Richard Spiegel (Code Review)