Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/28719 )
Change subject: sb/amd/pi/hudson: Add SPI controller support ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/28719/5/src/southbridge/amd/pi/huds... File src/southbridge/amd/pi/hudson/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/28719/5/src/southbridge/amd/pi/huds... PS5, Line 31: bootblock Did you consider all-$()? For any system with elog support, then it can also be good to have spi in smm as well. Or, it looks like in soc/amd/common/block/spi, it looks like we've qualified it with CONFIG_SPI_FLASH_SMM.
https://review.coreboot.org/c/coreboot/+/28719/5/src/southbridge/amd/pi/huds... File src/southbridge/amd/pi/hudson/pci_devs.h:
https://review.coreboot.org/c/coreboot/+/28719/5/src/southbridge/amd/pi/huds... PS5, Line 18: Was this file added to the commit by accident? It doesn't seem to fit with adding SPI.
https://review.coreboot.org/c/coreboot/+/28719/5/src/southbridge/amd/pi/huds... File src/southbridge/amd/pi/hudson/spi.c:
https://review.coreboot.org/c/coreboot/+/28719/5/src/southbridge/amd/pi/huds... PS5, Line 158: spi_write8(SPI_EXT_REG_INDX, SPI_EXT_REG_TXCOUNT); : spi_write8(SPI_EXT_REG_DATA, bytesout); Hmm, which device did you write this for? It looks like, for example, that Hudson 1 is completely different than Kabini. As a result, I think a lot of the definitions should be looked at more closely. Maybe break the steps out into their own functions?
-
Marshall Dawson (Code Review)