Hi, Fengwei Yin <nh26...@gmail.com> 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 <mgau...@codeaurora.org> >> --- >> 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
signature.asc
Description: PGP signature