Change in coreboot[master]: mb/google/dedede: Add a board specific reset

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44189 )

Change subject: mb/google/dedede: Add a board specific reset
......................................................................

mb/google/dedede: Add a board specific reset

When ME jumps from RO to RW, global reset is initiated. When AP is reset
as part of global reset, TPM initialization fails. This is because AP
reset is not detected by TPM hosting an older firmware version. Request
Embedded Controller (EC) to perform AP reset so that TPM can detect that
event.

BUG=b:162290856, b:162386991
TEST=Ensure that the device boots to OS with the board-specific reset
sequence when ME jumps from RO to RW with an older and newer Cr50
firmware.

Cq-Depend: chromium:2337430
Change-Id: Ib1f7271130e0b4b68c7f0917ecc4eadba1486206
Signed-off-by: Karthikeyan Ramasubramanian 
---
M src/mainboard/google/dedede/mainboard.c
1 file changed, 8 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/44189/1

diff --git a/src/mainboard/google/dedede/mainboard.c b/src/mainboard/google/dedede/mainboard.c
index cb84e1f..1fbdfce 100644
--- a/src/mainboard/google/dedede/mainboard.c
+++ b/src/mainboard/google/dedede/mainboard.c
@@ -5,8 +5,16 @@
 #include <baseboard/variants.h>
 #include <device/device.h>
 #include <ec/ec.h>
+#include <ec/google/chromeec/ec.h>
+#include <intelblocks/cse.h>
 #include <vendorcode/google/chromeos/chromeos.h>

+void cse_board_reset(void)
+{
+	/* TODO: Check tpm firmware version before initiating AP reset. */
+	google_chromeec_ap_reset();
+}
+
 __weak void variant_isst_override(void)
 {
    /*

[Justin TerAvest (Code Review)]

Justin TerAvest has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44189 )

Change subject: mb/google/dedede: Add a board specific reset
......................................................................

Patch Set 1: Code-Review+2

[Tim Wawrzynczak (Code Review)]

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

Change subject: mb/google/dedede: Add a board specific reset
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44189/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/44189/1//COMMIT_MSG@9 
PS1, Line 9: ME
CSE Lite

[Karthik Ramasubramanian (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Justin TerAvest, Tim Wawrzynczak, Marco Chen,

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

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

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

Change subject: mb/google/dedede: Add a board specific reset
......................................................................

mb/google/dedede: Add a board specific reset

When CSE Lite jumps from RO to RW, global reset is initiated. When AP is
reset as part of global reset, TPM initialization fails. This is because
AP reset is not detected by TPM hosting an older firmware version. Request
Embedded Controller (EC) to perform AP reset so that TPM can detect that
event.

BUG=b:162290856, b:162386991
TEST=Ensure that the device boots to OS with the board-specific reset
sequence when ME jumps from RO to RW with an older and newer Cr50
firmware.

Cq-Depend: chromium:2337430
Change-Id: Ib1f7271130e0b4b68c7f0917ecc4eadba1486206
Signed-off-by: Karthikeyan Ramasubramanian 
---
M src/mainboard/google/dedede/mainboard.c
1 file changed, 8 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/44189/2

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44189 )

Change subject: mb/google/dedede: Add a board specific reset
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44189/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/44189/1//COMMIT_MSG@9 
PS1, Line 9: ME
> CSE Lite

Done

[Tim Wawrzynczak (Code Review)]

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

Change subject: mb/google/dedede: Add a board specific reset
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/44189/2//COMMIT_MSG@17 
PS2, Line 17: ME
CSE Lite

[Karthik Ramasubramanian (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Justin TerAvest, Tim Wawrzynczak, Marco Chen,

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

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

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

Change subject: mb/google/dedede: Add a board specific reset
......................................................................

mb/google/dedede: Add a board specific reset

When CSE Lite jumps from RO to RW, global reset is initiated. When AP is
reset as part of global reset, TPM initialization fails. This is because
AP reset is not detected by TPM hosting an older firmware version. Request
Embedded Controller (EC) to perform AP reset so that TPM can detect that
event.

BUG=b:162290856, b:162386991
TEST=Ensure that the device boots to OS with the board-specific reset
sequence when CSE Lite jumps from RO to RW with an older and newer Cr50
firmware.

Cq-Depend: chromium:2337430
Change-Id: Ib1f7271130e0b4b68c7f0917ecc4eadba1486206
Signed-off-by: Karthikeyan Ramasubramanian 
---
M src/mainboard/google/dedede/mainboard.c
1 file changed, 8 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/44189/3

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44189 )

Change subject: mb/google/dedede: Add a board specific reset
......................................................................

Patch Set 3:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/44189/2//COMMIT_MSG@17 
PS2, Line 17: ME
> CSE Lite

Done

[Tim Wawrzynczak (Code Review)]

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

Change subject: mb/google/dedede: Add a board specific reset
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44189/4/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/mainboard.c:

https://review.coreboot.org/c/coreboot/+/44189/4/src/mainboard/google/dedede... 
PS4, Line 15: 	google_chromeec_ap_reset();
I think you should check the return value and halt() if it succeeded, i.e.,

if (google_chromeec_ap_reset())
       halt();

and wait for the EC to reset. Otherwise, you could fall through into the global reset and then it's a race for which reset happens first

[Karthik Ramasubramanian (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Justin TerAvest, Tim Wawrzynczak, Marco Chen,

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

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

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

Change subject: mb/google/dedede: Add a board specific reset
......................................................................

mb/google/dedede: Add a board specific reset

When CSE Lite jumps from RO to RW, global reset is initiated. When AP is
reset as part of global reset, TPM initialization fails. This is because
AP reset is not detected by TPM hosting an older firmware version. Request
Embedded Controller (EC) to perform AP reset so that TPM can detect that
event.

BUG=b:162290856, b:162386991
TEST=Ensure that the device boots to OS with the board-specific reset
sequence when CSE Lite jumps from RO to RW with an older and newer Cr50
firmware.

Cq-Depend: chromium:2337430
Change-Id: Ib1f7271130e0b4b68c7f0917ecc4eadba1486206
Signed-off-by: Karthikeyan Ramasubramanian 
---
M src/mainboard/google/dedede/mainboard.c
1 file changed, 10 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/44189/5

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44189 )

Change subject: mb/google/dedede: Add a board specific reset
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44189/4/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/mainboard.c:

https://review.coreboot.org/c/coreboot/+/44189/4/src/mainboard/google/dedede... 
PS4, Line 15: 	google_chromeec_ap_reset();
> I think you should check the return value and halt() if it succeeded, i.e., […]

Done

[Tim Wawrzynczak (Code Review)]

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

Change subject: mb/google/dedede: Add a board specific reset
......................................................................

Patch Set 5: Code-Review+2

[Furquan Shaikh (Code Review)]

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

Change subject: mb/google/dedede: Add a board specific reset
......................................................................

Patch Set 5: Code-Review+2

[Nick Vaccaro (Code Review)]

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

Change subject: mb/google/dedede: Add a board specific reset
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44189/5/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/mainboard.c:

https://review.coreboot.org/c/coreboot/+/44189/5/src/mainboard/google/dedede... 
PS5, Line 15: TODO
I think a bug should be filed instead of a TODO in the code if there is still more needed here that will not be added prior to merge.

[Nick Vaccaro (Code Review)]

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

Change subject: mb/google/dedede: Add a board specific reset
......................................................................

Patch Set 5: Code-Review+2

Please disregard my last comment.  I just finished walking the relation chain and see that this is addressed as part of the chain.

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44189 )

Change subject: mb/google/dedede: Add a board specific reset
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44189/5/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/mainboard.c:

https://review.coreboot.org/c/coreboot/+/44189/5/src/mainboard/google/dedede... 
PS5, Line 15: TODO
> I think a bug should be filed instead of a TODO in the code if there is still more needed here that  […]

Yes, one of the required changes to cache tpm firmware version is going through the review simultaneously. Once that change gets merged, the last CL in this stack can be rebased on top of that.

Even otherwise, this change works with both old and new cr50 firmware versions.

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44189 )

Change subject: mb/google/dedede: Add a board specific reset
......................................................................

mb/google/dedede: Add a board specific reset

When CSE Lite jumps from RO to RW, global reset is initiated. When AP is
reset as part of global reset, TPM initialization fails. This is because
AP reset is not detected by TPM hosting an older firmware version. Request
Embedded Controller (EC) to perform AP reset so that TPM can detect that
event.

BUG=b:162290856, b:162386991
TEST=Ensure that the device boots to OS with the board-specific reset
sequence when CSE Lite jumps from RO to RW with an older and newer Cr50
firmware.

Cq-Depend: chromium:2337430
Change-Id: Ib1f7271130e0b4b68c7f0917ecc4eadba1486206
Signed-off-by: Karthikeyan Ramasubramanian 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/44189
Tested-by: build bot (Jenkins) 
Reviewed-by: Tim Wawrzynczak 
Reviewed-by: Furquan Shaikh 
Reviewed-by: Nick Vaccaro 
---
M src/mainboard/google/dedede/mainboard.c
1 file changed, 10 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  Nick Vaccaro: Looks good to me, approved
  Tim Wawrzynczak: Looks good to me, approved

diff --git a/src/mainboard/google/dedede/mainboard.c b/src/mainboard/google/dedede/mainboard.c
index cb84e1f..4695a9f 100644
--- a/src/mainboard/google/dedede/mainboard.c
+++ b/src/mainboard/google/dedede/mainboard.c
@@ -5,8 +5,18 @@
 #include <baseboard/variants.h>
 #include <device/device.h>
 #include <ec/ec.h>
+#include <ec/google/chromeec/ec.h>
+#include <halt.h>
+#include <intelblocks/cse.h>
 #include <vendorcode/google/chromeos/chromeos.h>

+void cse_board_reset(void)
+{
+	/* TODO: Check tpm firmware version before initiating AP reset. */
+	if (!google_chromeec_ap_reset())
+		halt();
+}
+
 __weak void variant_isst_override(void)
 {
    /*