Attention is currently required from: Jes Klinke. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63285 )
Change subject: Factor TI50/CR50 config ......................................................................
Patch Set 2:
(2 comments)
Patchset:
PS2: Hmmm... okay, let's step back for a moment and try to look at the big picture. Because I feel like we are really drowing in a mess of different configs now that the previous method of organizing things doesn't really scale to. We have several different dimensions of things we need to express here (I2C vs SPI, Cr50 vs Ti50, etc.) and if we make a separate config for each dot on the cartesian product of all of them, they're just going to be way too many to handle.
The things we want to express are:
* user-declared choice of which type of TPM to use * -> this is `NO_TPM`, `TPM1` and `TPM2`, with `TPM` as the shorthand for the latter two * mainboard-selected default for the above choice * -> this is `MAINBOARD_HAS_TPM1` and `MAINBOARD_HAS_TPM2`. only exists to select the default for the above * which transport layer the TPM is connected with on the mainboard * -> I think we should go back to only having MAINBOARD_HAS_SPI_TPM and MAINBOARD_HAS_I2C_TPM here. I'm actually not sure if there's a useful difference between `I2C_TPM`/`SPI_TPM` and `MAINBOARD_HAS_I2C_TPM`/`MAINBOARD_HAS_SPI_TPM`... maybe we can cut one indirection layer out there and just use the former two * whether we're using a GSC variant, and which one * -> I think this should be a completely separate choice that's only based in the tss/vendor/cr50 (rename to tss/vendor/google?) directory.
So the way I'd imagine this, basically, a mainboard would select all of these separately:
select MAINBOARD_HAS_TPM2 select I2C_TPM select TPM_GOOGLE_TI50
other Kconfig symbols that get automatically turned on by these are `TPM2`, `TPM` and `TPM_GOOGLE`. In the Makefiles, if say you want to decide whether to build the Ti50 I2C driver, you could do something like
ifeq ($(CONFIG_I2C_TPM),y) all-$(CONFIG_TPM_GOOGLE) += cr50.c # now google.c? endif
I hope that might cut down on a bunch of the Kconfig madness. What do you think?
File src/drivers/i2c/tpm/Kconfig:
https://review.coreboot.org/c/coreboot/+/63285/comment/1091a33f_93b3c720 PS2, Line 17: GSC nit: This might be a good spot to write out what GSC stands for because most upstream coreboot developers are probably unfamiliar. Or should we just make it easier and call it "TPM_GOOGLE" instead?
-
Julius Werner (Code Review)