Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46855 )
Change subject: mrc_cache: Move force memory retraining code into mrc_cache ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/coreboot/+/46855/2/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/46855/2/src/drivers/intel/fsp2_0/me... PS2, Line 101: if (!CONFIG(HAS_RECOVERY_MRC_CACHE)) In order to keep behavior the same here, you need to now select the new option from fsp2_0/Kconfig.
https://review.coreboot.org/c/coreboot/+/46855/2/src/drivers/mrc_cache/Kconf... File src/drivers/mrc_cache/Kconfig:
https://review.coreboot.org/c/coreboot/+/46855/2/src/drivers/mrc_cache/Kconf... PS2, Line 67: and MRC_HAS_RECOVERY_MRC_CACHE is not selected Should also clarify what happens if this option is not selected (recovery sharing the normal cache).
https://review.coreboot.org/c/coreboot/+/46855/1/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46855/1/src/drivers/mrc_cache/mrc_c... PS1, Line 264: if (!CONFIG(HAS_RECOVERY_MRC_CACHE))
Ok, I added the CONFIG_MRC_ALWAYS_RETRAIN_IN_RECOVERY config and it seems to work just fine with jus […]
See comment in new Patch Set (line 72).
https://review.coreboot.org/c/coreboot/+/46855/2/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46855/2/src/drivers/mrc_cache/mrc_c... PS2, Line 72: .flags = NORMAL_FLAG | RECOVERY_FLAG, I think it would be better to introduce the new Kconfig option here, e.g.
#if CONFIG(MRC_ALWAYS_RETRAIN_IN_RECOVERY) .flags = NORMAL_FLAG | RECOVERY_FLAG, #else .flags = NORMAL_FLAG, #endif
(same for variable_data). What this does is make lookup_region() always return 0 in recovery mode. Then you don't need the separate check in mrc_cache_find_current() anymore, and more importantly, it will also block things like update_mrc_cache_by_type(). The ALWAYS_RETRAIN case is different from the retrain_switch case, because for retrain_switch we still want the new training results to be written back to the recovery cache afterwards, but for ALWAYS_RETRAIN I think it's better to not write it back anywhere.
https://review.coreboot.org/c/coreboot/+/46855/2/src/drivers/mrc_cache/mrc_c... PS2, Line 266: !CONFIG(HAS_RECOVERY_MRC_CACHE)) The second check is redundant because the Kconfig dependencies enforce that anyway. If you want to double-check that Kconfig dependencies were correctly enforced (because sometimes when two options are 'select'ed at the same time it doesn't), it would be better to add a _Static_assert(!CONFIG(MRC_ALWAYS_RETRAIN_IN_RECOVERY) || !CONFIG(HAS_RECOVERY_MRC_CACHE)) to the top of this file (although I think these days Kconfig itself will also throw an error for those cases so I'm not sure if that kind of extra check is still necessary anymore).