Change in coreboot[master]: mb/pcengines/apu1/mainboard.c: Add SMBIOS type 16 and 17 entries
Michał Żygowski has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38343 )
Change subject: mb/pcengines/apu1/mainboard.c: Add SMBIOS type 16 and 17 entries ......................................................................
mb/pcengines/apu1/mainboard.c: Add SMBIOS type 16 and 17 entries
Use information provided by AGESA to fill the SMBIOS memory tables.
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: Id73de7c2b23c6eb71722f1c78dbf0d246f429c63 --- M src/mainboard/pcengines/apu1/mainboard.c 1 file changed, 98 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/38343/1
diff --git a/src/mainboard/pcengines/apu1/mainboard.c b/src/mainboard/pcengines/apu1/mainboard.c index 854fb8a..d817d57 100644 --- a/src/mainboard/pcengines/apu1/mainboard.c +++ b/src/mainboard/pcengines/apu1/mainboard.c @@ -14,6 +14,8 @@ * GNU General Public License for more details. */
+#include <AGESA.h> +#include <AMD.h> #include <console/console.h> #include <device/device.h> #include <device/mmio.h> @@ -24,6 +26,7 @@ #include <string.h> #include <southbridge/amd/cimx/sb800/SBPLATFORM.h> #include <southbridge/amd/cimx/sb800/pci_devs.h> +#include <northbridge/amd/agesa/agesa_helper.h> #include <northbridge/amd/agesa/family14/pci_devs.h> #include <superio/nuvoton/nct5104d/nct5104d.h> #include "gpio_ftns.h" @@ -174,6 +177,98 @@ /********************************************** * Enable the dedicated functions of the board. **********************************************/ +#if CONFIG(GENERATE_SMBIOS_TABLES) +static int mainboard_smbios_type16(DMI_INFO *agesa_dmi, int *handle, + unsigned long *current) +{ + struct smbios_type16 *t; + u32 max_capacity; + int len = 0; + + t = (struct smbios_type16 *)*current; + len = sizeof(struct smbios_type16); + memset(t, 0, sizeof(struct smbios_type16)); + max_capacity = get_spd_offset() ? 4 : 2; /* 4GB or 2GB variant */ + + t->type = SMBIOS_PHYS_MEMORY_ARRAY; + t->handle = *handle; + t->length = len - 2; + t->type = SMBIOS_PHYS_MEMORY_ARRAY; + t->use = MEMORY_ARRAY_USE_SYSTEM; + t->location = MEMORY_ARRAY_LOCATION_SYSTEM_BOARD; + t->memory_error_correction = agesa_dmi->T16.MemoryErrorCorrection; + t->maximum_capacity = max_capacity * 1024 * 1024; + t->memory_error_information_handle = 0xfffe; + t->number_of_memory_devices = 1; + + *current += len; + + return len; +} + +static int mainboard_smbios_type17(DMI_INFO *agesa_dmi, int *handle, + unsigned long *current) +{ + struct smbios_type17 *t; + int len = 0; + + t = (struct smbios_type17 *)*current; + memset(t, 0, sizeof(struct smbios_type17)); + + t->type = SMBIOS_MEMORY_DEVICE; + t->length = sizeof(struct smbios_type17) - 2; + t->handle = *handle + 1; + t->phys_memory_array_handle = *handle; + t->memory_error_information_handle = 0xfffe; + t->total_width = agesa_dmi->T17[0][0][0].TotalWidth; + t->data_width = agesa_dmi->T17[0][0][0].DataWidth; + t->size = agesa_dmi->T17[0][0][0].MemorySize; + t->form_factor = agesa_dmi->T17[0][0][0].FormFactor; + t->device_set = agesa_dmi->T17[0][0][0].DeviceSet; + t->device_locator = smbios_add_string(t->eos, + agesa_dmi->T17[0][0][0].DeviceLocator); + t->bank_locator = smbios_add_string(t->eos, + agesa_dmi->T17[0][0][0].BankLocator); + t->memory_type = agesa_dmi->T17[0][0][0].MemoryType; + t->type_detail = *(u16 *)&agesa_dmi->T17[0][0][0].TypeDetail; + t->speed = agesa_dmi->T17[0][0][0].Speed; + t->manufacturer = agesa_dmi->T17[0][0][0].ManufacturerIdCode; + t->serial_number = smbios_add_string(t->eos, + agesa_dmi->T17[0][0][0].SerialNumber); + t->part_number = smbios_add_string(t->eos, + agesa_dmi->T17[0][0][0].PartNumber); + t->attributes = agesa_dmi->T17[0][0][0].Attributes; + t->extended_size = agesa_dmi->T17[0][0][0].ExtSize; + t->clock_speed = agesa_dmi->T17[0][0][0].ConfigSpeed; + t->minimum_voltage = 1500; /* From SPD: 1.5V */ + t->maximum_voltage = 1500; + + len = t->length + smbios_string_table_len(t->eos); + *current += len; + + return len; +} + +static int mainboard_smbios_data(struct device *dev, int *handle, + unsigned long *current) +{ + DMI_INFO *agesa_dmi; + int len = 0; + + agesa_dmi = agesawrapper_getlateinitptr(PICK_DMI); + + if (!agesa_dmi) + return len; + + len += mainboard_smbios_type16(agesa_dmi, handle, current); + len += mainboard_smbios_type17(agesa_dmi, handle, current); + + *handle += 2; + + return len; +} +#endif + static void mainboard_enable(struct device *dev) { printk(BIOS_INFO, "Mainboard " CONFIG_MAINBOARD_PART_NUMBER " Enable.\n"); @@ -195,6 +290,9 @@
/* Initialize the PIRQ data structures for consumption */ pirq_setup(); +#if CONFIG(GENERATE_SMBIOS_TABLES) + dev->ops->get_smbios_data = mainboard_smbios_data; +#endif }
/*
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38343 )
Change subject: mb/pcengines/apu1/mainboard.c: Add SMBIOS type 16 and 17 entries ......................................................................
Patch Set 2:
Sample output from 2GB variant: https://pastebin.com/MqRcrGkc
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38343 )
Change subject: mb/pcengines/apu1/mainboard.c: Add SMBIOS type 16 and 17 entries ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38343/1/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu1/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38343/1/src/mainboard/pcengines/apu... PS1, Line 186: int len = 0;
Doesn’t need to be initialized.
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38343 )
Change subject: mb/pcengines/apu1/mainboard.c: Add SMBIOS type 16 and 17 entries ......................................................................
Patch Set 3: Code-Review+1
(4 comments)
https://review.coreboot.org/c/coreboot/+/38343/3/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu1/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38343/3/src/mainboard/pcengines/apu... PS3, Line 183: unsigned long *current) Does this fit in 96 chars?
https://review.coreboot.org/c/coreboot/+/38343/3/src/mainboard/pcengines/apu... PS3, Line 211: unsigned long *current) Same here
https://review.coreboot.org/c/coreboot/+/38343/3/src/mainboard/pcengines/apu... PS3, Line 231: agesa_dmi->T17[0][0][0].DeviceLocator); And here
https://review.coreboot.org/c/coreboot/+/38343/3/src/mainboard/pcengines/apu... PS3, Line 255: unsigned long *current) This one definitely fits
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38343 )
Change subject: mb/pcengines/apu1/mainboard.c: Add SMBIOS type 16 and 17 entries ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38343/3/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu1/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38343/3/src/mainboard/pcengines/apu... PS3, Line 183: unsigned long *current)
Does this fit in 96 chars?
Still used to 80 chars. Can not switch my self to 96. Will fix it.
Hello build bot (Jenkins), Nico Huber, Patrick Rudolph, Angel Pons, Arthur Heymans, Kyösti Mälkki, Piotr Król,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38343
to look at the new patch set (#4).
Change subject: mb/pcengines/apu1/mainboard.c: Add SMBIOS type 16 and 17 entries ......................................................................
mb/pcengines/apu1/mainboard.c: Add SMBIOS type 16 and 17 entries
Use information provided by AGESA to fill the SMBIOS memory tables.
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: Id73de7c2b23c6eb71722f1c78dbf0d246f429c63 --- M src/mainboard/pcengines/apu1/buildOpts.c M src/mainboard/pcengines/apu1/mainboard.c 2 files changed, 94 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/38343/4
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38343 )
Change subject: mb/pcengines/apu1/mainboard.c: Add SMBIOS type 16 and 17 entries ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38343/3/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu1/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38343/3/src/mainboard/pcengines/apu... PS3, Line 183: unsigned long *current)
Still used to 80 chars. Can not switch my self to 96. Will fix it.
Done
https://review.coreboot.org/c/coreboot/+/38343/3/src/mainboard/pcengines/apu... PS3, Line 211: unsigned long *current)
Same here
Done
https://review.coreboot.org/c/coreboot/+/38343/3/src/mainboard/pcengines/apu... PS3, Line 231: agesa_dmi->T17[0][0][0].DeviceLocator);
And here
Done
https://review.coreboot.org/c/coreboot/+/38343/3/src/mainboard/pcengines/apu... PS3, Line 255: unsigned long *current)
This one definitely fits
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38343 )
Change subject: mb/pcengines/apu1/mainboard.c: Add SMBIOS type 16 and 17 entries ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/38343/3/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu1/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38343/3/src/mainboard/pcengines/apu... PS3, Line 255: unsigned long *current)
Done
Not really done, but I'm not going to bother you for only one instance
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38343 )
Change subject: mb/pcengines/apu1/mainboard.c: Add SMBIOS type 16 and 17 entries ......................................................................
mb/pcengines/apu1/mainboard.c: Add SMBIOS type 16 and 17 entries
Use information provided by AGESA to fill the SMBIOS memory tables.
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: Id73de7c2b23c6eb71722f1c78dbf0d246f429c63 Reviewed-on: https://review.coreboot.org/c/coreboot/+/38343 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/mainboard/pcengines/apu1/buildOpts.c M src/mainboard/pcengines/apu1/mainboard.c 2 files changed, 94 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/mainboard/pcengines/apu1/buildOpts.c b/src/mainboard/pcengines/apu1/buildOpts.c index 2b73fb1..f2e4ab5 100644 --- a/src/mainboard/pcengines/apu1/buildOpts.c +++ b/src/mainboard/pcengines/apu1/buildOpts.c @@ -87,7 +87,7 @@ #define BLDOPT_REMOVE_SRAT FALSE #define BLDOPT_REMOVE_SLIT FALSE #define BLDOPT_REMOVE_WHEA FALSE -#define BLDOPT_REMOVE_DMI TRUE +#define BLDOPT_REMOVE_DMI FALSE #define BLDOPT_REMOVE_HT_ASSIST TRUE #define BLDOPT_REMOVE_ATM_MODE TRUE //#define BLDOPT_REMOVE_MSG_BASED_C1E TRUE diff --git a/src/mainboard/pcengines/apu1/mainboard.c b/src/mainboard/pcengines/apu1/mainboard.c index 14eab8b..d72a8fb 100644 --- a/src/mainboard/pcengines/apu1/mainboard.c +++ b/src/mainboard/pcengines/apu1/mainboard.c @@ -13,6 +13,8 @@ */
#include <amdblocks/acpimmio.h> +#include <AGESA.h> +#include <AMD.h> #include <console/console.h> #include <device/device.h> #include <device/mmio.h> @@ -23,6 +25,7 @@ #include <string.h> #include <southbridge/amd/cimx/sb800/SBPLATFORM.h> #include <southbridge/amd/cimx/sb800/pci_devs.h> +#include <northbridge/amd/agesa/agesa_helper.h> #include <northbridge/amd/agesa/family14/pci_devs.h> #include <superio/nuvoton/nct5104d/nct5104d.h> #include "gpio_ftns.h" @@ -173,6 +176,93 @@ /********************************************** * Enable the dedicated functions of the board. **********************************************/ +#if CONFIG(GENERATE_SMBIOS_TABLES) +static int mainboard_smbios_type16(DMI_INFO *agesa_dmi, int *handle, unsigned long *current) +{ + struct smbios_type16 *t; + u32 max_capacity; + int len; + + t = (struct smbios_type16 *)*current; + len = sizeof(struct smbios_type16); + memset(t, 0, len); + max_capacity = get_spd_offset() ? 4 : 2; /* 4GB or 2GB variant */ + + t->type = SMBIOS_PHYS_MEMORY_ARRAY; + t->handle = *handle; + t->length = len - 2; + t->type = SMBIOS_PHYS_MEMORY_ARRAY; + t->use = MEMORY_ARRAY_USE_SYSTEM; + t->location = MEMORY_ARRAY_LOCATION_SYSTEM_BOARD; + t->memory_error_correction = agesa_dmi->T16.MemoryErrorCorrection; + t->maximum_capacity = max_capacity * 1024 * 1024; + t->memory_error_information_handle = 0xfffe; + t->number_of_memory_devices = 1; + + *current += len; + + return len; +} + +static int mainboard_smbios_type17(DMI_INFO *agesa_dmi, int *handle, unsigned long *current) +{ + struct smbios_type17 *t; + int len; + + t = (struct smbios_type17 *)*current; + memset(t, 0, sizeof(struct smbios_type17)); + + t->type = SMBIOS_MEMORY_DEVICE; + t->length = sizeof(struct smbios_type17) - 2; + t->handle = *handle + 1; + t->phys_memory_array_handle = *handle; + t->memory_error_information_handle = 0xfffe; + t->total_width = agesa_dmi->T17[0][0][0].TotalWidth; + t->data_width = agesa_dmi->T17[0][0][0].DataWidth; + t->size = agesa_dmi->T17[0][0][0].MemorySize; + /* AGESA DMI returns form factor = 0, override it with SPD value */ + t->form_factor = MEMORY_FORMFACTOR_SODIMM; + t->device_set = agesa_dmi->T17[0][0][0].DeviceSet; + t->device_locator = smbios_add_string(t->eos, agesa_dmi->T17[0][0][0].DeviceLocator); + t->bank_locator = smbios_add_string(t->eos, agesa_dmi->T17[0][0][0].BankLocator); + t->memory_type = agesa_dmi->T17[0][0][0].MemoryType; + t->type_detail = *(u16 *)&agesa_dmi->T17[0][0][0].TypeDetail; + t->speed = agesa_dmi->T17[0][0][0].Speed; + t->manufacturer = agesa_dmi->T17[0][0][0].ManufacturerIdCode; + t->serial_number = smbios_add_string(t->eos, agesa_dmi->T17[0][0][0].SerialNumber); + t->part_number = smbios_add_string(t->eos, agesa_dmi->T17[0][0][0].PartNumber); + t->attributes = agesa_dmi->T17[0][0][0].Attributes; + t->extended_size = agesa_dmi->T17[0][0][0].ExtSize; + t->clock_speed = agesa_dmi->T17[0][0][0].ConfigSpeed; + t->minimum_voltage = 1500; /* From SPD: 1.5V */ + t->maximum_voltage = 1500; + + len = t->length + smbios_string_table_len(t->eos); + *current += len; + + return len; +} + +static int mainboard_smbios_data(struct device *dev, int *handle, + unsigned long *current) +{ + DMI_INFO *agesa_dmi; + int len; + + agesa_dmi = agesawrapper_getlateinitptr(PICK_DMI); + + if (!agesa_dmi) + return 0; + + len = mainboard_smbios_type16(agesa_dmi, handle, current); + len += mainboard_smbios_type17(agesa_dmi, handle, current); + + *handle += 2; + + return len; +} +#endif + static void mainboard_enable(struct device *dev) { printk(BIOS_INFO, "Mainboard " CONFIG_MAINBOARD_PART_NUMBER " Enable.\n"); @@ -194,6 +284,9 @@
/* Initialize the PIRQ data structures for consumption */ pirq_setup(); +#if CONFIG(GENERATE_SMBIOS_TABLES) + dev->ops->get_smbios_data = mainboard_smbios_data; +#endif }
/*
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38343 )
Change subject: mb/pcengines/apu1/mainboard.c: Add SMBIOS type 16 and 17 entries ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38343/3/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu1/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38343/3/src/mainboard/pcengines/apu... PS3, Line 255: unsigned long *current)
Not really done, but I'm not going to bother you for only one instance
Really? You have not been looking at the latest patchset 😊 In the patchset 5 it was fixed.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38343 )
Change subject: mb/pcengines/apu1/mainboard.c: Add SMBIOS type 16 and 17 entries ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38343/4/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu1/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38343/4/src/mainboard/pcengines/apu... PS4, Line 246: struct device *dev, int *handle, : unsigned long *current Hrm
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38343 )
Change subject: mb/pcengines/apu1/mainboard.c: Add SMBIOS type 16 and 17 entries ......................................................................
Patch Set 5:
Automatic boot test returned (PASS/FAIL/TOTAL): 5/0/5 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1598 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1597 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1596 Non-emulation targets: HP_COMPAQ_8200_ELITE_SFF_PC using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1600 HP_COMPAQ_8200_ELITE_SFF_PC using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1599
Please note: This test is under development and might not be accurate at all!
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38343 )
Change subject: mb/pcengines/apu1/mainboard.c: Add SMBIOS type 16 and 17 entries ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38343/4/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu1/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38343/4/src/mainboard/pcengines/apu... PS4, Line 246: struct device *dev, int *handle, : unsigned long *current
Hrm
Ohh yeah... I'm blind...
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Michał Żygowski (Code Review)
-
Patrick Georgi (Code Review)