[PATCH v3 2/2] PCI: EIC7700: Add Eswin PCIe host controller driver

zhangsenchuan@eswincomputing.com posted 2 patches 1 week, 1 day ago
[PATCH v3 2/2] PCI: EIC7700: Add Eswin PCIe host controller driver
Posted by zhangsenchuan@eswincomputing.com 1 week, 1 day ago
From: Senchuan Zhang <zhangsenchuan@eswincomputing.com>

Add driver for the Eswin EIC7700 PCIe host controller,the controller is
based on the DesignWare PCIe core, IP revision 6.00a 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 | 446 ++++++++++++++++++++++
 3 files changed, 458 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 ff6b6d9e18ec..8474bc6356f7 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -375,6 +375,17 @@ config PCI_EXYNOS
 	  hardware and therefore the driver re-uses the DesignWare core
 	  functions to implement the driver.
 
+config PCIE_EIC7700
+	bool "ESWIN 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.
+	  The PCIe controller on Eswin is based on DesignWare hardware,
+	  enables support for the PCIe controller in the Eswin SoC to
+	  work in host mode.
+
 config PCIE_FU740
 	bool "SiFive FU740 PCIe controller"
 	depends on PCI_MSI
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index 6919d27798d1..97b2ac4eb949 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_PCIE_AMD_MDB) += pcie-amd-mdb.o
 obj-$(CONFIG_PCIE_BT1) += pcie-bt1.o
 obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
 obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
+obj-$(CONFIG_PCIE_EIC7700) += pcie-eic7700.o
 obj-$(CONFIG_PCIE_FU740) += pcie-fu740.o
 obj-$(CONFIG_PCI_IMX6) += pci-imx6.o
 obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.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..32da4a645bef
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-eic7700.c
@@ -0,0 +1,446 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ESWIN 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/resource.h>
+#include <linux/reset.h>
+#include <linux/types.h>
+
+#include "pcie-designware.h"
+
+/* PCIe top csr registers */
+#define PCIEMGMT_CTRL0_OFFSET		0x0
+#define PCIEMGMT_STATUS0_OFFSET		0x100
+
+/* LTSSM register fields */
+#define PCIEMGMT_APP_LTSSM_ENABLE	BIT(5)
+
+/* APP_HOLD_PHY_RST register fields */
+#define PCIEMGMT_APP_HOLD_PHY_RST	BIT(6)
+
+/* PM_SEL_AUX_CLK register fields */
+#define PCIEMGMT_PM_SEL_AUX_CLK		BIT(16)
+
+/* ROOT_PORT register fields */
+#define PCIEMGMT_CTRL0_ROOT_PORT_MASK	GENMASK(3, 0)
+
+/* Vendor and device id value */
+#define VENDOR_ID_VALUE			0x1fe1
+#define DEVICE_ID_VALUE			0x2030
+
+/* Disable MSI-X cap register fields */
+#define PCIE_MSIX_DISABLE_MASK		GENMASK(15, 8)
+
+struct eswin_pcie_data {
+	bool msix_cap;
+};
+
+struct eswin_pcie_port {
+	struct list_head list;
+	struct reset_control *perst;
+	int num_lanes;
+};
+
+struct eswin_pcie {
+	struct dw_pcie pci;
+	void __iomem *mgmt_base;
+	struct clk_bulk_data *clks;
+	struct reset_control *powerup_rst;
+	struct reset_control *cfg_rst;
+	struct list_head ports;
+	int num_clks;
+	bool suspended;
+	bool msix_cap;
+};
+
+#define to_eswin_pcie(x) dev_get_drvdata((x)->dev)
+
+static int eswin_pcie_start_link(struct dw_pcie *pci)
+{
+	struct eswin_pcie *pcie = to_eswin_pcie(pci);
+	u32 val;
+
+	/* Enable LTSSM */
+	val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
+	val |= PCIEMGMT_APP_LTSSM_ENABLE;
+	writel_relaxed(val, pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
+
+	return 0;
+}
+
+static bool eswin_pcie_link_up(struct dw_pcie *pci)
+{
+	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	u16 val = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
+
+	return val & PCI_EXP_LNKSTA_DLLLA;
+}
+
+static int eswin_pcie_deassert(struct eswin_pcie *pcie)
+{
+	int ret;
+
+	ret = reset_control_deassert(pcie->cfg_rst);
+	if (ret) {
+		dev_err(pcie->pci.dev, "Failed to deassert CFG#");
+		return ret;
+	}
+
+	ret = reset_control_deassert(pcie->powerup_rst);
+	if (ret) {
+		dev_err(pcie->pci.dev, "Failed to deassert POWERUP#");
+		goto err_powerup;
+	}
+
+	return 0;
+
+err_powerup:
+	reset_control_assert(pcie->cfg_rst);
+
+	return ret;
+}
+
+static void eswin_pcie_assert(struct eswin_pcie *pcie)
+{
+	reset_control_assert(pcie->powerup_rst);
+	reset_control_assert(pcie->cfg_rst);
+}
+
+static int eswin_pcie_perst_deassert(struct eswin_pcie_port *port,
+				     struct eswin_pcie *pcie)
+{
+	int ret;
+
+	ret = reset_control_assert(port->perst);
+	if (ret) {
+		dev_err(pcie->pci.dev, "Failed to assert PERST#");
+		goto err_perst;
+	}
+
+	/* 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#");
+		goto err_perst;
+	}
+
+	return 0;
+
+err_perst:
+	list_for_each_entry(port, &pcie->ports, list)
+		reset_control_put(port->perst);
+
+	return ret;
+}
+
+static int eswin_pcie_parse_port(struct eswin_pcie *pcie,
+				 struct device_node *node)
+{
+	struct device *dev = pcie->pci.dev;
+	struct eswin_pcie_port *port;
+
+	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+
+	port->perst = of_reset_control_get(node, "perst");
+	if (IS_ERR(port->perst)) {
+		dev_err(dev, "Failed to get perst reset\n");
+		return PTR_ERR(port->perst);
+	}
+
+	/*
+	 * Since the root port node is separated out by pcie devicetree, the
+	 * DWC core initialization code cannot 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.
+	*/
+	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 eswin_pcie_parse_ports(struct eswin_pcie *pcie)
+{
+	struct device *dev = pcie->pci.dev;
+	struct eswin_pcie_port *port, *tmp;
+	int ret;
+
+	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
+		ret = eswin_pcie_parse_port(pcie, of_port);
+		if (ret)
+			goto err_port;
+	}
+
+	return ret;
+
+err_port:
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+		list_del(&port->list);
+	return ret;
+}
+
+static void eswin_pcie_hide_broken_msix_cap(struct dw_pcie *pci)
+{
+	u16 offset, val;
+
+	/*
+	 * Hardware doesn't support MSI-X but it advertises MSI-X capability,
+	 * to avoid this problem, the MSI-X capability in the PCIe capabilities
+	 * linked-list needs to be disabled. Since the PCI Express capability
+	 * structure's next pointer points to the MSI-X capability, and the
+	 * MSI-X capability's next pointer is null (00H), so only the PCI
+	 * Express capability structure's next pointer needs to be set 00H.
+	 */
+	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	val = dw_pcie_readl_dbi(pci, offset);
+	val &= ~PCIE_MSIX_DISABLE_MASK;
+	dw_pcie_writel_dbi(pci, offset, val);
+}
+
+static int eswin_pcie_host_init(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct eswin_pcie *pcie = to_eswin_pcie(pci);
+	struct eswin_pcie_port *port;
+	u32 retries;
+	u8 msi_cap;
+	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");
+
+	ret = eswin_pcie_deassert(pcie);
+	if (ret)
+		return ret;
+
+	/* Configure root port type */
+	val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
+	val &= ~PCIEMGMT_CTRL0_ROOT_PORT_MASK;
+	writel_relaxed(val | PCI_EXP_TYPE_ROOT_PORT,
+		       pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
+
+	list_for_each_entry(port, &pcie->ports, list) {
+		ret = eswin_pcie_perst_deassert(port, pcie);
+			if (ret)
+				goto err_perst;
+	}
+
+	/* Configure app_hold_phy_rst */
+	val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
+	val &= ~PCIEMGMT_APP_HOLD_PHY_RST;
+	writel_relaxed(val, pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
+
+	/* The maximum waiting time for the clock switch lock is 20ms */
+	retries = 20;
+	do {
+		val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_STATUS0_OFFSET);
+		if (!(val & PCIEMGMT_PM_SEL_AUX_CLK))
+			break;
+		fsleep(1000);
+		retries--;
+	} while (retries);
+
+	if (!retries) {
+		dev_err(pci->dev, "Timeout waiting for PM_SEL_AUX_CLK ready\n");
+		ret = -ETIMEDOUT;
+		goto err_phy_init;
+	}
+
+	/*
+	 * Configure ESWIN VID:DID for Root Port as the default values are
+	 * invalid.
+	 */
+	dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, VENDOR_ID_VALUE);
+	dw_pcie_writew_dbi(pci, PCI_DEVICE_ID, DEVICE_ID_VALUE);
+
+	/* Configure support 32 MSI vectors */
+	msi_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
+	val = dw_pcie_readw_dbi(pci, msi_cap + PCI_MSI_FLAGS);
+	val &= ~PCI_MSI_FLAGS_QMASK;
+	val |= FIELD_PREP(PCI_MSI_FLAGS_QMASK, 5);
+	dw_pcie_writew_dbi(pci, msi_cap + PCI_MSI_FLAGS, val);
+
+	/* Configure disable MSI-X cap */
+	if (!pcie->msix_cap)
+		eswin_pcie_hide_broken_msix_cap(pci);
+
+	return 0;
+
+err_phy_init:
+	list_for_each_entry(port, &pcie->ports, list)
+		reset_control_assert(port->perst);
+err_perst:
+	eswin_pcie_assert(pcie);
+
+	return ret;
+}
+
+static const struct dw_pcie_host_ops eswin_pcie_host_ops = {
+	.init = eswin_pcie_host_init,
+};
+
+static const struct dw_pcie_ops dw_pcie_ops = {
+	.start_link = eswin_pcie_start_link,
+	.link_up = eswin_pcie_link_up,
+};
+
+static int eswin_pcie_probe(struct platform_device *pdev)
+{
+	const struct eswin_pcie_data *data;
+	struct eswin_pcie_port *port, *tmp;
+	struct device *dev = &pdev->dev;
+	struct eswin_pcie *pcie;
+	struct dw_pcie *pci;
+	int ret;
+
+	data = of_device_get_match_data(dev);
+	if (!data)
+		return dev_err_probe(dev, -EINVAL, "OF data missing\n");
+
+	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 = &eswin_pcie_host_ops;
+	pcie->msix_cap = data->msix_cap;
+
+	pcie->mgmt_base = devm_platform_ioremap_resource_byname(pdev, "mgmt");
+	if (IS_ERR(pcie->mgmt_base))
+		return dev_err_probe(dev, PTR_ERR(pcie->mgmt_base),
+				     "Failed to map mgmt registers\n");
+
+	pcie->powerup_rst = devm_reset_control_get(&pdev->dev, "powerup");
+	if (IS_ERR(pcie->powerup_rst))
+		return dev_err_probe(dev, PTR_ERR(pcie->powerup_rst),
+				     "Failed to get powerup reset\n");
+
+	pcie->cfg_rst = devm_reset_control_get(&pdev->dev, "cfg");
+	if (IS_ERR(pcie->cfg_rst))
+		return dev_err_probe(dev, PTR_ERR(pcie->cfg_rst),
+				     "Failed to get cfg reset\n");
+
+	ret = eswin_pcie_parse_ports(pcie);
+	if (ret)
+		dev_err_probe(pci->dev, ret,
+			      "Failed to parse Root Port: %d\n", ret);
+
+	platform_set_drvdata(pdev, pcie);
+
+	ret = dw_pcie_host_init(&pci->pp);
+	if (ret) {
+		dev_err(dev, "Failed to initialize host\n");
+		goto err_init;
+	}
+
+	return ret;
+
+err_init:
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+		list_del(&port->list);
+		reset_control_put(port->perst);
+	}
+	return ret;
+}
+
+static int eswin_pcie_suspend(struct device *dev)
+{
+	struct eswin_pcie *pcie = dev_get_drvdata(dev);
+	struct eswin_pcie_port *port;
+
+	/*
+	 * For controllers with active devices, resources are retained and
+	 * cannot be turned off.
+	 */
+	if (!dw_pcie_link_up(&pcie->pci)) {
+		list_for_each_entry(port, &pcie->ports, list)
+			reset_control_assert(port->perst);
+		eswin_pcie_assert(pcie);
+		clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
+		pcie->suspended = true;
+	}
+
+	return 0;
+}
+
+static int eswin_pcie_resume(struct device *dev)
+{
+	struct eswin_pcie *pcie = dev_get_drvdata(dev);
+	int ret;
+
+	if (!pcie->suspended)
+		return 0;
+
+	ret = eswin_pcie_host_init(&pcie->pci.pp);
+	if (ret) {
+		dev_err(dev, "Failed to init host: %d\n", ret);
+		return ret;
+	}
+
+	dw_pcie_setup_rc(&pcie->pci.pp);
+	eswin_pcie_start_link(&pcie->pci);
+	dw_pcie_wait_for_link(&pcie->pci);
+
+	pcie->suspended = false;
+
+	return 0;
+}
+
+static const struct dev_pm_ops eswin_pcie_pm_ops = {
+	NOIRQ_SYSTEM_SLEEP_PM_OPS(eswin_pcie_suspend, eswin_pcie_resume)
+};
+
+static const struct eswin_pcie_data eswin_7700_data = {
+	.msix_cap = false,
+};
+
+static const struct of_device_id eswin_pcie_of_match[] = {
+	{ .compatible = "eswin,eic7700-pcie", .data = &eswin_7700_data },
+	{},
+};
+
+static struct platform_driver eswin_pcie_driver = {
+	.probe = eswin_pcie_probe,
+	.driver = {
+		.name = "eic7700-pcie",
+		.of_match_table = eswin_pcie_of_match,
+		.suppress_bind_attrs = true,
+		.pm = &eswin_pcie_pm_ops,
+	},
+};
+builtin_platform_driver(eswin_pcie_driver);
+
+MODULE_DESCRIPTION("PCIe host controller driver for EIC7700 SoCs");
+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
Re: [PATCH v3 2/2] PCI: EIC7700: Add Eswin PCIe host controller driver
Posted by Frank Li 1 week, 1 day ago
On Tue, Sep 23, 2025 at 08:12:27PM +0800, zhangsenchuan@eswincomputing.com wrote:
> From: Senchuan Zhang <zhangsenchuan@eswincomputing.com>
>
> Add driver for the Eswin EIC7700 PCIe host controller,the controller is
> based on the DesignWare PCIe core, IP revision 6.00a 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 | 446 ++++++++++++++++++++++
>  3 files changed, 458 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 ff6b6d9e18ec..8474bc6356f7 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -375,6 +375,17 @@ config PCI_EXYNOS
>  	  hardware and therefore the driver re-uses the DesignWare core
>  	  functions to implement the driver.
>
...
> +
> +static void eswin_pcie_hide_broken_msix_cap(struct dw_pcie *pci)
> +{
> +	u16 offset, val;
> +
> +	/*
> +	 * Hardware doesn't support MSI-X but it advertises MSI-X capability,
> +	 * to avoid this problem, the MSI-X capability in the PCIe capabilities
> +	 * linked-list needs to be disabled. Since the PCI Express capability
> +	 * structure's next pointer points to the MSI-X capability, and the
> +	 * MSI-X capability's next pointer is null (00H), so only the PCI
> +	 * Express capability structure's next pointer needs to be set 00H.
> +	 */
> +	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	val = dw_pcie_readl_dbi(pci, offset);
> +	val &= ~PCIE_MSIX_DISABLE_MASK;

supposed PCIE_MSIX_DISABLE_MASK is standard defination for PCI_CAP_ID_EXP
register, it should be in pci.h

> +	dw_pcie_writel_dbi(pci, offset, val);
> +}
> +
> +static int eswin_pcie_host_init(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct eswin_pcie *pcie = to_eswin_pcie(pci);
> +	struct eswin_pcie_port *port;
> +	u32 retries;
> +	u8 msi_cap;
> +	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");
> +
> +	ret = eswin_pcie_deassert(pcie);
> +	if (ret)
> +		return ret;
> +
> +	/* Configure root port type */
> +	val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
> +	val &= ~PCIEMGMT_CTRL0_ROOT_PORT_MASK;
> +	writel_relaxed(val | PCI_EXP_TYPE_ROOT_PORT,
> +		       pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
> +
> +	list_for_each_entry(port, &pcie->ports, list) {
> +		ret = eswin_pcie_perst_deassert(port, pcie);
> +			if (ret)
> +				goto err_perst;
> +	}
> +
> +	/* Configure app_hold_phy_rst */
> +	val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
> +	val &= ~PCIEMGMT_APP_HOLD_PHY_RST;
> +	writel_relaxed(val, pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
> +
> +	/* The maximum waiting time for the clock switch lock is 20ms */
> +	retries = 20;
> +	do {
> +		val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_STATUS0_OFFSET);
> +		if (!(val & PCIEMGMT_PM_SEL_AUX_CLK))
> +			break;
> +		fsleep(1000);
> +		retries--;
> +	} while (retries);

use readl_poll_timeout()

> +
> +	if (!retries) {
> +		dev_err(pci->dev, "Timeout waiting for PM_SEL_AUX_CLK ready\n");
> +		ret = -ETIMEDOUT;
> +		goto err_phy_init;
> +	}
> +
> +	/*
> +	 * Configure ESWIN VID:DID for Root Port as the default values are
> +	 * invalid.
> +	 */
> +	dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, VENDOR_ID_VALUE);
> +	dw_pcie_writew_dbi(pci, PCI_DEVICE_ID, DEVICE_ID_VALUE);
> +
> +	/* Configure support 32 MSI vectors */
> +	msi_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
> +	val = dw_pcie_readw_dbi(pci, msi_cap + PCI_MSI_FLAGS);
> +	val &= ~PCI_MSI_FLAGS_QMASK;
> +	val |= FIELD_PREP(PCI_MSI_FLAGS_QMASK, 5);
> +	dw_pcie_writew_dbi(pci, msi_cap + PCI_MSI_FLAGS, val);
> +
> +	/* Configure disable MSI-X cap */
> +	if (!pcie->msix_cap)
> +		eswin_pcie_hide_broken_msix_cap(pci);
> +
> +	return 0;
> +
> +err_phy_init:
> +	list_for_each_entry(port, &pcie->ports, list)
> +		reset_control_assert(port->perst);
> +err_perst:
> +	eswin_pcie_assert(pcie);
> +
> +	return ret;
> +}
> +
...
> +
> +static int eswin_pcie_suspend(struct device *dev)
> +{
> +	struct eswin_pcie *pcie = dev_get_drvdata(dev);
> +	struct eswin_pcie_port *port;
> +
> +	/*
> +	 * For controllers with active devices, resources are retained and
> +	 * cannot be turned off.
> +	 */
> +	if (!dw_pcie_link_up(&pcie->pci)) {
> +		list_for_each_entry(port, &pcie->ports, list)
> +			reset_control_assert(port->perst);
> +		eswin_pcie_assert(pcie);
> +		clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
> +		pcie->suspended = true;
> +	}
> +
> +	return 0;
> +}

does dw_pcie_resume_noirq() work for you? If not, please update common
one.

> +
> +static int eswin_pcie_resume(struct device *dev)
> +{
> +	struct eswin_pcie *pcie = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (!pcie->suspended)
> +		return 0;
> +
> +	ret = eswin_pcie_host_init(&pcie->pci.pp);
> +	if (ret) {
> +		dev_err(dev, "Failed to init host: %d\n", ret);
> +		return ret;
> +	}
> +
> +	dw_pcie_setup_rc(&pcie->pci.pp);
> +	eswin_pcie_start_link(&pcie->pci);
> +	dw_pcie_wait_for_link(&pcie->pci);
> +
> +	pcie->suspended = false;
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops eswin_pcie_pm_ops = {
> +	NOIRQ_SYSTEM_SLEEP_PM_OPS(eswin_pcie_suspend, eswin_pcie_resume)
> +};
> +
> +static const struct eswin_pcie_data eswin_7700_data = {
> +	.msix_cap = false,
> +};
> +
> +static const struct of_device_id eswin_pcie_of_match[] = {
> +	{ .compatible = "eswin,eic7700-pcie", .data = &eswin_7700_data },
> +	{},
> +};
> +
> +static struct platform_driver eswin_pcie_driver = {
> +	.probe = eswin_pcie_probe,
> +	.driver = {
> +		.name = "eic7700-pcie",
> +		.of_match_table = eswin_pcie_of_match,
> +		.suppress_bind_attrs = true,
> +		.pm = &eswin_pcie_pm_ops,
> +	},
> +};
> +builtin_platform_driver(eswin_pcie_driver);
> +
> +MODULE_DESCRIPTION("PCIe host controller driver for EIC7700 SoCs");
> +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
>
Re: Re: [PATCH v3 2/2] PCI: EIC7700: Add Eswin PCIe host controller driver
Posted by zhangsenchuan 1 week ago


> -----Original Messages-----
> From: "Frank Li" <Frank.li@nxp.com>
> Send time:Wednesday, 24/09/2025 03:47:16
> To: zhangsenchuan@eswincomputing.com
> Cc: bhelgaas@google.com, lpieralisi@kernel.org, kwilczynski@kernel.org, mani@kernel.org, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, linux-pci@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, p.zabel@pengutronix.de, johan+linaro@kernel.org, quic_schintav@quicinc.com, shradha.t@samsung.com, cassel@kernel.org, thippeswamy.havalige@amd.com, mayank.rana@oss.qualcomm.com, inochiama@gmail.com, ningyu@eswincomputing.com, linmin@eswincomputing.com, pinkesh.vaghela@einfochips.com, "Yanghui Ou" <ouyanghui@eswincomputing.com>
> Subject: Re: [PATCH v3 2/2] PCI: EIC7700: Add Eswin PCIe host controller driver
> 
> On Tue, Sep 23, 2025 at 08:12:27PM +0800, zhangsenchuan@eswincomputing.com wrote:
> > From: Senchuan Zhang <zhangsenchuan@eswincomputing.com>
> >
> > Add driver for the Eswin EIC7700 PCIe host controller,the controller is
> > based on the DesignWare PCIe core, IP revision 6.00a 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 | 446 ++++++++++++++++++++++
> >  3 files changed, 458 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 ff6b6d9e18ec..8474bc6356f7 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -375,6 +375,17 @@ config PCI_EXYNOS
> >  	  hardware and therefore the driver re-uses the DesignWare core
> >  	  functions to implement the driver.
> >
> ...
> > +
> > +static void eswin_pcie_hide_broken_msix_cap(struct dw_pcie *pci)
> > +{
> > +	u16 offset, val;
> > +
> > +	/*
> > +	 * Hardware doesn't support MSI-X but it advertises MSI-X capability,
> > +	 * to avoid this problem, the MSI-X capability in the PCIe capabilities
> > +	 * linked-list needs to be disabled. Since the PCI Express capability
> > +	 * structure's next pointer points to the MSI-X capability, and the
> > +	 * MSI-X capability's next pointer is null (00H), so only the PCI
> > +	 * Express capability structure's next pointer needs to be set 00H.
> > +	 */
> > +	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > +	val = dw_pcie_readl_dbi(pci, offset);
> > +	val &= ~PCIE_MSIX_DISABLE_MASK;
> 
> supposed PCIE_MSIX_DISABLE_MASK is standard defination for PCI_CAP_ID_EXP
> register, it should be in pci.h
> 
> > +	dw_pcie_writel_dbi(pci, offset, val);
> > +}
> > +
> > +static int eswin_pcie_host_init(struct dw_pcie_rp *pp)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +	struct eswin_pcie *pcie = to_eswin_pcie(pci);
> > +	struct eswin_pcie_port *port;
> > +	u32 retries;
> > +	u8 msi_cap;
> > +	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");
> > +
> > +	ret = eswin_pcie_deassert(pcie);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Configure root port type */
> > +	val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
> > +	val &= ~PCIEMGMT_CTRL0_ROOT_PORT_MASK;
> > +	writel_relaxed(val | PCI_EXP_TYPE_ROOT_PORT,
> > +		       pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
> > +
> > +	list_for_each_entry(port, &pcie->ports, list) {
> > +		ret = eswin_pcie_perst_deassert(port, pcie);
> > +			if (ret)
> > +				goto err_perst;
> > +	}
> > +
> > +	/* Configure app_hold_phy_rst */
> > +	val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
> > +	val &= ~PCIEMGMT_APP_HOLD_PHY_RST;
> > +	writel_relaxed(val, pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
> > +
> > +	/* The maximum waiting time for the clock switch lock is 20ms */
> > +	retries = 20;
> > +	do {
> > +		val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_STATUS0_OFFSET);
> > +		if (!(val & PCIEMGMT_PM_SEL_AUX_CLK))
> > +			break;
> > +		fsleep(1000);
> > +		retries--;
> > +	} while (retries);
> 
> use readl_poll_timeout()
> 
> > +
> > +	if (!retries) {
> > +		dev_err(pci->dev, "Timeout waiting for PM_SEL_AUX_CLK ready\n");
> > +		ret = -ETIMEDOUT;
> > +		goto err_phy_init;
> > +	}
> > +
> > +	/*
> > +	 * Configure ESWIN VID:DID for Root Port as the default values are
> > +	 * invalid.
> > +	 */
> > +	dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, VENDOR_ID_VALUE);
> > +	dw_pcie_writew_dbi(pci, PCI_DEVICE_ID, DEVICE_ID_VALUE);
> > +
> > +	/* Configure support 32 MSI vectors */
> > +	msi_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
> > +	val = dw_pcie_readw_dbi(pci, msi_cap + PCI_MSI_FLAGS);
> > +	val &= ~PCI_MSI_FLAGS_QMASK;
> > +	val |= FIELD_PREP(PCI_MSI_FLAGS_QMASK, 5);
> > +	dw_pcie_writew_dbi(pci, msi_cap + PCI_MSI_FLAGS, val);
> > +
> > +	/* Configure disable MSI-X cap */
> > +	if (!pcie->msix_cap)
> > +		eswin_pcie_hide_broken_msix_cap(pci);
> > +
> > +	return 0;
> > +
> > +err_phy_init:
> > +	list_for_each_entry(port, &pcie->ports, list)
> > +		reset_control_assert(port->perst);
> > +err_perst:
> > +	eswin_pcie_assert(pcie);
> > +
> > +	return ret;
> > +}
> > +
> ...
> > +
> > +static int eswin_pcie_suspend(struct device *dev)
> > +{
> > +	struct eswin_pcie *pcie = dev_get_drvdata(dev);
> > +	struct eswin_pcie_port *port;
> > +
> > +	/*
> > +	 * For controllers with active devices, resources are retained and
> > +	 * cannot be turned off.
> > +	 */
> > +	if (!dw_pcie_link_up(&pcie->pci)) {
> > +		list_for_each_entry(port, &pcie->ports, list)
> > +			reset_control_assert(port->perst);
> > +		eswin_pcie_assert(pcie);
> > +		clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
> > +		pcie->suspended = true;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> does dw_pcie_resume_noirq() work for you? If not, please update common
> one.

dear Frank

Thank you very much for your review comments.
Clarification:
The dw_pcie_suspend_noirq and dw_pcie_resume_noirq code is a nice wrapped 
collection function, but the dw_pcie_suspend_noirq function implements 
sending the PME_Turn_Off message. Notify the device to enter low power 
consumption and wait for it to enter the LTSSM_L2 state. Our hardware only
supports entering the L0/L1 state and does not support entering the D3code 
and L2/L3 states. It will cause mistakes and can't resume. Therefore, I 
cannot directly call the dw_pcie_suspend_noirq function here.

I saw that other vendor didn't use it. For example:
pci-exynos.c
pcie-intel-gw.c
pcie-qcom.c

What's your opinion? Do you allow me to implement the current code.
Looking forward to your reply. Thank you very much!

> 
> > +
> > +static int eswin_pcie_resume(struct device *dev)
> > +{
> > +	struct eswin_pcie *pcie = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	if (!pcie->suspended)
> > +		return 0;
> > +
> > +	ret = eswin_pcie_host_init(&pcie->pci.pp);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to init host: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	dw_pcie_setup_rc(&pcie->pci.pp);
> > +	eswin_pcie_start_link(&pcie->pci);
> > +	dw_pcie_wait_for_link(&pcie->pci);
> > +
> > +	pcie->suspended = false;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops eswin_pcie_pm_ops = {
> > +	NOIRQ_SYSTEM_SLEEP_PM_OPS(eswin_pcie_suspend, eswin_pcie_resume)
> > +};
> > +
> > +static const struct eswin_pcie_data eswin_7700_data = {
> > +	.msix_cap = false,
> > +};
> > +
> > +static const struct of_device_id eswin_pcie_of_match[] = {
> > +	{ .compatible = "eswin,eic7700-pcie", .data = &eswin_7700_data },
> > +	{},
> > +};
> > +
> > +static struct platform_driver eswin_pcie_driver = {
> > +	.probe = eswin_pcie_probe,
> > +	.driver = {
> > +		.name = "eic7700-pcie",
> > +		.of_match_table = eswin_pcie_of_match,
> > +		.suppress_bind_attrs = true,
> > +		.pm = &eswin_pcie_pm_ops,
> > +	},
> > +};
> > +builtin_platform_driver(eswin_pcie_driver);
> > +
> > +MODULE_DESCRIPTION("PCIe host controller driver for EIC7700 SoCs");
> > +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
> >
Re: [PATCH v3 2/2] PCI: EIC7700: Add Eswin PCIe host controller driver
Posted by Bjorn Helgaas 1 week, 1 day ago
On Tue, Sep 23, 2025 at 08:12:27PM +0800, zhangsenchuan@eswincomputing.com wrote:
> Add driver for the Eswin EIC7700 PCIe host controller,the controller is
> based on the DesignWare PCIe core, IP revision 6.00a The PCIe Gen.3
> controller supports a data rate of 8 GT/s and 4 channels, support INTX
> and MSI interrupts.

s/host controller,the controller is/host controller, which is/

Add period at end of first sentence.

> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -375,6 +375,17 @@ config PCI_EXYNOS
>  	  hardware and therefore the driver re-uses the DesignWare core
>  	  functions to implement the driver.
>  
> +config PCIE_EIC7700
> +	bool "ESWIN 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.
> +	  The PCIe controller on Eswin is based on DesignWare hardware,
> +	  enables support for the PCIe controller in the Eswin SoC to
> +	  work in host mode.

Alphabetize by vendor name so the kconfig menus stay sorted:

  Baikal-T1 PCIe controller
  ESWIN PCIe controller
  Freescale i.MX6/7/8 PCIe controller (host mode)

> +++ b/drivers/pci/controller/dwc/pcie-eic7700.c

> +/* Vendor and device id value */
> +#define VENDOR_ID_VALUE			0x1fe1
> +#define DEVICE_ID_VALUE			0x2030

Use something like this to match definitions in
include/linux/pci_ids.h, where this might eventually be moved if used
in other drivers:

  #define PCI_VENDOR_ID_ESWIN            0x1fe1

> +/* Disable MSI-X cap register fields */
> +#define PCIE_MSIX_DISABLE_MASK		GENMASK(15, 8)

I think this value has nothing to do with MSI-X; it's just the "Next
Capability Pointer" in the capability header, i.e., the
PCI_CAP_LIST_NEXT_MASK added here:

  https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?id=37d1ade89606

That commit is queued but not merged, so you can't use it yet.  If
this driver is merged after v6.17, you can switch to using it.

> +static int eswin_pcie_parse_ports(struct eswin_pcie *pcie)
> +{
> +	struct device *dev = pcie->pci.dev;
> +	struct eswin_pcie_port *port, *tmp;
> +	int ret;
> +
> +	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> +		ret = eswin_pcie_parse_port(pcie, of_port);
> +		if (ret)
> +			goto err_port;
> +	}
> +
> +	return ret;

"ret" is potentially uninitialized here, but you never get here if
eswin_pcie_parse_port() fails, so I think you should "return 0"
directly instead.

> +static int eswin_pcie_probe(struct platform_device *pdev)
> +{
> +	const struct eswin_pcie_data *data;
> +	struct eswin_pcie_port *port, *tmp;
> +	struct device *dev = &pdev->dev;
> +	struct eswin_pcie *pcie;
> +	struct dw_pcie *pci;
> +	int ret;
> +
> +	data = of_device_get_match_data(dev);
> +	if (!data)
> +		return dev_err_probe(dev, -EINVAL, "OF data missing\n");
> +
> +	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 = &eswin_pcie_host_ops;
> +	pcie->msix_cap = data->msix_cap;
> +
> +	pcie->mgmt_base = devm_platform_ioremap_resource_byname(pdev, "mgmt");
> +	if (IS_ERR(pcie->mgmt_base))
> +		return dev_err_probe(dev, PTR_ERR(pcie->mgmt_base),
> +				     "Failed to map mgmt registers\n");
> +
> +	pcie->powerup_rst = devm_reset_control_get(&pdev->dev, "powerup");
> +	if (IS_ERR(pcie->powerup_rst))
> +		return dev_err_probe(dev, PTR_ERR(pcie->powerup_rst),
> +				     "Failed to get powerup reset\n");
> +
> +	pcie->cfg_rst = devm_reset_control_get(&pdev->dev, "cfg");
> +	if (IS_ERR(pcie->cfg_rst))
> +		return dev_err_probe(dev, PTR_ERR(pcie->cfg_rst),
> +				     "Failed to get cfg reset\n");
> +
> +	ret = eswin_pcie_parse_ports(pcie);
> +	if (ret)
> +		dev_err_probe(pci->dev, ret,
> +			      "Failed to parse Root Port: %d\n", ret);
> +
> +	platform_set_drvdata(pdev, pcie);
> +
> +	ret = dw_pcie_host_init(&pci->pp);
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize host\n");
> +		goto err_init;
> +	}
> +
> +	return ret;

Can "return 0" here since we know the value.

> +err_init:
> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> +		list_del(&port->list);
> +		reset_control_put(port->perst);
> +	}
> +	return ret;
> +}
> +
> +static int eswin_pcie_suspend(struct device *dev)
> +{
> +	struct eswin_pcie *pcie = dev_get_drvdata(dev);
> +	struct eswin_pcie_port *port;
> +
> +	/*
> +	 * For controllers with active devices, resources are retained and
> +	 * cannot be turned off.
> +	 */
> +	if (!dw_pcie_link_up(&pcie->pci)) {
> +		list_for_each_entry(port, &pcie->ports, list)
> +			reset_control_assert(port->perst);
> +		eswin_pcie_assert(pcie);
> +		clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
> +		pcie->suspended = true;

I'm a little dubious about this since none of the other drivers check
dw_pcie_link_up().

It also seems a little bit racy since dw_pcie_link_up() can always
change after it's called.

And tracking pcie->suspended is also unusual if not unique.

Should dw_pcie_suspend_noirq() and dw_pcie_resume_noirq() be used
here?

> +	}
> +
> +	return 0;
> +}
> +
> +static int eswin_pcie_resume(struct device *dev)
> +{
> +	struct eswin_pcie *pcie = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (!pcie->suspended)
> +		return 0;
> +
> +	ret = eswin_pcie_host_init(&pcie->pci.pp);
> +	if (ret) {
> +		dev_err(dev, "Failed to init host: %d\n", ret);
> +		return ret;
> +	}
> +
> +	dw_pcie_setup_rc(&pcie->pci.pp);
> +	eswin_pcie_start_link(&pcie->pci);
> +	dw_pcie_wait_for_link(&pcie->pci);
> +
> +	pcie->suspended = false;
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops eswin_pcie_pm_ops = {
> +	NOIRQ_SYSTEM_SLEEP_PM_OPS(eswin_pcie_suspend, eswin_pcie_resume)

Suggest adding "_noirq" to the end of these function names since this
sets .suspend_noirq, .resume_noirq, etc.  Also will match other drivers.
Re: Re: [PATCH v3 2/2] PCI: EIC7700: Add Eswin PCIe host controller driver
Posted by zhangsenchuan 1 week ago


> -----Original Messages-----
> From: "Bjorn Helgaas" <helgaas@kernel.org>
> Send time:Wednesday, 24/09/2025 00:32:54
> To: zhangsenchuan@eswincomputing.com
> Cc: bhelgaas@google.com, lpieralisi@kernel.org, kwilczynski@kernel.org, mani@kernel.org, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, linux-pci@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, p.zabel@pengutronix.de, johan+linaro@kernel.org, quic_schintav@quicinc.com, shradha.t@samsung.com, cassel@kernel.org, thippeswamy.havalige@amd.com, mayank.rana@oss.qualcomm.com, inochiama@gmail.com, ningyu@eswincomputing.com, linmin@eswincomputing.com, pinkesh.vaghela@einfochips.com, "Yanghui Ou" <ouyanghui@eswincomputing.com>
> Subject: Re: [PATCH v3 2/2] PCI: EIC7700: Add Eswin PCIe host controller driver
> 
> On Tue, Sep 23, 2025 at 08:12:27PM +0800, zhangsenchuan@eswincomputing.com wrote:
> > Add driver for the Eswin EIC7700 PCIe host controller,the controller is
> > based on the DesignWare PCIe core, IP revision 6.00a The PCIe Gen.3
> > controller supports a data rate of 8 GT/s and 4 channels, support INTX
> > and MSI interrupts.
> 
> s/host controller,the controller is/host controller, which is/
> 
> Add period at end of first sentence.
> 
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -375,6 +375,17 @@ config PCI_EXYNOS
> >  	  hardware and therefore the driver re-uses the DesignWare core
> >  	  functions to implement the driver.
> >  
> > 
> > +static int eswin_pcie_suspend(struct device *dev)
> > +{
> > +	struct eswin_pcie *pcie = dev_get_drvdata(dev);
> > +	struct eswin_pcie_port *port;
> > +
> > +	/*
> > +	 * For controllers with active devices, resources are retained and
> > +	 * cannot be turned off.
> > +	 */
> > +	if (!dw_pcie_link_up(&pcie->pci)) {
> > +		list_for_each_entry(port, &pcie->ports, list)
> > +			reset_control_assert(port->perst);
> > +		eswin_pcie_assert(pcie);
> > +		clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
> > +		pcie->suspended = true;
> 
> I'm a little dubious about this since none of the other drivers check
> dw_pcie_link_up().

dear Bjorn

Thank you very much for your review.
Clarification:
The previous patch, Mani gave me advice: "So you want to power off the 
device even if it intends to be in D0?" Like NVMe."

I referred to the implementation of "pcie-qcom.c" and added the 
judgment of the dw_pcie_link_up function.

The following are mani's comments on fixing this issue:
/*
 * Turn OFF the resources only for controllers without active PCIe
 * devices. For controllers with active devices, the resources are kept
 * ON and the link is expected to be in L0/L1 (sub)states.
 *
 * Turning OFF the resources for controllers with active PCIe devices
 * will trigger access violation during the end of the suspend cycle,
 * as kernel tries to access the PCIe devices config space for masking
 * MSIs.
 *
 * Also, it is not desirable to put the link into L2/L3 state as that
 * implies VDD supply will be removed and the devices may go into
 * powerdown state. This will affect the lifetime of the storage devices
 * like NVMe.
 */

> 
> It also seems a little bit racy since dw_pcie_link_up() can always
> change after it's called.
> 
> And tracking pcie->suspended is also unusual if not unique.
> 
> Should dw_pcie_suspend_noirq() and dw_pcie_resume_noirq() be used
> here?

Clarification:
The dw_pcie_suspend_noirq and dw_pcie_resume_noirq code is a nice wrapped 
collection function, but the dw_pcie_suspend_noirq function implements 
sending the PME_Turn_Off message. Notify the device to enter low power 
consumption and wait for it to enter the LTSSM_L2 state. Our hardware only
supports entering the L0/L1 state and does not support entering the D3code 
and L2/L3 states. It will cause mistakes and can't resume. Therefore, I 
cannot directly call the dw_pcie_suspend_noirq function here.

> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int eswin_pcie_resume(struct device *dev)
> > +{
> > +	struct eswin_pcie *pcie = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	if (!pcie->suspended)
> > +		return 0;
> > +
> > +	ret = eswin_pcie_host_init(&pcie->pci.pp);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to init host: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	dw_pcie_setup_rc(&pcie->pci.pp);
> > +	eswin_pcie_start_link(&pcie->pci);
> > +	dw_pcie_wait_for_link(&pcie->pci);
> > +
> > +	pcie->suspended = false;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops eswin_pcie_pm_ops = {
> > +	NOIRQ_SYSTEM_SLEEP_PM_OPS(eswin_pcie_suspend, eswin_pcie_resume)
> 
> Suggest adding "_noirq" to the end of these function names since this
> sets .suspend_noirq, .resume_noirq, etc.  Also will match other drivers.