[2/4] xhci: Add quirk to reset host back to default state at shutdown

Commit Message

Mathias Nyman Oct. 24, 2022, 2:27 p.m. UTC

From: Mathias Nyman <[e-mail]>

Systems based on Alder Lake P see significant boot time delay if
boot firmware tries to control usb ports in unexpected link states.

This is seen with self-powered usb devices that survive in U3 link
suspended state over S5.

A more generic solution to power off ports at shutdown was attempted in
commit 83810f84ecf1 ("xhci: turn off port power in shutdown")
but it caused regression.

Add host specific XHCI_RESET_TO_DEFAULT quirk which will reset host and
ports back to default state in shutdown.

Cc: [e-mail]
Signed-off-by: Mathias Nyman <[e-mail]>
---
 drivers/usb/host/xhci-pci.c |  4 ++++
 drivers/usb/host/xhci.c     | 10 ++++++++--
 drivers/usb/host/xhci.h     |  1 +
 3 files changed, 13 insertions(+), 2 deletions(-)

Comments

Reka Norman Oct. 26, 2022, 6:40 a.m. UTC | #1

On Wed, Oct 26, 2022 at 5:01 PM Mathias Nyman <[e-mail]> wrote:
>
> From: Mathias Nyman <[e-mail]>
>
> Systems based on Alder Lake P see significant boot time delay if
> boot firmware tries to control usb ports in unexpected link states.
>
> This is seen with self-powered usb devices that survive in U3 link
> suspended state over S5.
>
> A more generic solution to power off ports at shutdown was attempted in
> commit 83810f84ecf1 ("xhci: turn off port power in shutdown")
> but it caused regression.
>
> Add host specific XHCI_RESET_TO_DEFAULT quirk which will reset host and
> ports back to default state in shutdown.
>
> Cc: [e-mail]
> Signed-off-by: Mathias Nyman <[e-mail]>
> ---
>  drivers/usb/host/xhci-pci.c |  4 ++++
>  drivers/usb/host/xhci.c     | 10 ++++++++--
>  drivers/usb/host/xhci.h     |  1 +
>  3 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 6dd3102749b7..fbbd547ba12a 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -257,6 +257,10 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
>              pdev->device == PCI_DEVICE_ID_INTEL_DNV_XHCI))
>                 xhci->quirks |= XHCI_MISSING_CAS;
>
> +       if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> +           pdev->device == PCI_DEVICE_ID_INTEL_ALDER_LAKE_PCH_XHCI)

We need this quirk for ADL-N too (device ID 0x54ed). Would you mind
updating the patch? Or I can send a separate patch if you prefer.

> +               xhci->quirks |= XHCI_RESET_TO_DEFAULT;
> +
>         if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
>             (pdev->device == PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_XHCI ||
>              pdev->device == PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_XHCI ||
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 5176765c4013..79d7931c048a 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -810,9 +810,15 @@ void xhci_shutdown(struct usb_hcd *hcd)
>
>         spin_lock_irq(&xhci->lock);
>         xhci_halt(xhci);
> -       /* Workaround for spurious wakeups at shutdown with HSW */
> -       if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
> +
> +       /*
> +        * Workaround for spurious wakeps at shutdown with HSW, and for boot
> +        * firmware delay in ADL-P PCH if port are left in U3 at shutdown
> +        */
> +       if (xhci->quirks & XHCI_SPURIOUS_WAKEUP ||
> +           xhci->quirks & XHCI_RESET_TO_DEFAULT)
>                 xhci_reset(xhci, XHCI_RESET_SHORT_USEC);
> +
>         spin_unlock_irq(&xhci->lock);
>
>         xhci_cleanup_msix(xhci);
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index c0964fe8ac12..cc084d9505cd 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1897,6 +1897,7 @@ struct xhci_hcd {
>  #define XHCI_BROKEN_D3COLD     BIT_ULL(41)
>  #define XHCI_EP_CTX_BROKEN_DCS BIT_ULL(42)
>  #define XHCI_SUSPEND_RESUME_CLKS       BIT_ULL(43)
> +#define XHCI_RESET_TO_DEFAULT  BIT_ULL(44)
>
>         unsigned int            num_active_eps;
>         unsigned int            limit_active_eps;
> --
> 2.25.1
>

Greg KH Oct. 26, 2022, 7 a.m. UTC | #2

On Wed, Oct 26, 2022 at 05:40:10PM +1100, Reka Norman wrote:
> On Wed, Oct 26, 2022 at 5:01 PM Mathias Nyman <[e-mail]> wrote:
> >
> > From: Mathias Nyman <[e-mail]>
> >
> > Systems based on Alder Lake P see significant boot time delay if
> > boot firmware tries to control usb ports in unexpected link states.
> >
> > This is seen with self-powered usb devices that survive in U3 link
> > suspended state over S5.
> >
> > A more generic solution to power off ports at shutdown was attempted in
> > commit 83810f84ecf1 ("xhci: turn off port power in shutdown")
> > but it caused regression.
> >
> > Add host specific XHCI_RESET_TO_DEFAULT quirk which will reset host and
> > ports back to default state in shutdown.
> >
> > Cc: [e-mail]
> > Signed-off-by: Mathias Nyman <[e-mail]>
> > ---
> >  drivers/usb/host/xhci-pci.c |  4 ++++
> >  drivers/usb/host/xhci.c     | 10 ++++++++--
> >  drivers/usb/host/xhci.h     |  1 +
> >  3 files changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> > index 6dd3102749b7..fbbd547ba12a 100644
> > --- a/drivers/usb/host/xhci-pci.c
> > +++ b/drivers/usb/host/xhci-pci.c
> > @@ -257,6 +257,10 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
> >              pdev->device == PCI_DEVICE_ID_INTEL_DNV_XHCI))
> >                 xhci->quirks |= XHCI_MISSING_CAS;
> >
> > +       if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > +           pdev->device == PCI_DEVICE_ID_INTEL_ALDER_LAKE_PCH_XHCI)
> 
> We need this quirk for ADL-N too (device ID 0x54ed). Would you mind
> updating the patch? Or I can send a separate patch if you prefer.

A separate patch is required, please submit it.

thanks,

greg k-h

Reka Norman Oct. 27, 2022, 5:42 a.m. UTC | #3

On Wed, Oct 26, 2022 at 5:59 PM Greg KH <[e-mail]> wrote:
>
> On Wed, Oct 26, 2022 at 05:40:10PM +1100, Reka Norman wrote:
> > On Wed, Oct 26, 2022 at 5:01 PM Mathias Nyman <[e-mail]> wrote:
> > >
> > > From: Mathias Nyman <[e-mail]>
> > >
> > > Systems based on Alder Lake P see significant boot time delay if
> > > boot firmware tries to control usb ports in unexpected link states.
> > >
> > > This is seen with self-powered usb devices that survive in U3 link
> > > suspended state over S5.
> > >
> > > A more generic solution to power off ports at shutdown was attempted in
> > > commit 83810f84ecf1 ("xhci: turn off port power in shutdown")
> > > but it caused regression.
> > >
> > > Add host specific XHCI_RESET_TO_DEFAULT quirk which will reset host and
> > > ports back to default state in shutdown.
> > >
> > > Cc: [e-mail]
> > > Signed-off-by: Mathias Nyman <[e-mail]>
> > > ---
> > >  drivers/usb/host/xhci-pci.c |  4 ++++
> > >  drivers/usb/host/xhci.c     | 10 ++++++++--
> > >  drivers/usb/host/xhci.h     |  1 +
> > >  3 files changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> > > index 6dd3102749b7..fbbd547ba12a 100644
> > > --- a/drivers/usb/host/xhci-pci.c
> > > +++ b/drivers/usb/host/xhci-pci.c
> > > @@ -257,6 +257,10 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
> > >              pdev->device == PCI_DEVICE_ID_INTEL_DNV_XHCI))
> > >                 xhci->quirks |= XHCI_MISSING_CAS;
> > >
> > > +       if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > > +           pdev->device == PCI_DEVICE_ID_INTEL_ALDER_LAKE_PCH_XHCI)
> >
> > We need this quirk for ADL-N too (device ID 0x54ed). Would you mind
> > updating the patch? Or I can send a separate patch if you prefer.
>
> A separate patch is required, please submit it.

Done, thanks.
https://lore.kernel.org/linux-usb/[e-mail]

>
> thanks,
>
> greg k-h

Patch

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 6dd3102749b7..fbbd547ba12a 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -257,6 +257,10 @@  static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 	     pdev->device == PCI_DEVICE_ID_INTEL_DNV_XHCI))
 		xhci->quirks |= XHCI_MISSING_CAS;
 
+	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
+	    pdev->device == PCI_DEVICE_ID_INTEL_ALDER_LAKE_PCH_XHCI)
+		xhci->quirks |= XHCI_RESET_TO_DEFAULT;
+
 	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
 	    (pdev->device == PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_XHCI ||
 	     pdev->device == PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_XHCI ||
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 5176765c4013..79d7931c048a 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -810,9 +810,15 @@  void xhci_shutdown(struct usb_hcd *hcd)
 
 	spin_lock_irq(&xhci->lock);
 	xhci_halt(xhci);
-	/* Workaround for spurious wakeups at shutdown with HSW */
-	if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
+
+	/*
+	 * Workaround for spurious wakeps at shutdown with HSW, and for boot
+	 * firmware delay in ADL-P PCH if port are left in U3 at shutdown
+	 */
+	if (xhci->quirks & XHCI_SPURIOUS_WAKEUP ||
+	    xhci->quirks & XHCI_RESET_TO_DEFAULT)
 		xhci_reset(xhci, XHCI_RESET_SHORT_USEC);
+
 	spin_unlock_irq(&xhci->lock);
 
 	xhci_cleanup_msix(xhci);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index c0964fe8ac12..cc084d9505cd 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1897,6 +1897,7 @@  struct xhci_hcd {
 #define XHCI_BROKEN_D3COLD	BIT_ULL(41)
 #define XHCI_EP_CTX_BROKEN_DCS	BIT_ULL(42)
 #define XHCI_SUSPEND_RESUME_CLKS	BIT_ULL(43)
+#define XHCI_RESET_TO_DEFAULT	BIT_ULL(44)
 
 	unsigned int		num_active_eps;
 	unsigned int		limit_active_eps;