Message-ID: <31b9a79e.17e8.1997ba143a1.Coremail.zhangsenchuan@eswincomputing.com>
Date: Wed, 24 Sep 2025 20:09:50 +0800 (GMT+08:00)
From: zhangsenchuan <zhangsenchuan@...incomputing.com>
To: "Frank Li" <Frank.li@....com>
Cc: bhelgaas@...gle.com, lpieralisi@...nel.org, kwilczynski@...nel.org,
	mani@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, linux-pci@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	p.zabel@...gutronix.de, johan+linaro@...nel.org,
	quic_schintav@...cinc.com, shradha.t@...sung.com, cassel@...nel.org,
	thippeswamy.havalige@....com, mayank.rana@....qualcomm.com,
	inochiama@...il.com, ningyu@...incomputing.com,
	linmin@...incomputing.com, pinkesh.vaghela@...fochips.com,
	"Yanghui Ou" <ouyanghui@...incomputing.com>
Subject: Re: Re: [PATCH v3 2/2] PCI: EIC7700: Add Eswin PCIe host controller
 driver




> -----Original Messages-----
> From: "Frank Li" <Frank.li@....com>
> Send time:Wednesday, 24/09/2025 03:47:16
> To: zhangsenchuan@...incomputing.com
> Cc: bhelgaas@...gle.com, lpieralisi@...nel.org, kwilczynski@...nel.org, mani@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, linux-pci@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, p.zabel@...gutronix.de, johan+linaro@...nel.org, quic_schintav@...cinc.com, shradha.t@...sung.com, cassel@...nel.org, thippeswamy.havalige@....com, mayank.rana@....qualcomm.com, inochiama@...il.com, ningyu@...incomputing.com, linmin@...incomputing.com, pinkesh.vaghela@...fochips.com, "Yanghui Ou" <ouyanghui@...incomputing.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@...incomputing.com wrote:
> > From: Senchuan Zhang <zhangsenchuan@...incomputing.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@...incomputing.com>
> > Signed-off-by: Yanghui Ou <ouyanghui@...incomputing.com>
> > Signed-off-by: Senchuan Zhang <zhangsenchuan@...incomputing.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@...incomputing.com>");
> > +MODULE_AUTHOR("Senchuan Zhang <zhangsenchuan@...incomputing.com>");
> > +MODULE_AUTHOR("Yanghui Ou <ouyanghui@...incomputing.com>");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.25.1
> >

Powered by blists - more mailing lists