Richard Spiegel 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 4:
(6 comments)
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.
Common header would need to be spi-generic.h, so it will be a separate patch.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... PS2, Line 65: special_spi
non-standard?
ok, valid. Done.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... PS2, Line 94: crop_chunk
stick with spi_crop_chunk?
Would conflict with same function at drivers/spi/spi_flash.c as both are declared in headers, and both headers (spi_flash.h and fch_spi.h are used by all code files within common/amdblocks/spi. So it must have a different name. If you want spi as part of the name, it must have something else to differentiate. maybe fch_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?
Ack
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?
Yes. Initially I did not, and it was causing a endless loop when writing more bytes than SPI_FIFO_DEPTH.
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?
Ack