* [v2] usb: gadget: musb: fix short isoc packets with inventra dma
@ 2019-01-30 14:49 Greg Kroah-Hartman
0 siblings, 0 replies; 7+ messages in thread
From: Greg Kroah-Hartman @ 2019-01-30 14:49 UTC (permalink / raw)
To: Bin Liu; +Cc: linux-usb
On Wed, Jan 30, 2019 at 08:13:21AM -0600, Bin Liu wrote:
> From: Paul Elder <paul.elder@ideasonboard.com>
>
> 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 were set). 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, set MUSB_TXCSR_TXPKTRDY
> in dma_controller_irq (musbhsdma.c), just like host mode transfers.
>
> 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
>
> Fixes: 550a7375fe72 ("USB: Add MUSB and TUSB support")
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Bin Liu <b-liu@ti.com>
> Cc: stable <stable@vger.kernel.org>
> ---
> v2: add Fixes tag.
I added a stable tag so that you get notified by my scripts if/when it
fails to apply. Otherwise Sasha's scripts are going to try to evaluate
it and determine if this needs to be backported or not, and we already
know that it does need to be, so let's not waste their time.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 7+ messages in thread
* [v2] usb: gadget: musb: fix short isoc packets with inventra dma
@ 2019-01-30 15:15 Bin Liu
0 siblings, 0 replies; 7+ messages in thread
From: Bin Liu @ 2019-01-30 15:15 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-usb
On Wed, Jan 30, 2019 at 03:49:26PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Jan 30, 2019 at 08:13:21AM -0600, Bin Liu wrote:
> > From: Paul Elder <paul.elder@ideasonboard.com>
> >
> > 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 were set). 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, set MUSB_TXCSR_TXPKTRDY
> > in dma_controller_irq (musbhsdma.c), just like host mode transfers.
> >
> > 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
> >
> > Fixes: 550a7375fe72 ("USB: Add MUSB and TUSB support")
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Signed-off-by: Bin Liu <b-liu@ti.com>
> > Cc: stable <stable@vger.kernel.org>
> > ---
> > v2: add Fixes tag.
>
> I added a stable tag so that you get notified by my scripts if/when it
> fails to apply. Otherwise Sasha's scripts are going to try to evaluate
> it and determine if this needs to be backported or not, and we already
> know that it does need to be, so let's not waste their time.
Sounds good. Thanks.
I track those musb patches which require manual backport - it doesn't
happen very often. But since You and Sasha already have an automation to
take care of the tasks, I from now on will add 'cc: stable' whenever
applicable and let the scripts to alert me when any action is needed.
Regards,
-Bin.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [v2] usb: gadget: musb: fix short isoc packets with inventra dma
@ 2019-01-30 14:13 Bin Liu
0 siblings, 0 replies; 7+ messages in thread
From: Bin Liu @ 2019-01-30 14:13 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-usb, Bin Liu
From: Paul Elder <paul.elder@ideasonboard.com>
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 were set). 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, set MUSB_TXCSR_TXPKTRDY
in dma_controller_irq (musbhsdma.c), just like host mode transfers.
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
Fixes: 550a7375fe72 ("USB: Add MUSB and TUSB support")
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Bin Liu <b-liu@ti.com>
---
v2: add Fixes tag.
drivers/usb/musb/musb_gadget.c | 13 +------------
drivers/usb/musb/musbhsdma.c | 21 +++++++++++----------
2 files changed, 12 insertions(+), 22 deletions(-)
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index eae8b1b1b45b..ffe462a657b1 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -452,13 +452,10 @@ void musb_g_tx(struct musb *musb, u8 epnum)
}
if (request) {
- u8 is_dma = 0;
- bool short_packet = false;
trace_musb_req_tx(req);
if (dma && (csr & MUSB_TXCSR_DMAENAB)) {
- is_dma = 1;
csr |= MUSB_TXCSR_P_WZC_BITS;
csr &= ~(MUSB_TXCSR_DMAENAB | MUSB_TXCSR_P_UNDERRUN |
MUSB_TXCSR_TXPKTRDY | MUSB_TXCSR_AUTOSET);
@@ -476,16 +473,8 @@ void musb_g_tx(struct musb *musb, u8 epnum)
*/
if ((request->zero && request->length)
&& (request->length % musb_ep->packet_sz == 0)
- && (request->actual == request->length))
- short_packet = true;
+ && (request->actual == request->length)) {
- 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 (short_packet) {
/*
* On DMA completion, FIFO may not be
* available yet...
diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
index a688f7f87829..5fc6825745f2 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);
}
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [v2] usb: gadget: musb: fix short isoc packets with inventra dma
@ 2019-01-15 16:35 Bin Liu
0 siblings, 0 replies; 7+ messages in thread
From: Bin Liu @ 2019-01-15 16:35 UTC (permalink / raw)
To: Paul Elder
Cc: laurent.pinchart, kieran.bingham, drinkcat, balbi, gregkh,
linux-usb, linux-kernel
Hi Paul,
On Fri, Jan 11, 2019 at 12:31:59AM -0500, Paul Elder wrote:
> Hi Bin,
>
> On Wed, Jan 09, 2019 at 09:02:15AM -0600, Bin Liu wrote:
> > Hi Paul,
> >
> > On Wed, Jan 09, 2019 at 02:10:09AM -0500, 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 were set). 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, set MUSB_TXCSR_TXPKTRDY
> > > in dma_controller_irq (musbhsdma.c), just like host mode transfers.
> > >
> > > 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
> > >
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > ---
> > > Changes in v2:
> > >
> > > - no more flushing FIFO
> > > - greatly simplified short packet if guard in musb_g_tx, and removed
> > > unnecessary variables
> > > - minor indentation and wording changes
> > >
> > > drivers/usb/musb/musb_gadget.c | 19 +++++--------------
> > > drivers/usb/musb/musbhsdma.c | 21 +++++++++++----------
> > > 2 files changed, 16 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> > > index eae8b1b1b45b..496643f54faa 100644
> > > --- a/drivers/usb/musb/musb_gadget.c
> > > +++ b/drivers/usb/musb/musb_gadget.c
> > > @@ -452,13 +452,10 @@ void musb_g_tx(struct musb *musb, u8 epnum)
> > > }
> > >
> > > if (request) {
> > > - u8 is_dma = 0;
> > > - bool short_packet = false;
> > >
> > > trace_musb_req_tx(req);
> > >
> > > if (dma && (csr & MUSB_TXCSR_DMAENAB)) {
> > > - is_dma = 1;
> > > csr |= MUSB_TXCSR_P_WZC_BITS;
> > > csr &= ~(MUSB_TXCSR_DMAENAB | MUSB_TXCSR_P_UNDERRUN |
> > > MUSB_TXCSR_TXPKTRDY | MUSB_TXCSR_AUTOSET);
> > > @@ -476,16 +473,8 @@ void musb_g_tx(struct musb *musb, u8 epnum)
> > > */
> > > if ((request->zero && request->length)
> > > && (request->length % musb_ep->packet_sz == 0)
> > > - && (request->actual == request->length))
> > > - short_packet = true;
> > > + && (request->actual == request->length)) {
> > >
> > > - 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 (short_packet) {
> > > /*
> > > * On DMA completion, FIFO may not be
> > > * available yet...
> > > @@ -493,8 +482,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 short 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);
> >
> > Sorry I didn't catch this in the last review, but this change seems not
> > required, isn't it? In the first version of the patch, the code is
> > 'returned' in the 'if (musb_dma_inventra())' branch above, doesn't reach
> > here.
>
> Do you mean change compared to the last version of the patch, or this
> last chunk of the diff?
I meant that the chunk above seems not related to this webcam short
packet test case, so am wondering if this change is not neccessary
- musb_writew(epio, MUSB_TXCSR, MUSB_TXCSR_MODE
- | MUSB_TXCSR_TXPKTRDY);
+ musb_writew(epio, MUSB_TXCSR, csr | MUSB_TXCSR_TXPKTRDY);
>
> I guess I did also remove the return when I removed the 'if
> (musb_dma_inventra())' block that had the FLUSHFIFIO, but when I tested
> it it still worked. In fact, I reverted this last diff chunk and it
> still worked.
so the last chunk is not related, we don't have to touch it.
Regards,
-Bin.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [v2] usb: gadget: musb: fix short isoc packets with inventra dma
@ 2019-01-11 5:31 Paul Elder
0 siblings, 0 replies; 7+ messages in thread
From: Paul Elder @ 2019-01-11 5:31 UTC (permalink / raw)
To: Bin Liu, laurent.pinchart, kieran.bingham, drinkcat, balbi,
gregkh, linux-usb, linux-kernel
Hi Bin,
On Wed, Jan 09, 2019 at 09:02:15AM -0600, Bin Liu wrote:
> Hi Paul,
>
> On Wed, Jan 09, 2019 at 02:10:09AM -0500, 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 were set). 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, set MUSB_TXCSR_TXPKTRDY
> > in dma_controller_irq (musbhsdma.c), just like host mode transfers.
> >
> > 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
> >
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> > Changes in v2:
> >
> > - no more flushing FIFO
> > - greatly simplified short packet if guard in musb_g_tx, and removed
> > unnecessary variables
> > - minor indentation and wording changes
> >
> > drivers/usb/musb/musb_gadget.c | 19 +++++--------------
> > drivers/usb/musb/musbhsdma.c | 21 +++++++++++----------
> > 2 files changed, 16 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> > index eae8b1b1b45b..496643f54faa 100644
> > --- a/drivers/usb/musb/musb_gadget.c
> > +++ b/drivers/usb/musb/musb_gadget.c
> > @@ -452,13 +452,10 @@ void musb_g_tx(struct musb *musb, u8 epnum)
> > }
> >
> > if (request) {
> > - u8 is_dma = 0;
> > - bool short_packet = false;
> >
> > trace_musb_req_tx(req);
> >
> > if (dma && (csr & MUSB_TXCSR_DMAENAB)) {
> > - is_dma = 1;
> > csr |= MUSB_TXCSR_P_WZC_BITS;
> > csr &= ~(MUSB_TXCSR_DMAENAB | MUSB_TXCSR_P_UNDERRUN |
> > MUSB_TXCSR_TXPKTRDY | MUSB_TXCSR_AUTOSET);
> > @@ -476,16 +473,8 @@ void musb_g_tx(struct musb *musb, u8 epnum)
> > */
> > if ((request->zero && request->length)
> > && (request->length % musb_ep->packet_sz == 0)
> > - && (request->actual == request->length))
> > - short_packet = true;
> > + && (request->actual == request->length)) {
> >
> > - 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 (short_packet) {
> > /*
> > * On DMA completion, FIFO may not be
> > * available yet...
> > @@ -493,8 +482,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 short 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);
>
> Sorry I didn't catch this in the last review, but this change seems not
> required, isn't it? In the first version of the patch, the code is
> 'returned' in the 'if (musb_dma_inventra())' branch above, doesn't reach
> here.
Do you mean change compared to the last version of the patch, or this
last chunk of the diff?
I guess I did also remove the return when I removed the 'if
(musb_dma_inventra())' block that had the FLUSHFIFIO, but when I tested
it it still worked. In fact, I reverted this last diff chunk and it
still worked.
Paul
^ permalink raw reply [flat|nested] 7+ messages in thread
* [v2] usb: gadget: musb: fix short isoc packets with inventra dma
@ 2019-01-09 15:02 Bin Liu
0 siblings, 0 replies; 7+ messages in thread
From: Bin Liu @ 2019-01-09 15:02 UTC (permalink / raw)
To: Paul Elder
Cc: laurent.pinchart, kieran.bingham, drinkcat, balbi, gregkh,
linux-usb, linux-kernel
Hi Paul,
On Wed, Jan 09, 2019 at 02:10:09AM -0500, 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 were set). 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, set MUSB_TXCSR_TXPKTRDY
> in dma_controller_irq (musbhsdma.c), just like host mode transfers.
>
> 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
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
> Changes in v2:
>
> - no more flushing FIFO
> - greatly simplified short packet if guard in musb_g_tx, and removed
> unnecessary variables
> - minor indentation and wording changes
>
> drivers/usb/musb/musb_gadget.c | 19 +++++--------------
> drivers/usb/musb/musbhsdma.c | 21 +++++++++++----------
> 2 files changed, 16 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> index eae8b1b1b45b..496643f54faa 100644
> --- a/drivers/usb/musb/musb_gadget.c
> +++ b/drivers/usb/musb/musb_gadget.c
> @@ -452,13 +452,10 @@ void musb_g_tx(struct musb *musb, u8 epnum)
> }
>
> if (request) {
> - u8 is_dma = 0;
> - bool short_packet = false;
>
> trace_musb_req_tx(req);
>
> if (dma && (csr & MUSB_TXCSR_DMAENAB)) {
> - is_dma = 1;
> csr |= MUSB_TXCSR_P_WZC_BITS;
> csr &= ~(MUSB_TXCSR_DMAENAB | MUSB_TXCSR_P_UNDERRUN |
> MUSB_TXCSR_TXPKTRDY | MUSB_TXCSR_AUTOSET);
> @@ -476,16 +473,8 @@ void musb_g_tx(struct musb *musb, u8 epnum)
> */
> if ((request->zero && request->length)
> && (request->length % musb_ep->packet_sz == 0)
> - && (request->actual == request->length))
> - short_packet = true;
> + && (request->actual == request->length)) {
>
> - 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 (short_packet) {
> /*
> * On DMA completion, FIFO may not be
> * available yet...
> @@ -493,8 +482,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 short 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);
Sorry I didn't catch this in the last review, but this change seems not
required, isn't it? In the first version of the patch, the code is
'returned' in the 'if (musb_dma_inventra())' branch above, doesn't reach
here.
Regards,
-Bin.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [v2] usb: gadget: musb: fix short isoc packets with inventra dma
@ 2019-01-09 7:10 Paul Elder
0 siblings, 0 replies; 7+ messages in thread
From: Paul Elder @ 2019-01-09 7:10 UTC (permalink / raw)
To: laurent.pinchart, kieran.bingham
Cc: Paul Elder, drinkcat, b-liu, balbi, gregkh, linux-usb, linux-kernel
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 were set). 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, set MUSB_TXCSR_TXPKTRDY
in dma_controller_irq (musbhsdma.c), just like host mode transfers.
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
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
Changes in v2:
- no more flushing FIFO
- greatly simplified short packet if guard in musb_g_tx, and removed
unnecessary variables
- minor indentation and wording changes
drivers/usb/musb/musb_gadget.c | 19 +++++--------------
drivers/usb/musb/musbhsdma.c | 21 +++++++++++----------
2 files changed, 16 insertions(+), 24 deletions(-)
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index eae8b1b1b45b..496643f54faa 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -452,13 +452,10 @@ void musb_g_tx(struct musb *musb, u8 epnum)
}
if (request) {
- u8 is_dma = 0;
- bool short_packet = false;
trace_musb_req_tx(req);
if (dma && (csr & MUSB_TXCSR_DMAENAB)) {
- is_dma = 1;
csr |= MUSB_TXCSR_P_WZC_BITS;
csr &= ~(MUSB_TXCSR_DMAENAB | MUSB_TXCSR_P_UNDERRUN |
MUSB_TXCSR_TXPKTRDY | MUSB_TXCSR_AUTOSET);
@@ -476,16 +473,8 @@ void musb_g_tx(struct musb *musb, u8 epnum)
*/
if ((request->zero && request->length)
&& (request->length % musb_ep->packet_sz == 0)
- && (request->actual == request->length))
- short_packet = true;
+ && (request->actual == request->length)) {
- 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 (short_packet) {
/*
* On DMA completion, FIFO may not be
* available yet...
@@ -493,8 +482,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 short 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..5fc6825745f2 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);
}
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-01-30 15:15 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-30 14:49 [v2] usb: gadget: musb: fix short isoc packets with inventra dma Greg Kroah-Hartman
-- strict thread matches above, loose matches on Subject: below --
2019-01-30 15:15 Bin Liu
2019-01-30 14:13 Bin Liu
2019-01-15 16:35 Bin Liu
2019-01-11 5:31 Paul Elder
2019-01-09 15:02 Bin Liu
2019-01-09 7:10 Paul Elder
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.