Change in coreboot[master]: soc/intel/tigerlake: Move tco_configure to bootblock

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43313 )

Change subject: soc/intel/tigerlake: Move tco_configure to bootblock
......................................................................

soc/intel/tigerlake: Move tco_configure to bootblock

On ChromeOS systems with a serial-enabled BIOS and vboot writing a new
firmware image to the Chrome EC, it was possible for the TCO watchdog
timer to trip 2 times before tco_configure() was called in romstage.
This caused an extra reboot of the system (at a rather inopportune time)
and because the EC didn't perform a full reset, the system boots into
recovery mode.

This patch moves the call to tco_configure() for Tiger Lake from
romstage to bootblock, in order to make sure the TCO watchdog timer is
halted before vboot_sync_ec() runs in romstage. It should be harmless to
configure the TCO device earlier in the boot flow.

BUG=b:160272400
TEST=boot Volteer (to a non-recovery kernel!) with a freshly imaged EC

Signed-off-by: Tim Wawrzynczak 
Change-Id: Iefdc2c861ab8b5fde7f736c04149be7de7b3ae0c
---
M src/soc/intel/tigerlake/bootblock/bootblock.c
M src/soc/intel/tigerlake/romstage/pch.c
2 files changed, 4 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/43313/1

diff --git a/src/soc/intel/tigerlake/bootblock/bootblock.c b/src/soc/intel/tigerlake/bootblock/bootblock.c
index 54ad85a..e7d97c5 100644
--- a/src/soc/intel/tigerlake/bootblock/bootblock.c
+++ b/src/soc/intel/tigerlake/bootblock/bootblock.c
@@ -2,6 +2,7 @@

#include <bootblock_common.h>
 #include <intelblocks/systemagent.h>
+#include <intelblocks/tco.h>
 #include <intelblocks/uart.h>
 #include <soc/bootblock.h>

@@ -25,4 +26,7 @@
 {
    report_platform_info();
    pch_init();
+
+	/* Programming TCO_BASE_ADDRESS and TCO Timer Halt */
+	tco_configure();
 }
diff --git a/src/soc/intel/tigerlake/romstage/pch.c b/src/soc/intel/tigerlake/romstage/pch.c
index e800ce5..6297dda 100644
--- a/src/soc/intel/tigerlake/romstage/pch.c
+++ b/src/soc/intel/tigerlake/romstage/pch.c
@@ -6,9 +6,6 @@

void pch_init(void)
 {
-	/* Programming TCO_BASE_ADDRESS and TCO Timer Halt */
-	tco_configure();
-
    /* Program SMBUS_BASE_ADDRESS and Enable it */
    smbus_common_init();
 }

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43313 )

Change subject: soc/intel/tigerlake: Move tco_configure to bootblock
......................................................................

Patch Set 1: Code-Review+2

[Daisuke Nojiri (Code Review)]

Daisuke Nojiri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43313 )

Change subject: soc/intel/tigerlake: Move tco_configure to bootblock
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43313/1/src/soc/intel/tigerlake/rom... 
File src/soc/intel/tigerlake/romstage/pch.c:

https://review.coreboot.org/c/coreboot/+/43313/1/src/soc/intel/tigerlake/rom... 
PS1, Line 4: #include <intelblocks/tco.h>
Can you remove this?

[Caveh Jalali (Code Review)]

Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43313 )

Change subject: soc/intel/tigerlake: Move tco_configure to bootblock
......................................................................

Patch Set 1: Code-Review+2

[Tim Wawrzynczak (Code Review)]

Hello Furquan Shaikh, Caveh Jalali, Duncan Laurie, Daisuke Nojiri, Nick Vaccaro, Aaron Durbin, Patrick Rudolph,

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

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

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

Change subject: soc/intel/tigerlake: Move tco_configure to bootblock
......................................................................

soc/intel/tigerlake: Move tco_configure to bootblock

On ChromeOS systems with a serial-enabled BIOS and vboot writing a new
firmware image to the Chrome EC, it was possible for the TCO watchdog
timer to trip 2 times before tco_configure() was called in romstage.
This caused an extra reboot of the system (at a rather inopportune time)
and because the EC didn't perform a full reset, the system boots into
recovery mode.

This patch moves the call to tco_configure() for Tiger Lake from
romstage to bootblock, in order to make sure the TCO watchdog timer is
halted before vboot_sync_ec() runs in romstage. It should be harmless to
configure the TCO device earlier in the boot flow.

BUG=b:160272400
TEST=boot Volteer (to a non-recovery kernel!) with a freshly imaged EC

Signed-off-by: Tim Wawrzynczak 
Change-Id: Iefdc2c861ab8b5fde7f736c04149be7de7b3ae0c
---
M src/soc/intel/tigerlake/bootblock/bootblock.c
M src/soc/intel/tigerlake/romstage/pch.c
2 files changed, 4 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/43313/2

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43313 )

Change subject: soc/intel/tigerlake: Move tco_configure to bootblock
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43313/1/src/soc/intel/tigerlake/rom... 
File src/soc/intel/tigerlake/romstage/pch.c:

https://review.coreboot.org/c/coreboot/+/43313/1/src/soc/intel/tigerlake/rom... 
PS1, Line 4: #include <intelblocks/tco.h>
> Can you remove this?

Done

[Daisuke Nojiri (Code Review)]

Daisuke Nojiri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43313 )

Change subject: soc/intel/tigerlake: Move tco_configure to bootblock
......................................................................

Patch Set 2: Code-Review+2

[Caveh Jalali (Code Review)]

Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43313 )

Change subject: soc/intel/tigerlake: Move tco_configure to bootblock
......................................................................

Patch Set 2: Code-Review+2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43313 )

Change subject: soc/intel/tigerlake: Move tco_configure to bootblock
......................................................................

Patch Set 2: Code-Review+2

This should probably be done for all Intel SoCs?

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43313 )

Change subject: soc/intel/tigerlake: Move tco_configure to bootblock
......................................................................

Patch Set 2:
> Patch Set 2: Code-Review+2
>
> This should probably be done for all Intel SoCs?

I was thinking about that, it would make things simpler and avoid the problem on other platforms. Although AFAIK we never saw this on CML.

[Angel Pons (Code Review)]

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

Change subject: soc/intel/tigerlake: Move tco_configure to bootblock
......................................................................

Patch Set 2: Code-Review+2

[Nick Vaccaro (Code Review)]

Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43313 )

Change subject: soc/intel/tigerlake: Move tco_configure to bootblock
......................................................................

Patch Set 2: Code-Review+2

[Patrick Georgi (Code Review)]

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

Change subject: soc/intel/tigerlake: Move tco_configure to bootblock
......................................................................

soc/intel/tigerlake: Move tco_configure to bootblock

On ChromeOS systems with a serial-enabled BIOS and vboot writing a new
firmware image to the Chrome EC, it was possible for the TCO watchdog
timer to trip 2 times before tco_configure() was called in romstage.
This caused an extra reboot of the system (at a rather inopportune time)
and because the EC didn't perform a full reset, the system boots into
recovery mode.

This patch moves the call to tco_configure() for Tiger Lake from
romstage to bootblock, in order to make sure the TCO watchdog timer is
halted before vboot_sync_ec() runs in romstage. It should be harmless to
configure the TCO device earlier in the boot flow.

BUG=b:160272400
TEST=boot Volteer (to a non-recovery kernel!) with a freshly imaged EC

Signed-off-by: Tim Wawrzynczak 
Change-Id: Iefdc2c861ab8b5fde7f736c04149be7de7b3ae0c
Reviewed-on: https://review.coreboot.org/c/coreboot/+/43313
Reviewed-by: Daisuke Nojiri 
Reviewed-by: Caveh Jalali 
Reviewed-by: Furquan Shaikh 
Reviewed-by: Angel Pons 
Reviewed-by: Nick Vaccaro 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/tigerlake/bootblock/bootblock.c
M src/soc/intel/tigerlake/romstage/pch.c
2 files changed, 4 insertions(+), 4 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  Daisuke Nojiri: Looks good to me, approved
  Nick Vaccaro: Looks good to me, approved
  Caveh Jalali: Looks good to me, approved
  Angel Pons: Looks good to me, approved

diff --git a/src/soc/intel/tigerlake/bootblock/bootblock.c b/src/soc/intel/tigerlake/bootblock/bootblock.c
index 54ad85a..e7d97c5 100644
--- a/src/soc/intel/tigerlake/bootblock/bootblock.c
+++ b/src/soc/intel/tigerlake/bootblock/bootblock.c
@@ -2,6 +2,7 @@

#include <bootblock_common.h>
 #include <intelblocks/systemagent.h>
+#include <intelblocks/tco.h>
 #include <intelblocks/uart.h>
 #include <soc/bootblock.h>

@@ -25,4 +26,7 @@
 {
    report_platform_info();
    pch_init();
+
+	/* Programming TCO_BASE_ADDRESS and TCO Timer Halt */
+	tco_configure();
 }
diff --git a/src/soc/intel/tigerlake/romstage/pch.c b/src/soc/intel/tigerlake/romstage/pch.c
index e800ce5..9fd8a1e 100644
--- a/src/soc/intel/tigerlake/romstage/pch.c
+++ b/src/soc/intel/tigerlake/romstage/pch.c
@@ -1,14 +1,10 @@
 /* SPDX-License-Identifier: GPL-2.0-only */

#include <intelblocks/smbus.h>
-#include <intelblocks/tco.h>
 #include <soc/romstage.h>

void pch_init(void)
 {
-	/* Programming TCO_BASE_ADDRESS and TCO Timer Halt */
-	tco_configure();
-
    /* Program SMBUS_BASE_ADDRESS and Enable it */
    smbus_common_init();
 }