On Fri, Oct 26, 2012 at 10:45:56AM +0530, Chintan Mehta wrote: > Hi Sarah, > > Thanks for the patch, I will try this out and let you know the results. > > One thing I am curios about, will this same patch apply to windows > driver/Compliance Verification Suite as well? > > If this also applies to windows driver and Compliance Verification Suite, > let us know the version numbers, so we can verify our Host with it. The Linux xHCI driver, the windows driver, and the compliance verification driver are all separately architected drivers. One patch will not apply to another. Sarah Sharp > On Fri, Oct 26, 2012 at 4:55 AM, Sarah Sharp > <sarah.a.sharp@xxxxxxxxxxxxxxx>wrote: > > > Hi Chintan, > > > > I think I have a fix for the TD size issue. Can you install a custom > > kernel and test it out on your host controller? > > > > The directions for building a custom kernel are here: > > http://kernelnewbies.org/KernelBuild > > > > Instead of running any of the commands in "Which kernel to build?" > > section, use these commands instead: > > > > git clone git://git.kernel.org/pub/scm/linux/kernel/git/sarah/xhci.git -b > > for-usb-linus-queue > > cd xhci > > > > Use the "Duplicating your current config" section. > > > > If you have trouble booting the 3.7-rc2 kernel, let me know and I'll > > rebase the patch against a stable kernel version. > > > > Sarah Sharp > > > > > > On Thu, Oct 25, 2012 at 03:32:08PM -0700, Sarah Sharp wrote: > > > Going back over your example, it does look there is a couple bugs in the > > > Linux xHCI TD size calculations. Notes are below, I'll send you a patch > > > to test out on your host controller shortly. > > > > > > Thanks for catching this! > > > > > > Sarah Sharp > > > > > > On Thu, Oct 25, 2012 at 02:24:04PM -0700, Sarah Sharp wrote: > > > > On Fri, Oct 19, 2012 at 11:29:44AM +0530, Chintan Mehta wrote: > > > > > > > > 2. For Bulk Endpoint: > > > > > > > > > > > > > > > > - *Driver can put a TD with total TD transfer size less than > > > > > > maxpacket > > > > > > > > size and more than 1 TRB?* > > > > > > > > - For example, Maxpacketsize is 1K. And TD contains 3 TRBs > > as below: > > > > > > > > - 1st trb with TRB transfer length 600 Bytes, chain bit > > 1 and > > > > > > > > TDSize 0 > > > > > > > > - 2nd trb with TRB transfer length 200 Bytes, chain bit > > 1 and > > > > > > > > TDSize 0 > > > > > > > > - 3rd trb with TRB transfer length 100 Bytes, chain bit > > 0 and > > > > > > > > TDSize 0 > > > > > > > > - *What should be the value of TDSize in above TRBs of TD?* > > > > > > > > > > > > Again, see section 4.11.2.4. > > > > > > > > > > > > TRB 1 600 (600 + 200 + 100) >> 10 = 0 > > > > > > TRB 2 200 (200 + 100) >> 10 = 0 > > > > > > TRB 3 100 (100) >> 10 = 0 > > > > > > Let's see what the TD size for a 1.0 host controller should be here. > > > > > > TD packet count = > > > roundup(TD size / max packet size) = > > > roundup(900 / 1024) = 1 > > > > > > Packets Transferred (TRB 1) = > > > rounddown(TRB length sum(n) / max packet size) > > > > > > where TRB length sum is the sum of the trb lengths up to and including > > > this TRB, so > > > > > > Packets Transferred (TRB 1) = rounddown(600 / 1024) = 0 > > > > > > TD size = (TD packet count - Packets Transferred) > > > > > > Therefore, > > > > > > TD size(TRB 1) = (1 - 0) = 1 > > > > > > Packets Transferred (TRB 2) = > > > rounddown((600 + 200) / 1024) = 0 > > > TD size(TRB 2) = (1 - 0) = 1 > > > > > > The TD size for TRB 3 is supposed to be set to 0, since it is the last > > > TRB in the TD. > > > > > > So, the final answer should be > > > TRB 1: TD size = 1 > > > TRB 2: TD size = 1 > > > TRB 3: TD size = 0 > > > > > > Now let's see what the xHCI driver actually does. > > > > > > static u32 xhci_td_remainder(unsigned int remainder) > > > { > > > u32 max = (1 << (21 - 17 + 1)) - 1; > > > > > > if ((remainder >> 10) >= max) > > > return max << 17; > > > else > > > return (remainder >> 10) << 17; > > > } > > > > > > static u32 xhci_v1_0_td_remainder(int running_total, int trb_buff_len, > > > unsigned int total_packet_count, struct urb *urb) > > > { > > > int packets_transferred; > > > > > > /* One TRB with a zero-length data packet. */ > > > if (running_total == 0 && trb_buff_len == 0) > > > return 0; > > > > > > /* All the TRB queueing functions don't count the current TRB in > > > * running_total. > > > */ > > > packets_transferred = (running_total + trb_buff_len) / > > > usb_endpoint_maxp(&urb->ep->desc); > > > > > > return xhci_td_remainder(total_packet_count - > > packets_transferred); > > > } > > > > > > That doesn't look right from the start, because passing the result to > > > xhci_td_remainder() will left shift it by 10, which isn't what we want. > > > I'll assume I've fixed that, and make sure the math is right from there. > > > > > > The total_packet_count passed to xhci_v1_0_td_remainder() looks sane, > > > looking at how it's calculated in the isochronous and bulk queueing > > > functions (which also handles the interrupt TD queueing). > > > > > > running_total is the number of bytes in the previous TRBs (not including > > > this TRB), and trb_buff_len is the number of bytes in this TRB. > > > > > > So, for the first TRB, running_total = 0, trb_buff_len = 600, and > > > total_packet_count = 1. > > > packets_transferred = (0 + 600) / 1024 = 0 > > > TD size (TRB 1) = (1 - 0) = 1 > > > > > > TRB 2: > > > packets_transferred = (600 + 200) / 1024 = 0 > > > TD size (TRB 2) = (1 - 0) = 1 > > > > > > TRB 3: > > > packets_transferred = (600 + 200 + 100) / 1024 = 0 > > > TD size (TRB 3) = (1 - 0) = 1 > > > > > > That last TD size is wrong, of course, since the xHCI spec says it has > > > to be special-cased. Probably the URB enqueueing functions should > > > special case that, since they know whether this this the last TRB in a > > > TD. > > > > > > So, yes, there are two bugs in the Linux xHCI TD size code, and I'll > > > send you a patch shortly to fix it. > > > > > > Sarah Sharp > > > -- > > > 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 > > -- 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
