Jamie Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39139 )
Change subject: drivers/intel/fsp2_0: Add force memory retraining ......................................................................
drivers/intel/fsp2_0: Add force memory retraining
Add two weak functions: mainboard_prepare_dimm_info and is_the_same_memory_dimm. mainboard_preoare_dimm_info: for mainboard to provide its memory specific information. is_the_same_memory_dimm: identify the memory DIMMs are the same or not by compare spd cache and fspm_upd. It will trigger memory re-train if is_the_same_memory_dimm returns FALSE.
BUG=b:146457985 BRANCH=None TEST=1. build puff and boot up OS 2. power off and swap DDR4 memory dimm 3. power up and check log of coreboot to confirm that device re-train the DDR4 memory.
Change-Id: Id123191d478b0ec53b4c3fae5dd8b738487458fb Signed-off-by: Jamie Chen jamie.chen@intel.com --- M src/drivers/intel/fsp2_0/include/fsp/api.h M src/drivers/intel/fsp2_0/memory_init.c 2 files changed, 78 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/39139/1
diff --git a/src/drivers/intel/fsp2_0/include/fsp/api.h b/src/drivers/intel/fsp2_0/include/fsp/api.h index 60adb98..016a9d5 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/api.h +++ b/src/drivers/intel/fsp2_0/include/fsp/api.h @@ -83,6 +83,11 @@ void soc_update_memory_params_for_mma(FSP_M_CONFIG *memory_cfg, struct mma_config_param *mma_cfg);
+/* Provide specific DIMM information for recording in SPD cache. */ +void mainboard_prepare_dimm_info(FSPM_UPD *upd, void *data, size_t *data_sz); +/* Check if memory DIMMs are the same with previous time. */ +bool is_the_same_memory_dimm(FSPM_UPD *upd, const void *data, size_t *data_sz); + /* * # DOCUMENTATION: * diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c index 455dfa5..ea868ed 100644 --- a/src/drivers/intel/fsp2_0/memory_init.c +++ b/src/drivers/intel/fsp2_0/memory_init.c @@ -42,6 +42,20 @@ CONFIG(VBOOT_STARTS_IN_BOOTBLOCK), "for TPM MRC hash functionality, vboot must start in bootblock");
+static void save_memory_identify_info(FSPM_UPD *fspm_upd, uint32_t fsp_version) +{ + size_t spd_dimm_info_sz; + uint8_t spd_dimm_info[128]; + + spd_dimm_info_sz = sizeof(spd_dimm_info); + + mainboard_prepare_dimm_info(fspm_upd, spd_dimm_info, &spd_dimm_info_sz); + + if (mrc_cache_stash_data(MRC_SPD_DATA, fsp_version, spd_dimm_info, spd_dimm_info_sz) < 0) { + printk(BIOS_ERR, "Failed to stash SPD cache data\n"); + } +} + static void save_memory_training_data(bool s3wake, uint32_t fsp_version) { size_t mrc_data_size; @@ -265,6 +279,60 @@ return ver; }
+__weak +bool is_the_same_memory_dimm(FSPM_UPD *upd, const void *data, size_t *data_sz) +{ + return TRUE; +} + +__weak +void mainboard_prepare_dimm_info(FSPM_UPD *upd, void *data, size_t *data_sz) +{ + data = NULL; + *data_sz = 0; +} + +static void check_memory_dimm_identify_info(FSPM_UPD *fspm_upd, uint32_t fsp_version) +{ + size_t spd_cache_sz; + const void *spd_cache; + struct region_device rdev; + + if (mrc_cache_get_current(MRC_SPD_DATA, fsp_version, &rdev) < 0) { + printk(BIOS_ERR, "Cannt read dimm info from SPD_CACHE\n"); + return; + } + + /* Assume boot device is memory mapped. */ + assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); + spd_cache = rdev_mmap_full(&rdev); + + if (spd_cache == NULL) + return; + + if (CONFIG(FSP2_0_USES_TPM_MRC_HASH) && + !mrc_cache_verify_hash(spd_cache, region_device_sz(&rdev))) + return; + + /* SPD cache found */ + printk(BIOS_SPEW, "SPD cache found, size %zx\n", + region_device_sz(&rdev)); + + spd_cache = rdev_mmap_full(&rdev); + spd_cache_sz = region_device_sz(&rdev); + + printk(BIOS_ERR, "MRC_SPD_DATA at %p size=%ld\n", spd_cache, spd_cache_sz); + + // compare spd cache and spd info read from smbus + if (is_the_same_memory_dimm(fspm_upd, spd_cache, &spd_cache_sz)) + printk(BIOS_INFO, "jm: is the same dimm\n"); + else { + printk(BIOS_INFO, "jm: not the same dimm, set MRC cache to invalid\n"); + // invalid mrc cache + fspm_upd->FspmArchUpd.NvsBufferPtr = NULL; + } +} + static void do_fsp_memory_init(struct fsp_header *hdr, bool s3wake, const struct memranges *memmap) { @@ -304,6 +372,9 @@ if (CONFIG(MMA)) setup_mma(&fspm_upd.FspmConfig);
+ if (CONFIG(ROMSTAGE_SPD_SMBUS) && !s3wake) + check_memory_dimm_identify_info(&fspm_upd, fsp_version); + post_code(POST_MEM_PREINIT_PREP_END);
/* Call FspMemoryInit */ @@ -325,6 +396,8 @@ }
do_fsp_post_memory_init(s3wake, fsp_version); + if(CONFIG(ROMSTAGE_SPD_SMBUS) && !s3wake) + save_memory_identify_info(&fspm_upd, fsp_version);
/* * fsp_debug_after_memory_init() checks whether the end of the tolum
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39139 )
Change subject: drivers/intel/fsp2_0: Add force memory retraining ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39139/1/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/39139/1/src/drivers/intel/fsp2_0/me... PS1, Line 54: if (mrc_cache_stash_data(MRC_SPD_DATA, fsp_version, spd_dimm_info, spd_dimm_info_sz) < 0) { line over 96 characters
https://review.coreboot.org/c/coreboot/+/39139/1/src/drivers/intel/fsp2_0/me... PS1, Line 318: printk(BIOS_SPEW, "SPD cache found, size %zx\n", code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39139/1/src/drivers/intel/fsp2_0/me... PS1, Line 318: printk(BIOS_SPEW, "SPD cache found, size %zx\n", please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39139/1/src/drivers/intel/fsp2_0/me... PS1, Line 319: region_device_sz(&rdev)); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39139/1/src/drivers/intel/fsp2_0/me... PS1, Line 319: region_device_sz(&rdev)); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39139/1/src/drivers/intel/fsp2_0/me... PS1, Line 399: if(CONFIG(ROMSTAGE_SPD_SMBUS) && !s3wake) space required before the open parenthesis '('
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39139 )
Change subject: drivers/intel/fsp2_0: Add force memory retraining ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39139/1/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/39139/1/src/drivers/intel/fsp2_0/me... PS1, Line 282: __weak : bool is_the_same_memory_dimm(FSPM_UPD *upd, const void *data, size_t *data_sz) : { : return TRUE; : } : : __weak : void mainboard_prepare_dimm_info(FSPM_UPD *upd, void *data, size_t *data_sz) : { : data = NULL; : *data_sz = 0; : } Do you need these? You'd want a build failure if no implementation is provided instead of using invalid weakly linked functions.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39139 )
Change subject: drivers/intel/fsp2_0: Add force memory retraining ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39139/1/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/39139/1/src/drivers/intel/fsp2_0/me... PS1, Line 302: Cannt Cannot
https://review.coreboot.org/c/coreboot/+/39139/1/src/drivers/intel/fsp2_0/me... PS1, Line 324: printk(BIOS_ERR, "MRC_SPD_DATA at %p size=%ld\n", spd_cache, spd_cache_sz); Why error level?
https://review.coreboot.org/c/coreboot/+/39139/1/src/drivers/intel/fsp2_0/me... PS1, Line 328: jm: What does the prefix mean?
https://review.coreboot.org/c/coreboot/+/39139/1/src/drivers/intel/fsp2_0/me... PS1, Line 330: BIOS_INFO This should be error or warning level?
Jamie Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39139 )
Change subject: drivers/intel/fsp2_0: Add force memory retraining ......................................................................
Patch Set 1:
This change is ready for review.
Jamie Chen has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/39139 )
Change subject: drivers/intel/fsp2_0: Add force memory retraining ......................................................................
Abandoned
-
Arthur Heymans (Code Review) -
build bot (Jenkins) (Code Review)
-
Jamie Chen (Code Review)
-
Paul Menzel (Code Review)