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.
--
balbi
Attachment:
signature.asc
Description: PGP signature
