Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47411 )
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
Patch Set 5: Code-Review+1
(2 comments)
Overall LGTM. I think we only need to avoid the unconditional logging of wake source for controller.
https://review.coreboot.org/c/coreboot/+/47411/5/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/xhci.h:
https://review.coreboot.org/c/coreboot/+/47411/5/src/soc/intel/common/block/... PS5, Line 36: and a usb_info : * struct. Leftover comment? Or were you intending to add usb_info pointer to the above structure as well?
https://review.coreboot.org/c/coreboot/+/47411/5/src/soc/intel/common/block/... File src/soc/intel/common/block/xhci/elog.c:
https://review.coreboot.org/c/coreboot/+/47411/5/src/soc/intel/common/block/... PS5, Line 111: elog_add_event_wake(wake_info[i].elog_wake_type_host, 0); Isn't this being logged unconditionally now?
The problem we run into here is: we cannot just rely on the XHCI controller PMCS since that gets cleared in case of S0ix. So, the host controller wake event needs to be logged only if any of the USB2 or USB3 ports are wake sources. That makes it a little icky.
xhci_stash_host_wake(wake_info[i].elog_wake_type_host);
and within xhci_port_wake_check(...), elog_add_event_wake() will have to be changed to: xhci_port_log_wake() which calls xhci_pop_host_wake() and if it is not -1 (or some undefined value), then log host wake first and then the port.