Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 3:
(6 comments)
Sorry, I didn't finish my review. Here's what I had.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/fch_spi.h:
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... PS2, Line 53: #define VENDOR_ID_ADESTO 0x1f : #define VENDOR_ID_AMIC 0x37 : #define VENDOR_ID_ATMEL 0x1f : #define VENDOR_ID_EON 0x1c : #define VENDOR_ID_GIGADEVICE 0xc8 : #define VENDOR_ID_MACRONIX 0xc2 : #define VENDOR_ID_SPANSION 0x01 : #define VENDOR_ID_SST 0xbf : #define VENDOR_ID_STMICRO 0x20 : #define VENDOR_ID_STMICRO_FF 0xff : #define VENDOR_ID_WINBOND 0xef This has nothing to do with AMD, so should probably be moved to a common header.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... PS2, Line 65: special_spi non-standard?
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... PS2, Line 94: crop_chunk stick with spi_crop_chunk?
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 110: BIOS_DEBUG Maybe BIOS_ERR?
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 147: bytesout + bytesin We really need to fit both bytes in and out into the fifo at the same time?
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 148: BIOS_DEBUG Again, maybe a higher level here than debug?