[v6,1/2] dt-bindings: net: Add FSD EQoS device tree bindings

Commit Message

Swathi K S Feb. 13, 2025, 4:46 a.m. UTC

Add FSD Ethernet compatible in Synopsys dt-bindings document. Add FSD
Ethernet YAML schema to enable the DT validation.

Signed-off-by: Pankaj Dubey <[e-mail]>
Signed-off-by: Ravi Patel <[e-mail]>
Signed-off-by: Swathi K S <[e-mail]>
---
 .../devicetree/bindings/net/snps,dwmac.yaml   |   5 +-
 .../bindings/net/tesla,fsd-ethqos.yaml        | 114 ++++++++++++++++++
 2 files changed, 117 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/tesla,fsd-ethqos.yaml

Comments

Krzysztof Kozlowski Feb. 13, 2025, 7:54 a.m. UTC | #1

On Thu, Feb 13, 2025 at 10:16:23AM +0530, Swathi K S wrote:
> +  clock-names:
> +    minItems: 5
> +    maxItems: 10
> +    contains:
> +      enum:
> +        - ptp_ref
> +        - master_bus
> +        - slave_bus
> +        - tx
> +        - rx
> +        - master2_bus
> +        - slave2_bus
> +        - eqos_rxclk_mux
> +        - eqos_phyrxclk
> +        - dout_peric_rgmii_clk

This does not match the previous entry. It should be strictly ordered
with minItems: 5.


> +
> +  iommus:
> +    maxItems: 1
> +
> +  phy-mode:
> +    enum:
> +      - rgmii-id
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - iommus
> +  - phy-mode
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/fsd-clk.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +        ethernet1: ethernet@14300000 {
> +            compatible = "tesla,fsd-ethqos";
> +            reg = <0x0 0x14300000 0x0 0x10000>;
> +            interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "macirq";
> +            clocks = <&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 = "ptp_ref", "master_bus", "slave_bus","tx",
> +                          "rx", "master2_bus", "slave2_bus", "eqos_rxclk_mux",
> +                          "eqos_phyrxclk","dout_peric_rgmii_clk";
> +            pinctrl-names = "default";
> +            pinctrl-0 = <&eth1_tx_clk>, <&eth1_tx_data>, <&eth1_tx_ctrl>,
> +                        <&eth1_phy_intr>, <&eth1_rx_clk>, <&eth1_rx_data>,
> +                        <&eth1_rx_ctrl>, <&eth1_mdio>;
> +            iommus = <&smmu_peric 0x0 0x1>;
> +            phy-mode = "rgmii-id";
> +       };

Misaligned/misindented.

Best regards,
Krzysztof

Swathi K S Feb. 13, 2025, 11:04 a.m. UTC | #2

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: 13 February 2025 13:24
> To: Swathi K S <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v6 1/2] dt-bindings: net: Add FSD EQoS device tree
> bindings
> 
> On Thu, Feb 13, 2025 at 10:16:23AM +0530, Swathi K S wrote:
> > +  clock-names:
> > +    minItems: 5
> > +    maxItems: 10
> > +    contains:
> > +      enum:
> > +        - ptp_ref
> > +        - master_bus
> > +        - slave_bus
> > +        - tx
> > +        - rx
> > +        - master2_bus
> > +        - slave2_bus
> > +        - eqos_rxclk_mux
> > +        - eqos_phyrxclk
> > +        - dout_peric_rgmii_clk
> 
> This does not match the previous entry. It should be strictly ordered with
> minItems: 5.

Hi Krzysztof,
Thanks for reviewing.
In FSD SoC, we have 2 instances of ethernet in two blocks.
One instance needs 5 clocks and the other needs 10 clocks.

I tried to understand this by looking at some other dt-binding files as given below, but looks like they follow similar approach
Documentation/devicetree/bindings/net/stm32-dwmac.yaml
Documentation/devicetree/bindings/net/rockchip-dwmac.yaml

Could you please guide me on how to implement this?
Also, please help me understand what is meant by 'strictly ordered'

> 
> 
> > +
> > +  iommus:
> > +    maxItems: 1
> > +
> > +  phy-mode:
> > +    enum:
> > +      - rgmii-id
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +  - clock-names
> > +  - iommus
> > +  - phy-mode
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/fsd-clk.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    soc {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +        ethernet1: ethernet@14300000 {
> > +            compatible = "tesla,fsd-ethqos";
> > +            reg = <0x0 0x14300000 0x0 0x10000>;
> > +            interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
> > +            interrupt-names = "macirq";
> > +            clocks = <&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 = "ptp_ref", "master_bus", "slave_bus","tx",
> > +                          "rx", "master2_bus", "slave2_bus", "eqos_rxclk_mux",
> > +                          "eqos_phyrxclk","dout_peric_rgmii_clk";
> > +            pinctrl-names = "default";
> > +            pinctrl-0 = <&eth1_tx_clk>, <&eth1_tx_data>, <&eth1_tx_ctrl>,
> > +                        <&eth1_phy_intr>, <&eth1_rx_clk>, <&eth1_rx_data>,
> > +                        <&eth1_rx_ctrl>, <&eth1_mdio>;
> > +            iommus = <&smmu_peric 0x0 0x1>;
> > +            phy-mode = "rgmii-id";
> > +       };
> 
> Misaligned/misindented.

Ack

> 
> Best regards,
> Krzysztof

Krzysztof Kozlowski Feb. 13, 2025, noon UTC | #3

On 13/02/2025 12:04, Swathi K S wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[e-mail]>
>> Sent: 13 February 2025 13:24
>> To: Swathi K S <[e-mail]>
>> Cc: [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]
>> Subject: Re: [PATCH v6 1/2] dt-bindings: net: Add FSD EQoS device tree
>> bindings
>>
>> On Thu, Feb 13, 2025 at 10:16:23AM +0530, Swathi K S wrote:
>>> +  clock-names:
>>> +    minItems: 5
>>> +    maxItems: 10
>>> +    contains:
>>> +      enum:
>>> +        - ptp_ref
>>> +        - master_bus
>>> +        - slave_bus
>>> +        - tx
>>> +        - rx
>>> +        - master2_bus
>>> +        - slave2_bus
>>> +        - eqos_rxclk_mux
>>> +        - eqos_phyrxclk
>>> +        - dout_peric_rgmii_clk
>>
>> This does not match the previous entry. It should be strictly ordered with
>> minItems: 5.
> 
> Hi Krzysztof,
> Thanks for reviewing.
> In FSD SoC, we have 2 instances of ethernet in two blocks.
> One instance needs 5 clocks and the other needs 10 clocks.

I understand and I do not think this is contradictory to what I asked.
If it is, then why/how?

> 
> I tried to understand this by looking at some other dt-binding files as given below, but looks like they follow similar approach
> Documentation/devicetree/bindings/net/stm32-dwmac.yaml
> Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> 
> Could you please guide me on how to implement this?
> Also, please help me understand what is meant by 'strictly ordered'

Every other 99% of bindings. Just like your clocks property.

Best regards,
Krzysztof

Andrew Lunn Feb. 14, 2025, 12:19 a.m. UTC | #4

> +  phy-mode:
> +    enum:
> +      - rgmii-id

phy-mode is normally a board property, in the .dts file, since the
board might decide to have extra long clock lines and so want
'rgmii'.

The only reason i can think of putting rgmii-id here is if the MAC
only supports 'rgmii-id', it is impossible to make it not add delays.
If that is true, a comment would be good.

	Andrew

Swathi K S Feb. 14, 2025, 4:53 a.m. UTC | #5

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: 13 February 2025 17:31
> To: Swathi K S <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v6 1/2] dt-bindings: net: Add FSD EQoS device tree
> bindings
> 
> On 13/02/2025 12:04, Swathi K S wrote:
> >
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <[e-mail]>
> >> Sent: 13 February 2025 13:24
> >> To: Swathi K S <[e-mail]>
> >> Cc: [e-mail]; [e-mail];
> [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; linux-stm32@st-md-
> mailman.stormreply.com;
> >> [e-mail]; [e-mail]
> >> Subject: Re: [PATCH v6 1/2] dt-bindings: net: Add FSD EQoS device
> >> tree bindings
> >>
> >> On Thu, Feb 13, 2025 at 10:16:23AM +0530, Swathi K S wrote:
> >>> +  clock-names:
> >>> +    minItems: 5
> >>> +    maxItems: 10
> >>> +    contains:
> >>> +      enum:
> >>> +        - ptp_ref
> >>> +        - master_bus
> >>> +        - slave_bus
> >>> +        - tx
> >>> +        - rx
> >>> +        - master2_bus
> >>> +        - slave2_bus
> >>> +        - eqos_rxclk_mux
> >>> +        - eqos_phyrxclk
> >>> +        - dout_peric_rgmii_clk
> >>
> >> This does not match the previous entry. It should be strictly ordered
> >> with
> >> minItems: 5.
> >
> > Hi Krzysztof,
> > Thanks for reviewing.
> > In FSD SoC, we have 2 instances of ethernet in two blocks.
> > One instance needs 5 clocks and the other needs 10 clocks.
> 
> I understand and I do not think this is contradictory to what I asked.
> If it is, then why/how?
> 
> >
> > I tried to understand this by looking at some other dt-binding files
> > as given below, but looks like they follow similar approach
> > Documentation/devicetree/bindings/net/stm32-dwmac.yaml
> > Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> >
> > Could you please guide me on how to implement this?
> > Also, please help me understand what is meant by 'strictly ordered'
> 
> Every other 99% of bindings. Just like your clocks property.

Hi Krzysztof,
Thanks for your feedback.
I want to make sure I fully understand your comment. 
I can see we have added clocks and clock names in the same order.
Could you please help in detail what specifically needs to be modified regarding the ordering and minItems/maxItems usage?

-Swathi

> 
> Best regards,
> Krzysztof

Swathi K S Feb. 14, 2025, 5:17 a.m. UTC | #6

> -----Original Message-----
> From: Andrew Lunn <[e-mail]>
> Sent: 14 February 2025 05:50
> To: Swathi K S <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v6 1/2] dt-bindings: net: Add FSD EQoS device tree
> bindings
> 
> > +  phy-mode:
> > +    enum:
> > +      - rgmii-id
> 
> phy-mode is normally a board property, in the .dts file, since the board
might
> decide to have extra long clock lines and so want 'rgmii'.
> 
> The only reason i can think of putting rgmii-id here is if the MAC only
> supports 'rgmii-id', it is impossible to make it not add delays.
> If that is true, a comment would be good.


Hi Andrew,
Thanks for reviewing.
I think we already discussed this part some time back here [1]
[1] :
https://patchwork.kernel.org/project/linux-arm-kernel/patch/20230814112539.7
[e-mail]/#25879995
Please do let me know if there is any other concern on this.

-Swathi

> 
> 	Andrew

Krzysztof Kozlowski Feb. 14, 2025, 7:31 a.m. UTC | #7

On 14/02/2025 05:53, Swathi K S wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[e-mail]>
>> Sent: 13 February 2025 17:31
>> To: Swathi K S <[e-mail]>
>> Cc: [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]
>> Subject: Re: [PATCH v6 1/2] dt-bindings: net: Add FSD EQoS device tree
>> bindings
>>
>> On 13/02/2025 12:04, Swathi K S wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Krzysztof Kozlowski <[e-mail]>
>>>> Sent: 13 February 2025 13:24
>>>> To: Swathi K S <[e-mail]>
>>>> Cc: [e-mail]; [e-mail];
>> [e-mail];
>>>> [e-mail]; [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; linux-stm32@st-md-
>> mailman.stormreply.com;
>>>> [e-mail]; [e-mail]
>>>> Subject: Re: [PATCH v6 1/2] dt-bindings: net: Add FSD EQoS device
>>>> tree bindings
>>>>
>>>> On Thu, Feb 13, 2025 at 10:16:23AM +0530, Swathi K S wrote:
>>>>> +  clock-names:
>>>>> +    minItems: 5
>>>>> +    maxItems: 10
>>>>> +    contains:
>>>>> +      enum:
>>>>> +        - ptp_ref
>>>>> +        - master_bus
>>>>> +        - slave_bus
>>>>> +        - tx
>>>>> +        - rx
>>>>> +        - master2_bus
>>>>> +        - slave2_bus
>>>>> +        - eqos_rxclk_mux
>>>>> +        - eqos_phyrxclk
>>>>> +        - dout_peric_rgmii_clk
>>>>
>>>> This does not match the previous entry. It should be strictly ordered
>>>> with
>>>> minItems: 5.
>>>
>>> Hi Krzysztof,
>>> Thanks for reviewing.
>>> In FSD SoC, we have 2 instances of ethernet in two blocks.
>>> One instance needs 5 clocks and the other needs 10 clocks.
>>
>> I understand and I do not think this is contradictory to what I asked.
>> If it is, then why/how?
>>
>>>
>>> I tried to understand this by looking at some other dt-binding files
>>> as given below, but looks like they follow similar approach
>>> Documentation/devicetree/bindings/net/stm32-dwmac.yaml
>>> Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
>>>
>>> Could you please guide me on how to implement this?
>>> Also, please help me understand what is meant by 'strictly ordered'
>>
>> Every other 99% of bindings. Just like your clocks property.
> 
> Hi Krzysztof,
> Thanks for your feedback.
> I want to make sure I fully understand your comment. 
> I can see we have added clocks and clock names in the same order.

No, you did not. You can see syntax is very different - one uses items,
other uses contains-enum. And now test it, change the order in DTS and
see if you see any warning.

> Could you please help in detail what specifically needs to be modified regarding the ordering and minItems/maxItems usage?
You did not try hard enough.

Open other bindings and look how they list clocks. For example any
Samsung clock controller bindings. Or any qcom bindings.

Best regards,
Krzysztof

Swathi K S Feb. 14, 2025, 9:33 a.m. UTC | #8

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: 14 February 2025 13:02
> To: Swathi K S <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v6 1/2] dt-bindings: net: Add FSD EQoS device tree
> bindings
> 
> On 14/02/2025 05:53, Swathi K S wrote:
> >
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <[e-mail]>
> >> Sent: 13 February 2025 17:31
> >> To: Swathi K S <[e-mail]>
> >> Cc: [e-mail]; [e-mail];
> [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; linux-stm32@st-md-
> mailman.stormreply.com;
> >> [e-mail]; [e-mail]
> >> Subject: Re: [PATCH v6 1/2] dt-bindings: net: Add FSD EQoS device
> >> tree bindings
> >>
> >> On 13/02/2025 12:04, Swathi K S wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Krzysztof Kozlowski <[e-mail]>
> >>>> Sent: 13 February 2025 13:24
> >>>> To: Swathi K S <[e-mail]>
> >>>> Cc: [e-mail]; [e-mail];
> >> [e-mail];
> >>>> [e-mail]; [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail];
> >>>> [e-mail]; linux-stm32@st-md-
> >> mailman.stormreply.com;
> >>>> [e-mail]; [e-mail]
> >>>> Subject: Re: [PATCH v6 1/2] dt-bindings: net: Add FSD EQoS device
> >>>> tree bindings
> >>>>
> >>>> On Thu, Feb 13, 2025 at 10:16:23AM +0530, Swathi K S wrote:
> >>>>> +  clock-names:
> >>>>> +    minItems: 5
> >>>>> +    maxItems: 10
> >>>>> +    contains:
> >>>>> +      enum:
> >>>>> +        - ptp_ref
> >>>>> +        - master_bus
> >>>>> +        - slave_bus
> >>>>> +        - tx
> >>>>> +        - rx
> >>>>> +        - master2_bus
> >>>>> +        - slave2_bus
> >>>>> +        - eqos_rxclk_mux
> >>>>> +        - eqos_phyrxclk
> >>>>> +        - dout_peric_rgmii_clk
> >>>>
> >>>> This does not match the previous entry. It should be strictly
> >>>> ordered with
> >>>> minItems: 5.
> >>>
> >>> Hi Krzysztof,
> >>> Thanks for reviewing.
> >>> In FSD SoC, we have 2 instances of ethernet in two blocks.
> >>> One instance needs 5 clocks and the other needs 10 clocks.
> >>
> >> I understand and I do not think this is contradictory to what I asked.
> >> If it is, then why/how?
> >>
> >>>
> >>> I tried to understand this by looking at some other dt-binding files
> >>> as given below, but looks like they follow similar approach
> >>> Documentation/devicetree/bindings/net/stm32-dwmac.yaml
> >>> Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> >>>
> >>> Could you please guide me on how to implement this?
> >>> Also, please help me understand what is meant by 'strictly ordered'
> >>
> >> Every other 99% of bindings. Just like your clocks property.
> >
> > Hi Krzysztof,
> > Thanks for your feedback.
> > I want to make sure I fully understand your comment.
> > I can see we have added clocks and clock names in the same order.
> 
> No, you did not. You can see syntax is very different - one uses items, other
> uses contains-enum. And now test it, change the order in DTS and see if you
> see any warning.
> 
> > Could you please help in detail what specifically needs to be modified
> regarding the ordering and minItems/maxItems usage?
> You did not try hard enough.
> 
> Open other bindings and look how they list clocks. For example any Samsung
> clock controller bindings. Or any qcom bindings.

Thanks for your insight. I tried understanding the dt-binding implementations from Samsung/ qcom as you suggested.
I am thinking of making the following modification in clock-names:

clock-names:
     minItems: 5
     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
		 
Does this align with your feedback or do you suggest any further changes?

-Swathi

> 
> Best regards,
> Krzysztof

Krzysztof Kozlowski Feb. 14, 2025, 10:55 a.m. UTC | #9

On 14/02/2025 10:33, Swathi K S wrote:
>>> Could you please help in detail what specifically needs to be modified
>> regarding the ordering and minItems/maxItems usage?
>> You did not try hard enough.
>>
>> Open other bindings and look how they list clocks. For example any Samsung
>> clock controller bindings. Or any qcom bindings.
> 
> Thanks for your insight. I tried understanding the dt-binding implementations from Samsung/ qcom as you suggested.
> I am thinking of making the following modification in clock-names:
> 
> clock-names:
>      minItems: 5
>      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
> 		 
> Does this align with your feedback or do you suggest any further changes?

Yes.

Best regards,
Krzysztof

Andrew Lunn Feb. 14, 2025, 1:10 p.m. UTC | #10

On Fri, Feb 14, 2025 at 10:47:39AM +0530, Swathi K S wrote:
> 
> 
> > -----Original Message-----
> > From: Andrew Lunn <[e-mail]>
> > Sent: 14 February 2025 05:50
> > To: Swathi K S <[e-mail]>
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]
> > Subject: Re: [PATCH v6 1/2] dt-bindings: net: Add FSD EQoS device tree
> > bindings
> > 
> > > +  phy-mode:
> > > +    enum:
> > > +      - rgmii-id
> > 
> > phy-mode is normally a board property, in the .dts file, since the board
> might
> > decide to have extra long clock lines and so want 'rgmii'.
> > 
> > The only reason i can think of putting rgmii-id here is if the MAC only
> > supports 'rgmii-id', it is impossible to make it not add delays.
> > If that is true, a comment would be good.
> 
> 
> Hi Andrew,
> Thanks for reviewing.
> I think we already discussed this part some time back here [1]
> [1] :
> https://patchwork.kernel.org/project/linux-arm-kernel/patch/20230814112539.7
> [e-mail]/#25879995
> Please do let me know if there is any other concern on this.

We partially discussed this in this thread.

As i said, what value you need here depends on the board design. The
PCB could provide the 2ns delay, in which case, 'rgmii' would be the
correct value to have in the board .dts file. Hence the binding should
not restrict the value of phy-mode to just rgmii-id. All 4 rmgii
values should be accepted.

The only reason you would force only rgmii-id is if the MAC/PHY pair
cannot do anything else. If that really is true, i would expect a
comment in the binding, and the MAC driver to return -EINVAL for
anything but rgmii-id.

	Andrew

Swathi K S Feb. 17, 2025, 5:19 a.m. UTC | #11

> -----Original Message-----
> From: Andrew Lunn <[e-mail]>
> Sent: 14 February 2025 18:40
> To: Swathi K S <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
'Pankaj
> Dubey' <[e-mail]>
> Subject: Re: [PATCH v6 1/2] dt-bindings: net: Add FSD EQoS device tree
> bindings
> 
> On Fri, Feb 14, 2025 at 10:47:39AM +0530, Swathi K S wrote:
> >
> >
> > > -----Original Message-----
> > > From: Andrew Lunn <[e-mail]>
> > > Sent: 14 February 2025 05:50
> > > To: Swathi K S <[e-mail]>
> > > Cc: [e-mail]; [e-mail];
> [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail];
> > > [e-mail];
> > > [e-mail]; [e-mail]
> > > Subject: Re: [PATCH v6 1/2] dt-bindings: net: Add FSD EQoS device
> > > tree bindings
> > >
> > > > +  phy-mode:
> > > > +    enum:
> > > > +      - rgmii-id
> > >
> > > phy-mode is normally a board property, in the .dts file, since the
> > > board
> > might
> > > decide to have extra long clock lines and so want 'rgmii'.
> > >
> > > The only reason i can think of putting rgmii-id here is if the MAC
> > > only supports 'rgmii-id', it is impossible to make it not add delays.
> > > If that is true, a comment would be good.
> >
> >
> > Hi Andrew,
> > Thanks for reviewing.
> > I think we already discussed this part some time back here [1] [1] :
> > https://patchwork.kernel.org/project/linux-arm-kernel/patch/2023081411
> > 2539.7
> > [e-mail]/#25879995
> > Please do let me know if there is any other concern on this.
> 
> We partially discussed this in this thread.
> 
> As i said, what value you need here depends on the board design. The PCB
> could provide the 2ns delay, in which case, 'rgmii' would be the correct
value
> to have in the board .dts file. Hence the binding should not restrict the
value
> of phy-mode to just rgmii-id. All 4 rmgii values should be accepted.
> 
> The only reason you would force only rgmii-id is if the MAC/PHY pair
cannot
> do anything else. If that really is true, i would expect a comment in the
> binding, and the MAC driver to return -EINVAL for anything but rgmii-id.

Hi Andrew, 
What you said is right. Generally, PCB provides internal delay.
But in this case, due to customer request, the delay was added into SoC.

The following doc on rgmii says that "Devices which implement internal delay
shall be referred to as RGMII-ID. 
Devices may offer an option to operate with/without internal delay and still
remain compliant with this spec"
https://community.nxp.com/pwmxy87654/attachments/pwmxy87654/imx-processors/2
0655/1/RGMIIv2_0_final_hp.pdf

Also, the driver is in such a way that it handles all four rgmii in the same
way. 

Considering this, could you let us know what will be the right approach to
take in this case?

-Swathi

> 
> 	Andrew

Andrew Lunn Feb. 17, 2025, 3:18 p.m. UTC | #12

> > > > > +  phy-mode:
> > > > > +    enum:
> > > > > +      - rgmii-id
> > > >
> > > > phy-mode is normally a board property, in the .dts file, since the
> > > > board
> > > might
> > > > decide to have extra long clock lines and so want 'rgmii'.

> Hi Andrew, 
> What you said is right. Generally, PCB provides internal delay.

It is actually pretty unusual for the PCB to add the delays. But there
are some boards which do this. Which is why you should support it.

> But in this case, due to customer request, the delay was added into SoC.

Idealy, by the PHY. However, in terms of DT, the board .dts file just
needs to say 'rmgii-id', meaning that the board does not provide the
delays, so the MAC/PHY pair needs to provide the delay.

> The following doc on rgmii says that "Devices which implement internal delay
> shall be referred to as RGMII-ID. 
> Devices may offer an option to operate with/without internal delay and still
> remain compliant with this spec"
> https://community.nxp.com/pwmxy87654/attachments/pwmxy87654/imx-processors/2
> 0655/1/RGMIIv2_0_final_hp.pdf
> 
> Also, the driver is in such a way that it handles all four rgmii in the same
> way. 
> 
> Considering this, could you let us know what will be the right approach to
> take in this case?

List all four phy-modes in the binding. They should all be
valid. However, the .dtsi file should not list a phy-mode, since it is
a board property, not a SoC property.

	Andrew

Swathi K S Feb. 18, 2025, 3:55 a.m. UTC | #13

> -----Original Message-----
> From: Andrew Lunn <[e-mail]>
> Sent: 17 February 2025 20:48
> To: Swathi K S <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
'Pankaj
> Dubey' <[e-mail]>; [e-mail]
> Subject: Re: [PATCH v6 1/2] dt-bindings: net: Add FSD EQoS device tree
> bindings
> 
> > > > > > +  phy-mode:
> > > > > > +    enum:
> > > > > > +      - rgmii-id
> > > > >
> > > > > phy-mode is normally a board property, in the .dts file, since
> > > > > the board
> > > > might
> > > > > decide to have extra long clock lines and so want 'rgmii'.
> 
> > Hi Andrew,
> > What you said is right. Generally, PCB provides internal delay.
> 
> It is actually pretty unusual for the PCB to add the delays. But there are
some
> boards which do this. Which is why you should support it.
> 
> > But in this case, due to customer request, the delay was added into SoC.
> 
> Idealy, by the PHY. However, in terms of DT, the board .dts file just
needs to
> say 'rmgii-id', meaning that the board does not provide the delays, so the
> MAC/PHY pair needs to provide the delay.
> 
> > The following doc on rgmii says that "Devices which implement internal
> > delay shall be referred to as RGMII-ID.
> > Devices may offer an option to operate with/without internal delay and
> > still remain compliant with this spec"
> >
> https://community.nxp.com/pwmxy87654/attachments/pwmxy87654/imx-
> proces
> > sors/2
> > 0655/1/RGMIIv2_0_final_hp.pdf
> >
> > Also, the driver is in such a way that it handles all four rgmii in
> > the same way.
> >
> > Considering this, could you let us know what will be the right
> > approach to take in this case?
> 
> List all four phy-modes in the binding. They should all be valid. However,
the
> .dtsi file should not list a phy-mode, since it is a board property, not a
SoC
> property.

Hi Andrew, 
Thanks for the clarification.
Will post v7 with the following updates:

1. Changing phy-mode in dt-binding as shown below:
  phy-mode:
    enum:
      - rgmii
      - rgmii-id
      - rgmii-rxid
      - rgmii-txid
	  
2. Removing phy-mode from .dtsi and example given in dt-binding
3. Add phy-mode to .dts file and specify 'rgmii-id' there.

Hope this will address your review comment.

Thanks,
Swathi

> 
> 	Andrew

Andrew Lunn Feb. 18, 2025, 3:33 p.m. UTC | #14

> Hi Andrew, 
> Thanks for the clarification.
> Will post v7 with the following updates:
> 
> 1. Changing phy-mode in dt-binding as shown below:
>   phy-mode:
>     enum:
>       - rgmii
>       - rgmii-id
>       - rgmii-rxid
>       - rgmii-txid
> 	  
> 2. Removing phy-mode from .dtsi and example given in dt-binding

The example can use phy-mode, since the example is the combination of
the .dtsi and the .dts parts of the device tree. And having the
example using 'rgmii-id' will hopefully prevent some people wrongly
using 'rgmii'

> 3. Add phy-mode to .dts file and specify 'rgmii-id' there.

Great. History shows if you get the example and the first user
correct, everybody blindly copies it and gets it right by accident. If
the first user is wrong, everybody blindly copies it, and get is wrong
by not sanity checking what they copy.

	Andrew

Patch

diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
index 91e75eb3f329..c7004eaa8eae 100644
--- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
@@ -102,6 +102,7 @@  properties:
         - snps,dwxgmac-2.10
         - starfive,jh7100-dwmac
         - starfive,jh7110-dwmac
+        - tesla,fsd-ethqos
         - thead,th1520-gmac
 
   reg:
@@ -126,7 +127,7 @@  properties:
 
   clocks:
     minItems: 1
-    maxItems: 8
+    maxItems: 10
     additionalItems: true
     items:
       - description: GMAC main clock
@@ -138,7 +139,7 @@  properties:
 
   clock-names:
     minItems: 1
-    maxItems: 8
+    maxItems: 10
     additionalItems: true
     contains:
       enum:
diff --git a/Documentation/devicetree/bindings/net/tesla,fsd-ethqos.yaml b/Documentation/devicetree/bindings/net/tesla,fsd-ethqos.yaml
new file mode 100644
index 000000000000..9d7d632fcd92
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/tesla,fsd-ethqos.yaml
@@ -0,0 +1,114 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/tesla,fsd-ethqos.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: FSD Ethernet Quality of Service
+
+maintainers:
+  - Swathi K S <[e-mail]>
+
+description:
+  Tesla ethernet devices based on dwmmac support Gigabit ethernet.
+
+allOf:
+  - $ref: snps,dwmac.yaml#
+
+properties:
+  compatible:
+    const: tesla,fsd-ethqos
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-names:
+    items:
+      - const: macirq
+
+  clocks:
+    minItems: 5
+    items:
+      - description: PTP clock
+      - description: Master bus clock
+      - description: Slave bus clock
+      - description: MAC TX clock
+      - description: MAC RX clock
+      - description: Master2 bus clock
+      - description: Slave2 bus clock
+      - description: RX MUX clock
+      - description: PHY RX clock
+      - description: PERIC RGMII clock
+
+  clock-names:
+    minItems: 5
+    maxItems: 10
+    contains:
+      enum:
+        - ptp_ref
+        - master_bus
+        - slave_bus
+        - tx
+        - rx
+        - master2_bus
+        - slave2_bus
+        - eqos_rxclk_mux
+        - eqos_phyrxclk
+        - dout_peric_rgmii_clk
+
+  iommus:
+    maxItems: 1
+
+  phy-mode:
+    enum:
+      - rgmii-id
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - iommus
+  - phy-mode
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/fsd-clk.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+        ethernet1: ethernet@14300000 {
+            compatible = "tesla,fsd-ethqos";
+            reg = <0x0 0x14300000 0x0 0x10000>;
+            interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "macirq";
+            clocks = <&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 = "ptp_ref", "master_bus", "slave_bus","tx",
+                          "rx", "master2_bus", "slave2_bus", "eqos_rxclk_mux",
+                          "eqos_phyrxclk","dout_peric_rgmii_clk";
+            pinctrl-names = "default";
+            pinctrl-0 = <&eth1_tx_clk>, <&eth1_tx_data>, <&eth1_tx_ctrl>,
+                        <&eth1_phy_intr>, <&eth1_rx_clk>, <&eth1_rx_data>,
+                        <&eth1_rx_ctrl>, <&eth1_mdio>;
+            iommus = <&smmu_peric 0x0 0x1>;
+            phy-mode = "rgmii-id";
+       };
+    };
+
+...