Marshall Dawson 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 5:
(3 comments)
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 248: /* Create reg32 to be written into a range register and program required ranges */ : reg32 = rom_base & ROM_BASE_MASK; : reg32 |= range_base; The BKDG's wording looks deceptive to me, so I'm not convinced this is correct. (I'm using the Stoney Ridge BKDG because I find it easier to read.) It looks to me like the affected address is (very simplified):
RomBase <= addr <= fn(Range)
In other words, I don't believe RomBase in D14F3x[5C,58,54,50] is the same as rom_base above...
https://review.coreboot.org/c/coreboot/+/37957/4/src/soc/amd/common/block/sp... PS4, Line 258: for (range = 0; range < total_ranges; range++) { : ret = protect_a_range(reg32); : if (ret) : return ret; : /* : * Next range (lower 8 bits). 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. : */ : reg32++; : } ...and if I'm right, then it doesn't make sense that we'd burn a register for each multiple of the RangeUnit. Each multiple should instead affect the Range field.
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. […]
Thanks for pointing this out and making me look at it again - ugh, I'd forgotten about specifying the commands in the controller. FWIW, I'd never quite believed that write cycles were bridged from the host. But I wonder if WREN is never sent when the x86 does a mem write, and that's why nothing happens. OTOH, it seems like I'd looked closely at the SPI signals years ago and didn't see any wiggling on x86 write cycles - it's hard to say now. If we have Eric double-check, he should be aware that write cycles can also be blocked at the northbridge (e.g. D18F1x80 in Stoney Ridge). Picasso surely has a similar configuration somewhere but I'd need to find it.