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 8:
(15 comments)
Patchset:
PS6:
There are additional references to LPC_TPM which we might fix now as well. […]
I have previously stated that I want to create a separate CL for renaming cr50_ to google_tpm_ (or gsc_) in function names, and also rename some files/directories.
I would also like to defer renaming of lpc_ in function names, log messages, etc. to a later CL, for the sake of being able to submit this one, before modifications to any mainboards create too many conflicts.
Patchset:
PS8: PTAL
File src/drivers/i2c/tpm/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/63424/comment/c8b5a440_75817b9d PS6, Line 3: ifeq ($(CONFIG_I2C_TPM),y)
Might as well wrap all lines in the file in this? […]
Done
File src/drivers/pc80/tpm/Kconfig:
https://review.coreboot.org/c/coreboot/+/63424/comment/0092a02d_5a400c79 PS6, Line 5: LPC
If we are touching it now I would get rid if 'LPC' completely. […]
Done
File src/drivers/tpm/Makefile.inc:
PS6:
This whole Makefile should also be wrapped in a CONFIG_TPM check.
Done
File src/mainboard/google/herobrine/Kconfig:
https://review.coreboot.org/c/coreboot/+/63424/comment/d6d0013e_ae0e5127 PS6, Line 28: select TPM_GOOGLE_CR50
This needs an `if !BOARD_GOOGLE_SENOR`
Done, thanks for catching this.
File src/mainboard/google/volteer/mainboard.c:
https://review.coreboot.org/c/coreboot/+/63424/comment/2d881188_6fb9a2a3 PS6, Line 87: if (!CONFIG(SPI_TPM)) {
This doesn't seem to make a difference for the currently available configurations, but I think it wo […]
Good point, done.
File src/mainboard/siemens/mc_apl1/variants/mc_apl2/gpio.c:
https://review.coreboot.org/c/coreboot/+/63424/comment/fe98a83d_87c82702 PS6, Line 594: /* CLK_25M_MEMORY_MAPPED_TPM_CPU */
Yes, plese skip this one.
I guess I was a little too trigger happy with `perl -pi -e 's/.../'`.
File src/security/tpm/tss/vendor/cr50/Kconfig:
https://review.coreboot.org/c/coreboot/+/63424/comment/d1a55d49_2789429d PS6, Line 10: depends on TPM2
These shouldn't actually depend on TPM2 now, and neither should TPM_GOOGLE. […]
Good point, done.
https://review.coreboot.org/c/coreboot/+/63424/comment/df68775d_276bbffa PS6, Line 12: default n
nit: you don't need to specify `default n`, but if you do it's customary to put it above the `select […]
Done
https://review.coreboot.org/c/coreboot/+/63424/comment/3b060c6f_2df82ee8 PS6, Line 20: OOGLE_CR50 || TPM_GOOGLE_TI50
Just TPM_GOOGLE?
Yes, definitely. This code must have been from a time before I introduced the common TPM_GOOGLE.
File src/soc/intel/apollolake/Kconfig:
https://review.coreboot.org/c/coreboot/+/63424/comment/aa130823_877e03a9 PS6, Line 155: TPM part is conntected on Fast SPI interface, but the LPC MMIO : TPM transactions are decoded and serialized over the SPI interface.
Maybe rephrase this to get rid of the 'LPC' part as well? Something like: […]
Done
File src/soc/intel/common/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/63424/comment/073dbb8c_0e278c26 PS6, Line 23: all-$(CONFIG_TPM_GOOGLE) += tpm_tis.c
nit: a bit off-topic, but does Ti50 actually still require waiting for the sideband interrupt before […]
I do not think the H1 hardware allows us to get rid of the ready signal, before Cr50 can accept the next transaction.
File src/vendorcode/google/chromeos/Kconfig:
https://review.coreboot.org/c/coreboot/+/63424/comment/39f24ba1_575dbef4 PS6, Line 25: config TPM_GOOGLE_IMMEDIATELY_COMMIT_FW_SECDATA
Also a bit off-topic but it would probably be good to move this to tss/vendor/google. […]
I am unsure how it works when a `config` declaration sits inside a condition such as `if CHROMEOS` here, and is then used in code in cases where CHROMEOS is not enabled. It seems to be used that way, so I assume that it will act as disabled in such cases.
Based on the above, I have moved the declaration into `if TPM_GOOGLE` in security/tpm/tss/vendor/cr50 (which ought to be renamed to ../vendor/google in a followup), and given it a straight `default y`, which I assume then only applies if `TPM_GOOGLE` is enabled, and it will default to `n` otherwise. Please speak up if that is incorrect.
File src/vendorcode/google/chromeos/cse_board_reset.c:
https://review.coreboot.org/c/coreboot/+/63424/comment/06c713cb_971dc6b5 PS6, Line 19: if (CONFIG(SPI_TPM) && CONFIG(TPM_GOOGLE_CR50)) {
Is there a reason to specifically check SPI_TPM here? Looks to me like the important factor here was […]
The details elude me, but I seem to recall that getting the Cr50 version via I2C not working or not being implemented, meaning that the version comparison as done below cannot be relied upon if using I2C. I think I want to keep the logic the same, that is, let I2C designs always fall back as if Cr50 did not have the required support.