Shelley Chen 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 10:
(23 comments)
https://review.coreboot.org/c/coreboot/+/46855/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46855/3//COMMIT_MSG@9 PS3, Line 9: Currently forced memory retraining is handled in fsp 2.0
Thanks for the per-case explanation, I really like it! With this in mind, the change makes lots of s […]
Ack
https://review.coreboot.org/c/coreboot/+/46855/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46855/4//COMMIT_MSG@16 PS4, Line 16: mrc_cache
I think this is missing a "for recovery mode" or so (and a period) for the sentence to be clear.
Done
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
Done
https://review.coreboot.org/c/coreboot/+/46855/5//COMMIT_MSG@9 PS5, Line 9: make
made
Done
https://review.coreboot.org/c/coreboot/+/46855/2/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/46855/2/src/drivers/intel/fsp2_0/me... PS2, Line 101: if (!CONFIG(HAS_RECOVERY_MRC_CACHE))
In order to keep behavior the same here, you need to now select the new option from fsp2_0/Kconfig.
Done
https://review.coreboot.org/c/coreboot/+/46855/2/src/drivers/mrc_cache/Kconf... File src/drivers/mrc_cache/Kconfig:
https://review.coreboot.org/c/coreboot/+/46855/2/src/drivers/mrc_cache/Kconf... PS2, Line 67: and MRC_HAS_RECOVERY_MRC_CACHE is not selected
Should also clarify what happens if this option is not selected (recovery sharing the normal cache).
Removed this config.
https://review.coreboot.org/c/coreboot/+/46855/4/src/drivers/mrc_cache/Kconf... File src/drivers/mrc_cache/Kconfig:
https://review.coreboot.org/c/coreboot/+/46855/4/src/drivers/mrc_cache/Kconf... PS4, Line 23: default y if !VBOOT_STARTS_IN_ROMSTAGE
In the code it looks like you're removing this Kconfig completely? Then you should remove it here to […]
Done
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 beca […]
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/46855/2/src/drivers/mrc_cache/mrc_c... PS2, Line 266: !CONFIG(HAS_RECOVERY_MRC_CACHE))
The second check is redundant because the Kconfig dependencies enforce that anyway. […]
Reorganized this logic.
https://review.coreboot.org/c/coreboot/+/46855/4/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46855/4/src/drivers/mrc_cache/mrc_c... PS4, Line 75: RECOVERY_FLAG
Nono, this needs to be NORMAL_FLAG only, not RECOVERY_FLAG
Done
https://review.coreboot.org/c/coreboot/+/46855/4/src/drivers/mrc_cache/mrc_c... PS4, Line 88: .flags = RECOVERY_FLAG,
Same here
Done
https://review.coreboot.org/c/coreboot/+/46855/4/src/drivers/mrc_cache/mrc_c... PS4, Line 275: if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK))
If you do the things with flags in the cache_regions above (correctly), then you shouldn't need this […]
Removed.
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_RO […]
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/46855/6/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46855/6/src/drivers/mrc_cache/mrc_c... PS6, Line 659: const struct cache_region *cr; : : cr = lookup_region_type(type);
Not for this change, but I think this whole `lookup_region_type()` can be moved within the if block […]
Done
https://review.coreboot.org/c/coreboot/+/46855/6/src/drivers/mrc_cache/mrc_c... PS6, Line 663: failed to
I think this should say "skipped" because it is not necessarily a failure if the region type is not […]
Done
https://review.coreboot.org/c/coreboot/+/46855/6/src/drivers/mrc_cache/mrc_c... PS6, Line 665: 0
It is confusing that this returns 0 even though lookup_region_type() returns NULL. […]
So, I think that it's ok that we return 0 (success) because since there's no region type returned, then there's no need to stash the mrc_cache data, right? Perhaps we need to change the printout to say no region type returned, so skipping stashing mrc_data?
I do think that we should keep the return type though. Otherwise how will we indicate errors?
https://review.coreboot.org/c/coreboot/+/46855/8/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46855/8/src/drivers/mrc_cache/mrc_c... PS8, Line 609: so it's safe to use the mrc_cache : * data.
This path is not using the cache data. […]
Done
https://review.coreboot.org/c/coreboot/+/46855/8/src/drivers/mrc_cache/mrc_c... PS8, Line 616: invalidate when : * retrain switch is set.
Probably invert the comment i.e. no need to invalidate the cache if retrain switch is not set. […]
Done
https://review.coreboot.org/c/coreboot/+/46855/8/src/drivers/mrc_cache/mrc_c... PS8, Line 695: 0
Please see comment on patchset 6.
still under discussion I think? Let's continue discussing over there.
https://review.coreboot.org/c/coreboot/+/46855/4/src/northbridge/intel/haswe... File src/northbridge/intel/haswell/raminit.c:
https://review.coreboot.org/c/coreboot/+/46855/4/src/northbridge/intel/haswe... PS4, Line 118: if (pei_data->boot_mode == 2)
Careful, that was an OR and you just removed the first case now. […]
Done
https://review.coreboot.org/c/coreboot/+/46855/4/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_mrc.c:
https://review.coreboot.org/c/coreboot/+/46855/4/src/northbridge/intel/sandy... PS4, Line 138: if (pei_data->boot_mode == 2)
Same here.
Done