Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 22:
(2 comments)
I am going to address the detail-oriented comments, but first, I would like to settle the debate on whether to have a global version varialble, or not.
I believe it makes sense to gather information about the tpm chip in the tpm2_init() method, and store it in global variables, from where it can be later retrieved by getter methods, (with the contract that it is invalid to call those getters before the library has been initialized.)
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 51: tatic int cr50_firmware_version[3];
Agreed. […]
Passing reference to the stack frame works in one of the two places in which is it used in this CL. get_cr50_long_interrupt_pulses() also needs to know the version, and will be called "from the outside".
The tpm.h header already declares the following methods (among others):
int tpm2_init(struct spi_slave *spi_if); void tpm2_get_info(struct tpm2_info *info);
It is the contract, and clearly documented, that the getter method can only be called after the library has been initialized by tpm2_init().
I am adding one more method get_cr50_long_interrupt_pulses() with the same contract, internally it uses a singleton (global variable) to remember the version of the cr50 chip in the system from previous initialization.
As Furquan mentions, we may need to directly expose the version to the outside in yet another getter reading from the global variable.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 434: static int cr50_supports_board_cfg(void)
`static int cr50_supports_board_cfg(struct cr50_ver ver)` is the correct signature. […]
As I pointed out above, the suggested signature would mean that static uint32_t get_cr50_board_cfg() would need to call cr50_supports_board_cfg(cr50_firmware_version), referring to the global variable.
While doing that change does not have significant drawbacks, I question the need for it, assuming that we will have the global variable. (Let's resolve the global variable discussion first.)