Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32588
Change subject: intel/fsp1_1: Drop `boot_mode` from `pei_data` ......................................................................
intel/fsp1_1: Drop `boot_mode` from `pei_data`
It was only used locally.
Change-Id: Iaaad760e8ceca62655f5448c30846cf11959e8e1 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 5 files changed, 7 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/32588/1
diff --git a/src/drivers/intel/fsp1_1/include/fsp/romstage.h b/src/drivers/intel/fsp1_1/include/fsp/romstage.h index d608484..35cc103 100644 --- a/src/drivers/intel/fsp1_1/include/fsp/romstage.h +++ b/src/drivers/intel/fsp1_1/include/fsp/romstage.h @@ -76,7 +76,7 @@ void mainboard_add_dimm_info(struct romstage_params *params, struct memory_info *mem_info, int channel, int dimm, int index); -void raminit(struct romstage_params *params); +void raminit(struct romstage_params *params, bool s3wake); void report_memory_config(void); void romstage_common(struct romstage_params *params); asmlinkage void *romstage_main(FSP_INFO_HEADER *fih); diff --git a/src/drivers/intel/fsp1_1/raminit.c b/src/drivers/intel/fsp1_1/raminit.c index 8405c94..925feaa 100644 --- a/src/drivers/intel/fsp1_1/raminit.c +++ b/src/drivers/intel/fsp1_1/raminit.c @@ -26,7 +26,7 @@ #include <timestamp.h> #include <security/vboot/vboot_common.h>
-void raminit(struct romstage_params *params) +void raminit(struct romstage_params *params, bool s3wake) { const EFI_GUID bootldr_tolum_guid = FSP_BOOTLOADER_TOLUM_HOB_GUID; EFI_HOB_RESOURCE_DESCRIPTOR *cbmem_root; @@ -81,7 +81,7 @@ /* 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 (pei_ptr->boot_mode == ACPI_S3) { + if (s3wake) { fsp_rt_common_buffer.BootMode = BOOT_ON_S3_RESUME; } else if (pei_ptr->saved_data != NULL) { fsp_rt_common_buffer.BootMode = @@ -158,7 +158,7 @@
/* Migrate CAR data */ printk(BIOS_DEBUG, "0x%p: cbmem_top\n", cbmem_top()); - if (pei_ptr->boot_mode != ACPI_S3) { + if (!s3wake) { cbmem_initialize_empty_id_size(CBMEM_ID_FSP_RESERVED_MEMORY, fsp_reserved_bytes); } else if (cbmem_initialize_id_size(CBMEM_ID_FSP_RESERVED_MEMORY, diff --git a/src/drivers/intel/fsp1_1/romstage.c b/src/drivers/intel/fsp1_1/romstage.c index bfed121..a85cb2c 100644 --- a/src/drivers/intel/fsp1_1/romstage.c +++ b/src/drivers/intel/fsp1_1/romstage.c @@ -101,7 +101,6 @@ timestamp_add_now(TS_BEFORE_INITRAM);
pei_data = params->pei_data; - pei_data->boot_mode = params->power_state->prev_sleep_state; s3wake = params->power_state->prev_sleep_state == ACPI_S3;
if (CONFIG(ELOG_BOOT_COUNT) && !s3wake) @@ -129,7 +128,7 @@ params->pei_data->saved_data = rdev_mmap_full(&rdev); /* Assume boot device is memory mapped. */ assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); - } else if (params->pei_data->boot_mode == ACPI_S3) { + } else if (s3wake) { /* Waking from S3 and no cache. */ printk(BIOS_DEBUG, "No MRC cache found in S3 resume path.\n"); @@ -142,14 +141,14 @@ }
/* Initialize RAM */ - raminit(params); + raminit(params, s3wake); timestamp_add_now(TS_AFTER_INITRAM);
/* 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); - if ((params->pei_data->boot_mode != ACPI_S3) + if (!s3wake && (params->pei_data->data_to_save_size != 0) && (params->pei_data->data_to_save != NULL)) mrc_cache_stash_data(MRC_TRAINING_DATA, diff --git a/src/soc/intel/braswell/include/soc/pei_data.h b/src/soc/intel/braswell/include/soc/pei_data.h index 50aabed..7ea83ba 100644 --- a/src/soc/intel/braswell/include/soc/pei_data.h +++ b/src/soc/intel/braswell/include/soc/pei_data.h @@ -44,9 +44,6 @@ uint8_t spd_ch0_config; uint8_t spd_ch1_config;
- /* System state information */ - int boot_mode; - /* Fast boot and S3 resume MRC data */ int saved_data_size; const void *saved_data; diff --git a/src/soc/intel/skylake/include/soc/pei_data.h b/src/soc/intel/skylake/include/soc/pei_data.h index 02e04c6..7406a33 100644 --- a/src/soc/intel/skylake/include/soc/pei_data.h +++ b/src/soc/intel/skylake/include/soc/pei_data.h @@ -41,7 +41,6 @@ struct pei_data { uint32_t pei_version;
- int boot_mode; int ec_present;
/* Console output function */
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32588 )
Change subject: intel/fsp1_1: Drop `boot_mode` from `pei_data` ......................................................................
Patch Set 1: Code-Review+1
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32588 )
Change subject: intel/fsp1_1: Drop `boot_mode` from `pei_data` ......................................................................
Patch Set 1: Code-Review+2
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32588 )
Change subject: intel/fsp1_1: Drop `boot_mode` from `pei_data` ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32588/1/src/drivers/intel/fsp1_1/romstage.c File src/drivers/intel/fsp1_1/romstage.c:
https://review.coreboot.org/#/c/32588/1/src/drivers/intel/fsp1_1/romstage.c@... PS1, Line 144: raminit(params, s3wake); Any reason for not using params->power_state->prev_sleep_state in raminit()?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32588 )
Change subject: intel/fsp1_1: Drop `boot_mode` from `pei_data` ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32588/1/src/drivers/intel/fsp1_1/romstage.c File src/drivers/intel/fsp1_1/romstage.c:
https://review.coreboot.org/#/c/32588/1/src/drivers/intel/fsp1_1/romstage.c@... PS1, Line 144: raminit(params, s3wake);
Any reason for not using params->power_state->prev_sleep_state in raminit()?
Thanks. I lost track of all the `params` pointers. You are right, we already pass this information, I'll update.
Hello Patrick Rudolph, Huang Jin, Frans Hendriks, Lee Leahy, Matt DeVillier, build bot (Jenkins), Michał Żygowski,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32588
to look at the new patch set (#2).
Change subject: intel/fsp1_1: Drop `boot_mode` from `pei_data` ......................................................................
intel/fsp1_1: Drop `boot_mode` from `pei_data`
It was only used locally.
Change-Id: Iaaad760e8ceca62655f5448c30846cf11959e8e1 Signed-off-by: Nico Huber nico.h@gmx.de --- 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 4 files changed, 5 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/32588/2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32588 )
Change subject: intel/fsp1_1: Drop `boot_mode` from `pei_data` ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32588/1/src/drivers/intel/fsp1_1/romstage.c File src/drivers/intel/fsp1_1/romstage.c:
https://review.coreboot.org/#/c/32588/1/src/drivers/intel/fsp1_1/romstage.c@... PS1, Line 144: raminit(params, s3wake);
Thanks. I lost track of all the `params` pointers. You are right, […]
Done
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32588 )
Change subject: intel/fsp1_1: Drop `boot_mode` from `pei_data` ......................................................................
Patch Set 2: Code-Review+2
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32588 )
Change subject: intel/fsp1_1: Drop `boot_mode` from `pei_data` ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32588 )
Change subject: intel/fsp1_1: Drop `boot_mode` from `pei_data` ......................................................................
intel/fsp1_1: Drop `boot_mode` from `pei_data`
It was only used locally.
Change-Id: Iaaad760e8ceca62655f5448c30846cf11959e8e1 Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/32588 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Rudolph siro@das-labor.org Reviewed-by: Michał Żygowski michal.zygowski@3mdeb.com --- 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 4 files changed, 5 insertions(+), 9 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved Michał Żygowski: Looks good to me, approved
diff --git a/src/drivers/intel/fsp1_1/raminit.c b/src/drivers/intel/fsp1_1/raminit.c index 8405c94..4ea1f00 100644 --- a/src/drivers/intel/fsp1_1/raminit.c +++ b/src/drivers/intel/fsp1_1/raminit.c @@ -28,6 +28,7 @@
void raminit(struct romstage_params *params) { + const bool s3wake = params->power_state->prev_sleep_state == ACPI_S3; const EFI_GUID bootldr_tolum_guid = FSP_BOOTLOADER_TOLUM_HOB_GUID; EFI_HOB_RESOURCE_DESCRIPTOR *cbmem_root; FSP_INFO_HEADER *fsp_header; @@ -81,7 +82,7 @@ /* 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 (pei_ptr->boot_mode == ACPI_S3) { + if (s3wake) { fsp_rt_common_buffer.BootMode = BOOT_ON_S3_RESUME; } else if (pei_ptr->saved_data != NULL) { fsp_rt_common_buffer.BootMode = @@ -158,7 +159,7 @@
/* Migrate CAR data */ printk(BIOS_DEBUG, "0x%p: cbmem_top\n", cbmem_top()); - if (pei_ptr->boot_mode != ACPI_S3) { + if (!s3wake) { cbmem_initialize_empty_id_size(CBMEM_ID_FSP_RESERVED_MEMORY, fsp_reserved_bytes); } else if (cbmem_initialize_id_size(CBMEM_ID_FSP_RESERVED_MEMORY, diff --git a/src/drivers/intel/fsp1_1/romstage.c b/src/drivers/intel/fsp1_1/romstage.c index bfed121..17bd638 100644 --- a/src/drivers/intel/fsp1_1/romstage.c +++ b/src/drivers/intel/fsp1_1/romstage.c @@ -101,7 +101,6 @@ timestamp_add_now(TS_BEFORE_INITRAM);
pei_data = params->pei_data; - pei_data->boot_mode = params->power_state->prev_sleep_state; s3wake = params->power_state->prev_sleep_state == ACPI_S3;
if (CONFIG(ELOG_BOOT_COUNT) && !s3wake) @@ -129,7 +128,7 @@ params->pei_data->saved_data = rdev_mmap_full(&rdev); /* Assume boot device is memory mapped. */ assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); - } else if (params->pei_data->boot_mode == ACPI_S3) { + } else if (s3wake) { /* Waking from S3 and no cache. */ printk(BIOS_DEBUG, "No MRC cache found in S3 resume path.\n"); @@ -149,7 +148,7 @@ 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); - if ((params->pei_data->boot_mode != ACPI_S3) + if (!s3wake && (params->pei_data->data_to_save_size != 0) && (params->pei_data->data_to_save != NULL)) mrc_cache_stash_data(MRC_TRAINING_DATA, diff --git a/src/soc/intel/braswell/include/soc/pei_data.h b/src/soc/intel/braswell/include/soc/pei_data.h index 50aabed..7ea83ba 100644 --- a/src/soc/intel/braswell/include/soc/pei_data.h +++ b/src/soc/intel/braswell/include/soc/pei_data.h @@ -44,9 +44,6 @@ uint8_t spd_ch0_config; uint8_t spd_ch1_config;
- /* System state information */ - int boot_mode; - /* Fast boot and S3 resume MRC data */ int saved_data_size; const void *saved_data; diff --git a/src/soc/intel/skylake/include/soc/pei_data.h b/src/soc/intel/skylake/include/soc/pei_data.h index 02e04c6..7406a33 100644 --- a/src/soc/intel/skylake/include/soc/pei_data.h +++ b/src/soc/intel/skylake/include/soc/pei_data.h @@ -41,7 +41,6 @@ struct pei_data { uint32_t pei_version;
- int boot_mode; int ec_present;
/* Console output function */
-
Frans Hendriks (Code Review) -
Matt DeVillier (Code Review)
-
Michał Żygowski (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)