Re: [PATCH v2 1/4] dt-bindings: net: Add FSD EQoS device tree bindings

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 11/01/2023 08:54, Sriranjani P wrote:
> Add FSD Ethernet compatible in Synopsys dt-bindings document. Add FSD
> Ethernet YAML schema to enable the DT validation.
> 
> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> Signed-off-by: Ravi Patel <ravi.patel@samsung.com>
> Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
> ---
>  .../devicetree/bindings/net/snps,dwmac.yaml   |   1 +
>  .../net/tesla,dwc-qos-ethernet-4.21.yaml      | 103 ++++++++++++++++++
>  2 files changed, 104 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/tesla,dwc-qos-ethernet-4.21.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> index 2f909ffe2fe8..e8d53061fd35 100644
> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> @@ -89,6 +89,7 @@ properties:
>          - snps,dwmac-5.10a
>          - snps,dwxgmac
>          - snps,dwxgmac-2.10
> +        - tesla,dwc-qos-ethernet-4.21

I don't get, why did you add the IP version number? Can Tesla FSD come
with different ones? The compatible should be specific to SoC and that's
all. Also keep consistent naming, so this is a "mac", right?

>  
>    reg:
>      minItems: 1
> diff --git a/Documentation/devicetree/bindings/net/tesla,dwc-qos-ethernet-4.21.yaml b/Documentation/devicetree/bindings/net/tesla,dwc-qos-ethernet-4.21.yaml
> new file mode 100644
> index 000000000000..d0dfc4a38d17
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/tesla,dwc-qos-ethernet-4.21.yaml
> @@ -0,0 +1,103 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/tesla,dwc-qos-ethernet-4.21.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: FSD Ethernet Quality of Service Device Tree Bindings

Drop "Device Tree Bindings"


> +
> +allOf:
> +  - $ref: "snps,dwmac.yaml#"

Drop quoets.

> +
> +maintainers:
> +  - Sriranjani P <sriranjani.p@samsung.com>
> +
> +properties:
> +  compatible:
> +    const: tesla,dwc-qos-ethernet-4.21.yaml
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 4
> +
> +  clock-names:
> +    minItems: 4

Why rx clock might be not connected?

> +    items:
> +      - const: ptp_ref
> +      - const: master_bus
> +      - const: slave_bus
> +      - const: tx
> +      - const: rx
> +      - const: master2_bus
> +      - const: slave2_bus
> +      - const: eqos_rxclk_mux
> +      - const: eqos_phyrxclk
> +      - const: dout_peric_rgmii_clk

You have here 10 clocks, but snps,dwmac allows maximum 8. You need to
update it and fix any other dependent schemas.

> +
> +  rx-clock-skew:

Does not look like generic property. Missing vendor prefix, description,
constraints.

> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +
> +  iommus:
> +    maxItems: 1
> +
> +  phy-mode:
> +    $ref: ethernet-controller.yaml#/properties/phy-connection-type
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - rx-clock-skew
> +  - iommus
> +  - phy-mode
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/fsd-clk.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    ethernet_1: ethernet@14300000 {
> +              compatible = "tesla,dwc-qos-ethernet-4.21";
> +              reg = <0x0 0x14300000 0x0 0x10000>;

Are you sure you tested the bindings? 100% sure?

> +              interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
> +              clocks =

Don't introduce fake blank lines.

> +                       <&clock_peric PERIC_EQOS_TOP_IPCLKPORT_CLK_PTP_REF_I>,
> +                       <&clock_peric PERIC_EQOS_TOP_IPCLKPORT_ACLK_I>,
> +                       <&clock_peric PERIC_EQOS_TOP_IPCLKPORT_HCLK_I>,
> +                       <&clock_peric PERIC_EQOS_TOP_IPCLKPORT_RGMII_CLK_I>,
> +                       <&clock_peric PERIC_EQOS_TOP_IPCLKPORT_CLK_RX_I>,
> +                       <&clock_peric PERIC_BUS_D_PERIC_IPCLKPORT_EQOSCLK>,
> +                       <&clock_peric PERIC_BUS_P_PERIC_IPCLKPORT_EQOSCLK>,
> +                       <&clock_peric PERIC_EQOS_PHYRXCLK_MUX>,
> +                       <&clock_peric PERIC_EQOS_PHYRXCLK>,
> +                       <&clock_peric PERIC_DOUT_RGMII_CLK>;
> +              clock-names =

Ditto

> +                            "ptp_ref",
> +                            "master_bus",

Best regards,
Krzysztof