On Sat, Jan 14, 2012 at 07:55:17PM +0800, Alex Shi wrote:
> Current PCI hcd driver will check and enable line IRQ first, if line
> IRQ is no setting in BIOS, XHCI PCIe driver will exit. But infact, if
> MSI can work for this HCD, this behavior will refuse a workable HCD.
This description seems old. Your first patch took care of the Intel
system issue with the hcd-pci.c exiting when it couldn't enable legacy
PCI interrupts, so that shouldn't be mentioned here.
> This patch change the behavior. It skips the first line IRQ checking for
> PCI XHCI HCD in usb-core, and try to enable MSI first, if the MSI out of
> work, it is back to line IRQ at that time. And further more, trying MSI
> first can avoid a unnecessary request/fee irq for line IRQ for PCIe HCD.
Just leave this paragraph and say you're trying to make sure that the
USB core allows the xHCI driver to enable MSI or MSI-X before the legacy
PCI IRQ.
> Thanks for Sarah and Andiry's suggestion and review for this patch.
>
> Signed-off-by: Alex Shi <alex.shi@xxxxxxxxx>
> ---
> drivers/usb/core/hcd.c | 5 ++---
> drivers/usb/host/xhci-pci.c | 8 ++------
> drivers/usb/host/xhci.c | 25 ++++++++++---------------
> 3 files changed, 14 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 9dd8098..0a4f185 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2466,9 +2466,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. But Intel USB3
> - * hcd can't do this here on some platform, they will do it in
> - * following driver->start();
> + /* 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)) {
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index d453cb8..d9bef41 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -137,11 +137,6 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>
> driver = (struct hc_driver *)id->driver_data;
>
> - /* stop line IRQ checking in xhci_hcd_pci_probe. */
> - if (dev->vendor == PCI_VENDOR_ID_INTEL &&
> - dev->device == PCI_DEVICE_ID_INTEL_PANTHERPOINT_XHCI)
> - driver->flags |= HCD_MSI_FIRST;
> -
> /* Register the USB 2.0 roothub.
> * FIXME: USB core must know to register the USB 2.0 roothub first.
> * This is sort of silly, because we could just set the HCD driver flags
> @@ -249,7 +244,8 @@ static const struct hc_driver xhci_pci_hc_driver = {
> * generic hardware linkage
> */
> .irq = xhci_irq,
> - .flags = HCD_MEMORY | HCD_USB3 | HCD_SHARED,
> + .flags = HCD_MEMORY | HCD_USB3 | HCD_SHARED |
> + HCD_MSI_FIRST,
>
> /*
> * basic lifecycle operations
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 05cf62d..e385a55 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -331,26 +331,21 @@ static int xhci_try_enable_msi(struct usb_hcd *hcd)
> struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
> int ret;
>
> + hcd->irq = -1;
> /*
> * 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 (!(xhci->quirks & XHCI_BROKEN_MSI)) {
> + ret = xhci_setup_msix(xhci);
> + if (ret)
> + /* fall back to msi*/
> + ret = xhci_setup_msi(xhci);
I think this code is backwards. In xhci-pci.c, when we detect the xHCI
chipsets with broken MSI, we should clear the HCD_MSI_FIRST flag, and
allow the USB core to set up the legacy PCI interrupt. Then this
function should just return when it detects the XHCI_BROKEN_MSI quirk
(before it sets hcd->irq to -1). That way any changes to the legacy PCI
IRQ setup normal flow in the USB core will also effect these xHCI hosts
that really need it.
> - if (!ret)
> - /* hcd->irq is -1, we have MSI */
> - return 0;
> + 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");
The first patch looks fine, although I would have to apply it to see the
changes. BTW, have you tested the code on the Intel Panther Point xHCI
host to make sure it works when CONFIG_PCI_MSI is turned off? And by
"works" I mean that xHCI probe fails when MSI is turned off for Panther
Point systems.
Sarah Sharp
--
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
