Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Patch Set 11:
(11 comments)
Thanks, looks much better.
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 other TPMs.
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 versions or something (so 1.5.3 is the stable version corresponding to 0.5.3 or something)? Or am I misremembering things? Please double-check with them that this is really the right check for the feature.
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'... why not 'board_cfg' or something? (Or just use the tpm_board_cfg global directly?)
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 make it more noticeable?
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 511: /*const*/ What's this?
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)
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 631: 0 nit: I'd write this '\0' for clarity
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.
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.
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)) Why not just strcmp()?
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) I would make this an exported function that is called directly at the end of the FSP-S loading code, that's more precise than boot states (e.g. it could be possible that another device that runs after the SoC wants to access the TPM in its DEV_RESOURCES step).