Re: [PATCH v4] usb: musb: Enable DMA mode1 RX for USB-Mass-Storage

["Pandita, Vikram" <vikram.pandita@ti.com>]

On Tue, Jul 19, 2011 at 10:53 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> On Wed, Jul 20, 2011 at 11:15 AM, Pandita, Vikram <vikram.pandita@ti.com> wrote:
> > On Tue, Jul 19, 2011 at 10:34 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >>
> >> On Wed, Jul 20, 2011 at 10:41 AM, Vikram Pandita <vikram.pandita@ti.com> wrote:
> >> > From: Anand Gadiyar <gadiyar@ti.com>
> >> >
> >> > This patch enables the DMA mode1 RX support.
> >> > This feature is enabled based on the short_not_ok flag passed from
> >> > gadget drivers.
> >> >
> >> > This will result in a thruput performance gain of around
> >> > 40% for USB mass-storage/mtp use cases.
> >> >
> >> > Signed-off-by: Anand Gadiyar <gadiyar@ti.com>
> >> > Signed-off-by: Moiz Sonasath <m-sonasath@ti.com>
> >> > Signed-off-by: Vikram Pandita <vikram.pandita@ti.com>
> >> > Tested-by: Vikram Pandita <vikram.pandita@ti.com>
> >> > ---
> >> > v1 - initial push
> >> > v2 - fixed whitespace issues as per Sergei Shtylyov<sshtylyov@mvista.com> comments
> >> > v3 - restor authorship to Anand Gadiyar <gadiyar@ti.com>
> >> > v4 - adding my signed-off as per Kevin Hilman <khilman@ti.com>
> >> >
> >> >  drivers/usb/musb/musb_gadget.c |   68 ++++++++++++++++++++++++---------------
> >> >  1 files changed, 42 insertions(+), 26 deletions(-)
> >>
> >> > @@ -683,6 +684,18 @@ static void rxstate(struct musb *musb, struct musb_request *req)
> >> >
> >> >        if (csr & MUSB_RXCSR_RXPKTRDY) {
> >> >                len = musb_readw(epio, MUSB_RXCOUNT);
> >> > +
> >> > +               /*
> >> > +                * Enable Mode 1 for RX transfers only for mass-storage
> >> > +                * use-case, based on short_not_ok flag which is set only
> >> > +                * from file_storage and f_mass_storage drivers
> >> > +                */
> >> > +
> >> > +               if (request->short_not_ok && len == musb_ep->packet_sz)
> >> > +                       use_mode_1 = 1;
> >> > +               else
> >> > +                       use_mode_1 = 0;
> >> > +
> >> There is nothing UMS class specific in this patch...
> >> (request->short_not_ok && len == musb_ep->packet_sz) may not be the
> >> signature of, and only of, Mass Storage Functions. So maybe removing
> >> the UMS mention from
> >> comment and change-log is a good idea.
> >
> > Have you grepped the code in drivers/usb/gadget/*.*
> > only UMS sets this flag today and hence the use of this flag.
> >
> > As i understand, on UMS, CSW/data/CBW  - the data part size is a known
> > size and to be safe that mode=1 dma is not stuck up,
> > the mode is enabled only for the gadget driver that sets short_not_ok
> > flag - and that today happens to be only UMS.
>
> This *today happens to be only UMS* is my exact point here.
> Can you guarantee no other function driver will ever expect only
> full packet xfers and treat short as errors ?
>

We are trying to test if short_not_ok may not be needed at all. But
all gadgets need to be tested on MUSB for that.
We will need wider help from MUSB maintainer/author(anand g) to
determine if removing short_not_ok is fine on MUSB for _all_ gadgets.

To be safe we only enable for UMS use case today that is definitely
working fine.

Time for Maintainer/author to pitch in !!

>
> >> You might want to add is-ep-type-bulk-out check to the condition
> >> though, so that it doesn't affect
> >> cases that you didn't verify.
> >
> > TX path (IN host), already uses the mode=1 DMA and hence the comment
> > is not valid.
> > This patch just also enables mode=1 on RX path.
> Well, then no need for the ep-type check.

where did u see ep-type check? i can see only packet size check. Did i miss?
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html