Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32652 )
Change subject: soc/amd/stoneyridge: Rework SPI base address get/set ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/#/c/32652/4/src/soc/amd/stoneyridge/include/soc/... File src/soc/amd/stoneyridge/include/soc/southbridge.h:
https://review.coreboot.org/#/c/32652/4/src/soc/amd/stoneyridge/include/soc/... PS4, Line 360: 32
Shouldn't this be 64 to ensure bit 5 is also preserved?
Done
https://review.coreboot.org/#/c/32652/4/src/soc/amd/stoneyridge/southbridge.... File src/soc/amd/stoneyridge/southbridge.c:
https://review.coreboot.org/#/c/32652/4/src/soc/amd/stoneyridge/southbridge.... PS4, Line 410: sb_set_spibase
I am guessing this is going to be called by more than just sb_init_spi_base() in the future?
While possible, I don't have any particular plans. Separating it was deliberate, though. The SPI controller's base address & enables are in the LPC controller's non-standard PCI config space. So, it's done with the intent of moving to a different file.
https://review.coreboot.org/#/c/32652/4/src/soc/amd/stoneyridge/southbridge.... PS4, Line 414: /* only two types of CS# enables are allowed */
This is a difference in behavior than before. […]
Unless I goofed it, it should produce an identical result as before, but be more flexible. (I ran a quick test on Grunt and it seems to be true.) Rather than always enabling only SPI_ROM_ENABLE (b1), it allows a caller to also use SPI_ROM_ALT_ENABLE (b0).