From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Fri, 27 Jul 2012 19:44:04 +0200 Subject: [U-Boot] usb_stor_BBB_transport 5 ms delay - performance In-Reply-To: References: <1847444104.653748.1343349820819.JavaMail.root@advansee.com> <201207271706.26994.marex@denx.de> Message-ID: <201207271944.04577.marex@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Dear Jim Shimer, > I agree with everything, its up to you how to apply the change. Heh ;-) > I did see a flags field but thought having a new one was conservative (I > had no real reason to have a new field). As for the typecasts I was > following the API which tests for device ready (Monkey See Monkey Do). Ouch, the API seems so broken then :-( > Also I have no compelling reason to need a "setter function" either. I > have no compelling feelings towards the implementation other than the 5ms > adds an unnecessary delay when the device is already known to be ready, and > this delay accumulates to a very poor performance for large files. Correct! > Thanks for working on this! No, thank you! > On Fri, Jul 27, 2012 at 11:06 AM, Marek Vasut wrote: > > Dear Beno?t Th?baudeau, > > > > > Hi Jim, > > > > > > On Thu, Jul 26, 2012 at 10:20:48 PM, Jim Shimer wrote: > > > > I'm seeing a 5ms delay in usb_stor_BBB_transport, which occurs every > > > > 10K of > > > > data for fatload usb or 500ms of delay per 1MB of image size. This > > > > adds up > > > > to quite a bit of delay if you're loading a large ramdisk. > > > > > > > > Does anyone know what the reason for the 5ms delay really is? I'm > > > > assuming > > > > that this delay is to debounce the 5V/100ma USB power up. I made > > > > some > > > > modification, where the delay is skipped if the device has already > > > > been > > > > queried as ready. This has save me 500ms/M on fatload times (eg, > > > > 140M=70seconds). Is there anything wrong with this tweak? > > > > > > > > Here's a diff of what I've done to get the performance I need: > > > > > > > > --- usb_storage.c.orig 2012-07-26 16:06:40.775251000 -0400 > > > > +++ usb_storage.c 2012-07-26 13:49:36.000000000 -0400 > > > > @@ -132,6 +132,7 @@ static block_dev_desc_t usb_dev_desc[USB > > > > > > > > struct us_data; > > > > typedef int (*trans_cmnd)(ccb *cb, struct us_data *data); > > > > typedef int (*trans_reset)(struct us_data *data); > > > > > > > > +typedef enum us_status { USB_NOT_READY, USB_READY} us_status; > > > > Can we possibly avoid the typedef? > > > > > > struct us_data { > > > > > > > > struct usb_device *pusb_dev; /* this usb_device */ > > > > > > > > @@ -154,6 +155,7 @@ struct us_data { > > > > > > > > ccb *srb; /* current srb */ > > > > trans_reset transport_reset; /* reset routine */ > > > > trans_cmnd transport; /* transport routine > > > > */ > > > > > > > > + us_status status; > > > > Don't we have some flags for it already? > > > > > > }; > > > > > > > > static struct us_data usb_stor[USB_MAX_STOR_DEV]; > > > > > > > > @@ -691,7 +693,10 @@ int usb_stor_BBB_transport(ccb *srb, str > > > > > > > > usb_stor_BBB_reset(us); > > > > return USB_STOR_TRANSPORT_FAILED; > > > > > > > > } > > > > > > > > - wait_ms(5); > > > > + if(us->status != USB_READY) > > > > + { > > > > + wait_ms(5); > > > > + } > > > > > > > > pipein = usb_rcvbulkpipe(us->pusb_dev, us->ep_in); > > > > pipeout = usb_sndbulkpipe(us->pusb_dev, us->ep_out); > > > > /* DATA phase + error handling */ > > > > > > > > @@ -957,7 +962,10 @@ static int usb_test_unit_ready(ccb *srb, > > > > > > > > srb->datalen = 0; > > > > srb->cmdlen = 12; > > > > if (ss->transport(srb, ss) == > > > > USB_STOR_TRANSPORT_GOOD) > > > > > > > > + { > > > > + ss->status = USB_READY; > > > > > > > > return 0; > > > > > > > > + } > > > > > > > > usb_request_sense(srb, ss); > > > > wait_ms(100); > > > > > > > > } while (retries--); > > > > > > > > @@ -965,6 +973,11 @@ static int usb_test_unit_ready(ccb *srb, > > > > > > > > return -1; > > > > > > > > } > > > > > > > > +static void usb_set_unit_not_ready(struct us_data *ss) > > > > +{ > > > > + ss->status = USB_NOT_READY; > > > > +} > > > > + > > > > We don't need a setter function really. > > > > > > static int usb_read_capacity(ccb *srb, struct us_data *ss) > > > > { > > > > > > > > int retry; > > > > > > > > @@ -1108,6 +1121,7 @@ retry_it: > > > > blks -= smallblks; > > > > buf_addr += srb->datalen; > > > > > > > > } while (blks != 0); > > > > > > > > + usb_set_unit_not_ready((struct us_data *)dev->privptr); > > > > I think we should be much more careful about these typecasts. > > > > > > USB_STOR_PRINTF("usb_read: end startblk %lx, blccnt %x buffer > > > > > > > > %lx\n", > > > > > > > > start, smallblks, buf_addr); > > > > > > > > @@ -1188,6 +1202,7 @@ retry_it: > > > > blks -= smallblks; > > > > buf_addr += srb->datalen; > > > > > > > > } while (blks != 0); > > > > > > > > + usb_set_unit_not_ready((struct us_data *)dev->privptr); > > > > Same here. > > > > > > USB_STOR_PRINTF("usb_write: end startblk %lx, blccnt %x > > > > buffer > > > > > > > > %lx\n", > > > > > > > > start, smallblks, buf_addr); > > > > > > > > @@ -1398,6 +1413,7 @@ int usb_stor_get_info(struct usb_device > > > > > > > > cap[0] = 2880; > > > > cap[1] = 0x200; > > > > > > > > } > > > > > > > > + usb_set_unit_not_ready((struct us_data *)dev->privptr); > > > > The rest is cool. > > [...]