Change in coreboot[master]: mainboard/pcengines/apu2/mainboard.c: fill SMBIOS type 16 and 17
Piotr Kleinschmidt has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35889 )
Change subject: mainboard/pcengines/apu2/mainboard.c: fill SMBIOS type 16 and 17 ......................................................................
mainboard/pcengines/apu2/mainboard.c: fill SMBIOS type 16 and 17
Change-Id: I24cd5abceabd11a079a8331e4b95cb69af8e94bf Signed-off-by: Piotr Kleinschmidt piotr.kleins@gmail.com --- M src/device/dram/Makefile.inc M src/lib/Makefile.inc M src/mainboard/pcengines/apu2/mainboard.c 3 files changed, 124 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/35889/1
diff --git a/src/device/dram/Makefile.inc b/src/device/dram/Makefile.inc index f397a53..d9df410 100644 --- a/src/device/dram/Makefile.inc +++ b/src/device/dram/Makefile.inc @@ -1 +1,2 @@ romstage-y += ddr4.c ddr3.c ddr2.c ddr_common.c +ramstage-y += ddr4.c ddr3.c ddr2.c ddr_common.c \ No newline at end of file diff --git a/src/lib/Makefile.inc b/src/lib/Makefile.inc index e5678ff..dc09851 100644 --- a/src/lib/Makefile.inc +++ b/src/lib/Makefile.inc @@ -328,6 +328,7 @@ endif # CONFIG_RAMSTAGE_LIBHWBASE
romstage-y += spd_bin.c +ramstage-y += spd_bin.c
ifeq ($(CONFIG_GENERIC_SPD_BIN),y) LIB_SPD_BIN = $(obj)/spd.bin diff --git a/src/mainboard/pcengines/apu2/mainboard.c b/src/mainboard/pcengines/apu2/mainboard.c index 682120b..1d91a39 100644 --- a/src/mainboard/pcengines/apu2/mainboard.c +++ b/src/mainboard/pcengines/apu2/mainboard.c @@ -27,6 +27,9 @@ #include <superio/nuvoton/nct5104d/nct5104d.h> #include <smbios.h> #include <string.h> +#include <cbmem.h> +#include <spd_bin.h> +#include <cbfs.h> #include "gpio_ftns.h"
#define SPD_SIZE 128 @@ -154,6 +157,81 @@ gpio->enabled = CONFIG(APU2_PINMUX_GPIO1); }
+static void set_dimm_info(uint8_t *spd, struct dimm_info *dimm) +{ + const int spd_capmb[8] = { 1, 2, 4, 8, 16, 32, 64, 0 }; + const int spd_ranks[8] = { 1, 2, 3, 4, -1, -1, -1, -1 }; + const int spd_devw[8] = { 4, 8, 16, 32, -1, -1, -1, -1 }; + const int spd_busw[8] = { 8, 16, 32, 64, -1, -1, -1, -1 }; + + int capmb = spd_capmb[spd[SPD_DENSITY_BANKS] & 7] * 256; + int ranks = spd_ranks[(spd[DDR3_ORGANIZATION] >> 3) & 7]; + int devw = spd_devw[spd[DDR3_ORGANIZATION] & 7]; + int busw = spd_busw[spd[DDR3_BUS_DEV_WIDTH] & 7]; + + switch (spd[12]) { + case 0x0a: + dimm->ddr_frequency = 1600; + break; + case 0x0c: + dimm->ddr_frequency = 1333; + break; + default: + dimm->ddr_frequency = 0; + break; + } + + dimm->ddr_type = MEMORY_TYPE_DDR3; + + /* Parse the SPD data to determine the DIMM information */ + + dimm->dimm_size = capmb / 8 * busw / devw * ranks; /* MiB */ + dimm->mod_type = spd[3] & 0xf; + dimm->module_part_number[0] = '\0'; + dimm->mod_id = *(uint16_t *)&spd[117]; + + switch (busw) { + default: + case 8: + dimm->bus_width = MEMORY_BUS_WIDTH_8; + break; + case 16: + dimm->bus_width = MEMORY_BUS_WIDTH_16; + break; + case 32: + dimm->bus_width = MEMORY_BUS_WIDTH_32; + break; + case 64: + dimm->bus_width = MEMORY_BUS_WIDTH_64; + break; + } + + if(spd[3]==0x08){ + dimm->bus_width |= BIOS_MEMORY_ECC_SINGLE_BIT_CORRECTING; + } +} + +static void mainboard_get_dimm_info(u8 *spd_buffer) +{ + struct dimm_info *dimm; + struct memory_info *mem_info; + + /* + * Allocate CBMEM area for DIMM information used to populate SMBIOS + * table 17 + */ + mem_info = cbmem_add(CBMEM_ID_MEMINFO, sizeof(*mem_info)); + printk(BIOS_DEBUG, "CBMEM entry for DIMM info: 0x%p\n", mem_info); + if (mem_info == NULL) + return; + memset(mem_info, 0, sizeof(*mem_info)); + + /* Describe the first channel memory */ + dimm = &mem_info->dimm[0]; + set_dimm_info(spd_buffer, dimm); + mem_info->dimm_cnt = 1; +} + /********************************************** * enable the dedicated function in mainboard. **********************************************/ @@ -164,6 +242,11 @@
config_gpio_mux();
+ u8 spd_index = get_spd_offset(); + u8 spd_buffer[CONFIG_DIMM_SPD_SIZE]; + + read_ddr3_spd_from_cbfs(spd_buffer, spd_index); + // // Enable the RTC output // @@ -176,6 +259,8 @@
/* Initialize the PIRQ data structures for consumption */ pirq_setup(); + + mainboard_get_dimm_info(spd_buffer); }
static void mainboard_final(void *chip_info) @@ -241,6 +326,43 @@ return sku; }
+int fill_mainboard_smbios_type16(unsigned long *current, int *handle) +{ + u8 spd_index = get_spd_offset(); + u8 spd_buffer[CONFIG_DIMM_SPD_SIZE]; + + if (read_ddr3_spd_from_cbfs(spd_buffer, spd_index) < 0) + return 0; + + struct smbios_type16 *t = (struct smbios_type16 *)*current; + int len = sizeof(struct smbios_type16) - 2; + memset(t, 0, sizeof(struct smbios_type16)); + + t->handle = *handle; + t->length = len; + t->type = SMBIOS_PHYS_MEMORY_ARRAY; + t->use = MEMORY_ARRAY_USE_SYSTEM; + t->location = MEMORY_ARRAY_LOCATION_SYSTEM_BOARD; + t->maximum_capacity = 4 * 1024 * 1024; // 4GB (in kB) due to board design + t->extended_maximum_capacity = 0; + t->memory_error_information_handle = 0xFFFE; + t->number_of_memory_devices = 1; // only 1 device soldered down to 1 channel + + switch(spd_buffer[3]){ + case 0x08: + t->memory_error_correction = MEMORY_ARRAY_ECC_MULTI_BIT; + break; + case 0x03: + t->memory_error_correction = MEMORY_ARRAY_ECC_NONE; + break; + default: + t->memory_error_correction = MEMORY_ARRAY_ECC_UNKNOWN; + break; + } + len += smbios_string_table_len(t->eos); + return len; +} + struct chip_operations mainboard_ops = { .enable_dev = mainboard_enable, .final = mainboard_final,
Piotr Kleinschmidt has removed Piotr Król from this change. ( https://review.coreboot.org/c/coreboot/+/35889 )
Change subject: mainboard/pcengines/apu2/mainboard.c: fill SMBIOS type 16 and 17 ......................................................................
Removed reviewer Piotr Król.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35889 )
Change subject: mainboard/pcengines/apu2/mainboard.c: fill SMBIOS type 16 and 17 ......................................................................
Patch Set 1:
(8 comments)
https://review.coreboot.org/c/coreboot/+/35889/1/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/mainboard.c:
https://review.coreboot.org/c/coreboot/+/35889/1/src/mainboard/pcengines/apu... PS1, Line 172: switch (spd[12]) { switch and case should be at the same indent
https://review.coreboot.org/c/coreboot/+/35889/1/src/mainboard/pcengines/apu... PS1, Line 209: if(spd[3]==0x08){ spaces required around that '==' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35889/1/src/mainboard/pcengines/apu... PS1, Line 209: if(spd[3]==0x08){ space required before the open brace '{'
https://review.coreboot.org/c/coreboot/+/35889/1/src/mainboard/pcengines/apu... PS1, Line 209: if(spd[3]==0x08){ space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/35889/1/src/mainboard/pcengines/apu... PS1, Line 209: if(spd[3]==0x08){ braces {} are not necessary for single statement blocks
https://review.coreboot.org/c/coreboot/+/35889/1/src/mainboard/pcengines/apu... PS1, Line 351: switch(spd_buffer[3]){ switch and case should be at the same indent
https://review.coreboot.org/c/coreboot/+/35889/1/src/mainboard/pcengines/apu... PS1, Line 351: switch(spd_buffer[3]){ space required before the open brace '{'
https://review.coreboot.org/c/coreboot/+/35889/1/src/mainboard/pcengines/apu... PS1, Line 351: switch(spd_buffer[3]){ space required before the open parenthesis '('
Piotr Kleinschmidt has removed Patrick Georgi from this change. ( https://review.coreboot.org/c/coreboot/+/35889 )
Change subject: mainboard/pcengines/apu2/mainboard.c: fill SMBIOS type 16 and 17 ......................................................................
Removed reviewer Patrick Georgi.
Hello Kyösti Mälkki, Patrick Rudolph, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35889
to look at the new patch set (#2).
Change subject: mainboard/pcengines/apu2/mainboard.c: fill SMBIOS type 16 and 17 ......................................................................
mainboard/pcengines/apu2/mainboard.c: fill SMBIOS type 16 and 17
Change-Id: I24cd5abceabd11a079a8331e4b95cb69af8e94bf Signed-off-by: Piotr Kleinschmidt piotr.kleins@gmail.com --- M src/device/dram/Makefile.inc M src/lib/Makefile.inc M src/mainboard/pcengines/apu2/mainboard.c 3 files changed, 123 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/35889/2
Piotr Kleinschmidt has removed Patrick Georgi from this change. ( https://review.coreboot.org/c/coreboot/+/35889 )
Change subject: mainboard/pcengines/apu2/mainboard.c: fill SMBIOS type 16 and 17 ......................................................................
Removed reviewer Patrick Georgi.
Piotr Kleinschmidt has removed Piotr Król from this change. ( https://review.coreboot.org/c/coreboot/+/35889 )
Change subject: mainboard/pcengines/apu2/mainboard.c: fill SMBIOS type 16 and 17 ......................................................................
Removed reviewer Piotr Król.
Piotr Kleinschmidt has removed Michał Żygowski from this change. ( https://review.coreboot.org/c/coreboot/+/35889 )
Change subject: mainboard/pcengines/apu2/mainboard.c: fill SMBIOS type 16 and 17 ......................................................................
Removed reviewer Michał Żygowski.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35889 )
Change subject: mainboard/pcengines/apu2/mainboard.c: fill SMBIOS type 16 and 17 ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35889/2/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/mainboard.c:
https://review.coreboot.org/c/coreboot/+/35889/2/src/mainboard/pcengines/apu... PS2, Line 160: static void set_dimm_info(uint8_t *spd, struct dimm_info *dimm) that's not board specific
https://review.coreboot.org/c/coreboot/+/35889/2/src/mainboard/pcengines/apu... PS2, Line 222: mem_info = cbmem_add(CBMEM_ID_MEMINFO, sizeof(*mem_info)); usually added in romstage by DRAM init code
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35889 )
Change subject: mainboard/pcengines/apu2/mainboard.c: fill SMBIOS type 16 and 17 ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35889/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35889/2//COMMIT_MSG@8 PS2, Line 8: Tested how? `dmidecode …`?
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35889 )
Change subject: mainboard/pcengines/apu2/mainboard.c: fill SMBIOS type 16 and 17 ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35889/2/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/mainboard.c:
https://review.coreboot.org/c/coreboot/+/35889/2/src/mainboard/pcengines/apu... PS2, Line 160: static void set_dimm_info(uint8_t *spd, struct dimm_info *dimm)
that's not board specific
Any ready functions that do the same?
https://review.coreboot.org/c/coreboot/+/35889/2/src/mainboard/pcengines/apu... PS2, Line 222: mem_info = cbmem_add(CBMEM_ID_MEMINFO, sizeof(*mem_info));
usually added in romstage by DRAM init code
Usually, yes. We do not do it, since hooking to AGESA memory data after RAM init is more complex and not worth it (AGESA destroys the structures right after the executed init stage). Here we avoid using CBMEM init romstage hooks and stashing the memory information in CAR to get the ID_MEMINFO populated. Since apu2 has soldered memory, parsing SPD is sufficient.
https://review.coreboot.org/c/coreboot/+/35889/2/src/mainboard/pcengines/apu... PS2, Line 345: t->maximum_capacity = 4 * 1024 * 1024; // 4GB (in kB) due to board design This should be based on SPD I think. Since the 2GB mainboard cannot have 4GB of memory.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35889 )
Change subject: mainboard/pcengines/apu2/mainboard.c: fill SMBIOS type 16 and 17 ......................................................................
Patch Set 2:
Piotr, could you abandon the change? I took a slightly different approach here CB:38342
Piotr Kleinschmidt has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35889 )
Change subject: mainboard/pcengines/apu2/mainboard.c: fill SMBIOS type 16 and 17 ......................................................................
Abandoned
Patch with a different approach is applied: https://review.coreboot.org/c/coreboot/+/38342/
-
build bot (Jenkins) (Code Review) -
Michał Żygowski (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Piotr Kleinschmidt (Code Review)