usb/isp1760: Use polling instead of SOF interrupts to fix Errata 2

Errata 2 for the isp1760 explains that the chip sometimes does not issue
interrupts when an ATL (bulk or control) transfer is completed. There are
several issues with the current work-around (SOF interrupts) for this:

1) It seems the chip sometimes does not even set the done bit for a
   completed transfer, in which case SOF interrupts does not solve
   the problem since we still check the done map to find out which
   transfer descriptors to handle.

2) The above point seems to happen only when ATL and SOF interrupts
   are enabled at the same time. However, disabling ATL interrupts
   increases the latency between transfer completion and handling.
   This is very noticeable in the testusb suite, which take several
   minutes more to run with ATL interrupts disabled.

This patch removes the code to switch on SOF interrupts, and instead
use a kernel timer to periodically check for "old" descriptors that
have their VALID and ACTIVE flags unset, indicating completion, thus
avoiding the dependency on the chip's done map (and SOF interrupts)
to find transfers affected by this HW bug.

[bigeasy@linutronix: 80 lines limit]

Signed-off-by: Arvid Brodin <arvid.brodin@enea.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

diff --git a/drivers/usb/host/isp1760-hcd.c b/drivers/usb/host/isp1760-hcd.c
index e399e23..14c9238 100644 (file)
--- a/drivers/usb/host/isp1760-hcd.c
+++ b/drivers/usb/host/isp1760-hcd.c
@@ -21,6 +21,7 @@
 #include <linux/uaccess.h>
 #include <linux/io.h>
 #include <linux/mm.h>
+#include <linux/timer.h>
 #include <asm/unaligned.h>
 #include <asm/cacheflush.h>
 
@@ -39,7 +40,6 @@ struct isp1760_hcd {
        int                     int_done_map;
        struct memory_chunk memory_pool[BLOCKS];
        struct list_head        controlqhs, bulkqhs, interruptqhs;
-       int active_ptds;
 
        /* periodic schedule support */
 #define        DEFAULT_I_TDPS          1024
@@ -489,10 +489,6 @@ static int isp1760_hc_setup(struct usb_hcd *hcd)
                           16 : 32, (priv->devflags & ISP1760_FLAG_ANALOG_OC) ?
                           "analog" : "digital");
 
-       /* This is weird: at the first plug-in of a device there seems to be
-          one packet queued that never gets returned? */
-       priv->active_ptds = -1;
-
        /* ATL reset */
        reg_write32(hcd->regs, HC_HW_MODE_CTRL, hwmode | ALL_ATX_RESET);
        mdelay(10);
@@ -741,8 +737,8 @@ static void start_bus_transfer(struct usb_hcd *hcd, u32 ptd_offset, int slot,
        qh->slot = slot;
        qtd->status = QTD_XFER_STARTED; /* Set this before writing ptd, since
                interrupt routine may preempt and expects this value. */
+       slots[slot].timestamp = jiffies;
        ptd_write(hcd->regs, ptd_offset, slot, ptd);
-       priv->active_ptds++;
 
        /* Make sure done map has not triggered from some unlinked transfer */
        if (ptd_offset == ATL_PTD_OFFSET) {
@@ -1091,11 +1087,9 @@ static int check_atl_transfer(struct usb_hcd *hcd, struct ptd *ptd,
        return PTD_STATE_QTD_DONE;
 }
 
-static irqreturn_t isp1760_irq(struct usb_hcd *hcd)
+static void handle_done_ptds(struct usb_hcd *hcd)
 {
        struct isp1760_hcd *priv = hcd_to_priv(hcd);
-       u32 imask;
-       irqreturn_t irqret = IRQ_NONE;
        struct ptd ptd;
        struct isp1760_qh *qh;
        int slot;
@@ -1104,27 +1098,14 @@ static irqreturn_t isp1760_irq(struct usb_hcd *hcd)
        u32 ptd_offset;
        struct isp1760_qtd *qtd;
        int modified;
-       static int last_active_ptds;
-       int int_skip_map, atl_skip_map;
-
-       spin_lock(&priv->lock);
-
-       if (!(hcd->state & HC_STATE_RUNNING))
-               goto leave;
-
-       imask = reg_read32(hcd->regs, HC_INTERRUPT_REG);
-       if (unlikely(!imask))
-               goto leave;
-       reg_write32(hcd->regs, HC_INTERRUPT_REG, imask); /* Clear */
+       int skip_map;
 
-       int_skip_map = reg_read32(hcd->regs, HC_INT_PTD_SKIPMAP_REG);
-       atl_skip_map = reg_read32(hcd->regs, HC_ATL_PTD_SKIPMAP_REG);
-       priv->int_done_map |= reg_read32(hcd->regs, HC_INT_PTD_DONEMAP_REG);
-       priv->atl_done_map |= reg_read32(hcd->regs, HC_ATL_PTD_DONEMAP_REG);
-       priv->int_done_map &= ~int_skip_map;
-       priv->atl_done_map &= ~atl_skip_map;
+       skip_map = reg_read32(hcd->regs, HC_INT_PTD_SKIPMAP_REG);
+       priv->int_done_map &= ~skip_map;
+       skip_map = reg_read32(hcd->regs, HC_ATL_PTD_SKIPMAP_REG);
+       priv->atl_done_map &= ~skip_map;
 
-       modified = priv->int_done_map | priv->atl_done_map;
+       modified = priv->int_done_map || priv->atl_done_map;
 
        while (priv->int_done_map || priv->atl_done_map) {
                if (priv->int_done_map) {
@@ -1163,7 +1144,6 @@ static irqreturn_t isp1760_irq(struct usb_hcd *hcd)
                slots[slot].qtd = NULL;
                qh = slots[slot].qh;
                slots[slot].qh = NULL;
-               priv->active_ptds--;
                qh->slot = -1;
 
                WARN_ON(qtd->status != QTD_XFER_STARTED);
@@ -1234,22 +1214,28 @@ static irqreturn_t isp1760_irq(struct usb_hcd *hcd)
 
        if (modified)
                schedule_ptds(hcd);
+}
 
-       /* ISP1760 Errata 2 explains that interrupts may be missed (or not
-          happen?) if two USB devices are running simultaneously. Perhaps
-          this happens when a PTD is finished during interrupt handling;
-          enable SOF interrupts if PTDs are still scheduled when exiting this
-          interrupt handler, just to be safe. */
+static irqreturn_t isp1760_irq(struct usb_hcd *hcd)
+{
+       struct isp1760_hcd *priv = hcd_to_priv(hcd);
+       u32 imask;
+       irqreturn_t irqret = IRQ_NONE;
 
-       if (priv->active_ptds != last_active_ptds) {
-               if (priv->active_ptds > 0)
-                       reg_write32(hcd->regs, HC_INTERRUPT_ENABLE,
-                                               INTERRUPT_ENABLE_SOT_MASK);
-               else
-                       reg_write32(hcd->regs, HC_INTERRUPT_ENABLE,
-                                               INTERRUPT_ENABLE_MASK);
-               last_active_ptds = priv->active_ptds;
-       }
+       spin_lock(&priv->lock);
+
+       if (!(hcd->state & HC_STATE_RUNNING))
+               goto leave;
+
+       imask = reg_read32(hcd->regs, HC_INTERRUPT_REG);
+       if (unlikely(!imask))
+               goto leave;
+       reg_write32(hcd->regs, HC_INTERRUPT_REG, imask); /* Clear */
+
+       priv->int_done_map |= reg_read32(hcd->regs, HC_INT_PTD_DONEMAP_REG);
+       priv->atl_done_map |= reg_read32(hcd->regs, HC_ATL_PTD_DONEMAP_REG);
+
+       handle_done_ptds(hcd);
 
        irqret = IRQ_HANDLED;
 leave:
@@ -1258,6 +1244,63 @@ leave:
        return irqret;
 }
 
+/*
+ * Workaround for problem described in chip errata 2:
+ *
+ * Sometimes interrupts are not generated when ATL (not INT?) completion occurs.
+ * One solution suggested in the errata is to use SOF interrupts _instead_of_
+ * ATL done interrupts (the "instead of" might be important since it seems
+ * enabling ATL interrupts also causes the chip to sometimes - rarely - "forget"
+ * to set the PTD's done bit in addition to not generating an interrupt!).
+ *
+ * So if we use SOF + ATL interrupts, we sometimes get stale PTDs since their
+ * done bit is not being set. This is bad - it blocks the endpoint until reboot.
+ *
+ * If we use SOF interrupts only, we get latency between ptd completion and the
+ * actual handling. This is very noticeable in testusb runs which takes several
+ * minutes longer without ATL interrupts.
+ *
+ * A better solution is to run the code below every SLOT_CHECK_PERIOD ms. If it
+ * finds active ATL slots which are older than SLOT_TIMEOUT ms, it checks the
+ * slot's ACTIVE and VALID bits. If these are not set, the ptd is considered
+ * completed and its done map bit is set.
+ *
+ * The values of SLOT_TIMEOUT and SLOT_CHECK_PERIOD have been arbitrarily chosen
+ * not to cause too much lag when this HW bug occurs, while still hopefully
+ * ensuring that the check does not falsely trigger.
+ */
+#define SLOT_TIMEOUT 180
+#define SLOT_CHECK_PERIOD 200
+static struct timer_list errata2_timer;
+
+void errata2_function(unsigned long data)
+{
+       struct usb_hcd *hcd = (struct usb_hcd *) data;
+       struct isp1760_hcd *priv = hcd_to_priv(hcd);
+       int slot;
+       struct ptd ptd;
+       unsigned long spinflags;
+
+       spin_lock_irqsave(&priv->lock, spinflags);
+
+       for (slot = 0; slot < 32; slot++)
+               if ((priv->atl_slots[slot].qh || priv->atl_slots[slot].qtd) &&
+                               time_after(jiffies + SLOT_TIMEOUT * HZ / 1000,
+                               priv->atl_slots[slot].timestamp)) {
+                       ptd_read(hcd->regs, ATL_PTD_OFFSET, slot, &ptd);
+                       if (!FROM_DW0_VALID(ptd.dw0) &&
+                                       !FROM_DW3_ACTIVE(ptd.dw3))
+                               priv->atl_done_map |= 1 << slot;
+               }
+
+       handle_done_ptds(hcd);
+
+       spin_unlock_irqrestore(&priv->lock, spinflags);
+
+       errata2_timer.expires = jiffies + SLOT_CHECK_PERIOD * HZ / 1000;
+       add_timer(&errata2_timer);
+}
+
 static int isp1760_run(struct usb_hcd *hcd)
 {
        int retval;
@@ -1303,6 +1346,12 @@ static int isp1760_run(struct usb_hcd *hcd)
        if (retval)
                return retval;
 
+       init_timer(&errata2_timer);
+       errata2_timer.function = errata2_function;
+       errata2_timer.data = (unsigned long) hcd;
+       errata2_timer.expires = jiffies + SLOT_CHECK_PERIOD * HZ / 1000;
+       add_timer(&errata2_timer);
+
        chipid = reg_read32(hcd->regs, HC_CHIP_ID_REG);
        dev_info(hcd->self.controller, "USB ISP %04x HW rev. %d started\n",
                                        chipid & 0xffff, chipid >> 16);
@@ -1561,7 +1610,6 @@ static void kill_transfer(struct usb_hcd *hcd, struct urb *urb,
        }
 
        qh->slot = -1;
-       priv->active_ptds--;
 }
 
 static int isp1760_urb_dequeue(struct usb_hcd *hcd, struct urb *urb,
@@ -2012,6 +2060,8 @@ static void isp1760_stop(struct usb_hcd *hcd)
        struct isp1760_hcd *priv = hcd_to_priv(hcd);
        u32 temp;
 
+       del_timer(&errata2_timer);
+
        isp1760_hub_control(hcd, ClearPortFeature, USB_PORT_FEAT_POWER, 1,
                        NULL, 0);
        mdelay(20);

diff --git a/drivers/usb/host/isp1760-hcd.h b/drivers/usb/host/isp1760-hcd.h
index 014a7df..fda0f2d 100644 (file)
--- a/drivers/usb/host/isp1760-hcd.h
+++ b/drivers/usb/host/isp1760-hcd.h
@@ -73,7 +73,6 @@ void deinit_kmem_cache(void);
 #define HC_EOT_INT             (1 << 3)
 #define HC_SOT_INT             (1 << 1)
 #define INTERRUPT_ENABLE_MASK  (HC_INTL_INT | HC_ATL_INT)
-#define INTERRUPT_ENABLE_SOT_MASK      (HC_SOT_INT)
 
 #define HC_ISO_IRQ_MASK_OR_REG 0x318
 #define HC_INT_IRQ_MASK_OR_REG 0x31C
@@ -107,6 +106,7 @@ struct ptd {
 struct slotinfo {
        struct isp1760_qh *qh;
        struct isp1760_qtd *qtd;
+       unsigned long timestamp;
 };
 
 
@@ -188,6 +188,7 @@ struct memory_chunk {
 #define DW3_BABBLE_BIT                 (1 << 29)
 #define DW3_HALT_BIT                   (1 << 30)
 #define DW3_ACTIVE_BIT                 (1 << 31)
+#define FROM_DW3_ACTIVE(x)             (((x) >> 31) & 0x01)
 
 #define INT_UNDERRUN                   (1 << 2)
 #define INT_BABBLE                     (1 << 1)