Change in ...coreboot[master]: intel/fsp1_1: Move MRC cache pointers into `romstage_params`
Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32589
Change subject: intel/fsp1_1: Move MRC cache pointers into `romstage_params` ......................................................................
intel/fsp1_1: Move MRC cache pointers into `romstage_params`
These are part of a common concept and not SoC specific.
Change-Id: I9cb218d7825bd06a138f7f5d9e2b68e86077a3ec Signed-off-by: Nico Huber nico.h@gmx.de --- M src/drivers/intel/fsp1_1/include/fsp/romstage.h M src/drivers/intel/fsp1_1/raminit.c M src/drivers/intel/fsp1_1/romstage.c M src/soc/intel/braswell/include/soc/pei_data.h M src/soc/intel/skylake/include/soc/pei_data.h M src/soc/intel/skylake/pei_data.c M src/soc/intel/skylake/romstage/romstage.c 7 files changed, 31 insertions(+), 47 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/32589/1
diff --git a/src/drivers/intel/fsp1_1/include/fsp/romstage.h b/src/drivers/intel/fsp1_1/include/fsp/romstage.h index 35cc103..2e45f40 100644 --- a/src/drivers/intel/fsp1_1/include/fsp/romstage.h +++ b/src/drivers/intel/fsp1_1/include/fsp/romstage.h @@ -18,6 +18,7 @@ #ifndef _COMMON_ROMSTAGE_H_ #define _COMMON_ROMSTAGE_H_
+#include <stddef.h> #include <stdint.h> #include <arch/cpu.h> #include <memory_info.h> @@ -32,6 +33,15 @@ struct chipset_power_state *power_state; struct pei_data *pei_data; void *chipset_context; + + /* Fast boot and S3 resume MRC data */ + size_t saved_data_size; + const void *saved_data; + bool disable_saved_data; + + /* New save data from MRC */ + size_t data_to_save_size; + const void *data_to_save; };
/* diff --git a/src/drivers/intel/fsp1_1/raminit.c b/src/drivers/intel/fsp1_1/raminit.c index 925feaa..fcab375 100644 --- a/src/drivers/intel/fsp1_1/raminit.c +++ b/src/drivers/intel/fsp1_1/raminit.c @@ -46,7 +46,6 @@ u32 *mrc_hob; u32 fsp_reserved_bytes; MEMORY_INIT_UPD *original_params; - struct pei_data *pei_ptr; EFI_STATUS status; VPD_DATA_REGION *vpd_ptr; UPD_DATA_REGION *upd_ptr; @@ -80,10 +79,9 @@
/* Zero fill RT Buffer data and start populating fields. */ memset(&fsp_rt_common_buffer, 0, sizeof(fsp_rt_common_buffer)); - pei_ptr = params->pei_data; if (s3wake) { fsp_rt_common_buffer.BootMode = BOOT_ON_S3_RESUME; - } else if (pei_ptr->saved_data != NULL) { + } else if (params->saved_data != NULL) { fsp_rt_common_buffer.BootMode = BOOT_ASSUMING_NO_CONFIGURATION_CHANGES; } else { @@ -93,7 +91,7 @@ fsp_rt_common_buffer.BootLoaderTolumSize = cbmem_overhead_size();
/* Get any board specific changes */ - fsp_memory_init_params.NvsBufferPtr = (void *)pei_ptr->saved_data; + fsp_memory_init_params.NvsBufferPtr = (void *)params->saved_data; fsp_memory_init_params.RtBufferPtr = &fsp_rt_common_buffer; fsp_memory_init_params.HobListPtr = &hob_list_ptr;
@@ -220,7 +218,7 @@ } hob_ptr.Raw = get_next_guid_hob(&mrc_guid, hob_list_ptr); if (hob_ptr.Raw == NULL) { - if (params->pei_data->saved_data == NULL) { + if (params->saved_data == NULL) { printk(BIOS_ERR, "7.3: FSP_NON_VOLATILE_STORAGE_HOB missing!\n"); fsp_verification_failure = 1; } @@ -294,8 +292,8 @@ "Memory Configuration Data Hob not present\n"); else if (!vboot_recovery_mode_enabled()) { /* Do not save MRC data in recovery path */ - pei_ptr->data_to_save = GET_GUID_HOB_DATA(mrc_hob); - pei_ptr->data_to_save_size = ALIGN( + params->data_to_save = GET_GUID_HOB_DATA(mrc_hob); + params->data_to_save_size = ALIGN( ((u32)GET_HOB_LENGTH(mrc_hob)), 16); } } diff --git a/src/drivers/intel/fsp1_1/romstage.c b/src/drivers/intel/fsp1_1/romstage.c index a85cb2c..ff93565 100644 --- a/src/drivers/intel/fsp1_1/romstage.c +++ b/src/drivers/intel/fsp1_1/romstage.c @@ -94,13 +94,11 @@ { bool s3wake; struct region_device rdev; - struct pei_data *pei_data;
post_code(0x32);
timestamp_add_now(TS_BEFORE_INITRAM);
- pei_data = params->pei_data; s3wake = params->power_state->prev_sleep_state == ACPI_S3;
if (CONFIG(ELOG_BOOT_COUNT) && !s3wake) @@ -111,9 +109,9 @@ post_code(0x33);
/* Check recovery and MRC cache */ - params->pei_data->saved_data_size = 0; - params->pei_data->saved_data = NULL; - if (!params->pei_data->disable_saved_data) { + 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 */ printk(BIOS_DEBUG, @@ -123,9 +121,8 @@ params->fsp_version, &rdev))) { /* MRC cache found */ - params->pei_data->saved_data_size = - region_device_sz(&rdev); - params->pei_data->saved_data = rdev_mmap_full(&rdev); + params->saved_data_size = region_device_sz(&rdev); + params->saved_data = rdev_mmap_full(&rdev); /* Assume boot device is memory mapped. */ assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); } else if (s3wake) { @@ -147,14 +144,14 @@ /* Save MRC output */ if (CONFIG(CACHE_MRC_SETTINGS)) { printk(BIOS_DEBUG, "MRC data at %p %d bytes\n", - pei_data->data_to_save, pei_data->data_to_save_size); + params->data_to_save, params->data_to_save_size); if (!s3wake - && (params->pei_data->data_to_save_size != 0) - && (params->pei_data->data_to_save != NULL)) + && (params->data_to_save_size != 0) + && (params->data_to_save != NULL)) mrc_cache_stash_data(MRC_TRAINING_DATA, params->fsp_version, - params->pei_data->data_to_save, - params->pei_data->data_to_save_size); + params->data_to_save, + params->data_to_save_size); }
/* Save DIMM information */ diff --git a/src/soc/intel/braswell/include/soc/pei_data.h b/src/soc/intel/braswell/include/soc/pei_data.h index 7ea83ba..df18dc0 100644 --- a/src/soc/intel/braswell/include/soc/pei_data.h +++ b/src/soc/intel/braswell/include/soc/pei_data.h @@ -43,15 +43,6 @@ void *spd_data_ch1; uint8_t spd_ch0_config; uint8_t spd_ch1_config; - - /* Fast boot and S3 resume MRC data */ - int saved_data_size; - const void *saved_data; - int disable_saved_data; - - /* New save data from MRC */ - int data_to_save_size; - void *data_to_save; };
typedef struct pei_data PEI_DATA; diff --git a/src/soc/intel/skylake/include/soc/pei_data.h b/src/soc/intel/skylake/include/soc/pei_data.h index 7406a33..5ea2190 100644 --- a/src/soc/intel/skylake/include/soc/pei_data.h +++ b/src/soc/intel/skylake/include/soc/pei_data.h @@ -80,16 +80,7 @@ uint8_t dqs_map[2][8]; uint16_t RcompResistor[3]; uint16_t RcompTarget[5]; - /* Data read from flash and passed into MRC */ - const void *saved_data; - int saved_data_size;
- /* Disable use of saved data (can be set by mainboard) */ - int disable_saved_data; - - /* Data from MRC that should be saved to flash */ - void *data_to_save; - int data_to_save_size; int mem_cfg_id; } __packed;
diff --git a/src/soc/intel/skylake/pei_data.c b/src/soc/intel/skylake/pei_data.c index 203a1d8..7d314a1 100644 --- a/src/soc/intel/skylake/pei_data.c +++ b/src/soc/intel/skylake/pei_data.c @@ -33,16 +33,7 @@
void soc_fill_pei_data(struct pei_data *pei_data) { - const struct device *dev; - const struct soc_intel_skylake_config *config; - /* Set the parameters for MemoryInit */ - dev = dev_find_slot(0, PCH_DEVFN_LPC); - config = dev->chip_info; - pei_data->pei_version = PEI_VERSION; pei_data->tx_byte = &send_to_console; - - /* Force a full memory train if RMT is enabled */ - pei_data->disable_saved_data = config->Rmt; } diff --git a/src/soc/intel/skylake/romstage/romstage.c b/src/soc/intel/skylake/romstage/romstage.c index 0501b04..8ec08c2 100644 --- a/src/soc/intel/skylake/romstage/romstage.c +++ b/src/soc/intel/skylake/romstage/romstage.c @@ -46,6 +46,12 @@
/* Prepare to initialize memory */ soc_fill_pei_data(params->pei_data); + + const struct device *const dev = pcidev_path_on_root(PCH_DEVFN_LPC); + const struct soc_intel_skylake_config *const config = + dev ? dev->chip_info : NULL; + /* Force a full memory train if RMT is enabled */ + params->disable_saved_data = config && config->Rmt; }
/* UPD parameters to be initialized before MemoryInit */
Hello Patrick Rudolph, Huang Jin, Lee Leahy, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32589
to look at the new patch set (#2).
Change subject: intel/fsp1_1: Move MRC cache pointers into `romstage_params` ......................................................................
intel/fsp1_1: Move MRC cache pointers into `romstage_params`
These are part of a common concept and not SoC specific.
Change-Id: I9cb218d7825bd06a138f7f5d9e2b68e86077a3ec Signed-off-by: Nico Huber nico.h@gmx.de --- M src/drivers/intel/fsp1_1/include/fsp/romstage.h M src/drivers/intel/fsp1_1/raminit.c M src/drivers/intel/fsp1_1/romstage.c M src/soc/intel/braswell/include/soc/pei_data.h M src/soc/intel/skylake/include/soc/pei_data.h M src/soc/intel/skylake/pei_data.c M src/soc/intel/skylake/romstage/romstage.c 7 files changed, 32 insertions(+), 48 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/32589/2
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32589 )
Change subject: intel/fsp1_1: Move MRC cache pointers into `romstage_params` ......................................................................
Patch Set 2: Code-Review+1
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32589 )
Change subject: intel/fsp1_1: Move MRC cache pointers into `romstage_params` ......................................................................
Patch Set 2: Code-Review+2
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32589 )
Change subject: intel/fsp1_1: Move MRC cache pointers into `romstage_params` ......................................................................
Patch Set 2: Code-Review+1
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32589 )
Change subject: intel/fsp1_1: Move MRC cache pointers into `romstage_params` ......................................................................
intel/fsp1_1: Move MRC cache pointers into `romstage_params`
These are part of a common concept and not SoC specific.
Change-Id: I9cb218d7825bd06a138f7f5d9e2b68e86077a3ec Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/32589 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Matt DeVillier matt.devillier@gmail.com Reviewed-by: Patrick Rudolph siro@das-labor.org Reviewed-by: Frans Hendriks fhendriks@eltan.com --- M src/drivers/intel/fsp1_1/include/fsp/romstage.h M src/drivers/intel/fsp1_1/raminit.c M src/drivers/intel/fsp1_1/romstage.c M src/soc/intel/braswell/include/soc/pei_data.h M src/soc/intel/skylake/include/soc/pei_data.h M src/soc/intel/skylake/pei_data.c M src/soc/intel/skylake/romstage/romstage.c 7 files changed, 32 insertions(+), 48 deletions(-)
Approvals: build bot (Jenkins): Verified Matt DeVillier: Looks good to me, but someone else must approve Patrick Rudolph: Looks good to me, approved Frans Hendriks: Looks good to me, but someone else must approve
diff --git a/src/drivers/intel/fsp1_1/include/fsp/romstage.h b/src/drivers/intel/fsp1_1/include/fsp/romstage.h index d608484..4e95dad 100644 --- a/src/drivers/intel/fsp1_1/include/fsp/romstage.h +++ b/src/drivers/intel/fsp1_1/include/fsp/romstage.h @@ -18,6 +18,7 @@ #ifndef _COMMON_ROMSTAGE_H_ #define _COMMON_ROMSTAGE_H_
+#include <stddef.h> #include <stdint.h> #include <arch/cpu.h> #include <memory_info.h> @@ -32,6 +33,15 @@ struct chipset_power_state *power_state; struct pei_data *pei_data; void *chipset_context; + + /* Fast boot and S3 resume MRC data */ + size_t saved_data_size; + const void *saved_data; + bool disable_saved_data; + + /* New save data from MRC */ + size_t data_to_save_size; + const void *data_to_save; };
/* diff --git a/src/drivers/intel/fsp1_1/raminit.c b/src/drivers/intel/fsp1_1/raminit.c index 4ea1f00..726cc26 100644 --- a/src/drivers/intel/fsp1_1/raminit.c +++ b/src/drivers/intel/fsp1_1/raminit.c @@ -47,7 +47,6 @@ u32 *mrc_hob; u32 fsp_reserved_bytes; MEMORY_INIT_UPD *original_params; - struct pei_data *pei_ptr; EFI_STATUS status; VPD_DATA_REGION *vpd_ptr; UPD_DATA_REGION *upd_ptr; @@ -81,10 +80,9 @@
/* Zero fill RT Buffer data and start populating fields. */ memset(&fsp_rt_common_buffer, 0, sizeof(fsp_rt_common_buffer)); - pei_ptr = params->pei_data; if (s3wake) { fsp_rt_common_buffer.BootMode = BOOT_ON_S3_RESUME; - } else if (pei_ptr->saved_data != NULL) { + } else if (params->saved_data != NULL) { fsp_rt_common_buffer.BootMode = BOOT_ASSUMING_NO_CONFIGURATION_CHANGES; } else { @@ -94,7 +92,7 @@ fsp_rt_common_buffer.BootLoaderTolumSize = cbmem_overhead_size();
/* Get any board specific changes */ - fsp_memory_init_params.NvsBufferPtr = (void *)pei_ptr->saved_data; + fsp_memory_init_params.NvsBufferPtr = (void *)params->saved_data; fsp_memory_init_params.RtBufferPtr = &fsp_rt_common_buffer; fsp_memory_init_params.HobListPtr = &hob_list_ptr;
@@ -221,7 +219,7 @@ } hob_ptr.Raw = get_next_guid_hob(&mrc_guid, hob_list_ptr); if (hob_ptr.Raw == NULL) { - if (params->pei_data->saved_data == NULL) { + if (params->saved_data == NULL) { printk(BIOS_ERR, "7.3: FSP_NON_VOLATILE_STORAGE_HOB missing!\n"); fsp_verification_failure = 1; } @@ -295,8 +293,8 @@ "Memory Configuration Data Hob not present\n"); else if (!vboot_recovery_mode_enabled()) { /* Do not save MRC data in recovery path */ - pei_ptr->data_to_save = GET_GUID_HOB_DATA(mrc_hob); - pei_ptr->data_to_save_size = ALIGN( + params->data_to_save = GET_GUID_HOB_DATA(mrc_hob); + params->data_to_save_size = ALIGN( ((u32)GET_HOB_LENGTH(mrc_hob)), 16); } } diff --git a/src/drivers/intel/fsp1_1/romstage.c b/src/drivers/intel/fsp1_1/romstage.c index 17bd638..87fd1a4 100644 --- a/src/drivers/intel/fsp1_1/romstage.c +++ b/src/drivers/intel/fsp1_1/romstage.c @@ -94,13 +94,11 @@ { bool s3wake; struct region_device rdev; - struct pei_data *pei_data;
post_code(0x32);
timestamp_add_now(TS_BEFORE_INITRAM);
- pei_data = params->pei_data; s3wake = params->power_state->prev_sleep_state == ACPI_S3;
if (CONFIG(ELOG_BOOT_COUNT) && !s3wake) @@ -111,9 +109,9 @@ post_code(0x33);
/* Check recovery and MRC cache */ - params->pei_data->saved_data_size = 0; - params->pei_data->saved_data = NULL; - if (!params->pei_data->disable_saved_data) { + 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 */ printk(BIOS_DEBUG, @@ -123,9 +121,8 @@ params->fsp_version, &rdev))) { /* MRC cache found */ - params->pei_data->saved_data_size = - region_device_sz(&rdev); - params->pei_data->saved_data = rdev_mmap_full(&rdev); + params->saved_data_size = region_device_sz(&rdev); + params->saved_data = rdev_mmap_full(&rdev); /* Assume boot device is memory mapped. */ assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); } else if (s3wake) { @@ -146,15 +143,15 @@
/* Save MRC output */ if (CONFIG(CACHE_MRC_SETTINGS)) { - printk(BIOS_DEBUG, "MRC data at %p %d bytes\n", - pei_data->data_to_save, pei_data->data_to_save_size); + printk(BIOS_DEBUG, "MRC data at %p %zu bytes\n", + params->data_to_save, params->data_to_save_size); if (!s3wake - && (params->pei_data->data_to_save_size != 0) - && (params->pei_data->data_to_save != NULL)) + && (params->data_to_save_size != 0) + && (params->data_to_save != NULL)) mrc_cache_stash_data(MRC_TRAINING_DATA, params->fsp_version, - params->pei_data->data_to_save, - params->pei_data->data_to_save_size); + params->data_to_save, + params->data_to_save_size); }
/* Save DIMM information */ diff --git a/src/soc/intel/braswell/include/soc/pei_data.h b/src/soc/intel/braswell/include/soc/pei_data.h index 7ea83ba..df18dc0 100644 --- a/src/soc/intel/braswell/include/soc/pei_data.h +++ b/src/soc/intel/braswell/include/soc/pei_data.h @@ -43,15 +43,6 @@ void *spd_data_ch1; uint8_t spd_ch0_config; uint8_t spd_ch1_config; - - /* Fast boot and S3 resume MRC data */ - int saved_data_size; - const void *saved_data; - int disable_saved_data; - - /* New save data from MRC */ - int data_to_save_size; - void *data_to_save; };
typedef struct pei_data PEI_DATA; diff --git a/src/soc/intel/skylake/include/soc/pei_data.h b/src/soc/intel/skylake/include/soc/pei_data.h index 7406a33..5ea2190 100644 --- a/src/soc/intel/skylake/include/soc/pei_data.h +++ b/src/soc/intel/skylake/include/soc/pei_data.h @@ -80,16 +80,7 @@ uint8_t dqs_map[2][8]; uint16_t RcompResistor[3]; uint16_t RcompTarget[5]; - /* Data read from flash and passed into MRC */ - const void *saved_data; - int saved_data_size;
- /* Disable use of saved data (can be set by mainboard) */ - int disable_saved_data; - - /* Data from MRC that should be saved to flash */ - void *data_to_save; - int data_to_save_size; int mem_cfg_id; } __packed;
diff --git a/src/soc/intel/skylake/pei_data.c b/src/soc/intel/skylake/pei_data.c index 203a1d8..7d314a1 100644 --- a/src/soc/intel/skylake/pei_data.c +++ b/src/soc/intel/skylake/pei_data.c @@ -33,16 +33,7 @@
void soc_fill_pei_data(struct pei_data *pei_data) { - const struct device *dev; - const struct soc_intel_skylake_config *config; - /* Set the parameters for MemoryInit */ - dev = dev_find_slot(0, PCH_DEVFN_LPC); - config = dev->chip_info; - pei_data->pei_version = PEI_VERSION; pei_data->tx_byte = &send_to_console; - - /* Force a full memory train if RMT is enabled */ - pei_data->disable_saved_data = config->Rmt; } diff --git a/src/soc/intel/skylake/romstage/romstage.c b/src/soc/intel/skylake/romstage/romstage.c index 0501b04..8ec08c2 100644 --- a/src/soc/intel/skylake/romstage/romstage.c +++ b/src/soc/intel/skylake/romstage/romstage.c @@ -46,6 +46,12 @@
/* Prepare to initialize memory */ soc_fill_pei_data(params->pei_data); + + const struct device *const dev = pcidev_path_on_root(PCH_DEVFN_LPC); + const struct soc_intel_skylake_config *const config = + dev ? dev->chip_info : NULL; + /* Force a full memory train if RMT is enabled */ + params->disable_saved_data = config && config->Rmt; }
/* UPD parameters to be initialized before MemoryInit */
-
Frans Hendriks (Code Review) -
Matt DeVillier (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)