[U-Boot] usb_stor_BBB_transport 5 ms delay - performance

[Marek Vasut <marex@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 <marex@denx.de> 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.
> > [...]