Francois Toguo Fotso has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31235
Change subject: Fix for missing mem_rank info in SMBIOS ......................................................................
Fix for missing mem_rank info in SMBIOS
Found-by: Google BUG=None TEST=Boot to OS:
Change-Id: I212215040e4786c258a9c604cc5c2bb62867c842 Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- M 3rdparty/blobs M src/soc/intel/apollolake/meminit_util_apl.c M src/soc/intel/apollolake/meminit_util_glk.c M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/common/smbios.c M src/soc/intel/common/smbios.h M src/soc/intel/icelake/romstage/romstage.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 8 files changed, 9 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/31235/1
diff --git a/3rdparty/blobs b/3rdparty/blobs index 16058e5..678b4c4 160000 --- a/3rdparty/blobs +++ b/3rdparty/blobs @@ -1 +1 @@ -Subproject commit 16058e552279b4884b1f671e7a78752d28abd1cc +Subproject commit 678b4c4a81069bb6e10e2e59f5374b83d727cd2b diff --git a/src/soc/intel/apollolake/meminit_util_apl.c b/src/soc/intel/apollolake/meminit_util_apl.c index 755d708..0112789 100644 --- a/src/soc/intel/apollolake/meminit_util_apl.c +++ b/src/soc/intel/apollolake/meminit_util_apl.c @@ -85,6 +85,7 @@ src_dimm->SizeInMb, memory_info_hob->MemoryType, memory_info_hob->MemoryFrequencyInMHz, + src_dimm->RankInDimm, channel_info->ChannelId, src_dimm->DimmId, dram_part_num, diff --git a/src/soc/intel/apollolake/meminit_util_glk.c b/src/soc/intel/apollolake/meminit_util_glk.c index db69f51..9bfdf0b 100644 --- a/src/soc/intel/apollolake/meminit_util_glk.c +++ b/src/soc/intel/apollolake/meminit_util_glk.c @@ -91,6 +91,7 @@ src_dimm->DimmCapacity, memory_info_hob->MemoryType, memory_info_hob->ConfiguredMemoryClockSpeed, + src_dimm->RankInDimm, channel_info->ChannelId, src_dimm->DimmId, dram_part_num, diff --git a/src/soc/intel/cannonlake/romstage/romstage.c b/src/soc/intel/cannonlake/romstage/romstage.c index 9224ba6..24f1f12 100644 --- a/src/soc/intel/cannonlake/romstage/romstage.c +++ b/src/soc/intel/cannonlake/romstage/romstage.c @@ -93,6 +93,7 @@ src_dimm->DimmCapacity, memory_info_hob->MemoryType, memory_info_hob->ConfiguredMemoryClockSpeed, + src_dimm->RankInDimm, channel_info->ChannelId, src_dimm->DimmId, (const char *)src_dimm->ModulePartNum, diff --git a/src/soc/intel/common/smbios.c b/src/soc/intel/common/smbios.c index bcddb78..d714a34 100644 --- a/src/soc/intel/common/smbios.c +++ b/src/soc/intel/common/smbios.c @@ -20,13 +20,14 @@
/* Fill the SMBIOS memory information from FSP MEM_INFO_DATA_HOB in CBMEM.*/ void dimm_info_fill(struct dimm_info *dimm, u32 dimm_capacity, u8 ddr_type, - u32 frequency, u8 channel_id, u8 dimm_id, + u32 frequency, u8 rankInDimm, u8 channel_id, u8 dimm_id, const char *module_part_num, size_t module_part_number_size, u16 data_width) { dimm->dimm_size = dimm_capacity; dimm->ddr_type = ddr_type; dimm->ddr_frequency = frequency; + dimm->rank_per_dimm = rankInDimm; dimm->channel_num = channel_id; dimm->dimm_num = dimm_id; strncpy((char *)dimm->module_part_number, diff --git a/src/soc/intel/common/smbios.h b/src/soc/intel/common/smbios.h index 4750d3c..022271d 100644 --- a/src/soc/intel/common/smbios.h +++ b/src/soc/intel/common/smbios.h @@ -21,7 +21,7 @@
/* Fill the SMBIOS memory information from FSP MEM_INFO_DATA_HOB in CBMEM.*/ void dimm_info_fill(struct dimm_info *dimm, u32 dimm_capacity, u8 ddr_type, - u32 frequency, u8 channel_id, u8 dimm_id, + u32 frequency, u8 rank_per_dim, u8 channel_id, u8 dimm_id, const char *module_part_num, size_t module_part_number_size, u16 data_width);
diff --git a/src/soc/intel/icelake/romstage/romstage.c b/src/soc/intel/icelake/romstage/romstage.c index 432cae5..3951124 100644 --- a/src/soc/intel/icelake/romstage/romstage.c +++ b/src/soc/intel/icelake/romstage/romstage.c @@ -95,6 +95,7 @@ src_dimm->DimmCapacity, memory_info_hob->MemoryType, memory_info_hob->ConfiguredMemoryClockSpeed, + src_dimm->RankInDimm, channel_info->ChannelId, src_dimm->DimmId, (const char *)src_dimm->ModulePartNum, diff --git a/src/soc/intel/skylake/romstage/romstage_fsp20.c b/src/soc/intel/skylake/romstage/romstage_fsp20.c index 2a60158..7a3509f 100644 --- a/src/soc/intel/skylake/romstage/romstage_fsp20.c +++ b/src/soc/intel/skylake/romstage/romstage_fsp20.c @@ -121,6 +121,7 @@ src_dimm->DimmCapacity, ddr_type, memory_info_hob->ConfiguredMemoryClockSpeed, + src_dimm->RankInDim, channel_info->ChannelId, src_dimm->DimmId, (const char *)src_dimm->ModulePartNum,
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: Fix for missing mem_rank info in SMBIOS ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/#/c/31235/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31235/1//COMMIT_MSG@10 PS1, Line 10: None b:122329046
https://review.coreboot.org/#/c/31235/1//COMMIT_MSG@11 PS1, Line 11: Boot to OS: Is rank reported correctly in SMBIOS?
https://review.coreboot.org/#/c/31235/1//COMMIT_MSG@12 PS1, Line 12: BRANCH=octopus
https://review.coreboot.org/#/c/31235/1/3rdparty/blobs File 3rdparty/blobs:
PS1: This should not be added to the CL.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: Fix for missing mem_rank info in SMBIOS ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/31235/1/src/soc/intel/common/smbios.h File src/soc/intel/common/smbios.h:
https://review.coreboot.org/#/c/31235/1/src/soc/intel/common/smbios.h@24 PS1, Line 24: u32 frequency, u8 rank_per_dim, u8 channel_id, u8 dimm_id, prototype doesn't match implementation
https://review.coreboot.org/#/c/31235/1/src/soc/intel/common/smbios.c File src/soc/intel/common/smbios.c:
https://review.coreboot.org/#/c/31235/1/src/soc/intel/common/smbios.c@23 PS1, Line 23: u32 frequency, u8 rankInDimm, u8 channel_id, u8 dimm_id, please don't use camel case
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: Fix for missing mem_rank info in SMBIOS ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31235/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31235/1//COMMIT_MSG@7 PS1, Line 7: Fix for missing mem_rank info in SMBIOS Please add a prefix (cf. `git log --oneline`), remove one of the two spacse before `mem_rank`, and elaborate what the problem is in the commit message.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: Fix for missing mem_rank info in SMBIOS ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31235/1/src/soc/intel/skylake/romstage/romst... File src/soc/intel/skylake/romstage/romstage_fsp20.c:
https://review.coreboot.org/#/c/31235/1/src/soc/intel/skylake/romstage/romst... PS1, Line 124: m Is this the right field name?
Hello Aaron Durbin, Patrick Rudolph, Karthik Ramasubramanian, Justin TerAvest, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31235
to look at the new patch set (#2).
Change subject: soc/intel: Fix for missing mem_rank info in SMBIOS ......................................................................
soc/intel: Fix for missing mem_rank info in SMBIOS
Found-by: Google BUG=Wrong memory rank info in MOSYS TEST=Boot to OS:
Change-Id: I212215040e4786c258a9c604cc5c2bb62867c842 Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- M 3rdparty/blobs M src/soc/intel/apollolake/meminit_util_apl.c M src/soc/intel/apollolake/meminit_util_glk.c M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/common/smbios.c M src/soc/intel/common/smbios.h M src/soc/intel/icelake/romstage/romstage.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 8 files changed, 9 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/31235/2
Hello Aaron Durbin, Patrick Rudolph, Karthik Ramasubramanian, Justin TerAvest, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31235
to look at the new patch set (#3).
Change subject: soc/intel: Fix for missing mem_rank info in SMBIOS ......................................................................
soc/intel: Fix for missing mem_rank info in SMBIOS
Found-by: Google BUG=Wrong memory rank info in SMBIOS TEST=Boot to OS:
Change-Id: I212215040e4786c258a9c604cc5c2bb62867c842 Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- M 3rdparty/blobs M src/soc/intel/apollolake/meminit_util_apl.c M src/soc/intel/apollolake/meminit_util_glk.c M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/common/smbios.c M src/soc/intel/common/smbios.h M src/soc/intel/icelake/romstage/romstage.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 8 files changed, 9 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/31235/3
Hello Aaron Durbin, Patrick Rudolph, Karthik Ramasubramanian, Justin TerAvest, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31235
to look at the new patch set (#4).
Change subject: soc/intel: Fix for missing mem_rank info in SMBIOS ......................................................................
soc/intel: Fix for missing mem_rank info in SMBIOS
Found-by: Google BUG=Wrong memory rank info in SMBIOS TEST=Boot to OS BRANCH=octopus
Change-Id: I212215040e4786c258a9c604cc5c2bb62867c842 Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- M 3rdparty/blobs M src/soc/intel/apollolake/meminit_util_apl.c M src/soc/intel/apollolake/meminit_util_glk.c M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/common/smbios.c M src/soc/intel/common/smbios.h M src/soc/intel/icelake/romstage/romstage.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 8 files changed, 9 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/31235/4
Francois Toguo Fotso has uploaded a new patch set (#5) to the change originally created by Francois Toguo Fotso. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: soc/intel: Fix for missing mem_rank info in SMBIOS ......................................................................
soc/intel: Fix for missing mem_rank info in SMBIOS
Found-by: Google BUG=Wrong memory rank info in SMBIOS TEST=Boot to OS BRANCH=octopus
Change-Id: I212215040e4786c258a9c604cc5c2bb62867c842 Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- D 3rdparty/blobs M src/soc/intel/apollolake/meminit_util_apl.c M src/soc/intel/apollolake/meminit_util_glk.c M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/common/smbios.c M src/soc/intel/common/smbios.h M src/soc/intel/icelake/romstage/romstage.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 8 files changed, 8 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/31235/5
Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: soc/intel: Fix for missing mem_rank info in SMBIOS ......................................................................
Patch Set 5:
(5 comments)
Guys,
You review comments/suggestions have been implememted. Please take a look.
Regards Francois
https://review.coreboot.org/#/c/31235/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31235/1//COMMIT_MSG@7 PS1, Line 7: Fix for missing mem_rank info in SMBIOS
Please add a prefix (cf. […]
Done
https://review.coreboot.org/#/c/31235/1//COMMIT_MSG@10 PS1, Line 10: None
b:122329046
Done
https://review.coreboot.org/#/c/31235/1//COMMIT_MSG@11 PS1, Line 11: Boot to OS:
Is rank reported correctly in SMBIOS?
Done
https://review.coreboot.org/#/c/31235/1//COMMIT_MSG@12 PS1, Line 12:
BRANCH=octopus
Done
https://review.coreboot.org/#/c/31235/1/src/soc/intel/skylake/romstage/romst... File src/soc/intel/skylake/romstage/romstage_fsp20.c:
https://review.coreboot.org/#/c/31235/1/src/soc/intel/skylake/romstage/romst... PS1, Line 124: m
Is this the right field name?
Done
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: soc/intel: Fix for missing mem_rank info in SMBIOS ......................................................................
Patch Set 5:
(5 comments)
https://review.coreboot.org/#/c/31235/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31235/5//COMMIT_MSG@8 PS5, Line 8: Can you please add 2 or 3 line description regarding what the problem currently is without this change and how this change is addressing that.
https://review.coreboot.org/#/c/31235/5//COMMIT_MSG@9 PS5, Line 9: Found-by: Google I am not sure if this is common to add a "Found-by:" line. I haven't seen it in the past or done it myself. Again it is for my own knowledge.
https://review.coreboot.org/#/c/31235/5//COMMIT_MSG@10 PS5, Line 10: BUG=Wrong memory rank info in SMBIOS Please update this line to "BUG=b:122329046" as Furquan mentioned in one of his previous comments
https://review.coreboot.org/#/c/31235/5//COMMIT_MSG@11 PS5, Line 11: TEST=Boot to OS Seems this change was done to improve the performance. If so did you run any test related to performance profiling and can you please add that information here.
https://review.coreboot.org/#/c/31235/5/3rdparty/blobs File 3rdparty/blobs:
https://review.coreboot.org/#/c/31235/5/3rdparty/blobs@a1 PS5, Line 1: Is this Delete intentional? Is it related to the change?
Hello Aaron Durbin, Patrick Rudolph, Karthik Ramasubramanian, Justin TerAvest, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31235
to look at the new patch set (#6).
Change subject: soc/intel: Fix for missing mem_rank info in SMBIOS ......................................................................
soc/intel: Fix for missing mem_rank info in SMBIOS
"mosys memory spd print all" returns incorrect memory ranks info. This patch and 2 upcomming ones (one in FSP) will address the issue. BUG=Wrong memory rank info in SMBIOS TEST=Boot to OS BRANCH=octopus
Change-Id: I212215040e4786c258a9c604cc5c2bb62867c842 Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- M 3rdparty/blobs M src/soc/intel/apollolake/meminit_util_apl.c M src/soc/intel/apollolake/meminit_util_glk.c M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/common/smbios.c M src/soc/intel/common/smbios.h M src/soc/intel/icelake/romstage/romstage.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 8 files changed, 9 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/31235/6
Hello Aaron Durbin, Patrick Rudolph, Karthik Ramasubramanian, Justin TerAvest, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31235
to look at the new patch set (#7).
Change subject: soc/intel: Fix for missing mem_rank info in SMBIOS ......................................................................
soc/intel: Fix for missing mem_rank info in SMBIOS
"mosys memory spd print all" returns incorrect memory ranks info. This patch and 2 upcomming ones (one in FSP) will address the issue. BUG=b:122329046 TEST=Boot to OS BRANCH=octopus
Change-Id: I212215040e4786c258a9c604cc5c2bb62867c842 Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- M 3rdparty/blobs M src/soc/intel/apollolake/meminit_util_apl.c M src/soc/intel/apollolake/meminit_util_glk.c M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/common/smbios.c M src/soc/intel/common/smbios.h M src/soc/intel/icelake/romstage/romstage.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 8 files changed, 9 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/31235/7
Francois Toguo Fotso has uploaded a new patch set (#8) to the change originally created by Francois Toguo Fotso. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: soc/intel: Fix for missing mem_rank info in SMBIOS ......................................................................
soc/intel: Fix for missing mem_rank info in SMBIOS
"mosys memory spd print all" returns incorrect memory ranks info. This patch and 2 upcomming ones (one in FSP) will address the issue. BUG=b:122329046 TEST=Boot to OS BRANCH=octopus
Change-Id: I212215040e4786c258a9c604cc5c2bb62867c842 Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- M 3rdparty/blobs M src/soc/intel/apollolake/meminit_util_apl.c M src/soc/intel/apollolake/meminit_util_glk.c M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/common/smbios.c M src/soc/intel/common/smbios.h M src/soc/intel/icelake/romstage/romstage.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 8 files changed, 9 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/31235/8
Francois Toguo Fotso has uploaded a new patch set (#9) to the change originally created by Francois Toguo Fotso. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: soc/intel: Fix for missing mem_rank info in SMBIOS ......................................................................
soc/intel: Fix for missing mem_rank info in SMBIOS
"mosys memory spd print all" returns incorrect memory ranks info. This patch and 2 upcomming ones (one in FSP) will address the issue. BUG=b:122329046 TEST=Boot to OS BRANCH=octopus
Change-Id: I212215040e4786c258a9c604cc5c2bb62867c842 Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- D 3rdparty/blobs M src/soc/intel/apollolake/meminit_util_apl.c M src/soc/intel/apollolake/meminit_util_glk.c M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/common/smbios.c M src/soc/intel/common/smbios.h M src/soc/intel/icelake/romstage/romstage.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 8 files changed, 8 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/31235/9
Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: soc/intel: Fix for missing mem_rank info in SMBIOS ......................................................................
Patch Set 9:
Patch Set 5:
(5 comments)
"Seems this change was done to improve the performance" No this change was done to correct the memory information being repported by MOSYS
The deletion of the blob was intentional, it should not be in the patch as mentioned by one reviewer.
All other recommandation have been implemented.
Regards
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: soc/intel: Fix for missing mem_rank info in SMBIOS ......................................................................
Patch Set 9:
(3 comments)
https://review.coreboot.org/#/c/31235/9/3rdparty/blobs File 3rdparty/blobs:
PS9: This should be removed.
https://review.coreboot.org/#/c/31235/9/src/soc/intel/common/smbios.h File src/soc/intel/common/smbios.h:
https://review.coreboot.org/#/c/31235/9/src/soc/intel/common/smbios.h@24 PS9, Line 24: dim dimm?
https://review.coreboot.org/#/c/31235/9/src/soc/intel/common/smbios.c File src/soc/intel/common/smbios.c:
https://review.coreboot.org/#/c/31235/9/src/soc/intel/common/smbios.c@23 PS9, Line 23: rankInDimm keep this the same as the param name in declaration in smbios.h?
Francois Toguo Fotso has uploaded a new patch set (#10) to the change originally created by Francois Toguo Fotso. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: soc/intel: Fix for missing mem_rank info in SMBIOS ......................................................................
soc/intel: Fix for missing mem_rank info in SMBIOS
"mosys memory spd print all" returns incorrect memory ranks info. This patch and 2 upcomming ones (one in FSP) will address the issue. BUG=b:122329046 TEST=Boot to OS BRANCH=octopus
Change-Id: I212215040e4786c258a9c604cc5c2bb62867c842 Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- D 3rdparty/blobs M src/soc/intel/apollolake/meminit_util_apl.c M src/soc/intel/apollolake/meminit_util_glk.c M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/common/smbios.c M src/soc/intel/common/smbios.h M src/soc/intel/icelake/romstage/romstage.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 8 files changed, 8 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/31235/10
Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: soc/intel: Fix for missing mem_rank info in SMBIOS ......................................................................
Patch Set 10:
(8 comments)
Patch Set 9:
(3 comments)
The blob have already been deleted and is marked as "D" Other comments now implemented
https://review.coreboot.org/#/c/31235/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31235/5//COMMIT_MSG@8 PS5, Line 8:
Can you please add 2 or 3 line description regarding what the problem currently is without this chan […]
Done
https://review.coreboot.org/#/c/31235/5//COMMIT_MSG@9 PS5, Line 9: Found-by: Google
I am not sure if this is common to add a "Found-by:" line. […]
Done
https://review.coreboot.org/#/c/31235/5//COMMIT_MSG@10 PS5, Line 10: BUG=Wrong memory rank info in SMBIOS
Please update this line to "BUG=b:122329046" as Furquan mentioned in one of his previous comments
Done
https://review.coreboot.org/#/c/31235/5//COMMIT_MSG@11 PS5, Line 11: TEST=Boot to OS
Seems this change was done to improve the performance. […]
No this change was done to correct the memory information being repported by MOSYS.
https://review.coreboot.org/#/c/31235/5/3rdparty/blobs File 3rdparty/blobs:
https://review.coreboot.org/#/c/31235/5/3rdparty/blobs@a1 PS5, Line 1:
Is this Delete intentional? Is it related to the change?
The delete was intentional, it should not be in the patch as mentioned by one reviewer
https://review.coreboot.org/#/c/31235/9/3rdparty/blobs File 3rdparty/blobs:
PS9:
This should be removed.
It has already been removed. It is marked as "D". What elese should be done?
https://review.coreboot.org/#/c/31235/9/src/soc/intel/common/smbios.h File src/soc/intel/common/smbios.h:
https://review.coreboot.org/#/c/31235/9/src/soc/intel/common/smbios.h@24 PS9, Line 24: dim
dimm?
Done
https://review.coreboot.org/#/c/31235/9/src/soc/intel/common/smbios.c File src/soc/intel/common/smbios.c:
https://review.coreboot.org/#/c/31235/9/src/soc/intel/common/smbios.c@23 PS9, Line 23: rankInDimm
keep this the same as the param name in declaration in smbios. […]
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: soc/intel: Fix for missing mem_rank info in SMBIOS ......................................................................
Patch Set 10:
(3 comments)
The submodule pointer change needs to be removed.
https://review.coreboot.org/#/c/31235/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31235/10//COMMIT_MSG@7 PS10, Line 7: soc/intel: Fix for missing mem_rank info in SMBIOS Please make it a statement by using a verb (in imperative mood):
Fix missing mem_rank info in SMBIOS
Maybe better:
Add mem_rank info to SMBIOS
https://review.coreboot.org/#/c/31235/10//COMMIT_MSG@11 PS10, Line 11: BUG=b:122329046 Please add a blank line above.
https://review.coreboot.org/#/c/31235/10//COMMIT_MSG@12 PS10, Line 12: TEST=Boot to OS … and run `dmidecode`?
On what board (are there variants)?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: soc/intel: Fix for missing mem_rank info in SMBIOS ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/#/c/31235/5/3rdparty/blobs File 3rdparty/blobs:
https://review.coreboot.org/#/c/31235/5/3rdparty/blobs@a1 PS5, Line 1:
The delete was intentional, it should not be in the patch as mentioned by one reviewer
No, you should not add or delete the submodules pointer as part of your CL. I meant remove this from your change. Sorry if I hadn't been clear before.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: soc/intel: Fix for missing mem_rank info in SMBIOS ......................................................................
Patch Set 10:
Francois,
Here's what to do. Assume the coreboot.org repository name is 'upstream' (mine is. yours might be different):
$ git fetch upstream remote: Counting objects: 25377, done remote: Finding sources: 100% (160/160) remote: Total 160 (delta 75), reused 160 (delta 75) Receiving objects: 100% (160/160), 57.15 KiB | 6.35 MiB/s, done. Resolving deltas: 100% (75/75), completed with 43 local objects. From ssh://review.coreboot.org:29418/coreboot 06e33226b3..540a664045 master -> upstream/master $ git submodule update --init --checkout $ git rebase upstream/master $ git submodule update --init --checkout
You might need to amend your latest patch after that to include the latest submodule update.
Hello Aaron Durbin, Patrick Rudolph, Karthik Ramasubramanian, Justin TerAvest, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31235
to look at the new patch set (#11).
Change subject: soc/intel: Add mem_rank info in SMBIOS ......................................................................
soc/intel: Add mem_rank info in SMBIOS
"mosys memory spd print all" returns incorrect memory ranks info. This patch and 2 upcomming ones (one in FSP) will address the issue.
BUG=b:122329046 TEST=Boot to OS on Bobba variant of Octopus BRANCH=octopus
Change-Id: I212215040e4786c258a9c604cc5c2bb62867c842 Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- M src/soc/intel/apollolake/meminit_util_apl.c M src/soc/intel/apollolake/meminit_util_glk.c M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/common/smbios.c M src/soc/intel/common/smbios.h M src/soc/intel/icelake/romstage/romstage.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 7 files changed, 8 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/31235/11
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: soc/intel: Add mem_rank info in SMBIOS ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/#/c/31235/11/src/soc/intel/apollolake/meminit_ut... File src/soc/intel/apollolake/meminit_util_apl.c:
https://review.coreboot.org/#/c/31235/11/src/soc/intel/apollolake/meminit_ut... PS11, Line 88: src_dimm->RankInDimm Build bot seems to be failing because this field is not present in DIMM_INFO. Is it updated as part of FSP headers already?
Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: soc/intel: Add mem_rank info in SMBIOS ......................................................................
Patch Set 11:
Patch Set 11:
(1 comment)
Not quite sure why that is. The build failure is not related to this change. I just clone the coreboot.org repo and RankInDimm is indeed present in DIMM_INFO from /src/vendorcode/intel/fsp/fsp2_0/glk/FspmUpd.h
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: soc/intel: Add mem_rank info in SMBIOS ......................................................................
Patch Set 11:
Patch Set 11:
Patch Set 11:
(1 comment)
Not quite sure why that is. The build failure is not related to this change. I just clone the coreboot.org repo and RankInDimm is indeed present in DIMM_INFO from /src/vendorcode/intel/fsp/fsp2_0/glk/FspmUpd.h
But it's not in the apollolake fspupd which is causing the failures.
Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: soc/intel: Add mem_rank info in SMBIOS ......................................................................
Patch Set 11:
(1 comment)
Patch Set 11:
Patch Set 11:
Patch Set 11:
(1 comment)
Not quite sure why that is. The build failure is not related to this change. I just clone the coreboot.org repo and RankInDimm is indeed present in DIMM_INFO from /src/vendorcode/intel/fsp/fsp2_0/glk/FspmUpd.h
But it's not in the apollolake fspupd which is causing the failures.
My build infrastructure did build for GLK, not apollolake (APL). Which is why I did not get this error. This value will be set to 0 in the APL call. The originally missing RankInDimm bug was filed for GLK. Aaron, in case this is also needed for APL, I would suggest that you please file a partner issue for APL, so that FSP team for APL can add that field in their UDP info.
https://review.coreboot.org/#/c/31235/5/3rdparty/blobs File 3rdparty/blobs:
https://review.coreboot.org/#/c/31235/5/3rdparty/blobs@a1 PS5, Line 1:
No, you should not add or delete the submodules pointer as part of your CL. […]
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: soc/intel: Add mem_rank info in SMBIOS ......................................................................
Patch Set 11:
(1 comment)
Patch Set 11:
(1 comment)
Patch Set 11:
Patch Set 11:
Patch Set 11:
(1 comment)
Not quite sure why that is. The build failure is not related to this change. I just clone the coreboot.org repo and RankInDimm is indeed present in DIMM_INFO from /src/vendorcode/intel/fsp/fsp2_0/glk/FspmUpd.h
But it's not in the apollolake fspupd which is causing the failures.
My build infrastructure did build for GLK, not apollolake (APL). Which is why I did not get this error. This value will be set to 0 in the APL call. The originally missing RankInDimm bug was filed for GLK. Aaron, in case this is also needed for APL, I would suggest that you please file a partner issue for APL, so that FSP team for APL can add that field in their UDP info.
See my comment in meminit_util_apl.c. The CL cannot go in because its referencing things that don't exist.
https://review.coreboot.org/#/c/31235/11/src/soc/intel/apollolake/meminit_ut... File src/soc/intel/apollolake/meminit_util_apl.c:
https://review.coreboot.org/#/c/31235/11/src/soc/intel/apollolake/meminit_ut... PS11, Line 88: src_dimm->RankInDimm
Build bot seems to be failing because this field is not present in DIMM_INFO. […]
In order for the builds to pass this needs to be set to 0 as there aren't fields that cover this.
Francois Toguo Fotso has uploaded a new patch set (#12) to the change originally created by Francois Toguo Fotso. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: soc/intel: Add mem_rank info in SMBIOS ......................................................................
soc/intel: Add mem_rank info in SMBIOS
"mosys memory spd print all" returns incorrect memory ranks info. This patch and 2 upcomming ones (one in FSP) will address the issue.
BUG=b:122329046 TEST=Boot to OS on Bobba variant of Octopus BRANCH=octopus
Change-Id: I212215040e4786c258a9c604cc5c2bb62867c842 Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- M src/soc/intel/apollolake/meminit_util_apl.c M src/soc/intel/apollolake/meminit_util_glk.c M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/common/smbios.c M src/soc/intel/common/smbios.h M src/soc/intel/icelake/romstage/romstage.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 7 files changed, 8 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/31235/12
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: soc/intel: Add mem_rank info in SMBIOS ......................................................................
Patch Set 12: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31235 )
Change subject: soc/intel: Add mem_rank info in SMBIOS ......................................................................
soc/intel: Add mem_rank info in SMBIOS
"mosys memory spd print all" returns incorrect memory ranks info. This patch and 2 upcomming ones (one in FSP) will address the issue.
BUG=b:122329046 TEST=Boot to OS on Bobba variant of Octopus BRANCH=octopus
Change-Id: I212215040e4786c258a9c604cc5c2bb62867c842 Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com Reviewed-on: https://review.coreboot.org/c/31235 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/apollolake/meminit_util_apl.c M src/soc/intel/apollolake/meminit_util_glk.c M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/common/smbios.c M src/soc/intel/common/smbios.h M src/soc/intel/icelake/romstage/romstage.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 7 files changed, 8 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/soc/intel/apollolake/meminit_util_apl.c b/src/soc/intel/apollolake/meminit_util_apl.c index 755d708..a11c5d8 100644 --- a/src/soc/intel/apollolake/meminit_util_apl.c +++ b/src/soc/intel/apollolake/meminit_util_apl.c @@ -85,6 +85,7 @@ src_dimm->SizeInMb, memory_info_hob->MemoryType, memory_info_hob->MemoryFrequencyInMHz, + 0, channel_info->ChannelId, src_dimm->DimmId, dram_part_num, diff --git a/src/soc/intel/apollolake/meminit_util_glk.c b/src/soc/intel/apollolake/meminit_util_glk.c index db69f51..9bfdf0b 100644 --- a/src/soc/intel/apollolake/meminit_util_glk.c +++ b/src/soc/intel/apollolake/meminit_util_glk.c @@ -91,6 +91,7 @@ src_dimm->DimmCapacity, memory_info_hob->MemoryType, memory_info_hob->ConfiguredMemoryClockSpeed, + src_dimm->RankInDimm, channel_info->ChannelId, src_dimm->DimmId, dram_part_num, diff --git a/src/soc/intel/cannonlake/romstage/romstage.c b/src/soc/intel/cannonlake/romstage/romstage.c index 9e25e55..3755c83 100644 --- a/src/soc/intel/cannonlake/romstage/romstage.c +++ b/src/soc/intel/cannonlake/romstage/romstage.c @@ -92,6 +92,7 @@ src_dimm->DimmCapacity, memory_info_hob->MemoryType, memory_info_hob->ConfiguredMemoryClockSpeed, + src_dimm->RankInDimm, channel_info->ChannelId, src_dimm->DimmId, (const char *)src_dimm->ModulePartNum, diff --git a/src/soc/intel/common/smbios.c b/src/soc/intel/common/smbios.c index bcddb78..0b1be88 100644 --- a/src/soc/intel/common/smbios.c +++ b/src/soc/intel/common/smbios.c @@ -20,13 +20,14 @@
/* Fill the SMBIOS memory information from FSP MEM_INFO_DATA_HOB in CBMEM.*/ void dimm_info_fill(struct dimm_info *dimm, u32 dimm_capacity, u8 ddr_type, - u32 frequency, u8 channel_id, u8 dimm_id, + u32 frequency, u8 rank_per_dimm, u8 channel_id, u8 dimm_id, const char *module_part_num, size_t module_part_number_size, u16 data_width) { dimm->dimm_size = dimm_capacity; dimm->ddr_type = ddr_type; dimm->ddr_frequency = frequency; + dimm->rank_per_dimm = rank_per_dimm; dimm->channel_num = channel_id; dimm->dimm_num = dimm_id; strncpy((char *)dimm->module_part_number, diff --git a/src/soc/intel/common/smbios.h b/src/soc/intel/common/smbios.h index 4750d3c..33b5d0d 100644 --- a/src/soc/intel/common/smbios.h +++ b/src/soc/intel/common/smbios.h @@ -21,7 +21,7 @@
/* Fill the SMBIOS memory information from FSP MEM_INFO_DATA_HOB in CBMEM.*/ void dimm_info_fill(struct dimm_info *dimm, u32 dimm_capacity, u8 ddr_type, - u32 frequency, u8 channel_id, u8 dimm_id, + u32 frequency, u8 rank_per_dimm, u8 channel_id, u8 dimm_id, const char *module_part_num, size_t module_part_number_size, u16 data_width);
diff --git a/src/soc/intel/icelake/romstage/romstage.c b/src/soc/intel/icelake/romstage/romstage.c index 3df4f4f..9a611bb 100644 --- a/src/soc/intel/icelake/romstage/romstage.c +++ b/src/soc/intel/icelake/romstage/romstage.c @@ -94,6 +94,7 @@ src_dimm->DimmCapacity, memory_info_hob->MemoryType, memory_info_hob->ConfiguredMemoryClockSpeed, + src_dimm->RankInDimm, channel_info->ChannelId, src_dimm->DimmId, (const char *)src_dimm->ModulePartNum, diff --git a/src/soc/intel/skylake/romstage/romstage_fsp20.c b/src/soc/intel/skylake/romstage/romstage_fsp20.c index 6fe79f6..f92ddca 100644 --- a/src/soc/intel/skylake/romstage/romstage_fsp20.c +++ b/src/soc/intel/skylake/romstage/romstage_fsp20.c @@ -121,6 +121,7 @@ src_dimm->DimmCapacity, ddr_type, memory_info_hob->ConfiguredMemoryClockSpeed, + src_dimm->RankInDimm, channel_info->ChannelId, src_dimm->DimmId, (const char *)src_dimm->ModulePartNum,
-
Aaron Durbin (Code Review) -
Francois Toguo Fotso (Code Review)
-
Furquan Shaikh (Code Review)
-
Karthik Ramasubramanian (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)