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 <[email protected]>

Cc J?rg Krause <[email protected]>

Cc Stephen Warren <[email protected]>

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 <[email protected]>

Cc J?rg Krause <[email protected]>

Cc Stephen Warren <[email protected]>

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

drivers/usb/gadget/ci_udc.c +6 -13

index 1af6d12..8333db2 100644
@@ -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;

drivers/usb/gadget/ci_udc.h +0 -0

index c214402..3115b15 100644

Quoted 16 lines · 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 <[email protected]>

Cc J?rg Krause <[email protected]>

Cc Stephen Warren <[email protected]>

---

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

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.

Quoted 16 lines · J?rg Krause

Quoted 15 lines · 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 <[email protected]>

Cc J?rg Krause <[email protected]>

Cc Stephen Warren <[email protected]>

---

   drivers/usb/gadget/ci_udc.c | 19 ++++++-------------
   drivers/usb/gadget/ci_udc.h |  3 +--
   2 files changed, 7 insertions(+), 15 deletions(-)

Quoted 1 line

Quoted 1 line

V2: Rebase on top of u-boot-usb/master instead of the research branch

[...]

Quoted 2 lines

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

Quoted 7 lines

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

Quoted 17 lines · Marek Vasut

Quoted 16 lines · J?rg Krause

Quoted 15 lines · 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 <[email protected]>

Cc J?rg Krause <[email protected]>

Cc Stephen Warren <[email protected]>

---

   drivers/usb/gadget/ci_udc.c | 19 ++++++-------------
   drivers/usb/gadget/ci_udc.h |  3 +--
   2 files changed, 7 insertions(+), 15 deletions(-)

Quoted 8 lines

Quoted 1 line

Quoted 1 line

V2: Rebase on top of u-boot-usb/master instead of the research branch

[...]

Quoted 2 lines

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.

Quoted 8 lines

Quoted 7 lines

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.

Quoted 3 lines

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.

[snip]

Quoted 4 lines

Quoted 3 lines

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

Quoted 10 lines · Marek Vasut

[snip]

Quoted 5 lines

Quoted 3 lines

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

Quoted 11 lines · J?rg Krause

Quoted 8 lines · Marek Vasut

[snip]

Quoted 4 lines

Quoted 3 lines

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.

Quoted 7 lines

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

Quoted 7 lines · Marek Vasut

[snip]

Quoted 4 lines

Quoted 3 lines

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

Quoted 2 lines

Best regards,
Marek Vasut

Quoted 10 lines · J?rg Krause

Quoted 7 lines · Marek Vasut

[snip]

Quoted 4 lines

Quoted 3 lines

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: " ?

Quoted 1 line

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]

Quoted 14 lines · J?rg Krause

Quoted 9 lines · J?rg Krause

Quoted 7 lines · Marek Vasut

[snip]

Quoted 4 lines

Quoted 3 lines

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:

Quoted 2 lines

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.

Quoted 15 lines · Stephen Warren

Quoted 14 lines · J?rg Krause

Quoted 9 lines · J?rg Krause

Quoted 7 lines · Marek Vasut

[snip]

Quoted 4 lines

Quoted 3 lines

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:

Quoted 5 lines

Quoted 2 lines

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]

[...]

Quoted 5 lines

     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?

Quoted 6 lines · Stephen Warren

[...]

Quoted 5 lines

     Loading: ##
          4.3 MiB/s
     done
     Bytes transferred = 18003 (4653 hex)
     CACHE: Misaligned operation at range [40008000, 4000c653]

Quoted 3 lines

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?

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.

Quoted 5 lines · 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.

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.

Quoted 9 lines · Stephen Warren

Quoted 5 lines · 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.

As I mentioned on IRC, this patch isn't correct.

Quoted 6 lines

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.

Quoted 10 lines · Marek Vasut

Quoted 9 lines · Stephen Warren

Quoted 5 lines · 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.

As I mentioned on IRC, this patch isn't correct.

Quoted 8 lines

Quoted 6 lines

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.

Quoted 3 lines

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