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.
We have carefully reviewed your comments and made the corresponding changes. Could you please help us evaluate whether the updates we mentioned in our previous email properly address your concerns and meet the expected standards?
At the same time, we still have some questions that need clarification. We have included these in the original email — we would appreciate it if you could also take a moment to review those points.


@Krzysztof Kozlowski
We noticed your review comment on the following page, but we did not receive it via email:
🔗 https://lore.kernel.org/lkml/f096afa1-260e-4f8c-8595-3b41425b2964@kernel.org/
Thank you for your professional feedback on our Ethernet driver.
Regarding the issue you raised:
"There is no such property. I already said at v2 you cannot have undocumented ABI."
Upon rechecking, we realized that during verification, we mistakenly used underscore-separated property names in the driver, while dash-separated names were used in the YAML bindings. We have now synchronized the property naming.
Could you please confirm if this mismatch was the root cause of your concern?


Best regards,

Li Zhi
Eswin Computing

> -----原始邮件-----
> 发件人: "Andrew Lunn" <andrew@lunn.ch>
> 发送时间:2025-07-04 00:12:29 (星期五)
> 收件人: 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, lizhi2@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?
> 

In response to your suggestion, we added the following more detailed comments to the code. Is this appropriate?
+/*
+ * Default delay register values for different signals:
+ *
+ * EIC7700_DELAY_VALUE0: Used for TXD and RXD signals delay configuration.
+ * Bits layout:
+ *   Byte 0 (bits 0-7)   : TXD0 / RXD0 delay (0x20 = 3.2 ns)
+ *   Byte 1 (bits 8-15)  : TXD1 / RXD1 delay (0x20 = 3.2 ns)
+ *   Byte 2 (bits 16-23) : TXD2 / RXD2 delay (0x20 = 3.2 ns)
+ *   Byte 3 (bits 24-31) : TXD3 / RXD3 delay (0x20 = 3.2 ns)
+ *
+ * EIC7700_DELAY_VALUE1: Used for control signals delay configuration.
+ * Bits layout:
+ *   Bits 0-6     : TXEN delay
+ *   Bits 8-14    : TXCLK delay
+ *   Bit  15      : TXCLK invert (1 = invert)
+ *   Bits 16-22   : RXDV delay
+ *   Bits 24-30   : RXCLK delay
+ *   Bit  31      : RXCLK invert (1 = invert)
+ */
+#define EIC7700_DELAY_VALUE0 0x20202020
+#define EIC7700_DELAY_VALUE1 0x96205A20

> > +	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?

In response to your question, we have added the following more detailed comments to the code. Is this appropriate?
+        /* Initialize default delay parameters for 1000Mbps and 100Mbps speeds */
+        dwc_priv->dly_param_1000m[0] = EIC7700_DELAY_VALUE0; /* TXD delay */
+        dwc_priv->dly_param_1000m[1] = EIC7700_DELAY_VALUE1; /* Control signals delay */
+        dwc_priv->dly_param_1000m[2] = EIC7700_DELAY_VALUE0; /* RXD delay */
+        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;
+        /* For 10Mbps, no delay by default */
+        dwc_priv->dly_param_10m[0] = 0x0;
+        dwc_priv->dly_param_10m[1] = 0x0;
+        dwc_priv->dly_param_10m[2] = 0x0;

> 
> > +
> > +	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.
> 

Regarding your question, we have added default values ​​for tx delay and rx delay in the code, and as long as one of the two delays is configured in DTS, the original configuration can be overwritten. Does this process meet your suggestion?
+        /* Default delays in picoseconds */
+        dwc_priv->rx_delay_ps = 0;
+        dwc_priv->tx_delay_ps = 0;
+
+        if (has_rx_dly || has_tx_dly) {

> {
> > +		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.
> 

Our original strategy was to use the value used when initializing dly_param_*m as the default value, so should we continue to follow your suggestion and use 0ps as the default value?

> 	Andrew