usb: Set quirks for xhci/dwc3 host mode

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

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

drivers/usb/host/xhci-plat.c +0 -1

index c1edcc9b13ce..716ef3a338db 100644
@@ -21,7 +21,6 @@
 #include <linux/usb/of.h>
 
 #include "xhci.h"
-#include "xhci-plat.h"
 #include "xhci-mvebu.h"
 #include "xhci-rcar.h"
 

drivers/usb/host/xhci-plat.h deleted +0 -25

deleted file mode 100644
index 561d0b7bce09..000000000000
@@ -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 */

drivers/usb/host/xhci-rcar.c +0 -1

index 1bc4fe7b8c75..7690bee365fd 100644
@@ -14,7 +14,6 @@
 #include <linux/sys_soc.h>
 
 #include "xhci.h"
-#include "xhci-plat.h"
 #include "xhci-rcar.h"
 
 /*

drivers/usb/host/xhci.h +1 -52

index 2595a8f057c4..9a4e2808668b 100644
@@ -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;

include/linux/usb/xhci-quirks.h new file +65 -0

new file mode 100644
index 000000000000..c2cb35c5b273
@@ -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;

Quoted 216 lines · 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 <[email protected]>

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

drivers/usb/host/xhci-plat.c +0 -1

index c1edcc9b13ce..716ef3a338db 100644
@@ -21,7 +21,6 @@
 #include <linux/usb/of.h>
 
 #include "xhci.h"
-#include "xhci-plat.h"
 #include "xhci-mvebu.h"
 #include "xhci-rcar.h"
 

drivers/usb/host/xhci-plat.h deleted +0 -25

deleted file mode 100644
index 561d0b7bce09..000000000000
@@ -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 */

drivers/usb/host/xhci-rcar.c +0 -1

index 1bc4fe7b8c75..7690bee365fd 100644
@@ -14,7 +14,6 @@
 #include <linux/sys_soc.h>
 
 #include "xhci.h"
-#include "xhci-plat.h"
 #include "xhci-rcar.h"
 
 /*

drivers/usb/host/xhci.h +1 -52

index 2595a8f057c4..9a4e2808668b 100644
@@ -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;

include/linux/usb/xhci-quirks.h new file +65 -0

new file mode 100644
index 000000000000..c2cb35c5b273
@@ -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;

include/linux/usb/xhci-quirks.h line 75

Quoted 10 lines

include/linux/usb/xhci-quirks.h new file +10 -0

+	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

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

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

drivers/usb/dwc3/host.c +12 -0

index f29a264635aa..a486d7fbb163 100644
@@ -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];

Quoted 44 lines · 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 <[email protected]>

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

drivers/usb/dwc3/host.c +12 -0

index f29a264635aa..a486d7fbb163 100644
@@ -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];

drivers/usb/dwc3/host.c int dwc3_host_init(struct dwc3 *dwc) · line 64

Quoted 4 lines

drivers/usb/dwc3/host.c +1 -0

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

Quoted 53 lines · Greg Kroah-Hartman

Quoted 44 lines · 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 <[email protected]>

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

drivers/usb/dwc3/host.c +12 -0

index f29a264635aa..a486d7fbb163 100644
@@ -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];

drivers/usb/dwc3/host.c int dwc3_host_init(struct dwc3 *dwc) · line 64

Quoted 4 lines

drivers/usb/dwc3/host.c +1 -0

@@ -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?

Quoted 2 lines

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.

Quoted 57 lines · Thinh Nguyen

Quoted 53 lines · Greg Kroah-Hartman

Quoted 44 lines · 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 <[email protected]>

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

drivers/usb/dwc3/host.c +12 -0

index f29a264635aa..a486d7fbb163 100644
@@ -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];

drivers/usb/dwc3/host.c int dwc3_host_init(struct dwc3 *dwc) · line 64

Quoted 4 lines

drivers/usb/dwc3/host.c +1 -0

@@ -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?

Quoted 2 lines

greg k-h

Quoted 6 lines

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 :)

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.

Hi Thinh

Sorry about the delay.

Quoted 8 lines

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.

Quoted 9 lines

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?

Quoted 10 lines

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.

Quoted 19 lines · Thinh Nguyen

Quoted 17 lines

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.

Quoted 2 lines

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.

Quoted 12 lines

Quoted 8 lines

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?

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 19 lines · Thinh Nguyen

Quoted 19 lines · Thinh Nguyen

Quoted 17 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 12 lines

Quoted 12 lines

Quoted 8 lines

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?

Quoted 7 lines

Quoted 3 lines

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?

Quoted 10 lines

Quoted 9 lines

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.

Quoted 3 lines

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

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

---
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(+)

drivers/usb/host/xhci-mem.c +13 -0

index 1053b43008e4..e01d0ddb012a 100644
@@ -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)

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.

Quoted 8 lines · 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.

Quoted 8 lines

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.

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