| Message ID | 1311033103-15854-1-git-send-email-vikram.pandita@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> From: Vikram Pandita <vikram.pandita@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. > > Based on Original work by > Anand Gadiyar <gadiyar@ti.com> on 2.6.35 kernel > > Change-Id: I9b3a7cae73b63e86128d2caf4cdd67ab77556e75 > Signed-off-by: Moiz Sonasath <m-sonasath@ti.com> > Signed-off-by: Vikram Pandita <vikram.pandita@ti.com> I think the change-id should not be included in upstream submissions - it may not be useful to someone looking at the changelog. So you probably should drop it. Could you please retain my authorship and sign off from the original patch, since I did pretty much all the original work on writing this patch (and if I remember correctly several attempts to get this merged upstream)? I don't see any functional changes from my original patch. - Anand -- 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 Mon, Jul 18, 2011 at 11:22 PM, Gadiyar, Anand <gadiyar@ti.com> wrote: >> From: Vikram Pandita <vikram.pandita@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. >> >> Based on Original work by >> Anand Gadiyar <gadiyar@ti.com> on 2.6.35 kernel >> >> Change-Id: I9b3a7cae73b63e86128d2caf4cdd67ab77556e75 >> Signed-off-by: Moiz Sonasath <m-sonasath@ti.com> >> Signed-off-by: Vikram Pandita <vikram.pandita@ti.com> > > I think the change-id should not be included in upstream > submissions - it may not be useful to someone looking at > the changelog. So you probably should drop it. yes will drop that. This comes from gerrit commit hook that does not have a meaning for upstream. > > Could you please retain my authorship and sign off from the > original patch, since I did pretty much all the original work > on writing this patch That is given and clearly mentioned in the commit message. I will change the authorship with no issues, but would have been nice if you could have taken this upstream. We have been carrying this optimisation around in product kernels for a long time now and we keep redoing on each migration, with the downside of sometimes loosing the authorship. > (and if I remember correctly several > attempts to get this merged upstream)? I don't see any > functional changes from my original patch. Wonder what were the reasons for not getting accepted? Can you re-ignite the discussion why it cannot be taken in then? > > > - Anand > -- 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
Pandita, Vikram wrote: > On Mon, Jul 18, 2011 at 11:22 PM, Gadiyar, Anand <gadiyar@ti.com> wrote: > >> From: Vikram Pandita <vikram.pandita@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. > >> > >> Based on Original work by > >> Anand Gadiyar <gadiyar@ti.com> on 2.6.35 kernel > >> > >> Change-Id: I9b3a7cae73b63e86128d2caf4cdd67ab77556e75 > >> Signed-off-by: Moiz Sonasath <m-sonasath@ti.com> > >> Signed-off-by: Vikram Pandita <vikram.pandita@ti.com> > > > > I think the change-id should not be included in upstream > > submissions - it may not be useful to someone looking at > > the changelog. So you probably should drop it. > > yes will drop that. This comes from gerrit commit hook that does not > have a meaning for upstream. > > > > > Could you please retain my authorship and sign off from the > > original patch, since I did pretty much all the original work > > on writing this patch > > That is given and clearly mentioned in the commit message. > I will change the authorship with no issues, but would have been nice > if you could have taken this upstream. Yes, I ought to have followed up more. But this was at a time when we were promised a competing implementation from Nokia would be merged that would get mode1 working for all use cases. A patch series was posted to the mailing lists around Dec 2009 with promises off-list to repost with comments addressed - that has never happened so far. But you can't just change authorship when the entire functional code is the same. (It doesn't matter much to me - I'm not as active on MUSB as I used to be; it's just the principle of the thing). > We have been carrying this optimisation around in product kernels for > a long time now and we keep redoing on each migration, > with the downside of sometimes loosing the authorship. > > > (and if I remember correctly several > > attempts to get this merged upstream)? I don't see any > > functional changes from my original patch. > > Wonder what were the reasons for not getting accepted? > Can you re-ignite the discussion why it cannot be taken in then? You've already re-ignited this discussion. I haven't tested the patch with the current kernel (and will do so soon), but if it does still work and there are no objections, it ought to be merged. - Anand -- 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 12:46 AM, Gadiyar, Anand <gadiyar@ti.com> wrote: > Pandita, Vikram wrote: >> On Mon, Jul 18, 2011 at 11:22 PM, Gadiyar, Anand <gadiyar@ti.com> wrote: <snip> > But you can't just change authorship when the entire functional code > is the same. (It doesn't matter much to me - I'm not as active on > MUSB as I used to be; it's just the principle of the thing). Moiz fixed the second part of your patch - which was not there on your original work: <snip> + 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;+ 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; <snip end> The history is: Original author on .35 or .32 kernel : Anand Gadiyar Fixes done by and some more forward ports: Moiz Sonasath Tested on 3.0 and code cleanups, commit message updates, community comment fixes: Vikram Pandita Wonder if original author did not act all this while, is there anything wrong in changing authorship with proper accreditation to original author? For future pushes, i would really like to learn what the linux community suggests the right approach for such cases. As i said, i am open to change and will repost as decided - there is no attempt to sabotage anyone's work here :-) > >> We have been carrying this optimisation around in product kernels for >> a long time now and we keep redoing on each migration, >> with the downside of sometimes loosing the authorship. >> >> > (and if I remember correctly several >> > attempts to get this merged upstream)? I don't see any >> > functional changes from my original patch. >> >> Wonder what were the reasons for not getting accepted? >> Can you re-ignite the discussion why it cannot be taken in then? > > You've already re-ignited this discussion. I haven't tested the patch > with the current kernel (and will do so soon), but if it does still > work and there are no objections, it ought to be merged. > > - Anand > -- 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
Pandita, Vikram wrote: > On Tue, Jul 19, 2011 at 12:46 AM, Gadiyar, Anand <gadiyar@ti.com> wrote: > > Pandita, Vikram wrote: > >> On Mon, Jul 18, 2011 at 11:22 PM, Gadiyar, Anand <gadiyar@ti.com> wrote: > <snip> > > But you can't just change authorship when the entire functional code > > is the same. (It doesn't matter much to me - I'm not as active on > > MUSB as I used to be; it's just the principle of the thing). > > Moiz fixed the second part of your patch - which was not there on your > original work: > > <snip> ... > <snip end> > > The history is: > > Original author on .35 or .32 kernel : Anand Gadiyar > Fixes done by and some more forward ports: Moiz Sonasath > Tested on 3.0 and code cleanups, commit message updates, community > comment fixes: Vikram Pandita > > Wonder if original author did not act all this while, is there > anything wrong in changing authorship with proper accreditation to > original author? > For future pushes, i would really like to learn what the linux > community suggests the right approach for such cases. > > As i said, i am open to change and will repost as decided - there is > no attempt to sabotage anyone's work here :-) Checking the git tree history and the patch you just posted, you're right. I missed Moiz's changes. (but that same git tree shows you've got my sign-off on all the internal patches Moiz posted - and I don't remember if the original debugging was done by me or not) I'm withdrawing my objection - let's just get the patch merged. It's stayed out-of-tree for far too long. - Anand -- 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
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,