[PATCH v2 RFT] usb: xhci: Add to check CRR bit in xhci_suspend()

[PATCH v2 RFT] usb: xhci: Add to check CRR bit in xhci_suspend()

[Yoshihiro Shimoda]

The STS_HALT is not set until the CRR (CMD_RING_RUNNING) is cleared
on specific xHCI controllers (e.g. R-Car SoCs) after this driver set
the R/S (CMD_RUN) to 0. So, to avoid using a quirks XHCI_SLOW_SUSPEND,
this patch adds to wait for the CRR to clear.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 My environment (R-Car H2) could work correctly. I would like to know
 that this patch causes side effect or not. Also I'd like to know
 this patch can avoid the XHCI_SLOW_SUSPEND quirk on other environment
 (Fresco Logic XHCI controller).

 Changes from v1:
  - Remove a abort code.
  - Wait for the CRR to clear after this driver set the R/S to 0.
  - Rebase on the usb.git / usb-next branch.
    (commit id = aa519be34f45954f33a6c20430deac8e544a180f)

 drivers/usb/host/xhci.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index ec8ac16..a357917 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -923,6 +923,15 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
 	command &= ~CMD_RUN;
 	writel(command, &xhci->op_regs->command);
 
+	/*
+	 * The STS_HALT is not set until the CRR is cleared on specific
+	 * xHCI controllers (e.g. R-Car SoCs) after this driver set the R/S
+	 * to 0. So, to avoid using a quirks XHCI_SLOW_SUSPEND, this driver
+	 * waits for the CRR to clear.
+	 */
+	xhci_handshake(&xhci->op_regs->cmd_ring, CMD_RING_RUNNING, 0,
+		       5 * 1000 * 1000);
+
 	/* Some chips from Fresco Logic need an extraordinary delay */
 	delay *= (xhci->quirks & XHCI_SLOW_SUSPEND) ? 10 : 1;
 
-- 
1.9.1

Re: [PATCH v2 RFT] usb: xhci: Add to check CRR bit in xhci_suspend()

[Mathias Nyman]

On 14.05.2015 12:39, Yoshihiro Shimoda wrote:
> The STS_HALT is not set until the CRR (CMD_RING_RUNNING) is cleared
> on specific xHCI controllers (e.g. R-Car SoCs) after this driver set
> the R/S (CMD_RUN) to 0. So, to avoid using a quirks XHCI_SLOW_SUSPEND,
> this patch adds to wait for the CRR to clear.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  My environment (R-Car H2) could work correctly. I would like to know
>  that this patch causes side effect or not. Also I'd like to know
>  this patch can avoid the XHCI_SLOW_SUSPEND quirk on other environment
>  (Fresco Logic XHCI controller).
> 
>  Changes from v1:
>   - Remove a abort code.
>   - Wait for the CRR to clear after this driver set the R/S to 0.
>   - Rebase on the usb.git / usb-next branch.
>     (commit id = aa519be34f45954f33a6c20430deac8e544a180f)
> 
>  drivers/usb/host/xhci.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index ec8ac16..a357917 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -923,6 +923,15 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
>  	command &= ~CMD_RUN;
>  	writel(command, &xhci->op_regs->command);
>  
> +	/*
> +	 * The STS_HALT is not set until the CRR is cleared on specific
> +	 * xHCI controllers (e.g. R-Car SoCs) after this driver set the R/S
> +	 * to 0. So, to avoid using a quirks XHCI_SLOW_SUSPEND, this driver
> +	 * waits for the CRR to clear.
> +	 */
> +	xhci_handshake(&xhci->op_regs->cmd_ring, CMD_RING_RUNNING, 0,
> +		       5 * 1000 * 1000);
> +
>  	/* Some chips from Fresco Logic need an extraordinary delay */
>  	delay *= (xhci->quirks & XHCI_SLOW_SUSPEND) ? 10 : 1;
>  
> 

Hi, sorry about the delay.

Does polling for the command ring running bit really help?
We are anyway polling for the HCHalted (STS_HALT) status bit right after this,
and it should indicate the command ring was halted as well.

Is it possible that the R-Car xhci host controller halts just a tiny bit slower
than the default delay value, and the additional CRR check gives it
just enough time to HALT before timeout?

Any chance you could compare the actual halt time using your patch against 
the XHCI_SLOW_SUSPEND quirk?  (time from clearing CMD_RUN until STS_HALT is set)

-Mathias

RE: [PATCH v2 RFT] usb: xhci: Add to check CRR bit in xhci_suspend()

[Yoshihiro Shimoda]

Hi Mathias,

> Sent: Tuesday, May 26, 2015 8:45 PM
> 
> On 14.05.2015 12:39, Yoshihiro Shimoda wrote:
> > The STS_HALT is not set until the CRR (CMD_RING_RUNNING) is cleared
> > on specific xHCI controllers (e.g. R-Car SoCs) after this driver set
> > the R/S (CMD_RUN) to 0. So, to avoid using a quirks XHCI_SLOW_SUSPEND,
> > this patch adds to wait for the CRR to clear.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  My environment (R-Car H2) could work correctly. I would like to know
> >  that this patch causes side effect or not. Also I'd like to know
> >  this patch can avoid the XHCI_SLOW_SUSPEND quirk on other environment
> >  (Fresco Logic XHCI controller).
> >
> >  Changes from v1:
> >   - Remove a abort code.
> >   - Wait for the CRR to clear after this driver set the R/S to 0.
> >   - Rebase on the usb.git / usb-next branch.
> >     (commit id = aa519be34f45954f33a6c20430deac8e544a180f)
> >
> >  drivers/usb/host/xhci.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index ec8ac16..a357917 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -923,6 +923,15 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
> >  	command &= ~CMD_RUN;
> >  	writel(command, &xhci->op_regs->command);
> >
> > +	/*
> > +	 * The STS_HALT is not set until the CRR is cleared on specific
> > +	 * xHCI controllers (e.g. R-Car SoCs) after this driver set the R/S
> > +	 * to 0. So, to avoid using a quirks XHCI_SLOW_SUSPEND, this driver
> > +	 * waits for the CRR to clear.
> > +	 */
> > +	xhci_handshake(&xhci->op_regs->cmd_ring, CMD_RING_RUNNING, 0,
> > +		       5 * 1000 * 1000);
> > +
> >  	/* Some chips from Fresco Logic need an extraordinary delay */
> >  	delay *= (xhci->quirks & XHCI_SLOW_SUSPEND) ? 10 : 1;
> >
> >
> 
> Hi, sorry about the delay.

Thank you for your reply!
And, I'm sorry for the delayed response.

> Does polling for the command ring running bit really help?
> We are anyway polling for the HCHalted (STS_HALT) status bit right after this,
> and it should indicate the command ring was halted as well.
> 
> Is it possible that the R-Car xhci host controller halts just a tiny bit slower
> than the default delay value, and the additional CRR check gives it
> just enough time to HALT before timeout?

Yes or No.
If I used the XHCI_SLOW_SUSPEND quirk, the additional CRR check is not needed.

In last October, I submitted a patch to use the quirk. However, I don't have any
hardware documents why the R-Car xhci host controller needs this quirk:
http://thread.gmane.org/gmane.linux.usb.general/115965/focus\x116056

> Any chance you could compare the actual halt time using your patch against
> the XHCI_SLOW_SUSPEND quirk?  (time from clearing CMD_RUN until STS_HALT is set)

Using my patch:
 xhci_handshake: CMD_RUN  usec = 4930408
 xhci_handshake: STS_HALT usec =   16000

So, total waiting time was about 70msec.
(5000000 - 4930408 + 16000 - 16000 = 69592)

Without my patch + using XHCI_SLOW_SUSPEND quirk:
xhci_handshake: halt usec = 89796

So, total waiting time was about 70msec.
(160000 - 89796 = 70204)


Therefore, should I use the XHCI_SLOW_SUSPEND quirk instead of my patch?

Best regards,
Yoshihiro Shimoda

> -Mathias