Hello Jes Klinke,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/46436
to review the following change.
Change subject: volteer+vendorcode: Cr50 version code only via SPI ......................................................................
volteer+vendorcode: Cr50 version code only via SPI
No recent Chromebooks have used I2C for TPM communication, and as a result, a bug has crept in. The ability to extract Cr50 firmware string is only supported via SPI, yet code in mainboard and vendorcode attempts to do so unconditionally.
This CL makes it such that the code also compiles in case future designs will use I2C. (Whether we want to enhance the I2C protocol to be able to provide the version string, and then implement the support is a separate question.)
This effort is prompted by the desire to use reworked Volteer EVT devices for validating the new Ti50/Dauntless TPM, which will be connected via I2C in the upcoming designs.
TEST=util/abuild/abuild -t GOOGLE_VOLTEER -c max -x
Change-Id: Ida1d732e486b19bdff6d95062a3ac1a7c4b58b45 Signed-off-by: jbk@chromium.org --- M src/mainboard/google/volteer/mainboard.c M src/vendorcode/google/chromeos/cse_board_reset.c 2 files changed, 21 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/46436/1
diff --git a/src/mainboard/google/volteer/mainboard.c b/src/mainboard/google/volteer/mainboard.c index 1fcd5eb..1abb636 100644 --- a/src/mainboard/google/volteer/mainboard.c +++ b/src/mainboard/google/volteer/mainboard.c @@ -51,7 +51,8 @@ return; }
- if (cr50_is_long_interrupt_pulse_enabled()) { + if (CONFIG(MAINBOARD_HAS_SPI_TPM_CR50) + && cr50_is_long_interrupt_pulse_enabled()) { printk(BIOS_INFO, "Enabling S0i3.4\n"); } else { /* diff --git a/src/vendorcode/google/chromeos/cse_board_reset.c b/src/vendorcode/google/chromeos/cse_board_reset.c index 65e09ae..0b213a6 100644 --- a/src/vendorcode/google/chromeos/cse_board_reset.c +++ b/src/vendorcode/google/chromeos/cse_board_reset.c @@ -16,24 +16,27 @@ int ret; struct cr50_firmware_version version;
- /* Initialize TPM and get the cr50 firmware version. */ - ret = tlcl_lib_init(); - if (ret != VB2_SUCCESS) { - printk(BIOS_ERR, "tlcl_lib_init() failed: 0x%x\n", ret); - return; + if (CONFIG(MAINBOARD_HAS_SPI_TPM_CR50)) { + /* Initialize TPM and get the cr50 firmware version. */ + ret = tlcl_lib_init(); + if (ret != VB2_SUCCESS) { + printk(BIOS_ERR, "tlcl_lib_init() failed: 0x%x\n", ret); + return; + } + + cr50_get_firmware_version(&version); + + /* + * Cr50 firmware versions 0.[3|4].20 or newer support strap + * config 0xe where PLTRST from AP is connected to cr50's + * PLTRST# signal. So return immediately and trigger a global + * reset. + */ + if (version.epoch != 0 || version.major > 4 || + (version.major >= 3 && version.minor >= 20)) + return; }
- cr50_get_firmware_version(&version); - - /* - * Cr50 firmware versions 0.[3|4].20 or newer support strap config 0xe where PLTRST from - * AP is connected to cr50's PLTRST# signal. So return immediately and trigger a - * global reset. - */ - if (version.epoch != 0 || version.major > 4 || - (version.major >= 3 && version.minor >= 20)) - return; - printk(BIOS_INFO, "Initiating request to EC to trigger cold reset\n"); /* * Clean the data cache and set the full reset bit, so that when EC toggles
Hello Jes Klinke,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46436
to look at the new patch set (#2).
Change subject: volteer+vendorcode: Cr50 version code only via SPI ......................................................................
volteer+vendorcode: Cr50 version code only via SPI
No recent Chromebooks have used I2C for TPM communication, and as a result, a bug has crept in. The ability to extract Cr50 firmware string is only supported via SPI, yet code in mainboard and vendorcode attempts to do so unconditionally.
This CL makes it such that the code also compiles in case future designs will use I2C. (Whether we want to enhance the I2C protocol to be able to provide the version string, and then implement the support is a separate question.)
This effort is prompted by the desire to use reworked Volteer EVT devices for validating the new Ti50/Dauntless TPM, which will be connected via I2C in the upcoming designs.
TEST=util/abuild/abuild -t GOOGLE_VOLTEER -c max -x
Change-Id: Ida1d732e486b19bdff6d95062a3ac1a7c4b58b45 Signed-off-by: jbk@chromium.org --- M src/mainboard/google/volteer/mainboard.c M src/vendorcode/google/chromeos/cse_board_reset.c 2 files changed, 21 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/46436/2
Hello Jes Klinke,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46436
to look at the new patch set (#3).
Change subject: volteer+vendorcode: Cr50 version code only via SPI ......................................................................
volteer+vendorcode: Cr50 version code only via SPI
No recent Chromebooks have used I2C for TPM communication, and as a result, a bug has crept in. The ability to extract Cr50 firmware string is only supported via SPI, yet code in mainboard and vendorcode attempts to do so unconditionally.
This CL makes it such that the code also compiles in case future designs will use I2C. (Whether we want to enhance the I2C protocol to be able to provide the version string, and then implement the support is a separate question.)
This effort is prompted by the desire to use reworked Volteer EVT devices for validating the new Ti50/Dauntless TPM, which will be connected via I2C in the upcoming designs.
BRANCH=volteer TEST=util/abuild/abuild -t GOOGLE_VOLTEER -c max -x
Change-Id: Ida1d732e486b19bdff6d95062a3ac1a7c4b58b45 Signed-off-by: jbk@chromium.org --- M src/mainboard/google/volteer/mainboard.c M src/vendorcode/google/chromeos/cse_board_reset.c 2 files changed, 21 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/46436/3
Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46436 )
Change subject: volteer+vendorcode: Cr50 version code only via SPI ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46436/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46436/3//COMMIT_MSG@7 PS3, Line 7: r50 version code only via SPI add a verb?
https://review.coreboot.org/c/coreboot/+/46436/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/mainboard.c:
https://review.coreboot.org/c/coreboot/+/46436/3/src/mainboard/google/voltee... PS3, Line 55: && the operator usually goes on the preceding line
Hello build bot (Jenkins), Caveh Jalali, Nick Vaccaro, Jes Klinke,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46436
to look at the new patch set (#4).
Change subject: volteer+vendorcode: Retrieve Cr50 version only via SPI ......................................................................
volteer+vendorcode: Retrieve Cr50 version only via SPI
No recent Chromebooks have used I2C for TPM communication, and as a result, a bug has crept in. The ability to extract Cr50 firmware string is only supported via SPI, yet code in mainboard and vendorcode attempt to do so unconditionally.
This CL makes it such that the code also compiles for future designs using I2C. (Whether we want to enhance the I2C protocol to be able to provide the version string, and then implement the support is a separate question.)
This effort is prompted by the desire to use reworked Volteer EVT devices for validating the new Ti50/Dauntless TPM. Dauntless will primarily be using I2C in upcoming designs.
BRANCH=volteer TEST=util/abuild/abuild -t GOOGLE_VOLTEER -c max -x
Change-Id: Ida1d732e486b19bdff6d95062a3ac1a7c4b58b45 Signed-off-by: jbk@chromium.org --- M src/mainboard/google/volteer/mainboard.c M src/vendorcode/google/chromeos/cse_board_reset.c 2 files changed, 21 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/46436/4
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46436 )
Change subject: volteer+vendorcode: Retrieve Cr50 version only via SPI ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46436/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46436/3//COMMIT_MSG@7 PS3, Line 7: r50 version code only via SPI
add a verb?
Done
https://review.coreboot.org/c/coreboot/+/46436/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/mainboard.c:
https://review.coreboot.org/c/coreboot/+/46436/3/src/mainboard/google/voltee... PS3, Line 55: &&
the operator usually goes on the preceding line
Done
Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46436 )
Change subject: volteer+vendorcode: Retrieve Cr50 version only via SPI ......................................................................
Patch Set 4: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46436 )
Change subject: volteer+vendorcode: Retrieve Cr50 version only via SPI ......................................................................
volteer+vendorcode: Retrieve Cr50 version only via SPI
No recent Chromebooks have used I2C for TPM communication, and as a result, a bug has crept in. The ability to extract Cr50 firmware string is only supported via SPI, yet code in mainboard and vendorcode attempt to do so unconditionally.
This CL makes it such that the code also compiles for future designs using I2C. (Whether we want to enhance the I2C protocol to be able to provide the version string, and then implement the support is a separate question.)
This effort is prompted by the desire to use reworked Volteer EVT devices for validating the new Ti50/Dauntless TPM. Dauntless will primarily be using I2C in upcoming designs.
BRANCH=volteer TEST=util/abuild/abuild -t GOOGLE_VOLTEER -c max -x
Change-Id: Ida1d732e486b19bdff6d95062a3ac1a7c4b58b45 Signed-off-by: jbk@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/46436 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Caveh Jalali caveh@chromium.org --- M src/mainboard/google/volteer/mainboard.c M src/vendorcode/google/chromeos/cse_board_reset.c 2 files changed, 21 insertions(+), 17 deletions(-)
Approvals: build bot (Jenkins): Verified Caveh Jalali: Looks good to me, approved
diff --git a/src/mainboard/google/volteer/mainboard.c b/src/mainboard/google/volteer/mainboard.c index 1fcd5eb..03a78fd 100644 --- a/src/mainboard/google/volteer/mainboard.c +++ b/src/mainboard/google/volteer/mainboard.c @@ -51,7 +51,8 @@ return; }
- if (cr50_is_long_interrupt_pulse_enabled()) { + if (CONFIG(MAINBOARD_HAS_SPI_TPM_CR50) && + cr50_is_long_interrupt_pulse_enabled()) { printk(BIOS_INFO, "Enabling S0i3.4\n"); } else { /* diff --git a/src/vendorcode/google/chromeos/cse_board_reset.c b/src/vendorcode/google/chromeos/cse_board_reset.c index 65e09ae..0b213a6 100644 --- a/src/vendorcode/google/chromeos/cse_board_reset.c +++ b/src/vendorcode/google/chromeos/cse_board_reset.c @@ -16,24 +16,27 @@ int ret; struct cr50_firmware_version version;
- /* Initialize TPM and get the cr50 firmware version. */ - ret = tlcl_lib_init(); - if (ret != VB2_SUCCESS) { - printk(BIOS_ERR, "tlcl_lib_init() failed: 0x%x\n", ret); - return; + if (CONFIG(MAINBOARD_HAS_SPI_TPM_CR50)) { + /* Initialize TPM and get the cr50 firmware version. */ + ret = tlcl_lib_init(); + if (ret != VB2_SUCCESS) { + printk(BIOS_ERR, "tlcl_lib_init() failed: 0x%x\n", ret); + return; + } + + cr50_get_firmware_version(&version); + + /* + * Cr50 firmware versions 0.[3|4].20 or newer support strap + * config 0xe where PLTRST from AP is connected to cr50's + * PLTRST# signal. So return immediately and trigger a global + * reset. + */ + if (version.epoch != 0 || version.major > 4 || + (version.major >= 3 && version.minor >= 20)) + return; }
- cr50_get_firmware_version(&version); - - /* - * Cr50 firmware versions 0.[3|4].20 or newer support strap config 0xe where PLTRST from - * AP is connected to cr50's PLTRST# signal. So return immediately and trigger a - * global reset. - */ - if (version.epoch != 0 || version.major > 4 || - (version.major >= 3 && version.minor >= 20)) - return; - printk(BIOS_INFO, "Initiating request to EC to trigger cold reset\n"); /* * Clean the data cache and set the full reset bit, so that when EC toggles
-
Caveh Jalali (Code Review) -
Jes Klinke (Code Review)
-
Patrick Georgi (Code Review)