[PATCH 0/6] usb: Set quirks for xhci/dwc3 host mode

[PATCH 0/6] usb: Set quirks for xhci/dwc3 host mode

[Thinh Nguyen]

This series add 3 new quirks for DWC_usb31 host mode:
 * XHCI_ISOC_BLOCKED_DISCONNECT
 * XHCI_LIMIT_FS_BI_INTR_EP
 * XHCI_LOST_DISCONNECT_QUIRK

Different versions of DWC_usb3x controllers have different quirks. Typically we
set them based on PCI device VID:PID or DT compatible strings. However, we know
that a particular IP version(s) may share a common quirk across different
platforms. We can enable these quirks based on the IP type and version number.
This simplifies the designer work and consolidate the logic check. To do this,
we will need to expose the xHCI quirks to the common header along with the
private platform structure.


Thinh Nguyen (6):
  usb: xhci: Move quirks definitions to common usb header
  usb: xhci: Check for blocked disconnection
  usb: xhci: Workaround undercalculated BW for fullspeed BI
  usb: xhci: Rename Compliance mode timer quirk
  usb: xhci: Workaround lost disconnect port status
  usb: dwc3: host: Set quirks base on version

 drivers/usb/dwc3/host.c         |  21 +++++
 drivers/usb/host/xhci-hub.c     |  12 ++-
 drivers/usb/host/xhci-mem.c     |  26 +++++++
 drivers/usb/host/xhci-plat.c    |   1 -
 drivers/usb/host/xhci-plat.h    |  25 ------
 drivers/usb/host/xhci-rcar.c    |   1 -
 drivers/usb/host/xhci-ring.c    |  76 ++++++++++++++++++
 drivers/usb/host/xhci.c         | 134 ++++++++++++++++++++++++--------
 drivers/usb/host/xhci.h         |  71 ++++-------------
 include/linux/usb/xhci-quirks.h |  80 +++++++++++++++++++
 10 files changed, 328 insertions(+), 119 deletions(-)
 delete mode 100644 drivers/usb/host/xhci-plat.h
 create mode 100644 include/linux/usb/xhci-quirks.h


base-commit: e9fcb07704fcef6fa6d0333fd2b3a62442eaf45b
-- 
2.28.0

[PATCH 1/6] usb: xhci: Move quirks definitions to common usb header

[Thinh Nguyen]

DWC3 (and possibly others such as CDNS3) will need to access these xHCI
quirks' definitions to initialize their hosts. Currently, to set these
quirks, we'd need to create new DT properties matching the quirks. This
may not be necessary as the driver can check the controller IP and
version at runtime to determine which quirks are needed. Let's move
these quirks' definitions to a common header under include/linux/usb so
DWC3 can properly access them.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/host/xhci-plat.c    |  1 -
 drivers/usb/host/xhci-plat.h    | 25 -----------
 drivers/usb/host/xhci-rcar.c    |  1 -
 drivers/usb/host/xhci.h         | 53 +----------------------
 include/linux/usb/xhci-quirks.h | 77 +++++++++++++++++++++++++++++++++
 5 files changed, 78 insertions(+), 79 deletions(-)
 delete mode 100644 drivers/usb/host/xhci-plat.h
 create mode 100644 include/linux/usb/xhci-quirks.h

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index c1edcc9b13ce..716ef3a338db 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -21,7 +21,6 @@
 #include <linux/usb/of.h>
 
 #include "xhci.h"
-#include "xhci-plat.h"
 #include "xhci-mvebu.h"
 #include "xhci-rcar.h"
 
diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h
deleted file mode 100644
index 561d0b7bce09..000000000000
--- a/drivers/usb/host/xhci-plat.h
+++ /dev/null
@@ -1,25 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- * xhci-plat.h - xHCI host controller driver platform Bus Glue.
- *
- * Copyright (C) 2015 Renesas Electronics Corporation
- */
-
-#ifndef _XHCI_PLAT_H
-#define _XHCI_PLAT_H
-
-#include "xhci.h"	/* for hcd_to_xhci() */
-
-struct xhci_plat_priv {
-	const char *firmware_name;
-	unsigned long long quirks;
-	int (*plat_setup)(struct usb_hcd *);
-	void (*plat_start)(struct usb_hcd *);
-	int (*init_quirk)(struct usb_hcd *);
-	int (*suspend_quirk)(struct usb_hcd *);
-	int (*resume_quirk)(struct usb_hcd *);
-};
-
-#define hcd_to_xhci_priv(h) ((struct xhci_plat_priv *)hcd_to_xhci(h)->priv)
-#define xhci_to_priv(x) ((struct xhci_plat_priv *)(x)->priv)
-#endif	/* _XHCI_PLAT_H */
diff --git a/drivers/usb/host/xhci-rcar.c b/drivers/usb/host/xhci-rcar.c
index 1bc4fe7b8c75..7690bee365fd 100644
--- a/drivers/usb/host/xhci-rcar.c
+++ b/drivers/usb/host/xhci-rcar.c
@@ -14,7 +14,6 @@
 #include <linux/sys_soc.h>
 
 #include "xhci.h"
-#include "xhci-plat.h"
 #include "xhci-rcar.h"
 
 /*
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 2595a8f057c4..9a4e2808668b 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -17,6 +17,7 @@
 #include <linux/kernel.h>
 #include <linux/usb/hcd.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/usb/xhci-quirks.h>
 
 /* Code sharing between pci-quirks and xhci hcd */
 #include	"xhci-ext-caps.h"
@@ -1840,58 +1841,6 @@ struct xhci_hcd {
 #define XHCI_STATE_HALTED	(1 << 1)
 #define XHCI_STATE_REMOVING	(1 << 2)
 	unsigned long long	quirks;
-#define	XHCI_LINK_TRB_QUIRK	BIT_ULL(0)
-#define XHCI_RESET_EP_QUIRK	BIT_ULL(1)
-#define XHCI_NEC_HOST		BIT_ULL(2)
-#define XHCI_AMD_PLL_FIX	BIT_ULL(3)
-#define XHCI_SPURIOUS_SUCCESS	BIT_ULL(4)
-/*
- * Certain Intel host controllers have a limit to the number of endpoint
- * contexts they can handle.  Ideally, they would signal that they can't handle
- * anymore endpoint contexts by returning a Resource Error for the Configure
- * Endpoint command, but they don't.  Instead they expect software to keep track
- * of the number of active endpoints for them, across configure endpoint
- * commands, reset device commands, disable slot commands, and address device
- * commands.
- */
-#define XHCI_EP_LIMIT_QUIRK	BIT_ULL(5)
-#define XHCI_BROKEN_MSI		BIT_ULL(6)
-#define XHCI_RESET_ON_RESUME	BIT_ULL(7)
-#define	XHCI_SW_BW_CHECKING	BIT_ULL(8)
-#define XHCI_AMD_0x96_HOST	BIT_ULL(9)
-#define XHCI_TRUST_TX_LENGTH	BIT_ULL(10)
-#define XHCI_LPM_SUPPORT	BIT_ULL(11)
-#define XHCI_INTEL_HOST		BIT_ULL(12)
-#define XHCI_SPURIOUS_REBOOT	BIT_ULL(13)
-#define XHCI_COMP_MODE_QUIRK	BIT_ULL(14)
-#define XHCI_AVOID_BEI		BIT_ULL(15)
-#define XHCI_PLAT		BIT_ULL(16)
-#define XHCI_SLOW_SUSPEND	BIT_ULL(17)
-#define XHCI_SPURIOUS_WAKEUP	BIT_ULL(18)
-/* For controllers with a broken beyond repair streams implementation */
-#define XHCI_BROKEN_STREAMS	BIT_ULL(19)
-#define XHCI_PME_STUCK_QUIRK	BIT_ULL(20)
-#define XHCI_MTK_HOST		BIT_ULL(21)
-#define XHCI_SSIC_PORT_UNUSED	BIT_ULL(22)
-#define XHCI_NO_64BIT_SUPPORT	BIT_ULL(23)
-#define XHCI_MISSING_CAS	BIT_ULL(24)
-/* For controller with a broken Port Disable implementation */
-#define XHCI_BROKEN_PORT_PED	BIT_ULL(25)
-#define XHCI_LIMIT_ENDPOINT_INTERVAL_7	BIT_ULL(26)
-#define XHCI_U2_DISABLE_WAKE	BIT_ULL(27)
-#define XHCI_ASMEDIA_MODIFY_FLOWCONTROL	BIT_ULL(28)
-#define XHCI_HW_LPM_DISABLE	BIT_ULL(29)
-#define XHCI_SUSPEND_DELAY	BIT_ULL(30)
-#define XHCI_INTEL_USB_ROLE_SW	BIT_ULL(31)
-#define XHCI_ZERO_64B_REGS	BIT_ULL(32)
-#define XHCI_DEFAULT_PM_RUNTIME_ALLOW	BIT_ULL(33)
-#define XHCI_RESET_PLL_ON_DISCONNECT	BIT_ULL(34)
-#define XHCI_SNPS_BROKEN_SUSPEND    BIT_ULL(35)
-#define XHCI_RENESAS_FW_QUIRK	BIT_ULL(36)
-#define XHCI_SKIP_PHY_INIT	BIT_ULL(37)
-#define XHCI_DISABLE_SPARSE	BIT_ULL(38)
-#define XHCI_SG_TRB_CACHE_SIZE_QUIRK	BIT_ULL(39)
-#define XHCI_NO_SOFT_RETRY	BIT_ULL(40)
 
 	unsigned int		num_active_eps;
 	unsigned int		limit_active_eps;
diff --git a/include/linux/usb/xhci-quirks.h b/include/linux/usb/xhci-quirks.h
new file mode 100644
index 000000000000..c2cb35c5b273
--- /dev/null
+++ b/include/linux/usb/xhci-quirks.h
@@ -0,0 +1,77 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * This file holds the definitions of quirks found in xHCI USB hosts.
+ */
+
+#ifndef __LINUX_USB_XHCI_QUIRKS_H
+#define __LINUX_USB_XHCI_QUIRKS_H
+
+#define	XHCI_LINK_TRB_QUIRK		BIT_ULL(0)
+#define XHCI_RESET_EP_QUIRK		BIT_ULL(1)
+#define XHCI_NEC_HOST			BIT_ULL(2)
+#define XHCI_AMD_PLL_FIX		BIT_ULL(3)
+#define XHCI_SPURIOUS_SUCCESS		BIT_ULL(4)
+/*
+ * Certain Intel host controllers have a limit to the number of endpoint
+ * contexts they can handle.  Ideally, they would signal that they can't handle
+ * anymore endpoint contexts by returning a Resource Error for the Configure
+ * Endpoint command, but they don't.  Instead they expect software to keep track
+ * of the number of active endpoints for them, across configure endpoint
+ * commands, reset device commands, disable slot commands, and address device
+ * commands.
+ */
+#define XHCI_EP_LIMIT_QUIRK		BIT_ULL(5)
+#define XHCI_BROKEN_MSI			BIT_ULL(6)
+#define XHCI_RESET_ON_RESUME		BIT_ULL(7)
+#define	XHCI_SW_BW_CHECKING		BIT_ULL(8)
+#define XHCI_AMD_0x96_HOST		BIT_ULL(9)
+#define XHCI_TRUST_TX_LENGTH		BIT_ULL(10)
+#define XHCI_LPM_SUPPORT		BIT_ULL(11)
+#define XHCI_INTEL_HOST			BIT_ULL(12)
+#define XHCI_SPURIOUS_REBOOT		BIT_ULL(13)
+#define XHCI_COMP_MODE_QUIRK		BIT_ULL(14)
+#define XHCI_AVOID_BEI			BIT_ULL(15)
+#define XHCI_PLAT			BIT_ULL(16)
+#define XHCI_SLOW_SUSPEND		BIT_ULL(17)
+#define XHCI_SPURIOUS_WAKEUP		BIT_ULL(18)
+/* For controllers with a broken beyond repair streams implementation */
+#define XHCI_BROKEN_STREAMS		BIT_ULL(19)
+#define XHCI_PME_STUCK_QUIRK		BIT_ULL(20)
+#define XHCI_MTK_HOST			BIT_ULL(21)
+#define XHCI_SSIC_PORT_UNUSED		BIT_ULL(22)
+#define XHCI_NO_64BIT_SUPPORT		BIT_ULL(23)
+#define XHCI_MISSING_CAS		BIT_ULL(24)
+/* For controller with a broken Port Disable implementation */
+#define XHCI_BROKEN_PORT_PED		BIT_ULL(25)
+#define XHCI_LIMIT_ENDPOINT_INTERVAL_7	BIT_ULL(26)
+#define XHCI_U2_DISABLE_WAKE		BIT_ULL(27)
+#define XHCI_ASMEDIA_MODIFY_FLOWCONTROL	BIT_ULL(28)
+#define XHCI_HW_LPM_DISABLE		BIT_ULL(29)
+#define XHCI_SUSPEND_DELAY		BIT_ULL(30)
+#define XHCI_INTEL_USB_ROLE_SW		BIT_ULL(31)
+#define XHCI_ZERO_64B_REGS		BIT_ULL(32)
+#define XHCI_DEFAULT_PM_RUNTIME_ALLOW	BIT_ULL(33)
+#define XHCI_RESET_PLL_ON_DISCONNECT	BIT_ULL(34)
+#define XHCI_SNPS_BROKEN_SUSPEND	BIT_ULL(35)
+#define XHCI_RENESAS_FW_QUIRK		BIT_ULL(36)
+#define XHCI_SKIP_PHY_INIT		BIT_ULL(37)
+#define XHCI_DISABLE_SPARSE		BIT_ULL(38)
+#define XHCI_SG_TRB_CACHE_SIZE_QUIRK	BIT_ULL(39)
+#define XHCI_NO_SOFT_RETRY		BIT_ULL(40)
+
+struct usb_hcd;
+
+struct xhci_plat_priv {
+	const char *firmware_name;
+	unsigned long long quirks;
+	int (*plat_setup)(struct usb_hcd *hcd);
+	void (*plat_start)(struct usb_hcd *hcd);
+	int (*init_quirk)(struct usb_hcd *hcd);
+	int (*suspend_quirk)(struct usb_hcd *hcd);
+	int (*resume_quirk)(struct usb_hcd *hcd);
+};
+
+#define hcd_to_xhci_priv(h) ((struct xhci_plat_priv *)hcd_to_xhci(h)->priv)
+#define xhci_to_priv(x) ((struct xhci_plat_priv *)(x)->priv)
+
+#endif /* __LINUX_USB_XHCI_QUIRKS_H */
-- 
2.28.0

Re: [PATCH 1/6] usb: xhci: Move quirks definitions to common usb header

[Greg Kroah-Hartman]

On Thu, Apr 08, 2021 at 06:41:59PM -0700, Thinh Nguyen wrote:
> DWC3 (and possibly others such as CDNS3) will need to access these xHCI
> quirks' definitions to initialize their hosts. Currently, to set these
> quirks, we'd need to create new DT properties matching the quirks. This
> may not be necessary as the driver can check the controller IP and
> version at runtime to determine which quirks are needed. Let's move
> these quirks' definitions to a common header under include/linux/usb so
> DWC3 can properly access them.
> 
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> ---
>  drivers/usb/host/xhci-plat.c    |  1 -
>  drivers/usb/host/xhci-plat.h    | 25 -----------
>  drivers/usb/host/xhci-rcar.c    |  1 -
>  drivers/usb/host/xhci.h         | 53 +----------------------
>  include/linux/usb/xhci-quirks.h | 77 +++++++++++++++++++++++++++++++++
>  5 files changed, 78 insertions(+), 79 deletions(-)
>  delete mode 100644 drivers/usb/host/xhci-plat.h
>  create mode 100644 include/linux/usb/xhci-quirks.h
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index c1edcc9b13ce..716ef3a338db 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -21,7 +21,6 @@
>  #include <linux/usb/of.h>
>  
>  #include "xhci.h"
> -#include "xhci-plat.h"
>  #include "xhci-mvebu.h"
>  #include "xhci-rcar.h"
>  
> diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h
> deleted file mode 100644
> index 561d0b7bce09..000000000000
> --- a/drivers/usb/host/xhci-plat.h
> +++ /dev/null
> @@ -1,25 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -/*
> - * xhci-plat.h - xHCI host controller driver platform Bus Glue.
> - *
> - * Copyright (C) 2015 Renesas Electronics Corporation
> - */
> -
> -#ifndef _XHCI_PLAT_H
> -#define _XHCI_PLAT_H
> -
> -#include "xhci.h"	/* for hcd_to_xhci() */
> -
> -struct xhci_plat_priv {
> -	const char *firmware_name;
> -	unsigned long long quirks;
> -	int (*plat_setup)(struct usb_hcd *);
> -	void (*plat_start)(struct usb_hcd *);
> -	int (*init_quirk)(struct usb_hcd *);
> -	int (*suspend_quirk)(struct usb_hcd *);
> -	int (*resume_quirk)(struct usb_hcd *);
> -};
> -
> -#define hcd_to_xhci_priv(h) ((struct xhci_plat_priv *)hcd_to_xhci(h)->priv)
> -#define xhci_to_priv(x) ((struct xhci_plat_priv *)(x)->priv)
> -#endif	/* _XHCI_PLAT_H */
> diff --git a/drivers/usb/host/xhci-rcar.c b/drivers/usb/host/xhci-rcar.c
> index 1bc4fe7b8c75..7690bee365fd 100644
> --- a/drivers/usb/host/xhci-rcar.c
> +++ b/drivers/usb/host/xhci-rcar.c
> @@ -14,7 +14,6 @@
>  #include <linux/sys_soc.h>
>  
>  #include "xhci.h"
> -#include "xhci-plat.h"
>  #include "xhci-rcar.h"
>  
>  /*
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 2595a8f057c4..9a4e2808668b 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -17,6 +17,7 @@
>  #include <linux/kernel.h>
>  #include <linux/usb/hcd.h>
>  #include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/usb/xhci-quirks.h>
>  
>  /* Code sharing between pci-quirks and xhci hcd */
>  #include	"xhci-ext-caps.h"
> @@ -1840,58 +1841,6 @@ struct xhci_hcd {
>  #define XHCI_STATE_HALTED	(1 << 1)
>  #define XHCI_STATE_REMOVING	(1 << 2)
>  	unsigned long long	quirks;
> -#define	XHCI_LINK_TRB_QUIRK	BIT_ULL(0)
> -#define XHCI_RESET_EP_QUIRK	BIT_ULL(1)
> -#define XHCI_NEC_HOST		BIT_ULL(2)
> -#define XHCI_AMD_PLL_FIX	BIT_ULL(3)
> -#define XHCI_SPURIOUS_SUCCESS	BIT_ULL(4)
> -/*
> - * Certain Intel host controllers have a limit to the number of endpoint
> - * contexts they can handle.  Ideally, they would signal that they can't handle
> - * anymore endpoint contexts by returning a Resource Error for the Configure
> - * Endpoint command, but they don't.  Instead they expect software to keep track
> - * of the number of active endpoints for them, across configure endpoint
> - * commands, reset device commands, disable slot commands, and address device
> - * commands.
> - */
> -#define XHCI_EP_LIMIT_QUIRK	BIT_ULL(5)
> -#define XHCI_BROKEN_MSI		BIT_ULL(6)
> -#define XHCI_RESET_ON_RESUME	BIT_ULL(7)
> -#define	XHCI_SW_BW_CHECKING	BIT_ULL(8)
> -#define XHCI_AMD_0x96_HOST	BIT_ULL(9)
> -#define XHCI_TRUST_TX_LENGTH	BIT_ULL(10)
> -#define XHCI_LPM_SUPPORT	BIT_ULL(11)
> -#define XHCI_INTEL_HOST		BIT_ULL(12)
> -#define XHCI_SPURIOUS_REBOOT	BIT_ULL(13)
> -#define XHCI_COMP_MODE_QUIRK	BIT_ULL(14)
> -#define XHCI_AVOID_BEI		BIT_ULL(15)
> -#define XHCI_PLAT		BIT_ULL(16)
> -#define XHCI_SLOW_SUSPEND	BIT_ULL(17)
> -#define XHCI_SPURIOUS_WAKEUP	BIT_ULL(18)
> -/* For controllers with a broken beyond repair streams implementation */
> -#define XHCI_BROKEN_STREAMS	BIT_ULL(19)
> -#define XHCI_PME_STUCK_QUIRK	BIT_ULL(20)
> -#define XHCI_MTK_HOST		BIT_ULL(21)
> -#define XHCI_SSIC_PORT_UNUSED	BIT_ULL(22)
> -#define XHCI_NO_64BIT_SUPPORT	BIT_ULL(23)
> -#define XHCI_MISSING_CAS	BIT_ULL(24)
> -/* For controller with a broken Port Disable implementation */
> -#define XHCI_BROKEN_PORT_PED	BIT_ULL(25)
> -#define XHCI_LIMIT_ENDPOINT_INTERVAL_7	BIT_ULL(26)
> -#define XHCI_U2_DISABLE_WAKE	BIT_ULL(27)
> -#define XHCI_ASMEDIA_MODIFY_FLOWCONTROL	BIT_ULL(28)
> -#define XHCI_HW_LPM_DISABLE	BIT_ULL(29)
> -#define XHCI_SUSPEND_DELAY	BIT_ULL(30)
> -#define XHCI_INTEL_USB_ROLE_SW	BIT_ULL(31)
> -#define XHCI_ZERO_64B_REGS	BIT_ULL(32)
> -#define XHCI_DEFAULT_PM_RUNTIME_ALLOW	BIT_ULL(33)
> -#define XHCI_RESET_PLL_ON_DISCONNECT	BIT_ULL(34)
> -#define XHCI_SNPS_BROKEN_SUSPEND    BIT_ULL(35)
> -#define XHCI_RENESAS_FW_QUIRK	BIT_ULL(36)
> -#define XHCI_SKIP_PHY_INIT	BIT_ULL(37)
> -#define XHCI_DISABLE_SPARSE	BIT_ULL(38)
> -#define XHCI_SG_TRB_CACHE_SIZE_QUIRK	BIT_ULL(39)
> -#define XHCI_NO_SOFT_RETRY	BIT_ULL(40)
>  
>  	unsigned int		num_active_eps;
>  	unsigned int		limit_active_eps;
> diff --git a/include/linux/usb/xhci-quirks.h b/include/linux/usb/xhci-quirks.h
> new file mode 100644
> index 000000000000..c2cb35c5b273
> --- /dev/null
> +++ b/include/linux/usb/xhci-quirks.h
> @@ -0,0 +1,77 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * This file holds the definitions of quirks found in xHCI USB hosts.
> + */
> +
> +#ifndef __LINUX_USB_XHCI_QUIRKS_H
> +#define __LINUX_USB_XHCI_QUIRKS_H
> +
> +#define	XHCI_LINK_TRB_QUIRK		BIT_ULL(0)
> +#define XHCI_RESET_EP_QUIRK		BIT_ULL(1)
> +#define XHCI_NEC_HOST			BIT_ULL(2)
> +#define XHCI_AMD_PLL_FIX		BIT_ULL(3)
> +#define XHCI_SPURIOUS_SUCCESS		BIT_ULL(4)
> +/*
> + * Certain Intel host controllers have a limit to the number of endpoint
> + * contexts they can handle.  Ideally, they would signal that they can't handle
> + * anymore endpoint contexts by returning a Resource Error for the Configure
> + * Endpoint command, but they don't.  Instead they expect software to keep track
> + * of the number of active endpoints for them, across configure endpoint
> + * commands, reset device commands, disable slot commands, and address device
> + * commands.
> + */
> +#define XHCI_EP_LIMIT_QUIRK		BIT_ULL(5)
> +#define XHCI_BROKEN_MSI			BIT_ULL(6)
> +#define XHCI_RESET_ON_RESUME		BIT_ULL(7)
> +#define	XHCI_SW_BW_CHECKING		BIT_ULL(8)
> +#define XHCI_AMD_0x96_HOST		BIT_ULL(9)
> +#define XHCI_TRUST_TX_LENGTH		BIT_ULL(10)
> +#define XHCI_LPM_SUPPORT		BIT_ULL(11)
> +#define XHCI_INTEL_HOST			BIT_ULL(12)
> +#define XHCI_SPURIOUS_REBOOT		BIT_ULL(13)
> +#define XHCI_COMP_MODE_QUIRK		BIT_ULL(14)
> +#define XHCI_AVOID_BEI			BIT_ULL(15)
> +#define XHCI_PLAT			BIT_ULL(16)
> +#define XHCI_SLOW_SUSPEND		BIT_ULL(17)
> +#define XHCI_SPURIOUS_WAKEUP		BIT_ULL(18)
> +/* For controllers with a broken beyond repair streams implementation */
> +#define XHCI_BROKEN_STREAMS		BIT_ULL(19)
> +#define XHCI_PME_STUCK_QUIRK		BIT_ULL(20)
> +#define XHCI_MTK_HOST			BIT_ULL(21)
> +#define XHCI_SSIC_PORT_UNUSED		BIT_ULL(22)
> +#define XHCI_NO_64BIT_SUPPORT		BIT_ULL(23)
> +#define XHCI_MISSING_CAS		BIT_ULL(24)
> +/* For controller with a broken Port Disable implementation */
> +#define XHCI_BROKEN_PORT_PED		BIT_ULL(25)
> +#define XHCI_LIMIT_ENDPOINT_INTERVAL_7	BIT_ULL(26)
> +#define XHCI_U2_DISABLE_WAKE		BIT_ULL(27)
> +#define XHCI_ASMEDIA_MODIFY_FLOWCONTROL	BIT_ULL(28)
> +#define XHCI_HW_LPM_DISABLE		BIT_ULL(29)
> +#define XHCI_SUSPEND_DELAY		BIT_ULL(30)
> +#define XHCI_INTEL_USB_ROLE_SW		BIT_ULL(31)
> +#define XHCI_ZERO_64B_REGS		BIT_ULL(32)
> +#define XHCI_DEFAULT_PM_RUNTIME_ALLOW	BIT_ULL(33)
> +#define XHCI_RESET_PLL_ON_DISCONNECT	BIT_ULL(34)
> +#define XHCI_SNPS_BROKEN_SUSPEND	BIT_ULL(35)
> +#define XHCI_RENESAS_FW_QUIRK		BIT_ULL(36)
> +#define XHCI_SKIP_PHY_INIT		BIT_ULL(37)
> +#define XHCI_DISABLE_SPARSE		BIT_ULL(38)
> +#define XHCI_SG_TRB_CACHE_SIZE_QUIRK	BIT_ULL(39)
> +#define XHCI_NO_SOFT_RETRY		BIT_ULL(40)
> +
> +struct usb_hcd;
> +
> +struct xhci_plat_priv {
> +	const char *firmware_name;
> +	unsigned long long quirks;
> +	int (*plat_setup)(struct usb_hcd *hcd);
> +	void (*plat_start)(struct usb_hcd *hcd);
> +	int (*init_quirk)(struct usb_hcd *hcd);
> +	int (*suspend_quirk)(struct usb_hcd *hcd);
> +	int (*resume_quirk)(struct usb_hcd *hcd);
> +};
> +
> +#define hcd_to_xhci_priv(h) ((struct xhci_plat_priv *)hcd_to_xhci(h)->priv)
> +#define xhci_to_priv(x) ((struct xhci_plat_priv *)(x)->priv)

Why do you need this tructure and #defines for xhci priv stuff need to
be in a public .h file?

I would think that at most you just need the xhci bit fields.

thanks,

greg k-h

Re: [PATCH 1/6] usb: xhci: Move quirks definitions to common usb header

[Thinh Nguyen]

Greg Kroah-Hartman wrote:
> On Thu, Apr 08, 2021 at 06:41:59PM -0700, Thinh Nguyen wrote:
>> DWC3 (and possibly others such as CDNS3) will need to access these xHCI
>> quirks' definitions to initialize their hosts. Currently, to set these
>> quirks, we'd need to create new DT properties matching the quirks. This
>> may not be necessary as the driver can check the controller IP and
>> version at runtime to determine which quirks are needed. Let's move
>> these quirks' definitions to a common header under include/linux/usb so
>> DWC3 can properly access them.
>>
>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>> ---
>>  drivers/usb/host/xhci-plat.c    |  1 -
>>  drivers/usb/host/xhci-plat.h    | 25 -----------
>>  drivers/usb/host/xhci-rcar.c    |  1 -
>>  drivers/usb/host/xhci.h         | 53 +----------------------
>>  include/linux/usb/xhci-quirks.h | 77 +++++++++++++++++++++++++++++++++
>>  5 files changed, 78 insertions(+), 79 deletions(-)
>>  delete mode 100644 drivers/usb/host/xhci-plat.h
>>  create mode 100644 include/linux/usb/xhci-quirks.h
>>
>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>> index c1edcc9b13ce..716ef3a338db 100644
>> --- a/drivers/usb/host/xhci-plat.c
>> +++ b/drivers/usb/host/xhci-plat.c
>> @@ -21,7 +21,6 @@
>>  #include <linux/usb/of.h>
>>  
>>  #include "xhci.h"
>> -#include "xhci-plat.h"
>>  #include "xhci-mvebu.h"
>>  #include "xhci-rcar.h"
>>  
>> diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h
>> deleted file mode 100644
>> index 561d0b7bce09..000000000000
>> --- a/drivers/usb/host/xhci-plat.h
>> +++ /dev/null
>> @@ -1,25 +0,0 @@
>> -/* SPDX-License-Identifier: GPL-2.0 */
>> -/*
>> - * xhci-plat.h - xHCI host controller driver platform Bus Glue.
>> - *
>> - * Copyright (C) 2015 Renesas Electronics Corporation
>> - */
>> -
>> -#ifndef _XHCI_PLAT_H
>> -#define _XHCI_PLAT_H
>> -
>> -#include "xhci.h"	/* for hcd_to_xhci() */
>> -
>> -struct xhci_plat_priv {
>> -	const char *firmware_name;
>> -	unsigned long long quirks;
>> -	int (*plat_setup)(struct usb_hcd *);
>> -	void (*plat_start)(struct usb_hcd *);
>> -	int (*init_quirk)(struct usb_hcd *);
>> -	int (*suspend_quirk)(struct usb_hcd *);
>> -	int (*resume_quirk)(struct usb_hcd *);
>> -};
>> -
>> -#define hcd_to_xhci_priv(h) ((struct xhci_plat_priv *)hcd_to_xhci(h)->priv)
>> -#define xhci_to_priv(x) ((struct xhci_plat_priv *)(x)->priv)
>> -#endif	/* _XHCI_PLAT_H */
>> diff --git a/drivers/usb/host/xhci-rcar.c b/drivers/usb/host/xhci-rcar.c
>> index 1bc4fe7b8c75..7690bee365fd 100644
>> --- a/drivers/usb/host/xhci-rcar.c
>> +++ b/drivers/usb/host/xhci-rcar.c
>> @@ -14,7 +14,6 @@
>>  #include <linux/sys_soc.h>
>>  
>>  #include "xhci.h"
>> -#include "xhci-plat.h"
>>  #include "xhci-rcar.h"
>>  
>>  /*
>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> index 2595a8f057c4..9a4e2808668b 100644
>> --- a/drivers/usb/host/xhci.h
>> +++ b/drivers/usb/host/xhci.h
>> @@ -17,6 +17,7 @@
>>  #include <linux/kernel.h>
>>  #include <linux/usb/hcd.h>
>>  #include <linux/io-64-nonatomic-lo-hi.h>
>> +#include <linux/usb/xhci-quirks.h>
>>  
>>  /* Code sharing between pci-quirks and xhci hcd */
>>  #include	"xhci-ext-caps.h"
>> @@ -1840,58 +1841,6 @@ struct xhci_hcd {
>>  #define XHCI_STATE_HALTED	(1 << 1)
>>  #define XHCI_STATE_REMOVING	(1 << 2)
>>  	unsigned long long	quirks;
>> -#define	XHCI_LINK_TRB_QUIRK	BIT_ULL(0)
>> -#define XHCI_RESET_EP_QUIRK	BIT_ULL(1)
>> -#define XHCI_NEC_HOST		BIT_ULL(2)
>> -#define XHCI_AMD_PLL_FIX	BIT_ULL(3)
>> -#define XHCI_SPURIOUS_SUCCESS	BIT_ULL(4)
>> -/*
>> - * Certain Intel host controllers have a limit to the number of endpoint
>> - * contexts they can handle.  Ideally, they would signal that they can't handle
>> - * anymore endpoint contexts by returning a Resource Error for the Configure
>> - * Endpoint command, but they don't.  Instead they expect software to keep track
>> - * of the number of active endpoints for them, across configure endpoint
>> - * commands, reset device commands, disable slot commands, and address device
>> - * commands.
>> - */
>> -#define XHCI_EP_LIMIT_QUIRK	BIT_ULL(5)
>> -#define XHCI_BROKEN_MSI		BIT_ULL(6)
>> -#define XHCI_RESET_ON_RESUME	BIT_ULL(7)
>> -#define	XHCI_SW_BW_CHECKING	BIT_ULL(8)
>> -#define XHCI_AMD_0x96_HOST	BIT_ULL(9)
>> -#define XHCI_TRUST_TX_LENGTH	BIT_ULL(10)
>> -#define XHCI_LPM_SUPPORT	BIT_ULL(11)
>> -#define XHCI_INTEL_HOST		BIT_ULL(12)
>> -#define XHCI_SPURIOUS_REBOOT	BIT_ULL(13)
>> -#define XHCI_COMP_MODE_QUIRK	BIT_ULL(14)
>> -#define XHCI_AVOID_BEI		BIT_ULL(15)
>> -#define XHCI_PLAT		BIT_ULL(16)
>> -#define XHCI_SLOW_SUSPEND	BIT_ULL(17)
>> -#define XHCI_SPURIOUS_WAKEUP	BIT_ULL(18)
>> -/* For controllers with a broken beyond repair streams implementation */
>> -#define XHCI_BROKEN_STREAMS	BIT_ULL(19)
>> -#define XHCI_PME_STUCK_QUIRK	BIT_ULL(20)
>> -#define XHCI_MTK_HOST		BIT_ULL(21)
>> -#define XHCI_SSIC_PORT_UNUSED	BIT_ULL(22)
>> -#define XHCI_NO_64BIT_SUPPORT	BIT_ULL(23)
>> -#define XHCI_MISSING_CAS	BIT_ULL(24)
>> -/* For controller with a broken Port Disable implementation */
>> -#define XHCI_BROKEN_PORT_PED	BIT_ULL(25)
>> -#define XHCI_LIMIT_ENDPOINT_INTERVAL_7	BIT_ULL(26)
>> -#define XHCI_U2_DISABLE_WAKE	BIT_ULL(27)
>> -#define XHCI_ASMEDIA_MODIFY_FLOWCONTROL	BIT_ULL(28)
>> -#define XHCI_HW_LPM_DISABLE	BIT_ULL(29)
>> -#define XHCI_SUSPEND_DELAY	BIT_ULL(30)
>> -#define XHCI_INTEL_USB_ROLE_SW	BIT_ULL(31)
>> -#define XHCI_ZERO_64B_REGS	BIT_ULL(32)
>> -#define XHCI_DEFAULT_PM_RUNTIME_ALLOW	BIT_ULL(33)
>> -#define XHCI_RESET_PLL_ON_DISCONNECT	BIT_ULL(34)
>> -#define XHCI_SNPS_BROKEN_SUSPEND    BIT_ULL(35)
>> -#define XHCI_RENESAS_FW_QUIRK	BIT_ULL(36)
>> -#define XHCI_SKIP_PHY_INIT	BIT_ULL(37)
>> -#define XHCI_DISABLE_SPARSE	BIT_ULL(38)
>> -#define XHCI_SG_TRB_CACHE_SIZE_QUIRK	BIT_ULL(39)
>> -#define XHCI_NO_SOFT_RETRY	BIT_ULL(40)
>>  
>>  	unsigned int		num_active_eps;
>>  	unsigned int		limit_active_eps;
>> diff --git a/include/linux/usb/xhci-quirks.h b/include/linux/usb/xhci-quirks.h
>> new file mode 100644
>> index 000000000000..c2cb35c5b273
>> --- /dev/null
>> +++ b/include/linux/usb/xhci-quirks.h
>> @@ -0,0 +1,77 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * This file holds the definitions of quirks found in xHCI USB hosts.
>> + */
>> +
>> +#ifndef __LINUX_USB_XHCI_QUIRKS_H
>> +#define __LINUX_USB_XHCI_QUIRKS_H
>> +
>> +#define	XHCI_LINK_TRB_QUIRK		BIT_ULL(0)
>> +#define XHCI_RESET_EP_QUIRK		BIT_ULL(1)
>> +#define XHCI_NEC_HOST			BIT_ULL(2)
>> +#define XHCI_AMD_PLL_FIX		BIT_ULL(3)
>> +#define XHCI_SPURIOUS_SUCCESS		BIT_ULL(4)
>> +/*
>> + * Certain Intel host controllers have a limit to the number of endpoint
>> + * contexts they can handle.  Ideally, they would signal that they can't handle
>> + * anymore endpoint contexts by returning a Resource Error for the Configure
>> + * Endpoint command, but they don't.  Instead they expect software to keep track
>> + * of the number of active endpoints for them, across configure endpoint
>> + * commands, reset device commands, disable slot commands, and address device
>> + * commands.
>> + */
>> +#define XHCI_EP_LIMIT_QUIRK		BIT_ULL(5)
>> +#define XHCI_BROKEN_MSI			BIT_ULL(6)
>> +#define XHCI_RESET_ON_RESUME		BIT_ULL(7)
>> +#define	XHCI_SW_BW_CHECKING		BIT_ULL(8)
>> +#define XHCI_AMD_0x96_HOST		BIT_ULL(9)
>> +#define XHCI_TRUST_TX_LENGTH		BIT_ULL(10)
>> +#define XHCI_LPM_SUPPORT		BIT_ULL(11)
>> +#define XHCI_INTEL_HOST			BIT_ULL(12)
>> +#define XHCI_SPURIOUS_REBOOT		BIT_ULL(13)
>> +#define XHCI_COMP_MODE_QUIRK		BIT_ULL(14)
>> +#define XHCI_AVOID_BEI			BIT_ULL(15)
>> +#define XHCI_PLAT			BIT_ULL(16)
>> +#define XHCI_SLOW_SUSPEND		BIT_ULL(17)
>> +#define XHCI_SPURIOUS_WAKEUP		BIT_ULL(18)
>> +/* For controllers with a broken beyond repair streams implementation */
>> +#define XHCI_BROKEN_STREAMS		BIT_ULL(19)
>> +#define XHCI_PME_STUCK_QUIRK		BIT_ULL(20)
>> +#define XHCI_MTK_HOST			BIT_ULL(21)
>> +#define XHCI_SSIC_PORT_UNUSED		BIT_ULL(22)
>> +#define XHCI_NO_64BIT_SUPPORT		BIT_ULL(23)
>> +#define XHCI_MISSING_CAS		BIT_ULL(24)
>> +/* For controller with a broken Port Disable implementation */
>> +#define XHCI_BROKEN_PORT_PED		BIT_ULL(25)
>> +#define XHCI_LIMIT_ENDPOINT_INTERVAL_7	BIT_ULL(26)
>> +#define XHCI_U2_DISABLE_WAKE		BIT_ULL(27)
>> +#define XHCI_ASMEDIA_MODIFY_FLOWCONTROL	BIT_ULL(28)
>> +#define XHCI_HW_LPM_DISABLE		BIT_ULL(29)
>> +#define XHCI_SUSPEND_DELAY		BIT_ULL(30)
>> +#define XHCI_INTEL_USB_ROLE_SW		BIT_ULL(31)
>> +#define XHCI_ZERO_64B_REGS		BIT_ULL(32)
>> +#define XHCI_DEFAULT_PM_RUNTIME_ALLOW	BIT_ULL(33)
>> +#define XHCI_RESET_PLL_ON_DISCONNECT	BIT_ULL(34)
>> +#define XHCI_SNPS_BROKEN_SUSPEND	BIT_ULL(35)
>> +#define XHCI_RENESAS_FW_QUIRK		BIT_ULL(36)
>> +#define XHCI_SKIP_PHY_INIT		BIT_ULL(37)
>> +#define XHCI_DISABLE_SPARSE		BIT_ULL(38)
>> +#define XHCI_SG_TRB_CACHE_SIZE_QUIRK	BIT_ULL(39)
>> +#define XHCI_NO_SOFT_RETRY		BIT_ULL(40)
>> +
>> +struct usb_hcd;
>> +
>> +struct xhci_plat_priv {
>> +	const char *firmware_name;
>> +	unsigned long long quirks;
>> +	int (*plat_setup)(struct usb_hcd *hcd);
>> +	void (*plat_start)(struct usb_hcd *hcd);
>> +	int (*init_quirk)(struct usb_hcd *hcd);
>> +	int (*suspend_quirk)(struct usb_hcd *hcd);
>> +	int (*resume_quirk)(struct usb_hcd *hcd);
>> +};
>> +
>> +#define hcd_to_xhci_priv(h) ((struct xhci_plat_priv *)hcd_to_xhci(h)->priv)
>> +#define xhci_to_priv(x) ((struct xhci_plat_priv *)(x)->priv)
> 
> Why do you need this tructure and #defines for xhci priv stuff need to
> be in a public .h file?
> 
> I would think that at most you just need the xhci bit fields.
> 
> thanks,
> 
> greg k-h
> 

Ah right.... What I wanted to do was to also expose the xhci-plat.h as a
common header for DWC3. I should leave that a separate file separate
from this.

Thanks,
Thinh

Re: [PATCH 1/6] usb: xhci: Move quirks definitions to common usb header

[kernel test robot]

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

Hi Thinh,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on e9fcb07704fcef6fa6d0333fd2b3a62442eaf45b]

url:    https://github.com/0day-ci/linux/commits/Thinh-Nguyen/usb-Set-quirks-for-xhci-dwc3-host-mode/20210409-094426
base:   e9fcb07704fcef6fa6d0333fd2b3a62442eaf45b
config: powerpc-randconfig-r032-20210409 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project dd453a1389b6a7e6d9214b449d3c54981b1a89b6)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # https://github.com/0day-ci/linux/commit/95d5f56cdd16958cbe3078c6678c4b4a88d2278b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Thinh-Nguyen/usb-Set-quirks-for-xhci-dwc3-host-mode/20210409-094426
        git checkout 95d5f56cdd16958cbe3078c6678c4b4a88d2278b
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/usb/cdns3/host.c:18:10: fatal error: '../host/xhci-plat.h' file not found
   #include "../host/xhci-plat.h"
            ^~~~~~~~~~~~~~~~~~~~~
   1 error generated.


vim +18 drivers/usb/cdns3/host.c

ed22764847e810 Peter Chen 2020-05-22 @18  #include "../host/xhci-plat.h"
ed22764847e810 Peter Chen 2020-05-22  19  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40485 bytes --]

Re: [PATCH 1/6] usb: xhci: Move quirks definitions to common usb header

[kernel test robot]

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

Hi Thinh,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on e9fcb07704fcef6fa6d0333fd2b3a62442eaf45b]

url:    https://github.com/0day-ci/linux/commits/Thinh-Nguyen/usb-Set-quirks-for-xhci-dwc3-host-mode/20210409-094426
base:   e9fcb07704fcef6fa6d0333fd2b3a62442eaf45b
config: powerpc-randconfig-r032-20210409 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project dd453a1389b6a7e6d9214b449d3c54981b1a89b6)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # https://github.com/0day-ci/linux/commit/95d5f56cdd16958cbe3078c6678c4b4a88d2278b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Thinh-Nguyen/usb-Set-quirks-for-xhci-dwc3-host-mode/20210409-094426
        git checkout 95d5f56cdd16958cbe3078c6678c4b4a88d2278b
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/usb/cdns3/host.c:18:10: fatal error: '../host/xhci-plat.h' file not found
   #include "../host/xhci-plat.h"
            ^~~~~~~~~~~~~~~~~~~~~
   1 error generated.


vim +18 drivers/usb/cdns3/host.c

ed22764847e810 Peter Chen 2020-05-22 @18  #include "../host/xhci-plat.h"
ed22764847e810 Peter Chen 2020-05-22  19  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 40485 bytes --]

Re: [PATCH 1/6] usb: xhci: Move quirks definitions to common usb header

[kernel test robot]

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

Hi Thinh,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on e9fcb07704fcef6fa6d0333fd2b3a62442eaf45b]

url:    https://github.com/0day-ci/linux/commits/Thinh-Nguyen/usb-Set-quirks-for-xhci-dwc3-host-mode/20210409-094426
base:   e9fcb07704fcef6fa6d0333fd2b3a62442eaf45b
config: openrisc-randconfig-r035-20210409 (attached as .config)
compiler: or1k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/95d5f56cdd16958cbe3078c6678c4b4a88d2278b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Thinh-Nguyen/usb-Set-quirks-for-xhci-dwc3-host-mode/20210409-094426
        git checkout 95d5f56cdd16958cbe3078c6678c4b4a88d2278b
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=openrisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/usb/cdns3/host.c:18:10: fatal error: ../host/xhci-plat.h: No such file or directory
      18 | #include "../host/xhci-plat.h"
         |          ^~~~~~~~~~~~~~~~~~~~~
   compilation terminated.


vim +18 drivers/usb/cdns3/host.c

ed22764847e810 Peter Chen 2020-05-22 @18  #include "../host/xhci-plat.h"
ed22764847e810 Peter Chen 2020-05-22  19  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 22642 bytes --]

Re: [PATCH 1/6] usb: xhci: Move quirks definitions to common usb header

[kernel test robot]

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

Hi Thinh,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on e9fcb07704fcef6fa6d0333fd2b3a62442eaf45b]

url:    https://github.com/0day-ci/linux/commits/Thinh-Nguyen/usb-Set-quirks-for-xhci-dwc3-host-mode/20210409-094426
base:   e9fcb07704fcef6fa6d0333fd2b3a62442eaf45b
config: openrisc-randconfig-r035-20210409 (attached as .config)
compiler: or1k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/95d5f56cdd16958cbe3078c6678c4b4a88d2278b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Thinh-Nguyen/usb-Set-quirks-for-xhci-dwc3-host-mode/20210409-094426
        git checkout 95d5f56cdd16958cbe3078c6678c4b4a88d2278b
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=openrisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/usb/cdns3/host.c:18:10: fatal error: ../host/xhci-plat.h: No such file or directory
      18 | #include "../host/xhci-plat.h"
         |          ^~~~~~~~~~~~~~~~~~~~~
   compilation terminated.


vim +18 drivers/usb/cdns3/host.c

ed22764847e810 Peter Chen 2020-05-22 @18  #include "../host/xhci-plat.h"
ed22764847e810 Peter Chen 2020-05-22  19  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 22642 bytes --]

[PATCH 2/6] usb: xhci: Check for blocked disconnection

[Thinh Nguyen]

If there is a device with active enhanced super-speed (eSS) isoc IN
endpoint(s) behind one or more eSS hubs, DWC_usb31 (v1.90a and prior)
host controller will not detect the device disconnection until no more
isoc URB is submitted. If there's a device disconnection, internally
the wait for tHostTransactionTimeout (USB 3.2 spec 8.13) blocks the
other endpoints from being scheduled. So, it blocks the interrupt
endpoint of the eSS hub indicating the port change status.

This can be an issue for applications that continuously submitting isoc
URBs to the xHCI. To work around this, stop processing new URBs after 3
consecutive isoc transaction errors. If new isoc transfers are queued
after the device is disconnected, the host will respond with USB
transaction error. After 3 consecutive USB transaction errors, the
driver can wait a period of time (at least 2 * largest periodic interval
of the topology) without ringing isoc endpoint doorbell to detect the
port change status. If there is no disconnection detected, ring the
endpoint doorbell to resume isoc transfers.

This workaround tracks the max eSS periodic interval every time there's
an endpoint added or dropped, which happens when there's bandwidth
check. So, scan the topology and update the xhci->max_ess_interval
whenever there's a bandwidth check. Introduced a new flag
VDEV_DISCONN_CHECK_PENDING to prevent ringing the doorbell while waiting
for a disconnection status. After 2 * max_ess_interval time and no
disconnection detected, a delayed work will ring the doorbell to resume
the active isoc transfers.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/host/xhci-mem.c     |  3 ++
 drivers/usb/host/xhci-ring.c    | 76 +++++++++++++++++++++++++++++++++
 drivers/usb/host/xhci.c         | 49 +++++++++++++++++++++
 drivers/usb/host/xhci.h         | 10 +++++
 include/linux/usb/xhci-quirks.h |  1 +
 5 files changed, 139 insertions(+)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index f66815fe8482..1053b43008e4 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -995,6 +995,8 @@ int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
 	xhci_dbg(xhci, "Slot %d input ctx = 0x%llx (dma)\n", slot_id,
 			(unsigned long long)dev->in_ctx->dma);
 
+	INIT_DELAYED_WORK(&dev->resume_isoc, xhci_resume_isoc);
+
 	/* Initialize the cancellation list and watchdog timers for each ep */
 	for (i = 0; i < 31; i++) {
 		dev->eps[i].ep_index = i;
@@ -1010,6 +1012,7 @@ int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
 		goto fail;
 
 	dev->udev = udev;
+	dev->xhci = xhci;
 
 	/* Point to output device context in dcbaa. */
 	xhci->dcbaa->dev_context_ptrs[slot_id] = cpu_to_le64(dev->out_ctx->dma);
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 05c38dd3ee36..a434a4b3609f 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -419,6 +419,14 @@ void xhci_ring_ep_doorbell(struct xhci_hcd *xhci,
 	struct xhci_virt_ep *ep = &xhci->devs[slot_id]->eps[ep_index];
 	unsigned int ep_state = ep->ep_state;
 
+	/*
+	 * Don't ring the doorbell for isoc IN endpoint while checking for
+	 * device disconnection.
+	 */
+	if (ep->ring && ep->ring->type == TYPE_ISOC && !(ep_index % 2) &&
+	    (xhci->devs[slot_id]->flags & VDEV_DISCONN_CHECK_PENDING))
+		return;
+
 	/* Don't ring the doorbell for this endpoint if there are pending
 	 * cancellations because we don't want to interrupt processing.
 	 * We don't want to restart any stream rings if there's a set dequeue
@@ -2330,6 +2338,8 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 		struct xhci_ring *ep_ring, struct xhci_td *td,
 		union xhci_trb *ep_trb, struct xhci_transfer_event *event)
 {
+	struct usb_device *udev;
+	struct xhci_virt_device *vdev;
 	struct urb_priv *urb_priv;
 	int idx;
 	struct usb_iso_packet_descriptor *frame;
@@ -2347,10 +2357,13 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 	ep_trb_len = TRB_LEN(le32_to_cpu(ep_trb->generic.field[2]));
 	short_framestatus = td->urb->transfer_flags & URB_SHORT_NOT_OK ?
 		-EREMOTEIO : 0;
+	udev = td->urb->dev;
+	vdev = xhci->devs[udev->slot_id];
 
 	/* handle completion code */
 	switch (trb_comp_code) {
 	case COMP_SUCCESS:
+		ep->ring->err_count = 0;
 		if (remaining) {
 			frame->status = short_framestatus;
 			if (xhci->quirks & XHCI_TRUST_TX_LENGTH)
@@ -2375,6 +2388,23 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 		frame->status = -EPROTO;
 		break;
 	case COMP_USB_TRANSACTION_ERROR:
+		if ((xhci->quirks & XHCI_ISOC_BLOCKED_DISCONNECT) &&
+		    usb_urb_dir_in(td->urb) &&
+		    udev->parent && udev->parent->parent &&
+		    udev->speed >= USB_SPEED_SUPER) {
+			if (!(vdev->flags & VDEV_DISCONN_CHECK_PENDING) &&
+			    ep->ring->err_count++ >= 3) {
+				unsigned long timeout;
+
+				/* Wait for at least max interval x 2 x 125us */
+				timeout = (1 << xhci->max_ess_interval) * 250;
+				vdev->flags |= VDEV_DISCONN_CHECK_PENDING;
+				queue_delayed_work(system_wq,
+						   &vdev->resume_isoc,
+						   usecs_to_jiffies(timeout));
+			}
+		}
+
 		frame->status = -EPROTO;
 		if (ep_trb != td->last_trb)
 			return 0;
@@ -4171,6 +4201,9 @@ int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags,
 	for (i = 0; i < num_tds; i++)
 		num_trbs += count_isoc_trbs_needed(urb, i);
 
+	if ((xdev->flags & VDEV_DISCONN_CHECK_PENDING) && usb_urb_dir_in(urb))
+		return -EINVAL;
+
 	/* Check the ring to guarantee there is enough room for the whole urb.
 	 * Do not insert any td of the urb to the ring if the check failed.
 	 */
@@ -4359,3 +4392,46 @@ int xhci_queue_reset_ep(struct xhci_hcd *xhci, struct xhci_command *cmd,
 	return queue_command(xhci, cmd, 0, 0, 0,
 			trb_slot_id | trb_ep_index | type, false);
 }
+
+void xhci_resume_isoc(struct work_struct *work)
+{
+	struct xhci_hcd *xhci;
+	struct xhci_virt_device *vdev;
+	unsigned int slot_id;
+	unsigned long flags;
+
+	vdev = container_of(to_delayed_work(work),
+			    struct xhci_virt_device, resume_isoc);
+	xhci = vdev->xhci;
+
+	spin_lock_irqsave(&xhci->lock, flags);
+
+	/* Check if the device is dropped before this work takes place */
+	if (!vdev->udev)
+		goto out;
+
+	slot_id = vdev->udev->slot_id;
+
+	vdev->flags &= ~VDEV_DISCONN_CHECK_PENDING;
+
+	/* Resume isoc transfers if the device is still connected */
+	if (xhci->devs[slot_id]) {
+		int i;
+
+		/* Ring doorbell for IN isoc endpoints only */
+		for (i = 2; i < 31; i += 2) {
+			struct xhci_virt_ep *ep = &vdev->eps[i];
+
+			if (!ep)
+				break;
+
+			if (ep->ring && ep->ring->type == TYPE_ISOC) {
+				ep->ring->err_count = 0;
+				ring_doorbell_for_active_rings(xhci, slot_id, i);
+			}
+		}
+	}
+
+out:
+	spin_unlock_irqrestore(&xhci->lock, flags);
+}
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index ca9385d22f68..e1136a6b9372 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -2973,6 +2973,44 @@ static void xhci_check_bw_drop_ep_streams(struct xhci_hcd *xhci,
 	}
 }
 
+static void xhci_update_ess_max_interval(struct xhci_hcd *xhci)
+{
+	unsigned int max_ess_interval = 0;
+	int j;
+
+	for (j = 1; j < HCS_MAX_SLOTS(xhci->hcs_params1); j++) {
+		struct xhci_virt_device	*virt_dev;
+		int i;
+
+		virt_dev = xhci->devs[j];
+		if (!virt_dev)
+			continue;
+
+		/* Only update for eSS devices */
+		if (virt_dev->udev &&
+		    virt_dev->udev->speed < USB_SPEED_SUPER)
+			continue;
+
+		for (i = 0; i < 31; i++) {
+			struct xhci_ep_ctx *ep_ctx;
+			unsigned int ep_type;
+			unsigned int interval;
+
+			ep_ctx = xhci_get_ep_ctx(xhci, virt_dev->out_ctx, i);
+			ep_type = CTX_TO_EP_TYPE(le32_to_cpu(ep_ctx->ep_info2));
+
+			if (xhci_is_async_ep(ep_type))
+				continue;
+
+			interval = CTX_TO_EP_INTERVAL(le32_to_cpu(ep_ctx->ep_info));
+			if (interval > max_ess_interval)
+				max_ess_interval = interval;
+		}
+	}
+
+	xhci->max_ess_interval = max_ess_interval;
+}
+
 /* Called after one or more calls to xhci_add_endpoint() or
  * xhci_drop_endpoint().  If this call fails, the USB core is expected
  * to call xhci_reset_bandwidth().
@@ -3047,6 +3085,17 @@ int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
 		/* Callee should call reset_bandwidth() */
 		goto command_cleanup;
 
+	if (xhci->quirks & XHCI_ISOC_BLOCKED_DISCONNECT) {
+		xhci_update_ess_max_interval(xhci);
+
+		/* Cancel disconnection check on change of context */
+		if (delayed_work_pending(&virt_dev->resume_isoc) &&
+		    ctrl_ctx->drop_flags) {
+			cancel_delayed_work(&virt_dev->resume_isoc);
+			virt_dev->flags &= ~VDEV_DISCONN_CHECK_PENDING;
+		}
+	}
+
 	/* Free any rings that were dropped, but not changed. */
 	for (i = 1; i < 31; i++) {
 		if ((le32_to_cpu(ctrl_ctx->drop_flags) & (1 << (i + 1))) &&
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 9a4e2808668b..27d2c1176dd1 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1000,6 +1000,7 @@ struct xhci_interval_bw_table {
 
 struct xhci_virt_device {
 	int				slot_id;
+	struct xhci_hcd			*xhci;
 	struct usb_device		*udev;
 	/*
 	 * Commands to the hardware are passed an "input context" that
@@ -1025,11 +1026,15 @@ struct xhci_virt_device {
 	 */
 	unsigned long			flags;
 #define VDEV_PORT_ERROR			BIT(0) /* Port error, link inactive */
+#define VDEV_DISCONN_CHECK_PENDING	BIT(1) /* Disconnection check */
 
 	/* The current max exit latency for the enabled USB3 link states. */
 	u16				current_mel;
 	/* Used for the debugfs interfaces. */
 	void				*debugfs_private;
+
+	/* For undetected disconnection quirk */
+	struct delayed_work		resume_isoc;
 };
 
 /*
@@ -1864,6 +1869,9 @@ struct xhci_hcd {
 /* Compliance Mode Timer Triggered every 2 seconds */
 #define COMP_MODE_RCVRY_MSECS 2000
 
+	/* Track max eSS interval for XHCI_ISOC_BLOCKED_DISCONNECT */
+	unsigned int		max_ess_interval;
+
 	struct dentry		*debugfs_root;
 	struct dentry		*debugfs_slots;
 	struct list_head	regset_list;
@@ -1948,6 +1956,8 @@ char *xhci_get_slot_state(struct xhci_hcd *xhci,
 void xhci_dbg_trace(struct xhci_hcd *xhci, void (*trace)(struct va_format *),
 			const char *fmt, ...);
 
+void xhci_resume_isoc(struct work_struct *work);
+
 /* xHCI memory management */
 void xhci_mem_cleanup(struct xhci_hcd *xhci);
 int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags);
diff --git a/include/linux/usb/xhci-quirks.h b/include/linux/usb/xhci-quirks.h
index c2cb35c5b273..c78638ba4735 100644
--- a/include/linux/usb/xhci-quirks.h
+++ b/include/linux/usb/xhci-quirks.h
@@ -58,6 +58,7 @@
 #define XHCI_DISABLE_SPARSE		BIT_ULL(38)
 #define XHCI_SG_TRB_CACHE_SIZE_QUIRK	BIT_ULL(39)
 #define XHCI_NO_SOFT_RETRY		BIT_ULL(40)
+#define XHCI_ISOC_BLOCKED_DISCONNECT	BIT_ULL(41)
 
 struct usb_hcd;
 
-- 
2.28.0

[PATCH 3/6] usb: xhci: Workaround undercalculated BW for fullspeed BI

[Thinh Nguyen]

DWC_usb31 host version 1.90a and prior undercalculates the bandwidth
available for interrupt endpoints. The controller will return bandwidth
error on config endpoint commands if there are already 6 or more
fullspeed interrupt endpoints with bInterval of 4 (or 4ms) associated
with a single fullspeed bus instance (BI).

To workaround this, configure and use the endpoint at a shorter
interrupt interval. Lower the ep_ctx interval from 5 to 4 (or 2ms)
for interrupt endpoints of the fullspeed BI. Note: we have not observed
functional impact to the fullspeed devices by lowering the interrupt
service interval (at least for a few devices that we tested).

To simplify the workaround, let's just check and apply the workaround if
the endpoint is a fullspeed interrupt endpoint with interval of 4ms and
if the top parent device is also operating in fullspeed (i.e. associated
with fullspeed BI).

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
Note:
Checkpatch will give a warning below for getting top_dev:
	WARNING: suspect code indent for conditional statements

Since this logic is done everywhere else in the driver, I'm keeping it
consistent here.

 drivers/usb/host/xhci-mem.c     | 23 +++++++++++++++++++++++
 include/linux/usb/xhci-quirks.h |  1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 1053b43008e4..e01d0ddb012a 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1463,6 +1463,29 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,
 		}
 	}
 
+	/*
+	 * Check for undercalculated bandwidth quirk for interrupt endpoints
+	 * associated with fullspeed BI.
+	 */
+	if ((xhci->quirks & XHCI_LIMIT_FS_BI_INTR_EP) &&
+	    usb_endpoint_xfer_int(&ep->desc) &&
+	    udev->speed == USB_SPEED_FULL &&
+	    interval == 5) {
+		struct usb_device *top_dev;
+
+		for (top_dev = udev;
+		     top_dev->parent && top_dev->parent->parent;
+		     top_dev = top_dev->parent)
+			/* Found device below root hub */;
+
+		/*
+		 * If the top device is operating at fullspeed, then the
+		 * controller is using fullspeed BI for this device.
+		 */
+		if (top_dev->speed == USB_SPEED_FULL)
+			interval = 4;
+	}
+
 	mult = xhci_get_endpoint_mult(udev, ep);
 	max_packet = usb_endpoint_maxp(&ep->desc);
 	max_burst = xhci_get_endpoint_max_burst(udev, ep);
diff --git a/include/linux/usb/xhci-quirks.h b/include/linux/usb/xhci-quirks.h
index c78638ba4735..3a8566c902be 100644
--- a/include/linux/usb/xhci-quirks.h
+++ b/include/linux/usb/xhci-quirks.h
@@ -59,6 +59,7 @@
 #define XHCI_SG_TRB_CACHE_SIZE_QUIRK	BIT_ULL(39)
 #define XHCI_NO_SOFT_RETRY		BIT_ULL(40)
 #define XHCI_ISOC_BLOCKED_DISCONNECT	BIT_ULL(41)
+#define XHCI_LIMIT_FS_BI_INTR_EP	BIT_ULL(42)
 
 struct usb_hcd;
 
-- 
2.28.0

[PATCH 4/6] usb: xhci: Rename Compliance mode timer quirk

[Thinh Nguyen]

In preparation for a workaround that needs to poll for the port status,
rename the timer for XHCI_COMP_MODE_QUIRK to be more generic as it can
be used for the new workaround. No funtional change here.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/host/xhci-hub.c |  2 +-
 drivers/usb/host/xhci.c     | 41 +++++++++++++++++--------------------
 drivers/usb/host/xhci.h     |  8 ++++----
 3 files changed, 24 insertions(+), 27 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index e9b18fc17617..8bfafbd680ab 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -893,7 +893,7 @@ static void xhci_del_comp_mod_timer(struct xhci_hcd *xhci, u32 status,
 	if ((xhci->port_status_u0 != all_ports_seen_u0) && port_in_u0) {
 		xhci->port_status_u0 |= 1 << wIndex;
 		if (xhci->port_status_u0 == all_ports_seen_u0) {
-			del_timer_sync(&xhci->comp_mode_recovery_timer);
+			del_timer_sync(&xhci->port_check_timer);
 			xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
 				"All USB3 ports have entered U0 already!");
 			xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index e1136a6b9372..e1b3d1063f6b 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -475,7 +475,7 @@ static inline void xhci_msix_sync_irqs(struct xhci_hcd *xhci)
 
 #endif
 
-static void compliance_mode_recovery(struct timer_list *t)
+static void port_check(struct timer_list *t)
 {
 	struct xhci_hcd *xhci;
 	struct usb_hcd *hcd;
@@ -483,7 +483,7 @@ static void compliance_mode_recovery(struct timer_list *t)
 	u32 temp;
 	int i;
 
-	xhci = from_timer(xhci, t, comp_mode_recovery_timer);
+	xhci = from_timer(xhci, t, port_check_timer);
 	rhub = &xhci->usb3_rhub;
 
 	for (i = 0; i < rhub->num_ports; i++) {
@@ -508,8 +508,8 @@ static void compliance_mode_recovery(struct timer_list *t)
 	}
 
 	if (xhci->port_status_u0 != ((1 << rhub->num_ports) - 1))
-		mod_timer(&xhci->comp_mode_recovery_timer,
-			jiffies + msecs_to_jiffies(COMP_MODE_RCVRY_MSECS));
+		mod_timer(&xhci->port_check_timer,
+			jiffies + msecs_to_jiffies(PORT_CHECK_MSECS));
 }
 
 /*
@@ -522,15 +522,14 @@ static void compliance_mode_recovery(struct timer_list *t)
  * status event is generated when entering compliance mode (per xhci spec),
  * this quirk is needed on systems that have the failing hardware installed.
  */
-static void compliance_mode_recovery_timer_init(struct xhci_hcd *xhci)
+static void port_check_timer_init(struct xhci_hcd *xhci)
 {
 	xhci->port_status_u0 = 0;
-	timer_setup(&xhci->comp_mode_recovery_timer, compliance_mode_recovery,
-		    0);
-	xhci->comp_mode_recovery_timer.expires = jiffies +
-			msecs_to_jiffies(COMP_MODE_RCVRY_MSECS);
+	timer_setup(&xhci->port_check_timer, port_check, 0);
+	xhci->port_check_timer.expires = jiffies +
+			msecs_to_jiffies(PORT_CHECK_MSECS);
 
-	add_timer(&xhci->comp_mode_recovery_timer);
+	add_timer(&xhci->port_check_timer);
 	xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
 			"Compliance mode recovery timer initialized");
 }
@@ -596,7 +595,7 @@ static int xhci_init(struct usb_hcd *hcd)
 	/* Initializing Compliance Mode Recovery Data If Needed */
 	if (xhci_compliance_mode_recovery_timer_quirk_check()) {
 		xhci->quirks |= XHCI_COMP_MODE_QUIRK;
-		compliance_mode_recovery_timer_init(xhci);
+		port_check_timer_init(xhci);
 	}
 
 	return retval;
@@ -739,10 +738,9 @@ static void xhci_stop(struct usb_hcd *hcd)
 	/* Deleting Compliance Mode Recovery Timer */
 	if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
 			(!(xhci_all_ports_seen_u0(xhci)))) {
-		del_timer_sync(&xhci->comp_mode_recovery_timer);
+		del_timer_sync(&xhci->port_check_timer);
 		xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
-				"%s: compliance mode recovery timer deleted",
-				__func__);
+				"%s: port check timer deleted", __func__);
 	}
 
 	if (xhci->quirks & XHCI_AMD_PLL_FIX)
@@ -1057,15 +1055,14 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
 	spin_unlock_irq(&xhci->lock);
 
 	/*
-	 * Deleting Compliance Mode Recovery Timer because the xHCI Host
+	 * Deleting Port Check Timer because the xHCI Host
 	 * is about to be suspended.
 	 */
 	if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
 			(!(xhci_all_ports_seen_u0(xhci)))) {
-		del_timer_sync(&xhci->comp_mode_recovery_timer);
+		del_timer_sync(&xhci->port_check_timer);
 		xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
-				"%s: compliance mode recovery timer deleted",
-				__func__);
+				"%s: port check timer deleted", __func__);
 	}
 
 	/* step 5: remove core well power */
@@ -1150,9 +1147,9 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
 
 		if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
 				!(xhci_all_ports_seen_u0(xhci))) {
-			del_timer_sync(&xhci->comp_mode_recovery_timer);
+			del_timer_sync(&xhci->port_check_timer);
 			xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
-				"Compliance Mode Recovery Timer deleted!");
+				"Port Check Timer deleted!");
 		}
 
 		/* Let the USB core know _both_ roothubs lost power. */
@@ -1245,13 +1242,13 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
 		}
 	}
 	/*
-	 * If system is subject to the Quirk, Compliance Mode Timer needs to
+	 * If system is subject to the Quirk, Port Check Timer needs to
 	 * be re-initialized Always after a system resume. Ports are subject
 	 * to suffer the Compliance Mode issue again. It doesn't matter if
 	 * ports have entered previously to U0 before system's suspension.
 	 */
 	if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) && !comp_timer_running)
-		compliance_mode_recovery_timer_init(xhci);
+		port_check_timer_init(xhci);
 
 	if (xhci->quirks & XHCI_ASMEDIA_MODIFY_FLOWCONTROL)
 		usb_asmedia_modifyflowcontrol(to_pci_dev(hcd->self.controller));
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 27d2c1176dd1..b52b7dcb5bb9 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1862,12 +1862,12 @@ struct xhci_hcd {
 	/* cached extended protocol port capabilities */
 	struct xhci_port_cap	*port_caps;
 	unsigned int		num_port_caps;
-	/* Compliance Mode Recovery Data */
-	struct timer_list	comp_mode_recovery_timer;
+	/* For quirks that require to poll for port status */
+	struct timer_list	port_check_timer;
 	u32			port_status_u0;
 	u16			test_mode;
-/* Compliance Mode Timer Triggered every 2 seconds */
-#define COMP_MODE_RCVRY_MSECS 2000
+/* Port polling frequency */
+#define PORT_CHECK_MSECS 2000
 
 	/* Track max eSS interval for XHCI_ISOC_BLOCKED_DISCONNECT */
 	unsigned int		max_ess_interval;
-- 
2.28.0

[PATCH 5/6] usb: xhci: Workaround lost disconnect port status

[Thinh Nguyen]

If an eSS device with active periodic transfers is disconnected from the
DWC_usb31 (v1.90a and prior) host controller at root port, the host
controller may not detect a disconnection. By active transfers, it
means that the endpoint is not in flow control, and there are active
Transfer Descriptors available for the host to initiate transfers to the
endpoint. This issue can occur if the endpoint periodic interval is in
2ms, 4ms, or 8ms.

In addition, the host controller will not be able to detect a new device
connection while the disconnection is not processed. The controller will
set the link state of the affected port to eSS_INACTIVE.

To workaround this, have the xHCI driver polls for the eSS root port
status every 2 seconds. If eSS_INACTIVE state is detected, initiate a
fake connection change to stop all the active endpoints and start
polling for new connection change.

Since XHCI_COMP_MODE_QUIRK is basically doing the same thing except for
polling for a different link state, we will use the same timer and
polling rate for this new workaround. Introduce a new quirk
XHCI_LOST_DISCONNECT_QUIRK to poll for eSS_INACTIVE port link state and
fake a connection change.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/host/xhci-hub.c     | 10 +++++++-
 drivers/usb/host/xhci.c         | 44 ++++++++++++++++++++++++---------
 include/linux/usb/xhci-quirks.h |  1 +
 3 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 8bfafbd680ab..32fbf95f021b 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -868,6 +868,11 @@ static void xhci_hub_report_usb3_link_state(struct xhci_hcd *xhci,
 		if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
 				(pls == USB_SS_PORT_LS_COMP_MOD))
 			pls |= USB_PORT_STAT_CONNECTION;
+
+		/* Fake a connection change */
+		if ((xhci->quirks & XHCI_LOST_DISCONNECT_QUIRK) &&
+				pls == XDEV_INACTIVE)
+			pls |= USB_PORT_STAT_CONNECTION;
 	}
 
 	/* update status field */
@@ -887,7 +892,8 @@ static void xhci_del_comp_mod_timer(struct xhci_hcd *xhci, u32 status,
 	u32 all_ports_seen_u0 = ((1 << xhci->usb3_rhub.num_ports) - 1);
 	bool port_in_u0 = ((status & PORT_PLS_MASK) == XDEV_U0);
 
-	if (!(xhci->quirks & XHCI_COMP_MODE_QUIRK))
+	if (!(xhci->quirks & XHCI_COMP_MODE_QUIRK) ||
+	    (xhci->quirks & XHCI_LOST_DISCONNECT_QUIRK))
 		return;
 
 	if ((xhci->port_status_u0 != all_ports_seen_u0) && port_in_u0) {
@@ -1654,6 +1660,8 @@ int xhci_hub_status_data(struct usb_hcd *hcd, char *buf)
 		trace_xhci_hub_status_data(i, temp);
 
 		if ((temp & mask) != 0 ||
+			((xhci->quirks & XHCI_LOST_DISCONNECT_QUIRK) &&
+				(temp & PORT_PLS_MASK) == XDEV_INACTIVE) ||
 			(bus_state->port_c_suspend & 1 << i) ||
 			(bus_state->resume_done[i] && time_after_eq(
 			    jiffies, bus_state->resume_done[i]))) {
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index e1b3d1063f6b..62275ee88849 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -485,10 +485,14 @@ static void port_check(struct timer_list *t)
 
 	xhci = from_timer(xhci, t, port_check_timer);
 	rhub = &xhci->usb3_rhub;
+	hcd = xhci->shared_hcd;
 
 	for (i = 0; i < rhub->num_ports; i++) {
+		bool poll_rhub = false;
+
 		temp = readl(rhub->ports[i]->addr);
-		if ((temp & PORT_PLS_MASK) == USB_SS_PORT_LS_COMP_MOD) {
+		if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
+		    ((temp & PORT_PLS_MASK) == USB_SS_PORT_LS_COMP_MOD)) {
 			/*
 			 * Compliance Mode Detected. Letting USB Core
 			 * handle the Warm Reset
@@ -498,8 +502,15 @@ static void port_check(struct timer_list *t)
 					i + 1);
 			xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
 					"Attempting compliance mode recovery");
-			hcd = xhci->shared_hcd;
 
+			poll_rhub = true;
+		}
+
+		if ((xhci->quirks & XHCI_LOST_DISCONNECT_QUIRK) &&
+		    ((temp & PORT_PLS_MASK) == XDEV_INACTIVE))
+			poll_rhub = true;
+
+		if (poll_rhub) {
 			if (hcd->state == HC_STATE_SUSPENDED)
 				usb_hcd_resume_root_hub(hcd);
 
@@ -507,7 +518,9 @@ static void port_check(struct timer_list *t)
 		}
 	}
 
-	if (xhci->port_status_u0 != ((1 << rhub->num_ports) - 1))
+	if ((xhci->quirks & XHCI_LOST_DISCONNECT_QUIRK) ||
+	    ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
+	     xhci->port_status_u0 != ((1 << rhub->num_ports) - 1)))
 		mod_timer(&xhci->port_check_timer,
 			jiffies + msecs_to_jiffies(PORT_CHECK_MSECS));
 }
@@ -593,10 +606,12 @@ static int xhci_init(struct usb_hcd *hcd)
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Finished xhci_init");
 
 	/* Initializing Compliance Mode Recovery Data If Needed */
-	if (xhci_compliance_mode_recovery_timer_quirk_check()) {
+	if (xhci_compliance_mode_recovery_timer_quirk_check())
 		xhci->quirks |= XHCI_COMP_MODE_QUIRK;
+
+	if (xhci->quirks & XHCI_LOST_DISCONNECT_QUIRK ||
+	    xhci->quirks & XHCI_COMP_MODE_QUIRK)
 		port_check_timer_init(xhci);
-	}
 
 	return retval;
 }
@@ -736,8 +751,9 @@ static void xhci_stop(struct usb_hcd *hcd)
 	xhci_cleanup_msix(xhci);
 
 	/* Deleting Compliance Mode Recovery Timer */
-	if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
-			(!(xhci_all_ports_seen_u0(xhci)))) {
+	if ((xhci->quirks & XHCI_LOST_DISCONNECT_QUIRK) ||
+	    ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
+	     !(xhci_all_ports_seen_u0(xhci)))) {
 		del_timer_sync(&xhci->port_check_timer);
 		xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
 				"%s: port check timer deleted", __func__);
@@ -1058,8 +1074,9 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
 	 * Deleting Port Check Timer because the xHCI Host
 	 * is about to be suspended.
 	 */
-	if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
-			(!(xhci_all_ports_seen_u0(xhci)))) {
+	if ((xhci->quirks & XHCI_LOST_DISCONNECT_QUIRK) ||
+	    ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
+	     !(xhci_all_ports_seen_u0(xhci)))) {
 		del_timer_sync(&xhci->port_check_timer);
 		xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
 				"%s: port check timer deleted", __func__);
@@ -1145,8 +1162,9 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
 	/* If restore operation fails, re-initialize the HC during resume */
 	if ((temp & STS_SRE) || hibernated) {
 
-		if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
-				!(xhci_all_ports_seen_u0(xhci))) {
+		if ((xhci->quirks & XHCI_LOST_DISCONNECT_QUIRK) ||
+		    ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
+		     !(xhci_all_ports_seen_u0(xhci)))) {
 			del_timer_sync(&xhci->port_check_timer);
 			xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
 				"Port Check Timer deleted!");
@@ -1247,7 +1265,9 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
 	 * to suffer the Compliance Mode issue again. It doesn't matter if
 	 * ports have entered previously to U0 before system's suspension.
 	 */
-	if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) && !comp_timer_running)
+	if (!comp_timer_running &&
+	    ((xhci->quirks & XHCI_LOST_DISCONNECT_QUIRK) ||
+	     (xhci->quirks & XHCI_COMP_MODE_QUIRK)))
 		port_check_timer_init(xhci);
 
 	if (xhci->quirks & XHCI_ASMEDIA_MODIFY_FLOWCONTROL)
diff --git a/include/linux/usb/xhci-quirks.h b/include/linux/usb/xhci-quirks.h
index 3a8566c902be..ce428a7e9de8 100644
--- a/include/linux/usb/xhci-quirks.h
+++ b/include/linux/usb/xhci-quirks.h
@@ -60,6 +60,7 @@
 #define XHCI_NO_SOFT_RETRY		BIT_ULL(40)
 #define XHCI_ISOC_BLOCKED_DISCONNECT	BIT_ULL(41)
 #define XHCI_LIMIT_FS_BI_INTR_EP	BIT_ULL(42)
+#define XHCI_LOST_DISCONNECT_QUIRK	BIT_ULL(43)
 
 struct usb_hcd;
 
-- 
2.28.0

[PATCH 6/6] usb: dwc3: host: Set quirks base on version

[Thinh Nguyen]

We can check for host quirks at runtime base on the controller IP and
version check. Set the following quirks for the DWC_usb31 IP host mode
before creating a platform device for the xHCI driver:

 * XHCI_ISOC_BLOCKED_DISCONNECT
 * XHCI_LIMIT_FS_BI_INTR_EP
 * XHCI_LOST_DISCONNECT_QUIRK

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/dwc3/host.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index f29a264635aa..a486d7fbb163 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -9,6 +9,7 @@
 
 #include <linux/acpi.h>
 #include <linux/platform_device.h>
+#include <linux/usb/xhci-quirks.h>
 
 #include "core.h"
 
@@ -42,6 +43,17 @@ static int dwc3_host_get_irq(struct dwc3 *dwc)
 	return irq;
 }
 
+static void dwc3_host_init_quirks(struct dwc3 *dwc, struct xhci_plat_priv *priv)
+{
+	memset(priv, 0, sizeof(*priv));
+
+	if (DWC3_VER_IS_WITHIN(DWC31, ANY, 190A)) {
+		priv->quirks |= XHCI_ISOC_BLOCKED_DISCONNECT;
+		priv->quirks |= XHCI_LIMIT_FS_BI_INTR_EP;
+		priv->quirks |= XHCI_LOST_DISCONNECT_QUIRK;
+	}
+}
+
 int dwc3_host_init(struct dwc3 *dwc)
 {
 	struct property_entry	props[4];
@@ -49,6 +61,7 @@ int dwc3_host_init(struct dwc3 *dwc)
 	int			ret, irq;
 	struct resource		*res;
 	struct platform_device	*dwc3_pdev = to_platform_device(dwc->dev);
+	struct xhci_plat_priv	dwc3_priv;
 	int			prop_idx = 0;
 
 	irq = dwc3_host_get_irq(dwc);
@@ -87,6 +100,14 @@ int dwc3_host_init(struct dwc3 *dwc)
 		goto err;
 	}
 
+	dwc3_host_init_quirks(dwc, &dwc3_priv);
+
+	ret = platform_device_add_data(xhci, &dwc3_priv, sizeof(dwc3_priv));
+	if (ret) {
+		dev_err(dwc->dev, "couldn't add platform data to xHCI device\n");
+		goto err;
+	}
+
 	memset(props, 0, sizeof(struct property_entry) * ARRAY_SIZE(props));
 
 	if (dwc->usb3_lpm_capable)
-- 
2.28.0

Re: [PATCH 6/6] usb: dwc3: host: Set quirks base on version

[Greg Kroah-Hartman]

On Thu, Apr 08, 2021 at 06:42:32PM -0700, Thinh Nguyen wrote:
> We can check for host quirks at runtime base on the controller IP and
> version check. Set the following quirks for the DWC_usb31 IP host mode
> before creating a platform device for the xHCI driver:
> 
>  * XHCI_ISOC_BLOCKED_DISCONNECT
>  * XHCI_LIMIT_FS_BI_INTR_EP
>  * XHCI_LOST_DISCONNECT_QUIRK
> 
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> ---
>  drivers/usb/dwc3/host.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> index f29a264635aa..a486d7fbb163 100644
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -9,6 +9,7 @@
>  
>  #include <linux/acpi.h>
>  #include <linux/platform_device.h>
> +#include <linux/usb/xhci-quirks.h>
>  
>  #include "core.h"
>  
> @@ -42,6 +43,17 @@ static int dwc3_host_get_irq(struct dwc3 *dwc)
>  	return irq;
>  }
>  
> +static void dwc3_host_init_quirks(struct dwc3 *dwc, struct xhci_plat_priv *priv)
> +{
> +	memset(priv, 0, sizeof(*priv));
> +
> +	if (DWC3_VER_IS_WITHIN(DWC31, ANY, 190A)) {
> +		priv->quirks |= XHCI_ISOC_BLOCKED_DISCONNECT;
> +		priv->quirks |= XHCI_LIMIT_FS_BI_INTR_EP;
> +		priv->quirks |= XHCI_LOST_DISCONNECT_QUIRK;
> +	}
> +}
> +
>  int dwc3_host_init(struct dwc3 *dwc)
>  {
>  	struct property_entry	props[4];
> @@ -49,6 +61,7 @@ int dwc3_host_init(struct dwc3 *dwc)
>  	int			ret, irq;
>  	struct resource		*res;
>  	struct platform_device	*dwc3_pdev = to_platform_device(dwc->dev);
> +	struct xhci_plat_priv	dwc3_priv;

Tying the dwc3 code to the xhci code like this feels really wrong to me,
are you sure this is the correct resolution?

greg k-h

Re: [PATCH 6/6] usb: dwc3: host: Set quirks base on version

[Thinh Nguyen]

Greg Kroah-Hartman wrote:
> On Thu, Apr 08, 2021 at 06:42:32PM -0700, Thinh Nguyen wrote:
>> We can check for host quirks at runtime base on the controller IP and
>> version check. Set the following quirks for the DWC_usb31 IP host mode
>> before creating a platform device for the xHCI driver:
>>
>>  * XHCI_ISOC_BLOCKED_DISCONNECT
>>  * XHCI_LIMIT_FS_BI_INTR_EP
>>  * XHCI_LOST_DISCONNECT_QUIRK
>>
>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>> ---
>>  drivers/usb/dwc3/host.c | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
>> index f29a264635aa..a486d7fbb163 100644
>> --- a/drivers/usb/dwc3/host.c
>> +++ b/drivers/usb/dwc3/host.c
>> @@ -9,6 +9,7 @@
>>  
>>  #include <linux/acpi.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/usb/xhci-quirks.h>
>>  
>>  #include "core.h"
>>  
>> @@ -42,6 +43,17 @@ static int dwc3_host_get_irq(struct dwc3 *dwc)
>>  	return irq;
>>  }
>>  
>> +static void dwc3_host_init_quirks(struct dwc3 *dwc, struct xhci_plat_priv *priv)
>> +{
>> +	memset(priv, 0, sizeof(*priv));
>> +
>> +	if (DWC3_VER_IS_WITHIN(DWC31, ANY, 190A)) {
>> +		priv->quirks |= XHCI_ISOC_BLOCKED_DISCONNECT;
>> +		priv->quirks |= XHCI_LIMIT_FS_BI_INTR_EP;
>> +		priv->quirks |= XHCI_LOST_DISCONNECT_QUIRK;
>> +	}
>> +}
>> +
>>  int dwc3_host_init(struct dwc3 *dwc)
>>  {
>>  	struct property_entry	props[4];
>> @@ -49,6 +61,7 @@ int dwc3_host_init(struct dwc3 *dwc)
>>  	int			ret, irq;
>>  	struct resource		*res;
>>  	struct platform_device	*dwc3_pdev = to_platform_device(dwc->dev);
>> +	struct xhci_plat_priv	dwc3_priv;
> 
> Tying the dwc3 code to the xhci code like this feels really wrong to me,
> are you sure this is the correct resolution?
> 
> greg k-h
> 

Can you clarify what feels wrong? The way it's originally implemented
already tied them in that way. What we're doing here simply takes
advantage of what xhci-plat glue layer can use to set the xhci quirks.
With this, we don't have to create new and duplicate DT properties for
dwc3 and xhci to set some quirks. With the expanding list of dwc3 DT, I
see this as a plus.

Thanks,
Thinh

Re: [PATCH 6/6] usb: dwc3: host: Set quirks base on version

[Greg Kroah-Hartman]

On Fri, Apr 09, 2021 at 08:01:15AM +0000, Thinh Nguyen wrote:
> Greg Kroah-Hartman wrote:
> > On Thu, Apr 08, 2021 at 06:42:32PM -0700, Thinh Nguyen wrote:
> >> We can check for host quirks at runtime base on the controller IP and
> >> version check. Set the following quirks for the DWC_usb31 IP host mode
> >> before creating a platform device for the xHCI driver:
> >>
> >>  * XHCI_ISOC_BLOCKED_DISCONNECT
> >>  * XHCI_LIMIT_FS_BI_INTR_EP
> >>  * XHCI_LOST_DISCONNECT_QUIRK
> >>
> >> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> >> ---
> >>  drivers/usb/dwc3/host.c | 21 +++++++++++++++++++++
> >>  1 file changed, 21 insertions(+)
> >>
> >> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> >> index f29a264635aa..a486d7fbb163 100644
> >> --- a/drivers/usb/dwc3/host.c
> >> +++ b/drivers/usb/dwc3/host.c
> >> @@ -9,6 +9,7 @@
> >>  
> >>  #include <linux/acpi.h>
> >>  #include <linux/platform_device.h>
> >> +#include <linux/usb/xhci-quirks.h>
> >>  
> >>  #include "core.h"
> >>  
> >> @@ -42,6 +43,17 @@ static int dwc3_host_get_irq(struct dwc3 *dwc)
> >>  	return irq;
> >>  }
> >>  
> >> +static void dwc3_host_init_quirks(struct dwc3 *dwc, struct xhci_plat_priv *priv)
> >> +{
> >> +	memset(priv, 0, sizeof(*priv));
> >> +
> >> +	if (DWC3_VER_IS_WITHIN(DWC31, ANY, 190A)) {
> >> +		priv->quirks |= XHCI_ISOC_BLOCKED_DISCONNECT;
> >> +		priv->quirks |= XHCI_LIMIT_FS_BI_INTR_EP;
> >> +		priv->quirks |= XHCI_LOST_DISCONNECT_QUIRK;
> >> +	}
> >> +}
> >> +
> >>  int dwc3_host_init(struct dwc3 *dwc)
> >>  {
> >>  	struct property_entry	props[4];
> >> @@ -49,6 +61,7 @@ int dwc3_host_init(struct dwc3 *dwc)
> >>  	int			ret, irq;
> >>  	struct resource		*res;
> >>  	struct platform_device	*dwc3_pdev = to_platform_device(dwc->dev);
> >> +	struct xhci_plat_priv	dwc3_priv;
> > 
> > Tying the dwc3 code to the xhci code like this feels really wrong to me,
> > are you sure this is the correct resolution?
> > 
> > greg k-h
> > 
> 
> Can you clarify what feels wrong? The way it's originally implemented
> already tied them in that way. What we're doing here simply takes
> advantage of what xhci-plat glue layer can use to set the xhci quirks.
> With this, we don't have to create new and duplicate DT properties for
> dwc3 and xhci to set some quirks. With the expanding list of dwc3 DT, I
> see this as a plus.

Ok, still feels odd, but I'll let the xhci maintainer make the decision
on it as it's their code to maintain, not mine :)

thanks,

greg k-h

Re: [PATCH 6/6] usb: dwc3: host: Set quirks base on version

[Thinh Nguyen]

Greg Kroah-Hartman wrote:
> On Fri, Apr 09, 2021 at 08:01:15AM +0000, Thinh Nguyen wrote:
>> Greg Kroah-Hartman wrote:
>>> On Thu, Apr 08, 2021 at 06:42:32PM -0700, Thinh Nguyen wrote:
>>>> We can check for host quirks at runtime base on the controller IP and
>>>> version check. Set the following quirks for the DWC_usb31 IP host mode
>>>> before creating a platform device for the xHCI driver:
>>>>
>>>>  * XHCI_ISOC_BLOCKED_DISCONNECT
>>>>  * XHCI_LIMIT_FS_BI_INTR_EP
>>>>  * XHCI_LOST_DISCONNECT_QUIRK
>>>>
>>>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>>> ---
>>>>  drivers/usb/dwc3/host.c | 21 +++++++++++++++++++++
>>>>  1 file changed, 21 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
>>>> index f29a264635aa..a486d7fbb163 100644
>>>> --- a/drivers/usb/dwc3/host.c
>>>> +++ b/drivers/usb/dwc3/host.c
>>>> @@ -9,6 +9,7 @@
>>>>  
>>>>  #include <linux/acpi.h>
>>>>  #include <linux/platform_device.h>
>>>> +#include <linux/usb/xhci-quirks.h>
>>>>  
>>>>  #include "core.h"
>>>>  
>>>> @@ -42,6 +43,17 @@ static int dwc3_host_get_irq(struct dwc3 *dwc)
>>>>  	return irq;
>>>>  }
>>>>  
>>>> +static void dwc3_host_init_quirks(struct dwc3 *dwc, struct xhci_plat_priv *priv)
>>>> +{
>>>> +	memset(priv, 0, sizeof(*priv));
>>>> +
>>>> +	if (DWC3_VER_IS_WITHIN(DWC31, ANY, 190A)) {
>>>> +		priv->quirks |= XHCI_ISOC_BLOCKED_DISCONNECT;
>>>> +		priv->quirks |= XHCI_LIMIT_FS_BI_INTR_EP;
>>>> +		priv->quirks |= XHCI_LOST_DISCONNECT_QUIRK;
>>>> +	}
>>>> +}
>>>> +
>>>>  int dwc3_host_init(struct dwc3 *dwc)
>>>>  {
>>>>  	struct property_entry	props[4];
>>>> @@ -49,6 +61,7 @@ int dwc3_host_init(struct dwc3 *dwc)
>>>>  	int			ret, irq;
>>>>  	struct resource		*res;
>>>>  	struct platform_device	*dwc3_pdev = to_platform_device(dwc->dev);
>>>> +	struct xhci_plat_priv	dwc3_priv;
>>>
>>> Tying the dwc3 code to the xhci code like this feels really wrong to me,
>>> are you sure this is the correct resolution?
>>>
>>> greg k-h
>>>
>>
>> Can you clarify what feels wrong? The way it's originally implemented
>> already tied them in that way. What we're doing here simply takes
>> advantage of what xhci-plat glue layer can use to set the xhci quirks.
>> With this, we don't have to create new and duplicate DT properties for
>> dwc3 and xhci to set some quirks. With the expanding list of dwc3 DT, I
>> see this as a plus.
> 
> Ok, still feels odd, but I'll let the xhci maintainer make the decision
> on it as it's their code to maintain, not mine :)
> 

Sure. Let me know if you have suggestions to make this better, and I'll
try to make the necessary changes.

Thanks,
Thinh

[PATCH v2 0/7] usb: Set quirks for xhci/dwc3 host mode

[Thinh Nguyen]

This series add 3 new quirks for DWC_usb31 host mode:
 * XHCI_ISOC_BLOCKED_DISCONNECT
 * XHCI_LIMIT_FS_BI_INTR_EP
 * XHCI_LOST_DISCONNECT_QUIRK

Different versions of DWC_usb3x controllers have different quirks. Typically we
set them based on PCI device VID:PID or DT compatible strings. However, we know
that a particular IP version(s) may share a common quirk across different
platform. We can enable these quirks based on the IP type and version number.
This simplifies the designer work and consolidate the logic check. To do this,
we will need to expose the xHCI quirks to the common header along with the
private platform structure.


Changes in v2:
- Instead of combining xhci-plat private structure in xhci-squirks.h, keep it
  as a separate header file


Thinh Nguyen (7):
  usb: xhci: Move quirks definitions to common usb header
  usb: xhci: Move xhci-plat header to common usb header
  usb: xhci: Check for blocked disconnection
  usb: xhci: Workaround undercalculated BW for fullspeed BI
  usb: xhci: Rename Compliance mode timer quirk
  usb: xhci: Workaround lost disconnect port status
  usb: dwc3: host: Set quirks base on version

 drivers/usb/cdns3/host.c                      |   2 +-
 drivers/usb/dwc3/host.c                       |  22 +++
 drivers/usb/host/xhci-hub.c                   |  12 +-
 drivers/usb/host/xhci-mem.c                   |  26 ++++
 drivers/usb/host/xhci-plat.c                  |   2 +-
 drivers/usb/host/xhci-rcar.c                  |   2 +-
 drivers/usb/host/xhci-ring.c                  |  76 ++++++++++
 drivers/usb/host/xhci.c                       | 134 +++++++++++++-----
 drivers/usb/host/xhci.h                       |  71 ++--------
 .../host => include/linux/usb}/xhci-plat.h    |  18 +--
 include/linux/usb/xhci-quirks.h               |  65 +++++++++
 11 files changed, 326 insertions(+), 104 deletions(-)
 rename {drivers/usb/host => include/linux/usb}/xhci-plat.h (54%)
 create mode 100644 include/linux/usb/xhci-quirks.h


base-commit: 496960274153bdeb9d1f904ff1ea875cef8232c1
-- 
2.28.0

[PATCH v2 1/7] usb: xhci: Move quirks definitions to common usb header

[Thinh Nguyen]

DWC3 (and possibly others such as CDNS3) will need to access these xHCI
quirks' definitions to initialize their hosts. Currently, to set these
quirks, we'd need to create new DT properties matching the quirks. This
may not be necessary as the driver can check the controller IP and
version at runtime to determine which quirks are needed. Let's move
these quirks' definitions to a common header under include/linux/usb so
DWC3 can properly access them.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
Changes in v2:
- Only keep the xHCI quirks' definitions and remove xhci-plat priv structure

 drivers/usb/host/xhci.h         | 53 +---------------------------
 include/linux/usb/xhci-quirks.h | 62 +++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 52 deletions(-)
 create mode 100644 include/linux/usb/xhci-quirks.h

diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 2595a8f057c4..9a4e2808668b 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -17,6 +17,7 @@
 #include <linux/kernel.h>
 #include <linux/usb/hcd.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/usb/xhci-quirks.h>
 
 /* Code sharing between pci-quirks and xhci hcd */
 #include	"xhci-ext-caps.h"
@@ -1840,58 +1841,6 @@ struct xhci_hcd {
 #define XHCI_STATE_HALTED	(1 << 1)
 #define XHCI_STATE_REMOVING	(1 << 2)
 	unsigned long long	quirks;
-#define	XHCI_LINK_TRB_QUIRK	BIT_ULL(0)
-#define XHCI_RESET_EP_QUIRK	BIT_ULL(1)
-#define XHCI_NEC_HOST		BIT_ULL(2)
-#define XHCI_AMD_PLL_FIX	BIT_ULL(3)
-#define XHCI_SPURIOUS_SUCCESS	BIT_ULL(4)
-/*
- * Certain Intel host controllers have a limit to the number of endpoint
- * contexts they can handle.  Ideally, they would signal that they can't handle
- * anymore endpoint contexts by returning a Resource Error for the Configure
- * Endpoint command, but they don't.  Instead they expect software to keep track
- * of the number of active endpoints for them, across configure endpoint
- * commands, reset device commands, disable slot commands, and address device
- * commands.
- */
-#define XHCI_EP_LIMIT_QUIRK	BIT_ULL(5)
-#define XHCI_BROKEN_MSI		BIT_ULL(6)
-#define XHCI_RESET_ON_RESUME	BIT_ULL(7)
-#define	XHCI_SW_BW_CHECKING	BIT_ULL(8)
-#define XHCI_AMD_0x96_HOST	BIT_ULL(9)
-#define XHCI_TRUST_TX_LENGTH	BIT_ULL(10)
-#define XHCI_LPM_SUPPORT	BIT_ULL(11)
-#define XHCI_INTEL_HOST		BIT_ULL(12)
-#define XHCI_SPURIOUS_REBOOT	BIT_ULL(13)
-#define XHCI_COMP_MODE_QUIRK	BIT_ULL(14)
-#define XHCI_AVOID_BEI		BIT_ULL(15)
-#define XHCI_PLAT		BIT_ULL(16)
-#define XHCI_SLOW_SUSPEND	BIT_ULL(17)
-#define XHCI_SPURIOUS_WAKEUP	BIT_ULL(18)
-/* For controllers with a broken beyond repair streams implementation */
-#define XHCI_BROKEN_STREAMS	BIT_ULL(19)
-#define XHCI_PME_STUCK_QUIRK	BIT_ULL(20)
-#define XHCI_MTK_HOST		BIT_ULL(21)
-#define XHCI_SSIC_PORT_UNUSED	BIT_ULL(22)
-#define XHCI_NO_64BIT_SUPPORT	BIT_ULL(23)
-#define XHCI_MISSING_CAS	BIT_ULL(24)
-/* For controller with a broken Port Disable implementation */
-#define XHCI_BROKEN_PORT_PED	BIT_ULL(25)
-#define XHCI_LIMIT_ENDPOINT_INTERVAL_7	BIT_ULL(26)
-#define XHCI_U2_DISABLE_WAKE	BIT_ULL(27)
-#define XHCI_ASMEDIA_MODIFY_FLOWCONTROL	BIT_ULL(28)
-#define XHCI_HW_LPM_DISABLE	BIT_ULL(29)
-#define XHCI_SUSPEND_DELAY	BIT_ULL(30)
-#define XHCI_INTEL_USB_ROLE_SW	BIT_ULL(31)
-#define XHCI_ZERO_64B_REGS	BIT_ULL(32)
-#define XHCI_DEFAULT_PM_RUNTIME_ALLOW	BIT_ULL(33)
-#define XHCI_RESET_PLL_ON_DISCONNECT	BIT_ULL(34)
-#define XHCI_SNPS_BROKEN_SUSPEND    BIT_ULL(35)
-#define XHCI_RENESAS_FW_QUIRK	BIT_ULL(36)
-#define XHCI_SKIP_PHY_INIT	BIT_ULL(37)
-#define XHCI_DISABLE_SPARSE	BIT_ULL(38)
-#define XHCI_SG_TRB_CACHE_SIZE_QUIRK	BIT_ULL(39)
-#define XHCI_NO_SOFT_RETRY	BIT_ULL(40)
 
 	unsigned int		num_active_eps;
 	unsigned int		limit_active_eps;
diff --git a/include/linux/usb/xhci-quirks.h b/include/linux/usb/xhci-quirks.h
new file mode 100644
index 000000000000..4001b10b418a
--- /dev/null
+++ b/include/linux/usb/xhci-quirks.h
@@ -0,0 +1,62 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * This file holds the definitions of quirks found in xHCI USB hosts.
+ */
+
+#ifndef __LINUX_USB_XHCI_QUIRKS_H
+#define __LINUX_USB_XHCI_QUIRKS_H
+
+#define	XHCI_LINK_TRB_QUIRK		BIT_ULL(0)
+#define XHCI_RESET_EP_QUIRK		BIT_ULL(1)
+#define XHCI_NEC_HOST			BIT_ULL(2)
+#define XHCI_AMD_PLL_FIX		BIT_ULL(3)
+#define XHCI_SPURIOUS_SUCCESS		BIT_ULL(4)
+/*
+ * Certain Intel host controllers have a limit to the number of endpoint
+ * contexts they can handle.  Ideally, they would signal that they can't handle
+ * anymore endpoint contexts by returning a Resource Error for the Configure
+ * Endpoint command, but they don't.  Instead they expect software to keep track
+ * of the number of active endpoints for them, across configure endpoint
+ * commands, reset device commands, disable slot commands, and address device
+ * commands.
+ */
+#define XHCI_EP_LIMIT_QUIRK		BIT_ULL(5)
+#define XHCI_BROKEN_MSI			BIT_ULL(6)
+#define XHCI_RESET_ON_RESUME		BIT_ULL(7)
+#define	XHCI_SW_BW_CHECKING		BIT_ULL(8)
+#define XHCI_AMD_0x96_HOST		BIT_ULL(9)
+#define XHCI_TRUST_TX_LENGTH		BIT_ULL(10)
+#define XHCI_LPM_SUPPORT		BIT_ULL(11)
+#define XHCI_INTEL_HOST			BIT_ULL(12)
+#define XHCI_SPURIOUS_REBOOT		BIT_ULL(13)
+#define XHCI_COMP_MODE_QUIRK		BIT_ULL(14)
+#define XHCI_AVOID_BEI			BIT_ULL(15)
+#define XHCI_PLAT			BIT_ULL(16)
+#define XHCI_SLOW_SUSPEND		BIT_ULL(17)
+#define XHCI_SPURIOUS_WAKEUP		BIT_ULL(18)
+/* For controllers with a broken beyond repair streams implementation */
+#define XHCI_BROKEN_STREAMS		BIT_ULL(19)
+#define XHCI_PME_STUCK_QUIRK		BIT_ULL(20)
+#define XHCI_MTK_HOST			BIT_ULL(21)
+#define XHCI_SSIC_PORT_UNUSED		BIT_ULL(22)
+#define XHCI_NO_64BIT_SUPPORT		BIT_ULL(23)
+#define XHCI_MISSING_CAS		BIT_ULL(24)
+/* For controller with a broken Port Disable implementation */
+#define XHCI_BROKEN_PORT_PED		BIT_ULL(25)
+#define XHCI_LIMIT_ENDPOINT_INTERVAL_7	BIT_ULL(26)
+#define XHCI_U2_DISABLE_WAKE		BIT_ULL(27)
+#define XHCI_ASMEDIA_MODIFY_FLOWCONTROL	BIT_ULL(28)
+#define XHCI_HW_LPM_DISABLE		BIT_ULL(29)
+#define XHCI_SUSPEND_DELAY		BIT_ULL(30)
+#define XHCI_INTEL_USB_ROLE_SW		BIT_ULL(31)
+#define XHCI_ZERO_64B_REGS		BIT_ULL(32)
+#define XHCI_DEFAULT_PM_RUNTIME_ALLOW	BIT_ULL(33)
+#define XHCI_RESET_PLL_ON_DISCONNECT	BIT_ULL(34)
+#define XHCI_SNPS_BROKEN_SUSPEND	BIT_ULL(35)
+#define XHCI_RENESAS_FW_QUIRK		BIT_ULL(36)
+#define XHCI_SKIP_PHY_INIT		BIT_ULL(37)
+#define XHCI_DISABLE_SPARSE		BIT_ULL(38)
+#define XHCI_SG_TRB_CACHE_SIZE_QUIRK	BIT_ULL(39)
+#define XHCI_NO_SOFT_RETRY		BIT_ULL(40)
+
+#endif /* __LINUX_USB_XHCI_QUIRKS_H */
-- 
2.28.0

[PATCH v2 2/7] usb: xhci: Move xhci-plat header to common usb header

[Thinh Nguyen]

DWC3 and CDNS3 need to access the private platform structure to pass
quirks to the xhci-plat glue layer. Move xhci-plat.h to a common header
location under include/linux/usb so DWC3 and CDNS3 can properly access
them.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
Changes in v2:
- New patch for this series

 drivers/usb/cdns3/host.c                       |  2 +-
 drivers/usb/host/xhci-plat.c                   |  2 +-
 drivers/usb/host/xhci-rcar.c                   |  2 +-
 .../usb/host => include/linux/usb}/xhci-plat.h | 18 +++++++++---------
 4 files changed, 12 insertions(+), 12 deletions(-)
 rename {drivers/usb/host => include/linux/usb}/xhci-plat.h (54%)

diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c
index 84dadfa726aa..02634d9762a4 100644
--- a/drivers/usb/cdns3/host.c
+++ b/drivers/usb/cdns3/host.c
@@ -14,8 +14,8 @@
 #include "drd.h"
 #include "host-export.h"
 #include <linux/usb/hcd.h>
+#include <linux/usb/xhci-plat.h>
 #include "../host/xhci.h"
-#include "../host/xhci-plat.h"
 
 #define XECP_PORT_CAP_REG	0x8000
 #define XECP_AUX_CTRL_REG1	0x8120
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index c1edcc9b13ce..3003fde3b430 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -19,9 +19,9 @@
 #include <linux/slab.h>
 #include <linux/acpi.h>
 #include <linux/usb/of.h>
+#include <linux/usb/xhci-plat.h>
 
 #include "xhci.h"
-#include "xhci-plat.h"
 #include "xhci-mvebu.h"
 #include "xhci-rcar.h"
 
diff --git a/drivers/usb/host/xhci-rcar.c b/drivers/usb/host/xhci-rcar.c
index 1bc4fe7b8c75..8d779df7b1e1 100644
--- a/drivers/usb/host/xhci-rcar.c
+++ b/drivers/usb/host/xhci-rcar.c
@@ -12,9 +12,9 @@
 #include <linux/of.h>
 #include <linux/usb/phy.h>
 #include <linux/sys_soc.h>
+#include <linux/usb/xhci-plat.h>
 
 #include "xhci.h"
-#include "xhci-plat.h"
 #include "xhci-rcar.h"
 
 /*
diff --git a/drivers/usb/host/xhci-plat.h b/include/linux/usb/xhci-plat.h
similarity index 54%
rename from drivers/usb/host/xhci-plat.h
rename to include/linux/usb/xhci-plat.h
index 561d0b7bce09..a58f1d44306d 100644
--- a/drivers/usb/host/xhci-plat.h
+++ b/include/linux/usb/xhci-plat.h
@@ -5,21 +5,21 @@
  * Copyright (C) 2015 Renesas Electronics Corporation
  */
 
-#ifndef _XHCI_PLAT_H
-#define _XHCI_PLAT_H
+#ifndef __LINUX_USB_XHCI_PLAT_H
+#define __LINUX_USB_XHCI_PLAT_H
 
-#include "xhci.h"	/* for hcd_to_xhci() */
+struct usb_hcd;
 
 struct xhci_plat_priv {
 	const char *firmware_name;
 	unsigned long long quirks;
-	int (*plat_setup)(struct usb_hcd *);
-	void (*plat_start)(struct usb_hcd *);
-	int (*init_quirk)(struct usb_hcd *);
-	int (*suspend_quirk)(struct usb_hcd *);
-	int (*resume_quirk)(struct usb_hcd *);
+	int (*plat_setup)(struct usb_hcd *hcd);
+	void (*plat_start)(struct usb_hcd *hcd);
+	int (*init_quirk)(struct usb_hcd *hcd);
+	int (*suspend_quirk)(struct usb_hcd *hcd);
+	int (*resume_quirk)(struct usb_hcd *hcd);
 };
 
 #define hcd_to_xhci_priv(h) ((struct xhci_plat_priv *)hcd_to_xhci(h)->priv)
 #define xhci_to_priv(x) ((struct xhci_plat_priv *)(x)->priv)
-#endif	/* _XHCI_PLAT_H */
+#endif	/* __LINUX_USB_XHCI_PLAT_H */
-- 
2.28.0

[PATCH v2 3/7] usb: xhci: Check for blocked disconnection

[Thinh Nguyen]

If there is a device with active enhanced super-speed (eSS) isoc IN
endpoint(s) behind one or more eSS hubs, DWC_usb31 (v1.90a and prior)
host controller will not detect the device disconnection until no more
isoc URB is submitted. If there's a device disconnection, internally
the wait for tHostTransactionTimeout (USB 3.2 spec 8.13) blocks the
other endpoints from being scheduled. So, it blocks the interrupt
endpoint of the eSS hub indicating the port change status.

This can be an issue for applications that continuously submitting isoc
URBs to the xHCI. To work around this, stop processing new URBs after 3
consecutive isoc transaction errors. If new isoc transfers are queued
after the device is disconnected, the host will respond with USB
transaction error. After 3 consecutive USB transaction errors, the
driver can wait a period of time (at least 2 * largest periodic interval
of the topology) without ringing isoc endpoint doorbell to detect the
port change status. If there is no disconnection detected, ring the
endpoint doorbell to resume isoc transfers.

This workaround tracks the max eSS periodic interval every time there's
an endpoint added or dropped, which happens when there's bandwidth
check. So, scan the topology and update the xhci->max_ess_interval
whenever there's a bandwidth check. Introduced a new flag
VDEV_DISCONN_CHECK_PENDING to prevent ringing the doorbell while waiting
for a disconnection status. After 2 * max_ess_interval time and no
disconnection detected, a delayed work will ring the doorbell to resume
the active isoc transfers.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - None

 drivers/usb/host/xhci-mem.c     |  3 ++
 drivers/usb/host/xhci-ring.c    | 76 +++++++++++++++++++++++++++++++++
 drivers/usb/host/xhci.c         | 49 +++++++++++++++++++++
 drivers/usb/host/xhci.h         | 10 +++++
 include/linux/usb/xhci-quirks.h |  1 +
 5 files changed, 139 insertions(+)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index f66815fe8482..1053b43008e4 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -995,6 +995,8 @@ int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
 	xhci_dbg(xhci, "Slot %d input ctx = 0x%llx (dma)\n", slot_id,
 			(unsigned long long)dev->in_ctx->dma);
 
+	INIT_DELAYED_WORK(&dev->resume_isoc, xhci_resume_isoc);
+
 	/* Initialize the cancellation list and watchdog timers for each ep */
 	for (i = 0; i < 31; i++) {
 		dev->eps[i].ep_index = i;
@@ -1010,6 +1012,7 @@ int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
 		goto fail;
 
 	dev->udev = udev;
+	dev->xhci = xhci;
 
 	/* Point to output device context in dcbaa. */
 	xhci->dcbaa->dev_context_ptrs[slot_id] = cpu_to_le64(dev->out_ctx->dma);
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 05c38dd3ee36..a434a4b3609f 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -419,6 +419,14 @@ void xhci_ring_ep_doorbell(struct xhci_hcd *xhci,
 	struct xhci_virt_ep *ep = &xhci->devs[slot_id]->eps[ep_index];
 	unsigned int ep_state = ep->ep_state;
 
+	/*
+	 * Don't ring the doorbell for isoc IN endpoint while checking for
+	 * device disconnection.
+	 */
+	if (ep->ring && ep->ring->type == TYPE_ISOC && !(ep_index % 2) &&
+	    (xhci->devs[slot_id]->flags & VDEV_DISCONN_CHECK_PENDING))
+		return;
+
 	/* Don't ring the doorbell for this endpoint if there are pending
 	 * cancellations because we don't want to interrupt processing.
 	 * We don't want to restart any stream rings if there's a set dequeue
@@ -2330,6 +2338,8 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 		struct xhci_ring *ep_ring, struct xhci_td *td,
 		union xhci_trb *ep_trb, struct xhci_transfer_event *event)
 {
+	struct usb_device *udev;
+	struct xhci_virt_device *vdev;
 	struct urb_priv *urb_priv;
 	int idx;
 	struct usb_iso_packet_descriptor *frame;
@@ -2347,10 +2357,13 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 	ep_trb_len = TRB_LEN(le32_to_cpu(ep_trb->generic.field[2]));
 	short_framestatus = td->urb->transfer_flags & URB_SHORT_NOT_OK ?
 		-EREMOTEIO : 0;
+	udev = td->urb->dev;
+	vdev = xhci->devs[udev->slot_id];
 
 	/* handle completion code */
 	switch (trb_comp_code) {
 	case COMP_SUCCESS:
+		ep->ring->err_count = 0;
 		if (remaining) {
 			frame->status = short_framestatus;
 			if (xhci->quirks & XHCI_TRUST_TX_LENGTH)
@@ -2375,6 +2388,23 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 		frame->status = -EPROTO;
 		break;
 	case COMP_USB_TRANSACTION_ERROR:
+		if ((xhci->quirks & XHCI_ISOC_BLOCKED_DISCONNECT) &&
+		    usb_urb_dir_in(td->urb) &&
+		    udev->parent && udev->parent->parent &&
+		    udev->speed >= USB_SPEED_SUPER) {
+			if (!(vdev->flags & VDEV_DISCONN_CHECK_PENDING) &&
+			    ep->ring->err_count++ >= 3) {
+				unsigned long timeout;
+
+				/* Wait for at least max interval x 2 x 125us */
+				timeout = (1 << xhci->max_ess_interval) * 250;
+				vdev->flags |= VDEV_DISCONN_CHECK_PENDING;
+				queue_delayed_work(system_wq,
+						   &vdev->resume_isoc,
+						   usecs_to_jiffies(timeout));
+			}
+		}
+
 		frame->status = -EPROTO;
 		if (ep_trb != td->last_trb)
 			return 0;
@@ -4171,6 +4201,9 @@ int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags,
 	for (i = 0; i < num_tds; i++)
 		num_trbs += count_isoc_trbs_needed(urb, i);
 
+	if ((xdev->flags & VDEV_DISCONN_CHECK_PENDING) && usb_urb_dir_in(urb))
+		return -EINVAL;
+
 	/* Check the ring to guarantee there is enough room for the whole urb.
 	 * Do not insert any td of the urb to the ring if the check failed.
 	 */
@@ -4359,3 +4392,46 @@ int xhci_queue_reset_ep(struct xhci_hcd *xhci, struct xhci_command *cmd,
 	return queue_command(xhci, cmd, 0, 0, 0,
 			trb_slot_id | trb_ep_index | type, false);
 }
+
+void xhci_resume_isoc(struct work_struct *work)
+{
+	struct xhci_hcd *xhci;
+	struct xhci_virt_device *vdev;
+	unsigned int slot_id;
+	unsigned long flags;
+
+	vdev = container_of(to_delayed_work(work),
+			    struct xhci_virt_device, resume_isoc);
+	xhci = vdev->xhci;
+
+	spin_lock_irqsave(&xhci->lock, flags);
+
+	/* Check if the device is dropped before this work takes place */
+	if (!vdev->udev)
+		goto out;
+
+	slot_id = vdev->udev->slot_id;
+
+	vdev->flags &= ~VDEV_DISCONN_CHECK_PENDING;
+
+	/* Resume isoc transfers if the device is still connected */
+	if (xhci->devs[slot_id]) {
+		int i;
+
+		/* Ring doorbell for IN isoc endpoints only */
+		for (i = 2; i < 31; i += 2) {
+			struct xhci_virt_ep *ep = &vdev->eps[i];
+
+			if (!ep)
+				break;
+
+			if (ep->ring && ep->ring->type == TYPE_ISOC) {
+				ep->ring->err_count = 0;
+				ring_doorbell_for_active_rings(xhci, slot_id, i);
+			}
+		}
+	}
+
+out:
+	spin_unlock_irqrestore(&xhci->lock, flags);
+}
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index ca9385d22f68..e1136a6b9372 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -2973,6 +2973,44 @@ static void xhci_check_bw_drop_ep_streams(struct xhci_hcd *xhci,
 	}
 }
 
+static void xhci_update_ess_max_interval(struct xhci_hcd *xhci)
+{
+	unsigned int max_ess_interval = 0;
+	int j;
+
+	for (j = 1; j < HCS_MAX_SLOTS(xhci->hcs_params1); j++) {
+		struct xhci_virt_device	*virt_dev;
+		int i;
+
+		virt_dev = xhci->devs[j];
+		if (!virt_dev)
+			continue;
+
+		/* Only update for eSS devices */
+		if (virt_dev->udev &&
+		    virt_dev->udev->speed < USB_SPEED_SUPER)
+			continue;
+
+		for (i = 0; i < 31; i++) {
+			struct xhci_ep_ctx *ep_ctx;
+			unsigned int ep_type;
+			unsigned int interval;
+
+			ep_ctx = xhci_get_ep_ctx(xhci, virt_dev->out_ctx, i);
+			ep_type = CTX_TO_EP_TYPE(le32_to_cpu(ep_ctx->ep_info2));
+
+			if (xhci_is_async_ep(ep_type))
+				continue;
+
+			interval = CTX_TO_EP_INTERVAL(le32_to_cpu(ep_ctx->ep_info));
+			if (interval > max_ess_interval)
+				max_ess_interval = interval;
+		}
+	}
+
+	xhci->max_ess_interval = max_ess_interval;
+}
+
 /* Called after one or more calls to xhci_add_endpoint() or
  * xhci_drop_endpoint().  If this call fails, the USB core is expected
  * to call xhci_reset_bandwidth().
@@ -3047,6 +3085,17 @@ int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
 		/* Callee should call reset_bandwidth() */
 		goto command_cleanup;
 
+	if (xhci->quirks & XHCI_ISOC_BLOCKED_DISCONNECT) {
+		xhci_update_ess_max_interval(xhci);
+
+		/* Cancel disconnection check on change of context */
+		if (delayed_work_pending(&virt_dev->resume_isoc) &&
+		    ctrl_ctx->drop_flags) {
+			cancel_delayed_work(&virt_dev->resume_isoc);
+			virt_dev->flags &= ~VDEV_DISCONN_CHECK_PENDING;
+		}
+	}
+
 	/* Free any rings that were dropped, but not changed. */
 	for (i = 1; i < 31; i++) {
 		if ((le32_to_cpu(ctrl_ctx->drop_flags) & (1 << (i + 1))) &&
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 9a4e2808668b..27d2c1176dd1 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1000,6 +1000,7 @@ struct xhci_interval_bw_table {
 
 struct xhci_virt_device {
 	int				slot_id;
+	struct xhci_hcd			*xhci;
 	struct usb_device		*udev;
 	/*
 	 * Commands to the hardware are passed an "input context" that
@@ -1025,11 +1026,15 @@ struct xhci_virt_device {
 	 */
 	unsigned long			flags;
 #define VDEV_PORT_ERROR			BIT(0) /* Port error, link inactive */
+#define VDEV_DISCONN_CHECK_PENDING	BIT(1) /* Disconnection check */
 
 	/* The current max exit latency for the enabled USB3 link states. */
 	u16				current_mel;
 	/* Used for the debugfs interfaces. */
 	void				*debugfs_private;
+
+	/* For undetected disconnection quirk */
+	struct delayed_work		resume_isoc;
 };
 
 /*
@@ -1864,6 +1869,9 @@ struct xhci_hcd {
 /* Compliance Mode Timer Triggered every 2 seconds */
 #define COMP_MODE_RCVRY_MSECS 2000
 
+	/* Track max eSS interval for XHCI_ISOC_BLOCKED_DISCONNECT */
+	unsigned int		max_ess_interval;
+
 	struct dentry		*debugfs_root;
 	struct dentry		*debugfs_slots;
 	struct list_head	regset_list;
@@ -1948,6 +1956,8 @@ char *xhci_get_slot_state(struct xhci_hcd *xhci,
 void xhci_dbg_trace(struct xhci_hcd *xhci, void (*trace)(struct va_format *),
 			const char *fmt, ...);
 
+void xhci_resume_isoc(struct work_struct *work);
+
 /* xHCI memory management */
 void xhci_mem_cleanup(struct xhci_hcd *xhci);
 int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags);
diff --git a/include/linux/usb/xhci-quirks.h b/include/linux/usb/xhci-quirks.h
index 4001b10b418a..65bb62d3d31d 100644
--- a/include/linux/usb/xhci-quirks.h
+++ b/include/linux/usb/xhci-quirks.h
@@ -58,5 +58,6 @@
 #define XHCI_DISABLE_SPARSE		BIT_ULL(38)
 #define XHCI_SG_TRB_CACHE_SIZE_QUIRK	BIT_ULL(39)
 #define XHCI_NO_SOFT_RETRY		BIT_ULL(40)
+#define XHCI_ISOC_BLOCKED_DISCONNECT	BIT_ULL(41)
 
 #endif /* __LINUX_USB_XHCI_QUIRKS_H */
-- 
2.28.0

Re: [PATCH v2 3/7] usb: xhci: Check for blocked disconnection

[Mathias Nyman]

Hi Thinh

Sorry about the delay. 

On 10.4.2021 3.47, Thinh Nguyen wrote:
> If there is a device with active enhanced super-speed (eSS) isoc IN
> endpoint(s) behind one or more eSS hubs, DWC_usb31 (v1.90a and prior)
> host controller will not detect the device disconnection until no more
> isoc URB is submitted. If there's a device disconnection, internally
> the wait for tHostTransactionTimeout (USB 3.2 spec 8.13) blocks the
> other endpoints from being scheduled. So, it blocks the interrupt
> endpoint of the eSS hub indicating the port change status.
> 
> This can be an issue for applications that continuously submitting isoc
> URBs to the xHCI. To work around this, stop processing new URBs after 3
> consecutive isoc transaction errors. If new isoc transfers are queued
> after the device is disconnected, the host will respond with USB
> transaction error. After 3 consecutive USB transaction errors, the
> driver can wait a period of time (at least 2 * largest periodic interval
> of the topology) without ringing isoc endpoint doorbell to detect the
> port change status. If there is no disconnection detected, ring the
> endpoint doorbell to resume isoc transfers.

Is that enough? many Isoc URBs queue 16 - 32 Isoc TRBs per URB.
And drivers like UVC queue several URBs in advance.

If I remember correctly then a transaction errors won't stop Isoch endpoints,
so waiting for 2 * Interval after 3 consecutive transaction errors might not
be enough.

How about stopping the endpoint after 3 consecutive transaction errors,
and restating it a bit later? 

> 
> This workaround tracks the max eSS periodic interval every time there's
> an endpoint added or dropped, which happens when there's bandwidth
> check. So, scan the topology and update the xhci->max_ess_interval
> whenever there's a bandwidth check. Introduced a new flag
> VDEV_DISCONN_CHECK_PENDING to prevent ringing the doorbell while waiting
> for a disconnection status. After 2 * max_ess_interval time and no
> disconnection detected, a delayed work will ring the doorbell to resume
> the active isoc transfers.

Sounds very elaborate for a vendor specific disconnect workaround.
Isn't there a simpler way?

Maybe stop all isoc in endpoints if one them has 3 consecutive transaction error,
wait for 2x hub interrupt interval time, and then restart the endpoints if there is
no disconnect?

There is bigger concern with this series, it scatters a lot of vendor specific code 
around the generic xhci driver. It's not very clear afterwards what code is part of the
workaround and what is generic code.

We just got a lot of the Mediatek code moved to xhci-mtk*, maybe its time to add xhci-snps.c
instead of using the generic platform driver with tons of workarounds and quirks.

Thanks
-Mathias    

Re: [PATCH v2 3/7] usb: xhci: Check for blocked disconnection

[Thinh Nguyen]

Mathias Nyman wrote:
> Hi Thinh
> 
> Sorry about the delay. 

Np :)

> 
> On 10.4.2021 3.47, Thinh Nguyen wrote:
>> If there is a device with active enhanced super-speed (eSS) isoc IN
>> endpoint(s) behind one or more eSS hubs, DWC_usb31 (v1.90a and prior)
>> host controller will not detect the device disconnection until no more
>> isoc URB is submitted. If there's a device disconnection, internally
>> the wait for tHostTransactionTimeout (USB 3.2 spec 8.13) blocks the
>> other endpoints from being scheduled. So, it blocks the interrupt
>> endpoint of the eSS hub indicating the port change status.
>>
>> This can be an issue for applications that continuously submitting isoc
>> URBs to the xHCI. To work around this, stop processing new URBs after 3
>> consecutive isoc transaction errors. If new isoc transfers are queued
>> after the device is disconnected, the host will respond with USB
>> transaction error. After 3 consecutive USB transaction errors, the
>> driver can wait a period of time (at least 2 * largest periodic interval
>> of the topology) without ringing isoc endpoint doorbell to detect the
>> port change status. If there is no disconnection detected, ring the
>> endpoint doorbell to resume isoc transfers.
> 
> Is that enough? many Isoc URBs queue 16 - 32 Isoc TRBs per URB.
> And drivers like UVC queue several URBs in advance.

That's fine as long as the driver stops ringing more doorbell for a
certain period of time creating a gap that's enough to get the
notification from the interrupt endpoint. We tested with 128 isoc URBs
and was able to detect a disconnect after this delay.

> 
> If I remember correctly then a transaction errors won't stop Isoch endpoints,
> so waiting for 2 * Interval after 3 consecutive transaction errors might not
> be enough.
> 
> How about stopping the endpoint after 3 consecutive transaction errors,
> and restating it a bit later?

There's no need to stop and restart the endpoint.

> 
>>
>> This workaround tracks the max eSS periodic interval every time there's
>> an endpoint added or dropped, which happens when there's bandwidth
>> check. So, scan the topology and update the xhci->max_ess_interval
>> whenever there's a bandwidth check. Introduced a new flag
>> VDEV_DISCONN_CHECK_PENDING to prevent ringing the doorbell while waiting
>> for a disconnection status. After 2 * max_ess_interval time and no
>> disconnection detected, a delayed work will ring the doorbell to resume
>> the active isoc transfers.
> 
> Sounds very elaborate for a vendor specific disconnect workaround.
> Isn't there a simpler way?
> 
> Maybe stop all isoc in endpoints if one them has 3 consecutive transaction error,
> wait for 2x hub interrupt interval time, and then restart the endpoints if there is
> no disconnect?

We can also do this (but without stop + restart the endpoints). It just
creates a slightly larger gap that may be more noticeable to the user if
there's no actual disconnection.

> 
> There is bigger concern with this series, it scatters a lot of vendor specific code 
> around the generic xhci driver. It's not very clear afterwards what code is part of the
> workaround and what is generic code.
> 
> We just got a lot of the Mediatek code moved to xhci-mtk*, maybe its time to add xhci-snps.c
> instead of using the generic platform driver with tons of workarounds and quirks.
> 

Thanks for the reviews. I need to look into how this can be done. May
need your suggestion as not every scenarios can be overridden
easily/cleanly.

What about the other quirks, do you have any comments?

Thanks,
Thinh

Re: [PATCH v2 3/7] usb: xhci: Check for blocked disconnection

[Mathias Nyman]

On 28.4.2021 1.30, Thinh Nguyen wrote:
>> On 10.4.2021 3.47, Thinh Nguyen wrote:
>>> If there is a device with active enhanced super-speed (eSS) isoc IN
>>> endpoint(s) behind one or more eSS hubs, DWC_usb31 (v1.90a and prior)
>>> host controller will not detect the device disconnection until no more
>>> isoc URB is submitted. If there's a device disconnection, internally
>>> the wait for tHostTransactionTimeout (USB 3.2 spec 8.13) blocks the
>>> other endpoints from being scheduled. So, it blocks the interrupt
>>> endpoint of the eSS hub indicating the port change status.
>>>
>>> This can be an issue for applications that continuously submitting isoc
>>> URBs to the xHCI. To work around this, stop processing new URBs after 3
>>> consecutive isoc transaction errors. If new isoc transfers are queued
>>> after the device is disconnected, the host will respond with USB
>>> transaction error. After 3 consecutive USB transaction errors, the
>>> driver can wait a period of time (at least 2 * largest periodic interval
>>> of the topology) without ringing isoc endpoint doorbell to detect the
>>> port change status. If there is no disconnection detected, ring the
>>> endpoint doorbell to resume isoc transfers.
>>
>> Is that enough? many Isoc URBs queue 16 - 32 Isoc TRBs per URB.
>> And drivers like UVC queue several URBs in advance.
> 
> That's fine as long as the driver stops ringing more doorbell for a
> certain period of time creating a gap that's enough to get the
> notification from the interrupt endpoint. We tested with 128 isoc URBs
> and was able to detect a disconnect after this delay.

Ok, if not ringing the endpoint is enough then that is better than
stopping the whole endpoint. 

>>> This workaround tracks the max eSS periodic interval every time there's
>>> an endpoint added or dropped, which happens when there's bandwidth
>>> check. So, scan the topology and update the xhci->max_ess_interval
>>> whenever there's a bandwidth check. Introduced a new flag
>>> VDEV_DISCONN_CHECK_PENDING to prevent ringing the doorbell while waiting
>>> for a disconnection status. After 2 * max_ess_interval time and no
>>> disconnection detected, a delayed work will ring the doorbell to resume
>>> the active isoc transfers.
>>
>> Sounds very elaborate for a vendor specific disconnect workaround.
>> Isn't there a simpler way?
>>
>> Maybe stop all isoc in endpoints if one them has 3 consecutive transaction error,
>> wait for 2x hub interrupt interval time, and then restart the endpoints if there is
>> no disconnect?
> 
> We can also do this (but without stop + restart the endpoints). It just
> creates a slightly larger gap that may be more noticeable to the user if
> there's no actual disconnection.

Ok, if blocking the doorbell is enough then it sounds better.

How about that max interval tracking, is it necessary?
It will block the doorbell from 250us up to several seconds.
Is there some reasonable single value that can be used instead?

>>
>> There is bigger concern with this series, it scatters a lot of vendor specific code 
>> around the generic xhci driver. It's not very clear afterwards what code is part of the
>> workaround and what is generic code.
>>
>> We just got a lot of the Mediatek code moved to xhci-mtk*, maybe its time to add xhci-snps.c
>> instead of using the generic platform driver with tons of workarounds and quirks.
>>
> 
> Thanks for the reviews. I need to look into how this can be done. May
> need your suggestion as not every scenarios can be overridden
> easily/cleanly.

true, no easy overrides for this transfer error / doorbell blocking workaround.
Needs a bit of work

-Mathias

Re: [PATCH v2 3/7] usb: xhci: Check for blocked disconnection

[Thinh Nguyen]

Mathias Nyman wrote:
> On 28.4.2021 1.30, Thinh Nguyen wrote:
>>> On 10.4.2021 3.47, Thinh Nguyen wrote:
>>>> If there is a device with active enhanced super-speed (eSS) isoc IN
>>>> endpoint(s) behind one or more eSS hubs, DWC_usb31 (v1.90a and prior)
>>>> host controller will not detect the device disconnection until no more
>>>> isoc URB is submitted. If there's a device disconnection, internally
>>>> the wait for tHostTransactionTimeout (USB 3.2 spec 8.13) blocks the
>>>> other endpoints from being scheduled. So, it blocks the interrupt
>>>> endpoint of the eSS hub indicating the port change status.
>>>>
>>>> This can be an issue for applications that continuously submitting isoc
>>>> URBs to the xHCI. To work around this, stop processing new URBs after 3
>>>> consecutive isoc transaction errors. If new isoc transfers are queued
>>>> after the device is disconnected, the host will respond with USB
>>>> transaction error. After 3 consecutive USB transaction errors, the
>>>> driver can wait a period of time (at least 2 * largest periodic interval
>>>> of the topology) without ringing isoc endpoint doorbell to detect the
>>>> port change status. If there is no disconnection detected, ring the
>>>> endpoint doorbell to resume isoc transfers.
>>>
>>> Is that enough? many Isoc URBs queue 16 - 32 Isoc TRBs per URB.
>>> And drivers like UVC queue several URBs in advance.
>>
>> That's fine as long as the driver stops ringing more doorbell for a
>> certain period of time creating a gap that's enough to get the
>> notification from the interrupt endpoint. We tested with 128 isoc URBs
>> and was able to detect a disconnect after this delay.
> 
> Ok, if not ringing the endpoint is enough then that is better than
> stopping the whole endpoint. 
> 
>>>> This workaround tracks the max eSS periodic interval every time there's
>>>> an endpoint added or dropped, which happens when there's bandwidth
>>>> check. So, scan the topology and update the xhci->max_ess_interval
>>>> whenever there's a bandwidth check. Introduced a new flag
>>>> VDEV_DISCONN_CHECK_PENDING to prevent ringing the doorbell while waiting
>>>> for a disconnection status. After 2 * max_ess_interval time and no
>>>> disconnection detected, a delayed work will ring the doorbell to resume
>>>> the active isoc transfers.
>>>
>>> Sounds very elaborate for a vendor specific disconnect workaround.
>>> Isn't there a simpler way?
>>>
>>> Maybe stop all isoc in endpoints if one them has 3 consecutive transaction error,
>>> wait for 2x hub interrupt interval time, and then restart the endpoints if there is
>>> no disconnect?
>>
>> We can also do this (but without stop + restart the endpoints). It just
>> creates a slightly larger gap that may be more noticeable to the user if
>> there's no actual disconnection.
> 
> Ok, if blocking the doorbell is enough then it sounds better.
> 
> How about that max interval tracking, is it necessary?
> It will block the doorbell from 250us up to several seconds.
> Is there some reasonable single value that can be used instead?

Yeah, I agree with you. I think we can find a number that satisfies with
most applications while keeping this change small and less intrusive.

> 
>>>
>>> There is bigger concern with this series, it scatters a lot of vendor specific code 
>>> around the generic xhci driver. It's not very clear afterwards what code is part of the
>>> workaround and what is generic code.
>>>
>>> We just got a lot of the Mediatek code moved to xhci-mtk*, maybe its time to add xhci-snps.c
>>> instead of using the generic platform driver with tons of workarounds and quirks.
>>>
>>
>> Thanks for the reviews. I need to look into how this can be done. May
>> need your suggestion as not every scenarios can be overridden
>> easily/cleanly.
> 
> true, no easy overrides for this transfer error / doorbell blocking workaround.
> Needs a bit of work
> 

Thanks,
Thinh

[PATCH v2 4/7] usb: xhci: Workaround undercalculated BW for fullspeed BI

[Thinh Nguyen]

DWC_usb31 host version 1.90a and prior undercalculates the bandwidth
available for interrupt endpoints. The controller will return bandwidth
error on config endpoint commands if there are already 6 or more
fullspeed interrupt endpoints with bInterval of 4 (or 4ms) associated
with a single fullspeed bus instance (BI).

To workaround this, configure and use the endpoint at a shorter
interrupt interval. Lower the ep_ctx interval from 5 to 4 (or 2ms)
for interrupt endpoints of the fullspeed BI. Note: we have not observed
functional impact to the fullspeed devices by lowering the interrupt
service interval (at least for a few devices that we tested).

To simplify the workaround, let's just check and apply the workaround if
the endpoint is a fullspeed interrupt endpoint with interval of 4ms and
if the top parent device is also operating in fullspeed (i.e. associated
with fullspeed BI).

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
Changes in v2:
- None

Note:
Checkpatch will give a warning below for getting top_dev:
	WARNING: suspect code indent for conditional statements

Since this logic is done everywhere else in the driver, I'm keeping it
consistent here.

 drivers/usb/host/xhci-mem.c     | 23 +++++++++++++++++++++++
 include/linux/usb/xhci-quirks.h |  1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 1053b43008e4..e01d0ddb012a 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1463,6 +1463,29 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,
 		}
 	}
 
+	/*
+	 * Check for undercalculated bandwidth quirk for interrupt endpoints
+	 * associated with fullspeed BI.
+	 */
+	if ((xhci->quirks & XHCI_LIMIT_FS_BI_INTR_EP) &&
+	    usb_endpoint_xfer_int(&ep->desc) &&
+	    udev->speed == USB_SPEED_FULL &&
+	    interval == 5) {
+		struct usb_device *top_dev;
+
+		for (top_dev = udev;
+		     top_dev->parent && top_dev->parent->parent;
+		     top_dev = top_dev->parent)
+			/* Found device below root hub */;
+
+		/*
+		 * If the top device is operating at fullspeed, then the
+		 * controller is using fullspeed BI for this device.
+		 */
+		if (top_dev->speed == USB_SPEED_FULL)
+			interval = 4;
+	}
+
 	mult = xhci_get_endpoint_mult(udev, ep);
 	max_packet = usb_endpoint_maxp(&ep->desc);
 	max_burst = xhci_get_endpoint_max_burst(udev, ep);
diff --git a/include/linux/usb/xhci-quirks.h b/include/linux/usb/xhci-quirks.h
index 65bb62d3d31d..5bedf5a25f7a 100644
--- a/include/linux/usb/xhci-quirks.h
+++ b/include/linux/usb/xhci-quirks.h
@@ -59,5 +59,6 @@
 #define XHCI_SG_TRB_CACHE_SIZE_QUIRK	BIT_ULL(39)
 #define XHCI_NO_SOFT_RETRY		BIT_ULL(40)
 #define XHCI_ISOC_BLOCKED_DISCONNECT	BIT_ULL(41)
+#define XHCI_LIMIT_FS_BI_INTR_EP	BIT_ULL(42)
 
 #endif /* __LINUX_USB_XHCI_QUIRKS_H */
-- 
2.28.0

Re: [PATCH v2 4/7] usb: xhci: Workaround undercalculated BW for fullspeed BI

[Mathias Nyman]

On 10.4.2021 3.47, Thinh Nguyen wrote:
> DWC_usb31 host version 1.90a and prior undercalculates the bandwidth
> available for interrupt endpoints. The controller will return bandwidth
> error on config endpoint commands if there are already 6 or more
> fullspeed interrupt endpoints with bInterval of 4 (or 4ms) associated
> with a single fullspeed bus instance (BI).
> 
> To workaround this, configure and use the endpoint at a shorter
> interrupt interval. Lower the ep_ctx interval from 5 to 4 (or 2ms)
> for interrupt endpoints of the fullspeed BI. Note: we have not observed
> functional impact to the fullspeed devices by lowering the interrupt
> service interval (at least for a few devices that we tested).
> 
> To simplify the workaround, let's just check and apply the workaround if
> the endpoint is a fullspeed interrupt endpoint with interval of 4ms and
> if the top parent device is also operating in fullspeed (i.e. associated
> with fullspeed BI).
> 
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> ---
> Changes in v2:
> - None
> 
> Note:
> Checkpatch will give a warning below for getting top_dev:
> 	WARNING: suspect code indent for conditional statements
> 
> Since this logic is done everywhere else in the driver, I'm keeping it
> consistent here.
> 
>  drivers/usb/host/xhci-mem.c     | 23 +++++++++++++++++++++++
>  include/linux/usb/xhci-quirks.h |  1 +
>  2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 1053b43008e4..e01d0ddb012a 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -1463,6 +1463,29 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,
>  		}
>  	}
>  
> +	/*
> +	 * Check for undercalculated bandwidth quirk for interrupt endpoints
> +	 * associated with fullspeed BI.
> +	 */
> +	if ((xhci->quirks & XHCI_LIMIT_FS_BI_INTR_EP) &&
> +	    usb_endpoint_xfer_int(&ep->desc) &&
> +	    udev->speed == USB_SPEED_FULL &&
> +	    interval == 5) {
> +		struct usb_device *top_dev;
> +
> +		for (top_dev = udev;
> +		     top_dev->parent && top_dev->parent->parent;
> +		     top_dev = top_dev->parent)
> +			/* Found device below root hub */;
> +
> +		/*
> +		 * If the top device is operating at fullspeed, then the
> +		 * controller is using fullspeed BI for this device.
> +		 */
> +		if (top_dev->speed == USB_SPEED_FULL)
> +			interval = 4;
> +	}
> +


If we write a new xhci-snps driver then adjusting the interval could be done in the
.add_endpoint override, after calling xhci_add_endpoint()

If not then something like this should work.
Maybe turn it into a separate function for readability.

xhci_tune_interval_quirk(xhci, udev, ep, &interval)

-Mathias

[PATCH v2 5/7] usb: xhci: Rename Compliance mode timer quirk

[Thinh Nguyen]

In preparation for a workaround that needs to poll for the port status,
rename the timer for XHCI_COMP_MODE_QUIRK to be more generic as it can
be used for the new workaround. No funtional change here.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - None

 drivers/usb/host/xhci-hub.c |  2 +-
 drivers/usb/host/xhci.c     | 41 +++++++++++++++++--------------------
 drivers/usb/host/xhci.h     |  8 ++++----
 3 files changed, 24 insertions(+), 27 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index e9b18fc17617..8bfafbd680ab 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -893,7 +893,7 @@ static void xhci_del_comp_mod_timer(struct xhci_hcd *xhci, u32 status,
 	if ((xhci->port_status_u0 != all_ports_seen_u0) && port_in_u0) {
 		xhci->port_status_u0 |= 1 << wIndex;
 		if (xhci->port_status_u0 == all_ports_seen_u0) {
-			del_timer_sync(&xhci->comp_mode_recovery_timer);
+			del_timer_sync(&xhci->port_check_timer);
 			xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
 				"All USB3 ports have entered U0 already!");
 			xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index e1136a6b9372..e1b3d1063f6b 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -475,7 +475,7 @@ static inline void xhci_msix_sync_irqs(struct xhci_hcd *xhci)
 
 #endif
 
-static void compliance_mode_recovery(struct timer_list *t)
+static void port_check(struct timer_list *t)
 {
 	struct xhci_hcd *xhci;
 	struct usb_hcd *hcd;
@@ -483,7 +483,7 @@ static void compliance_mode_recovery(struct timer_list *t)
 	u32 temp;
 	int i;
 
-	xhci = from_timer(xhci, t, comp_mode_recovery_timer);
+	xhci = from_timer(xhci, t, port_check_timer);
 	rhub = &xhci->usb3_rhub;
 
 	for (i = 0; i < rhub->num_ports; i++) {
@@ -508,8 +508,8 @@ static void compliance_mode_recovery(struct timer_list *t)
 	}
 
 	if (xhci->port_status_u0 != ((1 << rhub->num_ports) - 1))
-		mod_timer(&xhci->comp_mode_recovery_timer,
-			jiffies + msecs_to_jiffies(COMP_MODE_RCVRY_MSECS));
+		mod_timer(&xhci->port_check_timer,
+			jiffies + msecs_to_jiffies(PORT_CHECK_MSECS));
 }
 
 /*
@@ -522,15 +522,14 @@ static void compliance_mode_recovery(struct timer_list *t)
  * status event is generated when entering compliance mode (per xhci spec),
  * this quirk is needed on systems that have the failing hardware installed.
  */
-static void compliance_mode_recovery_timer_init(struct xhci_hcd *xhci)
+static void port_check_timer_init(struct xhci_hcd *xhci)
 {
 	xhci->port_status_u0 = 0;
-	timer_setup(&xhci->comp_mode_recovery_timer, compliance_mode_recovery,
-		    0);
-	xhci->comp_mode_recovery_timer.expires = jiffies +
-			msecs_to_jiffies(COMP_MODE_RCVRY_MSECS);
+	timer_setup(&xhci->port_check_timer, port_check, 0);
+	xhci->port_check_timer.expires = jiffies +
+			msecs_to_jiffies(PORT_CHECK_MSECS);
 
-	add_timer(&xhci->comp_mode_recovery_timer);
+	add_timer(&xhci->port_check_timer);
 	xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
 			"Compliance mode recovery timer initialized");
 }
@@ -596,7 +595,7 @@ static int xhci_init(struct usb_hcd *hcd)
 	/* Initializing Compliance Mode Recovery Data If Needed */
 	if (xhci_compliance_mode_recovery_timer_quirk_check()) {
 		xhci->quirks |= XHCI_COMP_MODE_QUIRK;
-		compliance_mode_recovery_timer_init(xhci);
+		port_check_timer_init(xhci);
 	}
 
 	return retval;
@@ -739,10 +738,9 @@ static void xhci_stop(struct usb_hcd *hcd)
 	/* Deleting Compliance Mode Recovery Timer */
 	if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
 			(!(xhci_all_ports_seen_u0(xhci)))) {
-		del_timer_sync(&xhci->comp_mode_recovery_timer);
+		del_timer_sync(&xhci->port_check_timer);
 		xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
-				"%s: compliance mode recovery timer deleted",
-				__func__);
+				"%s: port check timer deleted", __func__);
 	}
 
 	if (xhci->quirks & XHCI_AMD_PLL_FIX)
@@ -1057,15 +1055,14 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
 	spin_unlock_irq(&xhci->lock);
 
 	/*
-	 * Deleting Compliance Mode Recovery Timer because the xHCI Host
+	 * Deleting Port Check Timer because the xHCI Host
 	 * is about to be suspended.
 	 */
 	if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
 			(!(xhci_all_ports_seen_u0(xhci)))) {
-		del_timer_sync(&xhci->comp_mode_recovery_timer);
+		del_timer_sync(&xhci->port_check_timer);
 		xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
-				"%s: compliance mode recovery timer deleted",
-				__func__);
+				"%s: port check timer deleted", __func__);
 	}
 
 	/* step 5: remove core well power */
@@ -1150,9 +1147,9 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
 
 		if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
 				!(xhci_all_ports_seen_u0(xhci))) {
-			del_timer_sync(&xhci->comp_mode_recovery_timer);
+			del_timer_sync(&xhci->port_check_timer);
 			xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
-				"Compliance Mode Recovery Timer deleted!");
+				"Port Check Timer deleted!");
 		}
 
 		/* Let the USB core know _both_ roothubs lost power. */
@@ -1245,13 +1242,13 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
 		}
 	}
 	/*
-	 * If system is subject to the Quirk, Compliance Mode Timer needs to
+	 * If system is subject to the Quirk, Port Check Timer needs to
 	 * be re-initialized Always after a system resume. Ports are subject
 	 * to suffer the Compliance Mode issue again. It doesn't matter if
 	 * ports have entered previously to U0 before system's suspension.
 	 */
 	if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) && !comp_timer_running)
-		compliance_mode_recovery_timer_init(xhci);
+		port_check_timer_init(xhci);
 
 	if (xhci->quirks & XHCI_ASMEDIA_MODIFY_FLOWCONTROL)
 		usb_asmedia_modifyflowcontrol(to_pci_dev(hcd->self.controller));
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 27d2c1176dd1..b52b7dcb5bb9 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1862,12 +1862,12 @@ struct xhci_hcd {
 	/* cached extended protocol port capabilities */
 	struct xhci_port_cap	*port_caps;
 	unsigned int		num_port_caps;
-	/* Compliance Mode Recovery Data */
-	struct timer_list	comp_mode_recovery_timer;
+	/* For quirks that require to poll for port status */
+	struct timer_list	port_check_timer;
 	u32			port_status_u0;
 	u16			test_mode;
-/* Compliance Mode Timer Triggered every 2 seconds */
-#define COMP_MODE_RCVRY_MSECS 2000
+/* Port polling frequency */
+#define PORT_CHECK_MSECS 2000
 
 	/* Track max eSS interval for XHCI_ISOC_BLOCKED_DISCONNECT */
 	unsigned int		max_ess_interval;
-- 
2.28.0

[PATCH v2 6/7] usb: xhci: Workaround lost disconnect port status

[Thinh Nguyen]

If an eSS device with active periodic transfers is disconnected from the
DWC_usb31 (v1.90a and prior) host controller at root port, the host
controller may not detect a disconnection. By active transfers, it
means that the endpoint is not in flow control, and there are active
Transfer Descriptors available for the host to initiate transfers to the
endpoint. This issue can occur if the endpoint periodic interval is in
2ms, 4ms, or 8ms.

In addition, the host controller will not be able to detect a new device
connection while the disconnection is not processed. The controller will
set the link state of the affected port to eSS_INACTIVE.

To workaround this, have the xHCI driver polls for the eSS root port
status every 2 seconds. If eSS_INACTIVE state is detected, initiate a
fake connection change to stop all the active endpoints and start
polling for new connection change.

Since XHCI_COMP_MODE_QUIRK is basically doing the same thing except for
polling for a different link state, we will use the same timer and
polling rate for this new workaround. Introduce a new quirk
XHCI_LOST_DISCONNECT_QUIRK to poll for eSS_INACTIVE port link state and
fake a connection change.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - None

 drivers/usb/host/xhci-hub.c     | 10 +++++++-
 drivers/usb/host/xhci.c         | 44 ++++++++++++++++++++++++---------
 include/linux/usb/xhci-quirks.h |  1 +
 3 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 8bfafbd680ab..32fbf95f021b 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -868,6 +868,11 @@ static void xhci_hub_report_usb3_link_state(struct xhci_hcd *xhci,
 		if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
 				(pls == USB_SS_PORT_LS_COMP_MOD))
 			pls |= USB_PORT_STAT_CONNECTION;
+
+		/* Fake a connection change */
+		if ((xhci->quirks & XHCI_LOST_DISCONNECT_QUIRK) &&
+				pls == XDEV_INACTIVE)
+			pls |= USB_PORT_STAT_CONNECTION;
 	}
 
 	/* update status field */
@@ -887,7 +892,8 @@ static void xhci_del_comp_mod_timer(struct xhci_hcd *xhci, u32 status,
 	u32 all_ports_seen_u0 = ((1 << xhci->usb3_rhub.num_ports) - 1);
 	bool port_in_u0 = ((status & PORT_PLS_MASK) == XDEV_U0);
 
-	if (!(xhci->quirks & XHCI_COMP_MODE_QUIRK))
+	if (!(xhci->quirks & XHCI_COMP_MODE_QUIRK) ||
+	    (xhci->quirks & XHCI_LOST_DISCONNECT_QUIRK))
 		return;
 
 	if ((xhci->port_status_u0 != all_ports_seen_u0) && port_in_u0) {
@@ -1654,6 +1660,8 @@ int xhci_hub_status_data(struct usb_hcd *hcd, char *buf)
 		trace_xhci_hub_status_data(i, temp);
 
 		if ((temp & mask) != 0 ||
+			((xhci->quirks & XHCI_LOST_DISCONNECT_QUIRK) &&
+				(temp & PORT_PLS_MASK) == XDEV_INACTIVE) ||
 			(bus_state->port_c_suspend & 1 << i) ||
 			(bus_state->resume_done[i] && time_after_eq(
 			    jiffies, bus_state->resume_done[i]))) {
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index e1b3d1063f6b..62275ee88849 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -485,10 +485,14 @@ static void port_check(struct timer_list *t)
 
 	xhci = from_timer(xhci, t, port_check_timer);
 	rhub = &xhci->usb3_rhub;
+	hcd = xhci->shared_hcd;
 
 	for (i = 0; i < rhub->num_ports; i++) {
+		bool poll_rhub = false;
+
 		temp = readl(rhub->ports[i]->addr);
-		if ((temp & PORT_PLS_MASK) == USB_SS_PORT_LS_COMP_MOD) {
+		if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
+		    ((temp & PORT_PLS_MASK) == USB_SS_PORT_LS_COMP_MOD)) {
 			/*
 			 * Compliance Mode Detected. Letting USB Core
 			 * handle the Warm Reset
@@ -498,8 +502,15 @@ static void port_check(struct timer_list *t)
 					i + 1);
 			xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
 					"Attempting compliance mode recovery");
-			hcd = xhci->shared_hcd;
 
+			poll_rhub = true;
+		}
+
+		if ((xhci->quirks & XHCI_LOST_DISCONNECT_QUIRK) &&
+		    ((temp & PORT_PLS_MASK) == XDEV_INACTIVE))
+			poll_rhub = true;
+
+		if (poll_rhub) {
 			if (hcd->state == HC_STATE_SUSPENDED)
 				usb_hcd_resume_root_hub(hcd);
 
@@ -507,7 +518,9 @@ static void port_check(struct timer_list *t)
 		}
 	}
 
-	if (xhci->port_status_u0 != ((1 << rhub->num_ports) - 1))
+	if ((xhci->quirks & XHCI_LOST_DISCONNECT_QUIRK) ||
+	    ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
+	     xhci->port_status_u0 != ((1 << rhub->num_ports) - 1)))
 		mod_timer(&xhci->port_check_timer,
 			jiffies + msecs_to_jiffies(PORT_CHECK_MSECS));
 }
@@ -593,10 +606,12 @@ static int xhci_init(struct usb_hcd *hcd)
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Finished xhci_init");
 
 	/* Initializing Compliance Mode Recovery Data If Needed */
-	if (xhci_compliance_mode_recovery_timer_quirk_check()) {
+	if (xhci_compliance_mode_recovery_timer_quirk_check())
 		xhci->quirks |= XHCI_COMP_MODE_QUIRK;
+
+	if (xhci->quirks & XHCI_LOST_DISCONNECT_QUIRK ||
+	    xhci->quirks & XHCI_COMP_MODE_QUIRK)
 		port_check_timer_init(xhci);
-	}
 
 	return retval;
 }
@@ -736,8 +751,9 @@ static void xhci_stop(struct usb_hcd *hcd)
 	xhci_cleanup_msix(xhci);
 
 	/* Deleting Compliance Mode Recovery Timer */
-	if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
-			(!(xhci_all_ports_seen_u0(xhci)))) {
+	if ((xhci->quirks & XHCI_LOST_DISCONNECT_QUIRK) ||
+	    ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
+	     !(xhci_all_ports_seen_u0(xhci)))) {
 		del_timer_sync(&xhci->port_check_timer);
 		xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
 				"%s: port check timer deleted", __func__);
@@ -1058,8 +1074,9 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
 	 * Deleting Port Check Timer because the xHCI Host
 	 * is about to be suspended.
 	 */
-	if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
-			(!(xhci_all_ports_seen_u0(xhci)))) {
+	if ((xhci->quirks & XHCI_LOST_DISCONNECT_QUIRK) ||
+	    ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
+	     !(xhci_all_ports_seen_u0(xhci)))) {
 		del_timer_sync(&xhci->port_check_timer);
 		xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
 				"%s: port check timer deleted", __func__);
@@ -1145,8 +1162,9 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
 	/* If restore operation fails, re-initialize the HC during resume */
 	if ((temp & STS_SRE) || hibernated) {
 
-		if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
-				!(xhci_all_ports_seen_u0(xhci))) {
+		if ((xhci->quirks & XHCI_LOST_DISCONNECT_QUIRK) ||
+		    ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
+		     !(xhci_all_ports_seen_u0(xhci)))) {
 			del_timer_sync(&xhci->port_check_timer);
 			xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
 				"Port Check Timer deleted!");
@@ -1247,7 +1265,9 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
 	 * to suffer the Compliance Mode issue again. It doesn't matter if
 	 * ports have entered previously to U0 before system's suspension.
 	 */
-	if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) && !comp_timer_running)
+	if (!comp_timer_running &&
+	    ((xhci->quirks & XHCI_LOST_DISCONNECT_QUIRK) ||
+	     (xhci->quirks & XHCI_COMP_MODE_QUIRK)))
 		port_check_timer_init(xhci);
 
 	if (xhci->quirks & XHCI_ASMEDIA_MODIFY_FLOWCONTROL)
diff --git a/include/linux/usb/xhci-quirks.h b/include/linux/usb/xhci-quirks.h
index 5bedf5a25f7a..df92e78b8083 100644
--- a/include/linux/usb/xhci-quirks.h
+++ b/include/linux/usb/xhci-quirks.h
@@ -60,5 +60,6 @@
 #define XHCI_NO_SOFT_RETRY		BIT_ULL(40)
 #define XHCI_ISOC_BLOCKED_DISCONNECT	BIT_ULL(41)
 #define XHCI_LIMIT_FS_BI_INTR_EP	BIT_ULL(42)
+#define XHCI_LOST_DISCONNECT_QUIRK	BIT_ULL(43)
 
 #endif /* __LINUX_USB_XHCI_QUIRKS_H */
-- 
2.28.0

Re: [PATCH v2 6/7] usb: xhci: Workaround lost disconnect port status

[Mathias Nyman]

On 10.4.2021 3.47, Thinh Nguyen wrote:
> If an eSS device with active periodic transfers is disconnected from the
> DWC_usb31 (v1.90a and prior) host controller at root port, the host
> controller may not detect a disconnection. By active transfers, it
> means that the endpoint is not in flow control, and there are active
> Transfer Descriptors available for the host to initiate transfers to the
> endpoint. This issue can occur if the endpoint periodic interval is in
> 2ms, 4ms, or 8ms.
> 
> In addition, the host controller will not be able to detect a new device
> connection while the disconnection is not processed. The controller will
> set the link state of the affected port to eSS_INACTIVE.
> 
> To workaround this, have the xHCI driver polls for the eSS root port
> status every 2 seconds. If eSS_INACTIVE state is detected, initiate a
> fake connection change to stop all the active endpoints and start
> polling for new connection change.


If this only happens with active periodic transfers can we skip the polling
and check port link state in the transfer error event handling?

Like in the previous case there the periodic device was behind a hub.

-Mathias

Re: [PATCH v2 6/7] usb: xhci: Workaround lost disconnect port status

[Thinh Nguyen]

Mathias Nyman wrote:
> On 10.4.2021 3.47, Thinh Nguyen wrote:
>> If an eSS device with active periodic transfers is disconnected from the
>> DWC_usb31 (v1.90a and prior) host controller at root port, the host
>> controller may not detect a disconnection. By active transfers, it
>> means that the endpoint is not in flow control, and there are active
>> Transfer Descriptors available for the host to initiate transfers to the
>> endpoint. This issue can occur if the endpoint periodic interval is in
>> 2ms, 4ms, or 8ms.
>>
>> In addition, the host controller will not be able to detect a new device
>> connection while the disconnection is not processed. The controller will
>> set the link state of the affected port to eSS_INACTIVE.
>>
>> To workaround this, have the xHCI driver polls for the eSS root port
>> status every 2 seconds. If eSS_INACTIVE state is detected, initiate a
>> fake connection change to stop all the active endpoints and start
>> polling for new connection change.
> 
> 
> If this only happens with active periodic transfers can we skip the polling
> and check port link state in the transfer error event handling?
> 
> Like in the previous case there the periodic device was behind a hub.
> 

No, this occurs with an application with interrupt endpoint (but also
affects isoc). We may not get a transfer completion event to notify to
check for port link state. I hope this change is not too intrusive since
we can reuse a timer polling for port status.

Thanks,
Thinh

[PATCH v2 7/7] usb: dwc3: host: Set quirks base on version

[Thinh Nguyen]

We can check for host quirks at runtime base on the controller IP and
version check. Set the following quirks for the DWC_usb31 IP host mode
before creating a platform device for the xHCI driver:

 * XHCI_ISOC_BLOCKED_DISCONNECT
 * XHCI_LIMIT_FS_BI_INTR_EP
 * XHCI_LOST_DISCONNECT_QUIRK

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - None

 drivers/usb/dwc3/host.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index f29a264635aa..8abb9be32cbb 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -9,6 +9,8 @@
 
 #include <linux/acpi.h>
 #include <linux/platform_device.h>
+#include <linux/usb/xhci-quirks.h>
+#include <linux/usb/xhci-plat.h>
 
 #include "core.h"
 
@@ -42,6 +44,17 @@ static int dwc3_host_get_irq(struct dwc3 *dwc)
 	return irq;
 }
 
+static void dwc3_host_init_quirks(struct dwc3 *dwc, struct xhci_plat_priv *priv)
+{
+	memset(priv, 0, sizeof(*priv));
+
+	if (DWC3_VER_IS_WITHIN(DWC31, ANY, 190A)) {
+		priv->quirks |= XHCI_ISOC_BLOCKED_DISCONNECT;
+		priv->quirks |= XHCI_LIMIT_FS_BI_INTR_EP;
+		priv->quirks |= XHCI_LOST_DISCONNECT_QUIRK;
+	}
+}
+
 int dwc3_host_init(struct dwc3 *dwc)
 {
 	struct property_entry	props[4];
@@ -49,6 +62,7 @@ int dwc3_host_init(struct dwc3 *dwc)
 	int			ret, irq;
 	struct resource		*res;
 	struct platform_device	*dwc3_pdev = to_platform_device(dwc->dev);
+	struct xhci_plat_priv	dwc3_priv;
 	int			prop_idx = 0;
 
 	irq = dwc3_host_get_irq(dwc);
@@ -87,6 +101,14 @@ int dwc3_host_init(struct dwc3 *dwc)
 		goto err;
 	}
 
+	dwc3_host_init_quirks(dwc, &dwc3_priv);
+
+	ret = platform_device_add_data(xhci, &dwc3_priv, sizeof(dwc3_priv));
+	if (ret) {
+		dev_err(dwc->dev, "couldn't add platform data to xHCI device\n");
+		goto err;
+	}
+
 	memset(props, 0, sizeof(struct property_entry) * ARRAY_SIZE(props));
 
 	if (dwc->usb3_lpm_capable)
-- 
2.28.0

Re: [PATCH v2 0/7] usb: Set quirks for xhci/dwc3 host mode

[Thinh Nguyen]

Hi Mathias,

Thinh Nguyen wrote:
> This series add 3 new quirks for DWC_usb31 host mode:
>  * XHCI_ISOC_BLOCKED_DISCONNECT
>  * XHCI_LIMIT_FS_BI_INTR_EP
>  * XHCI_LOST_DISCONNECT_QUIRK
> 
> Different versions of DWC_usb3x controllers have different quirks. Typically we
> set them based on PCI device VID:PID or DT compatible strings. However, we know
> that a particular IP version(s) may share a common quirk across different
> platform. We can enable these quirks based on the IP type and version number.
> This simplifies the designer work and consolidate the logic check. To do this,
> we will need to expose the xHCI quirks to the common header along with the
> private platform structure.
> 
> 
> Changes in v2:
> - Instead of combining xhci-plat private structure in xhci-squirks.h, keep it
>   as a separate header file
> 
> 
> Thinh Nguyen (7):
>   usb: xhci: Move quirks definitions to common usb header
>   usb: xhci: Move xhci-plat header to common usb header
>   usb: xhci: Check for blocked disconnection
>   usb: xhci: Workaround undercalculated BW for fullspeed BI
>   usb: xhci: Rename Compliance mode timer quirk
>   usb: xhci: Workaround lost disconnect port status
>   usb: dwc3: host: Set quirks base on version
> 
>  drivers/usb/cdns3/host.c                      |   2 +-
>  drivers/usb/dwc3/host.c                       |  22 +++
>  drivers/usb/host/xhci-hub.c                   |  12 +-
>  drivers/usb/host/xhci-mem.c                   |  26 ++++
>  drivers/usb/host/xhci-plat.c                  |   2 +-
>  drivers/usb/host/xhci-rcar.c                  |   2 +-
>  drivers/usb/host/xhci-ring.c                  |  76 ++++++++++
>  drivers/usb/host/xhci.c                       | 134 +++++++++++++-----
>  drivers/usb/host/xhci.h                       |  71 ++--------
>  .../host => include/linux/usb}/xhci-plat.h    |  18 +--
>  include/linux/usb/xhci-quirks.h               |  65 +++++++++
>  11 files changed, 326 insertions(+), 104 deletions(-)
>  rename {drivers/usb/host => include/linux/usb}/xhci-plat.h (54%)
>  create mode 100644 include/linux/usb/xhci-quirks.h
> 
> 
> base-commit: 496960274153bdeb9d1f904ff1ea875cef8232c1
> 

Did you get a chance to take a look at this series?

Thanks,
Thinh