Hi Balbi,
On Mon, Jul 24, 2017 at 3:42 PM, Felipe Balbi <balbi@xxxxxxxxxx> wrote:
>
> Hi,
>
> Fengwei Yin <nh26223@xxxxxxxxx> writes:
>>> The PIDs for Isochronous data transfers are incorrect
>>> for high bandwidth IN endpoints when the request length
>>> is less than EP wMaxPacketSize.
>>> As per spec correct PIDs for ISOC data transfers are:
>>> ->For request length <= wMaxPacketSize
>>> - DATA0,
>>> ->For wMaxPacketSize < length <= (2 * wMaxPacketSize)
>>> - DATA1,DATA0
>>> ->For (2 * wMaxPacketSize) < length <= (3 * wMaxPacketSize)
>>> - DATA2, DATA1, DATA0.
>>>
>>> But driver always sets PCM fields based on wMaxPacketSize
>>> due to which DATA2 happens even for small requests.
>>>
>>> Fix this by setting the PCM field of trb->size depending
>>> on request length rather than fixing it to the value
>>> depending on wMaxPacketSize.
>>>
>>> Ideally it shouldn't give any issues as dwc3 will send
>>> 0-length packet for next IN token if host sends (even
>>> after receiving a short packet). Windows seems to ignore
>>> this but with MacOS frame loss observed when using f_uvc.
>>>
>>> Signed-off-by: Manu Gautam <mgautam@xxxxxxxxxxxxxx>
>>> ---
>>> Changes for v2:
>>> - Simplify and add comment as per Felipe's suggestion
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 9e41605a..c0d6c54 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -894,9 +894,41 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
>>> if (!node) {
>>> trb->ctrl = DWC3_TRBCTL_ISOCHRONOUS_FIRST;
>>>
>>> + /*
>>> + * USB Specification 2.0 Section 5.9.2 states that: "If
>>> + * there is only a single transaction in the microframe,
>>> + * only a DATA0 data packet PID is used. If there are
>>> + * two transactions per microframe, DATA1 is used for
>>> + * the first transaction data packet and DATA0 is used
>>> + * for the second transaction data packet. If there are
>>> + * three transactions per microframe, DATA2 is used for
>>> + * the first transaction data packet, DATA1 is used for
>>> + * the second, and DATA0 is used for the third."
>>> + *
>>> + * IOW, we should satisfy the following cases:
>>> + *
>>> + * 1) length <= wMaxPacketSize
>>> + * - DATA0
>>> + *
>>> + * 2) wMaxPacketSize < length <= (2 * wMaxPacketSize)
>>> + * - DATA1, DATA0
>>> + *
>>> + * 3) (2*wMaxPacketSize) < length <= (3*wMaxPacketSize)
>>> + * - DATA2, DATA1, DATA0
>>> + */
>>> +
>>> if (speed == USB_SPEED_HIGH) {
>>> - struct usb_ep *ep = &dep->endpoint;
>>> - trb->size |= DWC3_TRB_SIZE_PCM1(ep->mult - 1);
>>> + unsigned int mult = dep->endpoint.mult - 1;
>>> + unsigned int maxp = usb_endpoint_maxp(
>>> + dep->endpoint.desc);
>>> +
>>> + if (length <= (maxp << 1))
>>> + mult--;
>>> +
>>> + if (length <= maxp)
>>> + mult--;
>>> +
>>> + trb->size |= DWC3_TRB_SIZE_PCM1(mult);
>>> }
>>> } else {
>>> trb->ctrl = DWC3_TRBCTL_ISOCHRONOUS;
>>> --
>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project
>>>
>>
>> Sorry for late response. The change looks fine to me.
>>
>> BTW, I never get high speed/high bandwidth mode work with Qualcom8953
>> platform. But I didn't see any issue related with
>> less length packet. :).
>
> Can you capture tracepoints from dwc3? We spent days with Roger Q just
> to get high bandwidth isoc working. It should be working unless we
> regressed something along the way.
Sorry. I can't access the hardware now. :(. We may need some guy from Q
try it on 8953 platform.
I tried the patch from Bin which could make high bandwidth isoc working on
his platform (partially). But still could work in my env.
Regards
Yin, Fengwei
>
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
