Message-ID: <27cd48ed-1a05-4619-a8c6-ddc07163bdd6@cherry.de>
Date: Wed, 16 Apr 2025 11:14:57 +0200
From: Quentin Schulz <quentin.schulz@...rry.de>
To: Rob Herring <robh@...nel.org>, Quentin Schulz <foss+kernel@...il.net>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Łukasz Czechowski
 <lukasz.czechowski@...umatec.com>, linux-usb@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] dt-bindings: usb: usb-device: allow multiple compatibles

Hi Rob,

On 4/15/25 9:01 PM, Rob Herring wrote:
> On Tue, Apr 15, 2025 at 04:34:27PM +0200, Quentin Schulz wrote:
>> From: Quentin Schulz <quentin.schulz@...rry.de>
>>
>> The dt-core typically allows multiple compatibles[1] but usb-device
>> currently forces a single compatible.
>>
>> This is an issue when multiple devices with slightly different productID
>> all behave the same. This would require the driver to keep updating its
>> compatible matching table and the bindings to include this new productID
>> instead of doing what is usually done: have two compatibles, the
>> leftmost which matches exactly the HW device definition, and the
>> rightmost one as a fallback which is assumed to be 100% compatible with
>> the device at hand. If this assumption turns out to be wrong, it is easy
>> to work around this without having to modify the device tree by handling
>> the leftmost compatible in the driver.
>>
>> [1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/dt-core.yaml#L21-L25
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@...rry.de>
>> ---
>> This came up while working on fixing USB on an RK3399 Puma which has an
>> onboard USB hub whose productID isn't in any driver compatible list
>> but which can be supported by a driver with a slightly different
>> productID matching another variant of the same IC, from the same
>> datasheet.
>>
>> See https://lore.kernel.org/linux-rockchip/20250326-onboard_usb_dev-v1-0-a4b0a5d1b32c@thaumatec.com/
>> ---
>>   Documentation/devicetree/bindings/usb/usb-device.yaml | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/usb-device.yaml b/Documentation/devicetree/bindings/usb/usb-device.yaml
>> index c676956810331b81f11f3624340fc3e612c98315..9d55be4fb5981164cca969dbda5ba70ab3a87773 100644
>> --- a/Documentation/devicetree/bindings/usb/usb-device.yaml
>> +++ b/Documentation/devicetree/bindings/usb/usb-device.yaml
>> @@ -28,7 +28,8 @@ description: |
>>   
>>   properties:
>>     compatible:
>> -    pattern: "^usb[0-9a-f]{1,4},[0-9a-f]{1,4}$"
>> +    items:
>> +      pattern: "^usb[0-9a-f]{1,4},[0-9a-f]{1,4}$"
> 
> I would use 'contains' here rather than 'items'. That's even more
> relaxed in allowing "normal" compatible strings, but is aligned with
> what we have for PCI device.
> 

Thanks for the suggestion, makes sense.

Now, I'm wondering how to handle that on the actual device binding?

For example I tried the following:

diff --git a/Documentation/devicetree/bindings/usb/usb-device.yaml 
b/Documentation/devicetree/bindings/usb/usb-device.yaml
index 09fceb469f105..20a6c021ebdba 100644
--- a/Documentation/devicetree/bindings/usb/usb-device.yaml
+++ b/Documentation/devicetree/bindings/usb/usb-device.yaml
@@ -124,3 +124,15 @@ examples:
              };
          };
      };
+  - |
+    usb@...70000 {
+        reg = <0x11270000 0x1000>;
+        interrupts = <0x0 0x4e 0x0>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        hub@1 {
+            compatible = "usb5e3,609", "usb5e3,608";
+            reg = <1>;
+        };
+    };


And I got:

/home/qschulz/work/upstream/linux/build/puma/Documentation/devicetree/bindings/usb/usb-device.example.dtb: 
hub@1: compatible:0: 'usb5e3,609' is not one of ['usb5e3,608', 
'usb5e3,610', 'usb5e3,620', 'usb5e3,626']
	from schema $id: http://devicetree.org/schemas/usb/genesys,gl850g.yaml#

Fair enough, this means the DT binding currently always needs all 
compatibles to be defined.

Then I modified usb5e3,609 to be usb5e3,610, and I got:

/home/qschulz/work/upstream/linux/build/puma/Documentation/devicetree/bindings/usb/usb-device.example.dtb: 
hub@1: compatible: ['usb5e3,610', 'usb5e3,608'] is too long
	from schema $id: http://devicetree.org/schemas/usb/genesys,gl850g.yaml#

So it seems like we need the driver DT binding to handle multiple 
compatibles too. I needed to do:

diff --git a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml 
b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
index 6fe2d356dcbde..e8e5f78356334 100644
--- a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
+++ b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
@@ -11,11 +11,12 @@ maintainers:

  properties:
    compatible:
-    enum:
-      - usb5e3,608
-      - usb5e3,610
-      - usb5e3,620
-      - usb5e3,626
+    items:
+      enum:
+        - usb5e3,608
+        - usb5e3,610
+        - usb5e3,620
+        - usb5e3,626

    reg: true

for it to pass the dt_binding_check. I assume we do not want to use 
contains: in the event that the leftmost compatible is handled by one dt 
binding and the rightmost by another one, in which case they would both 
match and apply their own requirements/constraints?

In short, is it correct that when we want to add a 
"just-in-case-we-discover-quirks-later"-compatible to a device tree, we 
anyway want it explicitly listed in the dt binding matching the 
rightmost compatible?

I'm basically trying to understand what we want/need to do for the 
cypress,hx3 binding in RK3399 Puma series: 
https://lore.kernel.org/linux-rockchip/20250326-onboard_usb_dev-v1-0-a4b0a5d1b32c@thaumatec.com/ 
to be able to use a fallback compatible in the driver and still pass the 
dt checks. (Considering we will want to backport the patches to stable 
releases too).

Cheers,
Quentin

Powered by blists - more mailing lists