Sarah,
We've found the TD_size issue when developing a new XHCI host controller also:
1. need fix xhci_td_remainder() and xhci_v1_0_td_remainder()
2. we need to use DIV_ROUND_UP() instead of roundup() when we
calculating total_packet_count .
As in recent kernel versions, roundup() is defined as follow
#define roundup(x, y) ( \
{ \
const typeof(y) __y = y; \
(((x) + (__y - 1)) / __y) * __y; \
} \
)
And DIV_ROUND_UP is defined as
#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
The function roundup() is used several times in xhci-ring.c to
calculate the number of packets needed:
total_packet_count = roundup(td_len,
usb_endpoint_maxp(&urb->ep->desc));
total_packet_count = roundup(urb->transfer_buffer_length,
usb_endpoint_maxp(&urb->ep->desc));
On Fri, Oct 26, 2012 at 7:25 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
--
- Shimmer
--
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
