Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] xhci: wait CNR when doing xhci resume
@ 2019-08-12  7:24 Rick Tseng
  2019-08-12  8:12 ` Felipe Balbi
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Rick Tseng @ 2019-08-12  7:24 UTC (permalink / raw)
  To: mathias.nyman, gregkh; +Cc: linux-usb, Rick

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.
-----------------------------------------------------------------------------------

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] xhci: wait CNR when doing xhci resume
  2019-08-12  7:24 [PATCH] xhci: wait CNR when doing xhci resume Rick Tseng
@ 2019-08-12  8:12 ` Felipe Balbi
  2019-08-12  8:19 ` Oliver Neukum
  2019-08-12 13:39 ` Greg KH
  2 siblings, 0 replies; 8+ messages in thread
From: Felipe Balbi @ 2019-08-12  8:12 UTC (permalink / raw)
  To: Rick Tseng, mathias.nyman, gregkh; +Cc: linux-usb, Rick

[-- 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 --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] xhci: wait CNR when doing xhci resume
  2019-08-12  7:24 [PATCH] xhci: wait CNR when doing xhci resume Rick Tseng
  2019-08-12  8:12 ` Felipe Balbi
@ 2019-08-12  8:19 ` Oliver Neukum
  2019-08-13 10:39   ` Mathias Nyman
  2019-08-12 13:39 ` Greg KH
  2 siblings, 1 reply; 8+ messages in thread
From: Oliver Neukum @ 2019-08-12  8:19 UTC (permalink / raw)
  To: Rick Tseng, mathias.nyman, gregkh; +Cc: linux-usb

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] xhci: wait CNR when doing xhci resume
  2019-08-12  7:24 [PATCH] xhci: wait CNR when doing xhci resume Rick Tseng
  2019-08-12  8:12 ` Felipe Balbi
  2019-08-12  8:19 ` Oliver Neukum
@ 2019-08-12 13:39 ` Greg KH
  2 siblings, 0 replies; 8+ messages in thread
From: Greg KH @ 2019-08-12 13:39 UTC (permalink / raw)
  To: Rick Tseng; +Cc: mathias.nyman, linux-usb

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] xhci: wait CNR when doing xhci resume
  2019-08-12  8:19 ` Oliver Neukum
@ 2019-08-13 10:39   ` Mathias Nyman
  2019-08-13 12:33     ` Rick Tseng
  0 siblings, 1 reply; 8+ messages in thread
From: Mathias Nyman @ 2019-08-13 10:39 UTC (permalink / raw)
  To: Oliver Neukum, Rick Tseng, mathias.nyman, gregkh; +Cc: linux-usb

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] xhci: wait CNR when doing xhci resume
  2019-08-13 10:39   ` Mathias Nyman
@ 2019-08-13 12:33     ` Rick Tseng
  2019-08-13 12:36       ` Felipe Balbi
  0 siblings, 1 reply; 8+ messages in thread
From: Rick Tseng @ 2019-08-13 12:33 UTC (permalink / raw)
  To: Mathias Nyman, Oliver Neukum, mathias.nyman, gregkh; +Cc: linux-usb

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
 
 
     

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] xhci: wait CNR when doing xhci resume
  2019-08-13 12:33     ` Rick Tseng
@ 2019-08-13 12:36       ` Felipe Balbi
  2019-08-13 13:12         ` Mathias Nyman
  0 siblings, 1 reply; 8+ messages in thread
From: Felipe Balbi @ 2019-08-13 12:36 UTC (permalink / raw)
  To: Rick Tseng, Mathias Nyman, Oliver Neukum, mathias.nyman\, gregkh\
  Cc: linux-usb\


(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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] xhci: wait CNR when doing xhci resume
  2019-08-13 12:36       ` Felipe Balbi
@ 2019-08-13 13:12         ` Mathias Nyman
  0 siblings, 0 replies; 8+ messages in thread
From: Mathias Nyman @ 2019-08-13 13:12 UTC (permalink / raw)
  To: Felipe Balbi, Rick Tseng, Oliver Neukum, mathias.nyman, gregkh; +Cc: linux-usb

> 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-12  7:24 [PATCH] xhci: wait CNR when doing xhci resume Rick Tseng
2019-08-12  8:12 ` Felipe Balbi
2019-08-12  8:19 ` Oliver Neukum
2019-08-13 10:39   ` Mathias Nyman
2019-08-13 12:33     ` Rick Tseng
2019-08-13 12:36       ` Felipe Balbi
2019-08-13 13:12         ` Mathias Nyman
2019-08-12 13:39 ` Greg KH

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git