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:
(9 comments)
Hi Richard, this is my first pass. Please kindly look over my suggestions. The common theme here is to trim off some bulk in repeated loops and cluttering the symbol table with unnecessary global state.
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 27: extern const struct spi_flash_ops fch_spi_flash_ops; see below to get rid of this global state.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 29: struct spi_data ctrl_spi_data; can be static with changes suggested below.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 248: * The following table holds all device probe functions Let's move this table into it's own file and expose flashes[] as const and flash_count = ARRAY_SIZE() as const also.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 380: flash->ops = &fch_spi_flash_ops; `fch_spi_flash_ops_init(flash);`
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 173: static int fch_spi_flash_write(const struct spi_flash *flash, uint32_t offset, size_t len, If I am not mistaken `non_standard_sst_write_aai()` is almost identical to `fch_spi_flash_write()` here you set `cmd[0] = ctrl_spi_data.write_cmd;` in terms of the main loop with a few random things at the start of `non_standard_sst_write_aai()`.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 184: return non_standard_spi_write(offset, len, buf); I think it best to remove the main loop from `non_standard_sst_write_aai()` and just figure out what things need to be set around this branching here, like `cmd[0]`.
With those changes that should trim down fch_spi_special.c quite some!
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 230: const struct spi_flash_ops fch_spi_flash_ops = { Can now be static
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 236: void fch_spi_flash_ops_init(struct spi_flash *flash) { flash->ops = &fch_spi_flash_ops; }
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_special.c:
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 115: int non_standard_spi_write(uint32_t offset, size_t len, const void *buf) This function is unnecessary indirection, just called `non_standard_sst_write_aai()` directly from `fch_spi_flash_write()` move all the branch logic into `non_standard_sst_write_aai()`s preamble and pass a const ref to ctrl_spi_data as an argument so we can drop the extern.