Marshall Dawson 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:
(7 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.
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 */
Completely sure. […]
The way I read it, the Range field is more like a size indicator that's applicable to RomBase. To me, when you say "point to next range", that sounds like an iterator. So it seems to me you're protecting overlapping regions.
Looks like you didn't update the 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.
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. FWIW, I prefer lower case here.
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?
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?
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. Is this not the SPI flash controller driver? And why mention "specialized SPI flash controllers"?