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.