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

[李志]

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 at lunn.ch>
> 发送时间:2025-07-04 00:12:29 (星期五)
> 收件人: weishangjuan at eswincomputing.com
> 抄送: andrew+netdev at lunn.ch, davem at davemloft.net, edumazet at google.com, kuba at kernel.org, robh at kernel.org, krzk+dt at kernel.org, conor+dt at kernel.org, netdev at vger.kernel.org, devicetree at vger.kernel.org, linux-kernel at vger.kernel.org, mcoquelin.stm32 at gmail.com, alexandre.torgue at foss.st.com, rmk+kernel at armlinux.org.uk, yong.liang.choong at linux.intel.com, vladimir.oltean at nxp.com, jszhang at kernel.org, jan.petrous at oss.nxp.com, prabhakar.mahadev-lad.rj at bp.renesas.com, inochiama at gmail.com, boon.khai.ng at altera.com, dfustini at tenstorrent.com, 0x1207 at gmail.com, linux-stm32 at st-md-mailman.stormreply.com, linux-arm-kernel at lists.infradead.org, ningyu at eswincomputing.com, linmin at eswincomputing.com, lizhi2 at eswincomputing.com
> 主题: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver
> 
> > +/* Default delay value*/
> > +#define EIC7700_DELAY_VALUE0 0x20202020
> > +#define EIC7700_DELAY_VALUE1 0x96205A20
> 
> We need a better explanation of what is going on here. What do these
> numbers mean?
> 

Let me clarify:
  EIC7700_DELAY_VALUE0 (0x20202020) is used to configure delay taps for TXD[3:0] signals. Each byte represents the delay value for one data line.
  EIC7700_DELAY_VALUE1 (0x96205A20) configures control signal delays, such as TX_EN, RX_DV, and others. Again, each byte corresponds to a specific signal line.
More detailed inline comments will be added in the next patch to explain the bit layout and purpose of each byte in these default values. Is this understanding correct?

> > +	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
These values are defined separately for different link speeds: 1000 Mbps, 100 Mbps, and 10 Mbps. During PHY initialization or when the link speed changes, the corresponding delay parameters are selected and applied to the hardware registers.
Inline comments will be added in the next patch to clarify the meaning and usage of each element. Is this understanding correct?

> > +
> > +	ret = of_property_read_u32(pdev->dev.of_node, "rx-internal-delay-ps",
> > +				   &dwc_priv->rx_delay_ps);
> > +	if (ret)
> > +		dev_dbg(&pdev->dev, "can't get rx-internal-delay-ps, ret(%d).", ret);
> > +	else
> > +		has_rx_dly = true;
> > +
> > +	ret = of_property_read_u32(pdev->dev.of_node, "tx-internal-delay-ps",
> > +				   &dwc_priv->tx_delay_ps);
> > +	if (ret)
> > +		dev_dbg(&pdev->dev, "can't get tx-internal-delay-ps, ret(%d).", ret);
> > +	else
> > +		has_tx_dly = true;
> > +	if (has_rx_dly && has_tx_dly)
> 
> What if i only to set a TX delay? I want the RX delay to default to
> 0ps.
> 

Yes, this can be handled separately by calling eic7700_set_rgmii_rx_dly() and eic7700_set_rgmii_tx_dly() in the next patch. Is this correct?

> {
> > +		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, which is used in the absence of the DTS attribute. The print message will be removed in the next patch. Is this correct?

> 	Andrew