From ff3fcac949187d98684aaf3f1c35c7cae7712649 Mon Sep 17 00:00:00 2001 From: Daniel Mack Date: Mon, 26 May 2014 14:52:38 +0200 Subject: usb: musb: introduce dma_channel.rx_packet_done The musb/cppi41 glue layer is capable of handling transactions that span over more than one USB packet by reloading the DMA descriptors partially. An urb is considered completed when either its transfer buffer has been filled entirely (actual_length == transfer_buffer_length) or if a packet in the stream has less bytes than the endpoint's wMaxPacketSize. Once one of the above conditions is met, musb_dma_completion() is called from cppi41_trans_done(). However, the final decision whether or not to return the urb to its owner is made by the core and its determination of the variable 'done' in musb_host_rx(). This code has currently no way of knowing what the size of the last packet was, and whether or not to give back the urb due to a short read. Fix this by introducing a new boolean flag in 'struct dma_channel', and set it from musb_cppi41.c. If set, it will make the core do what the DMA layer decided and complete the urb. Signed-off-by: Daniel Mack Acked-by: George Cherian Signed-off-by: Felipe Balbi --- drivers/usb/musb/musb_cppi41.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'drivers/usb/musb/musb_cppi41.c') diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c index 7b8bbf53127e..4187ef1fab29 100644 --- a/drivers/usb/musb/musb_cppi41.c +++ b/drivers/usb/musb/musb_cppi41.c @@ -139,6 +139,7 @@ static void cppi41_trans_done(struct cppi41_dma_channel *cppi41_channel) cppi41_channel->channel.actual_len = cppi41_channel->transferred; cppi41_channel->channel.status = MUSB_DMA_STATUS_FREE; + cppi41_channel->channel.rx_packet_done = true; musb_dma_completion(musb, hw_ep->epnum, cppi41_channel->is_tx); } else { /* next iteration, reload */ @@ -450,6 +451,7 @@ static bool cppi41_configure_channel(struct dma_channel *channel, dma_desc->callback = cppi41_dma_callback; dma_desc->callback_param = channel; cppi41_channel->cookie = dma_desc->tx_submit(dma_desc); + cppi41_channel->channel.rx_packet_done = false; save_rx_toggle(cppi41_channel); dma_async_issue_pending(dc); -- cgit v1.2.3 From f50e67853b363b96336718597823ed7a7e8652de Mon Sep 17 00:00:00 2001 From: Daniel Mack Date: Mon, 26 May 2014 14:52:39 +0200 Subject: usb: musb/cppi41: call musb_ep_select() before accessing an endpoint's CSR Before accessing any of an endpoint's CSR registers, make sure the correct endpoint is selected. Otherwise, data read from or written to the registers is likely to affect the wrong endpoint as long as the connected device has more than one endpoint. This, of course, leads to all sorts of strange effects such as stream starvation and driver internal state machine confusion due to spurious interrupts. Signed-off-by: Daniel Mack Signed-off-by: Felipe Balbi --- drivers/usb/musb/musb_cppi41.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) (limited to 'drivers/usb/musb/musb_cppi41.c') diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c index 4187ef1fab29..932464ffb10c 100644 --- a/drivers/usb/musb/musb_cppi41.c +++ b/drivers/usb/musb/musb_cppi41.c @@ -74,15 +74,18 @@ static void save_rx_toggle(struct cppi41_dma_channel *cppi41_channel) static void update_rx_toggle(struct cppi41_dma_channel *cppi41_channel) { + struct musb_hw_ep *hw_ep = cppi41_channel->hw_ep; + struct musb *musb = hw_ep->musb; u16 csr; u8 toggle; if (cppi41_channel->is_tx) return; - if (!is_host_active(cppi41_channel->controller->musb)) + if (!is_host_active(musb)) return; - csr = musb_readw(cppi41_channel->hw_ep->regs, MUSB_RXCSR); + musb_ep_select(musb->mregs, hw_ep->epnum); + csr = musb_readw(hw_ep->regs, MUSB_RXCSR); toggle = csr & MUSB_RXCSR_H_DATATOGGLE ? 1 : 0; /* @@ -107,6 +110,7 @@ static bool musb_is_tx_fifo_empty(struct musb_hw_ep *hw_ep) void __iomem *epio = musb->endpoints[epnum].regs; u16 csr; + musb_ep_select(musb->mregs, hw_ep->epnum); csr = musb_readw(epio, MUSB_TXCSR); if (csr & MUSB_TXCSR_TXPKTRDY) return false; @@ -173,6 +177,7 @@ static void cppi41_trans_done(struct cppi41_dma_channel *cppi41_channel) dma_async_issue_pending(dc); if (!cppi41_channel->is_tx) { + musb_ep_select(musb->mregs, hw_ep->epnum); csr = musb_readw(epio, MUSB_RXCSR); csr |= MUSB_RXCSR_H_REQPKT; musb_writew(epio, MUSB_RXCSR, csr); -- cgit v1.2.3 From eefae89e00cc7186fc8cde811e43074844af83c0 Mon Sep 17 00:00:00 2001 From: Daniel Mack Date: Fri, 20 Jun 2014 00:20:43 +0200 Subject: Revert "usb: musb: musb_cppi41: Handle ISOCH differently and not use the hrtimer." This reverts commit 1af54b7a4. The commit tried to address cases in which isochronous transfers are 'not reliable', most probably in the tests conducted, polling for the MUSB_TXCSR_TXPKTRDY bit in MUSB_TXCSR is done too late. Hence, it installs a work struct which basically busy-polls for the bit in a rather agressive way by rescheduling the work if the FIFO is not empty. With USB audio devices, tests have shown that it takes approximately 100 iterations of the asynchronous worker until the FIFO signals completion, which leads to 100% CPU loads when streaming audio. The issue the patch tried to address can be handled differently, which is what the next patch does. Signed-off-by: Daniel Mack Reported-by: Sebastian Reimers Signed-off-by: Felipe Balbi --- drivers/usb/musb/musb_cppi41.c | 53 ------------------------------------------ 1 file changed, 53 deletions(-) (limited to 'drivers/usb/musb/musb_cppi41.c') diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c index 932464ffb10c..a2c445608e75 100644 --- a/drivers/usb/musb/musb_cppi41.c +++ b/drivers/usb/musb/musb_cppi41.c @@ -39,7 +39,6 @@ struct cppi41_dma_channel { u32 transferred; u32 packet_sz; struct list_head tx_check; - struct work_struct dma_completion; }; #define MUSB_DMA_NUM_CHANNELS 15 @@ -117,18 +116,6 @@ static bool musb_is_tx_fifo_empty(struct musb_hw_ep *hw_ep) return true; } -static bool is_isoc(struct musb_hw_ep *hw_ep, bool in) -{ - if (in && hw_ep->in_qh) { - if (hw_ep->in_qh->type == USB_ENDPOINT_XFER_ISOC) - return true; - } else if (hw_ep->out_qh) { - if (hw_ep->out_qh->type == USB_ENDPOINT_XFER_ISOC) - return true; - } - return false; -} - static void cppi41_dma_callback(void *private_data); static void cppi41_trans_done(struct cppi41_dma_channel *cppi41_channel) @@ -185,32 +172,6 @@ static void cppi41_trans_done(struct cppi41_dma_channel *cppi41_channel) } } -static void cppi_trans_done_work(struct work_struct *work) -{ - unsigned long flags; - struct cppi41_dma_channel *cppi41_channel = - container_of(work, struct cppi41_dma_channel, dma_completion); - struct cppi41_dma_controller *controller = cppi41_channel->controller; - struct musb *musb = controller->musb; - struct musb_hw_ep *hw_ep = cppi41_channel->hw_ep; - bool empty; - - if (!cppi41_channel->is_tx && is_isoc(hw_ep, 1)) { - spin_lock_irqsave(&musb->lock, flags); - cppi41_trans_done(cppi41_channel); - spin_unlock_irqrestore(&musb->lock, flags); - } else { - empty = musb_is_tx_fifo_empty(hw_ep); - if (empty) { - spin_lock_irqsave(&musb->lock, flags); - cppi41_trans_done(cppi41_channel); - spin_unlock_irqrestore(&musb->lock, flags); - } else { - schedule_work(&cppi41_channel->dma_completion); - } - } -} - static enum hrtimer_restart cppi41_recheck_tx_req(struct hrtimer *timer) { struct cppi41_dma_controller *controller; @@ -274,14 +235,6 @@ static void cppi41_dma_callback(void *private_data) transferred < cppi41_channel->packet_sz) cppi41_channel->prog_len = 0; - if (!cppi41_channel->is_tx) { - if (is_isoc(hw_ep, 1)) - schedule_work(&cppi41_channel->dma_completion); - else - cppi41_trans_done(cppi41_channel); - goto out; - } - empty = musb_is_tx_fifo_empty(hw_ep); if (empty) { cppi41_trans_done(cppi41_channel); @@ -318,10 +271,6 @@ static void cppi41_dma_callback(void *private_data) goto out; } } - if (is_isoc(hw_ep, 0)) { - schedule_work(&cppi41_channel->dma_completion); - goto out; - } list_add_tail(&cppi41_channel->tx_check, &controller->early_tx_list); if (!hrtimer_active(&controller->early_tx)) { @@ -679,8 +628,6 @@ static int cppi41_dma_controller_start(struct cppi41_dma_controller *controller) cppi41_channel->port_num = port; cppi41_channel->is_tx = is_tx; INIT_LIST_HEAD(&cppi41_channel->tx_check); - INIT_WORK(&cppi41_channel->dma_completion, - cppi_trans_done_work); musb_dma = &cppi41_channel->channel; musb_dma->private_data = cppi41_channel; -- cgit v1.2.3 From 50aea6fca771d6daf3ec24f771da866f7fd836e4 Mon Sep 17 00:00:00 2001 From: Daniel Mack Date: Fri, 20 Jun 2014 00:20:44 +0200 Subject: usb: musb: cppi41: fire hrtimer according to programmed channel length The musb/cppi41 code installs a hrtimer to work around DMA completion interrupts that have fired too early on AM335x hardware. This timer is currently programmed to first fire 140 microseconds after the DMA completion callback. According to the commit which introduced it (a655f481d83, "usb: musb: musb_cppi41: handle pre-mature TX complete interrupt"), that value is is considered a 'rule of thumb' that worked well with the test case described in the commit log. Test show, however, that for USB audio devices and much smaller packet sizes, the timer has to fire earlier in order to correctly handle the audio stream. The original test case had output transfer sizes of 1514 bytes, and a delay of 140 microseconds. For audio devices with 24 bytes channel size, 3 microseconds seem to work well. Hence, let's assume that the time it takes to clear the bit correlates with the number of bytes transferred. The referenced commit log mentions such a suspicion as well. Let the timer fire in cppi41_channel->total_len/10 microseconds to correctly handle both cases. Also, shorten the interval in which the timer fires again in case of a non-empty early_tx list. With these changes in place, both FS and HS audio devices appear to work well on AM335x hardware. Signed-off-by: Daniel Mack Reported-by: Sebastian Reimers Signed-off-by: Felipe Balbi --- drivers/usb/musb/musb_cppi41.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'drivers/usb/musb/musb_cppi41.c') diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c index a2c445608e75..adfffe884891 100644 --- a/drivers/usb/musb/musb_cppi41.c +++ b/drivers/usb/musb/musb_cppi41.c @@ -200,7 +200,7 @@ static enum hrtimer_restart cppi41_recheck_tx_req(struct hrtimer *timer) if (!list_empty(&controller->early_tx_list)) { ret = HRTIMER_RESTART; hrtimer_forward_now(&controller->early_tx, - ktime_set(0, 150 * NSEC_PER_USEC)); + ktime_set(0, 50 * NSEC_PER_USEC)); } spin_unlock_irqrestore(&musb->lock, flags); @@ -274,8 +274,10 @@ static void cppi41_dma_callback(void *private_data) list_add_tail(&cppi41_channel->tx_check, &controller->early_tx_list); if (!hrtimer_active(&controller->early_tx)) { + unsigned long usecs = cppi41_channel->total_len / 10; + hrtimer_start_range_ns(&controller->early_tx, - ktime_set(0, 140 * NSEC_PER_USEC), + ktime_set(0, usecs * NSEC_PER_USEC), 40 * NSEC_PER_USEC, HRTIMER_MODE_REL); } -- cgit v1.2.3