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