[coreboot-gerrit] Change in coreboot[master]: drivers/i2c/tpm/cr50: Simplify and increase init delay to 30 seconds
Vadim Bendebury (Code Review)
gerrit at coreboot.org
Tue Nov 21 18:45:08 CET 2017
Vadim Bendebury has uploaded this change for review. ( https://review.coreboot.org/22554
Change subject: drivers/i2c/tpm/cr50: Simplify and increase init delay to 30 seconds
......................................................................
drivers/i2c/tpm/cr50: Simplify and increase init delay to 30 seconds
The Cr50 i2c driver provides separate entry points for probing and
initialization, but probing function does not really do much.
It also claims and releases locality on every coreboot stage, but
there is no need for this - locality should be claimed in the
beginning and retained through the boot process.
On top of that the driver does not properly account for long time it
could take the Cr50 chip to come around to reset processing if TPM
reset request was posted during a lengthy TPM operation.
This patch addresses the issues as follows:
- tpm_vendor_probe() and tpm_vendor_cleanup() become noops, kept
around to conform to the expected driver API.
- tpm_vendor_init() invokes request_locality() only when
initializing the TPM after reset (typically during verstage)
- request_locality() checks if locality is taken, and if so - waits
for it to be released, which is an indication of TPM finishing
reset processing.
BRANCH=none
BUG=b:65867313, b:68729265
TEST=Verified that reef no longer hangs during EC reboot and
firmware_Cr50ClearTPMOwner (not yet merged) tests.
Change-Id: Iba8445caf1342e3a5fefcb2664b0759a1a8e84e3
Signed-off-by: Vadim Bendebury <vbendeb at chromium.org>
---
M src/drivers/i2c/tpm/cr50.c
1 file changed, 60 insertions(+), 82 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/22554/1
diff --git a/src/drivers/i2c/tpm/cr50.c b/src/drivers/i2c/tpm/cr50.c
index 9cc9e95..38d9b73 100644
--- a/src/drivers/i2c/tpm/cr50.c
+++ b/src/drivers/i2c/tpm/cr50.c
@@ -170,57 +170,68 @@
return cr50_i2c_wait_tpm_ready(chip);
}
-static int check_locality(struct tpm_chip *chip, int loc)
+static int request_locality(struct tpm_chip *chip)
{
- uint8_t mask = TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY;
- uint8_t buf;
+ struct stopwatch sw;
+ uint8_t access;
- if (cr50_i2c_read(chip, TPM_ACCESS(loc), &buf, 1) < 0)
- return -1;
+ /*
+ * Locality is released by TPM reset.
+ *
+ * If locality is taken at this point, this could be due to the fact
+ * that the TPM is performing a long operation and has not processed
+ * reset request yet. We'll wait up to CR50_TIMEOUT_INIT_MS and see if
+ * it releases locality when reset is processed.
+ */
+ stopwatch_init_msecs_expire(&sw, CR50_TIMEOUT_INIT_MS);
+ do {
+ int rv;
+ uint8_t mask = TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY;
- if ((buf & mask) == mask) {
- chip->vendor.locality = loc;
- return loc;
- }
+ rv = cr50_i2c_read(chip, TPM_ACCESS(0),
+ &access, sizeof(access));
+ if (rv || ((access & mask) == mask)) {
+ /*
+ * Don't bombard the chip with traffic, let it keep
+ * processing the command.
+ */
+ mdelay(2);
+ continue;
+ }
+
+ /*
+ * Ok, the locality is free, TPM must be reset, let's claim
+ * it.
+ */
+ access = TPM_ACCESS_REQUEST_USE;
+ if (cr50_i2c_write(chip, TPM_ACCESS(0),
+ &access, sizeof(access)) < 0) {
+ printk(BIOS_ERR, "%s:%d failed requesting locality\n",
+ __func__, __LINE__);
+ return -1;
+ }
+
+ rv = cr50_i2c_read(chip, TPM_ACCESS(0),
+ &access, sizeof(access));
+ if (rv || ((access & mask) != mask)) {
+ printk(BIOS_ERR,
+ "%s:%d failed requesting locality, "
+ "rv=%d, access %#x\n",
+ __func__, __LINE__, rv, access);
+ return -1;
+ }
+
+ printk(BIOS_INFO, "TPM ready after %ld ms\n",
+ stopwatch_duration_msecs(&sw));
+
+ return 0;
+ } while (!stopwatch_expired(&sw));
+
+ printk(BIOS_ERR,
+ "Failed to claim locality 0 after %ld ms, status: %#x\n",
+ stopwatch_duration_msecs(&sw), access);
return -1;
-}
-
-static void release_locality(struct tpm_chip *chip, int force)
-{
- uint8_t mask = TPM_ACCESS_VALID | TPM_ACCESS_REQUEST_PENDING;
- uint8_t addr = TPM_ACCESS(chip->vendor.locality);
- uint8_t buf;
-
- if (cr50_i2c_read(chip, addr, &buf, 1) < 0)
- return;
-
- if (force || (buf & mask) == mask) {
- buf = TPM_ACCESS_ACTIVE_LOCALITY;
- cr50_i2c_write(chip, addr, &buf, 1);
- }
-
- chip->vendor.locality = 0;
-}
-
-static int request_locality(struct tpm_chip *chip, int loc)
-{
- uint8_t buf = TPM_ACCESS_REQUEST_USE;
- struct stopwatch sw;
-
- if (check_locality(chip, loc) >= 0)
- return loc;
-
- if (cr50_i2c_write(chip, TPM_ACCESS(loc), &buf, 1) < 0)
- return -1;
-
- stopwatch_init_msecs_expire(&sw, CR50_TIMEOUT_LONG_MS);
- while (check_locality(chip, loc) < 0) {
- if (stopwatch_expired(&sw))
- return -1;
- mdelay(CR50_TIMEOUT_SHORT_MS);
- }
- return loc;
}
/* cr50 requires all 4 bytes of status register to be read */
@@ -419,38 +430,6 @@
int tpm_vendor_probe(unsigned int bus, uint32_t addr)
{
- struct tpm_inf_dev *tpm_dev = car_get_var_ptr(&g_tpm_dev);
- struct tpm_chip probe_chip;
- struct stopwatch sw;
- uint8_t buf = 0;
- int ret;
- long sw_run_duration = CR50_TIMEOUT_INIT_MS;
-
- tpm_dev->bus = bus;
- tpm_dev->addr = addr;
-
- cr50_vendor_init(&probe_chip);
-
- /* Wait for TPM_ACCESS register ValidSts bit to be set */
- stopwatch_init_msecs_expire(&sw, sw_run_duration);
- do {
- ret = cr50_i2c_read(&probe_chip, TPM_ACCESS(0), &buf, 1);
- if (!ret && (buf & TPM_STS_VALID)) {
- sw_run_duration = stopwatch_duration_msecs(&sw);
- break;
- }
- mdelay(CR50_TIMEOUT_SHORT_MS);
- } while (!stopwatch_expired(&sw));
-
- printk(BIOS_INFO,
- "%s: ValidSts bit %s(%d) in TPM_ACCESS register after %ld ms\n",
- __func__, (buf & TPM_STS_VALID) ? "set" : "clear",
- (buf & TPM_STS_VALID) >> 7, sw_run_duration);
-
- /* Claim failure if the ValidSts (bit 7) is clear */
- if (!(buf & TPM_STS_VALID))
- return -1;
-
return 0;
}
@@ -469,8 +448,9 @@
cr50_vendor_init(chip);
- if (request_locality(chip, 0) != 0)
- return -1;
+ if (ENV_VERSTAGE || ENV_BOOTBLOCK)
+ if (request_locality(chip))
+ return -1;
/* Read four bytes from DID_VID register */
if (cr50_i2c_read(chip, TPM_DID_VID(0), (uint8_t *)&vendor, 4) < 0)
@@ -488,11 +468,9 @@
return 0;
out_err:
- release_locality(chip, 1);
return -1;
}
void tpm_vendor_cleanup(struct tpm_chip *chip)
{
- release_locality(chip, 1);
}
--
To view, visit https://review.coreboot.org/22554
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iba8445caf1342e3a5fefcb2664b0759a1a8e84e3
Gerrit-Change-Number: 22554
Gerrit-PatchSet: 1
Gerrit-Owner: Vadim Bendebury <vbendeb at chromium.org>
Gerrit-Reviewer: Duncan Laurie <dlaurie at chromium.org>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20171121/3f570255/attachment.html>
More information about the coreboot-gerrit
mailing list