xiaoxiang781216 commented on code in PR #19745:
URL: https://github.com/apache/nuttx/pull/19745#discussion_r3741301934
##########
drivers/usbhost/usbhost_xhci_pci.c:
##########
@@ -18,480 +20,68 @@
*
****************************************************************************/
+/* Finding an xHCI controller on a PCI bus.
+ *
+ * The controller itself is described by its specification and driven by
+ * usbhost_xhci.c, which is the same code wherever the part is fitted. This
+ * file is only the part that is true of PCI and of nothing else: which
+ * device identifiers to answer to, how to switch a device on and find its
+ * register window, and how its interrupt is arranged.
+ */
+
/****************************************************************************
* Included Files
****************************************************************************/
#include <nuttx/config.h>
#include <assert.h>
-#include <nuttx/debug.h>
+#include <debug.h>
Review Comment:
why change
##########
drivers/usbhost/usbhost_xhci.c:
##########
@@ -5157,11 +5208,186 @@ static int xhci_cancel(FAR struct usbhost_driver_s
*drvr, usbhost_ep_t ep)
****************************************************************************/
#ifdef CONFIG_USBHOST_HUB
+/****************************************************************************
+ * Name: xhci_rhport_from_hport
+ *
+ * Description:
+ * The root hub port a device descends from, however many hubs are in the
+ * way. The slot context names it, because that is the port the traffic
+ * physically leaves by.
+ *
+ ****************************************************************************/
+
+static FAR struct xhci_rhport_s *
+xhci_rhport_from_hport(FAR struct usbhost_xhci_s *priv,
+ FAR struct usbhost_hubport_s *hport)
+{
+ while (hport->parent != NULL)
+ {
+ hport = hport->parent;
+ }
+
+ return &priv->rhport[hport->port];
+}
+
+/****************************************************************************
+ * Name: xhci_hub_update
+ *
+ * Description:
+ * Tell the controller that a device is a hub, so that it will route to
+ * what is behind it.
+ *
+ * The slot was created before anyone knew: a hub is addressed and
+ * configured like any other device, and only then does its class driver
+ * read the descriptor saying how many ports it has. So the slot context
+ * is corrected here, the first time something appears behind it.
+ *
+ ****************************************************************************/
+
+static int xhci_hub_update(FAR struct usbhost_xhci_s *priv,
+ FAR struct usbhost_hubport_s *hubport)
+{
+ FAR struct xhci_slot_ctx_s *in;
+ FAR struct xhci_dev_s *dev;
+ uint64_t ctx;
+ int ret;
+
+ dev = xhci_dev_from_hport(priv, hubport);
+ if (dev == NULL || dev->ishub || hubport->nports == 0)
+ {
+ /* Nothing to correct: no slot for it, already done, or the hub class
+ * driver has not reported the descriptor.
+ */
+
+ return OK;
+ }
+
+ ret = nxmutex_lock(&priv->lock);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ /* Only the slot context changes, and it must go in carrying everything
+ * the controller already holds, so start from the output context it has
+ * been maintaining.
+ */
+
+ up_invalidate_dcache((uintptr_t)dev->ctx,
+ (uintptr_t)dev->ctx + XHCI_DEVCTX_SIZE(priv));
+
+ xhci_context_ctrl(priv, dev, 0, XHCI_IN_CTX1_A(XHCI_SLOT_FLAG));
+
+ in = xhci_in_slot(priv, dev->input);
+ in->ctx[0] = xhci_out_slot(dev->ctx)->ctx[0] | htole32(XHCI_ST_CTX0_HUB);
+ in->ctx[1] = (xhci_out_slot(dev->ctx)->ctx[1] &
+ ~htole32(XHCI_ST_CTX1_PORTS_MASK)) |
+ htole32(XHCI_ST_CTX1_PORTS_SET(hubport->nports));
+ in->ctx[2] = (xhci_out_slot(dev->ctx)->ctx[2] &
+ ~htole32(XHCI_ST_CTX2_TTT_MASK)) |
+ htole32(XHCI_ST_CTX2_TTT_SET(hubport->ttt));
+ in->ctx[3] = xhci_out_slot(dev->ctx)->ctx[3];
+
+ up_flush_dcache((uintptr_t)dev->input,
+ (uintptr_t)dev->input + XHCI_INCTX_SIZE(priv));
+
+ ctx = up_addrenv_va_to_pa(dev->input);
+
+ nxmutex_unlock(&priv->lock);
+
+ ret = xhci_cmd_cfgep(priv, dev->slot, ctx, false);
+ if (ret < 0)
+ {
+ uerr("failed to describe the hub on slot %d: %d\n", dev->slot, ret);
+ return ret;
+ }
+
+ dev->ishub = true;
+
+ syslog(LOG_INFO, "%s: port %d: hub with %d port%s\n",
Review Comment:
why use syslog, not uxxx
##########
drivers/usbhost/usbhost_xhci.c:
##########
@@ -3835,6 +3835,68 @@ static int xhci_ep0configure(FAR struct usbhost_driver_s
*drvr,
return ret;
}
+/****************************************************************************
+ * Name: xhci_interval
+ *
+ * Description:
+ * Work out the Interval an endpoint context wants.
+ *
+ * The field is an exponent: the controller services the endpoint every
+ * 2^Interval microframes. An endpoint descriptor does not say it that
+ * way, and what it does say depends on how fast the device is, so the
+ * number cannot simply be copied across.
+ *
+ * A low or full speed interrupt endpoint counts in frames, so its period
+ * is bInterval milliseconds, or bInterval * 8 microframes, and the
+ * exponent is the position of the highest bit of that. Everything else
+ * that is periodic already states an exponent, one greater than the one
+ * wanted here. Control and bulk endpoints are not periodic and the field
+ * means nothing to them.
+ *
+ ****************************************************************************/
+
+static uint8_t xhci_interval(uint8_t speed, uint8_t xfrtype,
+ uint8_t interval)
+{
+ unsigned int exp;
+
+ if (xfrtype != USB_EP_ATTR_XFER_INT && xfrtype != USB_EP_ATTR_XFER_ISOC)
+ {
+ return 0;
+ }
+
+ if ((speed == USB_SPEED_LOW || speed == USB_SPEED_FULL) &&
+ xfrtype == USB_EP_ATTR_XFER_INT)
+ {
+ /* Frames. Round down to a power of two, and keep it inside what the
+ * specification allows for this kind of endpoint: 2^3 microframes is
+ * one frame, 2^10 is 128 of them.
+ */
+
+ if (interval == 0)
+ {
+ interval = 1;
+ }
+
+ for (exp = 0; (1u << (exp + 1)) <= (unsigned int)interval * 8; exp++);
Review Comment:
remove the cast
##########
drivers/usbhost/usbhost_xhci.c:
##########
@@ -3835,6 +3835,68 @@ static int xhci_ep0configure(FAR struct usbhost_driver_s
*drvr,
return ret;
}
+/****************************************************************************
+ * Name: xhci_interval
+ *
+ * Description:
+ * Work out the Interval an endpoint context wants.
+ *
+ * The field is an exponent: the controller services the endpoint every
+ * 2^Interval microframes. An endpoint descriptor does not say it that
+ * way, and what it does say depends on how fast the device is, so the
+ * number cannot simply be copied across.
+ *
+ * A low or full speed interrupt endpoint counts in frames, so its period
+ * is bInterval milliseconds, or bInterval * 8 microframes, and the
+ * exponent is the position of the highest bit of that. Everything else
+ * that is periodic already states an exponent, one greater than the one
+ * wanted here. Control and bulk endpoints are not periodic and the field
+ * means nothing to them.
+ *
+ ****************************************************************************/
+
+static uint8_t xhci_interval(uint8_t speed, uint8_t xfrtype,
+ uint8_t interval)
+{
+ unsigned int exp;
+
+ if (xfrtype != USB_EP_ATTR_XFER_INT && xfrtype != USB_EP_ATTR_XFER_ISOC)
+ {
+ return 0;
+ }
+
+ if ((speed == USB_SPEED_LOW || speed == USB_SPEED_FULL) &&
+ xfrtype == USB_EP_ATTR_XFER_INT)
+ {
+ /* Frames. Round down to a power of two, and keep it inside what the
+ * specification allows for this kind of endpoint: 2^3 microframes is
+ * one frame, 2^10 is 128 of them.
+ */
+
+ if (interval == 0)
+ {
+ interval = 1;
+ }
+
+ for (exp = 0; (1u << (exp + 1)) <= (unsigned int)interval * 8; exp++);
+
+ if (exp < 3)
+ {
+ exp = 3;
+ }
+ else if (exp > 10)
+ {
+ exp = 10;
+ }
+
+ return (uint8_t)exp;
Review Comment:
remove the cast too
##########
drivers/usbhost/usbhost_xhci_pci.c:
##########
@@ -532,4391 +122,133 @@ static struct pci_driver_s g_pci_xhci_drv =
* Private Functions
****************************************************************************/
-/* Every register accessor below forces the value through a register with
- * an empty asm. Access width is part of the register interface: xHCI
- * requires aligned accesses of the register's own size, and a controller
- * may ignore anything narrower (QEMU's does). A volatile load does not
- * pin the width; GCC 16 at -Os narrows "load 32, test bit 0" to a byte
- * load. A value demanded in a register can only come from the full-width
- * access. The same constraint on stores stops a load-modify-store being
- * folded back into one instruction.
- */
-
-/****************************************************************************
- * Name: xhci_capa_getreg
- *
- * Description:
- * Get register (USB Legacy Support Capability)
- *
- ****************************************************************************/
-
-static uint32_t xhci_capa_getreg(FAR struct usbhost_xhci_s *priv,
- unsigned int offset)
-{
- uintptr_t addr = priv->capa_base + offset;
- uint32_t regval = *((FAR volatile uint32_t *)addr);
-
- __asm__ __volatile__("" : "+r"(regval));
- return regval;
-}
-
-/****************************************************************************
- * Name: xhci_capa_getreg_1b
- *
- * Description:
- * Get 1B register (USB Legacy Support Capability)
- *
- ****************************************************************************/
-
-static uint8_t xhci_capa_getreg_1b(FAR struct usbhost_xhci_s *priv,
- unsigned int offset)
-{
- uintptr_t addr = priv->capa_base + offset;
- uint8_t regval = *((FAR volatile uint8_t *)addr);
-
- __asm__ __volatile__("" : "+r"(regval));
- return regval;
-}
-
-/****************************************************************************
- * Name: xhci_capa_putreg_1b
- *
- * Description:
- * Put 1B register (USB Legacy Support Capability)
- *
- ****************************************************************************/
-
-static void xhci_capa_putreg_1b(FAR struct usbhost_xhci_s *priv,
- unsigned int offset,
- uint8_t value)
-{
- uintptr_t addr = priv->capa_base + offset;
-
- __asm__ __volatile__("" : "+r"(value));
- *((FAR volatile uint8_t *)addr) = value;
-}
-
-/****************************************************************************
- * Name: xhci_oper_getreg
- *
- * Description:
- * Get register (Host Controller Operational Registers)
- *
- ****************************************************************************/
-
-static uint32_t xhci_oper_getreg(FAR struct usbhost_xhci_s *priv,
- unsigned int offset)
-{
- uintptr_t addr = priv->oper_base + offset;
- uint32_t regval = *((FAR volatile uint32_t *)addr);
-
- __asm__ __volatile__("" : "+r"(regval));
- return regval;
-}
-
-/****************************************************************************
- * Name: xhci_oper_putreg
- *
- * Description:
- * Put register (Host Controller Operational Registers)
- *
- ****************************************************************************/
-
-static void xhci_oper_putreg(FAR struct usbhost_xhci_s *priv,
- unsigned int offset,
- uint32_t value)
-{
- uintptr_t addr = priv->oper_base + offset;
-
- __asm__ __volatile__("" : "+r"(value));
- *((FAR volatile uint32_t *)addr) = value;
-}
-
/****************************************************************************
- * Name: xhci_oper_putreg_8b
+ * Name: pci_xhci_irq_attach
*
* Description:
- * Put register (Host Controller Operational Registers)
+ * Give the controller an interrupt. On PCI that means asking for a
+ * message rather than finding a wire, so the vector is allocated here and
+ * only then attached.
*
****************************************************************************/
-static void xhci_oper_putreg_8b(FAR struct usbhost_xhci_s *priv,
- unsigned int offset,
- uint64_t value)
+static int pci_xhci_irq_attach(FAR void *arg, xcpt_t handler, FAR void *priv)
{
- uintptr_t addr = priv->oper_base + offset;
-
- __asm__ __volatile__("" : "+r"(value));
- *((FAR volatile uint64_t *)addr) = value;
-}
+ FAR struct pci_xhci_s *pcix = arg;
+ int ret;
-/****************************************************************************
- * Name: xhci_runt_getreg
- *
- * Description:
- * Get register (Host Controller Runtime Registers)
- *
- ****************************************************************************/
+ ret = pci_alloc_irq(pcix->dev, &pcix->irq, 1);
+ if (ret != 1)
+ {
+ pcierr("Failed to allocate MSI %d\n", ret);
+ return ret;
+ }
-static uint32_t xhci_runt_getreg(FAR struct usbhost_xhci_s *priv,
- unsigned int offset)
-{
- uintptr_t addr = priv->runt_base + offset;
- uint32_t regval = *((FAR volatile uint32_t *)addr);
+ irq_attach(pcix->irq, handler, priv);
- __asm__ __volatile__("" : "+r"(regval));
- return regval;
-}
+ ret = pci_connect_irq(pcix->dev, &pcix->irq, 1);
+ if (ret != OK)
+ {
+ pcierr("Failed to connect MSI %d\n", ret);
+ pci_release_irq(pcix->dev, &pcix->irq, 1);
-/****************************************************************************
- * Name: xhci_runt_putreg
- *
- * Description:
- * Put register (Host Controller Runtime Registers)
- *
- ****************************************************************************/
+ return -ENOTSUP;
+ }
-static void xhci_runt_putreg(FAR struct usbhost_xhci_s *priv,
- unsigned int offset,
- uint32_t value)
-{
- uintptr_t addr = priv->runt_base + offset;
+ up_enable_irq(pcix->irq);
- __asm__ __volatile__("" : "+r"(value));
- *((FAR volatile uint32_t *)addr) = value;
+ return OK;
}
/****************************************************************************
- * Name: xhci_runt_putreg_8b
- *
- * Description:
- * Put register (Host Controller Runtime Registers)
- *
+ * Name: pci_xhci_irq_detach
****************************************************************************/
-static void xhci_runt_putreg_8b(FAR struct usbhost_xhci_s *priv,
- unsigned int offset,
- uint64_t value)
+static void pci_xhci_irq_detach(FAR void *arg)
{
- uintptr_t addr = priv->runt_base + offset;
+ FAR struct pci_xhci_s *pcix = arg;
- __asm__ __volatile__("" : "+r"(value));
- *((FAR volatile uint64_t *)addr) = value;
+ pci_release_irq(pcix->dev, &pcix->irq, 1);
Review Comment:
do you need up_disable_irq
##########
drivers/usbhost/usbhost_xhci.c:
##########
@@ -5245,8 +5475,9 @@ static int xhci_hw_getparams(FAR struct usbhost_xhci_s
*priv)
priv->no_slots = CONFIG_USBHOST_XHCI_MAX_DEVS;
}
- uinfo("no slots = %d, no ports = %d\n",
- priv->no_slots, priv->no_ports);
+ syslog(LOG_INFO, "%s: %d root port%s, %d device slot%s\n",
Review Comment:
why change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
