Change in coreboot[master]: mb/google/brya: Reset XHCI controller while preparing for S5

[Subrata Banik (Code Review)]

Attention is currently required from: Maulik V Vaghela, Paul Menzel, Sridhar Siricilla, Nick Vaccaro, Eric Lai, Felix Held.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63552 )

Change subject: mb/google/brya: Reset XHCI controller while preparing for S5
......................................................................

Patch Set 4:

(6 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/63552/comment/32906929_61a758ac 
PS4, Line 12: the PMC IPC timeout issue
> As you use “the”, is it a common known problem? Is there some errata?

Please refer the prior discussion here in this patch

Felix Held
Patchset 4
Apr 14
is this problem specific to google/brya or will it happen on all boards using the same soc? if the latter is the case, this should probably be done in the corresponding soc code's smi sleep handler

Subrata Banik
Patchset 4
Apr 14
is this problem specific to google/brya or will it happen on all boards using the same soc? if the latter is the case, this should probably be done in the corresponding soc code's smi sleep handler

@Felix, so far we have only seen this issue with ADL-P and ADL-N chrome ref designs and Intel is investigating further to root cause and may be once they provide the TA for this issue, it can re-land into the SOC code. At this moment, don't want to disturb other devices with ADL-SoC unless issue is prove there.

https://review.coreboot.org/c/coreboot/+/63552/comment/ee9ff2ca_a2340903 
PS4, Line 34: active USB port
> Excuse my ignorance, does “active” mean, something needs to be connected to the port?

active => the port is enable

https://review.coreboot.org/c/coreboot/+/63552/comment/1579a5ca_5470a57e 
PS4, Line 37: SMI handler
> Why does this have to be done in SMM?

I guess, below lines are explanatory (isn't it) 
"This patch utilises the S5 SMI handler to reset the XHCI controller
using `xhci_host_reset()` **prior entering into the S5**,"

we wish to reset the XHCI controller state prior to S5 hence, this is the last function that coreboot has control.

https://review.coreboot.org/c/coreboot/+/63552/comment/ff1bc0d2_9ade1e97 
PS4, Line 9: This patch calls into `xhci_host_reset()` function to perform XHCI
           : controller reset.
           : 
           : Without this patch the PMC IPC timeout issue is seen while sending the
           : USB-C (0xA7) command during poweron from S5 (S5->S4->S3->S0).
           : 
           : On Brya variants, poweron from S5 state results in PMC error while
           : sending PMC IPC (0xA7) to USB-C active ports, log here:
           : 
           : localhost ~ # cbmem -c | grep ERROR
           : 
           : [ERROR]  PMC IPC timeout after 1000 ms
           : [ERROR]  PMC IPC command 0x200a7 failed
           : [ERROR]  pmc_send_ipc_cmd failed
           : [ERROR]  Failed to setup port:0 to initial state
           : [ERROR]  PMC IPC timeout after 1000 ms
           : [ERROR]  PMC IPC command 0x200a7 failed
           : [ERROR]  pmc_send_ipc_cmd failed
           : [ERROR]  Failed to setup port:1 to initial state
           : [ERROR]  PMC IPC timeout after 1000 ms
           : [ERROR]  PMC IPC command 0x20a0 failed
           : 
           : This problem is not seen while powering on from G3 (G3->S5->S4->S3->S0).
           : 
           : During poweron the state of USB ports are not the same between S5 and G3
           : and it appears that the active USB port still is in U3 (suspend) while
           : PMC tries to send the IPC command, which results in a timeout.
           : 
           : This patch utilises the S5 SMI handler to reset the XHCI controller
           : using `xhci_host_reset()` prior entering into the S5, it helps to
           : restore the port state to active hence, no PMC timeout is seen with
           : this code change.
> I’d start by stating the problem, and then describing the fix.

Please read the entire commit msg, it does the same IMO.

It started with the problem statement and given the serial log where you could witness the PMC IPC error and then it state the solution.

Please advice what is missing here ?

https://review.coreboot.org/c/coreboot/+/63552/comment/762080e8_afea76d6 
PS4, Line 41:
> Does resetting add anything to the shutdown time? (Boottime should be unaffected?)

I don't think we have KPIs to measure the shutdown time and not sure writing a bit would impact the time.

File src/mainboard/google/brya/smihandler.c:

https://review.coreboot.org/c/coreboot/+/63552/comment/aa03fd0d_9b6614d6 
PS4, Line 15: 	/* USB sleep preparations */
> Maybe elaborate?

Please advice, how you would like to elaborate apart from the function name itself is so clear `xhci_host_reset` and commit msg state the entire story.