Messages in this thread | | | Date | Mon, 3 Jan 2022 12:34:12 +0530 | From | Manivannan Sadhasivam <> | Subject | Re: [PATCH v1] PCI: qcom: Add system PM support |
| |
On Wed, Dec 22, 2021 at 09:37:42PM +0530, Prasad Malisetty wrote: > From: Prasad Malisetty <quic_pmaliset@quicinc.com> > > 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. > > When system resumes, PCIe link will be re-established and > setup rc settings. > > Signed-off-by: Prasad Malisetty <quic_pmaliset@quicinc.com> > --- > 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;
u64?
> + struct dw_pcie *pci = pcie->pci; > + struct device *dev = pci->dev; > + > + val = readl(pcie->elbi + PCIE20_ELBI_SYS_CTRL); > + val |= BIT(4);
Please define 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);
define BIT(5)
> + if (!ret) > + dev_dbg(dev, "PCIe: PM_Enter_L23 is received\n");
This is not a helpful debug message for an user. And there is no need of "PCIe" prefix also. Use something like,
dev_dbg(dev, "Device entered L23 link state\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));
dev_dbg(dev, "Device failed to enter L23 link state. 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; > +
Why only 2.7.0? This should be generic.
> + /*Assert the reset of endpoint */
Space after /*
> + qcom_ep_reset_assert(pcie); > + > + /* Put PHY into POWER DOWN state */ > + phy_power_off(pcie->phy); > + > + writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
define 1
> + > + /* Disable pipe clock */
No need of this comment. As of now only pipe clock is disabled in post_deinit but it may change in future, so this comment will be outdated.
> + pcie->ops->post_deinit(pcie); > + > + /* Change GCC_PCIE_1_PIPE_MUXR register to 0x2 for XO as parent */
/* Set pipe clock parent to XO clock if needed */
> + if (pcie->pipe_clk_need_muxing) > + clk_set_parent(res->pipe_clk_src, res->ref_clk_src); > + > + /* Disable PCIe clocks and regulators*/
Space before */
> + 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");
This should be dev_dbg()
> + 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");
This is not needed.
> + 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");
Again, no need of this.
> + > + /* Initialize PCIe host */ > + ret = qcom_pcie_host_init(pp); > + if (ret) > + dev_err(dev, "cannot initialize host\n");
Can the below functions succeed if host_init fails?
> + > + dw_pcie_iatu_detect(pci);
Why this is needed? This is a static info of the PCI controller and not supposed to change.
> + 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 = {
Why is this struct not used?
Thanks, Mani
> + 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 >
| |