Edward O'Callaghan 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 6:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 216: int do_fch_spi_flash_cmd(const void *dout, size_t bytes_out, void *din, size_t bytes_in) call this `fch_spi_flash_cmd()`
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 238: int fch_spi_flash_cmd(u8 cmd, void *response, size_t len) It's only used in `fch_spi_flash_probe()` may as well inline it.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 312: const struct spi_data * get_ctrl_spi_data(void) { return &ctrl_spi_data; }
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 379: save_flash_data s/save_flash_data()/set_ctrl_spi_data()/g
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 381: if (idp[0] == VENDOR_ID_SST) { seems like this if block is overriding things that were discovered and stored in flash then passed in to `save_flash_data()`. Maybe store them as local state override them in `flash` and call `save_flash_data()` later on.
At the very least this takes away some global mutations on these branches and delineates those transitions though function calls.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_flash.c:
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 34: int fch_spi_crop_chunk(unsigned int cmd_len, unsigned int buf_len) static
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 50: u8 buff[SPI_FIFO_DEPTH + 1]; put a check in here that cmd_len+ data_len isn't larger than SPI_FIFO_DEPTH and if it is return -1 to the call site.
`buf` is idiomatic tiny bit more however that's not hugely important.