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

[Wesley Cheng <quic_wcheng@quicinc.com>]

Hi Mathias,

On 4/25/2023 2:04 AM, Mathias Nyman wrote:
> 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)
> 

Thanks for the info.  I'll make a change to add the checks I mentioned 
above and submit it as a separate patch for review.

Thanks
Wesley Cheng