Furquan Shaikh 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,
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?
It's the latter. None of the platforms without the recovery MRC cache support the retrain switch. Since there is no retrain trigger, whenever the device goes into recovery, it would result in normal cache being invalidated (as a side-effect of updating data generated in recovery mode to normal cache). Only when we added the recovery MRC cache, the retrain switch was introduced.
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.
It is done in the individual platform code:
FSP1.1 - https://review.coreboot.org/cgit/coreboot.git/tree/src/drivers/intel/fsp1_1/...
Haswell - https://review.coreboot.org/cgit/coreboot.git/tree/src/northbridge/intel/has...
Sandybridge -https://review.coreboot.org/cgit/coreboot.git/tree/src/northbridge/intel/san...
Broadwell - https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/broadwell/r...
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.
Yes, that makes sense to me.