Change in coreboot[master]: mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage

[Johnny Lin (Code Review)]

Johnny Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42903 )

Change subject: mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage
......................................................................

mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage

Read VPD variable 'FSP_LOG' to decide enabling FSP log or not.
With VPD_RW_THEN_RO, VPD_RW takes precedence over VPD_RO, and
would be set to enabled if both places cannot find it.

Tested=On OCP Delta Lake, use vpd to create and set FSP_LOG
and verified the results are expected.

Change-Id: I0b3463acedd90e8e17f7e4eedc2fab63644f87e1
Signed-off-by: Johnny Lin 
---
M src/mainboard/ocp/deltalake/romstage.c
1 file changed, 24 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/42903/1

diff --git a/src/mainboard/ocp/deltalake/romstage.c b/src/mainboard/ocp/deltalake/romstage.c
index f56f7c7..5bca0bd 100644
--- a/src/mainboard/ocp/deltalake/romstage.c
+++ b/src/mainboard/ocp/deltalake/romstage.c
@@ -2,6 +2,7 @@

#include <console/console.h>
 #include <drivers/ipmi/ipmi_kcs.h>
+#include <drivers/vpd/vpd.h>
 #include <fsp/api.h>
 #include <FspmUpd.h>
 #include <soc/romstage.h>
@@ -9,6 +10,28 @@
 #include "cpxsp_dl_iio.h"
 #include "ipmi.h"

+/* Define the VPD keys for UPD variables that can be overwritten */
+#define FSP_LOG "FSP_LOG"
+
+/*
+ * Search for VPD_RW first then VPD_RO for UPD config variables,
+ * overwrites them from VPD if it's found.
+ */
+static void mainboard_config_upd(FSPM_UPD *mupd)
+{
+	uint8_t val;
+
+	/* Send FSP log message to SOL */
+	if (vpd_get_bool(FSP_LOG, VPD_RW_THEN_RO, &val))
+		mupd->FspmConfig.SerialIoUartDebugEnable = val;
+	else {
+		printk(BIOS_ERR, "Failed to get VPD %s, default set SerialIoUartDebugEnable to 1\n",
+			FSP_LOG);
+		mupd->FspmConfig.SerialIoUartDebugEnable = 1;
+	}
+	mupd->FspmConfig.SerialIoUartDebugIoBase = 0x2f8;
+}
+
 /* Update bifurcation settings according to different Configs */
 static void oem_update_iio(FSPM_UPD *mupd)
 {
@@ -54,10 +77,6 @@

static void mainboard_config_iio(FSPM_UPD *mupd)
 {
-	/* Send FSP log message to SOL */
-	mupd->FspmConfig.SerialIoUartDebugEnable = 1;
-	mupd->FspmConfig.SerialIoUartDebugIoBase = 0x2f8;
-
    /* Enable only PCH: PCIE[8:11] */
    for (uint8_t i = 0; i < 20; i++) {
    	if (i >= 8 && i <= 11)
@@ -102,4 +121,5 @@

mainboard_config_gpios(mupd);
    mainboard_config_iio(mupd);
+	mainboard_config_upd(mupd);
 }

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42903 )

Change subject: mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42903/1/src/mainboard/ocp/deltalake... 
File src/mainboard/ocp/deltalake/romstage.c:

https://review.coreboot.org/c/coreboot/+/42903/1/src/mainboard/ocp/deltalake... 
PS1, Line 14: #define FSP_LOG "FSP_LOG"
For VPD key name, let's use lower characters only, so use "fsp_log" instead of "FSP_LOG".

[Johnny Lin (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph, Jonathan Zhang, Christian Walter, Morgan Jang,

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

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

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

Change subject: mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage
......................................................................

mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage

Read VPD variable 'fsp_log' to decide enabling FSP log or not.
With VPD_RW_THEN_RO, VPD_RW takes precedence over VPD_RO, and
would be set to enabled if both places cannot find it.

Tested=On OCP Delta Lake, use vpd to create and set fsp_log
and verified the results are expected.

Change-Id: I0b3463acedd90e8e17f7e4eedc2fab63644f87e1
Signed-off-by: Johnny Lin 
---
M src/mainboard/ocp/deltalake/romstage.c
M src/mainboard/ocp/deltalake/vpd.h
2 files changed, 25 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/42903/2

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42903 )

Change subject: mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42903/1/src/mainboard/ocp/deltalake... 
File src/mainboard/ocp/deltalake/romstage.c:

https://review.coreboot.org/c/coreboot/+/42903/1/src/mainboard/ocp/deltalake... 
PS1, Line 14: #define FSP_LOG "FSP_LOG"
> For VPD key name, let's use lower characters only, so use "fsp_log" instead of "FSP_LOG".

Done

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42903 )

Change subject: mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42903/2/src/mainboard/ocp/deltalake... 
File src/mainboard/ocp/deltalake/romstage.c:

https://review.coreboot.org/c/coreboot/+/42903/2/src/mainboard/ocp/deltalake... 
PS2, Line 25: 		printk(BIOS_ERR, "Failed to get VPD %s, default set "
Such VPD not being said, is not really an error. Let's change BIOS_ERR to BIOS_INFO, and change "Failed to get" to "Not able to get".

[Johnny Lin (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph, Jonathan Zhang, Christian Walter, Morgan Jang,

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

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

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

Change subject: mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage
......................................................................

mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage

Read VPD variable 'fsp_log' to decide enabling FSP log or not.
With VPD_RW_THEN_RO, VPD_RW takes precedence over VPD_RO, and
would be set to enabled if both places cannot find it.

Tested=On OCP Delta Lake, use vpd to create and set fsp_log
and verified the results are expected.

Change-Id: I0b3463acedd90e8e17f7e4eedc2fab63644f87e1
Signed-off-by: Johnny Lin 
---
M src/mainboard/ocp/deltalake/romstage.c
M src/mainboard/ocp/deltalake/vpd.h
2 files changed, 25 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/42903/3

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42903 )

Change subject: mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42903/2/src/mainboard/ocp/deltalake... 
File src/mainboard/ocp/deltalake/romstage.c:

https://review.coreboot.org/c/coreboot/+/42903/2/src/mainboard/ocp/deltalake... 
PS2, Line 25: 		printk(BIOS_ERR, "Failed to get VPD %s, default set "
> Such VPD not being said, is not really an error. […]

Done

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42903 )

Change subject: mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage
......................................................................

Patch Set 3: Code-Review+2

[Johnny Lin (Code Review)]

Hello Philipp Deppenwiese, build bot (Jenkins), Patrick Rudolph, Jonathan Zhang, Christian Walter, Morgan Jang,

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

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

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

Change subject: mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage
......................................................................

mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage

Read VPD variable 'fsp_log' to decide enabling FSP log or not.
With VPD_RW_THEN_RO, VPD_RW takes precedence over VPD_RO, and
would be set to enabled if both places cannot find it.

Tested=On OCP Delta Lake, use vpd to create and set fsp_log
and verified the results are expected.

Change-Id: I0b3463acedd90e8e17f7e4eedc2fab63644f87e1
Signed-off-by: Johnny Lin 
---
M src/mainboard/ocp/deltalake/romstage.c
M src/mainboard/ocp/deltalake/vpd.h
2 files changed, 25 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/42903/4

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42903 )

Change subject: mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage
......................................................................

Patch Set 4: Code-Review+2

[insomniac (Code Review)]

insomniac has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42903 )

Change subject: mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage
......................................................................

Patch Set 4:

(1 comment)

Just one comment on the VPD variable name, otherwise looks good to me

https://review.coreboot.org/c/coreboot/+/42903/4/src/mainboard/ocp/deltalake... 
File src/mainboard/ocp/deltalake/vpd.h:

https://review.coreboot.org/c/coreboot/+/42903/4/src/mainboard/ocp/deltalake... 
PS4, Line 15: #define FSP_LOG "fsp_log" /* 1 or 0: enable or disable FSP SOL log */
I think this could be more explicit, e.g. "fsp_log_enable" or "enable_fsp_log"

[Johnny Lin (Code Review)]

Hello Philipp Deppenwiese, build bot (Jenkins), Patrick Rudolph, Jonathan Zhang, Christian Walter, Morgan Jang,

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

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

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

Change subject: mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage
......................................................................

mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage

Read VPD variable 'fsp_log_enable' to decide enabling FSP log or not.
With VPD_RW_THEN_RO, VPD_RW takes precedence over VPD_RO, and
would be set to enabled if both places cannot find it.

Tested=On OCP Delta Lake, use vpd to create and set fsp_log_enable
and verified the results are expected.

Change-Id: I0b3463acedd90e8e17f7e4eedc2fab63644f87e1
Signed-off-by: Johnny Lin 
---
M src/mainboard/ocp/deltalake/romstage.c
M src/mainboard/ocp/deltalake/vpd.h
2 files changed, 25 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/42903/5

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42903 )

Change subject: mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42903/4/src/mainboard/ocp/deltalake... 
File src/mainboard/ocp/deltalake/vpd.h:

https://review.coreboot.org/c/coreboot/+/42903/4/src/mainboard/ocp/deltalake... 
PS4, Line 15: #define FSP_LOG "fsp_log" /* 1 or 0: enable or disable FSP SOL log */
> I think this could be more explicit, e.g. […]

Done

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42903 )

Change subject: mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage
......................................................................

Patch Set 5: Code-Review+2

[insomniac (Code Review)]

insomniac has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42903 )

Change subject: mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage
......................................................................

Patch Set 5: Code-Review+1

[Patrick Georgi (Code Review)]

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

Change subject: mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage
......................................................................

mb/ocp/deltalake: Use VPD data to configure FSP UPD at romstage

Read VPD variable 'fsp_log_enable' to decide enabling FSP log or not.
With VPD_RW_THEN_RO, VPD_RW takes precedence over VPD_RO, and
would be set to enabled if both places cannot find it.

Tested=On OCP Delta Lake, use vpd to create and set fsp_log_enable
and verified the results are expected.

Change-Id: I0b3463acedd90e8e17f7e4eedc2fab63644f87e1
Signed-off-by: Johnny Lin 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/42903
Tested-by: build bot (Jenkins) 
Reviewed-by: Christian Walter 
Reviewed-by: insomniac 
---
M src/mainboard/ocp/deltalake/romstage.c
M src/mainboard/ocp/deltalake/vpd.h
2 files changed, 25 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  insomniac: Looks good to me, but someone else must approve
  Christian Walter: Looks good to me, approved

diff --git a/src/mainboard/ocp/deltalake/romstage.c b/src/mainboard/ocp/deltalake/romstage.c
index 3389e6e..9ce06ad 100644
--- a/src/mainboard/ocp/deltalake/romstage.c
+++ b/src/mainboard/ocp/deltalake/romstage.c
@@ -2,11 +2,32 @@

#include <console/console.h>
 #include <drivers/ipmi/ipmi_kcs.h>
+#include <drivers/vpd/vpd.h>
 #include <fsp/api.h>
 #include <FspmUpd.h>
 #include <soc/romstage.h>

#include "ipmi.h"
+#include "vpd.h"
+
+/*
+ * Search from VPD_RW first then VPD_RO for UPD config variables,
+ * overwrites them from VPD if it's found.
+ */
+static void mainboard_config_upd(FSPM_UPD *mupd)
+{
+	uint8_t val;
+
+	/* Send FSP log message to SOL */
+	if (vpd_get_bool(FSP_LOG, VPD_RW_THEN_RO, &val))
+		mupd->FspmConfig.SerialIoUartDebugEnable = val;
+	else {
+		printk(BIOS_INFO, "Not able to get VPD %s, default set "
+			"SerialIoUartDebugEnable to 1\n", FSP_LOG);
+		mupd->FspmConfig.SerialIoUartDebugEnable = 1;
+	}
+	mupd->FspmConfig.SerialIoUartDebugIoBase = 0x2f8;
+}

/* Update bifurcation settings according to different Configs */
 static void oem_update_iio(FSPM_UPD *mupd)
@@ -53,9 +74,6 @@

static void mainboard_config_iio(FSPM_UPD *mupd)
 {
-	/* Send FSP log message to SOL */
-	mupd->FspmConfig.SerialIoUartDebugEnable = 1;
-	mupd->FspmConfig.SerialIoUartDebugIoBase = 0x2f8;
    oem_update_iio(mupd);
 }

@@ -68,4 +86,5 @@

mainboard_config_gpios(mupd);
    mainboard_config_iio(mupd);
+	mainboard_config_upd(mupd);
 }
diff --git a/src/mainboard/ocp/deltalake/vpd.h b/src/mainboard/ocp/deltalake/vpd.h
index 65aae72..9e17bb9 100644
--- a/src/mainboard/ocp/deltalake/vpd.h
+++ b/src/mainboard/ocp/deltalake/vpd.h
@@ -11,4 +11,7 @@
 /* Default countdown is 15 minutes. */
 #define DEFAULT_COUNTDOWN 9000

+/* Define the VPD keys for UPD variables that can be overwritten */
+#define FSP_LOG "fsp_log_enable" /* 1 or 0: enable or disable FSP SOL log */
+
 #endif