Re: [PATCH] xhci: create one unified function to calculate TRB TD remainder.

[Mathias Nyman]

On 11.09.2015 07:08, chunfeng yun wrote:
> Hi,
> On Tue, 2015-09-08 at 14:09 +0300, Mathias Nyman wrote:
> > xhci versions 1.0 and later report the untransferred data remaining in a
> > TD a bit differently than older hosts.
> >
> > We used to have separate functions for these, and needed to check host
> >   version before calling the right function.
> >
> > Now Mediatek host has an additional quirk on how it uses the TD Size
> > field for remaining data. To prevent yet another function for calculating
> > remainder we instead want to make one quirk friendly unified function.
> >
> > Signed-off-by: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
> > ---
> >   drivers/usb/host/xhci-ring.c | 106 ++++++++++++++++++-------------------------
> >   drivers/usb/host/xhci.h      |   2 +
> >   2 files changed, 46 insertions(+), 62 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > index a47a1e8..57f40a1 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -3020,21 +3020,6 @@ int xhci_queue_intr_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> >   }
> >
> >   /*
> > - * The TD size is the number of bytes remaining in the TD (including this TRB),
> > - * right shifted by 10.
> > - * It must fit in bits 21:17, so it can't be bigger than 31.
> > - */
> > -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;
> > -}
> > -
> > -/*
> >    * For xHCI 1.0 host controllers, TD size is the number of max packet sized
> >    * packets remaining in the TD (*not* including this TRB).
> >    *
> > @@ -3046,30 +3031,36 @@ static u32 xhci_td_remainder(unsigned int remainder)
> >    *
> >    * TD size = total_packet_count - packets_transferred
> >    *
> > - * It must fit in bits 21:17, so it can't be bigger than 31.
> > + * For xHCI 0.96 and older, TD size field should be the remaining bytes
> > + * including this TRB, right shifted by 10
> > + *
> > + * For all hosts it must fit in bits 21:17, so it can't be bigger than 31.
> > + * This is taken care of in the TRB_TD_SIZE() macro
> > + *
> >    * The last TRB in a TD must have the TD size set to zero.
> >    */
> > -static u32 xhci_v1_0_td_remainder(int running_total, int trb_buff_len,
> > -		unsigned int total_packet_count, struct urb *urb,
> > -		unsigned int num_trbs_left)
> > +static u32 xhci_td_remainder(struct xhci_hcd *xhci, int transferred,
> > +			      int trb_buff_len, unsigned int td_total_len,
> > +			      struct urb *urb, unsigned int num_trbs_left)
> >   {
> > -	int packets_transferred;
> > +	u32 maxp, total_packet_count;
> > +
> > +	if (xhci->hci_version < 0x100)
> > +		return ((td_total_len - transferred) >> 10);
> > +
> > +	maxp = GET_MAX_PACKET(usb_endpoint_maxp(&urb->ep->desc));
> > +	total_packet_count = DIV_ROUND_UP(td_total_len, maxp);
> >
> >   	/* One TRB with a zero-length data packet. */
> > -	if (num_trbs_left == 0 || (running_total == 0 && trb_buff_len == 0))
> > +	if (num_trbs_left == 0 || (transferred == 0 && trb_buff_len == 0) ||
> > +	    trb_buff_len == td_total_len)
> >   		return 0;
> I think when trb_buff_len == td_total_len, the TD only needs one trb, so
> num_trbs_left will equal to 0; that means no need add this condition.

Sorry about the really long delay, I had to focus on other things for a while.

You're right, but I didn't want to change the functionality of the old code.

For some reason the old code called xhci_td_remainder() for both older (0.9)
and newer (>= 1.0) xhci hosts in the control transfer case.
I wanted the outcome to stay the same as with the old code, With the old code the
TD size of a control tranfers was usually 0, without the additional
check we would end up with a remainder of "1". (as num_trbs_left is one)

This should enable easier TD size quirking for control transfers

I'll add the tested-by tag you sent in a later mail

-Mathias
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/