Change in coreboot[master]: soc/baytrail/raminit: Populate SMBIOS type 17 tables

[Matt DeVillier (Code Review)]

Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40643 )

Change subject: soc/baytrail/raminit: Populate SMBIOS type 17 tables
......................................................................

soc/baytrail/raminit: Populate SMBIOS type 17 tables

Populate SMBIOS type 17 tables using data from SPD and read via IOSF.
Refactor print_dram_info() to pass thru SPD data and channel/speed info.
Move call to print_dram_info() after cbmem initialization so the SMBIOS
data has somewhere to go.

Test: build/boot google/swanky, verify via dmidecode.

Change-Id: I1c12b539c78d095713421b93115a4095f3d4278d
Signed-off-by: Matt DeVillier 
---
M src/soc/intel/baytrail/romstage/raminit.c
1 file changed, 27 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/40643/1

diff --git a/src/soc/intel/baytrail/romstage/raminit.c b/src/soc/intel/baytrail/romstage/raminit.c
index f1a3c17..1f6309f 100644
--- a/src/soc/intel/baytrail/romstage/raminit.c
+++ b/src/soc/intel/baytrail/romstage/raminit.c
@@ -8,6 +8,7 @@
 #include <cbmem.h>
 #include <cf9_reset.h>
 #include <console/console.h>
+#include <device/dram/ddr3.h>
 #include <device/pci_def.h>
 #include <device/pci_ops.h>
 #include <device/smbus_host.h>
@@ -20,6 +21,7 @@
 #include <ec/google/chromeec/ec.h>
 #include <ec/google/chromeec/ec_commands.h>
 #include <security/vboot/vboot_common.h>
+#include <spd_bin.h>

uintptr_t smbus_base(void)
 {
@@ -55,7 +57,27 @@
    do_putchar(b);
 }

-static void print_dram_info(void)
+static void populate_smbios_tables(void *dram_data, int speed, int num_channels)
+{
+	spd_raw_data spd;
+	dimm_attr dimm;
+	enum spd_status status;
+
+	/* copy raw data into spd struct */
+	memcpy(&spd, dram_data, SPD_PAGE_LEN);
+
+	/* decode into dimm_attr struct */
+	status = spd_decode_ddr3(&dimm, spd);
+
+	/* some SPDs have bad CRCs, nothing we can do about it */
+	if (status == SPD_STATUS_OK || status == SPD_STATUS_CRC_ERROR) {
+		/* add table 17 entry for each channel */
+		for (int i = 0; i < num_channels; i++)
+			spd_add_smbios17(i, 0, speed, &dimm);
+	}
+}
+
+static void print_dram_info(void *dram_data)
 {
    const int mrc_ver_reg = 0xf0;
    const uint32_t soc_dev = PCI_DEV(0, SOC_DEV, SOC_FUNC);
@@ -95,6 +117,8 @@
    	speed = 1600; break;
    }
    printk(BIOS_INFO, "%dMHz\n", speed);
+
+	populate_smbios_tables(dram_data, speed, num_channels);
 }

void raminit(struct mrc_params *mp, int prev_sleep_state)
@@ -147,8 +171,6 @@

ret = mrc_entry(mp);

-	print_dram_info();
-
    if (prev_sleep_state != ACPI_S3) {
    	cbmem_initialize_empty();
    } else if (cbmem_initialize()) {
@@ -159,6 +181,8 @@
    #endif
    }

+	print_dram_info(mp->mainboard.dram_data[0]);
+
    printk(BIOS_DEBUG, "MRC Wrapper returned %d\n", ret);
    printk(BIOS_DEBUG, "MRC data at %p %d bytes\n", mp->data_to_save,
           mp->data_to_save_size);

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40643 )

Change subject: soc/baytrail/raminit: Populate SMBIOS type 17 tables
......................................................................

Patch Set 1: Code-Review+1

How much time does that add to the boot time?

Do some other Intel chipsets already have similar code?

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40643 )

Change subject: soc/baytrail/raminit: Populate SMBIOS type 17 tables
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review+1
>
> How much time does that add to the boot time?

effectively none, it's within the noise
> Do some other Intel chipsets already have similar code?

all other chipsets get the data into SMBIOS type 17 tables somehow, Baytrail was the oddball in not doing so. Sandybridge also uses the spd_add_smbios17() function.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40643 )

Change subject: soc/baytrail/raminit: Populate SMBIOS type 17 tables
......................................................................

Patch Set 1: Code-Review+1

(2 comments)

https://review.coreboot.org/c/coreboot/+/40643/1/src/soc/intel/baytrail/roms... 
File src/soc/intel/baytrail/romstage/raminit.c:

https://review.coreboot.org/c/coreboot/+/40643/1/src/soc/intel/baytrail/roms... 
PS1, Line 66: 	/* copy raw data into spd struct */
Why do all the comments start in lowercase?

https://review.coreboot.org/c/coreboot/+/40643/1/src/soc/intel/baytrail/roms... 
PS1, Line 67: 	memcpy(&spd, dram_data, SPD_PAGE_LEN);
Why the memcpy? spd_decode_ddr3 does not modify the buffer (granted, the parameter should be const). Also, if the buffer pointed by `dram_data` isn't big enough it's going to run astray anyway.

[Matt DeVillier (Code Review)]

Hello build bot (Jenkins), Paul Menzel, Angel Pons, Patrick Rudolph,

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

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

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

Change subject: soc/baytrail/raminit: Populate SMBIOS type 17 tables
......................................................................

soc/baytrail/raminit: Populate SMBIOS type 17 tables

Populate SMBIOS type 17 tables using data from SPD and read via IOSF.
Refactor print_dram_info() to pass thru SPD data and channel/speed info.
Move call to print_dram_info() after cbmem initialization so the SMBIOS
data has somewhere to go.

Test: build/boot google/swanky, verify via dmidecode.

Change-Id: I1c12b539c78d095713421b93115a4095f3d4278d
Signed-off-by: Matt DeVillier 
---
M src/soc/intel/baytrail/romstage/raminit.c
1 file changed, 23 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/40643/2

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40643 )

Change subject: soc/baytrail/raminit: Populate SMBIOS type 17 tables
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/40643/1/src/soc/intel/baytrail/roms... 
File src/soc/intel/baytrail/romstage/raminit.c:

https://review.coreboot.org/c/coreboot/+/40643/1/src/soc/intel/baytrail/roms... 
PS1, Line 66: 	/* copy raw data into spd struct */
> Why do all the comments start in lowercase?

Done

https://review.coreboot.org/c/coreboot/+/40643/1/src/soc/intel/baytrail/roms... 
PS1, Line 67: 	memcpy(&spd, dram_data, SPD_PAGE_LEN);
> Why the memcpy? spd_decode_ddr3 does not modify the buffer (granted, the parameter should be const). […]

Done

[Matt DeVillier (Code Review)]

Hello build bot (Jenkins), Paul Menzel, Angel Pons, Patrick Rudolph,

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

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

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

Change subject: soc/baytrail/raminit: Populate SMBIOS type 17 tables
......................................................................

soc/baytrail/raminit: Populate SMBIOS type 17 tables

Populate SMBIOS type 17 tables using data from SPD and read via IOSF.
Refactor print_dram_info() to pass thru SPD data and channel/speed info.
Move call to print_dram_info() after cbmem initialization so the SMBIOS
data has somewhere to go.

Test: build/boot google/swanky, verify via dmidecode.

Change-Id: I1c12b539c78d095713421b93115a4095f3d4278d
Signed-off-by: Matt DeVillier 
---
M src/soc/intel/baytrail/romstage/raminit.c
1 file changed, 22 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/40643/3

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40643 )

Change subject: soc/baytrail/raminit: Populate SMBIOS type 17 tables
......................................................................

Patch Set 3: Code-Review+2

[Matt DeVillier (Code Review)]

Matt DeVillier has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40643 )

Change subject: soc/baytrail/raminit: Populate SMBIOS type 17 tables
......................................................................

soc/baytrail/raminit: Populate SMBIOS type 17 tables

Populate SMBIOS type 17 tables using data from SPD and read via IOSF.
Refactor print_dram_info() to pass thru SPD data and channel/speed info.
Move call to print_dram_info() after cbmem initialization so the SMBIOS
data has somewhere to go.

Test: build/boot google/swanky, verify via dmidecode.

Change-Id: I1c12b539c78d095713421b93115a4095f3d4278d
Signed-off-by: Matt DeVillier 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/40643
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
M src/soc/intel/baytrail/romstage/raminit.c
1 file changed, 22 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved

diff --git a/src/soc/intel/baytrail/romstage/raminit.c b/src/soc/intel/baytrail/romstage/raminit.c
index f1a3c17..3ebf8ff 100644
--- a/src/soc/intel/baytrail/romstage/raminit.c
+++ b/src/soc/intel/baytrail/romstage/raminit.c
@@ -8,6 +8,7 @@
 #include <cbmem.h>
 #include <cf9_reset.h>
 #include <console/console.h>
+#include <device/dram/ddr3.h>
 #include <device/pci_def.h>
 #include <device/pci_ops.h>
 #include <device/smbus_host.h>
@@ -55,7 +56,23 @@
    do_putchar(b);
 }

-static void print_dram_info(void)
+static void populate_smbios_tables(void *dram_data, int speed, int num_channels)
+{
+	dimm_attr dimm;
+	enum spd_status status;
+
+	/* Decode into dimm_attr struct */
+	status = spd_decode_ddr3(&dimm, *(spd_raw_data *)dram_data);
+
+	/* Some SPDs have bad CRCs, nothing we can do about it */
+	if (status == SPD_STATUS_OK || status == SPD_STATUS_CRC_ERROR) {
+		/* Add table 17 entry for each channel */
+		for (int i = 0; i < num_channels; i++)
+			spd_add_smbios17(i, 0, speed, &dimm);
+	}
+}
+
+static void print_dram_info(void *dram_data)
 {
    const int mrc_ver_reg = 0xf0;
    const uint32_t soc_dev = PCI_DEV(0, SOC_DEV, SOC_FUNC);
@@ -95,6 +112,8 @@
    	speed = 1600; break;
    }
    printk(BIOS_INFO, "%dMHz\n", speed);
+
+	populate_smbios_tables(dram_data, speed, num_channels);
 }

void raminit(struct mrc_params *mp, int prev_sleep_state)
@@ -147,8 +166,6 @@

ret = mrc_entry(mp);

-	print_dram_info();
-
    if (prev_sleep_state != ACPI_S3) {
    	cbmem_initialize_empty();
    } else if (cbmem_initialize()) {
@@ -159,6 +176,8 @@
    #endif
    }

+	print_dram_info(mp->mainboard.dram_data[0]);
+
    printk(BIOS_DEBUG, "MRC Wrapper returned %d\n", ret);
    printk(BIOS_DEBUG, "MRC data at %p %d bytes\n", mp->data_to_save,
           mp->data_to_save_size);

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40643 )

Change subject: soc/baytrail/raminit: Populate SMBIOS type 17 tables
......................................................................

Patch Set 4:

Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4
Emulation targets:
"QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/2758
"QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2757
"QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2756
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/2755

Please note: This test is under development and might not be accurate at all!