Skip to contentSkip to conversation

usb/ehci-hub: Add workaround for EG20T/ML7213/ML7223/ML7831

PATCH In review Tomoya MORINAGA 3 participants 5 messages Apr 10, 2012 – Apr 18, 2012

Subsystems (2)
USB USB ehci driver
lore
Share & tools
lore

Tomoya MORINAGA

author
#0 lore raw

PATCH 1/2 usb/ehci-hub: Add workaround for EG20T/ML7213/ML7223/ML7831

ISSUE: When a bit of EHCI status register (USBSTS) is set as 1, if the corresponded bit of EHCI interrupt enable register(USBINTR) is set as 1, an interrupt occurs. After that, even if the bit of USBINTR is set as 0, the interrupt continues occurring.
Workaround: Clear the bit 3 of USBSTS before enabling the interrupt, at resume process.

This patch is for
Intel EG20T PCH
LAPIS Semiconductor ML7213 IOH
LAPIS Semiconductor ML7223 IOH
LAPIS Semiconductor ML7831 IOH

Signed-off-by Tomoya MORINAGA <[email protected]>
---
 drivers/usb/host/ehci-hub.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)
drivers/usb/host/ehci-hub.c +18 -0
index 256fbd4..7ddb62e 100644
@@ -342,6 +342,7 @@ static int ehci_bus_resume (struct usb_hcd *hcd)
 	u32			power_okay;
 	int			i;
 	unsigned long		resume_needed = 0;
+	struct pci_dev		*pdev = to_pci_dev(hcd->self.controller);
 
 	if (time_before (jiffies, ehci->next_statechange))
 		msleep(5);

@@ -451,6 +452,23 @@ static int ehci_bus_resume (struct usb_hcd *hcd)
 
 	ehci->next_statechange = jiffies + msecs_to_jiffies(5);
 
+	if ((pdev->vendor == PCI_VENDOR_ID_INTEL && pdev->device == 0x8807) ||
+	   (pdev->vendor == PCI_VENDOR_ID_INTEL && pdev->device == 0x880F) ||
+	   (pdev->vendor == 0x10DB && pdev->device == 0x801C) || /* ML7213 */
+	   (pdev->vendor == 0x10DB && pdev->device == 0x8007) || /* ML7223 */
+	   (pdev->vendor == 0x10DB && pdev->device == 0x8807)) { /* ML7831 */
+		/* ISSUE: When a bit of EHCI status register (USBSTS) is set
+		 * as 1, if the corresponded bit of EHCI interrupt enable
+		 * register(USBINTR) is set as 1, an interrupt occurs.
+		 * After that, even if the bit of USBINTR is set as 0, the
+		 * interrupt continues occurring.
+		 * Workaround: Clear the bit 3 of USBSTS before enabling the
+		 * interrupt, at resume process.
+		 */
+		ehci_writel(ehci, STS_FLR, &ehci->regs->status);
+		ehci_dbg(ehci, "Workaround: %s: Clear STS_FLR\n", __func__);
+	}
+
 	/* Now we can safely re-enable irqs */
 	ehci_writel(ehci, INTR_MASK, &ehci->regs->intr_enable);
Signature · 1 line
1.7.7.6
author
#1 lore raw

PATCH 2/2 usb/ehci-pci: Add workaround for EG20T/ML7213/ML7223/ML7831

ISSUE: When a bit of EHCI status register (USBSTS) is set as 1, if the corresponded bit of EHCI interrupt enable register(USBINTR) is set as 1, an interrupt occurs. After that, even if the bit of USBINTR is set as 0, the interrupt continues occurring.
Workaround: Clear the bit 3 of USBSTS before enabling the interrupt, at resume process.

This patch is for
Intel EG20T PCH
LAPIS Semiconductor ML7213 IOH
LAPIS Semiconductor ML7223 IOH
LAPIS Semiconductor ML7831 IOH

Signed-off-by Tomoya MORINAGA <[email protected]>
---
 drivers/usb/host/ehci-pci.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)
drivers/usb/host/ehci-pci.c +24 -0
index 01bb7241d..30f3ad6 100644
@@ -419,6 +419,30 @@ static int ehci_pci_resume(struct usb_hcd *hcd, bool hibernated)
 		ehci_prepare_ports_for_controller_resume(ehci);
 		if (!hcd->self.root_hub->do_remote_wakeup)
 			mask &= ~STS_PCD;
+
+		if ((pdev->vendor == PCI_VENDOR_ID_INTEL &&
+		     pdev->device == 0x8807) ||
+		   (pdev->vendor == PCI_VENDOR_ID_INTEL &&
+		     pdev->device == 0x880F) ||
+		   (pdev->vendor == 0x10DB &&
+		    pdev->device == 0x801C) || /* ML7213 */
+		   (pdev->vendor == 0x10DB &&
+		    pdev->device == 0x8007) || /* ML7223 */
+		   (pdev->vendor == 0x10DB &&
+		    pdev->device == 0x8807)) { /* ML7831 */
+		/* ISSUE: When a bit of EHCI status register (USBSTS) is set
+		 * as 1, if the corresponded bit of EHCI interrupt enable
+		 * register(USBINTR) is set as 1, an interrupt occurs.
+		 * After that, even if the bit of USBINTR is set as 0, the
+		 * interrupt continues occurring.
+		 * Workaround: Clear the bit 3 of USBSTS before enabling the
+		 * interrupt, at resume process.
+		 */
+			ehci_writel(ehci, STS_FLR, &ehci->regs->status);
+			ehci_dbg(ehci, "Workaround: %s: Clear STS_FLR\n",
+				  __func__);
+		}
+
 		ehci_writel(ehci, mask, &ehci->regs->intr_enable);
 		ehci_readl(ehci, &ehci->regs->intr_enable);
 		return 0;
Signature · 1 line
1.7.7.6
#2 lore raw
In reply to Tomoya MORINAGA

Hello.

Quoted 23 lines · Tomoya MORINAGA

ISSUE: When a bit of EHCI status register (USBSTS) is set as 1, if the corresponded bit of EHCI interrupt enable register(USBINTR) is set as 1, an interrupt occurs. After that, even if the bit of USBINTR is set as 0, the interrupt continues occurring.
Workaround: Clear the bit 3 of USBSTS before enabling the interrupt, at resume process.

This patch is for
Intel EG20T PCH
LAPIS Semiconductor ML7213 IOH
LAPIS Semiconductor ML7223 IOH
LAPIS Semiconductor ML7831 IOH

Signed-off-by Tomoya MORINAGA<[email protected]>
---
  drivers/usb/host/ehci-hub.c |   18 ++++++++++++++++++
  1 files changed, 18 insertions(+), 0 deletions(-)
drivers/usb/host/ehci-hub.c +0 -0
index 256fbd4..7ddb62e 100644

[...]

drivers/usb/host/ehci-hub.c static int ehci_bus_resume (struct usb_hcd *hcd) · line 459

Quoted 9 lines
drivers/usb/host/ehci-hub.c +5 -0
@@ -451,6 +452,23 @@ static int ehci_bus_resume (struct usb_hcd *hcd)
 
  	ehci->next_statechange = jiffies + msecs_to_jiffies(5);
 
+	if ((pdev->vendor == PCI_VENDOR_ID_INTEL&&  pdev->device == 0x8807) ||
+	   (pdev->vendor == PCI_VENDOR_ID_INTEL&&  pdev->device == 0x880F) ||
+	   (pdev->vendor == 0x10DB&&  pdev->device == 0x801C) || /* ML7213 */
+	   (pdev->vendor == 0x10DB&&  pdev->device == 0x8007) || /* ML7223 */
+	   (pdev->vendor == 0x10DB&&  pdev->device == 0x8807)) { /* ML7831 */

Why not:

	if ((pdev->vendor == PCI_VENDOR_ID_INTEL &&
	    (pdev->device == 0x8807 || pdev->device == 0x880F)) ||
	    (pdev->vendor == 0x10DB &&
	    (pdev->device == 0x801C || /* ML7213 */
	     pdev->device == 0x8007 || /* ML7223 */
	     pdev->device == 0x8807)   /* ML7831 */
	)) {

which avoids the duplicate checks? The same about the next patch.

WBR, Sergei

maintainer
#3 lore raw
In reply to Tomoya MORINAGA
Quoted 7 lines · Tomoya MORINAGA

ISSUE: When a bit of EHCI status register (USBSTS) is set as 1, if the corresponded bit of EHCI interrupt enable register(USBINTR) is set as 1, an interrupt occurs. After that, even if the bit of USBINTR is set as 0, the interrupt continues occurring.
Workaround: Clear the bit 3 of USBSTS before enabling the interrupt, at resume process.

These two patches do a lot more than is necessary. At the same time, they also seem to do a lot less.

You don't have to check for the faulty product IDs; clearing the STS_FLR bit (bit 3 of USBSTS) is always okay because the driver doesn't use it.

On the other hand, the STS_FLR bit will get set once every second (at least) whenever the controller is running. According to your description above, this means the controller will cause an interrupt storm as soon as its first interrupt occurs. What you really need to do is clear STS_FLR in ehci_irq().

Does the patch below fix your problem?

Alan Stern

 drivers/usb/host/ehci-hcd.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/usb/host/ehci-hcd.c +6 -1
Index: usb-3.4/drivers/usb/host/ehci-hcd.c
===================================================================
@@ -860,8 +860,13 @@ static irqreturn_t ehci_irq (struct usb_
 		goto dead;
 	}
 
+	/*
+	 * We don't use STS_FLR, but some controllers don't like it to
+	 * remain on, so mask it out along with the other status bits.
+	 */
+	masked_status = status & (INTR_MASK | STS_FLR);
+
 	/* Shared IRQ? */
-	masked_status = status & INTR_MASK;
 	if (!masked_status || unlikely(ehci->rh_state == EHCI_RH_HALTED)) {
 		spin_unlock(&ehci->lock);
 		return IRQ_NONE;
author
#4 lore raw
Quoted 42 lines · Alan Stern
Quoted 7 lines · Tomoya MORINAGA

ISSUE: When a bit of EHCI status register (USBSTS) is set as 1, if the corresponded bit of EHCI interrupt enable register(USBINTR) is set as 1, an interrupt occurs. After that, even if the bit of USBINTR is set as 0, the interrupt continues occurring.
Workaround: Clear the bit 3 of USBSTS before enabling the interrupt, at resume process.

These two patches do a lot more than is necessary.  At the same time, they also seem to do a lot less.

You don't have to check for the faulty product IDs; clearing the STS_FLR bit (bit 3 of USBSTS) is always okay because the driver doesn't use it.

On the other hand, the STS_FLR bit will get set once every second (at least) whenever the controller is running.  According to your description above, this means the controller will cause an interrupt storm as soon as its first interrupt occurs.  What you really need to do is clear STS_FLR in ehci_irq().

Does the patch below fix your problem?

Alan Stern

 drivers/usb/host/ehci-hcd.c |    7 ++++++-

 1 file changed, 6 insertions(+), 1 deletion(-)

Index: usb-3.4/drivers/usb/host/ehci-hcd.c
===================================================================

drivers/usb/host/ehci-hcd.c +1 -0
@@ -860,8 +860,13 @@ static irqreturn_t ehci_irq (struct usb_
                goto dead;
        }
 
+       /*

drivers/usb/host/ehci-hcd.c static irqreturn_t ehci_irq (struct usb_ · line 872

Quoted 10 lines
drivers/usb/host/ehci-hcd.c +5 -1
+        * We don't use STS_FLR, but some controllers don't like it to
+        * remain on, so mask it out along with the other status bits.
+        */
+       masked_status = status & (INTR_MASK | STS_FLR);
+
        /* Shared IRQ? */
-       masked_status = status & INTR_MASK;
        if (!masked_status || unlikely(ehci->rh_state == EHCI_RH_HALTED)) {
                spin_unlock(&ehci->lock);
                return IRQ_NONE;

Your suggestion is very good for us.
Your patch fixes our issue. I confirmed your patch worked well on our platform. I hope for that the patch is applied.
Thanks a lot.

Signature · 2 lines
ROHM Co., Ltd.
tomoya