Message-ID: <63945b54.9c7.19ac016ce19.Coremail.zhangsenchuan@eswincomputing.com>
Date: Wed, 26 Nov 2025 20:15:11 +0800 (GMT+08:00)
From: zhangsenchuan <zhangsenchuan@...incomputing.com>
To: "Manivannan Sadhasivam" <mani@...nel.org>
Cc: bhelgaas@...gle.com, krzk+dt@...nel.org, conor+dt@...nel.org,
	lpieralisi@...nel.org, kwilczynski@...nel.org, robh@...nel.org,
	p.zabel@...gutronix.de, jingoohan1@...il.com,
	gustavo.pimentel@...opsys.com, linux-pci@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	christian.bruel@...s.st.com, mayank.rana@....qualcomm.com,
	shradha.t@...sung.com, krishna.chundru@....qualcomm.com,
	thippeswamy.havalige@....com, inochiama@...il.com,
	ningyu@...incomputing.com, linmin@...incomputing.com,
	pinkesh.vaghela@...fochips.com, ouyanghui@...incomputing.com,
	Frank.li@....com
Subject: Re: Re: [PATCH v6 2/3] PCI: eic7700: Add Eswin PCIe host controller
 driver




> -----Original Messages-----
> From: "Manivannan Sadhasivam" <mani@...nel.org>
> Send time:Thursday, 20/11/2025 20:43:47
> To: zhangsenchuan@...incomputing.com
> Cc: bhelgaas@...gle.com, krzk+dt@...nel.org, conor+dt@...nel.org, lpieralisi@...nel.org, kwilczynski@...nel.org, robh@...nel.org, p.zabel@...gutronix.de, jingoohan1@...il.com, gustavo.pimentel@...opsys.com, linux-pci@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, christian.bruel@...s.st.com, mayank.rana@....qualcomm.com, shradha.t@...sung.com, krishna.chundru@....qualcomm.com, thippeswamy.havalige@....com, inochiama@...il.com, ningyu@...incomputing.com, linmin@...incomputing.com, pinkesh.vaghela@...fochips.com, ouyanghui@...incomputing.com, Frank.li@....com
> Subject: Re: [PATCH v6 2/3] PCI: eic7700: Add Eswin PCIe host controller driver
> 
> On Thu, Nov 20, 2025 at 06:12:06PM +0800, zhangsenchuan@...incomputing.com wrote:
> > From: Senchuan Zhang <zhangsenchuan@...incomputing.com>
> > 
> > Add driver for the Eswin EIC7700 PCIe host controller, which 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 | 387 ++++++++++++++++++++++
> >  3 files changed, 399 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 349d4657393c..66568efb324f 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
> > +	bool "Eswin EIC7700 PCIe controller"
> 
> You can make it tristate as you've used builtin_platform_driver() which
> guarantees that this driver won't be removed once loaded.

Okey, thanks

> 
> > +	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 7ae28f3b0fb3..04f751c49eba 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..239fdbc501fe
> > --- /dev/null
> > +++ b/drivers/pci/controller/dwc/pcie-eic7700.c
> > @@ -0,0 +1,387 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ESWIN EIC7700 PCIe root complex driver
> > + *
> > + * Copyright 2025, Beijing ESWIN Computing Technology Co., Ltd.
> > + *
> > + * Authors: Yu Ning <ningyu@...incomputing.com>
> > + *          Senchuan Zhang <zhangsenchuan@...incomputing.com>
> > + *          Yanghui Ou <ouyanghui@...incomputing.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"
> > +
> > +/* 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_data {
> > +	bool msix_cap;
> > +	bool no_suspport_L2;
> 
> support?

Okey, spelling mistake:)

mani, by the way, our controller cannot broadcast PME_Turn_Off. Previously,
i skip broadcast PME_Turn_Off in our controller code. Frank suggested that 
set the flag bit to skip in the public code. At present, do you think it's 
okay for me to add no_support_L2?

> > +static void eic7700_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 &= ~PCI_CAP_LIST_NEXT_MASK;
> > +	dw_pcie_writel_dbi(pci, offset, val);
> 
> I hate to enforce dependency for your series, but this is properly handled here:
> https://lore.kernel.org/linux-pci/20251109-remove_cap-v1-2-2208f46f4dc2@oss.qualcomm.com

Okey, then I can rely on this patch to solve the problem of the missing MSI-X. 
Thank you for the reminder.

> 
> > +}
> > +
> > +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;
> > +	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 = reset_control_bulk_deassert(EIC7700_NUM_RSTS, pcie->resets);
> 
> I think this is being called too early.

Before accessing the register. I need to deassert, otherwise, there will
be problems with register access.

> 
> > +	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_deassert(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;
> > +	}
> 
> You seem to be configuring the PHY reset and Aux clock, which should come before
> deasserting PERST# IMO. PERST# deassertion indicates that the power and clock
> are stable and the endpoint can start its operation. I don't know the impact of
> these configurations, but it is safe to do them earlier.

I think your understanding is correct. Unfortunately, in our hardware design, we 
need to deassert perst first before we can operate the configuration of the phy.

> 
> > +
> > +	/*
> > +	 * Configure ESWIN VID:DID for Root Port as the default values are
> > +	 * invalid.
> > +	 */
> > +	dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, PCI_VENDOR_ID_ESWIN);
> > +	dw_pcie_writew_dbi(pci, PCI_DEVICE_ID, PCI_DEVICE_ID_ESWIN);
> > +
> > +	/* 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);
> 
> So this configures the MSI for Root Port. But are you sure that the internal MSI
> controller could receive MSI from the Root Port?
> 
> Take a look: https://lore.kernel.org/linux-pci/20251109-remove_cap-v1-3-2208f46f4dc2@oss.qualcomm.com

Currently, our driver defaults to using the iMSI-RX module as MSI controller. 
If that's the case, i might need to rely on this patch.

> 
> > +
> > +	/* Configure disable MSI-X cap */
> > +	if (!pcie->data->msix_cap)
> > +		eic7700_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:
> > +	reset_control_bulk_assert(EIC7700_NUM_RSTS, pcie->resets);
> > +
> > +	return ret;
> > +}
> > +
> > +static void eic7700_pcie_host_deinit(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;
> > +
> > +	list_for_each_entry(port, &pcie->ports, list)
> > +		reset_control_assert(port->perst);
> > +	reset_control_bulk_assert(EIC7700_NUM_RSTS, pcie->resets);
> > +	clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
> > +}
> > +
> > +static const struct dw_pcie_host_ops eic7700_pcie_host_ops = {
> > +	.init = eic7700_pcie_host_init,
> > +	.deinit = eic7700_pcie_host_deinit,
> > +};
> > +
> > +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)
> > +{
> > +	const struct eic7700_pcie_data *data;
> > +	struct eic7700_pcie_port *port, *tmp;
> > +	struct device *dev = &pdev->dev;
> > +	struct eic7700_pcie *pcie;
> > +	struct dw_pcie *pci;
> > +	int ret, i;
> > +
> > +	data = of_device_get_match_data(dev);
> > +	if (!data)
> > +		return dev_err_probe(dev, -EINVAL, "OF data missing\n");
> 
> -ENODATA

Okey, thanks

> 
> > +
> > +	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;
> > +	pcie->data = data;
> > +	pci->no_suspport_L2 = pcie->data->no_suspport_L2;
> > +
> > +	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: %d\n", ret);
> > +
> > +	platform_set_drvdata(pdev, pcie);
> > +
> 
> You need to set the runtime PM status for the controller:
> 
> 	pm_runtime_no_callbacks(dev);
> 	devm_pm_runtime_enable(dev);
> 	ret = pm_runtime_get_sync(dev);
> 	if (ret < 0)
> 		goto err_pm_runtime_put;

Okey, thanks

> 
> > +	ret = dw_pcie_host_init(&pci->pp);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to initialize host\n");
> > +		goto err_init;
> > +	}
> > +
> > +	return 0;
> > +
> > +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 eic7700_pcie_suspend_noirq(struct device *dev)
> > +{
> > +	struct eic7700_pcie *pcie = dev_get_drvdata(dev);
> > +
> > +	return dw_pcie_suspend_noirq(&pcie->pci);
> > +}
> > +
> > +static int eic7700_pcie_resume_noirq(struct device *dev)
> > +{
> > +	struct eic7700_pcie *pcie = dev_get_drvdata(dev);
> > +
> > +	return dw_pcie_resume_noirq(&pcie->pci);
> > +}
> > +
> > +static const struct dev_pm_ops eic7700_pcie_pm_ops = {
> > +	NOIRQ_SYSTEM_SLEEP_PM_OPS(eic7700_pcie_suspend_noirq,
> > +				  eic7700_pcie_resume_noirq)
> > +};
> > +
> > +static const struct eic7700_pcie_data eic7700_data = {
> > +	.msix_cap = false,
> > +	.no_suspport_L2 = true,
> > +};
> > +
> > +static const struct of_device_id eic7700_pcie_of_match[] = {
> > +	{ .compatible = "eswin,eic7700-pcie", .data = &eic7700_data },
> > +	{},
> > +};
> > +
> > +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_ops,
> 
> Use:
> 		.probe_type = PROBE_PREFER_ASYNCHRONOUS,

Okey, thanks

> 
> to speed up the controller probe.
> 
> - Mani
> 
> -- 
> மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists