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.