Re: EHCI driver

[Christer Weinigel]

On 2012-05-30 11:28, Christer Weinigel wrote:

> [talk about EHCI problems with low speed split transactions]

> Now I just have to fix up the queue management so that it properly keeps
> track of QHs for two queues instead of one. And figure out if there are
> any other problems.


So, how about something like this?

Fix low-speed USB split transactions on EHCI

Low-speed split transactions did not work on EHCI.  The reason is that
only control and interrupt transfers are allowed with low-speed, there
is no such thing as bulk transfers for low-speed.  GRUB doesn't know
about interrupt transfers, it always uses bulk transfers, and the EHCI
driver did not support interrupt transfers either.

It seems that when doing split transactions from a QH (queue header)
on the asynchronous schedule in the EHCI controller, the SPLIT packet
on the bus will be for a full-speed transfer, even if the QH says
that it is a low-speed transfer.  To actually get a low-speed transfer
the QH has to be on the periodic schedule.

This patch adds a hack that checks if a bulk transfer is for a
low-speed device and in that case puts the QH on the periodic schedule
instead of on the asynchronous schedule.  Checking for bulk transfers
to a low speed device is a rather ugly hack, it would be better to
implement proper support for interrupt transfers in GRUB.  But this
made my keyboard work with minimal changes.

The same queue of transfers is used for all 1024 frames in the periodic
schedule, so the interrupt transfer will be retried every frame, there
is no support for longer poll times.  I don't know if that is a problem
in practice, but I could imagine that some slow device might choke
if the host polls it too often.

To be able to use the same pool of QHs for both the periodic and the
asynchronous schedules the way QHs are allocated had to change a bit.
Instead of qh_virt[i-1] always being followed by qh_virt[i], they
are now proper linked lists.  When cancelling a transfer we have
to look through the list for the previous QH instead of just using
qh_virt[i-1].  On the other hand, cancelling happens very seldom
so this doesn't really affect performance.

The declarations of GRUB_EHCI_MULT_ONE, TWO and THREE were all zero
which is explicitly disallowed by the EHCI specification:
"A zero in this field yields undefined results."
I changed the defines to have the correct values.

BTW, there is one really strange issue when I have multiple keyboards 
For example if I have three keyboards, with devices addresses 2, 3, and 
4.  If I unplug and replug keyboard 3, this for some reason sets the
halted flags or clears the active flags for keyboards 2 and 4.  The 
alternate code selected by #if in grub_ehci_cancel_transfer makes things
work, but I have no idea why which is a bit frightening.

Note, this change is against some old copy of ehci.c from a few months
ago, so it probably won't apply to the latest code in bazaar.  But if
you think this looks good, I can figure out how to check out the
bazaar repo again and make a new patch.

I'm trying to use thunderbird to send this patch, so it might be a bit 
mangled, but hopefully I'm managed to get it right.

  /Christer

diff --git a/grub-core/bus/usb/ehci.c b/grub-core/bus/usb/ehci.c
index 0f41361..d8ecf26 100644
--- a/grub-core/bus/usb/ehci.c
+++ b/grub-core/bus/usb/ehci.c
@@ -209,18 +209,22 @@ enum
 {
   GRUB_EHCI_MULT_MASK = (3 << 30),
   GRUB_EHCI_MULT_RESERVED = (0 << 30),
-  GRUB_EHCI_MULT_ONE = (0 << 30),
-  GRUB_EHCI_MULT_TWO = (0 << 30),
-  GRUB_EHCI_MULT_THREE = (0 << 30),
+  GRUB_EHCI_MULT_ONE = (1 << 30),
+  GRUB_EHCI_MULT_TWO = (2 << 30),
+  GRUB_EHCI_MULT_THREE = (3 << 30),
   GRUB_EHCI_DEVPORT_MASK = (0x7f << 23),
-  GRUB_EHCI_HUBADDR_MASK = (0x7f << 16)
+  GRUB_EHCI_HUBADDR_MASK = (0x7f << 16),
+  GRUB_EHCI_CMASK_MASK = (0xff << 8),
+  GRUB_EHCI_SMASK_MASK = (0xff << 0),
 };
 
 enum
 {
   GRUB_EHCI_MULT_OFF = 30,
   GRUB_EHCI_DEVPORT_OFF = 23,
-  GRUB_EHCI_HUBADDR_OFF = 16
+  GRUB_EHCI_HUBADDR_OFF = 16,
+  GRUB_EHCI_CMASK_OFF = 8,
+  GRUB_EHCI_SMASK_OFF = 0,
 };
 
 #define GRUB_EHCI_TERMINATE      (1<<0)
@@ -782,6 +786,7 @@ grub_ehci_pci_iter (grub_pci_device_t dev,
   /* Enable asynchronous list */
   grub_ehci_oper_write32 (e, GRUB_EHCI_COMMAND,
                          GRUB_EHCI_CMD_AS_ENABL
+                         | GRUB_EHCI_CMD_PS_ENABL
                          | grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND));
 
   /* Now should be possible to power-up and enumerate ports etc. */
@@ -926,6 +931,12 @@ grub_ehci_setup_qh (grub_ehci_qh_t qh, grub_usb_transfer_t 
transfer)
     & GRUB_EHCI_DEVPORT_MASK;
   ep_cap |= (transfer->dev->hubaddr << GRUB_EHCI_HUBADDR_OFF)
     & GRUB_EHCI_HUBADDR_MASK;
+  if (transfer->dev->speed == GRUB_USB_SPEED_LOW &&
+      transfer->type != GRUB_USB_TRANSACTION_TYPE_CONTROL)
+  {
+    ep_cap |= (1<<0) << GRUB_EHCI_SMASK_OFF;
+    ep_cap |= (7<<2) << GRUB_EHCI_CMASK_OFF;
+  }
   qh->ep_cap = grub_cpu_to_le32 (ep_cap);
 
   grub_dprintf ("ehci", "setup_qh: qh=%08x, not changed: qh_hptr=%08x\n",
@@ -949,6 +960,7 @@ grub_ehci_find_qh (struct grub_ehci *e, grub_usb_transfer_t 
transfer)
   grub_uint32_t target, mask;
   int i;
   grub_ehci_qh_t qh = e->qh_virt;
+  grub_ehci_qh_t head;
 
   /* Prepare part of EP Characteristic to find existing QH */
   target = ((transfer->endpoint << GRUB_EHCI_EP_NUM_OFF) |
@@ -983,14 +995,21 @@ grub_ehci_find_qh (struct grub_ehci *e, 
grub_usb_transfer_t transfer)
    * de-allocate QHs of unplugged devices. */
   /* We should preset new QH and link it into AL */
   grub_ehci_setup_qh (&qh[i], transfer);
-  /* Linking - this new (last) QH will point to first QH */
-  qh[i].qh_hptr = grub_cpu_to_le32 (GRUB_EHCI_HPTR_TYPE_QH
-                                   | grub_dma_virt2phys (&qh[1],
-                                                         e->qh_chunk));
-  /* Linking - previous last QH will point to this new QH */
-  qh[i - 1].qh_hptr = grub_cpu_to_le32 (GRUB_EHCI_HPTR_TYPE_QH
-                                       | grub_dma_virt2phys (&qh[i],
-                                                             e->qh_chunk));
+
+  /* low speed interrupt transfers are linked to the periodic
+   * scheudle, everything else to the asynchronous schedule */
+  if (transfer->dev->speed == GRUB_USB_SPEED_LOW &&
+      transfer->type != GRUB_USB_TRANSACTION_TYPE_CONTROL)
+      head = &qh[0];
+  else
+      head = &qh[1];
+
+  /* Linking - this new (last) QH will copy the QH from the head QH */
+  qh[i].qh_hptr = head->qh_hptr;
+  /* Linking - the head QH will point to this new QH */
+  head->qh_hptr = grub_cpu_to_le32 (GRUB_EHCI_HPTR_TYPE_QH
+                                    | grub_dma_virt2phys (&qh[i],
+                                                          e->qh_chunk));
 
   return &qh[i];
 }
@@ -1221,7 +1240,7 @@ grub_ehci_setup_transfer (grub_usb_controller_t dev,
     /* XXX: Fix it: Currently we don't do anything to restart EHCI */
     return GRUB_USB_ERR_INTERNAL;
   if ((grub_ehci_oper_read32 (e, GRUB_EHCI_STATUS)
-       & GRUB_EHCI_ST_AS_STATUS) == 0)
+       & (GRUB_EHCI_ST_AS_STATUS | GRUB_EHCI_ST_PS_STATUS)) == 0)
     /* XXX: Fix it: Currently we don't do anything to restart EHCI */
     return GRUB_USB_ERR_INTERNAL;
 
@@ -1372,6 +1391,7 @@ grub_ehci_parse_notrun (grub_usb_controller_t dev,
   /* Try enable EHCI and AL */
   grub_ehci_oper_write32 (e, GRUB_EHCI_COMMAND,
                          GRUB_EHCI_CMD_RUNSTOP | GRUB_EHCI_CMD_AS_ENABL
+                         | GRUB_EHCI_CMD_PS_ENABL
                          | grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND));
   /* Ensure command is written */
   grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND);
@@ -1470,7 +1490,7 @@ grub_ehci_check_transfer (grub_usb_controller_t dev,
        & GRUB_EHCI_ST_HC_HALTED) != 0)
     return grub_ehci_parse_notrun (dev, transfer, actual);
   if ((grub_ehci_oper_read32 (e, GRUB_EHCI_STATUS)
-       & GRUB_EHCI_ST_AS_STATUS) == 0)
+       & (GRUB_EHCI_ST_AS_STATUS | GRUB_EHCI_ST_PS_STATUS)) == 0)
     return grub_ehci_parse_notrun (dev, transfer, actual);
 
   token = grub_le_to_cpu32 (cdata->qh_virt->td_overlay.token);
@@ -1508,6 +1528,7 @@ grub_ehci_cancel_transfer (grub_usb_controller_t dev,
   grub_size_t actual;
   int i;
   grub_uint64_t maxtime;
+  grub_uint32_t qh_phys;
 
   /* QH can be active and should be de-activated and halted */
 
@@ -1518,7 +1539,7 @@ grub_ehci_cancel_transfer (grub_usb_controller_t dev,
   if (((grub_ehci_oper_read32 (e, GRUB_EHCI_STATUS)
        & GRUB_EHCI_ST_HC_HALTED) != 0) ||
       ((grub_ehci_oper_read32 (e, GRUB_EHCI_STATUS)
-       & GRUB_EHCI_ST_AS_STATUS) == 0))
+       & (GRUB_EHCI_ST_AS_STATUS | GRUB_EHCI_ST_PS_STATUS)) == 0))
     {
       grub_ehci_pre_finish_transfer (transfer);
       grub_ehci_free_tds (e, cdata->td_first_virt, transfer, &actual);
@@ -1530,44 +1551,83 @@ grub_ehci_cancel_transfer (grub_usb_controller_t dev,
 
   /* EHCI and AL are running. What to do?
    * Try to Halt QH via de-scheduling QH. */
-  /* Find index of current QH - we need previous QH, i.e. i-1 */
-  i = ((int) (e->qh_virt - cdata->qh_virt)) / sizeof (struct grub_ehci_qh);
+  /* Find index of previous QH */
+  qh_phys = grub_dma_virt2phys(cdata->qh_virt, e->qh_chunk);
+  for (i = 0; i < GRUB_EHCI_N_QH; i++)
+    {
+      if ((e->qh_virt[i].qh_hptr & GRUB_EHCI_QHTDPTR_MASK) == qh_phys)
+        break;
+    }
+  if (i == GRUB_EHCI_N_QH)
+    {
+      grub_printf ("%s: prev not found, queues are corrupt\n", __func__);
+      return GRUB_USB_ERR_UNRECOVERABLE;
+    }
   /* Unlink QH from AL */
-  e->qh_virt[i - 1].qh_hptr = cdata->qh_virt->qh_hptr;
-  /* Ring the doorbell */
-  grub_ehci_oper_write32 (e, GRUB_EHCI_COMMAND,
-                         GRUB_EHCI_CMD_AS_ADV_D
-                         | grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND));
-  /* Ensure command is written */
-  grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND);
-  /* Wait answer with timeout */
-  maxtime = grub_get_time_ms () + 2;
-  while (((grub_ehci_oper_read32 (e, GRUB_EHCI_STATUS)
-          & GRUB_EHCI_ST_AS_ADVANCE) == 0)
-        && (grub_get_time_ms () < maxtime));
+  e->qh_virt[i].qh_hptr = cdata->qh_virt->qh_hptr;
+
+  /* If this is an interrupt transfer, we just wait for the periodic
+   * schedule to advance a few times and then assume that the EHCI
+   * controller has read the updated QH. */
+  if (cdata->qh_virt->ep_cap & GRUB_EHCI_SMASK_MASK)
+    {
+      grub_millisleep(20);
+    }
+  else
+    {
+      /* For the asynchronous schedule we use the advance doorbell to find
+       * out when the EHCI controller has read the updated QH. */
 
-  /* We do not detect the timeout because if timeout occurs, it most
-   * probably means something wrong with EHCI - maybe stopped etc. */
+      /* Ring the doorbell */
+      grub_ehci_oper_write32 (e, GRUB_EHCI_COMMAND,
+                              GRUB_EHCI_CMD_AS_ADV_D
+                              | grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND));
+      /* Ensure command is written */
+      grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND);
+      /* Wait answer with timeout */
+      maxtime = grub_get_time_ms () + 2;
+      while (((grub_ehci_oper_read32 (e, GRUB_EHCI_STATUS)
+               & GRUB_EHCI_ST_AS_ADVANCE) == 0)
+             && (grub_get_time_ms () < maxtime));
 
-  /* Shut up the doorbell */
-  grub_ehci_oper_write32 (e, GRUB_EHCI_COMMAND,
-                         ~GRUB_EHCI_CMD_AS_ADV_D
-                         & grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND));
-  grub_ehci_oper_write32 (e, GRUB_EHCI_STATUS,
-                         GRUB_EHCI_ST_AS_ADVANCE
-                         | grub_ehci_oper_read32 (e, GRUB_EHCI_STATUS));
-  /* Ensure command is written */
-  grub_ehci_oper_read32 (e, GRUB_EHCI_STATUS);
+      /* We do not detect the timeout because if timeout occurs, it most
+       * probably means something wrong with EHCI - maybe stopped etc. */
+
+      /* Shut up the doorbell */
+      grub_ehci_oper_write32 (e, GRUB_EHCI_COMMAND,
+                              ~GRUB_EHCI_CMD_AS_ADV_D
+                              & grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND));
+      grub_ehci_oper_write32 (e, GRUB_EHCI_STATUS,
+                              GRUB_EHCI_ST_AS_ADVANCE
+                              | grub_ehci_oper_read32 (e, GRUB_EHCI_STATUS));
+      /* Ensure command is written */
+      grub_ehci_oper_read32 (e, GRUB_EHCI_STATUS);
+    }
 
   /* Now is QH out of AL and we can do anything with it... */
   grub_ehci_pre_finish_transfer (transfer);
   grub_ehci_free_tds (e, cdata->td_first_virt, transfer, &actual);
   grub_ehci_free_td (e, cdata->td_alt_virt);
 
+  /* FIXME Putting the QH back on the list should work, but for some
+   * strange reason doing that will affect other QHs on the periodic
+   * list.  So free the QH instead of putting it back on the list
+   * which does seem to work, but I would like to know why. */
+
+#if 0
   /* Finaly we should return QH back to the AL... */
-  e->qh_virt[i - 1].qh_hptr =
+  e->qh_virt[i].qh_hptr =
     grub_cpu_to_le32 (grub_dma_virt2phys
                      (cdata->qh_virt, e->qh_chunk));
+#else
+  /* Free the QH */
+  cdata->qh_virt->ep_char = 0;
+  cdata->qh_virt->qh_hptr =
+    grub_cpu_to_le32 ((grub_dma_virt2phys (cdata->qh_virt,
+                                           e->qh_chunk) &
+                       GRUB_EHCI_POINTER_MASK) | GRUB_EHCI_HPTR_TYPE_QH);
+#endif
+
   grub_free (cdata);
 
   grub_dprintf ("ehci", "cancel_transfer: end\n");
@@ -1777,6 +1837,7 @@ grub_ehci_restore_hw (void)
       /* Enable asynchronous list */
       grub_ehci_oper_write32 (e, GRUB_EHCI_COMMAND,
                              GRUB_EHCI_CMD_AS_ENABL
+                             | GRUB_EHCI_CMD_PS_ENABL
                              | grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND));
 
       /* Now should be possible to power-up and enumerate ports etc. */