Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32593
Change subject: mb/google/cyan: Refactor to get rid of `pei_data` ......................................................................
mb/google/cyan: Refactor to get rid of `pei_data`
The SoC specific `struct pei_data` was filled with values that were later only consumed by the mainboard code again. Avoid jumping through this hoop and fill FSP UPDs directly.
The provided solution locates the SPD data in CBFS again to fill SMBIOS tables. This is not perfect. OTOH, this code isn't mainboard specific and doesn't belong here anyway.
Change-Id: Ib6103d5b9550846fe17c926631a013ff80b9598f Signed-off-by: Nico Huber nico.h@gmx.de --- M src/mainboard/google/cyan/romstage.c M src/mainboard/google/cyan/spd/spd.c M src/mainboard/google/cyan/spd/spd_util.h 3 files changed, 43 insertions(+), 39 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/32593/1
diff --git a/src/mainboard/google/cyan/romstage.c b/src/mainboard/google/cyan/romstage.c index 5470b9c..5d47339 100644 --- a/src/mainboard/google/cyan/romstage.c +++ b/src/mainboard/google/cyan/romstage.c @@ -17,13 +17,11 @@ #include <soc/romstage.h> #include <baseboard/variants.h>
+#include "spd/spd_util.h" + /* All FSP specific code goes in this block */ void mainboard_romstage_entry(struct romstage_params *rp) { - struct pei_data *ps = rp->pei_data; - - mainboard_fill_spd_data(ps); - /* Call back into chipset code with platform values updated. */ romstage_common(rp); } @@ -31,16 +29,7 @@ void mainboard_memory_init_params(struct romstage_params *params, MEMORY_INIT_UPD *memory_params) { - /* Update SPD data */ - if (CONFIG(BOARD_GOOGLE_CYAN)) { - memory_params->PcdMemoryTypeEnable = MEM_DDR3; - memory_params->PcdMemorySpdPtr = - (u32)params->pei_data->spd_data_ch0; - } else - memory_params->PcdMemoryTypeEnable = MEM_LPDDR3; - - memory_params->PcdMemChannel0Config = params->pei_data->spd_ch0_config; - memory_params->PcdMemChannel1Config = params->pei_data->spd_ch1_config; + mainboard_fill_spd_data(memory_params);
/* Variant-specific memory params */ variant_memory_init_params(memory_params); diff --git a/src/mainboard/google/cyan/spd/spd.c b/src/mainboard/google/cyan/spd/spd.c index af694a4..01d8463 100644 --- a/src/mainboard/google/cyan/spd/spd.c +++ b/src/mainboard/google/cyan/spd/spd.c @@ -40,11 +40,24 @@ return gpio_base2_value(spd_gpios, ARRAY_SIZE(spd_gpios)); }
-static void *get_spd_pointer(char *spd_file_content, int total_spds, int *dual) +static void *get_spd_pointer(int *dual) { + char *spd_file; + size_t spd_file_len; + int total_spds; int ram_id = 0; int spd_index = 0;
+ /* Find the SPD data in CBFS. */ + spd_file = cbfs_boot_map_with_leak("spd.bin", CBFS_TYPE_SPD, + &spd_file_len); + if (!spd_file) + die("SPD data not found."); + + if (spd_file_len < SPD_PAGE_LEN) + die("Missing SPD data."); + total_spds = spd_file_len / SPD_PAGE_LEN; + ram_id = get_ramid(); printk(BIOS_DEBUG, "ram_id=%d, total_spds: %d\n", ram_id, total_spds);
@@ -54,33 +67,20 @@ return NULL; } /* Return the serial product data for the RAM */ - return &spd_file_content[SPD_PAGE_LEN * spd_index]; + return &spd_file[SPD_PAGE_LEN * spd_index]; }
/* Copy SPD data for on-board memory */ -void mainboard_fill_spd_data(struct pei_data *ps) +void mainboard_fill_spd_data(MEMORY_INIT_UPD *memory_params) { - char *spd_file; - size_t spd_file_len; void *spd_content; int dual_channel = 0;
- /* Find the SPD data in CBFS. */ - spd_file = cbfs_boot_map_with_leak("spd.bin", CBFS_TYPE_SPD, - &spd_file_len); - if (!spd_file) - die("SPD data not found."); - - if (spd_file_len < SPD_PAGE_LEN) - die("Missing SPD data."); - /* * Both channels are always present in SPD data. Always use matched * DIMMs so use the same SPD data for each DIMM. */ - spd_content = get_spd_pointer(spd_file, - spd_file_len / SPD_PAGE_LEN, - &dual_channel); + spd_content = get_spd_pointer(&dual_channel); if (CONFIG(DISPLAY_SPD_DATA) && spd_content != NULL) { printk(BIOS_DEBUG, "SPD Data:\n"); hexdump(spd_content, SPD_PAGE_LEN); @@ -94,18 +94,24 @@ * 2=DimmDisabled */ if (spd_content != NULL) { - ps->spd_data_ch0 = spd_content; - ps->spd_ch0_config = 1; + memory_params->PcdMemChannel0Config = 1; printk(BIOS_DEBUG, "Channel 0 DIMM soldered down\n"); if (dual_channel) { printk(BIOS_DEBUG, "Channel 1 DIMM soldered down\n"); - ps->spd_data_ch1 = spd_content; - ps->spd_ch1_config = 1; + memory_params->PcdMemChannel1Config = 1; } else { printk(BIOS_DEBUG, "Channel 1 DIMM not installed\n"); - ps->spd_ch1_config = 2; + memory_params->PcdMemChannel1Config = 2; } } + + /* Update SPD data */ + if (CONFIG(BOARD_GOOGLE_CYAN)) { + memory_params->PcdMemoryTypeEnable = MEM_DDR3; + memory_params->PcdMemorySpdPtr = spd_content; + } else { + memory_params->PcdMemoryTypeEnable = MEM_LPDDR3; + } }
static void set_dimm_info(uint8_t *spd, struct dimm_info *dimm) @@ -171,9 +177,15 @@
void mainboard_save_dimm_info(struct romstage_params *params) { + const void *spd_content; + int dual_channel; struct dimm_info *dimm; struct memory_info *mem_info;
+ spd_content = get_spd_pointer(&dual_channel); + if (spd_content == NULL) + return; + /* * Allocate CBMEM area for DIMM information used to populate SMBIOS * table 17 @@ -186,13 +198,13 @@
/* Describe the first channel memory */ dimm = &mem_info->dimm[0]; - set_dimm_info(params->pei_data->spd_data_ch0, dimm); + set_dimm_info(spd_content, dimm); mem_info->dimm_cnt = 1;
/* Describe the second channel memory */ - if (params->pei_data->spd_ch1_config == 1) { + if (dual_channel) { dimm = &mem_info->dimm[1]; - set_dimm_info(params->pei_data->spd_data_ch1, dimm); + set_dimm_info(spd_content, dimm); dimm->channel_num = 1; mem_info->dimm_cnt = 2; } diff --git a/src/mainboard/google/cyan/spd/spd_util.h b/src/mainboard/google/cyan/spd/spd_util.h index 11d6eaa..42bd794 100644 --- a/src/mainboard/google/cyan/spd/spd_util.h +++ b/src/mainboard/google/cyan/spd/spd_util.h @@ -16,7 +16,10 @@ #ifndef SPD_UTIL_H #define SPD_UTIL_H
+#include <fsp/soc_binding.h> + uint8_t get_ramid(void); int get_variant_spd_index(int ram_id, int *dual); +void mainboard_fill_spd_data(MEMORY_INIT_UPD *memory_params);
#endif /* SPD_UTIL_H */
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32593
to look at the new patch set (#2).
Change subject: mb/google/cyan: Refactor to get rid of `pei_data` ......................................................................
mb/google/cyan: Refactor to get rid of `pei_data`
The SoC specific `struct pei_data` was filled with values that were later only consumed by the mainboard code again. Avoid jumping through this hoop and fill FSP UPDs directly.
The provided solution locates the SPD data in CBFS again to fill SMBIOS tables. This is not perfect. OTOH, this code isn't mainboard specific and doesn't belong here anyway.
Change-Id: Ib6103d5b9550846fe17c926631a013ff80b9598f Signed-off-by: Nico Huber nico.h@gmx.de --- M src/mainboard/google/cyan/romstage.c M src/mainboard/google/cyan/spd/spd.c M src/mainboard/google/cyan/spd/spd_util.h 3 files changed, 44 insertions(+), 40 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/32593/2
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32593 )
Change subject: mb/google/cyan: Refactor to get rid of `pei_data` ......................................................................
Patch Set 2: Code-Review+1
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32593 )
Change subject: mb/google/cyan: Refactor to get rid of `pei_data` ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32593 )
Change subject: mb/google/cyan: Refactor to get rid of `pei_data` ......................................................................
mb/google/cyan: Refactor to get rid of `pei_data`
The SoC specific `struct pei_data` was filled with values that were later only consumed by the mainboard code again. Avoid jumping through this hoop and fill FSP UPDs directly.
The provided solution locates the SPD data in CBFS again to fill SMBIOS tables. This is not perfect. OTOH, this code isn't mainboard specific and doesn't belong here anyway.
Change-Id: Ib6103d5b9550846fe17c926631a013ff80b9598f Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/32593 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 --- M src/mainboard/google/cyan/romstage.c M src/mainboard/google/cyan/spd/spd.c M src/mainboard/google/cyan/spd/spd_util.h 3 files changed, 44 insertions(+), 40 deletions(-)
Approvals: build bot (Jenkins): Verified Matt DeVillier: Looks good to me, but someone else must approve Patrick Rudolph: Looks good to me, approved
diff --git a/src/mainboard/google/cyan/romstage.c b/src/mainboard/google/cyan/romstage.c index 5470b9c..c877e42 100644 --- a/src/mainboard/google/cyan/romstage.c +++ b/src/mainboard/google/cyan/romstage.c @@ -17,13 +17,11 @@ #include <soc/romstage.h> #include <baseboard/variants.h>
+#include "spd/spd_util.h" + /* All FSP specific code goes in this block */ void mainboard_romstage_entry(struct romstage_params *rp) { - struct pei_data *ps = rp->pei_data; - - mainboard_fill_spd_data(ps); - /* Call back into chipset code with platform values updated. */ romstage_common(rp); } @@ -31,16 +29,7 @@ void mainboard_memory_init_params(struct romstage_params *params, MEMORY_INIT_UPD *memory_params) { - /* Update SPD data */ - if (CONFIG(BOARD_GOOGLE_CYAN)) { - memory_params->PcdMemoryTypeEnable = MEM_DDR3; - memory_params->PcdMemorySpdPtr = - (u32)params->pei_data->spd_data_ch0; - } else - memory_params->PcdMemoryTypeEnable = MEM_LPDDR3; - - memory_params->PcdMemChannel0Config = params->pei_data->spd_ch0_config; - memory_params->PcdMemChannel1Config = params->pei_data->spd_ch1_config; + spd_memory_init_params(memory_params);
/* Variant-specific memory params */ variant_memory_init_params(memory_params); diff --git a/src/mainboard/google/cyan/spd/spd.c b/src/mainboard/google/cyan/spd/spd.c index af694a4..7c66e94 100644 --- a/src/mainboard/google/cyan/spd/spd.c +++ b/src/mainboard/google/cyan/spd/spd.c @@ -40,11 +40,24 @@ return gpio_base2_value(spd_gpios, ARRAY_SIZE(spd_gpios)); }
-static void *get_spd_pointer(char *spd_file_content, int total_spds, int *dual) +static void *get_spd_pointer(int *dual) { + char *spd_file; + size_t spd_file_len; + int total_spds; int ram_id = 0; int spd_index = 0;
+ /* Find the SPD data in CBFS. */ + spd_file = cbfs_boot_map_with_leak("spd.bin", CBFS_TYPE_SPD, + &spd_file_len); + if (!spd_file) + die("SPD data not found."); + + if (spd_file_len < SPD_PAGE_LEN) + die("Missing SPD data."); + total_spds = spd_file_len / SPD_PAGE_LEN; + ram_id = get_ramid(); printk(BIOS_DEBUG, "ram_id=%d, total_spds: %d\n", ram_id, total_spds);
@@ -54,33 +67,20 @@ return NULL; } /* Return the serial product data for the RAM */ - return &spd_file_content[SPD_PAGE_LEN * spd_index]; + return &spd_file[SPD_PAGE_LEN * spd_index]; }
/* Copy SPD data for on-board memory */ -void mainboard_fill_spd_data(struct pei_data *ps) +void spd_memory_init_params(MEMORY_INIT_UPD *memory_params) { - char *spd_file; - size_t spd_file_len; void *spd_content; int dual_channel = 0;
- /* Find the SPD data in CBFS. */ - spd_file = cbfs_boot_map_with_leak("spd.bin", CBFS_TYPE_SPD, - &spd_file_len); - if (!spd_file) - die("SPD data not found."); - - if (spd_file_len < SPD_PAGE_LEN) - die("Missing SPD data."); - /* * Both channels are always present in SPD data. Always use matched * DIMMs so use the same SPD data for each DIMM. */ - spd_content = get_spd_pointer(spd_file, - spd_file_len / SPD_PAGE_LEN, - &dual_channel); + spd_content = get_spd_pointer(&dual_channel); if (CONFIG(DISPLAY_SPD_DATA) && spd_content != NULL) { printk(BIOS_DEBUG, "SPD Data:\n"); hexdump(spd_content, SPD_PAGE_LEN); @@ -94,21 +94,27 @@ * 2=DimmDisabled */ if (spd_content != NULL) { - ps->spd_data_ch0 = spd_content; - ps->spd_ch0_config = 1; + memory_params->PcdMemChannel0Config = 1; printk(BIOS_DEBUG, "Channel 0 DIMM soldered down\n"); if (dual_channel) { printk(BIOS_DEBUG, "Channel 1 DIMM soldered down\n"); - ps->spd_data_ch1 = spd_content; - ps->spd_ch1_config = 1; + memory_params->PcdMemChannel1Config = 1; } else { printk(BIOS_DEBUG, "Channel 1 DIMM not installed\n"); - ps->spd_ch1_config = 2; + memory_params->PcdMemChannel1Config = 2; } } + + /* Update SPD data */ + if (CONFIG(BOARD_GOOGLE_CYAN)) { + memory_params->PcdMemoryTypeEnable = MEM_DDR3; + memory_params->PcdMemorySpdPtr = (uintptr_t)spd_content; + } else { + memory_params->PcdMemoryTypeEnable = MEM_LPDDR3; + } }
-static void set_dimm_info(uint8_t *spd, struct dimm_info *dimm) +static void set_dimm_info(const uint8_t *spd, struct dimm_info *dimm) { const int spd_capmb[8] = { 1, 2, 4, 8, 16, 32, 64, 0 }; const int spd_ranks[8] = { 1, 2, 3, 4, -1, -1, -1, -1 }; @@ -171,9 +177,15 @@
void mainboard_save_dimm_info(struct romstage_params *params) { + const void *spd_content; + int dual_channel; struct dimm_info *dimm; struct memory_info *mem_info;
+ spd_content = get_spd_pointer(&dual_channel); + if (spd_content == NULL) + return; + /* * Allocate CBMEM area for DIMM information used to populate SMBIOS * table 17 @@ -186,13 +198,13 @@
/* Describe the first channel memory */ dimm = &mem_info->dimm[0]; - set_dimm_info(params->pei_data->spd_data_ch0, dimm); + set_dimm_info(spd_content, dimm); mem_info->dimm_cnt = 1;
/* Describe the second channel memory */ - if (params->pei_data->spd_ch1_config == 1) { + if (dual_channel) { dimm = &mem_info->dimm[1]; - set_dimm_info(params->pei_data->spd_data_ch1, dimm); + set_dimm_info(spd_content, dimm); dimm->channel_num = 1; mem_info->dimm_cnt = 2; } diff --git a/src/mainboard/google/cyan/spd/spd_util.h b/src/mainboard/google/cyan/spd/spd_util.h index 11d6eaa..0c5b326 100644 --- a/src/mainboard/google/cyan/spd/spd_util.h +++ b/src/mainboard/google/cyan/spd/spd_util.h @@ -16,7 +16,10 @@ #ifndef SPD_UTIL_H #define SPD_UTIL_H
+#include <fsp/soc_binding.h> + uint8_t get_ramid(void); int get_variant_spd_index(int ram_id, int *dual); +void spd_memory_init_params(MEMORY_INIT_UPD *memory_params);
#endif /* SPD_UTIL_H */
-
Matt DeVillier (Code Review) -
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)