From: Shangjuan Wei <weishangjuan@eswincomputing.com>

Add Ethernet controller support for Eswin's eic7700 SoC. The driver
provides management and control of Ethernet signals for the eiC7700
series chips.

Signed-off-by: Zhi Li <lizhi2@eswincomputing.com>
Signed-off-by: Shangjuan Wei <weishangjuan@eswincomputing.com>
---
 drivers/net/ethernet/stmicro/stmmac/Kconfig   |  11 +
 drivers/net/ethernet/stmicro/stmmac/Makefile  |   1 +
 .../ethernet/stmicro/stmmac/dwmac-eic7700.c   | 257 ++++++++++++++++++
 3 files changed, 269 insertions(+)
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c

diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index 67fa879b1e52..a13b15ce1abd 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -67,6 +67,17 @@ config DWMAC_ANARION

 	  This selects the Anarion SoC glue layer support for the stmmac driver.

+config DWMAC_EIC7700
+	tristate "Support for Eswin eic7700 ethernet driver"
+	select CRC32
+	select MII
+	depends on OF && HAS_DMA && ARCH_ESWIN || COMPILE_TEST
+	help
+	  This driver supports the Eswin EIC7700 Ethernet controller,
+	  which integrates Synopsys DesignWare QoS features. It enables
+	  high-speed networking with DMA acceleration and is optimized
+	  for embedded systems.
+
 config DWMAC_INGENIC
 	tristate "Ingenic MAC support"
 	default MACH_INGENIC
diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
index b591d93f8503..f4ec5fc16571 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Makefile
+++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
@@ -14,6 +14,7 @@ stmmac-$(CONFIG_STMMAC_SELFTESTS) += stmmac_selftests.o
 # Ordering matters. Generic driver must be last.
 obj-$(CONFIG_STMMAC_PLATFORM)	+= stmmac-platform.o
 obj-$(CONFIG_DWMAC_ANARION)	+= dwmac-anarion.o
+obj-$(CONFIG_DWMAC_EIC7700)	+= dwmac-eic7700.o
 obj-$(CONFIG_DWMAC_INGENIC)	+= dwmac-ingenic.o
 obj-$(CONFIG_DWMAC_IPQ806X)	+= dwmac-ipq806x.o
 obj-$(CONFIG_DWMAC_LPC18XX)	+= dwmac-lpc18xx.o
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c
new file mode 100644
index 000000000000..000362e9987d
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c
@@ -0,0 +1,257 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Eswin DWC Ethernet linux driver
+ *
+ * Copyright 2025, Beijing ESWIN Computing Technology Co., Ltd.
+ *
+ * Authors:
+ *   Shuang Liang <liangshuang@eswincomputing.com>
+ *   Shangjuan Wei <weishangjuan@eswincomputing.com>
+ */
+
+#include <linux/platform_device.h>
+#include <linux/mfd/syscon.h>
+#include <linux/stmmac.h>
+#include <linux/regmap.h>
+#include <linux/of.h>
+
+#include "stmmac_platform.h"
+
+/* eth_phy_ctrl_offset eth0:0x100; eth1:0x200 */
+#define EIC7700_ETH_TX_CLK_SEL		BIT(16)
+#define EIC7700_ETH_PHY_INTF_SELI	BIT(0)
+
+/* eth_axi_lp_ctrl_offset eth0:0x108; eth1:0x208 */
+#define EIC7700_ETH_CSYSREQ_VAL		BIT(0)
+
+/* hsp_aclk_ctrl_offset (0x148) */
+#define EIC7700_HSP_ACLK_CLKEN		BIT(31)
+#define EIC7700_HSP_ACLK_DIVSOR		(0x2 << 4)
+
+/* hsp_cfg_ctrl_offset (0x14c) */
+#define EIC7700_HSP_CFG_CLKEN		BIT(31)
+#define EIC7700_SCU_HSP_PCLK_EN		BIT(30)
+#define EIC7700_HSP_CFG_CTRL_REGSET	(EIC7700_HSP_CFG_CLKEN | EIC7700_SCU_HSP_PCLK_EN)
+
+/* TX/RX clock delay (unit: 0.1ns per bit) */
+#define EIC7700_ETH_TX_ADJ_DELAY	GENMASK(14, 8)
+#define EIC7700_ETH_RX_ADJ_DELAY	GENMASK(30, 24)
+
+/* Default delay value*/
+#define EIC7700_DELAY_VALUE0 0x20202020
+#define EIC7700_DELAY_VALUE1 0x96205A20
+
+struct eic7700_qos_priv {
+	struct device *dev;
+	struct regmap *crg_regmap;
+	struct regmap *hsp_regmap;
+	u32 tx_delay_ps;
+	u32 rx_delay_ps;
+	u32 dly_hsp_reg[3];
+	u32 dly_param_1000m[3];
+	u32 dly_param_100m[3];
+	u32 dly_param_10m[3];
+};
+
+static inline void eic7700_set_delay(u32 rx_ps, u32 tx_ps, u32 *reg)
+{
+	u32 rx_val = rx_ps / 100;
+
+	if (rx_val > 0x7F)
+		rx_val = 0x7F;
+
+	*reg &= ~EIC7700_ETH_RX_ADJ_DELAY;
+	*reg |= (rx_val << 24) & EIC7700_ETH_RX_ADJ_DELAY;
+
+	u32 tx_val = tx_ps / 100;
+
+	if (tx_val > 0x7F)
+		tx_val = 0x7F;
+
+	*reg &= ~EIC7700_ETH_TX_ADJ_DELAY;
+	*reg |= (tx_val << 8) & EIC7700_ETH_TX_ADJ_DELAY;
+}
+
+static void eic7700_qos_fix_speed(void *priv, int speed, u32 mode)
+{
+	struct eic7700_qos_priv *dwc_priv = priv;
+	int i;
+
+	switch (speed) {
+	case SPEED_1000:
+		for (i = 0; i < 3; i++)
+			regmap_write(dwc_priv->hsp_regmap,
+				     dwc_priv->dly_hsp_reg[i],
+				     dwc_priv->dly_param_1000m[i]);
+		break;
+	case SPEED_100:
+		for (i = 0; i < 3; i++) {
+			regmap_write(dwc_priv->hsp_regmap,
+				     dwc_priv->dly_hsp_reg[i],
+				     dwc_priv->dly_param_100m[i]);
+		}
+		break;
+	case SPEED_10:
+		for (i = 0; i < 3; i++) {
+			regmap_write(dwc_priv->hsp_regmap,
+				     dwc_priv->dly_hsp_reg[i],
+				     dwc_priv->dly_param_10m[i]);
+		}
+		break;
+	default:
+		dev_err(dwc_priv->dev, "invalid speed %u\n", speed);
+		break;
+	}
+}
+
+static int eic7700_dwmac_probe(struct platform_device *pdev)
+{
+	struct plat_stmmacenet_data *plat_dat;
+	struct stmmac_resources stmmac_res;
+	struct eic7700_qos_priv *dwc_priv;
+	u32 hsp_aclk_ctrl_offset;
+	u32 hsp_aclk_ctrl_regset;
+	u32 hsp_cfg_ctrl_offset;
+	u32 eth_axi_lp_ctrl_offset;
+	u32 eth_phy_ctrl_offset;
+	u32 eth_phy_ctrl_regset;
+	bool has_rx_dly = false;
+	bool has_tx_dly = false;
+	int ret;
+
+	ret = stmmac_get_platform_resources(pdev, &stmmac_res);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				"failed to get resources\n");
+
+	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	if (IS_ERR(plat_dat))
+		return dev_err_probe(&pdev->dev, PTR_ERR(plat_dat),
+				"dt configuration failed\n");
+
+	dwc_priv = devm_kzalloc(&pdev->dev, sizeof(*dwc_priv), GFP_KERNEL);
+	if (!dwc_priv)
+		return -ENOMEM;
+
+	dwc_priv->dev = &pdev->dev;
+	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;
+
+	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) {
+		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");
+	}
+
+	ret = of_property_read_variable_u32_array(pdev->dev.of_node, "eswin,dly_hsp_reg",
+						  &dwc_priv->dly_hsp_reg[0], 3, 0);
+	if (ret != 3) {
+		dev_err(&pdev->dev, "can't get delay hsp reg.ret(%d)\n", ret);
+		return ret;
+	}
+
+	dwc_priv->crg_regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+							       "eswin,syscrg_csr");
+	if (IS_ERR(dwc_priv->crg_regmap))
+		return dev_err_probe(&pdev->dev, PTR_ERR(dwc_priv->crg_regmap),
+				"Failed to get syscrg_csr regmap\n");
+
+	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,syscrg_csr", 1,
+					 &hsp_aclk_ctrl_offset);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "can't get hsp_aclk_ctrl_offset\n");
+
+	regmap_read(dwc_priv->crg_regmap, hsp_aclk_ctrl_offset, &hsp_aclk_ctrl_regset);
+	hsp_aclk_ctrl_regset |= (EIC7700_HSP_ACLK_CLKEN | EIC7700_HSP_ACLK_DIVSOR);
+	regmap_write(dwc_priv->crg_regmap, hsp_aclk_ctrl_offset, hsp_aclk_ctrl_regset);
+
+	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,syscrg_csr", 2,
+					 &hsp_cfg_ctrl_offset);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "can't get hsp_cfg_ctrl_offset\n");
+
+	regmap_write(dwc_priv->crg_regmap, hsp_cfg_ctrl_offset, EIC7700_HSP_CFG_CTRL_REGSET);
+
+	dwc_priv->hsp_regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+							       "eswin,hsp_sp_csr");
+	if (IS_ERR(dwc_priv->hsp_regmap))
+		return dev_err_probe(&pdev->dev, PTR_ERR(dwc_priv->hsp_regmap),
+				"Failed to get hsp_sp_csr regmap\n");
+
+	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,hsp_sp_csr", 2,
+					 &eth_phy_ctrl_offset);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "can't get eth_phy_ctrl_offset\n");
+
+	regmap_read(dwc_priv->hsp_regmap, eth_phy_ctrl_offset, &eth_phy_ctrl_regset);
+	eth_phy_ctrl_regset |= (EIC7700_ETH_TX_CLK_SEL | EIC7700_ETH_PHY_INTF_SELI);
+	regmap_write(dwc_priv->hsp_regmap, eth_phy_ctrl_offset, eth_phy_ctrl_regset);
+
+	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,hsp_sp_csr", 3,
+					 &eth_axi_lp_ctrl_offset);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "can't get eth_axi_lp_ctrl_offset\n");
+
+	regmap_write(dwc_priv->hsp_regmap, eth_axi_lp_ctrl_offset, EIC7700_ETH_CSYSREQ_VAL);
+
+	plat_dat->clk_tx_i = devm_clk_get_enabled(&pdev->dev, "tx");
+	if (IS_ERR(plat_dat->clk_tx_i))
+		return dev_err_probe(&pdev->dev, PTR_ERR(plat_dat->clk_tx_i),
+				"error getting tx clock\n");
+
+	plat_dat->fix_mac_speed = eic7700_qos_fix_speed;
+	plat_dat->set_clk_tx_rate = stmmac_set_clk_tx_rate;
+	plat_dat->bsp_priv = dwc_priv;
+
+	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "Failed to driver probe\n");
+
+	return ret;
+}
+
+static const struct of_device_id eic7700_dwmac_match[] = {
+	{ .compatible = "eswin,eic7700-qos-eth" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, eic7700_dwmac_match);
+
+static struct platform_driver eic7700_dwmac_driver = {
+	.probe  = eic7700_dwmac_probe,
+	.remove = stmmac_pltfr_remove,
+	.driver = {
+		.name           = "eic7700-eth-dwmac",
+		.pm             = &stmmac_pltfr_pm_ops,
+		.of_match_table = eic7700_dwmac_match,
+	},
+};
+module_platform_driver(eic7700_dwmac_driver);
+
+MODULE_AUTHOR("Eswin");
+MODULE_AUTHOR("Shuang Liang <liangshuang@eswincomputing.com>");
+MODULE_AUTHOR("Shangjuan Wei <weishangjuan@eswincomputing.com>");
+MODULE_DESCRIPTION("Eswin eic7700 qos ethernet driver");
+MODULE_LICENSE("GPL");
--
2.17.1

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

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

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

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

	Andrew

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

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

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

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

	Andrew

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

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

Please configure your email client to wrap at about 78
characters. Standard network etiquette.

Yes, if rx-internal-delay-ps or/and tx-internal-delay-ps are in DT,
they should configure the delay the MAC applies.


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

Please read:

https://elixir.bootlin.com/linux/v6.15.7/source/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L287

It gives a detailed description of what phy-mode = "rmgii-id" means. 

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

It is O.K. when the total delay is > 2ns. However, please note what is
said, the normal way to implement delays in Linux. The PHY does the
2ns delay. The MAC can then do fine tuning, adding additional small
delays.

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

So there is only every going to be one board? There will never produce
a cost optimised version with a different, cheaper PHY? You will never
support connecting the MAC directly an Ethernet switch? You will never
make use of a PHY which can translate to SGMII/1000BaseX, and then
have an SFP cage?

DT properties are there to make your hardware more flexible. You can
use it to describe such setups, and handle the timing needed for each.

By default, when phy-mode is rgmii-id, the MAC adds 0ns, the PHY 2ns,
and most systems will just work. That 2ns is what the RGMII standard
requires. You can then fine tune it with rx-internal-delay-ps and
tx-internal-delay-ps if your design does not correctly follow the
RGMII standard.

	Andrew

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-21 21:10:55 (星期一)
> 收件人: 李志 <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: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet
driver
> 
> > > > 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?
> 
> Please configure your email client to wrap at about 78
> characters. Standard network etiquette.
> 
> Yes, if rx-internal-delay-ps or/and tx-internal-delay-ps are in DT,
> they should configure the delay the MAC applies.
> 
> 
> > 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?
> 
> Please read:
> 
>
https://elixir.bootlin.com/linux/v6.15.7/source/Documentation/devicetree/bin
dings/net/ethernet-controller.yaml#L287
> 
> It gives a detailed description of what phy-mode = "rmgii-id" means. 
> 
> > 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?
> 
> It is O.K. when the total delay is > 2ns. However, please note what is
> said, the normal way to implement delays in Linux. The PHY does the
> 2ns delay. The MAC can then do fine tuning, adding additional small
> delays.
> 
> > 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.
> 
> So there is only every going to be one board? There will never produce
> a cost optimised version with a different, cheaper PHY? You will never
> support connecting the MAC directly an Ethernet switch? You will never
> make use of a PHY which can translate to SGMII/1000BaseX, and then
> have an SFP cage?
> 
> DT properties are there to make your hardware more flexible. You can
> use it to describe such setups, and handle the timing needed for each.
> 
> By default, when phy-mode is rgmii-id, the MAC adds 0ns, the PHY 2ns,
> and most systems will just work. That 2ns is what the RGMII standard
> requires. You can then fine tune it with rx-internal-delay-ps and
> tx-internal-delay-ps if your design does not correctly follow the
> RGMII standard.
> 

Yes, DT properties are there to make our hardware more flexible.

Our platform uses three dedicated registers to configure RGMII signal
delays, due to differences in board-level designs. These registers control
delays for signals including RXD0�C3, TXD0�C3, RXDV, RXCLK, and TXCLK.
Among these, RXCLK and TXCLK are directly related to the standard DT
properties `rx-internal-delay-ps` and `tx-internal-delay-ps`, respectively.
The remaining signals (such as RXD0-4, TXD0-4, RXDV, etc.) require
additional configuration that cannot be expressed using standard
properties.

In v2, `eswin,dly-param-xxx` is used to configure all delay registers via
device tree, including RXCLK and TXCLK. Based on the latest discussion,
this approach in the next version:
- The delay configuration for RXCLK and TXCLK will be handled using the
 standard DT properties `rx-internal-delay-ps` and `tx-internal-delay-ps`.
- The remaining delay configuration (e.g., for RXD0-4, TXD0-4, RXDV) will
 continue to use the vendor-specific `eswin,dly-param-xxx` properties.
- If the standard delay properties are not specified in DT, a default of 0
ps
 will be assumed.

Is this understanding and approach correct and feasible?

> 	Andrew

> In v2, `eswin,dly-param-xxx` is used to configure all delay registers via
> device tree, including RXCLK and TXCLK. Based on the latest discussion,
> this approach in the next version:
> - The delay configuration for RXCLK and TXCLK will be handled using the
>  standard DT properties `rx-internal-delay-ps` and `tx-internal-delay-ps`.
> - The remaining delay configuration (e.g., for RXD0-4, TXD0-4, RXDV) will
>  continue to use the vendor-specific `eswin,dly-param-xxx` properties.
> - If the standard delay properties are not specified in DT, a default of 0
> ps
>  will be assumed.

Please keep the RGMII standard in mind. All it says is that there
should be a 2ns delay between the data and the clock signal. It is
also quite generous on the range of delays which should actually
work. It says nothing about being able to configure that delay. And it
definitely says nothing about being able to configure each individual
single.

You hardware has a lot of flexibility, but none of if should actually
be needed, if you follow the standard.

So phy-mode = "rgmii-id"; should be all you need for most boards.
Everything else should be optional, with sensible defaults.

	Andrew

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-22 22:07:23 (星期二)
> 收件人: 李志 <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: Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver
> 
> > In v2, `eswin,dly-param-xxx` is used to configure all delay registers via
> > device tree, including RXCLK and TXCLK. Based on the latest discussion,
> > this approach in the next version:
> > - The delay configuration for RXCLK and TXCLK will be handled using the
> >  standard DT properties `rx-internal-delay-ps` and `tx-internal-delay-ps`.
> > - The remaining delay configuration (e.g., for RXD0-4, TXD0-4, RXDV) will
> >  continue to use the vendor-specific `eswin,dly-param-xxx` properties.
> > - If the standard delay properties are not specified in DT, a default of 0
> > ps
> >  will be assumed.
> 
> Please keep the RGMII standard in mind. All it says is that there
> should be a 2ns delay between the data and the clock signal. It is
> also quite generous on the range of delays which should actually
> work. It says nothing about being able to configure that delay. And it
> definitely says nothing about being able to configure each individual
> single.
> 
> You hardware has a lot of flexibility, but none of if should actually
> be needed, if you follow the standard.
> 
> So phy-mode = "rgmii-id"; should be all you need for most boards.
> Everything else should be optional, with sensible defaults.
> 

On our platform, the vendor-specific attributes eswin,dly-param-* were
initially introduced to compensate for board-specific variations in RGMII
signal timing, primarily due to differences in PCB trace lengths. These
attributes allow fine-grained, per-signal delay control for RXD, TXD,
TXEN, RXDV, RXCLK, and TXCLK, based on empirically derived optimal phase
settings.
In our experience, setting phy-mode = "rgmii-id" alone, along with only
the standard properties rx-internal-delay-ps and tx-internal-delay-ps,
has proven insufficient to meet our hardware's timing requirements.
Therefore these standard properties are treated as controlling only RXCLK
and TXCLK, while continuing to use the eswin,dly-param-* attributes for
other signals.
Additionally, if rx-internal-delay-ps and tx-internal-delay-ps are
omitted, their values default to 0ps due to the use of devm_kzalloc().
This behavior reinforces the need for explicit delay values in certain
configurations. For reference, TI platforms use a dedicated IODELAY
hardware module to program per-signal RGMII delays in a similar fashion.

As per your suggestion, we will set mode="rgmii-id".
We have questions on setting delay parameters from dts we have two
approches. Could you please let us know which approach is appropriate?

1. Setting all delay parameters (RXD, TXD, TXEN, RXDV, RXCLK, and TXCLK)
   using vendor-specific attributes eswin,dly-param-*.
   e.g.
   eswin,dly-param-1000m = <0x20202020 0x96205A20 0x20202020>;
2. Setting delay parameters (RXD, TXD, TXEN, RXDV) using vendor-specific
   attributes eswin,dly-param-* , RXCLK using rx-internal-delay-ps and
   TXCLK using tx-internal-delay-ps.
   e.g
   eswin,dly-param-1000m = <0x20202020 0x80200020 0x20202020>;
   rx-internal-delay-ps = <9000>;
   tx-internal-delay-ps = <2200>;

> 	Andrew

> > You hardware has a lot of flexibility, but none of if should actually
> > be needed, if you follow the standard.
> > 
> > So phy-mode = "rgmii-id"; should be all you need for most boards.
> > Everything else should be optional, with sensible defaults.
> > 
> 
> On our platform, the vendor-specific attributes eswin,dly-param-* were
> initially introduced to compensate for board-specific variations in RGMII
> signal timing, primarily due to differences in PCB trace lengths.

So it seems like, because you have the flexibility in the hardware,
you designed your PCB poorly, breaking the standard, so now must have
these properties.  It would of been much better if you had stuck to
the standard...

Please ensure your default values, when nothing is specified in DT,
correspond to a board which actually fulfils the standard. The next
board which is made using this device can then avoid having anything
special in there DT blob.

> These attributes allow fine-grained, per-signal delay control for RXD, TXD,
> TXEN, RXDV, RXCLK, and TXCLK, based on empirically derived optimal phase
> settings.
> In our experience, setting phy-mode = "rgmii-id" alone, along with only
> the standard properties rx-internal-delay-ps and tx-internal-delay-ps,
> has proven insufficient to meet our hardware's timing requirements.

You don't need vendor properties for RXCLK and TXCLK, that is what
tx-internal-delay-ps and rx-internal-delay-ps do. They change the
clock signal relative to TX and RX data. So you only need properties
for TXEN and RXDV. You should probably call these
eswin,txen-internal-delay-ps and eswin,rxdv-internal-delay-ps.  In the
binding you need to clearly define what these mean, for your hardware,
i.e.  what is the delay relative to?

> 1. Setting all delay parameters (RXD, TXD, TXEN, RXDV, RXCLK, and TXCLK)
>    using vendor-specific attributes eswin,dly-param-*.
>    e.g.
>    eswin,dly-param-1000m = <0x20202020 0x96205A20 0x20202020>;
> 2. Setting delay parameters (RXD, TXD, TXEN, RXDV) using vendor-specific
>    attributes eswin,dly-param-* , RXCLK using rx-internal-delay-ps and
>    TXCLK using tx-internal-delay-ps.
>    e.g
>    eswin,dly-param-1000m = <0x20202020 0x80200020 0x20202020>;
>    rx-internal-delay-ps = <9000>;
>    tx-internal-delay-ps = <2200>;

Neither. DT should not contain HW values you poke into registers. They
should be SI using, in this case, pico seconds. From these delays in
picoseconds, have the driver calculate what values should be written
into the registers.

And these delay values are unlikely to be correct. You are using
rgmii-id, so the PHY is adding 2ns. You want the MAC to make small
tuning adjustments, so 200 could be reasonable, but 9000ps is way too
big.

	Andrew

Dear Andrew Lunn,
Thank you for your valuable and professional suggestions.
Please find our questions and explanations embedded below your comments
in the original email.

Best regards,

Li Zhi
Eswin Computing





> -----原始邮件-----
> 发件人: "Andrew Lunn" <andrew@lunn.ch>
> 发送时间:2025-07-31 21:31:52 (星期四)
> 收件人: 李志 <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: Re: Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver
> 
> > > You hardware has a lot of flexibility, but none of if should actually
> > > be needed, if you follow the standard.
> > > 
> > > So phy-mode = "rgmii-id"; should be all you need for most boards.
> > > Everything else should be optional, with sensible defaults.
> > > 
> > 
> > On our platform, the vendor-specific attributes eswin,dly-param-* were
> > initially introduced to compensate for board-specific variations in RGMII
> > signal timing, primarily due to differences in PCB trace lengths.
> 
> So it seems like, because you have the flexibility in the hardware,
> you designed your PCB poorly, breaking the standard, so now must have
> these properties.  It would of been much better if you had stuck to
> the standard...
> 
> Please ensure your default values, when nothing is specified in DT,
> correspond to a board which actually fulfils the standard. The next
> board which is made using this device can then avoid having anything
> special in there DT blob.
> 
> > These attributes allow fine-grained, per-signal delay control for RXD, TXD,
> > TXEN, RXDV, RXCLK, and TXCLK, based on empirically derived optimal phase
> > settings.
> > In our experience, setting phy-mode = "rgmii-id" alone, along with only
> > the standard properties rx-internal-delay-ps and tx-internal-delay-ps,
> > has proven insufficient to meet our hardware's timing requirements.
> 
> You don't need vendor properties for RXCLK and TXCLK, that is what
> tx-internal-delay-ps and rx-internal-delay-ps do. They change the
> clock signal relative to TX and RX data. So you only need properties
> for TXEN and RXDV. You should probably call these
> eswin,txen-internal-delay-ps and eswin,rxdv-internal-delay-ps.  In the
> binding you need to clearly define what these mean, for your hardware,
> i.e.  what is the delay relative to?
> 
> > 1. Setting all delay parameters (RXD, TXD, TXEN, RXDV, RXCLK, and TXCLK)
> >    using vendor-specific attributes eswin,dly-param-*.
> >    e.g.
> >    eswin,dly-param-1000m = <0x20202020 0x96205A20 0x20202020>;
> > 2. Setting delay parameters (RXD, TXD, TXEN, RXDV) using vendor-specific
> >    attributes eswin,dly-param-* , RXCLK using rx-internal-delay-ps and
> >    TXCLK using tx-internal-delay-ps.
> >    e.g
> >    eswin,dly-param-1000m = <0x20202020 0x80200020 0x20202020>;
> >    rx-internal-delay-ps = <9000>;
> >    tx-internal-delay-ps = <2200>;
> 
> Neither. DT should not contain HW values you poke into registers. They
> should be SI using, in this case, pico seconds. From these delays in
> picoseconds, have the driver calculate what values should be written
> into the registers.
> 
> And these delay values are unlikely to be correct. You are using
> rgmii-id, so the PHY is adding 2ns. You want the MAC to make small
> tuning adjustments, so 200 could be reasonable, but 9000ps is way too
> big.
> 

We re-tuned and verified that setting the TXD and RXD delays to 0 and
configuring TXEN and RXDV to 0 yielded the same hardware performance as
long as we only applied delays (e.g. 200ps) to TXCLK and RXCLK.
Therefore, in the next patch, we will drop the vendor-specific properties
(e.g. eswin,dly-param-*) and keep only the standard attributes, namely
rx-internal-delay-ps and tx-internal-delay-ps.
Is this correct?

> 	Andrew

> We re-tuned and verified that setting the TXD and RXD delays to 0 and
> configuring TXEN and RXDV to 0 yielded the same hardware performance as
> long as we only applied delays (e.g. 200ps) to TXCLK and RXCLK.

This is in addition to phy-mode = 'rgmii-id'?

> Therefore, in the next patch, we will drop the vendor-specific properties
> (e.g. eswin,dly-param-*) and keep only the standard attributes, namely
> rx-internal-delay-ps and tx-internal-delay-ps.
> Is this correct?

Yes, 200ps is a small tuning value, when the PHY adds the 2ns. This is
O.K.

	Andrew

Dear Andrew Lunn,
Thank you for your valuable and professional suggestions.
Please find our explanations embedded below your comments in the
original email.

Best regards,

Li Zhi
Eswin Computing


> -----原始邮件-----
> 发件人: "Andrew Lunn" <andrew@lunn.ch>
> 发送时间:2025-08-22 11:17:37 (星期五)
> 收件人: 李志 <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: Re: Re: Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver
> 
> > We re-tuned and verified that setting the TXD and RXD delays to 0 and
> > configuring TXEN and RXDV to 0 yielded the same hardware performance as
> > long as we only applied delays (e.g. 200ps) to TXCLK and RXCLK.
> 
> This is in addition to phy-mode = 'rgmii-id'?
> 

Yes, our re-tuning and verification were performed with phy-mode set to
rgmii-id.

> > Therefore, in the next patch, we will drop the vendor-specific properties
> > (e.g. eswin,dly-param-*) and keep only the standard attributes, namely
> > rx-internal-delay-ps and tx-internal-delay-ps.
> > Is this correct?
> 
> Yes, 200ps is a small tuning value, when the PHY adds the 2ns. This is
> O.K.
> 
> 	Andrew

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

On Thu, Jul 03, 2025 at 05:20:15PM +0800, weishangjuan@eswincomputing.com wrote:
> +static void eic7700_qos_fix_speed(void *priv, int speed, u32 mode)
> +{
> +	struct eic7700_qos_priv *dwc_priv = priv;
> +	int i;
> +
> +	switch (speed) {
> +	case SPEED_1000:
> +		for (i = 0; i < 3; i++)
> +			regmap_write(dwc_priv->hsp_regmap,
> +				     dwc_priv->dly_hsp_reg[i],
> +				     dwc_priv->dly_param_1000m[i]);
> +		break;
> +	case SPEED_100:
> +		for (i = 0; i < 3; i++) {
> +			regmap_write(dwc_priv->hsp_regmap,
> +				     dwc_priv->dly_hsp_reg[i],
> +				     dwc_priv->dly_param_100m[i]);
> +		}

The other two instances don't have the curley braces, why does this need
it?

> +		break;
> +	case SPEED_10:
> +		for (i = 0; i < 3; i++) {
> +			regmap_write(dwc_priv->hsp_regmap,
> +				     dwc_priv->dly_hsp_reg[i],
> +				     dwc_priv->dly_param_10m[i]);
> +		}
> +		break;
> +	default:
> +		dev_err(dwc_priv->dev, "invalid speed %u\n", speed);
> +		break;
> +	}

Overall, wouldn't:

	const u32 *dly_param;

	switch (speed) {
	case SPEED_1000:
		dly_param = dwc_priv->dly_param_1000m;
		break;
	... etc ...
	default:
		dly_param = NULL;
		dev_err(dwc_priv->dev, "invalid speed %u\n", speed);
		break;
	}

	if (dly_param)
		for (i = 0; i < 3; i++)
			regmap_write(dwc_priv->hsp_regmap,
				     dwc_priv->dly_hsp_reg[i],
				     dly_param[i]);

be more concise and easier to read?

> +}
> +
> +static int eic7700_dwmac_probe(struct platform_device *pdev)
> +{
> +	struct plat_stmmacenet_data *plat_dat;
> +	struct stmmac_resources stmmac_res;
> +	struct eic7700_qos_priv *dwc_priv;
> +	u32 hsp_aclk_ctrl_offset;
> +	u32 hsp_aclk_ctrl_regset;
> +	u32 hsp_cfg_ctrl_offset;
> +	u32 eth_axi_lp_ctrl_offset;
> +	u32 eth_phy_ctrl_offset;
> +	u32 eth_phy_ctrl_regset;
> +	bool has_rx_dly = false;
> +	bool has_tx_dly = false;
> +	int ret;
> +
> +	ret = stmmac_get_platform_resources(pdev, &stmmac_res);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				"failed to get resources\n");
> +
> +	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
> +	if (IS_ERR(plat_dat))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(plat_dat),
> +				"dt configuration failed\n");
> +
> +	dwc_priv = devm_kzalloc(&pdev->dev, sizeof(*dwc_priv), GFP_KERNEL);
> +	if (!dwc_priv)
> +		return -ENOMEM;
> +
> +	dwc_priv->dev = &pdev->dev;
> +	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;
> +
> +	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);

Consider using %pe and ERR_PTR(ret) so that error codes can be
translated to human readable strings. Ditto elsewhere.

> +	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) {
> +		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");
> +	}
> +
> +	ret = of_property_read_variable_u32_array(pdev->dev.of_node, "eswin,dly_hsp_reg",
> +						  &dwc_priv->dly_hsp_reg[0], 3, 0);
> +	if (ret != 3) {
> +		dev_err(&pdev->dev, "can't get delay hsp reg.ret(%d)\n", ret);
> +		return ret;
> +	}
> +
> +	dwc_priv->crg_regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +							       "eswin,syscrg_csr");
> +	if (IS_ERR(dwc_priv->crg_regmap))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(dwc_priv->crg_regmap),
> +				"Failed to get syscrg_csr regmap\n");
> +
> +	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,syscrg_csr", 1,
> +					 &hsp_aclk_ctrl_offset);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "can't get hsp_aclk_ctrl_offset\n");
> +
> +	regmap_read(dwc_priv->crg_regmap, hsp_aclk_ctrl_offset, &hsp_aclk_ctrl_regset);
> +	hsp_aclk_ctrl_regset |= (EIC7700_HSP_ACLK_CLKEN | EIC7700_HSP_ACLK_DIVSOR);
> +	regmap_write(dwc_priv->crg_regmap, hsp_aclk_ctrl_offset, hsp_aclk_ctrl_regset);
> +
> +	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,syscrg_csr", 2,
> +					 &hsp_cfg_ctrl_offset);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "can't get hsp_cfg_ctrl_offset\n");
> +
> +	regmap_write(dwc_priv->crg_regmap, hsp_cfg_ctrl_offset, EIC7700_HSP_CFG_CTRL_REGSET);
> +
> +	dwc_priv->hsp_regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +							       "eswin,hsp_sp_csr");
> +	if (IS_ERR(dwc_priv->hsp_regmap))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(dwc_priv->hsp_regmap),
> +				"Failed to get hsp_sp_csr regmap\n");
> +
> +	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,hsp_sp_csr", 2,
> +					 &eth_phy_ctrl_offset);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "can't get eth_phy_ctrl_offset\n");
> +
> +	regmap_read(dwc_priv->hsp_regmap, eth_phy_ctrl_offset, &eth_phy_ctrl_regset);
> +	eth_phy_ctrl_regset |= (EIC7700_ETH_TX_CLK_SEL | EIC7700_ETH_PHY_INTF_SELI);
> +	regmap_write(dwc_priv->hsp_regmap, eth_phy_ctrl_offset, eth_phy_ctrl_regset);
> +
> +	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,hsp_sp_csr", 3,
> +					 &eth_axi_lp_ctrl_offset);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "can't get eth_axi_lp_ctrl_offset\n");
> +
> +	regmap_write(dwc_priv->hsp_regmap, eth_axi_lp_ctrl_offset, EIC7700_ETH_CSYSREQ_VAL);
> +

Consider more sensible wrapping of this (netdev frowns at >80
characters per line, except for message strings that should remain
greppable.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

On 03/07/2025 11:20, weishangjuan@eswincomputing.com wrote:
> +	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,syscrg_csr", 1,
> +					 &hsp_aclk_ctrl_offset);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "can't get hsp_aclk_ctrl_offset\n");
> +
> +	regmap_read(dwc_priv->crg_regmap, hsp_aclk_ctrl_offset, &hsp_aclk_ctrl_regset);
> +	hsp_aclk_ctrl_regset |= (EIC7700_HSP_ACLK_CLKEN | EIC7700_HSP_ACLK_DIVSOR);
> +	regmap_write(dwc_priv->crg_regmap, hsp_aclk_ctrl_offset, hsp_aclk_ctrl_regset);
> +
> +	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,syscrg_csr", 2,
> +					 &hsp_cfg_ctrl_offset);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "can't get hsp_cfg_ctrl_offset\n");
> +
> +	regmap_write(dwc_priv->crg_regmap, hsp_cfg_ctrl_offset, EIC7700_HSP_CFG_CTRL_REGSET);
> +
> +	dwc_priv->hsp_regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +							       "eswin,hsp_sp_csr");

There is no such property. I already said at v2 you cannot have
undocumented ABI.

> +	if (IS_ERR(dwc_priv->hsp_regmap))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(dwc_priv->hsp_regmap),
> +				"Failed to get hsp_sp_csr regmap\n");
> +
> +	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,hsp_sp_csr", 2,

NAK

> +					 &eth_phy_ctrl_offset);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "can't get eth_phy_ctrl_offset\n");
> +
> +	regmap_read(dwc_priv->hsp_regmap, eth_phy_ctrl_offset, &eth_phy_ctrl_regset);
> +	eth_phy_ctrl_regset |= (EIC7700_ETH_TX_CLK_SEL | EIC7700_ETH_PHY_INTF_SELI);
> +	regmap_write(dwc_priv->hsp_regmap, eth_phy_ctrl_offset, eth_phy_ctrl_regset);
> +
> +	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,hsp_sp_csr", 3,
> +					 &eth_axi_lp_ctrl_offset);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "can't get eth_axi_lp_ctrl_offset\n");
> +
> +	regmap_write(dwc_priv->hsp_regmap, eth_axi_lp_ctrl_offset, EIC7700_ETH_CSYSREQ_VAL);
> +
> +	plat_dat->clk_tx_i = devm_clk_get_enabled(&pdev->dev, "tx");
> +	if (IS_ERR(plat_dat->clk_tx_i))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(plat_dat->clk_tx_i),
> +				"error getting tx clock\n");
> +
> +	plat_dat->fix_mac_speed = eic7700_qos_fix_speed;
> +	plat_dat->set_clk_tx_rate = stmmac_set_clk_tx_rate;
> +	plat_dat->bsp_priv = dwc_priv;
> +
> +	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Failed to driver probe\n");
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id eic7700_dwmac_match[] = {
> +	{ .compatible = "eswin,eic7700-qos-eth" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, eic7700_dwmac_match);
> +
> +static struct platform_driver eic7700_dwmac_driver = {
> +	.probe  = eic7700_dwmac_probe,
> +	.remove = stmmac_pltfr_remove,
> +	.driver = {
> +		.name           = "eic7700-eth-dwmac",
> +		.pm             = &stmmac_pltfr_pm_ops,
> +		.of_match_table = eic7700_dwmac_match,
> +	},
> +};
> +module_platform_driver(eic7700_dwmac_driver);
> +
> +MODULE_AUTHOR("Eswin");

Drop, that's not a person.


Best regards,
Krzysztof