Karthik Ramasubramanian has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44187 )
Change subject: soc/intel/common/cse_lite: Perform a board specific reset ......................................................................
soc/intel/common/cse_lite: Perform a board specific reset
When ME jumps from RO to RW, global reset is initiated. When AP is reset as part of global reset, in some boards TPM initialization fails. This is because AP reset is not detected by TPM hosting an older firmware version. To signal TPMs running older firmware version about AP reset, a modified reset sequence needs to be performed. Hence add support to perform board-specific reset sequence.
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.
Change-Id: I8663e7f25461e58e45766e2ac00d752bfa191d8b Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/soc/intel/common/block/cse/cse_lite.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 13 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/44187/1
diff --git a/src/soc/intel/common/block/cse/cse_lite.c b/src/soc/intel/common/block/cse/cse_lite.c index a12f2d0..7e80e72 100644 --- a/src/soc/intel/common/block/cse/cse_lite.c +++ b/src/soc/intel/common/block/cse/cse_lite.c @@ -355,12 +355,21 @@ return true; }
+__weak void cse_board_reset(void) +{ + /* Default weak implementation, does nothing. */ +} + /* Set the CSE's next boot partition and issues system reset */ static bool cse_set_and_boot_from_next_bp(enum boot_partition_id bp) { if (!cse_set_next_boot_partition(bp)) return false;
+ /* Allow the board to perform a reset for CSE RO<->RW jump */ + cse_board_reset(); + + /* If board does not perform the reset, then perform global_reset */ do_global_reset();
die("cse_lite: Failed to reset the system\n"); diff --git a/src/soc/intel/common/block/include/intelblocks/cse.h b/src/soc/intel/common/block/include/intelblocks/cse.h index a1dc3d9..5466ba6 100644 --- a/src/soc/intel/common/block/include/intelblocks/cse.h +++ b/src/soc/intel/common/block/include/intelblocks/cse.h @@ -219,4 +219,8 @@ * currently selected partition. */ void cse_fw_sync(void *unused); + +/* Perform a board-specific reset sequence for CSE RO<->RW jump */ +void cse_board_reset(void); + #endif // SOC_INTEL_COMMON_CSE_H
Justin TerAvest has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44187 )
Change subject: soc/intel/common/cse_lite: Perform a board specific reset ......................................................................
Patch Set 1: Code-Review+2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44187 )
Change subject: soc/intel/common/cse_lite: Perform a board specific reset ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44187/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44187/1//COMMIT_MSG@9 PS1, Line 9: ME this is functionality that is specific to CSE Lite
https://review.coreboot.org/c/coreboot/+/44187/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/44187/1/src/soc/intel/common/block/... PS1, Line 360: /* Default weak implementation, does nothing. */ Should this weak function contain the `do_global_reset()`, since that would become the default implementation for boards that don't need the workaround?
Hello build bot (Jenkins), Furquan Shaikh, Justin TerAvest, Tim Wawrzynczak, Rizwan Qureshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44187
to look at the new patch set (#2).
Change subject: soc/intel/common/cse_lite: Perform a board specific reset ......................................................................
soc/intel/common/cse_lite: Perform 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, in some boards TPM initialization fails. This is because AP reset is not detected by TPM hosting an older firmware version. To signal TPMs running older firmware version about AP reset, a modified reset sequence needs to be performed. Hence add support to perform board-specific reset sequence.
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.
Change-Id: I8663e7f25461e58e45766e2ac00d752bfa191d8b Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/soc/intel/common/block/cse/cse_lite.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 12 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/44187/2
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44187 )
Change subject: soc/intel/common/cse_lite: Perform a board specific reset ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44187/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44187/1//COMMIT_MSG@9 PS1, Line 9: ME
this is functionality that is specific to CSE Lite
Done
https://review.coreboot.org/c/coreboot/+/44187/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/44187/1/src/soc/intel/common/block/... PS1, Line 360: /* Default weak implementation, does nothing. */
Should this weak function contain the `do_global_reset()`, since that would become the default imple […]
Done
Hello build bot (Jenkins), Furquan Shaikh, Justin TerAvest, Tim Wawrzynczak, Rizwan Qureshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44187
to look at the new patch set (#3).
Change subject: soc/intel/common/cse_lite: Perform a board specific reset ......................................................................
soc/intel/common/cse_lite: Perform 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, in some boards TPM initialization fails. This is because AP reset is not detected by TPM hosting an older firmware version. To signal TPMs running older firmware version about AP reset, a modified reset sequence needs to be performed. Hence add support to perform board-specific reset sequence.
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.
Change-Id: I8663e7f25461e58e45766e2ac00d752bfa191d8b Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/soc/intel/common/block/cse/cse_lite.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 12 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/44187/3
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44187 )
Change subject: soc/intel/common/cse_lite: Perform a board specific reset ......................................................................
Patch Set 3:
Actually I thought of something, if the `cse_board_reset` callback is unable to actually reset the system (because sending the EC a command may fail), then it might be better to leave the `do_global_reset` where it was before, so if the EC fails to reset the system, it would drop down into the call to `do_global_reset`, and it would at least boot into recovery mode (b/c of TPM re-init failure), instead of just hanging here.
WDYT?
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44187 )
Change subject: soc/intel/common/cse_lite: Perform a board specific reset ......................................................................
Patch Set 3:
Patch Set 3:
Actually I thought of something, if the `cse_board_reset` callback is unable to actually reset the system (because sending the EC a command may fail), then it might be better to leave the `do_global_reset` where it was before, so if the EC fails to reset the system, it would drop down into the call to `do_global_reset`, and it would at least boot into recovery mode (b/c of TPM re-init failure), instead of just hanging here.
WDYT?
I agree to that. Also if the TPM is not hosting old firmware, then cse_board_reset can be a no-op and global reset can be performed.
Hello build bot (Jenkins), Furquan Shaikh, Justin TerAvest, Tim Wawrzynczak, Rizwan Qureshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44187
to look at the new patch set (#4).
Change subject: soc/intel/common/cse_lite: Perform a board specific reset ......................................................................
soc/intel/common/cse_lite: Perform 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, in some boards TPM initialization fails. This is because AP reset is not detected by TPM hosting an older firmware version. To signal TPMs running older firmware version about AP reset, a modified reset sequence needs to be performed. Hence add support to perform board-specific reset sequence.
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.
Change-Id: I8663e7f25461e58e45766e2ac00d752bfa191d8b Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/soc/intel/common/block/cse/cse_lite.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 13 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/44187/4
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44187 )
Change subject: soc/intel/common/cse_lite: Perform a board specific reset ......................................................................
Patch Set 4:
Patch Set 3:
Patch Set 3:
Actually I thought of something, if the `cse_board_reset` callback is unable to actually reset the system (because sending the EC a command may fail), then it might be better to leave the `do_global_reset` where it was before, so if the EC fails to reset the system, it would drop down into the call to `do_global_reset`, and it would at least boot into recovery mode (b/c of TPM re-init failure), instead of just hanging here.
WDYT?
I agree to that. Also if the TPM is not hosting old firmware, then cse_board_reset can be a no-op and global reset can be performed.
Done.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44187 )
Change subject: soc/intel/common/cse_lite: Perform a board specific reset ......................................................................
Patch Set 4: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44187 )
Change subject: soc/intel/common/cse_lite: Perform a board specific reset ......................................................................
Patch Set 5: Code-Review+2
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44187 )
Change subject: soc/intel/common/cse_lite: Perform a board specific reset ......................................................................
Patch Set 5: Code-Review+2
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44187 )
Change subject: soc/intel/common/cse_lite: Perform a board specific reset ......................................................................
Patch Set 5: Code-Review+2
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44187 )
Change subject: soc/intel/common/cse_lite: Perform a board specific reset ......................................................................
soc/intel/common/cse_lite: Perform 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, in some boards TPM initialization fails. This is because AP reset is not detected by TPM hosting an older firmware version. To signal TPMs running older firmware version about AP reset, a modified reset sequence needs to be performed. Hence add support to perform board-specific reset sequence.
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.
Change-Id: I8663e7f25461e58e45766e2ac00d752bfa191d8b Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/44187 Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Nick Vaccaro nvaccaro@google.com Reviewed-by: Edward O'Callaghan quasisec@chromium.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/common/block/cse/cse_lite.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 13 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 Edward O'Callaghan: Looks good to me, approved
diff --git a/src/soc/intel/common/block/cse/cse_lite.c b/src/soc/intel/common/block/cse/cse_lite.c index ff489af..d89044f 100644 --- a/src/soc/intel/common/block/cse/cse_lite.c +++ b/src/soc/intel/common/block/cse/cse_lite.c @@ -364,12 +364,21 @@ return true; }
+__weak void cse_board_reset(void) +{ + /* Default weak implementation, does nothing. */ +} + /* Set the CSE's next boot partition and issues system reset */ static bool cse_set_and_boot_from_next_bp(enum boot_partition_id bp) { if (!cse_set_next_boot_partition(bp)) return false;
+ /* Allow the board to perform a reset for CSE RO<->RW jump */ + cse_board_reset(); + + /* If board does not perform the reset, then perform global_reset */ do_global_reset();
die("cse_lite: Failed to reset the system\n"); diff --git a/src/soc/intel/common/block/include/intelblocks/cse.h b/src/soc/intel/common/block/include/intelblocks/cse.h index a1dc3d9..5466ba6 100644 --- a/src/soc/intel/common/block/include/intelblocks/cse.h +++ b/src/soc/intel/common/block/include/intelblocks/cse.h @@ -219,4 +219,8 @@ * currently selected partition. */ void cse_fw_sync(void *unused); + +/* Perform a board-specific reset sequence for CSE RO<->RW jump */ +void cse_board_reset(void); + #endif // SOC_INTEL_COMMON_CSE_H
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44187 )
Change subject: soc/intel/common/cse_lite: Perform a board specific reset ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44187/6/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/44187/6/src/soc/intel/common/block/... PS6, Line 379: cse_board_reset Can this result global reset(power cycle)?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44187 )
Change subject: soc/intel/common/cse_lite: Perform a board specific reset ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44187/6/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/44187/6/src/soc/intel/common/block/... PS6, Line 379: cse_board_reset
Can this result global reset(power cycle)?
For dedede (& volteer), this is going to send a command to the EC, which will cold-reset the SoC (toggle SYSRST#). If the command successfully, the AP will hang waiting for the EC, and the global_reset() will not be triggered.
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44187 )
Change subject: soc/intel/common/cse_lite: Perform a board specific reset ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44187/6/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/44187/6/src/soc/intel/common/block/... PS6, Line 379: cse_board_reset
For dedede (& volteer), this is going to send a command to the EC, which will cold-reset the SoC (to […]
Looks like Toggling SYSRST# pin doesn't result cold reset(power cycle).
Excerpt from EDS .. *The normal behavior for a SYS_RESET# assertion is host partition reset without power cycle. However, if bit 3 of the CF9h I/O register is set to ‘1’ then SYS_RESET# will result in a full power-cycle reset*.
It's better to set 3rd bit of CF9 before sending AP_RESET command to EC. I think *TPM should also aware of reset of host cpu* in this case too. Can you please check if this helpful?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44187 )
Change subject: soc/intel/common/cse_lite: Perform a board specific reset ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44187/6/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/44187/6/src/soc/intel/common/block/... PS6, Line 379: cse_board_reset
Looks like Toggling SYSRST# pin doesn't result cold reset(power cycle). […]
Thanks Sridhar for catching this. You are absolutely right that we need to ensure that bit 3 of 0xcf9 is set to 1 before SYS_RESET# gets asserted by the EC. Something like this will have to be added before the call to EC to assert SYS_RESET#:
``` outb(FULL_RST | SYS_RST, RST_CNT); ```
Can you please push a change for this?
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44187 )
Change subject: soc/intel/common/cse_lite: Perform a board specific reset ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44187/6/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/44187/6/src/soc/intel/common/block/... PS6, Line 379: cse_board_reset
Thanks Sridhar for catching this. […]
Does the above comment apply to JSL as well. I am trying to find the similar statement in JSL, but I am not able to locate it. May be I am not searching in the right place.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44187 )
Change subject: soc/intel/common/cse_lite: Perform a board specific reset ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44187/6/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/44187/6/src/soc/intel/common/block/... PS6, Line 379: cse_board_reset
Does the above comment apply to JSL as well. […]
Yes, it will apply to JSL as well. It is an architectural register common across all x86 platforms.
-
Edward O'Callaghan (Code Review) -
Furquan Shaikh (Code Review)
-
Justin TerAvest (Code Review)
-
Karthik Ramasubramanian (Code Review)
-
Nick Vaccaro (Code Review)
-
Sridhar Siricilla (Code Review)
-
Tim Wawrzynczak (Code Review)