Re: [PATCH 2/2] xhci: Improve the XHCI resume time

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 25.4.2023 3.09, Wesley Cheng wrote:
> Hi Mathias,
> 
> On 4/24/2023 8:05 AM, Mathias Nyman wrote:
>> On 21.4.2023 7.58, Basavaraj Natikar wrote:
>>>
>>> On 4/20/2023 10:33 PM, Mark Hasemeyer wrote:
>>>>> It may be necessary to wait only for auto-resume cases.
>>>> I find this comment misleading as the patch assumes that it's only necessary to
>>>> wait for auto-resume cases. Are there any cases where the driver should wait
>>>> during system-resume?
>>>
>>> Only in case of auto-resume (runtime resume).
>>>
>>> Rewording the commit message as follows.
>>
>> Thanks for fixing this extra system resume delay
>>
>> Maybe some kind of big picture explanation could be added to the commit message,
>> such as:
>>
>> Avoid extra 120ms delay during system resume.
>>
>> xHC controller may signal wake up to 120ms before it shows which USB device
>> caused the wake on the xHC port registers.
>>
>> The xhci driver therefore checks for port activity up to 120ms during resume,
>> making sure that the hub driver can see the port change, and won't immediately
>> runtime suspend back due to no port activity.
>>
>> This is however only needed for runtime resume as system resume will resume
>> all child hubs and other child usb devices anyway.
>>
>>>
>>> Each XHCI controller while xhci_resumes by default takes 120 ms more if
>>> there is no activity on the ports or no ports connected. Therefore, if
>>> there are more USB controllers on the system, 120 ms more per controller
>>> will add delay to system resume from suspended states like s2idle, S3 or
>>> S4 states.
>>>
>>> Once the XHCI controller is in runtime suspended state (D3 state), on USB
>>> device hotplug controller will runtime resume (D0 state) and check for
>>> pending port events if no events, wait for 120 ms to re-check for port
>>> activity to handle missed wake signal.
>>>
>>> A delay of 120 ms more to re-check for port activity is needed only in
>>> auto-resume (runtime resume) cases. Hence, add a check only for runtime
>>> resume from runtime suspend (D3->D0) to avoid the 120ms more delay for
>>> other PM events (system resume from suspend states like s2idle, S3 or S4
>>> states) so that the system resume time can be improved.
>>>
>>> Please let me know if any inputs.
>>
>> I can only think of one minor side-effect that would be runtime suspending back
>> too early after system resume. This could happen when connecting the first
>> usb device to a roothub on a (system) suspended setup?
>>
>> steps:
>> 1. in system suspend, no usb devices connected, xhci in D3, can signal wake with PME#
>> 2. connect first usb device, xHC signals PME# wake
>> 3. system resumes, xhci resumes to D0, but no actity visible on xHC port registers
> 
> Thanks for bringing up this topic Basavaraj.
> 
> Sorry for jumping into this thread, but was looking to optimize this resume timing as well, since it is affecting some of the host driven bus resume situations.  Just had a quick question about where the 120ms delay is required...
> 
>  From what I'm gathering from the USB3 spec, the 120ms timeout is the recommended time for tU3WakeupRetryDelay ("Table 7-12. LTSSM State Transition Timeouts").  This is the retry time that the device will wait before re-issuing another (potential) LFPS U3 wake.
> 
> My idea was to see if we could limit this delay only for when a SSUSB device is already connected to the root hub.  (ignore if HSUSB device connected)  We would be able to eliminate the delay for:
> 1.  No device connected to root hub
> 2.  Only HSUSB device connected
> 
> Is that a possibility we can add on top of what Basavaraj is adding?
> 

Sounds reasonable,
Yes the 120ms was intended for the U3 wake delay for SuperSpeed devices.

We should probably also check for CAS bit in xhci_pending_portevent()
(I'll add that CAS check)

-Mathias