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, Even after flipping the stuff, there are some differences in behavior:
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 in bootblock:
a. FSP2.0 platforms (MRC_ALWAYS_RETRAIN_IN_RECOVERY is 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!!) 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!!) - Update data in normal mode cache based on current training.
3. Platforms using vboot after memory training (MRC_ALWAYS_RETRAIN_IN_RECOVERY is not selected):
- In recovery mode, these platforms will load training data from normal mode cache. - Update data in normal mode cache based on current training.
I think 2a and 2b are still not the same as before.
You say "vboot before bootblock", but I think you meant "vboot in bootblock" vs. "vboot in romstage", right?
Yes, you are right. Sorry, I was thinking "vboot in bootblock" but wrote "vboot before bootblock".
If we're okay with tying this 1-to-1 to where vboot runs we can do that as well, then we wouldn't need an extra Kconfig.
I think the requirement we have is: 1. Platforms with RECOVERY_MRC_CACHE: - Load data from recovery cache. - Update data to recovery cache. - Invalidate normal cache if retrain switch is set.
2. Platforms without RECOVERY_MRC_CACHE, but doing memory training after vboot is run (VBOOT_STARTS_IN_BOOTBLOCK): - Training data is not used. - Invalidate normal cache.
3. Platforms without RECOVERY_MRC_CACHE, but doing memory training before vboot is run (VBOOT_STARTS_IN_ROMSTAGE): - Load data from normal cache. - Update data to normal cache.
Based on that, I think it is okay to have 1-to-1 dependency on where vboot runs. i.e. #if CONFIG(VBOOT_STARTS_IN_ROMSTAGE) .flags = NORMAL_FLAG | RECOVERY_FLAG, #else .flags = NORMAL_FLAG, #endif
But, we will need one additional change: MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN must be set whenever !VBOOT_STARTS_IN_ROMSTAGE. And `invalidate_normal_cache()` must erase normal mode cache when: 1. Device is in recovery mode for platforms without RECOVERY_MRC_CACHE or 2. Device is in recovery mode and retrain switch is set for platforms with RECOVERY_MRC_CACHE.
This is to ensure that we retain the behavior of invalidating normal cache for platforms of type 2 above i.e. without recovery cache but perform memory training after vboot is run.
Also, `mrc_cache_stash_data()` and `update_mrc_cache_from_cbmem()` will have to be updated to not BIOS_ERR if cache region is not found.