Change in coreboot[master]: soc/amd/common: Allow the SPI base to be set for psp_verstage
Martin Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Allow the SPI base to be set for psp_verstage ......................................................................
soc/amd/common: Allow the SPI base to be set for psp_verstage
The PSP maps the devices into it's memory in a different fashion, so we need to be able to use a value other than the standard x86 address.
BUG=b:159811539 TEST=Build with following patch to set the SPI speed in psp_verstage.
Change-Id: I50d9de269bcb88fbf510056a6216e22a050cae6b --- M src/soc/amd/common/block/include/amdblocks/spi.h M src/soc/amd/common/block/spi/fch_spi.c 2 files changed, 15 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/43307/1
diff --git a/src/soc/amd/common/block/include/amdblocks/spi.h b/src/soc/amd/common/block/include/amdblocks/spi.h index d226e0c..02d2073 100644 --- a/src/soc/amd/common/block/include/amdblocks/spi.h +++ b/src/soc/amd/common/block/include/amdblocks/spi.h @@ -93,5 +93,6 @@ * settings from mainboard devicetree to configure speed and read mode. */ void fch_spi_config_modes(void); +void spi_set_base(void *base);
#endif /* __AMDBLOCKS_SPI_H__ */ diff --git a/src/soc/amd/common/block/spi/fch_spi.c b/src/soc/amd/common/block/spi/fch_spi.c index bf64c3f..04ed634 100644 --- a/src/soc/amd/common/block/spi/fch_spi.c +++ b/src/soc/amd/common/block/spi/fch_spi.c @@ -8,19 +8,28 @@ #include <soc/iomap.h> #include <stdint.h>
+static uintptr_t spi_base; + +void spi_set_base(void *base) +{ + spi_base = (uintptr_t)base; +} + static uintptr_t fch_spi_base(void) { - uintptr_t base; + if (spi_base) + return spi_base;
- base = lpc_get_spibase(); + spi_base = lpc_get_spibase();
- if (base) - return base; + if (spi_base) + return spi_base;
lpc_set_spibase(SPI_BASE_ADDRESS); lpc_enable_spi_rom(SPI_ROM_ENABLE);
- return SPI_BASE_ADDRESS; + spi_base = SPI_BASE_ADDRESS; + return spi_base; }
static void fch_spi_set_spi100(int norm, int fast, int alt, int tpm)
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43307
to look at the new patch set (#2).
Change subject: soc/amd/common: Allow the SPI base to be set for psp_verstage ......................................................................
soc/amd/common: Allow the SPI base to be set for psp_verstage
The PSP maps the devices into it's memory in a different fashion, so we need to be able to use a value other than the standard x86 address.
BUG=b:159811539 TEST=Build with following patch to set the SPI speed in psp_verstage.
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I50d9de269bcb88fbf510056a6216e22a050cae6b --- M src/soc/amd/common/block/include/amdblocks/spi.h M src/soc/amd/common/block/spi/fch_spi.c 2 files changed, 15 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/43307/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Allow the SPI base to be set for psp_verstage ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43307/2/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43307/2/src/soc/amd/common/block/sp... PS2, Line 23: spi_base = lpc_get_spibase(); Please make it so that garbage-collection can throw this out. I believe this pulls in invalid PCI config access code.
Same request for lpc/espi_util.c that was already submitted.
Could you quote the essentials of b/156305600 ?
https://review.coreboot.org/c/coreboot/+/43307/2/src/soc/amd/common/block/sp... PS2, Line 28: lpc_set_spibase(SPI_BASE_ADDRESS); Like before, I would rather have all x86-centric views of memory space hidden from ENV_ARM. The approach taken with DECLARE_ACPIMMIO() should work fine here.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Allow the SPI base to be set for psp_verstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43307/2/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43307/2/src/soc/amd/common/block/sp... PS2, Line 23: spi_base = lpc_get_spibase();
Please make it so that garbage-collection can throw this out. […]
b/156305600 | AMD/Picasso: Enable a mechanism for using different BARs on PSP v/s x86 On x86, all controllers have a fixed base address for MMIO. However, for the PSP the base address is not fixed and instead depends on the virtual address space mapping done by the PSP BL. We are currently adding -psp.c / _psp.c which shouldn't really be required.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43307
to look at the new patch set (#3).
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
soc/amd/common: Refactor and consolidate code for spi base
Previously, the spi base address code was using a number of different functions in a way that didn't work for use on the PSP.
This patch consolidates all of that to a single saved value that gets reads the LPC SPI base address by default on X86, and allows the PSP to set it to a different value.
BUG=b:159811539 TEST=Build with following patch to set the SPI speed in psp_verstage.
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I50d9de269bcb88fbf510056a6216e22a050cae6b --- M src/soc/amd/common/block/include/amdblocks/spi.h M src/soc/amd/common/block/spi/fch_spi.c M src/soc/amd/common/block/spi/fch_spi_ctrl.c M src/soc/amd/stoneyridge/southbridge.c 4 files changed, 32 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/43307/3
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43307/2/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43307/2/src/soc/amd/common/block/sp... PS2, Line 23: spi_base = lpc_get_spibase();
b/156305600 | AMD/Picasso: Enable a mechanism for using different BARs on PSP v/s x86 […]
Done
https://review.coreboot.org/c/coreboot/+/43307/2/src/soc/amd/common/block/sp... PS2, Line 28: lpc_set_spibase(SPI_BASE_ADDRESS);
Like before, I would rather have all x86-centric views of memory space hidden from ENV_ARM. […]
Done
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43307
to look at the new patch set (#4).
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
soc/amd/common: Refactor and consolidate code for spi base
Previously, the spi base address code was using a number of different functions in a way that didn't work for use on the PSP.
This patch consolidates all of that to a single saved value that gets reads the LPC SPI base address by default on X86, and allows the PSP to set it to a different value.
BUG=b:159811539 TEST=Build with following patch to set the SPI speed in psp_verstage.
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I50d9de269bcb88fbf510056a6216e22a050cae6b --- M src/soc/amd/common/block/include/amdblocks/spi.h M src/soc/amd/common/block/spi/fch_spi.c M src/soc/amd/common/block/spi/fch_spi_ctrl.c M src/soc/amd/stoneyridge/southbridge.c 4 files changed, 35 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/43307/4
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43307/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43307/4//COMMIT_MSG@13 PS4, Line 13: reads read from?
https://review.coreboot.org/c/coreboot/+/43307/4/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43307/4/src/soc/amd/common/block/sp... PS4, Line 20: if (ENV_X86 && !spi_base) : spi_set_base((void *)lpc_get_spibase()); Move this to fch_spi_early_init?
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Eric Peers, Rob Barnes, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43307
to look at the new patch set (#5).
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
soc/amd/common: Refactor and consolidate code for spi base
Previously, the spi base address code was using a number of different functions in a way that didn't work for use on the PSP.
This patch consolidates all of that to a single saved value that gets the LPC SPI BAR by default on X86, and allows the PSP to set it to a different value.
BUG=b:159811539 TEST=Build with following patch to set the SPI speed in psp_verstage.
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I50d9de269bcb88fbf510056a6216e22a050cae6b --- M src/soc/amd/common/block/include/amdblocks/spi.h M src/soc/amd/common/block/spi/fch_spi.c M src/soc/amd/common/block/spi/fch_spi_ctrl.c M src/soc/amd/stoneyridge/southbridge.c 4 files changed, 35 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/43307/5
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43307/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43307/4//COMMIT_MSG@13 PS4, Line 13: reads
read from?
Done
https://review.coreboot.org/c/coreboot/+/43307/4/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43307/4/src/soc/amd/common/block/sp... PS4, Line 20: if (ENV_X86 && !spi_base) : spi_set_base((void *)lpc_get_spibase());
Move this to fch_spi_early_init?
The global variables don't carry their values between stages. I was setting it in the init functions, but they don't get called in every stage.
I had an assert here to make sure the value was initialized, and we were hitting it in ramstage.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/43307/4/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43307/4/src/soc/amd/common/block/sp... PS4, Line 20: if (ENV_X86 && !spi_base) : spi_set_base((void *)lpc_get_spibase());
The global variables don't carry their values between stages. […]
Done
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 5:
(6 comments)
File soc/amd/common/block/lpc/lpc_util.c has more references to SPI_BASE_ADDRESS. Aaron restored CB:42523 which would block those x86-centric defines from appearing in psp-verstage builds.
There is some amount of asymmetry with the MMIO accessors, but I marked those resolved anyways.
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... PS5, Line 31: write16((void *)(base + SPI100_ENABLE), SPI_USE_SPI100); These are spi_write16() but we did not declare such a function.
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... PS5, Line 47: write32((void *)(base + SPI_CNTRL0), val | SPI_READ_MODE(mode)); These are really spi_read32() and spi_write32(). We had declared such functions in some of the other files.
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... PS5, Line 33: static inline uint8_t spi_read8(uint8_t reg) inline has no real purpose here
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... PS5, Line 35: return read8((void *)(spi_get_bar() + reg)); I would have preferred uint8_t *spi_bar over function call to keep these as simple MMIO leaf functions for better code density, compiler probably would have inlined them implicitly but most likely no longer does so.
Having and assert() in this file for the non-static functions should keep compiler happy about not checking NULL pointer separately every time.
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/stoneyridge/sou... File src/soc/amd/stoneyridge/southbridge.c:
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/stoneyridge/sou... PS5, Line 276: write16((void *)(base + SPI100_ENABLE), SPI_USE_SPI100); We could have declared spi_write16() and spi_read16() too?
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/stoneyridge/sou... PS5, Line 290: write32((void *)(base + SPI_CNTRL0), We called this spi_write32() and spi_read32() in that other file?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... PS5, Line 13: spi_set_base Who is calling spi_set_base() ? I don't see any callers aside from the one below. Wouldn't there need to be other callers?
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... PS5, Line 13: (void *base Why are we taking a void * but returning a uintptr_t?
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... PS5, Line 113: (unsigned int) Just change the format specifier to lx. No cast needed as these are LP32/LP64 systems w/ uinptr_t being long.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 5:
(9 comments)
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... PS5, Line 13: (void *base
Why are we taking a void * but returning a uintptr_t?
The code in psp verstage was written to write a void pointer to all the functions that it calls. We can save it as a void * if you'd prefer, but since we're using it for addition all over the place, that doesn't make sense.
Do you want me to refactor all the set_X_base() functions to take a uintptr_t?
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... PS5, Line 13: spi_set_base
Who is calling spi_set_base() ? I don't see any callers aside from the one below. […]
The other caller (and the main caller) is in psp_verstage. The use is in the following patch.
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... PS5, Line 31: write16((void *)(base + SPI100_ENABLE), SPI_USE_SPI100);
These are spi_write16() but we did not declare such a function.
ack
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... PS5, Line 47: write32((void *)(base + SPI_CNTRL0), val | SPI_READ_MODE(mode));
These are really spi_read32() and spi_write32(). […]
ack
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... PS5, Line 33: static inline uint8_t spi_read8(uint8_t reg)
inline has no real purpose here
Ack
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... PS5, Line 35: return read8((void *)(spi_get_bar() + reg));
I would have preferred uint8_t *spi_bar over function call to keep these as simple MMIO leaf functio […]
Changed to use spi_bar. Why would you prefer uint8_t * over uintptr_t *? My understanding is that they're equal in terms of math.
Sorry, I think I'm missing something about the assert. Non-static functions in this file?
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... PS5, Line 113: (unsigned int)
Just change the format specifier to lx. […]
Done
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/stoneyridge/sou... File src/soc/amd/stoneyridge/southbridge.c:
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/stoneyridge/sou... PS5, Line 276: write16((void *)(base + SPI100_ENABLE), SPI_USE_SPI100);
We could have declared spi_write16() and spi_read16() too?
Ack.
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/stoneyridge/sou... PS5, Line 290: write32((void *)(base + SPI_CNTRL0),
We called this spi_write32() and spi_read32() in that other file?
Yes, it looks like someone could clean this up some if they wanted.
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Eric Peers, Rob Barnes, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43307
to look at the new patch set (#6).
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
soc/amd/common: Refactor and consolidate code for spi base
Previously, the spi base address code was using a number of different functions in a way that didn't work for use on the PSP.
This patch consolidates all of that to a single saved value that gets reads the LPC SPI base address by default on X86, and allows the PSP to set it to a different value.
BUG=b:159811539 TEST=Build with following patch to set the SPI speed in psp_verstage.
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I50d9de269bcb88fbf510056a6216e22a050cae6b --- M src/soc/amd/common/block/include/amdblocks/spi.h M src/soc/amd/common/block/spi/fch_spi.c M src/soc/amd/common/block/spi/fch_spi_ctrl.c M src/soc/amd/stoneyridge/southbridge.c 4 files changed, 36 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/43307/6
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 6: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... PS6, Line 35: spi_base Don't you want spi_get_bar()?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... PS5, Line 13: (void *base
The code in psp verstage was written to write a void pointer to all the functions that it calls. […]
We can do that in a follow up for consistency but it's not necessary. Things are just asymmetric and throws me off.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... PS6, Line 35: spi_base
Don't you want spi_get_bar()?
Well, I thought I did, but just changed it in this patch version because Kyosti said he preferred it this way. It seems as safe as it was before this change where we read the spibar variable out of the LPC BAR.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 6: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... PS6, Line 35: spi_base
Well, I thought I did, but just changed it in this patch version because Kyosti said he preferred it […]
fwiw, I think Kyosti's comment was about using char * because of pointer aliasing rules using void *.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... PS6, Line 35: spi_base
fwiw, I think Kyosti's comment was about using char * because of pointer aliasing rules using void * […]
Ok, that makes sense.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... PS6, Line 33: static inline uint8_t spi_read8(uint8_t reg) You ack'd the inline and did not address it :(
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... PS6, Line 35: spi_base
Ok, that makes sense.
My comments were mostly about the asymmetry of MMIO accessors. When ACPIMMIO banks don't need function call here, why would SPI need?
But looks like there is no plan to unify these accessors even inside SPI, and there is now three different approaches for fundamentally the same thing.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... PS6, Line 33: static inline uint8_t spi_read8(uint8_t reg)
You ack'd the inline and did not address it :(
Yes, I was acknowledging the comment. I would have marked it 'done' if I had done it. As far as I can tell, it has nothing to do with this patch, especially since you wanted me to change the code back to using a simple variable, which I did. :)
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... PS6, Line 35: spi_base
My comments were mostly about the asymmetry of MMIO accessors. […]
Personally, I don't think this is the same thing, because the ACPIMMIO addresses aren't set by PCI BARs, where the LPC base is. To me that's a bug difference. I did absolutely consider simply hardcoding it and making the assumption that nothing would ever update it, but that didn't seem like it was the right thing to do.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 6:
(5 comments)
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/spi.h:
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/in... PS6, Line 8: extern uintptr_t spi_base; I dislike having this shared between compilation units; this should be encapsulated in one compilation unit
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... PS6, Line 22: shouldn't the case of !ENV_X86 && !spi_base be handeled as well?
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... PS6, Line 30: write16((void *)(base + SPI100_SPEED_CONFIG), SPI_SPEED_CFG(norm, fast, alt, tpm)); I'd use the spi_read/write8/16/32 functions here
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... PS6, Line 35: return read8((void *)(spi_base + reg)); I'd use spi_get_bar() here. sure, it is slower, but I doubt that this would have a measurable performance impact on the system level
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/stoneyridge/sou... File src/soc/amd/stoneyridge/southbridge.c:
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/stoneyridge/sou... PS6, Line 271: write16((void *)(base + SPI100_SPEED_CONFIG), I'd use the spi_read/write8/16/32 functions here
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 6:
(5 comments)
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/spi.h:
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/in... PS6, Line 8: extern uintptr_t spi_base;
I dislike having this shared between compilation units; this should be encapsulated in one compilati […]
I agree. I'll revert back to the old code, then look at reorganizing things in a follow-on patch.
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... PS6, Line 22:
shouldn't the case of !ENV_X86 && !spi_base be handeled as well?
We call to initialize that very early in the PSP verstage, and we can't read the value. I could die if it's NULL, but that's all.
Basically I wasn't worried about it because we can't do anything, and I know that it's already handled.
I'll add a die call if it's null.
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... PS6, Line 30: write16((void *)(base + SPI100_SPEED_CONFIG), SPI_SPEED_CFG(norm, fast, alt, tpm));
I'd use the spi_read/write8/16/32 functions here
Yeah, Kyosti mentioned that as well, and I can work on that in a future patch, but I was just refactoring the base address handling here.
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... PS6, Line 35: return read8((void *)(spi_base + reg));
I'd use spi_get_bar() here. […]
Will change back.
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/stoneyridge/sou... File src/soc/amd/stoneyridge/southbridge.c:
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/stoneyridge/sou... PS6, Line 271: write16((void *)(base + SPI100_SPEED_CONFIG),
I'd use the spi_read/write8/16/32 functions here
I'll look at that in a future patch.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... PS6, Line 22:
We call to initialize that very early in the PSP verstage, and we can't read the value. […]
Added ASSERT(spi_base)
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/43307/5/src/soc/amd/common/block/sp... PS5, Line 33: static inline uint8_t spi_read8(uint8_t reg)
Ack
Removed since I changed back to spi_get_bar()
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... PS6, Line 33: static inline uint8_t spi_read8(uint8_t reg)
Yes, I was acknowledging the comment. I would have marked it 'done' if I had done it. […]
Addressed.
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... PS6, Line 35: return read8((void *)(spi_base + reg));
Will change back.
Done.
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Eric Peers, Rob Barnes, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43307
to look at the new patch set (#7).
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
soc/amd/common: Refactor and consolidate code for spi base
Previously, the spi base address code was using a number of different functions in a way that didn't work for use on the PSP.
This patch consolidates all of that to a single saved value that gets the LPC SPI base address by default on X86, and allows the PSP to set it to a different value.
BUG=b:159811539 TEST=Build with following patch to set the SPI speed in psp_verstage.
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I50d9de269bcb88fbf510056a6216e22a050cae6b --- M src/soc/amd/common/block/include/amdblocks/spi.h M src/soc/amd/common/block/spi/fch_spi.c M src/soc/amd/common/block/spi/fch_spi_ctrl.c M src/soc/amd/stoneyridge/southbridge.c 4 files changed, 40 insertions(+), 41 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/43307/7
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 7: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 7:
Repeating comment from patchset #5. File soc/amd/common/block/lpc/lpc_util.c has more references to SPI_BASE_ADDRESS. Aaron restored CB:42523 which would block those x86-centric defines from appearing in psp-verstage builds.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... PS6, Line 35: return read8((void *)(spi_base + reg));
Done.
I would not call it slower. For some compilation units, it may double the .text section size.
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... PS6, Line 35: spi_base
Personally, I don't think this is the same thing, because the ACPIMMIO addresses aren't set by PCI B […]
It's exactly the same question; Do we guarantee the base variable is initialized prior to calling the MMIO accessors. For ENV_X86 and ACPIMMIO guarantee comes from const variable placed in .text, for ENV_ARM and ACPIMMIO it comes from the program flow.
You are now introducing third approach in which such guarantee does not exist. I just think it was better and cleaner without the function call. Then again, I also feel that access to such hardware (here SPI registers) is better restricted to single compilation unit.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 7: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... PS6, Line 35: spi_base
It's exactly the same question; Do we guarantee the base variable is initialized prior to calling th […]
Good point. I'd prefer encapsulation.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 8:
(3 comments)
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/spi.h:
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/in... PS6, Line 8: extern uintptr_t spi_base;
I agree. I'll revert back to the old code, then look at reorganizing things in a follow-on patch.
Done
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... PS6, Line 35: spi_base
Good point. I'd prefer encapsulation.
encapsulated in a single file in a follow-on patch. cb:43772
https://review.coreboot.org/c/coreboot/+/43307/6/src/soc/amd/common/block/sp... PS6, Line 35: return read8((void *)(spi_base + reg));
I would not call it slower. For some compilation units, it may double the .text section size.
Addressed in the commit message in a follow-on patch. cb:43772
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 8: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
Patch Set 8: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43307 )
Change subject: soc/amd/common: Refactor and consolidate code for spi base ......................................................................
soc/amd/common: Refactor and consolidate code for spi base
Previously, the spi base address code was using a number of different functions in a way that didn't work for use on the PSP.
This patch consolidates all of that to a single saved value that gets the LPC SPI base address by default on X86, and allows the PSP to set it to a different value.
BUG=b:159811539 TEST=Build with following patch to set the SPI speed in psp_verstage.
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I50d9de269bcb88fbf510056a6216e22a050cae6b Reviewed-on: https://review.coreboot.org/c/coreboot/+/43307 Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/common/block/include/amdblocks/spi.h M src/soc/amd/common/block/spi/fch_spi.c M src/soc/amd/common/block/spi/fch_spi_ctrl.c M src/soc/amd/stoneyridge/southbridge.c 4 files changed, 40 insertions(+), 41 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Furquan Shaikh: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve
diff --git a/src/soc/amd/common/block/include/amdblocks/spi.h b/src/soc/amd/common/block/include/amdblocks/spi.h index d226e0c..fa52da7 100644 --- a/src/soc/amd/common/block/include/amdblocks/spi.h +++ b/src/soc/amd/common/block/include/amdblocks/spi.h @@ -3,6 +3,8 @@ #ifndef __AMDBLOCKS_SPI_H__ #define __AMDBLOCKS_SPI_H__
+#include <stdint.h> + #define SPI_CNTRL0 0x00 #define SPI_BUSY BIT(31)
@@ -94,4 +96,10 @@ */ void fch_spi_config_modes(void);
+/* Set the SPI base address variable */ +void spi_set_base(void *base); + +/* Get the SPI base address variable's value */ +uintptr_t spi_get_bar(void); + #endif /* __AMDBLOCKS_SPI_H__ */ diff --git a/src/soc/amd/common/block/spi/fch_spi.c b/src/soc/amd/common/block/spi/fch_spi.c index bf64c3f..bac1452 100644 --- a/src/soc/amd/common/block/spi/fch_spi.c +++ b/src/soc/amd/common/block/spi/fch_spi.c @@ -4,28 +4,30 @@ #include <amdblocks/lpc.h> #include <amdblocks/spi.h> #include <arch/mmio.h> +#include <assert.h> #include <console/console.h> #include <soc/iomap.h> #include <stdint.h>
-static uintptr_t fch_spi_base(void) +static uintptr_t spi_base; + +void spi_set_base(void *base) { - uintptr_t base; + spi_base = (uintptr_t)base; +}
- base = lpc_get_spibase(); +uintptr_t spi_get_bar(void) +{ + if (ENV_X86 && !spi_base) + spi_set_base((void *)lpc_get_spibase()); + ASSERT(spi_base);
- if (base) - return base; - - lpc_set_spibase(SPI_BASE_ADDRESS); - lpc_enable_spi_rom(SPI_ROM_ENABLE); - - return SPI_BASE_ADDRESS; + return spi_base; }
static void fch_spi_set_spi100(int norm, int fast, int alt, int tpm) { - uintptr_t base = fch_spi_base(); + uintptr_t base = spi_get_bar();
write16((void *)(base + SPI100_SPEED_CONFIG), SPI_SPEED_CFG(norm, fast, alt, tpm)); write16((void *)(base + SPI100_ENABLE), SPI_USE_SPI100); @@ -33,7 +35,7 @@
static void fch_spi_disable_4dw_burst(void) { - uintptr_t base = fch_spi_base(); + uintptr_t base = spi_get_bar(); uint16_t val = read16((void *)(base + SPI100_HOST_PREF_CONFIG));
write16((void *)(base + SPI100_HOST_PREF_CONFIG), val & ~SPI_RD4DW_EN_HOST); @@ -41,7 +43,7 @@
static void fch_spi_set_read_mode(u32 mode) { - uintptr_t base = fch_spi_base(); + uintptr_t base = spi_get_bar(); uint32_t val = read32((void *)(base + SPI_CNTRL0)) & ~SPI_READ_MODE_MASK;
write32((void *)(base + SPI_CNTRL0), val | SPI_READ_MODE(mode)); @@ -77,7 +79,6 @@
void fch_spi_early_init(void) { - lpc_set_spibase(SPI_BASE_ADDRESS); lpc_enable_spi_rom(SPI_ROM_ENABLE); lpc_enable_spi_prefetch(); fch_spi_disable_4dw_burst(); diff --git a/src/soc/amd/common/block/spi/fch_spi_ctrl.c b/src/soc/amd/common/block/spi/fch_spi_ctrl.c index 13ad0cd..0be6b0e 100644 --- a/src/soc/amd/common/block/spi/fch_spi_ctrl.c +++ b/src/soc/amd/common/block/spi/fch_spi_ctrl.c @@ -30,26 +30,24 @@ #define SPI_FIFO_RD_PTR_SHIFT 16 #define SPI_FIFO_RD_PTR_MASK 0x7f
-static uint32_t spibar; - -static inline uint8_t spi_read8(uint8_t reg) +static uint8_t spi_read8(uint8_t reg) { - return read8((void *)(spibar + reg)); + return read8((void *)(spi_get_bar() + reg)); }
-static inline uint32_t spi_read32(uint8_t reg) +static uint32_t spi_read32(uint8_t reg) { - return read32((void *)(spibar + reg)); + return read32((void *)(spi_get_bar() + reg)); }
-static inline void spi_write8(uint8_t reg, uint8_t val) +static void spi_write8(uint8_t reg, uint8_t val) { - write8((void *)(spibar + reg), val); + write8((void *)(spi_get_bar() + reg), val); }
-static inline void spi_write32(uint8_t reg, uint32_t val) +static void spi_write32(uint8_t reg, uint32_t val) { - write32((void *)(spibar + reg), val); + write32((void *)(spi_get_bar() + reg), val); }
static void dump_state(const char *str, u8 phase) @@ -64,7 +62,7 @@ printk(BIOS_DEBUG, "Cntrl0: %x\n", spi_read32(SPI_CNTRL0)); printk(BIOS_DEBUG, "Status: %x\n", spi_read32(SPI_STATUS));
- addr = spibar + SPI_FIFO; + addr = spi_get_bar() + SPI_FIFO; if (phase == 0) { dump_size = spi_read8(SPI_TX_BYTE_COUNT); printk(BIOS_DEBUG, "TxByteCount: %x\n", dump_size); @@ -111,8 +109,7 @@
void spi_init(void) { - spibar = lpc_get_spibase(); - printk(BIOS_DEBUG, "%s: Spibar at 0x%08x\n", __func__, spibar); + printk(BIOS_DEBUG, "%s: SPI BAR at 0x%08lx\n", __func__, spi_get_bar()); }
static int spi_ctrlr_xfer(const struct spi_slave *slave, const void *dout, diff --git a/src/soc/amd/stoneyridge/southbridge.c b/src/soc/amd/stoneyridge/southbridge.c index 15219b4..534f33d 100644 --- a/src/soc/amd/stoneyridge/southbridge.c +++ b/src/soc/amd/stoneyridge/southbridge.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */
+#include <amdblocks/spi.h> #include <console/console.h> #include <device/mmio.h> #include <bootstate.h> @@ -256,25 +257,17 @@ misc_write32(MISC_CLK_CNTL1, ctrl); }
-static uintptr_t sb_init_spi_base(void) +static void sb_init_spi_base(void) { - uintptr_t base; - /* Make sure the base address is predictable */ - base = lpc_get_spibase(); - - if (base) - return base; - - lpc_set_spibase(SPI_BASE_ADDRESS); + if (ENV_X86) + lpc_set_spibase(SPI_BASE_ADDRESS); lpc_enable_spi_rom(SPI_ROM_ENABLE); - - return SPI_BASE_ADDRESS; }
void sb_set_spi100(u16 norm, u16 fast, u16 alt, u16 tpm) { - uintptr_t base = sb_init_spi_base(); + uintptr_t base = spi_get_bar(); write16((void *)(base + SPI100_SPEED_CONFIG), (norm << SPI_NORM_SPEED_NEW_SH) | (fast << SPI_FAST_SPEED_NEW_SH) | @@ -285,7 +278,7 @@
void sb_disable_4dw_burst(void) { - uintptr_t base = sb_init_spi_base(); + uintptr_t base = spi_get_bar(); write16((void *)(base + SPI100_HOST_PREF_CONFIG), read16((void *)(base + SPI100_HOST_PREF_CONFIG)) & ~SPI_RD4DW_EN_HOST); @@ -293,7 +286,7 @@
void sb_read_mode(u32 mode) { - uintptr_t base = sb_init_spi_base(); + uintptr_t base = spi_get_bar(); write32((void *)(base + SPI_CNTRL0), (read32((void *)(base + SPI_CNTRL0)) & ~SPI_READ_MODE_MASK) | mode);
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Felix Held (Code Review)
-
Furquan Shaikh (Code Review)
-
Kyösti Mälkki (Code Review)
-
Martin Roth (Code Review)
-
Patrick Georgi (Code Review)
-
Raul Rangel (Code Review)