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?