[PATCH 0/2] Add support for Intel SocFPGA DWC3 USB controller

[PATCH 0/2] Add support for Intel SocFPGA DWC3 USB controller

[adrian.ho.yin.ng]

From: Adrian Ng Ho Yin <adrian.ho.yin.ng@intel.com>

This patch set adds binding for Intel SoCFPGA DWC3 controller.
The new binding differs from the existing binding intel,keembay-dwc3.yaml
as the required properties are different.
This patch also adds compatible string for Intel Agilex5 in DWC3 simple
glue driver.

Adrian Ng Ho Yin (2):
  dt-bindings: usb: Add Intel SoCFPGA USB controller bindings
  usb: dwc3: of-simple: Add compatible string for Intel Agilex5 platform

 .../bindings/usb/intel,socfpga-dwc3.yaml      | 78 +++++++++++++++++++
 drivers/usb/dwc3/dwc3-of-simple.c             |  1 +
 2 files changed, 79 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml

-- 
2.26.2

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

[adrian.ho.yin.ng]

From: Adrian Ng Ho Yin <adrian.ho.yin.ng@intel.com>

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 <adrian.ho.yin.ng@intel.com>
---
 .../bindings/usb/intel,socfpga-dwc3.yaml      | 78 +++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml

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 <adrian.ho.yin.ng@intel.com>
+
+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";
+          };
+    };
-- 
2.26.2

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

[Krzysztof Kozlowski]

On 21/07/2023 06:30, adrian.ho.yin.ng@intel.com wrote:
> From: Adrian Ng Ho Yin <adrian.ho.yin.ng@intel.com>
> 
> 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 <adrian.ho.yin.ng@intel.com>

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 <adrian.ho.yin.ng@intel.com>
> +
> +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

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

[Ng, Adrian Ho Yin]

> On 21/07/2023 06:30, adrian.ho.yin.ng@intel.com wrote:
> > From: Adrian Ng Ho Yin <adrian.ho.yin.ng@intel.com>
> >
> > 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 <adrian.ho.yin.ng@intel.com>
> 
> 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 <adrian.ho.yin.ng@intel.com>
> > +
> > +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

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

[Krzysztof Kozlowski]

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 <adrian.ho.yin.ng@intel.com>
>>> +
>>> +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 2/2] usb: dwc3: of-simple: Add compatible string for Intel Agilex5 platform

[adrian.ho.yin.ng]

From: Adrian Ng Ho Yin <adrian.ho.yin.ng@intel.com>

Add compatible string to use this generic glue layer to support
Intel Agilex5 dwc3 controller.

Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@intel.com>
---
 drivers/usb/dwc3/dwc3-of-simple.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
index 7e6ad8fe61a5..3c4640a7e663 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -175,6 +175,7 @@ static const struct of_device_id of_dwc3_simple_match[] = {
 	{ .compatible = "allwinner,sun50i-h6-dwc3" },
 	{ .compatible = "hisilicon,hi3670-dwc3" },
 	{ .compatible = "intel,keembay-dwc3" },
+	{ .compatible = "intel,agilex5-dwc3" },
 	{ /* Sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, of_dwc3_simple_match);
-- 
2.26.2