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

[Rob Herring <robh@kernel.org>]

On Mon, Jul 24, 2023 at 02:36:58PM +0800, 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>
> ---
>  .../bindings/usb/intel,socfpga-dwc3.yaml      | 84 +++++++++++++++++++
>  1 file changed, 84 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..e36b087c2651
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> @@ -0,0 +1,84 @@
> +# 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:
> +    items:
> +      - enum:
> +          - intel,agilex5-dwc3
> +      - const: intel,socfpga-dwc3
> +
> +  reg:
> +    description: Offset and length of DWC3 controller register
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: Controller Master/Core clock
> +      - description: Controller Suspend clock
> +
> +  ranges: true
> +
> +  resets:
> +    description: A list of phandles for resets listed in reset-names
> +    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#

One node, no wrapper node and dwc3 child node please unless you have 
actual registers for the wrapper. Based on the example having the same 
register addresses in both nodes, you don't need the wrapper.

> +
> +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>
> +
> +    usb@11000000 {
> +          compatible = "intel,agilex5-dwc3", "intel,socfpga-dwc3";
> +          reg = <0x11000000 0x100000>;

You really have 1MB worth of registers? That chews up 1MB of 
kernel virtual space. Not a big deal for 64-bit, but it is a problem on 
32-bit systems. Define the length to just what you need.

> +          ranges;
> +          clocks = <&clkmgr 54>,
> +                   <&clkmgr 55>;
> +          resets = <&rst USB0_RESET>, <&rst USB1_RESET>;
> +          reset-names = "dwc3", "dwc3-ecc";
> +          #address-cells = <1>;
> +          #size-cells = <1>;

BTW, this implies that you can only DMA 32-bit bits. Not sure if the 
dwc3 supports more. 

> +
> +          usb@11000000 {
> +                compatible = "snps,dwc3";
> +                reg = <0x11000000 0x100000>;
> +                interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
> +                dr_mode = "host";
> +          };
> +    };
> +
> -- 
> 2.26.2
>