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

[Swathi K S]

Hi Krzysztof, 

Sorry for the delay in response.
Starting now, I will be taking over this task.
I have gone through your comments and feedback and will be implementing them in v4 of this patch.

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@xxxxxxxxxx]
> Sent: 16 August 2023 11:48
> 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 16/08/2023 07:58, Sriranjani P wrote:
> >>> +
> >>> +allOf:
> >>> +  - $ref: snps,dwmac.yaml#
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: tesla,fsd-ethqos-4.21.yaml
> >>
> >> ?
> >
> > Will fix this to tesla,fsd-ethqos.yaml
> 
> Test your patches before sending. REALLY TEST.

Sure. Will fix this to tesla,fsd-ethqos.yaml and test the same.

> 
> >
> >>> +
> >>> +  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.
> 
> No, the code is fine then.
> 
> >
> >>
> >>> +    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.
> 
> Remember about vendor prefixes for every custom property.

Sure, will fix this in v4.

> 
> 
> Best regards,
> Krzysztof

Regards, 
Swathi