Furquan Shaikh 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 20:
(8 comments)
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 510: if (get_cr50_board_cfg() & CR50_BOARD_CFG_100US_READY_PULSE)
So far I have resisted, because I feel that this is a low level library, and that it is not appropriate for it to call the higher-level tlcl_lib.
Agreed. I was thinking of a wrapper in src/vendorcode/google/chromeos so that we don't add tlcl_* accesses in this driver. That way we don't really have to keep copying around the tlcl_lib_init() call in every place this function gets called. For now we can go ahead with tlcl_lib_init() in mainboard. Probably should revisit this later.
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 440: (version[1] >= 5 && version[2] >= 4)) { nit: Probably fits on the previous line within the 96-column limit.
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 444: " TPM_BOARD_CFG, version: %d.%d.%d\n", This can go on the previous line. printk() strings are allowed to go over the 96-column limit, so no need to split it.
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 459: &board_cfg_value, sizeof(board_cfg_value))) { nit: This probably fits on the previous line within the 96-column limit?
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 484: &board_cfg_value, sizeof(board_cfg_value))) nit: Fits on previous line.
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 498: &board_cfg_value, sizeof(board_cfg_value))) { nit: This probably fits on the previous line within the 96-column limit.
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 622: ; = { 0 };
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 638: + 1 This is not correct. chunk_count is already incremented in line 637.
Can this loop be simplified a bit? This math looks more complicated than required.
``` char *version_ptr = version_str; char *version_end = version_ptr + sizeof(version_str) - 1;
while (version_ptr < version_end) { tpm2_read_reg(TPM_FW_VER, version_ptr, chunk_size);
if (version_ptr[chunk_size - 1] == '\0') break;
version_ptr += chunk_size; } ```