[coreboot-gerrit] Change in coreboot[master]: google/oak: support cr50 over i2c on rowan

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/19364 )

Change subject: google/oak: support cr50 over i2c on rowan
......................................................................


Patch Set 1:

(5 comments)

https://review.coreboot.org/#/c/19364/1/src/mainboard/google/oak/Kconfig
File src/mainboard/google/oak/Kconfig:

Line 22: 	bool "Build for a board revision with a SPI TPM 2.0"
> I don't think this needs to be user configurable. It should just select bas
The equivalent for Gru needed a string because we had Kevins of both flavors. Assuming we don't need that for Rowan then yes, it can go.


PS1, Line 66: 	default 0x20 if !OAK_HAS_TPM2
            : 	default 0x50 if OAK_HAS_TPM2
> I’d welcome a note in the commit message, why the address has to be changed
Cr50 is a completely different chip from the SLB9645 that is used on other Oak boards. I2C addresses are often hardcoded into the chip, so naturally it's a different one. (I guess the option could also be called OAK_HAS_CR50 if you wanted it more specific, but this matches what we did for Gru and there are no plans of using yet another different TPM chip for Oak boards in the future.)


https://review.coreboot.org/#/c/19364/1/src/mainboard/google/oak/chromeos.c
File src/mainboard/google/oak/chromeos.c:

PS1, Line 47: Cr50
Let's use "TPM interrupt" which already has precedent on Gru.


Line 59: int tis_plat_irq_status(void)
Technically this has the same CONFIG_CHROMEOS issue Aaron mentioned in the other patch, so maybe it should go in a separate tpm.c file?


https://review.coreboot.org/#/c/19364/1/src/mainboard/google/oak/gpio.h
File src/mainboard/google/oak/gpio.h:

Line 45: #if IS_ENABLED(CONFIG_OAK_HAS_TPM2)
No need to guard a constant.


-- 
To view, visit https://review.coreboot.org/19364
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If6cdd0e39e4ac86538f27f322c55c329179ee084
Gerrit-PatchSet: 1
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Daniel Kurtz <djkurtz at google.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Daniel Kurtz <djkurtz at chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner at chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Vadim Bendebury <vbendeb at chromium.org>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: Yes