[U-Boot] [PATCH 1/2] usb: ci_udc: allow multiple buffer allocs per ep

[U-Boot] [PATCH 1/2] usb: ci_udc: allow multiple buffer allocs per ep

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Modify ci_ep_alloc_request() to return a dynamically allocated request
object, rather than a singleton that's part of the endpoint. This
requires moving various state from the endpoint structure to the request
structure, since we need one copy per request.

The "fast bounce buffer" b_fast is removed by this change rather than
moved to the request object. Instead, we enhance the bounce buffer logic
in ci_bounce()/ci_debounce() to keep the bounce buffer around between
request submissions. This avoids the need to allocate an arbitrarily-
sized bounce buffer up-front, yet avoids incurring the allocation
overhead each time a request is submitted.

A future enhancement would be to actually submit multiple requests to HW
at once. The Linux driver shows that this is possible. That might improve
throughput (depending on the USB protocol in use), since USB could be
performing a transfer to one HW buffer in parallel with whatever SW
actions U-Boot performs on another buffer. However, I have not made this
change as part of this patch, in order to keep SW changes related to
buffer management separate from any change in the way the HW is
programmed.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/usb/gadget/ci_udc.c | 180 +++++++++++++++++++++++++++++---------------
 drivers/usb/gadget/ci_udc.h |  17 +++--
 2 files changed, 132 insertions(+), 65 deletions(-)

diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
index 02d3fdade86c..290863d61fc9 100644
--- a/drivers/usb/gadget/ci_udc.c
+++ b/drivers/usb/gadget/ci_udc.c
@@ -205,13 +205,26 @@ static void ci_invalidate_qtd(int ep_num)
 static struct usb_request *
 ci_ep_alloc_request(struct usb_ep *ep, unsigned int gfp_flags)
 {
-	struct ci_ep *ci_ep = container_of(ep, struct ci_ep, ep);
-	return &ci_ep->req;
+	struct ci_req *ci_req;
+
+	ci_req = memalign(ARCH_DMA_MINALIGN, sizeof(*ci_req));
+	if (!ci_req)
+		return NULL;
+
+	INIT_LIST_HEAD(&ci_req->queue);
+	ci_req->b_buf = 0;
+
+	return &ci_req->req;
 }
 
-static void ci_ep_free_request(struct usb_ep *ep, struct usb_request *_req)
+static void ci_ep_free_request(struct usb_ep *ep, struct usb_request *req)
 {
-	return;
+	struct ci_req *ci_req;
+
+	ci_req = container_of(req, struct ci_req, req);
+	if (ci_req->b_buf)
+		free(ci_req->b_buf);
+	free(ci_req);
 }
 
 static void ep_enable(int num, int in, int maxpacket)
@@ -267,99 +280,102 @@ static int ci_ep_disable(struct usb_ep *ep)
 	return 0;
 }
 
-static int ci_bounce(struct ci_ep *ep, int in)
+static int ci_bounce(struct ci_req *ci_req, int in)
 {
-	uint32_t addr = (uint32_t)ep->req.buf;
-	uint32_t ba;
+	struct usb_request *req = &ci_req->req;
+	uint32_t addr = (uint32_t)req->buf;
+	uint32_t hwaddr;
+	uint32_t aligned_used_len;
 
 	/* Input buffer address is not aligned. */
 	if (addr & (ARCH_DMA_MINALIGN - 1))
 		goto align;
 
 	/* Input buffer length is not aligned. */
-	if (ep->req.length & (ARCH_DMA_MINALIGN - 1))
+	if (req->length & (ARCH_DMA_MINALIGN - 1))
 		goto align;
 
 	/* The buffer is well aligned, only flush cache. */
-	ep->b_len = ep->req.length;
-	ep->b_buf = ep->req.buf;
+	ci_req->hw_len = req->length;
+	ci_req->hw_buf = req->buf;
 	goto flush;
 
 align:
-	/* Use internal buffer for small payloads. */
-	if (ep->req.length <= 64) {
-		ep->b_len = 64;
-		ep->b_buf = ep->b_fast;
-	} else {
-		ep->b_len = roundup(ep->req.length, ARCH_DMA_MINALIGN);
-		ep->b_buf = memalign(ARCH_DMA_MINALIGN, ep->b_len);
-		if (!ep->b_buf)
+	if (ci_req->b_buf && req->length > ci_req->b_len) {
+		free(ci_req->b_buf);
+		ci_req->b_buf = 0;
+	}
+	if (!ci_req->b_buf) {
+		ci_req->b_len = roundup(req->length, ARCH_DMA_MINALIGN);
+		ci_req->b_buf = memalign(ARCH_DMA_MINALIGN, ci_req->b_len);
+		if (!ci_req->b_buf)
 			return -ENOMEM;
 	}
+	ci_req->hw_len = ci_req->b_len;
+	ci_req->hw_buf = ci_req->b_buf;
+
 	if (in)
-		memcpy(ep->b_buf, ep->req.buf, ep->req.length);
+		memcpy(ci_req->hw_buf, req->buf, req->length);
 
 flush:
-	ba = (uint32_t)ep->b_buf;
-	flush_dcache_range(ba, ba + ep->b_len);
+	hwaddr = (uint32_t)ci_req->hw_buf;
+	aligned_used_len = roundup(req->length, ARCH_DMA_MINALIGN);
+	flush_dcache_range(hwaddr, hwaddr + aligned_used_len);
 
 	return 0;
 }
 
-static void ci_debounce(struct ci_ep *ep, int in)
+static void ci_debounce(struct ci_req *ci_req, int in)
 {
-	uint32_t addr = (uint32_t)ep->req.buf;
-	uint32_t ba = (uint32_t)ep->b_buf;
+	struct usb_request *req = &ci_req->req;
+	uint32_t addr = (uint32_t)req->buf;
+	uint32_t hwaddr = (uint32_t)ci_req->hw_buf;
+	uint32_t aligned_used_len;
 
-	if (in) {
-		if (addr == ba)
-			return;		/* not a bounce */
-		goto free;
-	}
-	invalidate_dcache_range(ba, ba + ep->b_len);
+	if (in)
+		return;
+
+	aligned_used_len = roundup(req->actual, ARCH_DMA_MINALIGN);
+	invalidate_dcache_range(hwaddr, hwaddr + aligned_used_len);
 
-	if (addr == ba)
-		return;		/* not a bounce */
+	if (addr == hwaddr)
+		return; /* not a bounce */
 
-	memcpy(ep->req.buf, ep->b_buf, ep->req.actual);
-free:
-	/* Large payloads use allocated buffer, free it. */
-	if (ep->b_buf != ep->b_fast)
-		free(ep->b_buf);
+	memcpy(req->buf, ci_req->hw_buf, req->actual);
 }
 
-static int ci_ep_queue(struct usb_ep *ep,
-		struct usb_request *req, gfp_t gfp_flags)
+static void ci_ep_submit_next_request(struct ci_ep *ci_ep)
 {
-	struct ci_ep *ci_ep = container_of(ep, struct ci_ep, ep);
 	struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
 	struct ept_queue_item *item;
 	struct ept_queue_head *head;
-	int bit, num, len, in, ret;
+	int bit, num, len, in;
+	struct ci_req *ci_req;
+
+	ci_ep->req_primed = true;
+
 	num = ci_ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
 	in = (ci_ep->desc->bEndpointAddress & USB_DIR_IN) != 0;
 	item = ci_get_qtd(num, in);
 	head = ci_get_qh(num, in);
-	len = req->length;
 
-	ret = ci_bounce(ci_ep, in);
-	if (ret)
-		return ret;
+	ci_req = list_first_entry(&ci_ep->queue, struct ci_req, queue);
+	len = ci_req->req.length;
 
 	item->next = TERMINATE;
 	item->info = INFO_BYTES(len) | INFO_IOC | INFO_ACTIVE;
-	item->page0 = (uint32_t)ci_ep->b_buf;
-	item->page1 = ((uint32_t)ci_ep->b_buf & 0xfffff000) + 0x1000;
-	item->page2 = ((uint32_t)ci_ep->b_buf & 0xfffff000) + 0x2000;
-	item->page3 = ((uint32_t)ci_ep->b_buf & 0xfffff000) + 0x3000;
-	item->page4 = ((uint32_t)ci_ep->b_buf & 0xfffff000) + 0x4000;
+	item->page0 = (uint32_t)ci_req->hw_buf;
+	item->page1 = ((uint32_t)ci_req->hw_buf & 0xfffff000) + 0x1000;
+	item->page2 = ((uint32_t)ci_req->hw_buf & 0xfffff000) + 0x2000;
+	item->page3 = ((uint32_t)ci_req->hw_buf & 0xfffff000) + 0x3000;
+	item->page4 = ((uint32_t)ci_req->hw_buf & 0xfffff000) + 0x4000;
 	ci_flush_qtd(num);
 
 	head->next = (unsigned) item;
 	head->info = 0;
 
-	DBG("ept%d %s queue len %x, buffer %p\n",
-	    num, in ? "in" : "out", len, ci_ep->b_buf);
+	DBG("ept%d %s queue len %x, req %p, buffer %p\n",
+	    num, in ? "in" : "out", len, ci_req, ci_req->hw_buf);
 	ci_flush_qh(num);
 
 	if (in)
@@ -368,6 +384,29 @@ static int ci_ep_queue(struct usb_ep *ep,
 		bit = EPT_RX(num);
 
 	writel(bit, &udc->epprime);
+}
+
+static int ci_ep_queue(struct usb_ep *ep,
+		struct usb_request *req, gfp_t gfp_flags)
+{
+	struct ci_ep *ci_ep = container_of(ep, struct ci_ep, ep);
+	struct ci_req *ci_req = container_of(req, struct ci_req, req);
+	int in, ret;
+	int __maybe_unused num;
+
+	num = ci_ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
+	in = (ci_ep->desc->bEndpointAddress & USB_DIR_IN) != 0;
+
+	ret = ci_bounce(ci_req, in);
+	if (ret)
+		return ret;
+
+	DBG("ept%d %s pre-queue req %p, buffer %p\n",
+	    num, in ? "in" : "out", ci_req, ci_req->hw_buf);
+	list_add_tail(&ci_req->queue, &ci_ep->queue);
+
+	if (!ci_ep->req_primed)
+		ci_ep_submit_next_request(ci_ep);
 
 	return 0;
 }
@@ -376,6 +415,8 @@ static void handle_ep_complete(struct ci_ep *ep)
 {
 	struct ept_queue_item *item;
 	int num, in, len;
+	struct ci_req *ci_req;
+
 	num = ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
 	in = (ep->desc->bEndpointAddress & USB_DIR_IN) != 0;
 	if (num == 0)
@@ -387,16 +428,23 @@ static void handle_ep_complete(struct ci_ep *ep)
 		printf("EP%d/%s FAIL info=%x pg0=%x\n",
 		       num, in ? "in" : "out", item->info, item->page0);
 
+	ci_req = list_first_entry(&ep->queue, struct ci_req, queue);
+	list_del_init(&ci_req->queue);
+	ep->req_primed = false;
+
+	if (!list_empty(&ep->queue))
+		ci_ep_submit_next_request(ep);
+
 	len = (item->info >> 16) & 0x7fff;
-	ep->req.actual = ep->req.length - len;
-	ci_debounce(ep, in);
+	ci_req->req.actual = ci_req->req.length - len;
+	ci_debounce(ci_req, in);
 
-	DBG("ept%d %s complete %x\n",
-			num, in ? "in" : "out", len);
-	ep->req.complete(&ep->ep, &ep->req);
+	DBG("ept%d %s req %p, complete %x\n",
+	    num, in ? "in" : "out", ci_req, len);
+	ci_req->req.complete(&ep->ep, &ci_req->req);
 	if (num == 0) {
-		ep->req.length = 0;
-		usb_ep_queue(&ep->ep, &ep->req, 0);
+		ci_req->req.length = 0;
+		usb_ep_queue(&ep->ep, &ci_req->req, 0);
 		ep->desc = &ep0_in_desc;
 	}
 }
@@ -405,13 +453,18 @@ static void handle_ep_complete(struct ci_ep *ep)
 
 static void handle_setup(void)
 {
-	struct usb_request *req = &controller.ep[0].req;
+	struct ci_ep *ci_ep = &controller.ep[0];
+	struct ci_req *ci_req;
+	struct usb_request *req;
 	struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
 	struct ept_queue_head *head;
 	struct usb_ctrlrequest r;
 	int status = 0;
 	int num, in, _num, _in, i;
 	char *buf;
+
+	ci_req = list_first_entry(&ci_ep->queue, struct ci_req, queue);
+	req = &ci_req->req;
 	head = ci_get_qh(0, 0);	/* EP0 OUT */
 
 	ci_invalidate_qh(0);
@@ -424,6 +477,9 @@ static void handle_setup(void)
 	DBG("handle setup %s, %x, %x index %x value %x\n", reqname(r.bRequest),
 	    r.bRequestType, r.bRequest, r.wIndex, r.wValue);
 
+	list_del_init(&ci_req->queue);
+	ci_ep->req_primed = false;
+
 	switch (SETUP(r.bRequestType, r.bRequest)) {
 	case SETUP(USB_RECIP_ENDPOINT, USB_REQ_CLEAR_FEATURE):
 		_num = r.wIndex & 15;
@@ -701,6 +757,8 @@ static int ci_udc_probe(void)
 	/* Init EP 0 */
 	memcpy(&controller.ep[0].ep, &ci_ep_init[0], sizeof(*ci_ep_init));
 	controller.ep[0].desc = &ep0_in_desc;
+	INIT_LIST_HEAD(&controller.ep[0].queue);
+	controller.ep[0].req_primed = false;
 	controller.gadget.ep0 = &controller.ep[0].ep;
 	INIT_LIST_HEAD(&controller.gadget.ep0->ep_list);
 
@@ -708,6 +766,8 @@ static int ci_udc_probe(void)
 	for (i = 1; i < NUM_ENDPOINTS; i++) {
 		memcpy(&controller.ep[i].ep, &ci_ep_init[1],
 		       sizeof(*ci_ep_init));
+		INIT_LIST_HEAD(&controller.ep[i].queue);
+		controller.ep[i].req_primed = false;
 		list_add_tail(&controller.ep[i].ep.ep_list,
 			      &controller.gadget.ep_list);
 	}
diff --git a/drivers/usb/gadget/ci_udc.h b/drivers/usb/gadget/ci_udc.h
index 4425fd934570..23cff56d7ec9 100644
--- a/drivers/usb/gadget/ci_udc.h
+++ b/drivers/usb/gadget/ci_udc.h
@@ -77,15 +77,22 @@ struct ci_udc {
 #define CTRL_TXT_BULK	(2 << 18)
 #define CTRL_RXT_BULK	(2 << 2)
 
+struct ci_req {
+	struct usb_request	req;
+	struct list_head	queue;
+	/* Bounce buffer allocated if needed to align the transfer */
+	uint8_t *b_buf;
+	uint32_t b_len;
+	/* Buffer for the current transfer. Either req.buf/len or b_buf/len */
+	uint8_t *hw_buf;
+	uint32_t hw_len;
+};
+
 struct ci_ep {
 	struct usb_ep ep;
 	struct list_head queue;
+	bool req_primed;
 	const struct usb_endpoint_descriptor *desc;
-
-	struct usb_request req;
-	uint8_t *b_buf;
-	uint32_t b_len;
-	uint8_t b_fast[64] __aligned(ARCH_DMA_MINALIGN);
 };
 
 struct ci_drv {
-- 
1.8.1.5

[U-Boot] [PATCH 2/2] usb: ums: remove ci_udc special case

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Now that the ci_udc driver supports allocating multiple requests per
endpoint, we can revert the special-case added by a022c1e13c01 "usb:
ums: use only 1 buffer for CI_UDC".

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/usb/gadget/storage_common.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c
index 74300746b9db..02803df23c52 100644
--- a/drivers/usb/gadget/storage_common.c
+++ b/drivers/usb/gadget/storage_common.c
@@ -311,11 +311,7 @@ static struct fsg_lun *fsg_lun_from_dev(struct device *dev)
 #define DELAYED_STATUS	(EP0_BUFSIZE + 999)	/* An impossibly large value */
 
 /* Number of buffers we will use.  2 is enough for double-buffering */
-#ifndef CONFIG_CI_UDC
 #define FSG_NUM_BUFFERS	2
-#else
-#define FSG_NUM_BUFFERS	1 /* ci_udc only allows 1 req per ep at present */
-#endif
 
 /* Default size of buffer length. */
 #define FSG_BUFLEN	((u32)16384)
-- 
1.8.1.5

[U-Boot] [PATCH 1/2] usb: ci_udc: allow multiple buffer allocs per ep

[Marek Vasut]

On Tuesday, May 06, 2014 at 01:48:11 AM, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> Modify ci_ep_alloc_request() to return a dynamically allocated request
> object, rather than a singleton that's part of the endpoint. This
> requires moving various state from the endpoint structure to the request
> structure, since we need one copy per request.
> 
> The "fast bounce buffer" b_fast is removed by this change rather than
> moved to the request object. Instead, we enhance the bounce buffer logic
> in ci_bounce()/ci_debounce() to keep the bounce buffer around between
> request submissions. This avoids the need to allocate an arbitrarily-
> sized bounce buffer up-front, yet avoids incurring the allocation
> overhead each time a request is submitted.
> 
> A future enhancement would be to actually submit multiple requests to HW
> at once. The Linux driver shows that this is possible. That might improve
> throughput (depending on the USB protocol in use), since USB could be
> performing a transfer to one HW buffer in parallel with whatever SW
> actions U-Boot performs on another buffer. However, I have not made this
> change as part of this patch, in order to keep SW changes related to
> buffer management separate from any change in the way the HW is
> programmed.

Applied, thanks

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/2] usb: ums: remove ci_udc special case

[Marek Vasut]

On Tuesday, May 06, 2014 at 01:48:12 AM, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> Now that the ci_udc driver supports allocating multiple requests per
> endpoint, we can revert the special-case added by a022c1e13c01 "usb:
> ums: use only 1 buffer for CI_UDC".
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

Applied, thanks

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/2] usb: ci_udc: allow multiple buffer allocs per ep

[Jörg Krause]

Since this commit my Ethernet Gadget on a custom Freescale i.MX28 board is
broken. Using tftp to download files I get in almost all cases a timeout:

This is one case:



This is another one:



I reverted this commit from my master branch it works again as expected.




--
View this message in context: http://u-boot.10912.n7.nabble.com/PATCH-1-2-usb-ci-udc-allow-multiple-buffer-allocs-per-ep-tp179430p181250.html
Sent from the U-Boot mailing list archive at Nabble.com.

[U-Boot] [PATCH 1/2] usb: ci_udc: allow multiple buffer allocs per ep

[Jörg Krause]

Sorry, my previous post was not shown correctly. The raw text was missing. I
removed the annotation.

Since this commit my Ethernet Gadget on a custom Freescale i.MX28 board is
broken. Using tftp to download files I get in almost all cases a timeout:

This is one case:

Updating rootfs ...
using ci_udc, OUT ep- IN ep- STATUS ep-
MAC 00:19:b8:00:00:02
HOST MAC 00:19:b8:00:00:01
high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
USB network up!
Using usb_ether device
TFTP from server 10.0.0.1; our IP address is 10.0.0.2
Filename 'rootfs.ubifs'.
Load address: 0x40008000
Loading: ###########timeout sending packets to usb ethernet

This is another one:

Updating Boot Firmware ...
using ci_udc, OUT ep- IN ep- STATUS ep-
MAC 00:19:b8:00:00:02
HOST MAC 00:19:b8:00:00:01
high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
ERROR: The remote end did not respond in time.
at drivers/usb/gadget/ether.c:2388/usb_eth_init()

I reverted this commit from my master branch it works again as expected.




--
View this message in context: http://u-boot.10912.n7.nabble.com/PATCH-1-2-usb-ci-udc-allow-multiple-buffer-allocs-per-ep-tp179430p181251.html
Sent from the U-Boot mailing list archive at Nabble.com.

[U-Boot] [PATCH 1/2] usb: ci_udc: allow multiple buffer allocs per ep

[Stephen Warren]

On 06/02/2014 05:02 PM, J?rg Krause wrote:
> Sorry, my previous post was not shown correctly. The raw text was missing. I
> removed the annotation.
> 
> Since this commit my Ethernet Gadget on a custom Freescale i.MX28 board is
> broken. Using tftp to download files I get in almost all cases a timeout:

Sorry for the slow response; for some reason I didn't get a copy of the
message so I didn't notice it.

Could you tell me which include/config/xxx.h file you're using? I guess
if it's a custom board, perhaps that file isn't upstream. If so, I'm
particularly interested in whether you have CONFIG_USB_GADGET or
CONFIG_USB_ETHER enabled.

I wonder if applying the following series rather than reverting this
commits solves the issue?

[U-Boot,1/4] usb: ci_udc: detect queued requests on ep0
http://patchwork.ozlabs.org/patch/353926/

[U-Boot,2/4] usb: ci_udc: use a single descriptor for ep0
http://patchwork.ozlabs.org/patch/353932/

[U-Boot,3/4] usb: ci_udc: pre-allocate ep0 req
http://patchwork.ozlabs.org/patch/353931/

[U-Boot,4/4] usb: ci_udc: complete ep0 direction handling
http://patchwork.ozlabs.org/patch/353941/

The only other thing I can think of is that there's some issue with the
bounce buffer alignment or size being wrong somehow. Perhaps try
increasing those?

Or, perhaps req->actual ends up being wrong, so ci_debounce() isn't
cache-invalidating or copying enough data? Perhaps try rounding up
req->len instead of req->actual when performing the cache invalidate?

> This is one case:
> 
> Updating rootfs ...
> using ci_udc, OUT ep- IN ep- STATUS ep-
> MAC 00:19:b8:00:00:02
> HOST MAC 00:19:b8:00:00:01
> high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
> USB network up!
> Using usb_ether device
> TFTP from server 10.0.0.1; our IP address is 10.0.0.2
> Filename 'rootfs.ubifs'.
> Load address: 0x40008000
> Loading: ###########timeout sending packets to usb ethernet

I'm slightly confused by this log. Do you have 2 boards running U-Boot,
one running the USB controller in device mode, and this is the log from
some other board that's talking to that first board?

[U-Boot] [PATCH 1/2] usb: ci_udc: allow multiple buffer allocs per ep

[Jörg Krause]

On 06/10/2014 09:34 PM, Stephen Warren wrote:
> On 06/02/2014 05:02 PM, J?rg Krause wrote:
>> Sorry, my previous post was not shown correctly. The raw text was missing. I
>> removed the annotation.
>>
>> Since this commit my Ethernet Gadget on a custom Freescale i.MX28 board is
>> broken. Using tftp to download files I get in almost all cases a timeout:
> Sorry for the slow response; for some reason I didn't get a copy of the
> message so I didn't notice it.
>
> Could you tell me which include/config/xxx.h file you're using? I guess
> if it's a custom board, perhaps that file isn't upstream. If so, I'm
> particularly interested in whether you have CONFIG_USB_GADGET or
> CONFIG_USB_ETHER enabled.
You're right, it's not upstream. This is a part of my config:

#    define CONFIG_CI_UDC     /* ChipIdea CI13xxx UDC */
#    define CONFIG_USB_REG_BASE    0x80080000
#    define CONFIG_USBD_HS    /* High speed support for usb device and 
usbtty. */
#    define CONFIG_USB_GADGET_DUALSPEED
#    define CONFIG_USB_ETHER
#    define CONFIG_USB_ETH_CDC

>
> I wonder if applying the following series rather than reverting this
> commits solves the issue?
>
> [U-Boot,1/4] usb: ci_udc: detect queued requests on ep0
> http://patchwork.ozlabs.org/patch/353926/
>
> [U-Boot,2/4] usb: ci_udc: use a single descriptor for ep0
> http://patchwork.ozlabs.org/patch/353932/
>
> [U-Boot,3/4] usb: ci_udc: pre-allocate ep0 req
> http://patchwork.ozlabs.org/patch/353931/
>
> [U-Boot,4/4] usb: ci_udc: complete ep0 direction handling
> http://patchwork.ozlabs.org/patch/353941/
Applied, but the error still exists.

> The only other thing I can think of is that there's some issue with the
> bounce buffer alignment or size being wrong somehow. Perhaps try
> increasing those?
I checked this and I found that ARCH_DMA_MINALIGN is set to 64, which is 
not true for Freescale i.MX28. This processor has an ARM926EJ-S, which 
has an cache line size of 32.

In arch/arm/include/asm/cache.h the macro ARCH_DMA_MINALIGN is defined 
as followed:

#ifdef CONFIG_SYS_CACHELINE_SIZE
#define ARCH_DMA_MINALIGN    CONFIG_SYS_CACHELINE_SIZE
#else
#define ARCH_DMA_MINALIGN    64
#endif

And in /arch/arm/cpu/arm926ejs/cache.c as followed:

#ifndef CONFIG_SYS_CACHELINE_SIZE
#define CONFIG_SYS_CACHELINE_SIZE    32
#endif

arch/arm/include/asm/cache.h does not see the definition of 
CONFIG_SYS_CACHELINE_SIZE in /arch/arm/cpu/arm926ejs/cache.c, so it's a 
bad place to put it in there.

I defined CONFIG_SYS_CACHELINE_SIZE to 32 in my config header file and 
it works under the following circumstances: I have to disable the macro 
CONFIG_USB_GADGET_DUALSPEED. But, and this is strange, it works only the 
first time of a tftp download after a reset of the board. If I try to 
use tftp a second time, I get the same timeout error as before.

So, in short:

    => reset
    => run update_rootfs
    [...]
    done
    => reset
    => run update_rootfs
    [...]
    done

works and

    => reset
    => run update_rootfs
    [...]
    done
    => run update_rootfs
    [...]
    timeout sending packets to usb ethernet

results in a timeout. Strange.

Lastly, I changed CONFIG_SYS_CACHELINE_SIZE to 16 and this works for me 
in normal mode an in dual speed mode.

> Or, perhaps req->actual ends up being wrong, so ci_debounce() isn't
> cache-invalidating or copying enough data? Perhaps try rounding up
> req->len instead of req->actual when performing the cache invalidate?
I tried this, but with no success.
> I'm slightly confused by this log. Do you have 2 boards running U-Boot,
> one running the USB controller in device mode, and this is the log from
> some other board that's talking to that first board?
I have one board connect to a PC. The log shows two different errors 
happening on the board. The first log shows a tftp command on the board 
stopping with a timeout after receiving some packets. The second log 
shows a tftp command on the same board throwing an error before 
receiving any packet.

> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 1/2] usb: ci_udc: allow multiple buffer allocs per ep

[Marek Vasut]

On Thursday, June 12, 2014 at 10:36:55 AM, J?rg Krause wrote:
> On 06/10/2014 09:34 PM, Stephen Warren wrote:
> > On 06/02/2014 05:02 PM, J?rg Krause wrote:
> >> Sorry, my previous post was not shown correctly. The raw text was
> >> missing. I removed the annotation.
> >> 
> >> Since this commit my Ethernet Gadget on a custom Freescale i.MX28 board
> >> is
> > 
> >> broken. Using tftp to download files I get in almost all cases a timeout:
> > Sorry for the slow response; for some reason I didn't get a copy of the
> > message so I didn't notice it.
> > 
> > Could you tell me which include/config/xxx.h file you're using? I guess
> > if it's a custom board, perhaps that file isn't upstream. If so, I'm
> > particularly interested in whether you have CONFIG_USB_GADGET or
> > CONFIG_USB_ETHER enabled.
> 
> You're right, it's not upstream. This is a part of my config:
> 
> #    define CONFIG_CI_UDC     /* ChipIdea CI13xxx UDC */
> #    define CONFIG_USB_REG_BASE    0x80080000
> #    define CONFIG_USBD_HS    /* High speed support for usb device and
> usbtty. */
> #    define CONFIG_USB_GADGET_DUALSPEED
> #    define CONFIG_USB_ETHER
> #    define CONFIG_USB_ETH_CDC
> 
> > I wonder if applying the following series rather than reverting this
> > commits solves the issue?
> > 
> > [U-Boot,1/4] usb: ci_udc: detect queued requests on ep0
> > http://patchwork.ozlabs.org/patch/353926/
> > 
> > [U-Boot,2/4] usb: ci_udc: use a single descriptor for ep0
> > http://patchwork.ozlabs.org/patch/353932/
> > 
> > [U-Boot,3/4] usb: ci_udc: pre-allocate ep0 req
> > http://patchwork.ozlabs.org/patch/353931/
> > 
> > [U-Boot,4/4] usb: ci_udc: complete ep0 direction handling
> > http://patchwork.ozlabs.org/patch/353941/
> 
> Applied, but the error still exists.
> 
> > The only other thing I can think of is that there's some issue with the
> > bounce buffer alignment or size being wrong somehow. Perhaps try
> > increasing those?
> 
> I checked this and I found that ARCH_DMA_MINALIGN is set to 64, which is
> not true for Freescale i.MX28. This processor has an ARM926EJ-S, which
> has an cache line size of 32.
> 
> In arch/arm/include/asm/cache.h the macro ARCH_DMA_MINALIGN is defined
> as followed:
> 
> #ifdef CONFIG_SYS_CACHELINE_SIZE
> #define ARCH_DMA_MINALIGN    CONFIG_SYS_CACHELINE_SIZE
> #else
> #define ARCH_DMA_MINALIGN    64
> #endif
> 
> And in /arch/arm/cpu/arm926ejs/cache.c as followed:
> 
> #ifndef CONFIG_SYS_CACHELINE_SIZE
> #define CONFIG_SYS_CACHELINE_SIZE    32
> #endif
> 
> arch/arm/include/asm/cache.h does not see the definition of
> CONFIG_SYS_CACHELINE_SIZE in /arch/arm/cpu/arm926ejs/cache.c, so it's a
> bad place to put it in there.
> 
> I defined CONFIG_SYS_CACHELINE_SIZE to 32 in my config header file and
> it works under the following circumstances: I have to disable the macro
> CONFIG_USB_GADGET_DUALSPEED. But, and this is strange, it works only the
> first time of a tftp download after a reset of the board. If I try to
> use tftp a second time, I get the same timeout error as before.
> 
> So, in short:
> 
>     => reset
>     => run update_rootfs
>     [...]
>     done
>     => reset
>     => run update_rootfs
>     [...]
>     done
> 
> works and
> 
>     => reset
>     => run update_rootfs
>     [...]
>     done
>     => run update_rootfs
>     [...]
>     timeout sending packets to usb ethernet
> 
> results in a timeout. Strange.
> 
> Lastly, I changed CONFIG_SYS_CACHELINE_SIZE to 16 and this works for me
> in normal mode an in dual speed mode.
> 
> > Or, perhaps req->actual ends up being wrong, so ci_debounce() isn't
> > cache-invalidating or copying enough data? Perhaps try rounding up
> > req->len instead of req->actual when performing the cache invalidate?
> 
> I tried this, but with no success.
> 
> > I'm slightly confused by this log. Do you have 2 boards running U-Boot,
> > one running the USB controller in device mode, and this is the log from
> > some other board that's talking to that first board?
> 
> I have one board connect to a PC. The log shows two different errors
> happening on the board. The first log shows a tftp command on the board
> stopping with a timeout after receiving some packets. The second log
> shows a tftp command on the same board throwing an error before
> receiving any packet.

I sense buffer alignment issue. MXS aligns some buffer to 32b , but the CI block 
expects the buffer to be aligned to 64b or somesuch weird stuff.

[U-Boot] [PATCH 1/2] usb: ci_udc: allow multiple buffer allocs per ep

[Stephen Warren]

On 06/12/2014 02:36 AM, J?rg Krause wrote:
> 
> On 06/10/2014 09:34 PM, Stephen Warren wrote:
>> On 06/02/2014 05:02 PM, J?rg Krause wrote:
>>> Sorry, my previous post was not shown correctly. The raw text was missing. I
>>> removed the annotation.
>>>
>>> Since this commit my Ethernet Gadget on a custom Freescale i.MX28 board is
>>> broken. Using tftp to download files I get in almost all cases a timeout:
...
>> The only other thing I can think of is that there's some issue with the
>> bounce buffer alignment or size being wrong somehow. Perhaps try
>> increasing those?
>
> I checked this and I found that ARCH_DMA_MINALIGN is set to 64, which is
> not true for Freescale i.MX28. This processor has an ARM926EJ-S, which
> has an cache line size of 32.

This should be fine. The only effect of setting ARCH_DMA_MINALIGN to a
value that's larger than it needs to be is: a tiny amount of RAM will be
wasted, since structures are allocated aligned to a larger boundary/size
than is strictly necessary in HW, and since SW should always round the
size of an area to be flushed up to ARCH_DMA_MINALIGN too, then perhaps
a few more cache lines are flushed than is strictly necessary.

> In arch/arm/include/asm/cache.h the macro ARCH_DMA_MINALIGN is defined
> as followed:
> 
> #ifdef CONFIG_SYS_CACHELINE_SIZE
> #define ARCH_DMA_MINALIGN    CONFIG_SYS_CACHELINE_SIZE
> #else
> #define ARCH_DMA_MINALIGN    64
> #endif
> 
> And in /arch/arm/cpu/arm926ejs/cache.c as followed:
> 
> #ifndef CONFIG_SYS_CACHELINE_SIZE
> #define CONFIG_SYS_CACHELINE_SIZE    32
> #endif
> 
> arch/arm/include/asm/cache.h does not see the definition of
> CONFIG_SYS_CACHELINE_SIZE in /arch/arm/cpu/arm926ejs/cache.c, so it's a
> bad place to put it in there.

Ah yes. That ifdef should certainly be in some global header that is
always included before/from arch/arm/include/asm/cache.h.

Without the correct CACHELINE_SIZE definition, the cache flushing
routines are probably flushing entirely the address or amount of memory.

> I defined CONFIG_SYS_CACHELINE_SIZE to 32 in my config header file and

Yes, that seems like the best fix.

FWIW, for Tegra we have a per-SoC header that defines SoC-specific
constants, so that each board file doesn't need to duplicate them.

> it works under the following circumstances: I have to disable the macro
> CONFIG_USB_GADGET_DUALSPEED. But, and this is strange, it works only the
> first time of a tftp download after a reset of the board. If I try to
> use tftp a second time, I get the same timeout error as before.

That's very odd. I can't explain why this wouldn't fix the issue
completely, unless there's *also* some other bug.

...
> Lastly, I changed CONFIG_SYS_CACHELINE_SIZE to 16 and this works for me
> in normal mode an in dual speed mode.

That's quite odd, and doesn't make much sense at all. You're sure the
cache line size really isn't 16?

...
>> I'm slightly confused by this log. Do you have 2 boards running U-Boot,
>> one running the USB controller in device mode, and this is the log from
>> some other board that's talking to that first board?
>
> I have one board connect to a PC. The log shows two different errors
> happening on the board. The first log shows a tftp command on the board
> stopping with a timeout after receiving some packets. The second log
> shows a tftp command on the same board throwing an error before
> receiving any packet.

So U-Boot is running on the board, and the logs are from the board, i.e.
you're running the "tftp" command in U-Boot on the board?

If so, I'm confused how ci_udc comes into play at all; doesn't "tftp"
use the USB controller in host mode, whereas ci_udc is only used for
device mode?

[U-Boot] [PATCH 1/2] usb: ci_udc: allow multiple buffer allocs per ep

[Marek Vasut]

On Thursday, June 12, 2014 at 05:55:40 PM, Stephen Warren wrote:
[...]
> >> I'm slightly confused by this log. Do you have 2 boards running U-Boot,
> >> one running the USB controller in device mode, and this is the log from
> >> some other board that's talking to that first board?
> > 
> > I have one board connect to a PC. The log shows two different errors
> > happening on the board. The first log shows a tftp command on the board
> > stopping with a timeout after receiving some packets. The second log
> > shows a tftp command on the same board throwing an error before
> > receiving any packet.
> 
> So U-Boot is running on the board, and the logs are from the board, i.e.
> you're running the "tftp" command in U-Boot on the board?
> 
> If so, I'm confused how ci_udc comes into play at all; doesn't "tftp"
> use the USB controller in host mode, whereas ci_udc is only used for
> device mode?

U-Boot CDC ethernet support, that's pretty normal mode of operation ;-)

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/2] usb: ci_udc: allow multiple buffer allocs per ep

[Stephen Warren]

On 06/12/2014 11:30 AM, Marek Vasut wrote:
> On Thursday, June 12, 2014 at 05:55:40 PM, Stephen Warren wrote:
> [...]
>>>> I'm slightly confused by this log. Do you have 2 boards running U-Boot,
>>>> one running the USB controller in device mode, and this is the log from
>>>> some other board that's talking to that first board?
>>>
>>> I have one board connect to a PC. The log shows two different errors
>>> happening on the board. The first log shows a tftp command on the board
>>> stopping with a timeout after receiving some packets. The second log
>>> shows a tftp command on the same board throwing an error before
>>> receiving any packet.
>>
>> So U-Boot is running on the board, and the logs are from the board, i.e.
>> you're running the "tftp" command in U-Boot on the board?
>>
>> If so, I'm confused how ci_udc comes into play at all; doesn't "tftp"
>> use the USB controller in host mode, whereas ci_udc is only used for
>> device mode?
> 
> U-Boot CDC ethernet support, that's pretty normal mode of operation ;-)

Oh, so that makes U-Boot into a kind of virtual NIC where the packets
don't go anywhere but U-Boot's internal network stack, and that feature
runs in the background with the interactive shell still fully operation
then?

All the USB device mode support I've used is a synchronous/blocking
shell command (dfu, ums) rather than something that runs in the
background, so I figured CDC Ethernet would work the same.

Perhaps I should try and get CDC Ethernet working... Can both
CONFIG_USB_GADGET and CONFIG_USB_ETHER co-exist I wonder? Actually,
U-Boot acting as a USB serial port running at the same time as dfu or
ums would be more useful to me.

[U-Boot] [PATCH 1/2] usb: ci_udc: allow multiple buffer allocs per ep

[Marek Vasut]

On Thursday, June 12, 2014 at 07:42:55 PM, Stephen Warren wrote:
> On 06/12/2014 11:30 AM, Marek Vasut wrote:
> > On Thursday, June 12, 2014 at 05:55:40 PM, Stephen Warren wrote:
> > [...]
> > 
> >>>> I'm slightly confused by this log. Do you have 2 boards running
> >>>> U-Boot, one running the USB controller in device mode, and this is
> >>>> the log from some other board that's talking to that first board?
> >>> 
> >>> I have one board connect to a PC. The log shows two different errors
> >>> happening on the board. The first log shows a tftp command on the board
> >>> stopping with a timeout after receiving some packets. The second log
> >>> shows a tftp command on the same board throwing an error before
> >>> receiving any packet.
> >> 
> >> So U-Boot is running on the board, and the logs are from the board, i.e.
> >> you're running the "tftp" command in U-Boot on the board?
> >> 
> >> If so, I'm confused how ci_udc comes into play at all; doesn't "tftp"
> >> use the USB controller in host mode, whereas ci_udc is only used for
> >> device mode?
> > 
> > U-Boot CDC ethernet support, that's pretty normal mode of operation ;-)
> 
> Oh, so that makes U-Boot into a kind of virtual NIC where the packets
> don't go anywhere but U-Boot's internal network stack, and that feature
> runs in the background with the interactive shell still fully operation
> then?

Nah, it runs in the foreground. I think I wrote some short howto on this when I 
initially wanted to boast about it [1].

[1] http://www.denx-cs.de/?q=blogm28singlewiredebug

> All the USB device mode support I've used is a synchronous/blocking
> shell command (dfu, ums) rather than something that runs in the
> background, so I figured CDC Ethernet would work the same.

It's sync, blocking :)

> Perhaps I should try and get CDC Ethernet working... Can both
> CONFIG_USB_GADGET and CONFIG_USB_ETHER co-exist I wonder? Actually,
> U-Boot acting as a USB serial port running at the same time as dfu or
> ums would be more useful to me.

Check the link ;-)

Best regards,
Marek Vasut