Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37957 )
Change subject: soc/amd/common/block/spi: remove code duplication ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37957/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37957/4//COMMIT_MSG@13 PS4, Line 13: We can add this later.
I don't yet see what's missing here. […]
The spi controller protection needs some help. See my TODOs in the file. I think that support needs another investigation because when reading the BKDG I don't think it's working the way it was originally written.
Eric did test a quickly yesterday. This code could definitely use some more testing, but I'm pretty sure it should work.
https://review.coreboot.org/c/coreboot/+/37957/4//COMMIT_MSG@17 PS4, Line 17:
Remove the blank line?
ok
https://review.coreboot.org/c/coreboot/+/37957/4/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/37957/4/src/soc/amd/common/block/sp... PS4, Line 274: printk(BIOS_INFO, "%s: Write Enable and Write Cmd not blocked\n", __func__); Marshall, see this diff. Because these commands aren't being exposed in the general support one can't program the registers like they were originally. That said, I'm not convinced this code is correct when reading the datasheet. We need to do some more investigation in the behavior of the chipset on this as it talks about blocking certain commands for address ranges. But I don't know for certain if the controller is interrogating the first 3 bytes in the fifo for address matching or not. Likewise the BKDG also talks about CPU reads and writes being converted into SPI transactions. Reading makes sense because of the memory map, but I find the write conversions to be interesting in that one has to erase to make the write be valid. I'm not sure how the chipset works in this area.