Brandon Breitenstein has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37870 )
Change subject: soc/intel/tigerlake: Add code for early tcss ......................................................................
Patch Set 10:
(6 comments)
https://review.coreboot.org/c/coreboot/+/37870/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37870/10//COMMIT_MSG@10 PS10, Line 10: PMC IPC driver is needed to communicate with EC in order to : get status of the Type C ports
What if the state of the ports changes after this configuration is done in coreboot? […]
Kernel is re-scanning the ports once it is loaded. We are currently working on an EC flag that will allow Kernel to check if Coreboot already connected the port or not
https://review.coreboot.org/c/coreboot/+/37870/10/src/soc/intel/tigerlake/Ma... File src/soc/intel/tigerlake/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/37870/10/src/soc/intel/tigerlake/Ma... PS10, Line 33: $(CONFIG_TGL_CHROME_EC)
Do we really need to restrict this to just Chrome EC? I think instead we can have a chip config that […]
will change this to early_tcss config flag
https://review.coreboot.org/c/coreboot/+/37870/10/src/soc/intel/tigerlake/ea... File src/soc/intel/tigerlake/early_tcss.c:
https://review.coreboot.org/c/coreboot/+/37870/10/src/soc/intel/tigerlake/ea... PS10, Line 18: #include <console/post_codes.h>
Is this used?
This was being used in an earlier iteration will remove
https://review.coreboot.org/c/coreboot/+/37870/10/src/soc/intel/tigerlake/ea... PS10, Line 81: google_chromeec_usb_get_pd_ports
Should we really be tying this to Chrome EC here? Or should this be a more general interface that al […]
Potentially doesn't need to be tied to Chrome EC, however with Intel EC all control of TCSS is handled by PMC so there isn't really a use case for this without Chrome EC at least for now. I'm not opposed to abstracting it out and allowing this to be used for any case though.
https://review.coreboot.org/c/coreboot/+/37870/10/src/soc/intel/tigerlake/ea... PS10, Line 115: BS_OS_RESUME
Why is this configuration required on OS resume? There is no payload involved in that case.
My understanding is that coreboot needed to reconnect the devices on resume. You are probably right here though I will do some testing and verify that this is actually doing nothing.
https://review.coreboot.org/c/coreboot/+/37870/10/src/soc/intel/tigerlake/ea... PS10, Line 116: BS_PAYLOAD_LOAD
Isn't this too late for some cases? Example external display. […]
This was done with the assumption that external display was not supported for bios. Can move this to earlier if we expand this code for early display as well