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 19:
(9 comments)
https://review.coreboot.org/c/coreboot/+/43741/19//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43741/19//COMMIT_MSG@20 PS19, Line 20: Bug: b:154333137 This needs to go before the Signed-off-by line. Also BUG=
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.h File src/drivers/spi/tpm/tpm.h:
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.h@... PS19, Line 45: get_cr50_ready_pulse_length_us Do we really need to pass a length back? I think this function can simply return a bool indicating whether cr50 supports long pulses.
bool cr50_supports_long_pulse(void);
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 451: constant value. nit: This probably fits on the previous line within the 96-column limit?
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 484: board_cfg_value, CR50_BOARD_CFG_VALUE); Should there be an early return if board_cfg_value == CR50_BOARD_CFG_VALUE?
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 628: char static?
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 645: sizeof(version_str) Shouldn't this be sizeof(version_str) - 1 to keep the logic same as before? Or was the change intentional?
https://review.coreboot.org/c/coreboot/+/43741/19/src/security/tpm/tss/vendo... File src/security/tpm/tss/vendor/cr50/Kconfig:
https://review.coreboot.org/c/coreboot/+/43741/19/src/security/tpm/tss/vendo... PS19, Line 19: " " not required. Add '.' instead.
https://review.coreboot.org/c/coreboot/+/43741/19/src/security/tpm/tss/vendo... PS19, Line 19: pulses interrupt pulses.
https://review.coreboot.org/c/coreboot/+/43741/19/src/security/tpm/tss/vendo... PS19, Line 19: " " not required