Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39693 )
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
drivers/spi/tpm: Add support for non CR50 SPI TPM2
Add support for a STM SPI TPM2 by adding checks for CR50. Tested using ST33HTPH2E32.
Change-Id: I015497ca078979a44ba2b84e4995493de1f7247b Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/drivers/spi/tpm/Kconfig M src/drivers/spi/tpm/tis.c M src/drivers/spi/tpm/tpm.c M src/security/tpm/Kconfig 4 files changed, 46 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/39693/1
diff --git a/src/drivers/spi/tpm/Kconfig b/src/drivers/spi/tpm/Kconfig index be43e23..8c4bace 100644 --- a/src/drivers/spi/tpm/Kconfig +++ b/src/drivers/spi/tpm/Kconfig @@ -19,3 +19,10 @@ select SPI_TPM help Board has SPI TPM support + +config MAINBOARD_HAS_SPI_TPM + bool + default n + select SPI_TPM + help + Board has SPI TPM support diff --git a/src/drivers/spi/tpm/tis.c b/src/drivers/spi/tpm/tis.c index 6230751..60dc705 100644 --- a/src/drivers/spi/tpm/tis.c +++ b/src/drivers/spi/tpm/tis.c @@ -18,6 +18,7 @@ } dev_map[] = { { 0x15d1, 0x001b, "SLB9670" }, { 0x1ae0, 0x0028, "CR50" }, + { 0x104a, 0x0000, "ST33HTPH2E32" }, };
static const char *tis_get_dev_name(struct tpm2_info *info) diff --git a/src/drivers/spi/tpm/tpm.c b/src/drivers/spi/tpm/tpm.c index 62d1bba..7db92d5 100644 --- a/src/drivers/spi/tpm/tpm.c +++ b/src/drivers/spi/tpm/tpm.c @@ -104,7 +104,7 @@ */ static int start_transaction(int read_write, size_t bytes, unsigned int addr) { - spi_frame_header header; + spi_frame_header header, header_resp; uint8_t byte; int i; struct stopwatch sw; @@ -114,7 +114,7 @@ * First Cr50 access in each coreboot stage where TPM is used will be * prepended by a wake up pulse on the CS line. */ - int wakeup_needed = 1; + int wakeup_needed = CONFIG(TPM2_CR50);
/* Wait for TPM to finish previous transaction if needed */ if (tpm_sync_needed) { @@ -181,26 +181,28 @@ * transmitted by the TPM during the transaction's last byte. * * We know that cr50 is guaranteed to set the flow control bit to 0 - * during the header transfer, but real TPM2 might be fast enough not - * to require to stall the master, this would present an issue. + * during the header transfer. Real TPM2 are fast enough to not require + * to stall the master. They might still use this feature, so test the + * last bit after shifting in the address bytes. * crosbug.com/p/52132 has been opened to track this. */ - spi_xfer(&spi_slave, header.body, sizeof(header.body), NULL, 0); - - /* - * Now poll the bus until TPM removes the stall bit. Give it up to 100 - * ms to sort it out - it could be saving stuff in nvram at some - * point. - */ - stopwatch_init_msecs_expire(&sw, 100); - do { - if (stopwatch_expired(&sw)) { - printk(BIOS_ERR, "TPM flow control failure\n"); - spi_release_bus(&spi_slave); - return 0; - } - spi_xfer(&spi_slave, NULL, 0, &byte, 1); - } while (!(byte & 1)); + spi_xfer(&spi_slave, header.body, sizeof(header.body), &header_resp, sizeof(header_resp.body)); + if (CONFIG(TPM2_CR50) || !(header_resp.body[3] & 1)) { + /* + * Now poll the bus until TPM removes the stall bit. Give it up to 100 + * ms to sort it out - it could be saving stuff in nvram at some + * point. + */ + stopwatch_init_msecs_expire(&sw, 100); + do { + if (stopwatch_expired(&sw)) { + printk(BIOS_ERR, "TPM flow control failure\n"); + spi_release_bus(&spi_slave); + return 0; + } + spi_xfer(&spi_slave, NULL, 0, &byte, 1); + } while (!(byte & 1)); + } return 1; }
@@ -408,7 +410,8 @@
/* Device/vendor ID values of the TPM devices this driver supports. */ static const uint32_t supported_did_vids[] = { - 0x00281ae0 /* H1 based Cr50 security chip. */ + 0x00281ae0, /* H1 based Cr50 security chip. */ + 0x0000104a /* ST33HTPH2E32 */ };
int tpm2_init(struct spi_slave *spi_if) @@ -454,7 +457,7 @@
printk(BIOS_INFO, " done!\n");
- if (ENV_VERSTAGE || ENV_BOOTBLOCK) + if (ENV_VERSTAGE || ENV_BOOTBLOCK || !CONFIG(VBOOT)) /* * Claim locality 0, do it only during the first * initialization after reset. @@ -462,7 +465,10 @@ if (!tpm2_claim_locality()) return -1;
- read_tpm_sts(&status); + if (!read_tpm_sts(&status)) { + printk(BIOS_ERR, "Reading status reg failed\n"); + return -1; + } if ((status & TPM_STS_FAMILY_MASK) != TPM_STS_FAMILY_TPM_2_0) { printk(BIOS_ERR, "unexpected TPM family value, status: %#x\n", status); diff --git a/src/security/tpm/Kconfig b/src/security/tpm/Kconfig index 1766939..7baff87 100644 --- a/src/security/tpm/Kconfig +++ b/src/security/tpm/Kconfig @@ -26,7 +26,14 @@ default y if MAINBOARD_HAS_TPM2 || USER_TPM2 depends on MAINBOARD_HAS_I2C_TPM_GENERIC || MAINBOARD_HAS_LPC_TPM \ || MAINBOARD_HAS_I2C_TPM_ATMEL || MAINBOARD_HAS_I2C_TPM_CR50 \ - || MAINBOARD_HAS_SPI_TPM_CR50 || MAINBOARD_HAS_CRB_TPM + || MAINBOARD_HAS_SPI_TPM_CR50 || MAINBOARD_HAS_CRB_TPM \ + || MAINBOARD_HAS_SPI_TPM +config TPM2_CR50 + bool + default y if MAINBOARD_HAS_TPM2 || USER_TPM2 + depends on MAINBOARD_HAS_I2C_TPM_CR50 || MAINBOARD_HAS_SPI_TPM_CR50 + help + A software TPM2 running in Google's security chip.
config MAINBOARD_HAS_TPM1 bool @@ -56,7 +63,7 @@ bool "2.0" depends on MAINBOARD_HAS_I2C_TPM_GENERIC || MAINBOARD_HAS_LPC_TPM \ || MAINBOARD_HAS_I2C_TPM_ATMEL || MAINBOARD_HAS_I2C_TPM_CR50 \ - || MAINBOARD_HAS_SPI_TPM_CR50 || MAINBOARD_HAS_CRB_TPM + || MAINBOARD_HAS_SPI_TPM_CR50 || MAINBOARD_HAS_CRB_TPM || MAINBOARD_HAS_SPI_TPM help Enable this option to enable TPM 2.0 support in coreboot.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39693 )
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/tpm.c@1... PS1, Line 189: spi_xfer(&spi_slave, header.body, sizeof(header.body), &header_resp, sizeof(header_resp.body)); line over 96 characters
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39693 )
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
Patch Set 1: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/tpm.c@1... PS1, Line 189: spi_xfer(&spi_slave, header.body, sizeof(header.body), &header_resp, sizeof(header_resp.body));
line over 96 characters
Could we fix this somehow?
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/tpm.c@1... PS1, Line 194: point. nit: fits on the previous line
https://review.coreboot.org/c/coreboot/+/39693/1/src/security/tpm/Kconfig File src/security/tpm/Kconfig:
https://review.coreboot.org/c/coreboot/+/39693/1/src/security/tpm/Kconfig@29 PS1, Line 29: || MAINBOARD_HAS_SPI_TPM_CR50 || MAINBOARD_HAS_CRB_TPM \ Uh, these lines are getting a bit too long
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39693 )
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/Kconfig File src/drivers/spi/tpm/Kconfig:
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/Kconfig... PS1, Line 19: select SPI_TPM I think this should select MAINBOARD_HAS_SPI_TPM, and then things in the other Kconfig file might become a little less cluttered (because you don't have to mention MAINBOARD_HAS_SPI_TPM_CR50 separately everywhere).
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/tpm.c@8... PS1, Line 85: static int tpm_sync(void) This whole out-of-band interrupt syncing thing is Cr50-specific, you'll want to bypass it for other TPMs.
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/tpm.c@1... PS1, Line 186: * last bit after shifting in the address bytes. The problem is that many of our SPI drivers don't support full-duplex communication. I think changing this everywhere will break things. Can we write this such that it will only run full-duplex when CONFIG(TPM2_CR50) is false?
header_resp[3] = 0; if (CONFIG(TPM2_CR50)) ret = spi_xfer(&spi_slave, header.body, sizeof(header.body), NULL, 0); else ret = spi_xfer(&spi_slave, header.body, sizeof(header.body), header_resp.body, sizeof(header_resp.body)); if (ret) ... print error and fail ... if (header_resp.body[3] & 1) return 1; ...do the polling...
(Also, while we're here, I don't know why the return values of those spi_xfer()s aren't checked, they really should be.)
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/tpm.c@1... PS1, Line 190: (header_resp.body[3] & 1) I haven't read the spec closely, but are you sure that the ready bit can only be set in the last byte of the header (not an earlier one)?
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/tpm.c@4... PS1, Line 460: if (ENV_VERSTAGE || ENV_BOOTBLOCK || !CONFIG(VBOOT)) Hmm... this is making an existing hacky situation even hackier.
Can we maybe change the whole TPM stack to only call tis_init() once in the right stage (or split it into things that need to be done once and things that need to be done every stage)? We do have a single tpm_setup() function already that's only called once, we already have to carry all the work of knowing when we talk to the TPM for the first time. We just don't manage to pipe it through all the way here. That's stupid.
I'll admit that would be a bigger change so I don't want to block this CL with it but it would be an important cleanup.
https://review.coreboot.org/c/coreboot/+/39693/1/src/security/tpm/Kconfig File src/security/tpm/Kconfig:
https://review.coreboot.org/c/coreboot/+/39693/1/src/security/tpm/Kconfig@31 PS1, Line 31: config TPM2_CR50 We already have TPM_CR50 in tss/vendor/cr50/Kconfig
Hello Philipp Deppenwiese, build bot (Jenkins), Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39693
to look at the new patch set (#2).
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
drivers/spi/tpm: Add support for non CR50 SPI TPM2
Add support for a STM SPI TPM2 by adding checks for CR50. Tested using ST33HTPH2E32.
Change-Id: I015497ca078979a44ba2b84e4995493de1f7247b Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/drivers/spi/tpm/Kconfig M src/drivers/spi/tpm/tis.c M src/drivers/spi/tpm/tpm.c M src/security/tpm/Kconfig 4 files changed, 42 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/39693/2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39693 )
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
Patch Set 1:
(7 comments)
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/Kconfig File src/drivers/spi/tpm/Kconfig:
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/Kconfig... PS1, Line 19: select SPI_TPM
I think this should select MAINBOARD_HAS_SPI_TPM, and then things in the other Kconfig file might be […]
Done
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/tpm.c@1... PS1, Line 186: * last bit after shifting in the address bytes.
The problem is that many of our SPI drivers don't support full-duplex communication. […]
Done
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/tpm.c@1... PS1, Line 189: spi_xfer(&spi_slave, header.body, sizeof(header.body), &header_resp, sizeof(header_resp.body));
Could we fix this somehow?
Done
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/tpm.c@1... PS1, Line 190: (header_resp.body[3] & 1)
I haven't read the spec closely, but are you sure that the ready bit can only be set in the last byt […]
that's what the spec says. The last byte of the address has the ready bit set.
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/tpm.c@1... PS1, Line 194: point.
nit: fits on the previous line
Done
https://review.coreboot.org/c/coreboot/+/39693/1/src/security/tpm/Kconfig File src/security/tpm/Kconfig:
https://review.coreboot.org/c/coreboot/+/39693/1/src/security/tpm/Kconfig@29 PS1, Line 29: || MAINBOARD_HAS_SPI_TPM_CR50 || MAINBOARD_HAS_CRB_TPM \
Uh, these lines are getting a bit too long
there's no limit for Kconfig, is there?
https://review.coreboot.org/c/coreboot/+/39693/1/src/security/tpm/Kconfig@31 PS1, Line 31: config TPM2_CR50
We already have TPM_CR50 in tss/vendor/cr50/Kconfig
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39693 )
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39693/1/src/security/tpm/Kconfig File src/security/tpm/Kconfig:
https://review.coreboot.org/c/coreboot/+/39693/1/src/security/tpm/Kconfig@29 PS1, Line 29: || MAINBOARD_HAS_SPI_TPM_CR50 || MAINBOARD_HAS_CRB_TPM \
there's no limit for Kconfig, is there?
Well, I don't understand why everything is aligned after "MAINBOARD_HAS_I2C_TPM_GENERIC". It looks pretty odd.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39693 )
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39693/2/src/drivers/spi/tpm/Kconfig File src/drivers/spi/tpm/Kconfig:
https://review.coreboot.org/c/coreboot/+/39693/2/src/drivers/spi/tpm/Kconfig... PS2, Line 21: Board has SPI TPM support nit: should say something different from the one below
https://review.coreboot.org/c/coreboot/+/39693/2/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/39693/2/src/drivers/spi/tpm/tpm.c@1... PS2, Line 198: SPI transfer error\n nit: Mention something about this being in the TPM driver?
https://review.coreboot.org/c/coreboot/+/39693/2/src/drivers/spi/tpm/tpm.c@2... PS2, Line 203: !CONFIG(TPM_CR50) && ( CONFIG() test is unnecessary here.
https://review.coreboot.org/c/coreboot/+/39693/1/src/security/tpm/Kconfig File src/security/tpm/Kconfig:
https://review.coreboot.org/c/coreboot/+/39693/1/src/security/tpm/Kconfig@29 PS1, Line 29: || MAINBOARD_HAS_SPI_TPM_CR50 || MAINBOARD_HAS_CRB_TPM \
Well, I don't understand why everything is aligned after "MAINBOARD_HAS_I2C_TPM_GENERIC". […]
I think the intention is still that Kconfig files adhere to the usual 80 (or now 96) character limit. Of course, they're currently probably not all compliant with that...
I agree that this looks odd and aligning after "depends on " would probably be better.
Christian Walter has uploaded a new patch set (#3) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/39693 )
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
drivers/spi/tpm: Add support for non CR50 SPI TPM2
Add support for a STM SPI TPM2 by adding checks for CR50. Tested using ST33HTPH2E32.
Change-Id: I015497ca078979a44ba2b84e4995493de1f7247b Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/drivers/spi/tpm/Kconfig M src/drivers/spi/tpm/tis.c M src/drivers/spi/tpm/tpm.c M src/security/tpm/Kconfig 4 files changed, 46 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/39693/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39693 )
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39693/3/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/39693/3/src/drivers/spi/tpm/tpm.c@1... PS3, Line 189: spi_xfer(&spi_slave, header.body, sizeof(header.body), &header_resp, sizeof(header_resp.body)); line over 96 characters
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39693 )
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39693/2/src/drivers/spi/tpm/Kconfig File src/drivers/spi/tpm/Kconfig:
https://review.coreboot.org/c/coreboot/+/39693/2/src/drivers/spi/tpm/Kconfig... PS2, Line 16: MAINBOARD_HAS_SPI_TPM_CR50 while at it, can we change MAINBOARD_HAS_SPI_TPM_CR50 to MAINBOARD_HAS_SPI_TPM? CR50 is not alone anymore
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39693 )
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39693/4/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/39693/4/src/drivers/spi/tpm/tpm.c@1... PS4, Line 189: spi_xfer(&spi_slave, header.body, sizeof(header.body), &header_resp, sizeof(header_resp.body)); line over 96 characters
Hello Philipp Deppenwiese, build bot (Jenkins), Christian Walter, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39693
to look at the new patch set (#5).
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
drivers/spi/tpm: Add support for non CR50 SPI TPM2
Add support for a STM SPI TPM2 by adding checks for CR50. Tested using ST33HTPH2E32.
Change-Id: I015497ca078979a44ba2b84e4995493de1f7247b Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/drivers/spi/tpm/Kconfig M src/drivers/spi/tpm/tis.c M src/drivers/spi/tpm/tpm.c M src/security/tpm/Kconfig 4 files changed, 90 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/39693/5
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39693 )
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
Patch Set 4:
(8 comments)
https://review.coreboot.org/c/coreboot/+/39693/2/src/drivers/spi/tpm/Kconfig File src/drivers/spi/tpm/Kconfig:
https://review.coreboot.org/c/coreboot/+/39693/2/src/drivers/spi/tpm/Kconfig... PS2, Line 16: MAINBOARD_HAS_SPI_TPM_CR50
while at it, can we change MAINBOARD_HAS_SPI_TPM_CR50 to MAINBOARD_HAS_SPI_TPM? CR50 is not alone an […]
Refactoring that part should not be done in this commit. While at it the MAINBOARD_HAS_I2C_TPM_CR50 needs to be updated, too.
https://review.coreboot.org/c/coreboot/+/39693/2/src/drivers/spi/tpm/Kconfig... PS2, Line 21: Board has SPI TPM support
nit: should say something different from the one below
Done
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/tpm.c@8... PS1, Line 85: static int tpm_sync(void)
This whole out-of-band interrupt syncing thing is Cr50-specific, you'll want to bypass it for other […]
Done
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/tpm.c@1... PS1, Line 190: (header_resp.body[3] & 1)
that's what the spec says. The last byte of the address has the ready bit set.
Done
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/tpm.c@4... PS1, Line 460: if (ENV_VERSTAGE || ENV_BOOTBLOCK || !CONFIG(VBOOT))
Hmm... this is making an existing hacky situation even hackier. […]
Added a FIXME
https://review.coreboot.org/c/coreboot/+/39693/2/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/39693/2/src/drivers/spi/tpm/tpm.c@1... PS2, Line 198: SPI transfer error\n
nit: Mention something about this being in the TPM driver?
Done
https://review.coreboot.org/c/coreboot/+/39693/2/src/drivers/spi/tpm/tpm.c@2... PS2, Line 203: !CONFIG(TPM_CR50) && (
CONFIG() test is unnecessary here.
Done
https://review.coreboot.org/c/coreboot/+/39693/1/src/security/tpm/Kconfig File src/security/tpm/Kconfig:
https://review.coreboot.org/c/coreboot/+/39693/1/src/security/tpm/Kconfig@29 PS1, Line 29: || MAINBOARD_HAS_SPI_TPM_CR50 || MAINBOARD_HAS_CRB_TPM \
I think the intention is still that Kconfig files adhere to the usual 80 (or now 96) character limit […]
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39693 )
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/39693/1/src/drivers/spi/tpm/tpm.c@4... PS1, Line 460: if (ENV_VERSTAGE || ENV_BOOTBLOCK || !CONFIG(VBOOT))
Added a FIXME
Ack
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39693 )
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
Patch Set 5: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/39693/5/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/39693/5/src/drivers/spi/tpm/tpm.c@1... PS5, Line 126: * During the first invocation of this function on each stage The alignment on these comments broke, it seems 😞
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39693 )
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
Patch Set 5: Code-Review+1
Hello Philipp Deppenwiese, build bot (Jenkins), Christian Walter, Angel Pons, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39693
to look at the new patch set (#6).
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
drivers/spi/tpm: Add support for non CR50 SPI TPM2
Add support for a STM SPI TPM2 by adding checks for CR50. Tested using ST33HTPH2E32.
Change-Id: I015497ca078979a44ba2b84e4995493de1f7247b Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/drivers/spi/tpm/Kconfig M src/drivers/spi/tpm/tis.c M src/drivers/spi/tpm/tpm.c M src/security/tpm/Kconfig 4 files changed, 89 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/39693/6
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39693 )
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39693/5/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/39693/5/src/drivers/spi/tpm/tpm.c@1... PS5, Line 126: * During the first invocation of this function on each stage
The alignment on these comments broke, it seems 😞
Done
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39693 )
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
Patch Set 6: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39693 )
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
Patch Set 6: Code-Review+2
Philipp Deppenwiese has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39693 )
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
drivers/spi/tpm: Add support for non CR50 SPI TPM2
Add support for a STM SPI TPM2 by adding checks for CR50. Tested using ST33HTPH2E32.
Change-Id: I015497ca078979a44ba2b84e4995493de1f7247b Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/39693 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Philipp Deppenwiese zaolin.daisuki@gmail.com Reviewed-by: Julius Werner jwerner@chromium.org --- M src/drivers/spi/tpm/Kconfig M src/drivers/spi/tpm/tis.c M src/drivers/spi/tpm/tpm.c M src/security/tpm/Kconfig 4 files changed, 89 insertions(+), 49 deletions(-)
Approvals: build bot (Jenkins): Verified Philipp Deppenwiese: Looks good to me, approved Julius Werner: Looks good to me, approved
diff --git a/src/drivers/spi/tpm/Kconfig b/src/drivers/spi/tpm/Kconfig index be43e23..8c39a4a 100644 --- a/src/drivers/spi/tpm/Kconfig +++ b/src/drivers/spi/tpm/Kconfig @@ -16,6 +16,13 @@ config MAINBOARD_HAS_SPI_TPM_CR50 bool default n + select MAINBOARD_HAS_SPI_TPM + help + Board has a CR50 SPI TPM + +config MAINBOARD_HAS_SPI_TPM + bool + default n select SPI_TPM help Board has SPI TPM support diff --git a/src/drivers/spi/tpm/tis.c b/src/drivers/spi/tpm/tis.c index 6230751..60dc705 100644 --- a/src/drivers/spi/tpm/tis.c +++ b/src/drivers/spi/tpm/tis.c @@ -18,6 +18,7 @@ } dev_map[] = { { 0x15d1, 0x001b, "SLB9670" }, { 0x1ae0, 0x0028, "CR50" }, + { 0x104a, 0x0000, "ST33HTPH2E32" }, };
static const char *tis_get_dev_name(struct tpm2_info *info) diff --git a/src/drivers/spi/tpm/tpm.c b/src/drivers/spi/tpm/tpm.c index 8f93e2a..c47aed3 100644 --- a/src/drivers/spi/tpm/tpm.c +++ b/src/drivers/spi/tpm/tpm.c @@ -104,47 +104,51 @@ */ static int start_transaction(int read_write, size_t bytes, unsigned int addr) { - spi_frame_header header; + spi_frame_header header, header_resp; uint8_t byte; int i; + int ret; struct stopwatch sw; static int tpm_sync_needed; static struct stopwatch wake_up_sw; - /* - * First Cr50 access in each coreboot stage where TPM is used will be - * prepended by a wake up pulse on the CS line. - */ - int wakeup_needed = 1;
- /* Wait for TPM to finish previous transaction if needed */ - if (tpm_sync_needed) { - tpm_sync(); + if (CONFIG(TPM_CR50)) { /* - * During the first invocation of this function on each stage - * this if () clause code does not run (as tpm_sync_needed - * value is zero), during all following invocations the - * stopwatch below is guaranteed to be started. + * First Cr50 access in each coreboot stage where TPM is used will be + * prepended by a wake up pulse on the CS line. */ - if (!stopwatch_expired(&wake_up_sw)) - wakeup_needed = 0; - } else { - tpm_sync_needed = 1; - } + int wakeup_needed = 1;
- if (wakeup_needed) { - /* Just in case Cr50 is asleep. */ - spi_claim_bus(&spi_slave); - udelay(1); - spi_release_bus(&spi_slave); - udelay(100); - } + /* Wait for TPM to finish previous transaction if needed */ + if (tpm_sync_needed) { + tpm_sync(); + /* + * During the first invocation of this function on each stage + * this if () clause code does not run (as tpm_sync_needed + * value is zero), during all following invocations the + * stopwatch below is guaranteed to be started. + */ + if (!stopwatch_expired(&wake_up_sw)) + wakeup_needed = 0; + } else { + tpm_sync_needed = 1; + }
- /* - * The Cr50 on H1 does not go to sleep for 1 second after any - * SPI slave activity, let's be conservative and limit the - * window to 900 ms. - */ - stopwatch_init_msecs_expire(&wake_up_sw, 900); + if (wakeup_needed) { + /* Just in case Cr50 is asleep. */ + spi_claim_bus(&spi_slave); + udelay(1); + spi_release_bus(&spi_slave); + udelay(100); + } + + /* + * The Cr50 on H1 does not go to sleep for 1 second after any + * SPI slave activity, let's be conservative and limit the + * window to 900 ms. + */ + stopwatch_init_msecs_expire(&wake_up_sw, 900); + }
/* * The first byte of the frame header encodes the transaction type @@ -181,16 +185,30 @@ * transmitted by the TPM during the transaction's last byte. * * We know that cr50 is guaranteed to set the flow control bit to 0 - * during the header transfer, but real TPM2 might be fast enough not - * to require to stall the master, this would present an issue. + * during the header transfer. Real TPM2 are fast enough to not require + * to stall the master. They might still use this feature, so test the + * last bit after shifting in the address bytes. * crosbug.com/p/52132 has been opened to track this. */ - spi_xfer(&spi_slave, header.body, sizeof(header.body), NULL, 0); + + header_resp.body[3] = 0; + if (CONFIG(TPM_CR50)) + ret = spi_xfer(&spi_slave, header.body, sizeof(header.body), NULL, 0); + else + ret = spi_xfer(&spi_slave, header.body, sizeof(header.body), + header_resp.body, sizeof(header_resp.body)); + if (ret) { + printk(BIOS_ERR, "SPI-TPM: transfer error\n"); + spi_release_bus(&spi_slave); + return 0; + } + + if (header_resp.body[3] & 1) + return 1;
/* * Now poll the bus until TPM removes the stall bit. Give it up to 100 - * ms to sort it out - it could be saving stuff in nvram at some - * point. + * ms to sort it out - it could be saving stuff in nvram at some point. */ stopwatch_init_msecs_expire(&sw, 100); do { @@ -201,6 +219,7 @@ } spi_xfer(&spi_slave, NULL, 0, &byte, 1); } while (!(byte & 1)); + return 1; }
@@ -408,7 +427,8 @@
/* Device/vendor ID values of the TPM devices this driver supports. */ static const uint32_t supported_did_vids[] = { - 0x00281ae0 /* H1 based Cr50 security chip. */ + 0x00281ae0, /* H1 based Cr50 security chip. */ + 0x0000104a /* ST33HTPH2E32 */ };
int tpm2_init(struct spi_slave *spi_if) @@ -454,7 +474,8 @@
printk(BIOS_INFO, " done!\n");
- if (ENV_SEPARATE_VERSTAGE || ENV_BOOTBLOCK) + // FIXME: Move this to tpm_setup() + if (ENV_SEPARATE_VERSTAGE || ENV_BOOTBLOCK || !CONFIG(VBOOT)) /* * Claim locality 0, do it only during the first * initialization after reset. @@ -462,7 +483,10 @@ if (!tpm2_claim_locality()) return -1;
- read_tpm_sts(&status); + if (!read_tpm_sts(&status)) { + printk(BIOS_ERR, "Reading status reg failed\n"); + return -1; + } if ((status & TPM_STS_FAMILY_MASK) != TPM_STS_FAMILY_TPM_2_0) { printk(BIOS_ERR, "unexpected TPM family value, status: %#x\n", status); diff --git a/src/security/tpm/Kconfig b/src/security/tpm/Kconfig index d8652b2..359b8f1 100644 --- a/src/security/tpm/Kconfig +++ b/src/security/tpm/Kconfig @@ -18,15 +18,19 @@ config TPM1 bool default y if MAINBOARD_HAS_TPM1 || USER_TPM1 - depends on MAINBOARD_HAS_LPC_TPM || MAINBOARD_HAS_I2C_TPM_GENERIC \ - || MAINBOARD_HAS_I2C_TPM_ATMEL + depends on MAINBOARD_HAS_LPC_TPM || \ + MAINBOARD_HAS_I2C_TPM_GENERIC || \ + MAINBOARD_HAS_I2C_TPM_ATMEL
config TPM2 bool default y if MAINBOARD_HAS_TPM2 || USER_TPM2 - depends on MAINBOARD_HAS_I2C_TPM_GENERIC || MAINBOARD_HAS_LPC_TPM \ - || MAINBOARD_HAS_I2C_TPM_ATMEL || MAINBOARD_HAS_I2C_TPM_CR50 \ - || MAINBOARD_HAS_SPI_TPM_CR50 || MAINBOARD_HAS_CRB_TPM + depends on MAINBOARD_HAS_I2C_TPM_GENERIC || \ + MAINBOARD_HAS_LPC_TPM || \ + MAINBOARD_HAS_I2C_TPM_ATMEL || \ + MAINBOARD_HAS_I2C_TPM_CR50 || \ + MAINBOARD_HAS_SPI_TPM || \ + MAINBOARD_HAS_CRB_TPM
config MAINBOARD_HAS_TPM1 bool @@ -45,8 +49,9 @@
config USER_TPM1 bool "1.2" - depends on MAINBOARD_HAS_LPC_TPM || MAINBOARD_HAS_I2C_TPM_GENERIC \ - || MAINBOARD_HAS_I2C_TPM_ATMEL + depends on MAINBOARD_HAS_LPC_TPM || \ + MAINBOARD_HAS_I2C_TPM_GENERIC || \ + MAINBOARD_HAS_I2C_TPM_ATMEL help Enable this option to enable TPM 1.0 - 1.2 support in coreboot.
@@ -54,9 +59,12 @@
config USER_TPM2 bool "2.0" - depends on MAINBOARD_HAS_I2C_TPM_GENERIC || MAINBOARD_HAS_LPC_TPM \ - || MAINBOARD_HAS_I2C_TPM_ATMEL || MAINBOARD_HAS_I2C_TPM_CR50 \ - || MAINBOARD_HAS_SPI_TPM_CR50 || MAINBOARD_HAS_CRB_TPM + depends on MAINBOARD_HAS_I2C_TPM_GENERIC || \ + MAINBOARD_HAS_LPC_TPM || \ + MAINBOARD_HAS_I2C_TPM_ATMEL || \ + MAINBOARD_HAS_I2C_TPM_CR50 || \ + MAINBOARD_HAS_SPI_TPM || \ + MAINBOARD_HAS_CRB_TPM help Enable this option to enable TPM 2.0 support in coreboot.
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Christian Walter (Code Review)
-
Julius Werner (Code Review)
-
Michael Niewöhner (Code Review)
-
Patrick Rudolph (Code Review)
-
Philipp Deppenwiese (Code Review)