[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

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