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 7:
(15 comments)
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/Kconfig:
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 6: This overwrites the structure spi_flash_ops to use FCH SPI code instead of individual
I would break this line at 80. I don't believe making it longer improves readability.
will do.
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 474: reg32++; /* next range */
The way I read it, the Range field is more like a size indicator that's applicable to RomBase. […]
Nope, I'm protecting 2 adjacent regions. Range points to the start address of a region. The range value must be multiplied by the granularity (which is also the size of the region) to get the actual offset from the SPI start address. Will add the above as a comment.
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 53: u32 addr;
Maybe add a blank line after your variables.
Will do.
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 93: E
Kind of a nit, but I'd make the case match in all the strings in this function. […]
Will do.
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 109: (u32)
Why not remove this cast and change the format string instead?
Will do.
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 137: this sequence is controlled : * by the SPI chip driver.
Does this still make sense? Or should it refer to a different file now?
I believe that I should just remove the section after the comma.
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 214: If some specialized SPI flash controllers : * (e.g. x86) can perform both command and response together, it should : * be handled at SPI flash controller driver level
I don't follow what this means. […]
Left over from copy/paste. Will remove.
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)
It's used by fch_spi_special. […]
Missed, next patch
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 50: u8 buff[SPI_FIFO_DEPTH + 1];
check not needed, it'll never be due to fch_spi_crop_chunk being called before this function is call […]
Done
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,
It's almost identical, only difference is protection if write address don't start on a word boundary […]
Done
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);
fch_spi_special was written as it was because I was planning for any weird flash that might show up […]
Done
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_flash.c:
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 40: cmd_len--;
Seems like a complex way to just write? […]
With an extra parenthesis, but I believe you're right. Will do.
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 184: if (spi_data_ptr->non_standard != NON_STANDARD_SPI_NONE)
isn't this essentially a duplicate check to the branch in non_standard_spi_write() ? […]
Modified already, you'll see when I post my next patch.
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)
Done this way if we ever get a new weird flash that also need its own function.
Done
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_special.c:
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 117: int ret = 0;
not sure if you missed this one but you can inline `non_standard_sst_write_aai()` here and pass `spi […]
This code don't exists anymore, you'see on my next patch.