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 23:
(5 comments)
Jes - I did not see any changes in the latest patchset. Did you intend to push any? Sorry about the long back and forth. I don't mean for this change to be stuck on naming conventions. To summarize:
1. The issue that Julius pointed out with check for \0 needs to be fixed. 2. Others I think are nits w.r.t. naming of function or register. I think it would be good to have better name for the function/registers to help understand the intent/what they mean. But, as I said I don't us to get stuck on the naming forever. So, let's pick one and move ahead with it.
Can you please push a new patchset with the \0 check fixed?
https://review.coreboot.org/c/coreboot/+/43741/21/src/drivers/spi/tpm/tpm.h File src/drivers/spi/tpm/tpm.h:
https://review.coreboot.org/c/coreboot/+/43741/21/src/drivers/spi/tpm/tpm.h@... PS21, Line 45: get_cr50_long_interrupt_pulses
This method does not merely tell whether cr50 supports the longer pulses, but whether they are curre […]
How about cr50_is_long_interrupt_pulse_enabled()
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 449: cr50
ACK, I don't know the full reasonings behind all the conventions so my view isn't strong either way […]
Jes - do you plan to update the name of TPM_BOARD_CFG register?
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 631: if (version_str[++chunk_count * chunk_size - 1])
You are right.
This is not yet addressed even in the latest patchset.
https://review.coreboot.org/c/coreboot/+/43741/23/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/23/src/drivers/spi/tpm/tpm.c@... PS23, Line 462: "matches desired = 0x%08x\n", nit: No need to break any of the print strings. They can also go beyond the 96-column limit.
https://review.coreboot.org/c/coreboot/+/43741/23/src/drivers/spi/tpm/tpm.c@... PS23, Line 631: if (version_str[++chunk_count * chunk_size - 1]) This is not correct as Julius pointed out on a previous patchset.