From: Senchuan Zhang <zhangsenchuan@eswincomputing.com>

Add driver for the Eswin EIC7700 PCIe host controller, which is based on
the DesignWare PCIe core, IP revision 5.96a. The PCIe Gen.3 controller
supports a data rate of 8 GT/s and 4 channels, support INTx and MSI
interrupts.

Signed-off-by: Yu Ning <ningyu@eswincomputing.com>
Signed-off-by: Yanghui Ou <ouyanghui@eswincomputing.com>
Signed-off-by: Senchuan Zhang <zhangsenchuan@eswincomputing.com>
---
 drivers/pci/controller/dwc/Kconfig        |  11 +
 drivers/pci/controller/dwc/Makefile       |   1 +
 drivers/pci/controller/dwc/pcie-eic7700.c | 393 ++++++++++++++++++++++
 3 files changed, 405 insertions(+)
 create mode 100644 drivers/pci/controller/dwc/pcie-eic7700.c

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 519b59422b47..c837cb5947b6 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -93,6 +93,17 @@ config PCIE_BT1
 	  Enables support for the PCIe controller in the Baikal-T1 SoC to work
 	  in host mode. It's based on the Synopsys DWC PCIe v4.60a IP-core.
 
+config PCIE_EIC7700
+	tristate "Eswin EIC7700 PCIe controller"
+	depends on ARCH_ESWIN || COMPILE_TEST
+	depends on PCI_MSI
+	select PCIE_DW_HOST
+	help
+	  Say Y here if you want PCIe controller support for the Eswin EIC7700.
+	  The PCIe controller on EIC7700 is based on DesignWare hardware,
+	  enables support for the PCIe controller in the EIC7700 SoC to work in
+	  host mode.
+
 config PCI_IMX6
 	bool
 
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index 67ba59c02038..7c5a5186ea83 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
 obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
 obj-$(CONFIG_PCIE_AMD_MDB) += pcie-amd-mdb.o
 obj-$(CONFIG_PCIE_BT1) += pcie-bt1.o
+obj-$(CONFIG_PCIE_EIC7700) += pcie-eic7700.o
 obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
 obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
 obj-$(CONFIG_PCIE_FU740) += pcie-fu740.o
diff --git a/drivers/pci/controller/dwc/pcie-eic7700.c b/drivers/pci/controller/dwc/pcie-eic7700.c
new file mode 100644
index 000000000000..b9c49805fd2f
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-eic7700.c
@@ -0,0 +1,393 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ESWIN EIC7700 PCIe root complex driver
+ *
+ * Copyright 2025, Beijing ESWIN Computing Technology Co., Ltd.
+ *
+ * Authors: Yu Ning <ningyu@eswincomputing.com>
+ *          Senchuan Zhang <zhangsenchuan@eswincomputing.com>
+ *          Yanghui Ou <ouyanghui@eswincomputing.com>
+ */
+
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/resource.h>
+#include <linux/reset.h>
+#include <linux/types.h>
+
+#include "pcie-designware.h"
+
+/* ELBI registers */
+#define PCIEELBI_CTRL0_OFFSET		0x0
+#define PCIEELBI_STATUS0_OFFSET		0x100
+
+/* LTSSM register fields */
+#define PCIEELBI_APP_LTSSM_ENABLE	BIT(5)
+
+/* APP_HOLD_PHY_RST register fields */
+#define PCIEELBI_APP_HOLD_PHY_RST	BIT(6)
+
+/* PM_SEL_AUX_CLK register fields */
+#define PCIEELBI_PM_SEL_AUX_CLK		BIT(16)
+
+/* DEV_TYPE register fields */
+#define PCIEELBI_CTRL0_DEV_TYPE		GENMASK(3, 0)
+
+/* Vendor and device ID value */
+#define PCI_VENDOR_ID_ESWIN		0x1fe1
+#define PCI_DEVICE_ID_ESWIN		0x2030
+
+#define EIC7700_NUM_RSTS		ARRAY_SIZE(eic7700_pcie_rsts)
+
+static const char * const eic7700_pcie_rsts[] = {
+	"pwr",
+	"dbi",
+};
+
+struct eic7700_pcie_port {
+	struct list_head list;
+	struct reset_control *perst;
+	int num_lanes;
+};
+
+struct eic7700_pcie {
+	struct dw_pcie pci;
+	struct clk_bulk_data *clks;
+	struct reset_control_bulk_data resets[EIC7700_NUM_RSTS];
+	struct list_head ports;
+	int num_clks;
+};
+
+#define to_eic7700_pcie(x) dev_get_drvdata((x)->dev)
+
+static int eic7700_pcie_start_link(struct dw_pcie *pci)
+{
+	u32 val;
+
+	/* Enable LTSSM */
+	val = readl_relaxed(pci->elbi_base + PCIEELBI_CTRL0_OFFSET);
+	val |= PCIEELBI_APP_LTSSM_ENABLE;
+	writel_relaxed(val, pci->elbi_base + PCIEELBI_CTRL0_OFFSET);
+
+	return 0;
+}
+
+static bool eic7700_pcie_link_up(struct dw_pcie *pci)
+{
+	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	u16 val = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA);
+
+	return val & PCI_EXP_LNKSTA_DLLLA;
+}
+
+static int eic7700_pcie_perst_reset(struct eic7700_pcie_port *port,
+				    struct eic7700_pcie *pcie)
+{
+	int ret;
+
+	ret = reset_control_assert(port->perst);
+	if (ret) {
+		dev_err(pcie->pci.dev, "Failed to assert PERST#\n");
+		return ret;
+	}
+
+	/* Ensure that PERST# has been asserted for at least 100 ms */
+	msleep(PCIE_T_PVPERL_MS);
+
+	ret = reset_control_deassert(port->perst);
+	if (ret) {
+		dev_err(pcie->pci.dev, "Failed to deassert PERST#\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static void eic7700_pcie_assert(struct eic7700_pcie *pcie)
+{
+	struct eic7700_pcie_port *port;
+
+	list_for_each_entry(port, &pcie->ports, list)
+		reset_control_assert(port->perst);
+	reset_control_bulk_assert(EIC7700_NUM_RSTS, pcie->resets);
+}
+
+static int eic7700_pcie_parse_port(struct eic7700_pcie *pcie,
+				   struct device_node *node)
+{
+	struct device *dev = pcie->pci.dev;
+	struct eic7700_pcie_port *port;
+
+	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+
+	port->perst = of_reset_control_get_exclusive(node, "perst");
+	if (IS_ERR(port->perst)) {
+		dev_err(dev, "Failed to get PERST# reset\n");
+		return PTR_ERR(port->perst);
+	}
+
+	/*
+	 * TODO: Since the Root Port node is separated out by pcie devicetree,
+	 * the DWC core initialization code can't parse the num-lanes attribute
+	 * in the Root Port. Before entering the DWC core initialization code,
+	 * the platform driver code parses the Root Port node. The EIC7700 only
+	 * supports one Root Port node, and the num-lanes attribute is suitable
+	 * for the case of one Root Port.
+	 */
+	if (!of_property_read_u32(node, "num-lanes", &port->num_lanes))
+		pcie->pci.num_lanes = port->num_lanes;
+
+	INIT_LIST_HEAD(&port->list);
+	list_add_tail(&port->list, &pcie->ports);
+
+	return 0;
+}
+
+static int eic7700_pcie_parse_ports(struct eic7700_pcie *pcie)
+{
+	struct eic7700_pcie_port *port, *tmp;
+	struct device *dev = pcie->pci.dev;
+	int ret;
+
+	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
+		ret = eic7700_pcie_parse_port(pcie, of_port);
+		if (ret)
+			goto err_port;
+	}
+
+	return 0;
+
+err_port:
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+		list_del(&port->list);
+
+	return ret;
+}
+
+static int eic7700_pcie_host_init(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct eic7700_pcie *pcie = to_eic7700_pcie(pci);
+	struct eic7700_pcie_port *port;
+	u32 val;
+	int ret;
+
+	pcie->num_clks = devm_clk_bulk_get_all_enabled(pci->dev, &pcie->clks);
+	if (pcie->num_clks < 0)
+		return dev_err_probe(pci->dev, pcie->num_clks,
+				     "Failed to get pcie clocks\n");
+
+	/*
+	 * The PWR and DBI reset signals are respectively used to reset the
+	 * PCIe controller and the DBI register.
+	 *
+	 * The PERST# signal is a reset signal that simultaneously controls the
+	 * PCIe controller, PHY, and Endpoint. Before configuring the PHY, the
+	 * PERST# signal must first be deasserted.
+	 *
+	 * The external reference clock is supplied simultaneously to the PHY
+	 * and EP. When the PHY is configurable, the entire chip already has
+	 * stable power and reference clock. The PHY will be ready within 20ms
+	 * after writing app_hold_phy_rst register bit of ELBI register space.
+	 */
+	ret = reset_control_bulk_deassert(EIC7700_NUM_RSTS, pcie->resets);
+	if (ret) {
+		dev_err(pcie->pci.dev, "Failed to deassert resets\n");
+		return ret;
+	}
+
+	/* Configure Root Port type */
+	val = readl_relaxed(pci->elbi_base + PCIEELBI_CTRL0_OFFSET);
+	val &= ~PCIEELBI_CTRL0_DEV_TYPE;
+	val |= FIELD_PREP(PCIEELBI_CTRL0_DEV_TYPE, PCI_EXP_TYPE_ROOT_PORT);
+	writel_relaxed(val, pci->elbi_base + PCIEELBI_CTRL0_OFFSET);
+
+	list_for_each_entry(port, &pcie->ports, list) {
+		ret = eic7700_pcie_perst_reset(port, pcie);
+		if (ret)
+			goto err_perst;
+	}
+
+	/* Configure app_hold_phy_rst */
+	val = readl_relaxed(pci->elbi_base + PCIEELBI_CTRL0_OFFSET);
+	val &= ~PCIEELBI_APP_HOLD_PHY_RST;
+	writel_relaxed(val, pci->elbi_base + PCIEELBI_CTRL0_OFFSET);
+
+	/* The maximum waiting time for the clock switch lock is 20ms */
+	ret = readl_poll_timeout(pci->elbi_base + PCIEELBI_STATUS0_OFFSET, val,
+				 !(val & PCIEELBI_PM_SEL_AUX_CLK), 1000,
+				 20000);
+	if (ret) {
+		dev_err(pci->dev, "Timeout waiting for PM_SEL_AUX_CLK ready\n");
+		goto err_phy_init;
+	}
+
+	/*
+	 * Configure ESWIN VID:DID for Root Port as the default values are
+	 * invalid.
+	 */
+	dw_pcie_dbi_ro_wr_en(pci);
+	dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, PCI_VENDOR_ID_ESWIN);
+	dw_pcie_writew_dbi(pci, PCI_DEVICE_ID, PCI_DEVICE_ID_ESWIN);
+	dw_pcie_dbi_ro_wr_dis(pci);
+
+	return 0;
+
+err_phy_init:
+	list_for_each_entry(port, &pcie->ports, list)
+		reset_control_assert(port->perst);
+err_perst:
+	reset_control_bulk_assert(EIC7700_NUM_RSTS, pcie->resets);
+
+	return ret;
+}
+
+static const struct dw_pcie_host_ops eic7700_pcie_host_ops = {
+	.init = eic7700_pcie_host_init,
+};
+
+static const struct dw_pcie_ops dw_pcie_ops = {
+	.start_link = eic7700_pcie_start_link,
+	.link_up = eic7700_pcie_link_up,
+};
+
+static int eic7700_pcie_probe(struct platform_device *pdev)
+{
+	struct eic7700_pcie_port *port, *tmp;
+	struct device *dev = &pdev->dev;
+	struct eic7700_pcie *pcie;
+	struct dw_pcie *pci;
+	int ret, i;
+
+	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&pcie->ports);
+
+	pci = &pcie->pci;
+	pci->dev = dev;
+	pci->ops = &dw_pcie_ops;
+	pci->pp.ops = &eic7700_pcie_host_ops;
+
+	for (i = 0; i < EIC7700_NUM_RSTS; i++)
+		pcie->resets[i].id = eic7700_pcie_rsts[i];
+
+	ret = devm_reset_control_bulk_get_exclusive(dev, EIC7700_NUM_RSTS,
+						    pcie->resets);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get resets\n");
+
+	ret = eic7700_pcie_parse_ports(pcie);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to parse Root Port\n");
+
+	platform_set_drvdata(pdev, pcie);
+
+	pm_runtime_no_callbacks(dev);
+	devm_pm_runtime_enable(dev);
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0)
+		goto err_pm_runtime_put;
+
+	ret = dw_pcie_host_init(&pci->pp);
+	if (ret) {
+		dev_err(dev, "Failed to init host\n");
+		goto err_pm_runtime_put;
+	}
+
+	return 0;
+
+err_pm_runtime_put:
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+		reset_control_put(port->perst);
+		list_del(&port->list);
+	}
+	pm_runtime_put(dev);
+
+	return ret;
+}
+
+static int eic7700_pcie_suspend_noirq(struct device *dev)
+{
+	struct eic7700_pcie *pcie = dev_get_drvdata(dev);
+
+	/*
+	 * The ESWIN EIC7700 SoC lacks hardware support for the L2/L3 low-power
+	 * link states. It cannot enter the L2/L3 Ready state through the
+	 * PME_Turn_Off/PME_To_Ack handshake protocol. To avoid this problem,
+	 * the dw_pcie_suspend_noirq API is not used.
+	 */
+	eic7700_pcie_assert(pcie);
+	clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
+
+	return 0;
+}
+
+static int eic7700_pcie_resume_noirq(struct device *dev)
+{
+	struct eic7700_pcie *pcie = dev_get_drvdata(dev);
+	struct eic7700_pcie_port *port, *tmp;
+	int ret;
+
+	ret = eic7700_pcie_host_init(&pcie->pci.pp);
+	if (ret) {
+		dev_err(dev, "Host init failed: %d\n", ret);
+		goto err_init;
+	}
+
+	ret = dw_pcie_setup_rc(&pcie->pci.pp);
+	if (ret)
+		goto err_setup_rc;
+
+	ret = eic7700_pcie_start_link(&pcie->pci);
+	if (ret)
+		goto err_setup_rc;
+
+	dw_pcie_wait_for_link(&pcie->pci);
+
+	return 0;
+
+err_setup_rc:
+	eic7700_pcie_assert(pcie);
+err_init:
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+		reset_control_put(port->perst);
+		list_del(&port->list);
+	}
+
+	return ret;
+}
+
+DEFINE_NOIRQ_DEV_PM_OPS(eic7700_pcie_pm, eic7700_pcie_suspend_noirq,
+			eic7700_pcie_resume_noirq);
+
+static const struct of_device_id eic7700_pcie_of_match[] = {
+	{ .compatible = "eswin,eic7700-pcie" },
+	{},
+};
+
+static struct platform_driver eic7700_pcie_driver = {
+	.probe = eic7700_pcie_probe,
+	.driver = {
+		.name = "eic7700-pcie",
+		.of_match_table = eic7700_pcie_of_match,
+		.suppress_bind_attrs = true,
+		.pm = &eic7700_pcie_pm,
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+	},
+};
+builtin_platform_driver(eic7700_pcie_driver);
+
+MODULE_DESCRIPTION("Eswin EIC7700 PCIe host controller driver");
+MODULE_AUTHOR("Yu Ning <ningyu@eswincomputing.com>");
+MODULE_AUTHOR("Senchuan Zhang <zhangsenchuan@eswincomputing.com>");
+MODULE_AUTHOR("Yanghui Ou <ouyanghui@eswincomputing.com>");
+MODULE_LICENSE("GPL");
-- 
2.25.1

[+cc Niklas, list vs array of ports]

On Mon, Dec 29, 2025 at 07:32:07PM +0800, zhangsenchuan@eswincomputing.com wrote:
> From: Senchuan Zhang <zhangsenchuan@eswincomputing.com>
> 
> Add driver for the Eswin EIC7700 PCIe host controller, which is based on
> the DesignWare PCIe core, IP revision 5.96a. The PCIe Gen.3 controller
> supports a data rate of 8 GT/s and 4 channels, support INTx and MSI
> interrupts.

> +config PCIE_EIC7700
> +	tristate "Eswin EIC7700 PCIe controller"

> +/* Vendor and device ID value */
> +#define PCI_VENDOR_ID_ESWIN		0x1fe1
> +#define PCI_DEVICE_ID_ESWIN		0x2030

Usually the device name is a little more than just the vendor.  What
if Eswin ever makes a second device?

> +static int eic7700_pcie_parse_port(struct eic7700_pcie *pcie,
> +				   struct device_node *node)
> +{
> +	struct device *dev = pcie->pci.dev;
> +	struct eic7700_pcie_port *port;
> +
> +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return -ENOMEM;
> +
> +	port->perst = of_reset_control_get_exclusive(node, "perst");
> +	if (IS_ERR(port->perst)) {
> +		dev_err(dev, "Failed to get PERST# reset\n");
> +		return PTR_ERR(port->perst);
> +	}
> +
> +	/*
> +	 * TODO: Since the Root Port node is separated out by pcie devicetree,
> +	 * the DWC core initialization code can't parse the num-lanes attribute
> +	 * in the Root Port. Before entering the DWC core initialization code,
> +	 * the platform driver code parses the Root Port node. The EIC7700 only
> +	 * supports one Root Port node, and the num-lanes attribute is suitable
> +	 * for the case of one Root Port.
> +	 */
> +	if (!of_property_read_u32(node, "num-lanes", &port->num_lanes))
> +		pcie->pci.num_lanes = port->num_lanes;
> +
> +	INIT_LIST_HEAD(&port->list);
> +	list_add_tail(&port->list, &pcie->ports);

Niklas raised an interesting question about whether a list or an array
is the best data structure for the set of Root Ports:

  https://lore.kernel.org/r/aVvkmkd5mWPmxeiS@ryzen

Might have to iterate over the child nodes twice (once to count, again
for eic7700_pcie_parse_port()), but otherwise the array is probably
simpler code.

> +	return 0;
> +}
> +
> +static int eic7700_pcie_parse_ports(struct eic7700_pcie *pcie)
> +{
> +	struct eic7700_pcie_port *port, *tmp;
> +	struct device *dev = pcie->pci.dev;
> +	int ret;
> +
> +	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> +		ret = eic7700_pcie_parse_port(pcie, of_port);
> +		if (ret)
> +			goto err_port;
> +	}
> +
> +	return 0;
> +
> +err_port:
> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> +		list_del(&port->list);

Is some kind of reset_control_put() needed to match the
of_reset_control_get_exclusive() above?

> +static struct platform_driver eic7700_pcie_driver = {
> +	.probe = eic7700_pcie_probe,

This driver is tristate but has no .remove() callback.  Seems like it
should have one?

> +	.driver = {
> +		.name = "eic7700-pcie",
> +		.of_match_table = eic7700_pcie_of_match,
> +		.suppress_bind_attrs = true,
> +		.pm = &eic7700_pcie_pm,
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> +	},
> +};
> +builtin_platform_driver(eic7700_pcie_driver);

> Subject: Re: [PATCH v9 2/2] PCI: eic7700: Add Eswin PCIe host controller driver
> 
> [+cc Niklas, list vs array of ports]
> 
> On Mon, Dec 29, 2025 at 07:32:07PM +0800, zhangsenchuan@eswincomputing.com wrote:
> > From: Senchuan Zhang <zhangsenchuan@eswincomputing.com>
> > 
> > Add driver for the Eswin EIC7700 PCIe host controller, which is based on
> > the DesignWare PCIe core, IP revision 5.96a. The PCIe Gen.3 controller
> > supports a data rate of 8 GT/s and 4 channels, support INTx and MSI
> > interrupts.
> 
> > +config PCIE_EIC7700
> > +	tristate "Eswin EIC7700 PCIe controller"
> 
> > +/* Vendor and device ID value */
> > +#define PCI_VENDOR_ID_ESWIN		0x1fe1
> > +#define PCI_DEVICE_ID_ESWIN		0x2030
> 
> Usually the device name is a little more than just the vendor.  What
> if Eswin ever makes a second device?

Okey, thanks.
Perhaps it's a problem. Maybe PCI_DEVICE_ID_EIC7700 is better?

> 
> > +static int eic7700_pcie_parse_port(struct eic7700_pcie *pcie,
> > +				   struct device_node *node)
> > +{
> > +	struct device *dev = pcie->pci.dev;
> > +	struct eic7700_pcie_port *port;
> > +
> > +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> > +	if (!port)
> > +		return -ENOMEM;
> > +
> > +	port->perst = of_reset_control_get_exclusive(node, "perst");
> > +	if (IS_ERR(port->perst)) {
> > +		dev_err(dev, "Failed to get PERST# reset\n");
> > +		return PTR_ERR(port->perst);
> > +	}
> > +
> > +	/*
> > +	 * TODO: Since the Root Port node is separated out by pcie devicetree,
> > +	 * the DWC core initialization code can't parse the num-lanes attribute
> > +	 * in the Root Port. Before entering the DWC core initialization code,
> > +	 * the platform driver code parses the Root Port node. The EIC7700 only
> > +	 * supports one Root Port node, and the num-lanes attribute is suitable
> > +	 * for the case of one Root Port.
> > +	 */
> > +	if (!of_property_read_u32(node, "num-lanes", &port->num_lanes))
> > +		pcie->pci.num_lanes = port->num_lanes;
> > +
> > +	INIT_LIST_HEAD(&port->list);
> > +	list_add_tail(&port->list, &pcie->ports);
> 
> Niklas raised an interesting question about whether a list or an array
> is the best data structure for the set of Root Ports:
> 
>   https://lore.kernel.org/r/aVvkmkd5mWPmxeiS@ryzen
> 
> Might have to iterate over the child nodes twice (once to count, again
> for eic7700_pcie_parse_port()), but otherwise the array is probably
> simpler code.

After reading patch's comments, lists and arrays seem to be good choices,
I don't have any particularly good ideas for the time being. Anyway, this
is a very good patch that supports multiple Root Ports resolutions.

> 
> > +	return 0;
> > +}
> > +
> > +static int eic7700_pcie_parse_ports(struct eic7700_pcie *pcie)
> > +{
> > +	struct eic7700_pcie_port *port, *tmp;
> > +	struct device *dev = pcie->pci.dev;
> > +	int ret;
> > +
> > +	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> > +		ret = eic7700_pcie_parse_port(pcie, of_port);
> > +		if (ret)
> > +			goto err_port;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_port:
> > +	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> > +		list_del(&port->list);
> 
> Is some kind of reset_control_put() needed to match the
> of_reset_control_get_exclusive() above?

I only considered that there is currently only one Root Port. Maybe 
there will be multiple Root Ports in the future.

Perhaps this is the best:
list_for_each_entry_safe(port, tmp, &pcie->ports, list){
        if (!IS_ERR_OR_NULL(port->perst))
            reset_control_put(port->perst);
        list_del(&port->list);
}

> 
> > +static struct platform_driver eic7700_pcie_driver = {
> > +	.probe = eic7700_pcie_probe,
> 
> This driver is tristate but has no .remove() callback.  Seems like it
> should have one?

In v2 patch, I referred to Mani's comments and removed the .remove()
callback, as follows:
"Since this controller implements irqchip using the DWC core driver,
it is not safe to remove it during runtime."
https://lore.kernel.org/linux-pci/jghozurjqyhmtunivotitgs67h6xo4sb46qcycnbbwyvjcm4ek@vgq75olazmoi/

In addition, remove .remove() callback, because this driver has been 
modified to builtin_platform_driver and does not support HotPlug, 
therefore, the .remove() callback is not needed. Do you have any
better suggestions?

Kind regards,
Senchuan Zhang

> 
> > +	.driver = {
> > +		.name = "eic7700-pcie",
> > +		.of_match_table = eic7700_pcie_of_match,
> > +		.suppress_bind_attrs = true,
> > +		.pm = &eic7700_pcie_pm,
> > +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> > +	},
> > +};
> > +builtin_platform_driver(eic7700_pcie_driver);

On Tue, Jan 06, 2026 at 08:43:11PM +0800, zhangsenchuan wrote:
> > Subject: Re: [PATCH v9 2/2] PCI: eic7700: Add Eswin PCIe host controller driver
> > 
> > [+cc Niklas, list vs array of ports]
> > 
> > On Mon, Dec 29, 2025 at 07:32:07PM +0800, zhangsenchuan@eswincomputing.com wrote:
> > > From: Senchuan Zhang <zhangsenchuan@eswincomputing.com>
> > > 
> > > Add driver for the Eswin EIC7700 PCIe host controller, which is based on
> > > the DesignWare PCIe core, IP revision 5.96a. The PCIe Gen.3 controller
> > > supports a data rate of 8 GT/s and 4 channels, support INTx and MSI
> > > interrupts.
> > 
> > > +config PCIE_EIC7700
> > > +	tristate "Eswin EIC7700 PCIe controller"
> > 
> > > +/* Vendor and device ID value */
> > > +#define PCI_VENDOR_ID_ESWIN		0x1fe1
> > > +#define PCI_DEVICE_ID_ESWIN		0x2030
> > 
> > Usually the device name is a little more than just the vendor.  What
> > if Eswin ever makes a second device?
> 
> Okey, thanks.
> Perhaps it's a problem. Maybe PCI_DEVICE_ID_EIC7700 is better?
> 
> > 
> > > +static int eic7700_pcie_parse_port(struct eic7700_pcie *pcie,
> > > +				   struct device_node *node)
> > > +{
> > > +	struct device *dev = pcie->pci.dev;
> > > +	struct eic7700_pcie_port *port;
> > > +
> > > +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> > > +	if (!port)
> > > +		return -ENOMEM;
> > > +
> > > +	port->perst = of_reset_control_get_exclusive(node, "perst");
> > > +	if (IS_ERR(port->perst)) {
> > > +		dev_err(dev, "Failed to get PERST# reset\n");
> > > +		return PTR_ERR(port->perst);
> > > +	}
> > > +
> > > +	/*
> > > +	 * TODO: Since the Root Port node is separated out by pcie devicetree,
> > > +	 * the DWC core initialization code can't parse the num-lanes attribute
> > > +	 * in the Root Port. Before entering the DWC core initialization code,
> > > +	 * the platform driver code parses the Root Port node. The EIC7700 only
> > > +	 * supports one Root Port node, and the num-lanes attribute is suitable
> > > +	 * for the case of one Root Port.
> > > +	 */
> > > +	if (!of_property_read_u32(node, "num-lanes", &port->num_lanes))
> > > +		pcie->pci.num_lanes = port->num_lanes;
> > > +
> > > +	INIT_LIST_HEAD(&port->list);
> > > +	list_add_tail(&port->list, &pcie->ports);
> > 
> > Niklas raised an interesting question about whether a list or an array
> > is the best data structure for the set of Root Ports:
> > 
> >   https://lore.kernel.org/r/aVvkmkd5mWPmxeiS@ryzen
> > 
> > Might have to iterate over the child nodes twice (once to count, again
> > for eic7700_pcie_parse_port()), but otherwise the array is probably
> > simpler code.
> 
> After reading patch's comments, lists and arrays seem to be good choices,
> I don't have any particularly good ideas for the time being. Anyway, this
> is a very good patch that supports multiple Root Ports resolutions.
> 

I still prefer using lists for the reasons mentioned in that thread.

> > 
> > > +	return 0;
> > > +}
> > > +
> > > +static int eic7700_pcie_parse_ports(struct eic7700_pcie *pcie)
> > > +{
> > > +	struct eic7700_pcie_port *port, *tmp;
> > > +	struct device *dev = pcie->pci.dev;
> > > +	int ret;
> > > +
> > > +	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> > > +		ret = eic7700_pcie_parse_port(pcie, of_port);
> > > +		if (ret)
> > > +			goto err_port;
> > > +	}
> > > +
> > > +	return 0;
> > > +
> > > +err_port:
> > > +	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> > > +		list_del(&port->list);
> > 
> > Is some kind of reset_control_put() needed to match the
> > of_reset_control_get_exclusive() above?
> 
> I only considered that there is currently only one Root Port. Maybe 
> there will be multiple Root Ports in the future.
> 
> Perhaps this is the best:
> list_for_each_entry_safe(port, tmp, &pcie->ports, list){
>         if (!IS_ERR_OR_NULL(port->perst))

You don't need this check since reset_control_put() does it for you.

>             reset_control_put(port->perst);
>         list_del(&port->list);
> }
> 
> > 
> > > +static struct platform_driver eic7700_pcie_driver = {
> > > +	.probe = eic7700_pcie_probe,
> > 
> > This driver is tristate but has no .remove() callback.  Seems like it
> > should have one?
> 
> In v2 patch, I referred to Mani's comments and removed the .remove()
> callback, as follows:
> "Since this controller implements irqchip using the DWC core driver,
> it is not safe to remove it during runtime."
> https://lore.kernel.org/linux-pci/jghozurjqyhmtunivotitgs67h6xo4sb46qcycnbbwyvjcm4ek@vgq75olazmoi/
> 
> In addition, remove .remove() callback, because this driver has been 
> modified to builtin_platform_driver and does not support HotPlug, 
> therefore, the .remove() callback is not needed. Do you have any
> better suggestions?
> 

Yes, builtin_platform_driver() wouldn't allow the users to remove the module. So
remove() callback will become useless. The reason why this driver is tristate is
that it could be loaded from rootfs and not always statically built to the
kernel image.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

+cc Sumit

> On Tue, Jan 06, 2026 at 08:43:11PM +0800, zhangsenchuan wrote:
> > > Subject: Re: [PATCH v9 2/2] PCI: eic7700: Add Eswin PCIe host controller driver
> > > 
> > > [+cc Niklas, list vs array of ports]
> > > 
> > > On Mon, Dec 29, 2025 at 07:32:07PM +0800, zhangsenchuan@eswincomputing.com wrote:
> > > > From: Senchuan Zhang <zhangsenchuan@eswincomputing.com>
> > > > 
> > > > Add driver for the Eswin EIC7700 PCIe host controller, which is based on
> > > > the DesignWare PCIe core, IP revision 5.96a. The PCIe Gen.3 controller
> > > > supports a data rate of 8 GT/s and 4 channels, support INTx and MSI
> > > > interrupts.
> > > 
> > > > +static int eic7700_pcie_parse_port(struct eic7700_pcie *pcie,
> > > > +				   struct device_node *node)
> > > > +{
> > > > +	struct device *dev = pcie->pci.dev;
> > > > +	struct eic7700_pcie_port *port;
> > > > +
> > > > +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> > > > +	if (!port)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	port->perst = of_reset_control_get_exclusive(node, "perst");
> > > > +	if (IS_ERR(port->perst)) {
> > > > +		dev_err(dev, "Failed to get PERST# reset\n");
> > > > +		return PTR_ERR(port->perst);
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * TODO: Since the Root Port node is separated out by pcie devicetree,
> > > > +	 * the DWC core initialization code can't parse the num-lanes attribute
> > > > +	 * in the Root Port. Before entering the DWC core initialization code,
> > > > +	 * the platform driver code parses the Root Port node. The EIC7700 only
> > > > +	 * supports one Root Port node, and the num-lanes attribute is suitable
> > > > +	 * for the case of one Root Port.
> > > > +	 */
> > > > +	if (!of_property_read_u32(node, "num-lanes", &port->num_lanes))
> > > > +		pcie->pci.num_lanes = port->num_lanes;
> > > > +
> > > > +	INIT_LIST_HEAD(&port->list);
> > > > +	list_add_tail(&port->list, &pcie->ports);
> > > 
> > > Niklas raised an interesting question about whether a list or an array
> > > is the best data structure for the set of Root Ports:
> > > 
> > >   https://lore.kernel.org/r/aVvkmkd5mWPmxeiS@ryzen
> > > 
> > > Might have to iterate over the child nodes twice (once to count, again
> > > for eic7700_pcie_parse_port()), but otherwise the array is probably
> > > simpler code.
> > 
> > After reading patch's comments, lists and arrays seem to be good choices,
> > I don't have any particularly good ideas for the time being. Anyway, this
> > is a very good patch that supports multiple Root Ports resolutions.
> > 
> 
> I still prefer using lists for the reasons mentioned in that thread.

Lists and arrays do not have too obvious disadvantages. For me, i prefer
to use list. We can use the standard kernel linked list API, which has a 
rich set of helper functions. It is developer-friendly and does not require 
familiarity with other API.

In addition, I saw that the device nodes "amd,versal2-mdb-host.yaml" and 
"sophgo,sg2044-pcie.yaml" have nodes that are not Port. Will this affect 
the node traversal?

Kind regards,
Senchuan

> 
> > > 
> > > > +	return 0;
> > > > +}

On Tue, Jan 06, 2026 at 06:49:58PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Jan 06, 2026 at 08:43:11PM +0800, zhangsenchuan wrote:
> > > On Mon, Dec 29, 2025 at 07:32:07PM +0800, zhangsenchuan@eswincomputing.com wrote:
> > > > From: Senchuan Zhang <zhangsenchuan@eswincomputing.com>
> > > > 
> > > > Add driver for the Eswin EIC7700 PCIe host controller, which is based on
> > > > the DesignWare PCIe core, IP revision 5.96a. The PCIe Gen.3 controller
> > > > supports a data rate of 8 GT/s and 4 channels, support INTx and MSI
> > > > interrupts.
> > > 
> > > > +config PCIE_EIC7700
> > > > +	tristate "Eswin EIC7700 PCIe controller"
> > > 
> > > > +/* Vendor and device ID value */
> > > > +#define PCI_VENDOR_ID_ESWIN		0x1fe1
> > > > +#define PCI_DEVICE_ID_ESWIN		0x2030
> > > 
> > > Usually the device name is a little more than just the vendor.  What
> > > if Eswin ever makes a second device?
> > 
> > Okey, thanks.
> > Perhaps it's a problem. Maybe PCI_DEVICE_ID_EIC7700 is better?

Check pci_ids.h and follow the style used there.  Device ID macros
typically include both the vendor and the device.

> > > > +static struct platform_driver eic7700_pcie_driver = {
> > > > +	.probe = eic7700_pcie_probe,
> > > 
> > > This driver is tristate but has no .remove() callback.  Seems like it
> > > should have one?
> > 
> > In v2 patch, I referred to Mani's comments and removed the .remove()
> > callback, as follows:
> > "Since this controller implements irqchip using the DWC core driver,
> > it is not safe to remove it during runtime."
> > https://lore.kernel.org/linux-pci/jghozurjqyhmtunivotitgs67h6xo4sb46qcycnbbwyvjcm4ek@vgq75olazmoi/
> > 
> > In addition, remove .remove() callback, because this driver has been 
> > modified to builtin_platform_driver and does not support HotPlug, 
> > therefore, the .remove() callback is not needed. Do you have any
> > better suggestions?
> 
> Yes, builtin_platform_driver() wouldn't allow the users to remove
> the module. So remove() callback will become useless. The reason why
> this driver is tristate is that it could be loaded from rootfs and
> not always statically built to the kernel image.

This .remove() vs IRQ thing is a perennial issue and it's hard to know
what style new drivers should copy.

There are lots of DWC-based drivers that are tristate, implement
.remove(), and use module_platform_driver() (e.g., bt1, kirin,
tegra194, rcar-gen4, exynos, k1, stm32).  Is there something different
about the way they implement irqchip that makes .remove() safe?

On Tue, Jan 06, 2026 at 11:43:48AM -0600, Bjorn Helgaas wrote:
> On Tue, Jan 06, 2026 at 06:49:58PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Jan 06, 2026 at 08:43:11PM +0800, zhangsenchuan wrote:
> > > > On Mon, Dec 29, 2025 at 07:32:07PM +0800, zhangsenchuan@eswincomputing.com wrote:
> > > > > From: Senchuan Zhang <zhangsenchuan@eswincomputing.com>
> > > > > 
> > > > > Add driver for the Eswin EIC7700 PCIe host controller, which is based on
> > > > > the DesignWare PCIe core, IP revision 5.96a. The PCIe Gen.3 controller
> > > > > supports a data rate of 8 GT/s and 4 channels, support INTx and MSI
> > > > > interrupts.
> > > > 
> > > > > +config PCIE_EIC7700
> > > > > +	tristate "Eswin EIC7700 PCIe controller"
> > > > 
> > > > > +/* Vendor and device ID value */
> > > > > +#define PCI_VENDOR_ID_ESWIN		0x1fe1
> > > > > +#define PCI_DEVICE_ID_ESWIN		0x2030
> > > > 
> > > > Usually the device name is a little more than just the vendor.  What
> > > > if Eswin ever makes a second device?
> > > 
> > > Okey, thanks.
> > > Perhaps it's a problem. Maybe PCI_DEVICE_ID_EIC7700 is better?
> 
> Check pci_ids.h and follow the style used there.  Device ID macros
> typically include both the vendor and the device.
> 
> > > > > +static struct platform_driver eic7700_pcie_driver = {
> > > > > +	.probe = eic7700_pcie_probe,
> > > > 
> > > > This driver is tristate but has no .remove() callback.  Seems like it
> > > > should have one?
> > > 
> > > In v2 patch, I referred to Mani's comments and removed the .remove()
> > > callback, as follows:
> > > "Since this controller implements irqchip using the DWC core driver,
> > > it is not safe to remove it during runtime."
> > > https://lore.kernel.org/linux-pci/jghozurjqyhmtunivotitgs67h6xo4sb46qcycnbbwyvjcm4ek@vgq75olazmoi/
> > > 
> > > In addition, remove .remove() callback, because this driver has been 
> > > modified to builtin_platform_driver and does not support HotPlug, 
> > > therefore, the .remove() callback is not needed. Do you have any
> > > better suggestions?
> > 
> > Yes, builtin_platform_driver() wouldn't allow the users to remove
> > the module. So remove() callback will become useless. The reason why
> > this driver is tristate is that it could be loaded from rootfs and
> > not always statically built to the kernel image.
> 
> This .remove() vs IRQ thing is a perennial issue and it's hard to know
> what style new drivers should copy.
> 
> There are lots of DWC-based drivers that are tristate, implement
> .remove(), and use module_platform_driver() (e.g., bt1, kirin,
> tegra194, rcar-gen4, exynos, k1, stm32).  Is there something different
> about the way they implement irqchip that makes .remove() safe?

Yes, there are differences currently. Mostly due to some drivers missed the
IRQ maintainers eyes when they got added (way before my involvement with PCI
controller drivers). I will fix them to maintain uniformity.

But the general undocumented rule is that if the controller driver implement
any irqchip (MSI/MSI-X/INTx), they should not get removed. So they can be
tristate, but builtin_platform_driver(). If they use any external irqchip
controller for receiving interrupts, they can safely be removed. This is the
case for dwc/pcie-stm32.

This prompts me to write the controller driver documentation that I was planning
for a while...

- Mani

-- 
மணிவண்ணன் சதாசிவம்

> > On Tue, Jan 06, 2026 at 08:43:11PM +0800, zhangsenchuan wrote:
> > > > On Mon, Dec 29, 2025 at 07:32:07PM +0800, zhangsenchuan@eswincomputing.com wrote:
> > > > > From: Senchuan Zhang <zhangsenchuan@eswincomputing.com>
> > > > > 
> > > > > Add driver for the Eswin EIC7700 PCIe host controller, which is based on
> > > > > the DesignWare PCIe core, IP revision 5.96a. The PCIe Gen.3 controller
> > > > > supports a data rate of 8 GT/s and 4 channels, support INTx and MSI
> > > > > interrupts.
> > > > 
> > > > > +config PCIE_EIC7700
> > > > > +	tristate "Eswin EIC7700 PCIe controller"
> > > > 
> > > > > +/* Vendor and device ID value */
> > > > > +#define PCI_VENDOR_ID_ESWIN		0x1fe1
> > > > > +#define PCI_DEVICE_ID_ESWIN		0x2030
> > > > 
> > > > Usually the device name is a little more than just the vendor.  What
> > > > if Eswin ever makes a second device?
> > > 
> > > Okey, thanks.
> > > Perhaps it's a problem. Maybe PCI_DEVICE_ID_EIC7700 is better?
> 
> Check pci_ids.h and follow the style used there.  Device ID macros
> typically include both the vendor and the device.

Okey, thanks.

> 
> > > > > +static struct platform_driver eic7700_pcie_driver = {
> > > > > +	.probe = eic7700_pcie_probe,
> > > > 
> > > > This driver is tristate but has no .remove() callback.  Seems like it
> > > > should have one?
> > > 
> > > In v2 patch, I referred to Mani's comments and removed the .remove()
> > > callback, as follows:
> > > "Since this controller implements irqchip using the DWC core driver,
> > > it is not safe to remove it during runtime."
> > > https://lore.kernel.org/linux-pci/jghozurjqyhmtunivotitgs67h6xo4sb46qcycnbbwyvjcm4ek@vgq75olazmoi/
> > > 
> > > In addition, remove .remove() callback, because this driver has been 
> > > modified to builtin_platform_driver and does not support HotPlug, 
> > > therefore, the .remove() callback is not needed. Do you have any
> > > better suggestions?
> > 
> > Yes, builtin_platform_driver() wouldn't allow the users to remove
> > the module. So remove() callback will become useless. The reason why
> > this driver is tristate is that it could be loaded from rootfs and
> > not always statically built to the kernel image.
> 
> This .remove() vs IRQ thing is a perennial issue and it's hard to know
> what style new drivers should copy.
> 
> There are lots of DWC-based drivers that are tristate, implement
> .remove(), and use module_platform_driver() (e.g., bt1, kirin,
> tegra194, rcar-gen4, exynos, k1, stm32).  Is there something different
> about the way they implement irqchip that makes .remove() safe?

Hi Bjorn, Mani,

The comments are as follows:
"You can make it tristate as you've used builtin_platform_driver() which
guarantees that this driver won't be removed once loaded."
https://lore.kernel.org/linux-pci/uijg47suvluvamftyxwc65kl34eo2eu2af2o5aia4nu45hanqc@grcr2bjgph2i/

Do not add the remove callback. It needs to be set to a bool：
In v6 patch, it was mentioned to set tristate. Now, after careful 
consideration, setting tristate can allow loading as a module, but the 
driver implementation does not have a remove function. If it exists in 
the form of a module, after testing, When insmod driver is followed by 
rmmod driver, the resources cannot be released, and problems will occur
when insmod driver is used again. So I think that if the remove callback
function is not provided in the form of builtin, it can only be set to 
bool.

Add the remove callback. It can make it tristate:
Questions about removing it during runtime. I don't have a very good idea.
I still don't quite understand why it's not safe. Could you explain it to 
me?

At present, refer to other manufacturers, i think there are two ways to 
achieve it.
1.Set a bool. Do not add the remove function, module loading is not 
allowed, and the driver currently does not support HotPlug.
2.Set a tristate, add .remove callback.

I think the first one might be better for me, because there is no need 
to add the remove function, my understanding might also be incorrect. 
Please review it for me. Thanks！

Kind regards,
Senchuan

On Fri, Jan 09, 2026 at 07:22:02PM +0800, zhangsenchuan wrote:
> > > On Tue, Jan 06, 2026 at 08:43:11PM +0800, zhangsenchuan wrote:
> > > > > On Mon, Dec 29, 2025 at 07:32:07PM +0800, zhangsenchuan@eswincomputing.com wrote:
> > > > > > From: Senchuan Zhang <zhangsenchuan@eswincomputing.com>
> > > > > > 
> > > > > > Add driver for the Eswin EIC7700 PCIe host controller, which is based on
> > > > > > the DesignWare PCIe core, IP revision 5.96a. The PCIe Gen.3 controller
> > > > > > supports a data rate of 8 GT/s and 4 channels, support INTx and MSI
> > > > > > interrupts.
> > > > > 
> > > > > > +config PCIE_EIC7700
> > > > > > +	tristate "Eswin EIC7700 PCIe controller"
> > > > > 
> > > > > > +/* Vendor and device ID value */
> > > > > > +#define PCI_VENDOR_ID_ESWIN		0x1fe1
> > > > > > +#define PCI_DEVICE_ID_ESWIN		0x2030
> > > > > 
> > > > > Usually the device name is a little more than just the vendor.  What
> > > > > if Eswin ever makes a second device?
> > > > 
> > > > Okey, thanks.
> > > > Perhaps it's a problem. Maybe PCI_DEVICE_ID_EIC7700 is better?
> > 
> > Check pci_ids.h and follow the style used there.  Device ID macros
> > typically include both the vendor and the device.
> 
> Okey, thanks.
> 
> > 
> > > > > > +static struct platform_driver eic7700_pcie_driver = {
> > > > > > +	.probe = eic7700_pcie_probe,
> > > > > 
> > > > > This driver is tristate but has no .remove() callback.  Seems like it
> > > > > should have one?
> > > > 
> > > > In v2 patch, I referred to Mani's comments and removed the .remove()
> > > > callback, as follows:
> > > > "Since this controller implements irqchip using the DWC core driver,
> > > > it is not safe to remove it during runtime."
> > > > https://lore.kernel.org/linux-pci/jghozurjqyhmtunivotitgs67h6xo4sb46qcycnbbwyvjcm4ek@vgq75olazmoi/
> > > > 
> > > > In addition, remove .remove() callback, because this driver has been 
> > > > modified to builtin_platform_driver and does not support HotPlug, 
> > > > therefore, the .remove() callback is not needed. Do you have any
> > > > better suggestions?
> > > 
> > > Yes, builtin_platform_driver() wouldn't allow the users to remove
> > > the module. So remove() callback will become useless. The reason why
> > > this driver is tristate is that it could be loaded from rootfs and
> > > not always statically built to the kernel image.
> > 
> > This .remove() vs IRQ thing is a perennial issue and it's hard to know
> > what style new drivers should copy.
> > 
> > There are lots of DWC-based drivers that are tristate, implement
> > .remove(), and use module_platform_driver() (e.g., bt1, kirin,
> > tegra194, rcar-gen4, exynos, k1, stm32).  Is there something different
> > about the way they implement irqchip that makes .remove() safe?
> 
> Hi Bjorn, Mani,
> 
> The comments are as follows:
> "You can make it tristate as you've used builtin_platform_driver() which
> guarantees that this driver won't be removed once loaded."
> https://lore.kernel.org/linux-pci/uijg47suvluvamftyxwc65kl34eo2eu2af2o5aia4nu45hanqc@grcr2bjgph2i/
> 
> Do not add the remove callback. It needs to be set to a bool：
> In v6 patch, it was mentioned to set tristate. Now, after careful 
> consideration, setting tristate can allow loading as a module, but the 
> driver implementation does not have a remove function. If it exists in 
> the form of a module, after testing, When insmod driver is followed by 
> rmmod driver, the resources cannot be released, and problems will occur
> when insmod driver is used again. So I think that if the remove callback
> function is not provided in the form of builtin, it can only be set to 
> bool.
> 
> Add the remove callback. It can make it tristate:
> Questions about removing it during runtime. I don't have a very good idea.
> I still don't quite understand why it's not safe. Could you explain it to 
> me?
> 

It is mostly due to IRQ disposal concern. Kernel's IRQ core gives no guarantee
that all the client drivers requested IRQs would've freed them before the
irqchip drivers like the controller drivers dispose the IRQs in their remove
callback.

More info here: https://lkml.iu.edu/hypermail/linux/kernel/2207.2/08367.html

> At present, refer to other manufacturers, i think there are two ways to 
> achieve it.
> 1.Set a bool. Do not add the remove function, module loading is not 
> allowed, and the driver currently does not support HotPlug.
> 2.Set a tristate, add .remove callback.
> 
> I think the first one might be better for me, because there is no need 
> to add the remove function, my understanding might also be incorrect. 
> Please review it for me. Thanks！
> 

I'd prefer to make it tristate, builtin_platform_driver() and not have
.remove() callback.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

> Subject: Re: Re: [PATCH v9 2/2] PCI: eic7700: Add Eswin PCIe host controller driver
> 
> > > On Tue, Jan 06, 2026 at 08:43:11PM +0800, zhangsenchuan wrote:
> > > > > On Mon, Dec 29, 2025 at 07:32:07PM +0800, zhangsenchuan@eswincomputing.com wrote:
> > > > > > From: Senchuan Zhang <zhangsenchuan@eswincomputing.com>
> > > > > > 
> > > > > > Add driver for the Eswin EIC7700 PCIe host controller, which is based on
> > > > > > the DesignWare PCIe core, IP revision 5.96a. The PCIe Gen.3 controller
> > > > > > supports a data rate of 8 GT/s and 4 channels, support INTx and MSI
> > > > > > interrupts.
> > > > > 
> > > > > > +config PCIE_EIC7700
> > > > > > +	tristate "Eswin EIC7700 PCIe controller"
> > > > > 
> > > > > > +/* Vendor and device ID value */
> > > > > > +#define PCI_VENDOR_ID_ESWIN		0x1fe1
> > > > > > +#define PCI_DEVICE_ID_ESWIN		0x2030
> > > > > 
> > > > > Usually the device name is a little more than just the vendor.  What
> > > > > if Eswin ever makes a second device?
> > > > 
> > > > Okey, thanks.
> > > > Perhaps it's a problem. Maybe PCI_DEVICE_ID_EIC7700 is better?
> > 
> > Check pci_ids.h and follow the style used there.  Device ID macros
> > typically include both the vendor and the device.
> 
> Okey, thanks.
> 
> > 
> > > > > > +static struct platform_driver eic7700_pcie_driver = {
> > > > > > +	.probe = eic7700_pcie_probe,
> > > > > 
> > > > > This driver is tristate but has no .remove() callback.  Seems like it
> > > > > should have one?
> > > > 
> > > > In v2 patch, I referred to Mani's comments and removed the .remove()
> > > > callback, as follows:
> > > > "Since this controller implements irqchip using the DWC core driver,
> > > > it is not safe to remove it during runtime."
> > > > https://lore.kernel.org/linux-pci/jghozurjqyhmtunivotitgs67h6xo4sb46qcycnbbwyvjcm4ek@vgq75olazmoi/
> > > > 
> > > > In addition, remove .remove() callback, because this driver has been 
> > > > modified to builtin_platform_driver and does not support HotPlug, 
> > > > therefore, the .remove() callback is not needed. Do you have any
> > > > better suggestions?
> > > 
> > > Yes, builtin_platform_driver() wouldn't allow the users to remove
> > > the module. So remove() callback will become useless. The reason why
> > > this driver is tristate is that it could be loaded from rootfs and
> > > not always statically built to the kernel image.
> > 
> > This .remove() vs IRQ thing is a perennial issue and it's hard to know
> > what style new drivers should copy.
> > 
> > There are lots of DWC-based drivers that are tristate, implement
> > .remove(), and use module_platform_driver() (e.g., bt1, kirin,
> > tegra194, rcar-gen4, exynos, k1, stm32).  Is there something different
> > about the way they implement irqchip that makes .remove() safe?
> 
> Hi Bjorn, Mani,
> 
> The comments are as follows:
> "You can make it tristate as you've used builtin_platform_driver() which
> guarantees that this driver won't be removed once loaded."
> https://lore.kernel.org/linux-pci/uijg47suvluvamftyxwc65kl34eo2eu2af2o5aia4nu45hanqc@grcr2bjgph2i/
> 
> Do not add the remove callback. It needs to be set to a bool：
> In v6 patch, it was mentioned to set tristate. Now, after careful 
> consideration, setting tristate can allow loading as a module, but the 
> driver implementation does not have a remove function. If it exists in 
> the form of a module, after testing, When insmod driver is followed by 
> rmmod driver, the resources cannot be released, and problems will occur
> when insmod driver is used again. So I think that if the remove callback
> function is not provided in the form of builtin, it can only be set to 
> bool.
> 
> Add the remove callback. It can make it tristate:
> Questions about removing it during runtime. I don't have a very good idea.
> I still don't quite understand why it's not safe. Could you explain it to 
> me?
> 
> At present, refer to other manufacturers, i think there are two ways to 
> achieve it.
> 1.Set a bool. Do not add the remove function, module loading is not 
> allowed, and the driver currently does not support HotPlug.
> 2.Set a tristate, add .remove callback.
> 
> I think the first one might be better for me, because there is no need 
> to add the remove function, my understanding might also be incorrect. 
> Please review it for me. Thanks！
> 

Hi Bjorn, Mani,

Regarding the issue of whether to add the.remove callback, could you 
please help me review it again? Thanks very much!

By the way, for the patch that parses multiple Root Ports, will it be 
updated later? Do I need to wait for it to send the next version?

Kind regards,
Senchuan

On Wed, Jan 21, 2026 at 08:00:26PM +0800, zhangsenchuan wrote:
> > Subject: Re: Re: [PATCH v9 2/2] PCI: eic7700: Add Eswin PCIe host controller driver
> > 
> > > > On Tue, Jan 06, 2026 at 08:43:11PM +0800, zhangsenchuan wrote:
> > > > > > On Mon, Dec 29, 2025 at 07:32:07PM +0800, zhangsenchuan@eswincomputing.com wrote:
> > > > > > > From: Senchuan Zhang <zhangsenchuan@eswincomputing.com>
> > > > > > > 
> > > > > > > Add driver for the Eswin EIC7700 PCIe host controller, which is based on
> > > > > > > the DesignWare PCIe core, IP revision 5.96a. The PCIe Gen.3 controller
> > > > > > > supports a data rate of 8 GT/s and 4 channels, support INTx and MSI
> > > > > > > interrupts.
> > > > > > 
> > > > > > > +config PCIE_EIC7700
> > > > > > > +	tristate "Eswin EIC7700 PCIe controller"
> > > > > > 
> > > > > > > +/* Vendor and device ID value */
> > > > > > > +#define PCI_VENDOR_ID_ESWIN		0x1fe1
> > > > > > > +#define PCI_DEVICE_ID_ESWIN		0x2030
> > > > > > 
> > > > > > Usually the device name is a little more than just the vendor.  What
> > > > > > if Eswin ever makes a second device?
> > > > > 
> > > > > Okey, thanks.
> > > > > Perhaps it's a problem. Maybe PCI_DEVICE_ID_EIC7700 is better?
> > > 
> > > Check pci_ids.h and follow the style used there.  Device ID macros
> > > typically include both the vendor and the device.
> > 
> > Okey, thanks.
> > 
> > > 
> > > > > > > +static struct platform_driver eic7700_pcie_driver = {
> > > > > > > +	.probe = eic7700_pcie_probe,
> > > > > > 
> > > > > > This driver is tristate but has no .remove() callback.  Seems like it
> > > > > > should have one?
> > > > > 
> > > > > In v2 patch, I referred to Mani's comments and removed the .remove()
> > > > > callback, as follows:
> > > > > "Since this controller implements irqchip using the DWC core driver,
> > > > > it is not safe to remove it during runtime."
> > > > > https://lore.kernel.org/linux-pci/jghozurjqyhmtunivotitgs67h6xo4sb46qcycnbbwyvjcm4ek@vgq75olazmoi/
> > > > > 
> > > > > In addition, remove .remove() callback, because this driver has been 
> > > > > modified to builtin_platform_driver and does not support HotPlug, 
> > > > > therefore, the .remove() callback is not needed. Do you have any
> > > > > better suggestions?
> > > > 
> > > > Yes, builtin_platform_driver() wouldn't allow the users to remove
> > > > the module. So remove() callback will become useless. The reason why
> > > > this driver is tristate is that it could be loaded from rootfs and
> > > > not always statically built to the kernel image.
> > > 
> > > This .remove() vs IRQ thing is a perennial issue and it's hard to know
> > > what style new drivers should copy.
> > > 
> > > There are lots of DWC-based drivers that are tristate, implement
> > > .remove(), and use module_platform_driver() (e.g., bt1, kirin,
> > > tegra194, rcar-gen4, exynos, k1, stm32).  Is there something different
> > > about the way they implement irqchip that makes .remove() safe?
> > 
> > Hi Bjorn, Mani,
> > 
> > The comments are as follows:
> > "You can make it tristate as you've used builtin_platform_driver() which
> > guarantees that this driver won't be removed once loaded."
> > https://lore.kernel.org/linux-pci/uijg47suvluvamftyxwc65kl34eo2eu2af2o5aia4nu45hanqc@grcr2bjgph2i/
> > 
> > Do not add the remove callback. It needs to be set to a bool：
> > In v6 patch, it was mentioned to set tristate. Now, after careful 
> > consideration, setting tristate can allow loading as a module, but the 
> > driver implementation does not have a remove function. If it exists in 
> > the form of a module, after testing, When insmod driver is followed by 
> > rmmod driver, the resources cannot be released, and problems will occur
> > when insmod driver is used again. So I think that if the remove callback
> > function is not provided in the form of builtin, it can only be set to 
> > bool.
> > 
> > Add the remove callback. It can make it tristate:
> > Questions about removing it during runtime. I don't have a very good idea.
> > I still don't quite understand why it's not safe. Could you explain it to 
> > me?
> > 
> > At present, refer to other manufacturers, i think there are two ways to 
> > achieve it.
> > 1.Set a bool. Do not add the remove function, module loading is not 
> > allowed, and the driver currently does not support HotPlug.
> > 2.Set a tristate, add .remove callback.
> > 
> > I think the first one might be better for me, because there is no need 
> > to add the remove function, my understanding might also be incorrect. 
> > Please review it for me. Thanks！
> > 
> 
> Hi Bjorn, Mani,
> 
> Regarding the issue of whether to add the.remove callback, could you 
> please help me review it again? Thanks very much!
> 
> By the way, for the patch that parses multiple Root Ports, will it be 
> updated later? Do I need to wait for it to send the next version?
> 

If you are referring to the patch from Sumit [1], then no need to wait for it as
it is not a dependency for this driver.

- Mani

[1] https://lore.kernel.org/all/20260105-dt-parser-v1-0-b11c63cb5e2c@oss.qualcomm.com/

> Kind regards,
> Senchuan
> 

-- 
மணிவண்ணன் சதாசிவம்

Le 29/12/2025 à 12:32, zhangsenchuan@eswincomputing.com a écrit :
> From: Senchuan Zhang <zhangsenchuan@eswincomputing.com>
> 
> Add driver for the Eswin EIC7700 PCIe host controller, which is based on
> the DesignWare PCIe core, IP revision 5.96a. The PCIe Gen.3 controller
> supports a data rate of 8 GT/s and 4 channels, support INTx and MSI
> interrupts.
> 
> Signed-off-by: Yu Ning <ningyu@eswincomputing.com>
> Signed-off-by: Yanghui Ou <ouyanghui@eswincomputing.com>
> Signed-off-by: Senchuan Zhang <zhangsenchuan@eswincomputing.com>
> ---

Hi,

> +static int eic7700_pcie_host_init(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct eic7700_pcie *pcie = to_eic7700_pcie(pci);
> +	struct eic7700_pcie_port *port;
> +	u32 val;
> +	int ret;
> +
> +	pcie->num_clks = devm_clk_bulk_get_all_enabled(pci->dev, &pcie->clks);

Is this the correct place to call this function?

eic7700_pcie_host_init() is called from eic7700_pcie_resume_noirq() and 
calling a devm function from a resume function is really unusual and is 
likely to leak memory.

> +	if (pcie->num_clks < 0)
> +		return dev_err_probe(pci->dev, pcie->num_clks,
> +				     "Failed to get pcie clocks\n");
> +
> +	/*
> +	 * The PWR and DBI reset signals are respectively used to reset the
> +	 * PCIe controller and the DBI register.
> +	 *
> +	 * The PERST# signal is a reset signal that simultaneously controls the
> +	 * PCIe controller, PHY, and Endpoint. Before configuring the PHY, the
> +	 * PERST# signal must first be deasserted.
> +	 *
> +	 * The external reference clock is supplied simultaneously to the PHY
> +	 * and EP. When the PHY is configurable, the entire chip already has
> +	 * stable power and reference clock. The PHY will be ready within 20ms
> +	 * after writing app_hold_phy_rst register bit of ELBI register space.
> +	 */
> +	ret = reset_control_bulk_deassert(EIC7700_NUM_RSTS, pcie->resets);
> +	if (ret) {
> +		dev_err(pcie->pci.dev, "Failed to deassert resets\n");
> +		return ret;
> +	}
> +
> +	/* Configure Root Port type */
> +	val = readl_relaxed(pci->elbi_base + PCIEELBI_CTRL0_OFFSET);
> +	val &= ~PCIEELBI_CTRL0_DEV_TYPE;
> +	val |= FIELD_PREP(PCIEELBI_CTRL0_DEV_TYPE, PCI_EXP_TYPE_ROOT_PORT);
> +	writel_relaxed(val, pci->elbi_base + PCIEELBI_CTRL0_OFFSET);
> +
> +	list_for_each_entry(port, &pcie->ports, list) {
> +		ret = eic7700_pcie_perst_reset(port, pcie);
> +		if (ret)
> +			goto err_perst;
> +	}
> +
> +	/* Configure app_hold_phy_rst */
> +	val = readl_relaxed(pci->elbi_base + PCIEELBI_CTRL0_OFFSET);
> +	val &= ~PCIEELBI_APP_HOLD_PHY_RST;
> +	writel_relaxed(val, pci->elbi_base + PCIEELBI_CTRL0_OFFSET);
> +
> +	/* The maximum waiting time for the clock switch lock is 20ms */
> +	ret = readl_poll_timeout(pci->elbi_base + PCIEELBI_STATUS0_OFFSET, val,
> +				 !(val & PCIEELBI_PM_SEL_AUX_CLK), 1000,
> +				 20000);
> +	if (ret) {
> +		dev_err(pci->dev, "Timeout waiting for PM_SEL_AUX_CLK ready\n");
> +		goto err_phy_init;
> +	}
> +
> +	/*
> +	 * Configure ESWIN VID:DID for Root Port as the default values are
> +	 * invalid.
> +	 */
> +	dw_pcie_dbi_ro_wr_en(pci);
> +	dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, PCI_VENDOR_ID_ESWIN);
> +	dw_pcie_writew_dbi(pci, PCI_DEVICE_ID, PCI_DEVICE_ID_ESWIN);
> +	dw_pcie_dbi_ro_wr_dis(pci);
> +
> +	return 0;
> +
> +err_phy_init:
> +	list_for_each_entry(port, &pcie->ports, list)
> +		reset_control_assert(port->perst);
> +err_perst:
> +	reset_control_bulk_assert(EIC7700_NUM_RSTS, pcie->resets);
> +
> +	return ret;
> +}

...

> +DEFINE_NOIRQ_DEV_PM_OPS(eic7700_pcie_pm, eic7700_pcie_suspend_noirq,
> +			eic7700_pcie_resume_noirq);
> +
> +static const struct of_device_id eic7700_pcie_of_match[] = {
> +	{ .compatible = "eswin,eic7700-pcie" },
> +	{},

Nitpick: No need for trailing comma after a terminator.

> +};

CJ

> > From: Senchuan Zhang <zhangsenchuan@eswincomputing.com>
> > 
> > Add driver for the Eswin EIC7700 PCIe host controller, which is based on
> > the DesignWare PCIe core, IP revision 5.96a. The PCIe Gen.3 controller
> > supports a data rate of 8 GT/s and 4 channels, support INTx and MSI
> > interrupts.
> > 
> > Signed-off-by: Yu Ning <ningyu@eswincomputing.com>
> > Signed-off-by: Yanghui Ou <ouyanghui@eswincomputing.com>
> > Signed-off-by: Senchuan Zhang <zhangsenchuan@eswincomputing.com>
> > ---
> 
> Hi,
> 
> > +static int eic7700_pcie_host_init(struct dw_pcie_rp *pp)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +	struct eic7700_pcie *pcie = to_eic7700_pcie(pci);
> > +	struct eic7700_pcie_port *port;
> > +	u32 val;
> > +	int ret;
> > +
> > +	pcie->num_clks = devm_clk_bulk_get_all_enabled(pci->dev, &pcie->clks);
> 
> Is this the correct place to call this function?

Thanks for your suggestion.
There may be cases where memory is allocated but not released. I will fix it
in the next patch.

> 
> eic7700_pcie_host_init() is called from eic7700_pcie_resume_noirq() and 
> calling a devm function from a resume function is really unusual and is 
> likely to leak memory.
> 
> > +	if (pcie->num_clks < 0)
> > +		return dev_err_probe(pci->dev, pcie->num_clks,
> > +				     "Failed to get pcie clocks\n");
> > +
> > +	/*
> > +	 * The PWR and DBI reset signals are respectively used to reset the
> > +	 * PCIe controller and the DBI register.
> > +	 *
> > +	 * The PERST# signal is a reset signal that simultaneously controls the
> > +	 * PCIe controller, PHY, and Endpoint. Before configuring the PHY, the
> > +	 * PERST# signal must first be deasserted.
> > +	 *
> > +	 * The external reference clock is supplied simultaneously to the PHY
> > +	 * and EP. When the PHY is configurable, the entire chip already has
> > +	 * stable power and reference clock. The PHY will be ready within 20ms
> > +	 * after writing app_hold_phy_rst register bit of ELBI register space.
> > +	 */
> > +	ret = reset_control_bulk_deassert(EIC7700_NUM_RSTS, pcie->resets);
> > +	if (ret) {
> > +		dev_err(pcie->pci.dev, "Failed to deassert resets\n");
> > +		return ret;
> > +	}
> > +
> > +	/* Configure Root Port type */
> > +	val = readl_relaxed(pci->elbi_base + PCIEELBI_CTRL0_OFFSET);
> > +	val &= ~PCIEELBI_CTRL0_DEV_TYPE;
> > +	val |= FIELD_PREP(PCIEELBI_CTRL0_DEV_TYPE, PCI_EXP_TYPE_ROOT_PORT);
> > +	writel_relaxed(val, pci->elbi_base + PCIEELBI_CTRL0_OFFSET);
> > +
> > +	list_for_each_entry(port, &pcie->ports, list) {
> > +		ret = eic7700_pcie_perst_reset(port, pcie);
> > +		if (ret)
> > +			goto err_perst;
> > +	}
> > +
> > +	/* Configure app_hold_phy_rst */
> > +	val = readl_relaxed(pci->elbi_base + PCIEELBI_CTRL0_OFFSET);
> > +	val &= ~PCIEELBI_APP_HOLD_PHY_RST;
> > +	writel_relaxed(val, pci->elbi_base + PCIEELBI_CTRL0_OFFSET);
> > +
> > +	/* The maximum waiting time for the clock switch lock is 20ms */
> > +	ret = readl_poll_timeout(pci->elbi_base + PCIEELBI_STATUS0_OFFSET, val,
> > +				 !(val & PCIEELBI_PM_SEL_AUX_CLK), 1000,
> > +				 20000);
> > +	if (ret) {
> > +		dev_err(pci->dev, "Timeout waiting for PM_SEL_AUX_CLK ready\n");
> > +		goto err_phy_init;
> > +	}
> > +
> > +	/*
> > +	 * Configure ESWIN VID:DID for Root Port as the default values are
> > +	 * invalid.
> > +	 */
> > +	dw_pcie_dbi_ro_wr_en(pci);
> > +	dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, PCI_VENDOR_ID_ESWIN);
> > +	dw_pcie_writew_dbi(pci, PCI_DEVICE_ID, PCI_DEVICE_ID_ESWIN);
> > +	dw_pcie_dbi_ro_wr_dis(pci);
> > +
> > +	return 0;
> > +
> > +err_phy_init:
> > +	list_for_each_entry(port, &pcie->ports, list)
> > +		reset_control_assert(port->perst);
> > +err_perst:
> > +	reset_control_bulk_assert(EIC7700_NUM_RSTS, pcie->resets);
> > +
> > +	return ret;
> > +}
> 
> ...
> 
> > +DEFINE_NOIRQ_DEV_PM_OPS(eic7700_pcie_pm, eic7700_pcie_suspend_noirq,
> > +			eic7700_pcie_resume_noirq);
> > +
> > +static const struct of_device_id eic7700_pcie_of_match[] = {
> > +	{ .compatible = "eswin,eic7700-pcie" },
> > +	{},
> 
> Nitpick: No need for trailing comma after a terminator.

Okey, thanks.

Kind regards,
Senchuan Zhang