[RFC PATCH 0/4] usb: xhci: Introduce xhci-snps

[RFC PATCH 0/4] usb: xhci: Introduce xhci-snps

[Thinh Nguyen]

Synopsys DWC_usb3x IPs are used on many different platforms. Since they share
the same IP, often the quirks are common across different platforms and
versions. This drives the need to find a way to handle all the common (and
platform specific) quirks and separate its logic from dwc3 and xhci core logic.
Hopefully it can also reduce introducing new device properties while
maintaining abstraction.

So, let's create a xhci-snps glue extension that can apply to xhci-plat and
xhci-pci glue drivers and teach it to handle DWC_usb3x hosts. For this
particular change, we'll start with xhci-plat glue driver.

NOTE: This is a quick implementation of how I'd imagine to handle this. I
apologize if it may lack documentation. It doesn't have all the common quirks
added. I'd like to receive some feedbacks before moving forward.

Many thanks!
Thinh


Thinh Nguyen (4):
  dt-bindings: usb: usb-xhci: Add xhci-snps-quirks
  usb: dwc3: host: Always set xhci-snps-quirks
  usb: dwc3: core: Share global register access with xhci driver
  usb: xhci: Introduce Synopsys glue extension for DWC_usb3x

 .../devicetree/bindings/usb/usb-xhci.yaml     |   4 +
 drivers/usb/dwc3/core.c                       |   4 +-
 drivers/usb/dwc3/host.c                       |   4 +-
 drivers/usb/host/Kconfig                      |   8 +
 drivers/usb/host/Makefile                     |   3 +
 drivers/usb/host/xhci-plat.c                  |  40 ++++
 drivers/usb/host/xhci-plat.h                  |   3 +
 drivers/usb/host/xhci-snps.c                  | 185 ++++++++++++++++++
 drivers/usb/host/xhci-snps.h                  |  32 +++
 9 files changed, 280 insertions(+), 3 deletions(-)
 create mode 100644 drivers/usb/host/xhci-snps.c
 create mode 100644 drivers/usb/host/xhci-snps.h


base-commit: 97fa5887cf283bb75ffff5f6b2c0e71794c02400
-- 
2.28.0

[RFC PATCH 1/4] dt-bindings: usb: usb-xhci: Add xhci-snps-quirks

[Thinh Nguyen]

Set this property to use xhci-snps extension to handle common Synopsys
DWC_usb3x host quirks.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Documentation/devicetree/bindings/usb/usb-xhci.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.yaml b/Documentation/devicetree/bindings/usb/usb-xhci.yaml
index 965f87fef702..540044a087a7 100644
--- a/Documentation/devicetree/bindings/usb/usb-xhci.yaml
+++ b/Documentation/devicetree/bindings/usb/usb-xhci.yaml
@@ -29,6 +29,10 @@ properties:
     description: Interrupt moderation interval
     default: 5000
 
+  xhci-snps-quirks:
+    description: Handles common Synopsys DWC_usb3x host quirks
+    type: boolean
+
 additionalProperties: true
 
 examples:
-- 
2.28.0

[RFC PATCH 2/4] usb: dwc3: host: Always set xhci-snps-quirks

[Thinh Nguyen]

Always enable xhci-snps-quirks to use xhci-snps glue extension. The
xhci-snps extension can fine tune xhci driver to better handle Synopsys
DWC_usb3x IPs.

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

diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index f56c30cf151e..ce359862f9aa 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -66,7 +66,7 @@ static int dwc3_host_get_irq(struct dwc3 *dwc)
 
 int dwc3_host_init(struct dwc3 *dwc)
 {
-	struct property_entry	props[4];
+	struct property_entry	props[5];
 	struct platform_device	*xhci;
 	int			ret, irq;
 	int			prop_idx = 0;
@@ -94,6 +94,8 @@ int dwc3_host_init(struct dwc3 *dwc)
 
 	memset(props, 0, sizeof(struct property_entry) * ARRAY_SIZE(props));
 
+	props[prop_idx++] = PROPERTY_ENTRY_BOOL("xhci-snps-quirks");
+
 	if (dwc->usb3_lpm_capable)
 		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb3-lpm-capable");
 
-- 
2.28.0

[RFC PATCH 3/4] usb: dwc3: core: Share global register access with xhci driver

[Thinh Nguyen]

When the dwc3 driver create a xhci platform device and pass its control
to the xhci driver, allow it to also have access to dwc3 global
registers. The xhci-snps needs access to the global register parameters
to handle some quirks.

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

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index e027c0420dc3..b19b7b540182 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1697,7 +1697,7 @@ static int dwc3_probe(struct platform_device *pdev)
 
 	dwc->xhci_resources[0].start = res->start;
 	dwc->xhci_resources[0].end = dwc->xhci_resources[0].start +
-					DWC3_XHCI_REGS_END;
+					DWC3_GLOBALS_REGS_END;
 	dwc->xhci_resources[0].flags = res->flags;
 	dwc->xhci_resources[0].name = res->name;
 
@@ -1708,7 +1708,7 @@ static int dwc3_probe(struct platform_device *pdev)
 	dwc_res = *res;
 	dwc_res.start += DWC3_GLOBALS_REGS_START;
 
-	regs = devm_ioremap_resource(dev, &dwc_res);
+	regs = devm_ioremap(dev, dwc_res.start, resource_size(&dwc_res));
 	if (IS_ERR(regs))
 		return PTR_ERR(regs);
 
-- 
2.28.0

[RFC PATCH 4/4] usb: xhci: Introduce Synopsys glue extension for DWC_usb3x

[Thinh Nguyen]

Synopsys DWC_usb3x IPs are used on many different platforms. Since they
share the same IP, often the quirks are common across different
platforms and versions. This drives the need to find a way to handle all
the common (and platform specific) quirks and separate its logic from
dwc3 and xhci core logic. Hopefully this helps reduce introducing new
device properties while maintaining abstraction.

So, let's create a xhci-snps glue extension that can apply to xhci-plat
and xhci-pci glue drivers and teach it to handle DWC_usb3x hosts. For
this particular change, we'll start with xhci-plat glue driver.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/host/Kconfig     |   8 ++
 drivers/usb/host/Makefile    |   3 +
 drivers/usb/host/xhci-plat.c |  40 ++++++++
 drivers/usb/host/xhci-plat.h |   3 +
 drivers/usb/host/xhci-snps.c | 185 +++++++++++++++++++++++++++++++++++
 drivers/usb/host/xhci-snps.h |  32 ++++++
 6 files changed, 271 insertions(+)
 create mode 100644 drivers/usb/host/xhci-snps.c
 create mode 100644 drivers/usb/host/xhci-snps.h

diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 57ca5f97a3dc..efbfb79baf44 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -62,6 +62,14 @@ config USB_XHCI_PLATFORM
 
 	  If unsure, say N.
 
+config USB_XHCI_SNPS
+	bool "xHCI fine tune for Synopsys platforms"
+	help
+	  Say 'Y' to enable additional fine tune for Synopsys DWC_usb3x xHCI
+	  controllers.
+
+	  If unsure, say N.
+
 config USB_XHCI_HISTB
 	tristate "xHCI support for HiSilicon STB SoCs"
 	depends on USB_XHCI_PLATFORM && (ARCH_HISI || COMPILE_TEST)
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index 2948983618fb..066b78fbc1d1 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -28,6 +28,9 @@ endif
 ifneq ($(CONFIG_USB_XHCI_RCAR), )
 	xhci-plat-hcd-y		+= xhci-rcar.o
 endif
+ifneq ($(CONFIG_USB_XHCI_SNPS), )
+	xhci-plat-hcd-y		+= xhci-snps.o
+endif
 
 ifneq ($(CONFIG_DEBUG_FS),)
 	xhci-hcd-y		+= xhci-debugfs.o
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 044855818cb1..73cc0262cc75 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -24,16 +24,20 @@
 #include "xhci-plat.h"
 #include "xhci-mvebu.h"
 #include "xhci-rcar.h"
+#include "xhci-snps.h"
 
 static struct hc_driver __read_mostly xhci_plat_hc_driver;
 
 static int xhci_plat_setup(struct usb_hcd *hcd);
 static int xhci_plat_start(struct usb_hcd *hcd);
+static int xhci_plat_add_endpoint(struct usb_hcd *hcd, struct usb_device *udev,
+				  struct usb_host_endpoint *ep);
 
 static const struct xhci_driver_overrides xhci_plat_overrides __initconst = {
 	.extra_priv_size = sizeof(struct xhci_plat_priv),
 	.reset = xhci_plat_setup,
 	.start = xhci_plat_start,
+	.add_endpoint = xhci_plat_add_endpoint,
 };
 
 static void xhci_priv_plat_start(struct usb_hcd *hcd)
@@ -105,6 +109,20 @@ static int xhci_plat_start(struct usb_hcd *hcd)
 	return xhci_run(hcd);
 }
 
+static int xhci_plat_add_endpoint(struct usb_hcd *hcd, struct usb_device *udev,
+				  struct usb_host_endpoint *ep)
+{
+	struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
+	int ret;
+
+	if (priv && priv->add_endpoint_quirk)
+		ret = priv->add_endpoint_quirk(hcd, udev, ep);
+	else
+		ret = xhci_add_endpoint(hcd, udev, ep);
+
+	return ret;
+}
+
 #ifdef CONFIG_OF
 static const struct xhci_plat_priv xhci_plat_marvell_armada = {
 	.init_quirk = xhci_mvebu_mbus_init_quirk,
@@ -173,6 +191,25 @@ static const struct of_device_id usb_xhci_of_match[] = {
 MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
 #endif
 
+static int xhci_plat_setup_snps_quirks(struct usb_hcd *hcd)
+{
+	struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_USB_XHCI_SNPS))
+		return 0;
+
+	ret = xhci_snps_init(hcd);
+	if (ret) {
+		dev_err(hcd->self.controller, "SNPS extension setup fails\n");
+		return ret;
+	}
+
+	priv->init_quirk = &xhci_snps_setup;
+	priv->add_endpoint_quirk = &xhci_snps_add_endpoint;
+	return 0;
+}
+
 static int xhci_plat_probe(struct platform_device *pdev)
 {
 	const struct xhci_plat_priv *priv_match;
@@ -301,6 +338,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
 		if (device_property_read_bool(tmpdev, "quirk-broken-port-ped"))
 			xhci->quirks |= XHCI_BROKEN_PORT_PED;
 
+		if (device_property_read_bool(tmpdev, "xhci-snps-quirks"))
+			xhci_plat_setup_snps_quirks(hcd);
+
 		device_property_read_u32(tmpdev, "imod-interval-ns",
 					 &xhci->imod_interval);
 	}
diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h
index 1fb149d1fbce..7b49aba73833 100644
--- a/drivers/usb/host/xhci-plat.h
+++ b/drivers/usb/host/xhci-plat.h
@@ -17,6 +17,9 @@ struct xhci_plat_priv {
 	int (*init_quirk)(struct usb_hcd *);
 	int (*suspend_quirk)(struct usb_hcd *);
 	int (*resume_quirk)(struct usb_hcd *);
+	int (*add_endpoint_quirk)(struct usb_hcd *hcd,
+				  struct usb_device *udev,
+				  struct usb_host_endpoint *ep);
 };
 
 #define hcd_to_xhci_priv(h) ((struct xhci_plat_priv *)hcd_to_xhci(h)->priv)
diff --git a/drivers/usb/host/xhci-snps.c b/drivers/usb/host/xhci-snps.c
new file mode 100644
index 000000000000..932d914a090a
--- /dev/null
+++ b/drivers/usb/host/xhci-snps.c
@@ -0,0 +1,185 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * xhci-snps.c - xHCI glue extension for Synopsys DWC_usb3x IPs.
+ *
+ * Copyright (C) 2022 Synopsys Inc.
+ *
+ * Author: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
+ *
+ * Contains some borrowed code from dwc3 driver.
+ */
+
+#include "xhci.h"
+#include "xhci-snps.h"
+
+#define DWC3_GSNPSID			0xc120
+#define DWC3_VER_NUMBER			0xc1a0
+#define DWC3_VER_TYPE			0xc1a4
+
+#define DWC3_GLOBALS_REGS_END		0xc6ff
+
+#define DWC3_GSNPSID_MASK		0xffff0000
+#define DWC3_GSNPS_ID(p)		(((p) & DWC3_GSNPSID_MASK) >> 16)
+
+#define DWC3_IP				0x5533
+#define DWC31_IP			0x3331
+#define DWC32_IP			0x3332
+
+#define DWC3_REVISION_ANY		0x0
+#define DWC3_REVISION_173A		0x5533173a
+#define DWC3_REVISION_175A		0x5533175a
+#define DWC3_REVISION_180A		0x5533180a
+#define DWC3_REVISION_183A		0x5533183a
+#define DWC3_REVISION_185A		0x5533185a
+#define DWC3_REVISION_187A		0x5533187a
+#define DWC3_REVISION_188A		0x5533188a
+#define DWC3_REVISION_190A		0x5533190a
+#define DWC3_REVISION_194A		0x5533194a
+#define DWC3_REVISION_200A		0x5533200a
+#define DWC3_REVISION_202A		0x5533202a
+#define DWC3_REVISION_210A		0x5533210a
+#define DWC3_REVISION_220A		0x5533220a
+#define DWC3_REVISION_230A		0x5533230a
+#define DWC3_REVISION_240A		0x5533240a
+#define DWC3_REVISION_250A		0x5533250a
+#define DWC3_REVISION_260A		0x5533260a
+#define DWC3_REVISION_270A		0x5533270a
+#define DWC3_REVISION_280A		0x5533280a
+#define DWC3_REVISION_290A		0x5533290a
+#define DWC3_REVISION_300A		0x5533300a
+#define DWC3_REVISION_310A		0x5533310a
+#define DWC3_REVISION_330A		0x5533330a
+
+#define DWC31_REVISION_ANY		0x0
+#define DWC31_REVISION_110A		0x3131302a
+#define DWC31_REVISION_120A		0x3132302a
+#define DWC31_REVISION_160A		0x3136302a
+#define DWC31_REVISION_170A		0x3137302a
+#define DWC31_REVISION_180A		0x3138302a
+#define DWC31_REVISION_190A		0x3139302a
+
+#define DWC32_REVISION_ANY		0x0
+#define DWC32_REVISION_100A		0x3130302a
+
+#define DWC31_VERSIONTYPE_ANY		0x0
+#define DWC31_VERSIONTYPE_EA01		0x65613031
+#define DWC31_VERSIONTYPE_EA02		0x65613032
+#define DWC31_VERSIONTYPE_EA03		0x65613033
+#define DWC31_VERSIONTYPE_EA04		0x65613034
+#define DWC31_VERSIONTYPE_EA05		0x65613035
+#define DWC31_VERSIONTYPE_EA06		0x65613036
+
+#define DWC3_IP_IS(_ip)							\
+	(snps->ip == _ip##_IP)
+
+#define DWC3_VER_IS(_ip, _ver)						\
+	(DWC3_IP_IS(_ip) && snps->revision == _ip##_REVISION_##_ver)
+
+#define DWC3_VER_IS_PRIOR(_ip, _ver)					\
+	(DWC3_IP_IS(_ip) && snps->revision < _ip##_REVISION_##_ver)
+
+#define DWC3_VER_IS_WITHIN(_ip, _from, _to)				\
+	(DWC3_IP_IS(_ip) &&						\
+	 snps->revision >= _ip##_REVISION_##_from &&			\
+	 (!(_ip##_REVISION_##_to) ||					\
+	  snps->revision <= _ip##_REVISION_##_to))
+
+#define DWC3_VER_TYPE_IS_WITHIN(_ip, _ver, _from, _to)			\
+	(DWC3_VER_IS(_ip, _ver) &&					\
+	 snps->version_type >= _ip##_VERSIONTYPE_##_from &&		\
+	 (!(_ip##_VERSIONTYPE_##_to) ||					\
+	  snps->version_type <= _ip##_VERSIONTYPE_##_to))
+
+/**
+ * struct xhci_snps - Wrapper for DWC_usb3x host controller
+ * @hcd: the main host device
+ * @ip: controller's ID
+ * @revision: controller's version of an IP
+ * @version_type: VERSIONTYPE register contents, a sub release of a revision
+ */
+struct xhci_snps {
+	struct usb_hcd	*hcd;
+	u32		ip;
+	u32		revision;
+	u32		version_type;
+};
+
+#define dev_to_xhci_snps(p) (container_of((p), struct xhci_snps, dev))
+
+int xhci_snps_add_endpoint(struct usb_hcd *hcd, struct usb_device *udev,
+			   struct usb_host_endpoint *ep)
+{
+	struct xhci_snps *snps = dev_get_drvdata(hcd->self.controller);
+
+	if (DWC3_VER_IS_WITHIN(DWC31, ANY, 190A) &&
+	    usb_endpoint_xfer_int(&ep->desc) &&
+	    udev->speed == USB_SPEED_FULL && ep->desc.bInterval == 6)
+		ep->desc.bInterval = 5;
+
+	return xhci_add_endpoint(hcd, udev, ep);
+}
+
+static bool xhci_is_snps(struct usb_hcd *hcd)
+{
+	u32 id;
+
+	if (hcd->rsrc_len < DWC3_GLOBALS_REGS_END)
+		return false;
+
+	id = DWC3_GSNPS_ID(readl(hcd->regs + DWC3_GSNPSID));
+
+	return (id == DWC3_IP || id == DWC31_IP || id == DWC32_IP);
+}
+
+static int xhci_snps_init_version(struct xhci_snps *snps)
+{
+	void __iomem *regs = snps->hcd->regs;
+	u32 reg;
+
+	reg = readl(regs + DWC3_GSNPSID);
+	snps->ip = DWC3_GSNPS_ID(reg);
+
+	if (DWC3_IP_IS(DWC3)) {
+		snps->revision = reg;
+	} else if (DWC3_IP_IS(DWC31) || DWC3_IP_IS(DWC32)) {
+		snps->revision = readl(regs + DWC3_VER_NUMBER);
+		snps->version_type = readl(regs + DWC3_VER_TYPE);
+	} else {
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+int xhci_snps_setup(struct usb_hcd *hcd)
+{
+	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+
+	/* These quirks are applicable to all DWC_usb3x IPs and versions */
+	xhci->quirks |= XHCI_SKIP_PHY_INIT | XHCI_SG_TRB_CACHE_SIZE_QUIRK;
+
+	return 0;
+}
+
+int xhci_snps_init(struct usb_hcd *hcd)
+{
+	struct device *dev = hcd->self.controller;
+	struct xhci_snps *snps;
+	int ret;
+
+	if (!xhci_is_snps(hcd))
+		return -EINVAL;
+
+	snps = devm_kzalloc(dev, sizeof(*snps), GFP_KERNEL);
+	if (!snps)
+		return -ENOMEM;
+
+	snps->hcd = hcd;
+
+	ret = xhci_snps_init_version(snps);
+	if (ret)
+		return ret;
+
+	dev_set_drvdata(dev, snps);
+	return 0;
+}
diff --git a/drivers/usb/host/xhci-snps.h b/drivers/usb/host/xhci-snps.h
new file mode 100644
index 000000000000..c35a7ffdeaa3
--- /dev/null
+++ b/drivers/usb/host/xhci-snps.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2022 Synopsys Inc.
+ */
+
+#ifndef _XHCI_SNPS_H
+#define _XHCI_SNPS_H
+
+#if IS_ENABLED(CONFIG_USB_XHCI_SNPS)
+int xhci_snps_init(struct usb_hcd *hcd);
+int xhci_snps_setup(struct usb_hcd *hcd);
+int xhci_snps_add_endpoint(struct usb_hcd *hcd, struct usb_device *udev,
+			   struct usb_host_endpoint *ep);
+#else
+static inline int xhci_snps_init(struct usb_hcd *hcd)
+{
+	return 0;
+}
+
+static inline int xhci_snps_setup(struct usb_hcd *hcd)
+{
+	return 0;
+}
+
+static int xhci_snps_add_endpoint(struct usb_hcd *hcd, struct usb_device *udev,
+				  struct usb_host_endpoint *ep)
+{
+	return 0;
+}
+#endif
+
+#endif /* _XHCI_SNPS_H */
-- 
2.28.0

Re: [RFC PATCH 4/4] usb: xhci: Introduce Synopsys glue extension for DWC_usb3x

[Pavan Kondeti]

Hi Thinh,

On Fri, Jun 03, 2022 at 07:48:28PM -0700, Thinh Nguyen wrote:
> Synopsys DWC_usb3x IPs are used on many different platforms. Since they
> share the same IP, often the quirks are common across different
> platforms and versions. This drives the need to find a way to handle all
> the common (and platform specific) quirks and separate its logic from
> dwc3 and xhci core logic. Hopefully this helps reduce introducing new
> device properties while maintaining abstraction.
> 
> So, let's create a xhci-snps glue extension that can apply to xhci-plat
> and xhci-pci glue drivers and teach it to handle DWC_usb3x hosts. For
> this particular change, we'll start with xhci-plat glue driver.
> 
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> ---
>  drivers/usb/host/Kconfig     |   8 ++
>  drivers/usb/host/Makefile    |   3 +
>  drivers/usb/host/xhci-plat.c |  40 ++++++++
>  drivers/usb/host/xhci-plat.h |   3 +
>  drivers/usb/host/xhci-snps.c | 185 +++++++++++++++++++++++++++++++++++
>  drivers/usb/host/xhci-snps.h |  32 ++++++
>  6 files changed, 271 insertions(+)
>  create mode 100644 drivers/usb/host/xhci-snps.c
>  create mode 100644 drivers/usb/host/xhci-snps.h
> 
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 57ca5f97a3dc..efbfb79baf44 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -62,6 +62,14 @@ config USB_XHCI_PLATFORM
>  
>  	  If unsure, say N.
>  
> +config USB_XHCI_SNPS
> +	bool "xHCI fine tune for Synopsys platforms"
> +	help
> +	  Say 'Y' to enable additional fine tune for Synopsys DWC_usb3x xHCI
> +	  controllers.
> +
> +	  If unsure, say N.
> +

Can this work without compiling in dwc3? i.e directly adding xhci-plat from
device tree or some wrapper to create platform device assuming that DWC3
core is initialized and forced in HOST role?

Thanks,
Pavan

Re: [RFC PATCH 4/4] usb: xhci: Introduce Synopsys glue extension for DWC_usb3x

[Thinh Nguyen]

Pavan Kondeti wrote:
> Hi Thinh,
> 
> On Fri, Jun 03, 2022 at 07:48:28PM -0700, Thinh Nguyen wrote:
>> Synopsys DWC_usb3x IPs are used on many different platforms. Since they
>> share the same IP, often the quirks are common across different
>> platforms and versions. This drives the need to find a way to handle all
>> the common (and platform specific) quirks and separate its logic from
>> dwc3 and xhci core logic. Hopefully this helps reduce introducing new
>> device properties while maintaining abstraction.
>>
>> So, let's create a xhci-snps glue extension that can apply to xhci-plat
>> and xhci-pci glue drivers and teach it to handle DWC_usb3x hosts. For
>> this particular change, we'll start with xhci-plat glue driver.
>>
>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>> ---
>>  drivers/usb/host/Kconfig     |   8 ++
>>  drivers/usb/host/Makefile    |   3 +
>>  drivers/usb/host/xhci-plat.c |  40 ++++++++
>>  drivers/usb/host/xhci-plat.h |   3 +
>>  drivers/usb/host/xhci-snps.c | 185 +++++++++++++++++++++++++++++++++++
>>  drivers/usb/host/xhci-snps.h |  32 ++++++
>>  6 files changed, 271 insertions(+)
>>  create mode 100644 drivers/usb/host/xhci-snps.c
>>  create mode 100644 drivers/usb/host/xhci-snps.h
>>
>> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
>> index 57ca5f97a3dc..efbfb79baf44 100644
>> --- a/drivers/usb/host/Kconfig
>> +++ b/drivers/usb/host/Kconfig
>> @@ -62,6 +62,14 @@ config USB_XHCI_PLATFORM
>>  
>>  	  If unsure, say N.
>>  
>> +config USB_XHCI_SNPS
>> +	bool "xHCI fine tune for Synopsys platforms"
>> +	help
>> +	  Say 'Y' to enable additional fine tune for Synopsys DWC_usb3x xHCI
>> +	  controllers.
>> +
>> +	  If unsure, say N.
>> +
> 
> Can this work without compiling in dwc3? i.e directly adding xhci-plat from
> device tree or some wrapper to create platform device assuming that DWC3
> core is initialized and forced in HOST role?
> 

Yes. We can enable "xhci-snps-quirks" device property for that.

Thanks,
Thinh

Re: [RFC PATCH 1/4] dt-bindings: usb: usb-xhci: Add xhci-snps-quirks

[Rob Herring]

On Fri, Jun 03, 2022 at 07:48:08PM -0700, Thinh Nguyen wrote:
> Set this property to use xhci-snps extension to handle common Synopsys
> DWC_usb3x host quirks.

I don't see why this needs to be in DT.

The DWC3 stuff is a mess of quirks which doesn't work well. Quirk 
properties in DT require either knowing the quirk up front (You don't 
always) or updating the DT on a platform when you find one. Quirks 
should be implied by the compatible string(s) instead.

> 
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> ---
>  Documentation/devicetree/bindings/usb/usb-xhci.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.yaml b/Documentation/devicetree/bindings/usb/usb-xhci.yaml
> index 965f87fef702..540044a087a7 100644
> --- a/Documentation/devicetree/bindings/usb/usb-xhci.yaml
> +++ b/Documentation/devicetree/bindings/usb/usb-xhci.yaml
> @@ -29,6 +29,10 @@ properties:
>      description: Interrupt moderation interval
>      default: 5000
>  
> +  xhci-snps-quirks:
> +    description: Handles common Synopsys DWC_usb3x host quirks
> +    type: boolean
> +
>  additionalProperties: true
>  
>  examples:
> -- 
> 2.28.0
> 
> 

Re: [RFC PATCH 1/4] dt-bindings: usb: usb-xhci: Add xhci-snps-quirks

[Thinh Nguyen]

Hi Rob,

Rob Herring wrote:
> On Fri, Jun 03, 2022 at 07:48:08PM -0700, Thinh Nguyen wrote:
>> Set this property to use xhci-snps extension to handle common Synopsys
>> DWC_usb3x host quirks.
> 
> I don't see why this needs to be in DT.
> 
> The DWC3 stuff is a mess of quirks which doesn't work well. Quirk 
> properties in DT require either knowing the quirk up front (You don't 
> always) or updating the DT on a platform when you find one. Quirks 
> should be implied by the compatible string(s) instead.
> 

Since different vendors share the same Synopsys DWC_usb3x IPs, the
controller's behavior is predictable based on the IP versions. Just
using the compatible strings will become unmanageable when we have the
common behavior across different vendors.

Can we rename this property to "xhci-snps-DWC_usb3x-ip" or something
similar?

The main goal for this device property is to indicate that it's
Synopsys's DWC_usb3x IP. As long as we know this, the xhci-snps glue
extension can handle the fine tuning for the controller's behavior.

We could use compatible string for this goal also, but that would mean
the host devices that go through the dwc3 driver path may not have the
compatible string. (e.g. host on pci bus but get recreated as platform
device). Then we would need a different way to determine that. We could
match the parent device driver for "dwc3", but that implementation looks
fragile.

So, will the device property "xhci-snps-DWC_usb3x-ip" work for you?

Thanks,
Thinh

>>
>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>> ---
>>  Documentation/devicetree/bindings/usb/usb-xhci.yaml | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.yaml b/Documentation/devicetree/bindings/usb/usb-xhci.yaml
>> index 965f87fef702..540044a087a7 100644
>> --- a/Documentation/devicetree/bindings/usb/usb-xhci.yaml
>> +++ b/Documentation/devicetree/bindings/usb/usb-xhci.yaml
>> @@ -29,6 +29,10 @@ properties:
>>      description: Interrupt moderation interval
>>      default: 5000
>>  
>> +  xhci-snps-quirks:
>> +    description: Handles common Synopsys DWC_usb3x host quirks
>> +    type: boolean
>> +
>>  additionalProperties: true
>>  
>>  examples:
>> -- 
>> 2.28.0
>>
>>

Re: [RFC PATCH 1/4] dt-bindings: usb: usb-xhci: Add xhci-snps-quirks

[Rob Herring]

On Thu, Jun 09, 2022 at 06:11:51PM +0000, Thinh Nguyen wrote:
> Hi Rob,
> 
> Rob Herring wrote:
> > On Fri, Jun 03, 2022 at 07:48:08PM -0700, Thinh Nguyen wrote:
> >> Set this property to use xhci-snps extension to handle common Synopsys
> >> DWC_usb3x host quirks.
> > 
> > I don't see why this needs to be in DT.
> > 
> > The DWC3 stuff is a mess of quirks which doesn't work well. Quirk 
> > properties in DT require either knowing the quirk up front (You don't 
> > always) or updating the DT on a platform when you find one. Quirks 
> > should be implied by the compatible string(s) instead.
> > 
> 
> Since different vendors share the same Synopsys DWC_usb3x IPs, the
> controller's behavior is predictable based on the IP versions. 

Not really, have you looked at the existing bindings. How does the same 
IP have different numbers of clocks, resets, etc.? It's a huge mess for 
these licensed IPs and partly because they don't have publicly available 
specs where we can check what resources there really are.

> Just
> using the compatible strings will become unmanageable when we have the
> common behavior across different vendors.

This IP is not special. We use compatible everywhere else and nowhere is 
it unmanageable. And again, that's the only way you can handle quirks in 
the OS without changing the DT.

> Can we rename this property to "xhci-snps-DWC_usb3x-ip" or something
> similar?
> 
> The main goal for this device property is to indicate that it's
> Synopsys's DWC_usb3x IP. As long as we know this, the xhci-snps glue
> extension can handle the fine tuning for the controller's behavior.

Can't you just check the parent device compatible is 'snps,dwc3'? (I'm 
assuming the struct device hierarchy is glue device -> dwc3 -> xhci.)

> We could use compatible string for this goal also, but that would mean
> the host devices that go through the dwc3 driver path may not have the
> compatible string. (e.g. host on pci bus but get recreated as platform
> device). Then we would need a different way to determine that. We could
> match the parent device driver for "dwc3", but that implementation looks
> fragile.
> 
> So, will the device property "xhci-snps-DWC_usb3x-ip" work for you?

Looking at the driver, you are just creating a property within the 
kernel. It's never in DT, so why are you documenting it? You can do 
whatever DWC3 and XHCI maintainers will allow and as DT maintainer I 
don't care. I don't really think it is a great approach, but ...

Rob

Re: [RFC PATCH 1/4] dt-bindings: usb: usb-xhci: Add xhci-snps-quirks

[Thinh Nguyen]

Rob Herring wrote:
> On Thu, Jun 09, 2022 at 06:11:51PM +0000, Thinh Nguyen wrote:
>> Hi Rob,
>>
>> Rob Herring wrote:
>>> On Fri, Jun 03, 2022 at 07:48:08PM -0700, Thinh Nguyen wrote:
>>>> Set this property to use xhci-snps extension to handle common Synopsys
>>>> DWC_usb3x host quirks.
>>>
>>> I don't see why this needs to be in DT.
>>>
>>> The DWC3 stuff is a mess of quirks which doesn't work well. Quirk 
>>> properties in DT require either knowing the quirk up front (You don't 
>>> always) or updating the DT on a platform when you find one. Quirks 
>>> should be implied by the compatible string(s) instead.
>>>
>>
>> Since different vendors share the same Synopsys DWC_usb3x IPs, the
>> controller's behavior is predictable based on the IP versions. 
> 
> Not really, have you looked at the existing bindings. How does the same 
> IP have different numbers of clocks, resets, etc.? It's a huge mess for 
> these licensed IPs and partly because they don't have publicly available 
> specs where we can check what resources there really are.

I'm not claiming every platform has the same setup. I'm talking about
the controller's common behavior base on IP versions.

> 
>> Just
>> using the compatible strings will become unmanageable when we have the
>> common behavior across different vendors.
> 
> This IP is not special. We use compatible everywhere else and nowhere is 
> it unmanageable. And again, that's the only way you can handle quirks in 
> the OS without changing the DT.

Try removing all the version checks in dwc3 driver and replace them with
compatibility strings. Not only will it explode the driver, now we'd
somehow need to notify all the vendors to add their compatibility string
where it can/should be a simple version check.

> 
>> Can we rename this property to "xhci-snps-DWC_usb3x-ip" or something
>> similar?
>>
>> The main goal for this device property is to indicate that it's
>> Synopsys's DWC_usb3x IP. As long as we know this, the xhci-snps glue
>> extension can handle the fine tuning for the controller's behavior.
> 
> Can't you just check the parent device compatible is 'snps,dwc3'? (I'm 
> assuming the struct device hierarchy is glue device -> dwc3 -> xhci.)

Some dwc3 host devices also exist as pci devices, there's no compatible
string for that. As noted below, if the xhci pci device goes through the
dwc3 code path, then the dwc3 driver will do some initialization and
wrap the device as a xhci platform device before passing it to the xhci
platform glue driver. But since it's a pci device underneath, there's no
compatibility string.

> 
>> We could use compatible string for this goal also, but that would mean
>> the host devices that go through the dwc3 driver path may not have the
>> compatible string. (e.g. host on pci bus but get recreated as platform
>> device). Then we would need a different way to determine that. We could
>> match the parent device driver for "dwc3", but that implementation looks
>> fragile.
>>
>> So, will the device property "xhci-snps-DWC_usb3x-ip" work for you?
> 
> Looking at the driver, you are just creating a property within the 
> kernel. It's never in DT, so why are you documenting it? You can do 
> whatever DWC3 and XHCI maintainers will allow and as DT maintainer I 
> don't care. I don't really think it is a great approach, but ...
> 

I figure that we should document any new property should we use it...

I just want to note that this RFC series is to garner feedbacks. In no
way that I want to force any one approach. So, your feedback is highly
appreciated. Since we're introducing a new glue extension, if possible,
it'd be great if we can start off with a proper approach and build on
it. If it's not a good approach, let's help come up with one that can
work for _all_ device types.

Thanks,
Thinh