Hello.
Felipe Balbi wrote:
From: Arnaud Mandy <ext-arnaud.2.mandy@xxxxxxxxx>
rewriting of dma code for musb_g_rx function.
Signed-off-by: Arnaud Mandy <ext-arnaud.2.mandy@xxxxxxxxx>
Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxx>
[...]
diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index c11d31f..90655df 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1556,7 +1556,7 @@ irqreturn_t musb_interrupt(struct musb *musb)
musb_host_rx(musb, ep_num);
} else {
if (is_peripheral_capable())
- musb_g_rx(musb, ep_num);
+ musb_g_rx(musb, ep_num, false);
}
}
@@ -1627,14 +1627,11 @@ void musb_dma_completion(struct musb *musb, u8 epnum, u8 transmit)
musb_host_rx(musb, epnum);
} else {
if (is_peripheral_capable())
- musb_g_rx(musb, epnum);
+ musb_g_rx(musb, epnum, true);
Actually, I think it would have been cleaner to have separate versions
of musb_g_[rt]x() for the DMA and non-DMA interrupt. The same can be said of
the host side...
}
}
}
}
-
-#else
-#define use_dma 0
This surely should be a part of another patch, one that's moving use_dma
module parameter to the start of file.
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index 45cd522..83d4ffc 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -584,28 +584,52 @@ static void do_pio_rx(struct musb *musb, struct musb_request *req)
/*
* Data ready for a request; called from IRQ
*/
-void musb_g_rx(struct musb *musb, u8 epnum)
+void musb_g_rx(struct musb *musb, u8 epnum, bool is_dma)
{
u16 csr;
+ struct musb_request *req;
struct usb_request *request;
void __iomem *mbase = musb->mregs;
struct musb_ep *musb_ep = &musb->endpoints[epnum].ep_out;
void __iomem *epio = musb->endpoints[epnum].regs;
+ struct dma_channel *dma;
musb_ep_select(mbase, epnum);
+ csr = musb_readw(epio, MUSB_RXCSR);
+restart:
+
Shouldn't an empty line *precede* the label?
+ if (csr == 0) {
+ DBG(3, "spurious IRQ\n");
+ return;
+ }
+
request = next_request(musb_ep);
+ if (!request) {
+ DBG(1, "waiting for request for %s (csr %04x)\n",
+ musb_ep->name, csr);
+ musb_ep->rx_pending = true;
+ return;
+ }
- csr = musb_readw(epio, MUSB_RXCSR);
+ dma = musb_ep->dma;
- DBG(4, "<== %s, rxcsr %04x %p\n", musb_ep->end_point.name,
- csr, request);
+ DBG(4, "<== %s, rxcsr %04x %p (dma %s, %s)\n", musb_ep->name,
+ csr, request, dma ? "enabled" : "disabled",
+ is_dma ? "true" : "false");
if (csr & MUSB_RXCSR_P_SENTSTALL) {
+ DBG(5, "ep%d is halted, cannot transfer\n", epnum);
csr |= MUSB_RXCSR_P_WZC_BITS;
csr &= ~MUSB_RXCSR_P_SENTSTALL;
musb_writew(epio, MUSB_RXCSR, csr);
+ if (dma != NULL &&
+ dma_channel_status(dma) == MUSB_DMA_STATUS_BUSY) {
+ dma->status = MUSB_DMA_STATUS_CORE_ABORT;
+ musb->dma_controller->channel_abort(dma);
That's pure nonsense -- my STALL handling fix is removing this stuff...
+ }
+
if (request)
musb_g_giveback(musb_ep, request, -EPIPE);
... and this too.
return;
@@ -625,21 +649,62 @@ void musb_g_rx(struct musb *musb, u8 epnum)
DBG(4, "%s, incomprx\n", musb_ep->end_point.name);
}
- /* analyze request if the ep is hot */
- if (request) {
- do_pio_rx(musb, to_musb_request(request));
- } else {
- DBG(3, "packet waiting for %s%s request\n",
- musb_ep->desc ? "" : "inactive ",
- musb_ep->end_point.name);
- musb_ep->rx_pending = true;
+ req = to_musb_request(request);
+
+ BUG_ON(dma == NULL && (csr & MUSB_RXCSR_DMAENAB));
+
+ if (dma != NULL) {
+ u32 len;
+
+ /* We do handle stalls yet. */
"We don't" perhaps?..
+ BUG_ON(csr & MUSB_RXCSR_P_SENDSTALL);
+
+ /* We abort() so dma->actual_len gets updated */
+ musb->dma_controller->channel_abort(dma);
+
+ /* We only expect full packets. */
+ BUG_ON(dma->actual_len & (musb_ep->packet_sz - 1));
+
+ request->actual += dma->actual_len;
+ len = dma->actual_len;
+
+ stop_dma(musb, musb_ep, req);
+ dma = NULL;
+
+ DBG(4, "RXCSR%d %04x, dma off, %04x, len %zu, req %p\n",
If 'len' is 'u32', why are you using %zu format?
+ epnum, csr, musb_readw(epio, MUSB_RXCSR),
+ len, request);
+
+ if (!is_dma) {
+ /* Unload with pio */
+ do_pio_rx(musb, req);
+ } else {
+ BUG_ON(request->actual != request->length);
Won't it almost always be the case since we've just aborted? Why bug then?
+ musb_g_giveback(musb_ep, request, 0);
+ }
+ return;
+ }
+
+ if (dma == NULL && musb->use_dma) {
+ if (start_dma(musb, req) == 0)
Why not "if (!start_dma(musb, req))"?
+ dma = musb_ep->dma;
+ }
+
+ if (dma == NULL) {
+ do_pio_rx(musb, req);
+ csr = musb_readw(epio, MUSB_RXCSR);
+ if (csr & MUSB_RXCSR_RXPKTRDY) {
+ DBG(2, "new packet in FIFO, restarting RX "
+ "(CSR %04x)\n", csr);
+ goto restart;
Won't you get another interrupt anyway? And the label is mispaced
anyway, it's pointless to check for csr == 0 when it's known to have
RxPktRdy bit set...
diff --git a/drivers/usb/musb/musb_gadget.h b/drivers/usb/musb/musb_gadget.h
index f9cb3a8..3e62716 100644
--- a/drivers/usb/musb/musb_gadget.h
+++ b/drivers/usb/musb/musb_gadget.h
@@ -98,7 +98,7 @@ static inline struct usb_request *next_request(struct musb_ep *ep)
}
extern void musb_g_tx(struct musb *musb, u8 epnum);
-extern void musb_g_rx(struct musb *musb, u8 epnum);
+extern void musb_g_rx(struct musb *musb, u8 epnum, bool);
Why you're only naming 2 parameters out of 3?
WBR, Sergei
--
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
