Hi Sergei, Thanks for your comments. On 07/20/2010 11:43 AM, Sergei Shtylyov wrote: > 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@xxxxxxxxxx> > >> 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? :-) 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
