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 6:
(16 comments)
os
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.
Gotcha. Will do.
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.
Would need to create functions to return needed parameters. Why do you have problem with a global?
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()`
Will do.
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.
Will do.
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( […]
On original code (drivers/spi/spi_flash.c) it was done this way. I have no problem either way.
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) […]
understood... if I create a separate file for the table.
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
Will do.
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);`
Will do.
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 i […]
non_standard is only set here, no where else. Yes, page_size is overwritten if it's SST, however there's no way for save_flash_data to check 2 possible values on this particular case only (unless I moved this code into save_flash_data).
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
It's used by fch_spi_special.c
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 t […]
check not needed, it'll never be due to fch_spi_crop_chunk being called before this function is called.
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()` h […]
It's almost identical, only difference is protection if write address don't start on a word boundary (writes a single byte with a different command before continuing to write with page_size). Also, see what I wrote in your next comment.
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 […]
fch_spi_special was written as it was because I was planning for any weird flash that might show up in the future, though currently SST is the only one with special code. I don't think it's too much, you never know when something new come (though it looks like now there's a "de facto" for flash commands, which only SST does not obeys).
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
will do.
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) […]
will do.
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 ` […]
Done this way if we ever get a new weird flash that also need its own function.