Furquan Shaikh 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:
(8 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?
One example that comes to mind: Chrome OS requests all USB devices be disconnected when booting into recovery and then the devices can be connected after recovery screen is shown. Wouldn't this be a problem?
https://review.coreboot.org/c/coreboot/+/37870/10//COMMIT_MSG@12 PS10, Line 12: Can you please add relevant partner bug?
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 mainboard can select if it wants to do early_tcss.
(BTW, there is already a Kconfig for CHROMEEC which can be used in case this really has to be done only for Chrome EC)
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?
https://review.coreboot.org/c/coreboot/+/37870/10/src/soc/intel/tigerlake/ea... PS10, Line 37: bool usb; : bool polarity; : bool ufp; : bool acc; : uint8_t usb3_port; : uint8_t usb2_port; Can you please add comments indicating what these members really mean?
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 allows any driver to provide these APIs? i.e. tcss_get_usb_pd_count_count(), tcss_get_usb_pd_port_info(). And in case of Chrome EC, it can implement those. Thoughts?
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.
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. Wouldn't you want the mux to be configured before FSP-S runs?
-
Furquan Shaikh (Code Review)