Shelley Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46855 )
Change subject: mrc_cache: Move force memory retraining code into mrc_cache ......................................................................
mrc_cache: Move force memory retraining code into mrc_cache
Currently forced memory retraining is handled in fsp 2.0. Moving the code into mrc_cache so more platforms can utilize it.
BOG=b:150502246 BRANCH=None TEST=run dut-control power_state:rec_force_mrc twice on lazor ensure that memory training happens both times run dut-control power_state:rec twice on lazor ensure that memory training happens only first time
Change-Id: I3875a7b4a4ba3c1aa8a3c1507b3993036a7155fc Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/mrc_cache.c 2 files changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/46855/1
diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c index 68cc121..27e34fe 100644 --- a/src/drivers/intel/fsp2_0/memory_init.c +++ b/src/drivers/intel/fsp2_0/memory_init.c @@ -92,18 +92,6 @@ if (!CONFIG(CACHE_MRC_SETTINGS)) return;
- /* - * In recovery mode, force retraining: - * 1. Recovery cache is not supported, or - * 2. Memory retrain switch is set. - */ - if (vboot_recovery_mode_enabled()) { - if (!CONFIG(HAS_RECOVERY_MRC_CACHE)) - return; - if (get_recovery_mode_retrain_switch()) - return; - } - /* Assume boot device is memory mapped. */ assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED));
diff --git a/src/drivers/mrc_cache/mrc_cache.c b/src/drivers/mrc_cache/mrc_cache.c index 3b98dba..58fa70d 100644 --- a/src/drivers/mrc_cache/mrc_cache.c +++ b/src/drivers/mrc_cache/mrc_cache.c @@ -255,6 +255,18 @@ const size_t md_size = sizeof(*md); const bool fail_bad_data = true;
+ /* + * In recovery mode, force retraining: + * 1. Recovery cache is not supported, or + * 2. Memory retrain switch is set. + */ + if (vboot_recovery_mode_enabled()) { + if (!CONFIG(HAS_RECOVERY_MRC_CACHE)) + return -1; + if (get_recovery_mode_retrain_switch()) + return -1; + } + cr = lookup_region(®ion, type);
if (cr == NULL)
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 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46855/1/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46855/1/src/drivers/mrc_cache/mrc_c... PS1, Line 264: if (!CONFIG(HAS_RECOVERY_MRC_CACHE)) I don't think we can add this here unless we want to change policy. I think the way this driver is currently written is that by default, recovery mode will also use the normal MRC cache if HAS_RECOVERY_MRC_CACHE is not set. That's why the normal_training region has both NORMAL_FLAG and RECOVERY_FLAG set, and the cache_regions[] array is carefully ordered so the explicit recovery cache (if it exists) comes first.
So either this check will have to stay in the platform code, or we need to turn it into a generic Kconfig (e.g. CONFIG_MRC_ALWAYS_RETRAIN_IN_RECOVERY or something) so platforms can select which behavior they want (and then, that Kconfig would probably be best implemented by just making it decide whether normal_training has RECOVERY_FLAG set or not).
Hello build bot (Jenkins), Furquan Shaikh, Julius Werner, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46855
to look at the new patch set (#2).
Change subject: mrc_cache: Move force memory retraining code into mrc_cache ......................................................................
mrc_cache: Move force memory retraining code into mrc_cache
Currently forced memory retraining is handled in fsp 2.0. Moving the code into mrc_cache so more platforms can utilize it. Since it is unclear what is supposed to occur when there RECOVERY_MRC_CACHE present in the fmap, added a config MRC_ALWAYS_RETRAIN_IN_RECOVERY for users to explicity specify that if there is no RECOVERY_MRC_CACHE, then they will always perform memory training when booting into recovery mode. The default behavior when this config is not selected is that recovery boot and normal boot will share the same MRC_CACHE.
BOG=b:150502246 BRANCH=None TEST=1. run dut-control power_state:rec_force_mrc twice on lazor ensure that memory training happens both times run dut-control power_state:rec twice on lazor ensure that memory training happens only first time 2. remove HAS_RECOVERY_MRC_CACHE from lazor Kconfig boot twice to ensure memory training has occurred dut-control power_state:rec and make sure memory training doesn't occur. 3. Add MRC_ALWAYS_RETRAIN_IN_RECOVERY to lazor Kconfig boot twice to ensure memory training has occurred dut-control power_state:rec and make sure memory training occurs. dut-control power_state:rec a few more times to make sure memory training always occurs on a recovery boot.
Change-Id: I3875a7b4a4ba3c1aa8a3c1507b3993036a7155fc Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/Kconfig M src/drivers/mrc_cache/mrc_cache.c 3 files changed, 23 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/46855/2
Shelley Chen 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/1/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46855/1/src/drivers/mrc_cache/mrc_c... PS1, Line 264: if (!CONFIG(HAS_RECOVERY_MRC_CACHE))
I don't think we can add this here unless we want to change policy. […]
Ok, I added the CONFIG_MRC_ALWAYS_RETRAIN_IN_RECOVERY config and it seems to work just fine with just that config. I think that I got all the test cases in TEST=.
I'm not sure what you mean by making the Kconfig "decide whether normal_training has RECOVERY_FLAG set or not"? Can you please elaborate?
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 2:
(5 comments)
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.
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).
https://review.coreboot.org/c/coreboot/+/46855/1/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46855/1/src/drivers/mrc_cache/mrc_c... PS1, Line 264: if (!CONFIG(HAS_RECOVERY_MRC_CACHE))
Ok, I added the CONFIG_MRC_ALWAYS_RETRAIN_IN_RECOVERY config and it seems to work just fine with jus […]
See comment in new Patch Set (line 72).
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.
#if CONFIG(MRC_ALWAYS_RETRAIN_IN_RECOVERY) .flags = NORMAL_FLAG | RECOVERY_FLAG, #else .flags = NORMAL_FLAG, #endif
(same for variable_data). What this does is make lookup_region() always return 0 in recovery mode. Then you don't need the separate check in mrc_cache_find_current() anymore, and more importantly, it will also block things like update_mrc_cache_by_type(). The ALWAYS_RETRAIN case is different from the retrain_switch case, because for retrain_switch we still want the new training results to be written back to the recovery cache afterwards, but for ALWAYS_RETRAIN I think it's better to not write it back anywhere.
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. If you want to double-check that Kconfig dependencies were correctly enforced (because sometimes when two options are 'select'ed at the same time it doesn't), it would be better to add a _Static_assert(!CONFIG(MRC_ALWAYS_RETRAIN_IN_RECOVERY) || !CONFIG(HAS_RECOVERY_MRC_CACHE)) to the top of this file (although I think these days Kconfig itself will also throw an error for those cases so I'm not sure if that kind of extra check is still necessary anymore).
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.
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 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 don't understand this check completely. The different combinations of platforms are: […]
Sorry, I just flipped stuff in my suggestion. It should have been
#if CONFIG(MRC_ALWAYS_RETRAIN_IN_RECOVERY) .flags = NORMAL_FLAG, #else .flags = NORMAL_FLAG | RECOVERY_FLAG, #endif
of course, i.e. the normal cache is *not* valid for recovery mode when ALWAYS_RETRAIN_IN_RECOVERY is set. With that, I think it should keep existing behavior?
However, it sounds like you think that the only thing that makes a difference in whether we want to always retrain or reuse the normal cache is where vboot runs? (You say "vboot before bootblock", but I think you meant "vboot in bootblock" vs. "vboot in romstage", right? Or "before bootblock" is a special case of "in bootblock", I guess.) 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.
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.
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 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,
a. FSP2.0 platforms (MRC_ALWAYS_RETRAIN_IN_RECOVERY is selected) - No updates to normal mode cache based on current training. (Difference in behavior!!)
Yes, that's a change, true... but I'm not so convinced the old behavior makes sense, honestly. Why would you always want to update/invalidate the RW cache after you booted in recovery mode? If the user explicitly wanted to retrain, they could use the retrain combination. But if they just go into recovery mode for other reasons, why retrain? Or is that configuration only meant for boards that don't support a separate retrain trigger?
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!!)
I don't really follow this one, where do you see that behavior implemented? Sounds like you're saying that currently, if HAS_RECOVERY_MRC_CACHE is not enabled, everything (at least where vboot starts in bootblock) will always retrain from scratch in recovery mode? I don't think that's true. The FSP 2.0 boards do that, but only because this behavior is implemented explicitly in platform code (in fsp_fill_mrc_cache() in fsp2_0/memory_init.c) -- that's why FSP 2.0 would now select MRC_ALWAYS_RETRAIN_IN_RECOVERY with this patch. But other platforms wouldn't do that (unless they have similar platform-specific code, in which case we would also need to manually select the new Kconfig for them), the default behavior is to reuse the normal mode cache.
Based on that, I think it is okay to have 1-to-1 dependency on where vboot runs. i.e. [...]
Yeah, I agree with everything you said after this, it sounds good to simplify it like this if we agree that there's no need to have separate Kconfigs for everything. Just tie it straight to the vboot config. I think we can get rid of CONFIG_MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN as well, probably? That behavior would be completely decided by !VBOOT_STARTS_IN_ROMSTAGE, then.
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,
Yes, that's a change, true... but I'm not so convinced the old behavior makes sense, honestly. Why would you always want to update/invalidate the RW cache after you booted in recovery mode? If the user explicitly wanted to retrain, they could use the retrain combination. But if they just go into recovery mode for other reasons, why retrain? Or is that configuration only meant for boards that don't support a separate retrain trigger?
It's the latter. None of the platforms without the recovery MRC cache support the retrain switch. Since there is no retrain trigger, whenever the device goes into recovery, it would result in normal cache being invalidated (as a side-effect of updating data generated in recovery mode to normal cache). Only when we added the recovery MRC cache, the retrain switch was introduced.
I don't really follow this one, where do you see that behavior implemented? Sounds like you're saying that currently, if HAS_RECOVERY_MRC_CACHE is not enabled, everything (at least where vboot starts in bootblock) will always retrain from scratch in recovery mode? I don't think that's true.
It is done in the individual platform code:
FSP1.1 - https://review.coreboot.org/cgit/coreboot.git/tree/src/drivers/intel/fsp1_1/...
Haswell - https://review.coreboot.org/cgit/coreboot.git/tree/src/northbridge/intel/has...
Sandybridge -https://review.coreboot.org/cgit/coreboot.git/tree/src/northbridge/intel/san...
Broadwell - https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/broadwell/r...
I think we can get rid of CONFIG_MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN as well, probably? That behavior would be completely decided by !VBOOT_STARTS_IN_ROMSTAGE, then.
Yes, that makes sense to me.
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 2:
Okay, sounds like we're in agreement on how to proceed here (the stuff Furquan outlined)? Shelley, please let us know if that makes sense or you have any questions on the details. We definitely still need this in Lazor so it would be good if we could get it landed some time this week...
Hello build bot (Jenkins), Furquan Shaikh, Julius Werner, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46855
to look at the new patch set (#3).
Change subject: mrc_cache: Move force memory retraining code into mrc_cache ......................................................................
mrc_cache: Move force memory retraining code into mrc_cache
Currently forced memory retraining is handled in fsp 2.0. Moving the code into mrc_cache so more platforms can utilize it. In the case of !HAS_RECOVERY_MRC_CACHE, have linked the use of the mrc_cache to the configs VBOOT_STARTS_IN_BOOTBLOCK and VBOOT_STARTS_IN_ROMSTAGE. If VBOOT_STARTS IN_BOOTBLOCK, this means that memory training will occur after verified boot, meaning that mrc_cache will be filled with data from executing RW code, so in this case, we never want to use the training data in the mrc_cache If VBOOT_STARTS_IN_ROMSTAGE, this means that memory training happens before verfied boot, meaning that the mrc_cache data is generated by RO code, so it is safe to use for a recovery boot.
BUG=b:150502246 BRANCH=None TEST=1. run dut-control power_state:rec_force_mrc twice on lazor ensure that memory training happens both times run dut-control power_state:rec twice on lazor ensure that memory training happens only first time 2. remove HAS_RECOVERY_MRC_CACHE from lazor Kconfig boot twice to ensure memory training has occurred dut-control power_state:rec and make sure memory training doesn't occur. 3. Add MRC_ALWAYS_RETRAIN_IN_RECOVERY to lazor Kconfig boot twice to ensure memory training has occurred dut-control power_state:rec and make sure memory training occurs. dut-control power_state:rec a few more times to make sure memory training always occurs on a recovery boot.
Change-Id: I3875a7b4a4ba3c1aa8a3c1507b3993036a7155fc Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/Kconfig M src/drivers/mrc_cache/mrc_cache.c M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/soc/intel/broadwell/romstage/raminit.c 8 files changed, 80 insertions(+), 71 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/46855/3
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.
Hello build bot (Jenkins), Furquan Shaikh, Lee Leahy, Julius Werner, Angel Pons, Huang Jin, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46855
to look at the new patch set (#5).
Change subject: mrc_cache: Move code for triggering memory training into mrc_cache ......................................................................
mrc_cache: Move code for triggering memory training into mrc_cache
Currently the decision of whether or not to use mrc_cache is make within the individual platforms' drivers (ie: fsp2.0, fsp1.1, etc.). Moving the code that makes this decision into the mrc_cache. The conditions are as follows:
1. If HAS_RECOVERY_MRC_CACHE, use mrc_cache data (unless retrain switch is true) 2. If !HAS_RECOVERY_MRC_CACHE && VBOOT_STARTS_IN_BOOTBLOCK, this means that memory training will occur after verified boot, meaning that mrc_cache will be filled with data from executing RW code. So in this case, we never want to use the training data in the mrc_cache for recovery mode. 3. IF !HAS_RECOVERY_MRC_CACHE && VBOOT_STARTS_IN_ROMSTAGE, this means that memory training happens before verfied boot, meaning that the mrc_cache data is generated by RO code, so it is safe to use for a recovery boot.
Any platform that does not use vboot should be unaffected.
BUG=b:150502246 BRANCH=None TEST=1. run dut-control power_state:rec_force_mrc twice on lazor ensure that memory training happens both times run dut-control power_state:rec twice on lazor ensure that memory training happens only first time 2. remove HAS_RECOVERY_MRC_CACHE from lazor Kconfig boot twice to ensure memory training has occurred dut-control power_state:rec and make sure memory training doesn't occur.
Change-Id: I3875a7b4a4ba3c1aa8a3c1507b3993036a7155fc Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/Kconfig M src/drivers/mrc_cache/mrc_cache.c M src/mainboard/google/dedede/Kconfig M src/mainboard/google/deltaur/Kconfig M src/mainboard/google/drallion/Kconfig M src/mainboard/google/eve/Kconfig M src/mainboard/google/fizz/Kconfig M src/mainboard/google/hatch/Kconfig M src/mainboard/google/octopus/Kconfig M src/mainboard/google/poppy/Kconfig M src/mainboard/google/reef/Kconfig M src/mainboard/google/sarien/Kconfig M src/mainboard/google/volteer/Kconfig M src/mainboard/intel/adlrvp/Kconfig M src/mainboard/intel/glkrvp/Kconfig M src/mainboard/intel/jasperlake_rvp/Kconfig M src/mainboard/intel/tglrvp/Kconfig M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/soc/intel/broadwell/romstage/raminit.c M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sdm845/Kconfig 24 files changed, 64 insertions(+), 97 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/46855/5
Hello build bot (Jenkins), Furquan Shaikh, Lee Leahy, Julius Werner, Angel Pons, Huang Jin, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46855
to look at the new patch set (#6).
Change subject: mrc_cache: Move code for triggering memory training into mrc_cache ......................................................................
mrc_cache: Move code for triggering memory training into mrc_cache
Currently the decision of whether or not to use mrc_cache is make within the individual platforms' drivers (ie: fsp2.0, fsp1.1, etc.). Moving the code that makes this decision into the mrc_cache. The conditions are as follows:
1. If HAS_RECOVERY_MRC_CACHE, use mrc_cache data (unless retrain switch is true) 2. If !HAS_RECOVERY_MRC_CACHE && VBOOT_STARTS_IN_BOOTBLOCK, this means that memory training will occur after verified boot, meaning that mrc_cache will be filled with data from executing RW code. So in this case, we never want to use the training data in the mrc_cache for recovery mode. 3. IF !HAS_RECOVERY_MRC_CACHE && VBOOT_STARTS_IN_ROMSTAGE, this means that memory training happens before verfied boot, meaning that the mrc_cache data is generated by RO code, so it is safe to use for a recovery boot.
Any platform that does not use vboot should be unaffected.
BUG=b:150502246 BRANCH=None TEST=1. run dut-control power_state:rec_force_mrc twice on lazor ensure that memory retraining happens both times run dut-control power_state:rec twice on lazor ensure that memory retraining happens only first time 2. remove HAS_RECOVERY_MRC_CACHE from lazor Kconfig boot twice to ensure caching of memory training occurred on each boot.
Change-Id: I3875a7b4a4ba3c1aa8a3c1507b3993036a7155fc Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/Kconfig M src/drivers/mrc_cache/mrc_cache.c M src/mainboard/google/dedede/Kconfig M src/mainboard/google/deltaur/Kconfig M src/mainboard/google/drallion/Kconfig M src/mainboard/google/eve/Kconfig M src/mainboard/google/fizz/Kconfig M src/mainboard/google/hatch/Kconfig M src/mainboard/google/octopus/Kconfig M src/mainboard/google/poppy/Kconfig M src/mainboard/google/reef/Kconfig M src/mainboard/google/sarien/Kconfig M src/mainboard/google/volteer/Kconfig M src/mainboard/intel/adlrvp/Kconfig M src/mainboard/intel/glkrvp/Kconfig M src/mainboard/intel/jasperlake_rvp/Kconfig M src/mainboard/intel/tglrvp/Kconfig M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/soc/intel/broadwell/romstage/raminit.c M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sdm845/Kconfig 24 files changed, 65 insertions(+), 98 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/46855/6
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.
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:
(3 comments)
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 for (CONFIG(MRC_STASH_TO_CBMEM)) below on line 677. If we are not stashing to CBMEM, there is no use looking up the region type.
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 found.
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. Should we instead drop the return type for mrc_cache_stash_data()?
Paul Menzel 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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46855/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46855/6//COMMIT_MSG@11 PS6, Line 11: Moving the code that makes this decision into the mrc_cache. This sentence comes a little unmotivated for me. Maybe (if correct):
As this is not platform specific, but uses common vboot infrastructure, the code can be unified and moved into mrc_cache.
Hello build bot (Jenkins), Furquan Shaikh, Lee Leahy, Julius Werner, Angel Pons, Huang Jin, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46855
to look at the new patch set (#7).
Change subject: mrc_cache: Move code for triggering memory training into mrc_cache ......................................................................
mrc_cache: Move code for triggering memory training into mrc_cache
Currently the decision of whether or not to use mrc_cache in recovery mode is made within the individual platforms' drivers (ie: fsp2.0, fsp1.1, etc.). Moving the code that makes this decision into the mrc_cache. The conditions are as follows:
1. If HAS_RECOVERY_MRC_CACHE, use mrc_cache data (unless retrain switch is true) 2. If !HAS_RECOVERY_MRC_CACHE && VBOOT_STARTS_IN_BOOTBLOCK, this means that memory training will occur after verified boot, meaning that mrc_cache will be filled with data from executing RW code. So in this case, we never want to use the training data in the mrc_cache for recovery mode. 3. If !HAS_RECOVERY_MRC_CACHE && VBOOT_STARTS_IN_ROMSTAGE, this means that memory training happens before verfied boot, meaning that the mrc_cache data is generated by RO code, so it is safe to use for a recovery boot. 4. Any platform that does not use vboot should be unaffected.
Additionally, we have removed the MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN config because the mrc_cache driver takes care of invalidating the mrc_cache data for normal mode. If the platform: 1. !HAS_RECOVERY_MRC_CACHE, always invalidate mrc_cache data 2. HAS_RECOVERY_MRC_CACHE, only invalidate if retrain switch is set
BUG=b:150502246 BRANCH=None TEST=1. run dut-control power_state:rec_force_mrc twice on lazor ensure that memory retraining happens both times run dut-control power_state:rec twice on lazor ensure that memory retraining happens only first time 2. remove HAS_RECOVERY_MRC_CACHE from lazor Kconfig boot twice to ensure caching of memory training occurred on each boot.
Change-Id: I3875a7b4a4ba3c1aa8a3c1507b3993036a7155fc Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/Kconfig M src/drivers/mrc_cache/mrc_cache.c M src/mainboard/google/dedede/Kconfig M src/mainboard/google/deltaur/Kconfig M src/mainboard/google/drallion/Kconfig M src/mainboard/google/eve/Kconfig M src/mainboard/google/fizz/Kconfig M src/mainboard/google/hatch/Kconfig M src/mainboard/google/octopus/Kconfig M src/mainboard/google/poppy/Kconfig M src/mainboard/google/reef/Kconfig M src/mainboard/google/sarien/Kconfig M src/mainboard/google/volteer/Kconfig M src/mainboard/intel/adlrvp/Kconfig M src/mainboard/intel/glkrvp/Kconfig M src/mainboard/intel/jasperlake_rvp/Kconfig M src/mainboard/intel/tglrvp/Kconfig M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/soc/intel/broadwell/romstage/raminit.c M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sdm845/Kconfig 24 files changed, 98 insertions(+), 101 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/46855/7
Hello build bot (Jenkins), Furquan Shaikh, Lee Leahy, Julius Werner, Angel Pons, Huang Jin, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46855
to look at the new patch set (#8).
Change subject: mrc_cache: Move code for triggering memory training into mrc_cache ......................................................................
mrc_cache: Move code for triggering memory training into mrc_cache
Currently the decision of whether or not to use mrc_cache in recovery mode is made within the individual platforms' drivers (ie: fsp2.0, fsp1.1, etc.). As this is not platform specific, but uses common vboot infrastructure, the code can be unified and moved into mrc_cache. The conditions are as follows:
1. If HAS_RECOVERY_MRC_CACHE, use mrc_cache data (unless retrain switch is true) 2. If !HAS_RECOVERY_MRC_CACHE && VBOOT_STARTS_IN_BOOTBLOCK, this means that memory training will occur after verified boot, meaning that mrc_cache will be filled with data from executing RW code. So in this case, we never want to use the training data in the mrc_cache for recovery mode. 3. If !HAS_RECOVERY_MRC_CACHE && VBOOT_STARTS_IN_ROMSTAGE, this means that memory training happens before verfied boot, meaning that the mrc_cache data is generated by RO code, so it is safe to use for a recovery boot. 4. Any platform that does not use vboot should be unaffected.
Additionally, we have removed the MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN config because the mrc_cache driver takes care of invalidating the mrc_cache data for normal mode. If the platform: 1. !HAS_RECOVERY_MRC_CACHE, always invalidate mrc_cache data 2. HAS_RECOVERY_MRC_CACHE, only invalidate if retrain switch is set
BUG=b:150502246 BRANCH=None TEST=1. run dut-control power_state:rec_force_mrc twice on lazor ensure that memory retraining happens both times run dut-control power_state:rec twice on lazor ensure that memory retraining happens only first time 2. remove HAS_RECOVERY_MRC_CACHE from lazor Kconfig boot twice to ensure caching of memory training occurred on each boot.
Change-Id: I3875a7b4a4ba3c1aa8a3c1507b3993036a7155fc Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/Kconfig M src/drivers/mrc_cache/mrc_cache.c M src/mainboard/google/dedede/Kconfig M src/mainboard/google/deltaur/Kconfig M src/mainboard/google/drallion/Kconfig M src/mainboard/google/eve/Kconfig M src/mainboard/google/fizz/Kconfig M src/mainboard/google/hatch/Kconfig M src/mainboard/google/octopus/Kconfig M src/mainboard/google/poppy/Kconfig M src/mainboard/google/reef/Kconfig M src/mainboard/google/sarien/Kconfig M src/mainboard/google/volteer/Kconfig M src/mainboard/intel/adlrvp/Kconfig M src/mainboard/intel/glkrvp/Kconfig M src/mainboard/intel/jasperlake_rvp/Kconfig M src/mainboard/intel/tglrvp/Kconfig M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/soc/intel/broadwell/romstage/raminit.c M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sdm845/Kconfig 24 files changed, 98 insertions(+), 101 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/46855/8
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 8:
(3 comments)
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. Probably say: "so normal mode cache does not need to be invalidated".
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.
(Reason: All these checks in the function are for deciding if we want to skip the invalidation. If none of them match, then we will invalidate cache.)
https://review.coreboot.org/c/coreboot/+/46855/8/src/drivers/mrc_cache/mrc_c... PS8, Line 695: 0 Please see comment on patchset 6.
Hello build bot (Jenkins), Furquan Shaikh, Lee Leahy, Julius Werner, Angel Pons, Huang Jin, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46855
to look at the new patch set (#9).
Change subject: mrc_cache: Move code for triggering memory training into mrc_cache ......................................................................
mrc_cache: Move code for triggering memory training into mrc_cache
Currently the decision of whether or not to use mrc_cache in recovery mode is made within the individual platforms' drivers (ie: fsp2.0, fsp1.1, etc.). As this is not platform specific, but uses common vboot infrastructure, the code can be unified and moved into mrc_cache. The conditions are as follows:
1. If HAS_RECOVERY_MRC_CACHE, use mrc_cache data (unless retrain switch is true) 2. If !HAS_RECOVERY_MRC_CACHE && VBOOT_STARTS_IN_BOOTBLOCK, this means that memory training will occur after verified boot, meaning that mrc_cache will be filled with data from executing RW code. So in this case, we never want to use the training data in the mrc_cache for recovery mode. 3. If !HAS_RECOVERY_MRC_CACHE && VBOOT_STARTS_IN_ROMSTAGE, this means that memory training happens before verfied boot, meaning that the mrc_cache data is generated by RO code, so it is safe to use for a recovery boot. 4. Any platform that does not use vboot should be unaffected.
Additionally, we have removed the MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN config because the mrc_cache driver takes care of invalidating the mrc_cache data for normal mode. If the platform: 1. !HAS_RECOVERY_MRC_CACHE, always invalidate mrc_cache data 2. HAS_RECOVERY_MRC_CACHE, only invalidate if retrain switch is set
BUG=b:150502246 BRANCH=None TEST=1. run dut-control power_state:rec_force_mrc twice on lazor ensure that memory retraining happens both times run dut-control power_state:rec twice on lazor ensure that memory retraining happens only first time 2. remove HAS_RECOVERY_MRC_CACHE from lazor Kconfig boot twice to ensure caching of memory training occurred on each boot.
Change-Id: I3875a7b4a4ba3c1aa8a3c1507b3993036a7155fc Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/Kconfig M src/drivers/mrc_cache/mrc_cache.c M src/mainboard/google/dedede/Kconfig M src/mainboard/google/deltaur/Kconfig M src/mainboard/google/drallion/Kconfig M src/mainboard/google/eve/Kconfig M src/mainboard/google/fizz/Kconfig M src/mainboard/google/hatch/Kconfig M src/mainboard/google/octopus/Kconfig M src/mainboard/google/poppy/Kconfig M src/mainboard/google/reef/Kconfig M src/mainboard/google/sarien/Kconfig M src/mainboard/google/volteer/Kconfig M src/mainboard/intel/adlrvp/Kconfig M src/mainboard/intel/glkrvp/Kconfig M src/mainboard/intel/jasperlake_rvp/Kconfig M src/mainboard/intel/tglrvp/Kconfig M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/soc/intel/broadwell/romstage/raminit.c M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sdm845/Kconfig 24 files changed, 104 insertions(+), 106 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/46855/9
Hello build bot (Jenkins), Furquan Shaikh, Lee Leahy, Julius Werner, Angel Pons, Huang Jin, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46855
to look at the new patch set (#10).
Change subject: mrc_cache: Move code for triggering memory training into mrc_cache ......................................................................
mrc_cache: Move code for triggering memory training into mrc_cache
Currently the decision of whether or not to use mrc_cache in recovery mode is made within the individual platforms' drivers (ie: fsp2.0, fsp1.1, etc.). As this is not platform specific, but uses common vboot infrastructure, the code can be unified and moved into mrc_cache. The conditions are as follows:
1. If HAS_RECOVERY_MRC_CACHE, use mrc_cache data (unless retrain switch is true) 2. If !HAS_RECOVERY_MRC_CACHE && VBOOT_STARTS_IN_BOOTBLOCK, this means that memory training will occur after verified boot, meaning that mrc_cache will be filled with data from executing RW code. So in this case, we never want to use the training data in the mrc_cache for recovery mode. 3. If !HAS_RECOVERY_MRC_CACHE && VBOOT_STARTS_IN_ROMSTAGE, this means that memory training happens before verfied boot, meaning that the mrc_cache data is generated by RO code, so it is safe to use for a recovery boot. 4. Any platform that does not use vboot should be unaffected.
Additionally, we have removed the MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN config because the mrc_cache driver takes care of invalidating the mrc_cache data for normal mode. If the platform: 1. !HAS_RECOVERY_MRC_CACHE, always invalidate mrc_cache data 2. HAS_RECOVERY_MRC_CACHE, only invalidate if retrain switch is set
BUG=b:150502246 BRANCH=None TEST=1. run dut-control power_state:rec_force_mrc twice on lazor ensure that memory retraining happens both times run dut-control power_state:rec twice on lazor ensure that memory retraining happens only first time 2. remove HAS_RECOVERY_MRC_CACHE from lazor Kconfig boot twice to ensure caching of memory training occurred on each boot.
Change-Id: I3875a7b4a4ba3c1aa8a3c1507b3993036a7155fc Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/Kconfig M src/drivers/mrc_cache/mrc_cache.c M src/mainboard/google/dedede/Kconfig M src/mainboard/google/deltaur/Kconfig M src/mainboard/google/drallion/Kconfig M src/mainboard/google/eve/Kconfig M src/mainboard/google/fizz/Kconfig M src/mainboard/google/hatch/Kconfig M src/mainboard/google/octopus/Kconfig M src/mainboard/google/poppy/Kconfig M src/mainboard/google/reef/Kconfig M src/mainboard/google/sarien/Kconfig M src/mainboard/google/volteer/Kconfig M src/mainboard/intel/adlrvp/Kconfig M src/mainboard/intel/glkrvp/Kconfig M src/mainboard/intel/jasperlake_rvp/Kconfig M src/mainboard/intel/tglrvp/Kconfig M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/soc/intel/broadwell/romstage/raminit.c M src/soc/qualcomm/sc7180/Kconfig 23 files changed, 104 insertions(+), 105 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/46855/10
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
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:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46855/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46855/6//COMMIT_MSG@11 PS6, Line 11: Moving the code that makes this decision into the mrc_cache.
This sentence comes a little unmotivated for me. Maybe (if correct): […]
Done
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 659: CONFIG(HAS_RECOVERY_MRC_CACHE
In (!A || (A && B)) the second A is redundant, you can just write (!A || B) […]
All this logic has been moved into invalidate_normal_cache()
Julius Werner 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: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/46855/10/src/drivers/mrc_cache/mrc_... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46855/10/src/drivers/mrc_cache/mrc_... PS10, Line 612: if (!CONFIG(HAS_RECOVERY_MRC_CACHE) && CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) nit: since this is a static check and the one on line 603 is a dynamic one, it would be better to flip the two around (so that STARTS_IN_ROMSTAGE platforms can eliminate the call to recovery_mode_enabled()).
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 10: Code-Review+2
(2 comments)
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 665: 0
I do think that we should keep the return type though. Otherwise how will we indicate errors?
The way the function is written, there is only one case where this function can fail i.e. when `cbmem_add()` fails. I see that most callers of this function don't really do anything with the return value. Some print out an error and only 2 platforms take action: 1. qualcomm - this asserts in case there is a failure 2. amd - this actually skips stashing of volatile data to stage cache.
I think we can take a common action within this function -- assert if cbmem_add fails. There is no other action taken by any other platform anyways.
We can go ahead with what you have right now i.e. printing out a message that cache region wasn't found and return 0. But, I think we should fix this in a separate CL.
https://review.coreboot.org/c/coreboot/+/46855/10/src/drivers/mrc_cache/mrc_... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46855/10/src/drivers/mrc_cache/mrc_... PS10, Line 708: "MRC: No region type found. " : "Skip adding to cbmem for type %d.\n", No need to break print strings. They can be longer than the column limit.
Hello build bot (Jenkins), Furquan Shaikh, Lee Leahy, Julius Werner, Angel Pons, Huang Jin, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46855
to look at the new patch set (#11).
Change subject: mrc_cache: Move code for triggering memory training into mrc_cache ......................................................................
mrc_cache: Move code for triggering memory training into mrc_cache
Currently the decision of whether or not to use mrc_cache in recovery mode is made within the individual platforms' drivers (ie: fsp2.0, fsp1.1, etc.). As this is not platform specific, but uses common vboot infrastructure, the code can be unified and moved into mrc_cache. The conditions are as follows:
1. If HAS_RECOVERY_MRC_CACHE, use mrc_cache data (unless retrain switch is true) 2. If !HAS_RECOVERY_MRC_CACHE && VBOOT_STARTS_IN_BOOTBLOCK, this means that memory training will occur after verified boot, meaning that mrc_cache will be filled with data from executing RW code. So in this case, we never want to use the training data in the mrc_cache for recovery mode. 3. If !HAS_RECOVERY_MRC_CACHE && VBOOT_STARTS_IN_ROMSTAGE, this means that memory training happens before verfied boot, meaning that the mrc_cache data is generated by RO code, so it is safe to use for a recovery boot. 4. Any platform that does not use vboot should be unaffected.
Additionally, we have removed the MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN config because the mrc_cache driver takes care of invalidating the mrc_cache data for normal mode. If the platform: 1. !HAS_RECOVERY_MRC_CACHE, always invalidate mrc_cache data 2. HAS_RECOVERY_MRC_CACHE, only invalidate if retrain switch is set
BUG=b:150502246 BRANCH=None TEST=1. run dut-control power_state:rec_force_mrc twice on lazor ensure that memory retraining happens both times run dut-control power_state:rec twice on lazor ensure that memory retraining happens only first time 2. remove HAS_RECOVERY_MRC_CACHE from lazor Kconfig boot twice to ensure caching of memory training occurred on each boot.
Change-Id: I3875a7b4a4ba3c1aa8a3c1507b3993036a7155fc Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/Kconfig M src/drivers/mrc_cache/mrc_cache.c M src/mainboard/google/dedede/Kconfig M src/mainboard/google/deltaur/Kconfig M src/mainboard/google/drallion/Kconfig M src/mainboard/google/eve/Kconfig M src/mainboard/google/fizz/Kconfig M src/mainboard/google/hatch/Kconfig M src/mainboard/google/octopus/Kconfig M src/mainboard/google/poppy/Kconfig M src/mainboard/google/reef/Kconfig M src/mainboard/google/sarien/Kconfig M src/mainboard/google/volteer/Kconfig M src/mainboard/intel/adlrvp/Kconfig M src/mainboard/intel/glkrvp/Kconfig M src/mainboard/intel/jasperlake_rvp/Kconfig M src/mainboard/intel/tglrvp/Kconfig M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/soc/intel/broadwell/romstage/raminit.c M src/soc/qualcomm/sc7180/Kconfig 23 files changed, 103 insertions(+), 105 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/46855/11
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 11:
(3 comments)
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 665: 0
I do think that we should keep the return type though. Otherwise how will we indicate errors? […]
Ok, I'll clean up the return value in the next CL.
https://review.coreboot.org/c/coreboot/+/46855/10/src/drivers/mrc_cache/mrc_... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46855/10/src/drivers/mrc_cache/mrc_... PS10, Line 612: if (!CONFIG(HAS_RECOVERY_MRC_CACHE) && CONFIG(VBOOT_STARTS_IN_ROMSTAGE))
nit: since this is a static check and the one on line 603 is a dynamic one, it would be better to fl […]
Done.
https://review.coreboot.org/c/coreboot/+/46855/10/src/drivers/mrc_cache/mrc_... PS10, Line 708: "MRC: No region type found. " : "Skip adding to cbmem for type %d.\n",
No need to break print strings. They can be longer than the column limit.
Done
Hello build bot (Jenkins), Furquan Shaikh, Lee Leahy, Julius Werner, Angel Pons, Huang Jin, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46855
to look at the new patch set (#12).
Change subject: mrc_cache: Move code for triggering memory training into mrc_cache ......................................................................
mrc_cache: Move code for triggering memory training into mrc_cache
Currently the decision of whether or not to use mrc_cache in recovery mode is made within the individual platforms' drivers (ie: fsp2.0, fsp1.1, etc.). As this is not platform specific, but uses common vboot infrastructure, the code can be unified and moved into mrc_cache. The conditions are as follows:
1. If HAS_RECOVERY_MRC_CACHE, use mrc_cache data (unless retrain switch is true) 2. If !HAS_RECOVERY_MRC_CACHE && VBOOT_STARTS_IN_BOOTBLOCK, this means that memory training will occur after verified boot, meaning that mrc_cache will be filled with data from executing RW code. So in this case, we never want to use the training data in the mrc_cache for recovery mode. 3. If !HAS_RECOVERY_MRC_CACHE && VBOOT_STARTS_IN_ROMSTAGE, this means that memory training happens before verfied boot, meaning that the mrc_cache data is generated by RO code, so it is safe to use for a recovery boot. 4. Any platform that does not use vboot should be unaffected.
Additionally, we have removed the MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN config because the mrc_cache driver takes care of invalidating the mrc_cache data for normal mode. If the platform: 1. !HAS_RECOVERY_MRC_CACHE, always invalidate mrc_cache data 2. HAS_RECOVERY_MRC_CACHE, only invalidate if retrain switch is set
BUG=b:150502246 BRANCH=None TEST=1. run dut-control power_state:rec_force_mrc twice on lazor ensure that memory retraining happens both times run dut-control power_state:rec twice on lazor ensure that memory retraining happens only first time 2. remove HAS_RECOVERY_MRC_CACHE from lazor Kconfig boot twice to ensure caching of memory training occurred on each boot.
Change-Id: I3875a7b4a4ba3c1aa8a3c1507b3993036a7155fc Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/Kconfig M src/drivers/mrc_cache/mrc_cache.c M src/mainboard/google/dedede/Kconfig M src/mainboard/google/deltaur/Kconfig M src/mainboard/google/drallion/Kconfig M src/mainboard/google/eve/Kconfig M src/mainboard/google/fizz/Kconfig M src/mainboard/google/hatch/Kconfig M src/mainboard/google/octopus/Kconfig M src/mainboard/google/poppy/Kconfig M src/mainboard/google/reef/Kconfig M src/mainboard/google/sarien/Kconfig M src/mainboard/google/volteer/Kconfig M src/mainboard/intel/adlrvp/Kconfig M src/mainboard/intel/glkrvp/Kconfig M src/mainboard/intel/jasperlake_rvp/Kconfig M src/mainboard/intel/tglrvp/Kconfig M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/soc/intel/broadwell/romstage/raminit.c M src/soc/qualcomm/sc7180/Kconfig 23 files changed, 103 insertions(+), 105 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/46855/12
Hello build bot (Jenkins), Furquan Shaikh, Lee Leahy, Julius Werner, Angel Pons, Huang Jin, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46855
to look at the new patch set (#13).
Change subject: mrc_cache: Move code for triggering memory training into mrc_cache ......................................................................
mrc_cache: Move code for triggering memory training into mrc_cache
Currently the decision of whether or not to use mrc_cache in recovery mode is made within the individual platforms' drivers (ie: fsp2.0, fsp1.1, etc.). As this is not platform specific, but uses common vboot infrastructure, the code can be unified and moved into mrc_cache. The conditions are as follows:
1. If HAS_RECOVERY_MRC_CACHE, use mrc_cache data (unless retrain switch is true) 2. If !HAS_RECOVERY_MRC_CACHE && VBOOT_STARTS_IN_BOOTBLOCK, this means that memory training will occur after verified boot, meaning that mrc_cache will be filled with data from executing RW code. So in this case, we never want to use the training data in the mrc_cache for recovery mode. 3. If !HAS_RECOVERY_MRC_CACHE && VBOOT_STARTS_IN_ROMSTAGE, this means that memory training happens before verfied boot, meaning that the mrc_cache data is generated by RO code, so it is safe to use for a recovery boot. 4. Any platform that does not use vboot should be unaffected.
Additionally, we have removed the MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN config because the mrc_cache driver takes care of invalidating the mrc_cache data for normal mode. If the platform: 1. !HAS_RECOVERY_MRC_CACHE, always invalidate mrc_cache data 2. HAS_RECOVERY_MRC_CACHE, only invalidate if retrain switch is set
BUG=b:150502246 BRANCH=None TEST=1. run dut-control power_state:rec_force_mrc twice on lazor ensure that memory retraining happens both times run dut-control power_state:rec twice on lazor ensure that memory retraining happens only first time 2. remove HAS_RECOVERY_MRC_CACHE from lazor Kconfig boot twice to ensure caching of memory training occurred on each boot.
Change-Id: I3875a7b4a4ba3c1aa8a3c1507b3993036a7155fc Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/Kconfig M src/drivers/mrc_cache/mrc_cache.c M src/mainboard/google/dedede/Kconfig M src/mainboard/google/deltaur/Kconfig M src/mainboard/google/drallion/Kconfig M src/mainboard/google/eve/Kconfig M src/mainboard/google/fizz/Kconfig M src/mainboard/google/hatch/Kconfig M src/mainboard/google/octopus/Kconfig M src/mainboard/google/poppy/Kconfig M src/mainboard/google/reef/Kconfig M src/mainboard/google/sarien/Kconfig M src/mainboard/google/volteer/Kconfig M src/mainboard/intel/adlrvp/Kconfig M src/mainboard/intel/glkrvp/Kconfig M src/mainboard/intel/jasperlake_rvp/Kconfig M src/mainboard/intel/tglrvp/Kconfig M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/soc/intel/broadwell/raminit.c M src/soc/qualcomm/sc7180/Kconfig 23 files changed, 103 insertions(+), 105 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/46855/13
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 13: Code-Review+2
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 13:
Sorry about the noise. Was trying to get rid of the merge conflict in preparation for landing. The CL is ready for review now.
Shelley Chen has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46855 )
Change subject: mrc_cache: Move code for triggering memory training into mrc_cache ......................................................................
mrc_cache: Move code for triggering memory training into mrc_cache
Currently the decision of whether or not to use mrc_cache in recovery mode is made within the individual platforms' drivers (ie: fsp2.0, fsp1.1, etc.). As this is not platform specific, but uses common vboot infrastructure, the code can be unified and moved into mrc_cache. The conditions are as follows:
1. If HAS_RECOVERY_MRC_CACHE, use mrc_cache data (unless retrain switch is true) 2. If !HAS_RECOVERY_MRC_CACHE && VBOOT_STARTS_IN_BOOTBLOCK, this means that memory training will occur after verified boot, meaning that mrc_cache will be filled with data from executing RW code. So in this case, we never want to use the training data in the mrc_cache for recovery mode. 3. If !HAS_RECOVERY_MRC_CACHE && VBOOT_STARTS_IN_ROMSTAGE, this means that memory training happens before verfied boot, meaning that the mrc_cache data is generated by RO code, so it is safe to use for a recovery boot. 4. Any platform that does not use vboot should be unaffected.
Additionally, we have removed the MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN config because the mrc_cache driver takes care of invalidating the mrc_cache data for normal mode. If the platform: 1. !HAS_RECOVERY_MRC_CACHE, always invalidate mrc_cache data 2. HAS_RECOVERY_MRC_CACHE, only invalidate if retrain switch is set
BUG=b:150502246 BRANCH=None TEST=1. run dut-control power_state:rec_force_mrc twice on lazor ensure that memory retraining happens both times run dut-control power_state:rec twice on lazor ensure that memory retraining happens only first time 2. remove HAS_RECOVERY_MRC_CACHE from lazor Kconfig boot twice to ensure caching of memory training occurred on each boot.
Change-Id: I3875a7b4a4ba3c1aa8a3c1507b3993036a7155fc Signed-off-by: Shelley Chen shchen@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/46855 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/Kconfig M src/drivers/mrc_cache/mrc_cache.c M src/mainboard/google/dedede/Kconfig M src/mainboard/google/deltaur/Kconfig M src/mainboard/google/drallion/Kconfig M src/mainboard/google/eve/Kconfig M src/mainboard/google/fizz/Kconfig M src/mainboard/google/hatch/Kconfig M src/mainboard/google/octopus/Kconfig M src/mainboard/google/poppy/Kconfig M src/mainboard/google/reef/Kconfig M src/mainboard/google/sarien/Kconfig M src/mainboard/google/volteer/Kconfig M src/mainboard/intel/adlrvp/Kconfig M src/mainboard/intel/glkrvp/Kconfig M src/mainboard/intel/jasperlake_rvp/Kconfig M src/mainboard/intel/tglrvp/Kconfig M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/soc/intel/broadwell/raminit.c M src/soc/qualcomm/sc7180/Kconfig 23 files changed, 103 insertions(+), 105 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/drivers/intel/fsp1_1/romstage.c b/src/drivers/intel/fsp1_1/romstage.c index 5a59c50..5129dc6 100644 --- a/src/drivers/intel/fsp1_1/romstage.c +++ b/src/drivers/intel/fsp1_1/romstage.c @@ -41,35 +41,29 @@ params->saved_data_size = 0; params->saved_data = NULL; if (!params->disable_saved_data) { - if (vboot_recovery_mode_enabled()) { - /* Recovery mode does not use MRC cache */ + /* Assume boot device is memory mapped. */ + assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); + + params->saved_data = NULL; + if (CONFIG(CACHE_MRC_SETTINGS)) + params->saved_data = + mrc_cache_current_mmap_leak(MRC_TRAINING_DATA, + params->fsp_version, + &mrc_size); + if (params->saved_data) { + /* MRC cache found */ + params->saved_data_size = mrc_size; + + } else if (s3wake) { + /* Waking from S3 and no cache. */ printk(BIOS_DEBUG, - "Recovery mode: not using MRC cache.\n"); + "No MRC cache " + "found in S3 resume path.\n"); + post_code(POST_RESUME_FAILURE); + /* FIXME: A "system" reset is likely enough: */ + full_reset(); } else { - /* Assume boot device is memory mapped. */ - assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); - - params->saved_data = NULL; - if (CONFIG(CACHE_MRC_SETTINGS)) - params->saved_data = - mrc_cache_current_mmap_leak(MRC_TRAINING_DATA, - params->fsp_version, - &mrc_size); - if (params->saved_data) { - /* MRC cache found */ - params->saved_data_size = mrc_size; - - } else if (s3wake) { - /* Waking from S3 and no cache. */ - printk(BIOS_DEBUG, - "No MRC cache " - "found in S3 resume path.\n"); - post_code(POST_RESUME_FAILURE); - /* FIXME: A "system" reset is likely enough: */ - full_reset(); - } else { - printk(BIOS_DEBUG, "No MRC cache found.\n"); - } + printk(BIOS_DEBUG, "No MRC cache found.\n"); } }
diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c index 68cc121..27e34fe 100644 --- a/src/drivers/intel/fsp2_0/memory_init.c +++ b/src/drivers/intel/fsp2_0/memory_init.c @@ -92,18 +92,6 @@ if (!CONFIG(CACHE_MRC_SETTINGS)) return;
- /* - * In recovery mode, force retraining: - * 1. Recovery cache is not supported, or - * 2. Memory retrain switch is set. - */ - if (vboot_recovery_mode_enabled()) { - if (!CONFIG(HAS_RECOVERY_MRC_CACHE)) - return; - if (get_recovery_mode_retrain_switch()) - return; - } - /* Assume boot device is memory mapped. */ assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED));
diff --git a/src/drivers/mrc_cache/Kconfig b/src/drivers/mrc_cache/Kconfig index b09c196..df6973b 100644 --- a/src/drivers/mrc_cache/Kconfig +++ b/src/drivers/mrc_cache/Kconfig @@ -17,11 +17,6 @@ bool default n
-config MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN - bool - depends on VBOOT_STARTS_IN_BOOTBLOCK - default n - config MRC_SETTINGS_VARIABLE_DATA bool default n diff --git a/src/drivers/mrc_cache/mrc_cache.c b/src/drivers/mrc_cache/mrc_cache.c index eb43123..8b26ea5 100644 --- a/src/drivers/mrc_cache/mrc_cache.c +++ b/src/drivers/mrc_cache/mrc_cache.c @@ -69,7 +69,20 @@ .type = MRC_TRAINING_DATA, .elog_slot = ELOG_MEM_CACHE_UPDATE_SLOT_NORMAL, .tpm_hash_index = MRC_RW_HASH_NV_INDEX, +#if CONFIG(VBOOT_STARTS_IN_ROMSTAGE) + /* + * If VBOOT_STARTS_IN_ROMSTAGE is selected, this means that + * memory training happens before vboot (in RO) and the + * mrc_cache data is always safe to use. + */ .flags = NORMAL_FLAG | RECOVERY_FLAG, +#else + /* + * If !VBOOT_STARTS_IN_ROMSTAGE, this means that memory training happens after + * vboot (in RW code) and is never safe to use in recovery. + */ + .flags = NORMAL_FLAG, +#endif };
static const struct cache_region variable_data = { @@ -78,7 +91,20 @@ .type = MRC_VARIABLE_DATA, .elog_slot = ELOG_MEM_CACHE_UPDATE_SLOT_VARIABLE, .tpm_hash_index = 0, +#if CONFIG(VBOOT_STARTS_IN_ROMSTAGE) + /* + * If VBOOT_STARTS_IN_ROMSTAGE is selected, this means that + * memory training happens before vboot (in RO) and the + * mrc_cache data is always safe to use. + */ .flags = NORMAL_FLAG | RECOVERY_FLAG, +#else + /* + * If !VBOOT_STARTS_IN_ROMSTAGE, this means that memory training happens after + * vboot (in RW code) and is never safe to use in recovery. + */ + .flags = NORMAL_FLAG, +#endif };
/* Order matters here for priority in matching. */ @@ -255,6 +281,13 @@ const size_t md_size = sizeof(*md); const bool fail_bad_data = true;
+ /* + * In recovery mode, force retraining if the memory retrain + * switch is set. + */ + if (vboot_recovery_mode_enabled() && get_recovery_mode_retrain_switch()) + return -1; + cr = lookup_region(®ion, type);
if (cr == NULL) @@ -566,10 +599,24 @@ const char *name = DEFAULT_MRC_CACHE; const uint32_t invalid = ~MRC_DATA_SIGNATURE;
- /* Invalidate only on recovery mode with retraining enabled. */ + /* + * If !HAS_RECOVERY_MRC_CACHE and VBOOT_STARTS_IN_ROMSTAGE is + * selected, this means that memory training occurs before + * verified boot (in RO), so normal mode cache does not need + * to be invalidated. + */ + if (!CONFIG(HAS_RECOVERY_MRC_CACHE) && CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) + return; + + /* We only invalidate the normal cache in recovery mode. */ if (!vboot_recovery_mode_enabled()) return; - if (!get_recovery_mode_retrain_switch()) + + /* + * For platforms with a recovery mrc_cache, no need to + * invalidate when retrain switch is not set. + */ + if (CONFIG(HAS_RECOVERY_MRC_CACHE) && !get_recovery_mode_retrain_switch()) return;
if (fmap_locate_area_as_rdev_rw(name, &rdev) < 0) { @@ -599,7 +646,7 @@ cr = lookup_region(®ion, type);
if (cr == NULL) { - printk(BIOS_ERR, "MRC: could not find cache_region type %d\n", type); + printk(BIOS_INFO, "MRC: could not find cache_region type %d\n", type); return; }
@@ -631,8 +678,7 @@ update_mrc_cache_from_cbmem(MRC_VARIABLE_DATA); }
- if (CONFIG(MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN)) - invalidate_normal_cache(); + invalidate_normal_cache();
protect_mrc_region(); } @@ -642,13 +688,6 @@ { const struct cache_region *cr;
- cr = lookup_region_type(type); - if (cr == NULL) { - printk(BIOS_ERR, "MRC: failed to add to cbmem for type %d.\n", - type); - return -1; - } - struct mrc_metadata md = { .signature = MRC_DATA_SIGNATURE, .data_size = size, @@ -664,6 +703,13 @@ size_t cbmem_size; cbmem_size = sizeof(*cbmem_md) + size;
+ cr = lookup_region_type(type); + if (cr == NULL) { + printk(BIOS_INFO, "MRC: No region type found. Skip adding to cbmem for type %d.\n", + type); + return 0; + } + cbmem_md = cbmem_add(cr->cbmem_id, cbmem_size);
if (cbmem_md == NULL) { diff --git a/src/mainboard/google/dedede/Kconfig b/src/mainboard/google/dedede/Kconfig index 6da6505..891b48a 100644 --- a/src/mainboard/google/dedede/Kconfig +++ b/src/mainboard/google/dedede/Kconfig @@ -47,7 +47,6 @@ select GBB_FLAG_FORCE_DEV_BOOT_LEGACY select GBB_FLAG_FORCE_MANUAL_RECOVERY select HAS_RECOVERY_MRC_CACHE - select MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN select VBOOT_EARLY_EC_SYNC select VBOOT_LID_SWITCH
diff --git a/src/mainboard/google/deltaur/Kconfig b/src/mainboard/google/deltaur/Kconfig index dafd593..8d95849 100644 --- a/src/mainboard/google/deltaur/Kconfig +++ b/src/mainboard/google/deltaur/Kconfig @@ -90,7 +90,6 @@
config VBOOT select HAS_RECOVERY_MRC_CACHE - select MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN select VBOOT_LID_SWITCH
endif # BOARD_GOOGLE_BASEBOARD_DELTAUR diff --git a/src/mainboard/google/drallion/Kconfig b/src/mainboard/google/drallion/Kconfig index d535d14..6c90eed 100644 --- a/src/mainboard/google/drallion/Kconfig +++ b/src/mainboard/google/drallion/Kconfig @@ -93,7 +93,6 @@
config VBOOT select HAS_RECOVERY_MRC_CACHE - select MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN select VBOOT_LID_SWITCH
endif # BOARD_GOOGLE_BASEBOARD_DRALLION diff --git a/src/mainboard/google/eve/Kconfig b/src/mainboard/google/eve/Kconfig index 9316c7f..ece0119 100644 --- a/src/mainboard/google/eve/Kconfig +++ b/src/mainboard/google/eve/Kconfig @@ -28,7 +28,6 @@ select EC_GOOGLE_CHROMEEC_SWITCHES select HAS_RECOVERY_MRC_CACHE select VBOOT_LID_SWITCH - select MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN
config CHROMEOS select DSAR_ENABLE diff --git a/src/mainboard/google/fizz/Kconfig b/src/mainboard/google/fizz/Kconfig index a21df37..e09f853 100644 --- a/src/mainboard/google/fizz/Kconfig +++ b/src/mainboard/google/fizz/Kconfig @@ -40,7 +40,6 @@ config VBOOT select EC_GOOGLE_CHROMEEC_SWITCHES select HAS_RECOVERY_MRC_CACHE - select MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN
config DRIVER_TPM_SPI_BUS default 0x1 diff --git a/src/mainboard/google/hatch/Kconfig b/src/mainboard/google/hatch/Kconfig index 4d7d5ec..12e5638 100644 --- a/src/mainboard/google/hatch/Kconfig +++ b/src/mainboard/google/hatch/Kconfig @@ -54,7 +54,6 @@ select GBB_FLAG_FORCE_DEV_BOOT_LEGACY select GBB_FLAG_FORCE_MANUAL_RECOVERY select HAS_RECOVERY_MRC_CACHE - select MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN select VBOOT_LID_SWITCH select CHROMEOS_CSE_BOARD_RESET_OVERRIDE if SOC_INTEL_CSE_LITE_SKU
@@ -183,7 +182,6 @@
config VBOOT select HAS_RECOVERY_MRC_CACHE - select MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN select VBOOT_EARLY_EC_SYNC
config USE_PM_ACPI_TIMER diff --git a/src/mainboard/google/octopus/Kconfig b/src/mainboard/google/octopus/Kconfig index b9c6a1b..588c8ed 100644 --- a/src/mainboard/google/octopus/Kconfig +++ b/src/mainboard/google/octopus/Kconfig @@ -46,7 +46,6 @@ default y select EC_GOOGLE_CHROMEEC_SWITCHES select HAS_RECOVERY_MRC_CACHE - select MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN select VBOOT_LID_SWITCH
config MAINBOARD_DIR diff --git a/src/mainboard/google/poppy/Kconfig b/src/mainboard/google/poppy/Kconfig index d8b90bc..2656d28 100644 --- a/src/mainboard/google/poppy/Kconfig +++ b/src/mainboard/google/poppy/Kconfig @@ -208,7 +208,6 @@ config VBOOT select EC_GOOGLE_CHROMEEC_SWITCHES select HAS_RECOVERY_MRC_CACHE - select MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN select VBOOT_LID_SWITCH
config UART_FOR_CONSOLE diff --git a/src/mainboard/google/reef/Kconfig b/src/mainboard/google/reef/Kconfig index 76a8640..9744d74 100644 --- a/src/mainboard/google/reef/Kconfig +++ b/src/mainboard/google/reef/Kconfig @@ -43,7 +43,6 @@ config VBOOT select EC_GOOGLE_CHROMEEC_SWITCHES select HAS_RECOVERY_MRC_CACHE - select MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN select VBOOT_LID_SWITCH if BASEBOARD_REEF_LAPTOP
config MAINBOARD_DIR diff --git a/src/mainboard/google/sarien/Kconfig b/src/mainboard/google/sarien/Kconfig index 5b58072..9b0d251 100644 --- a/src/mainboard/google/sarien/Kconfig +++ b/src/mainboard/google/sarien/Kconfig @@ -94,7 +94,6 @@
config VBOOT select HAS_RECOVERY_MRC_CACHE - select MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN select VBOOT_LID_SWITCH
endif # BOARD_GOOGLE_BASEBOARD_SARIEN diff --git a/src/mainboard/google/volteer/Kconfig b/src/mainboard/google/volteer/Kconfig index a5cc196..07a5fde 100644 --- a/src/mainboard/google/volteer/Kconfig +++ b/src/mainboard/google/volteer/Kconfig @@ -45,7 +45,6 @@ select GBB_FLAG_FORCE_DEV_BOOT_LEGACY select GBB_FLAG_FORCE_MANUAL_RECOVERY select HAS_RECOVERY_MRC_CACHE - select MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN select VBOOT_LID_SWITCH select VBOOT_EARLY_EC_SYNC
diff --git a/src/mainboard/intel/adlrvp/Kconfig b/src/mainboard/intel/adlrvp/Kconfig index 57b0524..b6d3ff3 100644 --- a/src/mainboard/intel/adlrvp/Kconfig +++ b/src/mainboard/intel/adlrvp/Kconfig @@ -25,7 +25,6 @@ select GBB_FLAG_FORCE_MANUAL_RECOVERY select GBB_FLAG_DISABLE_PD_SOFTWARE_SYNC select HAS_RECOVERY_MRC_CACHE - select MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN
config MAINBOARD_DIR string @@ -82,7 +81,6 @@ select VBOOT_LID_SWITCH select VBOOT_MOCK_SECDATA select HAS_RECOVERY_MRC_CACHE - select MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN
config UART_FOR_CONSOLE int diff --git a/src/mainboard/intel/glkrvp/Kconfig b/src/mainboard/intel/glkrvp/Kconfig index 7b1b564..172dfa2 100644 --- a/src/mainboard/intel/glkrvp/Kconfig +++ b/src/mainboard/intel/glkrvp/Kconfig @@ -45,7 +45,6 @@
config VBOOT select HAS_RECOVERY_MRC_CACHE - select MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN select EC_GOOGLE_CHROMEEC_SWITCHES if GLK_CHROME_EC
config MAINBOARD_DIR diff --git a/src/mainboard/intel/jasperlake_rvp/Kconfig b/src/mainboard/intel/jasperlake_rvp/Kconfig index 1125a9b..7bfd158 100644 --- a/src/mainboard/intel/jasperlake_rvp/Kconfig +++ b/src/mainboard/intel/jasperlake_rvp/Kconfig @@ -59,7 +59,6 @@ select GBB_FLAG_FORCE_MANUAL_RECOVERY select GBB_FLAG_DISABLE_PD_SOFTWARE_SYNC select HAS_RECOVERY_MRC_CACHE - select MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN
config VBOOT select VBOOT_LID_SWITCH diff --git a/src/mainboard/intel/tglrvp/Kconfig b/src/mainboard/intel/tglrvp/Kconfig index 4d43e23..9df542d 100644 --- a/src/mainboard/intel/tglrvp/Kconfig +++ b/src/mainboard/intel/tglrvp/Kconfig @@ -31,7 +31,6 @@ select EC_GOOGLE_CHROMEEC_SWITCHES if TGL_CHROME_EC select GBB_FLAG_FORCE_MANUAL_RECOVERY select HAS_RECOVERY_MRC_CACHE - select MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN select GBB_FLAG_FORCE_DEV_SWITCH_ON select GBB_FLAG_FORCE_DEV_BOOT_USB select VBOOT_EARLY_EC_SYNC diff --git a/src/northbridge/intel/haswell/raminit.c b/src/northbridge/intel/haswell/raminit.c index 63d70f7..290e402 100644 --- a/src/northbridge/intel/haswell/raminit.c +++ b/src/northbridge/intel/haswell/raminit.c @@ -112,10 +112,11 @@
printk(BIOS_DEBUG, "Starting UEFI PEI System Agent\n");
- /* Do not pass MRC data in for recovery mode boot, always pass it in for S3 resume */ - if (!(CONFIG(HASWELL_VBOOT_IN_BOOTBLOCK) && vboot_recovery_mode_enabled()) - || pei_data->boot_mode == 2) - prepare_mrc_cache(pei_data); + /* + * Always pass in mrc_cache data. The driver will determine + * whether to use the data or not. + */ + prepare_mrc_cache(pei_data);
/* If MRC data is not found, we cannot continue S3 resume */ if (pei_data->boot_mode == 2 && !pei_data->mrc_input) { diff --git a/src/northbridge/intel/sandybridge/raminit_mrc.c b/src/northbridge/intel/sandybridge/raminit_mrc.c index 444ecf8..8df7d1b 100644 --- a/src/northbridge/intel/sandybridge/raminit_mrc.c +++ b/src/northbridge/intel/sandybridge/raminit_mrc.c @@ -133,12 +133,10 @@ printk(BIOS_DEBUG, "Starting UEFI PEI System Agent\n");
/* - * Do not pass MRC data in for recovery mode boot, - * Always pass it in for S3 resume. + * Always pass in mrc_cache data. The driver will determine + * whether to use the data or not. */ - if (!(CONFIG(SANDYBRIDGE_VBOOT_IN_BOOTBLOCK) && vboot_recovery_mode_enabled()) || - pei_data->boot_mode == 2) - prepare_mrc_cache(pei_data); + prepare_mrc_cache(pei_data);
/* If MRC data is not found we cannot continue S3 resume. */ if (pei_data->boot_mode == 2 && !pei_data->mrc_input) { diff --git a/src/soc/intel/broadwell/raminit.c b/src/soc/intel/broadwell/raminit.c index 7020ddf..e51b4f7 100644 --- a/src/soc/intel/broadwell/raminit.c +++ b/src/soc/intel/broadwell/raminit.c @@ -85,29 +85,23 @@
broadwell_fill_pei_data(pei_data);
- if (CONFIG(BROADWELL_VBOOT_IN_BOOTBLOCK) && - vboot_recovery_mode_enabled()) { - /* Recovery mode does not use MRC cache */ - printk(BIOS_DEBUG, "Recovery mode: not using MRC cache.\n"); - } else { - /* Assume boot device is memory mapped. */ - assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); + /* Assume boot device is memory mapped. */ + assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED));
- pei_data->saved_data = - mrc_cache_current_mmap_leak(MRC_TRAINING_DATA, 0, - &mrc_size); - if (pei_data->saved_data) { - /* MRC cache found */ - pei_data->saved_data_size = mrc_size; - } else if (pei_data->boot_mode == ACPI_S3) { - /* Waking from S3 and no cache. */ - printk(BIOS_DEBUG, - "No MRC cache found in S3 resume path.\n"); - post_code(POST_RESUME_FAILURE); - system_reset(); - } else { - printk(BIOS_DEBUG, "No MRC cache found.\n"); - } + pei_data->saved_data = + mrc_cache_current_mmap_leak(MRC_TRAINING_DATA, 0, + &mrc_size); + if (pei_data->saved_data) { + /* MRC cache found */ + pei_data->saved_data_size = mrc_size; + } else if (pei_data->boot_mode == ACPI_S3) { + /* Waking from S3 and no cache. */ + printk(BIOS_DEBUG, + "No MRC cache found in S3 resume path.\n"); + post_code(POST_RESUME_FAILURE); + system_reset(); + } else { + printk(BIOS_DEBUG, "No MRC cache found.\n"); }
/* diff --git a/src/soc/qualcomm/sc7180/Kconfig b/src/soc/qualcomm/sc7180/Kconfig index 4cd1c41..066ff5d 100644 --- a/src/soc/qualcomm/sc7180/Kconfig +++ b/src/soc/qualcomm/sc7180/Kconfig @@ -33,7 +33,6 @@ select VBOOT_RETURN_FROM_VERSTAGE select VBOOT_MUST_REQUEST_DISPLAY select VBOOT_STARTS_IN_BOOTBLOCK - select MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN
config SC7180_QSPI bool
-
Furquan Shaikh (Code Review) -
Julius Werner (Code Review)
-
Paul Menzel (Code Review)
-
Shelley Chen (Code Review)