Re: [PATCH v6 18/22] usb: dwc2: host: Schedule periodic right away if it's time

[Doug Anderson]

Kever,

On Mon, Feb 1, 2016 at 11:04 PM, Kever Yang <kever.yang@xxxxxxxxxxxxxx> wrote:
>>> Oh, now I get what you're saying!
>>>
>>> A) You've got dwc2_release_channel() -> dwc2_deactivate_qh() ->
>>> dwc2_hcd_qh_deactivate()
>>> ...and always in that case we'll do a select / queue, so we don't need it
>>> there.
>>>
>>> B) You've got dwc2_hcd_urb_dequeue() -> dwc2_hcd_qh_deactivate()
>>>
>>> ...but why don't we need it for dwc2_hcd_urb_dequeue()?  Yes, you're
>>> not continuing a split so timing isn't quite as urgent, but you still
>>> might have an INT or ISOC packet that's scheduled with an interval of
>>> 1.  We still might want to schedule right away if there are remaining
>>> QTDs, right?
>>
>> I ran out of time to fully test today, but I couldn't actually get a
>> case where we needed to schedule right away for B).  ...so given your
>> point about the the select / queue already present in case A, we could
>> probably just drop this patch ("usb: dwc2: host: Schedule periodic
>> right away if it's time") and if we can find a case where it's needed
>> in case B we can add the select / queue there.
>>
>> Sound OK?  I'll try to do more testing tomorrow...
>
> Yes, we don't get a case we need to schedule right away for case B).
>
> For INT or ISOC packet, I can recall I have seen somewhere but I can find
> it now, the synchronous transfer is happen in the next uframe instead of the
> uframe
> when the host channel initialized, so there is no difference of setting the
> host channel register sooner or later inside the same frame.
> Which means the existent code should be OK for case A).
>
> We can drop this patch before we have the exact use case.

I put in some printouts and I finally did manage to find a place where
we needed to queue things up in dwc2_hcd_urb_dequeue().  I saw:

314.587916: QH=d9535340 next(0) fn=2a52, sch=2a51=>2a52 (+1) miss=0
314.588040: QH=d9535340 next(0) fn=2a53, sch=2a52=>2a53 (+1) miss=0
314.588162: QH=d9535340 next(0) fn=2a54, sch=2a53=>2a54 (+1) miss=0
314.588299: QH=d9535340 next(0) fn=2a55, sch=2a54=>2a55 (+1) miss=0
314.588304: QH=d9535340 queue in dwc2_hcd_urb_dequeue
314.588363: QH=d9535340 next(0) fn=2a55, sch=2a55=>2a56 (+1) miss=0
314.588413: dwc2_handle_hcd_intr: ff540000.usb: SCH: QH=e5cea380 ready
fn=2a56, nxt=2a56
314.588414: dwc2_handle_hcd_intr: ff540000.usb: SCH: QH=e73ccc40 ready
fn=2a56, nxt=2a56
314.588415: dwc2_handle_hcd_intr: ff540000.usb: SCH: QH=e5cea8c0 ready
fn=2a56, nxt=2a56

It's not something that's terribly common.  It's fine to just drop
this patch, or I can replace it with
<https://chromium-review.googlesource.com/325540>.

-Doug