Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
In-Reply-To: <1646679306-4768-1-git-send-email-quic_pmaliset@quicinc.com>
References: <1646679306-4768-1-git-send-email-quic_pmaliset@quicinc.com>
From:   Stephen Boyd <swboyd@chromium.org>
User-Agent: alot/0.10
Date:   Thu, 17 Mar 2022 18:07:49 -0400
Message-ID: <CAE-0n53ho2DX2rqQMvvKAuDCfsWW62TceTaNPzv5Mn_NQ-U6dA@mail.gmail.com>
Subject: Re: [PATCH v4] PCI: qcom: Add system PM support
To:     Prasad Malisetty <quic_pmaliset@quicinc.com>, agross@kernel.org,
        bhelgaas@google.com, bjorn.andersson@linaro.org, kw@linux.com,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-pci@vger.kernel.org, lorenzo.pieralisi@arm.com,
        rajatja@google.com, refactormyself@gmail.com, robh@kernel.org
Cc:     quic_vbadigan@quicinc.com, quic_ramkri@quicinc.com,
        manivannan.sadhasivam@linaro.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Quoting Prasad Malisetty (2022-03-07 10:55:06)
> Add suspend_noirq and resume_noirq callbacks to handle

Nitpick: They're no longer noirq so this should say "Add suspend and
resume callbacks"

> system suspend and resume in dwc PCIe controller driver.
>
> When system suspends, send PME turnoff message to enter
> link into L2 state. Along with powerdown the PHY, disable
> pipe clock, switch gcc_pcie_1_pipe_clk_src to XO if mux is
> supported and disable the pcie clocks, regulators.

I think Bjorn A. and Dmitry are trying to avoid this whole parking thing
in another series? Can you take a look at that?

>
> When system resumes, PCIe link will be re-established and
> setup rc settings.
>
> Signed-off-by: Prasad Malisetty <quic_pmaliset@quicinc.com>
>
> ---
> Changes since v3:
>         - Replaced noirq hooks with normal suspend/resume hooks.
>         - Removed local variable and placed in function itself.
>
> Changes since v2:
>         - Removed unnecessary variable initializations and comments.
>         - Removed platform specific variables declarations.
>         - Added MACRO names for the BIT shiftings.
>
> Changes since v1:
>         - Removed unnecessary logs and modified log level suggested by Manivannan.
>         - Removed platform specific callbacks as PM support is generic.
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 97 ++++++++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 6ab9089..4d29c80 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -48,6 +48,7 @@
>  #define PCIE20_PARF_PHY_REFCLK                 0x4C
>  #define PHY_REFCLK_SSP_EN                      BIT(16)
>  #define PHY_REFCLK_USE_PAD                     BIT(12)
> +#define PHY_POWER_DOWN                         0x1
>
>  #define PCIE20_PARF_DBI_BASE_ADDR              0x168
>  #define PCIE20_PARF_SLV_ADDR_SPACE_SIZE                0x16C
> @@ -62,6 +63,8 @@
>
>  #define PCIE20_ELBI_SYS_CTRL                   0x04
>  #define PCIE20_ELBI_SYS_CTRL_LT_ENABLE         BIT(0)
> +#define PCIE_PME_TURNOFF_MSG                   BIT(4)
> +#define PCIE_PM_LINKST_IN_L2                   BIT(5)
>
>  #define PCIE20_AXI_MSTR_RESP_COMP_CTRL0                0x818
>  #define CFG_REMOTE_RD_REQ_BRIDGE_SIZE_2K       0x4
> @@ -73,6 +76,8 @@
>
>  #define PCIE20_PARF_Q2A_FLUSH                  0x1AC
>
> +#define PCIE20_PARF_PM_STTS                    0x24
> +
>  #define PCIE20_MISC_CONTROL_1_REG              0x8BC
>  #define DBI_RO_WR_EN                           1
>
> @@ -1645,6 +1650,97 @@ 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;
> +       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, 100000);
> +       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->cfg->ops->post_deinit)
> +               pcie->cfg->ops->post_deinit(pcie);
> +
> +       /* Disable PCIe clocks and regulators */
> +       pcie->cfg->ops->deinit(pcie);
> +}
> +
> +static int __maybe_unused qcom_pcie_pm_suspend(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(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");

Capitalize cannot?

> +               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");
> +               return ret;

Drop return and braces.

> +       }
> +
> +       return 0;

return ret;

> +}
> +
> +static const struct dev_pm_ops qcom_pcie_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend, qcom_pcie_pm_resume)
> +};
> +