Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34381 )
Change subject: security/tpm/tss: Add support for PTT ......................................................................
Patch Set 15: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/34381/12/src/security/tpm/tss/tcg-2... File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/c/coreboot/+/34381/12/src/security/tpm/tss/tcg-2... PS12, Line 195: if (CONFIG(HAVE_INTEL_PTT)) {
That's right. […]
Sorry, but I really disagree. The stack goes down from the high-level protocol stuff to the low-level platform implementation. This seems like a low-level platform detail. It doesn't belong in here. I mean, a big open-coded if (INTEL_STUFF) {...} block doesn't belong in here anyway... but if you encapsulated it properly, you'd have to create a function like platform_tpm_initialized() and call it here, and I just don't really see how that would make sense for any other platform than this one. All normal TPMs are connected through an external peripheral interface where it doesn't really make sense for the SoC to have some magic internal knowledge about the state of the TPM. So I think the CRB interface is really the only one for which something like this makes sense, and you should put it there.
Also, why are you calling this *after* tis_init() and tis_open() anyway? Wouldn't those fail already when the PTT is not active? Does the CRB interface not have an in-band mechanism to tell if the thing is active? That would really be a way better way to check this anyway.