Hi Arvid,
On Fri, Apr 13, 2012 at 01:58:25PM +0000, Arvid Brodin wrote:
> On 2012-04-13 12:59, Michael Grzeschik wrote:
> > The errata2 workaround is described in the Datasheet as follows:
> >
> > An SOF INT can be used instead of an ATL INT with polling on Done
> > bits. A time-out can be implemented and if a certain Done bit is never
> > set, verification of the PTD completion can be done by reading PTD
> > contents (valid bit). This is a proven workaround implemented in
> > software.
> >
> > Therefore, we should do so, and ignore the check
> > for the ACTIVE_BIT in DW3. Because it is possible
> > that this bit is still set when the issue accures.
>
> I believe you trust the datasheet too much! :) I've tried this before (in fact, most of
> your code seems to be from an earlier patch of mine). It doesn't work, as explained in the
> comment to errata2_function() (and as tested with the usbtest/testusb suite).
Yes, i know the older patches struggling with that issue. But in both
cases there were some details which does not fit to the exact errata2
fix description.
In the first patch (USB: isp1760: Implement solution for erratum 2)
the content checking (Valid Bit: 0) was totaly left out of sight.
The second patch (usb/isp1760: Use polling instead of SOF interrupts to
fix Errata 2) was checking too much. As described above, the check for
the ACTIVE_BIT must not be done.
In the current implementation we still run into hanging unhandled urbs
because they sometimes doesn't get both bits toggled. (V==0 && A==0)
After solving this, it is easy to change the polling function back to
always enabled interrupts. In fact the enabling of SOF Interrupts
doesn't have anything else in mind than to guarantee that we never run
out of interrupts. That actually is nothing else then polling in
hardware. Instead of doing it with a timer function.
> Do you have a specific bug that this patch solves for you? Then please describe the bug in
> detail so that we can try to find a solution.
Yes i try to solve the errata2 issue which is still occurs with the
current mainline implementation.
cheers,
Michael
> > Signed-off-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx>
> > ---
> > drivers/usb/host/isp1760-hcd.c | 110 +++++++++++++++++-----------------------
> > drivers/usb/host/isp1760-hcd.h | 1 +
> > 2 files changed, 48 insertions(+), 63 deletions(-)
> >
> > diff --git a/drivers/usb/host/isp1760-hcd.c b/drivers/usb/host/isp1760-hcd.c
> > index 90c57dc..4beedba 100644
> > --- a/drivers/usb/host/isp1760-hcd.c
> > +++ b/drivers/usb/host/isp1760-hcd.c
> > @@ -48,6 +48,8 @@ struct isp1760_hcd {
> > int int_done_map;
> > struct memory_chunk memory_pool[BLOCKS];
> > struct list_head qh_list[QH_END];
> > + int active_ptds;
> > + int last_active_ptds;
> >
> > /* periodic schedule support */
> > #define DEFAULT_I_TDPS 1024
> > @@ -512,6 +514,11 @@ 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;
> > + priv->last_active_ptds = 0;
> > +
> > /* ATL reset */
> > reg_write32(hcd->regs, HC_HW_MODE_CTRL, hwmode | ALL_ATX_RESET);
> > mdelay(10);
> > @@ -775,6 +782,7 @@ static void start_bus_transfer(struct usb_hcd *hcd, u32 ptd_offset, int slot,
> > slots[slot].qtd = qtd;
> > slots[slot].qh = qh;
> > ptd_write(hcd->regs, ptd_offset, slot, ptd);
> > + priv->active_ptds++;
> >
> > if (ptd_offset == ATL_PTD_OFFSET) {
> > skip_map = reg_read32(hcd->regs, HC_ATL_PTD_SKIPMAP_REG);
> > @@ -1147,6 +1155,7 @@ static void handle_done_ptds(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);
> > @@ -1228,10 +1237,13 @@ static void handle_done_ptds(struct usb_hcd *hcd)
> > schedule_ptds(hcd);
> > }
> >
> > +#define SLOT_TIMEOUT 300
> > static irqreturn_t isp1760_irq(struct usb_hcd *hcd)
> > {
> > struct isp1760_hcd *priv = hcd_to_priv(hcd);
> > u32 imask;
> > + int slot;
> > + struct ptd ptd;
> > irqreturn_t irqret = IRQ_NONE;
> >
> > spin_lock(&priv->lock);
> > @@ -1247,71 +1259,50 @@ static irqreturn_t isp1760_irq(struct usb_hcd *hcd)
> > 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:
> > - spin_unlock(&priv->lock);
> > -
> > - 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 300
> > -#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);
> > + /* Errata2: A time-out can be implemented and if a certain Done bit is
> > + never set, verification of the PTD completion is done by reading PTD
> > + contents (valid bit). This is a proven workaround implemented in
> > + software. */
> >
> > for (slot = 0; slot < 32; slot++)
> > if (priv->atl_slots[slot].qh && time_after(jiffies,
> > priv->atl_slots[slot].timestamp +
> > - SLOT_TIMEOUT * HZ / 1000)) {
> > + msecs_to_jiffies(SLOT_TIMEOUT)) &&
> > + !(priv->atl_done_map & (1 << slot))) {
> > ptd_read(hcd->regs, ATL_PTD_OFFSET, slot, &ptd);
> > - if (!FROM_DW0_VALID(ptd.dw0) &&
> > - !FROM_DW3_ACTIVE(ptd.dw3))
> > +
> > + priv->atl_slots[slot].timestamp = jiffies;
> > + if (!FROM_DW0_VALID(ptd.dw0)) {
> > priv->atl_done_map |= 1 << slot;
> > + dev_info(hcd->self.controller, "errata2 issue!\n");
> > + continue;
> > + }
> > }
> >
> > - if (priv->atl_done_map)
> > - handle_done_ptds(hcd);
> >
> > - spin_unlock_irqrestore(&priv->lock, spinflags);
> > + handle_done_ptds(hcd);
> >
> > - errata2_timer.expires = jiffies + SLOT_CHECK_PERIOD * HZ / 1000;
> > - add_timer(&errata2_timer);
> > + /* 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. */
> > +
> > + if (priv->active_ptds != priv->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);
> > + priv->last_active_ptds = priv->active_ptds;
> > + }
> > +
> > + irqret = IRQ_HANDLED;
> > +leave:
> > + spin_unlock(&priv->lock);
> > +
> > + return irqret;
> > }
> >
> > static int isp1760_run(struct usb_hcd *hcd)
> > @@ -1359,12 +1350,6 @@ 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);
> > @@ -1627,6 +1612,7 @@ static void kill_transfer(struct usb_hcd *hcd, struct urb *urb,
> > }
> >
> > qh->slot = -1;
> > + priv->active_ptds--;
> > }
> >
> > /*
> > @@ -2113,8 +2099,6 @@ 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 33dc79c..b20ae45 100644
> > --- a/drivers/usb/host/isp1760-hcd.h
> > +++ b/drivers/usb/host/isp1760-hcd.h
> > @@ -74,6 +74,7 @@ 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
>
>
> --
> Arvid Brodin
> Enea Services Stockholm AB - since February 16 a part of Xdin in the Alten Group. Soon we
> will be working under the common brand name Xdin. Read more at www.xdin.com.--
> 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
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
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
