> -----Original Messages-----
> From: "Bjorn Helgaas" <helgaas@xxxxxxxxxx>
> Send time:Wednesday, 24/09/2025 00:32:54
> To: zhangsenchuan@xxxxxxxxxxxxxxxxxx
> Cc: bhelgaas@xxxxxxxxxx, lpieralisi@xxxxxxxxxx, kwilczynski@xxxxxxxxxx, mani@xxxxxxxxxx, robh@xxxxxxxxxx, krzk+dt@xxxxxxxxxx, conor+dt@xxxxxxxxxx, linux-pci@xxxxxxxxxxxxxxx, devicetree@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, p.zabel@xxxxxxxxxxxxxx, johan+linaro@xxxxxxxxxx, quic_schintav@xxxxxxxxxxx, shradha.t@xxxxxxxxxxx, cassel@xxxxxxxxxx, thippeswamy.havalige@xxxxxxx, mayank.rana@xxxxxxxxxxxxxxxx, inochiama@xxxxxxxxx, ningyu@xxxxxxxxxxxxxxxxxx, linmin@xxxxxxxxxxxxxxxxxx, pinkesh.vaghela@xxxxxxxxxxxxxx, "Yanghui Ou" <ouyanghui@xxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxx 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.
