Furquan Shaikh 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:
(2 comments)
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.
With respect to fixing the I2C driver, I think it's okay. We can do that in parallel. I am more concerned about ensuring that we capture the requirements/expectations correctly so that what is being implemented on the Ti50 firmware does not lead to more problems down the line.
https://review.coreboot.org/c/coreboot/+/47049/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47049/7//COMMIT_MSG@16 PS7, Line 16: I2C : controller base address
Again, the I2C driver appears to branch based on which stage pre-ramstage vs. […]
Yes, it is generally true that we wouldn't access I2C or SPI devices early on i.e. before resource allocation is complete. But, I think ensuring we have a way to handle it if required isn't bad. At minimum, I think there should be a bug to track it so that it can be fixed.
https://review.coreboot.org/c/coreboot/+/47049/7//COMMIT_MSG@19 PS7, Line 19: can be made to support : longer interrupts
Yes, by default, Dauntless will guarantee 100us interrupt pulse width and spacing. […]
Are these all points captured in some place? I want to make sure that as we are doing the validation we capture the expectations/assumptions correctly. Again, there should at least be a bug/doc to track this behavior so that we don't run into surprises with different hardware requirements.