[U-Boot] [PATCH] usb: ci_udc: Allocate the qTD list directly

[U-Boot] [PATCH] usb: ci_udc: Allocate the qTD list directly

[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 | 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];
 };
 
-- 
2.0.0.rc2

[U-Boot] [PATCH] usb: ci_udc: Allocate the qTD list directly

[Marek Vasut]

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

[U-Boot] [PATCH V2] usb: ci_udc: Allocate the qTD list directly

[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];
 };
 
-- 
2.0.0.rc2

[U-Boot] [PATCH V2] usb: ci_udc: Allocate the qTD list directly

[Jörg Krause]

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);
>   		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];
>   };
>

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.

-------------- next part --------------
=> reset
resetting ...
HTLLCLLC

U-Boot 2014.07-rc3-g18e0313 (Jul 01 2014 - 08:32:45)

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 ${rootfs_file}
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: #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #######################################################
	 5.6 MiB/s
done
Bytes transferred = 13205504 (c98000 hex)
=> 
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: #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #######################################################
	 5.6 MiB/s
done
Bytes transferred = 13205504 (c98000 hex)
=> 
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: #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #######################################################
	 5.6 MiB/s
done
Bytes transferred = 13205504 (c98000 hex)
=> 
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()
=> 

[U-Boot] [PATCH V2] usb: ci_udc: Allocate the qTD list directly

[Marek Vasut]

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

[U-Boot] [PATCH V2] usb: ci_udc: Allocate the qTD list directly

[Jörg Krause]

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

[U-Boot] [PATCH V2] usb: ci_udc: Allocate the qTD list directly

[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

[U-Boot] [PATCH V2] usb: ci_udc: Allocate the qTD list directly

[Jörg Krause]

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

[U-Boot] [PATCH V2] usb: ci_udc: Allocate the qTD list directly

[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

[U-Boot] [PATCH V2] usb: ci_udc: Allocate the qTD list directly

[Jörg Krause]

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

[U-Boot] [PATCH V2] usb: ci_udc: Allocate the qTD list directly

[Stephen Warren]

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.

[U-Boot] [PATCH V2] usb: ci_udc: Allocate the qTD list directly

[Marek Vasut]

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

[U-Boot] [PATCH V2] usb: ci_udc: Allocate the qTD list directly

[Stephen Warren]

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

[U-Boot] [PATCH V2] usb: ci_udc: Allocate the qTD list directly

[Marek Vasut]

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

[U-Boot] [PATCH V2] usb: ci_udc: Allocate the qTD list directly

[Jörg Krause]

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
>

[U-Boot] [PATCH V2] usb: ci_udc: Allocate the qTD list directly

[Stephen Warren]

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.

[U-Boot] [PATCH V2] usb: ci_udc: Allocate the qTD list directly

[Jörg Krause]

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()

[U-Boot] [PATCH V2] usb: ci_udc: Allocate the qTD list directly

[Stephen Warren]

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?

[U-Boot] [PATCH V2] usb: ci_udc: Allocate the qTD list directly

[Jörg Krause]

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.

[U-Boot] [PATCH V2] usb: ci_udc: Allocate the qTD list directly

[Stephen Warren]

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