xhci driver cannot call pci_set_power_state() on non-pci xhci host
controllers.
Signed-off-by: Henry Lin <[email protected]>
---
drivers/usb/host/xhci.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 6c17e3fe181a..1fc16763dcda 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -761,6 +761,8 @@ static void xhci_stop(struct usb_hcd *hcd)
mutex_unlock(&xhci->mutex);
}
+extern struct device_type pci_dev_type;
+
/*
* Shutdown HC (not bus-specific)
*
@@ -791,8 +793,12 @@ static void xhci_shutdown(struct usb_hcd *hcd)
readl(&xhci->op_regs->status));
/* Yet another workaround for spurious wakeups at shutdown with HSW */
- if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
- pci_set_power_state(to_pci_dev(hcd->self.sysdev), PCI_D3hot);
+ if (xhci->quirks & XHCI_SPURIOUS_WAKEUP) {
+ struct device *dev = hcd->self.sysdev;
+
+ if (dev->type == &pci_dev_type)
+ pci_set_power_state(to_pci_dev(dev), PCI_D3hot);
+ }
}
#ifdef CONFIG_PM
--
2.17.1
Xhci driver cannot call pci_set_power_state() on non-pci xhci host
controllers. For example, NVIDIA Tegra XHCI host controller which acts
as platform device with XHCI_SPURIOUS_WAKEUP quirk set in some platform
hits this issue during shutdown.
Signed-off-by: Henry Lin <[email protected]>
---
drivers/usb/host/xhci.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 6c17e3fe181a..61718b126d2b 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -791,8 +791,11 @@ static void xhci_shutdown(struct usb_hcd *hcd)
readl(&xhci->op_regs->status));
/* Yet another workaround for spurious wakeups at shutdown with HSW */
- if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
- pci_set_power_state(to_pci_dev(hcd->self.sysdev), PCI_D3hot);
+ if (xhci->quirks & XHCI_SPURIOUS_WAKEUP) {
+ if (dev_is_pci(hcd->self.sysdev))
+ pci_set_power_state(to_pci_dev(hcd->self.sysdev),
+ PCI_D3hot);
+ }
}
#ifdef CONFIG_PM
--
2.17.1
On 13.11.2019 3.49, Henry Lin wrote:
> Xhci driver cannot call pci_set_power_state() on non-pci xhci host
> controllers. For example, NVIDIA Tegra XHCI host controller which acts
> as platform device with XHCI_SPURIOUS_WAKEUP quirk set in some platform
> hits this issue during shutdown.
>
The XHCI_SPURIOUS_WAKEUP quirk both resets the controller at shutdown
and sets it to PCI D3 at remove/shutdown.
Is it so that the NVIDIA Tegra xHC just needs to be reset at shutdown?
The quirk is not set for Tegra xHC in current mainline kernel.
> Signed-off-by: Henry Lin <[email protected]>
> ---
> drivers/usb/host/xhci.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 6c17e3fe181a..61718b126d2b 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -791,8 +791,11 @@ static void xhci_shutdown(struct usb_hcd *hcd)
> readl(&xhci->op_regs->status));
>
> /* Yet another workaround for spurious wakeups at shutdown with HSW */
> - if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
> - pci_set_power_state(to_pci_dev(hcd->self.sysdev), PCI_D3hot);
> + if (xhci->quirks & XHCI_SPURIOUS_WAKEUP) {
> + if (dev_is_pci(hcd->self.sysdev))
> + pci_set_power_state(to_pci_dev(hcd->self.sysdev),
> + PCI_D3hot);
> + }
Agree that we shouldn't call PCI specific functions in the generic shutdown code.
Would be better if we could move the PCI parts to xhci-pci.c or hcd-pci.c
Maybe we need to add a xhci_pci_shutdown() function for the xhci pci driver
that could take care of the pci related shutdown quirks before calling
usb_hcd_pci_shutdown(), and call that instead of directly calling
usb_hcd_pci_shutdown().
-Mathias
> The XHCI_SPURIOUS_WAKEUP quirk both resets the controller at shutdown
> and sets it to PCI D3 at remove/shutdown.
> Is it so that the NVIDIA Tegra xHC just needs to be reset at shutdown?
> The quirk is not set for Tegra xHC in current mainline kernel.
Some versions of NVIDIA Tegra xHC support VF on virtualization environment. In that case, they need reset at shutdown. For now, Tegra xHC's VF support has not been submitted to mainline kernel.
> Agree that we shouldn't call PCI specific functions in the generic shutdown code.
> Would be better if we could move the PCI parts to xhci-pci.c or hcd-pci.c
> Maybe we need to add a xhci_pci_shutdown() function for the xhci pci driver
> that could take care of the pci related shutdown quirks before calling
> usb_hcd_pci_shutdown(), and call that instead of directly calling
> usb_hcd_pci_shutdown().
To keep original code sequence, I implemented this in a similar way to set xhci_pci_shutdown() as hc_driver.shutdown(). In xhci_pci_shutdown(), it calls xhci_shutdown() first and then does XHCI_SPURIOUS_WAKEUP quirk. Will post it as v3 patch.
Xhci driver cannot call pci_set_power_state() on non-pci xhci host
controllers. For example, NVIDIA Tegra XHCI host controller which acts
as platform device with XHCI_SPURIOUS_WAKEUP quirk set in some platform
hits this issue during shutdown.
Signed-off-by: Henry Lin <[email protected]>
---
drivers/usb/host/xhci-pci.c | 13 +++++++++++++
drivers/usb/host/xhci.c | 6 +-----
drivers/usb/host/xhci.h | 1 +
3 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 1e0236e90687..1904ef56f61c 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -519,6 +519,18 @@ static int xhci_pci_resume(struct usb_hcd *hcd, bool hibernated)
}
#endif /* CONFIG_PM */
+static void xhci_pci_shutdown(struct usb_hcd *hcd)
+{
+ struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+ struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
+
+ xhci_shutdown(hcd);
+
+ /* Yet another workaround for spurious wakeups at shutdown with HSW */
+ if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
+ pci_set_power_state(pdev, PCI_D3hot);
+}
+
/*-------------------------------------------------------------------------*/
/* PCI driver selection metadata; PCI hotplugging uses this */
@@ -554,6 +566,7 @@ static int __init xhci_pci_init(void)
#ifdef CONFIG_PM
xhci_pci_hc_driver.pci_suspend = xhci_pci_suspend;
xhci_pci_hc_driver.pci_resume = xhci_pci_resume;
+ xhci_pci_hc_driver.shutdown = xhci_pci_shutdown;
#endif
return pci_register_driver(&xhci_pci_driver);
}
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 6c17e3fe181a..e59346488f64 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -770,7 +770,7 @@ static void xhci_stop(struct usb_hcd *hcd)
*
* This will only ever be called with the main usb_hcd (the USB3 roothub).
*/
-static void xhci_shutdown(struct usb_hcd *hcd)
+void xhci_shutdown(struct usb_hcd *hcd)
{
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
@@ -789,10 +789,6 @@ static void xhci_shutdown(struct usb_hcd *hcd)
xhci_dbg_trace(xhci, trace_xhci_dbg_init,
"xhci_shutdown completed - status = %x",
readl(&xhci->op_regs->status));
-
- /* Yet another workaround for spurious wakeups at shutdown with HSW */
- if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
- pci_set_power_state(to_pci_dev(hcd->self.sysdev), PCI_D3hot);
}
#ifdef CONFIG_PM
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index f9f88626a57a..973d665052a2 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -2050,6 +2050,7 @@ int xhci_start(struct xhci_hcd *xhci);
int xhci_reset(struct xhci_hcd *xhci);
int xhci_run(struct usb_hcd *hcd);
int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks);
+void xhci_shutdown(struct usb_hcd *hcd);
void xhci_init_driver(struct hc_driver *drv,
const struct xhci_driver_overrides *over);
int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id);
--
2.17.1
On 19.11.2019 10.16, Henry Lin wrote:
> Xhci driver cannot call pci_set_power_state() on non-pci xhci host
> controllers. For example, NVIDIA Tegra XHCI host controller which acts
> as platform device with XHCI_SPURIOUS_WAKEUP quirk set in some platform
> hits this issue during shutdown.
>
> Signed-off-by: Henry Lin <[email protected]>
Thanks, looks great.
I like this solution.
Keeps original code sequence, removes pci specific calls from generic xhci code,
and at the same time it fixes the NVIDIA Tegra xHC shutdown issue.
Adding to queue
-Mathias
On Tue, Nov 19, 2019 at 04:16:56PM +0800, Henry Lin wrote:
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 6c17e3fe181a..e59346488f64 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -770,7 +770,7 @@ static void xhci_stop(struct usb_hcd *hcd)
> *
> * This will only ever be called with the main usb_hcd (the USB3 roothub).
> */
> -static void xhci_shutdown(struct usb_hcd *hcd)
> +void xhci_shutdown(struct usb_hcd *hcd)
> {
> struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>
> @@ -789,10 +789,6 @@ static void xhci_shutdown(struct usb_hcd *hcd)
> xhci_dbg_trace(xhci, trace_xhci_dbg_init,
> "xhci_shutdown completed - status = %x",
> readl(&xhci->op_regs->status));
> -
> - /* Yet another workaround for spurious wakeups at shutdown with HSW */
> - if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
> - pci_set_power_state(to_pci_dev(hcd->self.sysdev), PCI_D3hot);
> }
Shouldn't this function also now need to be EXPORTed?
Jack
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
>> @@ -770,7 +770,7 @@ static void xhci_stop(struct usb_hcd *hcd)
>> *
>> * This will only ever be called with the main usb_hcd (the USB3 roothub).
>> */
>> -static void xhci_shutdown(struct usb_hcd *hcd)
>> +void xhci_shutdown(struct usb_hcd *hcd)
>> {
>> struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>
>> @@ -789,10 +789,6 @@ static void xhci_shutdown(struct usb_hcd *hcd)
>> xhci_dbg_trace(xhci, trace_xhci_dbg_init,
>> "xhci_shutdown completed - status = %x",
>> readl(&xhci->op_regs->status));
>> -
>> - /* Yet another workaround for spurious wakeups at shutdown with HSW */
>> - if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
>> - pci_set_power_state(to_pci_dev(hcd->self.sysdev), PCI_D3hot);
>> }
>Shouldn't this function also now need to be EXPORTed?
Yes. I will add EXPORT_SYMBOL_GPL() for it.
Xhci driver cannot call pci_set_power_state() on non-pci xhci host
controllers. For example, NVIDIA Tegra XHCI host controller which acts
as platform device with XHCI_SPURIOUS_WAKEUP quirk set in some platform
hits this issue during shutdown.
Signed-off-by: Henry Lin <[email protected]>
---
v4:
1. export xhci_shutdown for CONFIG_USB_XHCI_PCI=m
---
drivers/usb/host/xhci-pci.c | 13 +++++++++++++
drivers/usb/host/xhci.c | 7 ++-----
drivers/usb/host/xhci.h | 1 +
3 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 1e0236e90687..1904ef56f61c 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -519,6 +519,18 @@ static int xhci_pci_resume(struct usb_hcd *hcd, bool hibernated)
}
#endif /* CONFIG_PM */
+static void xhci_pci_shutdown(struct usb_hcd *hcd)
+{
+ struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+ struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
+
+ xhci_shutdown(hcd);
+
+ /* Yet another workaround for spurious wakeups at shutdown with HSW */
+ if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
+ pci_set_power_state(pdev, PCI_D3hot);
+}
+
/*-------------------------------------------------------------------------*/
/* PCI driver selection metadata; PCI hotplugging uses this */
@@ -554,6 +566,7 @@ static int __init xhci_pci_init(void)
#ifdef CONFIG_PM
xhci_pci_hc_driver.pci_suspend = xhci_pci_suspend;
xhci_pci_hc_driver.pci_resume = xhci_pci_resume;
+ xhci_pci_hc_driver.shutdown = xhci_pci_shutdown;
#endif
return pci_register_driver(&xhci_pci_driver);
}
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 6c17e3fe181a..90aa811165f1 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -770,7 +770,7 @@ static void xhci_stop(struct usb_hcd *hcd)
*
* This will only ever be called with the main usb_hcd (the USB3 roothub).
*/
-static void xhci_shutdown(struct usb_hcd *hcd)
+void xhci_shutdown(struct usb_hcd *hcd)
{
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
@@ -789,11 +789,8 @@ static void xhci_shutdown(struct usb_hcd *hcd)
xhci_dbg_trace(xhci, trace_xhci_dbg_init,
"xhci_shutdown completed - status = %x",
readl(&xhci->op_regs->status));
-
- /* Yet another workaround for spurious wakeups at shutdown with HSW */
- if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
- pci_set_power_state(to_pci_dev(hcd->self.sysdev), PCI_D3hot);
}
+EXPORT_SYMBOL_GPL(xhci_shutdown);
#ifdef CONFIG_PM
static void xhci_save_registers(struct xhci_hcd *xhci)
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index f9f88626a57a..973d665052a2 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -2050,6 +2050,7 @@ int xhci_start(struct xhci_hcd *xhci);
int xhci_reset(struct xhci_hcd *xhci);
int xhci_run(struct usb_hcd *hcd);
int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks);
+void xhci_shutdown(struct usb_hcd *hcd);
void xhci_init_driver(struct hc_driver *drv,
const struct xhci_driver_overrides *over);
int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id);
--
2.17.1