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 23:
(10 comments)
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.h File src/drivers/spi/tpm/tpm.h:
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.h@... PS22, Line 45: get_cr50_long_interrupt_pulses
I still think that this should be named cr50_supports_long_interrupt_pulses() rather than get_cr50_. […]
Ack
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
nit: Since this is returning true/false, I think we should name this something like `cr50_supports_l […]
This method does not merely tell whether cr50 supports the longer pulses, but whether they are currently enabled, so I do not want the word "supports" in the name of the method. I do see that the get_ prefix may not make sense on a boolean getter, and that we want cr50_ to be the prefix of all cr50-specific functions.
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 51: tatic int cr50_firmware_version[3];
Keep in mind that when I reviewed this commit, as Furquan noted above, I only observed the current u […]
I have pulled the parsing, and the comparison if version is new enough to support our register, into a pure functions. These are called with reference to global variables, but could be called otherwise in tests.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 434: static int cr50_supports_board_cfg(void)
As I pointed out above, the suggested signature would mean that static uint32_t get_cr50_board_cfg() […]
Done
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 515: tatic void parse_version(const char *version_str)
`static int parse_version(const char *version_str, struct cr50_ver *ver)` is the correct signature. […]
Done
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 521: printk(BIOS_ERR, "Did not recognize Cr50 version prefix\n"); : return;
Respectfully I disagree. […]
Done
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 525: cr50_firmware_version[0] = skip_atoi(&number); : if (*number++ != '.') { : printk(BIOS_ERR, "Error parsing first dot in Cr50 version\n"); : return; : } : cr50_firmware_version[1] = skip_atoi(&number); : if (*number++ != '.') { : printk(BIOS_ERR, "Error parsing second dot in Cr50 version\n"); : return; : } : cr50_firmware_version[2] = skip_atoi(&number);
parse out major.minor. […]
Done
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])
Careful, I think you flipped a while condition to a break condition here and forgot to negate? This […]
You are right.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 638: parse_version(version_str);
`struct cr50_ver cr50_fw_ver;` then pass `&cr50_fw_ver` and validate the return valid of the parse f […]
Done
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 641: ();
`set_cr50_board_cfg(cr50_fw_ver)`
set_cr50_board_cfg is not the only method that needs to branch on the version, and the other (get_board_cfg) does not have the option of having the version passed in as an argument, so for consistency, both of them still read the global version field.