Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47049 )
Change subject: mb/google/volteer: Skip TPM detection except on SPI ......................................................................
Patch Set 7:
(3 comments)
I understand your concerns Furquan. It would be nice to clean up the I2C driver code of its shortcomings, but question whether there is much benefit to doing so.
Right now, I am trying to enable other Dauntless developers to compile firmware for Volteer, for our reworked setup. And this workaround code, which was relevant only for Cr50, and will not be for Ti50, is getting in the way, so I hoped to be able to disable it.
https://review.coreboot.org/c/coreboot/+/47049/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47049/7//COMMIT_MSG@14 PS7, Line 14: The I2C drivers do not support : being accessed early in ramstage
So, shouldn't we fix the I2C drivers in that case?
That may depend on whether we have any real use case for it. The SPI drivers also had an issue here, specifically, the SPI driver did not like being accessed both before and after chip init, due to caching.
My point is that the only reason we accessed TPM early in RAM stage was this ugly workaround to handle Cr50 firmware possibly not supporting the long interrupt pulses. At some time in the future, all our Cr50 chips will support this from the factory, and we will no longer need to access Cr50 early in ramstage.
Ti50/Dauntless is newly developed, and we will make sure that it supports whatever interrupt requirements from the first firmware version, so we will not need the early ramstage detection for Ti50, (and I do not know of plans of putting Cr50 on I2C.)
My stance is that we can fix this when we need it, and we will likely never need it.
https://review.coreboot.org/c/coreboot/+/47049/7//COMMIT_MSG@16 PS7, Line 16: I2C : controller base address
It is because the I2C driver seems to be assuming that the BAR is allocated before I2C initializatio […]
Again, the I2C driver appears to branch based on which stage pre-ramstage vs. ramstage, which has worked so far, and as I argue above that we have no plans of accessing I2C in early ramstage, I think that though it is a shortcoming of the driver, we do not strictly need to fix it.
https://review.coreboot.org/c/coreboot/+/47049/7//COMMIT_MSG@19 PS7, Line 19: can be made to support : longer interrupts
Is that the default for Dauntless firmware i.e. […]
Yes, by default, Dauntless will guarantee 100us interrupt pulse width and spacing.
Due to the nature of I2C, and specifically "clock stretching", it is possible to modify the protocol and drivers to not even need a ready signal to be asserted before the host initiates each bus transaction. And that is very much our plan.
Furthermore, Andrey told me that the TPM standard specification actually calls for an interrupt line with different more high level semantics, probably something about the completion of long-running operations, rather than each bus transaction, so we may re-purpose the signal to fit that bill on Ti50. In any case, the Ti50 on I2C will definitely not have a register to request 100us interrupt pulses.