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

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

[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 <tomoya.rohm@gmail.com>
---
 drivers/usb/host/ehci-hub.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index 256fbd4..7ddb62e 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -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);
 
-- 
1.7.7.6

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

[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 <tomoya.rohm@gmail.com>
---
 drivers/usb/host/ehci-pci.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index 01bb7241d..30f3ad6 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -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;
-- 
1.7.7.6

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

[Sergei Shtylyov]

Hello.

On 10-04-2012 8:07, Tomoya MORINAGA wrote:

> 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<tomoya.rohm@gmail.com>
> ---
>   drivers/usb/host/ehci-hub.c |   18 ++++++++++++++++++
>   1 files changed, 18 insertions(+), 0 deletions(-)

> diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
> index 256fbd4..7ddb62e 100644
> --- a/drivers/usb/host/ehci-hub.c
> +++ b/drivers/usb/host/ehci-hub.c
[...]
> @@ -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

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

[Alan Stern]

On Tue, 10 Apr 2012, Tomoya MORINAGA wrote:

> 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
===================================================================
--- usb-3.4.orig/drivers/usb/host/ehci-hcd.c
+++ 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;

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

[Tomoya MORINAGA]

On Tue, Apr 10, 2012 at 11:57 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 10 Apr 2012, Tomoya MORINAGA wrote:
>
>> 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
> ===================================================================
> --- usb-3.4.orig/drivers/usb/host/ehci-hcd.c
> +++ 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;
>

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.

-- 
ROHM Co., Ltd.
tomoya