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 3:
(18 comments)
g
https://review.coreboot.org/c/coreboot/+/35018/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35018/2//COMMIT_MSG@11 PS2, Line 11:
Please update the commit message per Paul's request. The gerrit comment means nothing.
Done, but only the Family 15h public version. Only reason it has not been out was that I was waiting for more reviews for a single patch.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/fch_spi.h:
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... PS2, Line 21:
Add a comment that these registers are in D14F3. OTOH why not add them to lpc. […]
I didn't want to change any file outside what I included. Moved to lpc.h.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... PS2, Line 30: #define ROM_PROTECT_SKIP (ROM_PROTECT_RANGE1 - ROM_PROTECT_RANGE0) : #define ROM_PROTECT_LIMIT (ROM_PROTECT_RANGE3 + ROM_PROTECT_SKIP)
This really makes the c confusing. […]
All I really need is the limit, though your suggestion has merit if I also change the code using it. Done.
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 402: /* fin
add a blank line
Done
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 405: reg32 & ROM_BASE_MASK)
ALIGN_DOWN(reg32, 4 * KiB) […]
ALIGN_DOWN(reg32, 4 * KiB)? Instead of the single AND operation? I don't like it. Simply (!reg32). I need to double check, but if reserved bits read as 0 (I have found reserved bit set to 1 at reset before) you are correct.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 422: Know
"Known" […]
It's the restricted commands that are the problem, once written without a read command, a read command can't be added... and no, it's not a bug, just a warning. Changed from "Known issue" do "Warning". The one problem is that I don't know if adding the read command as a restricted command will cause problem if the read protect is not enabled for the range. If that is true, than I could simply add all commands and be done with it, without this warning.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 434: granularity
This looks like it's written to essentially be a boolean. […]
Done
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 439: ROM_ADDRESS_RANGE2_START
Are you assuming this will contain the base address of the flash device? I don't think you should c […]
It's definitely programmed with grunt. And if not programmed and I use the calculation as you define, protection would fall on the wrong place due to mismatch between registers.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 440: 16
Would prefer to see this as a #define
Done
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 440: & UPPER_16
can't you skip this?
Playing safe.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 441: %
Is it possible for a caller to request a region below rom_base? Maybe give it a quick check and ret […]
Good idea, done.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 444: GRANULARITY_TEST_4k & range_base
You're anding range_base and len with 0xf000; won't you get false positives it either of those value […]
No false positives. Even if address or size was greater than 64K, if any of bits 15-12 is set than it has to be 4K granularity. The worst that could happen would be if for example, addr = ((n * 64K) - 4K) and size 68K. This could be solved with a 4K range and a 64K range... but my code is not smart enough to realize it. It would do the first as 4K (as it should), but the remaining 64K would be broken into 4K chunks and there would be not enough regions. OTOH, if any bit 11 through 0 is set, that's an error I'm not checking and results would be unpredictable.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 458: /* Bits 7-0 */
I don't think this comment adds much for the reader. […]
This is needed, it's a mask. Declared the mask value and used it here.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 460: programed
sp, but BTW I would reword the whole comment so you don't use two forms of "program". […]
Done
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 474: reg32++; /* next range */
Are you sure? Because it looks wrong. […]
Completely sure. I'm increasing the range address (lower 8 bits) to point to next range (4K or 64K depending on granularity).
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 477: /* define commands to be blocked if in range */
Are your certain about this? With no comments, it looks like you're precluding the commands to 100% […]
It's all needed. From BKDG: Step 1 enables protection for direct memory access to SPI (Direct memory access is when Host access SPI memory using normal Read / Write commands these access are converted to SPI Read and Write commands by the SPI controller). Step 2 enables protection for indirect memory access to the SPI controller. Indirect memory accesses are when the SPIx00 [SPI_Cntrl0] register is used to send SPI commands to the SPI ROM. Step 3 By programming the SPIx00[SpiAccessMacRomEn] = 0, all SPI commands (defined in register SPIx14 [SPI_CmdValue1] and SPIx18 [SPI_CmdValue2]) and Restricted commands (defined in SPIx04 [SPI_RestrictedCmd] and SPIx08 [SPI_RestrictedCmd2]) are protected.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 488: SPI_RESTRICTED_CMD1
Isn't this an MMIO register?
Correct, fixed.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 489: SPI_CNTRL0
MMIO
Correct, fixed.