Change in coreboot[master]: lib, mb, soc: change mainboard_get_dram_part_num() prototype
Nick Vaccaro has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45873 )
Change subject: lib, mb, soc: change mainboard_get_dram_part_num() prototype ......................................................................
lib, mb, soc: change mainboard_get_dram_part_num() prototype
Change mainboard_get_dram_part_num() to return a constant character pointer to a null-terminated C string and to take no input parameters. This also addresses the issue that different SOCs and motherboards were using different definitions for mainboard_get_dram_part_num by consolidating to a single definition.
BUG=b:168724473 TEST="emerge-volteer coreboot && emerge-dedede coreboot && emerge-hatch coreboot" and verify build completes successfully.
Change-Id: Ie7664eab65a2b9e25b7853bf68baf2525b040487 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/mainboard/google/dedede/romstage.c M src/mainboard/google/hatch/romstage_spd_cbfs.c M src/mainboard/google/volteer/romstage.c M src/soc/intel/alderlake/include/soc/romstage.h M src/soc/intel/alderlake/romstage/romstage.c M src/soc/intel/cannonlake/include/soc/romstage.h M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/elkhartlake/include/soc/romstage.h M src/soc/intel/elkhartlake/romstage/romstage.c M src/soc/intel/jasperlake/include/soc/romstage.h M src/soc/intel/jasperlake/romstage/romstage.c M src/soc/intel/tigerlake/include/soc/romstage.h M src/soc/intel/tigerlake/romstage/romstage.c 13 files changed, 64 insertions(+), 51 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/45873/1
diff --git a/src/mainboard/google/dedede/romstage.c b/src/mainboard/google/dedede/romstage.c index 8028db0..e143f70 100644 --- a/src/mainboard/google/dedede/romstage.c +++ b/src/mainboard/google/dedede/romstage.c @@ -22,17 +22,15 @@ memcfg_init(&memupd->FspmConfig, board_cfg, &spd_info, half_populated); }
-bool mainboard_get_dram_part_num(const char **part_num, size_t *len) +const char *mainboard_get_dram_part_num(void) { static char part_num_store[DIMM_INFO_PART_NUMBER_SIZE];
if (google_chromeec_cbi_get_dram_part_num(&part_num_store[0], sizeof(part_num_store)) < 0) { printk(BIOS_ERR, "No DRAM part number in CBI!\n"); - return false; + return NULL; }
- *part_num = &part_num_store[0]; - *len = strlen(part_num_store); - return true; + return part_num_store; } diff --git a/src/mainboard/google/hatch/romstage_spd_cbfs.c b/src/mainboard/google/hatch/romstage_spd_cbfs.c index a9a65e9..b8937de 100644 --- a/src/mainboard/google/hatch/romstage_spd_cbfs.c +++ b/src/mainboard/google/hatch/romstage_spd_cbfs.c @@ -57,7 +57,7 @@ cannonlake_memcfg_init(&memupd->FspmConfig, &memcfg); }
-void mainboard_get_dram_part_num(const char **part_num, size_t *len) +const char *mainboard_get_dram_part_num(void) { static char part_num_store[DIMM_INFO_PART_NUMBER_SIZE]; static enum { @@ -77,8 +77,7 @@ }
if (part_num_state == PART_NUM_NOT_IN_CBI) - return; + return NULL;
- *part_num = &part_num_store[0]; - *len = strlen(part_num_store) + 1; + return part_num_store; } diff --git a/src/mainboard/google/volteer/romstage.c b/src/mainboard/google/volteer/romstage.c index 552648b..51a4ebc 100644 --- a/src/mainboard/google/volteer/romstage.c +++ b/src/mainboard/google/volteer/romstage.c @@ -29,16 +29,14 @@ meminit_ddr(mem_cfg, board_cfg, &spd_info, half_populated); }
-bool mainboard_get_dram_part_num(const char **part_num, size_t *len) +const char *mainboard_get_dram_part_num(void) { static char part_num_store[DIMM_INFO_PART_NUMBER_SIZE];
if (google_chromeec_cbi_get_dram_part_num(part_num_store, sizeof(part_num_store)) < 0) { printk(BIOS_ERR, "ERROR: Couldn't obtain DRAM part number from CBI\n"); - return false; + return NULL; } - *part_num = part_num_store; - *len = strlen(part_num_store); - return true; + return part_num_store; } diff --git a/src/soc/intel/alderlake/include/soc/romstage.h b/src/soc/intel/alderlake/include/soc/romstage.h index 716602c..55469a3 100644 --- a/src/soc/intel/alderlake/include/soc/romstage.h +++ b/src/soc/intel/alderlake/include/soc/romstage.h @@ -7,7 +7,7 @@ #include <stddef.h>
/* Provide a callback to allow mainboard to override the DRAM part number. */ -const char *mainboard_get_dram_part_num(size_t *len); +const char *mainboard_get_dram_part_num(void); void mainboard_memory_init_params(FSPM_UPD *mupd); void systemagent_early_init(void); void romstage_pch_init(void); diff --git a/src/soc/intel/alderlake/romstage/romstage.c b/src/soc/intel/alderlake/romstage/romstage.c index 9f4fbb6..a62a726 100644 --- a/src/soc/intel/alderlake/romstage/romstage.c +++ b/src/soc/intel/alderlake/romstage/romstage.c @@ -21,7 +21,7 @@ 0x8d, 0x09, 0x11, 0xcf, 0x8b, 0x9f, 0x03, 0x23 \ }
-__weak const char *mainboard_get_dram_part_num(size_t *len) +const char * __weak mainboard_get_dram_part_num(void) { /* Default weak implementation, no need to override part number. */ return NULL; @@ -41,7 +41,8 @@ const uint8_t smbios_memory_info_guid[sizeof(EFI_GUID)] = FSP_SMBIOS_MEMORY_INFO_GUID; const uint8_t *serial_num; const char *dram_part_num = NULL; - size_t dram_part_num_len; + size_t dram_part_num_len = 0; + bool part_name_overridden = false;
/* Locate the memory info HOB, presence validated by raminit */ meminfo_hob = fsp_find_extension_hob_by_guid( @@ -64,7 +65,11 @@ memset(mem_info, 0, sizeof(*mem_info));
/* Allow mainboard to override DRAM part number. */ - dram_part_num = mainboard_get_dram_part_num(&dram_part_num_len); + dram_part_num = mainboard_get_dram_part_num(); + if (dram_part_num) { + dram_part_num_len = strlen(dram_part_num); + part_name_overridden = true; + }
/* Save available DIMM information */ index = 0; @@ -84,7 +89,7 @@
/* If there is no DRAM part number overridden by * mainboard then use original one. */ - if (!dram_part_num) { + if (!part_name_overridden) { dram_part_num_len = sizeof(src_dimm->ModulePartNum); dram_part_num = (const char *) &src_dimm->ModulePartNum[0]; diff --git a/src/soc/intel/cannonlake/include/soc/romstage.h b/src/soc/intel/cannonlake/include/soc/romstage.h index 4e513f0..f99175f 100644 --- a/src/soc/intel/cannonlake/include/soc/romstage.h +++ b/src/soc/intel/cannonlake/include/soc/romstage.h @@ -8,7 +8,7 @@ void mainboard_memory_init_params(FSPM_UPD *mupd);
/* Provide a callback to allow mainboard to override the DRAM part number. */ -void mainboard_get_dram_part_num(const char **part_num, size_t *len); +const char *mainboard_get_dram_part_num(void); void systemagent_early_init(void); void romstage_pch_init(void);
diff --git a/src/soc/intel/cannonlake/romstage/romstage.c b/src/soc/intel/cannonlake/romstage/romstage.c index c56add7..74dc20f 100644 --- a/src/soc/intel/cannonlake/romstage/romstage.c +++ b/src/soc/intel/cannonlake/romstage/romstage.c @@ -23,7 +23,7 @@ 0x8d, 0x09, 0x11, 0xcf, 0x8b, 0x9f, 0x03, 0x23 \ }
-void __weak mainboard_get_dram_part_num(const char **part_num, size_t *len) +const char * __weak mainboard_get_dram_part_num(void) { /* Default weak implementation, no need to override part number. */ } @@ -42,7 +42,7 @@ const uint8_t smbios_memory_info_guid[16] = FSP_SMBIOS_MEMORY_INFO_GUID; const char *dram_part_num; - size_t dram_part_num_len; + size_t dram_part_num_len = 0;
/* Locate the memory info HOB, presence validated by raminit */ memory_info_hob = fsp_find_extension_hob_by_guid( @@ -79,13 +79,16 @@ if (src_dimm->Status != DIMM_PRESENT) continue;
- dram_part_num_len = sizeof(src_dimm->ModulePartNum); - dram_part_num = (const char *) - &src_dimm->ModulePartNum[0]; - /* Allow mainboard to override DRAM part number. */ - mainboard_get_dram_part_num(&dram_part_num, - &dram_part_num_len); + dram_part_num = mainboard_get_dram_part_num(); + if (dram_part_num) { + dram_part_num_len = strlen(dram_part_num); + } else { + dram_part_num_len = + sizeof(src_dimm->ModulePartNum); + dram_part_num = (const char *) + &src_dimm->ModulePartNum[0]; + }
u8 memProfNum = memory_info_hob->MemoryProfile;
diff --git a/src/soc/intel/elkhartlake/include/soc/romstage.h b/src/soc/intel/elkhartlake/include/soc/romstage.h index baa35c5..1cffcb9 100644 --- a/src/soc/intel/elkhartlake/include/soc/romstage.h +++ b/src/soc/intel/elkhartlake/include/soc/romstage.h @@ -6,7 +6,7 @@ #include <fsp/api.h>
/* Provide a callback to allow mainboard to override the DRAM part number. */ -bool mainboard_get_dram_part_num(const char **part_num, size_t *len); +const char *mainboard_get_dram_part_num(void); void mainboard_memory_init_params(FSPM_UPD *mupd); void systemagent_early_init(void); void romstage_pch_init(void); diff --git a/src/soc/intel/elkhartlake/romstage/romstage.c b/src/soc/intel/elkhartlake/romstage/romstage.c index 09b1744..d7b2959 100644 --- a/src/soc/intel/elkhartlake/romstage/romstage.c +++ b/src/soc/intel/elkhartlake/romstage/romstage.c @@ -21,10 +21,10 @@ 0x8d, 0x09, 0x11, 0xcf, 0x8b, 0x9f, 0x03, 0x23 \ }
-bool __weak mainboard_get_dram_part_num(const char **part_num, size_t *len) +const char * __weak mainboard_get_dram_part_num(void) { /* Default implementation, no need to override part number. */ - return false; + return NULL; }
/* Save the DIMM information for SMBIOS table 17 */ @@ -42,8 +42,8 @@ FSP_SMBIOS_MEMORY_INFO_GUID; const uint8_t *serial_num; const char *dram_part_num = NULL; - size_t dram_part_num_len; - bool is_dram_part_overridden = false; + size_t dram_part_num_len = 0; + bool part_name_overridden = false;
/* Locate the memory info HOB, presence validated by raminit */ meminfo_hob = fsp_find_extension_hob_by_guid( @@ -66,8 +66,11 @@ memset(mem_info, 0, sizeof(*mem_info));
/* Allow mainboard to override DRAM part number. */ - is_dram_part_overridden = mainboard_get_dram_part_num(&dram_part_num, - &dram_part_num_len); + dram_part_num = mainboard_get_dram_part_num(); + if (dram_part_num) { + dram_part_num = strlen(dram_part_num); + part_name_overridden = true; + }
/* Save available DIMM information */ index = 0; @@ -89,7 +92,7 @@
/* If there is no DRAM part number overridden by * mainboard then use original one. */ - if (!is_dram_part_overridden) { + if (!part_name_overridden) { dram_part_num_len = sizeof(src_dimm->ModulePartNum); dram_part_num = (const char *) &src_dimm->ModulePartNum[0]; diff --git a/src/soc/intel/jasperlake/include/soc/romstage.h b/src/soc/intel/jasperlake/include/soc/romstage.h index baa35c5..1cffcb9 100644 --- a/src/soc/intel/jasperlake/include/soc/romstage.h +++ b/src/soc/intel/jasperlake/include/soc/romstage.h @@ -6,7 +6,7 @@ #include <fsp/api.h>
/* Provide a callback to allow mainboard to override the DRAM part number. */ -bool mainboard_get_dram_part_num(const char **part_num, size_t *len); +const char *mainboard_get_dram_part_num(void); void mainboard_memory_init_params(FSPM_UPD *mupd); void systemagent_early_init(void); void romstage_pch_init(void); diff --git a/src/soc/intel/jasperlake/romstage/romstage.c b/src/soc/intel/jasperlake/romstage/romstage.c index db014ea..6a3ebd6 100644 --- a/src/soc/intel/jasperlake/romstage/romstage.c +++ b/src/soc/intel/jasperlake/romstage/romstage.c @@ -21,10 +21,10 @@ 0x8d, 0x09, 0x11, 0xcf, 0x8b, 0x9f, 0x03, 0x23 \ }
-bool __weak mainboard_get_dram_part_num(const char **part_num, size_t *len) +const char * __weak mainboard_get_dram_part_num(void) { /* Default weak implementation, no need to override part number. */ - return false; + return NULL; }
/* Save the DIMM information for SMBIOS table 17 */ @@ -42,8 +42,8 @@ FSP_SMBIOS_MEMORY_INFO_GUID; const uint8_t *serial_num; const char *dram_part_num = NULL; - size_t dram_part_num_len; - bool is_dram_part_overridden = false; + size_t dram_part_num_len = 0; + bool part_name_overridden = false;
/* Locate the memory info HOB, presence validated by raminit */ meminfo_hob = fsp_find_extension_hob_by_guid( @@ -66,8 +66,12 @@ memset(mem_info, 0, sizeof(*mem_info));
/* Allow mainboard to override DRAM part number. */ - is_dram_part_overridden = mainboard_get_dram_part_num(&dram_part_num, + dram_part_num = mainboard_get_dram_part_num(&, &dram_part_num_len); + if (dram_part_num) { + dram_part_num_len = strlen(dram_part_num); + part_name_overridden = true; + }
/* Save available DIMM information */ index = 0; @@ -89,7 +93,7 @@
/* If there is no DRAM part number overridden by * mainboard then use original one. */ - if (!is_dram_part_overridden) { + if (!part_name_overridden) { dram_part_num_len = sizeof(src_dimm->ModulePartNum); dram_part_num = (const char *) &src_dimm->ModulePartNum[0]; diff --git a/src/soc/intel/tigerlake/include/soc/romstage.h b/src/soc/intel/tigerlake/include/soc/romstage.h index baa35c5..1cffcb9 100644 --- a/src/soc/intel/tigerlake/include/soc/romstage.h +++ b/src/soc/intel/tigerlake/include/soc/romstage.h @@ -6,7 +6,7 @@ #include <fsp/api.h>
/* Provide a callback to allow mainboard to override the DRAM part number. */ -bool mainboard_get_dram_part_num(const char **part_num, size_t *len); +const char *mainboard_get_dram_part_num(void); void mainboard_memory_init_params(FSPM_UPD *mupd); void systemagent_early_init(void); void romstage_pch_init(void); diff --git a/src/soc/intel/tigerlake/romstage/romstage.c b/src/soc/intel/tigerlake/romstage/romstage.c index db014ea..fd18fea 100644 --- a/src/soc/intel/tigerlake/romstage/romstage.c +++ b/src/soc/intel/tigerlake/romstage/romstage.c @@ -21,10 +21,10 @@ 0x8d, 0x09, 0x11, 0xcf, 0x8b, 0x9f, 0x03, 0x23 \ }
-bool __weak mainboard_get_dram_part_num(const char **part_num, size_t *len) +const char * __weak mainboard_get_dram_part_num(void) { /* Default weak implementation, no need to override part number. */ - return false; + return NULL; }
/* Save the DIMM information for SMBIOS table 17 */ @@ -42,8 +42,8 @@ FSP_SMBIOS_MEMORY_INFO_GUID; const uint8_t *serial_num; const char *dram_part_num = NULL; - size_t dram_part_num_len; - bool is_dram_part_overridden = false; + size_t dram_part_num_len = 0; + bool part_name_overridden = false;
/* Locate the memory info HOB, presence validated by raminit */ meminfo_hob = fsp_find_extension_hob_by_guid( @@ -66,8 +66,11 @@ memset(mem_info, 0, sizeof(*mem_info));
/* Allow mainboard to override DRAM part number. */ - is_dram_part_overridden = mainboard_get_dram_part_num(&dram_part_num, - &dram_part_num_len); + dram_part_num = mainboard_get_dram_part_num(); + if (dram_part_num) { + dram_part_num_len = strlen(dram_part_num); + part_name_overridden = true; + }
/* Save available DIMM information */ index = 0; @@ -89,7 +92,7 @@
/* If there is no DRAM part number overridden by * mainboard then use original one. */ - if (!is_dram_part_overridden) { + if (!part_name_overridden) { dram_part_num_len = sizeof(src_dimm->ModulePartNum); dram_part_num = (const char *) &src_dimm->ModulePartNum[0];
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45873 )
Change subject: lib, mb, soc: change mainboard_get_dram_part_num() prototype ......................................................................
Patch Set 2:
This change is ready for review.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45873
to look at the new patch set (#3).
Change subject: lib, mb, soc: change mainboard_get_dram_part_num() prototype ......................................................................
lib, mb, soc: change mainboard_get_dram_part_num() prototype
Change mainboard_get_dram_part_num() to return a constant character pointer to a null-terminated C string and to take no input parameters. This also addresses the issue that different SOCs and motherboards were using different definitions for mainboard_get_dram_part_num by consolidating to a single definition.
BUG=b:169774661, b:168724473 TEST="emerge-volteer coreboot && emerge-dedede coreboot && emerge-hatch coreboot" and verify build completes successfully.
Change-Id: Ie7664eab65a2b9e25b7853bf68baf2525b040487 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/mainboard/google/dedede/romstage.c M src/mainboard/google/hatch/romstage_spd_cbfs.c M src/mainboard/google/volteer/romstage.c M src/soc/intel/alderlake/include/soc/romstage.h M src/soc/intel/alderlake/romstage/romstage.c M src/soc/intel/cannonlake/include/soc/romstage.h M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/elkhartlake/include/soc/romstage.h M src/soc/intel/elkhartlake/romstage/romstage.c M src/soc/intel/jasperlake/include/soc/romstage.h M src/soc/intel/jasperlake/romstage/romstage.c M src/soc/intel/tigerlake/include/soc/romstage.h M src/soc/intel/tigerlake/romstage/romstage.c 13 files changed, 64 insertions(+), 52 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/45873/3
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45873 )
Change subject: lib, mb, soc: change mainboard_get_dram_part_num() prototype ......................................................................
Patch Set 3:
(8 comments)
https://review.coreboot.org/c/coreboot/+/45873/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45873/3//COMMIT_MSG@7 PS3, Line 7: lib You haven't really added this to lib in this CL. So, just "soc/intel, mb/google: "
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/alderlake/rom... File src/soc/intel/alderlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/alderlake/rom... PS3, Line 44: = 0 Not required. dram_part_num_len is always set before using.
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/alderlake/rom... PS3, Line 92: part_name_overridden Why is this bool added? You can still check !dram_part_num here.
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/cannonlake/ro... PS3, Line 28: /* Default weak implementation, no need to override part number. */ return NULL;
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/elkhartlake/r... File src/soc/intel/elkhartlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/elkhartlake/r... PS3, Line 45: = 0 Same comment as alderlake.
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/elkhartlake/r... PS3, Line 46: part_name_overridden Same comment as alderlake. This flag is not required.
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/jasperlake/ro... File src/soc/intel/jasperlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/jasperlake/ro... PS3, Line 45: = 0 Same comment as alderlake.
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/tigerlake/rom... PS3, Line 45: = 0; Not required.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45873
to look at the new patch set (#4).
Change subject: lib, mb, soc: change mainboard_get_dram_part_num() prototype ......................................................................
lib, mb, soc: change mainboard_get_dram_part_num() prototype
Change mainboard_get_dram_part_num() to return a constant character pointer to a null-terminated C string and to take no input parameters. This also addresses the issue that different SOCs and motherboards were using different definitions for mainboard_get_dram_part_num by consolidating to a single definition.
BUG=b:169774661, b:168724473 TEST="emerge-volteer coreboot && emerge-dedede coreboot && emerge-hatch coreboot" and verify build completes successfully.
Change-Id: Ie7664eab65a2b9e25b7853bf68baf2525b040487 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/mainboard/google/dedede/romstage.c M src/mainboard/google/hatch/romstage_spd_cbfs.c M src/mainboard/google/volteer/romstage.c M src/soc/intel/alderlake/include/soc/romstage.h M src/soc/intel/alderlake/romstage/romstage.c M src/soc/intel/cannonlake/include/soc/romstage.h M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/elkhartlake/include/soc/romstage.h M src/soc/intel/elkhartlake/romstage/romstage.c M src/soc/intel/jasperlake/include/soc/romstage.h M src/soc/intel/jasperlake/romstage/romstage.c M src/soc/intel/tigerlake/include/soc/romstage.h M src/soc/intel/tigerlake/romstage/romstage.c 13 files changed, 71 insertions(+), 52 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/45873/4
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45873 )
Change subject: lib, mb, soc: change mainboard_get_dram_part_num() prototype ......................................................................
Patch Set 4:
(7 comments)
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/alderlake/rom... File src/soc/intel/alderlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/alderlake/rom... PS3, Line 44: = 0
Not required. dram_part_num_len is always set before using.
The compiler complains that "it could be uninitialized"
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/alderlake/rom... PS3, Line 92: part_name_overridden
Why is this bool added? You can still check !dram_part_num here.
The bool is added because if dram_part_num is not set above (i.e. name not overridden), then the first pass through this loop, it propertly sets dram_part_num. However, next time through this loop, dram_part_num will be set, so it will not execute this block.
Checking dram_part_num for null here would change initial intent of this code. This code was written to specifically use whatever part number was defined for that particular dimm (each dimm record has string storage for part name). In reality, I doubt we would ever see a mixed-dimm model where different slots may have different parts, but the original code was written to support that possibility, so I was retaining that.
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/cannonlake/ro... PS3, Line 28: /* Default weak implementation, no need to override part number. */
return NULL;
Done
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/elkhartlake/r... File src/soc/intel/elkhartlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/elkhartlake/r... PS3, Line 45: = 0
Same comment as alderlake.
Please see comment on alderlake.
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/elkhartlake/r... PS3, Line 46: part_name_overridden
Same comment as alderlake. This flag is not required.
I disagree, please see explanation on alderlake.
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/jasperlake/ro... File src/soc/intel/jasperlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/jasperlake/ro... PS3, Line 45: = 0
Same comment as alderlake.
please see my alderlake response
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/tigerlake/rom... PS3, Line 45: = 0;
Not required.
please see my alderlake response
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45873 )
Change subject: lib, mb, soc: change mainboard_get_dram_part_num() prototype ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/alderlake/rom... File src/soc/intel/alderlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/alderlake/rom... PS3, Line 92: part_name_overridden
Checking dram_part_num for null here would change initial intent of this code. This code was written to specifically use whatever part number was defined for that particular dimm (each dimm record has string storage for part name). In reality, I doubt we would ever see a mixed-dimm model where different slots may have different parts, but the original code was written to support that possibility, so I was retaining that.
Original code set dram_part_num on line 67. If it was NULL, it would end up setting the dram_part_num here on line 89 and use that for all the DIMMs. I understand your comment about multiple DIMMs reporting different part numbers, but the original code did not really account for that. Hence my comment about keeping the logic same.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45873
to look at the new patch set (#5).
Change subject: mb, soc: change mainboard_get_dram_part_num() prototype ......................................................................
mb, soc: change mainboard_get_dram_part_num() prototype
Change mainboard_get_dram_part_num() to return a constant character pointer to a null-terminated C string and to take no input parameters. This also addresses the issue that different SOCs and motherboards were using different definitions for mainboard_get_dram_part_num by consolidating to a single definition.
BUG=b:169774661, b:168724473 TEST="emerge-volteer coreboot && emerge-dedede coreboot && emerge-hatch coreboot" and verify build completes successfully.
Change-Id: Ie7664eab65a2b9e25b7853bf68baf2525b040487 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/mainboard/google/dedede/romstage.c M src/mainboard/google/hatch/romstage_spd_cbfs.c M src/mainboard/google/volteer/romstage.c M src/soc/intel/alderlake/include/soc/romstage.h M src/soc/intel/alderlake/romstage/romstage.c M src/soc/intel/cannonlake/include/soc/romstage.h M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/elkhartlake/include/soc/romstage.h M src/soc/intel/elkhartlake/romstage/romstage.c M src/soc/intel/jasperlake/include/soc/romstage.h M src/soc/intel/jasperlake/romstage/romstage.c M src/soc/intel/tigerlake/include/soc/romstage.h M src/soc/intel/tigerlake/romstage/romstage.c 13 files changed, 71 insertions(+), 52 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/45873/5
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45873 )
Change subject: mb, soc: change mainboard_get_dram_part_num() prototype ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/alderlake/rom... File src/soc/intel/alderlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/alderlake/rom... PS3, Line 44: = 0
The compiler complains that "it could be uninitialized"
Ack.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45873
to look at the new patch set (#6).
Change subject: mb, soc: change mainboard_get_dram_part_num() prototype ......................................................................
mb, soc: change mainboard_get_dram_part_num() prototype
Change mainboard_get_dram_part_num() to return a constant character pointer to a null-terminated C string and to take no input parameters. This also addresses the issue that different SOCs and motherboards were using different definitions for mainboard_get_dram_part_num by consolidating to a single definition.
BUG=b:169774661, b:168724473 TEST="emerge-volteer coreboot && emerge-dedede coreboot && emerge-hatch coreboot" and verify build completes successfully.
Change-Id: Ie7664eab65a2b9e25b7853bf68baf2525b040487 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/mainboard/google/dedede/romstage.c M src/mainboard/google/hatch/romstage_spd_cbfs.c M src/mainboard/google/volteer/romstage.c M src/soc/intel/alderlake/include/soc/romstage.h M src/soc/intel/alderlake/romstage/romstage.c M src/soc/intel/cannonlake/include/soc/romstage.h M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/elkhartlake/include/soc/romstage.h M src/soc/intel/elkhartlake/romstage/romstage.c M src/soc/intel/jasperlake/include/soc/romstage.h M src/soc/intel/jasperlake/romstage/romstage.c M src/soc/intel/tigerlake/include/soc/romstage.h M src/soc/intel/tigerlake/romstage/romstage.c 13 files changed, 53 insertions(+), 51 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/45873/6
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45873 )
Change subject: mb, soc: change mainboard_get_dram_part_num() prototype ......................................................................
Patch Set 6:
(6 comments)
https://review.coreboot.org/c/coreboot/+/45873/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45873/3//COMMIT_MSG@7 PS3, Line 7: lib
You haven't really added this to lib in this CL. So, just […]
Done
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/alderlake/rom... File src/soc/intel/alderlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/alderlake/rom... PS3, Line 44: = 0
Ack.
Done
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/elkhartlake/r... File src/soc/intel/elkhartlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/elkhartlake/r... PS3, Line 45: = 0
Please see comment on alderlake.
Done
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/elkhartlake/r... PS3, Line 46: part_name_overridden
I disagree, please see explanation on alderlake.
Done
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/jasperlake/ro... File src/soc/intel/jasperlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/jasperlake/ro... PS3, Line 45: = 0
please see my alderlake response
Done
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/tigerlake/rom... PS3, Line 45: = 0;
please see my alderlake response
Done
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45873 )
Change subject: mb, soc: change mainboard_get_dram_part_num() prototype ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/alderlake/rom... File src/soc/intel/alderlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/alderlake/rom... PS3, Line 92: part_name_overridden
Checking dram_part_num for null here would change initial intent of this code. […]
Done
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45873 )
Change subject: mb, soc: change mainboard_get_dram_part_num() prototype ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/alderlake/rom... File src/soc/intel/alderlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/alderlake/rom... PS3, Line 92: part_name_overridden
Done
JFYI, I realize now I was looking at the logic at https://source.corp.google.com/chromeos_public/src/third_party/coreboot/src/... when I made my statement. I do see now that not all implementations did the same (respected each dimm's name). We should probably clean this up afterwards.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45873
to look at the new patch set (#7).
Change subject: mb, soc: change mainboard_get_dram_part_num() prototype ......................................................................
mb, soc: change mainboard_get_dram_part_num() prototype
Change mainboard_get_dram_part_num() to return a constant character pointer to a null-terminated C string and to take no input parameters. This also addresses the issue that different SOCs and motherboards were using different definitions for mainboard_get_dram_part_num by consolidating to a single definition.
BUG=b:169774661, b:168724473 TEST="emerge-volteer coreboot && emerge-dedede coreboot && emerge-hatch coreboot" and verify build completes successfully.
Change-Id: Ie7664eab65a2b9e25b7853bf68baf2525b040487 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/mainboard/google/dedede/romstage.c M src/mainboard/google/hatch/romstage_spd_cbfs.c M src/mainboard/google/volteer/romstage.c M src/soc/intel/alderlake/include/soc/romstage.h M src/soc/intel/alderlake/romstage/romstage.c M src/soc/intel/cannonlake/include/soc/romstage.h M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/elkhartlake/include/soc/romstage.h M src/soc/intel/elkhartlake/romstage/romstage.c M src/soc/intel/jasperlake/include/soc/romstage.h M src/soc/intel/jasperlake/romstage/romstage.c M src/soc/intel/tigerlake/include/soc/romstage.h M src/soc/intel/tigerlake/romstage/romstage.c 13 files changed, 56 insertions(+), 45 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/45873/7
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45873 )
Change subject: mb, soc: change mainboard_get_dram_part_num() prototype ......................................................................
Patch Set 7: Code-Review+2
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45873
to look at the new patch set (#8).
Change subject: mb, soc: change mainboard_get_dram_part_num() prototype ......................................................................
mb, soc: change mainboard_get_dram_part_num() prototype
Change mainboard_get_dram_part_num() to return a constant character pointer to a null-terminated C string and to take no input parameters. This also addresses the issue that different SOCs and motherboards were using different definitions for mainboard_get_dram_part_num by consolidating to a single definition.
BUG=b:169774661, b:168724473 TEST="emerge-volteer coreboot && emerge-dedede coreboot && emerge-hatch coreboot" and verify build completes successfully.
Change-Id: Ie7664eab65a2b9e25b7853bf68baf2525b040487 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/mainboard/google/dedede/romstage.c M src/mainboard/google/hatch/romstage_spd_cbfs.c M src/mainboard/google/volteer/romstage.c M src/soc/intel/alderlake/include/soc/romstage.h M src/soc/intel/alderlake/romstage/romstage.c M src/soc/intel/cannonlake/include/soc/romstage.h M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/elkhartlake/include/soc/romstage.h M src/soc/intel/elkhartlake/romstage/romstage.c M src/soc/intel/jasperlake/include/soc/romstage.h M src/soc/intel/jasperlake/romstage/romstage.c M src/soc/intel/tigerlake/include/soc/romstage.h M src/soc/intel/tigerlake/romstage/romstage.c 13 files changed, 59 insertions(+), 45 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/45873/8
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45873 )
Change subject: mb, soc: change mainboard_get_dram_part_num() prototype ......................................................................
Patch Set 8: Code-Review+2
Nick Vaccaro has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45873 )
Change subject: mb, soc: change mainboard_get_dram_part_num() prototype ......................................................................
mb, soc: change mainboard_get_dram_part_num() prototype
Change mainboard_get_dram_part_num() to return a constant character pointer to a null-terminated C string and to take no input parameters. This also addresses the issue that different SOCs and motherboards were using different definitions for mainboard_get_dram_part_num by consolidating to a single definition.
BUG=b:169774661, b:168724473 TEST="emerge-volteer coreboot && emerge-dedede coreboot && emerge-hatch coreboot" and verify build completes successfully.
Change-Id: Ie7664eab65a2b9e25b7853bf68baf2525b040487 Signed-off-by: Nick Vaccaro nvaccaro@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/45873 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/dedede/romstage.c M src/mainboard/google/hatch/romstage_spd_cbfs.c M src/mainboard/google/volteer/romstage.c M src/soc/intel/alderlake/include/soc/romstage.h M src/soc/intel/alderlake/romstage/romstage.c M src/soc/intel/cannonlake/include/soc/romstage.h M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/elkhartlake/include/soc/romstage.h M src/soc/intel/elkhartlake/romstage/romstage.c M src/soc/intel/jasperlake/include/soc/romstage.h M src/soc/intel/jasperlake/romstage/romstage.c M src/soc/intel/tigerlake/include/soc/romstage.h M src/soc/intel/tigerlake/romstage/romstage.c 13 files changed, 59 insertions(+), 45 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/mainboard/google/dedede/romstage.c b/src/mainboard/google/dedede/romstage.c index 8028db0..e143f70 100644 --- a/src/mainboard/google/dedede/romstage.c +++ b/src/mainboard/google/dedede/romstage.c @@ -22,17 +22,15 @@ memcfg_init(&memupd->FspmConfig, board_cfg, &spd_info, half_populated); }
-bool mainboard_get_dram_part_num(const char **part_num, size_t *len) +const char *mainboard_get_dram_part_num(void) { static char part_num_store[DIMM_INFO_PART_NUMBER_SIZE];
if (google_chromeec_cbi_get_dram_part_num(&part_num_store[0], sizeof(part_num_store)) < 0) { printk(BIOS_ERR, "No DRAM part number in CBI!\n"); - return false; + return NULL; }
- *part_num = &part_num_store[0]; - *len = strlen(part_num_store); - return true; + return part_num_store; } diff --git a/src/mainboard/google/hatch/romstage_spd_cbfs.c b/src/mainboard/google/hatch/romstage_spd_cbfs.c index a9a65e9..b8937de 100644 --- a/src/mainboard/google/hatch/romstage_spd_cbfs.c +++ b/src/mainboard/google/hatch/romstage_spd_cbfs.c @@ -57,7 +57,7 @@ cannonlake_memcfg_init(&memupd->FspmConfig, &memcfg); }
-void mainboard_get_dram_part_num(const char **part_num, size_t *len) +const char *mainboard_get_dram_part_num(void) { static char part_num_store[DIMM_INFO_PART_NUMBER_SIZE]; static enum { @@ -77,8 +77,7 @@ }
if (part_num_state == PART_NUM_NOT_IN_CBI) - return; + return NULL;
- *part_num = &part_num_store[0]; - *len = strlen(part_num_store) + 1; + return part_num_store; } diff --git a/src/mainboard/google/volteer/romstage.c b/src/mainboard/google/volteer/romstage.c index 552648b..51a4ebc 100644 --- a/src/mainboard/google/volteer/romstage.c +++ b/src/mainboard/google/volteer/romstage.c @@ -29,16 +29,14 @@ meminit_ddr(mem_cfg, board_cfg, &spd_info, half_populated); }
-bool mainboard_get_dram_part_num(const char **part_num, size_t *len) +const char *mainboard_get_dram_part_num(void) { static char part_num_store[DIMM_INFO_PART_NUMBER_SIZE];
if (google_chromeec_cbi_get_dram_part_num(part_num_store, sizeof(part_num_store)) < 0) { printk(BIOS_ERR, "ERROR: Couldn't obtain DRAM part number from CBI\n"); - return false; + return NULL; } - *part_num = part_num_store; - *len = strlen(part_num_store); - return true; + return part_num_store; } diff --git a/src/soc/intel/alderlake/include/soc/romstage.h b/src/soc/intel/alderlake/include/soc/romstage.h index 716602c..55469a3 100644 --- a/src/soc/intel/alderlake/include/soc/romstage.h +++ b/src/soc/intel/alderlake/include/soc/romstage.h @@ -7,7 +7,7 @@ #include <stddef.h>
/* Provide a callback to allow mainboard to override the DRAM part number. */ -const char *mainboard_get_dram_part_num(size_t *len); +const char *mainboard_get_dram_part_num(void); void mainboard_memory_init_params(FSPM_UPD *mupd); void systemagent_early_init(void); void romstage_pch_init(void); diff --git a/src/soc/intel/alderlake/romstage/romstage.c b/src/soc/intel/alderlake/romstage/romstage.c index 9f4fbb6..32657c7 100644 --- a/src/soc/intel/alderlake/romstage/romstage.c +++ b/src/soc/intel/alderlake/romstage/romstage.c @@ -21,7 +21,7 @@ 0x8d, 0x09, 0x11, 0xcf, 0x8b, 0x9f, 0x03, 0x23 \ }
-__weak const char *mainboard_get_dram_part_num(size_t *len) +const char * __weak mainboard_get_dram_part_num(void) { /* Default weak implementation, no need to override part number. */ return NULL; @@ -41,7 +41,7 @@ const uint8_t smbios_memory_info_guid[sizeof(EFI_GUID)] = FSP_SMBIOS_MEMORY_INFO_GUID; const uint8_t *serial_num; const char *dram_part_num = NULL; - size_t dram_part_num_len; + size_t dram_part_num_len = 0;
/* Locate the memory info HOB, presence validated by raminit */ meminfo_hob = fsp_find_extension_hob_by_guid( @@ -64,7 +64,9 @@ memset(mem_info, 0, sizeof(*mem_info));
/* Allow mainboard to override DRAM part number. */ - dram_part_num = mainboard_get_dram_part_num(&dram_part_num_len); + dram_part_num = mainboard_get_dram_part_num(); + if (dram_part_num) + dram_part_num_len = strlen(dram_part_num);
/* Save available DIMM information */ index = 0; diff --git a/src/soc/intel/cannonlake/include/soc/romstage.h b/src/soc/intel/cannonlake/include/soc/romstage.h index 4e513f0..f99175f 100644 --- a/src/soc/intel/cannonlake/include/soc/romstage.h +++ b/src/soc/intel/cannonlake/include/soc/romstage.h @@ -8,7 +8,7 @@ void mainboard_memory_init_params(FSPM_UPD *mupd);
/* Provide a callback to allow mainboard to override the DRAM part number. */ -void mainboard_get_dram_part_num(const char **part_num, size_t *len); +const char *mainboard_get_dram_part_num(void); void systemagent_early_init(void); void romstage_pch_init(void);
diff --git a/src/soc/intel/cannonlake/romstage/romstage.c b/src/soc/intel/cannonlake/romstage/romstage.c index c56add7..e8947f1 100644 --- a/src/soc/intel/cannonlake/romstage/romstage.c +++ b/src/soc/intel/cannonlake/romstage/romstage.c @@ -23,9 +23,10 @@ 0x8d, 0x09, 0x11, 0xcf, 0x8b, 0x9f, 0x03, 0x23 \ }
-void __weak mainboard_get_dram_part_num(const char **part_num, size_t *len) +const char * __weak mainboard_get_dram_part_num(void) { /* Default weak implementation, no need to override part number. */ + return NULL; }
/* Save the DIMM information for SMBIOS table 17 */ @@ -42,7 +43,8 @@ const uint8_t smbios_memory_info_guid[16] = FSP_SMBIOS_MEMORY_INFO_GUID; const char *dram_part_num; - size_t dram_part_num_len; + size_t dram_part_num_len = 0; + bool part_num_overridden = false;
/* Locate the memory info HOB, presence validated by raminit */ memory_info_hob = fsp_find_extension_hob_by_guid( @@ -64,6 +66,13 @@ } memset(mem_info, 0, sizeof(*mem_info));
+ /* Allow mainboard to override DRAM part number. */ + dram_part_num = mainboard_get_dram_part_num(); + if (dram_part_num) { + dram_part_num_len = strlen(dram_part_num); + part_num_overridden = true; + } + /* Describe the first N DIMMs in the system */ index = 0; dimm_max = ARRAY_SIZE(mem_info->dimm); @@ -79,13 +88,12 @@ if (src_dimm->Status != DIMM_PRESENT) continue;
- dram_part_num_len = sizeof(src_dimm->ModulePartNum); - dram_part_num = (const char *) + if (!part_num_overridden) { + dram_part_num_len = + sizeof(src_dimm->ModulePartNum); + dram_part_num = (const char *) &src_dimm->ModulePartNum[0]; - - /* Allow mainboard to override DRAM part number. */ - mainboard_get_dram_part_num(&dram_part_num, - &dram_part_num_len); + }
u8 memProfNum = memory_info_hob->MemoryProfile;
diff --git a/src/soc/intel/elkhartlake/include/soc/romstage.h b/src/soc/intel/elkhartlake/include/soc/romstage.h index baa35c5..1cffcb9 100644 --- a/src/soc/intel/elkhartlake/include/soc/romstage.h +++ b/src/soc/intel/elkhartlake/include/soc/romstage.h @@ -6,7 +6,7 @@ #include <fsp/api.h>
/* Provide a callback to allow mainboard to override the DRAM part number. */ -bool mainboard_get_dram_part_num(const char **part_num, size_t *len); +const char *mainboard_get_dram_part_num(void); void mainboard_memory_init_params(FSPM_UPD *mupd); void systemagent_early_init(void); void romstage_pch_init(void); diff --git a/src/soc/intel/elkhartlake/romstage/romstage.c b/src/soc/intel/elkhartlake/romstage/romstage.c index 09b1744..06fa114 100644 --- a/src/soc/intel/elkhartlake/romstage/romstage.c +++ b/src/soc/intel/elkhartlake/romstage/romstage.c @@ -21,10 +21,10 @@ 0x8d, 0x09, 0x11, 0xcf, 0x8b, 0x9f, 0x03, 0x23 \ }
-bool __weak mainboard_get_dram_part_num(const char **part_num, size_t *len) +const char * __weak mainboard_get_dram_part_num(void) { /* Default implementation, no need to override part number. */ - return false; + return NULL; }
/* Save the DIMM information for SMBIOS table 17 */ @@ -42,7 +42,7 @@ FSP_SMBIOS_MEMORY_INFO_GUID; const uint8_t *serial_num; const char *dram_part_num = NULL; - size_t dram_part_num_len; + size_t dram_part_num_len = 0; bool is_dram_part_overridden = false;
/* Locate the memory info HOB, presence validated by raminit */ @@ -66,8 +66,11 @@ memset(mem_info, 0, sizeof(*mem_info));
/* Allow mainboard to override DRAM part number. */ - is_dram_part_overridden = mainboard_get_dram_part_num(&dram_part_num, - &dram_part_num_len); + dram_part_num = mainboard_get_dram_part_num(); + if (dram_part_num) { + dram_part_num = strlen(dram_part_num); + is_dram_part_overridden = true; + }
/* Save available DIMM information */ index = 0; diff --git a/src/soc/intel/jasperlake/include/soc/romstage.h b/src/soc/intel/jasperlake/include/soc/romstage.h index baa35c5..1cffcb9 100644 --- a/src/soc/intel/jasperlake/include/soc/romstage.h +++ b/src/soc/intel/jasperlake/include/soc/romstage.h @@ -6,7 +6,7 @@ #include <fsp/api.h>
/* Provide a callback to allow mainboard to override the DRAM part number. */ -bool mainboard_get_dram_part_num(const char **part_num, size_t *len); +const char *mainboard_get_dram_part_num(void); void mainboard_memory_init_params(FSPM_UPD *mupd); void systemagent_early_init(void); void romstage_pch_init(void); diff --git a/src/soc/intel/jasperlake/romstage/romstage.c b/src/soc/intel/jasperlake/romstage/romstage.c index db014ea..6fddbc4 100644 --- a/src/soc/intel/jasperlake/romstage/romstage.c +++ b/src/soc/intel/jasperlake/romstage/romstage.c @@ -21,10 +21,10 @@ 0x8d, 0x09, 0x11, 0xcf, 0x8b, 0x9f, 0x03, 0x23 \ }
-bool __weak mainboard_get_dram_part_num(const char **part_num, size_t *len) +const char * __weak mainboard_get_dram_part_num(void) { /* Default weak implementation, no need to override part number. */ - return false; + return NULL; }
/* Save the DIMM information for SMBIOS table 17 */ @@ -42,7 +42,7 @@ FSP_SMBIOS_MEMORY_INFO_GUID; const uint8_t *serial_num; const char *dram_part_num = NULL; - size_t dram_part_num_len; + size_t dram_part_num_len = 0; bool is_dram_part_overridden = false;
/* Locate the memory info HOB, presence validated by raminit */ @@ -66,8 +66,11 @@ memset(mem_info, 0, sizeof(*mem_info));
/* Allow mainboard to override DRAM part number. */ - is_dram_part_overridden = mainboard_get_dram_part_num(&dram_part_num, - &dram_part_num_len); + dram_part_num = mainboard_get_dram_part_num(); + if (dram_part_num) { + dram_part_num_len = strlen(dram_part_num); + is_dram_part_overridden = true; + }
/* Save available DIMM information */ index = 0; diff --git a/src/soc/intel/tigerlake/include/soc/romstage.h b/src/soc/intel/tigerlake/include/soc/romstage.h index baa35c5..1cffcb9 100644 --- a/src/soc/intel/tigerlake/include/soc/romstage.h +++ b/src/soc/intel/tigerlake/include/soc/romstage.h @@ -6,7 +6,7 @@ #include <fsp/api.h>
/* Provide a callback to allow mainboard to override the DRAM part number. */ -bool mainboard_get_dram_part_num(const char **part_num, size_t *len); +const char *mainboard_get_dram_part_num(void); void mainboard_memory_init_params(FSPM_UPD *mupd); void systemagent_early_init(void); void romstage_pch_init(void); diff --git a/src/soc/intel/tigerlake/romstage/romstage.c b/src/soc/intel/tigerlake/romstage/romstage.c index db014ea..6fddbc4 100644 --- a/src/soc/intel/tigerlake/romstage/romstage.c +++ b/src/soc/intel/tigerlake/romstage/romstage.c @@ -21,10 +21,10 @@ 0x8d, 0x09, 0x11, 0xcf, 0x8b, 0x9f, 0x03, 0x23 \ }
-bool __weak mainboard_get_dram_part_num(const char **part_num, size_t *len) +const char * __weak mainboard_get_dram_part_num(void) { /* Default weak implementation, no need to override part number. */ - return false; + return NULL; }
/* Save the DIMM information for SMBIOS table 17 */ @@ -42,7 +42,7 @@ FSP_SMBIOS_MEMORY_INFO_GUID; const uint8_t *serial_num; const char *dram_part_num = NULL; - size_t dram_part_num_len; + size_t dram_part_num_len = 0; bool is_dram_part_overridden = false;
/* Locate the memory info HOB, presence validated by raminit */ @@ -66,8 +66,11 @@ memset(mem_info, 0, sizeof(*mem_info));
/* Allow mainboard to override DRAM part number. */ - is_dram_part_overridden = mainboard_get_dram_part_num(&dram_part_num, - &dram_part_num_len); + dram_part_num = mainboard_get_dram_part_num(); + if (dram_part_num) { + dram_part_num_len = strlen(dram_part_num); + is_dram_part_overridden = true; + }
/* Save available DIMM information */ index = 0;
-
Furquan Shaikh (Code Review) -
Nick Vaccaro (Code Review)