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

["Swathi K S" <swathi.ks@xxxxxxxxxxx>]

Hi Andrew, 

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: Andrew Lunn [mailto:andrew@xxxxxxx]
> Sent: 15 August 2023 02:10
> To: Sriranjani P <sriranjani.p@xxxxxxxxxxx>
> Cc: 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; 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
> 
> > +  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.
> 
> What clock are you skew-ing here? And why?

As per customer's requirement, we need 2ns delay in fsys block both in TX
and RX path.

> 
> > +    ethernet_1: ethernet@14300000 {
> > +              compatible = "tesla,dwc-qos-ethernet-4.21";
> > +              reg = <0x0 0x14300000 0x0 0x10000>;
> > +              interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
> > +              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>;
> > +              fsd-rx-clock-skew = <&sysreg_peric 0x10>;
> > +              iommus = <&smmu_peric 0x0 0x1>;
> > +              phy-mode = "rgmii";
> 
> I know it is just an example, but "rgmii" is generally wrong. "rgmii-id"
is
> generally what you need. And when i do see "rgmii", it starts ringing
alarm
> bells for me, it could mean your RGMII delays are being handled wrongly.

Thanks for bringing this to our notice. Will correct this in v4 as rgmii-id.

> 
>        Andrew

Regards,
Swathi