Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46855 )
Change subject: mrc_cache: Move code for triggering memory training into mrc_cache ......................................................................
Patch Set 6:
(5 comments)
https://review.coreboot.org/c/coreboot/+/46855/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46855/5//COMMIT_MSG@9 PS5, Line 9: in recovery mode
https://review.coreboot.org/c/coreboot/+/46855/5//COMMIT_MSG@9 PS5, Line 9: make made
https://review.coreboot.org/c/coreboot/+/46855/5/src/drivers/mrc_cache/Kconf... File src/drivers/mrc_cache/Kconfig:
https://review.coreboot.org/c/coreboot/+/46855/5/src/drivers/mrc_cache/Kconf... PS5, Line 20: MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN Can you please add to commit message that MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN is dropped because the mrc_cache driver ensures that the MRC cache for normal mode is invalidated in the following cases: 1. For platforms not using recovery MRC cache --> All boots in recovery mode 2. For platforms using recovery MRC cache --> Recovery mode boot with retrain switch set.
https://review.coreboot.org/c/coreboot/+/46855/5/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46855/5/src/drivers/mrc_cache/mrc_c... PS5, Line 72: #if CONFIG(VBOOT_STARTS_IN_ROMSTAGE) Can you please add a comment here explaining why the flags are different based on VBOOT_STARTS_IN_ROMSTAGE?
https://review.coreboot.org/c/coreboot/+/46855/5/src/drivers/mrc_cache/mrc_c... PS5, Line 649: if (vboot_recovery_mode_enabled() && : !CONFIG(HAS_RECOVERY_MRC_CACHE) && !CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) I don't think this is correct. This will never invalidate the normal cache for platforms with HAS_RECOVERY_MRC_CACHE.
There are two cases that need to be handled:
1. Platforms with recovery MRC cache --> invalidate when retrain switch is set in recovery mode 2. Platforms without recovery MRC cache --> invalidate when in recovery mode if !VBOOT_STARTS_IN_ROMSTAGE
Also, invalidate_normal_cache() already has some checks. I think it would be better to make the call to `invalidate_normal_cache()` unconditionally here and then use the following checks in that function:
``` static void invalidate_normal_cache(void) { ... /* If not in recovery mode, do not invalidate cache. */ if (!vboot_recovery_mode_enabled()) return;
/* * If vboot runs in romstage (i.e. after memory training), * there is no need to invalidate cache as memory training * is performed always in RO component. */ if (CONFIG(VBOOT_STARTS_IN_ROMSTAGE) && !CONFIG(HAS_RECOVERY_MRC_CACHE)) return;
/* * For platforms that have separate recovery MRC cache, invalidate * normal mode cache only when retrain switch is set. */ if (CONFIG(HAS_RECOVERY_MRC_CACHE) && !get_recovery_mode_retrain_switch()) return; ... } ```
In the second check, !CONFIG(HAS_RECOVERY_MRC_CACHE) is not really required because it is never used by platforms using VBOOT_STARTS_IN_ROMSTAGE. I think we can update the Kconfig to say depends on !HAS_RECOVERY_MRC_CACHE and drop the unnecessary check here.