Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44653 )
Change subject: drivers/spi/tpm: Add helper to identify how cr50 sees AP reset ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/44653/1/src/drivers/spi/tpm/tpm.h File src/drivers/spi/tpm/tpm.h:
https://review.coreboot.org/c/coreboot/+/44653/1/src/drivers/spi/tpm/tpm.h@4... PS1, Line 47: Indicates whether Cr50 observes AP reset using SYS_RESET# signal. If we want to keep this function as a common helper routine, then we should add a comment explaining under what conditions this is applicable. As I understand this, currently this is applicable only to boards that are using strap cfg 0xe where the old firmware defaults to assuming that PLTRST# is connected to a pin which is actually SYS_RESET# signal. For boards using all other strap configs this function is not really applicable.
https://review.coreboot.org/c/coreboot/+/44653/1/src/drivers/spi/tpm/tpm.h@4... PS1, Line 48: cr50_sees_ap_reset_on_sys_reset Some thoughts just to ensure that this does not get used incorrectly for any board in the future. I am wondering if we should:
1. Add a Kconfig "CR50_STRAP_REQUIRES_UPDATED_FIRMWARE" or "CR50_STRAP_CHECK_FIRMWARE_SUPPORT" so that it can be set by mainboards that actually use a new strap that old cr50 firmware does not know about.
2. Set this Kconfig in boards that are actually affected - Puff, Dedede, Volteer
3. Rename function to cr50_firmware_pltrst_on_wrong_pin()
4. This function will: a. Return false if CR50_STRAP_REQUIRES_UPDATED_FIRMWARE/CR50_STRAP_CHECK_FIRMWARE_SUPPORT is not selected. b. Else, return false if firmware version is newer than the minimum version required for the new strap. c. Else, return true.
The other option is to simply add a helper function to read cr50 firmware version i.e. cr50_get_version() and implement the logic to decide whether EC-driven SYSRST# is required directly in csme_board_reset() since it is implemented only by boards that require this workaround. Thoughts?
https://review.coreboot.org/c/coreboot/+/44653/1/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/44653/1/src/drivers/spi/tpm/tpm.c@8... PS1, Line 856: cr50_firmware_version.epoch == 0 && cr50_firmware_version.major == 0 && : cr50_firmware_version.minor <= 22 Can you please check with Mary or Namyoon to get a confirmation on what firmware version really supports the new strap 0xe. I think we might need a check for more than just 0.0.22. i.e. firmware versions newer than 0.0.22 as well.
https://review.coreboot.org/c/coreboot/+/44653/1/src/drivers/spi/tpm/tpm.c@8... PS1, Line 860: printk(BIOS_INFO, "Cr50 firmware does not use SYS_RESET#, version: %d.%d.%d\n", : cr50_firmware_version.epoch, cr50_firmware_version.major, : cr50_firmware_version.minor); Is this print really required? It is not really accurate. cr50 firmware that understands strap 0xe uses SYS_RESET# signal to reset the AP.