Re: Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver

[李志 <lizhi2@eswincomputing.com>]

Dear Andrew Lunn,
Thank you for your professional and valuable suggestions.
Our questions are embedded below your comments in the original email below.


Best regards,

Li Zhi
Eswin Computing


> -----原始邮件-----
> 发件人: "Andrew Lunn" <andrew@lunn.ch>
> 发送时间:2025-07-15 21:09:17 (星期二)
> 收件人: 李志 <lizhi2@eswincomputing.com>
> 抄送: weishangjuan@eswincomputing.com, andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, netdev@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, mcoquelin.stm32@gmail.com, alexandre.torgue@foss.st.com, rmk+kernel@armlinux.org.uk, yong.liang.choong@linux.intel.com, vladimir.oltean@nxp.com, jszhang@kernel.org, jan.petrous@oss.nxp.com, prabhakar.mahadev-lad.rj@bp.renesas.com, inochiama@gmail.com, boon.khai.ng@altera.com, dfustini@tenstorrent.com, 0x1207@gmail.com, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, ningyu@eswincomputing.com, linmin@eswincomputing.com, pinkesh.vaghela@einfochips.com
> 主题: Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver
> 
> > > > +	dwc_priv->dly_param_1000m[0] = EIC7700_DELAY_VALUE0;
> > > > +	dwc_priv->dly_param_1000m[1] = EIC7700_DELAY_VALUE1;
> > > > +	dwc_priv->dly_param_1000m[2] = EIC7700_DELAY_VALUE0;
> > > > +	dwc_priv->dly_param_100m[0] = EIC7700_DELAY_VALUE0;
> > > > +	dwc_priv->dly_param_100m[1] = EIC7700_DELAY_VALUE1;
> > > > +	dwc_priv->dly_param_100m[2] = EIC7700_DELAY_VALUE0;
> > > > +	dwc_priv->dly_param_10m[0] = 0x0;
> > > > +	dwc_priv->dly_param_10m[1] = 0x0;
> > > > +	dwc_priv->dly_param_10m[2] = 0x0;
> > > 
> > > What are the three different values for?
> > > 
> > 
> > Let me clarify the purpose of the three elements in each dly_param_* array:
> >   dly_param_[x][0]: Delay configuration for TXD signals
> >   dly_param_[x][1]: Delay configuration for control signals (e.g., TX_EN, RX_DV, RX_CLK)
> >   dly_param_[x][2]: Delay configuration for RXD signals
> 
> Maybe add a #define or an enum for the index.
> 
> Do these delays represent the RGMII 2ns delay?
> 

Yes, these delays refer to the RGMII delay, but they are not strictly 2ns. There are a few points that require further clarification:
1. Regarding delay configuration logic:
   As you mentioned in version V2, rx-internal-delay-ps and tx-internal-delay-ps will be mapped to and overwrite the corresponding bits in the EIC7700_DELAY_VALUE1 register, which controls the rx_clk and tx_clk delays. Is this understanding and approach correct and feasible?
2. About the phy-mode setting:
   In our platform, the internal delays are provided by the MAC. When configuring rx-internal-delay-ps and tx-internal-delay-ps in the device tree, is it appropriate to set phy-mode = "rgmii-id" in this case?
3. Delay values being greater than 2ns:
   In our platform, the optimal delay values for rx_clk and tx_clk are determined based on the board-level timing adjustment, and both are greater than 2ns. Given this, is it reasonable and compliant with the RGMII specification to set both rx-internal-delay-ps and tx-internal-delay-ps to values greater than 2ns in the Device Tree?

> > > {
> > > > +		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
> > > > +				  &dwc_priv->dly_param_1000m[1]);
> > > > +		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
> > > > +				  &dwc_priv->dly_param_100m[1]);
> > > > +		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
> > > > +				  &dwc_priv->dly_param_10m[1]);
> > > > +	} else {
> > > > +		dev_dbg(&pdev->dev, " use default dly\n");
> > > 
> > > What is the default? It should be 0ps. So there is no point printing
> > > this message.
> > > 
> > 
> > The default value is EIC7700_DELAY_VALUE1
> 
> But what does EIC7700_DELAY_VALUE1 mean? It should mean 0ps? But i'm
> not sure it does.
> 

There is a question that needs clarification:
The EIC7700_DELAY_VALUE0 and EIC7700_DELAY_VALUE1 registers contain the optimal delay configurations determined through board-level phase adjustment. Therefore, they are also used as the default values in our platform. If the default delay is set to 0ps, the Ethernet interface may fail to function correctly in our platform.

> 	Andrew