> -----Original Message----- > From: linux-usb-owner@xxxxxxxxxxxxxxx [mailto:linux-usb-owner@xxxxxxxxxxxxxxx] On Behalf Of Sergei > Shtylyov > Sent: Saturday, February 21, 2009 2:05 AM > To: felipe.balbi@xxxxxxxxx; gregkh@xxxxxxx; david-b@xxxxxxxxxxx > Cc: linux-usb@xxxxxxxxxxxxxxx; davinci-linux-open-source@xxxxxxxxxxxxxxxxxxxx > Subject: [PATCH 2/2] musb_host: fix isochronous Tx DMA > > Multi-frame isochronous Tx URBs transfers in DMA mode never complete because > musb_host_tx() just doesn't restart DMA on a second frame, only emitting debug > message instead. In order to amend that, we need to factor out programming the > DMA transfer from musb_ep_program() into musb_tx_dma_program() (dropping bogus > REVISIT comment on the CPPI/TUSB DMA failure path), reorder the code at the end > of musb_host_tx() to facilitate the fallback to PIO iff DMA fails (dropping the > useless label, while at it), and add a variable to handle the buffer offset > consistently for both PIO and DMA modes. We also need to add an argument to > musb_ep_program() for the same reason (it only worked correctly with non-zero > offset of the first frame in PIO mode). Also, set the completed isochronous > frame descriptor's 'actual_length' and 'status' fields correctly in DMA mode > (in fact, the latter didn't get set at all). > > Oh, and also CPPI reportedly doesn't like sending isochronous packets in the > RNDIS mode, so change the criterion for this mode to be used only on multi- > packet sends (in the single-packet case using it didn't make sense anyway)... > > Signed-off-by: Pavel Kiryukhin <pkiryukhin@xxxxxxxxxxxxx> > Signed-off-by: Sergei Shtylyov <sshtylyov@xxxxxxxxxxxxx> > > --- > The patch is against the recent Linus' kernel, atop of the previosly sent patch > series; it requires the previosly posted patch to be applied too. > It has only been tested with CPPI 3.0/4.1 DMA drivers, and needs to be verified with other > DMA drivers... I tested it on OMAP35x EVM with Inventra DMA and found that when I try to record camera Images (ISOC IN) using Creative Live Cam and play USB audio simultaneously (ISOC OUT) then both the transfer fails after few seconds. This scenario works fine without this patch or with PIO mode. Regards, Ajay > > drivers/usb/musb/cppi_dma.c | 1 > drivers/usb/musb/musb_host.c | 237 +++++++++++++++++++------------------------ > 2 files changed, 107 insertions(+), 131 deletions(-) > > Index: linux-2.6/drivers/usb/musb/cppi_dma.c > =================================================================== > --- linux-2.6.orig/drivers/usb/musb/cppi_dma.c > +++ linux-2.6/drivers/usb/musb/cppi_dma.c > @@ -579,6 +579,7 @@ cppi_next_tx_segment(struct musb *musb, > * trigger the "send a ZLP?" confusion. > */ > rndis = (maxpacket & 0x3f) == 0 > + && length > maxpacket > && length < 0xffff > && (length % maxpacket) != 0; > > Index: linux-2.6/drivers/usb/musb/musb_host.c > =================================================================== > --- linux-2.6.orig/drivers/usb/musb/musb_host.c > +++ linux-2.6/drivers/usb/musb/musb_host.c > @@ -102,9 +102,8 @@ > */ > > > -static void musb_ep_program(struct musb *musb, u8 epnum, > - struct urb *urb, unsigned int nOut, > - u8 *buf, u32 len); > +static void musb_ep_program(struct musb *musb, u8 epnum, struct urb *urb, > + int is_out, u8 *buf, u32 ofs, u32 len); > > /* > * Clear TX fifo. Needed to avoid BABBLE errors. > @@ -174,10 +173,10 @@ static void > musb_start_urb(struct musb *musb, int is_in, struct musb_qh *qh) > { > u16 frame; > - u32 len; > - void *buf; > + u32 len, ofs = 0; > void __iomem *mbase = musb->mregs; > struct urb *urb = next_urb(qh); > + void *buf = urb->transfer_buffer; > struct musb_hw_ep *hw_ep = qh->hw_ep; > unsigned pipe = urb->pipe; > u8 address = usb_pipedevice(pipe); > @@ -200,11 +199,10 @@ musb_start_urb(struct musb *musb, int is > case USB_ENDPOINT_XFER_ISOC: > qh->iso_idx = 0; > qh->frame = 0; > - buf = urb->transfer_buffer + urb->iso_frame_desc[0].offset; > + ofs = urb->iso_frame_desc[0].offset; > len = urb->iso_frame_desc[0].length; > break; > default: /* bulk, interrupt */ > - buf = urb->transfer_buffer; > len = urb->transfer_buffer_length; > } > > @@ -217,14 +215,14 @@ musb_start_urb(struct musb *musb, int is > case USB_ENDPOINT_XFER_ISOC: s = "-iso"; break; > default: s = "-intr"; break; > }; s; }), > - epnum, buf, len); > + epnum, buf + ofs, len); > > /* Configure endpoint */ > if (is_in || hw_ep->is_shared_fifo) > hw_ep->in_qh = qh; > else > hw_ep->out_qh = qh; > - musb_ep_program(musb, epnum, urb, !is_in, buf, len); > + musb_ep_program(musb, epnum, urb, !is_in, buf, ofs, len); > > /* transmit may have more work: start it when it is time */ > if (is_in) > @@ -235,7 +233,6 @@ musb_start_urb(struct musb *musb, int is > case USB_ENDPOINT_XFER_ISOC: > case USB_ENDPOINT_XFER_INT: > DBG(3, "check whether there's still time for periodic Tx\n"); > - qh->iso_idx = 0; > frame = musb_readw(mbase, MUSB_FRAME); > /* FIXME this doesn't implement that scheduling policy ... > * or handle framecounter wrapping > @@ -616,14 +613,66 @@ musb_rx_reinit(struct musb *musb, struct > ep->rx_reinit = 0; > } > > +static bool musb_tx_dma_program(struct dma_controller *dma, > + struct musb_hw_ep *hw_ep, struct musb_qh *qh, > + struct urb *urb, u32 offset, u32 length) > +{ > + struct dma_channel *channel = hw_ep->tx_channel; > + void __iomem *epio = hw_ep->regs; > + u16 pkt_size = qh->maxpacket; > + u16 csr; > + u8 mode; > + > +#ifdef CONFIG_USB_INVENTRA_DMA > + if (length > channel->max_len) > + length = channel->max_len; > + > + csr = musb_readw(epio, MUSB_TXCSR); > + if (length > pkt_size) { > + mode = 1; > + csr |= MUSB_TXCSR_AUTOSET | MUSB_TXCSR_DMAMODE | > + MUSB_TXCSR_DMAENAB; > + } else { > + mode = 0; > + csr &= ~(MUSB_TXCSR_AUTOSET | MUSB_TXCSR_DMAMODE); > + csr |= MUSB_TXCSR_DMAENAB; /* against the programming guide */ > + } > + channel->desired_mode = mode; > + musb_writew(epio, MUSB_TXCSR, csr); > +#else > + if (!is_cppi_enabled() && !tusb_dma_omap()) > + return false; > + > + channel->actual_len = 0; > + > + /* > + * TX uses "RNDIS" mode automatically but needs help > + * to identify the zero-length-final-packet case. > + */ > + mode = urb->transfer_flags & URB_ZERO_PACKET ? 1 : 0; > +#endif > + > + qh->segsize = length; > + > + if (!dma->channel_program(channel, pkt_size, mode, > + urb->transfer_dma + offset, length)) { > + dma->channel_release(channel); > + hw_ep->tx_channel = NULL; > + > + csr = musb_readw(epio, MUSB_TXCSR); > + csr &= ~(MUSB_TXCSR_AUTOSET | MUSB_TXCSR_DMAENAB); > + musb_writew(epio, MUSB_TXCSR, csr | MUSB_TXCSR_H_WZC_BITS); > + return false; > + } > + return true; > +} > > /* > * Program an HDRC endpoint as per the given URB > * Context: irqs blocked, controller lock held > */ > -static void musb_ep_program(struct musb *musb, u8 epnum, > - struct urb *urb, unsigned int is_out, > - u8 *buf, u32 len) > +static void musb_ep_program(struct musb *musb, u8 epnum, struct urb *urb, > + int is_out, u8 *buf, u32 ofs, u32 len) > { > struct dma_controller *dma_controller; > struct dma_channel *dma_channel; > @@ -750,87 +799,14 @@ static void musb_ep_program(struct musb > else > load_count = min((u32) packet_sz, len); > > -#ifdef CONFIG_USB_INVENTRA_DMA > - if (dma_channel) { > - qh->segsize = min(len, dma_channel->max_len); > - if (qh->segsize <= packet_sz) > - dma_channel->desired_mode = 0; > - else > - dma_channel->desired_mode = 1; > - > - if (dma_channel->desired_mode == 0) > - /* Against the programming guide */ > - csr |= MUSB_TXCSR_DMAENAB; > - else > - csr |= MUSB_TXCSR_AUTOSET | > - MUSB_TXCSR_DMAMODE | > - MUSB_TXCSR_DMAENAB; > - musb_writew(epio, MUSB_TXCSR, csr); > - > - dma_ok = dma_controller->channel_program( > - dma_channel, packet_sz, > - dma_channel->desired_mode, > - urb->transfer_dma, > - qh->segsize); > - if (dma_ok) { > - load_count = 0; > - } else { > - dma_controller->channel_release(dma_channel); > - if (is_out) > - hw_ep->tx_channel = NULL; > - else > - hw_ep->rx_channel = NULL; > - dma_channel = NULL; > - > - /* > - * The programming guide says that we must only > - * clear the DMAReqEnab bit before DMAReqMode... > - */ > - csr = musb_readw(epio, MUSB_TXCSR); > - csr &= ~(MUSB_TXCSR_DMAENAB | > - MUSB_TXCSR_AUTOSET); > - musb_writew(epio, MUSB_TXCSR, csr); > - csr &= ~MUSB_TXCSR_DMAMODE; > - musb_writew(epio, MUSB_TXCSR, csr); > - } > - } > -#endif > - > - /* candidate for DMA */ > - if ((is_cppi_enabled() || tusb_dma_omap()) && dma_channel) { > - > - /* Defer enabling DMA */ > - dma_channel->actual_len = 0L; > - qh->segsize = len; > - > - /* TX uses "rndis" mode automatically, but needs help > - * to identify the zero-length-final-packet case. > - */ > - dma_ok = dma_controller->channel_program( > - dma_channel, packet_sz, > - (urb->transfer_flags > - & URB_ZERO_PACKET) > - == URB_ZERO_PACKET, > - urb->transfer_dma, > - qh->segsize); > - if (dma_ok) { > - load_count = 0; > - } else { > - dma_controller->channel_release(dma_channel); > - hw_ep->tx_channel = NULL; > - dma_channel = NULL; > - > - /* REVISIT there's an error path here that > - * needs handling: can't do dma, but > - * there's no pio buffer address... > - */ > - } > - } > + if (dma_channel && musb_tx_dma_program(dma_controller, hw_ep, > + qh, urb, ofs, len)) > + load_count = 0; > > if (load_count) { > /* PIO to load FIFO */ > qh->segsize = load_count; > - musb_write_fifo(hw_ep, load_count, buf); > + musb_write_fifo(hw_ep, load_count, buf + ofs); > } > > /* re-enable interrupt */ > @@ -885,7 +861,7 @@ static void musb_ep_program(struct musb > dma_channel, packet_sz, > !(urb->transfer_flags > & URB_SHORT_NOT_OK), > - urb->transfer_dma, > + urb->transfer_dma + ofs, > qh->segsize); > if (!dma_ok) { > dma_controller->channel_release( > @@ -1134,8 +1110,7 @@ void musb_host_tx(struct musb *musb, u8 > int pipe; > bool done = false; > u16 tx_csr; > - size_t wLength = 0; > - u8 *buf = NULL; > + size_t offset = 0, length = 0; > struct urb *urb; > struct musb_hw_ep *hw_ep = musb->endpoints + epnum; > void __iomem *epio = hw_ep->regs; > @@ -1153,7 +1128,7 @@ void musb_host_tx(struct musb *musb, u8 > /* with CPPI, DMA sometimes triggers "extra" irqs */ > if (!urb) { > DBG(4, "extra TX%d ready, csr %04x\n", epnum, tx_csr); > - goto finish; > + return; > } > > pipe = urb->pipe; > @@ -1189,7 +1164,7 @@ void musb_host_tx(struct musb *musb, u8 > musb_writew(epio, MUSB_TXCSR, > MUSB_TXCSR_H_WZC_BITS > | MUSB_TXCSR_TXPKTRDY); > - goto finish; > + return; > } > > if (status) { > @@ -1221,8 +1196,7 @@ void musb_host_tx(struct musb *musb, u8 > /* second cppi case */ > if (dma_channel_status(dma) == MUSB_DMA_STATUS_BUSY) { > DBG(4, "extra TX%d ready, csr %04x\n", epnum, tx_csr); > - goto finish; > - > + return; > } > > if (is_dma_capable() && dma && !status) { > @@ -1291,24 +1265,23 @@ void musb_host_tx(struct musb *musb, u8 > > /* REVISIT this looks wrong... */ > if (!status || dma || usb_pipeisoc(pipe)) { > - if (dma) > - wLength = dma->actual_len; > - else > - wLength = qh->segsize; > - qh->offset += wLength; > + length = dma ? dma->actual_len : qh->segsize; > + qh->offset += length; > > if (usb_pipeisoc(pipe)) { > struct usb_iso_packet_descriptor *d; > > d = urb->iso_frame_desc + qh->iso_idx; > - d->actual_length = qh->segsize; > - if (++qh->iso_idx >= urb->number_of_packets) { > + d->actual_length = length; > + d->status = status; > + if (++qh->iso_idx >= urb->number_of_packets) > done = true; > - } else { > + else { > d++; > - buf = urb->transfer_buffer + d->offset; > - wLength = d->length; > + offset = d->offset; > + length = d->length; > } > + > } else if (dma) { > done = true; > } else { > @@ -1320,10 +1293,8 @@ void musb_host_tx(struct musb *musb, u8 > & URB_ZERO_PACKET)) > done = true; > if (!done) { > - buf = urb->transfer_buffer > - + qh->offset; > - wLength = urb->transfer_buffer_length > - - qh->offset; > + offset = qh->offset; > + length = urb->transfer_buffer_length - offset; > } > } > } > @@ -1342,28 +1313,32 @@ void musb_host_tx(struct musb *musb, u8 > urb->status = status; > urb->actual_length = qh->offset; > musb_advance_schedule(musb, urb, hw_ep, USB_DIR_OUT); > + return; > + } else if (tx_csr & MUSB_TXCSR_DMAENAB) { > + if (usb_pipeisoc(pipe) && dma) { > + if (musb_tx_dma_program(musb->dma_controller, hw_ep, qh, > urb, offset, length)) > + return; > + } else { > + DBG(1, "not complete, but DMA enabled?\n"); > + return; > + } > + } > > - } else if (!(tx_csr & MUSB_TXCSR_DMAENAB)) { > - /* WARN_ON(!buf); */ > - > - /* REVISIT: some docs say that when hw_ep->tx_double_buffered, > - * (and presumably, fifo is not half-full) we should write TWO > - * packets before updating TXCSR ... other docs disagree ... > - */ > - /* PIO: start next packet in this URB */ > - if (wLength > qh->maxpacket) > - wLength = qh->maxpacket; > - musb_write_fifo(hw_ep, wLength, buf); > - qh->segsize = wLength; > - > - musb_ep_select(mbase, epnum); > - musb_writew(epio, MUSB_TXCSR, > - MUSB_TXCSR_H_WZC_BITS | MUSB_TXCSR_TXPKTRDY); > - } else > - DBG(1, "not complete, but dma enabled?\n"); > + /* > + * PIO: start next packet in this URB. > + * > + * REVISIT: some docs say that when hw_ep->tx_double_buffered, > + * (and presumably, FIFO is not half-full) we should write *two* > + * packets before updating TXCSR; other docs disagree... > + */ > + if (length > qh->maxpacket) > + length = qh->maxpacket; > + musb_write_fifo(hw_ep, length, urb->transfer_buffer + offset); > + qh->segsize = length; > > -finish: > - return; > + musb_ep_select(mbase, epnum); > + musb_writew(epio, MUSB_TXCSR, > + MUSB_TXCSR_H_WZC_BITS | MUSB_TXCSR_TXPKTRDY); > } > > > > -- > 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 -- 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
