[coreboot-gerrit] Change in coreboot[master]: tpm: Refactor TPM Kconfig dimensions

[Jes Klinke (Code Review)]

Attention is currently required from: Stefan Ott, Felix Singer, Patrick Rudolph, Jonathan Zhang, Nick Vaccaro, Arthur Heymans, Andrey Petrov, Piotr Król, Nico Huber, Sean Rhodes, Michał Żygowski, Johnny Lin, Christian Walter, Werner Zeh, Alexander Couzens, Yu-Ping Wu, Tim Chu, Frans Hendriks, Tristan Corrick, Jeremy Soller, Julius Werner, Angel Pons, Michael Niewöhner, Tim Crawford, Maxim Polyakov, Tim Wawrzynczak.
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63424 )

Change subject: tpm: Refactor TPM Kconfig dimensions
......................................................................

Patch Set 13:

(5 comments)

File src/mainboard/google/volteer/mainboard.c:

https://review.coreboot.org/c/coreboot/+/63424/comment/7f39702f_467c61b2 
PS10, Line 87: CONFIG(TPM_GOOGLE_CR50)
> Missing negation, it needs to be !CR50. […]

Good catch again.  Actually, with the new _CR50/_TI50 split, the SPI_TPM part could be dropped here.  However, I am not completely sure that we do not have some devices declaring _CR50, even though a Ti50 chip is on the daugtherboard, or something.

File src/soc/intel/common/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/63424/comment/74a5768a_47f4a682 
PS6, Line 23: all-$(CONFIG_TPM_GOOGLE) += tpm_tis.c
> Right, I'm just asking if that changed for D1/Ti50?

H1D3x/Ti50 should have the hardware support to serve SPI read transactions and stretch the clock on I2C transactions, such that the "ready interrupt" is not needed.  For a variety of reasons, we have chosen to continue using the same convention as Cr50, though.  In the future, this may change, and we could implement the official TPM specification, that allows the AP to configure the circumstances under which it wants to be notified by a sideband signal.  Probably this will happen when we put the Ti50 codebase on the future OpenTitan chips.

File src/soc/intel/tigerlake/Kconfig:

https://review.coreboot.org/c/coreboot/+/63424/comment/36042091_c865a83a 
PS10, Line 93: # in all low power states.  Cr50 TPM, if used, needs to be told to generate longer pulses.
> nit: I don't think this comment says anything that the help text for CR50_USE_LONG_INTERRUPT_PULSES  […]

Done

File src/vendorcode/google/chromeos/Kconfig:

https://review.coreboot.org/c/coreboot/+/63424/comment/3576f2e0_bc27829b 
PS6, Line 25: config TPM_GOOGLE_IMMEDIATELY_COMMIT_FW_SECDATA
> Yes, […]

Thanks for the explanation, it makes sense that the if blocks works just as if "depends on" were added to each of the declarations inside.  So there is no such thing as declarations inside if blocks being "undefined" when the if condition is not met.  Good to know.

File src/vendorcode/google/chromeos/cse_board_reset.c:

https://review.coreboot.org/c/coreboot/+/63424/comment/858ee5b9_02da7433 
PS10, Line 39: (CONFIG(TPM2)
> I don't think this check needs to be here. […]

Done