[U-Boot] [patch 1/2 V2] fix USB initialisation procedure

[Daniel Hellstrom <daniel@gaisler.com>]

Hello,

Please see my comment on commit 48867208444cb2a82e2af9c3249e90b7ed4a1751 
below.


Remy Bohmer wrote:

>The max packet size is encoded as 0,1,2,3 for 8,16,32,64 bytes.
>At some places directly 8,16,32,64 was used instead of the encoded
>value. Made a enum for the options to make this more clear and to help
>preventing similar errors in the future.
>
>After fixing this bug it became clear that another bug existed where
>the 'pipe' is and-ed with PIPE_* flags, where it should have been
>'usb_pipetype(pipe)', or even better usb_pipeint(pipe).
>
>Also removed the triple 'get_device_descriptor' sequence, it has no use,
>and Windows nor Linux behaves that way.
>  
>

>There is also a poll going on with a timeout when usb_control_msg() fails.
>However, the poll is useless, because the flag will never be set on a error,
>because there is no code that runs in a parallel that can set this flag.
>Changed this to something more logical.
>  
>
The USB-driver ISR may set dev->status, I believe that is why the code 
is waiting for for a couple of ms. This patch makes the LEON3 UHCI 
support fail in usb_control_msg(), changing back to the old code the 
LEON3 UHCI driver (cpu/leon3/usb_uhci.c) works again.

I believe this is also the case for board/MAI/AmigaOneG3SE/usb_uhci.c 
and board/mpl/common/usb_uhci.c, they are also modifying the flag from 
the interrupt handler.

Best regards,
Daniel Hellstrom

>Tested on AT91SAM9261ek and compared the flow on the USB bus to what
>Linux is doing. There is no difference anymore in the early initialisation
>sequence.
>
>Signed-off-by: Remy Bohmer <linux@bohmer.net>
>---
> common/usb.c           |   56 ++++++++++++++++++++++++-------------------------
> drivers/usb/usb_ohci.c |   14 +++++-------
> include/usb.h          |   16 +++++++++++---
> 3 files changed, 47 insertions(+), 39 deletions(-)
>
>Index: u-boot-git-22092008/common/usb.c
>===================================================================
>--- u-boot-git-22092008.orig/common/usb.c	2008-10-10 09:42:59.000000000 +0200
>+++ u-boot-git-22092008/common/usb.c	2008-10-10 10:20:43.000000000 +0200
>@@ -196,15 +196,16 @@ int usb_control_msg(struct usb_device *d
> 	if (timeout == 0)
> 		return (int)size;
> 
>-	while (timeout--) {
>-		if (!((volatile unsigned long)dev->status & USB_ST_NOT_PROC))
>-			break;
>-		wait_ms(1);
>-	}
>-	if (dev->status == 0)
>-		return dev->act_len;
>-	else
>+	if (dev->status != 0) {
>+		/*
>+		 * Let's wait a while for the timeout to elapse.
>+		 * It has no real use, but it keeps the interface happy.
>+		 */
>+		wait_ms(timeout);
> 		return -1;
>+	}
>+
>+	return dev->act_len;
> }
> 
> /*-------------------------------------------------------------------
>@@ -442,14 +443,14 @@ int usb_get_configuration_no(struct usb_
> 
> 
> 	config = (struct usb_config_descriptor *)&buffer[0];
>-	result = usb_get_descriptor(dev, USB_DT_CONFIG, cfgno, buffer, 8);
>-	if (result < 8) {
>+	result = usb_get_descriptor(dev, USB_DT_CONFIG, cfgno, buffer, 9);
>+	if (result < 9) {
> 		if (result < 0)
> 			printf("unable to get descriptor, error %lX\n",
> 				dev->status);
> 		else
> 			printf("config descriptor too short " \
>-				"(expected %i, got %i)\n", 8, result);
>+				"(expected %i, got %i)\n", 9, result);
> 		return -1;
> 	}
> 	tmp = le16_to_cpu(config->wTotalLength);
>@@ -777,7 +778,7 @@ int usb_new_device(struct usb_device *de
> 	 * Several USB stick devices report ERR: CTL_TIMEOUT, caused by an
> 	 * invalid header while reading 8 bytes as device descriptor. */
> 	dev->descriptor.bMaxPacketSize0 = 8;	    /* Start off at 8 bytes  */
>-	dev->maxpacketsize = 0;		/* Default to 8 byte max packet size */
>+	dev->maxpacketsize = PACKET_SIZE_8;
> 	dev->epmaxpacketin [0] = 8;
> 	dev->epmaxpacketout[0] = 8;
> 
>@@ -788,15 +789,12 @@ int usb_new_device(struct usb_device *de
> 		return 1;
> 	}
> #else
>-	/* this is a Windows scheme of initialization sequence, with double
>-	 * reset of the device (Linux uses the same sequence, but without double
>-	 * reset. This double reset is not considered harmful and matches the
>-	 * Windows behaviour)
>+	/* This is a Windows scheme of initialization sequence, with double
>+	 * reset of the device (Linux uses the same sequence)
> 	 * Some equipment is said to work only with such init sequence; this
> 	 * patch is based on the work by Alan Stern:
> 	 * http://sourceforge.net/mailarchive/forum.php?thread_id=5729457&forum_id=5398
> 	 */
>-	int j;
> 	struct usb_device_descriptor *desc;
> 	int port = -1;
> 	struct usb_device *parent = dev->parent;
>@@ -809,20 +807,22 @@ int usb_new_device(struct usb_device *de
> 
> 	desc = (struct usb_device_descriptor *)tmpbuf;
> 	dev->descriptor.bMaxPacketSize0 = 64;	    /* Start off at 64 bytes  */
>-	dev->maxpacketsize = 64;	/* Default to 64 byte max packet size */
>+	/* Default to 64 byte max packet size */
>+	dev->maxpacketsize = PACKET_SIZE_64;
> 	dev->epmaxpacketin [0] = 64;
> 	dev->epmaxpacketout[0] = 64;
>-	for (j = 0; j < 3; ++j) {
>-		err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, 64);
>-		if (err < 0) {
>-			USB_PRINTF("usb_new_device: 64 byte descr\n");
>-			break;
>-		}
>+
>+	err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, 64);
>+	if (err < 0) {
>+		USB_PRINTF("usb_new_device: usb_get_descriptor() failed\n");
>+		return 1;
> 	}
>+
> 	dev->descriptor.bMaxPacketSize0 = desc->bMaxPacketSize0;
> 
> 	/* find the port number we're at */
> 	if (parent) {
>+		int j;
> 
> 		for (j = 0; j < parent->maxchild; j++) {
> 			if (parent->children[j] == dev) {
>@@ -847,10 +847,10 @@ int usb_new_device(struct usb_device *de
> 	dev->epmaxpacketin [0] = dev->descriptor.bMaxPacketSize0;
> 	dev->epmaxpacketout[0] = dev->descriptor.bMaxPacketSize0;
> 	switch (dev->descriptor.bMaxPacketSize0) {
>-	case 8: dev->maxpacketsize = 0; break;
>-	case 16: dev->maxpacketsize = 1; break;
>-	case 32: dev->maxpacketsize = 2; break;
>-	case 64: dev->maxpacketsize = 3; break;
>+	case 8: dev->maxpacketsize  = PACKET_SIZE_8; break;
>+	case 16: dev->maxpacketsize = PACKET_SIZE_16; break;
>+	case 32: dev->maxpacketsize = PACKET_SIZE_32; break;
>+	case 64: dev->maxpacketsize = PACKET_SIZE_64; break;
> 	}
> 	dev->devnum = addr;
> 
>Index: u-boot-git-22092008/include/usb.h
>===================================================================
>--- u-boot-git-22092008.orig/include/usb.h	2008-10-10 09:42:59.000000000 +0200
>+++ u-boot-git-22092008/include/usb.h	2008-10-10 09:43:01.000000000 +0200
>@@ -129,6 +129,13 @@ struct usb_config_descriptor {
> 	struct usb_interface_descriptor if_desc[USB_MAXINTERFACES];
> } __attribute__ ((packed));
> 
>+enum {
>+	/* Maximum packet size; encoded as 0,1,2,3 = 8,16,32,64 */
>+	PACKET_SIZE_8   = 0,
>+	PACKET_SIZE_16  = 1,
>+	PACKET_SIZE_32  = 2,
>+	PACKET_SIZE_64  = 3,
>+};
> 
> struct usb_device {
> 	int devnum;			/* Device number on USB bus */
>@@ -137,9 +144,12 @@ struct usb_device {
> 	char prod[32];			/* product */
> 	char serial[32];		/* serial number */
> 
>-	int maxpacketsize;		/* Maximum packet size; encoded as 0,1,2,3 = 8,16,32,64 */
>-	unsigned int toggle[2];		/* one bit for each endpoint ([0] = IN, [1] = OUT) */
>-	unsigned int halted[2];		/* endpoint halts; one bit per endpoint # & direction; */
>+	/* Maximum packet size; one of: PACKET_SIZE_* */
>+	int maxpacketsize;
>+	/* one bit for each endpoint ([0] = IN, [1] = OUT) */
>+	unsigned int toggle[2];
>+	/* endpoint halts; one bit per endpoint # & direction; */
>+	unsigned int halted[2];
> 			    /* [0] = IN, [1] = OUT */
> 	int epmaxpacketin[16];		/* INput endpoint specific maximums */
> 	int epmaxpacketout[16];		/* OUTput endpoint specific maximums */
>Index: u-boot-git-22092008/drivers/usb/usb_ohci.c
>===================================================================
>--- u-boot-git-22092008.orig/drivers/usb/usb_ohci.c	2008-10-10 09:42:59.000000000 +0200
>+++ u-boot-git-22092008/drivers/usb/usb_ohci.c	2008-10-10 09:43:01.000000000 +0200
>@@ -856,8 +856,7 @@ static void td_fill(ohci_t *ohci, unsign
> 	td->index = index;
> 	td->data = (__u32)data;
> #ifdef OHCI_FILL_TRACE
>-	if ((usb_pipetype(urb_priv->pipe) == PIPE_BULK) &&
>-						usb_pipeout(urb_priv->pipe)) {
>+	if (usb_pipebulk(urb_priv->pipe) && usb_pipeout(urb_priv->pipe)) {
> 		for (i = 0; i < len; i++)
> 		printf("td->data[%d] %#2x ", i, ((unsigned char *)td->data)[i]);
> 		printf("\n");
>@@ -983,7 +982,7 @@ static void dl_transfer_length(td_t *td)
> 	tdBE   = m32_swap(td->hwBE);
> 	tdCBP  = m32_swap(td->hwCBP);
> 
>-	if (!(usb_pipetype(lurb_priv->pipe) == PIPE_CONTROL &&
>+	if (!(usb_pipecontrol(lurb_priv->pipe) &&
> 	    ((td->index == 0) || (td->index == lurb_priv->length - 1)))) {
> 		if (tdBE != 0) {
> 			if (td->hwCBP == 0)
>@@ -1096,8 +1095,7 @@ static int takeback_td(ohci_t *ohci, td_
> 	dbg("dl_done_list: processing TD %x, len %x\n",
> 		lurb_priv->td_cnt, lurb_priv->length);
> 
>-	if (ed->state != ED_NEW &&
>-		(usb_pipetype(lurb_priv->pipe) != PIPE_INTERRUPT)) {
>+	if (ed->state != ED_NEW && (!usb_pipeint(lurb_priv->pipe))) {
> 		edHeadP = m32_swap(ed->hwHeadP) & 0xfffffff0;
> 		edTailP = m32_swap(ed->hwTailP);
> 
>@@ -1287,7 +1285,7 @@ pkt_print(NULL, dev, pipe, buffer, trans
> #else
> 	wait_ms(1);
> #endif
>-	if ((pipe & PIPE_INTERRUPT) == PIPE_INTERRUPT) {
>+	if (usb_pipeint(pipe)) {
> 		info("Root-Hub submit IRQ: NOT implemented");
> 		return 0;
> 	}
>@@ -1538,7 +1536,7 @@ int submit_common_msg(struct usb_device 
> 
> 	/* allow more time for a BULK device to react - some are slow */
> #define BULK_TO	 5000	/* timeout in milliseconds */
>-	if (usb_pipetype(pipe) == PIPE_BULK)
>+	if (usb_pipebulk(pipe))
> 		timeout = BULK_TO;
> 	else
> 		timeout = 100;
>@@ -1591,7 +1589,7 @@ int submit_common_msg(struct usb_device 
> #endif
> 
> 	/* free TDs in urb_priv */
>-	if (usb_pipetype(pipe) != PIPE_INTERRUPT)
>+	if (!usb_pipeint(pipe))
> 		urb_free_priv(urb);
> 	return 0;
> }
>
>  
>