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:
(1 comment)
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,
a. FSP2.0 platforms (MRC_ALWAYS_RETRAIN_IN_RECOVERY is selected) - No updates to normal mode cache based on current training. (Difference in behavior!!)
Yes, that's a change, true... but I'm not so convinced the old behavior makes sense, honestly. Why would you always want to update/invalidate the RW cache after you booted in recovery mode? If the user explicitly wanted to retrain, they could use the retrain combination. But if they just go into recovery mode for other reasons, why retrain? Or is that configuration only meant for boards that don't support a separate retrain trigger?
b. Other platforms (MRC_ALWAYS_RETRAIN_IN_RECOVERY is not selected) - In recovery mode, these platforms will load training data from normal mode cache. (Difference in behavior!!)
I don't really follow this one, where do you see that behavior implemented? Sounds like you're saying that currently, if HAS_RECOVERY_MRC_CACHE is not enabled, everything (at least where vboot starts in bootblock) will always retrain from scratch in recovery mode? I don't think that's true. The FSP 2.0 boards do that, but only because this behavior is implemented explicitly in platform code (in fsp_fill_mrc_cache() in fsp2_0/memory_init.c) -- that's why FSP 2.0 would now select MRC_ALWAYS_RETRAIN_IN_RECOVERY with this patch. But other platforms wouldn't do that (unless they have similar platform-specific code, in which case we would also need to manually select the new Kconfig for them), the default behavior is to reuse the normal mode cache.
Based on that, I think it is okay to have 1-to-1 dependency on where vboot runs. i.e. [...]
Yeah, I agree with everything you said after this, it sounds good to simplify it like this if we agree that there's no need to have separate Kconfigs for everything. Just tie it straight to the vboot config. I think we can get rid of CONFIG_MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN as well, probably? That behavior would be completely decided by !VBOOT_STARTS_IN_ROMSTAGE, then.