Change in coreboot[master]: drivers/vpd: Add support to read device serial from VPD

[Matt DeVillier (Code Review)]

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

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

drivers/vpd: Add support to read device serial from VPD

Add functions to read the system and mainboard serial numbers
from VPD tables stored in flash.

Test: build/boot google/akemi with RO_VPD region persisted from
stock Google firmware, verify system/mainboard serial numbers
present via dmidecode.

Change-Id: I14ae07cd8b764e1e22d58577c7cc697ca1496bd5
Signed-off-by: Matt DeVillier 
---
M src/drivers/vpd/Kconfig
M src/drivers/vpd/vpd.c
2 files changed, 28 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/49050/1

diff --git a/src/drivers/vpd/Kconfig b/src/drivers/vpd/Kconfig
index 0b16058..6bfbf4f 100644
--- a/src/drivers/vpd/Kconfig
+++ b/src/drivers/vpd/Kconfig
@@ -19,3 +19,8 @@
    default 0x4000
    help
      Size in bytes of the FMAP region created to store VPD tables.
+
+config SMBIOS_SERIAL_FROM_VPD
+	bool "Load device serial from VPD"
+	depends on VPD && GENERATE_SMBIOS_TABLES
+	default y
diff --git a/src/drivers/vpd/vpd.c b/src/drivers/vpd/vpd.c
index d3ff370..14dad8a 100644
--- a/src/drivers/vpd/vpd.c
+++ b/src/drivers/vpd/vpd.c
@@ -6,6 +6,7 @@
 #include <ctype.h>
 #include <fmap.h>
 #include <program_loading.h>
+#include <smbios.h>
 #include <string.h>
 #include <timestamp.h>
 #include <types.h>
@@ -297,4 +298,26 @@
    return true;
 }

+#if CONFIG(SMBIOS_SERIAL_FROM_VPD)
+#define VPD_KEY_SYSTEM_SERIAL		"serial_number"
+#define VPD_KEY_MAINBOARD_SERIAL	"mlb_serial_number"
+#define VPD_SERIAL_LEN			64
+
+const char *smbios_system_serial_number(void)
+{
+	static char serial[VPD_SERIAL_LEN];
+	if (vpd_gets(VPD_KEY_SYSTEM_SERIAL, serial, VPD_SERIAL_LEN, VPD_RO))
+		return serial;
+	return "";
+}
+
+const char *smbios_mainboard_serial_number(void)
+{
+	static char serial[VPD_SERIAL_LEN];
+	if (vpd_gets(VPD_KEY_MAINBOARD_SERIAL, serial, VPD_SERIAL_LEN, VPD_RO))
+		return serial;
+	return "";
+}
+#endif
+
 ROMSTAGE_CBMEM_INIT_HOOK(cbmem_add_cros_vpd)

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49050 )

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

Patch Set 1:

(3 comments)

https://review.coreboot.org/c/coreboot/+/49050/1/src/drivers/vpd/vpd.c 
File src/drivers/vpd/vpd.c:

https://review.coreboot.org/c/coreboot/+/49050/1/src/drivers/vpd/vpd.c@303 
PS1, Line 303: mlb
is that a typo or is that a name from some spec?

https://review.coreboot.org/c/coreboot/+/49050/1/src/drivers/vpd/vpd.c@308 
PS1, Line 308: static
why static?

https://review.coreboot.org/c/coreboot/+/49050/1/src/drivers/vpd/vpd.c@306 
PS1, Line 306: har *smbios_system_serial_number(void)
             : {
             : 	static char serial[VPD_SERIAL_LEN];
             : 	if (vpd_gets(VPD_KEY_SYSTEM_SERIAL, serial, VPD_SERIAL_LEN, VPD_RO))
             : 		return serial;
             : 	return "";
             : }
             : 
             : const char *smbios_mainboard_serial_number(void)
             : {
             : 	static char serial[VPD_SERIAL_LEN];
             : 	if (vpd_gets(VPD_KEY_MAINBOARD_SERIAL, serial, VPD_SERIAL_LEN, VPD_RO))
             : 		return serial;
             : 	return "";
             : }
             : #endif
shouldn't that go to smbios and make use of vpd functionality from there? (also the Kconfig)

[Matt DeVillier (Code Review)]

Hello build bot (Jenkins), Nico Huber, Patrick Georgi, Paul Menzel, Angel Pons, Michael Niewöhner,

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

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

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

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

drivers/vpd: Add support to read device serial from VPD

Add functions to read the system and mainboard serial numbers
from VPD tables stored in flash.

Test: build/boot google/akemi with RO_VPD region persisted from
stock Google firmware, verify system/mainboard serial numbers
present via dmidecode.

Change-Id: I14ae07cd8b764e1e22d58577c7cc697ca1496bd5
Signed-off-by: Matt DeVillier 
---
M src/drivers/vpd/Kconfig
M src/drivers/vpd/vpd.c
2 files changed, 28 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/49050/2

[Matt DeVillier (Code Review)]

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

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/c/coreboot/+/49050/1/src/drivers/vpd/vpd.c 
File src/drivers/vpd/vpd.c:

https://review.coreboot.org/c/coreboot/+/49050/1/src/drivers/vpd/vpd.c@303 
PS1, Line 303: mlb
> is that a typo or is that a name from some spec?

from VPD 2.0 spec

https://review.coreboot.org/c/coreboot/+/49050/1/src/drivers/vpd/vpd.c@308 
PS1, Line 308: static
> why static?

good question. I just cribbed this from google/sarien

https://review.coreboot.org/c/coreboot/+/49050/1/src/drivers/vpd/vpd.c@306 
PS1, Line 306: har *smbios_system_serial_number(void)
             : {
             : 	static char serial[VPD_SERIAL_LEN];
             : 	if (vpd_gets(VPD_KEY_SYSTEM_SERIAL, serial, VPD_SERIAL_LEN, VPD_RO))
             : 		return serial;
             : 	return "";
             : }
             : 
             : const char *smbios_mainboard_serial_number(void)
             : {
             : 	static char serial[VPD_SERIAL_LEN];
             : 	if (vpd_gets(VPD_KEY_MAINBOARD_SERIAL, serial, VPD_SERIAL_LEN, VPD_RO))
             : 		return serial;
             : 	return "";
             : }
             : #endif
> shouldn't that go to smbios and make use of vpd functionality from there? (also the Kconfig)

IDK, should it? seems like these methods are located in the overriding entity, so would make sense to have them in vpd driver

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49050 )

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/49050/2/src/drivers/vpd/vpd.c 
File src/drivers/vpd/vpd.c:

https://review.coreboot.org/c/coreboot/+/49050/2/src/drivers/vpd/vpd.c@301 
PS2, Line 301: #if CONFIG(SMBIOS_SERIAL_FROM_VPD)
Does the code need to be in this compilation unit? If not, the guard could
be placed in the Makefile.inc proper.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49050 )

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/49050/1/src/drivers/vpd/vpd.c 
File src/drivers/vpd/vpd.c:

https://review.coreboot.org/c/coreboot/+/49050/1/src/drivers/vpd/vpd.c@306 
PS1, Line 306: har *smbios_system_serial_number(void)
             : {
             : 	static char serial[VPD_SERIAL_LEN];
             : 	if (vpd_gets(VPD_KEY_SYSTEM_SERIAL, serial, VPD_SERIAL_LEN, VPD_RO))
             : 		return serial;
             : 	return "";
             : }
             : 
             : const char *smbios_mainboard_serial_number(void)
             : {
             : 	static char serial[VPD_SERIAL_LEN];
             : 	if (vpd_gets(VPD_KEY_MAINBOARD_SERIAL, serial, VPD_SERIAL_LEN, VPD_RO))
             : 		return serial;
             : 	return "";
             : }
             : #endif
> IDK, should it? seems like these methods are located in the overriding entity, so would make sense t […]

Seems right to have the VPD backed override in `vpd/`. Probably not in this
file, though, see other comment.

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49050 )

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/c/coreboot/+/49050/1/src/drivers/vpd/vpd.c 
File src/drivers/vpd/vpd.c:

https://review.coreboot.org/c/coreboot/+/49050/1/src/drivers/vpd/vpd.c@303 
PS1, Line 303: mlb
> from VPD 2. […]

ack

https://review.coreboot.org/c/coreboot/+/49050/1/src/drivers/vpd/vpd.c@308 
PS1, Line 308: static
> good question. […]

ok, can be dropped then I guess. at least I don't see any reason to make it static; same below

https://review.coreboot.org/c/coreboot/+/49050/1/src/drivers/vpd/vpd.c@306 
PS1, Line 306: har *smbios_system_serial_number(void)
             : {
             : 	static char serial[VPD_SERIAL_LEN];
             : 	if (vpd_gets(VPD_KEY_SYSTEM_SERIAL, serial, VPD_SERIAL_LEN, VPD_RO))
             : 		return serial;
             : 	return "";
             : }
             : 
             : const char *smbios_mainboard_serial_number(void)
             : {
             : 	static char serial[VPD_SERIAL_LEN];
             : 	if (vpd_gets(VPD_KEY_MAINBOARD_SERIAL, serial, VPD_SERIAL_LEN, VPD_RO))
             : 		return serial;
             : 	return "";
             : }
             : #endif
> Seems right to have the VPD backed override in `vpd/`. Probably not in this […]

ack

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49050 )

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/49050/1/src/drivers/vpd/vpd.c 
File src/drivers/vpd/vpd.c:

https://review.coreboot.org/c/coreboot/+/49050/1/src/drivers/vpd/vpd.c@306 
PS1, Line 306: har *smbios_system_serial_number(void)
             : {
             : 	static char serial[VPD_SERIAL_LEN];
             : 	if (vpd_gets(VPD_KEY_SYSTEM_SERIAL, serial, VPD_SERIAL_LEN, VPD_RO))
             : 		return serial;
             : 	return "";
             : }
             : 
             : const char *smbios_mainboard_serial_number(void)
             : {
             : 	static char serial[VPD_SERIAL_LEN];
             : 	if (vpd_gets(VPD_KEY_MAINBOARD_SERIAL, serial, VPD_SERIAL_LEN, VPD_RO))
             : 		return serial;
             : 	return "";
             : }
             : #endif
> ack

ah, I missed that it's defined weak in smbios code, nvm then

[Matt DeVillier (Code Review)]

Hello build bot (Jenkins), Nico Huber, Patrick Georgi, Paul Menzel, Angel Pons, Michael Niewöhner,

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

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

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

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

drivers/vpd: Add support to read device serial from VPD

Add functions to read the system and mainboard serial numbers
from VPD tables stored in flash. Select by default when VPD driver selected.

Disable for boards which use VPD but have separate overrides
for reading the SMBIOS serial number.

Remove board-specific implementations for google/drallion and
google/sarien and select the new Kconfig instead.

Test: build/boot google/akemi with RO_VPD region persisted from
stock Google firmware, verify system/mainboard serial numbers
present via dmidecode.

Change-Id: I14ae07cd8b764e1e22d58577c7cc697ca1496bd5
Signed-off-by: Matt DeVillier 
---
M src/drivers/vpd/Kconfig
M src/drivers/vpd/Makefile.inc
A src/drivers/vpd/vpd_serial.c
M src/mainboard/facebook/monolith/Kconfig
M src/mainboard/google/drallion/Kconfig
M src/mainboard/google/drallion/ramstage.c
M src/mainboard/google/sarien/Kconfig
M src/mainboard/google/sarien/ramstage.c
M src/mainboard/ocp/deltalake/Kconfig
M src/mainboard/ocp/tiogapass/Kconfig
10 files changed, 43 insertions(+), 42 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/49050/3

[Angel Pons (Code Review)]

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

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/49050/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/49050/3//COMMIT_MSG@10 
PS3, Line 10: Select by default when VPD driver selected.
This sentence is odd. Select what? It also goes beyond the 72-char limit.

https://review.coreboot.org/c/coreboot/+/49050/3/src/drivers/vpd/Kconfig 
File src/drivers/vpd/Kconfig:

https://review.coreboot.org/c/coreboot/+/49050/3/src/drivers/vpd/Kconfig@26 
PS3, Line 26: 	default y if !CHROMEOS
What's the point of adding this default? I feel it unnecessarily makes things more complex.

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49050 )

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

Patch Set 3:

(4 comments)

https://review.coreboot.org/c/coreboot/+/49050/1/src/drivers/vpd/vpd.c 
File src/drivers/vpd/vpd.c:

https://review.coreboot.org/c/coreboot/+/49050/1/src/drivers/vpd/vpd.c@308 
PS1, Line 308: static
> ok, can be dropped then I guess. […]

without static you're returning a pointer to that string but the variable is local. that won't work ofc. that's why vpd_gets uses that buffer :D my fault, sry ;)

https://review.coreboot.org/c/coreboot/+/49050/1/src/drivers/vpd/vpd.c@306 
PS1, Line 306: har *smbios_system_serial_number(void)
             : {
             : 	static char serial[VPD_SERIAL_LEN];
             : 	if (vpd_gets(VPD_KEY_SYSTEM_SERIAL, serial, VPD_SERIAL_LEN, VPD_RO))
             : 		return serial;
             : 	return "";
             : }
             : 
             : const char *smbios_mainboard_serial_number(void)
             : {
             : 	static char serial[VPD_SERIAL_LEN];
             : 	if (vpd_gets(VPD_KEY_MAINBOARD_SERIAL, serial, VPD_SERIAL_LEN, VPD_RO))
             : 		return serial;
             : 	return "";
             : }
             : #endif
> ah, I missed that it's defined weak in smbios code, nvm then

Done

https://review.coreboot.org/c/coreboot/+/49050/2/src/drivers/vpd/vpd.c 
File src/drivers/vpd/vpd.c:

https://review.coreboot.org/c/coreboot/+/49050/2/src/drivers/vpd/vpd.c@301 
PS2, Line 301: #if CONFIG(SMBIOS_SERIAL_FROM_VPD)
> Does the code need to be in this compilation unit? If not, the guard could […]

Done

https://review.coreboot.org/c/coreboot/+/49050/3/src/mainboard/google/dralli... 
File src/mainboard/google/drallion/ramstage.c:

https://review.coreboot.org/c/coreboot/+/49050/3/src/mainboard/google/dralli... 
PS3, Line 9: #include <vendorcode/google/chromeos/chromeos.h>
shouldn't be needed anymore

[Matt DeVillier (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Frans Hendriks, Jonathan Zhang, Paul Menzel, Angel Pons, Michael Niewöhner, Anjaneya "Reddy" Chagam, Nico Huber, Martin Roth, Johnny Lin, Morgan Jang, Wim Vervoorn,

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

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

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

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

drivers/vpd: Add support to read device serial from VPD

Add functions to read the system and mainboard serial numbers
from VPD tables stored in flash.

Remove board-specific implementations for google/drallion and
google/sarien and select the new Kconfig instead.

Test: build/boot google/akemi with RO_VPD region persisted from
stock Google firmware, verify system/mainboard serial numbers
present via dmidecode.

Change-Id: I14ae07cd8b764e1e22d58577c7cc697ca1496bd5
Signed-off-by: Matt DeVillier 
---
M src/drivers/vpd/Kconfig
M src/drivers/vpd/Makefile.inc
A src/drivers/vpd/vpd_serial.c
M src/mainboard/facebook/monolith/Kconfig
M src/mainboard/google/drallion/Kconfig
M src/mainboard/google/drallion/ramstage.c
M src/mainboard/google/sarien/Kconfig
M src/mainboard/google/sarien/ramstage.c
8 files changed, 37 insertions(+), 42 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/49050/4

[Matt DeVillier (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Frans Hendriks, Jonathan Zhang, Paul Menzel, Angel Pons, Michael Niewöhner, Anjaneya "Reddy" Chagam, Nico Huber, Martin Roth, Johnny Lin, Morgan Jang, Wim Vervoorn,

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

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

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

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

drivers/vpd: Add support to read device serial from VPD

Add functions to read the system and mainboard serial numbers
from VPD tables stored in flash.

Remove board-specific implementations for google/drallion and
google/sarien and select the new Kconfig instead.

Test: build/boot google/akemi with RO_VPD region persisted from
stock Google firmware, verify system/mainboard serial numbers
present via dmidecode.

Change-Id: I14ae07cd8b764e1e22d58577c7cc697ca1496bd5
Signed-off-by: Matt DeVillier 
---
M src/drivers/vpd/Kconfig
M src/drivers/vpd/Makefile.inc
A src/drivers/vpd/vpd_serial.c
M src/mainboard/google/drallion/Kconfig
M src/mainboard/google/drallion/ramstage.c
M src/mainboard/google/sarien/Kconfig
M src/mainboard/google/sarien/ramstage.c
7 files changed, 34 insertions(+), 42 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/49050/5

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49050 )

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

Patch Set 5: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/49050/3/src/drivers/vpd/Kconfig 
File src/drivers/vpd/Kconfig:

https://review.coreboot.org/c/coreboot/+/49050/3/src/drivers/vpd/Kconfig@26 
PS3, Line 26: 	default y if !CHROMEOS
> What's the point of adding this default? I feel it unnecessarily makes things more complex.

Done

[Angel Pons (Code Review)]

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

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

Patch Set 5: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/49050/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/49050/3//COMMIT_MSG@10 
PS3, Line 10: Select by default when VPD driver selected.
> This sentence is odd. Select what? It also goes beyond the 72-char limit.

Done

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49050 )

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

Patch Set 5: Code-Review+2

[Angel Pons (Code Review)]

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

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

Patch Set 5: Code-Review-1

This changes the behavior for !CHROMEOS builds

[Matt DeVillier (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Frans Hendriks, Jonathan Zhang, Paul Menzel, Angel Pons, Michael Niewöhner, Anjaneya "Reddy" Chagam, Nico Huber, Martin Roth, Johnny Lin, Morgan Jang, Wim Vervoorn,

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

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

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

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

drivers/vpd: Add support to read device serial from VPD

Add functions to read the system and mainboard serial numbers
from VPD tables stored in flash.

Remove board-specific implementations for google/drallion and
google/sarien and select the new Kconfig instead.

Test: build/boot google/akemi with RO_VPD region persisted from
stock Google firmware, verify system/mainboard serial numbers
present via dmidecode.

Change-Id: I14ae07cd8b764e1e22d58577c7cc697ca1496bd5
Signed-off-by: Matt DeVillier 
---
M src/drivers/vpd/Kconfig
M src/drivers/vpd/Makefile.inc
A src/drivers/vpd/vpd_serial.c
M src/mainboard/google/drallion/Kconfig
M src/mainboard/google/drallion/ramstage.c
M src/mainboard/google/sarien/Kconfig
M src/mainboard/google/sarien/ramstage.c
7 files changed, 34 insertions(+), 42 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/49050/6

[Matt DeVillier (Code Review)]

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

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

Patch Set 6:
> Patch Set 5: Code-Review-1
>
> This changes the behavior for !CHROMEOS builds

how so?

[Angel Pons (Code Review)]

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

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

Patch Set 6: Code-Review+2

[Matt DeVillier (Code Review)]

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

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/49050/3/src/mainboard/google/dralli... 
File src/mainboard/google/drallion/ramstage.c:

https://review.coreboot.org/c/coreboot/+/49050/3/src/mainboard/google/dralli... 
PS3, Line 9: #include <vendorcode/google/chromeos/chromeos.h>
> shouldn't be needed anymore

I agree, but it wasn't needed before this change either, so it's out of scope to remove it here

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49050 )

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

Patch Set 6: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/49050/3/src/mainboard/google/dralli... 
File src/mainboard/google/drallion/ramstage.c:

https://review.coreboot.org/c/coreboot/+/49050/3/src/mainboard/google/dralli... 
PS3, Line 9: #include <vendorcode/google/chromeos/chromeos.h>
> I agree, but it wasn't needed before this change either, so it's out of scope to remove it here

Done

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/49050 )

Change subject: drivers/vpd: Add support to read device serial from VPD
......................................................................

drivers/vpd: Add support to read device serial from VPD

Add functions to read the system and mainboard serial numbers
from VPD tables stored in flash.

Remove board-specific implementations for google/drallion and
google/sarien and select the new Kconfig instead.

Test: build/boot google/akemi with RO_VPD region persisted from
stock Google firmware, verify system/mainboard serial numbers
present via dmidecode.

Change-Id: I14ae07cd8b764e1e22d58577c7cc697ca1496bd5
Signed-off-by: Matt DeVillier 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/49050
Reviewed-by: Angel Pons 
Reviewed-by: Michael Niewöhner 
Tested-by: build bot (Jenkins) 
---
M src/drivers/vpd/Kconfig
M src/drivers/vpd/Makefile.inc
A src/drivers/vpd/vpd_serial.c
M src/mainboard/google/drallion/Kconfig
M src/mainboard/google/drallion/ramstage.c
M src/mainboard/google/sarien/Kconfig
M src/mainboard/google/sarien/ramstage.c
7 files changed, 34 insertions(+), 42 deletions(-)

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

diff --git a/src/drivers/vpd/Kconfig b/src/drivers/vpd/Kconfig
index 0b16058..7d45eb6 100644
--- a/src/drivers/vpd/Kconfig
+++ b/src/drivers/vpd/Kconfig
@@ -19,3 +19,8 @@
    default 0x4000
    help
      Size in bytes of the FMAP region created to store VPD tables.
+
+config SMBIOS_SERIAL_FROM_VPD
+	bool "Load device serial from VPD"
+	depends on VPD && GENERATE_SMBIOS_TABLES
+	default n
diff --git a/src/drivers/vpd/Makefile.inc b/src/drivers/vpd/Makefile.inc
index f54c4d0..615e48a 100644
--- a/src/drivers/vpd/Makefile.inc
+++ b/src/drivers/vpd/Makefile.inc
@@ -5,3 +5,4 @@
 romstage-$(CONFIG_VPD) += vpd_decode.c vpd.c
 postcar-$(CONFIG_VPD) += vpd_decode.c vpd.c
 ramstage-$(CONFIG_VPD) += vpd_decode.c vpd.c
+ramstage-$(CONFIG_SMBIOS_SERIAL_FROM_VPD) += vpd_serial.c
diff --git a/src/drivers/vpd/vpd_serial.c b/src/drivers/vpd/vpd_serial.c
new file mode 100644
index 0000000..528dcbb
--- /dev/null
+++ b/src/drivers/vpd/vpd_serial.c
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+
+#include <smbios.h>
+
+#include "vpd.h"
+#include "vpd_tables.h"
+
+#define VPD_KEY_SYSTEM_SERIAL		"serial_number"
+#define VPD_KEY_MAINBOARD_SERIAL	"mlb_serial_number"
+#define VPD_SERIAL_LEN			64
+
+const char *smbios_system_serial_number(void)
+{
+	static char serial[VPD_SERIAL_LEN];
+	if (vpd_gets(VPD_KEY_SYSTEM_SERIAL, serial, VPD_SERIAL_LEN, VPD_RO))
+		return serial;
+	return "";
+}
+
+const char *smbios_mainboard_serial_number(void)
+{
+	static char serial[VPD_SERIAL_LEN];
+	if (vpd_gets(VPD_KEY_MAINBOARD_SERIAL, serial, VPD_SERIAL_LEN, VPD_RO))
+		return serial;
+	return "";
+}
diff --git a/src/mainboard/google/drallion/Kconfig b/src/mainboard/google/drallion/Kconfig
index 6c90eed..7470107 100644
--- a/src/mainboard/google/drallion/Kconfig
+++ b/src/mainboard/google/drallion/Kconfig
@@ -16,6 +16,7 @@
    select MAINBOARD_HAS_CHROMEOS
    select MAINBOARD_HAS_I2C_TPM_CR50
    select MAINBOARD_HAS_TPM2
+	select SMBIOS_SERIAL_FROM_VPD if VPD
    select SOC_INTEL_COMETLAKE_1
    select SOC_INTEL_COMMON_BLOCK_HDA_VERB
    select SOC_INTEL_COMMON_BLOCK_SMM_ESPI_DISABLE
diff --git a/src/mainboard/google/drallion/ramstage.c b/src/mainboard/google/drallion/ramstage.c
index 8725b83..384e44b 100644
--- a/src/mainboard/google/drallion/ramstage.c
+++ b/src/mainboard/google/drallion/ramstage.c
@@ -2,33 +2,12 @@

#include <acpi/acpi.h>
 #include <boardid.h>
-#include <drivers/vpd/vpd.h>
 #include <smbios.h>
 #include <soc/gpio.h>
 #include <soc/ramstage.h>
 #include <variant/gpio.h>
 #include <vendorcode/google/chromeos/chromeos.h>

-#define VPD_KEY_SYSTEM_SERIAL		"serial_number"
-#define VPD_KEY_MAINBOARD_SERIAL	"mlb_serial_number"
-#define VPD_SERIAL_LEN			64
-
-const char *smbios_system_serial_number(void)
-{
-	static char serial[VPD_SERIAL_LEN];
-	if (vpd_gets(VPD_KEY_SYSTEM_SERIAL, serial, VPD_SERIAL_LEN, VPD_RO))
-		return serial;
-	return "";
-}
-
-const char *smbios_mainboard_serial_number(void)
-{
-	static char serial[VPD_SERIAL_LEN];
-	if (vpd_gets(VPD_KEY_MAINBOARD_SERIAL, serial, VPD_SERIAL_LEN, VPD_RO))
-		return serial;
-	return "";
-}
-
 /* mainboard silk screen shows DIMM-A and DIMM-B */
 void smbios_fill_dimm_locator(const struct dimm_info *dimm,
    struct smbios_type17 *t)
diff --git a/src/mainboard/google/sarien/Kconfig b/src/mainboard/google/sarien/Kconfig
index 9b0d251..444234a 100644
--- a/src/mainboard/google/sarien/Kconfig
+++ b/src/mainboard/google/sarien/Kconfig
@@ -16,6 +16,7 @@
    select MAINBOARD_HAS_CHROMEOS
    select MAINBOARD_HAS_I2C_TPM_CR50
    select MAINBOARD_HAS_TPM2
+	select SMBIOS_SERIAL_FROM_VPD if VPD
    select SOC_INTEL_WHISKEYLAKE
    select SOC_INTEL_COMMON_BLOCK_HDA_VERB
    select SOC_INTEL_COMMON_BLOCK_SMM_ESPI_DISABLE
diff --git a/src/mainboard/google/sarien/ramstage.c b/src/mainboard/google/sarien/ramstage.c
index f4704fa..de97c68 100644
--- a/src/mainboard/google/sarien/ramstage.c
+++ b/src/mainboard/google/sarien/ramstage.c
@@ -2,33 +2,12 @@

#include <acpi/acpi.h>
 #include <boardid.h>
-#include <drivers/vpd/vpd.h>
 #include <smbios.h>
 #include <soc/gpio.h>
 #include <variant/gpio.h>
 #include <vendorcode/google/chromeos/chromeos.h>

#if CONFIG(GENERATE_SMBIOS_TABLES)
-#define VPD_KEY_SYSTEM_SERIAL		"serial_number"
-#define VPD_KEY_MAINBOARD_SERIAL	"mlb_serial_number"
-#define VPD_SERIAL_LEN			64
-
-const char *smbios_system_serial_number(void)
-{
-	static char serial[VPD_SERIAL_LEN];
-	if (vpd_gets(VPD_KEY_SYSTEM_SERIAL, serial, VPD_SERIAL_LEN, VPD_RO))
-		return serial;
-	return "";
-}
-
-const char *smbios_mainboard_serial_number(void)
-{
-	static char serial[VPD_SERIAL_LEN];
-	if (vpd_gets(VPD_KEY_MAINBOARD_SERIAL, serial, VPD_SERIAL_LEN, VPD_RO))
-		return serial;
-	return "";
-}
-
 /* mainboard silk screen shows DIMM-A and DIMM-B */
 void smbios_fill_dimm_locator(const struct dimm_info *dimm,
    struct smbios_type17 *t)