RE: [PATCH v2 10/12] usb: dwc2/gadget: assign TX FIFO dynamically

From: Paul Zimmerman
Date: Wed Jul 16 2014 - 16:22:39 EST

Next message: Sam Asadi: "[PATCH 3/3] Staging: adl_pci9118: a language issue fixed"
Previous message: Kees Cook: "Re: [PATCH 5/7] x86: Split syscall_trace_enter into two phases"
In reply to: Paul Zimmerman: "RE: [PATCH v2 10/12] usb: dwc2/gadget: assign TX FIFO dynamically"
Next in thread: Robert Baldyga: "Re: [PATCH v2 10/12] usb: dwc2/gadget: assign TX FIFO dynamically"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> With a TX FIFO size of 3072, an entire microframe worth of Isoc data
> (when MaxBurst=2) can fit in the FIFO. I guess that is why you chose

s/MaxBurst/Mult/ i.e. bits 12..11 of wMaxPacketSize.

--
Paul

> From: linux-usb-owner@xxxxxxxxxxxxxxx [mailto:linux-usb-owner@xxxxxxxxxxxxxxx] On Behalf Of Paul Zimmerman
> Sent: Wednesday, July 16, 2014 12:58 PM
>
> > From: Robert Baldyga [mailto:r.baldyga@xxxxxxxxxxx]
> > Sent: Wednesday, July 16, 2014 3:22 AM
> >
> > Because we have not enough memory to have each TX FIFO of size at least 3072
> > bytes (the maximum single packet size), we create four FIFOs of lenght 1024,
> > and four of length 3072 bytes, and assing them to endpoints dynamically
> > according to maxpacket size value of given endpoint.
>
> I don't think this commit message entirely explains what you are doing
> here.
>
> 3072 is actually 3 times the max packet size for an Isoc endpoint. So you
> want to have four TX FIFOs of that size, presumably to be assigned to
> Isoc endpoints. Where before this change, all TX FIFOs were of size 768,
> which is not even 1 max packet size for an Isoc endpoint. So I guess you
> were seeing some problem with that?
>
> With a TX FIFO size of 3072, an entire microframe worth of Isoc data
> (when MaxBurst=2) can fit in the FIFO. I guess that is why you chose
> 3072?
>
> Also, after this change, you are only initializing 8 TX FIFOs, where
> before you were initializing all 15. So now there can only be a maximum
> of 8 IN endpoints active at the same time. That's OK I guess, but I
> think you should mention that in the commit message.
>
> --
> Paul
>
> > It needed to do some small modifications in few places in code, because there
> > was assumption that TX FIFO numbers assigned to endpoints are the same as
> > the endpoint numbers, which is not true since we have dynamic FIFO assigning.
> >
> > Signed-off-by: Robert Baldyga <r.baldyga@xxxxxxxxxxx>
> > ---
> > drivers/usb/dwc2/core.h | 2 ++
> > drivers/usb/dwc2/gadget.c | 84 +++++++++++++++++++++++++++++------------------
> > 2 files changed, 54 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> > index 067390e..3b4bd4c 100644
> > --- a/drivers/usb/dwc2/core.h
> > +++ b/drivers/usb/dwc2/core.h
> > @@ -139,6 +139,7 @@ struct s3c_hsotg_ep {
> > unsigned int last_load;
> > unsigned int fifo_load;
> > unsigned short fifo_size;
> > + unsigned short fifo_index;
> >
> > unsigned char dir_in;
> > unsigned char index;
> > @@ -197,6 +198,7 @@ struct s3c_hsotg {
> > int fifo_mem;
> > unsigned int dedicated_fifos:1;
> > unsigned char num_of_eps;
> > + u32 fifo_map;
> >
> > struct dentry *debug_root;
> > struct dentry *debug_file;
> > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> > index 3435711..1b5e9ff 100644
> > --- a/drivers/usb/dwc2/gadget.c
> > +++ b/drivers/usb/dwc2/gadget.c
> > @@ -184,14 +184,29 @@ static void s3c_hsotg_init_fifo(struct s3c_hsotg *hsotg)
> >
> > /* start at the end of the GNPTXFSIZ, rounded up */
> > addr = 2048 + 1024;
> > - size = 768;
> >
> > /*
> > - * currently we allocate TX FIFOs for all possible endpoints,
> > - * and assume that they are all the same size.
> > + * Because we have not enough memory to have each TX FIFO of size at
> > + * least 3072 bytes (the maximum single packet size), we create four
> > + * FIFOs of lenght 1024, and four of length 3072 bytes, and assing
> > + * them to endpoints dynamically according to maxpacket size value of
> > + * given endpoint.
> > */
> >
> > - for (ep = 1; ep <= 15; ep++) {
> > + /* 256*4=1024 bytes FIFO length */
> > + size = 256;
> > + for (ep = 1; ep <= 4; ep++) {
> > + val = addr;
> > + val |= size << FIFOSIZE_DEPTH_SHIFT;
> > + WARN_ONCE(addr + size > hsotg->fifo_mem,
> > + "insufficient fifo memory");
> > + addr += size;
> > +
> > + writel(val, hsotg->regs + DPTXFSIZN(ep));
> > + }
> > + /* 768*4=3072 bytes FIFO length */
> > + size = 768;
> > + for (ep = 5; ep <= 8; ep++) {
> > val = addr;
> > val |= size << FIFOSIZE_DEPTH_SHIFT;
> > WARN_ONCE(addr + size > hsotg->fifo_mem,
> > @@ -450,7 +465,7 @@ static int s3c_hsotg_write_fifo(struct s3c_hsotg *hsotg,
> > to_write = DIV_ROUND_UP(to_write, 4);
> > data = hs_req->req.buf + buf_pos;
> >
> > - iowrite32_rep(hsotg->regs + EPFIFO(hs_ep->index), data, to_write);
> > + iowrite32_rep(hsotg->regs + EPFIFO(hs_ep->fifo_index), data, to_write);
> >
> > return (to_write >= can_write) ? -ENOSPC : 0;
> > }
> > @@ -1281,7 +1296,7 @@ static void s3c_hsotg_rx_data(struct s3c_hsotg *hsotg, int ep_idx, int size)
> > {
> > struct s3c_hsotg_ep *hs_ep = &hsotg->eps[ep_idx];
> > struct s3c_hsotg_req *hs_req = hs_ep->req;
> > - void __iomem *fifo = hsotg->regs + EPFIFO(ep_idx);
> > + void __iomem *fifo = hsotg->regs + EPFIFO(hs_ep->fifo_index);
> > int to_read;
> > int max_req;
> > int read_ptr;
> > @@ -1835,7 +1850,7 @@ static void s3c_hsotg_epint(struct s3c_hsotg *hsotg, unsigned int idx,
> > if (dir_in) {
> > int epctl = readl(hsotg->regs + epctl_reg);
> >
> > - s3c_hsotg_txfifo_flush(hsotg, idx);
> > + s3c_hsotg_txfifo_flush(hsotg, hs_ep->fifo_index);
> >
> > if ((epctl & DXEPCTL_STALL) &&
> > (epctl & DXEPCTL_EPTYPE_BULK)) {
> > @@ -1984,6 +1999,7 @@ static void kill_all_requests(struct s3c_hsotg *hsotg,
> > int result, bool force)
> > {
> > struct s3c_hsotg_req *req, *treq;
> > + unsigned size;
> >
> > list_for_each_entry_safe(req, treq, &ep->queue, queue) {
> > /*
> > @@ -1997,9 +2013,11 @@ static void kill_all_requests(struct s3c_hsotg *hsotg,
> > s3c_hsotg_complete_request(hsotg, ep, req,
> > result);
> > }
> > - if(hsotg->dedicated_fifos)
> > - if ((readl(hsotg->regs + DTXFSTS(ep->index)) & 0xffff) * 4 < 3072)
> > - s3c_hsotg_txfifo_flush(hsotg, ep->index);
> > + if (hsotg->dedicated_fifos) {
> > + size = (readl(hsotg->regs + DTXFSTS(ep->index)) & 0xffff) * 4;
> > + if (size < ep->fifo_size)
> > + s3c_hsotg_txfifo_flush(hsotg, ep->fifo_index);
> > + }
> > }
> >
> > /**
> > @@ -2440,6 +2458,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
> > u32 epctrl;
> > u32 mps;
> > int dir_in;
> > + int i, val, size;
> > int ret = 0;
> >
> > dev_dbg(hsotg->dev,
> > @@ -2512,17 +2531,8 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
> > break;
> >
> > case USB_ENDPOINT_XFER_INT:
> > - if (dir_in) {
> > - /*
> > - * Allocate our TxFNum by simply using the index
> > - * of the endpoint for the moment. We could do
> > - * something better if the host indicates how
> > - * many FIFOs we are expecting to use.
> > - */
> > -
> > + if (dir_in)
> > hs_ep->periodic = 1;
> > - epctrl |= DXEPCTL_TXFNUM(index);
> > - }
> >
> > epctrl |= DXEPCTL_EPTYPE_INTERRUPT;
> > break;
> > @@ -2536,8 +2546,25 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
> > * if the hardware has dedicated fifos, we must give each IN EP
> > * a unique tx-fifo even if it is non-periodic.
> > */
> > - if (dir_in && hsotg->dedicated_fifos)
> > - epctrl |= DXEPCTL_TXFNUM(index);
> > + if (dir_in && hsotg->dedicated_fifos) {
> > + size = hs_ep->ep.maxpacket*hs_ep->mc;
> > + for (i = 1; i <= 8; ++i) {
> > + if (hsotg->fifo_map & (1<<i))
> > + continue;
> > + val = readl(hsotg->regs + DPTXFSIZN(i));
> > + val = (val >> FIFOSIZE_DEPTH_SHIFT)*4;
> > + if (val < size)
> > + continue;
> > + hsotg->fifo_map |= 1<<i;
> > +
> > + epctrl |= DXEPCTL_TXFNUM(i);
> > + hs_ep->fifo_index = i;
> > + hs_ep->fifo_size = val;
> > + break;
> > + }
> > + if (i == 8)
> > + return -ENOMEM;
> > + }
> >
> > /* for non control endpoints, set PID to D0 */
> > if (index)
> > @@ -2584,6 +2611,9 @@ static int s3c_hsotg_ep_disable(struct usb_ep *ep)
> > /* terminate all requests with shutdown */
> > kill_all_requests(hsotg, hs_ep, -ESHUTDOWN, false);
> >
> > + hsotg->fifo_map &= ~(1<<hs_ep->fifo_index);
> > + hs_ep->fifo_index = 0;
> > + hs_ep->fifo_size = 0;
> >
> > ctrl = readl(hsotg->regs + epctrl_reg);
> > ctrl &= ~DXEPCTL_EPENA;
> > @@ -2975,7 +3005,6 @@ static void s3c_hsotg_initep(struct s3c_hsotg *hsotg,
> > struct s3c_hsotg_ep *hs_ep,
> > int epnum)
> > {
> > - u32 ptxfifo;
> > char *dir;
> >
> > if (epnum == 0)
> > @@ -3004,15 +3033,6 @@ static void s3c_hsotg_initep(struct s3c_hsotg *hsotg,
> > hs_ep->ep.ops = &s3c_hsotg_ep_ops;
> >
> > /*
> > - * Read the FIFO size for the Periodic TX FIFO, even if we're
> > - * an OUT endpoint, we may as well do this if in future the
> > - * code is changed to make each endpoint's direction changeable.
> > - */
> > -
> > - ptxfifo = readl(hsotg->regs + DPTXFSIZN(epnum));
> > - hs_ep->fifo_size = FIFOSIZE_DEPTH_GET(ptxfifo) * 4;
> > -
> > - /*
> > * if we're using dma, we need to set the next-endpoint pointer
> > * to be something valid.
> > */
> > --
> > 1.9.1
>
> --
> 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-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/

Next message: Sam Asadi: "[PATCH 3/3] Staging: adl_pci9118: a language issue fixed"
Previous message: Kees Cook: "Re: [PATCH 5/7] x86: Split syscall_trace_enter into two phases"
In reply to: Paul Zimmerman: "RE: [PATCH v2 10/12] usb: dwc2/gadget: assign TX FIFO dynamically"
Next in thread: Robert Baldyga: "Re: [PATCH v2 10/12] usb: dwc2/gadget: assign TX FIFO dynamically"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]