Patchwork [U-Boot] usb_stor_BBB_transport 5 ms delay - performance

login
register
mail settings
Submitter Jim Shimer
Date July 26, 2012, 8:20 p.m.
Message ID <CACjqNxtkmChjpDYkd5wHV-2d=MY1=Si2Hm_Z1x7_-t9PiOYZyA@mail.gmail.com>
Download mbox | patch
Permalink /patch/173534/
State Superseded
Delegated to: Marek Vasut
Headers show

Comments

Jim Shimer - July 26, 2012, 8:20 p.m.
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:

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

        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);
        USB_STOR_PRINTF("Read Capacity returns: 0x%lx, 0x%lx\n", cap[0],
                        cap[1]);
 #if 0


I'd appreciate any feedback.
Regards
Benoît Thébaudeau - July 27, 2012, 12:43 a.m.
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;
> 
>  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;
>  };
> 
>  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;
> +}
> +
>  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);
> 
>         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);
> 
>         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);
>         USB_STOR_PRINTF("Read Capacity returns: 0x%lx, 0x%lx\n",
>         cap[0],
>                         cap[1]);
>  #if 0
> 
> 
> I'd appreciate any feedback.
> Regards

I have not looked into this delay issue, but I had similar performance issues
that I fixed with the following series:
http://patchwork.ozlabs.org/patch/172052/
http://patchwork.ozlabs.org/patch/172204/
http://patchwork.ozlabs.org/patch/172054/
http://patchwork.ozlabs.org/patch/172055/
http://patchwork.ozlabs.org/patch/172056/

Your suggestion is interesting and might be a complement to my series. I don't
have time to check its correctness right now, but I'll try soon.

Best regards,
Benoît
Jim Shimer - July 27, 2012, 4:47 a.m.
With the code that skips the 5 msecond delay if the device is ready, my fat
load time went from 80 seconds to 8 seconds.  This is actually fairly close
to what it takes to do the same transfer in Linux (5 seconds).  So I assume
the 5 msdelay when the device is already ready is not necessary.

On Thu, Jul 26, 2012 at 8:43 PM, Benoît Thébaudeau <
benoit.thebaudeau@advansee.com> wrote:

> 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;
> >
> >  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;
> >  };
> >
> >  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;
> > +}
> > +
> >  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);
> >
> >         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);
> >
> >         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);
> >         USB_STOR_PRINTF("Read Capacity returns: 0x%lx, 0x%lx\n",
> >         cap[0],
> >                         cap[1]);
> >  #if 0
> >
> >
> > I'd appreciate any feedback.
> > Regards
>
> I have not looked into this delay issue, but I had similar performance
> issues
> that I fixed with the following series:
> http://patchwork.ozlabs.org/patch/172052/
> http://patchwork.ozlabs.org/patch/172204/
> http://patchwork.ozlabs.org/patch/172054/
> http://patchwork.ozlabs.org/patch/172055/
> http://patchwork.ozlabs.org/patch/172056/
>
> Your suggestion is interesting and might be a complement to my series. I
> don't
> have time to check its correctness right now, but I'll try soon.
>
> Best regards,
> Benoît
>
Marek Vasut - July 27, 2012, 12:59 p.m.
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;
> > 
> >  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;
> > 
> >  };
> >  
> >  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;
> > +}
> > +
> > 
> >  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);
> > 
> >         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);
> > 
> >         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);
> > 
> >         USB_STOR_PRINTF("Read Capacity returns: 0x%lx, 0x%lx\n",
> >         cap[0],
> >         
> >                         cap[1]);
> >  
> >  #if 0
> > 
> > I'd appreciate any feedback.
> > Regards
> 
> I have not looked into this delay issue, but I had similar performance
> issues that I fixed with the following series:
> http://patchwork.ozlabs.org/patch/172052/
> http://patchwork.ozlabs.org/patch/172204/
> http://patchwork.ozlabs.org/patch/172054/
> http://patchwork.ozlabs.org/patch/172055/
> http://patchwork.ozlabs.org/patch/172056/
> 
> Your suggestion is interesting and might be a complement to my series. I
> don't have time to check its correctness right now, but I'll try soon.

Will you two have time to work these into V2 of your series somehow please?

> Best regards,
> Benoît

Best regards,
Marek Vasut
Benoît Thébaudeau - July 27, 2012, 2:07 p.m.
Dear Marek,

On Fri, Jul 27, 2012 at 02:59:29 PM, Marek Vasut wrote:
> > 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;
> > > 
> > >  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;
> > > 
> > >  };
> > >  
> > >  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;
> > > +}
> > > +
> > > 
> > >  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);
> > > 
> > >         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);
> > > 
> > >         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);
> > > 
> > >         USB_STOR_PRINTF("Read Capacity returns: 0x%lx, 0x%lx\n",
> > >         cap[0],
> > >         
> > >                         cap[1]);
> > >  
> > >  #if 0
> > > 
> > > I'd appreciate any feedback.
> > > Regards
> > 
> > I have not looked into this delay issue, but I had similar
> > performance
> > issues that I fixed with the following series:
> > http://patchwork.ozlabs.org/patch/172052/
> > http://patchwork.ozlabs.org/patch/172204/
> > http://patchwork.ozlabs.org/patch/172054/
> > http://patchwork.ozlabs.org/patch/172055/
> > http://patchwork.ozlabs.org/patch/172056/
> > 
> > Your suggestion is interesting and might be a complement to my
> > series. I
> > don't have time to check its correctness right now, but I'll try
> > soon.
> 
> Will you two have time to work these into V2 of your series somehow
> please?

Are you asking me to integrate Jim's patch in my series with his SoB once
reviewed?

Since I have already issued a v2 for 2/5, do you want a v3 of the whole series
to be more clear?

Regards,
Benoît
Marek Vasut - July 27, 2012, 2:09 p.m.
Dear Benoמt Thיbaudeau,

[...]
> > > Your suggestion is interesting and might be a complement to my
> > > series. I
> > > don't have time to check its correctness right now, but I'll try
> > > soon.
> > 
> > Will you two have time to work these into V2 of your series somehow
> > please?
> 
> Are you asking me to integrate Jim's patch in my series with his SoB once
> reviewed?

If you can negotiate ...

> Since I have already issued a v2 for 2/5, do you want a v3 of the whole
> series to be more clear?

Otherwise I'm fine with picking up your series and applying Jims patch 
afterwards ... though I'm not quite sure if it'll apply then. So guys, what do 
you think?

> Regards,
> Benoמt

Best regards,
Marek Vasut
Benoît Thébaudeau - July 27, 2012, 2:17 p.m.
Dear Marek,

On Fri, Jul 27, 2012 at 04:09:42 PM, Marek Vasut wrote:
> [...]
> > > > Your suggestion is interesting and might be a complement to my
> > > > series. I
> > > > don't have time to check its correctness right now, but I'll
> > > > try
> > > > soon.
> > > 
> > > Will you two have time to work these into V2 of your series
> > > somehow
> > > please?
> > 
> > Are you asking me to integrate Jim's patch in my series with his
> > SoB once
> > reviewed?
> 
> If you can negotiate ...
> 
> > Since I have already issued a v2 for 2/5, do you want a v3 of the
> > whole
> > series to be more clear?
> 
> Otherwise I'm fine with picking up your series and applying Jims
> patch
> afterwards ... though I'm not quite sure if it'll apply then. So
> guys, what do
> you think?

I'm fine with merging Jim's patch once reviewed if he agrees.

Regards,
Benoît
Jim Shimer - July 27, 2012, 2:55 p.m.
Feel free to merge it into your work.  Thanks.

On Fri, Jul 27, 2012 at 10:17 AM, Benoît Thébaudeau <
benoit.thebaudeau@advansee.com> wrote:

> Dear Marek,
>
> On Fri, Jul 27, 2012 at 04:09:42 PM, Marek Vasut wrote:
> > [...]
> > > > > Your suggestion is interesting and might be a complement to my
> > > > > series. I
> > > > > don't have time to check its correctness right now, but I'll
> > > > > try
> > > > > soon.
> > > >
> > > > Will you two have time to work these into V2 of your series
> > > > somehow
> > > > please?
> > >
> > > Are you asking me to integrate Jim's patch in my series with his
> > > SoB once
> > > reviewed?
> >
> > If you can negotiate ...
> >
> > > Since I have already issued a v2 for 2/5, do you want a v3 of the
> > > whole
> > > series to be more clear?
> >
> > Otherwise I'm fine with picking up your series and applying Jims
> > patch
> > afterwards ... though I'm not quite sure if it'll apply then. So
> > guys, what do
> > you think?
>
> I'm fine with merging Jim's patch once reviewed if he agrees.
>
> Regards,
> Benoît
>
Marek Vasut - July 27, 2012, 3:06 p.m.
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.
[...]
Jim Shimer - July 27, 2012, 3:43 p.m.
I agree with everything, its up to you how to apply the change.

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).
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.

Thanks for working on this!

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.
> [...]
>
Marek Vasut - July 27, 2012, 5:44 p.m.
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.
> > [...]
Benoît Thébaudeau - July 29, 2012, 1:31 a.m.
Dear Marek,

On Fri, Jul 27, 2012 at 07:44:04 PM, Marek Vasut wrote:
> > 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 :-(

There is already a local ss variable defined for that purpose anyway, so
duplicating this typecast can be avoided.

> > 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!

I'll try to do that on Monday. That will make many changes to the patch, so I'll
add my SoB after Jim's, saying what I did.

Best regards,
Benoît
Marek Vasut - July 29, 2012, 1:38 a.m.
Dear Benoît Thébaudeau,

> Dear Marek,
> 
> On Fri, Jul 27, 2012 at 07:44:04 PM, Marek Vasut wrote:
> > > 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 :-(
> 
> There is already a local ss variable defined for that purpose anyway, so
> duplicating this typecast can be avoided.
> 
> > > 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!
> 
> I'll try to do that on Monday. That will make many changes to the patch, so
> I'll add my SoB after Jim's, saying what I did.

Thanks, will look forward to it.

> Best regards,
> Benoît

Best regards,
Marek Vasut

Patch

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

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

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

        USB_STOR_PRINTF("usb_read: end startblk %lx, blccnt %x buffer
%lx\n",