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:28:49
> To: "Bjorn Helgaas" <helgaas@kernel.org>
> 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: "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.

Supplement：
Since hot plugging is not supported and the device does not support 
entering L2/L3 states, once the device's initial link is established, 
it will not be disconnected. dw_pcie_link_up() will not change. Only
when the device is not inserted will the link be disconnected, and 
at this time, the resource can be turned off.

> > 
> > And tracking pcie->suspended is also unusual if not unique.

Supplement：
I saw such usage in the functions dw_pcie_suspend_noirq() and 
dw_pcie_resume_noirq(), and then referred to it.

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

Best Regards,
Senchuan Zhang

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