Shelley Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46651 )
Change subject: sc7180: enable RECOVERY_MRC_CACHE ......................................................................
sc7180: enable RECOVERY_MRC_CACHE
Enable caching of memory training data for recovery as well as normal mode because memory training is taking too long in recovery as well. This required creating a space in the fmap for RECOVERY_MRC_CACHE.
BUG=b:150502246 BRANCH=None TEST=Run power_state:rec twice on lazor. Ensure that on first boot, memory training occurs and on second boot, memory training is skipped.
Change-Id: Id9059a8edd7527b0fe6cdc0447920d5ecbdf296e Signed-off-by: Shelley Chen shchen@google.com --- M src/mainboard/google/trogdor/chromeos.fmd M src/soc/qualcomm/common/qclib.c M src/soc/qualcomm/sc7180/Kconfig 3 files changed, 10 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/46651/1
diff --git a/src/mainboard/google/trogdor/chromeos.fmd b/src/mainboard/google/trogdor/chromeos.fmd index d5324ee..e7985f2 100644 --- a/src/mainboard/google/trogdor/chromeos.fmd +++ b/src/mainboard/google/trogdor/chromeos.fmd @@ -14,7 +14,10 @@
RW_VPD(PRESERVE) 32K RW_NVRAM(PRESERVE) 16K - RW_MRC_CACHE(PRESERVE) 8K + UNIFIED_MRC_CACHE 16K { + RECOVERY_MRC_CACHE(PRESERVE) 8K + RW_MRC_CACHE(PRESERVE) 8K + } RW_ELOG(PRESERVE) 4K RW_SHARED 4K { SHARED_DATA diff --git a/src/soc/qualcomm/common/qclib.c b/src/soc/qualcomm/common/qclib.c index d4796a2..9358890 100644 --- a/src/soc/qualcomm/common/qclib.c +++ b/src/soc/qualcomm/common/qclib.c @@ -74,14 +74,6 @@
} else if (!strncmp(QCLIB_TE_DDR_TRAINING_DATA, te->name, sizeof(te->name))) { - /* - * Don't store training data if we're in recovery mode - * because we always want to retrain due to - * possibility of RW training data possibly being - * updated to a different format. - */ - if (vboot_recovery_mode_enabled()) - return; assert(!mrc_cache_stash_data(MRC_TRAINING_DATA, QCLIB_VERSION, (const void *)te->blob_address, te->size));
@@ -138,20 +130,12 @@ /* output area, QCLib fills in DDR details */ qclib_add_if_table_entry(QCLIB_TE_DDR_INFORMATION, NULL, 0, 0);
- /* - * We never want to use training data when booting into - * recovery mode. - */ - if (vboot_recovery_mode_enabled()) { + /* Attempt to load DDR Training Blob */ + data_size = mrc_cache_load_current(MRC_TRAINING_DATA, QCLIB_VERSION, + _ddr_training, REGION_SIZE(ddr_training)); + if (data_size < 0) { + printk(BIOS_ERR, "Unable to load previous training data.\n"); memset(_ddr_training, 0, REGION_SIZE(ddr_training)); - } else { - /* Attempt to load DDR Training Blob */ - data_size = mrc_cache_load_current(MRC_TRAINING_DATA, QCLIB_VERSION, - _ddr_training, REGION_SIZE(ddr_training)); - if (data_size < 0) { - printk(BIOS_ERR, "Unable to load previous training data.\n"); - memset(_ddr_training, 0, REGION_SIZE(ddr_training)); - } } qclib_add_if_table_entry(QCLIB_TE_DDR_TRAINING_DATA, _ddr_training, REGION_SIZE(ddr_training), 0); diff --git a/src/soc/qualcomm/sc7180/Kconfig b/src/soc/qualcomm/sc7180/Kconfig index c66dc92..db819ad 100644 --- a/src/soc/qualcomm/sc7180/Kconfig +++ b/src/soc/qualcomm/sc7180/Kconfig @@ -18,6 +18,7 @@ select MAINBOARD_FORCE_NATIVE_VGA_INIT select HAVE_LINEAR_FRAMEBUFFER select CACHE_MRC_SETTINGS + select HAS_RECOVERY_MRC_CACHE select COMPRESS_BOOTBLOCK
if SOC_QUALCOMM_SC7180
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46651 )
Change subject: sc7180: enable RECOVERY_MRC_CACHE ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46651/1/src/mainboard/google/trogdo... File src/mainboard/google/trogdor/chromeos.fmd:
https://review.coreboot.org/c/coreboot/+/46651/1/src/mainboard/google/trogdo... PS1, Line 17: I think you should be able to set PRESERVE here for UNIFIED_MRC_CACHE and not worry about setting them individually for each of the caches below?
https://review.coreboot.org/c/coreboot/+/46651/1/src/soc/qualcomm/sc7180/Kco... File src/soc/qualcomm/sc7180/Kconfig:
https://review.coreboot.org/c/coreboot/+/46651/1/src/soc/qualcomm/sc7180/Kco... PS1, Line 21: HAS_RECOVERY_MRC_CACHE On Intel platforms, we have done this at the mainboard level so that it stays in sync with fmap changes to add the required MRC cache region and let's the board decide if it needs it depending upon whether it is using VBOOT.
Additionally, I think you would want to set the `MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN` config too.
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46651 )
Change subject: sc7180: enable RECOVERY_MRC_CACHE ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46651/1/src/soc/qualcomm/sc7180/Kco... File src/soc/qualcomm/sc7180/Kconfig:
https://review.coreboot.org/c/coreboot/+/46651/1/src/soc/qualcomm/sc7180/Kco... PS1, Line 21: HAS_RECOVERY_MRC_CACHE
Additionally, I think you would want to set the `MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN` config too.
Yes, this is already below on line 35. Shall I add it under VBOOT? It seems like for arm the defines are here instead.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46651 )
Change subject: sc7180: enable RECOVERY_MRC_CACHE ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46651/1/src/soc/qualcomm/sc7180/Kco... File src/soc/qualcomm/sc7180/Kconfig:
https://review.coreboot.org/c/coreboot/+/46651/1/src/soc/qualcomm/sc7180/Kco... PS1, Line 21: HAS_RECOVERY_MRC_CACHE
Yes, this is already below on line 35.
Ack.
Shall I add it under VBOOT? It seems like for arm the defines are here instead.
I am not sure if all boards (using VBOOT) for this SoC will really have a recovery MRC cache. But, if you/Julius think that this should be done here in SoC Kconfig, I am okay with that.
Hello build bot (Jenkins), Furquan Shaikh, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46651
to look at the new patch set (#2).
Change subject: sc7180: enable RECOVERY_MRC_CACHE ......................................................................
sc7180: enable RECOVERY_MRC_CACHE
Enable caching of memory training data for recovery as well as normal mode because memory training is taking too long in recovery as well. This required creating a space in the fmap for RECOVERY_MRC_CACHE.
BUG=b:150502246 BRANCH=None TEST=Run power_state:rec twice on lazor. Ensure that on first boot, memory training occurs and on second boot, memory training is skipped.
Change-Id: Id9059a8edd7527b0fe6cdc0447920d5ecbdf296e Signed-off-by: Shelley Chen shchen@google.com --- M src/mainboard/google/trogdor/chromeos.fmd M src/soc/qualcomm/common/qclib.c M src/soc/qualcomm/sc7180/Kconfig 3 files changed, 10 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/46651/2
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46651 )
Change subject: sc7180: enable RECOVERY_MRC_CACHE ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46651/1/src/mainboard/google/trogdo... File src/mainboard/google/trogdor/chromeos.fmd:
https://review.coreboot.org/c/coreboot/+/46651/1/src/mainboard/google/trogdo... PS1, Line 17:
I think you should be able to set PRESERVE here for UNIFIED_MRC_CACHE and not worry about setting th […]
Done
https://review.coreboot.org/c/coreboot/+/46651/1/src/soc/qualcomm/sc7180/Kco... File src/soc/qualcomm/sc7180/Kconfig:
https://review.coreboot.org/c/coreboot/+/46651/1/src/soc/qualcomm/sc7180/Kco... PS1, Line 21: HAS_RECOVERY_MRC_CACHE
Yes, this is already below on line 35. […]
Ok, I updated the fmap. Let me figure out what to do with the HAS_RECOVERY_MRC_CACHE with Julius.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46651 )
Change subject: sc7180: enable RECOVERY_MRC_CACHE ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/46651/1/src/soc/qualcomm/sc7180/Kco... File src/soc/qualcomm/sc7180/Kconfig:
https://review.coreboot.org/c/coreboot/+/46651/1/src/soc/qualcomm/sc7180/Kco... PS1, Line 21: HAS_RECOVERY_MRC_CACHE
Ok, I updated the fmap. Let me figure out what to do with the HAS_RECOVERY_MRC_CACHE with Julius.
I think putting it here is correct. Just disabling it at the mainboard level without adding extra code back into qclib.c would be incorrect. If the requirement for a mainboard that doesn't use this ever comes up we can figure out a more complicated solution, but for now I think this is fine.
Shelley Chen has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46651 )
Change subject: sc7180: enable RECOVERY_MRC_CACHE ......................................................................
sc7180: enable RECOVERY_MRC_CACHE
Enable caching of memory training data for recovery as well as normal mode because memory training is taking too long in recovery as well. This required creating a space in the fmap for RECOVERY_MRC_CACHE.
BUG=b:150502246 BRANCH=None TEST=Run power_state:rec twice on lazor. Ensure that on first boot, memory training occurs and on second boot, memory training is skipped.
Change-Id: Id9059a8edd7527b0fe6cdc0447920d5ecbdf296e Signed-off-by: Shelley Chen shchen@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/46651 Reviewed-by: Julius Werner jwerner@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/trogdor/chromeos.fmd M src/soc/qualcomm/common/qclib.c M src/soc/qualcomm/sc7180/Kconfig 3 files changed, 10 insertions(+), 22 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/mainboard/google/trogdor/chromeos.fmd b/src/mainboard/google/trogdor/chromeos.fmd index d5324ee..f696076 100644 --- a/src/mainboard/google/trogdor/chromeos.fmd +++ b/src/mainboard/google/trogdor/chromeos.fmd @@ -14,7 +14,10 @@
RW_VPD(PRESERVE) 32K RW_NVRAM(PRESERVE) 16K - RW_MRC_CACHE(PRESERVE) 8K + UNIFIED_MRC_CACHE(PRESERVE) 16K { + RECOVERY_MRC_CACHE 8K + RW_MRC_CACHE 8K + } RW_ELOG(PRESERVE) 4K RW_SHARED 4K { SHARED_DATA diff --git a/src/soc/qualcomm/common/qclib.c b/src/soc/qualcomm/common/qclib.c index d4796a2..9358890 100644 --- a/src/soc/qualcomm/common/qclib.c +++ b/src/soc/qualcomm/common/qclib.c @@ -74,14 +74,6 @@
} else if (!strncmp(QCLIB_TE_DDR_TRAINING_DATA, te->name, sizeof(te->name))) { - /* - * Don't store training data if we're in recovery mode - * because we always want to retrain due to - * possibility of RW training data possibly being - * updated to a different format. - */ - if (vboot_recovery_mode_enabled()) - return; assert(!mrc_cache_stash_data(MRC_TRAINING_DATA, QCLIB_VERSION, (const void *)te->blob_address, te->size));
@@ -138,20 +130,12 @@ /* output area, QCLib fills in DDR details */ qclib_add_if_table_entry(QCLIB_TE_DDR_INFORMATION, NULL, 0, 0);
- /* - * We never want to use training data when booting into - * recovery mode. - */ - if (vboot_recovery_mode_enabled()) { + /* Attempt to load DDR Training Blob */ + data_size = mrc_cache_load_current(MRC_TRAINING_DATA, QCLIB_VERSION, + _ddr_training, REGION_SIZE(ddr_training)); + if (data_size < 0) { + printk(BIOS_ERR, "Unable to load previous training data.\n"); memset(_ddr_training, 0, REGION_SIZE(ddr_training)); - } else { - /* Attempt to load DDR Training Blob */ - data_size = mrc_cache_load_current(MRC_TRAINING_DATA, QCLIB_VERSION, - _ddr_training, REGION_SIZE(ddr_training)); - if (data_size < 0) { - printk(BIOS_ERR, "Unable to load previous training data.\n"); - memset(_ddr_training, 0, REGION_SIZE(ddr_training)); - } } qclib_add_if_table_entry(QCLIB_TE_DDR_TRAINING_DATA, _ddr_training, REGION_SIZE(ddr_training), 0); diff --git a/src/soc/qualcomm/sc7180/Kconfig b/src/soc/qualcomm/sc7180/Kconfig index c37aff9..4cd1c41 100644 --- a/src/soc/qualcomm/sc7180/Kconfig +++ b/src/soc/qualcomm/sc7180/Kconfig @@ -19,6 +19,7 @@ select MAINBOARD_FORCE_NATIVE_VGA_INIT select HAVE_LINEAR_FRAMEBUFFER select CACHE_MRC_SETTINGS + select HAS_RECOVERY_MRC_CACHE select COMPRESS_BOOTBLOCK
if SOC_QUALCOMM_SC7180
-
Furquan Shaikh (Code Review) -
Julius Werner (Code Review)
-
Shelley Chen (Code Review)