[U-Boot] [PATCH 1/2] usb: ci_udc: allow multiple buffer allocs per ep
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 {
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)
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
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
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... Sent from the U-Boot mailing list archive at Nabble.com.
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... Sent from the U-Boot mailing list archive at Nabble.com.
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?
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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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 [...] doneworks and
=> reset => run update_rootfs [...] done => run update_rootfs [...] timeout sending packets to usb ethernetresults 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.
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?
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
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.
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
participants (4)
-
Jörg Krause -
Jörg Krause -
Marek Vasut -
Stephen Warren