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 [email protected]
> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
