Hi,
On Thu, Dec 4, 2014 at 11:15 AM, Sebastian Andrzej Siewior
<bigeasy@xxxxxxxxxxxxx> wrote:
> 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:
This has to be tested thoroughly. The TI maintained 3.2 kernel for
AM335x only enables the TX-FIFO-Empty interrupt for ISO transfer, and
still uses polling for other transfers. I don't know the details but
what I heard is that the FIFO-Empty interrupt is only reliable for ISO
xfer.
Regards,
-Bin.
>
> - 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
--
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
