On Tue, 2020-09-08 at 13:41 +0800, Bin Meng wrote:
> On Mon, Sep 7, 2020 at 3:14 PM Chunfeng Yun <chunfeng....@mediatek.com> wrote:
> >
>
> nits: please remove the ending period in the commit title
Ok, will fix it
>
> > 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.
> >
> > Porting from the Linux:
> > c840d6ce772d("xhci: create one unified function to calculate TRB TD
> > remainder.")
> > 124c39371114("xhci: use boolean to indicate last trb in td remainder
> > calculation")
> >
> > Signed-off-by: Chunfeng Yun <chunfeng....@mediatek.com>
> > ---
> > v2~v3: no changes
> > ---
> > drivers/usb/host/xhci-ring.c | 105
> > +++++++++++++++++++++----------------------
> > include/usb/xhci.h | 2 +
> > 2 files changed, 52 insertions(+), 55 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > index 79bfc34..0f86b01 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -298,55 +298,52 @@ void xhci_queue_command(struct xhci_ctrl *ctrl, u8
> > *ptr, u32 slot_id,
> > xhci_writel(&ctrl->dba->doorbell[0], DB_VALUE_HOST);
> > }
> >
> > -/**
> > - * 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.
> > +/*
> > + * For xHCI 1.0 host controllers, TD size is the number of max packet sized
> > + * packets remaining in the TD (*not* including this TRB).
> > *
> > - * @param remainder remaining packets to be sent
> > - * @return remainder if remainder is less than max else max
> > - */
> > -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;
> > -}
> > -
> > -/**
> > - * Finds out the remanining packets to be sent
> > + * Total TD packet count = total_packet_count =
> > + * DIV_ROUND_UP(TD size in bytes / wMaxPacketSize)
> > + *
> > + * Packets transferred up to and including this TRB = packets_transferred =
> > + * rounddown(total bytes transferred including this TRB /
> > wMaxPacketSize)
> > + *
> > + * TD size = total_packet_count - packets_transferred
> > *
> > - * @param running_total total size sent so far
> > + * 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.
> > + *
> > + * @param ctrl host controller data structure
> > + * @param transferred total size sent so far
> > * @param trb_buff_len length of the TRB Buffer
> > - * @param total_packet_count total packet count
> > - * @param maxpacketsize max packet size of current pipe
> > - * @param num_trbs_left number of TRBs left to be processed
> > - * @return 0 if running_total or trb_buff_len is 0, else remainder
> > + * @param td_total_len total packet count
> > + * @param maxp max packet size of current pipe
> > + * @param more_trbs_coming indicate last trb in TD
> > + * @return remainder
> > */
> > -static u32 xhci_v1_0_td_remainder(int running_total,
> > - int trb_buff_len,
> > - unsigned int total_packet_count,
> > - int maxpacketsize,
> > - unsigned int num_trbs_left)
> > +static u32 xhci_td_remainder(struct xhci_ctrl *ctrl, int transferred,
> > + int trb_buff_len, unsigned int td_total_len,
> > + int maxp, bool more_trbs_coming)
> > {
> > - int packets_transferred;
> > + u32 total_packet_count;
> > +
> > + if (ctrl->hci_version < 0x100)
> > + return ((td_total_len - transferred) >> 10);
> >
> > /* One TRB with a zero-length data packet. */
> > - if (num_trbs_left == 0 || (running_total == 0 && trb_buff_len == 0))
> > + if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
> > + trb_buff_len == td_total_len)
> > return 0;
> >
> > - /*
> > - * All the TRB queueing functions don't count the current TRB in
> > - * running_total.
> > - */
> > - packets_transferred = (running_total + trb_buff_len) /
> > maxpacketsize;
> > + total_packet_count = DIV_ROUND_UP(td_total_len, maxp);
> >
> > - if ((total_packet_count - packets_transferred) > 31)
> > - return 31 << 17;
> > - return (total_packet_count - packets_transferred) << 17;
> > + /* Queueing functions don't count the current TRB into transferred
> > */
> > + return (total_packet_count - ((transferred + trb_buff_len) / maxp));
> > }
> >
> > /**
> > @@ -572,7 +569,7 @@ int xhci_bulk_tx(struct usb_device *udev, unsigned long
> > pipe,
> > union xhci_trb *event;
> >
> > int running_total, trb_buff_len;
> > - unsigned int total_packet_count;
> > + bool more_trbs_coming = true;
> > int maxpacketsize;
> > u64 addr;
> > int ret;
> > @@ -636,8 +633,6 @@ int xhci_bulk_tx(struct usb_device *udev, unsigned long
> > pipe,
> > running_total = 0;
> > maxpacketsize = usb_maxpacket(udev, pipe);
> >
> > - total_packet_count = DIV_ROUND_UP(length, maxpacketsize);
> > -
> > /* How much data is in the first TRB? */
> > /*
> > * How much data is (potentially) left before the 64KB boundary?
> > @@ -672,27 +667,24 @@ int xhci_bulk_tx(struct usb_device *udev, unsigned
> > long pipe,
> > * Chain all the TRBs together; clear the chain bit in the
> > last
> > * TRB to indicate it's the last TRB in the chain.
> > */
> > - if (num_trbs > 1)
> > + if (num_trbs > 1) {
> > field |= TRB_CHAIN;
> > - else
> > + } else {
> > field |= TRB_IOC;
> > + more_trbs_coming = false;
> > + }
> >
> > /* Only set interrupt on short packet for IN endpoints */
> > if (usb_pipein(pipe))
> > field |= TRB_ISP;
> >
> > /* Set the TRB length, TD size, and interrupter fields. */
> > - if (ctrl->hci_version < 0x100)
> > - remainder = xhci_td_remainder(length -
> > running_total);
> > - else
> > - remainder = xhci_v1_0_td_remainder(running_total,
> > - trb_buff_len,
> > -
> > total_packet_count,
> > - maxpacketsize,
> > - num_trbs - 1);
> > + remainder = xhci_td_remainder(ctrl, running_total,
> > trb_buff_len,
> > + length, maxpacketsize,
> > + more_trbs_coming);
> >
> > length_field = ((trb_buff_len & TRB_LEN_MASK) |
> > - remainder |
> > + TRB_TD_SIZE(remainder) |
> > ((0 & TRB_INTR_TARGET_MASK) <<
> > TRB_INTR_TARGET_SHIFT));
> >
> > @@ -764,6 +756,7 @@ int xhci_ctrl_tx(struct usb_device *udev, unsigned long
> > pipe,
> > struct xhci_virt_device *virt_dev = ctrl->devs[slot_id];
> > struct xhci_ring *ep_ring;
> > union xhci_trb *event;
> > + u32 remainder;
> >
> > debug("req=%u (%#x), type=%u (%#x), value=%u (%#x), index=%u\n",
> > req->request, req->request,
> > @@ -866,12 +859,14 @@ int xhci_ctrl_tx(struct usb_device *udev, unsigned
> > long pipe,
> > else
> > field = (TRB_DATA << TRB_TYPE_SHIFT);
> >
> > - length_field = (length & TRB_LEN_MASK) | xhci_td_remainder(length) |
> > + remainder = xhci_td_remainder(ctrl, 0, length, length,
> > + usb_maxpacket(udev, pipe), 1);
>
> 1 should be true
Ok, it's better, will modify it
>
> > + length_field = (length & TRB_LEN_MASK) | TRB_TD_SIZE(remainder) |
> > ((0 & TRB_INTR_TARGET_MASK) <<
> > TRB_INTR_TARGET_SHIFT);
> > debug("length_field = %d, length = %d,"
> > "xhci_td_remainder(length) = %d , TRB_INTR_TARGET(0) =
> > %d\n",
> > length_field, (length & TRB_LEN_MASK),
> > - xhci_td_remainder(length), 0);
> > + TRB_TD_SIZE(remainder), 0);
> >
> > if (length > 0) {
> > if (req->requesttype & USB_DIR_IN)
> > diff --git a/include/usb/xhci.h b/include/usb/xhci.h
> > index a3e5914..15926eb 100644
> > --- a/include/usb/xhci.h
> > +++ b/include/usb/xhci.h
> > @@ -850,6 +850,8 @@ struct xhci_event_cmd {
> > /* transfer_len bitmasks - bits 0:16 */
> > #define TRB_LEN(p) ((p) & 0x1ffff)
> > #define TRB_LEN_MASK (0x1ffff)
> > +/* TD Size, packets remaining in this TD, bits 21:17 (5 bits, so max 31) */
> > +#define TRB_TD_SIZE(p) (min((p), (u32)31) << 17)
> > /* Interrupter Target - which MSI-X vector to target the completion event
> > at */
> > #define TRB_INTR_TARGET_SHIFT (22)
> > #define TRB_INTR_TARGET_MASK (0x3ff)
>
> Otherwise,
> Reviewed-by: Bin Meng <bmeng...@gmail.com>
Thanks a lot
