usb: xhci: Introduce xhci-snps
Subsystems (4)
Thinh Nguyen
maintainercover usb: xhci: Introduce xhci-snps
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
Signature · 1 line
2.28.0
Thinh Nguyen
maintainerRFC PATCH 1/4 dt-bindings: usb: usb-xhci: Add xhci-snps-quirks
Set this property to use xhci-snps extension to handle common Synopsys DWC_usb3x host quirks.
--- Documentation/devicetree/bindings/usb/usb-xhci.yaml | 4 ++++ 1 file changed, 4 insertions(+)
Documentation/devicetree/bindings/usb/usb-xhci.yaml +4 -0
@@ -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:
Signature · 1 line
2.28.0
Rob Herring
maintainerRe: [RFC PATCH 1/4] dt-bindings: usb: usb-xhci: Add xhci-snps-quirks
Quoted 2 lines · Thinh Nguyen
Set this property to use xhci-snps extension to handle common Synopsys
DWC_usb3xhost 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.
Quoted 25 lines
Signed-off-by Thinh Nguyen <[email protected]>--- Documentation/devicetree/bindings/usb/usb-xhci.yaml | 4 ++++ 1 file changed, 4 insertions(+)Documentation/devicetree/bindings/usb/usb-xhci.yaml +4
-0@@ -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:Signature · 1 line
2.28.0
Thinh Nguyen
maintainerRe: [RFC PATCH 1/4] dt-bindings: usb: usb-xhci: Add xhci-snps-quirks
Hi Rob,
Quoted 11 lines · Rob Herring
Quoted 2 lines · Thinh Nguyen
Set this property to use xhci-snps extension to handle common Synopsys
DWC_usb3xhost 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
Quoted 25 lines
Quoted 25 lines
Signed-off-by Thinh Nguyen <[email protected]>--- Documentation/devicetree/bindings/usb/usb-xhci.yaml | 4 ++++ 1 file changed, 4 insertions(+)Documentation/devicetree/bindings/usb/usb-xhci.yaml +4
-0@@ -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:Signature · 1 line
2.28.0
Rob Herring
maintainerRe: [RFC PATCH 1/4] dt-bindings: usb: usb-xhci: Add xhci-snps-quirks
Quoted 9 lines · Thinh Nguyen
Hi Rob,
Quoted 6 lines · Rob Herring
Quoted 2 lines · Thinh Nguyen
Set this property to use xhci-snps extension to handle common Synopsys
DWC_usb3xhost quirks.I don't see why this needs to be in DT.
Quoted 8 lines
Quoted 5 lines
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_usb3xIPs, 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.
Quoted 3 lines
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.
Quoted 6 lines
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_usb3xIP. 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.)
Quoted 8 lines
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
Thinh Nguyen
maintainerRe: [RFC PATCH 1/4] dt-bindings: usb: usb-xhci: Add xhci-snps-quirks
Quoted 16 lines · Rob Herring
Quoted 15 lines · Thinh Nguyen
Hi Rob,
Quoted 11 lines · Rob Herring
Quoted 2 lines · Thinh Nguyen
Set this property to use xhci-snps extension to handle common Synopsys
DWC_usb3xhost 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.
Quoted 7 lines
Quoted 2 lines
Since different vendors share the same Synopsys
DWC_usb3xIPs, 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.
Quoted 8 lines
Quoted 4 lines
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.
Quoted 10 lines
Quoted 7 lines
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_usb3xIP. 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.
Quoted 8 lines
Quoted 8 lines
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.
Quoted 7 lines
Quoted 1 line
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
Thinh Nguyen
maintainerRFC PATCH 2/4 usb: dwc3: host: Always set xhci-snps-quirks
In reply to Thinh NguyenAlways 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.
--- drivers/usb/dwc3/host.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/usb/dwc3/host.c +3 -1
@@ -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" );
Signature · 1 line
2.28.0
Thinh Nguyen
maintainerRFC PATCH 3/4 usb: dwc3: core: Share global register access with xhci driver
In reply to Thinh NguyenWhen 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.
--- drivers/usb/dwc3/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/usb/dwc3/core.c +2 -2
@@ -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);
Signature · 1 line
2.28.0
Thinh Nguyen
maintainerRFC PATCH 4/4 usb: xhci: Introduce Synopsys glue extension for DWC_usb3x
In reply to Thinh NguyenSynopsys 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.
--- 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
drivers/usb/host/Kconfig +8 -0
@@ -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)
drivers/usb/host/Makefile +3 -0
@@ -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
drivers/usb/host/xhci-plat.c +40 -0
@@ -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); }
drivers/usb/host/xhci-plat.h +3 -0
@@ -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)
drivers/usb/host/xhci-snps.c new file +185 -0
185 lines not on this page · raw message
drivers/usb/host/xhci-snps.h new file +32 -0
@@ -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 */
Signature · 1 line
2.28.0
Re: [RFC PATCH 4/4] usb: xhci: Introduce Synopsys glue extension for DWC_usb3x
Hi Thinh,
Quoted 29 lines · Thinh Nguyen
Synopsys
DWC_usb3xIPs 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_usb3xhosts. For this particular change, we'll start with xhci-plat glue driver.Signed-off-by Thinh Nguyen <[email protected]>--- 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.hdrivers/usb/host/Kconfig +0
-0@@ -62,6 +62,14 @@ config USB_XHCI_PLATFORM
drivers/usb/host/Kconfig config USB_XHCI_PLATFORM · line 72
Quoted 10 lines
drivers/usb/host/Kconfig +8
-0If 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
Thinh Nguyen
maintainerRe: [RFC PATCH 4/4] usb: xhci: Introduce Synopsys glue extension for DWC_usb3x
Quoted 34 lines · Pavan Kondeti
Hi Thinh,
Quoted 31 lines · Thinh Nguyen
Synopsys
DWC_usb3xIPs 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_usb3xhosts. For this particular change, we'll start with xhci-plat glue driver.Signed-off-by Thinh Nguyen <[email protected]>--- 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.hdrivers/usb/host/Kconfig +0
-0@@ -62,6 +62,14 @@ config USB_XHCI_PLATFORM If unsure, say N.
Quoted 13 lines
drivers/usb/host/Kconfig config USB_XHCI_PLATFORM · line 72
Quoted 8 lines
drivers/usb/host/Kconfig +8
-0+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