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 18:
(26 comments)
https://review.coreboot.org/c/coreboot/+/43741/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43741/11//COMMIT_MSG@6 PS11, Line 6:
This change should be split into multiple CLs for each logical change being made.
Ack
https://review.coreboot.org/c/coreboot/+/43741/11//COMMIT_MSG@7 PS11, Line 7: Tigerlake
Tiger Lake
Done
https://review.coreboot.org/c/coreboot/+/43741/11//COMMIT_MSG@7 PS11, Line 7: Enable long cr50 ready pulses for Tigerlake systems
Missing prefix.
Done
https://review.coreboot.org/c/coreboot/+/43741/11//COMMIT_MSG@20 PS11, Line 20: Bug: b:154333137
Please summarize the bug in the commit message.
I have modified the first paragraph of the comment message to capture the main requirement.
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 44: tpm_board_cfg
I would call this (and probably also the Kconfig) cr50_board_cfg because it has nothing to do with o […]
Ack
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 355: tpm_board_cfg
Do we really need to stash this? I think we can just read it when a caller wants it using tpm2_read_ […]
I have changed the logic to do version number check, and conditional reading of Cr50 register "on demand" from this function.
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 441: version[0]
Wasn't there a thing where the Cr50 team uses the major number to distinguish between dev and stable […]
It is something like what you describe. 0.5.3 is the stable release that corresponds to dev version 0.6.3, that is why I say that anything 0.7 or above is OK, while 0.5.4+ and 0.6.4+ are also OK.
I will double check with the Cr50 team, whether 0.{5,6}.4 is the right cutoff.
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 452: TMP
TPM
Done
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 457: uint32_t status;
I find it pretty confusing that this is called 'status'... […]
Clearly, I had spent too little thought on the code I copied from read_tpm_sts(). I have renamed the variable to board_cfg_value. Here and elsewhere.
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 466: 0x80000000U
Can you please define a macro for this rather than using a magic value here?
Done
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 470: 0xC0000000U
Same for this.
Done
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 474: ? "Inconsistency!: " : ""),
Probably want to print this as a separate BIOS_ERR line if it happens and start with "ERROR: " to ma […]
Done
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 511: /*const*/
What's this?
I wanted to indicate to readers that this method does not modify its argument. But the skip_atoi() function demands a non-const pointer. It seems however, that through the questionable signature of strstr(), we are able to declare this argument as const for real.
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 541: memset(cr50_firmware_version, 0, sizeof(cr50_firmware_version));
nit: unnecessary (globals are already zero-initialized)
Thanks.
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 621: /* : * Read chunk_size bytes at a time, last chunk will be zero : * padded. : */ : do {
Update on my previous comment: […]
Correct, the firmware version will not be read and parsed multiple times in the same stage.
I agree that there may be merit in avoiding the repeated logging of Cr50 firmware version. However, since cr50 is heavily involved in the functioning of verstage. When diagnosing problems causing verstage to fail to load a ramstage, it could be useful to know the version of Cr50. So I am not sure that dropping the logging from verstage has no disadvantages, and hence, I would like to not couple it to this functional change.
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 638: load_cr50_board_cfg();
I would only do this when get_cr50_board_cfg() is called, not in every stage.
Done
https://review.coreboot.org/c/coreboot/+/43741/11/src/include/string.h File src/include/string.h:
https://review.coreboot.org/c/coreboot/+/43741/11/src/include/string.h@32 PS11, Line 32: char *strstr(const char *haystack, const char *needle);
Please add this as a separate patch.
Done. https://review.coreboot.org/c/coreboot/+/44085
https://review.coreboot.org/c/coreboot/+/43741/11/src/lib/string.c File src/lib/string.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/lib/string.c@170 PS11, Line 170: if (!strncmp(haystack, needle, needle_len))
The needle may hit '\0' before the haystack. That will cause strcmp to return *haystack - '\0'. […]
I think I agree with Julius that the current way of using strstr is probably the simplest way, that is, a random future reader of the code will have easiest time of completely understanding how the code works by merely glancing over it.
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... File src/mainboard/google/volteer/Kconfig:
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 60: config TPM_BOARD_CFG : default 0x1
Instead of making the mainboard provide a specific value for TPM_BOARD_CFG, I think it would be bett […]
Done
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... File src/mainboard/google/volteer/chromeos.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 6:
This change should go in a separate CL.
Done, please post further comments about the volteer directory in: https://review.coreboot.org/c/coreboot/+/44359
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 46: 0x01
Use macros please.
Done
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 45: tlcl_lib_init(); : if (get_cr50_board_cfg() & 0x01) {
I think it would be better to have some helper functions: cr50_has_long_pulse() which returns true u […]
I have added a method get_cr50_ready_pulse_length_us() to drivers/spi/tpm/tpm.c, that way, all knowledge of the meaning of each bit of the board_cfg register (even the existence of such a register), is encapsulated in tpm.c.
However, methods in that file should not call tlcl_lib_init, in my opinion, so I have left the explicit initialization call above.
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 52: 0x80
It would be good to add some macros for this as well.
Done
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 55:
nit: extra blank lines not required.
Done
https://review.coreboot.org/c/coreboot/+/43741/11/src/security/tpm/tss/vendo... File src/security/tpm/tss/vendor/cr50/Kconfig:
https://review.coreboot.org/c/coreboot/+/43741/11/src/security/tpm/tss/vendo... PS11, Line 15: TPM_BOARD_CFG
As mentioned on the mainboard file, I think it would be better to add Kconfig for the specific featu […]
Done
https://review.coreboot.org/c/coreboot/+/43741/11/src/soc/intel/common/block... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/soc/intel/common/block... PS11, Line 261: static void gspi_clear_base_stash(void *unused)
The problem turned to be not the FSP-S invocation, but the change in BAR that happens because of res […]
I have created https://review.coreboot.org/c/coreboot/+/44084