| Message ID | 1311138718-5072-1-git-send-email-vikram.pandita@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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. -- 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
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. > 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. -- 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
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 ? >> 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. -- 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
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
Hi, On Wed, Jul 20, 2011 at 09:48:53AM -0700, Pandita, Vikram wrote: > > 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 !! I'm thinking on allowing this patch to go in if nobody has really strong arguments against it. The real fix, though, would need a bigger re-write of the endpoint IRQ handling and that would need more time to write and stabilize. Together with the huge amount of HW issues MUSB has, it's quite a task (been there, done that).
On Tue, Jul 26, 2011 at 8:36 PM, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Wed, Jul 20, 2011 at 09:48:53AM -0700, Pandita, Vikram wrote: >> > 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 !! > > I'm thinking on allowing this patch to go in if nobody has really strong > arguments against it. The real fix, though, would need a bigger re-write > of the endpoint IRQ handling and that would need more time to write and > stabilize. Together with the huge amount of HW issues MUSB has, it's > quite a task (been there, done that). Please note that I never objected to the _code_. I just think if the _comments_ could be made UMS agnostic... for ex if it works for other protocols just fine(quite possible) in future the reader might wonder what is UMS specific about the code snippet. Please feel free to go ahead and apply the patch. thnx -- 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
Hi, On Wed, Jul 27, 2011 at 09:20:02AM +0530, Jassi Brar wrote: > On Tue, Jul 26, 2011 at 8:36 PM, Felipe Balbi <balbi@ti.com> wrote: > > Hi, > > > > On Wed, Jul 20, 2011 at 09:48:53AM -0700, Pandita, Vikram wrote: > >> > 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 !! > > > > I'm thinking on allowing this patch to go in if nobody has really strong > > arguments against it. The real fix, though, would need a bigger re-write > > of the endpoint IRQ handling and that would need more time to write and > > stabilize. Together with the huge amount of HW issues MUSB has, it's > > quite a task (been there, done that). > > Please note that I never objected to the _code_. > > I just think if the _comments_ could be made UMS agnostic... for ex if it works > for other protocols just fine(quite possible) in future the reader > might wonder what > is UMS specific about the code snippet. for sure, makes sense to me. Vikram, do you want to update the comment to remove UMS mentions ? Most likely UASP will have the same deal, btw.
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c index 6aeb363..4a1432e 100644 --- a/drivers/usb/musb/musb_gadget.c +++ b/drivers/usb/musb/musb_gadget.c @@ -634,6 +634,7 @@ static void rxstate(struct musb *musb, struct musb_request *req) u16 len; u16 csr = musb_readw(epio, MUSB_RXCSR); struct musb_hw_ep *hw_ep = &musb->endpoints[epnum]; + u8 use_mode_1; if (hw_ep->is_shared_fifo) musb_ep = &hw_ep->ep_in; @@ -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; + if (request->actual < request->length) { #ifdef CONFIG_USB_INVENTRA_DMA if (is_buffer_mapped(req)) { @@ -714,37 +727,40 @@ static void rxstate(struct musb *musb, struct musb_request *req) * then becomes usable as a runtime "use mode 1" hint... */ - csr |= MUSB_RXCSR_DMAENAB; -#ifdef USE_MODE1 - csr |= MUSB_RXCSR_AUTOCLEAR; - /* csr |= MUSB_RXCSR_DMAMODE; */ - - /* this special sequence (enabling and then - * disabling MUSB_RXCSR_DMAMODE) is required - * to get DMAReq to activate - */ - musb_writew(epio, MUSB_RXCSR, - csr | MUSB_RXCSR_DMAMODE); -#else - if (!musb_ep->hb_mult && - musb_ep->hw_ep->rx_double_buffered) + /* Experimental: Mode1 works with mass storage use cases */ + if (use_mode_1) { csr |= MUSB_RXCSR_AUTOCLEAR; -#endif - musb_writew(epio, MUSB_RXCSR, csr); + musb_writew(epio, MUSB_RXCSR, csr); + csr |= MUSB_RXCSR_DMAENAB; + musb_writew(epio, MUSB_RXCSR, csr); + + /* this special sequence (enabling and then + * disabling MUSB_RXCSR_DMAMODE) is required + * to get DMAReq to activate + */ + musb_writew(epio, MUSB_RXCSR, + csr | MUSB_RXCSR_DMAMODE); + musb_writew(epio, MUSB_RXCSR, csr); + + } else { + if (!musb_ep->hb_mult && + musb_ep->hw_ep->rx_double_buffered) + csr |= MUSB_RXCSR_AUTOCLEAR; + csr |= MUSB_RXCSR_DMAENAB; + musb_writew(epio, MUSB_RXCSR, csr); + } if (request->actual < request->length) { int transfer_size = 0; -#ifdef USE_MODE1 - transfer_size = min(request->length - request->actual, - channel->max_len); -#else - transfer_size = min(request->length - request->actual, - (unsigned)len); -#endif - if (transfer_size <= musb_ep->packet_sz) - musb_ep->dma->desired_mode = 0; - else + if (use_mode_1) { + transfer_size = min(request->length - request->actual, + channel->max_len); musb_ep->dma->desired_mode = 1; + } else { + transfer_size = min(request->length - request->actual, + (unsigned)len); + musb_ep->dma->desired_mode = 0; + } use_dma = c->channel_program( channel,