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