[PATCH v4 0/2] Add driver support for Eswin eic7700 SoC ethernet controller

[PATCH v4 0/2] Add driver support for Eswin eic7700 SoC ethernet controller

[weishangjuan]

From: Shangjuan Wei <weishangjuan@eswincomputing.com>

This series depends on the vendor prefix [1] and config option patch [2].

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20250825&id=ac29e4487aa20a21b7c3facbd1f14f5093835dc9
[2] https://lore.kernel.org/all/20250825132427.1618089-3-pinkesh.vaghela@einfochips.com/

Updates:

  Changes in v4:
  - Updated eswin,eic7700-eth.yaml
    - Modify reg:minItems:1 to reg:maxItems: 1
    - Delete minItems and maxItems of clock and clock-names
    - Delete phy-mode and phy-handle properties
    - Add description for clock
    - Add types of clock-names
    - Delete descriptions for rx-internal-delay-ps and tx-internal-delay-ps
    - Add enum value for rx-internal-delay-ps and tx-internal-delay-ps
    - Modify description for eswin,hsp-sp-csr property
    - Delete eswin,syscrg-csr and eswin,dly-hsp-reg properties
    - Modify phy-mode="rgmii" to phy-mode="rgmii-id"
  - Updated dwmac-eic7700.c
    - Remove fix_mac_speed and configure different delays for different rates
    - Merge the offset of the dly register into the eswin, hsp sp csr attributes
      for unified management
    - Add missing Author and optimize the number of characters per
      line to within 80
    - Support default delay configuration and add the handling of vendor delay 
      configuration
    - Add clks_config for pm_runtime
    - Modify the attribute format, such as eswin,hsp_sp_csr to eswin,hsp-sp-csr
  - Link to v3: https://lore.kernel.org/all/20250703091808.1092-1-weishangjuan@eswincomputing.com/

  Changes in v3:
  - Updated eswin,eic7700-eth.yaml
    - Modify snps,dwmac to snps,dwmac-5.20
    - Remove the description of reg
    - Modify the value of clock minItems and maxItems
    - Modify the value of clock-names minItems and maxItems
    - Add descriptions of snps,write-questions, snps,read-questions
    - Add rx-internal-delay-ps and tx-internal-delay-ps properties
    - Modify descriptions for custom properties, such as eswin,hsp-sp-csr
    - Delete snps,axi-config property
    - Add snps,fixed-burst snps,aal snps,tso properties
    - Delete snps,lpi_en property
    - Modify format of custom properties
  - Updated dwmac-eic7700.c
    - Simplify drivers and remove unnecessary API and DTS attribute configurations
    - Increase the mapping from tx/rx_delay_ps to private dly
  - Link to v2: https://lore.kernel.org/all/aDad+8YHEFdOIs38@mev-dev.igk.intel.com/

  Changes in v2:
  - Updated eswin,eic7700-eth.yaml
    - Add snps,dwmac in binding file
    - Modify the description of reg
    - Modify the number of clock-names
    - Changed the names of reset-names and phy-mode
    - Add description for custom properties, such as eswin,hsp_sp_csr
    - Delete snps,blen snps,rd_osr_lmt snps,wr_osr_lmt properties
  - Updated dwmac-eic7700.c
    - Remove the code related to PHY LED configuration from the MAC driver
    - Adjust the code format and driver interfaces, such as replacing kzalloc
      with devm_kzalloc, etc.
    - Use phylib instead of the GPIO API in the driver to implement the PHY
      reset function
  - Link to v1: https://lore.kernel.org/all/20250516010849.784-1-weishangjuan@eswincomputing.com/

Shangjuan Wei (2):
  dt-bindings: ethernet: eswin: Document for EIC7700 SoC
  ethernet: eswin: Add eic7700 ethernet driver

 .../bindings/net/eswin,eic7700-eth.yaml       | 130 +++++++++
 drivers/net/ethernet/stmicro/stmmac/Kconfig   |  11 +
 drivers/net/ethernet/stmicro/stmmac/Makefile  |   1 +
 .../ethernet/stmicro/stmmac/dwmac-eic7700.c   | 270 ++++++++++++++++++
 4 files changed, 412 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c

--
2.17.1

[PATCH v4 1/2] dt-bindings: ethernet: eswin: Document for EIC7700 SoC

[weishangjuan]

From: Shangjuan Wei <weishangjuan@eswincomputing.com>

Add ESWIN EIC7700 Ethernet controller, supporting clock
configuration, delay adjustment and speed adaptive functions.

Signed-off-by: Zhi Li <lizhi2@eswincomputing.com>
Signed-off-by: Shangjuan Wei <weishangjuan@eswincomputing.com>
---
 .../bindings/net/eswin,eic7700-eth.yaml       | 130 ++++++++++++++++++
 1 file changed, 130 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml

diff --git a/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
new file mode 100644
index 000000000000..aa8bf2be9af7
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
@@ -0,0 +1,130 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/eswin,eic7700-eth.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Eswin EIC7700 SOC Eth Controller
+
+maintainers:
+  - Shuang Liang <liangshuang@eswincomputing.com>
+  - Zhi Li <lizhi2@eswincomputing.com>
+  - Shangjuan Wei <weishangjuan@eswincomputing.com>
+
+description:
+  The eth controller registers are part of the syscrg block on
+  the EIC7700 SoC.
+
+select:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - eswin,eic7700-qos-eth
+  required:
+    - compatible
+
+allOf:
+  - $ref: snps,dwmac.yaml#
+
+properties:
+  compatible:
+    items:
+      - const: eswin,eic7700-qos-eth
+      - const: snps,dwmac-5.20
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-names:
+    const: macirq
+
+  clocks:
+    items:
+      - description: GMAC main clock
+      - description: Tx clock
+      - description: AXI clock
+      - description: Configuration clock
+
+  clock-names:
+    contains:
+      enum:
+        - axi
+        - cfg
+        - stmmaceth
+        - tx
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    items:
+      - const: stmmaceth
+
+  rx-internal-delay-ps:
+    enum: [0, 200, 600, 1200, 1600, 1800, 2000, 2200, 2400]
+
+  tx-internal-delay-ps:
+    enum: [0, 200, 600, 1200, 1600, 1800, 2000, 2200, 2400]
+
+  eswin,hsp-sp-csr:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - description: Phandle to HSP(High-Speed Peripheral) device
+      - description: Offset of phy control register for internal
+                     or external clock selection
+      - description: Offset of AXI clock controller Low-Power request
+                     register
+      - description: Offset of register controlling TX/RX clock delay
+    description: |
+      A phandle to hsp-sp-csr with three arguments that configure
+      HSP(High-Speed Peripheral) device. The argument one is the
+      offset of phy control register for internal or external clock
+      selection, the argument two is Offset of AXI clock controller
+      Low-Power request register, the argument three is Offset of
+      register controlling TX/RX clock delay.
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - interrupts
+  - interrupt-names
+  - phy-mode
+  - resets
+  - reset-names
+  - rx-internal-delay-ps
+  - tx-internal-delay-ps
+  - eswin,hsp-sp-csr
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    ethernet@50400000 {
+        compatible = "eswin,eic7700-qos-eth", "snps,dwmac-5.20";
+        reg = <0x50400000 0x10000>;
+        clocks = <&d0_clock 186>, <&d0_clock 171>, <&d0_clock 40>,
+                <&d0_clock 193>;
+        clock-names = "axi", "cfg", "stmmaceth", "tx";
+        interrupt-parent = <&plic>;
+        interrupts = <61>;
+        interrupt-names = "macirq";
+        phy-mode = "rgmii-id";
+        phy-handle = <&phy0>;
+        resets = <&reset 95>;
+        reset-names = "stmmaceth";
+        rx-internal-delay-ps = <200>;
+        tx-internal-delay-ps = <200>;
+        eswin,hsp-sp-csr = <&hsp_sp_csr 0x100 0x108 0x118>;
+        snps,axi-config = <&stmmac_axi_setup>;
+        snps,aal;
+        snps,fixed-burst;
+        snps,tso;
+        stmmac_axi_setup: stmmac-axi-config {
+            snps,blen = <0 0 0 0 16 8 4>;
+            snps,rd_osr_lmt = <2>;
+            snps,wr_osr_lmt = <2>;
+        };
+    };
--
2.17.1

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

[weishangjuan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=yes, Size: 10145 bytes --]

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   | 270 ++++++++++++++++++
 3 files changed, 282 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..8b2082126a42
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c
@@ -0,0 +1,270 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Eswin DWC Ethernet linux driver
+ *
+ * Copyright 2025, Beijing ESWIN Computing Technology Co., Ltd.
+ *
+ * Authors:
+ *   Zhi Li <lizhi2@eswincomputing.com>
+ *   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 */
+#define EIC7700_ETH_TX_CLK_SEL		BIT(16)
+#define EIC7700_ETH_PHY_INTF_SELI	BIT(0)
+
+/* eth_axi_lp_ctrl_offset eth0:0x108 */
+#define EIC7700_ETH_CSYSREQ_VAL		BIT(0)
+
+/*
+ * TX/RX Clock Delay Bit Masks:
+ * - TX Delay: bits [14:8] — TX_CLK delay (unit: 0.1ns per bit)
+ * - RX Delay: bits [30:24] — RX_CLK delay (unit: 0.1ns per bit)
+ */
+#define EIC7700_ETH_TX_ADJ_DELAY	GENMASK(14, 8)
+#define EIC7700_ETH_RX_ADJ_DELAY	GENMASK(30, 24)
+
+#define EIC7700_MAX_DELAY_UNIT 0x7F
+
+struct eic7700_qos_priv {
+	struct device *dev;
+	struct regmap *hsp_regmap;
+	struct clk *clk_tx;
+	struct clk *clk_axi;
+	struct clk *clk_cfg;
+	u32 tx_delay_ps;
+	u32 rx_delay_ps;
+};
+
+/**
+ * eic7700_apply_delay - Update TX or RX delay bits in delay parameter value.
+ * @delay_ps: Delay in picoseconds (capped at 12.7ns).
+ * @reg:      Pointer to register value to modify.
+ * @is_rx:    True for RX delay (bits 30:24), false for TX delay (bits 14:8).
+ *
+ * Converts delay to 0.1ns units, caps at 0x7F, and sets appropriate bits.
+ * Only RX or TX bits are updated; other bits remain unchanged.
+ */
+static inline void eic7700_apply_delay(u32 delay_ps, u32 *reg, bool is_rx)
+{
+	if (!reg)
+		return;
+
+	u32 val = min(delay_ps / 100, EIC7700_MAX_DELAY_UNIT);
+
+	if (is_rx) {
+		*reg &= ~EIC7700_ETH_RX_ADJ_DELAY;
+		*reg |= (val << 24) & EIC7700_ETH_RX_ADJ_DELAY;
+	} else {
+		*reg &= ~EIC7700_ETH_TX_ADJ_DELAY;
+		*reg |= (val << 8) & EIC7700_ETH_TX_ADJ_DELAY;
+	}
+}
+
+static int eic7700_clks_config(void *priv, bool enabled)
+{
+	struct eic7700_qos_priv *dwc = (struct eic7700_qos_priv *)priv;
+	int ret = 0;
+
+	if (enabled) {
+		ret = clk_prepare_enable(dwc->clk_tx);
+		if (ret < 0) {
+			dev_err(dwc->dev, "Failed to enable tx clock: %d\n",
+				ret);
+			goto err;
+		}
+
+		ret = clk_prepare_enable(dwc->clk_axi);
+		if (ret < 0) {
+			dev_err(dwc->dev, "Failed to enable axi clock: %d\n",
+				ret);
+			goto err_tx;
+		}
+
+		ret = clk_prepare_enable(dwc->clk_cfg);
+		if (ret < 0) {
+			dev_err(dwc->dev, "Failed to enable cfg clock: %d\n",
+				ret);
+			goto err_axi;
+		}
+	} else {
+		clk_disable_unprepare(dwc->clk_tx);
+		clk_disable_unprepare(dwc->clk_axi);
+		clk_disable_unprepare(dwc->clk_cfg);
+	}
+	return ret;
+
+err_axi:
+	clk_disable_unprepare(dwc->clk_axi);
+err_tx:
+	clk_disable_unprepare(dwc->clk_tx);
+err:
+	return ret;
+}
+
+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 eth_axi_lp_ctrl_offset;
+	u32 eth_phy_ctrl_offset;
+	u32 eth_phy_ctrl_regset;
+	u32 eth_rxd_dly_offset;
+	u32 eth_dly_param = 0;
+	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;
+
+	/* Read rx-internal-delay-ps and update rx_clk delay */
+	if (!of_property_read_u32(pdev->dev.of_node,
+				  "rx-internal-delay-ps",
+				  &dwc_priv->rx_delay_ps)) {
+		eic7700_apply_delay(dwc_priv->rx_delay_ps,
+				    &eth_dly_param, true);
+	} else {
+		dev_warn(&pdev->dev, "can't get rx-internal-delay-ps\n");
+	}
+
+	/* Read tx-internal-delay-ps and update tx_clk delay */
+	if (!of_property_read_u32(pdev->dev.of_node,
+				  "tx-internal-delay-ps",
+				  &dwc_priv->tx_delay_ps)) {
+		eic7700_apply_delay(dwc_priv->tx_delay_ps,
+				    &eth_dly_param, false);
+	} else {
+		dev_warn(&pdev->dev, "can't get tx-internal-delay-ps\n");
+	}
+
+	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",
+					 1, &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",
+					 2, &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);
+
+	ret = of_property_read_u32_index(pdev->dev.of_node,
+					 "eswin,hsp-sp-csr",
+					 3, &eth_rxd_dly_offset);
+	if (ret)
+		return dev_err_probe(&pdev->dev,
+				ret,
+				"can't get eth_rxd_dly_offset\n");
+
+	regmap_write(dwc_priv->hsp_regmap, eth_rxd_dly_offset,
+		     eth_dly_param);
+
+	dwc_priv->clk_tx = devm_clk_get(&pdev->dev, "tx");
+	if (IS_ERR(dwc_priv->clk_tx))
+		return dev_err_probe(&pdev->dev,
+				PTR_ERR(dwc_priv->clk_tx),
+				"error getting tx clock\n");
+
+	dwc_priv->clk_axi = devm_clk_get(&pdev->dev, "axi");
+	if (IS_ERR(dwc_priv->clk_axi))
+		return dev_err_probe(&pdev->dev,
+				PTR_ERR(dwc_priv->clk_axi),
+				"error getting axi clock\n");
+
+	dwc_priv->clk_cfg = devm_clk_get(&pdev->dev, "cfg");
+	if (IS_ERR(dwc_priv->clk_cfg))
+		return dev_err_probe(&pdev->dev,
+				PTR_ERR(dwc_priv->clk_cfg),
+				"error getting cfg clock\n");
+
+	ret = eic7700_clks_config(dwc_priv, true);
+	if (ret)
+		return dev_err_probe(&pdev->dev,
+				ret,
+				"error enable clock\n");
+
+	plat_dat->clk_tx_i = dwc_priv->clk_tx;
+	plat_dat->set_clk_tx_rate = stmmac_set_clk_tx_rate;
+	plat_dat->bsp_priv = dwc_priv;
+	plat_dat->clks_config = eic7700_clks_config;
+
+	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
+	if (ret) {
+		eic7700_clks_config(dwc_priv, false);
+		return dev_err_probe(&pdev->dev,
+				ret,
+				"Failed to driver probe\n");
+	}
+
+	return ret;
+}
+
+static void eic7700_dwmac_remove(struct platform_device *pdev)
+{
+	struct eic7700_qos_priv *dwc_priv = get_stmmac_bsp_priv(&pdev->dev);
+
+	stmmac_pltfr_remove(pdev);
+	eic7700_clks_config(dwc_priv, false);
+}
+
+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 = eic7700_dwmac_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("Zhi Li <lizhi2@eswincomputing.com>");
+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

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

[Russell King (Oracle)]

On Wed, Aug 27, 2025 at 04:14:17PM +0800, weishangjuan@eswincomputing.com wrote:
> +struct eic7700_qos_priv {
> +	struct device *dev;
> +	struct regmap *hsp_regmap;
> +	struct clk *clk_tx;

Consider putting a pointer to the plat_dat here instead of clk_tx.

> +	struct clk *clk_axi;
> +	struct clk *clk_cfg;

Consider moving these into plat_dat->clks.

> +	u32 tx_delay_ps;
> +	u32 rx_delay_ps;
> +};
> +
> +/**
> + * eic7700_apply_delay - Update TX or RX delay bits in delay parameter value.
> + * @delay_ps: Delay in picoseconds (capped at 12.7ns).
> + * @reg:      Pointer to register value to modify.
> + * @is_rx:    True for RX delay (bits 30:24), false for TX delay (bits 14:8).
> + *
> + * Converts delay to 0.1ns units, caps at 0x7F, and sets appropriate bits.
> + * Only RX or TX bits are updated; other bits remain unchanged.
> + */
> +static inline void eic7700_apply_delay(u32 delay_ps, u32 *reg, bool is_rx)
> +{
> +	if (!reg)
> +		return;
> +
> +	u32 val = min(delay_ps / 100, EIC7700_MAX_DELAY_UNIT);
> +
> +	if (is_rx) {
> +		*reg &= ~EIC7700_ETH_RX_ADJ_DELAY;
> +		*reg |= (val << 24) & EIC7700_ETH_RX_ADJ_DELAY;
> +	} else {
> +		*reg &= ~EIC7700_ETH_TX_ADJ_DELAY;
> +		*reg |= (val << 8) & EIC7700_ETH_TX_ADJ_DELAY;
> +	}
> +}
> +
> +static int eic7700_clks_config(void *priv, bool enabled)
> +{
> +	struct eic7700_qos_priv *dwc = (struct eic7700_qos_priv *)priv;
> +	int ret = 0;
> +
> +	if (enabled) {
> +		ret = clk_prepare_enable(dwc->clk_tx);
> +		if (ret < 0) {
> +			dev_err(dwc->dev, "Failed to enable tx clock: %d\n",
> +				ret);
> +			goto err;
> +		}
> +
> +		ret = clk_prepare_enable(dwc->clk_axi);
> +		if (ret < 0) {
> +			dev_err(dwc->dev, "Failed to enable axi clock: %d\n",
> +				ret);
> +			goto err_tx;
> +		}
> +
> +		ret = clk_prepare_enable(dwc->clk_cfg);
> +		if (ret < 0) {
> +			dev_err(dwc->dev, "Failed to enable cfg clock: %d\n",
> +				ret);
> +			goto err_axi;
> +		}

You can then use clk_bulk_prepare_enable() here without the complex
unwinding if one enable fails.

> +	} else {
> +		clk_disable_unprepare(dwc->clk_tx);
> +		clk_disable_unprepare(dwc->clk_axi);
> +		clk_disable_unprepare(dwc->clk_cfg);

and clk_bulk_disable_unprepare() here.

> +	}
> +	return ret;
> +
> +err_axi:
> +	clk_disable_unprepare(dwc->clk_axi);
> +err_tx:
> +	clk_disable_unprepare(dwc->clk_tx);
> +err:
> +	return ret;
> +}
> +
> +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 eth_axi_lp_ctrl_offset;
> +	u32 eth_phy_ctrl_offset;
> +	u32 eth_phy_ctrl_regset;
> +	u32 eth_rxd_dly_offset;
> +	u32 eth_dly_param = 0;
> +	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;
> +
> +	/* Read rx-internal-delay-ps and update rx_clk delay */
> +	if (!of_property_read_u32(pdev->dev.of_node,
> +				  "rx-internal-delay-ps",
> +				  &dwc_priv->rx_delay_ps)) {
> +		eic7700_apply_delay(dwc_priv->rx_delay_ps,
> +				    &eth_dly_param, true);
> +	} else {
> +		dev_warn(&pdev->dev, "can't get rx-internal-delay-ps\n");
> +	}
> +
> +	/* Read tx-internal-delay-ps and update tx_clk delay */
> +	if (!of_property_read_u32(pdev->dev.of_node,
> +				  "tx-internal-delay-ps",
> +				  &dwc_priv->tx_delay_ps)) {
> +		eic7700_apply_delay(dwc_priv->tx_delay_ps,
> +				    &eth_dly_param, false);
> +	} else {
> +		dev_warn(&pdev->dev, "can't get tx-internal-delay-ps\n");
> +	}
> +
> +	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",
> +					 1, &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",
> +					 2, &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);
> +
> +	ret = of_property_read_u32_index(pdev->dev.of_node,
> +					 "eswin,hsp-sp-csr",
> +					 3, &eth_rxd_dly_offset);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev,
> +				ret,
> +				"can't get eth_rxd_dly_offset\n");
> +
> +	regmap_write(dwc_priv->hsp_regmap, eth_rxd_dly_offset,
> +		     eth_dly_param);
> +
> +	dwc_priv->clk_tx = devm_clk_get(&pdev->dev, "tx");
> +	if (IS_ERR(dwc_priv->clk_tx))
> +		return dev_err_probe(&pdev->dev,
> +				PTR_ERR(dwc_priv->clk_tx),
> +				"error getting tx clock\n");
> +
> +	dwc_priv->clk_axi = devm_clk_get(&pdev->dev, "axi");
> +	if (IS_ERR(dwc_priv->clk_axi))
> +		return dev_err_probe(&pdev->dev,
> +				PTR_ERR(dwc_priv->clk_axi),
> +				"error getting axi clock\n");
> +
> +	dwc_priv->clk_cfg = devm_clk_get(&pdev->dev, "cfg");
> +	if (IS_ERR(dwc_priv->clk_cfg))
> +		return dev_err_probe(&pdev->dev,
> +				PTR_ERR(dwc_priv->clk_cfg),
> +				"error getting cfg clock\n");

These then become devm_clk_bulk_get_all().

> +
> +	ret = eic7700_clks_config(dwc_priv, true);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev,
> +				ret,
> +				"error enable clock\n");

Maybe even devm_clk_bulk_get_all_enabled() which will omit this
step...

> +
> +	plat_dat->clk_tx_i = dwc_priv->clk_tx;
> +	plat_dat->set_clk_tx_rate = stmmac_set_clk_tx_rate;
> +	plat_dat->bsp_priv = dwc_priv;
> +	plat_dat->clks_config = eic7700_clks_config;
> +
> +	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
> +	if (ret) {
> +		eic7700_clks_config(dwc_priv, false);

... and means you don't need this call...

> +		return dev_err_probe(&pdev->dev,
> +				ret,
> +				"Failed to driver probe\n");
> +	}
> +
> +	return ret;
> +}
> +
> +static void eic7700_dwmac_remove(struct platform_device *pdev)
> +{
> +	struct eic7700_qos_priv *dwc_priv = get_stmmac_bsp_priv(&pdev->dev);
> +
> +	stmmac_pltfr_remove(pdev);
> +	eic7700_clks_config(dwc_priv, false);
> +}

... and you can remove this function entirely ...

> +
> +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 = eic7700_dwmac_remove,

... replacing this with stmmac_pltfr_remove().

Thanks.

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

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

[Andrew Lunn]

> +/**
> + * eic7700_apply_delay - Update TX or RX delay bits in delay parameter value.
> + * @delay_ps: Delay in picoseconds (capped at 12.7ns).
> + * @reg:      Pointer to register value to modify.
> + * @is_rx:    True for RX delay (bits 30:24), false for TX delay (bits 14:8).
> + *
> + * Converts delay to 0.1ns units, caps at 0x7F, and sets appropriate bits.
> + * Only RX or TX bits are updated; other bits remain unchanged.
> + */
> +static inline void eic7700_apply_delay(u32 delay_ps, u32 *reg, bool is_rx)
> +{
> +	if (!reg)
> +		return;
> +

Please don't use inline functions in .c files. Leave the compile to
decide.


> +	/* Read rx-internal-delay-ps and update rx_clk delay */
> +	if (!of_property_read_u32(pdev->dev.of_node,
> +				  "rx-internal-delay-ps",
> +				  &dwc_priv->rx_delay_ps)) {
> +		eic7700_apply_delay(dwc_priv->rx_delay_ps,
> +				    &eth_dly_param, true);
> +	} else {
> +		dev_warn(&pdev->dev, "can't get rx-internal-delay-ps\n");
> +	}
> +
> +	/* Read tx-internal-delay-ps and update tx_clk delay */
> +	if (!of_property_read_u32(pdev->dev.of_node,
> +				  "tx-internal-delay-ps",
> +				  &dwc_priv->tx_delay_ps)) {
> +		eic7700_apply_delay(dwc_priv->tx_delay_ps,
> +				    &eth_dly_param, false);

Given this code, why does eic7700_apply_delay() test for reg?  Don't
use defensive code, read your own code and make sure it cannot happen.

    Andrew

---
pw-bot: cr

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

[Vadim Fedorenko]

On 27/08/2025 09:14, weishangjuan@eswincomputing.com wrote:
> 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>
> ---

[...]

> +/**
> + * eic7700_apply_delay - Update TX or RX delay bits in delay parameter value.
> + * @delay_ps: Delay in picoseconds (capped at 12.7ns).
> + * @reg:      Pointer to register value to modify.
> + * @is_rx:    True for RX delay (bits 30:24), false for TX delay (bits 14:8).
> + *
> + * Converts delay to 0.1ns units, caps at 0x7F, and sets appropriate bits.
> + * Only RX or TX bits are updated; other bits remain unchanged.
> + */
> +static inline void eic7700_apply_delay(u32 delay_ps, u32 *reg, bool is_rx)

inlining functions in .c files is discouraged in netdev, let the
compiler decide inlining

> +{
> +	if (!reg)
> +		return;

please, avoid defensive programming. with this check you also mixing
code and variables..

> +
> +	u32 val = min(delay_ps / 100, EIC7700_MAX_DELAY_UNIT);
 > +> +	if (is_rx) {
> +		*reg &= ~EIC7700_ETH_RX_ADJ_DELAY;
> +		*reg |= (val << 24) & EIC7700_ETH_RX_ADJ_DELAY;
> +	} else {
> +		*reg &= ~EIC7700_ETH_TX_ADJ_DELAY;
> +		*reg |= (val << 8) & EIC7700_ETH_TX_ADJ_DELAY;

these 2 assignments should be converted to FIELD_PREP()

> +	}
> +}
> +

Re: [PATCH v4 1/2] dt-bindings: ethernet: eswin: Document for EIC7700 SoC

[Krzysztof Kozlowski]

On 27/08/2025 10:13, weishangjuan@eswincomputing.com wrote:
> +  clocks:
> +    items:
> +      - description: GMAC main clock
> +      - description: Tx clock
> +      - description: AXI clock
> +      - description: Configuration clock
> +
> +  clock-names:
> +    contains:

This part did not improve:
items: instead

> +      enum:
> +        - axi
> +        - cfg
> +        - stmmaceth
> +        - tx
> +
> +  resets:
> +    maxItems: 1
> +
> +  reset-names:
> +    items:
> +      - const: stmmaceth
> +
> +  rx-internal-delay-ps:
> +    enum: [0, 200, 600, 1200, 1600, 1800, 2000, 2200, 2400]
> +
> +  tx-internal-delay-ps:
> +    enum: [0, 200, 600, 1200, 1600, 1800, 2000, 2200, 2400]
> +
> +  eswin,hsp-sp-csr:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - description: Phandle to HSP(High-Speed Peripheral) device
> +      - description: Offset of phy control register for internal
> +                     or external clock selection
> +      - description: Offset of AXI clock controller Low-Power request
> +                     register
> +      - description: Offset of register controlling TX/RX clock delay
> +    description: |
> +      A phandle to hsp-sp-csr with three arguments that configure
> +      HSP(High-Speed Peripheral) device. The argument one is the
> +      offset of phy control register for internal or external clock
> +      selection, the argument two is Offset of AXI clock controller
> +      Low-Power request register, the argument three is Offset of
> +      register controlling TX/RX clock delay.

Description is mostly redundant - it is already part of items:. Just say
here "HSP (High Spee....) device needed to configure clock selection,
clock low-power mode and clock delay." or something similar.

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
Best regards,
Krzysztof

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

[李志]

Dear Russell King,
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


> -----原始邮件-----
> 发件人: "Russell King (Oracle)" <linux@armlinux.org.uk>
> 发送时间:2025-08-27 16:24:51 (星期三)
> 收件人: weishangjuan@eswincomputing.com
> 抄送: devicetree@vger.kernel.org, andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, linux-arm-kernel@lists.infradead.org, mcoquelin.stm32@gmail.com, alexandre.torgue@foss.st.com, yong.liang.choong@linux.intel.com, vladimir.oltean@nxp.com, faizal.abdul.rahim@linux.intel.com, prabhakar.mahadev-lad.rj@bp.renesas.com, inochiama@gmail.com, jan.petrous@oss.nxp.com, jszhang@kernel.org, p.zabel@pengutronix.de, boon.khai.ng@altera.com, 0x1207@gmail.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, ningyu@eswincomputing.com, linmin@eswincomputing.com, lizhi2@eswincomputing.com
> 主题: Re: [PATCH v4 2/2] ethernet: eswin: Add eic7700 ethernet driver
> 
> On Wed, Aug 27, 2025 at 04:14:17PM +0800, weishangjuan@eswincomputing.com wrote:
> > +struct eic7700_qos_priv {
> > +	struct device *dev;
> > +	struct regmap *hsp_regmap;
> > +	struct clk *clk_tx;
> 
> Consider putting a pointer to the plat_dat here instead of clk_tx.
> 
> > +	struct clk *clk_axi;
> > +	struct clk *clk_cfg;
> 
> Consider moving these into plat_dat->clks.
> 
> > +	u32 tx_delay_ps;
> > +	u32 rx_delay_ps;
> > +};
> > +
> > +/**
> > + * eic7700_apply_delay - Update TX or RX delay bits in delay parameter value.
> > + * @delay_ps: Delay in picoseconds (capped at 12.7ns).
> > + * @reg:      Pointer to register value to modify.
> > + * @is_rx:    True for RX delay (bits 30:24), false for TX delay (bits 14:8).
> > + *
> > + * Converts delay to 0.1ns units, caps at 0x7F, and sets appropriate bits.
> > + * Only RX or TX bits are updated; other bits remain unchanged.
> > + */
> > +static inline void eic7700_apply_delay(u32 delay_ps, u32 *reg, bool is_rx)
> > +{
> > +	if (!reg)
> > +		return;
> > +
> > +	u32 val = min(delay_ps / 100, EIC7700_MAX_DELAY_UNIT);
> > +
> > +	if (is_rx) {
> > +		*reg &= ~EIC7700_ETH_RX_ADJ_DELAY;
> > +		*reg |= (val << 24) & EIC7700_ETH_RX_ADJ_DELAY;
> > +	} else {
> > +		*reg &= ~EIC7700_ETH_TX_ADJ_DELAY;
> > +		*reg |= (val << 8) & EIC7700_ETH_TX_ADJ_DELAY;
> > +	}
> > +}
> > +
> > +static int eic7700_clks_config(void *priv, bool enabled)
> > +{
> > +	struct eic7700_qos_priv *dwc = (struct eic7700_qos_priv *)priv;
> > +	int ret = 0;
> > +
> > +	if (enabled) {
> > +		ret = clk_prepare_enable(dwc->clk_tx);
> > +		if (ret < 0) {
> > +			dev_err(dwc->dev, "Failed to enable tx clock: %d\n",
> > +				ret);
> > +			goto err;
> > +		}
> > +
> > +		ret = clk_prepare_enable(dwc->clk_axi);
> > +		if (ret < 0) {
> > +			dev_err(dwc->dev, "Failed to enable axi clock: %d\n",
> > +				ret);
> > +			goto err_tx;
> > +		}
> > +
> > +		ret = clk_prepare_enable(dwc->clk_cfg);
> > +		if (ret < 0) {
> > +			dev_err(dwc->dev, "Failed to enable cfg clock: %d\n",
> > +				ret);
> > +			goto err_axi;
> > +		}
> 
> You can then use clk_bulk_prepare_enable() here without the complex
> unwinding if one enable fails.
> 
> > +	} else {
> > +		clk_disable_unprepare(dwc->clk_tx);
> > +		clk_disable_unprepare(dwc->clk_axi);
> > +		clk_disable_unprepare(dwc->clk_cfg);
> 
> and clk_bulk_disable_unprepare() here.
> 
> > +	}
> > +	return ret;
> > +
> > +err_axi:
> > +	clk_disable_unprepare(dwc->clk_axi);
> > +err_tx:
> > +	clk_disable_unprepare(dwc->clk_tx);
> > +err:
> > +	return ret;
> > +}
> > +
> > +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 eth_axi_lp_ctrl_offset;
> > +	u32 eth_phy_ctrl_offset;
> > +	u32 eth_phy_ctrl_regset;
> > +	u32 eth_rxd_dly_offset;
> > +	u32 eth_dly_param = 0;
> > +	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;
> > +
> > +	/* Read rx-internal-delay-ps and update rx_clk delay */
> > +	if (!of_property_read_u32(pdev->dev.of_node,
> > +				  "rx-internal-delay-ps",
> > +				  &dwc_priv->rx_delay_ps)) {
> > +		eic7700_apply_delay(dwc_priv->rx_delay_ps,
> > +				    &eth_dly_param, true);
> > +	} else {
> > +		dev_warn(&pdev->dev, "can't get rx-internal-delay-ps\n");
> > +	}
> > +
> > +	/* Read tx-internal-delay-ps and update tx_clk delay */
> > +	if (!of_property_read_u32(pdev->dev.of_node,
> > +				  "tx-internal-delay-ps",
> > +				  &dwc_priv->tx_delay_ps)) {
> > +		eic7700_apply_delay(dwc_priv->tx_delay_ps,
> > +				    &eth_dly_param, false);
> > +	} else {
> > +		dev_warn(&pdev->dev, "can't get tx-internal-delay-ps\n");
> > +	}
> > +
> > +	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",
> > +					 1, &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",
> > +					 2, &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);
> > +
> > +	ret = of_property_read_u32_index(pdev->dev.of_node,
> > +					 "eswin,hsp-sp-csr",
> > +					 3, &eth_rxd_dly_offset);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev,
> > +				ret,
> > +				"can't get eth_rxd_dly_offset\n");
> > +
> > +	regmap_write(dwc_priv->hsp_regmap, eth_rxd_dly_offset,
> > +		     eth_dly_param);
> > +
> > +	dwc_priv->clk_tx = devm_clk_get(&pdev->dev, "tx");
> > +	if (IS_ERR(dwc_priv->clk_tx))
> > +		return dev_err_probe(&pdev->dev,
> > +				PTR_ERR(dwc_priv->clk_tx),
> > +				"error getting tx clock\n");
> > +
> > +	dwc_priv->clk_axi = devm_clk_get(&pdev->dev, "axi");
> > +	if (IS_ERR(dwc_priv->clk_axi))
> > +		return dev_err_probe(&pdev->dev,
> > +				PTR_ERR(dwc_priv->clk_axi),
> > +				"error getting axi clock\n");
> > +
> > +	dwc_priv->clk_cfg = devm_clk_get(&pdev->dev, "cfg");
> > +	if (IS_ERR(dwc_priv->clk_cfg))
> > +		return dev_err_probe(&pdev->dev,
> > +				PTR_ERR(dwc_priv->clk_cfg),
> > +				"error getting cfg clock\n");
> 
> These then become devm_clk_bulk_get_all().
> 

Several technical points in the driver patches need clarification:
1. Our platform uses four clocks: stmmaceth, tx, axi, and cfg. The stmmaceth
   clock is automatically managed by stmmac_platform.c when
   "snps,dwc-qos-ethernet-4.10" is not configured in the DTS. Therefore,
   devm_clk_bulk_get_all() / devm_clk_bulk_get_all_enabled() are not applicable
   in our driver. For the "tx", "axi", and "cfg" clocks, we plan to refer to
   dwmac-rk.c in the next patch and use devm_clk_bulk_get_optional() to manage
   them in bulk. This approach allows automatic handling of probe and remove,
   simplifying the current enable/unwind logic. Is this correct?
2. With devm_clk_bulk_get_optional() managing the clocks in
   plat_stmmacenet_data, the eic7700_clks_config() API will access
   plat_stmmacenet_data via eic7700_qos_priv in the next patch. This
   design approach is appropriate, correct?

> > +
> > +	ret = eic7700_clks_config(dwc_priv, true);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev,
> > +				ret,
> > +				"error enable clock\n");
> 
> Maybe even devm_clk_bulk_get_all_enabled() which will omit this
> step...
> 
> > +
> > +	plat_dat->clk_tx_i = dwc_priv->clk_tx;
> > +	plat_dat->set_clk_tx_rate = stmmac_set_clk_tx_rate;
> > +	plat_dat->bsp_priv = dwc_priv;
> > +	plat_dat->clks_config = eic7700_clks_config;
> > +
> > +	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
> > +	if (ret) {
> > +		eic7700_clks_config(dwc_priv, false);
> 
> ... and means you don't need this call...
> 
> > +		return dev_err_probe(&pdev->dev,
> > +				ret,
> > +				"Failed to driver probe\n");
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static void eic7700_dwmac_remove(struct platform_device *pdev)
> > +{
> > +	struct eic7700_qos_priv *dwc_priv = get_stmmac_bsp_priv(&pdev->dev);
> > +
> > +	stmmac_pltfr_remove(pdev);
> > +	eic7700_clks_config(dwc_priv, false);
> > +}
> 
> ... and you can remove this function entirely ...
> 
> > +
> > +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 = eic7700_dwmac_remove,
> 
> ... replacing this with stmmac_pltfr_remove().
> 
> Thanks.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[李志]

Dear Russell King,

In last week’s reply, we addressed two questions.
The main question now is whether it makes sense to use
devm_clk_bulk_get_optional() in the next patch.

Could you please share your thoughts or specific suggestions on this?
Thanks for your time and guidance.

Li Zhi
Eswin Computing


> -----原始邮件-----
> 发件人: "Russell King (Oracle)" <linux@armlinux.org.uk>
> 发送时间:2025-08-27 16:24:51 (星期三)
> 收件人: weishangjuan@eswincomputing.com
> 抄送: devicetree@vger.kernel.org, andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, linux-arm-kernel@lists.infradead.org, mcoquelin.stm32@gmail.com, alexandre.torgue@foss.st.com, yong.liang.choong@linux.intel.com, vladimir.oltean@nxp.com, faizal.abdul.rahim@linux.intel.com, prabhakar.mahadev-lad.rj@bp.renesas.com, inochiama@gmail.com, jan.petrous@oss.nxp.com, jszhang@kernel.org, p.zabel@pengutronix.de, boon.khai.ng@altera.com, 0x1207@gmail.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, ningyu@eswincomputing.com, linmin@eswincomputing.com, lizhi2@eswincomputing.com
> 主题: Re: [PATCH v4 2/2] ethernet: eswin: Add eic7700 ethernet driver
> 
> On Wed, Aug 27, 2025 at 04:14:17PM +0800, weishangjuan@eswincomputing.com wrote:
> > +struct eic7700_qos_priv {
> > +	struct device *dev;
> > +	struct regmap *hsp_regmap;
> > +	struct clk *clk_tx;
> 
> Consider putting a pointer to the plat_dat here instead of clk_tx.
> 
> > +	struct clk *clk_axi;
> > +	struct clk *clk_cfg;
> 
> Consider moving these into plat_dat->clks.
> 
> > +	u32 tx_delay_ps;
> > +	u32 rx_delay_ps;
> > +};
> > +
> > +/**
> > + * eic7700_apply_delay - Update TX or RX delay bits in delay parameter value.
> > + * @delay_ps: Delay in picoseconds (capped at 12.7ns).
> > + * @reg:      Pointer to register value to modify.
> > + * @is_rx:    True for RX delay (bits 30:24), false for TX delay (bits 14:8).
> > + *
> > + * Converts delay to 0.1ns units, caps at 0x7F, and sets appropriate bits.
> > + * Only RX or TX bits are updated; other bits remain unchanged.
> > + */
> > +static inline void eic7700_apply_delay(u32 delay_ps, u32 *reg, bool is_rx)
> > +{
> > +	if (!reg)
> > +		return;
> > +
> > +	u32 val = min(delay_ps / 100, EIC7700_MAX_DELAY_UNIT);
> > +
> > +	if (is_rx) {
> > +		*reg &= ~EIC7700_ETH_RX_ADJ_DELAY;
> > +		*reg |= (val << 24) & EIC7700_ETH_RX_ADJ_DELAY;
> > +	} else {
> > +		*reg &= ~EIC7700_ETH_TX_ADJ_DELAY;
> > +		*reg |= (val << 8) & EIC7700_ETH_TX_ADJ_DELAY;
> > +	}
> > +}
> > +
> > +static int eic7700_clks_config(void *priv, bool enabled)
> > +{
> > +	struct eic7700_qos_priv *dwc = (struct eic7700_qos_priv *)priv;
> > +	int ret = 0;
> > +
> > +	if (enabled) {
> > +		ret = clk_prepare_enable(dwc->clk_tx);
> > +		if (ret < 0) {
> > +			dev_err(dwc->dev, "Failed to enable tx clock: %d\n",
> > +				ret);
> > +			goto err;
> > +		}
> > +
> > +		ret = clk_prepare_enable(dwc->clk_axi);
> > +		if (ret < 0) {
> > +			dev_err(dwc->dev, "Failed to enable axi clock: %d\n",
> > +				ret);
> > +			goto err_tx;
> > +		}
> > +
> > +		ret = clk_prepare_enable(dwc->clk_cfg);
> > +		if (ret < 0) {
> > +			dev_err(dwc->dev, "Failed to enable cfg clock: %d\n",
> > +				ret);
> > +			goto err_axi;
> > +		}
> 
> You can then use clk_bulk_prepare_enable() here without the complex
> unwinding if one enable fails.
> 
> > +	} else {
> > +		clk_disable_unprepare(dwc->clk_tx);
> > +		clk_disable_unprepare(dwc->clk_axi);
> > +		clk_disable_unprepare(dwc->clk_cfg);
> 
> and clk_bulk_disable_unprepare() here.
> 
> > +	}
> > +	return ret;
> > +
> > +err_axi:
> > +	clk_disable_unprepare(dwc->clk_axi);
> > +err_tx:
> > +	clk_disable_unprepare(dwc->clk_tx);
> > +err:
> > +	return ret;
> > +}
> > +
> > +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 eth_axi_lp_ctrl_offset;
> > +	u32 eth_phy_ctrl_offset;
> > +	u32 eth_phy_ctrl_regset;
> > +	u32 eth_rxd_dly_offset;
> > +	u32 eth_dly_param = 0;
> > +	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;
> > +
> > +	/* Read rx-internal-delay-ps and update rx_clk delay */
> > +	if (!of_property_read_u32(pdev->dev.of_node,
> > +				  "rx-internal-delay-ps",
> > +				  &dwc_priv->rx_delay_ps)) {
> > +		eic7700_apply_delay(dwc_priv->rx_delay_ps,
> > +				    &eth_dly_param, true);
> > +	} else {
> > +		dev_warn(&pdev->dev, "can't get rx-internal-delay-ps\n");
> > +	}
> > +
> > +	/* Read tx-internal-delay-ps and update tx_clk delay */
> > +	if (!of_property_read_u32(pdev->dev.of_node,
> > +				  "tx-internal-delay-ps",
> > +				  &dwc_priv->tx_delay_ps)) {
> > +		eic7700_apply_delay(dwc_priv->tx_delay_ps,
> > +				    &eth_dly_param, false);
> > +	} else {
> > +		dev_warn(&pdev->dev, "can't get tx-internal-delay-ps\n");
> > +	}
> > +
> > +	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",
> > +					 1, &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",
> > +					 2, &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);
> > +
> > +	ret = of_property_read_u32_index(pdev->dev.of_node,
> > +					 "eswin,hsp-sp-csr",
> > +					 3, &eth_rxd_dly_offset);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev,
> > +				ret,
> > +				"can't get eth_rxd_dly_offset\n");
> > +
> > +	regmap_write(dwc_priv->hsp_regmap, eth_rxd_dly_offset,
> > +		     eth_dly_param);
> > +
> > +	dwc_priv->clk_tx = devm_clk_get(&pdev->dev, "tx");
> > +	if (IS_ERR(dwc_priv->clk_tx))
> > +		return dev_err_probe(&pdev->dev,
> > +				PTR_ERR(dwc_priv->clk_tx),
> > +				"error getting tx clock\n");
> > +
> > +	dwc_priv->clk_axi = devm_clk_get(&pdev->dev, "axi");
> > +	if (IS_ERR(dwc_priv->clk_axi))
> > +		return dev_err_probe(&pdev->dev,
> > +				PTR_ERR(dwc_priv->clk_axi),
> > +				"error getting axi clock\n");
> > +
> > +	dwc_priv->clk_cfg = devm_clk_get(&pdev->dev, "cfg");
> > +	if (IS_ERR(dwc_priv->clk_cfg))
> > +		return dev_err_probe(&pdev->dev,
> > +				PTR_ERR(dwc_priv->clk_cfg),
> > +				"error getting cfg clock\n");
> 
> These then become devm_clk_bulk_get_all().
> 
> > +
> > +	ret = eic7700_clks_config(dwc_priv, true);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev,
> > +				ret,
> > +				"error enable clock\n");
> 
> Maybe even devm_clk_bulk_get_all_enabled() which will omit this
> step...
> 
> > +
> > +	plat_dat->clk_tx_i = dwc_priv->clk_tx;
> > +	plat_dat->set_clk_tx_rate = stmmac_set_clk_tx_rate;
> > +	plat_dat->bsp_priv = dwc_priv;
> > +	plat_dat->clks_config = eic7700_clks_config;
> > +
> > +	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
> > +	if (ret) {
> > +		eic7700_clks_config(dwc_priv, false);
> 
> ... and means you don't need this call...
> 
> > +		return dev_err_probe(&pdev->dev,
> > +				ret,
> > +				"Failed to driver probe\n");
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static void eic7700_dwmac_remove(struct platform_device *pdev)
> > +{
> > +	struct eic7700_qos_priv *dwc_priv = get_stmmac_bsp_priv(&pdev->dev);
> > +
> > +	stmmac_pltfr_remove(pdev);
> > +	eic7700_clks_config(dwc_priv, false);
> > +}
> 
> ... and you can remove this function entirely ...
> 
> > +
> > +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 = eic7700_dwmac_remove,
> 
> ... replacing this with stmmac_pltfr_remove().
> 
> Thanks.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!