[PATCH] xhci: wait CNR when doing xhci resume

[PATCH] xhci: wait CNR when doing xhci resume

[Rick Tseng]

From: Rick <rtseng@nvidia.com>

NVIDIA 3.1 xHCI card would lose power when moving power state into D3Cold.
Thus we need to wait CNR bit to clear when xhci resmue as xhci init.

Signed-off-by: Rick <rtseng@nvidia.com>
---
 drivers/usb/host/xhci-pci.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 1e0236e..857ad8a 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -12,6 +12,7 @@
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/acpi.h>
+#include <linux/iopoll.h>
 
 #include "xhci.h"
 #include "xhci-trace.h"
@@ -455,6 +456,19 @@ static void xhci_pme_quirk(struct usb_hcd *hcd)
 	readl(reg);
 }
 
+static int xhci_poll_cnr(struct usb_hcd *hcd)
+{
+	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+	void __iomem *reg = &xhci->op_regs->status;
+	u32 result;
+	int ret;
+
+	ret = readl_poll_timeout_atomic(reg, result,
+					(result & STS_CNR) == 0,
+					1, 100 * 1000);
+	return ret;
+}
+
 static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
 {
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
@@ -508,6 +522,12 @@ static int xhci_pci_resume(struct usb_hcd *hcd, bool hibernated)
 	if (pdev->vendor == PCI_VENDOR_ID_INTEL)
 		usb_enable_intel_xhci_ports(pdev);
 
+	if (pdev->vendor == PCI_VENDOR_ID_NVIDIA) {
+		retval = xhci_poll_cnr(hcd);
+		if (retval != 0)
+			return retval;
+	}
+
 	if (xhci->quirks & XHCI_SSIC_PORT_UNUSED)
 		xhci_ssic_port_unused_quirk(hcd, false);
 
-- 
2.1.4


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [PATCH] xhci: wait CNR when doing xhci resume

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 873 bytes --]


Hi,

Rick Tseng <rtseng@nvidia.com> writes:
> +static int xhci_poll_cnr(struct usb_hcd *hcd)
> +{
> +	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> +	void __iomem *reg = &xhci->op_regs->status;
> +	u32 result;
> +	int ret;
> +
> +	ret = readl_poll_timeout_atomic(reg, result,
> +					(result & STS_CNR) == 0,
> +					1, 100 * 1000);
> +	return ret;
> +}
> +

instead of defining your own function...

>  static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
>  {
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> @@ -508,6 +522,12 @@ static int xhci_pci_resume(struct usb_hcd *hcd, bool hibernated)
>  	if (pdev->vendor == PCI_VENDOR_ID_INTEL)
>  		usb_enable_intel_xhci_ports(pdev);
>  
> +	if (pdev->vendor == PCI_VENDOR_ID_NVIDIA) {
> +		retval = xhci_poll_cnr(hcd);

you could just use xhci_handshake() here, right?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] xhci: wait CNR when doing xhci resume

[Oliver Neukum]

Am Montag, den 12.08.2019, 15:24 +0800 schrieb Rick Tseng:
> From: Rick <rtseng@nvidia.com>
> 
> NVIDIA 3.1 xHCI card would lose power when moving power state into D3Cold.
> Thus we need to wait CNR bit to clear when xhci resmue as xhci init.

Should any controller have CNR set? Why is this specific to a vendor?

	Regards
		Oliver

Re: [PATCH] xhci: wait CNR when doing xhci resume

[Mathias Nyman]

On 12.8.2019 11.19, Oliver Neukum wrote:
> Am Montag, den 12.08.2019, 15:24 +0800 schrieb Rick Tseng:
>> From: Rick <rtseng@nvidia.com>
>>
>> NVIDIA 3.1 xHCI card would lose power when moving power state into D3Cold.
>> Thus we need to wait CNR bit to clear when xhci resmue as xhci init.
> 
> Should any controller have CNR set? Why is this specific to a vendor?
> 

Good point, always checking CNR in resume sounds like a good idea.

And use xhci_handshake() as Felipe pointed out, just like in xhci_reset():

ret = xhci_handshake(&xhci->op_regs->status, STS_CNR, 0, 10 * 1000 * 1000);

Rick, would you like to write a patch for this?

-Mathias

RE: [PATCH] xhci: wait CNR when doing xhci resume

[Rick Tseng]

Hi Mathias,

Thanks for suggestion.
The reason I do not use xhci_handshake() is we get build fail when configuring below as module:
USB_XHCI_HCD = m
USB_XHCI_PCI = m

Fail message as below:
ERROR: "xhci_handshake" [drivers/usb/host/xhci-pci.ko] undefined!

So I write my own function to check CNR.

Thanks,
Rick

--
nvpublic

  
寄件者: Mathias Nyman <mathias.nyman@linux.intel.com>
 寄件日期: 2019年8月13日 上午 03:39
 收件者: Oliver Neukum <oneukum@suse.com>; Rick Tseng <rtseng@nvidia.com>; mathias.nyman@intel.com <mathias.nyman@intel.com>; gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>
 副本: linux-usb@vger.kernel.org <linux-usb@vger.kernel.org>
 主旨: Re: [PATCH] xhci: wait CNR when doing xhci resume 
   
 
On 12.8.2019 11.19, Oliver Neukum wrote:
 > Am Montag, den 12.08.2019, 15:24 +0800 schrieb Rick Tseng:
 >> From: Rick <rtseng@nvidia.com>
 >>
 >> NVIDIA 3.1 xHCI card would lose power when moving power state into D3Cold.
 >> Thus we need to wait CNR bit to clear when xhci resmue as xhci init.
 > 
 > Should any controller have CNR set? Why is this specific to a vendor?
 > 
 
 Good point, always checking CNR in resume sounds like a good idea.
 
 And use xhci_handshake() as Felipe pointed out, just like in xhci_reset():
 
 ret = xhci_handshake(&xhci->op_regs->status, STS_CNR, 0, 10 * 1000 * 1000);
 
 Rick, would you like to write a patch for this?
 
 -Mathias
 
 
     

RE: [PATCH] xhci: wait CNR when doing xhci resume

[Felipe Balbi]

(no top-posting, please)

Hi,

Rick Tseng <rtseng@nvidia.com> writes:

> Hi Mathias,
>
> Thanks for suggestion.
> The reason I do not use xhci_handshake() is we get build fail when configuring below as module:
> USB_XHCI_HCD = m
> USB_XHCI_PCI = m
>
> Fail message as below:
> ERROR: "xhci_handshake" [drivers/usb/host/xhci-pci.ko] undefined!
>
> So I write my own function to check CNR.

yeah, move that code to xhci_suspend(). It's valid for any XHCI host.

-- 
balbi

Re: [PATCH] xhci: wait CNR when doing xhci resume

[Mathias Nyman]

> Hi,
> 
> Rick Tseng <rtseng@nvidia.com> writes:
> 
>> Hi Mathias,
>>
>> Thanks for suggestion.
>> The reason I do not use xhci_handshake() is we get build fail when configuring below as module:
>> USB_XHCI_HCD = m
>> USB_XHCI_PCI = m
>>
>> Fail message as below:
>> ERROR: "xhci_handshake" [drivers/usb/host/xhci-pci.ko] undefined!
>>
>> So I write my own function to check CNR.

Adding EXPORT_SYMBOL_GPL(xhci_handshake) after the xhci_handshake() function in xhci.c
should solve that.

But I agree with Felipe that checking for Controller Not Ready (CNR)
is useful for any xHCI host, not just PCI xHCI hosts,
so move the CNR check to xhci_resume()

> 
> yeah, move that code to xhci_suspend(). It's valid for any XHCI host.
> 

xhci_resume()

-Mathias

Re: [PATCH] xhci: wait CNR when doing xhci resume

[Greg KH]

On Mon, Aug 12, 2019 at 03:24:52PM +0800, Rick Tseng wrote:
> From: Rick <rtseng@nvidia.com>
> 
> NVIDIA 3.1 xHCI card would lose power when moving power state into D3Cold.
> Thus we need to wait CNR bit to clear when xhci resmue as xhci init.
> 
> Signed-off-by: Rick <rtseng@nvidia.com>

We need a "full" name on the from and signed-off-by lines, please.

> ---
>  drivers/usb/host/xhci-pci.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 1e0236e..857ad8a 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -12,6 +12,7 @@
>  #include <linux/slab.h>
>  #include <linux/module.h>
>  #include <linux/acpi.h>
> +#include <linux/iopoll.h>
>  
>  #include "xhci.h"
>  #include "xhci-trace.h"
> @@ -455,6 +456,19 @@ static void xhci_pme_quirk(struct usb_hcd *hcd)
>  	readl(reg);
>  }
>  
> +static int xhci_poll_cnr(struct usb_hcd *hcd)
> +{
> +	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> +	void __iomem *reg = &xhci->op_regs->status;
> +	u32 result;
> +	int ret;
> +
> +	ret = readl_poll_timeout_atomic(reg, result,
> +					(result & STS_CNR) == 0,
> +					1, 100 * 1000);
> +	return ret;
> +}
> +
>  static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
>  {
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> @@ -508,6 +522,12 @@ static int xhci_pci_resume(struct usb_hcd *hcd, bool hibernated)
>  	if (pdev->vendor == PCI_VENDOR_ID_INTEL)
>  		usb_enable_intel_xhci_ports(pdev);
>  
> +	if (pdev->vendor == PCI_VENDOR_ID_NVIDIA) {

So all devices of this vendor need that?  Are you _sure_?  Why not just
blacklist a single device?

thanks,

greg k-h