[U-Boot] [PATCH] usb: ci_udc: Allocate the qTD list directly
Instead of weird allocation of ci_drv->items_mem and then even weirder distribution of offsets in this memory area into ci_drv->items array, just allocate ci_drv->items as a big slab of aligned memory (replaces ci_drv->items_mem) and let ci_get_qtd() do the distribution of offsets in this memory area.
Signed-off-by: Marek Vasut marex@denx.de Cc: Jörg Krause jkrause@posteo.de Cc: Stephen Warren swarren@wwwdotorg.org --- drivers/usb/gadget/ci_udc.c | 18 +++++++----------- drivers/usb/gadget/ci_udc.h | 3 +-- 2 files changed, 8 insertions(+), 13 deletions(-)
Note: Please test, it's too late and I'm barely conscious anymore ...
diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c index 334b5d2..8d92324 100644 --- a/drivers/usb/gadget/ci_udc.c +++ b/drivers/usb/gadget/ci_udc.c @@ -137,7 +137,7 @@ static struct ept_queue_head *ci_get_qh(int ep_num, int dir_in) */ static struct ept_queue_item *ci_get_qtd(int ep_num, int dir_in) { - return controller.items[(ep_num * 2) + dir_in]; + return controller.items + ((ep_num * 2) + dir_in); }
/** @@ -727,7 +727,8 @@ static int ci_udc_probe(void) const int ilist_sz = NUM_ENDPOINTS * ilist_ent_sz;
/* The QH list must be aligned to 4096 bytes. */ - controller.epts = memalign(eplist_align, eplist_sz); + if (!controller.epts) + controller.epts = memalign(eplist_align, eplist_sz); if (!controller.epts) return -ENOMEM; memset(controller.epts, 0, eplist_sz); @@ -738,12 +739,13 @@ static int ci_udc_probe(void) * only one of them is used for the endpoint at time, so we can group * them together. */ - controller.items_mem = memalign(ilist_align, ilist_sz); - if (!controller.items_mem) { + if (!controller.items) + controller.items = memalign(ilist_align, ilist_sz); + if (!controller.items) { free(controller.epts); return -ENOMEM; } - memset(controller.items_mem, 0, ilist_sz); + memset(controller.items, 0, ilist_sz);
for (i = 0; i < 2 * NUM_ENDPOINTS; i++) { /* @@ -763,12 +765,6 @@ static int ci_udc_probe(void) head->next = TERMINATE; head->info = 0;
- imem = controller.items_mem + ((i >> 1) * ilist_ent_sz); - if (i & 1) - imem += sizeof(struct ept_queue_item); - - controller.items[i] = (struct ept_queue_item *)imem; - if (i & 1) { ci_flush_qh(i - 1); ci_flush_qtd(i - 1); diff --git a/drivers/usb/gadget/ci_udc.h b/drivers/usb/gadget/ci_udc.h index 7d76af8..d464368 100644 --- a/drivers/usb/gadget/ci_udc.h +++ b/drivers/usb/gadget/ci_udc.h @@ -101,8 +101,7 @@ struct ci_drv { struct usb_gadget_driver *driver; struct ehci_ctrl *ctrl; struct ept_queue_head *epts; - struct ept_queue_item *items[2 * NUM_ENDPOINTS]; - uint8_t *items_mem; + struct ept_queue_item *items; struct ci_ep ep[NUM_ENDPOINTS]; };
On Tuesday, July 01, 2014 at 01:53:18 AM, Marek Vasut wrote:
Instead of weird allocation of ci_drv->items_mem and then even weirder distribution of offsets in this memory area into ci_drv->items array, just allocate ci_drv->items as a big slab of aligned memory (replaces ci_drv->items_mem) and let ci_get_qtd() do the distribution of offsets in this memory area.
Signed-off-by: Marek Vasut marex@denx.de Cc: Jörg Krause jkrause@posteo.de Cc: Stephen Warren swarren@wwwdotorg.org
Ah yes, this will complain about unused *imem , I will fix it when applying if this is the right patch.
Best regards, Marek Vasut
Instead of weird allocation of ci_drv->items_mem and then even weirder distribution of offsets in this memory area into ci_drv->items array, just allocate ci_drv->items as a big slab of aligned memory (replaces ci_drv->items_mem) and let ci_get_qtd() do the distribution of offsets in this memory area.
Signed-off-by: Marek Vasut marex@denx.de Cc: Jörg Krause jkrause@posteo.de Cc: Stephen Warren swarren@wwwdotorg.org --- drivers/usb/gadget/ci_udc.c | 19 ++++++------------- drivers/usb/gadget/ci_udc.h | 3 +-- 2 files changed, 7 insertions(+), 15 deletions(-)
V2: Rebase on top of u-boot-usb/master instead of the research branch
diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c index 1af6d12..8333db2 100644 --- a/drivers/usb/gadget/ci_udc.c +++ b/drivers/usb/gadget/ci_udc.c @@ -130,7 +130,7 @@ static struct ept_queue_head *ci_get_qh(int ep_num, int dir_in) */ static struct ept_queue_item *ci_get_qtd(int ep_num, int dir_in) { - return controller.items[(ep_num * 2) + dir_in]; + return controller.items + ((ep_num * 2) + dir_in); }
/** @@ -769,7 +769,6 @@ static int ci_pullup(struct usb_gadget *gadget, int is_on) static int ci_udc_probe(void) { struct ept_queue_head *head; - uint8_t *imem; int i;
const int num = 2 * NUM_ENDPOINTS; @@ -796,12 +795,12 @@ static int ci_udc_probe(void) * only one of them is used for the endpoint at time, so we can group * them together. */ - controller.items_mem = memalign(ilist_align, ilist_sz); - if (!controller.items_mem) { + controller.items = memalign(ilist_align, ilist_sz); + if (!controller.items) { free(controller.epts); return -ENOMEM; } - memset(controller.items_mem, 0, ilist_sz); + memset(controller.items, 0, ilist_sz);
for (i = 0; i < 2 * NUM_ENDPOINTS; i++) { /* @@ -821,12 +820,6 @@ static int ci_udc_probe(void) head->next = TERMINATE; head->info = 0;
- imem = controller.items_mem + ((i >> 1) * ilist_ent_sz); - if (i & 1) - imem += sizeof(struct ept_queue_item); - - controller.items[i] = (struct ept_queue_item *)imem; - if (i & 1) { ci_flush_qh(i - 1); ci_flush_qtd(i - 1); @@ -855,7 +848,7 @@ static int ci_udc_probe(void)
ci_ep_alloc_request(&controller.ep[0].ep, 0); if (!controller.ep0_req) { - free(controller.items_mem); + free(controller.items); free(controller.epts); return -ENOMEM; } @@ -910,7 +903,7 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver) controller.driver = NULL;
ci_ep_free_request(&controller.ep[0].ep, &controller.ep0_req->req); - free(controller.items_mem); + free(controller.items); free(controller.epts);
return 0; diff --git a/drivers/usb/gadget/ci_udc.h b/drivers/usb/gadget/ci_udc.h index c214402..3115b15 100644 --- a/drivers/usb/gadget/ci_udc.h +++ b/drivers/usb/gadget/ci_udc.h @@ -102,8 +102,7 @@ struct ci_drv { struct usb_gadget_driver *driver; struct ehci_ctrl *ctrl; struct ept_queue_head *epts; - struct ept_queue_item *items[2 * NUM_ENDPOINTS]; - uint8_t *items_mem; + struct ept_queue_item *items; struct ci_ep ep[NUM_ENDPOINTS]; };
On 07/01/2014 02:04 AM, Marek Vasut wrote:
Instead of weird allocation of ci_drv->items_mem and then even weirder distribution of offsets in this memory area into ci_drv->items array, just allocate ci_drv->items as a big slab of aligned memory (replaces ci_drv->items_mem) and let ci_get_qtd() do the distribution of offsets in this memory area.
Signed-off-by: Marek Vasut marex@denx.de Cc: Jörg Krause jkrause@posteo.de Cc: Stephen Warren swarren@wwwdotorg.org
drivers/usb/gadget/ci_udc.c | 19 ++++++------------- drivers/usb/gadget/ci_udc.h | 3 +-- 2 files changed, 7 insertions(+), 15 deletions(-)
V2: Rebase on top of u-boot-usb/master instead of the research branch
diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c index 1af6d12..8333db2 100644 --- a/drivers/usb/gadget/ci_udc.c +++ b/drivers/usb/gadget/ci_udc.c @@ -130,7 +130,7 @@ static struct ept_queue_head *ci_get_qh(int ep_num, int dir_in) */ static struct ept_queue_item *ci_get_qtd(int ep_num, int dir_in) {
- return controller.items[(ep_num * 2) + dir_in];
return controller.items + ((ep_num * 2) + dir_in); }
/**
@@ -769,7 +769,6 @@ static int ci_pullup(struct usb_gadget *gadget, int is_on) static int ci_udc_probe(void) { struct ept_queue_head *head;
uint8_t *imem; int i;
const int num = 2 * NUM_ENDPOINTS;
@@ -796,12 +795,12 @@ static int ci_udc_probe(void) * only one of them is used for the endpoint at time, so we can group * them together. */
- controller.items_mem = memalign(ilist_align, ilist_sz);
- if (!controller.items_mem) {
- controller.items = memalign(ilist_align, ilist_sz);
- if (!controller.items) { free(controller.epts); return -ENOMEM; }
- memset(controller.items_mem, 0, ilist_sz);
memset(controller.items, 0, ilist_sz);
for (i = 0; i < 2 * NUM_ENDPOINTS; i++) { /*
@@ -821,12 +820,6 @@ static int ci_udc_probe(void) head->next = TERMINATE; head->info = 0;
imem = controller.items_mem + ((i >> 1) * ilist_ent_sz);if (i & 1)imem += sizeof(struct ept_queue_item);controller.items[i] = (struct ept_queue_item *)imem;- if (i & 1) { ci_flush_qh(i - 1); ci_flush_qtd(i - 1);
@@ -855,7 +848,7 @@ static int ci_udc_probe(void)
ci_ep_alloc_request(&controller.ep[0].ep, 0); if (!controller.ep0_req) {
free(controller.items_mem);
free(controller.items);@@ -910,7 +903,7 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver) controller.driver = NULL;
ci_ep_free_request(&controller.ep[0].ep, &controller.ep0_req->req);
- free(controller.items_mem);
free(controller.items); free(controller.epts);
return 0;
diff --git a/drivers/usb/gadget/ci_udc.h b/drivers/usb/gadget/ci_udc.h index c214402..3115b15 100644 --- a/drivers/usb/gadget/ci_udc.h +++ b/drivers/usb/gadget/ci_udc.h @@ -102,8 +102,7 @@ struct ci_drv { struct usb_gadget_driver *driver; struct ehci_ctrl *ctrl; struct ept_queue_head *epts;
- struct ept_queue_item *items[2 * NUM_ENDPOINTS];
- uint8_t *items_mem;
- struct ept_queue_item *items; struct ci_ep ep[NUM_ENDPOINTS]; };
I made a test on u-boot-arm/master before (Test#1) and after applying this patch (Test#2). After a reset I run this script: test_usb=setenv i 64; while test ${i} -gt 0; do echo ${i}; tftp ${rootfs_file}; setexpr i ${i} - 1; done; setenv i;
Both tests (Test#1 and Test#2) runs fine with the script. But if I do run tftp ${rootfs_file} manually from the console, I get the known error starting the fourth run for both Test#1 and Test#2.
I attached a log file for the error.
On Tuesday, July 01, 2014 at 08:51:45 AM, Jörg Krause wrote:
On 07/01/2014 02:04 AM, Marek Vasut wrote:
Instead of weird allocation of ci_drv->items_mem and then even weirder distribution of offsets in this memory area into ci_drv->items array, just allocate ci_drv->items as a big slab of aligned memory (replaces ci_drv->items_mem) and let ci_get_qtd() do the distribution of offsets in this memory area.
Signed-off-by: Marek Vasut marex@denx.de Cc: Jörg Krause jkrause@posteo.de Cc: Stephen Warren swarren@wwwdotorg.org
drivers/usb/gadget/ci_udc.c | 19 ++++++------------- drivers/usb/gadget/ci_udc.h | 3 +-- 2 files changed, 7 insertions(+), 15 deletions(-)
V2: Rebase on top of u-boot-usb/master instead of the research branch
[...]
I made a test on u-boot-arm/master before (Test#1) and after applying this patch (Test#2).
Can we please sync on the same codebase (u-boot-usb/master) ?
After a reset I run this script: test_usb=setenv i 64; while test ${i} -gt 0; do echo ${i}; tftp ${rootfs_file}; setexpr i ${i} - 1; done; setenv i;
Both tests (Test#1 and Test#2) runs fine with the script. But if I do run tftp ${rootfs_file} manually from the console, I get the known error starting the fourth run for both Test#1 and Test#2.
Can you edit arch/arm/cpu/arm926ejs/cache.c and change the debug() to printf() , then re-test please ? I suspect this might trap something still. Ah, and please test on u-boot-usb/master with this patch.
Best regards, Marek Vasut
On 07/01/2014 12:17 PM, Marek Vasut wrote:
On Tuesday, July 01, 2014 at 08:51:45 AM, Jörg Krause wrote:
On 07/01/2014 02:04 AM, Marek Vasut wrote:
Instead of weird allocation of ci_drv->items_mem and then even weirder distribution of offsets in this memory area into ci_drv->items array, just allocate ci_drv->items as a big slab of aligned memory (replaces ci_drv->items_mem) and let ci_get_qtd() do the distribution of offsets in this memory area.
Signed-off-by: Marek Vasut marex@denx.de Cc: Jörg Krause jkrause@posteo.de Cc: Stephen Warren swarren@wwwdotorg.org
drivers/usb/gadget/ci_udc.c | 19 ++++++------------- drivers/usb/gadget/ci_udc.h | 3 +-- 2 files changed, 7 insertions(+), 15 deletions(-)
V2: Rebase on top of u-boot-usb/master instead of the research branch
[...]
I made a test on u-boot-arm/master before (Test#1) and after applying this patch (Test#2).
Can we please sync on the same codebase (u-boot-usb/master) ?
Sorry, this is a type. I tested on u-boot-usb/master.
After a reset I run this script: test_usb=setenv i 64; while test ${i} -gt 0; do echo ${i}; tftp ${rootfs_file}; setexpr i ${i} - 1; done; setenv i;
Both tests (Test#1 and Test#2) runs fine with the script. But if I do run tftp ${rootfs_file} manually from the console, I get the known error starting the fourth run for both Test#1 and Test#2.
Can you edit arch/arm/cpu/arm926ejs/cache.c and change the debug() to printf() , then re-test please ? I suspect this might trap something still. Ah, and please test on u-boot-usb/master with this patch.
No additional output on the console.
Best regards, Marek Vasut
On Tuesday, July 01, 2014 at 01:03:25 PM, Jörg Krause wrote:
On 07/01/2014 12:17 PM, Marek Vasut wrote:
On Tuesday, July 01, 2014 at 08:51:45 AM, Jörg Krause wrote:
On 07/01/2014 02:04 AM, Marek Vasut wrote:
Instead of weird allocation of ci_drv->items_mem and then even weirder distribution of offsets in this memory area into ci_drv->items array, just allocate ci_drv->items as a big slab of aligned memory (replaces ci_drv->items_mem) and let ci_get_qtd() do the distribution of offsets in this memory area.
Signed-off-by: Marek Vasut marex@denx.de Cc: Jörg Krause jkrause@posteo.de Cc: Stephen Warren swarren@wwwdotorg.org
drivers/usb/gadget/ci_udc.c | 19 ++++++------------- drivers/usb/gadget/ci_udc.h | 3 +-- 2 files changed, 7 insertions(+), 15 deletions(-)
V2: Rebase on top of u-boot-usb/master instead of the research branch
[...]
I made a test on u-boot-arm/master before (Test#1) and after applying this patch (Test#2).
Can we please sync on the same codebase (u-boot-usb/master) ?
Sorry, this is a type. I tested on u-boot-usb/master.
After a reset I run this script: test_usb=setenv i 64; while test ${i} -gt 0; do echo ${i}; tftp ${rootfs_file}; setexpr i ${i} - 1; done; setenv i;
Both tests (Test#1 and Test#2) runs fine with the script. But if I do run tftp ${rootfs_file} manually from the console, I get the known error starting the fourth run for both Test#1 and Test#2.
Can you edit arch/arm/cpu/arm926ejs/cache.c and change the debug() to printf() , then re-test please ? I suspect this might trap something still. Ah, and please test on u-boot-usb/master with this patch.
No additional output on the console.
What does this mean? Do you see warning messages prefixed with "CACHE: " ?
Best regards, Marek Vasut
On 07/01/2014 01:19 PM, Marek Vasut wrote:
[snip]
Can you edit arch/arm/cpu/arm926ejs/cache.c and change the debug() to printf() , then re-test please ? I suspect this might trap something still. Ah, and please test on u-boot-usb/master with this patch.
No additional output on the console.
What does this mean? Do you see warning messages prefixed with "CACHE: " ?
No messages prefixed with "CACHE: ". Just the usual error message.
=> 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:2392/usb_eth_init()
Best regards, Marek Vasut
On Tuesday, July 01, 2014 at 01:22:41 PM, Jörg Krause wrote:
On 07/01/2014 01:19 PM, Marek Vasut wrote:
[snip]
Can you edit arch/arm/cpu/arm926ejs/cache.c and change the debug() to printf() , then re-test please ? I suspect this might trap something still. Ah, and please test on u-boot-usb/master with this patch.
No additional output on the console.
What does this mean? Do you see warning messages prefixed with "CACHE: " ?
No messages prefixed with "CACHE: ". Just the usual error message.
=> 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:2392/usb_eth_init()
Just to make sure, did you remove any CDC ethernet tunables (like cdc_connect_timeout) from your env ?
Best regards, Marek Vasut
On 07/01/2014 01:35 PM, Marek Vasut wrote:
On Tuesday, July 01, 2014 at 01:22:41 PM, Jörg Krause wrote:
On 07/01/2014 01:19 PM, Marek Vasut wrote:
[snip]
Can you edit arch/arm/cpu/arm926ejs/cache.c and change the debug() to printf() , then re-test please ? I suspect this might trap something still. Ah, and please test on u-boot-usb/master with this patch.
No additional output on the console.
What does this mean? Do you see warning messages prefixed with "CACHE: " ?
No messages prefixed with "CACHE: ". Just the usual error message.
=> 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:2392/usb_eth_init()
Just to make sure, did you remove any CDC ethernet tunables (like cdc_connect_timeout) from your env ?
=> printenv cdc_connect_timeout ## Error: "cdc_connect_timeout" not defined
Also, no other CDC ethernet variables.
These are the related env variables from my config header file:
#define CONFIG_IPADDR 10.0.0.2 #define CONFIG_SERVERIP 10.0.0.1 #define CONFIG_NETMASK 255.255.255.0
#define CONFIG_EXTRA_ENV_SETTINGS \ "ethact=usb_ether\0" \ "ethprime=usb_ether\0" \ "usbnet_hostaddr=00:19:B8:00:00:01\0" \ "usbnet_devaddr=00:19:B8:00:00:02\0" \ [...]
And these are my settings for USB:
/* USB */ #ifdef CONFIG_CMD_USB # define CONFIG_EHCI_MXS_PORT0 # define CONFIG_USB_MAX_CONTROLLER_COUNT 1 # 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 #endif
Best regards, Marek Vasut
On 07/01/2014 01:22 PM, Jörg Krause wrote:
On 07/01/2014 01:19 PM, Marek Vasut wrote:
[snip]
Can you edit arch/arm/cpu/arm926ejs/cache.c and change the debug() to printf() , then re-test please ? I suspect this might trap something still. Ah, and please test on u-boot-usb/master with this patch.
No additional output on the console.
What does this mean? Do you see warning messages prefixed with "CACHE: " ?
No messages prefixed with "CACHE: ". Just the usual error message.
I am sorry, but maybe I edited arch/arm/cpu/arm926ejs/ in a another branch and compiled in u-boot-usb. If I run now tftp with printf enabled in arch/arm/cpu/arm926ejs/cache.c I get the following "CACHE: " messages:
CACHE: Misaligned operation at range [40008000, 4000c653] CACHE: Misaligned operation at range [43fd0b0c, 43fd0b60]
=> 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:2392/usb_eth_init()
Best regards, Marek Vasut
On 07/01/2014 04:34 PM, Jörg Krause wrote:
On 07/01/2014 01:22 PM, Jörg Krause wrote:
On 07/01/2014 01:19 PM, Marek Vasut wrote:
[snip]
Can you edit arch/arm/cpu/arm926ejs/cache.c and change the debug() to printf() , then re-test please ? I suspect this might trap something still. Ah, and please test on u-boot-usb/master with this patch.
No additional output on the console.
What does this mean? Do you see warning messages prefixed with "CACHE: " ?
No messages prefixed with "CACHE: ". Just the usual error message.
I am sorry, but maybe I edited arch/arm/cpu/arm926ejs/ in a another branch and compiled in u-boot-usb. If I run now tftp with printf enabled in arch/arm/cpu/arm926ejs/cache.c I get the following "CACHE: " messages:
CACHE: Misaligned operation at range [40008000, 4000c653] CACHE: Misaligned operation at range [43fd0b0c, 43fd0b60]
That happens right when you first use the UDC driver right? If so, I hope that "[U-Boot] [PATCH 1/6] usb: ci_udc: fix ci_flush_{qh, qtd} calls in ci_udc_probe()" will fix that.
On 07/02/2014 12:36 AM, Stephen Warren wrote:
On 07/01/2014 04:34 PM, Jörg Krause wrote:
On 07/01/2014 01:22 PM, Jörg Krause wrote:
On 07/01/2014 01:19 PM, Marek Vasut wrote:
[snip]
Can you edit arch/arm/cpu/arm926ejs/cache.c and change the debug() to printf() , then re-test please ? I suspect this might trap something still. Ah, and please test on u-boot-usb/master with this patch.
No additional output on the console.
What does this mean? Do you see warning messages prefixed with "CACHE: " ?
No messages prefixed with "CACHE: ". Just the usual error message.
I am sorry, but maybe I edited arch/arm/cpu/arm926ejs/ in a another branch and compiled in u-boot-usb. If I run now tftp with printf enabled in arch/arm/cpu/arm926ejs/cache.c I get the following "CACHE: " messages:
CACHE: Misaligned operation at range [40008000, 4000c653] CACHE: Misaligned operation at range [43fd0b0c, 43fd0b60]That happens right when you first use the UDC driver right? If so, I hope that "[U-Boot] [PATCH 1/6] usb: ci_udc: fix ci_flush_{qh, qtd} calls in ci_udc_probe()" will fix that.
Checkout clean u-boot-usb/master, applied board specific patches and applied the mentioned patch. Running tftp three times in a row:
=> reset resetting ... HTLLCLLC
U-Boot 2014.07-rc3-g0b32423-dirty (Jul 02 2014 - 00:44:53)
CPU: Freescale i.MX28 rev1.2 at 454 MHz BOOT: NAND, 3V3 DRAM: 64 MiB NAND: 128 MiB In: serial Out: serial Err: serial Net: usb_ether [PRIME] Hit any key to stop autoboot: 0 => tftp imx28-airlino.dtb 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 'imx28-airlino.dtb'. Load address: 0x40008000 Loading: ## 4.3 MiB/s done Bytes transferred = 18003 (4653 hex) CACHE: Misaligned operation at range [40008000, 4000c653] => 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 'imx28-airlino.dtb'. Load address: 0x40008000 Loading: ## 1.7 MiB/s done Bytes transferred = 18003 (4653 hex) CACHE: Misaligned operation at range [40008000, 4000c653] => 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 'imx28-airlino.dtb'. Load address: 0x40008000 Loading: ## 3.4 MiB/s done Bytes transferred = 18003 (4653 hex) CACHE: Misaligned operation at range [40008000, 4000c653] => 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()
On 07/01/2014 04:47 PM, Jörg Krause wrote:
On 07/02/2014 12:36 AM, Stephen Warren wrote:
On 07/01/2014 04:34 PM, Jörg Krause wrote:
On 07/01/2014 01:22 PM, Jörg Krause wrote:
On 07/01/2014 01:19 PM, Marek Vasut wrote:
[snip]
> Can you edit arch/arm/cpu/arm926ejs/cache.c and change the debug() to > printf() , then re-test please ? I suspect this might trap something > still. Ah, and please test on u-boot-usb/master with this patch. No additional output on the console.
What does this mean? Do you see warning messages prefixed with "CACHE: " ?
No messages prefixed with "CACHE: ". Just the usual error message.
I am sorry, but maybe I edited arch/arm/cpu/arm926ejs/ in a another branch and compiled in u-boot-usb. If I run now tftp with printf enabled in arch/arm/cpu/arm926ejs/cache.c I get the following "CACHE: " messages:
CACHE: Misaligned operation at range [40008000, 4000c653] CACHE: Misaligned operation at range [43fd0b0c, 43fd0b60]That happens right when you first use the UDC driver right? If so, I hope that "[U-Boot] [PATCH 1/6] usb: ci_udc: fix ci_flush_{qh, qtd} calls in ci_udc_probe()" will fix that.
Checkout clean u-boot-usb/master, applied board specific patches and applied the mentioned patch. Running tftp three times in a row:
...
U-Boot 2014.07-rc3-g0b32423-dirty (Jul 02 2014 - 00:44:53)
...
=> tftp imx28-airlino.dtb
...
Loading: ## 4.3 MiB/s done Bytes transferred = 18003 (4653 hex) CACHE: Misaligned operation at range [40008000, 4000c653]
OK, that particular error happens well after the network transfer phase of the tftp command, so is likely nothing to do with ci_udc. It'd be great if you could track it down and fix it though.
Ah, I bet that 40008000 is your load address; the address that the downloaded data is being copied to?
On 07/02/2014 12:51 AM, Stephen Warren wrote:
[...]
Loading: ## 4.3 MiB/s done Bytes transferred = 18003 (4653 hex) CACHE: Misaligned operation at range [40008000, 4000c653]OK, that particular error happens well after the network transfer phase of the tftp command, so is likely nothing to do with ci_udc. It'd be great if you could track it down and fix it though.
Oh, any idea where to look at?
Ah, I bet that 40008000 is your load address; the address that the downloaded data is being copied to?
You're right, 40008000 is my load address.
On 07/01/2014 04:57 PM, Jörg Krause wrote:
On 07/02/2014 12:51 AM, Stephen Warren wrote:
[...]
Loading: ## 4.3 MiB/s done Bytes transferred = 18003 (4653 hex) CACHE: Misaligned operation at range [40008000, 4000c653]OK, that particular error happens well after the network transfer phase of the tftp command, so is likely nothing to do with ci_udc. It'd be great if you could track it down and fix it though.
Oh, any idea where to look at?
My guess is common/cmd_net.c netboot_common() which does:
/* flush cache */ flush_cache(load_addr, size);
That seems pointless and wrong:
* The Ethernet driver should do any cache management it requires as part of its low-level IO code, before copying the data to the load address.
* A cache flush after copying data into RAM seems pointless. A cache *invalidate* /might/ make sense in some circumstances (e.g. after DMA before CPU reads), but that's not the case here.
* The top-level TFTP code can't do the cache management, since it can't be sure that the file length is an exact multiple of the cache line size, and hence can't guarantee that cache management won't corrupt other adjacent data. That's why the cache management must be performed by the low-level network driver on its own aligned buffers.
In summary, I think we should remove that flush_cache() call, but I'm not going to send a patch for that right before I go on vacation right before a release...
On 06/30/2014 06:04 PM, Marek Vasut wrote:
Instead of weird allocation of ci_drv->items_mem and then even weirder distribution of offsets in this memory area into ci_drv->items array, just allocate ci_drv->items as a big slab of aligned memory (replaces ci_drv->items_mem) and let ci_get_qtd() do the distribution of offsets in this memory area.
As I mentioned on IRC, this patch isn't correct.
The existing code ensures that each pair of QTDs are correctly aligned, hence the slight complexity. The new code only actively ensures that the first QTD is correctly aligned; the subsequent QTDs will only be correctly aligned if the sizeof the QTD structure is an exact multiple of the alignment requirements. In practice, it is on my system at least, but that may not be generally true.
On Tuesday, July 01, 2014 at 05:03:17 PM, Stephen Warren wrote:
On 06/30/2014 06:04 PM, Marek Vasut wrote:
Instead of weird allocation of ci_drv->items_mem and then even weirder distribution of offsets in this memory area into ci_drv->items array, just allocate ci_drv->items as a big slab of aligned memory (replaces ci_drv->items_mem) and let ci_get_qtd() do the distribution of offsets in this memory area.
As I mentioned on IRC, this patch isn't correct.
The existing code ensures that each pair of QTDs are correctly aligned, hence the slight complexity. The new code only actively ensures that the first QTD is correctly aligned; the subsequent QTDs will only be correctly aligned if the sizeof the QTD structure is an exact multiple of the alignment requirements. In practice, it is on my system at least, but that may not be generally true.
It is on every system with 32b cachelines. On system with 64b cachelines, you won't be able to use one endpoint as both in and out, which I don't think is doable now either.
Best regards, Marek Vasut
On 07/01/2014 09:13 AM, Marek Vasut wrote:
On Tuesday, July 01, 2014 at 05:03:17 PM, Stephen Warren wrote:
On 06/30/2014 06:04 PM, Marek Vasut wrote:
Instead of weird allocation of ci_drv->items_mem and then even weirder distribution of offsets in this memory area into ci_drv->items array, just allocate ci_drv->items as a big slab of aligned memory (replaces ci_drv->items_mem) and let ci_get_qtd() do the distribution of offsets in this memory area.
As I mentioned on IRC, this patch isn't correct.
The existing code ensures that each pair of QTDs are correctly aligned, hence the slight complexity. The new code only actively ensures that the first QTD is correctly aligned; the subsequent QTDs will only be correctly aligned if the sizeof the QTD structure is an exact multiple of the alignment requirements. In practice, it is on my system at least, but that may not be generally true.
It is on every system with 32b cachelines.
Sure.
On system with 64b cachelines, you won't be able to use one endpoint as both in and out, which I don't think is doable now either.
Yes, the 2nd QTD in each pair isn't correctly aligned at present for 64byte cachelines. I was thinking about sending a patch to fix that (perhaps theoretical) issue.
Actually, I wonder if that's related to Jörg's issue at all; his system has the cache line size set to 64 even though that's incorrect, perhaps that influences the behaviour of the cache code...
On Tuesday, July 01, 2014 at 05:16:26 PM, Stephen Warren wrote:
On 07/01/2014 09:13 AM, Marek Vasut wrote:
On Tuesday, July 01, 2014 at 05:03:17 PM, Stephen Warren wrote:
On 06/30/2014 06:04 PM, Marek Vasut wrote:
Instead of weird allocation of ci_drv->items_mem and then even weirder distribution of offsets in this memory area into ci_drv->items array, just allocate ci_drv->items as a big slab of aligned memory (replaces ci_drv->items_mem) and let ci_get_qtd() do the distribution of offsets in this memory area.
As I mentioned on IRC, this patch isn't correct.
The existing code ensures that each pair of QTDs are correctly aligned, hence the slight complexity. The new code only actively ensures that the first QTD is correctly aligned; the subsequent QTDs will only be correctly aligned if the sizeof the QTD structure is an exact multiple of the alignment requirements. In practice, it is on my system at least, but that may not be generally true.
It is on every system with 32b cachelines.
Sure.
On system with 64b cachelines, you won't be able to use one endpoint as both in and out, which I don't think is doable now either.
Yes, the 2nd QTD in each pair isn't correctly aligned at present for 64byte cachelines. I was thinking about sending a patch to fix that (perhaps theoretical) issue.
Ah bah, you're right.
Actually, I wonder if that's related to Jörg's issue at all; his system has the cache line size set to 64 even though that's incorrect, perhaps that influences the behaviour of the cache code...
My understanding of his test is that he uses u-boot-usb/master + this patch and there are not modifications to the CPU support code. If there are, then his test is useless of course.
With u-boot-usb/master + this patch , on MX28 there should be no problem.
Best regards, Marek Vasut
participants (3)
-
Jörg Krause -
Marek Vasut -
Stephen Warren