On Thu, 2012-01-12 at 10:37 -0500, Alan Stern wrote:
> On Thu, 12 Jan 2012, Alex,Shi wrote:
>
> > Thanks, Sarah. I understand your concern now.
> > Maybe checking MSI first in usb_hcd_request_irqs is your favorite too,
> > right?
> >
> > I take a quick glance on the code. May we have 2 ways to implement it.
> >
> > A, move current xhci code and a few MSI related objects into usb_hcd
> > core code and structure. the skeleton like below.
>
> MSI is used only for PCI, right? Therefore we should try to keep as
> much as possible of the core MSI-related code in hcd-pci.c, not in
> hcd.c.
>
Considering all of your suggestions, I write a draft patch to enable
MSI/MSIX support in usb core. It base on my 2 patches that check MSI in
xhci driver.
The patch was tested with NEC uPD720200 usb 3.0 HCD and our Intel usb
3.0 pantherpoint, and can resume from S3 mode.
Now it just open the MSI checking for XHCI driver. but other driver is
easy to enable this capability by assign HCD_MSI_FIRST on driver's
flags.
Be leaking of enough knowledge of USB and hardware ENV, there are still
some problems didn't resolved, like how to get the max msix counter
during hcd pci probe? testing on some platforms without PCI bus.
But I know there should lots of suggestions and comments on the problems
and for this patch, So, sent it out. :)
diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index d6369a4..c42dcfe 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -153,6 +153,224 @@ static inline void wait_for_companions(struct pci_dev *d, struct usb_hcd *h) {}
/*-------------------------------------------------------------------------*/
+static int hcd_free_msix(struct usb_hcd *hcd)
+{
+ int i;
+ struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
+
+ if (!hcd->msix_entries) {
+ printk(KERN_ERR "No msix entries found!\n");
+ return -EINVAL;
+ }
+ for (i = 0; i < hcd->msix_count; i++)
+ if (hcd->msix_entries[i].vector)
+ free_irq(hcd->msix_entries[i].vector,
+ hcd);
+
+ pci_disable_msix(pdev);
+ kfree(hcd->msix_entries);
+ hcd->msix_entries = NULL;
+ hcd->msix_enabled = 0;
+
+ return 0;
+}
+static int hcd_free_msi(struct usb_hcd *hcd)
+{
+ struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
+
+ dev_dbg(&pdev->dev, "disable MSI interrupt\n");
+ pci_disable_msi(pdev);
+ return 0;
+}
+
+/* Free and disable msi/msix */
+void usb_hcd_free_msi_msix(struct usb_hcd *hcd)
+{
+ if (hcd->msix_entries)
+ hcd_free_msix(hcd);
+ else
+ hcd_free_msi(hcd);
+
+ return;
+}
+EXPORT_SYMBOL_GPL(usb_hcd_free_msi_msix);
+
+void usb_hcd_msix_sync_irqs(struct usb_hcd *hcd)
+{
+ int i;
+ if (hcd->msix_entries) {
+ for (i = 0; i < hcd->msix_count; i++)
+ synchronize_irq(hcd->msix_entries[i].vector);
+ }
+
+}
+EXPORT_SYMBOL_GPL(usb_hcd_msix_sync_irqs);
+
+/*
+ * Set up MSI
+ */
+static int hcd_setup_msi(struct pci_dev *pdev, struct usb_hcd *hcd)
+{
+ int ret;
+
+ ret = pci_enable_msi(pdev);
+ if (ret)
+ dev_dbg(&pdev->dev, "failed to allocate MSI entry\n");
+ return ret;
+}
+/*
+ * Set up MSI-X
+ */
+static int hcd_setup_msix(struct pci_dev *pdev, struct usb_hcd *hcd)
+{
+ int i, ret = 0;
+
+ /*
+ * calculate number of msi-x vectors supported.
+ * - HCS_MAX_INTRS: the max number of interrupts the host can handle,
+ * with max number of interrupters based on the xhci HCSPARAMS1.
+ * - num_online_cpus: maximum msi-x vectors per CPUs core.
+ * Add additional 1 vector to ensure always available interrupt.
+ *
+ * Don't know how to get the max MSIX number here, any help? --alex
+ hcd->msix_count = min(num_online_cpus() + 1,
+ HCS_MAX_INTRS(hcd->hcs_params1));
+ */
+ hcd->msix_count = 1;
+
+ hcd->msix_entries =
+ kmalloc((sizeof(struct msix_entry))*hcd->msix_count,
+ GFP_KERNEL);
+ if (!hcd->msix_entries) {
+ dev_err(&pdev->dev, "Failed to allocate MSI-X entries\n");
+ hcd->msix_count = 0;
+ return -ENOMEM;
+ }
+
+ for (i = 0; i < hcd->msix_count; i++) {
+ hcd->msix_entries[i].entry = i;
+ hcd->msix_entries[i].vector = 0;
+ }
+
+ ret = pci_enable_msix(pdev, hcd->msix_entries, hcd->msix_count);
+ if (ret) {
+ dev_dbg(&pdev->dev, "Failed to enable MSI-X\n");
+ goto free_entries;
+ }
+
+ return ret;
+
+free_entries:
+ kfree(hcd->msix_entries);
+ hcd->msix_entries = NULL;
+ hcd->msix_count = 0;
+ return ret;
+}
+
+/* Device for a quirk */
+#define PCI_VENDOR_ID_FRESCO_LOGIC 0x1b73
+#define PCI_DEVICE_ID_FRESCO_LOGIC_PDK 0x1000
+
+/* Check for bad HCD devices, and driver desires for MSI.
+ * Now, only in xhci driver want it by HCD_MSI_FIRST setting.
+ * Other driver like ehci/uhci can follow this setting.
+ */
+static int hcd_no_msi(struct pci_dev *pdev, struct usb_hcd *hcd)
+{
+ if (pdev->vendor == PCI_VENDOR_ID_FRESCO_LOGIC &&
+ pdev->device == PCI_DEVICE_ID_FRESCO_LOGIC_PDK) {
+
+ /* Fresco Logic confirms: all revisions of this chip do not
+ * support MSI, even though some of them claim to in their PCI
+ * capabilities.
+ */
+ dev_dbg(&pdev->dev, "QUIRK: Fresco Logic revision %u "
+ "has broken MSI implementation\n",
+ pdev->revision);
+ return 1;
+ } else if (!(hcd->driver->flags & HCD_MSI_FIRST))
+ return 1;
+
+ return 0;
+}
+
+/* setup msi/msix interrupts. if can not do this, fallback to line irq */
+static int hcd_setup_int(struct pci_dev *pdev, struct usb_hcd *hcd)
+{
+ int ret;
+
+ hcd->irq = -1;
+
+ if (!hcd_no_msi(pdev, hcd)) {
+
+ ret = hcd_setup_msix(pdev, hcd);
+ if (ret)
+ /* fall back to msi*/
+ ret = hcd_setup_msi(pdev, hcd);
+
+ if (!ret)
+ /* hcd->irq is -1, we have MSI */
+ return 0;
+ }
+
+ if (!pdev->irq) {
+ dev_err(&pdev->dev, "No msi or line irq found!\n");
+ return -1;
+ }
+ /* fallback to line irq */
+ hcd->irq = pdev->irq;
+ return 0;
+}
+
+/* msi irq handler should be here, if driver has */
+irqreturn_t hcd_msi_irq(int irq, struct usb_hcd *hcd)
+{
+ return hcd->driver->irq(hcd);
+}
+
+int usb_hcd_request_msi_msix_irqs(struct usb_hcd *hcd, unsigned int irqnum,
+ unsigned long irqflags)
+{
+ int retval = 1;
+ if (hcd->msix_entries && hcd->driver->irq) {
+ int i;
+ /* register hc_driver's irq handler */
+ for (i = 0; i < hcd->msix_count; i++) {
+ retval = request_irq(hcd->msix_entries[i].vector,
+ (irq_handler_t)hcd_msi_irq,
+ 0, hcd->driver->description, hcd);
+ if (retval) {
+ hcd_free_msix(hcd);
+ break;
+ }
+ }
+ if (i == hcd->msix_count)
+ hcd->msix_enabled = 1;
+ } else if (hcd->irq == -1) {
+ /* use msi */
+ retval = request_irq(irqnum, (irq_handler_t)hcd_msi_irq,
+ 0, hcd->driver->description, hcd);
+ if (retval)
+ hcd_free_msi(hcd);
+ }
+
+ return retval;
+}
+EXPORT_SYMBOL_GPL(usb_hcd_request_msi_msix_irqs);
+
+/* setup msi/msix interrupts and requestion irqs for them */
+int usb_hcd_register_msi_msix_irqs(struct usb_hcd *hcd)
+{
+ struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
+ int ret = -1;
+
+ if (!hcd_setup_int(pdev, hcd))
+ ret = usb_hcd_request_msi_msix_irqs(hcd,
+ pdev->irq, IRQF_SHARED);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(usb_hcd_register_msi_msix_irqs);
+
/* configure so an HC device and id are always provided */
/* always called with process context; sleeping is OK */
@@ -243,6 +461,11 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
pci_set_master(dev);
+ /* try enable MSI, if fail, seek line irq */
+ retval = hcd_setup_int(dev, hcd);
+ if (retval != 0)
+ goto unmap_registers;
+
retval = usb_add_hcd(hcd, dev->irq, IRQF_SHARED);
if (retval != 0)
goto unmap_registers;
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index b3a7920..8b5f7bb 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2322,7 +2322,7 @@ int usb_hcd_is_primary_hcd(struct usb_hcd *hcd)
}
EXPORT_SYMBOL_GPL(usb_hcd_is_primary_hcd);
-static int usb_hcd_request_irqs(struct usb_hcd *hcd,
+static int usb_hcd_request_default_irqs(struct usb_hcd *hcd,
unsigned int irqnum, unsigned long irqflags)
{
int retval;
@@ -2362,6 +2362,17 @@ static int usb_hcd_request_irqs(struct usb_hcd *hcd,
return 0;
}
+int usb_hcd_request_irqs(struct usb_hcd *hcd, unsigned int irqnum,
+ unsigned long irqflags)
+{
+ int retval;
+ retval = usb_hcd_request_msi_msix_irqs(hcd, irqnum, irqflags);
+ if (retval)
+ retval = usb_hcd_request_default_irqs(hcd, irqnum, irqflags);
+
+ return retval;
+}
+
/**
* usb_add_hcd - finish generic HCD structure initialization and register
* @hcd: the usb_hcd structure to initialize
@@ -2447,11 +2458,8 @@ int usb_add_hcd(struct usb_hcd *hcd,
&& device_can_wakeup(&hcd->self.root_hub->dev))
dev_dbg(hcd->self.controller, "supports USB remote wakeup\n");
- /* enable irqs just before we start the controller, except MSI
- * first try HCD. That will do it in following driver->start();
- */
- if (usb_hcd_is_primary_hcd(hcd) &&
- !(hcd->driver->flags & HCD_MSI_FIRST)) {
+ /* enable irqs just before we start the controller */
+ if (usb_hcd_is_primary_hcd(hcd)) {
retval = usb_hcd_request_irqs(hcd, irqnum, irqflags);
if (retval)
goto err_request_irq;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index c939f5f..9aa3d27 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -176,210 +176,14 @@ int xhci_reset(struct xhci_hcd *xhci)
}
#ifdef CONFIG_PCI
-static int xhci_free_msi(struct xhci_hcd *xhci)
-{
- int i;
-
- if (!xhci->msix_entries)
- return -EINVAL;
-
- for (i = 0; i < xhci->msix_count; i++)
- if (xhci->msix_entries[i].vector)
- free_irq(xhci->msix_entries[i].vector,
- xhci_to_hcd(xhci));
- return 0;
-}
-
-/*
- * Set up MSI
- */
-static int xhci_setup_msi(struct xhci_hcd *xhci)
-{
- int ret;
- struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
-
- ret = pci_enable_msi(pdev);
- if (ret) {
- xhci_dbg(xhci, "failed to allocate MSI entry\n");
- return ret;
- }
-
- ret = request_irq(pdev->irq, (irq_handler_t)xhci_msi_irq,
- 0, "xhci_hcd", xhci_to_hcd(xhci));
- if (ret) {
- xhci_dbg(xhci, "disable MSI interrupt\n");
- pci_disable_msi(pdev);
- }
-
- return ret;
-}
-
-/*
- * Free IRQs
- * free all IRQs request
- */
-static void xhci_free_irq(struct xhci_hcd *xhci)
-{
- struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
- int ret;
-
- /* return if using legacy interrupt */
- if (xhci_to_hcd(xhci)->irq >= 0)
- return;
-
- ret = xhci_free_msi(xhci);
- if (!ret)
- return;
- if (pdev->irq >= 0)
- free_irq(pdev->irq, xhci_to_hcd(xhci));
-
- return;
-}
-
-/*
- * Set up MSI-X
- */
-static int xhci_setup_msix(struct xhci_hcd *xhci)
-{
- int i, ret = 0;
- struct usb_hcd *hcd = xhci_to_hcd(xhci);
- struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
-
- /*
- * calculate number of msi-x vectors supported.
- * - HCS_MAX_INTRS: the max number of interrupts the host can handle,
- * with max number of interrupters based on the xhci HCSPARAMS1.
- * - num_online_cpus: maximum msi-x vectors per CPUs core.
- * Add additional 1 vector to ensure always available interrupt.
- */
- xhci->msix_count = min(num_online_cpus() + 1,
- HCS_MAX_INTRS(xhci->hcs_params1));
-
- xhci->msix_entries =
- kmalloc((sizeof(struct msix_entry))*xhci->msix_count,
- GFP_KERNEL);
- if (!xhci->msix_entries) {
- xhci_err(xhci, "Failed to allocate MSI-X entries\n");
- return -ENOMEM;
- }
-
- for (i = 0; i < xhci->msix_count; i++) {
- xhci->msix_entries[i].entry = i;
- xhci->msix_entries[i].vector = 0;
- }
-
- ret = pci_enable_msix(pdev, xhci->msix_entries, xhci->msix_count);
- if (ret) {
- xhci_dbg(xhci, "Failed to enable MSI-X\n");
- goto free_entries;
- }
-
- for (i = 0; i < xhci->msix_count; i++) {
- ret = request_irq(xhci->msix_entries[i].vector,
- (irq_handler_t)xhci_msi_irq,
- 0, "xhci_hcd", xhci_to_hcd(xhci));
- if (ret)
- goto disable_msix;
- }
-
- hcd->msix_enabled = 1;
- return ret;
-
-disable_msix:
- xhci_dbg(xhci, "disable MSI-X interrupt\n");
- xhci_free_irq(xhci);
- pci_disable_msix(pdev);
-free_entries:
- kfree(xhci->msix_entries);
- xhci->msix_entries = NULL;
- return ret;
-}
-
-/* Free any IRQs and disable MSI-X */
-static void xhci_cleanup_msix(struct xhci_hcd *xhci)
-{
- struct usb_hcd *hcd = xhci_to_hcd(xhci);
- struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
-
- xhci_free_irq(xhci);
-
- if (xhci->msix_entries) {
- pci_disable_msix(pdev);
- kfree(xhci->msix_entries);
- xhci->msix_entries = NULL;
- } else {
- pci_disable_msi(pdev);
- }
-
- hcd->msix_enabled = 0;
- return;
-}
static void xhci_msix_sync_irqs(struct xhci_hcd *xhci)
{
- int i;
-
- if (xhci->msix_entries) {
- for (i = 0; i < xhci->msix_count; i++)
- synchronize_irq(xhci->msix_entries[i].vector);
- }
-}
-
-static int xhci_try_enable_msi(struct usb_hcd *hcd)
-{
- struct xhci_hcd *xhci = hcd_to_xhci(hcd);
- struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
- int ret;
-
- /*
- * Some Fresco Logic host controllers advertise MSI, but fail to
- * generate interrupts. Don't even try to enable MSI.
- */
- if (xhci->quirks & XHCI_BROKEN_MSI)
- return 0;
-
- /* unregister the legacy interrupt */
- if (hcd->irq)
- free_irq(hcd->irq, hcd);
- hcd->irq = -1;
-
- ret = xhci_setup_msix(xhci);
- if (ret)
- /* fall back to msi*/
- ret = xhci_setup_msi(xhci);
-
- if (!ret)
- /* hcd->irq is -1, we have MSI */
- return 0;
-
- if (!pdev->irq) {
- xhci_err(xhci, "No msi-x/msi found and no IRQ in BIOS\n");
- return -EINVAL;
- }
-
- /* fall back to legacy interrupt*/
- ret = request_irq(pdev->irq, &usb_hcd_irq, IRQF_SHARED,
- hcd->irq_descr, hcd);
- if (ret) {
- xhci_err(xhci, "request interrupt %d failed\n",
- pdev->irq);
- return ret;
- }
- hcd->irq = pdev->irq;
- return 0;
+ usb_hcd_msix_sync_irqs(xhci_to_hcd(xhci));
}
#else
-static int xhci_try_enable_msi(struct usb_hcd *hcd)
-{
- return 0;
-}
-
-static void xhci_cleanup_msix(struct xhci_hcd *xhci)
-{
-}
-
static void xhci_msix_sync_irqs(struct xhci_hcd *xhci)
{
}
@@ -497,7 +301,6 @@ int xhci_run(struct usb_hcd *hcd)
{
u32 temp;
u64 temp_64;
- int ret;
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
/* Start the xHCI host controller running only after the USB 2.0 roothub
@@ -510,10 +313,6 @@ int xhci_run(struct usb_hcd *hcd)
xhci_dbg(xhci, "xhci_run\n");
- ret = xhci_try_enable_msi(hcd);
- if (ret)
- return ret;
-
#ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
init_timer(&xhci->event_ring_timer);
xhci->event_ring_timer.data = (unsigned long) xhci;
@@ -609,7 +408,7 @@ void xhci_stop(struct usb_hcd *hcd)
xhci_reset(xhci);
spin_unlock_irq(&xhci->lock);
- xhci_cleanup_msix(xhci);
+ usb_hcd_free_msi_msix(hcd);
#ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
/* Tell the event ring poll function not to reschedule */
@@ -651,7 +450,7 @@ void xhci_shutdown(struct usb_hcd *hcd)
xhci_halt(xhci);
spin_unlock_irq(&xhci->lock);
- xhci_cleanup_msix(xhci);
+ usb_hcd_free_msi_msix(hcd);
xhci_dbg(xhci, "xhci_shutdown completed - status = %x\n",
xhci_readl(xhci, &xhci->op_regs->status));
@@ -853,7 +652,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
xhci_halt(xhci);
xhci_reset(xhci);
spin_unlock_irq(&xhci->lock);
- xhci_cleanup_msix(xhci);
+ usb_hcd_free_msi_msix(hcd);
#ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
/* Tell the event ring poll function not to reschedule */
@@ -888,7 +687,11 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
if (retval)
return retval;
xhci_dbg(xhci, "Start the primary HCD\n");
- retval = xhci_run(hcd->primary_hcd);
+ if (dev_is_pci(hcd->self.controller))
+ retval = usb_hcd_register_msi_msix_irqs(
+ hcd->primary_hcd);
+ if (!retval)
+ retval = xhci_run(hcd->primary_hcd);
if (!retval) {
xhci_dbg(xhci, "Start the secondary HCD\n");
retval = xhci_run(secondary_hcd);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index fb99c83..6841a1c 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1388,9 +1388,6 @@ struct xhci_hcd {
int page_size;
/* Valid values are 12 to 20, inclusive */
int page_shift;
- /* msi-x vectors */
- int msix_count;
- struct msix_entry *msix_entries;
/* data structures */
struct xhci_device_context_array *dcbaa;
struct xhci_ring *cmd_ring;
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 2cc8673..697a8e1 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -22,6 +22,7 @@
#ifdef __KERNEL__
#include <linux/rwsem.h>
+#include <linux/pci.h> /* for struct msix_entry */
#define MAX_TOPO_LEVEL 6
@@ -133,6 +134,12 @@ struct usb_hcd {
u64 rsrc_len; /* memory/io resource length */
unsigned power_budget; /* in mA, 0 = no limit */
+#ifdef CONFIG_PCI
+ /* msi-x vectors */
+ int msix_count;
+ struct msix_entry *msix_entries;
+#endif
+
/* bandwidth_mutex should be taken before adding or removing
* any new bus bandwidth constraints:
* 1. Before adding a configuration for a new device.
@@ -393,10 +400,29 @@ extern int usb_hcd_pci_probe(struct pci_dev *dev,
const struct pci_device_id *id);
extern void usb_hcd_pci_remove(struct pci_dev *dev);
extern void usb_hcd_pci_shutdown(struct pci_dev *dev);
+extern void usb_hcd_free_msi_msix(struct usb_hcd *hcd);
+extern void usb_hcd_msix_sync_irqs(struct usb_hcd *hcd);
+extern int usb_hcd_request_msi_msix_irqs(struct usb_hcd *hcd,
+ unsigned int irqnum, unsigned long irqflags);
+extern int usb_hcd_register_msi_msix_irqs(struct usb_hcd *hcd);
#ifdef CONFIG_PM_SLEEP
extern const struct dev_pm_ops usb_hcd_pci_pm_ops;
#endif
+
+#else
+void usb_hcd_free_msi_msix(struct usb_hcd *hcd)
+{
+}
+void usb_hcd_msix_sync_irqs(struct usb_hcd *hcd)
+{
+}
+int usb_hcd_request_msi_msix_irqs(struct usb_hcd *hcd,
+ unsigned int irqnum, unsigned long irqflags)
+{
+ return 1;
+}
+
#endif /* CONFIG_PCI */
/* pci-ish (pdev null is ok) buffer alloc/mapping support */
--
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
