Attention is currently required from: Jason Glenesk, Raul Rangel, Furquan Shaikh, Marshall Dawson, Felix Held. Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51748 )
Change subject: soc/amd/common: Add func to clear eSPI IO & memory decode ranges ......................................................................
Patch Set 3:
(2 comments)
File src/soc/amd/common/block/lpc/espi_util.c:
https://review.coreboot.org/c/coreboot/+/51748/comment/3bc03e9b_32f2905f PS2, Line 106: ffff
Are these left as 1s because they are reserved? I see some reserved bits in the lower 16 bits too.
Yes. The ones in the lower bits are marked reserved, but they're actually used in our code.
https://review.coreboot.org/c/coreboot/+/51748/comment/5fb33549_75a5f258 PS2, Line 108: for (idx = 0; idx < ESPI_GENERIC_IO_WIN_COUNT; idx++) { : espi_write16(ESPI_IO_RANGE_BASE(idx), 0); : espi_write8(ESPI_IO_RANGE_SIZE(idx), 0); : } : for (idx = 0; idx < ESPI_GENERIC_MMIO_WIN_COUNT; idx++) { : espi_write32(ESPI_MMIO_RANGE_BASE(idx), 0); : espi_write16(ESPI_MMIO_RANGE_SIZE(idx), 0); : }
Limiting this to clearing the enables seems reasonable to me.
I'd rather clear everything rather than just the decode bits. It may be overkill, but my reason is that when I'm looking at the register printout it's easy to tell that they're disabled when they're 0 instead of having to look at the bits in the enable register.
Since it doesn't hurt anything to clear all the registers, I'm going to leave it the way it is unless there's a stronger objection.