drivers/usbhost: Fix xHCI faults on a conforming controller.

drivers/usbhost/usbhost_xhci.h

@@ -268,6 +268,15 @@
 #define XHCI_PORTSC_DR               (1 << 30)             /* Bit 30: Device Removable */
 #define XHCI_PORTSC_WPR              (1 << 31)             /* Bit 31: Warm Port Reset */
 
+/* The write-one-to-clear bits of PORTSC.  Mask these out of any
+ * read-modify-write of the register, unless clearing them is intended.
+ */
+
+#define XHCI_PORTSC_RW1C             (XHCI_PORTSC_PED | XHCI_PORTSC_CSC | \
+                                      XHCI_PORTSC_PEC | XHCI_PORTSC_WRC | \
+                                      XHCI_PORTSC_OCC | XHCI_PORTSC_PRC | \
+                                      XHCI_PORTSC_PLC | XHCI_PORTSC_CEC)
+
 /* Port Power Management Status and Control (USB3) */
 
 #define XHCI_PORTPMSC_U1TO_SHIFT     (0)                   /* Bits 0-7: U1 Timeout */

drivers/usbhost/usbhost_xhci_pci.c

@@ -68,6 +68,16 @@
 #define XHCI_CMD_MAX             (16)
 #define XHCI_EVENT_MAX           (232)
 #define XHCI_TD_MAX              (8)
+
+/* Milliseconds allowed for the controller to halt.  The specification
+ * asks for 16.
+ */
+
+#define XHCI_HALT_TIMEOUT_MS     (100)
+
+/* Milliseconds allowed for a port to enable after reset. */
+
+#define XHCI_PORT_RESET_MS       (500)
 #define XHCI_BUFSIZE             (512)
 
 /* Port numbers macros */
@@ -519,6 +529,12 @@ static struct pci_driver_s g_pci_xhci_drv =
  * Private Functions
  ****************************************************************************/
 
+/* xHCI requires aligned accesses of each register's own size, and
+ * narrower ones may be ignored.  volatile does not pin the access width,
+ * so every accessor below launders the value through a register with an
+ * empty asm, on loads and stores both, to force the full-width access.
+ */
+
 /****************************************************************************
  * Name: xhci_capa_getreg
  *
@@ -530,8 +546,11 @@ static struct pci_driver_s g_pci_xhci_drv =
 static uint32_t xhci_capa_getreg(FAR struct usbhost_xhci_s *priv,
                                  unsigned int offset)
 {
-  uintptr_t addr = priv->capa_base + offset;
-  return *((FAR volatile uint32_t *)addr);
+  uintptr_t addr   = priv->capa_base + offset;
+  uint32_t  regval = *((FAR volatile uint32_t *)addr);
+
+  __asm__ __volatile__("" : "+r"(regval));
+  return regval;
 }
 
 /****************************************************************************
@@ -545,8 +564,11 @@ static uint32_t xhci_capa_getreg(FAR struct usbhost_xhci_s *priv,
 static uint8_t xhci_capa_getreg_1b(FAR struct usbhost_xhci_s *priv,
                                    unsigned int offset)
 {
-  uintptr_t addr = priv->capa_base + offset;
-  return *((FAR volatile uint8_t *)addr);
+  uintptr_t addr   = priv->capa_base + offset;
+  uint8_t   regval = *((FAR volatile uint8_t *)addr);
+
+  __asm__ __volatile__("" : "+r"(regval));
+  return regval;
 }
 
 /****************************************************************************
@@ -562,6 +584,8 @@ static void xhci_capa_putreg_1b(FAR struct usbhost_xhci_s *priv,
                                 uint8_t value)
 {
   uintptr_t addr = priv->capa_base + offset;
+
+  __asm__ __volatile__("" : "+r"(value));
   *((FAR volatile uint8_t *)addr) = value;
 }
 
@@ -576,8 +600,11 @@ static void xhci_capa_putreg_1b(FAR struct usbhost_xhci_s *priv,
 static uint32_t xhci_oper_getreg(FAR struct usbhost_xhci_s *priv,
                                  unsigned int offset)
 {
-  uintptr_t addr = priv->oper_base + offset;
-  return *((FAR volatile uint32_t *)addr);
+  uintptr_t addr   = priv->oper_base + offset;
+  uint32_t  regval = *((FAR volatile uint32_t *)addr);
+
+  __asm__ __volatile__("" : "+r"(regval));
+  return regval;
 }
 
 /****************************************************************************
@@ -593,6 +620,8 @@ static void xhci_oper_putreg(FAR struct usbhost_xhci_s *priv,
                              uint32_t value)
 {
   uintptr_t addr = priv->oper_base + offset;
+
+  __asm__ __volatile__("" : "+r"(value));
   *((FAR volatile uint32_t *)addr) = value;
 }
 
@@ -609,6 +638,8 @@ static void xhci_oper_putreg_8b(FAR struct usbhost_xhci_s *priv,
                                 uint64_t value)
 {
   uintptr_t addr = priv->oper_base + offset;
+
+  __asm__ __volatile__("" : "+r"(value));
   *((FAR volatile uint64_t *)addr) = value;
 }
 
@@ -623,8 +654,11 @@ static void xhci_oper_putreg_8b(FAR struct usbhost_xhci_s *priv,
 static uint32_t xhci_runt_getreg(FAR struct usbhost_xhci_s *priv,
                                  unsigned int offset)
 {
-  uintptr_t addr = priv->runt_base + offset;
-  return *((FAR volatile uint32_t *)addr);
+  uintptr_t addr   = priv->runt_base + offset;
+  uint32_t  regval = *((FAR volatile uint32_t *)addr);
+
+  __asm__ __volatile__("" : "+r"(regval));
+  return regval;
 }
 
 /****************************************************************************
@@ -640,6 +674,8 @@ static void xhci_runt_putreg(FAR struct usbhost_xhci_s *priv,
                              uint32_t value)
 {
   uintptr_t addr = priv->runt_base + offset;
+
+  __asm__ __volatile__("" : "+r"(value));
   *((FAR volatile uint32_t *)addr) = value;
 }
 
@@ -656,6 +692,8 @@ static void xhci_runt_putreg_8b(FAR struct usbhost_xhci_s *priv,
                                 uint64_t value)
 {
   uintptr_t addr = priv->runt_base + offset;
+
+  __asm__ __volatile__("" : "+r"(value));
   *((FAR volatile uint64_t *)addr) = value;
 }
 
@@ -672,6 +710,8 @@ static void xhci_door_putreg(FAR struct usbhost_xhci_s *priv,
                              uint32_t value)
 {
   uintptr_t addr = priv->door_base + offset;
+
+  __asm__ __volatile__("" : "+r"(value));
   *((FAR volatile uint32_t *)addr) = value;
 }
 
@@ -1074,9 +1114,12 @@ static int xhci_ctrl_start(FAR struct usbhost_xhci_s *priv)
 
   xhci_oper_putreg(priv, XHCI_CONFIG, priv->no_slots);
 
-  /* Slot 0 in Device Context is reserved for Scratchpad Buffer Array */
+  /* Slot 0 of the Device Context array points at the Scratchpad Buffer
+   * Array, or is zero when the controller asked for none.
+   */
 
-  priv->pg_ctx[0] = htole64(up_addrenv_va_to_pa(priv->pg_sb));
+  priv->pg_ctx[0] = priv->pg_sb ?
+                    htole64(up_addrenv_va_to_pa(priv->pg_sb)) : 0;
 
   /* Device Context Base Address Array Pointer */
 
@@ -1197,27 +1240,41 @@ static int xhci_ctrl_start(FAR struct usbhost_xhci_s *priv)
 
 static int xhci_ctrl_halt(FAR struct usbhost_xhci_s *priv)
 {
-  int ret = -EAGAIN;
-  int i;
+  uint32_t regval;
+  int      i;
 
-  /* Halt controller */
+  /* A controller that was never started is already halted and says so.
+   * There is no transition to wait for, so check before waiting.
+   */
+
+  regval = xhci_oper_getreg(priv, XHCI_USBSTS);
+  if ((regval & XHCI_USBSTS_HCH) != 0)
+    {
+      return OK;
+    }
 
-  xhci_oper_putreg(priv, XHCI_USBCMD, 0);
+  /* Clear Run/Stop and leave the rest of the register alone.  Writing the
+   * whole of it zero would clear the interrupt and host system error
+   * enables along with it.
+   */
 
-  /* Wait for controller halted */
+  regval  = xhci_oper_getreg(priv, XHCI_USBCMD);
+  regval &= ~XHCI_USBCMD_RS;
+  xhci_oper_putreg(priv, XHCI_USBCMD, regval);
 
-  for (i = 0; i < 10; i++)
+  for (i = 0; i < XHCI_HALT_TIMEOUT_MS; i++)
     {
-      up_mdelay(100);
-
-      if (xhci_oper_getreg(priv, XHCI_USBSTS) & XHCI_USBSTS_HCH)
+      regval = xhci_oper_getreg(priv, XHCI_USBSTS);
+      if ((regval & XHCI_USBSTS_HCH) != 0)
         {
-          ret = OK;
-          break;
+          return OK;
         }
+
+      up_udelay(1000);
     }
 
-  return ret;
+  pcierr("controller will not halt, USBSTS %08" PRIx32 "\n", regval);
+  return -EAGAIN;
 }
 
 /****************************************************************************
@@ -1307,26 +1364,38 @@ static int xhci_port_enable(FAR struct usbhost_xhci_s *priv,
 
   if (!(regval & XHCI_PORTSC_PED))
     {
-      /* Reset the port */
+      /* Reset the port, masking the write-one-to-clear bits out of the
+       * value first.  See XHCI_PORTSC_RW1C.
+       */
 
-      regval = xhci_oper_getreg(priv, XHCI_PORTSC(rhpndx));
+      regval  = xhci_oper_getreg(priv, XHCI_PORTSC(rhpndx));
+      regval &= ~XHCI_PORTSC_RW1C;
       regval |= XHCI_PORTSC_PR;
       xhci_oper_putreg(priv, XHCI_PORTSC(rhpndx), regval);
 
       /* REVISIT: we get Port Status Change Event here */
 
       /* Wait for Enabled state for port */
 
-      retries = 10;
-      while (!(xhci_oper_getreg(priv, XHCI_PORTSC(rhpndx))
-               & XHCI_PORTSC_PED) && retries > 0)
+      for (retries = XHCI_PORT_RESET_MS; retries > 0; retries--)
         {
-          retries--;
-          up_mdelay(100);
+          regval = xhci_oper_getreg(priv, XHCI_PORTSC(rhpndx));
+          if ((regval & XHCI_PORTSC_PED) != 0)
+            {
+              break;
+            }
+
+          up_mdelay(1);
         }
 
-      if (retries == 0)
+      /* Test the port, not the counter: a port that comes up on the last
+       * attempt leaves the loop with the count exhausted too.
+       */
+
+      if ((regval & XHCI_PORTSC_PED) == 0)
         {
+          pcierr("port %d will not enable, PORTSC %08" PRIx32 "\n", rhpndx,
+                 regval);
           return -ETIMEDOUT;
         }
     }
@@ -1869,7 +1938,15 @@ static int xhci_command(FAR struct usbhost_xhci_s *priv,
   trb->d1 = priv->cmdres.d1;
   trb->d2 = priv->cmdres.d2;
 
-  if (XHCI_TRB_D1_CC_GET(trb->d1) != XHCI_TRB_CC_SUCCESS)
+  if (XHCI_TRB_D1_CC_GET(trb->d1) == XHCI_TRB_CC_SUCCESS)
+    {
+      /* The completion event decides the result, whether it arrived by
+       * interrupt or was found by the poll above.
+       */
+
+      ret = OK;
+    }
+  else
     {
       pcierr("event CC = %d\n", XHCI_TRB_D1_CC_GET(trb->d1));
       ret = -EIO;
@@ -4335,17 +4412,22 @@ static int xhci_mem_alloc(FAR struct usbhost_xhci_s *priv)
   size_t tmp;
   int    i;
 
-  /* Allocate Scratchpad Buffer Array */
+  /* Allocate the Scratchpad Buffer Array, if one is wanted.  no_scratch
+   * may be zero, and a zero byte allocation returns NULL.
+   */
 
-  tmp = priv->no_scratch * sizeof(uint64_t);
-  priv->pg_sb = kmm_memalign(XHCI_BUF_ALIGN, tmp);
-  if (!priv->pg_sb)
+  if (priv->no_scratch > 0)
     {
-      pcierr("pg_sb malloc failed\n");
-      return -ENOMEM;
-    }
+      tmp = priv->no_scratch * sizeof(uint64_t);
+      priv->pg_sb = kmm_memalign(XHCI_BUF_ALIGN, tmp);
+      if (!priv->pg_sb)
+        {
+          pcierr("pg_sb malloc failed\n");
+          return -ENOMEM;
+        }
 
-  memset(priv->pg_sb, 0, tmp);
+      memset(priv->pg_sb, 0, tmp);
+    }
 
   for (i = 0; i < priv->no_scratch; i++)
     {