Brandon Breitenstein has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42079 )
Change subject: soc/intel/tigerlake: Add code for early tcss ......................................................................
Patch Set 25:
(8 comments)
https://review.coreboot.org/c/coreboot/+/42079/24/src/mainboard/google/volte... File src/mainboard/google/volteer/Kconfig:
PS24:
This is a mainboard change. It should go in the next CL.
will move to mainboard cl...this one used to encompass both
https://review.coreboot.org/c/coreboot/+/42079/24/src/soc/intel/tigerlake/ea... File src/soc/intel/tigerlake/early_tcss.c:
https://review.coreboot.org/c/coreboot/+/42079/24/src/soc/intel/tigerlake/ea... PS24, Line 3: #include <bootstate.h>
Why is this required?
removed
https://review.coreboot.org/c/coreboot/+/42079/24/src/soc/intel/tigerlake/ea... PS24, Line 6: #include <ec/google/chromeec/ec.h>
Why is this required?
leftover from when this was calling ec commands removed
https://review.coreboot.org/c/coreboot/+/42079/24/src/soc/intel/tigerlake/ea... PS24, Line 102: while (--tries >= 0)
Just curious: Why are we trying 3 times? Is this operation known to fail for some reason?
This is for consistency with the depthcharge code, it can be removed. This loop should almost never be executed cause the only case it executes is if the response buffer is empty.
https://review.coreboot.org/c/coreboot/+/42079/24/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42079/24/src/soc/intel/tigerlake/fs... PS24, Line 376: DSK
What does DSK mean?
Will clean this up...was using this for debug
https://review.coreboot.org/c/coreboot/+/42079/24/src/soc/intel/tigerlake/fs... PS24, Line 376: WEAK
Why WEAK?
Ack
https://review.coreboot.org/c/coreboot/+/42079/24/src/soc/intel/tigerlake/fs... PS24, Line 378: vboot_recovery_mode_enabled
What about developer mode? Wouldn't you need this in developer mode as well?
my understanding was this was mainly for user facing modes that had no OS I will add a developer mode check as well
https://review.coreboot.org/c/coreboot/+/42079/24/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/early_tcss.h:
https://review.coreboot.org/c/coreboot/+/42079/24/src/soc/intel/tigerlake/in... PS24, Line 130: mainboard_early_tcss_enable
What is the expectation from mainboard here? There should be a comment explaining the expectations.
adding comment in next patch