| Message ID | 20181009063220.13745-1-paul.elder@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: gadget: musb: fix short isoc packets with inventra dma for pandaboard es | expand |
On Tue, Oct 09, 2018 at 02:32:20AM -0400, Paul Elder wrote: > Handling short packets (length < max packet size) in the Inventra DMA > engine in the MUSB driver causes the MUSB DMA controller to hang. An > example of a problem that is caused by this problem is when streaming > video out of a UVC gadget, only the first video frame is transferred. > > For short packets (mode-0 or mode-1 DMA), MUSB_TXCSR_TXPKTRDY must be > set manually by the driver. This was previously done in musb_g_tx > (musb_gadget.c), but incorrectly (all csr flags were cleared, and only > MUSB_TXCSR_MODE and MUSB_TXCSR_TXPKTRDY). Fixing that problem allows > some requests to be transferred correctly, but multiple requests were > often put together in one USB packet, and caused problems if the packet > size was not a multiple of 4. > > Instead, MUSB_TXCSR_TXPKTRDY is set in dma_controller_irq (musbhsdma.c), > just like host mode transfers, then musb_g_tx forces the packet to be > flushed, by setting MUSB_TXCSR_FLUSHFIFO. > > This topic was originally tackled by Nicolas Boichat [0] [1] and is discussed > further at [2] as part of his GSoC project [3]. > > [0] https://groups.google.com/forum/?hl=en#!topic/beagleboard-gsoc/k8Azwfp75CU > [1] https://gitorious.org/beagleboard-usbsniffer/beagleboard-usbsniffer-kernel/commit/b0be3b6cc195ba732189b04f1d43ec843c3e54c9?p=beagleboard-usbsniffer:beagleboard-usbsniffer-kernel.git;a=patch;h=b0be3b6cc195ba732189b04f1d43ec843c3e54c9 > [2] http://beagleboard-usbsniffer.blogspot.com/2010/07/musb-isochronous-transfers-fixed.html > [3] http://elinux.org/BeagleBoard/GSoC/USBSniffer > > I have forward-ported this patch from 2.6.34 to 4.19-rc1. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> Hello all, ping I was wondering if any of you had a chance to take a look at this yet. Thanks, Paul > --- > drivers/usb/musb/musb_gadget.c | 21 ++++++++++++++------- > drivers/usb/musb/musbhsdma.c | 21 +++++++++++---------- > 2 files changed, 25 insertions(+), 17 deletions(-) > > diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c > index eae8b1b1b45b..d3f33f449445 100644 > --- a/drivers/usb/musb/musb_gadget.c > +++ b/drivers/usb/musb/musb_gadget.c > @@ -479,11 +479,16 @@ void musb_g_tx(struct musb *musb, u8 epnum) > && (request->actual == request->length)) > short_packet = true; > > - if ((musb_dma_inventra(musb) || musb_dma_ux500(musb)) && > - (is_dma && (!dma->desired_mode || > - (request->actual & > - (musb_ep->packet_sz - 1))))) > - short_packet = true; > + if (is_dma && (musb_dma_inventra(musb) || musb_dma_ux500(musb))) { > + if (!dma->desired_mode || > + request->actual % musb_ep->packet_sz) { > + musb_dbg(musb, "%s Flushing (FIFO) EP : KPB\n", > + musb_ep->end_point.name); > + musb_writew(epio, MUSB_TXCSR, > + csr | MUSB_TXCSR_FLUSHFIFO); > + return; > + } > + } > > if (short_packet) { > /* > @@ -493,8 +498,10 @@ void musb_g_tx(struct musb *musb, u8 epnum) > if (csr & MUSB_TXCSR_TXPKTRDY) > return; > > - musb_writew(epio, MUSB_TXCSR, MUSB_TXCSR_MODE > - | MUSB_TXCSR_TXPKTRDY); > + musb_dbg(musb, "sending zero pkt (zero=%d, length=%d, actual=%d, dma->desired_mode=%d)\n", > + request->zero, request->length, request->actual, > + dma->desired_mode); > + musb_writew(epio, MUSB_TXCSR, csr | MUSB_TXCSR_TXPKTRDY); > request->zero = 0; > } > > diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c > index a688f7f87829..e514d4700a6b 100644 > --- a/drivers/usb/musb/musbhsdma.c > +++ b/drivers/usb/musb/musbhsdma.c > @@ -346,12 +346,10 @@ static irqreturn_t dma_controller_irq(int irq, void *private_data) > channel->status = MUSB_DMA_STATUS_FREE; > > /* completed */ > - if ((devctl & MUSB_DEVCTL_HM) > - && (musb_channel->transmit) > - && ((channel->desired_mode == 0) > - || (channel->actual_len & > - (musb_channel->max_packet_sz - 1))) > - ) { > + if (musb_channel->transmit && > + (!channel->desired_mode || > + (channel->actual_len % > + musb_channel->max_packet_sz))) { > u8 epnum = musb_channel->epnum; > int offset = musb->io.ep_offset(epnum, > MUSB_TXCSR); > @@ -363,11 +361,14 @@ static irqreturn_t dma_controller_irq(int irq, void *private_data) > */ > musb_ep_select(mbase, epnum); > txcsr = musb_readw(mbase, offset); > - txcsr &= ~(MUSB_TXCSR_DMAENAB > + if (channel->desired_mode == 1) { > + txcsr &= ~(MUSB_TXCSR_DMAENAB > | MUSB_TXCSR_AUTOSET); > - musb_writew(mbase, offset, txcsr); > - /* Send out the packet */ > - txcsr &= ~MUSB_TXCSR_DMAMODE; > + musb_writew(mbase, offset, txcsr); > + /* Send out the packet */ > + txcsr &= ~MUSB_TXCSR_DMAMODE; > + txcsr |= MUSB_TXCSR_DMAENAB; > + } > txcsr |= MUSB_TXCSR_TXPKTRDY; > musb_writew(mbase, offset, txcsr); > } > -- > 2.18.0 >
Hi Paul, Sorry for the delay on reviewing it. For the subject, can you please use usb: musb: gadget: fix short isoc packets with inventra dma On Tue, Oct 09, 2018 at 02:32:20AM -0400, Paul Elder wrote: > Handling short packets (length < max packet size) in the Inventra DMA > engine in the MUSB driver causes the MUSB DMA controller to hang. An > example of a problem that is caused by this problem is when streaming > video out of a UVC gadget, only the first video frame is transferred. > > For short packets (mode-0 or mode-1 DMA), MUSB_TXCSR_TXPKTRDY must be > set manually by the driver. This was previously done in musb_g_tx > (musb_gadget.c), but incorrectly (all csr flags were cleared, and only > MUSB_TXCSR_MODE and MUSB_TXCSR_TXPKTRDY). Fixing that problem allows > some requests to be transferred correctly, but multiple requests were > often put together in one USB packet, and caused problems if the packet > size was not a multiple of 4. > > Instead, MUSB_TXCSR_TXPKTRDY is set in dma_controller_irq (musbhsdma.c), > just like host mode transfers, then musb_g_tx forces the packet to be > flushed, by setting MUSB_TXCSR_FLUSHFIFO. > > This topic was originally tackled by Nicolas Boichat [0] [1] and is discussed this line is longer than 75 chars. > further at [2] as part of his GSoC project [3]. > > [0] https://groups.google.com/forum/?hl=en#!topic/beagleboard-gsoc/k8Azwfp75CU > [1] https://gitorious.org/beagleboard-usbsniffer/beagleboard-usbsniffer-kernel/commit/b0be3b6cc195ba732189b04f1d43ec843c3e54c9?p=beagleboard-usbsniffer:beagleboard-usbsniffer-kernel.git;a=patch;h=b0be3b6cc195ba732189b04f1d43ec843c3e54c9 > [2] http://beagleboard-usbsniffer.blogspot.com/2010/07/musb-isochronous-transfers-fixed.html > [3] http://elinux.org/BeagleBoard/GSoC/USBSniffer > > I have forward-ported this patch from 2.6.34 to 4.19-rc1. this line is irrelevant to the commit message, please move it down to under '---'. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > drivers/usb/musb/musb_gadget.c | 21 ++++++++++++++------- > drivers/usb/musb/musbhsdma.c | 21 +++++++++++---------- > 2 files changed, 25 insertions(+), 17 deletions(-) > > diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c > index eae8b1b1b45b..d3f33f449445 100644 > --- a/drivers/usb/musb/musb_gadget.c > +++ b/drivers/usb/musb/musb_gadget.c > @@ -479,11 +479,16 @@ void musb_g_tx(struct musb *musb, u8 epnum) > && (request->actual == request->length)) > short_packet = true; > > - if ((musb_dma_inventra(musb) || musb_dma_ux500(musb)) && > - (is_dma && (!dma->desired_mode || > - (request->actual & > - (musb_ep->packet_sz - 1))))) > - short_packet = true; > + if (is_dma && (musb_dma_inventra(musb) || musb_dma_ux500(musb))) { more than 80 chars. > + if (!dma->desired_mode || I understand you forward-port Nicolas' patch, but do you have a specific readon to re-write this 'if' condition? I'd like to see minimum code change in bug fixes, > + request->actual % musb_ep->packet_sz) { but I like this version though, easier to read. > + musb_dbg(musb, "%s Flushing (FIFO) EP : KPB\n", what is 'KPB'? the message could be more meaningful? > + musb_ep->end_point.name); > + musb_writew(epio, MUSB_TXCSR, > + csr | MUSB_TXCSR_FLUSHFIFO); What if without this line? The short packet doesn't send out? setting TXSCR_TXPKTRDY in the dma driver is not sufficient? TXSCR_FLUSHFIFO is only used for aborting cases. > + return; > + } > + } > > if (short_packet) { > /* > @@ -493,8 +498,10 @@ void musb_g_tx(struct musb *musb, u8 epnum) > if (csr & MUSB_TXCSR_TXPKTRDY) > return; > > - musb_writew(epio, MUSB_TXCSR, MUSB_TXCSR_MODE > - | MUSB_TXCSR_TXPKTRDY); > + musb_dbg(musb, "sending zero pkt (zero=%d, length=%d, actual=%d, dma->desired_mode=%d)\n", > + request->zero, request->length, request->actual, more than 80 chars. > + dma->desired_mode); > + musb_writew(epio, MUSB_TXCSR, csr | MUSB_TXCSR_TXPKTRDY); more than 80 chars. > request->zero = 0; > } > > diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c > index a688f7f87829..e514d4700a6b 100644 > --- a/drivers/usb/musb/musbhsdma.c > +++ b/drivers/usb/musb/musbhsdma.c > @@ -346,12 +346,10 @@ static irqreturn_t dma_controller_irq(int irq, void *private_data) > channel->status = MUSB_DMA_STATUS_FREE; > > /* completed */ > - if ((devctl & MUSB_DEVCTL_HM) > - && (musb_channel->transmit) > - && ((channel->desired_mode == 0) > - || (channel->actual_len & > - (musb_channel->max_packet_sz - 1))) > - ) { > + if (musb_channel->transmit && > + (!channel->desired_mode || > + (channel->actual_len % > + musb_channel->max_packet_sz))) { improper indentation. > u8 epnum = musb_channel->epnum; > int offset = musb->io.ep_offset(epnum, > MUSB_TXCSR); > @@ -363,11 +361,14 @@ static irqreturn_t dma_controller_irq(int irq, void *private_data) > */ > musb_ep_select(mbase, epnum); > txcsr = musb_readw(mbase, offset); > - txcsr &= ~(MUSB_TXCSR_DMAENAB > + if (channel->desired_mode == 1) { > + txcsr &= ~(MUSB_TXCSR_DMAENAB > | MUSB_TXCSR_AUTOSET); > - musb_writew(mbase, offset, txcsr); > - /* Send out the packet */ > - txcsr &= ~MUSB_TXCSR_DMAMODE; > + musb_writew(mbase, offset, txcsr); more than 80 chars. > + /* Send out the packet */ > + txcsr &= ~MUSB_TXCSR_DMAMODE; > + txcsr |= MUSB_TXCSR_DMAENAB; > + } > txcsr |= MUSB_TXCSR_TXPKTRDY; > musb_writew(mbase, offset, txcsr); > } Regards, -Bin.
Hi Bin, On Mon, Jan 07, 2019 at 01:11:57PM -0600, Bin Liu wrote: > Hi Paul, > > Sorry for the delay on reviewing it. Thanks for the review. > For the subject, can you please use > > usb: musb: gadget: fix short isoc packets with inventra dma ack > On Tue, Oct 09, 2018 at 02:32:20AM -0400, Paul Elder wrote: > > Handling short packets (length < max packet size) in the Inventra DMA > > engine in the MUSB driver causes the MUSB DMA controller to hang. An > > example of a problem that is caused by this problem is when streaming > > video out of a UVC gadget, only the first video frame is transferred. > > > > For short packets (mode-0 or mode-1 DMA), MUSB_TXCSR_TXPKTRDY must be > > set manually by the driver. This was previously done in musb_g_tx > > (musb_gadget.c), but incorrectly (all csr flags were cleared, and only > > MUSB_TXCSR_MODE and MUSB_TXCSR_TXPKTRDY). Fixing that problem allows > > some requests to be transferred correctly, but multiple requests were > > often put together in one USB packet, and caused problems if the packet > > size was not a multiple of 4. > > > > Instead, MUSB_TXCSR_TXPKTRDY is set in dma_controller_irq (musbhsdma.c), > > just like host mode transfers, then musb_g_tx forces the packet to be > > flushed, by setting MUSB_TXCSR_FLUSHFIFO. > > > > This topic was originally tackled by Nicolas Boichat [0] [1] and is discussed > > this line is longer than 75 chars. ack > > further at [2] as part of his GSoC project [3]. > > > > [0] https://groups.google.com/forum/?hl=en#!topic/beagleboard-gsoc/k8Azwfp75CU > > [1] https://gitorious.org/beagleboard-usbsniffer/beagleboard-usbsniffer-kernel/commit/b0be3b6cc195ba732189b04f1d43ec843c3e54c9?p=beagleboard-usbsniffer:beagleboard-usbsniffer-kernel.git;a=patch;h=b0be3b6cc195ba732189b04f1d43ec843c3e54c9 > > [2] http://beagleboard-usbsniffer.blogspot.com/2010/07/musb-isochronous-transfers-fixed.html > > [3] http://elinux.org/BeagleBoard/GSoC/USBSniffer > > > > I have forward-ported this patch from 2.6.34 to 4.19-rc1. > > this line is irrelevant to the commit message, please move it down to > under '---'. ack > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > drivers/usb/musb/musb_gadget.c | 21 ++++++++++++++------- > > drivers/usb/musb/musbhsdma.c | 21 +++++++++++---------- > > 2 files changed, 25 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c > > index eae8b1b1b45b..d3f33f449445 100644 > > --- a/drivers/usb/musb/musb_gadget.c > > +++ b/drivers/usb/musb/musb_gadget.c > > @@ -479,11 +479,16 @@ void musb_g_tx(struct musb *musb, u8 epnum) > > && (request->actual == request->length)) > > short_packet = true; > > > > - if ((musb_dma_inventra(musb) || musb_dma_ux500(musb)) && > > - (is_dma && (!dma->desired_mode || > > - (request->actual & > > - (musb_ep->packet_sz - 1))))) > > - short_packet = true; > > + if (is_dma && (musb_dma_inventra(musb) || musb_dma_ux500(musb))) { > > more than 80 chars. > > > + if (!dma->desired_mode || > > I understand you forward-port Nicolas' patch, but do you have a specific > readon to re-write this 'if' condition? I'd like to see minimum code > change in bug fixes, > > > + request->actual % musb_ep->packet_sz) { > > but I like this version though, easier to read. > > > + musb_dbg(musb, "%s Flushing (FIFO) EP : KPB\n", > > what is 'KPB'? the message could be more meaningful? > > > + musb_ep->end_point.name); > > + musb_writew(epio, MUSB_TXCSR, > > + csr | MUSB_TXCSR_FLUSHFIFO); > > What if without this line? The short packet doesn't send out? setting > TXSCR_TXPKTRDY in the dma driver is not sufficient? TXSCR_FLUSHFIFO is > only used for aborting cases. I just tested this and you are right, it does work without this line. Since this is the only significant line in this very complex if block, I'll just remove this entire block too. > > + return; > > + } > > + } > > > > if (short_packet) { > > /* > > @@ -493,8 +498,10 @@ void musb_g_tx(struct musb *musb, u8 epnum) > > if (csr & MUSB_TXCSR_TXPKTRDY) > > return; > > > > - musb_writew(epio, MUSB_TXCSR, MUSB_TXCSR_MODE > > - | MUSB_TXCSR_TXPKTRDY); > > + musb_dbg(musb, "sending zero pkt (zero=%d, length=%d, actual=%d, dma->desired_mode=%d)\n", > > + request->zero, request->length, request->actual, > > more than 80 chars. The format string or the parameters? > > > + dma->desired_mode); > > + musb_writew(epio, MUSB_TXCSR, csr | MUSB_TXCSR_TXPKTRDY); > > more than 80 chars. It's only by 1 character. Should I still put the last argument on a new line? > > > request->zero = 0; > > } > > > > diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c > > index a688f7f87829..e514d4700a6b 100644 > > --- a/drivers/usb/musb/musbhsdma.c > > +++ b/drivers/usb/musb/musbhsdma.c > > @@ -346,12 +346,10 @@ static irqreturn_t dma_controller_irq(int irq, void *private_data) > > channel->status = MUSB_DMA_STATUS_FREE; > > > > /* completed */ > > - if ((devctl & MUSB_DEVCTL_HM) > > - && (musb_channel->transmit) > > - && ((channel->desired_mode == 0) > > - || (channel->actual_len & > > - (musb_channel->max_packet_sz - 1))) > > - ) { > > + if (musb_channel->transmit && > > + (!channel->desired_mode || > > + (channel->actual_len % > > + musb_channel->max_packet_sz))) { > > improper indentation. ack. You don't think this is more readable than if everything was lined up? > > u8 epnum = musb_channel->epnum; > > int offset = musb->io.ep_offset(epnum, > > MUSB_TXCSR); > > @@ -363,11 +361,14 @@ static irqreturn_t dma_controller_irq(int irq, void *private_data) > > */ > > musb_ep_select(mbase, epnum); > > txcsr = musb_readw(mbase, offset); > > - txcsr &= ~(MUSB_TXCSR_DMAENAB > > + if (channel->desired_mode == 1) { > > + txcsr &= ~(MUSB_TXCSR_DMAENAB > > | MUSB_TXCSR_AUTOSET); > > - musb_writew(mbase, offset, txcsr); > > - /* Send out the packet */ > > - txcsr &= ~MUSB_TXCSR_DMAMODE; > > + musb_writew(mbase, offset, txcsr); > > more than 80 chars. This is also only by 2 characters; do you still want just the last argument on a new line? > > + /* Send out the packet */ > > + txcsr &= ~MUSB_TXCSR_DMAMODE; > > + txcsr |= MUSB_TXCSR_DMAENAB; > > + } > > txcsr |= MUSB_TXCSR_TXPKTRDY; > > musb_writew(mbase, offset, txcsr); > > } Thanks, Paul
On Mon, Jan 07, 2019 at 11:45:24PM -0500, Paul Elder wrote: > Hi Bin, > > On Mon, Jan 07, 2019 at 01:11:57PM -0600, Bin Liu wrote: > > Hi Paul, > > > > Sorry for the delay on reviewing it. > > Thanks for the review. > > > For the subject, can you please use > > > > usb: musb: gadget: fix short isoc packets with inventra dma > > ack > > > On Tue, Oct 09, 2018 at 02:32:20AM -0400, Paul Elder wrote: > > > Handling short packets (length < max packet size) in the Inventra DMA > > > engine in the MUSB driver causes the MUSB DMA controller to hang. An > > > example of a problem that is caused by this problem is when streaming > > > video out of a UVC gadget, only the first video frame is transferred. > > > > > > For short packets (mode-0 or mode-1 DMA), MUSB_TXCSR_TXPKTRDY must be > > > set manually by the driver. This was previously done in musb_g_tx > > > (musb_gadget.c), but incorrectly (all csr flags were cleared, and only > > > MUSB_TXCSR_MODE and MUSB_TXCSR_TXPKTRDY). Fixing that problem allows > > > some requests to be transferred correctly, but multiple requests were > > > often put together in one USB packet, and caused problems if the packet > > > size was not a multiple of 4. > > > > > > Instead, MUSB_TXCSR_TXPKTRDY is set in dma_controller_irq (musbhsdma.c), > > > just like host mode transfers, then musb_g_tx forces the packet to be > > > flushed, by setting MUSB_TXCSR_FLUSHFIFO. This should be reworded, since we don't set MUSB_TXCSR_FLUSHFIFO in the code. > > > > > > This topic was originally tackled by Nicolas Boichat [0] [1] and is discussed > > > > this line is longer than 75 chars. > > ack > > > > further at [2] as part of his GSoC project [3]. > > > > > > [0] https://groups.google.com/forum/?hl=en#!topic/beagleboard-gsoc/k8Azwfp75CU > > > [1] https://gitorious.org/beagleboard-usbsniffer/beagleboard-usbsniffer-kernel/commit/b0be3b6cc195ba732189b04f1d43ec843c3e54c9?p=beagleboard-usbsniffer:beagleboard-usbsniffer-kernel.git;a=patch;h=b0be3b6cc195ba732189b04f1d43ec843c3e54c9 > > > [2] http://beagleboard-usbsniffer.blogspot.com/2010/07/musb-isochronous-transfers-fixed.html > > > [3] http://elinux.org/BeagleBoard/GSoC/USBSniffer > > > > > > I have forward-ported this patch from 2.6.34 to 4.19-rc1. > > > > this line is irrelevant to the commit message, please move it down to > > under '---'. > > ack > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > --- > > > drivers/usb/musb/musb_gadget.c | 21 ++++++++++++++------- > > > drivers/usb/musb/musbhsdma.c | 21 +++++++++++---------- > > > 2 files changed, 25 insertions(+), 17 deletions(-) > > > > > > diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c > > > index eae8b1b1b45b..d3f33f449445 100644 > > > --- a/drivers/usb/musb/musb_gadget.c > > > +++ b/drivers/usb/musb/musb_gadget.c > > > @@ -479,11 +479,16 @@ void musb_g_tx(struct musb *musb, u8 epnum) > > > && (request->actual == request->length)) > > > short_packet = true; > > > > > > - if ((musb_dma_inventra(musb) || musb_dma_ux500(musb)) && > > > - (is_dma && (!dma->desired_mode || > > > - (request->actual & > > > - (musb_ep->packet_sz - 1))))) > > > - short_packet = true; > > > + if (is_dma && (musb_dma_inventra(musb) || musb_dma_ux500(musb))) { > > > > more than 80 chars. > > > > > + if (!dma->desired_mode || > > > > I understand you forward-port Nicolas' patch, but do you have a specific > > readon to re-write this 'if' condition? I'd like to see minimum code > > change in bug fixes, > > > > > + request->actual % musb_ep->packet_sz) { > > > > but I like this version though, easier to read. > > > > > + musb_dbg(musb, "%s Flushing (FIFO) EP : KPB\n", > > > > what is 'KPB'? the message could be more meaningful? > > > > > + musb_ep->end_point.name); > > > + musb_writew(epio, MUSB_TXCSR, > > > + csr | MUSB_TXCSR_FLUSHFIFO); > > > > What if without this line? The short packet doesn't send out? setting > > TXSCR_TXPKTRDY in the dma driver is not sufficient? TXSCR_FLUSHFIFO is > > only used for aborting cases. > > I just tested this and you are right, it does work without this line. > Since this is the only significant line in this very complex if block, > I'll just remove this entire block too. > > > > + return; > > > + } > > > + } > > > > > > if (short_packet) { > > > /* > > > @@ -493,8 +498,10 @@ void musb_g_tx(struct musb *musb, u8 epnum) > > > if (csr & MUSB_TXCSR_TXPKTRDY) > > > return; > > > > > > - musb_writew(epio, MUSB_TXCSR, MUSB_TXCSR_MODE > > > - | MUSB_TXCSR_TXPKTRDY); > > > + musb_dbg(musb, "sending zero pkt (zero=%d, length=%d, actual=%d, dma->desired_mode=%d)\n", actually this is not a 'zero' packet, it is a short packet. > > > + request->zero, request->length, request->actual, > > > > more than 80 chars. > > The format string or the parameters? Never mind. I meant the parameters, but it is only 1 char longer, just leave it like this. > > > > > > + dma->desired_mode); > > > + musb_writew(epio, MUSB_TXCSR, csr | MUSB_TXCSR_TXPKTRDY); > > > > more than 80 chars. > > It's only by 1 character. Should I still put the last argument on a new > line? same here, never mind my comment. > > > > > > request->zero = 0; > > > } > > > > > > diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c > > > index a688f7f87829..e514d4700a6b 100644 > > > --- a/drivers/usb/musb/musbhsdma.c > > > +++ b/drivers/usb/musb/musbhsdma.c > > > @@ -346,12 +346,10 @@ static irqreturn_t dma_controller_irq(int irq, void *private_data) > > > channel->status = MUSB_DMA_STATUS_FREE; > > > > > > /* completed */ > > > - if ((devctl & MUSB_DEVCTL_HM) > > > - && (musb_channel->transmit) > > > - && ((channel->desired_mode == 0) > > > - || (channel->actual_len & > > > - (musb_channel->max_packet_sz - 1))) > > > - ) { > > > + if (musb_channel->transmit && > > > + (!channel->desired_mode || > > > + (channel->actual_len % > > > + musb_channel->max_packet_sz))) { > > > > improper indentation. > > ack. You don't think this is more readable than if everything was lined > up? The original version has 4-chars indentation, I think it is clearer. > > > > u8 epnum = musb_channel->epnum; > > > int offset = musb->io.ep_offset(epnum, > > > MUSB_TXCSR); > > > @@ -363,11 +361,14 @@ static irqreturn_t dma_controller_irq(int irq, void *private_data) > > > */ > > > musb_ep_select(mbase, epnum); > > > txcsr = musb_readw(mbase, offset); > > > - txcsr &= ~(MUSB_TXCSR_DMAENAB > > > + if (channel->desired_mode == 1) { > > > + txcsr &= ~(MUSB_TXCSR_DMAENAB > > > | MUSB_TXCSR_AUTOSET); > > > - musb_writew(mbase, offset, txcsr); > > > - /* Send out the packet */ > > > - txcsr &= ~MUSB_TXCSR_DMAMODE; > > > + musb_writew(mbase, offset, txcsr); > > > > more than 80 chars. > > This is also only by 2 characters; do you still want just the last > argument on a new line? never mind my comment. Regards, -Bin.
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c index eae8b1b1b45b..d3f33f449445 100644 --- a/drivers/usb/musb/musb_gadget.c +++ b/drivers/usb/musb/musb_gadget.c @@ -479,11 +479,16 @@ void musb_g_tx(struct musb *musb, u8 epnum) && (request->actual == request->length)) short_packet = true; - if ((musb_dma_inventra(musb) || musb_dma_ux500(musb)) && - (is_dma && (!dma->desired_mode || - (request->actual & - (musb_ep->packet_sz - 1))))) - short_packet = true; + if (is_dma && (musb_dma_inventra(musb) || musb_dma_ux500(musb))) { + if (!dma->desired_mode || + request->actual % musb_ep->packet_sz) { + musb_dbg(musb, "%s Flushing (FIFO) EP : KPB\n", + musb_ep->end_point.name); + musb_writew(epio, MUSB_TXCSR, + csr | MUSB_TXCSR_FLUSHFIFO); + return; + } + } if (short_packet) { /* @@ -493,8 +498,10 @@ void musb_g_tx(struct musb *musb, u8 epnum) if (csr & MUSB_TXCSR_TXPKTRDY) return; - musb_writew(epio, MUSB_TXCSR, MUSB_TXCSR_MODE - | MUSB_TXCSR_TXPKTRDY); + musb_dbg(musb, "sending zero pkt (zero=%d, length=%d, actual=%d, dma->desired_mode=%d)\n", + request->zero, request->length, request->actual, + dma->desired_mode); + musb_writew(epio, MUSB_TXCSR, csr | MUSB_TXCSR_TXPKTRDY); request->zero = 0; } diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c index a688f7f87829..e514d4700a6b 100644 --- a/drivers/usb/musb/musbhsdma.c +++ b/drivers/usb/musb/musbhsdma.c @@ -346,12 +346,10 @@ static irqreturn_t dma_controller_irq(int irq, void *private_data) channel->status = MUSB_DMA_STATUS_FREE; /* completed */ - if ((devctl & MUSB_DEVCTL_HM) - && (musb_channel->transmit) - && ((channel->desired_mode == 0) - || (channel->actual_len & - (musb_channel->max_packet_sz - 1))) - ) { + if (musb_channel->transmit && + (!channel->desired_mode || + (channel->actual_len % + musb_channel->max_packet_sz))) { u8 epnum = musb_channel->epnum; int offset = musb->io.ep_offset(epnum, MUSB_TXCSR); @@ -363,11 +361,14 @@ static irqreturn_t dma_controller_irq(int irq, void *private_data) */ musb_ep_select(mbase, epnum); txcsr = musb_readw(mbase, offset); - txcsr &= ~(MUSB_TXCSR_DMAENAB + if (channel->desired_mode == 1) { + txcsr &= ~(MUSB_TXCSR_DMAENAB | MUSB_TXCSR_AUTOSET); - musb_writew(mbase, offset, txcsr); - /* Send out the packet */ - txcsr &= ~MUSB_TXCSR_DMAMODE; + musb_writew(mbase, offset, txcsr); + /* Send out the packet */ + txcsr &= ~MUSB_TXCSR_DMAMODE; + txcsr |= MUSB_TXCSR_DMAENAB; + } txcsr |= MUSB_TXCSR_TXPKTRDY; musb_writew(mbase, offset, txcsr); }
Handling short packets (length < max packet size) in the Inventra DMA engine in the MUSB driver causes the MUSB DMA controller to hang. An example of a problem that is caused by this problem is when streaming video out of a UVC gadget, only the first video frame is transferred. For short packets (mode-0 or mode-1 DMA), MUSB_TXCSR_TXPKTRDY must be set manually by the driver. This was previously done in musb_g_tx (musb_gadget.c), but incorrectly (all csr flags were cleared, and only MUSB_TXCSR_MODE and MUSB_TXCSR_TXPKTRDY). Fixing that problem allows some requests to be transferred correctly, but multiple requests were often put together in one USB packet, and caused problems if the packet size was not a multiple of 4. Instead, MUSB_TXCSR_TXPKTRDY is set in dma_controller_irq (musbhsdma.c), just like host mode transfers, then musb_g_tx forces the packet to be flushed, by setting MUSB_TXCSR_FLUSHFIFO. This topic was originally tackled by Nicolas Boichat [0] [1] and is discussed further at [2] as part of his GSoC project [3]. [0] https://groups.google.com/forum/?hl=en#!topic/beagleboard-gsoc/k8Azwfp75CU [1] https://gitorious.org/beagleboard-usbsniffer/beagleboard-usbsniffer-kernel/commit/b0be3b6cc195ba732189b04f1d43ec843c3e54c9?p=beagleboard-usbsniffer:beagleboard-usbsniffer-kernel.git;a=patch;h=b0be3b6cc195ba732189b04f1d43ec843c3e54c9 [2] http://beagleboard-usbsniffer.blogspot.com/2010/07/musb-isochronous-transfers-fixed.html [3] http://elinux.org/BeagleBoard/GSoC/USBSniffer I have forward-ported this patch from 2.6.34 to 4.19-rc1. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- drivers/usb/musb/musb_gadget.c | 21 ++++++++++++++------- drivers/usb/musb/musbhsdma.c | 21 +++++++++++---------- 2 files changed, 25 insertions(+), 17 deletions(-)