Change in coreboot[master]: src/arch/x86/smbios.c: add SMBIOS type 16 structure

[Piotr Kleinschmidt (Code Review)]

Piotr Kleinschmidt has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35888 )

Change subject: src/arch/x86/smbios.c: add SMBIOS type 16 structure
......................................................................

src/arch/x86/smbios.c: add SMBIOS type 16 structure

Change-Id: I590ebf89ecb3f97a9ce028ca34696e5809d66d49
Signed-off-by: Piotr Kleinschmidt 
---
M src/arch/x86/smbios.c
M src/include/smbios.h
2 files changed, 18 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/35888/1

diff --git a/src/arch/x86/smbios.c b/src/arch/x86/smbios.c
index 4eb8726..4e8a794 100644
--- a/src/arch/x86/smbios.c
+++ b/src/arch/x86/smbios.c
@@ -527,6 +527,11 @@
    return "";
 }

+int __weak fill_mainboard_smbios_type16(unsigned long *current, int *handle)
+{
+	return 0;
+}
+
 static int get_socket_type(void)
 {
    if (CONFIG(CPU_INTEL_SLOT_1))
@@ -938,6 +943,16 @@
    return len;
 }

+static int smbios_write_type16(unsigned long *current, int *handle)
+{
+	int len = fill_mainboard_smbios_type16(current, handle);
+	if(len){
+		*current += len;
+		(*handle)++;
+	}
+	return len;
+}
+
 static int smbios_write_type17(unsigned long *current, int *handle)
 {
    int len = sizeof(struct smbios_type17);
@@ -1176,6 +1191,8 @@
    if (CONFIG(ELOG))
    	update_max(len, max_struct_size,
    		elog_smbios_write_type15(&current,handle++));
+	update_max(len, max_struct_size, smbios_write_type16(&current,
+		&handle));
    update_max(len, max_struct_size, smbios_write_type17(&current,
    	&handle));
    update_max(len, max_struct_size, smbios_write_type32(&current,
diff --git a/src/include/smbios.h b/src/include/smbios.h
index 0bba0a7..e808177 100644
--- a/src/include/smbios.h
+++ b/src/include/smbios.h
@@ -52,6 +52,7 @@
 const char *smbios_system_version(void);
 void smbios_system_set_uuid(u8 *uuid);
 const char *smbios_system_sku(void);
+int fill_mainboard_smbios_type16(unsigned long *current, int *handle);

const char *smbios_mainboard_manufacturer(void);
 const char *smbios_mainboard_product_name(void);

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35888 )

Change subject: src/arch/x86/smbios.c: add SMBIOS type 16 structure
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/35888/1/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/c/coreboot/+/35888/1/src/arch/x86/smbios.c@949 
PS1, Line 949: 	if(len){
space required before the open brace '{'

https://review.coreboot.org/c/coreboot/+/35888/1/src/arch/x86/smbios.c@949 
PS1, Line 949: 	if(len){
space required before the open parenthesis '('

[Piotr Kleinschmidt (Code Review)]

Hello Patrick Rudolph,

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

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

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

Change subject: src/arch/x86/smbios.c: add SMBIOS type 16 structure
......................................................................

src/arch/x86/smbios.c: add SMBIOS type 16 structure

Change-Id: I590ebf89ecb3f97a9ce028ca34696e5809d66d49
Signed-off-by: Piotr Kleinschmidt 
---
M src/arch/x86/smbios.c
M src/include/smbios.h
2 files changed, 18 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/35888/2

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35888 )

Change subject: src/arch/x86/smbios.c: add SMBIOS type 16 structure
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35888/2/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/c/coreboot/+/35888/2/src/arch/x86/smbios.c@530 
PS2, Line 530: int __weak fill_mainboard_smbios_type16(unsigned long *current, int *handle)
that might collide with the type16 installed by some north-bridges. Not the case on APU2, but as soon as it is in place.
How do you plan to prevent a duplicated entry on those boards/future boards?

[Piotr Kleinschmidt (Code Review)]

Piotr Kleinschmidt has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35888 )

Change subject: src/arch/x86/smbios.c: add SMBIOS type 16 structure
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35888/2/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/c/coreboot/+/35888/2/src/arch/x86/smbios.c@530 
PS2, Line 530: int __weak fill_mainboard_smbios_type16(unsigned long *current, int *handle)
> that might collide with the type16 installed by some north-bridges. […]

Do you suggest to use .get_smbios_data from device_operations struct, as it is done in e.g. amd/amdfam10/northbridge?

[Paul Menzel (Code Review)]

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

Change subject: src/arch/x86/smbios.c: add SMBIOS type 16 structure
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35888/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35888/2//COMMIT_MSG@7 
PS2, Line 7: src/arch/x86/smbios.c: add SMBIOS type 16 structure
Please remove `src`. The prefix does not need to be the full path.

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35888 )

Change subject: src/arch/x86/smbios.c: add SMBIOS type 16 structure
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35888/2/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/c/coreboot/+/35888/2/src/arch/x86/smbios.c@530 
PS2, Line 530: int __weak fill_mainboard_smbios_type16(unsigned long *current, int *handle)
> Do you suggest to use .get_smbios_data from device_operations struct, as it is done in e.g. […]

That'd be great.

Change in coreboot[master]: mb/pcengines/apu2/mainboard.c: create SMBIOS type 16

[Piotr Kleinschmidt (Code Review)]

Hello Kyösti Mälkki, Patrick Rudolph, build bot (Jenkins),

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

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

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

Change subject: mb/pcengines/apu2/mainboard.c: create SMBIOS type 16
......................................................................

mb/pcengines/apu2/mainboard.c: create SMBIOS type 16

Tested with dmidecode in Linux debian 4.19.0-6-amd64

Change-Id: I590ebf89ecb3f97a9ce028ca34696e5809d66d49
Signed-off-by: Piotr Kleinschmidt 
---
M src/device/dram/Makefile.inc
M src/lib/Makefile.inc
M src/mainboard/pcengines/apu2/mainboard.c
3 files changed, 68 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/35888/3

Change in coreboot[master]: mb/pcengines/apu2/mainboard.c: create SMBIOS type 16

[Piotr Kleinschmidt (Code Review)]

Piotr Kleinschmidt has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35888 )

Change subject: mb/pcengines/apu2/mainboard.c: create SMBIOS type 16
......................................................................

Patch Set 3:

(1 comment)

This change is ready for review.

https://review.coreboot.org/c/coreboot/+/35888/3/src/mainboard/pcengines/apu... 
File src/mainboard/pcengines/apu2/mainboard.c:

https://review.coreboot.org/c/coreboot/+/35888/3/src/mainboard/pcengines/apu... 
PS3, Line 200: 	switch (spd_buffer[3]) {
> that seems complex. […]

At this point CBMEM_ID_MEMINFO is not presented in cbmem, so SPD is good way to read memory information. Indeed, in patch 35889 (SMBIOS type 17 for apu) I fill cbmem with memory info (previously read with SPD), but I wonder if it is not unnecessary duplication since that data is available with SPD.

Change in coreboot[master]: mb/pcengines/apu2/mainboard.c: create SMBIOS type 16

[Piotr Kleinschmidt (Code Review)]

Piotr Kleinschmidt has removed Patrick Rudolph from this change.  ( https://review.coreboot.org/c/coreboot/+/35888 )

Change subject: mb/pcengines/apu2/mainboard.c: create SMBIOS type 16
......................................................................

Removed reviewer Patrick Rudolph.

Change in coreboot[master]: mb/pcengines/apu2/mainboard.c: create SMBIOS type 16

[Piotr Kleinschmidt (Code Review)]

Piotr Kleinschmidt has removed Patrick Georgi from this change.  ( https://review.coreboot.org/c/coreboot/+/35888 )

Change subject: mb/pcengines/apu2/mainboard.c: create SMBIOS type 16
......................................................................

Removed reviewer Patrick Georgi.

Change in coreboot[master]: mb/pcengines/apu2/mainboard.c: create SMBIOS type 16

[Piotr Kleinschmidt (Code Review)]

Piotr Kleinschmidt has removed Piotr Król from this change.  ( https://review.coreboot.org/c/coreboot/+/35888 )

Change subject: mb/pcengines/apu2/mainboard.c: create SMBIOS type 16
......................................................................

Removed reviewer Piotr Król.

Change in coreboot[master]: mb/pcengines/apu2/mainboard.c: create SMBIOS type 16

[Piotr Kleinschmidt (Code Review)]

Piotr Kleinschmidt has removed Martin Roth from this change.  ( https://review.coreboot.org/c/coreboot/+/35888 )

Change subject: mb/pcengines/apu2/mainboard.c: create SMBIOS type 16
......................................................................

Removed reviewer Martin Roth.

Change in coreboot[master]: mb/pcengines/apu2/mainboard.c: create SMBIOS type 16

[Piotr Kleinschmidt (Code Review)]

Hello Kyösti Mälkki, Patrick Rudolph, Paul Menzel, build bot (Jenkins), Michał Żygowski,

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

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

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

Change subject: mb/pcengines/apu2/mainboard.c: create SMBIOS type 16
......................................................................

mb/pcengines/apu2/mainboard.c: create SMBIOS type 16

Tested with dmidecode in Linux debian 4.19.0-6-amd64
on PC Engines apu4 platform

Change-Id: I590ebf89ecb3f97a9ce028ca34696e5809d66d49
Signed-off-by: Piotr Kleinschmidt 
---
M src/device/dram/Makefile.inc
M src/lib/Makefile.inc
M src/mainboard/pcengines/apu2/mainboard.c
3 files changed, 68 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/35888/4

Change in coreboot[master]: mb/pcengines/apu2/mainboard.c: create SMBIOS type 16

[Michał Żygowski (Code Review)]

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35888 )

Change subject: mb/pcengines/apu2/mainboard.c: create SMBIOS type 16
......................................................................

Patch Set 4:

Piotr, could you abandon the change? I took a slightly different approach here CB:38342

Change in coreboot[master]: mb/pcengines/apu2/mainboard.c: create SMBIOS type 16

[Piotr Kleinschmidt (Code Review)]

Piotr Kleinschmidt has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35888 )

Change subject: mb/pcengines/apu2/mainboard.c: create SMBIOS type 16
......................................................................

Abandoned

Patch with a different approach is applied: https://review.coreboot.org/c/coreboot/+/38342/