Change in coreboot[master]: soc/intel/tigerlake: Allow mainboard to override DRAM part number
Hello Marco Chen,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/39860
to review the following change.
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
soc/intel/tigerlake: Allow mainboard to override DRAM part number
In order to support mainboards that do not store DRAM part number in the traditional way i.e. within the CBFS SPD for soldered memory, this change provides a runtime callback to allow mainboards to provide DRAM part number from a custom location e.g. external EEPROM on volteer / dedede.
For other boards it should be a NOP since the weak implementation of mainboard_get_dram_part_num does nothing.
BUG=b:152019429
Change-Id: If940a76d36a7645a7441ba418aa7aec9af9f6319 Signed-off-by: Marco Chen marcochen@google.com --- M src/soc/intel/tigerlake/include/soc/romstage.h M src/soc/intel/tigerlake/romstage/romstage.c 2 files changed, 17 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/39860/1
diff --git a/src/soc/intel/tigerlake/include/soc/romstage.h b/src/soc/intel/tigerlake/include/soc/romstage.h index 1672e8b..9c8758a 100644 --- a/src/soc/intel/tigerlake/include/soc/romstage.h +++ b/src/soc/intel/tigerlake/include/soc/romstage.h @@ -17,6 +17,8 @@
#include <fsp/api.h>
+/* Provide a callback to allow mainboard to override the DRAM part number. */ +void mainboard_get_dram_part_num(const char **part_num, size_t *len); void mainboard_memory_init_params(FSPM_UPD *mupd); void systemagent_early_init(void); void pch_init(void); diff --git a/src/soc/intel/tigerlake/romstage/romstage.c b/src/soc/intel/tigerlake/romstage/romstage.c index f78ea29..1ad6847 100644 --- a/src/soc/intel/tigerlake/romstage/romstage.c +++ b/src/soc/intel/tigerlake/romstage/romstage.c @@ -33,6 +33,11 @@ 0x8d, 0x09, 0x11, 0xcf, 0x8b, 0x9f, 0x03, 0x23 \ }
+void __weak mainboard_get_dram_part_num(const char **part_num, size_t *len) +{ + /* Default weak implementation, no need to override part number. */ +} + /* Save the DIMM information for SMBIOS table 17 */ static void save_dimm_info(void) { @@ -47,6 +52,8 @@ const uint8_t smbios_memory_info_guid[16] = FSP_SMBIOS_MEMORY_INFO_GUID; const uint8_t *serial_num; + const char *dram_part_num; + size_t dram_part_num_len;
/* Locate the memory info HOB, presence validated by raminit */ meminfo_hob = fsp_find_extension_hob_by_guid( @@ -86,6 +93,14 @@ if (src_dimm->Status != DIMM_PRESENT) continue;
+ dram_part_num_len = sizeof(src_dimm->ModulePartNum); + dram_part_num = (const char *) + &src_dimm->ModulePartNum[0]; + + /* Allow mainboard to override DRAM part number. */ + mainboard_get_dram_part_num(&dram_part_num, + &dram_part_num_len); + u8 memProfNum = meminfo_hob->MemoryProfile; serial_num = src_dimm->SpdSave + SPD_SAVE_OFFSET_SERIAL;
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 1:
This CL basically mimics from https://review.coreboot.org/c/coreboot/+/31850 for the same purpose on tigerlake and jasperlake.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39860/1/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/1/src/soc/intel/tigerlake/rom... PS1, Line 101: mainboard_get_dram_part_num(&dram_part_num, : &dram_part_num_len); Do we really need to do this in a loop?
Can we read dram part number from mainboard before the loop and then check if it is not-NULL to decide which one to use here.
Hello build bot (Jenkins), Furquan Shaikh, Marco Chen, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39860
to look at the new patch set (#2).
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
soc/intel/tigerlake: Allow mainboard to override DRAM part number
In order to support mainboards that do not store DRAM part number in the traditional way i.e. within the CBFS SPD for soldered memory, this change provides a runtime callback to allow mainboards to provide DRAM part number from a custom location e.g. external EEPROM on volteer / dedede.
For other boards it should be a NOP since the weak implementation of mainboard_get_dram_part_num does nothing.
BUG=b:152019429
Change-Id: If940a76d36a7645a7441ba418aa7aec9af9f6319 Signed-off-by: Marco Chen marcochen@google.com --- M src/soc/intel/tigerlake/include/soc/romstage.h M src/soc/intel/tigerlake/romstage/romstage.c 2 files changed, 23 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/39860/2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 2:
(1 comment)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/39860/1/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/1/src/soc/intel/tigerlake/rom... PS1, Line 101: mainboard_get_dram_part_num(&dram_part_num, : &dram_part_num_len);
Do we really need to do this in a loop? […]
Careful. We had some weird cases where there were 2 DRAM types stuffed in the same PCBA because of material sourcing issues. Admittedly, we didn't handle it well then nor are we set up to do it today because it's a single entry in cbi. However, it's not out of the norm.
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39860/1/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/1/src/soc/intel/tigerlake/rom... PS1, Line 101: mainboard_get_dram_part_num(&dram_part_num, : &dram_part_num_len);
Careful. […]
Yes, that is what I would like to mention here. Currently we assume all packages are from the same vendor so we only get one string from mainboard instead of basing on index. But the implementation now at least follows what we support so far.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39860/1/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/1/src/soc/intel/tigerlake/rom... PS1, Line 101: mainboard_get_dram_part_num(&dram_part_num, : &dram_part_num_len);
Careful. […]
Aah I do remember that now.
Humm.. that would definitely require a better way to deal with this -- something like allowing mainboard to understand what channel this is and provide part number accordingly.
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39860/1/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/1/src/soc/intel/tigerlake/rom... PS1, Line 101: mainboard_get_dram_part_num(&dram_part_num, : &dram_part_num_len);
Yes, that is what I would like to mention here. […]
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39860/1/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/1/src/soc/intel/tigerlake/rom... PS1, Line 101: mainboard_get_dram_part_num(&dram_part_num, : &dram_part_num_len);
Aah I do remember that now. […]
If we do want to optimize things and handle that possibility we should probably suck in all the cbi raw into coreboot and operate on it locally instead of calling out to the EC for everything. Not for this patch, obviously.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39860/1/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/1/src/soc/intel/tigerlake/rom... PS1, Line 101: mainboard_get_dram_part_num(&dram_part_num, : &dram_part_num_len);
If we do want to optimize things and handle that possibility we should probably suck in all the cbi […]
Sorry, I was just addressing the performance aspect. CBI provisioning for this field needs a channel dimension that we'd have to work on also. :)
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39860/1/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/1/src/soc/intel/tigerlake/rom... PS1, Line 101: mainboard_get_dram_part_num(&dram_part_num, : &dram_part_num_len);
Done
Will we want to address this issue here? I will suggest to have a separate issue to improve this situation and land basic support here.
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39860/1/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/1/src/soc/intel/tigerlake/rom... PS1, Line 101: mainboard_get_dram_part_num(&dram_part_num, : &dram_part_num_len);
Will we want to address this issue here? I will suggest to have a separate issue to improve this sit […]
On the other hand, in case of strapping pin per DRAM part number our current HW design is to reserve one set of strapping pins only. So in this case, HW design needs to reserve 4 set of strapping pins actually.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39860/1/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/1/src/soc/intel/tigerlake/rom... PS1, Line 101: mainboard_get_dram_part_num(&dram_part_num, : &dram_part_num_len);
On the other hand, in case of strapping pin per DRAM part number our current HW design is to reserve […]
Marco, great point. We'd need more granularity in all parts of the stack to support such a scenario.
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 3:
Regarding the support of multiple DRAM parts in the board, I created b/152581499 for tracking this discussion. And should we proceed the basic support here for a single DRAM part in the board? Thanks.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39860/3/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/3/src/soc/intel/tigerlake/rom... PS3, Line 81: &dram_part_num_len); I believe this can fit on the previous line?
https://review.coreboot.org/c/coreboot/+/39860/3/src/soc/intel/tigerlake/rom... PS3, Line 104: dram_part_num == NULL As I look at this closely, one side-effect of this change would be: If mainboard does not provide an override, then dram_part_num will be set to src_dimm->ModulePartNum of dimm0 and that will be used for all DIMMs going forward. For memory down, I don't think it would be a problem, but for SODIMMs, I believe this could potentially be a problem if someone decides to stuff in different parts.
What do you think about having a flag: bool dram_part_num_override;
... mainboard_get_dram_part_num(&dram_part_num, &dram_part_num_len); dram_part_num_override = (dram_part_num != NULL);
And then here you can check if dram_part_num_override if false to set dram_part_num and dram_part_num_len.
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39860/3/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/3/src/soc/intel/tigerlake/rom... PS3, Line 104: dram_part_num == NULL
will be set to src_dimm->ModulePartNum of dimm0
I didn't catch the point because the for-loop here already makes src_dimm to unique specific dimm entry instead of always 0.
And if you check the original codes then dimm_info_fill also pass the src_dimm->ModulePartNum directly.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39860/3/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/3/src/soc/intel/tigerlake/rom... PS3, Line 104: dram_part_num == NULL
will be set to src_dimm->ModulePartNum of dimm0 […]
The way the code is written now, it will lead to the following:
Consider mainboard does not provide override for part number. L80: dram_part_num will remain NULL
For channel0 dimm0, dram_part_num is NULL. L104-107: src_dimm->ModulePartNum for &channel_info->DimmInfo[dimm]; i.e. channel0 dimm0 will be used to set dram_part_num. Thus, dram_part_num is no longer NULL.
For all other dimms, dram_part_num is not NULL now. L104-107: These lines will not take effect and so dram_part_num will still be pointing to src_dimm->ModulePartNum for Channel0 dimm0.
Hello build bot (Jenkins), Furquan Shaikh, Marco Chen, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39860
to look at the new patch set (#4).
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
soc/intel/tigerlake: Allow mainboard to override DRAM part number
In order to support mainboards that do not store DRAM part number in the traditional way i.e. within the CBFS SPD for soldered memory, this change provides a runtime callback to allow mainboards to provide DRAM part number from a custom location e.g. external EEPROM on volteer / dedede.
For other boards it should be a NOP since the weak implementation of mainboard_get_dram_part_num does nothing.
BUG=b:152019429
Change-Id: If940a76d36a7645a7441ba418aa7aec9af9f6319 Signed-off-by: Marco Chen marcochen@google.com --- M src/soc/intel/tigerlake/include/soc/romstage.h M src/soc/intel/tigerlake/romstage/romstage.c 2 files changed, 27 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/39860/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39860/4/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/4/src/soc/intel/tigerlake/rom... PS4, Line 58: bool is_dram_part_overriden = false; 'overriden' may be misspelled - perhaps 'overridden'?
https://review.coreboot.org/c/coreboot/+/39860/4/src/soc/intel/tigerlake/rom... PS4, Line 82: is_dram_part_overriden = mainboard_get_dram_part_num(&dram_part_num, 'overriden' may be misspelled - perhaps 'overridden'?
https://review.coreboot.org/c/coreboot/+/39860/4/src/soc/intel/tigerlake/rom... PS4, Line 106: if (!is_dram_part_overriden) { 'overriden' may be misspelled - perhaps 'overridden'?
Hello build bot (Jenkins), Furquan Shaikh, Marco Chen, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39860
to look at the new patch set (#5).
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
soc/intel/tigerlake: Allow mainboard to override DRAM part number
In order to support mainboards that do not store DRAM part number in the traditional way i.e. within the CBFS SPD for soldered memory, this change provides a runtime callback to allow mainboards to provide DRAM part number from a custom location e.g. external EEPROM on volteer / dedede.
For other boards it should be a NOP since the weak implementation of mainboard_get_dram_part_num does nothing.
BUG=b:152019429
Change-Id: If940a76d36a7645a7441ba418aa7aec9af9f6319 Signed-off-by: Marco Chen marcochen@google.com --- M src/soc/intel/tigerlake/include/soc/romstage.h M src/soc/intel/tigerlake/romstage/romstage.c 2 files changed, 27 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/39860/5
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39860/3/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/3/src/soc/intel/tigerlake/rom... PS3, Line 104: dram_part_num == NULL
The way the code is written now, it will lead to the following: […]
You are correct actually and I should not change the judgement itself. Thanks.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/39860/5/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/5/src/soc/intel/tigerlake/rom... PS5, Line 85: nit: extra blank line not required.
Hello build bot (Jenkins), Furquan Shaikh, Marco Chen, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39860
to look at the new patch set (#6).
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
soc/intel/tigerlake: Allow mainboard to override DRAM part number
In order to support mainboards that do not store DRAM part number in the traditional way i.e. within the CBFS SPD for soldered memory, this change provides a runtime callback to allow mainboards to provide DRAM part number from a custom location e.g. external EEPROM on volteer / dedede.
For other boards it should be a NOP since the weak implementation of mainboard_get_dram_part_num does nothing.
BUG=b:152019429
Change-Id: If940a76d36a7645a7441ba418aa7aec9af9f6319 Signed-off-by: Marco Chen marcochen@google.com --- M src/soc/intel/tigerlake/include/soc/romstage.h M src/soc/intel/tigerlake/romstage/romstage.c 2 files changed, 26 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/39860/6
Hello build bot (Jenkins), Furquan Shaikh, Marco Chen, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39860
to look at the new patch set (#7).
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
soc/intel/tigerlake: Allow mainboard to override DRAM part number
In order to support mainboards that do not store DRAM part number in the traditional way i.e. within the CBFS SPD for soldered memory, this change provides a runtime callback to allow mainboards to provide DRAM part number from a custom location e.g. external EEPROM on volteer / dedede.
For other boards it should be a NOP since the weak implementation of mainboard_get_dram_part_num does nothing.
BUG=b:152019429
Change-Id: If940a76d36a7645a7441ba418aa7aec9af9f6319 Signed-off-by: Marco Chen marcochen@google.com --- M src/soc/intel/tigerlake/include/soc/romstage.h M src/soc/intel/tigerlake/romstage/romstage.c 2 files changed, 25 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/39860/7
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39860/5/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/5/src/soc/intel/tigerlake/rom... PS5, Line 85:
nit: extra blank line not required.
done and remove additional one at line 80 as well.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39860/7/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/7/src/soc/intel/tigerlake/rom... PS7, Line 39: return false; Print out a debug message, that the weak implementation was used?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 7: Code-Review+2
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39860/7/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/7/src/soc/intel/tigerlake/rom... PS7, Line 39: return false;
Print out a debug message, that the weak implementation was used?
This function is not the one designed for every board to implement so it might be not needed to print the warning as I understand.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 7: Code-Review+2
Need to apply the same to JSL.
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 7:
Patch Set 7: Code-Review+2
Need to apply the same to JSL.
We can discuss it here - b/152019429#comment6
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39860/3/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/3/src/soc/intel/tigerlake/rom... PS3, Line 104: dram_part_num == NULL
You are correct actually and I should not change the judgement itself. Thanks.
Done
https://review.coreboot.org/c/coreboot/+/39860/7/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/7/src/soc/intel/tigerlake/rom... PS7, Line 39: return false;
This function is not the one designed for every board to implement so it might be not needed to prin […]
Done
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 7:
ping for the merging? Thanks.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39860/3/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/3/src/soc/intel/tigerlake/rom... PS3, Line 81: &dram_part_num_len);
I believe this can fit on the previous line?
Done
https://review.coreboot.org/c/coreboot/+/39860/5/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/39860/5/src/soc/intel/tigerlake/rom... PS5, Line 85:
done and remove additional one at line 80 as well.
Done
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
soc/intel/tigerlake: Allow mainboard to override DRAM part number
In order to support mainboards that do not store DRAM part number in the traditional way i.e. within the CBFS SPD for soldered memory, this change provides a runtime callback to allow mainboards to provide DRAM part number from a custom location e.g. external EEPROM on volteer / dedede.
For other boards it should be a NOP since the weak implementation of mainboard_get_dram_part_num does nothing.
BUG=b:152019429
Change-Id: If940a76d36a7645a7441ba418aa7aec9af9f6319 Signed-off-by: Marco Chen marcochen@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/39860 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Karthik Ramasubramanian kramasub@google.com --- M src/soc/intel/tigerlake/include/soc/romstage.h M src/soc/intel/tigerlake/romstage/romstage.c 2 files changed, 25 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Karthik Ramasubramanian: Looks good to me, approved
diff --git a/src/soc/intel/tigerlake/include/soc/romstage.h b/src/soc/intel/tigerlake/include/soc/romstage.h index 4a4fbe6..e3c7969 100644 --- a/src/soc/intel/tigerlake/include/soc/romstage.h +++ b/src/soc/intel/tigerlake/include/soc/romstage.h @@ -6,6 +6,8 @@
#include <fsp/api.h>
+/* Provide a callback to allow mainboard to override the DRAM part number. */ +bool mainboard_get_dram_part_num(const char **part_num, size_t *len); void mainboard_memory_init_params(FSPM_UPD *mupd); void systemagent_early_init(void); void pch_init(void); diff --git a/src/soc/intel/tigerlake/romstage/romstage.c b/src/soc/intel/tigerlake/romstage/romstage.c index fa9db6e..dc5dcf1 100644 --- a/src/soc/intel/tigerlake/romstage/romstage.c +++ b/src/soc/intel/tigerlake/romstage/romstage.c @@ -22,6 +22,12 @@ 0x8d, 0x09, 0x11, 0xcf, 0x8b, 0x9f, 0x03, 0x23 \ }
+bool __weak mainboard_get_dram_part_num(const char **part_num, size_t *len) +{ + /* Default weak implementation, no need to override part number. */ + return false; +} + /* Save the DIMM information for SMBIOS table 17 */ static void save_dimm_info(void) { @@ -36,6 +42,9 @@ const uint8_t smbios_memory_info_guid[16] = FSP_SMBIOS_MEMORY_INFO_GUID; const uint8_t *serial_num; + const char *dram_part_num = NULL; + size_t dram_part_num_len; + bool is_dram_part_overridden = false;
/* Locate the memory info HOB, presence validated by raminit */ meminfo_hob = fsp_find_extension_hob_by_guid( @@ -57,6 +66,10 @@ } memset(mem_info, 0, sizeof(*mem_info));
+ /* Allow mainboard to override DRAM part number. */ + is_dram_part_overridden = mainboard_get_dram_part_num(&dram_part_num, + &dram_part_num_len); + /* Save available DIMM information */ index = 0; dimm_max = ARRAY_SIZE(mem_info->dimm); @@ -75,6 +88,14 @@ if (src_dimm->Status != DIMM_PRESENT) continue;
+ /* If there is no DRAM part number overridden by + * mainboard then use original one. */ + if (!is_dram_part_overridden) { + dram_part_num_len = sizeof(src_dimm->ModulePartNum); + dram_part_num = (const char *) + &src_dimm->ModulePartNum[0]; + } + u8 memProfNum = meminfo_hob->MemoryProfile; serial_num = src_dimm->SpdSave + SPD_SAVE_OFFSET_SERIAL; @@ -87,8 +108,8 @@ src_dimm->RankInDimm, channel_info->ChannelId, src_dimm->DimmId, - (const char *)src_dimm->ModulePartNum, - sizeof(src_dimm->ModulePartNum), + dram_part_num, + dram_part_num_len, serial_num, meminfo_hob->DataWidth, meminfo_hob->VddVoltage[memProfNum],
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39860 )
Change subject: soc/intel/tigerlake: Allow mainboard to override DRAM part number ......................................................................
Patch Set 8:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/2147 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2146 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2145
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Aaron Durbin (Code Review)
-
build bot (Jenkins) (Code Review)
-
Furquan Shaikh (Code Review)
-
Karthik Ramasubramanian (Code Review)
-
Marco Chen (Code Review)
-
Paul Menzel (Code Review)