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

[Paul Zimmerman]

> 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-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/