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 5:
(7 comments)
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. […]
Done
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
Actually, I am starting to think that this Kconfig setting belongs in soc/tigerlake. […]
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/44359/4/src/mainboard/google/voltee... PS4, Line 41: mainboard_silicon_init_params
I would recommend moving this to mainboard_chip_init() in mainboard.c: https://source.chromium. […]
Done
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 […]
I think we had a discussion elsewhere. The call to tlcl_lib_init() cannot be put into get_cr50_ready_pulse_length_us(), but we could in theory make a new helper function, somewhere under a /google/ directory, but shared between mainboards, which would do tlcl_lib_init() followed by get_cr50_ready_pulse_length_us() , or maybe even better, a function that does all the logic that I am currently adding here, to be shared between future Intel mainboards.
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.
Ack
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.
With the new convention of a bit being 1 meaning that the corresponding substate is DISabled, there is no need to add anything to this file, for Volteer or future Tiger Lake devices.