Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Patch Set 11:
(13 comments)
https://review.coreboot.org/c/coreboot/+/43741/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43741/11//COMMIT_MSG@6 PS11, Line 6: This change should be split into multiple CLs for each logical change being made.
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 355: tpm_board_cfg Do we really need to stash this? I think we can just read it when a caller wants it using tpm2_read_reg().
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 452: TMP TPM
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 466: 0x80000000U Can you please define a macro for this rather than using a magic value here?
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 470: 0xC0000000U Same for this.
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... File src/mainboard/google/volteer/Kconfig:
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 60: config TPM_BOARD_CFG : default 0x1 Instead of making the mainboard provide a specific value for TPM_BOARD_CFG, I think it would be better to have Kconfigs for specific feature enabling e.g. CR50_ENABLE_LONG_INTERRUPT_PULSE. So, mainboard doesn't really need to care how it is done i.e. what value to use for what register.
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... File src/mainboard/google/volteer/chromeos.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 6: This change should go in a separate CL.
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 46: 0x01 Use macros please.
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 45: tlcl_lib_init(); : if (get_cr50_board_cfg() & 0x01) { I think it would be better to have some helper functions: cr50_has_long_pulse() which returns true using its internal calls to tlcl_lib_init() and get_cr50_board_cfg().
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 52: 0x80 It would be good to add some macros for this as well.
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 55: nit: extra blank lines not required.
https://review.coreboot.org/c/coreboot/+/43741/11/src/security/tpm/tss/vendo... File src/security/tpm/tss/vendor/cr50/Kconfig:
https://review.coreboot.org/c/coreboot/+/43741/11/src/security/tpm/tss/vendo... PS11, Line 15: TPM_BOARD_CFG As mentioned on the mainboard file, I think it would be better to add Kconfig for the specific feature being enabled. TPM driver can then take care of translating that into appropriate value for the required register.
https://review.coreboot.org/c/coreboot/+/43741/11/src/soc/intel/common/block... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/soc/intel/common/block... PS11, Line 261: static void gspi_clear_base_stash(void *unused)
I would make this an exported function that is called directly at the end of the FSP-S loading code, […]
The problem turned to be not the FSP-S invocation, but the change in BAR that happens because of resource allocation in BS_DEV_RESOURCES boot state within coreboot. Hence, the call is added on BS_ON_EXIT for BS_DEV_RESOURCES.
Jes - this change should go in a CL of its own with explanation as to why it is needed.