Attention is currently required from: Jes Klinke, Tim Wawrzynczak, Christian Walter, Angel Pons, Jett Rink. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63158 )
Change subject: tpm: Allow separate handling of Google Ti50 TPM ......................................................................
Patch Set 16:
(6 comments)
File src/drivers/i2c/tpm/cr50.c:
https://review.coreboot.org/c/coreboot/+/63158/comment/d0019c38_ee0c0b24 PS6, Line 478: gsc
I have reverted the naming to cr50_ in this and other source files, even though many of the methods […]
It would be great if you provide a general naming overhaul in a follow-up patch!
File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/63158/comment/f27a7cd7_325d5c76 PS6, Line 422: H1 based
I guess it is H1D3-based (updated now), I assumed that H1 could be taken to be any generation of the […]
Oh okay, I didn't actually know the naming worked that way.
File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/63158/comment/902999ad_1209b57d PS16, Line 502: (tpm_info.vendor_id == 0x1ae0 FWIW I'm not sure why this check is here in the first place. We use CONFIG(TPM_GOOGLE) without extra runtime version checks in other critical parts of this driver already, it already wouldn't work if that option was set but the TPM wasn't actually a GSC. There's really no point in doing any of these checks here.
File src/drivers/tpm/cr50.c:
https://review.coreboot.org/c/coreboot/+/63158/comment/cb2297fd_75eec8fc PS16, Line 104: if (!CONFIG(TPM_GOOGLE_CR50)) So now you check the same Kconfig both in the caller and the callee... seems unnecessary? I think we should decide which of the two places make more sense and then only do it there (probably callee, I think).
https://review.coreboot.org/c/coreboot/+/63158/comment/74a0bc86_d12e49c5 PS16, Line 158: return false; Does this line have any purpose? We don't have a TPM_GOOGLE that's neither cr50 nor ti50 right now, and if we ever had one in the future it would most likely have the long pulses by default as well. So maybe just take the explicit TI50 check out and just always return true for !CR50?
File src/mainboard/google/brya/Kconfig:
https://review.coreboot.org/c/coreboot/+/63158/comment/5fbb4eda_9b1e0962 PS14, Line 47: TI50
Seems like that would be the case, yes. […]
Considering that this is a one-off case that we'll never have again after Ti50 has rolled out, I'd prefer not to make things more complicated just for that. I'm sure we can sufficiently support the Ti50 team with the special devices by just providing custom AP images for them in a Drive directory or something like that.
-
Julius Werner (Code Review)