Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44196 )
Change subject: mrc_cache: Update mrc_cache data in romstage ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44196/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44196/8//COMMIT_MSG@16 PS8, Line 16: Updating spi driver Kconfig to set : BOOT_DEVICE_SPI_FLASH_RW_NOMMAP_EARLY=y if both : BOOT_DEVICE_SPI_FLASH_RW_NOMMAP=y and BOOT_DEVICE_SPI_FLASH=y to bring : the appropriate libraries into romstage.
MRC_STASH_TO_CBMEM which will be auto-selected if MRC_WRITE_NV_LATE or !BOOT_DEVICE_SPI_FLASH_RW_NOMMAP_EARLY.
Let's be careful not to create a situation where a new SoC generation could accidentally fall into the ramstage path just because it doesn't select BOOT_DEVICE_SPI_FLASH_RW_NOMMAP_EARLY. So adding that option sounds okay and MRC_WRITE_NV_LATE can select it, but it shouldn't be automatic from !NOMMAP_EARLY.
Or, alternatively, how about flipping NOMMAP_EARLY around into BOOT_DEVICE_SPI_FLASH_NO_EARLY_WRITES? Then we could make all those old chipsets select that, make the MRC cache behavior auto-select based on that, and all newly added SoCs would get the early write ability (for MRC cache and whatever else they may need) automatically without needing another boilerplate config line. I always think it's better to build these things so the stuff that you expect everything going forward to support is the default.
If for some reason the resulting timings aren't good enough, the boards might hang randomly. If the board hangs in ramstage, it's best to try initializing the memory from scratch again.
Yikes, that sounds... terrifying. ^^ Can't we come up with a better way to detect if the trained values are actually good than "let's see if ramstage crashes" (like primitive_memtest() or something)? But okay, sounds like we need to special case those boards anyway because of the issue Furquan mentioned.