Karthik Ramasubramanian has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44276 )
Change subject: drivers/spi/tpm: Re-organize the TPM Initialization ......................................................................
drivers/spi/tpm: Re-organize the TPM Initialization
Re-organize the tpm_init2 to facilitate caching the TPM firmware. Also read the TPM firmware version only during ramstage where it will be used. In other stages, it is read, logged and dropped.
BUG=None TEST=Ensure that the device boots to OS. Ensure that the TPM initialization sequence works as expected.
Change-Id: I94fef8ed813f4c0482b23ba513465faaf1ceae9f Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/drivers/spi/tpm/tpm.c 1 file changed, 58 insertions(+), 46 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/44276/1
diff --git a/src/drivers/spi/tpm/tpm.c b/src/drivers/spi/tpm/tpm.c index 4263dce..b44c87e 100644 --- a/src/drivers/spi/tpm/tpm.c +++ b/src/drivers/spi/tpm/tpm.c @@ -34,6 +34,8 @@
#define CR50_TIMEOUT_INIT_MS 30000 /* Very long timeout for TPM init */
+#define TPM_FW_VER_STRLEN 301 + /* SPI slave structure for TPM device. */ static struct spi_slave spi_slave;
@@ -421,23 +423,46 @@ return 0; }
+static void read_tpm_fw_version(void) +{ + char vstr[TPM_FW_VER_STRLEN]; + int chunk_count = 0; + /* + * let's read 50 bytes at a time; leave room for the trailing + * zero. + */ + size_t chunk_size = 50; + + /* Only cr50 reports device FW version */ + if (tpm_info.vendor_id != 0x1ae0) + return; + + /* + * Does not really matter what's written, this just makes sure + * the version is reported from the beginning. + */ + tpm2_write_reg(TPM_FW_VER, vstr, 1); + + do { + tpm2_read_reg(TPM_FW_VER, + vstr + chunk_count * chunk_size, chunk_size); + chunk_count++; + } while (vstr[chunk_count * chunk_size - 1] && + (chunk_count + 1) * chunk_size < sizeof(vstr)); + vstr[chunk_count * chunk_size] = 0; + printk(BIOS_INFO, "Firmware version: %s\n", vstr); +} + /* Device/vendor ID values of the TPM devices this driver supports. */ static const uint32_t supported_did_vids[] = { 0x00281ae0, /* H1 based Cr50 security chip. */ 0x0000104a /* ST33HTPH2E32 */ };
-int tpm2_init(struct spi_slave *spi_if) +static int probe_tpm(uint32_t *did_vid) { - uint32_t did_vid, status; - uint8_t cmd; int retries;
- memcpy(&spi_slave, spi_if, sizeof(*spi_if)); - - /* clear any pending IRQs */ - tis_plat_irq_status(); - /* * 150 ms should be enough to synchronize with the TPM even under the * worst nested reset request conditions. In vast majority of cases @@ -447,11 +472,11 @@ for (retries = 15; retries > 0; retries--) { int i;
- /* In case of failure to read div_vid is set to zero. */ - tpm2_read_reg(TPM_DID_VID_REG, &did_vid, sizeof(did_vid)); + /* In case of failure to read did_vid is set to zero. */ + tpm2_read_reg(TPM_DID_VID_REG, did_vid, sizeof(*did_vid));
for (i = 0; i < ARRAY_SIZE(supported_did_vids); i++) - if (did_vid == supported_did_vids[i]) + if (*did_vid == supported_did_vids[i]) break; /* TPM is up and ready. */
if (i < ARRAY_SIZE(supported_did_vids)) @@ -469,6 +494,22 @@ }
printk(BIOS_INFO, " done!\n"); + return 0; +} + +int tpm2_init(struct spi_slave *spi_if) +{ + uint32_t did_vid, status; + uint8_t cmd; + + memcpy(&spi_slave, spi_if, sizeof(*spi_if)); + + /* clear any pending IRQs */ + tis_plat_irq_status(); + + /* Probe TPM and get the did_vid */ + if (probe_tpm(&did_vid)) + return -1;
// FIXME: Move this to tpm_setup() if (ENV_SEPARATE_VERSTAGE || ENV_BOOTBLOCK || !CONFIG(VBOOT)) @@ -501,41 +542,12 @@ printk(BIOS_INFO, "Connected to device vid:did:rid of %4.4x:%4.4x:%2.2x\n", tpm_info.vendor_id, tpm_info.device_id, tpm_info.revision);
- /* Let's report device FW version if available. */ - if (tpm_info.vendor_id == 0x1ae0) { - int chunk_count = 0; - size_t chunk_size; - /* - * let's read 50 bytes at a time; leave room for the trailing - * zero. - */ - char vstr[51]; - - chunk_size = sizeof(vstr) - 1; - - printk(BIOS_INFO, "Firmware version: "); - - /* - * Does not really matter what's written, this just makes sure - * the version is reported from the beginning. - */ - tpm2_write_reg(TPM_FW_VER, &chunk_size, 1); - - /* Print it out in sizeof(vstr) - 1 byte chunks. */ - vstr[chunk_size] = 0; - do { - tpm2_read_reg(TPM_FW_VER, vstr, chunk_size); - printk(BIOS_INFO, "%s", vstr); - - /* - * While string is not over, and is no longer than 300 - * characters. - */ - } while (vstr[chunk_size - 1] && - (chunk_count++ < (300 / chunk_size))); - - printk(BIOS_INFO, "\n"); - } + /* + * Firmware version is used only in ramstage. In other stages it is + * read, logged and dropped. So read FW version only in ramstage. + */ + if (ENV_RAMSTAGE) + read_tpm_fw_version(); return 0; }
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44276 )
Change subject: drivers/spi/tpm: Re-organize the TPM Initialization ......................................................................
Patch Set 2:
Are you aware of Jes' work (CB:43741) or did the two of you just accidentally implement the same thing at the same time? This series is doing a bunch of things that I thought were handled better in Jes' version, I'm not sure if those were intentional changes?
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44276 )
Change subject: drivers/spi/tpm: Re-organize the TPM Initialization ......................................................................
Patch Set 2:
Patch Set 2:
Are you aware of Jes' work (CB:43741) or did the two of you just accidentally implement the same thing at the same time? This series is doing a bunch of things that I thought were handled better in Jes' version, I'm not sure if those were intentional changes?
I was aware of the requirement to cache the firmware version, but was not aware of the CL CB:43741. If the shared goals between these 2 CLs i.e. parsing and caching the CR50 Firmware version, can be split out as a separate CL, then we can drop this CL.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44276 )
Change subject: drivers/spi/tpm: Re-organize the TPM Initialization ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
Are you aware of Jes' work (CB:43741) or did the two of you just accidentally implement the same thing at the same time? This series is doing a bunch of things that I thought were handled better in Jes' version, I'm not sure if those were intentional changes?
I was aware of the requirement to cache the firmware version, but was not aware of the CL CB:43741. If the shared goals between these 2 CLs i.e. parsing and caching the CR50 Firmware version, can be split out as a separate CL, then we can drop this CL.
Looked at Jes's CL in detail now. Posted my comments there.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44276 )
Change subject: drivers/spi/tpm: Re-organize the TPM Initialization ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/44276/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44276/2//COMMIT_MSG@14 PS2, Line 14: device Which one?
https://review.coreboot.org/c/coreboot/+/44276/2//COMMIT_MSG@15 PS2, Line 15: initialization sequence works as expected. How?
https://review.coreboot.org/c/coreboot/+/44276/2//COMMIT_MSG@16 PS2, Line 16: Did you notice any decrease in boot time?
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44276 )
Change subject: drivers/spi/tpm: Re-organize the TPM Initialization ......................................................................
Patch Set 2: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/44276/2/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/44276/2/src/drivers/spi/tpm/tpm.c@4... PS2, Line 450: } while (vstr[chunk_count * chunk_size - 1] && : (chunk_count + 1) * chunk_size < sizeof(vstr)); I find this loop construct overly complex for what it is trying to achieve. At minimum I would take the upper bound portion of this boundary predicate and make it into a if-break construction in the loop body.
https://review.coreboot.org/c/coreboot/+/44276/2/src/drivers/spi/tpm/tpm.c@4... PS2, Line 452: 0 '\0' as vstr is char typed.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44276 )
Change subject: drivers/spi/tpm: Re-organize the TPM Initialization ......................................................................
Patch Set 2: Code-Review-2
As Julius pointed out earlier, CB:43741 CL stack is chosen as the candidate to cache and re-use the TPM Firmware version.
I have to abandon this CL in favor of that one. So please post any comments regarding the TPM firmware version in that CL stack. Sorry for the inconvenience.
Karthik Ramasubramanian has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/44276 )
Change subject: drivers/spi/tpm: Re-organize the TPM Initialization ......................................................................
Abandoned
Abandoning since the required support landed as part of CB:43741
-
Edward O'Callaghan (Code Review) -
Julius Werner (Code Review)
-
Karthik Ramasubramanian (Code Review)
-
Paul Menzel (Code Review)