[U-Boot] [patch 0/2] Some more USB-OHCI bugfixes [V2]

[U-Boot] [patch 0/2] Some more USB-OHCI bugfixes [V2]

[Remy Bohmer]

This is version 2 of these series!
Only patch 1 of 2 has changed, some pedantic Coding Style issues updated that
were not detected by Linux/scripts/checkpatch.pl.

The USB OHCI init procedure sets the maximum message length the wrong way.
A max of 64 bits should not be done by writing '64' in maxpacketsize, but '3'.
While fixing this problem it turned out that there is more wrong here in this
code it turned out that the wrong bits were checked to determine if the pipe
was of type PIPE_INTERRUPT. This series fixes those errors also.

I made it 2 seperate patches. The 1st of this series is fully tested and correct
on at least the AT91SAM9261 cores.

The 2nd patch, however, is created by search-for-the-same-errors-and-replace.
I am not able to test that patch, I do not have the boards, so that needs to be
done by others or by review. It is clear that code there is buggy in the
first place.

These patches require my previous series called
'Improve stability USB memory sticks for the common OHCI USB layer.' to be
applied before this series. So they should apply on the u-boot-usb git tree.
--> git://git.denx.de/u-boot-usb.git

I want to mention also, that everytime I look deeper into this code, I find new
bugs. It appears that several parts of this code is written with interrupt
handling in mind, while we have no interrupt handling at all.
Assumptions are done that a interrupt handler does things asynchronous, causing
long loops that have no real use at all...
So, no guarantees that _all_ problems are solved by now...

-- 

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

[Remy Bohmer]

An embedded and charset-unspecified text was scrubbed...
Name: outgoing/fix-usb-init-procedure.patch
Url: http://lists.denx.de/pipermail/u-boot/attachments/20081010/3e0641b5/attachment.txt 

[U-Boot] [patch 2/2] The PIPE_INTERRUPT flag is used wrong

[Remy Bohmer]

An embedded and charset-unspecified text was scrubbed...
Name: outgoing/usb-fix-PIPE_flag-usage.patch
Url: http://lists.denx.de/pipermail/u-boot/attachments/20081010/7b7f79c1/attachment.txt 

[U-Boot] [patch 0/2] Some more USB-OHCI bugfixes [V2]

[Markus Klotzbücher]

Dear Remy,

On Fri, Oct 10, 2008 at 10:23:20AM +0200, Remy Bohmer wrote:
> This is version 2 of these series!
> Only patch 1 of 2 has changed, some pedantic Coding Style issues updated that
> were not detected by Linux/scripts/checkpatch.pl.
> 
> The USB OHCI init procedure sets the maximum message length the wrong way.
> A max of 64 bits should not be done by writing '64' in maxpacketsize, but '3'.
> While fixing this problem it turned out that there is more wrong here in this
> code it turned out that the wrong bits were checked to determine if the pipe
> was of type PIPE_INTERRUPT. This series fixes those errors also.
> 
> I made it 2 seperate patches. The 1st of this series is fully tested and correct
> on at least the AT91SAM9261 cores.
> 
> The 2nd patch, however, is created by search-for-the-same-errors-and-replace.
> I am not able to test that patch, I do not have the boards, so that needs to be
> done by others or by review. It is clear that code there is buggy in the
> first place.
> 
> These patches require my previous series called
> 'Improve stability USB memory sticks for the common OHCI USB layer.' to be
> applied before this series. So they should apply on the u-boot-usb git tree.
> --> git://git.denx.de/u-boot-usb.git
> 
> I want to mention also, that everytime I look deeper into this code, I find new
> bugs. It appears that several parts of this code is written with interrupt
> handling in mind, while we have no interrupt handling at all.
> Assumptions are done that a interrupt handler does things asynchronous, causing
> long loops that have no real use at all...
> So, no guarantees that _all_ problems are solved by now...

Both patches passed my usual sequoia regression test, so I added both
to the USB custodian repository.

If nobody reports any regressions I intend to send Wolfgang a pull
request in upcoming merge window.

Thanks!

Best regards
Markus


-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de")

[U-Boot] [patch 0/2] Some more USB-OHCI bugfixes [V2]

[Wolfgang Denk]

Dear Markus,

In message <20081014151340.GA26504@lisa> you wrote:
> 
> Both patches passed my usual sequoia regression test, so I added both
> to the USB custodian repository.
> 
> If nobody reports any regressions I intend to send Wolfgang a pull
> request in upcoming merge window.

What's the state about this pull request?  The merge window will be
closing soon!

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"No problem is so formidable that you can't walk away from it."
- C. Schulz

[U-Boot] [patch 0/2] Some more USB-OHCI bugfixes [V2]

[Markus Klotzbücher]

On Wed, Oct 29, 2008 at 12:07:38AM +0100, Wolfgang Denk wrote:
> Dear Markus,
> 
> In message <20081014151340.GA26504@lisa> you wrote:
> > 
> > Both patches passed my usual sequoia regression test, so I added both
> > to the USB custodian repository.
> > 
> > If nobody reports any regressions I intend to send Wolfgang a pull
> > request in upcoming merge window.
> 
> What's the state about this pull request?  The merge window will be
> closing soon!

They're in already. You pulled them with my last pull request.

Best regards
Markus

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de")

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

[Daniel Hellstrom]

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

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

[Remy Bohmer]

Hi,

2010/1/29 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.

Could you make a patch?

Kind regards,

Remy

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

[Daniel Hellstrom]

Hi,

Remy Bohmer wrote:

>Hi,
>
>2010/1/29 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.
>>    
>>
>
>Could you make a patch?
>
>  
>
I supplied a patch on the list, it is also available in the git sparc 
repository in the usb branch.
http://git.denx.de/?p=u-boot/u-boot-sparc.git;a=shortlog;h=refs/heads/usb

Thanks,
Daniel