Attention is currently required from: Jes Klinke, Tim Wawrzynczak, Christian Walter, Julius Werner, Jett Rink. Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63158 )
Change subject: tpm: Accept Google Ti50 TPM DID:VID ......................................................................
Patch Set 12:
(10 comments)
File src/drivers/crb/tis.c:
https://review.coreboot.org/c/coreboot/+/63158/comment/445c1422_f63ee2ce PS6, Line 20: {0x6666, 0x50a4, "TI50"},
nit: I don't think this really needs to be here, and Cr50 should've never been here either. […]
Done
File src/drivers/i2c/tpm/cr50.c:
https://review.coreboot.org/c/coreboot/+/63158/comment/51c0b10d_48c0ef7b PS6, Line 478: gsc
It's a bit odd to change only this and none of the other instances of "cr50" in here. […]
I have reverted the naming to cr50_ in this and other source files, even though many of the methods apply also to ti50.
https://review.coreboot.org/c/coreboot/+/63158/comment/a92cc49b_c03fe6a4 PS6, Line 509: cr50_set_board_cfg(); /* No-op for Ti50. */
Does this mean we send the command and Ti50 just ignores it? That sounds like a waste of boot time. […]
No, by no-op I mean that internally cr50_set_board_cfg() checks that if we have a Cr50 earlier than 0.5.5 or a Ti50, then the GSC does not support a board_cfg register, and thus does not send any TPM commands in the bus.
File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/63158/comment/6d55c8e1_0ffdedb1 PS6, Line 422: H1 based
Is it?
I guess it is H1D3-based (updated now), I assumed that H1 could be taken to be any generation of the GSC.
File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/63158/comment/d4c33bee_ad2b3177 PS11, Line 422: H1D3
nit: H1D3C
Done
File src/drivers/tpm/cr50.h:
https://review.coreboot.org/c/coreboot/+/63158/comment/f9d18af5_afb74b8e PS6, Line 11: GSC_TI50 = 2,
I'm wondering if we really need to handle this differentiation at runtime? Long-term (beyond the cur […]
I have added a refactoring CL which adds a CONFIG_TPM_TI50 next to CONFIG_TPM_CR50, and also a CONFIG_TPM_GSC which will be set if either of the former are set.
File src/drivers/tpm/cr50.c:
https://review.coreboot.org/c/coreboot/+/63158/comment/784be590_c322bddd PS6, Line 249: INFO
ERR?
Done, here and two other otherwise untouched places.
File src/drivers/tpm/cr50.c:
https://review.coreboot.org/c/coreboot/+/63158/comment/28065ddb_b969e062 PS11, Line 109: if (!CONFIG(TPM_CR50)) : return 0;
let's add a comment to why we are skipping here (e.g. […]
Done
File src/mainboard/google/brya/Kconfig:
https://review.coreboot.org/c/coreboot/+/63158/comment/e620d82d_5e17880e PS11, Line 32: MAINBOARD_HAS_I2C_TPM_TI50
Brya boards do not have Ti50 on them in general, this should go in BASEBOARD_NISSA I believe
That is right, thanks.
I know that Nissa and Brya variant has Ti50, what about Brask?
https://review.coreboot.org/c/coreboot/+/63158/comment/53a2d4c4_25da5734 PS11, Line 58: BOARD_GOOGLE_BASEBOARD_NISSA
shouldn't we add `MAINBOARD_HAS_I2C_TPM_TI50` to this list if we are removing the workaround?
Done
-
Jes Klinke (Code Review)