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

[Dmitry Baryshkov]

Hi Prasad,

On Wed, 29 Dec 2021 at 13:59, Prasad Malisetty (Temp)
<pmaliset@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Dmitry,
>
> Thanks for the review and comments.
>
> Please find the response inline.

Could you please use proper quoting nesting, it's hard to read if your
answers are marked as level 2 quote (>>).

>
> Thanks
> -Prasad
>
> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> Sent: Thursday, December 23, 2021 8:44 PM
> To: Prasad Malisetty <quic_c_pmaliset@xxxxxxxxxxx>
> Cc: agross@xxxxxxxxxx; bjorn.andersson@xxxxxxxxxx; lorenzo.pieralisi@xxxxxxx; robh@xxxxxxxxxx; kw@xxxxxxxxx; bhelgaas@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; quic_vbadigan <quic_vbadigan@xxxxxxxxxxx>; Rama Krishna (QUIC) <quic_ramkri@xxxxxxxxxxx>; manivannan.sadhasivam@xxxxxxxxxx; swboyd@xxxxxxxxxxxx; quic_pmaliset <quic_pmaliset@xxxxxxxxxxx>
> Subject: Re: [PATCH v1] PCI: qcom: Add system PM support
>
> WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
>
> On Wed, 22 Dec 2021 at 19:08, Prasad Malisetty <quic_c_pmaliset@xxxxxxxxxxx> wrote:
> >
> > From: Prasad Malisetty <quic_pmaliset@xxxxxxxxxxx>
> >
> > Add suspend_noirq and resume_noirq callbacks to handle 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.
>
> The GDSC stays on, if I'm not mistaken. Is this an expected behaviour for the suspend procedure?
>
> >> No, GDSC will be disabled as part of system suspend. We are switching gcc_pcie_1_clk_src to XO as GDSC should be enable in resume path.

I think you should call the pm_runtime_suspend() kind of function to
let the Linux core power down the GDSC power domains.

>
> Also as a side note, the qcom-pcie driver supports a variety of SoCs from different generations. Which platforms were really tested?
> Judging from your patch I suppose that you did not test this on any non-recent platform.
>
> >> We have tested on SC7280 SoC which is recently added.

Please take care and check that your code doesn't break previous
generations. You are using structures specific to 2.7.0 from the
generic code path. This is not legit.

>
> > When system resumes, PCIe link will be re-established and setup rc
> > settings.
> >
> > Signed-off-by: Prasad Malisetty <quic_pmaliset@xxxxxxxxxxx>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 103
> > +++++++++++++++++++++++++++++++++
> >  1 file changed, 103 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c
> > b/drivers/pci/controller/dwc/pcie-qcom.c
> > index c19cd506..24dcf5a 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -73,6 +73,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
> >
> > @@ -1616,6 +1618,107 @@ 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 = 0;
> > +       u32 val = 0, poll_val = 0;
> > +       uint64_t l23_rdy_poll_timeout = 100000;
> > +       struct dw_pcie *pci = pcie->pci;
> > +       struct device *dev = pci->dev;
> > +
> > +       val = readl(pcie->elbi + PCIE20_ELBI_SYS_CTRL);
> > +       val |= BIT(4);
> > +       writel(val, pcie->elbi + PCIE20_ELBI_SYS_CTRL);
> > +
> > +       ret = readl_poll_timeout((pcie->parf + PCIE20_PARF_PM_STTS), poll_val,
> > +                       (poll_val & BIT(5)), 10000, l23_rdy_poll_timeout);
> > +       if (!ret)
> > +               dev_dbg(dev, "PCIe: PM_Enter_L23 is received\n");
> > +       else
> > +               dev_err(dev, "PM_Enter_L23 is NOT received.PARF_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) {
> > +       struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > +
> > +       /*Assert the reset of endpoint */
> > +       qcom_ep_reset_assert(pcie);
> > +
> > +       /* Put PHY into POWER DOWN state */
> > +       phy_power_off(pcie->phy);
> > +
> > +       writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
> > +
> > +       /* Disable pipe clock */
> > +       pcie->ops->post_deinit(pcie);
> > +
> > +       /* Change GCC_PCIE_1_PIPE_MUXR register to 0x2 for XO as parent */
> > +       if (pcie->pipe_clk_need_muxing)
> > +               clk_set_parent(res->pipe_clk_src, res->ref_clk_src);
> > +
> > +       /* Disable PCIe clocks and regulators*/
> > +       pcie->ops->deinit(pcie);
> > +}
> > +
> > +static int __maybe_unused qcom_pcie_pm_suspend_noirq(struct device
> > +*dev) {
> > +       int ret = 0;
> > +       struct qcom_pcie *pcie = dev_get_drvdata(dev);
> > +       struct dw_pcie *pci = pcie->pci;
> > +
> > +       if (!dw_pcie_link_up(pci)) {
> > +               dev_err(dev, "Power has been turned off already\n");
> > +               return ret;
> > +       }
> > +
> > +       /* Send PME turnoff msg */
> > +       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);
> > +
> > +       dev_info(dev, "PM: PCI is suspended\n");
> > +       return ret;
> > +}
> > +
> > +/* Resume the PCIe link */
> > +static int __maybe_unused qcom_pcie_pm_resume_noirq(struct device
> > +*dev) {
> > +       int ret = 0;
> > +       struct qcom_pcie *pcie = dev_get_drvdata(dev);
> > +       struct dw_pcie *pci = pcie->pci;
> > +       struct pcie_port *pp = &pci->pp;
> > +
> > +       dev_info(dev, "PM: Resuming\n");
> > +
> > +       /* Initialize PCIe host */
> > +       ret = qcom_pcie_host_init(pp);
> > +       if (ret)
> > +               dev_err(dev, "cannot initialize host\n");
> > +
> > +       dw_pcie_iatu_detect(pci);
> > +       dw_pcie_setup_rc(pp);
> > +
> > +       /* Start the PCIe link */
> > +       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;
> > +}
> > +
> > +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) };
> > +
> >  static const struct of_device_id qcom_pcie_match[] = {
> >         { .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
> >         { .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
> > --
> > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
> > member of Code Aurora Forum, hosted by The Linux Foundation
> >
>
>
> --
> With best wishes
> Dmitry



-- 
With best wishes
Dmitry