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 2:
(18 comments)
This isn't a 100% review.
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:
Only datasheet used were Family 15h BKDG revision 3.06 (public) and Family 17h (NDA). […]
Please update the commit message per Paul's request. The gerrit comment means nothing.
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.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. Why not just make a macro for ROM_PROTECT_RANGE_REG(n) and a #define for MAX_ROM_PROTECT_RANGES.
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
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)
Arguably, you could simply check if (!reg32).
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 422: Know "Known"
Is this really an issue? That makes it sounds like a chipset bug. Why not just state that the registers are write-once, and therefore the implementation naturally follows.
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. If you leave it non-bool, how about renaming it so it's obvious what a value of 1 is, e.g. gran64.
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 count on the register having been programmed. Why not simply use CONFIG_COREBOOT_ROMSIZE_KB and calculate it?
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
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 440: & UPPER_16 can't you skip this?
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 return an error if the region is too low.
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 values are >= 64K? How about comparing (range_base != ALIGN_DOWN(range_base, 4 * KiB).
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. I would much rather know _why_ we're anding; I assume because the max is 16MB. FWIW, if you check for legal regions, this could probably go away.
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". Also, is it necessary to point out that reg32 is the value?
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. Don't we need a completely different reg32? A better comment might help.
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% of the flash if WP or RP, regardless of what the range was. The documentation looks like this blocks the MAC from issuing the commands, which doesn't seem to be quite what you want. (While we'd probably want to ensure the MAC can't write a region we're protecting, I believe this only protects the top 512K what AMD calls "BIOS space".)
Once you write the D14F3x[5c,58,54,50] registers, the SPI controller should ignore the commands from the x86 already.
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?
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 489: SPI_CNTRL0 MMIO