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

[Sriranjani P]

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@xxxxxxxxxx]
> Sent: 15 August 2023 01:10
> To: Sriranjani P <sriranjani.p@xxxxxxxxxxx>; davem@xxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx;
> robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
> conor+dt@xxxxxxxxxx; richardcochran@xxxxxxxxx;
> alexandre.torgue@xxxxxxxxxxx; joabreu@xxxxxxxxxxxx;
> mcoquelin.stm32@xxxxxxxxx; alim.akhtar@xxxxxxxxxxx; linux-
> fsd@xxxxxxxxx; pankaj.dubey@xxxxxxxxxxx; swathi.ks@xxxxxxxxxxx;
> ravi.patel@xxxxxxxxxxx
> Cc: netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-samsung-soc@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3 1/4] dt-bindings: net: Add FSD EQoS device tree
> bindings
> 
> On 14/08/2023 13:25, 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@xxxxxxxxxxx>
> > Signed-off-by: Ravi Patel <ravi.patel@xxxxxxxxxxx>
> > Signed-off-by: Swathi K S <swathi.ks@xxxxxxxxxxx>
> > Signed-off-by: Sriranjani P <sriranjani.p@xxxxxxxxxxx>
> > ---
> >  .../devicetree/bindings/net/snps,dwmac.yaml   |   5 +-
> >  .../devicetree/bindings/net/tesla,ethqos.yaml | 114
> > ++++++++++++++++++
> >  2 files changed, 117 insertions(+), 2 deletions(-)  create mode
> > 100644 Documentation/devicetree/bindings/net/tesla,ethqos.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > index ddf9522a5dc2..0ced7901e644 100644
> > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > @@ -96,6 +96,7 @@ properties:
> >          - snps,dwxgmac
> >          - snps,dwxgmac-2.10
> >          - starfive,jh7110-dwmac
> > +        - tesla,fsd-ethqos-4.21
> 
> I don't think one given SoC - and I was told FSD is strictly defined one specific
> SoC - can have different versions of the same block, so drop the block
> versioning.
> 

Ok, will remove versioning.

> >
> >    reg:
> >      minItems: 1
> > @@ -117,7 +118,7 @@ properties:
> >
> >    clocks:
> >      minItems: 1
> > -    maxItems: 8
> > +    maxItems: 10
> >      additionalItems: true
> >      items:
> >        - description: GMAC main clock
> > @@ -129,7 +130,7 @@ properties:
> >
> >    clock-names:
> >      minItems: 1
> > -    maxItems: 8
> > +    maxItems: 10
> >      additionalItems: true
> >      contains:
> >        enum:
> > diff --git a/Documentation/devicetree/bindings/net/tesla,ethqos.yaml
> > b/Documentation/devicetree/bindings/net/tesla,ethqos.yaml
> > new file mode 100644
> > index 000000000000..b78829246364
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/tesla,ethqos.yaml
> > @@ -0,0 +1,114 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause %YAML 1.2
> > +---
> > +$id:
> > +https://protect2.fireeye.com/v1/url?k=0d5f9085-6cd485b3-0d5e1bca-
> 74fe
> > +485cbff1-9835d59b137d73e5&q=1&e=93f28da2-6d86-4cc2-a07a-
> 9be1380616cc&
> >
> +u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fnet%2Ftesla%2Cethqos.
> yaml%2
> > +3
> > +$schema:
> > +https://protect2.fireeye.com/v1/url?k=67d3522a-0658471c-67d2d965-
> 74fe
> > +485cbff1-b9570dbbedf33f81&q=1&e=93f28da2-6d86-4cc2-a07a-
> 9be1380616cc&
> > +u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
> > +
> > +title: FSD Ethernet Quality of Service
> > +
> > +maintainers:
> > +  - Sriranjani P <sriranjani.p@xxxxxxxxxxx>
> > +  - Swathi K S <swathi.ks@xxxxxxxxxxx>
> > +
> > +description:
> > +  dwmmac based tesla ethernet devices which support Gigabit
> > +  ethernet.
> > +
> > +allOf:
> > +  - $ref: snps,dwmac.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: tesla,fsd-ethqos-4.21.yaml
> 
> ?

Will fix this to tesla,fsd-ethqos.yaml 

> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    minItems: 5
> 
> Why? I expect it to be specific.

Sorry, I could not understood this comment. In FSD we have two instances of EQoS IP, one in PERIC block, which requires total 10 clocks  to be configured and another instance exist in FSYS0 block which needs 5 clocks to be configured, so we kept minItems as 5 and maxItems as 10, but looks like latest items schema do not need maxItems entry so we will drop maxItems entry. In my understanding minItems still required so it should be kept with minimum number of clock requirements.

> 
> > +    maxItems: 10
> > +
> > +  clock-names:
> > +    minItems: 5
> > +    maxItems: 10
> > +    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
> > +
> > +  fsd-rx-clock-skew:
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    items:
> > +      - items:
> > +          - description: phandle to the syscon node
> > +          - description: offset of the control register
> > +    description:
> > +      Should be phandle/offset pair. The phandle to the syscon node.
> > +
> > +  iommus:
> > +    maxItems: 1
> > +
> > +  phy-mode:
> > +    $ref: ethernet-controller.yaml#/properties/phy-connection-type
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +  - clock-names
> > +  - rx-clock-skew
> 
> Eee? Isn't it fsd-rx-clock-skew which anyway is not correct?

Sorry, I missed to change this in DT schema before posting, I will make this to fsd-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";
> 
> Three different compatibles for the same.
> 
> No, please test your patches before sending.
> 
> I am not even checking if previous feedback was applied... Did you really go
> through it?
> 

Sorry, I missed this will take care. 

> Best regards,
> Krzysztof