From: Prasad Malisetty <[email protected]>
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 <[email protected]>
---
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
On Wed, 22 Dec 2021 at 19:08, Prasad Malisetty
<[email protected]> wrote:
>
> From: Prasad Malisetty <[email protected]>
>
> 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?
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.
> When system resumes, PCIe link will be re-established and
> setup rc settings.
>
> Signed-off-by: Prasad Malisetty <[email protected]>
> ---
> 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
Hi Prasad,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on helgaas-pci/next]
[also build test WARNING on v5.16-rc7 next-20211224]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Prasad-Malisetty/PCI-qcom-Add-system-PM-support/20211223-001052
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: s390-randconfig-c005-20211227 (https://download.01.org/0day-ci/archive/20211228/[email protected]/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 511726c64d3b6cca66f7c54d457d586aa3129f67)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
# https://github.com/0day-ci/linux/commit/880ea4b05c26f859dd3f59f330d9aa08b7fc1d9f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Prasad-Malisetty/PCI-qcom-Add-system-PM-support/20211223-001052
git checkout 880ea4b05c26f859dd3f59f330d9aa08b7fc1d9f
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/pci/controller/dwc/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
In file included from drivers/pci/controller/dwc/pcie-qcom.c:16:
In file included from include/linux/io.h:13:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:36:59: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
^
In file included from drivers/pci/controller/dwc/pcie-qcom.c:16:
In file included from include/linux/io.h:13:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
^
In file included from drivers/pci/controller/dwc/pcie-qcom.c:16:
In file included from include/linux/io.h:13:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
>> drivers/pci/controller/dwc/pcie-qcom.c:1718:32: warning: unused variable 'qcom_pcie_pm_ops' [-Wunused-const-variable]
static const struct dev_pm_ops qcom_pcie_pm_ops = {
^
13 warnings generated.
vim +/qcom_pcie_pm_ops +1718 drivers/pci/controller/dwc/pcie-qcom.c
1717
> 1718 static const struct dev_pm_ops qcom_pcie_pm_ops = {
1719 SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend_noirq, qcom_pcie_pm_resume_noirq)
1720 };
1721
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Dmitry,
Thanks for the review and comments.
Please find the response inline.
Thanks
-Prasad
-----Original Message-----
From: Dmitry Baryshkov <[email protected]>
Sent: Thursday, December 23, 2021 8:44 PM
To: Prasad Malisetty <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; quic_vbadigan <[email protected]>; Rama Krishna (QUIC) <[email protected]>; [email protected]; [email protected]; quic_pmaliset <[email protected]>
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 <[email protected]> wrote:
>
> From: Prasad Malisetty <[email protected]>
>
> 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.
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.
> When system resumes, PCIe link will be re-established and setup rc
> settings.
>
> Signed-off-by: Prasad Malisetty <[email protected]>
> ---
> 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
Hi Prasad,
On Wed, 29 Dec 2021 at 13:59, Prasad Malisetty (Temp)
<[email protected]> 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 <[email protected]>
> Sent: Thursday, December 23, 2021 8:44 PM
> To: Prasad Malisetty <[email protected]>
> Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; quic_vbadigan <[email protected]>; Rama Krishna (QUIC) <[email protected]>; [email protected]; [email protected]; quic_pmaliset <[email protected]>
> 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 <[email protected]> wrote:
> >
> > From: Prasad Malisetty <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
On Wed, Dec 22, 2021 at 09:37:42PM +0530, Prasad Malisetty wrote:
> From: Prasad Malisetty <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
>