Change in ...coreboot[master]: soc/intel/cannonlake: Fill DIMM serial number from SPD

[Duncan Laurie (Code Review)]

Duncan Laurie has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32853

Change subject: soc/intel/cannonlake: Fill DIMM serial number from SPD
......................................................................

soc/intel/cannonlake: Fill DIMM serial number from SPD

Fill the DIMM serial number field for SMBIOS from the saved SPD
data that is returned by FSP.

BUG=b:132970635
TEST=This was tested on sarien to ensure that SMBIOS type 17
filled the serial number from the DIMM:

Handle 0x000B, DMI type 17, 40 bytes
Memory Device
        Locator: DIMM-A
        Serial Number: 41164beb

Change-Id: I85438bd1d581095ea3482dcf077a7f3389f1cd47
Signed-off-by: Duncan Laurie 
---
M src/soc/intel/cannonlake/romstage/romstage.c
1 file changed, 7 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/32853/1

diff --git a/src/soc/intel/cannonlake/romstage/romstage.c b/src/soc/intel/cannonlake/romstage/romstage.c
index 661c98a..8dd9d4b 100644
--- a/src/soc/intel/cannonlake/romstage/romstage.c
+++ b/src/soc/intel/cannonlake/romstage/romstage.c
@@ -37,6 +37,8 @@
    0x8d, 0x09, 0x11, 0xcf, 0x8b, 0x9f, 0x03, 0x23	\
 }

+#define SPD_SAVE_SERIAL_OFFSET	5
+
 void __weak mainboard_get_dram_part_num(const char **part_num, size_t *len)
 {
    /* Default weak implementation, no need to override part number. */
@@ -113,6 +115,11 @@
    			dram_part_num_len,
    			memory_info_hob->DataWidth);
    		index++;
+
+			/* Extract serial number from SPD */
+			memcpy(dest_dimm->serial,
+			       src_dimm->SpdSave + SPD_SAVE_SERIAL_OFFSET,
+			       DIMM_INFO_SERIAL_SIZE);
    	}
    }
    mem_info->dimm_cnt = index;

[Duncan Laurie (Code Review)]

Hello Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32853

to look at the new patch set (#2).

Change subject: soc/intel/cannonlake: Fill DIMM serial number from SPD
......................................................................

soc/intel/cannonlake: Fill DIMM serial number from SPD

Fill the DIMM serial number field for SMBIOS from the saved SPD
data that is returned by FSP.

BUG=b:132970635
TEST=This was tested on sarien to ensure that SMBIOS type 17
filled the serial number from the DIMM:

Handle 0x000B, DMI type 17, 40 bytes
Memory Device
        Locator: DIMM-A
        Serial Number: 41164beb

Change-Id: I85438bd1d581095ea3482dcf077a7f3389f1cd47
Signed-off-by: Duncan Laurie 
---
M src/soc/intel/cannonlake/include/soc/romstage.h
M src/soc/intel/cannonlake/romstage/romstage.c
2 files changed, 8 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/32853/2

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32853 )

Change subject: soc/intel/cannonlake: Fill DIMM serial number from SPD
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32853/2/src/soc/intel/cannonlake/romstage/ro... 
File src/soc/intel/cannonlake/romstage/romstage.c:

https://review.coreboot.org/#/c/32853/2/src/soc/intel/cannonlake/romstage/ro... 
PS2, Line 117: Extract serial number from SPD
looks like this SpdSave is not unique to cannonlake so I'll add this to dimm_info_fill() and fix the others.

Change in ...coreboot[master]: soc/intel: Fill DIMM serial number from SPD

[Duncan Laurie (Code Review)]

Hello Patrick Rudolph, Lijian Zhao, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32853

to look at the new patch set (#3).

Change subject: soc/intel: Fill DIMM serial number from SPD
......................................................................

soc/intel: Fill DIMM serial number from SPD

Fill the DIMM serial number field for SMBIOS from the saved SPD
data that is returned by FSP.

BUG=b:132970635
TEST=This was tested on sarien to ensure that SMBIOS type 17
filled the serial number from the DIMM:

Handle 0x000B, DMI type 17, 40 bytes
Memory Device
        Locator: DIMM-A
        Serial Number: 41164beb

Change-Id: I85438bd1d581095ea3482dcf077a7f3389f1cd47
Signed-off-by: Duncan Laurie 
---
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, 13 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/32853/3

Change in ...coreboot[master]: soc/intel: Fill DIMM serial number from SPD

[Lijian Zhao (Code Review)]

Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32853 )

Change subject: soc/intel: Fill DIMM serial number from SPD
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/32853/2/src/soc/intel/cannonlake/romstage/ro... 
File src/soc/intel/cannonlake/romstage/romstage.c:

https://review.coreboot.org/#/c/32853/2/src/soc/intel/cannonlake/romstage/ro... 
PS2, Line 117: Extract serial number from SPD
> looks like this SpdSave is not unique to cannonlake so I'll add this to dimm_info_fill() and fix the […]

Yes, SpdSave shall be generic in FSP. However in upstream, seems Apollolake don't have meminfo.h updated.

Change in ...coreboot[master]: soc/intel: Fill DIMM serial number from SPD

[Duncan Laurie (Code Review)]

Hello Patrick Rudolph, Lijian Zhao, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32853

to look at the new patch set (#4).

Change subject: soc/intel: Fill DIMM serial number from SPD
......................................................................

soc/intel: Fill DIMM serial number from SPD

Fill the DIMM serial number field for SMBIOS from the saved SPD
data that is returned by FSP.

BUG=b:132970635
TEST=This was tested on sarien to ensure that SMBIOS type 17
filled the serial number from the DIMM:

Handle 0x000B, DMI type 17, 40 bytes
Memory Device
        Locator: DIMM-A
        Serial Number: 41164beb

Change-Id: I85438bd1d581095ea3482dcf077a7f3389f1cd47
Signed-off-by: Duncan Laurie 
---
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, 13 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/32853/4

Change in ...coreboot[master]: soc/intel: Fill DIMM serial number from SPD

[Lijian Zhao (Code Review)]

Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32853 )

Change subject: soc/intel: Fill DIMM serial number from SPD
......................................................................

Patch Set 4: Code-Review+2

Change in ...coreboot[master]: soc/intel: Fill DIMM serial number from SPD

[Duncan Laurie (Code Review)]

Duncan Laurie has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32853 )

Change subject: soc/intel: Fill DIMM serial number from SPD
......................................................................

soc/intel: Fill DIMM serial number from SPD

Fill the DIMM serial number field for SMBIOS from the saved SPD
data that is returned by FSP.

BUG=b:132970635
TEST=This was tested on sarien to ensure that SMBIOS type 17
filled the serial number from the DIMM:

Handle 0x000B, DMI type 17, 40 bytes
Memory Device
        Locator: DIMM-A
        Serial Number: 41164beb

Change-Id: I85438bd1d581095ea3482dcf077a7f3389f1cd47
Signed-off-by: Duncan Laurie 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32853
Tested-by: build bot (Jenkins) 
Reviewed-by: Lijian Zhao 
---
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, 13 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Lijian Zhao: 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 a11c5d8..b272a99 100644
--- a/src/soc/intel/apollolake/meminit_util_apl.c
+++ b/src/soc/intel/apollolake/meminit_util_apl.c
@@ -90,6 +90,7 @@
    			src_dimm->DimmId,
    			dram_part_num,
    			strlen(dram_part_num),
+				NULL, /* SPD not available */
    			memory_info_hob->DataWidth);
    		index++;
    	}
diff --git a/src/soc/intel/apollolake/meminit_util_glk.c b/src/soc/intel/apollolake/meminit_util_glk.c
index 9bfdf0b..29dcd56 100644
--- a/src/soc/intel/apollolake/meminit_util_glk.c
+++ b/src/soc/intel/apollolake/meminit_util_glk.c
@@ -96,6 +96,7 @@
    			    src_dimm->DimmId,
    			    dram_part_num,
    			    strlen(dram_part_num),
+				    src_dimm->SpdSave + SPD_SAVE_OFFSET_SERIAL,
    			    memory_info_hob->DataWidth);
    			index++;
    		}
diff --git a/src/soc/intel/cannonlake/romstage/romstage.c b/src/soc/intel/cannonlake/romstage/romstage.c
index 661c98a..98d4c00 100644
--- a/src/soc/intel/cannonlake/romstage/romstage.c
+++ b/src/soc/intel/cannonlake/romstage/romstage.c
@@ -111,6 +111,7 @@
    			src_dimm->DimmId,
    			dram_part_num,
    			dram_part_num_len,
+				src_dimm->SpdSave + SPD_SAVE_OFFSET_SERIAL,
    			memory_info_hob->DataWidth);
    		index++;
    	}
diff --git a/src/soc/intel/common/smbios.c b/src/soc/intel/common/smbios.c
index 0b1be88..d89e9d5 100644
--- a/src/soc/intel/common/smbios.c
+++ b/src/soc/intel/common/smbios.c
@@ -22,7 +22,7 @@
 void dimm_info_fill(struct dimm_info *dimm, u32 dimm_capacity, u8 ddr_type,
    	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)
+		const u8 *module_serial_num, u16 data_width)
 {
    dimm->dimm_size = dimm_capacity;
    dimm->ddr_type = ddr_type;
@@ -34,6 +34,9 @@
    		module_part_num,
    		min(sizeof(dimm->module_part_number),
    			module_part_number_size));
+	if (module_serial_num)
+		memcpy(dimm->serial, module_serial_num,
+		       DIMM_INFO_SERIAL_SIZE);
    switch (data_width) {
    case 8:
    	dimm->bus_width = MEMORY_BUS_WIDTH_8;
diff --git a/src/soc/intel/common/smbios.h b/src/soc/intel/common/smbios.h
index 33b5d0d..5824f5d 100644
--- a/src/soc/intel/common/smbios.h
+++ b/src/soc/intel/common/smbios.h
@@ -19,10 +19,13 @@
 #include <stdint.h>
 #include <memory_info.h>

+/* Offset info DIMM_INFO SpdSave for start of serial number */
+#define SPD_SAVE_OFFSET_SERIAL	5
+
 /* 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 rank_per_dimm, u8 channel_id, u8 dimm_id,
    	const char *module_part_num, size_t module_part_number_size,
-		u16 data_width);
+		const u8 *module_serial_num, u16 data_width);

#endif /* _COMMON_SMBIOS_H_ */
diff --git a/src/soc/intel/icelake/romstage/romstage.c b/src/soc/intel/icelake/romstage/romstage.c
index 1a0c4ff..179d99c 100644
--- a/src/soc/intel/icelake/romstage/romstage.c
+++ b/src/soc/intel/icelake/romstage/romstage.c
@@ -96,6 +96,7 @@
    			src_dimm->DimmId,
    			(const char *)src_dimm->ModulePartNum,
    			sizeof(src_dimm->ModulePartNum),
+				src_dimm->SpdSave + SPD_SAVE_OFFSET_SERIAL,
    			memory_info_hob->DataWidth);
    		index++;
    	}
diff --git a/src/soc/intel/skylake/romstage/romstage_fsp20.c b/src/soc/intel/skylake/romstage/romstage_fsp20.c
index 2f75479..96937d6 100644
--- a/src/soc/intel/skylake/romstage/romstage_fsp20.c
+++ b/src/soc/intel/skylake/romstage/romstage_fsp20.c
@@ -126,6 +126,7 @@
    			src_dimm->DimmId,
    			(const char *)src_dimm->ModulePartNum,
    			sizeof(src_dimm->ModulePartNum),
+				src_dimm->SpdSave + SPD_SAVE_OFFSET_SERIAL,
    			memory_info_hob->DataWidth);
    		index++;
    	}