Change in coreboot[master]: spd: move mainboard_get_dram_part_num to src/vendor/google/chromeos
Nick Vaccaro has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45588 )
Change subject: spd: move mainboard_get_dram_part_num to src/vendor/google/chromeos ......................................................................
spd: move mainboard_get_dram_part_num to src/vendor/google/chromeos
Move mainboard_get_dram_part_num() into src/vendor/google/chromeos to allow mainboards to use it without having to duplicate that code.
Add SPD_PART_NUMBER_IN_CBI Kconfig option to declare whether the SPD Module Part Number (memory part name) is stored in the CBI.
Remove existing mainboard_get_dram_part_num() definitions from individual mainboards and replaced with appropriate SPD_PART_NUMBER_IN_CBI setting.
BUG=b:168724473 TEST="emerge-volteer coreboot chromeos-bootimage", flash and boot volteer to kernel and verify that the BIOS log shows a part name when logging SPD information:
SPD: module part number is K4U6E3S4AA-MGCL
I also built coreboot for hatch and dedede and verified that the build succeeds without error.
Change-Id: Ie173b81b9c9a98f3744cc9da4b034d2251f5589b Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/mainboard/google/dedede/Kconfig M src/mainboard/google/dedede/romstage.c M src/mainboard/google/hatch/Kconfig M src/mainboard/google/hatch/romstage_spd_cbfs.c M src/mainboard/google/volteer/Kconfig M src/mainboard/google/volteer/romstage.c M src/vendorcode/google/chromeos/Makefile.inc A src/vendorcode/google/chromeos/mainboard.c 8 files changed, 34 insertions(+), 56 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/45588/1
diff --git a/src/mainboard/google/dedede/Kconfig b/src/mainboard/google/dedede/Kconfig index 79c2995..137247b 100644 --- a/src/mainboard/google/dedede/Kconfig +++ b/src/mainboard/google/dedede/Kconfig @@ -100,6 +100,9 @@ string default "variants/$(CONFIG_VARIANT_DIR)/overridetree.cb" if !BOARD_GOOGLE_DEDEDE
+config SPD_PART_NUMBER_IN_CBI + def_bool y + config TPM_TIS_ACPI_INTERRUPT int default 4 # GPE0_DW0_4 (GPP_B4) diff --git a/src/mainboard/google/dedede/romstage.c b/src/mainboard/google/dedede/romstage.c index db6f7db..02ebfac 100644 --- a/src/mainboard/google/dedede/romstage.c +++ b/src/mainboard/google/dedede/romstage.c @@ -21,19 +21,3 @@
memcfg_init(&memupd->FspmConfig, board_cfg, &spd_info, half_populated); } - -bool mainboard_get_dram_part_num(const char **part_num, size_t *len) -{ - 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; - } - - - *part_num = &part_num_store[0]; - *len = strlen(part_num_store); - return true; -} diff --git a/src/mainboard/google/hatch/Kconfig b/src/mainboard/google/hatch/Kconfig index e4e343b..0b456b8 100644 --- a/src/mainboard/google/hatch/Kconfig +++ b/src/mainboard/google/hatch/Kconfig @@ -148,6 +148,10 @@ default "variants/helios_diskswap/overridetree.cb" if BOARD_GOOGLE_HELIOS_DISKSWAP default "variants/$(CONFIG_VARIANT_DIR)/overridetree.cb"
+config SPD_PART_NUMBER_IN_CBI + def_bool y + default n if ROMSTAGE_SPD_SMBUS + config TPM_TIS_ACPI_INTERRUPT int default 53 # GPE0_DW1_21 (GPP_C21) diff --git a/src/mainboard/google/hatch/romstage_spd_cbfs.c b/src/mainboard/google/hatch/romstage_spd_cbfs.c index a9a65e9..0dae6cd 100644 --- a/src/mainboard/google/hatch/romstage_spd_cbfs.c +++ b/src/mainboard/google/hatch/romstage_spd_cbfs.c @@ -7,6 +7,7 @@ #include <memory_info.h> #include <soc/cnl_memcfg_init.h> #include <soc/romstage.h> +#include <spd_bin.h> #include <string.h> #include <variant/gpio.h>
@@ -56,29 +57,3 @@
cannonlake_memcfg_init(&memupd->FspmConfig, &memcfg); } - -void mainboard_get_dram_part_num(const char **part_num, size_t *len) -{ - static char part_num_store[DIMM_INFO_PART_NUMBER_SIZE]; - static enum { - PART_NUM_NOT_READ, - PART_NUM_AVAILABLE, - PART_NUM_NOT_IN_CBI, - } part_num_state = PART_NUM_NOT_READ; - - if (part_num_state == PART_NUM_NOT_READ) { - 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"); - part_num_state = PART_NUM_NOT_IN_CBI; - } else { - part_num_state = PART_NUM_AVAILABLE; - } - } - - if (part_num_state == PART_NUM_NOT_IN_CBI) - return; - - *part_num = &part_num_store[0]; - *len = strlen(part_num_store) + 1; -} diff --git a/src/mainboard/google/volteer/Kconfig b/src/mainboard/google/volteer/Kconfig index dda2f4e..040a171 100644 --- a/src/mainboard/google/volteer/Kconfig +++ b/src/mainboard/google/volteer/Kconfig @@ -8,6 +8,7 @@ select DRIVERS_I2C_SX9310 select DRIVERS_INTEL_DPTF select DRIVERS_INTEL_PMC + select DRIVERS_INTEL_USB4_RETIMER select DRIVERS_I2C_MAX98373 select DRIVERS_INTEL_SOUNDWIRE select DRIVERS_SPI_ACPI @@ -100,6 +101,9 @@ hex default 0x1c000000 # 448 MiB
+config SPD_PART_NUMBER_IN_CBI + def_bool y + config TPM_TIS_ACPI_INTERRUPT int default 21 # GPE0_DW0_21 (GPP_C21) diff --git a/src/mainboard/google/volteer/romstage.c b/src/mainboard/google/volteer/romstage.c index 8893785..bec8f47 100644 --- a/src/mainboard/google/volteer/romstage.c +++ b/src/mainboard/google/volteer/romstage.c @@ -29,17 +29,3 @@
meminit_ddr(mem_cfg, board_cfg, &spd_info, half_populated); } - -bool mainboard_get_dram_part_num(const char **part_num, size_t *len) -{ - 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; - } - *part_num = part_num_store; - *len = strlen(part_num_store); - return true; -} diff --git a/src/vendorcode/google/chromeos/Makefile.inc b/src/vendorcode/google/chromeos/Makefile.inc index e17236d..04f68c4 100644 --- a/src/vendorcode/google/chromeos/Makefile.inc +++ b/src/vendorcode/google/chromeos/Makefile.inc @@ -16,3 +16,5 @@ verstage-y += watchdog.c romstage-y += watchdog.c ramstage-y += watchdog.c + +romstage-y += mainboard.c diff --git a/src/vendorcode/google/chromeos/mainboard.c b/src/vendorcode/google/chromeos/mainboard.c new file mode 100644 index 0000000..6cc6901 --- /dev/null +++ b/src/vendorcode/google/chromeos/mainboard.c @@ -0,0 +1,20 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <console/console.h> +#include <ec/google/chromeec/ec.h> +#include <spd_bin.h> +#include <string.h> + +bool mainboard_get_dram_part_num(const char **part_num, size_t *len) +{ + 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; + } + *part_num = part_num_store; + *len = strlen(part_num_store); + return true; +}
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45588 )
Change subject: spd: move mainboard_get_dram_part_num to src/vendor/google/chromeos ......................................................................
Patch Set 7:
(3 comments)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/45588/7/src/include/spd_bin.h File src/include/spd_bin.h:
https://review.coreboot.org/c/coreboot/+/45588/7/src/include/spd_bin.h@50 PS7, Line 50: bool mainboard_get_dram_part_num(const char **part_num, size_t *len); Is len inclusive or exclusive of the NUL terminator? Please add a comment here indicating expectations of the API.
And shouldn't this declaration be in memory_info.h? I don't see how it directly relates to SPD.
https://review.coreboot.org/c/coreboot/+/45588/7/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45588/7/src/vendorcode/google/chrom... PS7, Line 20: CONFIG_PART_NUMBER_IN_CBI We're relying on each mainboard's setting to expose this Kconfig option? Any reason we wouldn't have that option under Kconfig in vc/google/chromeos ?
Aslo, I think PART_NUMBER_IN_CBI is pretty generic. We should put 'DRAM' in there.
https://review.coreboot.org/c/coreboot/+/45588/7/src/vendorcode/google/chrom... PS7, Line 20: spd I don't think this file is named adequately. SPD is not contributing to what the function is providing.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45588 )
Change subject: spd: move mainboard_get_dram_part_num to src/vendor/google/chromeos ......................................................................
Patch Set 7:
(1 comment)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/45588/7/src/include/spd_bin.h File src/include/spd_bin.h:
https://review.coreboot.org/c/coreboot/+/45588/7/src/include/spd_bin.h@50 PS7, Line 50: bool mainboard_get_dram_part_num(const char **part_num, size_t *len);
Is len inclusive or exclusive of the NUL terminator? Please add a comment here indicating expectatio […]
Also, what does the return value represent?
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45588 )
Change subject: spd: move mainboard_get_dram_part_num to src/vendor/google/chromeos ......................................................................
Patch Set 7:
Patch Set 7:
(1 comment)
This change is ready for review.
Thanks for the feedback, Aaron. I had marked this as WIP so that I could rework it before folks spent time again, but the WIP setting doesn't work as I thought it did, and I think my latest upload for patchset 7 unset the WIP as I see it's not currently set. I've been using coreboot as a faster way to build for all boards to find issues, so I may upload a few times before the patch is ready. I will remove reviewers for now and add them back once CL is ready for review.
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45588 )
Change subject: spd: move mainboard_get_dram_part_num to src/vendor/google/chromeos ......................................................................
Patch Set 8:
(1 comment)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/45588/8/src/mainboard/google/dedede... File src/mainboard/google/dedede/Kconfig:
https://review.coreboot.org/c/coreboot/+/45588/8/src/mainboard/google/dedede... PS8, Line 40: CHROMEOS_DRAM_PART_NUMBER_IN_CBI QUESTION: Should this be : select CHROMEOS_DRAM_PART_NUMBER_IN_CBI if !ROMSTAGE_SPD_SMBUS
That holds true for puff, just not sure if the fact that ROMSTAGE_SPD_SMBUS is set means that they will get their SPD data through SMBUS instead and that they would never require ability to override the dram part number.
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45588 )
Change subject: spd: move weak mainboard_get_dram_part_num implementation to lib/spd_bin.c ......................................................................
Patch Set 18:
(1 comment)
This is now ready for review.
https://review.coreboot.org/c/coreboot/+/45588/8/src/mainboard/google/dedede... File src/mainboard/google/dedede/Kconfig:
https://review.coreboot.org/c/coreboot/+/45588/8/src/mainboard/google/dedede... PS8, Line 40: CHROMEOS_DRAM_PART_NUMBER_IN_CBI
!ROMSTAGE_SPD_SMBUS is required only in mainboard hatch because it supports Hatch reference (using m […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45588 )
Change subject: spd: move weak mainboard_get_dram_part_num implementation to lib/spd_bin.c ......................................................................
Patch Set 18:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45588/18/src/include/memory_info.h File src/include/memory_info.h:
https://review.coreboot.org/c/coreboot/+/45588/18/src/include/memory_info.h@... PS18, Line 116: ength of string not including the null terminator I don't think that is correct. Looking at the implementation, len seems to return the length of string including '\0' terminator.
https://review.coreboot.org/c/coreboot/+/45588/18/src/include/memory_info.h@... PS18, Line 119: bool Can we use `const char *` as the return type for this function with expectation that: NULL = No part number override provided by mainboard non-NULL = Pointer to a string terminating in '\0' and whose length is returned in `len`.
I think it is easier to read and understand what the function does rather than the bool true/false.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45588
to look at the new patch set (#19).
Change subject: spd: move weak mainboard_get_dram_part_num implementation to lib/spd_bin.c ......................................................................
spd: move weak mainboard_get_dram_part_num implementation to lib/spd_bin.c
Move mainboard_get_dram_part_num() prototype to include/memory_info.h.
Remove existing weak mainboard_get_dram_part_num() definitions from individual mainboards and replaced with single weak implementation in src/lib/spd_bin.c.
BUG=b:168724473 TEST="emerge-volteer coreboot chromeos-bootimage", flash and boot volteer to kernel and verify that the BIOS log shows a part name when logging SPD information:
SPD: module part number is K4U6E3S4AA-MGCL
I also built coreboot for hatch and dedede and verified that the build succeeds without error.
Change-Id: Ie173b81b9c9a98f3744cc9da4b034d2251f5589b Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/memory_info.h M src/lib/spd_bin.c 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 15 files changed, 38 insertions(+), 63 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/45588/19
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45588
to look at the new patch set (#20).
Change subject: spd: move weak mainboard_get_dram_part_num implementation to lib/spd_bin.c ......................................................................
spd: move weak mainboard_get_dram_part_num implementation to lib/spd_bin.c
Move mainboard_get_dram_part_num() prototype to include/memory_info.h.
Remove existing weak mainboard_get_dram_part_num() definitions from individual mainboards and replaced with single weak implementation in src/lib/spd_bin.c.
BUG=b:168724473 TEST="emerge-volteer coreboot chromeos-bootimage", flash and boot volteer to kernel and verify that the BIOS log shows a part name when logging SPD information:
SPD: module part number is K4U6E3S4AA-MGCL
I also built coreboot for hatch and dedede and verified that the build succeeds without error.
Change-Id: Ie173b81b9c9a98f3744cc9da4b034d2251f5589b Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/memory_info.h M src/lib/spd_bin.c 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 15 files changed, 52 insertions(+), 71 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/45588/20
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45588 )
Change subject: spd: move weak mainboard_get_dram_part_num implementation to lib/spd_bin.c ......................................................................
Patch Set 20:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45588/20/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45588/20/src/lib/spd_bin.c@187 PS20, Line 187: nameptr = spd_get_name spd_get_name returns void
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45588 )
Change subject: spd: move weak mainboard_get_dram_part_num implementation to lib/spd_bin.c ......................................................................
Patch Set 20:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45588/20/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45588/20/src/lib/spd_bin.c@146 PS20, Line 146: char spd_name[] This is not really required anymore. You can just return &spd[offset];
where offset is dependent on dram_type.
https://review.coreboot.org/c/coreboot/+/45588/20/src/mainboard/google/deded... File src/mainboard/google/dedede/romstage.c:
https://review.coreboot.org/c/coreboot/+/45588/20/src/mainboard/google/deded... PS20, Line 35: (const char *) Typecast not really required here and in other files.
https://review.coreboot.org/c/coreboot/+/45588/20/src/soc/intel/elkhartlake/... File src/soc/intel/elkhartlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45588/20/src/soc/intel/elkhartlake/... PS20, Line 24: const char *mainboard_get_dram_part_num(void) : { : /* Default implementation, no need to override part number. */ : return NULL; : } This is no longer required. This platform seems to have missed adding __weak to the function originally.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45588 )
Change subject: spd: move weak mainboard_get_dram_part_num implementation to lib/spd_bin.c ......................................................................
Patch Set 20:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45588/20/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45588/20/src/lib/spd_bin.c@146 PS20, Line 146: char spd_name[]
This is not really required anymore. You can just return &spd[offset]; […]
Actually this change should be made in CB:45459
https://review.coreboot.org/c/coreboot/+/45588/20/src/lib/spd_bin.c@170 PS20, Line 170: print_spd_info None of the changes in this function should be part of this CL. They should move to CB:45459
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45588
to look at the new patch set (#21).
Change subject: spd: move weak mainboard_get_dram_part_num implementation to lib/spd_bin.c ......................................................................
spd: move weak mainboard_get_dram_part_num implementation to lib/spd_bin.c
Move mainboard_get_dram_part_num() prototype to include/memory_info.h.
Remove existing weak mainboard_get_dram_part_num() definitions from individual mainboards and replaced with single weak implementation in src/lib/spd_bin.c.
BUG=b:168724473 TEST="emerge-volteer coreboot chromeos-bootimage", flash and boot volteer to kernel and verify that the BIOS log shows a part name when logging SPD information:
SPD: module part number is K4U6E3S4AA-MGCL
I also built coreboot for hatch and dedede and verified that the build succeeds without error.
Change-Id: Ie173b81b9c9a98f3744cc9da4b034d2251f5589b Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/memory_info.h M src/lib/spd_bin.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/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 11 files changed, 15 insertions(+), 34 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/45588/21
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45588 )
Change subject: spd: move weak mainboard_get_dram_part_num implementation to lib/spd_bin.c ......................................................................
Patch Set 21:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45588/18/src/include/memory_info.h File src/include/memory_info.h:
https://review.coreboot.org/c/coreboot/+/45588/18/src/include/memory_info.h@... PS18, Line 116: ength of string not including the null terminator
I don't think that is correct. […]
re: "Looking at the implementation, len seems to return the length of string including '\0' terminator "
I guess that depends on who you ask.
In the hard version that's added 3 CL's up (45636), it uses the following :
*len = strlen(part_num_store);
My original version had that change at the base, so this was not an issue. I do see that in this patchset, without the inclusion of 45636, one returns strlen+1 (hatch) and the other two return strlen (dedede, volteer). The final version added in 45636 no longer includes length since it returns a null-terminated string.
https://review.coreboot.org/c/coreboot/+/45588/20/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45588/20/src/lib/spd_bin.c@187 PS20, Line 187: nameptr = spd_get_name
spd_get_name returns void
Sorry, last round of changes were still sitting in chromeos coreboot (testing grounds).
https://review.coreboot.org/c/coreboot/+/45588/20/src/mainboard/google/deded... File src/mainboard/google/dedede/romstage.c:
https://review.coreboot.org/c/coreboot/+/45588/20/src/mainboard/google/deded... PS20, Line 35: (const char *)
Typecast not really required here and in other files.
Done
https://review.coreboot.org/c/coreboot/+/45588/20/src/soc/intel/elkhartlake/... File src/soc/intel/elkhartlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45588/20/src/soc/intel/elkhartlake/... PS20, Line 24: const char *mainboard_get_dram_part_num(void) : { : /* Default implementation, no need to override part number. */ : return NULL; : }
This is no longer required. […]
This gets removed in https://review.coreboot.org/c/coreboot/+/45636/13/src/soc/intel/elkhartlake/...
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45588 )
Change subject: spd: move weak mainboard_get_dram_part_num implementation to lib/spd_bin.c ......................................................................
Patch Set 23:
This change is ready for review.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45588
to look at the new patch set (#24).
Change subject: spd: move weak mainboard_get_dram_part_num implementation to lib/spd_bin.c ......................................................................
spd: move weak mainboard_get_dram_part_num implementation to lib/spd_bin.c
Move mainboard_get_dram_part_num() prototype to include/memory_info.h.
Remove existing weak mainboard_get_dram_part_num() definitions from individual mainboards and replaced with single weak implementation in src/lib/spd_bin.c.
BUG=b:168724473 TEST="emerge-volteer coreboot chromeos-bootimage && emerge-dedede coreboot chromeos-bootimage && "emerge-hatch coreboot chromeos-bootimage" and verify builds succeed.
Change-Id: Ie173b81b9c9a98f3744cc9da4b034d2251f5589b Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/memory_info.h M src/lib/spd_bin.c M src/mainboard/google/hatch/romstage_spd_cbfs.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/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 12 files changed, 18 insertions(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/45588/24
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45588 )
Change subject: spd: move weak mainboard_get_dram_part_num implementation to lib/spd_bin.c ......................................................................
Patch Set 24:
(1 comment)
ping...
https://review.coreboot.org/c/coreboot/+/45588/20/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45588/20/src/lib/spd_bin.c@170 PS20, Line 170: print_spd_info
None of the changes in this function should be part of this CL. […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45588 )
Change subject: spd: move weak mainboard_get_dram_part_num implementation to lib/spd_bin.c ......................................................................
Patch Set 24:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45588/18/src/include/memory_info.h File src/include/memory_info.h:
https://review.coreboot.org/c/coreboot/+/45588/18/src/include/memory_info.h@... PS18, Line 116: ength of string not including the null terminator
re: "Looking at the implementation, len seems to return the length of string including '\0' terminat […]
Since this CL is the one providing the notion of a common mainboard_get_dram_part_num(), why not start with the correct function prototype in this CL itself? This will avoid the change in CB:45636 to again update the prototype and touch the common code in memory_info.h and spd_bin.c.
Nick Vaccaro has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/45588 )
Change subject: spd: move weak mainboard_get_dram_part_num implementation to lib/spd_bin.c ......................................................................
Abandoned
abandoning for change-reduced approach
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Nick Vaccaro (Code Review)
-
Tim Wawrzynczak (Code Review)