The TX-interrupt fires (sometimes) too early and therefore we have the early_tx timer to check periodically if the transfer is done. Initially I started I started with a 150us delay which seemed to work well. This value was reduced further, because depending on the usecase a smaller poll interval was desired. Instead of tunning the number further I hereby try to get rid of the timer and instead use the TX-FIFO-empty interrupt. I've been playing with it for a while and tried various things. Here is a small summary: - Enable TX-empty interrupt "late" The TX-empty interrupt would be enabled after "DMA IRQ" if the FIFO is still full. This seems to work in generall but after removing the debug code the TX-empty interrupt isn't generated. - Use one of the two interrups In general, the TX-empty interrupt comes after the DMA-interrupt. But I've also seen it the other way around. So it not an option. - Use both interrupts. This patch. For every TX-transfer we receive two interrupts: one from the DMA side another from USB (FIFO empty). Once we seen both interrupts we consider the USB-transfer as completed. The "downside" is that we have two interrupts per TX-transfer. On the other hand we don't have the timer anymore which may lead may to multiple timer-interrupts especially on slow USB-disk media (where the device sends plenty of NAKs). On the testing side: - mass storage seems to play fine. The write perfomance is unchanged. - g_zero test 12 gives sometimes "did not went well" Testing is very welcome. Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> --- drivers/usb/musb/musb_cppi41.c | 147 +++++++++++++++++------------------------ drivers/usb/musb/musb_dsps.c | 90 ++++--------------------- drivers/usb/musb/musb_dsps.h | 83 +++++++++++++++++++++++ 3 files changed, 158 insertions(+), 162 deletions(-) create mode 100644 drivers/usb/musb/musb_dsps.h diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c index f64fd964dc6d..08846d05e671 100644 --- a/drivers/usb/musb/musb_cppi41.c +++ b/drivers/usb/musb/musb_cppi41.c @@ -6,6 +6,7 @@ #include <linux/of.h> #include "musb_core.h" +#include "musb_dsps.h" #define RNDIS_REG(x) (0x80 + ((x - 1) * 4)) @@ -32,6 +33,7 @@ struct cppi41_dma_channel { u8 is_tx; u8 is_allocated; u8 usb_toggle; + unsigned done_irqs; dma_addr_t buf_addr; u32 total_len; @@ -49,8 +51,6 @@ struct cppi41_dma_controller { struct cppi41_dma_channel rx_channel[MUSB_DMA_NUM_CHANNELS]; struct cppi41_dma_channel tx_channel[MUSB_DMA_NUM_CHANNELS]; struct musb *musb; - struct hrtimer early_tx; - struct list_head early_tx_list; u32 rx_mode; u32 tx_mode; u32 auto_req; @@ -184,40 +184,40 @@ static void cppi41_trans_done(struct cppi41_dma_channel *cppi41_channel) } } -static enum hrtimer_restart cppi41_recheck_tx_req(struct hrtimer *timer) +static void glue_tx_fifo_empty_irq(struct musb *musb, u8 epnum, bool enable) +{ + struct device *dev = musb->controller; + struct platform_device *pdev = to_platform_device(dev->parent); + struct dsps_glue *glue = platform_get_drvdata(pdev); + const struct dsps_musb_wrapper *wrp = glue->wrp; + u32 ep_mask; + u32 reg; + + ep_mask = 1 << (16 + epnum); + if (enable) + reg = wrp->coreintr_set; + else + reg = wrp->coreintr_clear; + musb_writel(musb->ctrl_base, reg, ep_mask); +} + +static void cppi41_txep_complete(struct musb *musb, u8 epnum) { struct cppi41_dma_controller *controller; - struct cppi41_dma_channel *cppi41_channel, *n; - struct musb *musb; - unsigned long flags; - enum hrtimer_restart ret = HRTIMER_NORESTART; + struct cppi41_dma_channel *cppi41_channel; - controller = container_of(timer, struct cppi41_dma_controller, - early_tx); - musb = controller->musb; + controller = container_of(musb->dma_controller, + struct cppi41_dma_controller, controller); + cppi41_channel = &controller->tx_channel[epnum - 1]; - spin_lock_irqsave(&musb->lock, flags); - list_for_each_entry_safe(cppi41_channel, n, &controller->early_tx_list, - tx_check) { - bool empty; - struct musb_hw_ep *hw_ep = cppi41_channel->hw_ep; - - empty = musb_is_tx_fifo_empty(hw_ep); - if (empty) { - list_del_init(&cppi41_channel->tx_check); - cppi41_trans_done(cppi41_channel); - } - } - - if (!list_empty(&controller->early_tx_list) && - !hrtimer_is_queued(&controller->early_tx)) { - ret = HRTIMER_RESTART; - hrtimer_forward_now(&controller->early_tx, - ktime_set(0, 20 * NSEC_PER_USEC)); - } + cppi41_channel->done_irqs--; + if (cppi41_channel->done_irqs) + return; - spin_unlock_irqrestore(&musb->lock, flags); - return ret; + if (cppi41_channel->channel.status == MUSB_DMA_STATUS_BUSY) + cppi41_trans_done(cppi41_channel); + else + pr_err("%s(%d) oh no\n", __func__, __LINE__); } static void cppi41_dma_callback(void *private_data) @@ -248,60 +248,17 @@ static void cppi41_dma_callback(void *private_data) transferred < cppi41_channel->packet_sz) cppi41_channel->prog_len = 0; + if (cppi41_channel->is_tx) { + cppi41_channel->done_irqs--; + if (cppi41_channel->done_irqs) + goto out; + } + empty = musb_is_tx_fifo_empty(hw_ep); if (empty) { cppi41_trans_done(cppi41_channel); } else { - struct cppi41_dma_controller *controller; - int is_hs = 0; - /* - * On AM335x it has been observed that the TX interrupt fires - * too early that means the TXFIFO is not yet empty but the DMA - * engine says that it is done with the transfer. We don't - * receive a FIFO empty interrupt so the only thing we can do is - * to poll for the bit. On HS it usually takes 2us, on FS around - * 110us - 150us depending on the transfer size. - * We spin on HS (no longer than than 25us and setup a timer on - * FS to check for the bit and complete the transfer. - */ - controller = cppi41_channel->controller; - - if (is_host_active(musb)) { - if (musb->port1_status & USB_PORT_STAT_HIGH_SPEED) - is_hs = 1; - } else { - if (musb->g.speed == USB_SPEED_HIGH) - is_hs = 1; - } - if (is_hs) { - unsigned wait = 25; - - do { - empty = musb_is_tx_fifo_empty(hw_ep); - if (empty) - break; - wait--; - if (!wait) - break; - udelay(1); - } while (1); - - empty = musb_is_tx_fifo_empty(hw_ep); - if (empty) { - cppi41_trans_done(cppi41_channel); - goto out; - } - } - list_add_tail(&cppi41_channel->tx_check, - &controller->early_tx_list); - if (!hrtimer_is_queued(&controller->early_tx)) { - unsigned long usecs = cppi41_channel->total_len / 10; - - hrtimer_start_range_ns(&controller->early_tx, - ktime_set(0, usecs * NSEC_PER_USEC), - 20 * NSEC_PER_USEC, - HRTIMER_MODE_REL); - } + pr_err("did not went well\n"); } out: spin_unlock_irqrestore(&musb->lock, flags); @@ -390,8 +347,10 @@ static bool cppi41_configure_channel(struct dma_channel *channel, * Due to AM335x' Advisory 1.0.13 we are not allowed to transfer more * than max packet size at a time. */ - if (cppi41_channel->is_tx) + if (cppi41_channel->is_tx) { use_gen_rndis = 1; + cppi41_channel->done_irqs = 2; + } if (use_gen_rndis) { /* RNDIS mode */ @@ -461,6 +420,9 @@ static struct dma_channel *cppi41_dma_channel_allocate(struct dma_controller *c, cppi41_channel->hw_ep = hw_ep; cppi41_channel->is_allocated = 1; + if (cppi41_channel->is_tx) + glue_tx_fifo_empty_irq(controller->musb, + cppi41_channel->hw_ep->epnum, true); return &cppi41_channel->channel; } @@ -472,6 +434,10 @@ static void cppi41_dma_channel_release(struct dma_channel *channel) cppi41_channel->is_allocated = 0; channel->status = MUSB_DMA_STATUS_FREE; channel->actual_len = 0; + if (cppi41_channel->is_tx) + glue_tx_fifo_empty_irq(cppi41_channel->controller->musb, + cppi41_channel->hw_ep->epnum, + false); } } @@ -544,6 +510,7 @@ static int cppi41_dma_channel_abort(struct dma_channel *channel) return 0; list_del_init(&cppi41_channel->tx_check); + glue_tx_fifo_empty_irq(musb, cppi41_channel->hw_ep->epnum, false); if (is_tx) { csr = musb_readw(epio, MUSB_TXCSR); csr &= ~MUSB_TXCSR_DMAENAB; @@ -581,6 +548,7 @@ static int cppi41_dma_channel_abort(struct dma_channel *channel) } cppi41_channel->channel.status = MUSB_DMA_STATUS_FREE; + glue_tx_fifo_empty_irq(musb, cppi41_channel->hw_ep->epnum, true); return 0; } @@ -677,7 +645,6 @@ void dma_controller_destroy(struct dma_controller *c) struct cppi41_dma_controller *controller = container_of(c, struct cppi41_dma_controller, controller); - hrtimer_cancel(&controller->early_tx); cppi41_dma_controller_stop(controller); kfree(controller); } @@ -685,6 +652,9 @@ void dma_controller_destroy(struct dma_controller *c) struct dma_controller *dma_controller_create(struct musb *musb, void __iomem *base) { + struct device *dev = musb->controller; + struct platform_device *pdev = to_platform_device(dev->parent); + struct dsps_glue *glue = platform_get_drvdata(pdev); struct cppi41_dma_controller *controller; int ret = 0; @@ -693,13 +663,18 @@ struct dma_controller *dma_controller_create(struct musb *musb, return NULL; } + if (glue->id_magic != DSPS_ID_MAGIC) { + dev_err(musb->controller, + "Due to the early-TX workaround I need DSPS " + "platform.\n"); + return NULL; + } + glue->tx_ep_fifo_empty = cppi41_txep_complete; + controller = kzalloc(sizeof(*controller), GFP_KERNEL); if (!controller) goto kzalloc_fail; - hrtimer_init(&controller->early_tx, CLOCK_MONOTONIC, HRTIMER_MODE_REL); - controller->early_tx.function = cppi41_recheck_tx_req; - INIT_LIST_HEAD(&controller->early_tx_list); controller->musb = musb; controller->controller.channel_alloc = cppi41_dma_channel_allocate; diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c index 53bd0e71d19f..d49402c034d2 100644 --- a/drivers/usb/musb/musb_dsps.c +++ b/drivers/usb/musb/musb_dsps.c @@ -48,6 +48,7 @@ #include <linux/debugfs.h> #include "musb_core.h" +#include "musb_dsps.h" static const struct of_device_id musb_dsps_of_match[]; @@ -75,82 +76,6 @@ static inline void dsps_writel(void __iomem *addr, unsigned offset, u32 data) __raw_writel(data, addr + offset); } -/** - * DSPS musb wrapper register offset. - * FIXME: This should be expanded to have all the wrapper registers from TI DSPS - * musb ips. - */ -struct dsps_musb_wrapper { - u16 revision; - u16 control; - u16 status; - u16 epintr_set; - u16 epintr_clear; - u16 epintr_status; - u16 coreintr_set; - u16 coreintr_clear; - u16 coreintr_status; - u16 phy_utmi; - u16 mode; - u16 tx_mode; - u16 rx_mode; - - /* bit positions for control */ - unsigned reset:5; - - /* bit positions for interrupt */ - unsigned usb_shift:5; - u32 usb_mask; - u32 usb_bitmap; - unsigned drvvbus:5; - - unsigned txep_shift:5; - u32 txep_mask; - u32 txep_bitmap; - - unsigned rxep_shift:5; - u32 rxep_mask; - u32 rxep_bitmap; - - /* bit positions for phy_utmi */ - unsigned otg_disable:5; - - /* bit positions for mode */ - unsigned iddig:5; - unsigned iddig_mux:5; - /* miscellaneous stuff */ - u8 poll_seconds; -}; - -/* - * register shadow for suspend - */ -struct dsps_context { - u32 control; - u32 epintr; - u32 coreintr; - u32 phy_utmi; - u32 mode; - u32 tx_mode; - u32 rx_mode; -}; - -/** - * DSPS glue structure. - */ -struct dsps_glue { - struct device *dev; - struct platform_device *musb; /* child musb pdev */ - const struct dsps_musb_wrapper *wrp; /* wrapper register offsets */ - struct timer_list timer; /* otg_workaround timer */ - unsigned long last_timer; /* last timer data for each instance */ - bool sw_babble_enabled; - - struct dsps_context context; - struct debugfs_regset32 regset; - struct dentry *dbgfs_root; -}; - static const struct debugfs_reg32 dsps_musb_regs[] = { { "revision", 0x00 }, { "control", 0x14 }, @@ -398,6 +323,18 @@ static irqreturn_t dsps_interrupt(int irq, void *hci) ret = IRQ_HANDLED; } + if (usbintr >> 16) { + u32 tx_ep_mask; + u8 ep; + + tx_ep_mask = usbintr >> 16; + while (tx_ep_mask) { + ep = __fls(tx_ep_mask); + tx_ep_mask &= ~(1 << ep); + glue->tx_ep_fifo_empty(musb, ep); + } + } + if (musb->int_tx || musb->int_rx || musb->int_usb) ret |= musb_interrupt(musb); @@ -783,6 +720,7 @@ static int dsps_probe(struct platform_device *pdev) glue->dev = &pdev->dev; glue->wrp = wrp; + glue->id_magic = DSPS_ID_MAGIC; platform_set_drvdata(pdev, glue); pm_runtime_enable(&pdev->dev); diff --git a/drivers/usb/musb/musb_dsps.h b/drivers/usb/musb/musb_dsps.h new file mode 100644 index 000000000000..192c0ac6af39 --- /dev/null +++ b/drivers/usb/musb/musb_dsps.h @@ -0,0 +1,83 @@ +#ifndef _MUSB_DSPS_H__ +#define _MUSB_DSPS_H__ +#include <linux/debugfs.h> + +/** + * DSPS musb wrapper register offset. + * FIXME: This should be expanded to have all the wrapper registers from TI DSPS + * musb ips. + */ +struct dsps_musb_wrapper { + u16 revision; + u16 control; + u16 status; + u16 epintr_set; + u16 epintr_clear; + u16 epintr_status; + u16 coreintr_set; + u16 coreintr_clear; + u16 coreintr_status; + u16 phy_utmi; + u16 mode; + u16 tx_mode; + u16 rx_mode; + + /* bit positions for control */ + unsigned reset:5; + + /* bit positions for interrupt */ + unsigned usb_shift:5; + u32 usb_mask; + u32 usb_bitmap; + unsigned drvvbus:5; + + unsigned txep_shift:5; + u32 txep_mask; + u32 txep_bitmap; + + unsigned rxep_shift:5; + u32 rxep_mask; + u32 rxep_bitmap; + + /* bit positions for phy_utmi */ + unsigned otg_disable:5; + + /* bit positions for mode */ + unsigned iddig:5; + unsigned iddig_mux:5; + /* miscellaneous stuff */ + u8 poll_seconds; +}; +/* + * register shadow for suspend + */ +struct dsps_context { + u32 control; + u32 epintr; + u32 coreintr; + u32 phy_utmi; + u32 mode; + u32 tx_mode; + u32 rx_mode; +}; + +#define DSPS_ID_MAGIC 0x29788445 +/** + * DSPS glue structure. + */ +struct dsps_glue { + unsigned int id_magic; + struct device *dev; + struct platform_device *musb; /* child musb pdev */ + const struct dsps_musb_wrapper *wrp; /* wrapper register offsets */ + struct timer_list timer; /* otg_workaround timer */ + unsigned long last_timer; /* last timer data for each instance */ + bool sw_babble_enabled; + void (*tx_ep_fifo_empty)(struct musb *musb, u8 epnum); + + struct dsps_context context; + struct debugfs_regset32 regset; + struct dentry *dbgfs_root; +}; + +#endif -- 2.1.3 -- 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