Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/58601 )
Change subject: lib: Add new argument as `ddr_type` to smbios_bus_width_to_spd_width() ......................................................................
lib: Add new argument as `ddr_type` to smbios_bus_width_to_spd_width()
Add DDR5 and LPDDR5 memory type checks while calculating bus width extension (in bits).
Additionally, update all caller functions of smbios_bus_width_to_spd_width() to pass `MemoryType` as argument.
Update `test_smbios_bus_width_to_spd_width()` to accommodate different memory types.
Create new macro to fix incorrect bus width reporting on platform with DDR5 and LPDDR5 memory.
With this code changes, on DDR5 system with 2 Ch per DIMM, 32 bit primary bus width per Ch showed the Total width as:
Handle 0x000F, DMI type 17, 40 bytes Memory Device Array Handle: 0x0009 Error Information Handle: Not Provided Total Width: 80 bits Data Width: 64 bits Size: 16 GB ...
BUG=b:194659789 Tested=On Alder Lake DDR5 RVP, SMBIOS type 17 shows expected `Total Width`.
Change-Id: I79ec64c9d522a34cb44b3f575725571823048380 Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/58601 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-by: Rob Barnes robbarnes@google.com --- M src/include/dimm_info_util.h M src/include/spd.h M src/lib/dimm_info_util.c M src/soc/amd/common/fsp/dmi.c M src/soc/amd/common/pi/amd_late_init.c M src/soc/intel/common/smbios.c M tests/lib/dimm_info_util-test.c 7 files changed, 52 insertions(+), 24 deletions(-)
Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, but someone else must approve Rob Barnes: Looks good to me, approved
diff --git a/src/include/dimm_info_util.h b/src/include/dimm_info_util.h index d20e8cd..04e9336 100644 --- a/src/include/dimm_info_util.h +++ b/src/include/dimm_info_util.h @@ -12,7 +12,7 @@ * Use this when setting dimm_info.bus_width if the raw SPD values are not * available. */ -uint8_t smbios_bus_width_to_spd_width(uint16_t total_width, +uint8_t smbios_bus_width_to_spd_width(uint8_t ddr_type, uint16_t total_width, uint16_t data_width);
/** diff --git a/src/include/spd.h b/src/include/spd.h index ec5296e..af3072e 100644 --- a/src/include/spd.h +++ b/src/include/spd.h @@ -209,5 +209,6 @@ #define SPD_MINI_UDIMM 0x20
#define SPD_ECC_8BIT (1<<3) +#define SPD_ECC_8BIT_LP5_DDR5 (1<<4)
#endif diff --git a/src/lib/dimm_info_util.c b/src/lib/dimm_info_util.c index fb8e06f..0e7cc12 100644 --- a/src/lib/dimm_info_util.c +++ b/src/lib/dimm_info_util.c @@ -5,7 +5,8 @@ #include <spd.h> #include <console/console.h>
-uint8_t smbios_bus_width_to_spd_width(uint16_t total_width, uint16_t data_width) +uint8_t smbios_bus_width_to_spd_width(uint8_t ddr_type, uint16_t total_width, + uint16_t data_width) { uint8_t out;
@@ -38,7 +39,10 @@
switch (extension_bits) { case 8: - out |= SPD_ECC_8BIT; + if (ddr_type == MEMORY_TYPE_DDR5 || ddr_type == MEMORY_TYPE_LPDDR5) + out |= SPD_ECC_8BIT_LP5_DDR5; + else + out |= SPD_ECC_8BIT; break; case 0: /* No extension bits */ diff --git a/src/soc/amd/common/fsp/dmi.c b/src/soc/amd/common/fsp/dmi.c index 61d43f7..223cce1 100644 --- a/src/soc/amd/common/fsp/dmi.c +++ b/src/soc/amd/common/fsp/dmi.c @@ -54,8 +54,8 @@
dimm->mod_type = smbios_form_factor_to_spd_mod_type(dmi17->FormFactor);
- dimm->bus_width = - smbios_bus_width_to_spd_width(dmi17->TotalWidth, dmi17->DataWidth); + dimm->bus_width = smbios_bus_width_to_spd_width(dmi17->MemoryType, dmi17->TotalWidth, + dmi17->DataWidth);
dimm->mod_id = dmi17->ManufacturerIdCode;
diff --git a/src/soc/amd/common/pi/amd_late_init.c b/src/soc/amd/common/pi/amd_late_init.c index 25aaea3..3714404 100644 --- a/src/soc/amd/common/pi/amd_late_init.c +++ b/src/soc/amd/common/pi/amd_late_init.c @@ -38,8 +38,8 @@
dimm->mod_type = smbios_form_factor_to_spd_mod_type(dmi17->FormFactor);
- dimm->bus_width = - smbios_bus_width_to_spd_width(dmi17->TotalWidth, dmi17->DataWidth); + dimm->bus_width = smbios_bus_width_to_spd_width(dmi17->MemoryType, dmi17->TotalWidth, + dmi17->DataWidth);
dimm->mod_id = dmi17->ManufacturerIdCode;
diff --git a/src/soc/intel/common/smbios.c b/src/soc/intel/common/smbios.c index a2b4a0b..213be65 100644 --- a/src/soc/intel/common/smbios.c +++ b/src/soc/intel/common/smbios.c @@ -65,5 +65,5 @@ if (ecc_support) total_width += EXTENSION_BUS_WIDTH_8BITS;
- dimm->bus_width = smbios_bus_width_to_spd_width(total_width, data_width); + dimm->bus_width = smbios_bus_width_to_spd_width(ddr_type, total_width, data_width); } diff --git a/tests/lib/dimm_info_util-test.c b/tests/lib/dimm_info_util-test.c index 4b3f979..25800d9 100644 --- a/tests/lib/dimm_info_util-test.c +++ b/tests/lib/dimm_info_util-test.c @@ -4,28 +4,51 @@ #include <spd.h> #include <tests/test.h>
-static void test_smbios_bus_width_to_spd_width(void **state) +static void test_smbios_bus_width_to_spd_width_parametrized(smbios_memory_type ddr_type) { /* Non-ECC variants */ - assert_int_equal(MEMORY_BUS_WIDTH_64, smbios_bus_width_to_spd_width(64, 64)); - assert_int_equal(MEMORY_BUS_WIDTH_32, smbios_bus_width_to_spd_width(32, 32)); - assert_int_equal(MEMORY_BUS_WIDTH_16, smbios_bus_width_to_spd_width(16, 16)); - assert_int_equal(MEMORY_BUS_WIDTH_8, smbios_bus_width_to_spd_width(8, 8)); + assert_int_equal(MEMORY_BUS_WIDTH_64, smbios_bus_width_to_spd_width(ddr_type, 64, 64)); + assert_int_equal(MEMORY_BUS_WIDTH_32, smbios_bus_width_to_spd_width(ddr_type, 32, 32)); + assert_int_equal(MEMORY_BUS_WIDTH_16, smbios_bus_width_to_spd_width(ddr_type, 16, 16)); + assert_int_equal(MEMORY_BUS_WIDTH_8, smbios_bus_width_to_spd_width(ddr_type, 8, 8)); /* Incorrect data width. Fallback to 8-bit */ - assert_int_equal(MEMORY_BUS_WIDTH_8, smbios_bus_width_to_spd_width(15, 15)); + assert_int_equal(MEMORY_BUS_WIDTH_8, smbios_bus_width_to_spd_width(ddr_type, 15, 15));
/* ECC variants */ - assert_int_equal(MEMORY_BUS_WIDTH_64 | SPD_ECC_8BIT, - smbios_bus_width_to_spd_width(64 + 8, 64)); - assert_int_equal(MEMORY_BUS_WIDTH_32 | SPD_ECC_8BIT, - smbios_bus_width_to_spd_width(32 + 8, 32)); - assert_int_equal(MEMORY_BUS_WIDTH_16 | SPD_ECC_8BIT, - smbios_bus_width_to_spd_width(16 + 8, 16)); - assert_int_equal(MEMORY_BUS_WIDTH_8 | SPD_ECC_8BIT, - smbios_bus_width_to_spd_width(8 + 8, 8)); + uint8_t extension_8bits = SPD_ECC_8BIT; + if (ddr_type == MEMORY_TYPE_DDR5 || ddr_type == MEMORY_TYPE_LPDDR5) + extension_8bits = SPD_ECC_8BIT_LP5_DDR5; + + assert_int_equal(MEMORY_BUS_WIDTH_64 | extension_8bits, + smbios_bus_width_to_spd_width(ddr_type, 64 + 8, 64)); + assert_int_equal(MEMORY_BUS_WIDTH_32 | extension_8bits, + smbios_bus_width_to_spd_width(ddr_type, 32 + 8, 32)); + assert_int_equal(MEMORY_BUS_WIDTH_16 | extension_8bits, + smbios_bus_width_to_spd_width(ddr_type, 16 + 8, 16)); + assert_int_equal(MEMORY_BUS_WIDTH_8 | extension_8bits, + smbios_bus_width_to_spd_width(ddr_type, 8 + 8, 8)); /* Incorrect data width. Fallback to 8-bit */ - assert_int_equal(MEMORY_BUS_WIDTH_8 | SPD_ECC_8BIT, - smbios_bus_width_to_spd_width(15 + 8, 15)); + assert_int_equal(MEMORY_BUS_WIDTH_8 | extension_8bits, + smbios_bus_width_to_spd_width(ddr_type, 15 + 8, 15)); +} + +static void test_smbios_bus_width_to_spd_width(void **state) +{ + smbios_memory_type memory_type[] = { + MEMORY_TYPE_DDR2, + MEMORY_TYPE_DDR3, + MEMORY_TYPE_DDR4, + MEMORY_TYPE_DDR5, + MEMORY_TYPE_LPDDR3, + MEMORY_TYPE_LPDDR4, + MEMORY_TYPE_LPDDR5, + }; + + for (int i = 0; i < ARRAY_SIZE(memory_type); i++) { + print_message("test_smbios_bus_width_to_spd_width_parametrized(%d)\n", + memory_type[i]); + test_smbios_bus_width_to_spd_width_parametrized(memory_type[i]); + } }
static void test_smbios_memory_size_to_mib(void **state)