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