On Thu, Apr 27, 2017 at 03:43:28PM +0300, Mathias Nyman wrote:
> 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
>
Yes, but my platform is not PCI based, if it is a big patch, I am may
not suitable for it, will you do it? This issue blocks my USB3 driver
working.
--
Best Regards,
Peter Chen
--
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
