Furquan Shaikh 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:
(7 comments)
Patch Set 4:
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.
Okay, fair enough, that's a reasonable argument. I'll let the x86 guys chime in on where they think this should be. Either option sounds reasonable to me.
I don't think we need to worry about category 3 for now. All peripherals are expected to meet the 100us requirement. Considering that, I think we can directly compare TPM_CR50 and CR50_USE_LONG_INTERRUPT_PULSES to decide if LpmStateEnableMask should be updated.
https://review.coreboot.org/c/coreboot/+/44359/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44359/4//COMMIT_MSG@20 PS4, Line 20: Bug: b:154333137 Move before Change-Id above. Also, BUG=
https://review.coreboot.org/c/coreboot/+/44359/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/Kconfig:
https://review.coreboot.org/c/coreboot/+/44359/4/src/mainboard/google/voltee... PS4, Line 61: config CR50_USE_LONG_INTERRUPT_PULSES : bool : default y You can just do a "select CR50_USE_LONG_INTERRUPT_PULSES" under BOARD_GOOGLE_BASEBOARD_VOLTEER
https://review.coreboot.org/c/coreboot/+/44359/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/chromeos.c:
https://review.coreboot.org/c/coreboot/+/44359/4/src/mainboard/google/voltee... PS4, Line 12: #define MIN_INTERRUPT_PULSE_LENGTH_US_FOR_S0i3_4 100 In my opinion, we don't really need this. Instead using a bool should be sufficient for checking if cr50 supports long pulses.
https://review.coreboot.org/c/coreboot/+/44359/4/src/mainboard/google/voltee... PS4, Line 43: tlcl_lib_init(); I think this should be added to the function `get_cr50_ready_pulse_length_us()` or whatever the name is if you change it to bool return type. It is a must when making the call to cr50. So, instead of having each mainboard/SoC do it, I think it is better to have the TPM driver code do this.
https://review.coreboot.org/c/coreboot/+/44359/4/src/mainboard/google/voltee... PS4, Line 45: >= MIN_INTERRUPT_PULSE_LENGTH_US_FOR_S0i3_4) { I believe this can go on the previous line within the 96-column limit.
https://review.coreboot.org/c/coreboot/+/44359/4/src/mainboard/google/voltee... PS4, Line 48: S03.4 S0i3.4
https://review.coreboot.org/c/coreboot/+/44359/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/44359/4/src/mainboard/google/voltee... PS4, Line 206: register "LpmStateEnableMask" = "0xFF" Add a comment indicating all S0ix substates are supported.