Re: [PATCH 1/1] usb: host: xhci-ring: don't need to clear interrupt pending for MSI enabled hcd

[Mathias Nyman]

On 28.04.2017 04:04, Peter Chen wrote:

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 
---
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, >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(>ir_set->irq_pending);
irq_pending |= IMAN_IP;
writel(irq_pending, >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 a46..50398b6 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -148,6 +148,7 @@ struct usb_hcd {
unsignedrh_registered:1;/* is root hub registered? */
unsignedrh_pollable:1;  /* may we poll the root hub? */
unsignedmsix_enabled:1; /* driver has MSI-X enabled? */
+   unsignedmsi_enabled:1;  /* driver has MSI enabled? */
unsignedremove_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 == 0means anything else than "legacy" is used, might be 
nothing as well
msi_enabled means msi or msix is in use
msix_enabledmeans 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.



Ok, lets have this one to get things working. I'll send it forward after rc1

-Mathias
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] usb: host: xhci-ring: don't need to clear interrupt pending for MSI enabled hcd

[Peter Chen]

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 
> >>---
> >>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, >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(>ir_set->irq_pending);
> >>irq_pending |= IMAN_IP;
> >>writel(irq_pending, >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 a46..50398b6 100644
> >>--- a/include/linux/usb/hcd.h
> >>+++ b/include/linux/usb/hcd.h
> >>@@ -148,6 +148,7 @@ struct usb_hcd {
> >>unsignedrh_registered:1;/* is root hub registered? */
> >>unsignedrh_pollable:1;  /* may we poll the root hub? */
> >>unsignedmsix_enabled:1; /* driver has MSI-X enabled? */
> >>+   unsignedmsi_enabled:1;  /* driver has MSI enabled? */
> >>unsignedremove_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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] usb: host: xhci-ring: don't need to clear interrupt pending for MSI enabled hcd

[Mathias Nyman]

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 
---
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, >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(>ir_set->irq_pending);
irq_pending |= IMAN_IP;
writel(irq_pending, >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 a46..50398b6 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -148,6 +148,7 @@ struct usb_hcd {
unsignedrh_registered:1;/* is root hub registered? */
unsignedrh_pollable:1;  /* may we poll the root hub? */
unsignedmsix_enabled:1; /* driver has MSI-X enabled? */
+   unsignedmsi_enabled:1;  /* driver has MSI enabled? */
unsignedremove_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 == 0means anything else than "legacy" is used, might be 
nothing as well
msi_enabled means msi or msix is in use
msix_enabledmeans 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] usb: host: xhci-ring: don't need to clear interrupt pending for MSI enabled hcd

[Peter Chen]

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...

> 
> Signed-off-by: Peter Chen 
> ---
> 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, >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(>ir_set->irq_pending);
>   irq_pending |= IMAN_IP;
>   writel(irq_pending, >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 a46..50398b6 100644
> --- a/include/linux/usb/hcd.h
> +++ b/include/linux/usb/hcd.h
> @@ -148,6 +148,7 @@ struct usb_hcd {
>   unsignedrh_registered:1;/* is root hub registered? */
>   unsignedrh_pollable:1;  /* may we poll the root hub? */
>   unsignedmsix_enabled:1; /* driver has MSI-X enabled? */
> + unsignedmsi_enabled:1;  /* driver has MSI enabled? */
>   unsignedremove_phy:1;   /* auto-remove USB phy */
>  
>   /* The next flag is a stopgap, to be removed when all the HCDs
> -- 

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/1] usb: host: xhci-ring: don't need to clear interrupt pending for MSI enabled hcd

[Peter Chen]

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.

Signed-off-by: Peter Chen 
---
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, >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(>ir_set->irq_pending);
irq_pending |= IMAN_IP;
writel(irq_pending, >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 a46..50398b6 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -148,6 +148,7 @@ struct usb_hcd {
unsignedrh_registered:1;/* is root hub registered? */
unsignedrh_pollable:1;  /* may we poll the root hub? */
unsignedmsix_enabled:1; /* driver has MSI-X enabled? */
+   unsignedmsi_enabled:1;  /* driver has MSI enabled? */
unsignedremove_phy:1;   /* auto-remove USB phy */
 
/* The next flag is a stopgap, to be removed when all the HCDs
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html