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

[zhangsenchuan <zhangsenchuan@eswincomputing.com>]

> -----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.