| Message ID | 1565594692-23683-1-git-send-email-rtseng@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xhci: wait CNR when doing xhci resume | expand |
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?
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
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
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
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
(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.
> 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
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);