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,
I think it would be better to introduce the new Kconfig option here, e.g. […]
I don't understand this check completely. The different combinations of platforms are:
1. Platforms with recovery MRC cache (only FSP2.0 platforms and QC): - In recovery mode, these platforms read data from the recovery MRC cache only if retrain switch is not set. - Update data in recovery MRC cache based on current training.
2. Platforms without recovery MRC cache but using vboot before bootblock: - In recovery mode, these platforms do not load training data at all. - Update data in normal mode cache based on current training. This is done to force memory training on next non-recovery boot.
3. Platforms using vboot after memory training: - In recovery mode, load data from normal mode cache. - Update data in normal mode cache based on current training.
4. Platforms not using vboot - Doesn't matter since they will never have recovery mode true.
With the above changes for MRC_ALWAYS_RETRAIN_IN_RECOVERY (which is set only for FSP2.0 platforms without recovery MRC cache):
1. Platforms with recovery MRC cache (only FSP2.0 platforms and QC): --> No change (Considering that we still have the retrain switch check below)
2. Platforms without recovery MRC cache but using vboot before bootblock: a. FSP2.0 platforms (MRC_ALWAYS_RETRAIN_IN_RECOVERY is selected) - In recovery mode, these platforms will load data from normal mode cache. (Difference in behavior!!) - Update data in normal mode cache based on current training.
b. Other platforms (MRC_ALWAYS_RETRAIN_IN_RECOVERY is not selected) - In recovery mode, these platforms will not load training data at all. - No updates to normal mode cache based on current training. (Difference in behavior!!)
3. Platforms using vboot after memory training (MRC_ALWAYS_RETRAIN_IN_RECOVERY is not selected): - In recovery mode, these platforms will not load training data at all. (Difference in behavior!!) - No update to normal mode cache based on current training (Difference in behavior!!)
Out of these, I think the concerning change in behavior is 2a and 2b.
I think 3 is fine i.e. okay to not write back anywhere.