Change in coreboot[master]: mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD varia...

[Johnny Lin (Code Review)]

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

Change subject: mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables
......................................................................

mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables

Add VPD variables for enabling/disabling FRB2 watchdog timer and
setting the timer countdown value. By default it would start the
timer and trigger hard reset when it's expired. The timer is
expected to be stopped later by payload or OS.
Right now the timer is started after FSP-M. Ideally it should be
before FSP-M (to detect memory training error).

Tested on OCP Mono Lake.

Change-Id: I82b244d08380a0461c92662e025d8b95b3133e23
Signed-off-by: Johnny Lin 
---
M src/mainboard/ocp/monolake/ipmi.c
M src/mainboard/ocp/monolake/ipmi.h
2 files changed, 1 insertion(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/36180/1

diff --git a/src/mainboard/ocp/monolake/ipmi.c b/src/mainboard/ocp/monolake/ipmi.c
index 37aacc8..3f178dc 100644
--- a/src/mainboard/ocp/monolake/ipmi.c
+++ b/src/mainboard/ocp/monolake/ipmi.c
@@ -17,8 +17,6 @@
 #include <console/console.h>
 #include "ipmi.h"

-#define BMC_KCS_BASE 0xca2
-
 int is_ipmi_clear_cmos_set(ipmi_oem_rsp_t *rsp)
 {
    int ret;
diff --git a/src/mainboard/ocp/monolake/ipmi.h b/src/mainboard/ocp/monolake/ipmi.h
index 5863eb5..0464911 100644
--- a/src/mainboard/ocp/monolake/ipmi.h
+++ b/src/mainboard/ocp/monolake/ipmi.h
@@ -23,7 +23,7 @@
 #define GET_CMOS_BIT(x) ((x) & (1 << 1))
 #define GET_VALID_BIT(x) ((x) & (1 << 7))
 #define CLEAR_CMOS_AND_VALID_BIT(x) ((x) &= 0x7d)
-
+#define BMC_KCS_BASE 0xca2
 typedef struct {
    u8 BootMode; /* Bit 1:CMOS clear, bit 7:valid bit. */
    u8 Boot0000;

[Johnny Lin (Code Review)]

Hello Jingle Hsu, Patrick Rudolph, Jonathan Zhang, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Andrey Petrov, Martin Roth,

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

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

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

Change subject: mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables
......................................................................

mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables

Add VPD variables for enabling/disabling FRB2 watchdog timer and
setting the timer countdown value. By default it would start the
timer and trigger hard reset when it's expired. The timer is
expected to be stopped later by payload or OS.
Right now the timer is started after FSP-M. Ideally it should be
before FSP-M (to detect memory training error).

Tested on OCP Mono Lake.

Change-Id: I82b244d08380a0461c92662e025d8b95b3133e23
Signed-off-by: Johnny Lin 
---
M src/mainboard/ocp/monolake/ipmi.c
M src/mainboard/ocp/monolake/ipmi.h
M src/mainboard/ocp/monolake/mainboard.c
3 files changed, 35 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/36180/2

[Johnny Lin (Code Review)]

Hello Jingle Hsu, Patrick Rudolph, Jonathan Zhang, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Andrey Petrov, Martin Roth,

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

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

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

Change subject: mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables
......................................................................

mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables

Add VPD variables for enabling/disabling FRB2 watchdog timer and
setting the timer countdown value. By default it would start the
timer and trigger hard reset when it's expired. The timer is
expected to be stopped later by payload or OS.
Right now the timer is started after FSP-M. Ideally it should be
before FSP-M (to detect memory training error).

Tested on OCP Mono Lake.

Change-Id: I82b244d08380a0461c92662e025d8b95b3133e23
Signed-off-by: Johnny Lin 
---
M src/mainboard/ocp/monolake/Kconfig
M src/mainboard/ocp/monolake/ipmi.c
M src/mainboard/ocp/monolake/ipmi.h
M src/mainboard/ocp/monolake/mainboard.c
4 files changed, 36 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/36180/3

[Patrick Rudolph (Code Review)]

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

Change subject: mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables
......................................................................

Patch Set 3:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/36180/3//COMMIT_MSG@14 
PS3, Line 14: error
How does that work? If you reset the training will fail again on the same place.

https://review.coreboot.org/c/coreboot/+/36180/3/src/mainboard/ocp/monolake/... 
File src/mainboard/ocp/monolake/mainboard.c:

https://review.coreboot.org/c/coreboot/+/36180/3/src/mainboard/ocp/monolake/... 
PS3, Line 62: 1
Use enum instead

[Johnny Lin (Code Review)]

Hello Jingle Hsu, Patrick Rudolph, Jonathan Zhang, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Andrey Petrov, Martin Roth,

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

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

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

Change subject: mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables
......................................................................

mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables

Add VPD variables for enabling/disabling FRB2 watchdog timer and
setting the timer countdown value. By default it would start the
timer and trigger hard reset when it's expired. The timer is
expected to be stopped later by payload or OS.
Right now the timer is started after FSP-M. Ideally it should be
before FSP-M (to detect memory training error).

Tested on OCP Mono Lake.

Change-Id: I82b244d08380a0461c92662e025d8b95b3133e23
Signed-off-by: Johnny Lin 
---
M src/mainboard/ocp/monolake/Kconfig
M src/mainboard/ocp/monolake/ipmi.c
M src/mainboard/ocp/monolake/ipmi.h
M src/mainboard/ocp/monolake/mainboard.c
4 files changed, 37 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/36180/5

[Johnny Lin (Code Review)]

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

Change subject: mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables
......................................................................

Patch Set 5:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/36180/3//COMMIT_MSG@14 
PS3, Line 14: error
> How does that work? If you reset the training will fail again on the same place.

The general idea is that the FRB2 timer should be started as early as possible in order to have a wider coverage during booting up. Right now because we can't send/receive IPMI before ramstage, so if the booting got stuck at romstage it can't be reset by IPMI watchdog timer and will be blocked indefinitely.
As for FSP-M if after watchdog reset it still got blocked at memory training then yes it will reset again.

https://review.coreboot.org/c/coreboot/+/36180/3/src/mainboard/ocp/monolake/... 
File src/mainboard/ocp/monolake/mainboard.c:

https://review.coreboot.org/c/coreboot/+/36180/3/src/mainboard/ocp/monolake/... 
PS3, Line 62: 1
> Use enum instead

Done

[Johnny Lin (Code Review)]

Hello Jingle Hsu, Patrick Rudolph, Jonathan Zhang, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Andrey Petrov, Martin Roth,

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

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

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

Change subject: mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables
......................................................................

mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables

Add VPD variables for enabling/disabling FRB2 watchdog timer and
setting the timer countdown value. By default it would start the
timer and trigger hard reset when it's expired. The timer is
expected to be stopped later by payload or OS.
Right now the timer is started after FSP-M. Ideally it should be
before FSP-M (to detect memory training error).

Tested on OCP Mono Lake.

Change-Id: I82b244d08380a0461c92662e025d8b95b3133e23
Signed-off-by: Johnny Lin 
---
M src/mainboard/ocp/monolake/Kconfig
M src/mainboard/ocp/monolake/ipmi.c
M src/mainboard/ocp/monolake/ipmi.h
M src/mainboard/ocp/monolake/mainboard.c
4 files changed, 36 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/36180/6

[Johnny Lin (Code Review)]

Hello Jingle Hsu, Patrick Rudolph, Jonathan Zhang, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Andrey Petrov, Martin Roth,

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

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

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

Change subject: mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables
......................................................................

mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables

Add VPD variables for enabling/disabling FRB2 watchdog timer and
setting the timer countdown value. By default it would start the
timer and trigger hard reset when it's expired. The timer is
expected to be stopped later by payload or OS.
Right now the timer is started after FSP-M. Ideally it should be
before FSP-M (to detect memory training error).

Tested on OCP Mono Lake.

Change-Id: I82b244d08380a0461c92662e025d8b95b3133e23
Signed-off-by: Johnny Lin 
---
M src/mainboard/ocp/monolake/Kconfig
M src/mainboard/ocp/monolake/ipmi.c
M src/mainboard/ocp/monolake/ipmi.h
M src/mainboard/ocp/monolake/mainboard.c
4 files changed, 36 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/36180/8

[Patrick Rudolph (Code Review)]

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

Change subject: mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables
......................................................................

Patch Set 8: Code-Review+1

[Johnny Lin (Code Review)]

Hello Jingle Hsu, Patrick Rudolph, Jonathan Zhang, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Andrey Petrov, Martin Roth,

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

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

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

Change subject: mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables
......................................................................

mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables

Add VPD variables for enabling/disabling FRB2 watchdog timer and
setting the timer countdown value. By default it would start the
timer and trigger hard reset when it's expired. The timer is
expected to be stopped later by payload or OS.
Right now the timer is started after FSP-M. Ideally it should be
before FSP-M (to detect memory training error).

Tested on OCP Mono Lake.

Change-Id: I82b244d08380a0461c92662e025d8b95b3133e23
Signed-off-by: Johnny Lin 
---
M src/mainboard/ocp/monolake/Kconfig
M src/mainboard/ocp/monolake/ipmi.c
M src/mainboard/ocp/monolake/ipmi.h
M src/mainboard/ocp/monolake/mainboard.c
4 files changed, 36 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/36180/10

[Johnny Lin (Code Review)]

Hello Jingle Hsu, Patrick Rudolph, Jonathan Zhang, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Andrey Petrov, Martin Roth,

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

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

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

Change subject: mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables
......................................................................

mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables

Add VPD variables for enabling/disabling FRB2 watchdog timer and
setting the timer countdown value. By default it would start the
timer and trigger hard reset when it's expired. The timer is
expected to be stopped later by payload or OS.

Right now the timer is started after FSP-M. Ideally it should be
before FSP-M (to detect memory training error).

Tested on OCP Mono Lake.

Change-Id: I82b244d08380a0461c92662e025d8b95b3133e23
Signed-off-by: Johnny Lin 
---
M src/mainboard/ocp/monolake/Kconfig
M src/mainboard/ocp/monolake/ipmi.c
M src/mainboard/ocp/monolake/ipmi.h
M src/mainboard/ocp/monolake/mainboard.c
4 files changed, 42 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/36180/12

[Johnny Lin (Code Review)]

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

Change subject: mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables
......................................................................

Patch Set 12:

(6 comments)

https://review.coreboot.org/c/coreboot/+/36180/10//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/36180/10//COMMIT_MSG@13 
PS10, Line 13: Right now the timer is started after FSP-M. Ideally it should be
> Please add a blank line above to separate the paragraphs.

Done

https://review.coreboot.org/c/coreboot/+/36180/10//COMMIT_MSG@16 
PS10, Line 16: Tested on OCP Mono Lake.
> How did you force a failure to get the watchdog to trigger?

coreboot started the watchdog timer, if I don't stop it in payload or OS it will trigger system hard reset in this change.

https://review.coreboot.org/c/coreboot/+/36180/10/src/mainboard/ocp/monolake... 
File src/mainboard/ocp/monolake/mainboard.c:

https://review.coreboot.org/c/coreboot/+/36180/10/src/mainboard/ocp/monolake... 
PS10, Line 32: #define DEFAULT_COUNTDOWN 9000
> Isn’t that too long?

It's the conventional default value set by UEFI BIOS, here we simply follow. It can be changed via VPD variable.

https://review.coreboot.org/c/coreboot/+/36180/10/src/mainboard/ocp/monolake... 
PS10, Line 54: 			printk(BIOS_DEBUG, "FRB2 timer countdown set to:%d\n",
> Please add a space after :.

Done

https://review.coreboot.org/c/coreboot/+/36180/10/src/mainboard/ocp/monolake... 
PS10, Line 57: 			printk(BIOS_DEBUG, "FRB2 timer use default value:%d\n",
> Ditto.

Done

https://review.coreboot.org/c/coreboot/+/36180/10/src/mainboard/ocp/monolake... 
PS10, Line 63: 	}
> Can this be put into a separate function?

Done

[Johnny Lin (Code Review)]

Hello Jingle Hsu, Patrick Rudolph, Jonathan Zhang, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Andrey Petrov, Martin Roth,

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

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

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

Change subject: mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables
......................................................................

mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables

Add VPD variables for enabling/disabling FRB2 watchdog timer and
setting the timer countdown value. By default it would start the
timer and trigger hard reset when it's expired. The timer is
expected to be stopped later by payload or OS.

Right now the timer is started after FSP-M. Ideally it should be
before FSP-M (to detect memory training error).

Tested on OCP Mono Lake.

Change-Id: I82b244d08380a0461c92662e025d8b95b3133e23
Signed-off-by: Johnny Lin 
---
M src/mainboard/ocp/monolake/Kconfig
M src/mainboard/ocp/monolake/ipmi.c
M src/mainboard/ocp/monolake/ipmi.h
M src/mainboard/ocp/monolake/mainboard.c
4 files changed, 42 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/36180/13

[Patrick Rudolph (Code Review)]

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

Change subject: mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables
......................................................................

Patch Set 13: Code-Review+2

[Patrick Georgi (Code Review)]

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

Change subject: mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables
......................................................................

mb/ocp/monolake: Configure IPMI BMC FRB2 watchdog timer via VPD variables

Add VPD variables for enabling/disabling FRB2 watchdog timer and
setting the timer countdown value. By default it would start the
timer and trigger hard reset when it's expired. The timer is
expected to be stopped later by payload or OS.

Right now the timer is started after FSP-M. Ideally it should be
before FSP-M (to detect memory training error).

Tested on OCP Mono Lake.

Change-Id: I82b244d08380a0461c92662e025d8b95b3133e23
Signed-off-by: Johnny Lin 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/36180
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Rudolph 
---
M src/mainboard/ocp/monolake/Kconfig
M src/mainboard/ocp/monolake/ipmi.c
M src/mainboard/ocp/monolake/ipmi.h
M src/mainboard/ocp/monolake/mainboard.c
4 files changed, 42 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Rudolph: Looks good to me, approved

diff --git a/src/mainboard/ocp/monolake/Kconfig b/src/mainboard/ocp/monolake/Kconfig
index ad4e9c7..7d85bbb 100644
--- a/src/mainboard/ocp/monolake/Kconfig
+++ b/src/mainboard/ocp/monolake/Kconfig
@@ -13,6 +13,7 @@
    select MAINBOARD_HAS_LPC_TPM
    select MAINBOARD_HAS_TPM1
    select IPMI_KCS
+	select VPD

config VBOOT
    select VBOOT_VBNV_CMOS
diff --git a/src/mainboard/ocp/monolake/ipmi.c b/src/mainboard/ocp/monolake/ipmi.c
index 37aacc8..3f178dc 100644
--- a/src/mainboard/ocp/monolake/ipmi.c
+++ b/src/mainboard/ocp/monolake/ipmi.c
@@ -17,8 +17,6 @@
 #include <console/console.h>
 #include "ipmi.h"

-#define BMC_KCS_BASE 0xca2
-
 int is_ipmi_clear_cmos_set(ipmi_oem_rsp_t *rsp)
 {
    int ret;
diff --git a/src/mainboard/ocp/monolake/ipmi.h b/src/mainboard/ocp/monolake/ipmi.h
index 5863eb5..0464911 100644
--- a/src/mainboard/ocp/monolake/ipmi.h
+++ b/src/mainboard/ocp/monolake/ipmi.h
@@ -23,7 +23,7 @@
 #define GET_CMOS_BIT(x) ((x) & (1 << 1))
 #define GET_VALID_BIT(x) ((x) & (1 << 7))
 #define CLEAR_CMOS_AND_VALID_BIT(x) ((x) &= 0x7d)
-
+#define BMC_KCS_BASE 0xca2
 typedef struct {
    u8 BootMode; /* Bit 1:CMOS clear, bit 7:valid bit. */
    u8 Boot0000;
diff --git a/src/mainboard/ocp/monolake/mainboard.c b/src/mainboard/ocp/monolake/mainboard.c
index 010e064..dffd19f 100644
--- a/src/mainboard/ocp/monolake/mainboard.c
+++ b/src/mainboard/ocp/monolake/mainboard.c
@@ -19,7 +19,46 @@
 #include <cf9_reset.h>
 #include <smbios.h>
 #include <string.h>
+#include <drivers/vpd/vpd.h>
+#include <console/console.h>
+#include <drivers/ipmi/ipmi_ops.h>
 #include "ipmi.h"
+/* VPD variable for enabling/disabling FRB2 timer. */
+#define FRB2_TIMER "FRB2_TIMER"
+/* VPD variable for setting FRB2 timer countdown value. */
+#define FRB2_COUNTDOWN "FRB2_COUNTDOWN"
+#define VPD_LEN 10
+/* Default countdown is 15 minutes. */
+#define DEFAULT_COUNTDOWN 9000
+
+static void init_frb2_wdt(void)
+{
+
+	char val[VPD_LEN];
+	/* Enable FRB2 timer by default. */
+	u8 enable = 1;
+	uint16_t countdown;
+
+	if (vpd_get_bool(FRB2_TIMER, VPD_RW, &enable)) {
+		if (!enable) {
+			printk(BIOS_DEBUG, "Disable FRB2 timer\n");
+			ipmi_stop_bmc_wdt(BMC_KCS_BASE);
+		}
+	}
+	if (enable) {
+		if (vpd_gets(FRB2_COUNTDOWN, val, VPD_LEN, VPD_RW)) {
+			countdown = (uint16_t)atol(val);
+			printk(BIOS_DEBUG, "FRB2 timer countdown set to: %d\n",
+				countdown);
+		} else {
+			printk(BIOS_DEBUG, "FRB2 timer use default value: %d\n",
+				DEFAULT_COUNTDOWN);
+			countdown = DEFAULT_COUNTDOWN;
+		}
+		ipmi_init_and_start_bmc_wdt(BMC_KCS_BASE, countdown,
+			TIMEOUT_HARD_RESET);
+	}
+}

/*
  * mainboard_enable is executed as first thing after enumerate_buses().
@@ -29,6 +68,7 @@
 {
    ipmi_oem_rsp_t rsp;

+	init_frb2_wdt();
    if (is_ipmi_clear_cmos_set(&rsp)) {
    	/* TODO: Should also try to restore CMOS to cmos.default
    	 * if USE_OPTION_TABLE is set */