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