Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44359 )
Change subject: mainboard/google/volteer: Enable long cr50 ready pulses ......................................................................
Patch Set 4:
Wouldn't it make more sense to put all this with the SoC code (guarded by TPM_CR50)?
Possibly. I assume that you mean to put some conditionals into soc/intel/tigerlake/Kconfig, such that if TPM_CR50 is enabled, then it will also enable CR50_USE_LONG_INTERRUPT_PULSES. Besides, there will be some C code to set LpmStateEnableMask depending on the Cr50 response.
Among Tiger Lake devices, I can see three categories: 1) Devices without Cr50 (likely not Chromebooks) 2) Devices on which all other interrupt sources beside Cr50 guarantee 100us pulses 3) Devices on which at least one other interrupt source generates pulses shorter than 100us
Putting some code in soc/intel/tigerlake guarded by TPM_CR50 will nicely separate category 1 from the other two. However, we need some mainboard declarations in order to distinguish between categories 2 and 3.
I am not familiar with the exact precedence of Kconfig, but assuming that mainboard/ defaults take precedence over soc/, and soc/ takes precendence over drivers/, then I suppose that category 3 could be handled by the mainboard declarations setting CR50_USE_LONG_INTERRUPT_PULSES to n (overriding the conditional logic in soc/tigerlake), and also claring bit7 in LpmStateEnableMask.
In this proposal, the C code in soc/intel/tigerlake should be guarded by CR50_USE_LONG_INTERRUPT_PULSES, rather than TPM_CR50.
Do you think that is preferrable to the current patchset?