Julius Werner 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 4:
(8 comments)
This change is ready for review.
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.
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 too (and from all mainboards that select it).
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
https://review.coreboot.org/c/coreboot/+/46855/4/src/drivers/mrc_cache/mrc_c... PS4, Line 88: .flags = RECOVERY_FLAG, Same here
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 check. lookup_region() will automatically fail to find a valid cache in recovery mode when !VBOOT_STARTS_IN_ROMSTAGE.
https://review.coreboot.org/c/coreboot/+/46855/4/src/drivers/mrc_cache/mrc_c... PS4, Line 659: CONFIG(HAS_RECOVERY_MRC_CACHE In (!A || (A && B)) the second A is redundant, you can just write (!A || B)
But is this actually correct? For the old boards that do vboot in romstage, they also don't have HAS_RECOVERY_MRC_CACHE set... so this would be true for them and their normal cache would be cleared, even though they always do memory training in RO. That doesn't sound right and I don't think it matches the current behavior either.
I think what we want here is
if (!CONFIG(VBOOT_STARTS_IN_ROMSTAGE) && !CONFIG(HAS_RECOVERY_MRC_CACHE))
(Also, remember to not use CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) as a synonym for !CONFIG(VBOOT_STARTS_IN_ROMSTAGE)... we also have CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK) now, so the two aren't equivalent anymore. The two cases we want to distinguish here is in-romstage and the rest.)
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. I think what you actually need to do is remove the condition completely (and always call prepare_mrc_cache() unconditionally)?
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.