[1/2] dt-bindings: usb: Add Intel SoCFPGA USB controller bindings

Commit Message

Ng, Adrian Ho Yin July 21, 2023, 4:30 a.m. UTC

From: Adrian Ng Ho Yin <[e-mail]>

Existing binding intel,keembay-dwc3.yaml does not have the required
properties for Intel SoCFPGA devices.
Introduce new binding description for Intel SoCFPGA USB controller
which will be used for current and future SoCFPGA devices.

Signed-off-by: Adrian Ng Ho Yin <[e-mail]>
---
 .../bindings/usb/intel,socfpga-dwc3.yaml      | 78 +++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml

Comments

Krzysztof Kozlowski July 21, 2023, 8:04 a.m. UTC | #1

On 21/07/2023 06:30, [e-mail] wrote:
> From: Adrian Ng Ho Yin <[e-mail]>
> 
> Existing binding intel,keembay-dwc3.yaml does not have the required
> properties for Intel SoCFPGA devices.
> Introduce new binding description for Intel SoCFPGA USB controller
> which will be used for current and future SoCFPGA devices.
> 
> Signed-off-by: Adrian Ng Ho Yin <[e-mail]>

A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.

> ---
>  .../bindings/usb/intel,socfpga-dwc3.yaml      | 78 +++++++++++++++++++
>  1 file changed, 78 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml

Filename matching compatible.

> 
> diff --git a/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> new file mode 100644
> index 000000000000..dedef70df887
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> @@ -0,0 +1,78 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/intel,socfpga-dwc3.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Intel SoCFPGA DWC3 USB controller
> +
> +maintainers:
> +  - Adrian Ng Ho Yin <[e-mail]>
> +
> +properties:
> +  compatible:
> +    const: intel,agilex5-dwc3

Why using compatible style different than other Agilex blocks? Which one
is recommended/official/correct?

> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 2

What are the items?

> +
> +  ranges: true
> +
> +  resets:
> +    maxItems: 2
> +
> +  reset-names:
> +    items:
> +      - const: dwc3
> +      - const: dwc3-ecc
> +
> +  '#address-cells':
> +    enum: [ 1, 2 ]
> +
> +  '#size-cells':
> +    enum: [ 1, 2 ]
> +
> +# Required child node:
> +
> +patternProperties:
> +  "^usb@[0-9a-f]+$":
> +    $ref: snps,dwc3.yaml#
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - resets
> +  - ranges
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/reset/altr,rst-mgr.h>
> +    #define AGILEX5_USB31_SUSPEND_CLK
> +    #define AGILEX5_USB31_BUS_CLK_EARLY

Drop defines. Include proper header or use some numbers, if the headers
are not there yet.

> +
> +    usb1@11000000 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +          compatible = "intel,agilex5-dwc3";
Best regards,
Krzysztof

Ng, Adrian Ho Yin July 24, 2023, 6:35 a.m. UTC | #2

> On 21/07/2023 06:30, [e-mail] wrote:
> > From: Adrian Ng Ho Yin <[e-mail]>
> >
> > Existing binding intel,keembay-dwc3.yaml does not have the required 
> > properties for Intel SoCFPGA devices.
> > Introduce new binding description for Intel SoCFPGA USB controller 
> > which will be used for current and future SoCFPGA devices.
> >
> > Signed-off-by: Adrian Ng Ho Yin <[e-mail]>
> 
> A nit, subject: drop second/last, redundant "bindings". The 
> "dt-bindings" prefix is already stating that these are bindings.

Dropped "bindings" in V2 patch.

> 
> > ---
> >  .../bindings/usb/intel,socfpga-dwc3.yaml      | 78 +++++++++++++++++++
> >  1 file changed, 78 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> 
> Filename matching compatible.

Update compatible to match file name in V2 patch.

> 
> >
> > diff --git
> > a/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> > b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> > new file mode 100644
> > index 000000000000..dedef70df887
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> > @@ -0,0 +1,78 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/usb/intel,socfpga-dwc3.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Intel SoCFPGA DWC3 USB controller
> > +
> > +maintainers:
> > +  - Adrian Ng Ho Yin <[e-mail]>
> > +
> > +properties:
> > +  compatible:
> > +    const: intel,agilex5-dwc3
> 
> Why using compatible style different than other Agilex blocks? Which 
> one is recommended/official/correct?

This style is used so that future SoCFPGA products can reuse the same binding. 

> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 2
> 
> What are the items?
> 

Update with items description and dropped maxItems  for clocks in V2 patch.

> > +
> > +  ranges: true
> > +
> > +  resets:
> > +    maxItems: 2
> > +
> > +  reset-names:
> > +    items:
> > +      - const: dwc3
> > +      - const: dwc3-ecc
> > +
> > +  '#address-cells':
> > +    enum: [ 1, 2 ]
> > +
> > +  '#size-cells':
> > +    enum: [ 1, 2 ]
> > +
> > +# Required child node:
> > +
> > +patternProperties:
> > +  "^usb@[0-9a-f]+$":
> > +    $ref: snps,dwc3.yaml#
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - resets
> > +  - ranges
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/reset/altr,rst-mgr.h>
> > +    #define AGILEX5_USB31_SUSPEND_CLK
> > +    #define AGILEX5_USB31_BUS_CLK_EARLY
> 
> Drop defines. Include proper header or use some numbers, if the 
> headers are not there yet.

Removed defines and update to use numbers. 

> 
> > +
> > +    usb1@11000000 {
> 
> Node names should be generic. See also an explanation and list of 
> examples (not
> exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-dev
> icetree- basics.html#generic-names-recommendation
> 
> 
> > +          compatible = "intel,agilex5-dwc3";

Update node name to be generic in V2 patch.

Thank You
Adrian Ng

Krzysztof Kozlowski July 24, 2023, 7:01 a.m. UTC | #3

On 24/07/2023 08:35, Ng, Adrian Ho Yin wrote:
>>> diff --git
>>> a/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
>>> b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
>>> new file mode 100644
>>> index 000000000000..dedef70df887
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
>>> @@ -0,0 +1,78 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/usb/intel,socfpga-dwc3.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Intel SoCFPGA DWC3 USB controller
>>> +
>>> +maintainers:
>>> +  - Adrian Ng Ho Yin <[e-mail]>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: intel,agilex5-dwc3
>>
>> Why using compatible style different than other Agilex blocks? Which 
>> one is recommended/official/correct?
> 
> This style is used so that future SoCFPGA products can reuse the same binding. 

Wait, why? How future products are relevant to style of compatible for
existing device? Again - why using different style? Which one is correct?


Best regards,
Krzysztof

Patch

diff --git a/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
new file mode 100644
index 000000000000..dedef70df887
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
@@ -0,0 +1,78 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/intel,socfpga-dwc3.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Intel SoCFPGA DWC3 USB controller
+
+maintainers:
+  - Adrian Ng Ho Yin <[e-mail]>
+
+properties:
+  compatible:
+    const: intel,agilex5-dwc3
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 2
+
+  ranges: true
+
+  resets:
+    maxItems: 2
+
+  reset-names:
+    items:
+      - const: dwc3
+      - const: dwc3-ecc
+
+  '#address-cells':
+    enum: [ 1, 2 ]
+
+  '#size-cells':
+    enum: [ 1, 2 ]
+
+# Required child node:
+
+patternProperties:
+  "^usb@[0-9a-f]+$":
+    $ref: snps,dwc3.yaml#
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - resets
+  - ranges
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/reset/altr,rst-mgr.h>
+    #define AGILEX5_USB31_SUSPEND_CLK
+    #define AGILEX5_USB31_BUS_CLK_EARLY
+
+    usb1@11000000 {
+          compatible = "intel,agilex5-dwc3";
+          reg = <0x11000000 0x100000>;
+          ranges;
+          clocks = <&clkmgr AGILEX5_USB31_SUSPEND_CLK>,
+                   <&clkmgr AGILEX5_USB31_BUS_CLK_EARLY>;
+          resets = <&rst USB0_RESET>, <&rst USB1_RESET>;
+          reset-names = "dwc3", "dwc3-ecc";
+          #address-cells = <1>;
+          #size-cells = <1>;
+
+          usb@11000000 {
+                compatible = "snps,dwc3";
+                reg = <0x11000000 0x100000>;
+                interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
+                dr_mode = "host";
+          };
+    };