Received-SPF: pass (google.com: domain of linux-kernel+bounces-204023-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: "Swathi K S" <swathi.ks@samsung.com>
To: "'Andrew Lunn'" <andrew@lunn.ch>
Cc: <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
	<pabeni@redhat.com>, <robh+dt@kernel.org>,
	<krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
	<richardcochran@gmail.com>, <alexandre.torgue@foss.st.com>,
	<joabreu@synopsys.com>, <mcoquelin.stm32@gmail.com>,
	<alim.akhtar@samsung.com>, <linux-fsd@tesla.com>,
	<pankaj.dubey@samsung.com>, <ravi.patel@samsung.com>,
	<netdev@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <linux-samsung-soc@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>
In-Reply-To: <4e745c2a-57bd-45da-8bd2-ee1cb2bab84f@lunn.ch>
Subject: RE: [PATCH v3 1/4] dt-bindings: net: Add FSD EQoS device tree
 bindings
Date: Thu, 6 Jun 2024 14:44:40 +0530
Message-ID: <000201dab7f2$1c8d4580$55a7d080$@samsung.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Thread-Index: AQG0kE2cByMDcfrFjxkR49X5VWx9JAKGcKNgAXd78/0C+uayQ7HDw7+g
Content-Language: en-in
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 105P
DLP-Filter: Pass
References: <20230814112539.70453-1-sriranjani.p@samsung.com>
	<CGME20230814112605epcas5p31aca7b23e70e8d93df11414291f7ce66@epcas5p3.samsung.com>
	<20230814112539.70453-2-sriranjani.p@samsung.com>
	<4e745c2a-57bd-45da-8bd2-ee1cb2bab84f@lunn.ch>

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@lunn.ch]
> Sent: 15 August 2023 02:10
> To: Sriranjani P <sriranjani.p@samsung.com>
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
> richardcochran@gmail.com; alexandre.torgue@foss.st.com;
> joabreu@synopsys.com; mcoquelin.stm32@gmail.com;
> alim.akhtar@samsung.com; linux-fsd@tesla.com;
> pankaj.dubey@samsung.com; swathi.ks@samsung.com;
> ravi.patel@samsung.com; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-samsung-
> soc@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> 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