[U-Boot] [RFC/PATCH] common/usb: use cache-alligned buffers

[U-Boot] [RFC/PATCH] common/usb: use cache-alligned buffers

[Ilya Yanok]

Make sure that all transfer buffers are cache-alligned so that HCD
driver could safely do cache invalidate on them.

Signed-off-by: Ilya Yanok <yanok@emcraft.com>
---

Hi guys,

I'm working with EHCI-controller on OMAP3 board and I've found that
ehci-hcd.c driver procudes a lot of unalligned cache invalidate
warnings.
This patch makes all buffers used internally inside common/usb.c
cache-alligned and now I can scan the bus without warnings.

I've started doing the same with usb_storage but then I realized that
there is a lot of buffers comming from upper level (FS driver), so
going this way we will need to fix them all.

What do you think, how should we deal with it? Make all buffers
alligned or implement bounce-buffering inside host driver?

Regards, Ilya.

 common/usb.c |   63 ++++++++++++++++++++++++++++++++++-----------------------
 1 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/common/usb.c b/common/usb.c
index 2cd50db..1e04d08 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -48,6 +48,7 @@
 #include <command.h>
 #include <asm/processor.h>
 #include <linux/ctype.h>
+#include <linux/compiler.h>
 #include <asm/byteorder.h>
 
 #include <usb.h>
@@ -72,7 +73,11 @@ static struct usb_device usb_dev[USB_MAX_DEVICE];
 static int dev_index;
 static int running;
 static int asynch_allowed;
-static struct devrequest setup_packet;
+static uchar __setup_packet[ALIGN(sizeof(struct devrequest),
+		ARCH_DMA_MINALIGN)]
+		__aligned(ARCH_DMA_MINALIGN);
+static struct devrequest * const setup_packet =
+		(struct devrequest *)&__setup_packet;
 
 char usb_started; /* flag for the started/stopped USB status */
 
@@ -190,17 +195,17 @@ int usb_control_msg(struct usb_device *dev, unsigned int pipe,
 	}
 
 	/* set setup command */
-	setup_packet.requesttype = requesttype;
-	setup_packet.request = request;
-	setup_packet.value = cpu_to_le16(value);
-	setup_packet.index = cpu_to_le16(index);
-	setup_packet.length = cpu_to_le16(size);
+	setup_packet->requesttype = requesttype;
+	setup_packet->request = request;
+	setup_packet->value = cpu_to_le16(value);
+	setup_packet->index = cpu_to_le16(index);
+	setup_packet->length = cpu_to_le16(size);
 	USB_PRINTF("usb_control_msg: request: 0x%X, requesttype: 0x%X, " \
 		   "value 0x%X index 0x%X length 0x%X\n",
 		   request, requesttype, value, index, size);
 	dev->status = USB_ST_NOT_PROC; /*not yet processed */
 
-	submit_control_msg(dev, pipe, data, size, &setup_packet);
+	submit_control_msg(dev, pipe, data, size, setup_packet);
 	if (timeout == 0)
 		return (int)size;
 
@@ -271,7 +276,7 @@ int usb_maxpacket(struct usb_device *dev, unsigned long pipe)
  * to update the compiler (Occurs with at least several GCC 4.{1,2},x
  * CodeSourcery compilers like e.g. 2007q3, 2008q1, 2008q3 lite editions on ARM)
  */
-static void  __attribute__((noinline))
+static void noinline
 usb_set_maxpacket_ep(struct usb_device *dev, struct usb_endpoint_descriptor *ep)
 {
 	int b;
@@ -678,7 +683,7 @@ static int usb_string_sub(struct usb_device *dev, unsigned int langid,
  */
 int usb_string(struct usb_device *dev, int index, char *buf, size_t size)
 {
-	unsigned char mybuf[USB_BUFSIZ];
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, mybuf, USB_BUFSIZ);
 	unsigned char *tbuf;
 	int err;
 	unsigned int u, idx;
@@ -778,7 +783,7 @@ int usb_new_device(struct usb_device *dev)
 {
 	int addr, err;
 	int tmp;
-	unsigned char tmpbuf[USB_BUFSIZ];
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, tmpbuf, USB_BUFSIZ);
 
 	/* We still haven't set the Address yet */
 	addr = dev->devnum;
@@ -795,12 +800,13 @@ int usb_new_device(struct usb_device *dev)
 	dev->epmaxpacketin[0] = 8;
 	dev->epmaxpacketout[0] = 8;
 
-	err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, &dev->descriptor, 8);
+	err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, tmpbuf, 8);
 	if (err < 8) {
 		printf("\n      USB device not responding, " \
 		       "giving up (status=%lX)\n", dev->status);
 		return 1;
 	}
+	memcpy(&dev->descriptor, tmpbuf, 8);
 #else
 	/* This is a Windows scheme of initialization sequence, with double
 	 * reset of the device (Linux uses the same sequence)
@@ -888,8 +894,8 @@ int usb_new_device(struct usb_device *dev)
 
 	tmp = sizeof(dev->descriptor);
 
-	err = usb_get_descriptor(dev, USB_DT_DEVICE, 0,
-				 &dev->descriptor, sizeof(dev->descriptor));
+	err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, tmpbuf,
+				 sizeof(dev->descriptor));
 	if (err < tmp) {
 		if (err < 0)
 			printf("unable to get device descriptor (error=%d)\n",
@@ -899,6 +905,7 @@ int usb_new_device(struct usb_device *dev)
 				"(expected %i, got %i)\n", tmp, err);
 		return 1;
 	}
+	memcpy(&dev->descriptor, tmpbuf, sizeof(dev->descriptor));
 	/* correct le values */
 	le16_to_cpus(&dev->descriptor.bcdUSB);
 	le16_to_cpus(&dev->descriptor.idVendor);
@@ -1067,7 +1074,8 @@ static int hub_port_reset(struct usb_device *dev, int port,
 			unsigned short *portstat)
 {
 	int tries;
-	struct usb_port_status portsts;
+	ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts,
+			sizeof(struct usb_port_status));
 	unsigned short portstatus, portchange;
 
 	USB_HUB_PRINTF("hub_port_reset: resetting port %d...\n", port);
@@ -1076,13 +1084,13 @@ static int hub_port_reset(struct usb_device *dev, int port,
 		usb_set_port_feature(dev, port + 1, USB_PORT_FEAT_RESET);
 		wait_ms(200);
 
-		if (usb_get_port_status(dev, port + 1, &portsts) < 0) {
+		if (usb_get_port_status(dev, port + 1, portsts) < 0) {
 			USB_HUB_PRINTF("get_port_status failed status %lX\n",
 					dev->status);
 			return -1;
 		}
-		portstatus = le16_to_cpu(portsts.wPortStatus);
-		portchange = le16_to_cpu(portsts.wPortChange);
+		portstatus = le16_to_cpu(portsts->wPortStatus);
+		portchange = le16_to_cpu(portsts->wPortChange);
 
 		USB_HUB_PRINTF("portstatus %x, change %x, %s\n",
 				portstatus, portchange,
@@ -1120,19 +1128,20 @@ static int hub_port_reset(struct usb_device *dev, int port,
 void usb_hub_port_connect_change(struct usb_device *dev, int port)
 {
 	struct usb_device *usb;
-	struct usb_port_status portsts;
+	ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts,
+			sizeof(struct usb_port_status));
 	unsigned short portstatus;
 
 	/* Check status */
-	if (usb_get_port_status(dev, port + 1, &portsts) < 0) {
+	if (usb_get_port_status(dev, port + 1, portsts) < 0) {
 		USB_HUB_PRINTF("get_port_status failed\n");
 		return;
 	}
 
-	portstatus = le16_to_cpu(portsts.wPortStatus);
+	portstatus = le16_to_cpu(portsts->wPortStatus);
 	USB_HUB_PRINTF("portstatus %x, change %x, %s\n",
 			portstatus,
-			le16_to_cpu(portsts.wPortChange),
+			le16_to_cpu(portsts->wPortChange),
 			portspeed(portstatus));
 
 	/* Clear the connection change status */
@@ -1181,7 +1190,8 @@ void usb_hub_port_connect_change(struct usb_device *dev, int port)
 int usb_hub_configure(struct usb_device *dev)
 {
 	int i;
-	unsigned char buffer[USB_BUFSIZ], *bitmap;
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, USB_BUFSIZ);
+	unsigned char *bitmap;
 	struct usb_hub_descriptor *descriptor;
 	struct usb_hub_device *hub;
 #ifdef USB_HUB_DEBUG
@@ -1303,16 +1313,17 @@ int usb_hub_configure(struct usb_device *dev)
 	usb_hub_power_on(hub);
 
 	for (i = 0; i < dev->maxchild; i++) {
-		struct usb_port_status portsts;
+		ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts,
+				sizeof(struct usb_port_status));
 		unsigned short portstatus, portchange;
 
-		if (usb_get_port_status(dev, i + 1, &portsts) < 0) {
+		if (usb_get_port_status(dev, i + 1, portsts) < 0) {
 			USB_HUB_PRINTF("get_port_status failed\n");
 			continue;
 		}
 
-		portstatus = le16_to_cpu(portsts.wPortStatus);
-		portchange = le16_to_cpu(portsts.wPortChange);
+		portstatus = le16_to_cpu(portsts->wPortStatus);
+		portchange = le16_to_cpu(portsts->wPortChange);
 		USB_HUB_PRINTF("Port %d Status %X Change %X\n",
 				i + 1, portstatus, portchange);
 
-- 
1.7.6.4

[U-Boot] [RFC/PATCH] common/usb: use cache-alligned buffers

[Mike Frysinger]

On Wed, Oct 26, 2011 at 23:05, Ilya Yanok wrote:
> What do you think, how should we deal with it? Make all buffers
> alligned or implement bounce-buffering inside host driver?

since this is a common issue to most USB controllers, aligning the
buffers is probably best.  and should be relatively painless.

what's the code size before/after your change ?

> --- a/common/usb.c
> +++ b/common/usb.c
>
> -static struct devrequest setup_packet;
> +static uchar __setup_packet[ALIGN(sizeof(struct devrequest),
> + ? ? ? ? ? ? ? ARCH_DMA_MINALIGN)]
> + ? ? ? ? ? ? ? __aligned(ARCH_DMA_MINALIGN);
> +static struct devrequest * const setup_packet =
> + ? ? ? ? ? ? ? (struct devrequest *)&__setup_packet;

i'm not sure about this.  why won't this work ?
static struct devrequest setup_packet __aligned(ARCH_DMA_MINALIGN);
-mike

[U-Boot] [RFC/PATCH] common/usb: use cache-alligned buffers

[Ilya Yanok]

Hi Mike,

On 27.10.2011 02:20, Mike Frysinger wrote:
>> What do you think, how should we deal with it? Make all buffers
>> alligned or implement bounce-buffering inside host driver?
> 
> since this is a common issue to most USB controllers, aligning the
> buffers is probably best.  and should be relatively painless.

I agree with 'best' part but doubt about 'painless'.

> what's the code size before/after your change ?

[u-boot (mcx-new)]$ ls -l u-boot.bin-*
-rw-rw-r-- 1 yanok yanok 314248 ???.  30 22:55 u-boot.bin-after
-rw-rw-r-- 1 yanok yanok 314020 ???.  30 22:52 u-boot.bin-before

It's 248 bytes. But please note that this is only common/usb.c that has
buffers aligned with my patch. We also need to fix all other code that
passes buffers to HCD (for ex. for USB storage that means that we have
to fix usb_storage itself as well as partition support and FS driver).
And even with that we still will need some bounce buffer to handle the
situation with unaligned address passed by user...

>> --- a/common/usb.c
>> +++ b/common/usb.c
>>
>> -static struct devrequest setup_packet;
>> +static uchar __setup_packet[ALIGN(sizeof(struct devrequest),
>> +               ARCH_DMA_MINALIGN)]
>> +               __aligned(ARCH_DMA_MINALIGN);
>> +static struct devrequest * const setup_packet =
>> +               (struct devrequest *)&__setup_packet;
> 
> i'm not sure about this.  why won't this work ?
> static struct devrequest setup_packet __aligned(ARCH_DMA_MINALIGN);

Well, we have to align both start address and size for safe cache
operations and my understanding is that __aligned gives up only start
address not size. Please correct me if I'm wrong.

Regards, Ilya.