Re: [PATCH v3] PCI: qcom: Add system PM support

[Stephen Boyd <swboyd@chromium.org>]

Quoting Prasad Malisetty (2022-02-24 01:43:34)
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index c19cd506..f4a5e3c 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1616,6 +1621,96 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>         return ret;
>  }
>
> +static int qcom_pcie_send_pme_turnoff_msg(struct qcom_pcie *pcie)
> +{
> +       int ret;
> +       u32 val, poll_val;
> +       u64 l23_rdy_poll_timeout = 100000; /* microseconds */

unsigned long instead of u64? But why is it a local variable at all?
Just inline it in the one place it is used?

> +       struct dw_pcie *pci = pcie->pci;
> +       struct device *dev = pci->dev;
> +
> +       val = readl(pcie->elbi + PCIE20_ELBI_SYS_CTRL);
> +       val |= PCIE_PME_TURNOFF_MSG;
> +       writel(val, pcie->elbi + PCIE20_ELBI_SYS_CTRL);
> +
> +       ret = readl_poll_timeout((pcie->parf + PCIE20_PARF_PM_STTS), poll_val,
> +                       (poll_val & PCIE_PM_LINKST_IN_L2),
> +                       10000, l23_rdy_poll_timeout);
> +       if (!ret)
> +               dev_dbg(dev, "Device entered L23_Ready state\n");
> +       else
> +               dev_err(dev, "Device failed to enter L23_Ready. PM_STTS 0x%x\n",
> +                       readl_relaxed(pcie->parf + PCIE20_PARF_PM_STTS));
> +
> +       return ret;
> +}
> +
> +static void qcom_pcie_host_disable(struct qcom_pcie *pcie)
> +{
> +       qcom_ep_reset_assert(pcie);
> +
> +       /* Put PHY into POWER DOWN state */
> +       phy_power_off(pcie->phy);
> +
> +       writel(PHY_POWER_DOWN, pcie->parf + PCIE20_PARF_PHY_CTRL);
> +
> +       if (pcie->ops->post_deinit)
> +               pcie->ops->post_deinit(pcie);
> +
> +       /* Disable PCIe clocks and regulators */
> +       pcie->ops->deinit(pcie);
> +}
> +
> +static int __maybe_unused qcom_pcie_pm_suspend_noirq(struct device *dev)
> +{
> +       int ret;
> +       struct qcom_pcie *pcie = dev_get_drvdata(dev);
> +       struct dw_pcie *pci = pcie->pci;
> +
> +       if (!dw_pcie_link_up(pci)) {
> +               dev_dbg(dev, "Power has been turned off already\n");
> +               return 0;
> +       }
> +
> +       ret = qcom_pcie_send_pme_turnoff_msg(pcie);
> +       if (ret)
> +               return ret;
> +
> +       /* Power down the PHY, disable clock and regulators */
> +       qcom_pcie_host_disable(pcie);
> +
> +       return 0;
> +}
> +
> +/* Resume the PCIe link */
> +static int __maybe_unused qcom_pcie_pm_resume_noirq(struct device *dev)
> +{
> +       int ret;
> +       struct qcom_pcie *pcie = dev_get_drvdata(dev);
> +       struct dw_pcie *pci = pcie->pci;
> +       struct pcie_port *pp = &pci->pp;
> +
> +       ret = qcom_pcie_host_init(pp);
> +       if (ret) {
> +               dev_err(dev, "cannot initialize host\n");
> +               return ret;
> +       }
> +
> +       dw_pcie_setup_rc(pp);
> +
> +       qcom_pcie_start_link(pci);
> +
> +       ret = dw_pcie_wait_for_link(pci);
> +       if (ret)
> +               dev_err(dev, "Link never came up, Resume failed\n");

But we ignore and don't return ret? Please add a comment about why
that's done, or return ret below.

> +
> +       return 0;
> +}
> +
> +static const struct dev_pm_ops qcom_pcie_pm_ops = {
> +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend_noirq, qcom_pcie_pm_resume_noirq)

Why is noirq used? Please add a comment. And better yet don't use noirq
hooks and use the normal suspend/resume hooks.

> +};
> +
>  static const struct of_device_id qcom_pcie_match[] = {
>         { .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
>         { .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },