[PATCH] Fix MUSB short isochronous packets, with Inventra DMA

(sorry for those of you who are receiving this message twice, I got the usb mailing list wrong)
Hi all,

See the patch below. This works on the BeagleBoard C4 (TI OMAP3530), but
it would be nice if people can test it with some other hardware using
the MUSB block.

Thank you,

Best regards,

Nicolas

(sorry for the spam, this one _should_ be good)
Hi all,

See the patch below. This works on the BeagleBoard C4 (TI OMAP3530),
but it would be nice if people can test it with some other hardware
using the MUSB block.

Thank you,

Best regards,

Nicolas

Hi Sergei,

Thanks for your comments.

Hello.

Nicolas Boichat wrote:

(sorry for the spam, this one _should_ be good)
Hi all,

See the patch below. This works on the BeagleBoard C4 (TI OMAP3530),
but it would be nice if people can test it with some other hardware
using the MUSB block.

Thank you,

Best regards,

Nicolas

   The above test should be under --- tearline to simplify the work for
the maintainer (everything under --- will be dropped automatically when
applying the patch).

Noted.

===

MUSB - Fix iso short packets when using Inventra DMA

   There's not need to duplicate subject.

From: Nicolas Boichat <nicolas@boichat.ch>

This patch fixes isochronous short packets (i.e., length < maximum packet
size) handling in the MUSB driver, when the Inventra DMA engine is
active.

It is based on this tree:
http://arago-project.org/git/people/sriram/ti-psp-omap.git?p=people/sriram/ti-psp-omap.git;a=shortlog;h=refs/heads/OMAPPSP_03.00.02.07

but seems to apply cleanly against a 2.6.35-rc5 kernel (untested though).

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

   What about them? :slight_smile:

They were set ,-)

). Fixing that problem allows some requests to be
transfered correctly, but multiple requests were often put together in
one
USB packet, which I believe should not be done, 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.

A more complete description is available at
http://beagleboard-usbsniffer.blogspot.com/2010/07/musb-isochronous-transfers-fixed.html

and the gadgetfs driver used to reproduce the problem is available at
http://elinux.org/BeagleBoard/GSoC/USBSniffer#MUSB_testing_code .

[...]

diff --git a/drivers/usb/musb/musb_gadget.c
b/drivers/usb/musb/musb_gadget.c
index 82bcb0d..eea05e5 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -495,18 +495,23 @@ void musb_g_tx(struct musb *musb, u8 epnum)
         }

         if (is_dma || request->actual == request->length) {
+#ifdef CONFIG_USB_INVENTRA_DMA
+ if (is_dma && (!dma->desired_mode ||
+ ((request->actual %
+ musb_ep->packet_sz) != 0))) {

   Parens not necessary either around % or !=...

Ok.

+ DBG(4, "Flushing FIFO...\n");
+ musb_writew(epio, MUSB_TXCSR,
+ csr | MUSB_TXCSR_FLUSHFIFO);

   Shouldn't you write to TXCSR twice to flush the double-buffered FIFO.

Good question... I do not use double buffering, so I haven't tested that.

Maybe the debugging message is unclear, what I want is to flush the
current packet, not necessarily the whole FIFO (so I think that even
with double-buffering, writing FLUSHFIFO once should be enough...).

I will test that.

diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
index 22a2978..a07e6a3 100644
--- a/drivers/usb/musb/musbhsdma.c
+++ b/drivers/usb/musb/musbhsdma.c
@@ -530,12 +530,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) &&

   Parns not needed around 'musb_channel->transmit'.

+ ((channel->desired_mode == 0) ||

   And neither around ==...

Well, these were already there ,-) But since I'm modifying these
line, ok.

+ ((channel->actual_len %
+ musb_channel->max_packet_sz) != 0))) {

   And neither around % or !=...

@@ -547,11 +545,16 @@ 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);
+ musb_writew(mbase, offset, txcsr);
+ txcsr &= ~MUSB_TXCSR_DMAMODE;
+ txcsr |= MUSB_TXCSR_DMAENAB;

   Why set DMAReqEnab once again?

The problem is that if MUSB_TXCSR_DMAENAB is not set, then this test in
musb_g_tx (musb_gadget.c:481):
if (dma && (csr & MUSB_TXCSR_DMAENAB)) {
is false, and the packet is not processed properly...
But I'm not sure of the side effects of setting MUSB_TXCSR_DMAENAB, so
if you have a better suggestion... Maybe clear DMAMODE in musb_g_tx only?

Thanks,

Best regards,

Nicolas