Julius Werner 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 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/43741/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43741/2//COMMIT_MSG@15 PS2, Line 15: to FSP. For Volteer (and future Tigerlake boards) we enable mode S0i3.4 I only skimmed the bug but it sounds like all this passing around is only necessary because trying to initialize the Cr50 SPI before FSP-S leads to some not understood problems when trying to use it again afterwards, which doesn't sound like a very satisfactory reason. Is there no way to find and fix that problem? In general, I would say blobs are not allowed to disturb coreboot interfaces (I have to keep yelling at Qualcomm for that all the time too ;) ), so this is a platform problem that the blob owner needs to fix (at least with a workaround, e.g. figuring out what exactly needs to be reinitialized afterwards and then doing that at the end of the coreboot code that handles the blob execution).
Alternatively, would it maybe work out to move all the ramstage CR50 accesses before FSP-S? I don't think there's any explicit reason why the update check currently runs where it does, it could run a bit earlier. We could just combine this all in one big "handle random Cr50 tasks" function that runs in BS_PRE_DEVICE/BS_ON_ENTRY (i.e. before everything else).
https://review.coreboot.org/c/coreboot/+/43741/2/src/security/vboot/cr50.c File src/security/vboot/cr50.c:
https://review.coreboot.org/c/coreboot/+/43741/2/src/security/vboot/cr50.c@2... PS2, Line 26: return; Is all the version checking really necessary, btw? What happens if you try to read the board_cfg register on an older version? If it gives you a consistent error response, can't we just use that to detect whether this is supported?
https://review.coreboot.org/c/coreboot/+/43741/2/src/security/vboot/vboot_lo... File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/c/coreboot/+/43741/2/src/security/vboot/vboot_lo... PS2, Line 304: set_cr50_board_cfg(); This has nothing to do with vboot, please don't put this here. I would consider this a TPM initialization task so it should be with the other TPM initialization stuff in src/drivers/spi/tpm/tpm.c. Look for the ENV_BOOTBLOCK || ... check that guards tpm_claim_locality(), that's basically the "is this the first stage using the TPM" check. We should factor that out into a separate function and use it to kick off this as well right after we read the version. (It's technically CR50-specific but so are still many other things in that file unfortunately, so I'd say just guard it with a CONFIG(TPM_CR50) for now and at some point in the future we'll have to separate all those points out into cleaner interfaces.)