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

[Julius Werner (Code Review)]

Attention is currently required from: Stefan Ott, Felix Singer, Patrick Rudolph, Jonathan Zhang, Nick Vaccaro, Arthur Heymans, Andrey Petrov, Piotr Król, Jes Klinke, 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, Angel Pons, Michael Niewöhner, Tim Crawford, Maxim Polyakov, Tim Wawrzynczak.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63424 )

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

Patch Set 10:

(5 comments)

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

https://review.coreboot.org/c/coreboot/+/63424/comment/7b5ebe2a_a6987ee1 
PS10, Line 87: CONFIG(TPM_GOOGLE_CR50)
Missing negation, it needs to be !CR50. This check skips enabling the log pulses, and we only need to enable them explicitly for CR50.

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

https://review.coreboot.org/c/coreboot/+/63424/comment/a9e8035b_8bea0807 
PS6, Line 23: all-$(CONFIG_TPM_GOOGLE) += tpm_tis.c
> I do not think the H1 hardware allows us to get rid of the ready signal, before Cr50 can accept the  […]

Right, I'm just asking if that changed for D1/Ti50?

File src/soc/intel/tigerlake/Kconfig:

https://review.coreboot.org/c/coreboot/+/63424/comment/e8cc9678_9e3b8f9e 
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 doesn't already say, and breaking up this select list with comments looks ugly

File src/vendorcode/google/chromeos/Kconfig:

https://review.coreboot.org/c/coreboot/+/63424/comment/bdecdc07_89ba3e5e 
PS6, Line 25: config TPM_GOOGLE_IMMEDIATELY_COMMIT_FW_SECDATA
> I am unsure how it works when a `config` declaration sits inside a condition such as `if CHROMEOS` h […]

Yes,

if CHROMEOS

config XXX
   ...

is 100% identical syntactic sugar for

config XXX
   depends on CHROMEOS
   ...

configs where a dependency is not met are forced to 0/n/"" regardless of all other factors.

In this case you did exactly the right thing.

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

https://review.coreboot.org/c/coreboot/+/63424/comment/bd34221c_da744782 
PS10, Line 39: (CONFIG(TPM2)
I don't think this check needs to be here. It needs to be above, because the cr50_get_firmware_version() call can only work if CONFIG(TPM2) has been enabled. But down here, we only care about the hardware design of the board, and I believe(?) the Ti50-specific PLTRST routing should still work the same on that same board and solve the problem here even if the coreboot TPM driver is not enabled.