Hi On 27.04.2017 12:49, Peter Chen wrote:
On Mon, Apr 17, 2017 at 04:20:43PM +0800, Peter Chen wrote:According to xHCI spec Figure 30: Interrupt Throttle Flow Diagram If PCI Message Signaled Interrupts (MSI or MSI-X) are enabled, then the assertion of the Interrupt Pending (IP) flag in Figure 30 generates a PCI Dword write. The IP flag is automatically cleared by the completion of the PCI write. the MSI enabled HCs don't need to clear interrupt pending bit, but hcd->irq = 0 doesn't equal to MSI enabled HCD. At some Dual-role controller software designs, it sets hcd->irq as 0 to avoid HCD requesting interrupt, and they want to decide when to call usb_hcd_irq by software.ping...
This start to look like xhci msi(x) needs a bigger cleanup. The msi parts should probably be moved to xhci-pci.c, and let xhci_try_enable_msi() be pci specific. xhci platform drivers should not need to worry about msi.
Signed-off-by: Peter Chen <peter.chen@xxxxxxx> --- Mathias, I am not very sure if the comments I delete is complete useless, plese help to check. drivers/usb/host/xhci-ring.c | 5 +---- drivers/usb/host/xhci.c | 5 +++-- include/linux/usb/hcd.h | 1 + 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index d9936c7..f5adbb3 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -2642,12 +2642,9 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd) */ status |= STS_EINT; writel(status, &xhci->op_regs->status); - /* FIXME when MSI-X is supported and there are multiple vectors */ - /* Clear the MSI-X event interrupt status */ - if (hcd->irq) { + if (!hcd->msi_enabled) { u32 irq_pending; - /* Acknowledge the PCI interrupt */ irq_pending = readl(&xhci->ir_set->irq_pending); irq_pending |= IMAN_IP; writel(irq_pending, &xhci->ir_set->irq_pending); diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 1828695..dc81041 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -401,9 +401,10 @@ static int xhci_try_enable_msi(struct usb_hcd *hcd) /* fall back to msi*/ ret = xhci_setup_msi(xhci); - if (!ret) - /* hcd->irq is 0, we have MSI */ + if (!ret) { + hcd->msi_enabled = 1; return 0; + } if (!pdev->irq) { xhci_err(xhci, "No msi-x/msi found and no IRQ in BIOS\n"); diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h index a469999..50398b6 100644 --- a/include/linux/usb/hcd.h +++ b/include/linux/usb/hcd.h @@ -148,6 +148,7 @@ struct usb_hcd { unsigned rh_registered:1;/* is root hub registered? */ unsigned rh_pollable:1; /* may we poll the root hub? */ unsigned msix_enabled:1; /* driver has MSI-X enabled? */ + unsigned msi_enabled:1; /* driver has MSI enabled? */ unsigned remove_phy:1; /* auto-remove USB phy */ /* The next flag is a stopgap, to be removed when all the HCDs --
so after this hcd->irq > 0 means a "legacy" interrupt is used. hcd->irq == 0 means anything else than "legacy" is used, might be nothing as well msi_enabled means msi or msix is in use msix_enabled means msix is in use This probably works, but moving msi to be xhci-pci specific feels like a better way to fix this, but might end up as a more work -Mathias -- 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
