Nick Vaccaro has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
soc/intel/tigerlake: log the memory part name
THe BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. In those cases, devices define their DRAM Part Name in the CBI, which can be retrieved by calling mainboard_get_dram_part_num().
Add a spd_set_name() call to the spd library to allow logging the memory part name in cases where the name does not exist in the actual SPD data, and call it in cases where the mainboard is overriding the part name.
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
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c M src/soc/intel/tigerlake/meminit.c 3 files changed, 24 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/1
diff --git a/src/include/spd_bin.h b/src/include/spd_bin.h index 11a0084..80c2e62 100644 --- a/src/include/spd_bin.h +++ b/src/include/spd_bin.h @@ -47,6 +47,7 @@ int get_spd_cbfs_rdev(struct region_device *spd_rdev, u8 spd_index); void dump_spd_info(struct spd_block *blk); void get_spd_smbus(struct spd_block *blk); +void spd_set_name(uint8_t spd[], char part_name[]);
/* * get_spd_sn returns the SODIMM serial number. It only supports DDR3 and DDR4. diff --git a/src/lib/spd_bin.c b/src/lib/spd_bin.c index 3888896..78c3e98 100644 --- a/src/lib/spd_bin.c +++ b/src/lib/spd_bin.c @@ -136,8 +136,23 @@ return spd_busw[index]; }
+static char *spd_dram_part_name; +static bool spd_part_name_overridden = false; +void spd_set_name(uint8_t spd[], char *part_name) +{ + spd_dram_part_name = part_name; + spd_part_name_overridden = true; +} + static void spd_get_name(const uint8_t spd[], char spd_name[], int dram_type) { + /* If memory part name is overridden, use override copy */ + if (spd_part_name_overridden) { + memcpy(spd_name, spd_dram_part_name, + strlen(spd_dram_part_name)); + return; + } + switch (dram_type) { case SPD_DRAM_DDR3: memcpy(spd_name, &spd[DDR3_SPD_PART_OFF], DDR3_SPD_PART_LEN); diff --git a/src/soc/intel/tigerlake/meminit.c b/src/soc/intel/tigerlake/meminit.c index 0c6f0b0..a027f2f 100644 --- a/src/soc/intel/tigerlake/meminit.c +++ b/src/soc/intel/tigerlake/meminit.c @@ -4,6 +4,7 @@ #include <console/console.h> #include <fsp/util.h> #include <soc/meminit.h> +#include <soc/romstage.h> #include <spd_bin.h> #include <string.h>
@@ -216,6 +217,9 @@
static void read_md_spd(const struct spd_info *info, uintptr_t *data, size_t *len) { + const char *spd_name; + size_t spd_name_len; + if (info->md_spd_loc == SPD_MEMPTR) { *data = info->data_ptr; *len = info->data_len; @@ -225,6 +229,10 @@ die("Not a valid location(%d) for Memory-down SPD!\n", info->md_spd_loc); }
+ /* if mainboard overrides module name, use override name */ + if (mainboard_get_dram_part_num(&spd_name, &spd_name_len)) + spd_set_name((uint8_t *) *data, (char *) spd_name); + print_spd_info((uint8_t *) *data); }
Hello Furquan Shaikh, Caveh Jalali, Tim Wawrzynczak, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#2).
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
soc/intel/tigerlake: log the memory part name
THe BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. In those cases, devices define their DRAM part pame in the CBI and will declare the DRAM part name via overriding mainboard_get_dram_part_num().
Add a spd_set_name() call to the spd library to allow logging the memory part name in cases where the name does not exist in the actual SPD data, and call it in cases where the mainboard is overriding the part name.
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
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c M src/soc/intel/tigerlake/meminit.c 3 files changed, 24 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/2
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Tim Wawrzynczak, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#3).
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
soc/intel/tigerlake: log the memory part name
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. In those cases, devices define their DRAM part pame in the CBI and will declare the DRAM part name via overriding mainboard_get_dram_part_num().
Add a spd_set_name() call to the spd library to allow logging the memory part name in cases where the name does not exist in the actual SPD data, and call it in cases where the mainboard is overriding the part name.
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
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c M src/soc/intel/tigerlake/meminit.c 3 files changed, 24 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/3
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45459/3/src/include/spd_bin.h File src/include/spd_bin.h:
https://review.coreboot.org/c/coreboot/+/45459/3/src/include/spd_bin.h@50 PS3, Line 50: void spd_set_name(uint8_t spd[], char part_name[]); nit: it's functionally the same, but you declare it here as `char part_name[]` but in the .c file it's `char *part_name`.
https://review.coreboot.org/c/coreboot/+/45459/3/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/3/src/lib/spd_bin.c@140 PS3, Line 140: static bool spd_part_name_overridden = false; all statics are cleared to 0 (bss section). Also this variable seems redundant; you could just check if` spd_dram_part_name != 0` to know if the override has been called;
https://review.coreboot.org/c/coreboot/+/45459/3/src/lib/spd_bin.c@141 PS3, Line 141: spd_set_name suggestion: `spd_override_name`
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Tim Wawrzynczak, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#4).
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
soc/intel/tigerlake: log the memory part name
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. In those cases, devices define their DRAM part pame in the CBI and will declare the DRAM part name via overriding mainboard_get_dram_part_num().
Add a spd_set_name() call to the spd library to allow logging the memory part name in cases where the name does not exist in the actual SPD data, and call it in cases where the mainboard is overriding the part name.
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
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c M src/soc/intel/tigerlake/meminit.c 3 files changed, 22 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/4
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45459/3/src/include/spd_bin.h File src/include/spd_bin.h:
https://review.coreboot.org/c/coreboot/+/45459/3/src/include/spd_bin.h@50 PS3, Line 50: void spd_set_name(uint8_t spd[], char part_name[]);
nit: it's functionally the same, but you declare it here as `char part_name[]` but in the . […]
Done
https://review.coreboot.org/c/coreboot/+/45459/3/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/3/src/lib/spd_bin.c@140 PS3, Line 140: static bool spd_part_name_overridden = false;
all statics are cleared to 0 (bss section). […]
Agreed. I originally had this someplace else and didn't want to include strings, but I ended up having to move it here, which already includes strings.
https://review.coreboot.org/c/coreboot/+/45459/3/src/lib/spd_bin.c@141 PS3, Line 141: spd_set_name
suggestion: `spd_override_name`
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45459/4/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/4/src/lib/spd_bin.c@139 PS4, Line 139: spd_dram_part_name suggestion: spd_dram_part_name_override
https://review.coreboot.org/c/coreboot/+/45459/4/src/lib/spd_bin.c@148 PS4, Line 148: strlen(spd_dram_part_name) If spd_dram_part_name is never set, then it will be NULL and strlen() will deref the NULL pointer
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Tim Wawrzynczak, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#5).
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
soc/intel/tigerlake: log the memory part name
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. In those cases, devices define their DRAM part pame in the CBI and will declare the DRAM part name via overriding mainboard_get_dram_part_num().
Add a spd_set_name() call to the spd library to allow logging the memory part name in cases where the name does not exist in the actual SPD data, and call it in cases where the mainboard is overriding the part name.
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
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c M src/soc/intel/tigerlake/meminit.c 3 files changed, 22 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/5
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45459/4/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/4/src/lib/spd_bin.c@139 PS4, Line 139: spd_dram_part_name
suggestion: spd_dram_part_name_override
Done
https://review.coreboot.org/c/coreboot/+/45459/4/src/lib/spd_bin.c@148 PS4, Line 148: strlen(spd_dram_part_name)
If spd_dram_part_name is never set, then it will be NULL and strlen() will deref the NULL pointer
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/5/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/5/src/lib/spd_bin.c@148 PS5, Line 148: if (spd_part_name_override && strlen(spd_part_name_override)) { Since in this case, you don't know what type of `spd` it is you're dealing with, how do you know how many bytes should be copied into `spd_name`? If it's DDR3, then it should be DDR3_SPD_PART_LEN, if it's DDR4, then it should be DDR4_SPD_PART_LEN... I think this may need to go into the different case statements below because of that. At minimum, a length check is needed when doing a memcpy() w/ a strlen()
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Tim Wawrzynczak, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#6).
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
soc/intel/tigerlake: log the memory part name
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. In those cases, devices define their DRAM part pame in the CBI and will declare the DRAM part name via overriding mainboard_get_dram_part_num().
Add a spd_set_name() call to the spd library to allow logging the memory part name in cases where the name does not exist in the actual SPD data, and call it in cases where the mainboard is overriding the part name.
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 modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c M src/soc/intel/tigerlake/meminit.c 3 files changed, 63 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/6
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/6/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/6/src/lib/spd_bin.c@182 PS6, Line 182: switch (dram_type) { Since both functions switch on `dram_type`, couldn't the `len = *_SPD_PART_LEN` just go in here too ?
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45459/5/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/5/src/lib/spd_bin.c@148 PS5, Line 148: if (spd_part_name_override && strlen(spd_part_name_override)) {
Since in this case, you don't know what type of `spd` it is you're dealing with, how do you know how […]
I had a different version that did what you suggest, but that change seemed overly complicated, and I realized that this was a static routine and was only called from one place, which already uses a string size for the biggest string it might encounter (DDR4 - 20 chars). But you're correct, this still should have had a MIN to assure that the override name is not longer than the jedec spec for the name size.
https://review.coreboot.org/c/coreboot/+/45459/6/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/6/src/lib/spd_bin.c@154 PS6, Line 154: /* verify whether LPDDR3_JEDEC size is same as LPDDR3 or DDR4 */ Tim, do you have or know where I can find a JEDEC SPD spec for LPDDR3_JEDEC? I need to know what the length of a LPDDR3_JEDEC Module Part Number size is.
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/6/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/6/src/lib/spd_bin.c@154 PS6, Line 154: /* verify whether LPDDR3_JEDEC size is same as LPDDR3 or DDR4 */
Tim, do you have or know where I can find a JEDEC SPD spec for LPDDR3_JEDEC? I need to know what th […]
The old code used DDR4_SPD_PART_LEN, just want to make sure that's correct.
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/6/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/6/src/lib/spd_bin.c@182 PS6, Line 182: switch (dram_type) {
Since both functions switch on `dram_type`, couldn't the `len = *_SPD_PART_LEN` just go in here too […]
The slight (but important) difference in the case statements is that the cases in this switch statement is grouped based on the offset within the spd for the part string and not the length of the part name in the SPD. They may be the same, or this may have been a bug in the prior code where it didn't notice that an LPDDR3_JEDEC string was using a DDR4_SPD_PART_LEN size.
But I can change this slightly to remove that and merge the two routines.
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Tim Wawrzynczak, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#7).
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
soc/intel/tigerlake: log the memory part name
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. In those cases, devices define their DRAM part pame in the CBI and will declare the DRAM part name via overriding mainboard_get_dram_part_num().
Add a spd_set_name() call to the spd library to allow logging the memory part name in cases where the name does not exist in the actual SPD data, and call it in cases where the mainboard is overriding the part name.
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 modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c M src/soc/intel/tigerlake/meminit.c 3 files changed, 56 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/7
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/6/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/6/src/lib/spd_bin.c@182 PS6, Line 182: switch (dram_type) {
The slight (but important) difference in the case statements is that the cases in this switch statem […]
It's now reduced to a single routine, not sure if it's actually cleaner this way, but I'm ok with it if this is your preference to have it as a single routine.
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Tim Wawrzynczak, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#8).
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
soc/intel/tigerlake: log the memory part name
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. In those cases, devices define their DRAM part pame in the CBI and will declare the DRAM part name via overriding mainboard_get_dram_part_num().
Add a spd_set_name() call to the spd library to allow logging the memory part name in cases where the name does not exist in the actual SPD data, and call it in cases where the mainboard is overriding the part name.
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 modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c M src/soc/intel/tigerlake/meminit.c 3 files changed, 48 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/8
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/7/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/7/src/lib/spd_bin.c@196 PS7, Line 196: You could also do: switch (dram_type) { case SPD_DRAM_DDR3: name_ptr = (char *) &spd[DDR3_SPD_PART_OFF]; name_len = DDR3_SPD_PART_LEN; break; // ... others }
if (spd_part_name_override) { name_ptr = spd_name_part_override; name_len = MIN(strlen(spd_part_name_override), name_len); }
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Tim Wawrzynczak, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#9).
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
soc/intel/tigerlake: log the memory part name
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. In those cases, devices define their DRAM part pame in the CBI and will declare the DRAM part name via overriding mainboard_get_dram_part_num().
Add a spd_set_name() call to the spd library to allow logging the memory part name in cases where the name does not exist in the actual SPD data, and call it in cases where the mainboard is overriding the part name.
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 modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c M src/soc/intel/tigerlake/meminit.c 3 files changed, 35 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/9
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/7/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/7/src/lib/spd_bin.c@196 PS7, Line 196:
You could also do: […]
Good suggestion!
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/9/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit.c:
PS9: One last thing 😄, the changes in this file should go in a separate CL on top of this one
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: soc/intel/tigerlake: log the memory part name ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/9/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit.c:
PS9:
One last thing 😄, the changes in this file should go in a separate CL on top of this one
Another good point 😊 I'll fix this.
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Tim Wawrzynczak, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#10).
Change subject: lib/spd: log override for spd memory part name ......................................................................
lib/spd: log override for spd memory part name
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. In those cases, devices define their DRAM part pame in the CBI and will declare the DRAM part name via overriding mainboard_get_dram_part_num().
Add a spd_set_name() call to the spd library to allow logging the memory part name in cases where the name does not exist in the actual SPD data.
BUG=b:168724473 TEST=cherry-pick change to enable this on Tiger Lake, then execute "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 modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c 2 files changed, 27 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/10
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Tim Wawrzynczak, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#11).
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
lib/spd: respect spd memory part name override if present
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. In those cases, devices define their DRAM part pame in the CBI and will declare the DRAM part name via overriding mainboard_get_dram_part_num().
Add a spd_set_name() call to the spd library to allow logging the memory part name in cases where the name does not exist in the actual SPD data.
BUG=b:168724473 TEST=cherry-pick change to enable this on Tiger Lake, then execute "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 modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c 2 files changed, 27 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/11
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/9/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit.c:
PS9:
Another good point 😊 I'll fix this.
Done
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/6/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/6/src/lib/spd_bin.c@154 PS6, Line 154: /* verify whether LPDDR3_JEDEC size is same as LPDDR3 or DDR4 */
The old code used DDR4_SPD_PART_LEN, just want to make sure that's correct.
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45459/12/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/12/src/lib/spd_bin.c@140 PS12, Line 140: uint8_t spd[] why is spd passed in when you're trying to define the part name that is out of band from the spd?
https://review.coreboot.org/c/coreboot/+/45459/12/src/lib/spd_bin.c@177 PS12, Line 177: spd_name[name_len + 1] = 0; This is actually a change. e.g. in ddr4 case this index is now DDR4_SPD_PART_LEN + 1 where it wasn't prior, and it is now an out of bound access.
This function needs the buffer size of spd_name to be correct.
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Tim Wawrzynczak, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#13).
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
lib/spd: respect spd memory part name override if present
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. In those cases, devices define their DRAM part pame in the CBI and will declare the DRAM part name via overriding mainboard_get_dram_part_num().
Add a spd_set_name() call to the spd library to allow logging the memory part name in cases where the name does not exist in the actual SPD data.
BUG=b:168724473 TEST=cherry-pick change to enable this on Tiger Lake, then execute "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 modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c 2 files changed, 27 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/13
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/12/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/12/src/lib/spd_bin.c@140 PS12, Line 140: uint8_t spd[]
why is spd passed in when you're trying to define the part name that is out of band from the spd?
Good catch, thanks, Aaron. It was a left-over from a previous implementation that I missed removing once optimized.
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/12/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/12/src/lib/spd_bin.c@177 PS12, Line 177: spd_name[name_len + 1] = 0;
This is actually a change. e.g. […]
I had found the descrepancy while changing the code and considered adding a comment in code to point it out and explain my logic behind why I did not match the original code in this respect, but felt it probably better to point the issue out in a review comment instead of a large comment in the code.
I think it was incorrect / a minor bug to begin with, and here's why I think that.
The purpose of spd_get_name() is to get the "Module Part Number" field from the spd and return a null-terminated string containing that data in the input char * spd_name. In the actual SPD, from what I can tell from the JEDEC spec, the part number is not specified to be a null-terminated string, but is to have 0x20's anywhere that isn't the part name. In the case of this routine, the callers expect those trailing spaces to be stripped (or at a minimum expect the string to be null-terminated). Because a string can be the same size as DDR_SPD_PART_LEN, it must be stored into a buffer that is DDR_SPD_PART_LEN + 1 to assure space for the null termination, which the caller of this static routine does assure, so it is not an out of bounds access given the caller uses proper logic in picking the size of the spd_name buffer it passes. The logic of the previous code would chop off the last character of the part number in the string it returned IF the part number took up the entire field space with part name that contained DDR_SPD_PART_LEN significant characters (no null-termination and no 0x20 spaces). This version does not truncate that case.
This code is only ever used for logging purposes, so my guess is that this truncating behavior hasn't been noticed as one would have to encounter a part number that took up the entire space reserved in SPD AND an observant reviewer of the log to see that the last character in the "Module Part Number" field was truncated on that memory combination.
Would you agree, or am I missing something in my logic?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/12/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/12/src/lib/spd_bin.c@177 PS12, Line 177: spd_name[name_len + 1] = 0;
I had found the descrepancy while changing the code and considered adding a comment in code to point […]
I disagree on my specific comment. name_len +1 where name_len == DDR4_SPD_PART_LEN would be an out of bounds array access smashing the stack.
The point of my comment is that we should pass in ARRAY_SIZE(spd_name) into this function to correctly fill out the buffer. Your change will write to the array out of bounds.
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/12/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/12/src/lib/spd_bin.c@177 PS12, Line 177: spd_name[name_len + 1] = 0;
I had found the descrepancy while changing the code and considered adding a comment in code to point […]
re: "This version does not truncate that case."
Actually, the initial version did not truncate the string, I see my mistake. I will re-work this patch, but need to take a break first and clear my head, I am a bit too tired / missing details. Sorry for the thrash.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/12/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/12/src/lib/spd_bin.c@177 PS12, Line 177: spd_name[name_len + 1] = 0;
re: "This version does not truncate that case." […]
no worries.
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Tim Wawrzynczak, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#14).
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
lib/spd: respect spd memory part name override if present
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. In those cases, devices define their DRAM part pame in the CBI and will declare the DRAM part name via overriding mainboard_get_dram_part_num().
Add a spd_set_name() call to the spd library to allow logging the memory part name in cases where the name does not exist in the actual SPD data.
BUG=b:168724473 TEST=cherry-pick change to enable this on Tiger Lake, then execute "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 modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c 2 files changed, 27 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/14
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/14/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/14/src/lib/spd_bin.c@163 PS14, Line 163: DDR4_SPD_PART_LEN DDR4_SPD_PART_OFF
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45459/14/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/14/src/lib/spd_bin.c@177 PS14, Line 177: name_len I think the correct thing to do is pass in the size of the target buffer -- either in a separate CL or in this one.
https://review.coreboot.org/c/coreboot/+/45459/14/src/lib/spd_bin.c@177 PS14, Line 177: 0 since this is a string we should use '\0' to be more correct, imo.
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Tim Wawrzynczak, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#15).
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
lib/spd: respect spd memory part name override if present
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. In those cases, devices define their DRAM part pame in the CBI and will declare the DRAM part name via overriding mainboard_get_dram_part_num().
Add a spd_set_name() call to the spd library to allow logging the memory part name in cases where the name does not exist in the actual SPD data.
BUG=b:168724473 TEST=cherry-pick change to enable this on Tiger Lake, then execute "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 modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c 2 files changed, 28 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/15
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Tim Wawrzynczak, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#16).
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
lib/spd: respect spd memory part name override if present
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. In those cases, devices define their DRAM part pame in the CBI and will declare the DRAM part name via overriding mainboard_get_dram_part_num().
Add a spd_set_name() call to the spd library to allow logging the memory part name in cases where the name does not exist in the actual SPD data.
BUG=b:168724473 TEST=cherry-pick change to enable this on Tiger Lake, then execute "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 modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c 2 files changed, 34 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/16
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 16:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45459/14/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/14/src/lib/spd_bin.c@163 PS14, Line 163: DDR4_SPD_PART_LEN
DDR4_SPD_PART_OFF
Done
https://review.coreboot.org/c/coreboot/+/45459/14/src/lib/spd_bin.c@177 PS14, Line 177: name_len
I think the correct thing to do is pass in the size of the target buffer -- either in a separate CL […]
Done
https://review.coreboot.org/c/coreboot/+/45459/14/src/lib/spd_bin.c@177 PS14, Line 177: 0
since this is a string we should use '\0' to be more correct, imo.
I agree, I was just following example in this file.
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/12/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/12/src/lib/spd_bin.c@177 PS12, Line 177: spd_name[name_len + 1] = 0;
no worries.
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/16/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/16/src/lib/spd_bin.c@202 PS16, Line 202: spd_get_name(spd, spd_name, ARRAY_SIZE(spd_name), type); : : printk(BIOS_INFO, "SPD: module part number is %s\n", spd_name); Sorry, I am a little late to the party here. I am thinking if we really need to do memcpy of the part name into a local buffer here? Instead can we organize the code such that `spd_get_name()` returns a pointer to a character buffer which is the start of the part name and length indicating how long the name is.
Something like: spd_get_name(spd, type, &spd_name, &spd_name_len); printk(BIOS_INFO, "SPD: module part number is "%.*s\n", spd_name_len, spd_name);
And the spd_get_name() function can return a pointer within the SPD buffer or whatever the mainboard provides w.r.t. part name:
static void spd_get_name(const uint8_t spd[], int type, const char **spd_name, size_t *spd_name_len) { if (mainboard_get_dram_part_num(&spd_name, &spd_name_len)) return;
switch (type) { } }
Also, I think we should move the declaration of `mainboard_get_dram_part_number()` to src/include/spd_bin.h and provide a weak declaration here in spd_bin.c. This function is going to be required for all Intel and AMD platforms going forward at least for Chrome OS devices. I think rather than duplicating the weak function in multiple places and adding it to every SoC header file, we can just have it in one place here in spd_bin.c/h.
Additionally, we can provide the strong implementation of `mainboard_get_dram_part_num()` in src/vendorcode/google/chromeos/ so that it can be used by mainboards without having to duplicate that code by selecting a Kconfig.
I know this is much more than what you started with, but I think there is some clean up that we can benefit from. Also, the part about `mainboard_get_dram_part_number()` should really be done as separate CLs.
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/16/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/16/src/lib/spd_bin.c@202 PS16, Line 202: spd_get_name(spd, spd_name, ARRAY_SIZE(spd_name), type); : : printk(BIOS_INFO, "SPD: module part number is %s\n", spd_name);
Sorry, I am a little late to the party here. […]
Great suggestion, Furquan, I will upload a CL implementing this change later today.
re: Additionally, we can provide the strong implementation of `mainboard_get_dram_part_num()` in src/vendorcode/google/chromeos/ so that it can be used by mainboards without having to duplicate that code by selecting a Kconfig.
Does this mean (this change will mean) that all mainboards going forward on chromeos will define their memory part number in the CBI? Is that a mandate for all chromebooks?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/16/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/16/src/lib/spd_bin.c@202 PS16, Line 202: spd_get_name(spd, spd_name, ARRAY_SIZE(spd_name), type); : : printk(BIOS_INFO, "SPD: module part number is %s\n", spd_name);
Does this mean (this change will mean) that all mainboards going forward on chromeos will define their memory part number in the CBI? Is that a mandate for all chromebooks?
Yes, all boards using memory down configuration will have their part number in CBI(at least for Intel and AMD-based platforms). This is already true for hatch, octopus, dedede, volteer and zork. Same will continue for future platforms.
Also, having a Kconfig that allows mainboard to select whether it wants the part number to be read from CBI will ensure that this strong implementation is enabled only on the boards that really need it.
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Tim Wawrzynczak, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#17).
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
lib/spd: respect spd memory part name override if present
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. In those cases, devices define their DRAM part name in the CBI.
BUG=b:168724473 TEST=cherry-pick change to enable this on Tiger Lake, then execute "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 modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c M src/soc/intel/tigerlake/include/soc/romstage.h M src/soc/intel/tigerlake/romstage/romstage.c 4 files changed, 34 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/17
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 22:
(3 comments)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/45459/22/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/22/src/lib/spd_bin.c@148 PS22, Line 148: if (mainboard_get_dram_part_num(spd_name, len)) The default implementation for this function is sitting under chromeos. It won't unconditionally be available.
https://review.coreboot.org/c/coreboot/+/45459/22/src/lib/spd_bin.c@197 PS22, Line 197: memcpy(spd_name, spd_name_ptr, len); spd_name size needs to be taken into account here.
https://review.coreboot.org/c/coreboot/+/45459/22/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/romstage.h:
https://review.coreboot.org/c/coreboot/+/45459/22/src/soc/intel/tigerlake/in... PS22, Line 7: #include <spd_bin.h> What is this #include for?
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 35:
(3 comments)
This change is now ready for review.
https://review.coreboot.org/c/coreboot/+/45459/22/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/22/src/lib/spd_bin.c@195 PS22, Line 195: if string is not null terminated, copy local and add termination
I don't think that is really required. You can use printk with %. […]
Great trick ! I didn't know about that printk option.
https://review.coreboot.org/c/coreboot/+/45459/22/src/lib/spd_bin.c@197 PS22, Line 197: memcpy(spd_name, spd_name_ptr, len);
spd_name size needs to be taken into account here.
Done
https://review.coreboot.org/c/coreboot/+/45459/22/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/romstage.h:
https://review.coreboot.org/c/coreboot/+/45459/22/src/soc/intel/tigerlake/in... PS22, Line 7: #include <spd_bin.h>
What is this #include for?
Originally, my thinking was that a caller of mainboard_get_dram_part_num() would already be including soc/romstage.h, so if I added the new location of the prototype as an include here, no modification would be needed for all the source files needing the mainboard_get_dram_part_num prototype, keeping the files touched as part of this change minimal. I felt that the extra level of indirection, if you will, was worth not touching all those other files. However, that was before Furquan suggested collapsing down the calls / moving mainboard_get_dram_part_num() routines declared by mainboards to vendorcode/google/chromeos. With that change, there are less places that need the prototype, so it makes more sense for the caller to #include the header directly.
For my (and our code base's) future benefit, would you have changed all files that required the mainboard_get_dram_part_num prototype to include spd_bin.h directly, or reduce risk of touching the other files and just replace the prototype in this header file with an include containing the prototype like I did in this case?
I think in this case, there wouldn't have been much risk of adding an extra #include line to each of those files, but depending on how long it takes to merge this change, my experience has shown that having many mainboards connected to a CL can cause a fair amount of extra rebase work along the way as other mainboards move forward, so that was a weighting factor used in my decision as well.
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 35:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/16/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/16/src/lib/spd_bin.c@202 PS16, Line 202: spd_get_name(spd, spd_name, ARRAY_SIZE(spd_name), type); : : printk(BIOS_INFO, "SPD: module part number is %s\n", spd_name);
Does this mean (this change will mean) that all mainboards going forward on chromeos will define t […]
Done
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 35:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/22/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/22/src/lib/spd_bin.c@148 PS22, Line 148: if (mainboard_get_dram_part_num(spd_name, len))
The default implementation for this function is sitting under chromeos. […]
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 35:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/22/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/romstage.h:
https://review.coreboot.org/c/coreboot/+/45459/22/src/soc/intel/tigerlake/in... PS22, Line 7: #include <spd_bin.h>
Originally, my thinking was that a caller of mainboard_get_dram_part_num() would already be includin […]
We typically subscribe to 'include what you use'. That way the compilation unit needing the declaration would include the header providing it directly.
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 35:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/22/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/romstage.h:
https://review.coreboot.org/c/coreboot/+/45459/22/src/soc/intel/tigerlake/in... PS22, Line 7: #include <spd_bin.h>
We typically subscribe to 'include what you use'. […]
Thanks, Aaron, I will follow "include what you use" rule.
Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 35:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45459/35/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/35/src/lib/spd_bin.c@154 PS35, Line 154: char const char
https://review.coreboot.org/c/coreboot/+/45459/35/src/lib/spd_bin.c@176 PS35, Line 176: uint8_t const uint8_t
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Tim Wawrzynczak, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#36).
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
lib/spd: respect spd memory part name override if present
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. In those cases, devices define their DRAM part name in the CBI.
Check for the existence of a mainboard part name override and if present, use the override name.
Add weak implementation of mainboard_get_dram_part_num() to spd_bin.c and remove all other weak implementaitons.
BUG=b:168724473 TEST=cherry-pick change to enable this on Tiger Lake, then execute "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 modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/elkhartlake/meminit.c M src/soc/intel/jasperlake/meminit.c M src/soc/intel/tigerlake/meminit.c 6 files changed, 29 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/36
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 35:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45459/35/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/35/src/lib/spd_bin.c@154 PS35, Line 154: char
const char
Done
https://review.coreboot.org/c/coreboot/+/45459/35/src/lib/spd_bin.c@176 PS35, Line 176: uint8_t
const uint8_t
Done
Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 36:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45459/36/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/36/src/lib/spd_bin.c@10 PS36, Line 10: struct ugh, i see const is propagating... could you fix this API so we don't need the awkward cast on 17?
https://review.coreboot.org/c/coreboot/+/45459/36/src/lib/spd_bin.c@193 PS36, Line 193: (const uint8_t *) isn't spd already of this type?
https://review.coreboot.org/c/coreboot/+/45459/36/src/soc/intel/cannonlake/c... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/c/coreboot/+/45459/36/src/soc/intel/cannonlake/c... PS36, Line 83: spd_data_ptr rdev_mmap_full returns a void * so i think you can eliminate eliminate the (const uint8_t *) cast by making this a void *.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 36:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/36/src/soc/intel/cannonlake/c... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/c/coreboot/+/45459/36/src/soc/intel/cannonlake/c... PS36, Line 83: spd_data_ptr
rdev_mmap_full returns a void * […]
variables of type `void *` are very awkward to have on hand... I think either a byte pointer (`const uint8_t *` or `uintptr_t` is appropriate here.
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Tim Wawrzynczak, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#37).
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
lib/spd: respect spd memory part name override if present
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. In those cases, devices define their DRAM part name in the CBI.
Check for the existence of a mainboard part name override and if present, use the override name.
Add weak implementation of mainboard_get_dram_part_num() to spd_bin.c and remove all other weak implementaitons.
BUG=b:168724473 TEST=cherry-pick change to enable this on Tiger Lake, then execute "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 modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/elkhartlake/meminit.c M src/soc/intel/jasperlake/meminit.c M src/soc/intel/tigerlake/meminit.c 6 files changed, 29 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/37
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Tim Wawrzynczak, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#38).
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
lib/spd: respect spd memory part name override if present
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. In those cases, devices define their DRAM part name in the CBI.
Check for the existence of a mainboard part name override and if present, use the override name.
Add weak implementation of mainboard_get_dram_part_num() to spd_bin.c and remove all other weak implementaitons.
BUG=b:168724473 TEST=cherry-pick change to enable this on Tiger Lake, then execute "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 modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/elkhartlake/meminit.c M src/soc/intel/jasperlake/meminit.c M src/soc/intel/tigerlake/meminit.c 6 files changed, 26 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/38
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Tim Wawrzynczak, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#40).
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
lib/spd: respect spd memory part name override if present
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. In those cases, devices define their DRAM part name in the CBI.
Check for the existence of a mainboard part name override and if present, use the override name.
Add weak implementation of mainboard_get_dram_part_num() to spd_bin.c and remove all other weak implementaitons.
BUG=b:168724473 TEST=cherry-pick change to enable this on Tiger Lake, then execute "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 modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/elkhartlake/meminit.c M src/soc/intel/jasperlake/meminit.c M src/soc/intel/tigerlake/meminit.c 5 files changed, 6 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/40
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 40:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45459/36/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/36/src/lib/spd_bin.c@10 PS36, Line 10: struct
ugh, i see const is propagating... could you fix this API […]
Yes, it was propagating, but the right thing to do.
I'm hesitant to change the blk->spd_array[] from it's current type to a constant (it's an array, so it's likely initialized at some point).
https://review.coreboot.org/c/coreboot/+/45459/36/src/lib/spd_bin.c@193 PS36, Line 193: (const uint8_t *)
isn't spd already of this type?
You're correct, I just changed the input param to add const in this patchset revision, so this cast is no longer required.
https://review.coreboot.org/c/coreboot/+/45459/36/src/soc/intel/cannonlake/c... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/c/coreboot/+/45459/36/src/soc/intel/cannonlake/c... PS36, Line 83: spd_data_ptr
variables of type `void *` are very awkward to have on hand... […]
If you feel this is an issue, a bug should be written up and this fixed in a separate CL. Resolving for now.
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 49:
This change is ready for review.
Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 49:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45459/49//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45459/49//COMMIT_MSG@7 PS49, Line 7: lib/spd: respect spd memory part name override if present this description no longer patches the diffs. the only changes here are the const casts.
https://review.coreboot.org/c/coreboot/+/45459/49/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/49/src/lib/spd_bin.c@17 PS49, Line 17: (const uint8_t *) is this cast needed?, this is a u8 * which is the same as uint8_t *
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Tim Wawrzynczak, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#50).
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
lib/spd: respect spd memory part name override if present
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. In those cases, devices define their DRAM part name in the CBI.
Check for the existence of a mainboard part name override and if present, use the override name.
BUG=b:168724473 TEST=cherry-pick change to enable this on Tiger Lake, then execute "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 modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/elkhartlake/meminit.c M src/soc/intel/jasperlake/meminit.c M src/soc/intel/tigerlake/meminit.c 6 files changed, 30 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/50
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Tim Wawrzynczak, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#51).
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
lib/spd: respect spd memory part name override if present
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. In those cases, devices define their DRAM part name in the CBI.
Check for the existence of a mainboard part name override and if present, use the override name.
BUG=b:168724473 TEST=cherry-pick change to enable this on Tiger Lake, then execute "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 modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/elkhartlake/meminit.c M src/soc/intel/jasperlake/meminit.c M src/soc/intel/tigerlake/meminit.c 6 files changed, 30 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/51
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 51:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/49//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45459/49//COMMIT_MSG@7 PS49, Line 7: lib/spd: respect spd memory part name override if present
this description no longer patches the diffs. […]
Fixed it. This patchset has been git rebase --continue resolution hell, sorry about the thrash.
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override ......................................................................
Patch Set 57:
This change is ready for review.
Nick Vaccaro has removed Caveh Jalali from this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override ......................................................................
Removed reviewer Caveh Jalali.
Nick Vaccaro has removed Tim Wawrzynczak from this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override ......................................................................
Removed reviewer Tim Wawrzynczak.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#58).
Change subject: lib/spd: respect spd memory part name override ......................................................................
lib/spd: respect spd memory part name override
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. For these cases, a mainboard can override the dram part number string, so the spd logging code needs to check for an override string when logging the dram part number.
Change print_spd_info() to use an override string if declared.
BUG=b:168724473 TEST="emerge-volteer coreboot chromeos-bootimage", flash and boot volteer2 and verify that the BIOS log shows a part name when logging SPD information:
SPD: module part number is K4U6E3S4AA-MGCL
I also modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/elkhartlake/meminit.c M src/soc/intel/jasperlake/meminit.c M src/soc/intel/tigerlake/meminit.c 6 files changed, 29 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/58
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override ......................................................................
Patch Set 58:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45459/58/src/include/spd_bin.h File src/include/spd_bin.h:
https://review.coreboot.org/c/coreboot/+/45459/58/src/include/spd_bin.h@45 PS58, Line 45: const This is a completely unrelated change and unnecessarily results in touching all the SoC files. If this change is required, it should be done in a CL of its own.
https://review.coreboot.org/c/coreboot/+/45459/58/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/58/src/lib/spd_bin.c@147 PS58, Line 147: size_t *len) nit: This will fit on the previous line within the 96-column limit.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override ......................................................................
Patch Set 58:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/58/src/include/spd_bin.h File src/include/spd_bin.h:
https://review.coreboot.org/c/coreboot/+/45459/58/src/include/spd_bin.h@45 PS58, Line 45: const
This is a completely unrelated change and unnecessarily results in touching all the SoC files. […]
That shouldn't need to change any SoC files. I agree it should be a separate change, but it just means that print_spd_info isn't allowed to modify `spd`. The type of the pointer passed to it doesn't have to be const.
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override ......................................................................
Patch Set 58:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/58/src/include/spd_bin.h File src/include/spd_bin.h:
https://review.coreboot.org/c/coreboot/+/45459/58/src/include/spd_bin.h@45 PS58, Line 45: const
That shouldn't need to change any SoC files. […]
This change was in response to a review comment, sounded like the right thing to do. If I remove the casts for the calling functions, is it ok for this change to print_spd_info to remain, or should I jettison it as well?
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#59).
Change subject: lib/spd: respect spd memory part name override ......................................................................
lib/spd: respect spd memory part name override
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. For these cases, a mainboard can override the dram part number string, so the spd logging code needs to check for an override string when logging the dram part number.
Change print_spd_info() to use an override string if declared.
BUG=b:168724473 TEST="emerge-volteer coreboot chromeos-bootimage", flash and boot volteer2 and verify that the BIOS log shows a part name when logging SPD information:
SPD: module part number is K4U6E3S4AA-MGCL
I also modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c 2 files changed, 24 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/59
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override ......................................................................
Patch Set 59:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/58/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/58/src/lib/spd_bin.c@147 PS58, Line 147: size_t *len)
nit: This will fit on the previous line within the 96-column limit.
Then we need to fix checkpatch :
Running checkpatch WARNING: line over 80 characters #11: FILE: src/lib/spd_bin.c:146: +static void spd_get_name(const uint8_t spd[], int type, const char **spd_name, size_t *len)
Should I bypass checkpatch with --no-verify? I don't generally like to do that.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#60).
Change subject: lib/spd: respect spd memory part name override ......................................................................
lib/spd: respect spd memory part name override
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. For these cases, a mainboard can override the dram part number string, so the spd logging code needs to check for an override string when logging the dram part number.
Change print_spd_info() to use an override string if declared.
BUG=b:168724473 TEST="emerge-volteer coreboot chromeos-bootimage", flash and boot volteer2 and verify that the BIOS log shows a part name when logging SPD information:
SPD: module part number is K4U6E3S4AA-MGCL
I also modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c 2 files changed, 23 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/60
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#61).
Change subject: lib/spd: respect spd memory part name override ......................................................................
lib/spd: respect spd memory part name override
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. For these cases, a mainboard can override the dram part number string, so the spd logging code needs to check for an override string when logging the dram part number.
Change print_spd_info() to use an override string if declared.
BUG=b:168724473 TEST="emerge-volteer coreboot chromeos-bootimage", flash and boot volteer2 and verify that the BIOS log shows a part name when logging SPD information:
SPD: module part number is K4U6E3S4AA-MGCL
I also modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/lib/spd_bin.c 1 file changed, 22 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/61
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#62).
Change subject: lib/spd: respect spd memory part name override ......................................................................
lib/spd: respect spd memory part name override
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. For these cases, a mainboard can override the dram part number string, so the spd logging code needs to check for an override string when logging the dram part number.
Change print_spd_info() to use an override string if declared.
BUG=b:168724473 TEST="emerge-volteer coreboot chromeos-bootimage", flash and boot volteer2 and verify that the BIOS log shows a part name when logging SPD information:
SPD: module part number is K4U6E3S4AA-MGCL
I also modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c 2 files changed, 23 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/62
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override ......................................................................
Patch Set 62:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45459/58/src/include/spd_bin.h File src/include/spd_bin.h:
https://review.coreboot.org/c/coreboot/+/45459/58/src/include/spd_bin.h@45 PS58, Line 45: const
This change was in response to a review comment, sounded like the right thing to do. […]
I would prefer keeping it in a separate CL.
https://review.coreboot.org/c/coreboot/+/45459/58/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/58/src/lib/spd_bin.c@147 PS58, Line 147: size_t *len)
Then we need to fix checkpatch : […]
Can you please submit an update to checkpatch to allow 96 characters: https://doc.coreboot.org/coding_style.html#breaking-long-lines-and-strings
https://review.coreboot.org/c/coreboot/+/45459/62/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/62/src/lib/spd_bin.c@196 PS62, Line 196: %s Looks like use of len somehow got dropped here?
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#63).
Change subject: lib/spd: respect spd memory part name override ......................................................................
lib/spd: respect spd memory part name override
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. For these cases, a mainboard can override the dram part number string, so the spd logging code needs to check for an override string when logging the dram part number.
Change print_spd_info() to use an override string if declared.
BUG=b:168724473 TEST="emerge-volteer coreboot chromeos-bootimage", flash and boot volteer2 and verify that the BIOS log shows a part name when logging SPD information:
SPD: module part number is K4U6E3S4AA-MGCL
I also modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/lib/spd_bin.c 1 file changed, 14 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/63
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#64).
Change subject: lib/spd: respect spd memory part name override ......................................................................
lib/spd: respect spd memory part name override
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. For these cases, a mainboard can override the dram part number string, so the spd logging code needs to check for an override string when logging the dram part number.
Change print_spd_info() to use an override string if declared.
BUG=b:168724473 TEST="emerge-volteer coreboot chromeos-bootimage", flash and boot volteer2 and verify that the BIOS log shows a part name when logging SPD information:
SPD: module part number is K4U6E3S4AA-MGCL
I also modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/lib/spd_bin.c 1 file changed, 23 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/64
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override ......................................................................
Patch Set 64:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45459/58/src/include/spd_bin.h File src/include/spd_bin.h:
https://review.coreboot.org/c/coreboot/+/45459/58/src/include/spd_bin.h@45 PS58, Line 45: const
I would prefer keeping it in a separate CL.
Done
https://review.coreboot.org/c/coreboot/+/45459/58/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/58/src/lib/spd_bin.c@147 PS58, Line 147: size_t *len)
Can you please submit an update to checkpatch to allow 96 characters: https://doc.coreboot. […]
https://buganizer.corp.google.com/issues/170217700
https://review.coreboot.org/c/coreboot/+/45459/62/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/62/src/lib/spd_bin.c@196 PS62, Line 196: %s
Looks like use of len somehow got dropped here?
Done
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#65).
Change subject: lib/spd: respect spd memory part name override ......................................................................
lib/spd: respect spd memory part name override
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. For these cases, a mainboard can override the dram part number string, so the spd logging code needs to check for an override string when logging the dram part number.
Change print_spd_info() to use an override string if declared.
BUG=b:168724473 TEST="emerge-volteer coreboot chromeos-bootimage", flash and boot volteer2 and verify that the BIOS log shows a part name when logging SPD information:
SPD: module part number is K4U6E3S4AA-MGCL
I also modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/lib/spd_bin.c 1 file changed, 22 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/65
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override ......................................................................
Patch Set 65:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45459/49/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/49/src/lib/spd_bin.c@17 PS49, Line 17: (const uint8_t *)
is this cast needed?, this is a u8 * which is the same as uint8_t *
Done
https://review.coreboot.org/c/coreboot/+/45459/58/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/58/src/lib/spd_bin.c@147 PS58, Line 147: size_t *len)
Done.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override ......................................................................
Patch Set 65: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/65/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/65/src/lib/spd_bin.c@197 PS65, Line 197: (int) len, nameptr); nit: fits on the previous line?
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45459
to look at the new patch set (#66).
Change subject: lib/spd: respect spd memory part name override ......................................................................
lib/spd: respect spd memory part name override
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. For these cases, a mainboard can override the dram part number string, so the spd logging code needs to check for an override string when logging the dram part number.
Change print_spd_info() to use an override string if declared.
BUG=b:168724473 TEST="emerge-volteer coreboot chromeos-bootimage", flash and boot volteer2 and verify that the BIOS log shows a part name when logging SPD information:
SPD: module part number is K4U6E3S4AA-MGCL
I also modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/lib/spd_bin.c 1 file changed, 21 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45459/66
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override ......................................................................
Patch Set 66:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/65/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/65/src/lib/spd_bin.c@197 PS65, Line 197: (int) len, nameptr);
nit: fits on the previous line?
Yes, it does :)
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override ......................................................................
Patch Set 66: Code-Review+2
Nick Vaccaro has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override ......................................................................
lib/spd: respect spd memory part name override
The BIOS log was looking in the spd data for the part name, but part names are stripped from generic SPDs. For these cases, a mainboard can override the dram part number string, so the spd logging code needs to check for an override string when logging the dram part number.
Change print_spd_info() to use an override string if declared.
BUG=b:168724473 TEST="emerge-volteer coreboot chromeos-bootimage", flash and boot volteer2 and verify that the BIOS log shows a part name when logging SPD information:
SPD: module part number is K4U6E3S4AA-MGCL
I also modified volteer to not override the part name and verified that this change did as expected and printed a blank string.
Change-Id: I91971e07c450492dbb0588abd1c3c692ee0d3bb0 Signed-off-by: Nick Vaccaro nvaccaro@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/45459 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/lib/spd_bin.c 1 file changed, 21 insertions(+), 14 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/lib/spd_bin.c b/src/lib/spd_bin.c index 4125973..38edc5f 100644 --- a/src/lib/spd_bin.c +++ b/src/lib/spd_bin.c @@ -143,34 +143,41 @@ return spd_busw[index]; }
-static void spd_get_name(const uint8_t spd[], char spd_name[], int dram_type) +static void spd_get_name(const uint8_t spd[], int type, const char **spd_name, size_t *len) { - switch (dram_type) { + *spd_name = mainboard_get_dram_part_num(); + if (*spd_name != NULL) { + *len = strlen(*spd_name); + return; + } + + switch (type) { case SPD_DRAM_DDR3: - memcpy(spd_name, &spd[DDR3_SPD_PART_OFF], DDR3_SPD_PART_LEN); - spd_name[DDR3_SPD_PART_LEN] = 0; + *spd_name = (const char *) &spd[DDR3_SPD_PART_OFF]; + *len = DDR3_SPD_PART_LEN; break; case SPD_DRAM_LPDDR3_INTEL: - memcpy(spd_name, &spd[LPDDR3_SPD_PART_OFF], - LPDDR3_SPD_PART_LEN); - spd_name[LPDDR3_SPD_PART_LEN] = 0; + *spd_name = (const char *) &spd[LPDDR3_SPD_PART_OFF]; + *len = LPDDR3_SPD_PART_LEN; break; - /* LPDDR3, LPDDR4 and DDR4 have the same part number offset */ + /* LPDDR3, LPDDR4 and DDR4 have same part number offset and length */ case SPD_DRAM_LPDDR3_JEDEC: case SPD_DRAM_DDR4: case SPD_DRAM_LPDDR4: case SPD_DRAM_LPDDR4X: - memcpy(spd_name, &spd[DDR4_SPD_PART_OFF], DDR4_SPD_PART_LEN); - spd_name[DDR4_SPD_PART_LEN] = 0; + *spd_name = (const char *) &spd[DDR4_SPD_PART_OFF]; + *len = DDR4_SPD_PART_LEN; break; default: + *len = 0; break; } }
void print_spd_info(uint8_t spd[]) { - char spd_name[DDR4_SPD_PART_LEN + 1] = { 0 }; + const char *nameptr = NULL; + size_t len; int type = spd[SPD_DRAM_TYPE]; int banks = spd_get_banks(spd, type); int capmb = spd_get_capmb(spd); @@ -184,9 +191,9 @@ printk(BIOS_INFO, "SPD: module type is %s\n", spd_get_module_type_string(type)); /* Module Part Number */ - spd_get_name(spd, spd_name, type); - - printk(BIOS_INFO, "SPD: module part number is %s\n", spd_name); + spd_get_name(spd, type, &nameptr, &len); + if (nameptr) + printk(BIOS_INFO, "SPD: module part number is %.*s\n", (int) len, nameptr);
printk(BIOS_INFO, "SPD: banks %d, ranks %d, rows %d, columns %d, density %d Mb\n",
-
Aaron Durbin (Code Review) -
Caveh Jalali (Code Review)
-
Furquan Shaikh (Code Review)
-
Nick Vaccaro (Code Review)
-
Tim Wawrzynczak (Code Review)