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).
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.
> 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
