On Fri, Aug 29, 2025 at 04:24:05PM +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.

Wrap to fill 75 columns.

Should say "ESWIN PCIe controller" to match other entries.

+	depends on PCI_MSI
+	depends on ARCH_ESWIN || COMPILE_TEST

Reorder these to match other entries, i.e.,

	depends on ARCH_ESWIN || COMPILE_TEST
	depends on PCI_MSI

+	select PCIE_DW_HOST
+	help
+	  Enables support for the PCIe controller in the Eswin SoC
+	  The PCI controller on Eswin is based on DesignWare hardware
+	  It is a high-speed hardware bus standard used to connect
+	  processors with external devices. Say Y here if you want
+	  PCIe controller support for the ESWIN.

Alphabetize so the menuconfig entries remain sorted by vendor.

+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/resource.h>
+#include <linux/types.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/reset.h>
+#include <linux/pm_runtime.h>

Usually people sort these alphabetically.

+#define PCIE_PM_SEL_AUX_CLK BIT(16)
+#define PCIEMGMT_APP_LTSSM_ENABLE BIT(5)

Put these under the offset of the register that contains them so we
can tell the connections.

+#define PCIEMGMT_CTRL0_OFFSET 0x0
+#define PCIEMGMT_STATUS0_OFFSET 0x100
+
+#define PCIE_TYPE_DEV_VEND_ID 0x0

This looks like PCI_VENDOR_ID; use that instead.

+#define PCIE_DSP_PF0_MSI_CAP 0x50
+#define PCIE_NEXT_CAP_PTR 0x70

These look like fixed offsets in config space that should be
discovered by the usual method of traversing the capability lists,
e.g., dw_pcie_find_capability().

+#define DEVICE_CONTROL_DEVICE_STATUS 0x78

I don't think you need this at all (see below).  But if you do, the
use below should include PCI_EXP_DEVCTL (e.g., as an offset from the
start of the PCIe Capability) so grep can find it.

+#define PCIE_MSI_MULTIPLE_MSG_32 (0x5 << 17)
+#define PCIE_MSI_MULTIPLE_MSG_MASK (0x7 << 17)

Use PCI_MSI_FLAGS_QMASK instead.

+#define PCIEMGMT_LINKUP_STATE_VALIDATE ((0x11 << 2) | 0x3)
+#define PCIEMGMT_LINKUP_STATE_MASK 0xff

Line up all the values of these #defines.

+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 = dev_get_drvdata(pci->dev);
+	int ret;
+	u32 val;
+	u32 retries;
+
+	/* Fetch clocks */
+	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_power_on(pcie);
+	if (ret)
+		return ret;
+
+	/* set device type : rc */
+	val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
+	val &= 0xfffffff0;
+	writel_relaxed(val | 0x4, pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);

"rc" is not a device type (a Root Complex is not itself a PCI device
that appears in config space).  I think you're talking about a Root
Port, which is a PCIe device, so this should be
PCI_EXP_TYPE_ROOT_PORT.

+	ret = reset_control_assert(pcie->perst);
+	if (ret) {
+		dev_err(pci->dev, "perst assert signal is invalid");
+		goto err_perst;
+	}
+	msleep(100);

This sleep needs a comment (if it's specific to eic7700) or a standard
#define from drivers/pci/pci.h (if something from the PCIe spec).

+	ret = reset_control_deassert(pcie->perst);
+	if (ret) {
+		dev_err(pci->dev, "perst deassert signal is invalid");
+		goto err_perst;
+	}
+
+	/* app_hold_phy_rst */
+	val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
+	val &= ~(0x40);
+	writel_relaxed(val, pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
+
+	/*
+	 * It takes at least 20ms to wait for the pcie
+	 * status register to be 0.

s/pcie/PCIe/ (follow spec usage in comments, messages, etc)

+	 */
+	retries = 30;
+	do {
+		val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_STATUS0_OFFSET);
+		if (!(val & PCIE_PM_SEL_AUX_CLK))
+			break;
+		usleep_range(1000, 1100);
+		retries--;
+	} while (retries);

This delay should also have a citation to eic7700 spec if it came from
there.  Suggest fsleep() instead of usleep_range().  The exact delay
doesn't look critical here.

+	if (!retries) {
+		dev_err(pci->dev, "No clock exist.\n");

Drop period at end of messages.

+		ret = -ENODEV;
+		goto err_clock;
+	}
+
+	/* config eswin vendor id and eic7700 device id */
+	dw_pcie_writel_dbi(pci, PCIE_TYPE_DEV_VEND_ID, 0x20301fe1);
+
+	/* lane fix config, real driver NOT need, default x4 */
+	val = dw_pcie_readl_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL);
+	val &= 0xffffff80;
+	val |= 0x44;
+	dw_pcie_writel_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL, val);
+
+	val = dw_pcie_readl_dbi(pci, DEVICE_CONTROL_DEVICE_STATUS);
+	val &= ~(0x7 << 5);
+	val |= (0x2 << 5);
+	dw_pcie_writel_dbi(pci, DEVICE_CONTROL_DEVICE_STATUS, val);

This sets MPS, which should be done by the PCI core, not by the
driver.

+	/*  config support 32 msi vectors */

Use "MSI" and "MSI-X" in comments, etc to match spec usage.

+	val = dw_pcie_readl_dbi(pci, PCIE_DSP_PF0_MSI_CAP);
+	val &= ~PCIE_MSI_MULTIPLE_MSG_MASK;
+	val |= PCIE_MSI_MULTIPLE_MSG_32;

Use FIELD_PREP() as in dw_pcie_ep_set_msi().

+	dw_pcie_writel_dbi(pci, PCIE_DSP_PF0_MSI_CAP, val);
+
+	/* disable msix cap */
+	val = dw_pcie_readl_dbi(pci, PCIE_NEXT_CAP_PTR);
+	val &= 0xffff00ff;
+	dw_pcie_writel_dbi(pci, PCIE_NEXT_CAP_PTR, val);
+
+	return 0;
+
+err_clock:
+	reset_control_assert(pcie->perst);
+err_perst:
+	eswin_pcie_power_off(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)
+{
+	struct device *dev = &pdev->dev;
+	struct dw_pcie *pci;
+	struct eswin_pcie *pcie;
+	int ret;
+
+	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	pci = &pcie->pci;
+	pci->dev = dev;
+	pci->ops = &dw_pcie_ops;
+	pci->pp.ops = &eswin_pcie_host_ops;
+
+	/* SiFive specific region: mgmt */
+	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 memory\n");
+
+	/* Fetch reset */
+	pcie->powerup_rst = devm_reset_control_get_optional(&pdev->dev,
+							    "powerup");
+	if (IS_ERR_OR_NULL(pcie->powerup_rst))
+		return dev_err_probe(dev, PTR_ERR(pcie->powerup_rst),
+				     "unable to get powerup reset\n");
+
+	pcie->cfg_rst = devm_reset_control_get_optional(&pdev->dev, "cfg");
+	if (IS_ERR_OR_NULL(pcie->cfg_rst))
+		return dev_err_probe(dev, PTR_ERR(pcie->cfg_rst),
+				     "unable to get cfg reset\n");
+
+	pcie->perst = devm_reset_control_get_optional(&pdev->dev, "pwren");

Why is this not called "perst" in devicetree?

+	if (IS_ERR_OR_NULL(pcie->perst))
+		return dev_err_probe(dev, PTR_ERR(pcie->perst),
+				     "unable to get perst reset\n");
+
+	platform_set_drvdata(pdev, pcie);
+
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		dev_err(dev, "pm_runtime_get_sync failed: %d\n", ret);
+		goto err_get_sync;
+	}
+
+	ret = dw_pcie_host_init(&pci->pp);
+	if (ret) {
+		dev_err(dev, "failed to initialize host: %d\n", ret);
+		goto err_host_init;
+	}
+
+	return ret;
+
+err_host_init:
+	pm_runtime_put_sync(dev);
+err_get_sync:
+	pm_runtime_disable(dev);
+	return ret;
+}