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

[zhangsenchuan <zhangsenchuan@eswincomputing.com>]

> -----Original Messages-----
> From: zhangsenchuan <zhangsenchuan@eswincomputing.com>
> Send time:Wednesday, 24/09/2025 20:09:50
> To: "Frank Li" <Frank.li@nxp.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: Re: [PATCH v3 2/2] PCI: EIC7700: Add Eswin PCIe host controller driver
> 
> 
> 
> 
> > -----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 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

Supplements：
If the dw_pcie_suspend_noirq() and dw_pcie_resume_noirq() functions 
need to be used, there are currently two solutions：

Plan 1：
In the dw_pcie_suspend_noirq function, add the suspport_L2 flag to 
determine whether to block the pme_turn_off and read_poll_timeout 
functions by checking whether L2 link state is supported.

	if (!pci->suspport_L2)
		goto skip_L2;
         
             .......

     skip_L2:
        dw_pcie_stop_link(pci);
	if (pci->pp.ops->deinit)
		pci->pp.ops->deinit(&pci->pp);

Plan 2：
Register the pme_turn_off and get_ltssm callback functions in 
our platform driver. The callback functions do not implement 
specific functions, Only display log information.

The following is the registered function：
static void eswin_pcie_pme_turn_off(struct dw_pcie_rp *pp)
{
	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);

	dev_info(pci->dev, "Can't send PME_TURN_OFF message\n");
}

static enum dw_pcie_ltssm eswin_pcie_get_ltssm(struct dw_pcie *pci)
{

	dev_info(pci->dev, "LTSSM_L2 not supported\n");

	return 0;
}

What's your opinion?  Which one do you think is better or are 
there any other better methods.

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;
> > > +}
> > > +

Best Regards,
Senchuan Zhang