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

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

[Yoshihiro Shimoda]

According to the xHCI spec "4.23.2 xHCI Power Management", the CRR bit
of CRCR register should be zero before setting Run/Stop (R/S) = '0'.
Otherwise, the STS_HALT is not set until the CRR is cleared on specific
xHCI controllers. In case of R-Car SoCs, it spent about 90 ms to clear
the CRR. So, to avoid the quirks XHCI_SLOW_SUSPEND, the driver calls
the aborting function if the CRR is set to 1.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/usb/host/xhci-ring.c |  2 +-
 drivers/usb/host/xhci.c      | 21 ++++++++++++++++++++-
 drivers/usb/host/xhci.h      |  1 +
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index f5397a5..21f3932 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -280,7 +280,7 @@ void xhci_ring_cmd_db(struct xhci_hcd *xhci)
 	readl(&xhci->dba->doorbell[0]);
 }
 
-static int xhci_abort_cmd_ring(struct xhci_hcd *xhci)
+int xhci_abort_cmd_ring(struct xhci_hcd *xhci)
 {
 	u64 temp_64;
 	int ret;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index ec8ac16..d2d81a0 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -892,7 +892,7 @@ static void xhci_disable_port_wake_on_bits(struct xhci_hcd *xhci)
  */
 int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
 {
-	int			rc = 0;
+	int			rc = 0, ret;
 	unsigned int		delay = XHCI_MAX_HALT_USEC;
 	struct usb_hcd		*hcd = xhci_to_hcd(xhci);
 	u32			command;
@@ -918,6 +918,25 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
 	/* step 1: stop endpoint */
 	/* skipped assuming that port suspend has done */
 
+	/*
+	 * According to the xHCI spec "4.23.2 xHCI Power Management", the CRR
+	 * bit of CRCR register should be zero before setting Run/Stop (R/S) +	 * '0', the driver calls the aborting function if the CRR is set to 1.
+	 */
+	if (xhci_read_64(xhci, &xhci->op_regs->cmd_ring) & CMD_RING_RUNNING) {
+		/* unlock here because this may wait about 5 seconds */
+		spin_unlock_irq(&xhci->lock);
+		ret = xhci_abort_cmd_ring(xhci);
+		if (unlikely(ret = -ESHUTDOWN)) {
+			xhci_err(xhci, "Abort command ring failed\n");
+			xhci_cleanup_command_queue(xhci);
+			usb_hc_died(xhci_to_hcd(xhci)->primary_hcd);
+			xhci_dbg(xhci, "xHCI host controller is dead.\n");
+			return ret;
+		}
+		spin_lock_irq(&xhci->lock);
+	}
+
 	/* step 2: clear Run/Stop bit */
 	command = readl(&xhci->op_regs->command);
 	command &= ~CMD_RUN;
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 8e421b8..c861bf0 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1796,6 +1796,7 @@ struct xhci_segment *trb_in_td(struct xhci_hcd *xhci,
 		union xhci_trb *end_trb, dma_addr_t suspect_dma, bool debug);
 int xhci_is_vendor_info_code(struct xhci_hcd *xhci, unsigned int trb_comp_code);
 void xhci_ring_cmd_db(struct xhci_hcd *xhci);
+int xhci_abort_cmd_ring(struct xhci_hcd *xhci);
 int xhci_queue_slot_control(struct xhci_hcd *xhci, struct xhci_command *cmd,
 		u32 trb_type, u32 slot_id);
 int xhci_queue_address_device(struct xhci_hcd *xhci, struct xhci_command *cmd,
-- 
1.9.1

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

[Mathias Nyman]

On 27.04.2015 11:55, Yoshihiro Shimoda wrote:
Hi

> According to the xHCI spec "4.23.2 xHCI Power Management", the CRR bit
> of CRCR register should be zero before setting Run/Stop (R/S) = '0'.
> Otherwise, the STS_HALT is not set until the CRR is cleared on specific
> xHCI controllers. In case of R-Car SoCs, it spent about 90 ms to clear
> the CRR. So, to avoid the quirks XHCI_SLOW_SUSPEND, the driver calls
> the aborting function if the CRR is set to 1.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/usb/host/xhci-ring.c |  2 +-
>  drivers/usb/host/xhci.c      | 21 ++++++++++++++++++++-
>  drivers/usb/host/xhci.h      |  1 +
>  3 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index f5397a5..21f3932 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -280,7 +280,7 @@ void xhci_ring_cmd_db(struct xhci_hcd *xhci)
>  	readl(&xhci->dba->doorbell[0]);
>  }
>  
> -static int xhci_abort_cmd_ring(struct xhci_hcd *xhci)
> +int xhci_abort_cmd_ring(struct xhci_hcd *xhci)
>  {
>  	u64 temp_64;
>  	int ret;
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index ec8ac16..d2d81a0 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -892,7 +892,7 @@ static void xhci_disable_port_wake_on_bits(struct xhci_hcd *xhci)
>   */
>  int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
>  {
> -	int			rc = 0;
> +	int			rc = 0, ret;
>  	unsigned int		delay = XHCI_MAX_HALT_USEC;
>  	struct usb_hcd		*hcd = xhci_to_hcd(xhci);
>  	u32			command;
> @@ -918,6 +918,25 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
>  	/* step 1: stop endpoint */
>  	/* skipped assuming that port suspend has done */
>  
> +	/*
> +	 * According to the xHCI spec "4.23.2 xHCI Power Management", the CRR
> +	 * bit of CRCR register should be zero before setting Run/Stop (R/S) > +	 * '0', the driver calls the aborting function if the CRR is set to 1.
> +	 */
> +	if (xhci_read_64(xhci, &xhci->op_regs->cmd_ring) & CMD_RING_RUNNING) {
> +		/* unlock here because this may wait about 5 seconds */
> +		spin_unlock_irq(&xhci->lock);
> +		ret = xhci_abort_cmd_ring(xhci);

Would it work for R-Car if we instead of unlocking and and aborting the command just wait for
the CRR bit to turn 0 before setting Run/stop = 0? 

Aborting the command ring by setting CA bit in CRCR will generate a command abort/stop completion event,
which will point to the stopped/aborted event on the command ring. We are however clearing the command
ring completely in xhci_suspend() right after this, and setting the dequeue pointer to the beginning of
the ring. This will likely mess with the handling of the abort/stop event.   

Waiting for the CRR to clear could be done using: 
xhci_handshake(&xhci->op_regs->cmd_ring, CMD_RING_RUNNING, timeout)

-Mathias

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

[Yoshihiro Shimoda]

Hi,

Thank you for the comments!

> Sent: Monday, April 27, 2015 10:15 PM
> 
> On 27.04.2015 11:55, Yoshihiro Shimoda wrote:
> Hi
> 
> > According to the xHCI spec "4.23.2 xHCI Power Management", the CRR bit
> > of CRCR register should be zero before setting Run/Stop (R/S) = '0'.
> > Otherwise, the STS_HALT is not set until the CRR is cleared on specific
> > xHCI controllers. In case of R-Car SoCs, it spent about 90 ms to clear
> > the CRR. So, to avoid the quirks XHCI_SLOW_SUSPEND, the driver calls
> > the aborting function if the CRR is set to 1.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/usb/host/xhci-ring.c |  2 +-
> >  drivers/usb/host/xhci.c      | 21 ++++++++++++++++++++-
> >  drivers/usb/host/xhci.h      |  1 +
> >  3 files changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > index f5397a5..21f3932 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
< snip >
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index ec8ac16..d2d81a0 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -892,7 +892,7 @@ static void xhci_disable_port_wake_on_bits(struct xhci_hcd *xhci)
> >   */
> >  int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
> >  {
> > -	int			rc = 0;
> > +	int			rc = 0, ret;
> >  	unsigned int		delay = XHCI_MAX_HALT_USEC;
> >  	struct usb_hcd		*hcd = xhci_to_hcd(xhci);
> >  	u32			command;
> > @@ -918,6 +918,25 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
> >  	/* step 1: stop endpoint */
> >  	/* skipped assuming that port suspend has done */
> >
> > +	/*
> > +	 * According to the xHCI spec "4.23.2 xHCI Power Management", the CRR
> > +	 * bit of CRCR register should be zero before setting Run/Stop (R/S) > > +	 * '0', the driver calls the aborting function if the CRR is set to 1.
> > +	 */
> > +	if (xhci_read_64(xhci, &xhci->op_regs->cmd_ring) & CMD_RING_RUNNING) {
> > +		/* unlock here because this may wait about 5 seconds */
> > +		spin_unlock_irq(&xhci->lock);
> > +		ret = xhci_abort_cmd_ring(xhci);
> 
> Would it work for R-Car if we instead of unlocking and and aborting the command just wait for
> the CRR bit to turn 0 before setting Run/stop = 0?

Unfortunately, it didn't work correctly. However, after setting Run/stop = 0, it worked correctly.
(According to the Table 36 of xHCI spec, the CRR bit will be cleared if the R/S but us cleared to 0.)

> Aborting the command ring by setting CA bit in CRCR will generate a command abort/stop completion event,
> which will point to the stopped/aborted event on the command ring. We are however clearing the command
> ring completely in xhci_suspend() right after this, and setting the dequeue pointer to the beginning of
> the ring. This will likely mess with the handling of the abort/stop event.

Thank you for the comment. I understood that this driver should not call aborting function here.

> Waiting for the CRR to clear could be done using:
> xhci_handshake(&xhci->op_regs->cmd_ring, CMD_RING_RUNNING, timeout)

Thank you for the suggestion.

As I tested above, after I applied the following patch, R-Car SoCs environment worked correctly.
However, I don't know that it is a reasonable solution for all xHCI controllers.
Should I add a new quirks? Or, should it use a XHCI_SLOW_SUSPEND instead of the below patch?
(If R-Car SoCs with XHCI_SLOW_SUSPEND, it also worked correctly.)

======================================
--- 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 using xhci_handshake().
+	 */
+	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;
======================================

Best regards,
Yoshihiro Shimoda

> -Mathias