[PATCH v6 0/5] PCI: qcom: Add system suspend & resume support

[PATCH v6 0/5] PCI: qcom: Add system suspend & resume support

[Krishna chaitanya chundru]

Add suspend and resume syscore ops.

When system suspends, and if the link is in L1ss, disable the clocks
and power down the phy so that system enters into low power state by
parking link in L1ss to save the maximum power. And when the system
resumes, enable the clocks back and power on phy if they are disabled
in the suspend path.

we are doing this only when link is in l1ss but not in L2/L3 as
nowhere we are forcing link to L2/L3 by sending PME turn off.

is_suspended flag indicates if the clocks are disabled in the suspend
path or not.

There is access to Ep PCIe space to mask MSI/MSIX after pm suspend ops
(getting hit by affinity changes while making CPUs offline during suspend,
this will happen after devices are suspended (all phases of suspend ops)).
When registered with pm ops there is a crash due to un-clocked access,
as in the pm suspend op clocks are disabled. So, registering with syscore
ops which will called after making CPUs offline.

Make GDSC always on to ensure controller and its dependent clocks
won't go down during system suspend.

Krishna chaitanya chundru (5):
  PCI: qcom: Add system suspend and resume support
  PCI: qcom: Add retry logic for link to be stable in L1ss
  phy: core: Add support for phy power down & power up
  phy: qcom: Add power down/up callbacks to pcie phy
  clk: qcom: Alwaya on pcie gdsc

 drivers/clk/qcom/gcc-sc7280.c            |   2 +-
 drivers/pci/controller/dwc/pcie-qcom.c   | 156 ++++++++++++++++++++++++++++++-
 drivers/phy/phy-core.c                   |  30 ++++++
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c |  50 ++++++++++
 include/linux/phy/phy.h                  |  20 ++++
 5 files changed, 256 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH v6 1/5] PCI: qcom: Add system suspend and resume support

[Krishna chaitanya chundru]

Add suspend and resume syscore ops.

When system suspends and if the link is in L1ss, disable the clocks
and power down the phy so that system enters into low power state to
save the maximum power. And when the system resumes, enable the clocks
back and power on phy if they are disabled in the suspend path.

we are doing this only when link is in l1ss but not in L2/L3 as
nowhere we are forcing link to L2/L3 by sending PME turn off.

is_suspended flag indicates if the clocks are disabled in the suspend
path or not.

There is access to Ep PCIe space to mask MSI/MSIX after pm suspend ops
(getting hit by affinity changes while making CPUs offline during suspend,
this will happen after devices are suspended (all phases of suspend ops)).
When registered with pm ops there is a crash due to un-clocked access,
as in the pm suspend op clocks are disabled. So, registering with syscore
ops which will called after making CPUs offline.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
changes since v5:
	- Rebasing the code and replaced pm ops with syscore ops as
	  we are getting acciess to pci region after pm ops. syscore ops
	  will called after disabling non boot cpus and there is no pci
	  access after that.
Changes since v4:
	- Rebasing the code and removed the supports_system_suspend flag
	- in the resume path as is_suspended will serve its purpose.
Changes since v3:
	- Powering down the phy in suspend and powering it on resume to
	  acheive maximum power savings.
Changes since v2:
	- Replaced the enable, disable clks ops with suspend and resume
	- Renamed support_pm_opsi flag  with supports_system_suspend.
Changes since v1:
	- Fixed compilation errors.
---
 drivers/pci/controller/dwc/pcie-qcom.c | 140 ++++++++++++++++++++++++++++++++-
 1 file changed, 139 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 39ca06f..6e04d0d 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -27,6 +27,7 @@
 #include <linux/reset.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/syscore_ops.h>
 
 #include "../../pci.h"
 #include "pcie-designware.h"
@@ -44,6 +45,9 @@
 #define PCIE20_PARF_PM_CTRL			0x20
 #define REQ_NOT_ENTR_L1				BIT(5)
 
+#define PCIE20_PARF_PM_STTS			0x24
+#define PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB	BIT(8)
+
 #define PCIE20_PARF_PHY_CTRL			0x40
 #define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK	GENMASK(20, 16)
 #define PHY_CTRL_PHY_TX0_TERM_OFFSET(x)		((x) << 16)
@@ -122,6 +126,8 @@
 
 #define QCOM_PCIE_CRC8_POLYNOMIAL (BIT(2) | BIT(1) | BIT(0))
 
+static LIST_HEAD(qcom_pcie_list);
+
 struct qcom_pcie_resources_2_1_0 {
 	struct clk_bulk_data clks[QCOM_PCIE_2_1_0_MAX_CLOCKS];
 	struct reset_control *pci_reset;
@@ -211,13 +217,21 @@ struct qcom_pcie_ops {
 	void (*post_deinit)(struct qcom_pcie *pcie);
 	void (*ltssm_enable)(struct qcom_pcie *pcie);
 	int (*config_sid)(struct qcom_pcie *pcie);
+	int (*suspend)(struct qcom_pcie *pcie);
+	int (*resume)(struct qcom_pcie *pcie);
 };
 
 struct qcom_pcie_cfg {
 	const struct qcom_pcie_ops *ops;
+	/*
+	 * Flag ensures which devices will turn off clks, phy
+	 * in system suspend.
+	 */
+	unsigned int supports_system_suspend:1;
 };
 
 struct qcom_pcie {
+	struct list_head list;	/* list to probed instances */
 	struct dw_pcie *pci;
 	void __iomem *parf;			/* DT parf */
 	void __iomem *elbi;			/* DT elbi */
@@ -225,10 +239,14 @@ struct qcom_pcie {
 	struct phy *phy;
 	struct gpio_desc *reset;
 	const struct qcom_pcie_cfg *cfg;
+	unsigned int is_suspended:1;
 };
 
 #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
 
+static int __maybe_unused qcom_pcie_syscore_op_suspend(void);
+static void __maybe_unused qcom_pcie_syscore_op_resume(void);
+
 static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
 {
 	gpiod_set_value_cansleep(pcie->reset, 1);
@@ -1301,6 +1319,28 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
 	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
 }
 
+static int qcom_pcie_resume_2_7_0(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
+	int ret;
+
+	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
+
+	phy_power_on(pcie->phy);
+
+	return ret;
+}
+
+static int qcom_pcie_suspend_2_7_0(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
+
+	phy_power_off(pcie->phy);
+
+	clk_bulk_disable_unprepare(res->num_clks, res->clks);
+	return 0;
+}
+
 static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
@@ -1594,6 +1634,8 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
 	.deinit = qcom_pcie_deinit_2_7_0,
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
 	.config_sid = qcom_pcie_config_sid_sm8250,
+	.suspend = qcom_pcie_suspend_2_7_0,
+	.resume = qcom_pcie_resume_2_7_0,
 };
 
 /* Qcom IP rev.: 2.9.0  Synopsys IP rev.: 5.00a */
@@ -1613,6 +1655,11 @@ static const struct qcom_pcie_cfg cfg_1_9_0 = {
 	.ops = &ops_1_9_0,
 };
 
+static const struct qcom_pcie_cfg sc7280_cfg = {
+	.ops = &ops_1_9_0,
+	.supports_system_suspend = true,
+};
+
 static const struct qcom_pcie_cfg cfg_2_1_0 = {
 	.ops = &ops_2_1_0,
 };
@@ -1642,6 +1689,23 @@ static const struct dw_pcie_ops dw_pcie_ops = {
 	.start_link = qcom_pcie_start_link,
 };
 
+/*
+ * There is access to Ep PCIe space to mask MSI/MSIX after pm suspend
+ * ops.(getting hit by affinity changes while making CPUs offline during
+ * suspend, this will happen after devices are suspended
+ * (all phases of suspend ops)).
+ *
+ * When registered with pm ops there is a crash due to un-clocked access,
+ * as in the pm suspend op clocks are disabled.
+ *
+ * So, registering with syscore ops which will called after making
+ * CPU's offline.
+ */
+static struct syscore_ops qcom_pcie_syscore_ops = {
+	.suspend = qcom_pcie_syscore_op_suspend,
+	.resume = qcom_pcie_syscore_op_resume,
+};
+
 static int qcom_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1720,6 +1784,17 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 		goto err_phy_exit;
 	}
 
+	/* Register for syscore ops only when first instance probed */
+	if (list_empty(&qcom_pcie_list))
+		register_syscore_ops(&qcom_pcie_syscore_ops);
+
+	/*
+	 * Add the qcom_pcie list of each PCIe instance probed to
+	 * the global list so that we use it iterate through each PCIe
+	 * instance in the syscore ops.
+	 */
+	list_add_tail(&pcie->list, &qcom_pcie_list);
+
 	return 0;
 
 err_phy_exit:
@@ -1731,6 +1806,69 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static int __maybe_unused qcom_pcie_pm_suspend(struct qcom_pcie *pcie)
+{
+	u32 val;
+	struct dw_pcie *pci = pcie->pci;
+	struct device *dev = pci->dev;
+
+	if (!pcie->cfg->supports_system_suspend)
+		return 0;
+
+	/* if the link is not active turn off clocks */
+	if (!dw_pcie_link_up(pci)) {
+		dev_info(dev, "Link is not active\n");
+		goto suspend;
+	}
+
+	/* if the link is not in l1ss don't turn off clocks */
+	val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
+	if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
+		dev_warn(dev, "Link is not in L1ss\n");
+		return 0;
+	}
+
+suspend:
+	if (pcie->cfg->ops->suspend)
+		pcie->cfg->ops->suspend(pcie);
+
+	pcie->is_suspended = true;
+
+	return 0;
+}
+
+static int __maybe_unused qcom_pcie_pm_resume(struct qcom_pcie *pcie)
+{
+	if (!pcie->is_suspended)
+		return 0;
+
+	if (pcie->cfg->ops->resume)
+		pcie->cfg->ops->resume(pcie);
+
+	pcie->is_suspended = false;
+
+	return 0;
+}
+
+static int __maybe_unused qcom_pcie_syscore_op_suspend(void)
+{
+	struct qcom_pcie *qcom_pcie;
+
+	list_for_each_entry(qcom_pcie, &qcom_pcie_list, list) {
+		qcom_pcie_pm_suspend(qcom_pcie);
+	}
+	return 0;
+}
+
+static void __maybe_unused qcom_pcie_syscore_op_resume(void)
+{
+	struct qcom_pcie *qcom_pcie;
+
+	list_for_each_entry(qcom_pcie, &qcom_pcie_list, list) {
+		qcom_pcie_pm_resume(qcom_pcie);
+	}
+}
+
 static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-apq8064", .data = &cfg_2_1_0 },
 	{ .compatible = "qcom,pcie-apq8084", .data = &cfg_1_0_0 },
@@ -1742,7 +1880,7 @@ static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 },
 	{ .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 },
 	{ .compatible = "qcom,pcie-sa8540p", .data = &cfg_1_9_0 },
-	{ .compatible = "qcom,pcie-sc7280", .data = &cfg_1_9_0 },
+	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
 	{ .compatible = "qcom,pcie-sc8180x", .data = &cfg_1_9_0 },
 	{ .compatible = "qcom,pcie-sc8280xp", .data = &cfg_1_9_0 },
 	{ .compatible = "qcom,pcie-sdm845", .data = &cfg_2_7_0 },
-- 
2.7.4

Re: [PATCH v6 1/5] PCI: qcom: Add system suspend and resume support

[Matthias Kaehlcke]

On Fri, Sep 09, 2022 at 02:14:40PM +0530, Krishna chaitanya chundru wrote:
> Add suspend and resume syscore ops.
> 
> When system suspends and if the link is in L1ss, disable the clocks
> and power down the phy so that system enters into low power state to
> save the maximum power. And when the system resumes, enable the clocks
> back and power on phy if they are disabled in the suspend path.
> 
> we are doing this only when link is in l1ss but not in L2/L3 as
> nowhere we are forcing link to L2/L3 by sending PME turn off.
> 
> is_suspended flag indicates if the clocks are disabled in the suspend
> path or not.
> 
> There is access to Ep PCIe space to mask MSI/MSIX after pm suspend ops
> (getting hit by affinity changes while making CPUs offline during suspend,
> this will happen after devices are suspended (all phases of suspend ops)).
> When registered with pm ops there is a crash due to un-clocked access,
> as in the pm suspend op clocks are disabled. So, registering with syscore
> ops which will called after making CPUs offline.

My knowledge of PCI is limited, but given the issues you are seeing which
don't seem to impact other DWC drivers I wonder if keeping the link in
l1ss is the right thing to do. The intel, tegra and imx6 drivers all turn
the PME off, which IIUC results in the link to transition ot L2 or L3.
Shouldn't the QC driver do the same?

Some more comments inline, for if the current approach moves forward.

> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
> changes since v5:
> 	- Rebasing the code and replaced pm ops with syscore ops as
> 	  we are getting acciess to pci region after pm ops. syscore ops
> 	  will called after disabling non boot cpus and there is no pci
> 	  access after that.
> Changes since v4:
> 	- Rebasing the code and removed the supports_system_suspend flag
> 	- in the resume path as is_suspended will serve its purpose.
> Changes since v3:
> 	- Powering down the phy in suspend and powering it on resume to
> 	  acheive maximum power savings.
> Changes since v2:
> 	- Replaced the enable, disable clks ops with suspend and resume
> 	- Renamed support_pm_opsi flag  with supports_system_suspend.
> Changes since v1:
> 	- Fixed compilation errors.
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 140 ++++++++++++++++++++++++++++++++-
>  1 file changed, 139 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 39ca06f..6e04d0d 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -27,6 +27,7 @@
>  #include <linux/reset.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
> +#include <linux/syscore_ops.h>
>  
>  #include "../../pci.h"
>  #include "pcie-designware.h"
> @@ -44,6 +45,9 @@
>  #define PCIE20_PARF_PM_CTRL			0x20
>  #define REQ_NOT_ENTR_L1				BIT(5)
>  
> +#define PCIE20_PARF_PM_STTS			0x24
> +#define PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB	BIT(8)
> +
>  #define PCIE20_PARF_PHY_CTRL			0x40
>  #define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK	GENMASK(20, 16)
>  #define PHY_CTRL_PHY_TX0_TERM_OFFSET(x)		((x) << 16)
> @@ -122,6 +126,8 @@
>  
>  #define QCOM_PCIE_CRC8_POLYNOMIAL (BIT(2) | BIT(1) | BIT(0))
>  
> +static LIST_HEAD(qcom_pcie_list);
> +
>  struct qcom_pcie_resources_2_1_0 {
>  	struct clk_bulk_data clks[QCOM_PCIE_2_1_0_MAX_CLOCKS];
>  	struct reset_control *pci_reset;
> @@ -211,13 +217,21 @@ struct qcom_pcie_ops {
>  	void (*post_deinit)(struct qcom_pcie *pcie);
>  	void (*ltssm_enable)(struct qcom_pcie *pcie);
>  	int (*config_sid)(struct qcom_pcie *pcie);
> +	int (*suspend)(struct qcom_pcie *pcie);
> +	int (*resume)(struct qcom_pcie *pcie);
>  };
>  
>  struct qcom_pcie_cfg {
>  	const struct qcom_pcie_ops *ops;
> +	/*
> +	 * Flag ensures which devices will turn off clks, phy
> +	 * in system suspend.
> +	 */
> +	unsigned int supports_system_suspend:1;
>  };
>  
>  struct qcom_pcie {
> +	struct list_head list;	/* list to probed instances */
>  	struct dw_pcie *pci;
>  	void __iomem *parf;			/* DT parf */
>  	void __iomem *elbi;			/* DT elbi */
> @@ -225,10 +239,14 @@ struct qcom_pcie {
>  	struct phy *phy;
>  	struct gpio_desc *reset;
>  	const struct qcom_pcie_cfg *cfg;
> +	unsigned int is_suspended:1;
>  };
>  
>  #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
>  
> +static int __maybe_unused qcom_pcie_syscore_op_suspend(void);
> +static void __maybe_unused qcom_pcie_syscore_op_resume(void);
> +
>  static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
>  {
>  	gpiod_set_value_cansleep(pcie->reset, 1);
> @@ -1301,6 +1319,28 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
>  	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
>  }
>  
> +static int qcom_pcie_resume_2_7_0(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> +	int ret;
> +
> +	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
> +
> +	phy_power_on(pcie->phy);
> +
> +	return ret;
> +}
> +
> +static int qcom_pcie_suspend_2_7_0(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> +
> +	phy_power_off(pcie->phy);
> +
> +	clk_bulk_disable_unprepare(res->num_clks, res->clks);
> +	return 0;
> +}
> +
>  static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
>  {
>  	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> @@ -1594,6 +1634,8 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
>  	.deinit = qcom_pcie_deinit_2_7_0,
>  	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
>  	.config_sid = qcom_pcie_config_sid_sm8250,
> +	.suspend = qcom_pcie_suspend_2_7_0,
> +	.resume = qcom_pcie_resume_2_7_0,
>  };
>  
>  /* Qcom IP rev.: 2.9.0  Synopsys IP rev.: 5.00a */
> @@ -1613,6 +1655,11 @@ static const struct qcom_pcie_cfg cfg_1_9_0 = {
>  	.ops = &ops_1_9_0,
>  };
>  
> +static const struct qcom_pcie_cfg sc7280_cfg = {
> +	.ops = &ops_1_9_0,
> +	.supports_system_suspend = true,
> +};
> +
>  static const struct qcom_pcie_cfg cfg_2_1_0 = {
>  	.ops = &ops_2_1_0,
>  };
> @@ -1642,6 +1689,23 @@ static const struct dw_pcie_ops dw_pcie_ops = {
>  	.start_link = qcom_pcie_start_link,
>  };
>  
> +/*
> + * There is access to Ep PCIe space to mask MSI/MSIX after pm suspend
> + * ops.(getting hit by affinity changes while making CPUs offline during
> + * suspend, this will happen after devices are suspended
> + * (all phases of suspend ops)).
> + *
> + * When registered with pm ops there is a crash due to un-clocked access,
> + * as in the pm suspend op clocks are disabled.
> + *
> + * So, registering with syscore ops which will called after making
> + * CPU's offline.
> + */
> +static struct syscore_ops qcom_pcie_syscore_ops = {
> +	.suspend = qcom_pcie_syscore_op_suspend,
> +	.resume = qcom_pcie_syscore_op_resume,
> +};
> +
>  static int qcom_pcie_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -1720,6 +1784,17 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  		goto err_phy_exit;
>  	}
>  
> +	/* Register for syscore ops only when first instance probed */
> +	if (list_empty(&qcom_pcie_list))
> +		register_syscore_ops(&qcom_pcie_syscore_ops);
> +
> +	/*
> +	 * Add the qcom_pcie list of each PCIe instance probed to
> +	 * the global list so that we use it iterate through each PCIe
> +	 * instance in the syscore ops.
> +	 */
> +	list_add_tail(&pcie->list, &qcom_pcie_list);
> +
>  	return 0;
>  
>  err_phy_exit:
> @@ -1731,6 +1806,69 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> +static int __maybe_unused qcom_pcie_pm_suspend(struct qcom_pcie *pcie)
> +{
> +	u32 val;
> +	struct dw_pcie *pci = pcie->pci;
> +	struct device *dev = pci->dev;
> +
> +	if (!pcie->cfg->supports_system_suspend)
> +		return 0;

This check could be done in qcom_pcie_syscore_op_suspend()

> +
> +	/* if the link is not active turn off clocks */
> +	if (!dw_pcie_link_up(pci)) {
> +		dev_info(dev, "Link is not active\n");

level info seems to verbose, it's not particularly interesting that the
link is not active, except for debugging.

> +		goto suspend;
> +	}
> +
> +	/* if the link is not in l1ss don't turn off clocks */
> +	val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
> +	if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> +		dev_warn(dev, "Link is not in L1ss\n");
> +		return 0;
> +	}

I think the following would be clearer:

  	if (dw_pcie_link_up(pci)) {
		/* if the link is not in l1ss don't turn off clocks */
		val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
		if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
			dev_warn(dev, "Link is not in L1ss\n");
			return 0;
		}
	} else {
		dev_dbg(dev, "Link is not active\n");
	}

> +
> +suspend:
> +	if (pcie->cfg->ops->suspend)
> +		pcie->cfg->ops->suspend(pcie);
> +
> +	pcie->is_suspended = true;
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused qcom_pcie_pm_resume(struct qcom_pcie *pcie)
> +{
> +	if (!pcie->is_suspended)
> +		return 0;
> +
> +	if (pcie->cfg->ops->resume)
> +		pcie->cfg->ops->resume(pcie);
> +
> +	pcie->is_suspended = false;
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused qcom_pcie_syscore_op_suspend(void)
> +{
> +	struct qcom_pcie *qcom_pcie;
> +
> +	list_for_each_entry(qcom_pcie, &qcom_pcie_list, list) {
> +		qcom_pcie_pm_suspend(qcom_pcie);
> +	}
> +	return 0;
> +}
> +
> +static void __maybe_unused qcom_pcie_syscore_op_resume(void)
> +{
> +	struct qcom_pcie *qcom_pcie;
> +
> +	list_for_each_entry(qcom_pcie, &qcom_pcie_list, list) {
> +		qcom_pcie_pm_resume(qcom_pcie);
> +	}
> +}
> +
>  static const struct of_device_id qcom_pcie_match[] = {
>  	{ .compatible = "qcom,pcie-apq8064", .data = &cfg_2_1_0 },
>  	{ .compatible = "qcom,pcie-apq8084", .data = &cfg_1_0_0 },
> @@ -1742,7 +1880,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>  	{ .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 },
>  	{ .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 },
>  	{ .compatible = "qcom,pcie-sa8540p", .data = &cfg_1_9_0 },
> -	{ .compatible = "qcom,pcie-sc7280", .data = &cfg_1_9_0 },
> +	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
>  	{ .compatible = "qcom,pcie-sc8180x", .data = &cfg_1_9_0 },
>  	{ .compatible = "qcom,pcie-sc8280xp", .data = &cfg_1_9_0 },
>  	{ .compatible = "qcom,pcie-sdm845", .data = &cfg_2_7_0 },
> -- 
> 2.7.4
> 

Re: [PATCH v6 1/5] PCI: qcom: Add system suspend and resume support

[Krishna Chaitanya Chundru]

On 9/9/2022 11:01 PM, Matthias Kaehlcke wrote:
> On Fri, Sep 09, 2022 at 02:14:40PM +0530, Krishna chaitanya chundru wrote:
>> Add suspend and resume syscore ops.
>>
>> When system suspends and if the link is in L1ss, disable the clocks
>> and power down the phy so that system enters into low power state to
>> save the maximum power. And when the system resumes, enable the clocks
>> back and power on phy if they are disabled in the suspend path.
>>
>> we are doing this only when link is in l1ss but not in L2/L3 as
>> nowhere we are forcing link to L2/L3 by sending PME turn off.
>>
>> is_suspended flag indicates if the clocks are disabled in the suspend
>> path or not.
>>
>> There is access to Ep PCIe space to mask MSI/MSIX after pm suspend ops
>> (getting hit by affinity changes while making CPUs offline during suspend,
>> this will happen after devices are suspended (all phases of suspend ops)).
>> When registered with pm ops there is a crash due to un-clocked access,
>> as in the pm suspend op clocks are disabled. So, registering with syscore
>> ops which will called after making CPUs offline.
> My knowledge of PCI is limited, but given the issues you are seeing which
> don't seem to impact other DWC drivers I wonder if keeping the link in
> l1ss is the right thing to do. The intel, tegra and imx6 drivers all turn
> the PME off, which IIUC results in the link to transition ot L2 or L3.
> Shouldn't the QC driver do the same?
The other dwc drivers are trying to turn off PME in the suspend and in 
resume path
they are trying to do link training again, this is what we have done 
previously
But this change is rejected by nvme developers because this will 
decrease device life cycle
(turning off the link and bringing up is treated as a power cycle of nvme).

So we are trying this approach.
>
> Some more comments inline, for if the current approach moves forward.
>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>> changes since v5:
>> 	- Rebasing the code and replaced pm ops with syscore ops as
>> 	  we are getting acciess to pci region after pm ops. syscore ops
>> 	  will called after disabling non boot cpus and there is no pci
>> 	  access after that.
>> Changes since v4:
>> 	- Rebasing the code and removed the supports_system_suspend flag
>> 	- in the resume path as is_suspended will serve its purpose.
>> Changes since v3:
>> 	- Powering down the phy in suspend and powering it on resume to
>> 	  acheive maximum power savings.
>> Changes since v2:
>> 	- Replaced the enable, disable clks ops with suspend and resume
>> 	- Renamed support_pm_opsi flag  with supports_system_suspend.
>> Changes since v1:
>> 	- Fixed compilation errors.
>> ---
>>   drivers/pci/controller/dwc/pcie-qcom.c | 140 ++++++++++++++++++++++++++++++++-
>>   1 file changed, 139 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 39ca06f..6e04d0d 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -27,6 +27,7 @@
>>   #include <linux/reset.h>
>>   #include <linux/slab.h>
>>   #include <linux/types.h>
>> +#include <linux/syscore_ops.h>
>>   
>>   #include "../../pci.h"
>>   #include "pcie-designware.h"
>> @@ -44,6 +45,9 @@
>>   #define PCIE20_PARF_PM_CTRL			0x20
>>   #define REQ_NOT_ENTR_L1				BIT(5)
>>   
>> +#define PCIE20_PARF_PM_STTS			0x24
>> +#define PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB	BIT(8)
>> +
>>   #define PCIE20_PARF_PHY_CTRL			0x40
>>   #define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK	GENMASK(20, 16)
>>   #define PHY_CTRL_PHY_TX0_TERM_OFFSET(x)		((x) << 16)
>> @@ -122,6 +126,8 @@
>>   
>>   #define QCOM_PCIE_CRC8_POLYNOMIAL (BIT(2) | BIT(1) | BIT(0))
>>   
>> +static LIST_HEAD(qcom_pcie_list);
>> +
>>   struct qcom_pcie_resources_2_1_0 {
>>   	struct clk_bulk_data clks[QCOM_PCIE_2_1_0_MAX_CLOCKS];
>>   	struct reset_control *pci_reset;
>> @@ -211,13 +217,21 @@ struct qcom_pcie_ops {
>>   	void (*post_deinit)(struct qcom_pcie *pcie);
>>   	void (*ltssm_enable)(struct qcom_pcie *pcie);
>>   	int (*config_sid)(struct qcom_pcie *pcie);
>> +	int (*suspend)(struct qcom_pcie *pcie);
>> +	int (*resume)(struct qcom_pcie *pcie);
>>   };
>>   
>>   struct qcom_pcie_cfg {
>>   	const struct qcom_pcie_ops *ops;
>> +	/*
>> +	 * Flag ensures which devices will turn off clks, phy
>> +	 * in system suspend.
>> +	 */
>> +	unsigned int supports_system_suspend:1;
>>   };
>>   
>>   struct qcom_pcie {
>> +	struct list_head list;	/* list to probed instances */
>>   	struct dw_pcie *pci;
>>   	void __iomem *parf;			/* DT parf */
>>   	void __iomem *elbi;			/* DT elbi */
>> @@ -225,10 +239,14 @@ struct qcom_pcie {
>>   	struct phy *phy;
>>   	struct gpio_desc *reset;
>>   	const struct qcom_pcie_cfg *cfg;
>> +	unsigned int is_suspended:1;
>>   };
>>   
>>   #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
>>   
>> +static int __maybe_unused qcom_pcie_syscore_op_suspend(void);
>> +static void __maybe_unused qcom_pcie_syscore_op_resume(void);
>> +
>>   static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
>>   {
>>   	gpiod_set_value_cansleep(pcie->reset, 1);
>> @@ -1301,6 +1319,28 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
>>   	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
>>   }
>>   
>> +static int qcom_pcie_resume_2_7_0(struct qcom_pcie *pcie)
>> +{
>> +	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
>> +	int ret;
>> +
>> +	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
>> +
>> +	phy_power_on(pcie->phy);
>> +
>> +	return ret;
>> +}
>> +
>> +static int qcom_pcie_suspend_2_7_0(struct qcom_pcie *pcie)
>> +{
>> +	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
>> +
>> +	phy_power_off(pcie->phy);
>> +
>> +	clk_bulk_disable_unprepare(res->num_clks, res->clks);
>> +	return 0;
>> +}
>> +
>>   static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
>>   {
>>   	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
>> @@ -1594,6 +1634,8 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
>>   	.deinit = qcom_pcie_deinit_2_7_0,
>>   	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
>>   	.config_sid = qcom_pcie_config_sid_sm8250,
>> +	.suspend = qcom_pcie_suspend_2_7_0,
>> +	.resume = qcom_pcie_resume_2_7_0,
>>   };
>>   
>>   /* Qcom IP rev.: 2.9.0  Synopsys IP rev.: 5.00a */
>> @@ -1613,6 +1655,11 @@ static const struct qcom_pcie_cfg cfg_1_9_0 = {
>>   	.ops = &ops_1_9_0,
>>   };
>>   
>> +static const struct qcom_pcie_cfg sc7280_cfg = {
>> +	.ops = &ops_1_9_0,
>> +	.supports_system_suspend = true,
>> +};
>> +
>>   static const struct qcom_pcie_cfg cfg_2_1_0 = {
>>   	.ops = &ops_2_1_0,
>>   };
>> @@ -1642,6 +1689,23 @@ static const struct dw_pcie_ops dw_pcie_ops = {
>>   	.start_link = qcom_pcie_start_link,
>>   };
>>   
>> +/*
>> + * There is access to Ep PCIe space to mask MSI/MSIX after pm suspend
>> + * ops.(getting hit by affinity changes while making CPUs offline during
>> + * suspend, this will happen after devices are suspended
>> + * (all phases of suspend ops)).
>> + *
>> + * When registered with pm ops there is a crash due to un-clocked access,
>> + * as in the pm suspend op clocks are disabled.
>> + *
>> + * So, registering with syscore ops which will called after making
>> + * CPU's offline.
>> + */
>> +static struct syscore_ops qcom_pcie_syscore_ops = {
>> +	.suspend = qcom_pcie_syscore_op_suspend,
>> +	.resume = qcom_pcie_syscore_op_resume,
>> +};
>> +
>>   static int qcom_pcie_probe(struct platform_device *pdev)
>>   {
>>   	struct device *dev = &pdev->dev;
>> @@ -1720,6 +1784,17 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>   		goto err_phy_exit;
>>   	}
>>   
>> +	/* Register for syscore ops only when first instance probed */
>> +	if (list_empty(&qcom_pcie_list))
>> +		register_syscore_ops(&qcom_pcie_syscore_ops);
>> +
>> +	/*
>> +	 * Add the qcom_pcie list of each PCIe instance probed to
>> +	 * the global list so that we use it iterate through each PCIe
>> +	 * instance in the syscore ops.
>> +	 */
>> +	list_add_tail(&pcie->list, &qcom_pcie_list);
>> +
>>   	return 0;
>>   
>>   err_phy_exit:
>> @@ -1731,6 +1806,69 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>   	return ret;
>>   }
>>   
>> +static int __maybe_unused qcom_pcie_pm_suspend(struct qcom_pcie *pcie)
>> +{
>> +	u32 val;
>> +	struct dw_pcie *pci = pcie->pci;
>> +	struct device *dev = pci->dev;
>> +
>> +	if (!pcie->cfg->supports_system_suspend)
>> +		return 0;
> This check could be done in qcom_pcie_syscore_op_suspend()
sure , we will take of it in next patch.
>
>> +
>> +	/* if the link is not active turn off clocks */
>> +	if (!dw_pcie_link_up(pci)) {
>> +		dev_info(dev, "Link is not active\n");
> level info seems to verbose, it's not particularly interesting that the
> link is not active, except for debugging.
sure , we will take of it in next patch.
>
>> +		goto suspend;
>> +	}
>> +
>> +	/* if the link is not in l1ss don't turn off clocks */
>> +	val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
>> +	if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
>> +		dev_warn(dev, "Link is not in L1ss\n");
>> +		return 0;
>> +	}
> I think the following would be clearer:
>
>    	if (dw_pcie_link_up(pci)) {
> 		/* if the link is not in l1ss don't turn off clocks */
> 		val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
> 		if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> 			dev_warn(dev, "Link is not in L1ss\n");
> 			return 0;
> 		}
> 	} else {
> 		dev_dbg(dev, "Link is not active\n");
> 	}
But as we are adding retry logic in other patch  with while loop this 
logic will not be applicable.
>> +
>> +suspend:
>> +	if (pcie->cfg->ops->suspend)
>> +		pcie->cfg->ops->suspend(pcie);
>> +
>> +	pcie->is_suspended = true;
>> +
>> +	return 0;
>> +}
>> +
>> +static int __maybe_unused qcom_pcie_pm_resume(struct qcom_pcie *pcie)
>> +{
>> +	if (!pcie->is_suspended)
>> +		return 0;
>> +
>> +	if (pcie->cfg->ops->resume)
>> +		pcie->cfg->ops->resume(pcie);
>> +
>> +	pcie->is_suspended = false;
>> +
>> +	return 0;
>> +}
>> +
>> +static int __maybe_unused qcom_pcie_syscore_op_suspend(void)
>> +{
>> +	struct qcom_pcie *qcom_pcie;
>> +
>> +	list_for_each_entry(qcom_pcie, &qcom_pcie_list, list) {
>> +		qcom_pcie_pm_suspend(qcom_pcie);
>> +	}
>> +	return 0;
>> +}
>> +
>> +static void __maybe_unused qcom_pcie_syscore_op_resume(void)
>> +{
>> +	struct qcom_pcie *qcom_pcie;
>> +
>> +	list_for_each_entry(qcom_pcie, &qcom_pcie_list, list) {
>> +		qcom_pcie_pm_resume(qcom_pcie);
>> +	}
>> +}
>> +
>>   static const struct of_device_id qcom_pcie_match[] = {
>>   	{ .compatible = "qcom,pcie-apq8064", .data = &cfg_2_1_0 },
>>   	{ .compatible = "qcom,pcie-apq8084", .data = &cfg_1_0_0 },
>> @@ -1742,7 +1880,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>>   	{ .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 },
>>   	{ .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 },
>>   	{ .compatible = "qcom,pcie-sa8540p", .data = &cfg_1_9_0 },
>> -	{ .compatible = "qcom,pcie-sc7280", .data = &cfg_1_9_0 },
>> +	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
>>   	{ .compatible = "qcom,pcie-sc8180x", .data = &cfg_1_9_0 },
>>   	{ .compatible = "qcom,pcie-sc8280xp", .data = &cfg_1_9_0 },
>>   	{ .compatible = "qcom,pcie-sdm845", .data = &cfg_2_7_0 },
>> -- 
>> 2.7.4
>>

Re: [PATCH v6 1/5] PCI: qcom: Add system suspend and resume support

[Matthias Kaehlcke]

On Mon, Sep 12, 2022 at 09:36:53PM +0530, Krishna Chaitanya Chundru wrote:
> 
> On 9/9/2022 11:01 PM, Matthias Kaehlcke wrote:
> > On Fri, Sep 09, 2022 at 02:14:40PM +0530, Krishna chaitanya chundru wrote:
> > > Add suspend and resume syscore ops.
> > > 
> > > When system suspends and if the link is in L1ss, disable the clocks
> > > and power down the phy so that system enters into low power state to
> > > save the maximum power. And when the system resumes, enable the clocks
> > > back and power on phy if they are disabled in the suspend path.
> > > 
> > > we are doing this only when link is in l1ss but not in L2/L3 as
> > > nowhere we are forcing link to L2/L3 by sending PME turn off.
> > > 
> > > is_suspended flag indicates if the clocks are disabled in the suspend
> > > path or not.
> > > 
> > > There is access to Ep PCIe space to mask MSI/MSIX after pm suspend ops
> > > (getting hit by affinity changes while making CPUs offline during suspend,
> > > this will happen after devices are suspended (all phases of suspend ops)).
> > > When registered with pm ops there is a crash due to un-clocked access,
> > > as in the pm suspend op clocks are disabled. So, registering with syscore
> > > ops which will called after making CPUs offline.
> > My knowledge of PCI is limited, but given the issues you are seeing which
> > don't seem to impact other DWC drivers I wonder if keeping the link in
> > l1ss is the right thing to do. The intel, tegra and imx6 drivers all turn
> > the PME off, which IIUC results in the link to transition ot L2 or L3.
> > Shouldn't the QC driver do the same?
> The other dwc drivers are trying to turn off PME in the suspend and in
> resume path
> they are trying to do link training again, this is what we have done
> previously
> But this change is rejected by nvme developers because this will decrease
> device life cycle
> (turning off the link and bringing up is treated as a power cycle of nvme).

I see, so the other drivers are currently not really suitable for use with
NVMe :(

For reference, Christoph pointed out here [1] that switching the link off
wears out the flash.

So IIUC keeping the link in >= L1 during system suspend is a requirement
for NVMe and the question is how to achieve that in the dwc/pcie-qcom
drivers without resorting to ugly hacks.

[1] https://lore.kernel.org/all/20220518084356.GA6933@lst.de/#R

> So we are trying this approach.
> > 
> > Some more comments inline, for if the current approach moves forward.
> > 
> > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > > ---
> > > changes since v5:
> > > 	- Rebasing the code and replaced pm ops with syscore ops as
> > > 	  we are getting acciess to pci region after pm ops. syscore ops
> > > 	  will called after disabling non boot cpus and there is no pci
> > > 	  access after that.
> > > Changes since v4:
> > > 	- Rebasing the code and removed the supports_system_suspend flag
> > > 	- in the resume path as is_suspended will serve its purpose.
> > > Changes since v3:
> > > 	- Powering down the phy in suspend and powering it on resume to
> > > 	  acheive maximum power savings.
> > > Changes since v2:
> > > 	- Replaced the enable, disable clks ops with suspend and resume
> > > 	- Renamed support_pm_opsi flag  with supports_system_suspend.
> > > Changes since v1:
> > > 	- Fixed compilation errors.
> > > ---
> > >   drivers/pci/controller/dwc/pcie-qcom.c | 140 ++++++++++++++++++++++++++++++++-
> > >   1 file changed, 139 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > index 39ca06f..6e04d0d 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > @@ -27,6 +27,7 @@
> > >   #include <linux/reset.h>
> > >   #include <linux/slab.h>
> > >   #include <linux/types.h>
> > > +#include <linux/syscore_ops.h>
> > >   #include "../../pci.h"
> > >   #include "pcie-designware.h"
> > > @@ -44,6 +45,9 @@
> > >   #define PCIE20_PARF_PM_CTRL			0x20
> > >   #define REQ_NOT_ENTR_L1				BIT(5)
> > > +#define PCIE20_PARF_PM_STTS			0x24
> > > +#define PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB	BIT(8)
> > > +
> > >   #define PCIE20_PARF_PHY_CTRL			0x40
> > >   #define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK	GENMASK(20, 16)
> > >   #define PHY_CTRL_PHY_TX0_TERM_OFFSET(x)		((x) << 16)
> > > @@ -122,6 +126,8 @@
> > >   #define QCOM_PCIE_CRC8_POLYNOMIAL (BIT(2) | BIT(1) | BIT(0))
> > > +static LIST_HEAD(qcom_pcie_list);
> > > +
> > >   struct qcom_pcie_resources_2_1_0 {
> > >   	struct clk_bulk_data clks[QCOM_PCIE_2_1_0_MAX_CLOCKS];
> > >   	struct reset_control *pci_reset;
> > > @@ -211,13 +217,21 @@ struct qcom_pcie_ops {
> > >   	void (*post_deinit)(struct qcom_pcie *pcie);
> > >   	void (*ltssm_enable)(struct qcom_pcie *pcie);
> > >   	int (*config_sid)(struct qcom_pcie *pcie);
> > > +	int (*suspend)(struct qcom_pcie *pcie);
> > > +	int (*resume)(struct qcom_pcie *pcie);
> > >   };
> > >   struct qcom_pcie_cfg {
> > >   	const struct qcom_pcie_ops *ops;
> > > +	/*
> > > +	 * Flag ensures which devices will turn off clks, phy
> > > +	 * in system suspend.
> > > +	 */
> > > +	unsigned int supports_system_suspend:1;
> > >   };
> > >   struct qcom_pcie {
> > > +	struct list_head list;	/* list to probed instances */
> > >   	struct dw_pcie *pci;
> > >   	void __iomem *parf;			/* DT parf */
> > >   	void __iomem *elbi;			/* DT elbi */
> > > @@ -225,10 +239,14 @@ struct qcom_pcie {
> > >   	struct phy *phy;
> > >   	struct gpio_desc *reset;
> > >   	const struct qcom_pcie_cfg *cfg;
> > > +	unsigned int is_suspended:1;
> > >   };
> > >   #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
> > > +static int __maybe_unused qcom_pcie_syscore_op_suspend(void);
> > > +static void __maybe_unused qcom_pcie_syscore_op_resume(void);
> > > +
> > >   static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> > >   {
> > >   	gpiod_set_value_cansleep(pcie->reset, 1);
> > > @@ -1301,6 +1319,28 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
> > >   	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> > >   }
> > > +static int qcom_pcie_resume_2_7_0(struct qcom_pcie *pcie)
> > > +{
> > > +	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > > +	int ret;
> > > +
> > > +	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
> > > +
> > > +	phy_power_on(pcie->phy);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int qcom_pcie_suspend_2_7_0(struct qcom_pcie *pcie)
> > > +{
> > > +	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > > +
> > > +	phy_power_off(pcie->phy);
> > > +
> > > +	clk_bulk_disable_unprepare(res->num_clks, res->clks);
> > > +	return 0;
> > > +}
> > > +
> > >   static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
> > >   {
> > >   	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> > > @@ -1594,6 +1634,8 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
> > >   	.deinit = qcom_pcie_deinit_2_7_0,
> > >   	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> > >   	.config_sid = qcom_pcie_config_sid_sm8250,
> > > +	.suspend = qcom_pcie_suspend_2_7_0,
> > > +	.resume = qcom_pcie_resume_2_7_0,
> > >   };
> > >   /* Qcom IP rev.: 2.9.0  Synopsys IP rev.: 5.00a */
> > > @@ -1613,6 +1655,11 @@ static const struct qcom_pcie_cfg cfg_1_9_0 = {
> > >   	.ops = &ops_1_9_0,
> > >   };
> > > +static const struct qcom_pcie_cfg sc7280_cfg = {
> > > +	.ops = &ops_1_9_0,
> > > +	.supports_system_suspend = true,
> > > +};
> > > +
> > >   static const struct qcom_pcie_cfg cfg_2_1_0 = {
> > >   	.ops = &ops_2_1_0,
> > >   };
> > > @@ -1642,6 +1689,23 @@ static const struct dw_pcie_ops dw_pcie_ops = {
> > >   	.start_link = qcom_pcie_start_link,
> > >   };
> > > +/*
> > > + * There is access to Ep PCIe space to mask MSI/MSIX after pm suspend
> > > + * ops.(getting hit by affinity changes while making CPUs offline during
> > > + * suspend, this will happen after devices are suspended
> > > + * (all phases of suspend ops)).
> > > + *
> > > + * When registered with pm ops there is a crash due to un-clocked access,
> > > + * as in the pm suspend op clocks are disabled.
> > > + *
> > > + * So, registering with syscore ops which will called after making
> > > + * CPU's offline.
> > > + */
> > > +static struct syscore_ops qcom_pcie_syscore_ops = {
> > > +	.suspend = qcom_pcie_syscore_op_suspend,
> > > +	.resume = qcom_pcie_syscore_op_resume,
> > > +};
> > > +
> > >   static int qcom_pcie_probe(struct platform_device *pdev)
> > >   {
> > >   	struct device *dev = &pdev->dev;
> > > @@ -1720,6 +1784,17 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > >   		goto err_phy_exit;
> > >   	}
> > > +	/* Register for syscore ops only when first instance probed */
> > > +	if (list_empty(&qcom_pcie_list))
> > > +		register_syscore_ops(&qcom_pcie_syscore_ops);
> > > +
> > > +	/*
> > > +	 * Add the qcom_pcie list of each PCIe instance probed to
> > > +	 * the global list so that we use it iterate through each PCIe
> > > +	 * instance in the syscore ops.
> > > +	 */
> > > +	list_add_tail(&pcie->list, &qcom_pcie_list);
> > > +
> > >   	return 0;
> > >   err_phy_exit:
> > > @@ -1731,6 +1806,69 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > >   	return ret;
> > >   }
> > > +static int __maybe_unused qcom_pcie_pm_suspend(struct qcom_pcie *pcie)
> > > +{
> > > +	u32 val;
> > > +	struct dw_pcie *pci = pcie->pci;
> > > +	struct device *dev = pci->dev;
> > > +
> > > +	if (!pcie->cfg->supports_system_suspend)
> > > +		return 0;
> > This check could be done in qcom_pcie_syscore_op_suspend()
> sure , we will take of it in next patch.
> > 
> > > +
> > > +	/* if the link is not active turn off clocks */
> > > +	if (!dw_pcie_link_up(pci)) {
> > > +		dev_info(dev, "Link is not active\n");
> > level info seems to verbose, it's not particularly interesting that the
> > link is not active, except for debugging.
> sure , we will take of it in next patch.
> > 
> > > +		goto suspend;
> > > +	}
> > > +
> > > +	/* if the link is not in l1ss don't turn off clocks */
> > > +	val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
> > > +	if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> > > +		dev_warn(dev, "Link is not in L1ss\n");
> > > +		return 0;
> > > +	}
> > I think the following would be clearer:
> > 
> >    	if (dw_pcie_link_up(pci)) {
> > 		/* if the link is not in l1ss don't turn off clocks */
> > 		val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
> > 		if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> > 			dev_warn(dev, "Link is not in L1ss\n");
> > 			return 0;
> > 		}
> > 	} else {
> > 		dev_dbg(dev, "Link is not active\n");
> > 	}
> But as we are adding retry logic in other patch  with while loop this logic
> will not be applicable.
> > > +
> > > +suspend:
> > > +	if (pcie->cfg->ops->suspend)
> > > +		pcie->cfg->ops->suspend(pcie);
> > > +
> > > +	pcie->is_suspended = true;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int __maybe_unused qcom_pcie_pm_resume(struct qcom_pcie *pcie)
> > > +{
> > > +	if (!pcie->is_suspended)
> > > +		return 0;
> > > +
> > > +	if (pcie->cfg->ops->resume)
> > > +		pcie->cfg->ops->resume(pcie);
> > > +
> > > +	pcie->is_suspended = false;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int __maybe_unused qcom_pcie_syscore_op_suspend(void)
> > > +{
> > > +	struct qcom_pcie *qcom_pcie;
> > > +
> > > +	list_for_each_entry(qcom_pcie, &qcom_pcie_list, list) {
> > > +		qcom_pcie_pm_suspend(qcom_pcie);
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > > +static void __maybe_unused qcom_pcie_syscore_op_resume(void)
> > > +{
> > > +	struct qcom_pcie *qcom_pcie;
> > > +
> > > +	list_for_each_entry(qcom_pcie, &qcom_pcie_list, list) {
> > > +		qcom_pcie_pm_resume(qcom_pcie);
> > > +	}
> > > +}
> > > +
> > >   static const struct of_device_id qcom_pcie_match[] = {
> > >   	{ .compatible = "qcom,pcie-apq8064", .data = &cfg_2_1_0 },
> > >   	{ .compatible = "qcom,pcie-apq8084", .data = &cfg_1_0_0 },
> > > @@ -1742,7 +1880,7 @@ static const struct of_device_id qcom_pcie_match[] = {
> > >   	{ .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 },
> > >   	{ .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 },
> > >   	{ .compatible = "qcom,pcie-sa8540p", .data = &cfg_1_9_0 },
> > > -	{ .compatible = "qcom,pcie-sc7280", .data = &cfg_1_9_0 },
> > > +	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
> > >   	{ .compatible = "qcom,pcie-sc8180x", .data = &cfg_1_9_0 },
> > >   	{ .compatible = "qcom,pcie-sc8280xp", .data = &cfg_1_9_0 },
> > >   	{ .compatible = "qcom,pcie-sdm845", .data = &cfg_2_7_0 },
> > > -- 
> > > 2.7.4
> > > 

Re: [PATCH v6 1/5] PCI: qcom: Add system suspend and resume support

[Manivannan Sadhasivam]

On Fri, Sep 09, 2022 at 05:31:18PM +0000, Matthias Kaehlcke wrote:
> On Fri, Sep 09, 2022 at 02:14:40PM +0530, Krishna chaitanya chundru wrote:
> > Add suspend and resume syscore ops.
> > 
> > When system suspends and if the link is in L1ss, disable the clocks
> > and power down the phy so that system enters into low power state to
> > save the maximum power. And when the system resumes, enable the clocks
> > back and power on phy if they are disabled in the suspend path.
> > 
> > we are doing this only when link is in l1ss but not in L2/L3 as
> > nowhere we are forcing link to L2/L3 by sending PME turn off.
> > 
> > is_suspended flag indicates if the clocks are disabled in the suspend
> > path or not.
> > 
> > There is access to Ep PCIe space to mask MSI/MSIX after pm suspend ops
> > (getting hit by affinity changes while making CPUs offline during suspend,
> > this will happen after devices are suspended (all phases of suspend ops)).
> > When registered with pm ops there is a crash due to un-clocked access,
> > as in the pm suspend op clocks are disabled. So, registering with syscore
> > ops which will called after making CPUs offline.
> 
> My knowledge of PCI is limited, but given the issues you are seeing which
> don't seem to impact other DWC drivers I wonder if keeping the link in
> l1ss is the right thing to do. The intel, tegra and imx6 drivers all turn
> the PME off, which IIUC results in the link to transition ot L2 or L3.
> Shouldn't the QC driver do the same?

Some PCI endpoint drivers (NVMe, WLAN) expects the device to be powered ON
during system suspend. So transitioning the link to L2/L3 will put them in
power down mode (if Vaux is not supported) and will break those drivers during
resume.

IIRC, Tegra is also facing this issue with NVMe.

Thanks,
Mani

> 
> Some more comments inline, for if the current approach moves forward.
> 
> > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > ---
> > changes since v5:
> > 	- Rebasing the code and replaced pm ops with syscore ops as
> > 	  we are getting acciess to pci region after pm ops. syscore ops
> > 	  will called after disabling non boot cpus and there is no pci
> > 	  access after that.
> > Changes since v4:
> > 	- Rebasing the code and removed the supports_system_suspend flag
> > 	- in the resume path as is_suspended will serve its purpose.
> > Changes since v3:
> > 	- Powering down the phy in suspend and powering it on resume to
> > 	  acheive maximum power savings.
> > Changes since v2:
> > 	- Replaced the enable, disable clks ops with suspend and resume
> > 	- Renamed support_pm_opsi flag  with supports_system_suspend.
> > Changes since v1:
> > 	- Fixed compilation errors.
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 140 ++++++++++++++++++++++++++++++++-
> >  1 file changed, 139 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 39ca06f..6e04d0d 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -27,6 +27,7 @@
> >  #include <linux/reset.h>
> >  #include <linux/slab.h>
> >  #include <linux/types.h>
> > +#include <linux/syscore_ops.h>
> >  
> >  #include "../../pci.h"
> >  #include "pcie-designware.h"
> > @@ -44,6 +45,9 @@
> >  #define PCIE20_PARF_PM_CTRL			0x20
> >  #define REQ_NOT_ENTR_L1				BIT(5)
> >  
> > +#define PCIE20_PARF_PM_STTS			0x24
> > +#define PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB	BIT(8)
> > +
> >  #define PCIE20_PARF_PHY_CTRL			0x40
> >  #define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK	GENMASK(20, 16)
> >  #define PHY_CTRL_PHY_TX0_TERM_OFFSET(x)		((x) << 16)
> > @@ -122,6 +126,8 @@
> >  
> >  #define QCOM_PCIE_CRC8_POLYNOMIAL (BIT(2) | BIT(1) | BIT(0))
> >  
> > +static LIST_HEAD(qcom_pcie_list);
> > +
> >  struct qcom_pcie_resources_2_1_0 {
> >  	struct clk_bulk_data clks[QCOM_PCIE_2_1_0_MAX_CLOCKS];
> >  	struct reset_control *pci_reset;
> > @@ -211,13 +217,21 @@ struct qcom_pcie_ops {
> >  	void (*post_deinit)(struct qcom_pcie *pcie);
> >  	void (*ltssm_enable)(struct qcom_pcie *pcie);
> >  	int (*config_sid)(struct qcom_pcie *pcie);
> > +	int (*suspend)(struct qcom_pcie *pcie);
> > +	int (*resume)(struct qcom_pcie *pcie);
> >  };
> >  
> >  struct qcom_pcie_cfg {
> >  	const struct qcom_pcie_ops *ops;
> > +	/*
> > +	 * Flag ensures which devices will turn off clks, phy
> > +	 * in system suspend.
> > +	 */
> > +	unsigned int supports_system_suspend:1;
> >  };
> >  
> >  struct qcom_pcie {
> > +	struct list_head list;	/* list to probed instances */
> >  	struct dw_pcie *pci;
> >  	void __iomem *parf;			/* DT parf */
> >  	void __iomem *elbi;			/* DT elbi */
> > @@ -225,10 +239,14 @@ struct qcom_pcie {
> >  	struct phy *phy;
> >  	struct gpio_desc *reset;
> >  	const struct qcom_pcie_cfg *cfg;
> > +	unsigned int is_suspended:1;
> >  };
> >  
> >  #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
> >  
> > +static int __maybe_unused qcom_pcie_syscore_op_suspend(void);
> > +static void __maybe_unused qcom_pcie_syscore_op_resume(void);
> > +
> >  static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> >  {
> >  	gpiod_set_value_cansleep(pcie->reset, 1);
> > @@ -1301,6 +1319,28 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
> >  	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> >  }
> >  
> > +static int qcom_pcie_resume_2_7_0(struct qcom_pcie *pcie)
> > +{
> > +	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > +	int ret;
> > +
> > +	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
> > +
> > +	phy_power_on(pcie->phy);
> > +
> > +	return ret;
> > +}
> > +
> > +static int qcom_pcie_suspend_2_7_0(struct qcom_pcie *pcie)
> > +{
> > +	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > +
> > +	phy_power_off(pcie->phy);
> > +
> > +	clk_bulk_disable_unprepare(res->num_clks, res->clks);
> > +	return 0;
> > +}
> > +
> >  static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
> >  {
> >  	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> > @@ -1594,6 +1634,8 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
> >  	.deinit = qcom_pcie_deinit_2_7_0,
> >  	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> >  	.config_sid = qcom_pcie_config_sid_sm8250,
> > +	.suspend = qcom_pcie_suspend_2_7_0,
> > +	.resume = qcom_pcie_resume_2_7_0,
> >  };
> >  
> >  /* Qcom IP rev.: 2.9.0  Synopsys IP rev.: 5.00a */
> > @@ -1613,6 +1655,11 @@ static const struct qcom_pcie_cfg cfg_1_9_0 = {
> >  	.ops = &ops_1_9_0,
> >  };
> >  
> > +static const struct qcom_pcie_cfg sc7280_cfg = {
> > +	.ops = &ops_1_9_0,
> > +	.supports_system_suspend = true,
> > +};
> > +
> >  static const struct qcom_pcie_cfg cfg_2_1_0 = {
> >  	.ops = &ops_2_1_0,
> >  };
> > @@ -1642,6 +1689,23 @@ static const struct dw_pcie_ops dw_pcie_ops = {
> >  	.start_link = qcom_pcie_start_link,
> >  };
> >  
> > +/*
> > + * There is access to Ep PCIe space to mask MSI/MSIX after pm suspend
> > + * ops.(getting hit by affinity changes while making CPUs offline during
> > + * suspend, this will happen after devices are suspended
> > + * (all phases of suspend ops)).
> > + *
> > + * When registered with pm ops there is a crash due to un-clocked access,
> > + * as in the pm suspend op clocks are disabled.
> > + *
> > + * So, registering with syscore ops which will called after making
> > + * CPU's offline.
> > + */
> > +static struct syscore_ops qcom_pcie_syscore_ops = {
> > +	.suspend = qcom_pcie_syscore_op_suspend,
> > +	.resume = qcom_pcie_syscore_op_resume,
> > +};
> > +
> >  static int qcom_pcie_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > @@ -1720,6 +1784,17 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> >  		goto err_phy_exit;
> >  	}
> >  
> > +	/* Register for syscore ops only when first instance probed */
> > +	if (list_empty(&qcom_pcie_list))
> > +		register_syscore_ops(&qcom_pcie_syscore_ops);
> > +
> > +	/*
> > +	 * Add the qcom_pcie list of each PCIe instance probed to
> > +	 * the global list so that we use it iterate through each PCIe
> > +	 * instance in the syscore ops.
> > +	 */
> > +	list_add_tail(&pcie->list, &qcom_pcie_list);
> > +
> >  	return 0;
> >  
> >  err_phy_exit:
> > @@ -1731,6 +1806,69 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> >  	return ret;
> >  }
> >  
> > +static int __maybe_unused qcom_pcie_pm_suspend(struct qcom_pcie *pcie)
> > +{
> > +	u32 val;
> > +	struct dw_pcie *pci = pcie->pci;
> > +	struct device *dev = pci->dev;
> > +
> > +	if (!pcie->cfg->supports_system_suspend)
> > +		return 0;
> 
> This check could be done in qcom_pcie_syscore_op_suspend()
> 
> > +
> > +	/* if the link is not active turn off clocks */
> > +	if (!dw_pcie_link_up(pci)) {
> > +		dev_info(dev, "Link is not active\n");
> 
> level info seems to verbose, it's not particularly interesting that the
> link is not active, except for debugging.
> 
> > +		goto suspend;
> > +	}
> > +
> > +	/* if the link is not in l1ss don't turn off clocks */
> > +	val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
> > +	if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> > +		dev_warn(dev, "Link is not in L1ss\n");
> > +		return 0;
> > +	}
> 
> I think the following would be clearer:
> 
>   	if (dw_pcie_link_up(pci)) {
> 		/* if the link is not in l1ss don't turn off clocks */
> 		val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
> 		if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> 			dev_warn(dev, "Link is not in L1ss\n");
> 			return 0;
> 		}
> 	} else {
> 		dev_dbg(dev, "Link is not active\n");
> 	}
> 
> > +
> > +suspend:
> > +	if (pcie->cfg->ops->suspend)
> > +		pcie->cfg->ops->suspend(pcie);
> > +
> > +	pcie->is_suspended = true;
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused qcom_pcie_pm_resume(struct qcom_pcie *pcie)
> > +{
> > +	if (!pcie->is_suspended)
> > +		return 0;
> > +
> > +	if (pcie->cfg->ops->resume)
> > +		pcie->cfg->ops->resume(pcie);
> > +
> > +	pcie->is_suspended = false;
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused qcom_pcie_syscore_op_suspend(void)
> > +{
> > +	struct qcom_pcie *qcom_pcie;
> > +
> > +	list_for_each_entry(qcom_pcie, &qcom_pcie_list, list) {
> > +		qcom_pcie_pm_suspend(qcom_pcie);
> > +	}
> > +	return 0;
> > +}
> > +
> > +static void __maybe_unused qcom_pcie_syscore_op_resume(void)
> > +{
> > +	struct qcom_pcie *qcom_pcie;
> > +
> > +	list_for_each_entry(qcom_pcie, &qcom_pcie_list, list) {
> > +		qcom_pcie_pm_resume(qcom_pcie);
> > +	}
> > +}
> > +
> >  static const struct of_device_id qcom_pcie_match[] = {
> >  	{ .compatible = "qcom,pcie-apq8064", .data = &cfg_2_1_0 },
> >  	{ .compatible = "qcom,pcie-apq8084", .data = &cfg_1_0_0 },
> > @@ -1742,7 +1880,7 @@ static const struct of_device_id qcom_pcie_match[] = {
> >  	{ .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 },
> >  	{ .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 },
> >  	{ .compatible = "qcom,pcie-sa8540p", .data = &cfg_1_9_0 },
> > -	{ .compatible = "qcom,pcie-sc7280", .data = &cfg_1_9_0 },
> > +	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
> >  	{ .compatible = "qcom,pcie-sc8180x", .data = &cfg_1_9_0 },
> >  	{ .compatible = "qcom,pcie-sc8280xp", .data = &cfg_1_9_0 },
> >  	{ .compatible = "qcom,pcie-sdm845", .data = &cfg_2_7_0 },
> > -- 
> > 2.7.4
> > 

-- 
மணிவண்ணன் சதாசிவம்

[PATCH v6 2/5] PCI: qcom: Add retry logic for link to be stable in L1ss

[Krishna chaitanya chundru]

Some specific devices are taking time to settle the link in L1ss.
So added a retry logic before returning from the suspend op.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 6e04d0d..15c2067 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1809,26 +1809,40 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 static int __maybe_unused qcom_pcie_pm_suspend(struct qcom_pcie *pcie)
 {
 	u32 val;
+	ktime_t timeout, start;
 	struct dw_pcie *pci = pcie->pci;
 	struct device *dev = pci->dev;
 
 	if (!pcie->cfg->supports_system_suspend)
 		return 0;
 
-	/* if the link is not active turn off clocks */
-	if (!dw_pcie_link_up(pci)) {
-		dev_info(dev, "Link is not active\n");
-		goto suspend;
-	}
+	start = ktime_get();
+	/* Wait max 200 ms */
+	timeout = ktime_add_ms(start, 200);
 
-	/* if the link is not in l1ss don't turn off clocks */
-	val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
-	if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
-		dev_warn(dev, "Link is not in L1ss\n");
-		return 0;
+	while (1) {
+
+		if (!dw_pcie_link_up(pci)) {
+			dev_warn(dev, "Link is not active\n");
+			break;
+		}
+
+		/* if the link is not in l1ss don't turn off clocks */
+		val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
+		if ((val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
+			dev_dbg(dev, "Link enters L1ss after %d  ms\n",
+					ktime_to_ms(ktime_get() - start));
+			break;
+		}
+
+		if (ktime_after(ktime_get(), timeout)) {
+			dev_warn(dev, "Link is not in L1ss\n");
+			return 0;
+		}
+
+		udelay(1000);
 	}
 
-suspend:
 	if (pcie->cfg->ops->suspend)
 		pcie->cfg->ops->suspend(pcie);
 
-- 
2.7.4

Re: [PATCH v6 2/5] PCI: qcom: Add retry logic for link to be stable in L1ss

[Bjorn Helgaas]

On Fri, Sep 09, 2022 at 02:14:41PM +0530, Krishna chaitanya chundru wrote:
> Some specific devices are taking time to settle the link in L1ss.
> So added a retry logic before returning from the suspend op.

"L1ss" is not a state.  If you mean "L1.1" or "L1.2", say that.  Also
in code comments below.

s/So added a/Add/

What are these specific devices?  Is this a qcom controller defect?
An endpoint defect that should be addressed via some kind of generic
quirk?

> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++-----------
>  1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 6e04d0d..15c2067 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1809,26 +1809,40 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  static int __maybe_unused qcom_pcie_pm_suspend(struct qcom_pcie *pcie)
>  {
>  	u32 val;
> +	ktime_t timeout, start;
>  	struct dw_pcie *pci = pcie->pci;
>  	struct device *dev = pci->dev;
>  
>  	if (!pcie->cfg->supports_system_suspend)
>  		return 0;
>  
> -	/* if the link is not active turn off clocks */
> -	if (!dw_pcie_link_up(pci)) {
> -		dev_info(dev, "Link is not active\n");
> -		goto suspend;
> -	}
> +	start = ktime_get();
> +	/* Wait max 200 ms */
> +	timeout = ktime_add_ms(start, 200);
>  
> -	/* if the link is not in l1ss don't turn off clocks */
> -	val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
> -	if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> -		dev_warn(dev, "Link is not in L1ss\n");
> -		return 0;
> +	while (1) {
> +
> +		if (!dw_pcie_link_up(pci)) {
> +			dev_warn(dev, "Link is not active\n");
> +			break;
> +		}
> +
> +		/* if the link is not in l1ss don't turn off clocks */
> +		val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
> +		if ((val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> +			dev_dbg(dev, "Link enters L1ss after %d  ms\n",
> +					ktime_to_ms(ktime_get() - start));
> +			break;
> +		}
> +
> +		if (ktime_after(ktime_get(), timeout)) {
> +			dev_warn(dev, "Link is not in L1ss\n");
> +			return 0;
> +		}
> +
> +		udelay(1000);
>  	}
>  
> -suspend:
>  	if (pcie->cfg->ops->suspend)
>  		pcie->cfg->ops->suspend(pcie);
>  
> -- 
> 2.7.4
> 

Re: [PATCH v6 2/5] PCI: qcom: Add retry logic for link to be stable in L1ss

[Krishna Chaitanya Chundru]

On 9/10/2022 1:20 AM, Bjorn Helgaas wrote:
> On Fri, Sep 09, 2022 at 02:14:41PM +0530, Krishna chaitanya chundru wrote:
>> Some specific devices are taking time to settle the link in L1ss.
>> So added a retry logic before returning from the suspend op.
> "L1ss" is not a state.  If you mean "L1.1" or "L1.2", say that.  Also
> in code comments below.
Yes L1ss means L1.2 and L1.2 We will update it next patch
> s/So added a/Add/
>
> What are these specific devices?  Is this a qcom controller defect?
> An endpoint defect that should be addressed via some kind of generic
> quirk?

This is depending up on the endpoint devices and it varies to device to 
device.

We are thinking this is not a defect if there is some traffic in the 
link the link will

not go to L1ss .

>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++-----------
>>   1 file changed, 25 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 6e04d0d..15c2067 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -1809,26 +1809,40 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>   static int __maybe_unused qcom_pcie_pm_suspend(struct qcom_pcie *pcie)
>>   {
>>   	u32 val;
>> +	ktime_t timeout, start;
>>   	struct dw_pcie *pci = pcie->pci;
>>   	struct device *dev = pci->dev;
>>   
>>   	if (!pcie->cfg->supports_system_suspend)
>>   		return 0;
>>   
>> -	/* if the link is not active turn off clocks */
>> -	if (!dw_pcie_link_up(pci)) {
>> -		dev_info(dev, "Link is not active\n");
>> -		goto suspend;
>> -	}
>> +	start = ktime_get();
>> +	/* Wait max 200 ms */
>> +	timeout = ktime_add_ms(start, 200);
>>   
>> -	/* if the link is not in l1ss don't turn off clocks */
>> -	val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
>> -	if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
>> -		dev_warn(dev, "Link is not in L1ss\n");
>> -		return 0;
>> +	while (1) {
>> +
>> +		if (!dw_pcie_link_up(pci)) {
>> +			dev_warn(dev, "Link is not active\n");
>> +			break;
>> +		}
>> +
>> +		/* if the link is not in l1ss don't turn off clocks */
>> +		val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
>> +		if ((val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
>> +			dev_dbg(dev, "Link enters L1ss after %d  ms\n",
>> +					ktime_to_ms(ktime_get() - start));
>> +			break;
>> +		}
>> +
>> +		if (ktime_after(ktime_get(), timeout)) {
>> +			dev_warn(dev, "Link is not in L1ss\n");
>> +			return 0;
>> +		}
>> +
>> +		udelay(1000);
>>   	}
>>   
>> -suspend:
>>   	if (pcie->cfg->ops->suspend)
>>   		pcie->cfg->ops->suspend(pcie);
>>   
>> -- 
>> 2.7.4
>>

Re: [PATCH v6 2/5] PCI: qcom: Add retry logic for link to be stable in L1ss

[Manivannan Sadhasivam]

On Mon, Sep 12, 2022 at 09:39:36PM +0530, Krishna Chaitanya Chundru wrote:
> 
> On 9/10/2022 1:20 AM, Bjorn Helgaas wrote:
> > On Fri, Sep 09, 2022 at 02:14:41PM +0530, Krishna chaitanya chundru wrote:
> > > Some specific devices are taking time to settle the link in L1ss.
> > > So added a retry logic before returning from the suspend op.
> > "L1ss" is not a state.  If you mean "L1.1" or "L1.2", say that.  Also
> > in code comments below.
> Yes L1ss means L1.2 and L1.2 We will update it next patch
> > s/So added a/Add/
> > 
> > What are these specific devices?  Is this a qcom controller defect?
> > An endpoint defect that should be addressed via some kind of generic
> > quirk?
> 
> This is depending up on the endpoint devices and it varies to device to
> device.
> 

Can we identify the source of the traffic? Is the NVMe driver not
flushing it's queues correctly?

> We are thinking this is not a defect if there is some traffic in the link
> the link will
> 
> not go to L1ss .
> 

Is this hack still required even after switching to syscore ops?

Thanks,
Mani

> > 
> > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > > ---
> > >   drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++-----------
> > >   1 file changed, 25 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > index 6e04d0d..15c2067 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > @@ -1809,26 +1809,40 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > >   static int __maybe_unused qcom_pcie_pm_suspend(struct qcom_pcie *pcie)
> > >   {
> > >   	u32 val;
> > > +	ktime_t timeout, start;
> > >   	struct dw_pcie *pci = pcie->pci;
> > >   	struct device *dev = pci->dev;
> > >   	if (!pcie->cfg->supports_system_suspend)
> > >   		return 0;
> > > -	/* if the link is not active turn off clocks */
> > > -	if (!dw_pcie_link_up(pci)) {
> > > -		dev_info(dev, "Link is not active\n");
> > > -		goto suspend;
> > > -	}
> > > +	start = ktime_get();
> > > +	/* Wait max 200 ms */
> > > +	timeout = ktime_add_ms(start, 200);
> > > -	/* if the link is not in l1ss don't turn off clocks */
> > > -	val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
> > > -	if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> > > -		dev_warn(dev, "Link is not in L1ss\n");
> > > -		return 0;
> > > +	while (1) {
> > > +
> > > +		if (!dw_pcie_link_up(pci)) {
> > > +			dev_warn(dev, "Link is not active\n");
> > > +			break;
> > > +		}
> > > +
> > > +		/* if the link is not in l1ss don't turn off clocks */
> > > +		val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
> > > +		if ((val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> > > +			dev_dbg(dev, "Link enters L1ss after %d  ms\n",
> > > +					ktime_to_ms(ktime_get() - start));
> > > +			break;
> > > +		}
> > > +
> > > +		if (ktime_after(ktime_get(), timeout)) {
> > > +			dev_warn(dev, "Link is not in L1ss\n");
> > > +			return 0;
> > > +		}
> > > +
> > > +		udelay(1000);
> > >   	}
> > > -suspend:
> > >   	if (pcie->cfg->ops->suspend)
> > >   		pcie->cfg->ops->suspend(pcie);
> > > -- 
> > > 2.7.4
> > > 

Re: [PATCH v6 2/5] PCI: qcom: Add retry logic for link to be stable in L1ss

[Krishna Chaitanya Chundru]

On 9/12/2022 11:03 PM, Manivannan Sadhasivam wrote:
> On Mon, Sep 12, 2022 at 09:39:36PM +0530, Krishna Chaitanya Chundru wrote:
>> On 9/10/2022 1:20 AM, Bjorn Helgaas wrote:
>>> On Fri, Sep 09, 2022 at 02:14:41PM +0530, Krishna chaitanya chundru wrote:
>>>> Some specific devices are taking time to settle the link in L1ss.
>>>> So added a retry logic before returning from the suspend op.
>>> "L1ss" is not a state.  If you mean "L1.1" or "L1.2", say that.  Also
>>> in code comments below.
>> Yes L1ss means L1.2 and L1.2 We will update it next patch
>>> s/So added a/Add/
>>>
>>> What are these specific devices?  Is this a qcom controller defect?
>>> An endpoint defect that should be addressed via some kind of generic
>>> quirk?
>> This is depending up on the endpoint devices and it varies to device to
>> device.
>>
> Can we identify the source of the traffic? Is the NVMe driver not
> flushing it's queues correctly?

We found that it is not from nvme data, we are seeing some physical 
layer activity on the

protocol analyzer.

>
>> We are thinking this is not a defect if there is some traffic in the link
>> the link will
>>
>> not go to L1ss .
>>
> Is this hack still required even after switching to syscore ops?
>
> Thanks,
> Mani

Yes, mani it is still required. And just before this sycore ops there 
will be a pci transaction to

mask msix interrupts.

>>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>>>> ---
>>>>    drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++-----------
>>>>    1 file changed, 25 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>>>> index 6e04d0d..15c2067 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>>> @@ -1809,26 +1809,40 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>>>    static int __maybe_unused qcom_pcie_pm_suspend(struct qcom_pcie *pcie)
>>>>    {
>>>>    	u32 val;
>>>> +	ktime_t timeout, start;
>>>>    	struct dw_pcie *pci = pcie->pci;
>>>>    	struct device *dev = pci->dev;
>>>>    	if (!pcie->cfg->supports_system_suspend)
>>>>    		return 0;
>>>> -	/* if the link is not active turn off clocks */
>>>> -	if (!dw_pcie_link_up(pci)) {
>>>> -		dev_info(dev, "Link is not active\n");
>>>> -		goto suspend;
>>>> -	}
>>>> +	start = ktime_get();
>>>> +	/* Wait max 200 ms */
>>>> +	timeout = ktime_add_ms(start, 200);
>>>> -	/* if the link is not in l1ss don't turn off clocks */
>>>> -	val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
>>>> -	if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
>>>> -		dev_warn(dev, "Link is not in L1ss\n");
>>>> -		return 0;
>>>> +	while (1) {
>>>> +
>>>> +		if (!dw_pcie_link_up(pci)) {
>>>> +			dev_warn(dev, "Link is not active\n");
>>>> +			break;
>>>> +		}
>>>> +
>>>> +		/* if the link is not in l1ss don't turn off clocks */
>>>> +		val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
>>>> +		if ((val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
>>>> +			dev_dbg(dev, "Link enters L1ss after %d  ms\n",
>>>> +					ktime_to_ms(ktime_get() - start));
>>>> +			break;
>>>> +		}
>>>> +
>>>> +		if (ktime_after(ktime_get(), timeout)) {
>>>> +			dev_warn(dev, "Link is not in L1ss\n");
>>>> +			return 0;
>>>> +		}
>>>> +
>>>> +		udelay(1000);
>>>>    	}
>>>> -suspend:
>>>>    	if (pcie->cfg->ops->suspend)
>>>>    		pcie->cfg->ops->suspend(pcie);
>>>> -- 
>>>> 2.7.4
>>>>

Re: [PATCH v6 2/5] PCI: qcom: Add retry logic for link to be stable in L1ss

[Manivannan Sadhasivam]

On Tue, Sep 13, 2022 at 07:54:22PM +0530, Krishna Chaitanya Chundru wrote:
> 
> On 9/12/2022 11:03 PM, Manivannan Sadhasivam wrote:
> > On Mon, Sep 12, 2022 at 09:39:36PM +0530, Krishna Chaitanya Chundru wrote:
> > > On 9/10/2022 1:20 AM, Bjorn Helgaas wrote:
> > > > On Fri, Sep 09, 2022 at 02:14:41PM +0530, Krishna chaitanya chundru wrote:
> > > > > Some specific devices are taking time to settle the link in L1ss.
> > > > > So added a retry logic before returning from the suspend op.
> > > > "L1ss" is not a state.  If you mean "L1.1" or "L1.2", say that.  Also
> > > > in code comments below.
> > > Yes L1ss means L1.2 and L1.2 We will update it next patch
> > > > s/So added a/Add/
> > > > 
> > > > What are these specific devices?  Is this a qcom controller defect?
> > > > An endpoint defect that should be addressed via some kind of generic
> > > > quirk?
> > > This is depending up on the endpoint devices and it varies to device to
> > > device.
> > > 
> > Can we identify the source of the traffic? Is the NVMe driver not
> > flushing it's queues correctly?
> 
> We found that it is not from nvme data, we are seeing some physical layer
> activity on the
> 
> protocol analyzer.
> 

Okay

> > 
> > > We are thinking this is not a defect if there is some traffic in the link
> > > the link will
> > > 
> > > not go to L1ss .
> > > 
> > Is this hack still required even after switching to syscore ops?
> > 
> > Thanks,
> > Mani
> 
> Yes, mani it is still required. And just before this sycore ops there will
> be a pci transaction to
> 
> mask msix interrupts.
> 

Hmm. I'm getting slightly confused here. What really happens when you do
the resource teardown during suspend and the link has not entered L1SS?

Since PHY is powered by MX domain, I'm wondering why we should wait for
the link to be in L1SS?

Thanks,
Mani

> > > > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > > > > ---
> > > > >    drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++-----------
> > > > >    1 file changed, 25 insertions(+), 11 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > > index 6e04d0d..15c2067 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > > @@ -1809,26 +1809,40 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > > > >    static int __maybe_unused qcom_pcie_pm_suspend(struct qcom_pcie *pcie)
> > > > >    {
> > > > >    	u32 val;
> > > > > +	ktime_t timeout, start;
> > > > >    	struct dw_pcie *pci = pcie->pci;
> > > > >    	struct device *dev = pci->dev;
> > > > >    	if (!pcie->cfg->supports_system_suspend)
> > > > >    		return 0;
> > > > > -	/* if the link is not active turn off clocks */
> > > > > -	if (!dw_pcie_link_up(pci)) {
> > > > > -		dev_info(dev, "Link is not active\n");
> > > > > -		goto suspend;
> > > > > -	}
> > > > > +	start = ktime_get();
> > > > > +	/* Wait max 200 ms */
> > > > > +	timeout = ktime_add_ms(start, 200);
> > > > > -	/* if the link is not in l1ss don't turn off clocks */
> > > > > -	val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
> > > > > -	if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> > > > > -		dev_warn(dev, "Link is not in L1ss\n");
> > > > > -		return 0;
> > > > > +	while (1) {
> > > > > +
> > > > > +		if (!dw_pcie_link_up(pci)) {
> > > > > +			dev_warn(dev, "Link is not active\n");
> > > > > +			break;
> > > > > +		}
> > > > > +
> > > > > +		/* if the link is not in l1ss don't turn off clocks */
> > > > > +		val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
> > > > > +		if ((val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> > > > > +			dev_dbg(dev, "Link enters L1ss after %d  ms\n",
> > > > > +					ktime_to_ms(ktime_get() - start));
> > > > > +			break;
> > > > > +		}
> > > > > +
> > > > > +		if (ktime_after(ktime_get(), timeout)) {
> > > > > +			dev_warn(dev, "Link is not in L1ss\n");
> > > > > +			return 0;
> > > > > +		}
> > > > > +
> > > > > +		udelay(1000);
> > > > >    	}
> > > > > -suspend:
> > > > >    	if (pcie->cfg->ops->suspend)
> > > > >    		pcie->cfg->ops->suspend(pcie);
> > > > > -- 
> > > > > 2.7.4
> > > > > 

Re: [PATCH v6 2/5] PCI: qcom: Add retry logic for link to be stable in L1ss

[Krishna Chaitanya Chundru]

On 9/13/2022 10:09 PM, Manivannan Sadhasivam wrote:
> On Tue, Sep 13, 2022 at 07:54:22PM +0530, Krishna Chaitanya Chundru wrote:
>> On 9/12/2022 11:03 PM, Manivannan Sadhasivam wrote:
>>> On Mon, Sep 12, 2022 at 09:39:36PM +0530, Krishna Chaitanya Chundru wrote:
>>>> On 9/10/2022 1:20 AM, Bjorn Helgaas wrote:
>>>>> On Fri, Sep 09, 2022 at 02:14:41PM +0530, Krishna chaitanya chundru wrote:
>>>>>> Some specific devices are taking time to settle the link in L1ss.
>>>>>> So added a retry logic before returning from the suspend op.
>>>>> "L1ss" is not a state.  If you mean "L1.1" or "L1.2", say that.  Also
>>>>> in code comments below.
>>>> Yes L1ss means L1.2 and L1.2 We will update it next patch
>>>>> s/So added a/Add/
>>>>>
>>>>> What are these specific devices?  Is this a qcom controller defect?
>>>>> An endpoint defect that should be addressed via some kind of generic
>>>>> quirk?
>>>> This is depending up on the endpoint devices and it varies to device to
>>>> device.
>>>>
>>> Can we identify the source of the traffic? Is the NVMe driver not
>>> flushing it's queues correctly?
>> We found that it is not from nvme data, we are seeing some physical layer
>> activity on the
>>
>> protocol analyzer.
>>
> Okay
>
>>>> We are thinking this is not a defect if there is some traffic in the link
>>>> the link will
>>>>
>>>> not go to L1ss .
>>>>
>>> Is this hack still required even after switching to syscore ops?
>>>
>>> Thanks,
>>> Mani
>> Yes, mani it is still required. And just before this sycore ops there will
>> be a pci transaction to
>>
>> mask msix interrupts.
>>
> Hmm. I'm getting slightly confused here. What really happens when you do
> the resource teardown during suspend and the link has not entered L1SS?
>
> Since PHY is powered by MX domain, I'm wondering why we should wait for
> the link to be in L1SS?
>
> Thanks,
> Mani

Mani, we need to turn off the link only after link entered in to L1ss. 
If we do before that

some transactions will be disturbed and we see a link down.

Mx power rail will control digital logic of the PHY and tries to retain 
the link state only,

The analog logic is controlled by the CX rail only, so when the link is 
in L1ss only we turn off

clks and phy.

>>>>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>>>>>> ---
>>>>>>     drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++-----------
>>>>>>     1 file changed, 25 insertions(+), 11 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>>>>>> index 6e04d0d..15c2067 100644
>>>>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>>>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>>>>> @@ -1809,26 +1809,40 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>>>>>     static int __maybe_unused qcom_pcie_pm_suspend(struct qcom_pcie *pcie)
>>>>>>     {
>>>>>>     	u32 val;
>>>>>> +	ktime_t timeout, start;
>>>>>>     	struct dw_pcie *pci = pcie->pci;
>>>>>>     	struct device *dev = pci->dev;
>>>>>>     	if (!pcie->cfg->supports_system_suspend)
>>>>>>     		return 0;
>>>>>> -	/* if the link is not active turn off clocks */
>>>>>> -	if (!dw_pcie_link_up(pci)) {
>>>>>> -		dev_info(dev, "Link is not active\n");
>>>>>> -		goto suspend;
>>>>>> -	}
>>>>>> +	start = ktime_get();
>>>>>> +	/* Wait max 200 ms */
>>>>>> +	timeout = ktime_add_ms(start, 200);
>>>>>> -	/* if the link is not in l1ss don't turn off clocks */
>>>>>> -	val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
>>>>>> -	if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
>>>>>> -		dev_warn(dev, "Link is not in L1ss\n");
>>>>>> -		return 0;
>>>>>> +	while (1) {
>>>>>> +
>>>>>> +		if (!dw_pcie_link_up(pci)) {
>>>>>> +			dev_warn(dev, "Link is not active\n");
>>>>>> +			break;
>>>>>> +		}
>>>>>> +
>>>>>> +		/* if the link is not in l1ss don't turn off clocks */
>>>>>> +		val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
>>>>>> +		if ((val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
>>>>>> +			dev_dbg(dev, "Link enters L1ss after %d  ms\n",
>>>>>> +					ktime_to_ms(ktime_get() - start));
>>>>>> +			break;
>>>>>> +		}
>>>>>> +
>>>>>> +		if (ktime_after(ktime_get(), timeout)) {
>>>>>> +			dev_warn(dev, "Link is not in L1ss\n");
>>>>>> +			return 0;
>>>>>> +		}
>>>>>> +
>>>>>> +		udelay(1000);
>>>>>>     	}
>>>>>> -suspend:
>>>>>>     	if (pcie->cfg->ops->suspend)
>>>>>>     		pcie->cfg->ops->suspend(pcie);
>>>>>> -- 
>>>>>> 2.7.4
>>>>>>

Re: [PATCH v6 2/5] PCI: qcom: Add retry logic for link to be stable in L1ss

[Manivannan Sadhasivam]

On Wed, Sep 14, 2022 at 07:15:35AM +0530, Krishna Chaitanya Chundru wrote:
> 
> On 9/13/2022 10:09 PM, Manivannan Sadhasivam wrote:
> > On Tue, Sep 13, 2022 at 07:54:22PM +0530, Krishna Chaitanya Chundru wrote:
> > > On 9/12/2022 11:03 PM, Manivannan Sadhasivam wrote:
> > > > On Mon, Sep 12, 2022 at 09:39:36PM +0530, Krishna Chaitanya Chundru wrote:
> > > > > On 9/10/2022 1:20 AM, Bjorn Helgaas wrote:
> > > > > > On Fri, Sep 09, 2022 at 02:14:41PM +0530, Krishna chaitanya chundru wrote:
> > > > > > > Some specific devices are taking time to settle the link in L1ss.
> > > > > > > So added a retry logic before returning from the suspend op.
> > > > > > "L1ss" is not a state.  If you mean "L1.1" or "L1.2", say that.  Also
> > > > > > in code comments below.
> > > > > Yes L1ss means L1.2 and L1.2 We will update it next patch
> > > > > > s/So added a/Add/
> > > > > > 
> > > > > > What are these specific devices?  Is this a qcom controller defect?
> > > > > > An endpoint defect that should be addressed via some kind of generic
> > > > > > quirk?
> > > > > This is depending up on the endpoint devices and it varies to device to
> > > > > device.
> > > > > 
> > > > Can we identify the source of the traffic? Is the NVMe driver not
> > > > flushing it's queues correctly?
> > > We found that it is not from nvme data, we are seeing some physical layer
> > > activity on the
> > > 
> > > protocol analyzer.
> > > 
> > Okay
> > 
> > > > > We are thinking this is not a defect if there is some traffic in the link
> > > > > the link will
> > > > > 
> > > > > not go to L1ss .
> > > > > 
> > > > Is this hack still required even after switching to syscore ops?
> > > > 
> > > > Thanks,
> > > > Mani
> > > Yes, mani it is still required. And just before this sycore ops there will
> > > be a pci transaction to
> > > 
> > > mask msix interrupts.
> > > 
> > Hmm. I'm getting slightly confused here. What really happens when you do
> > the resource teardown during suspend and the link has not entered L1SS?
> > 
> > Since PHY is powered by MX domain, I'm wondering why we should wait for
> > the link to be in L1SS?
> > 
> > Thanks,
> > Mani
> 
> Mani, we need to turn off the link only after link entered in to L1ss. If we
> do before that
> 
> some transactions will be disturbed and we see a link down.
> 
> Mx power rail will control digital logic of the PHY and tries to retain the
> link state only,
> 
> The analog logic is controlled by the CX rail only, so when the link is in
> L1ss only we turn off
> 
> clks and phy.
> 

Okay, thanks for the clarification. Please add this info as a comment just above
the change.

Thanks,
Mani

> > > > > > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > > > > > > ---
> > > > > > >     drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++-----------
> > > > > > >     1 file changed, 25 insertions(+), 11 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > > > > index 6e04d0d..15c2067 100644
> > > > > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > > > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > > > > @@ -1809,26 +1809,40 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > > > > > >     static int __maybe_unused qcom_pcie_pm_suspend(struct qcom_pcie *pcie)
> > > > > > >     {
> > > > > > >     	u32 val;
> > > > > > > +	ktime_t timeout, start;
> > > > > > >     	struct dw_pcie *pci = pcie->pci;
> > > > > > >     	struct device *dev = pci->dev;
> > > > > > >     	if (!pcie->cfg->supports_system_suspend)
> > > > > > >     		return 0;
> > > > > > > -	/* if the link is not active turn off clocks */
> > > > > > > -	if (!dw_pcie_link_up(pci)) {
> > > > > > > -		dev_info(dev, "Link is not active\n");
> > > > > > > -		goto suspend;
> > > > > > > -	}
> > > > > > > +	start = ktime_get();
> > > > > > > +	/* Wait max 200 ms */
> > > > > > > +	timeout = ktime_add_ms(start, 200);
> > > > > > > -	/* if the link is not in l1ss don't turn off clocks */
> > > > > > > -	val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
> > > > > > > -	if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> > > > > > > -		dev_warn(dev, "Link is not in L1ss\n");
> > > > > > > -		return 0;
> > > > > > > +	while (1) {
> > > > > > > +
> > > > > > > +		if (!dw_pcie_link_up(pci)) {
> > > > > > > +			dev_warn(dev, "Link is not active\n");
> > > > > > > +			break;
> > > > > > > +		}
> > > > > > > +
> > > > > > > +		/* if the link is not in l1ss don't turn off clocks */
> > > > > > > +		val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
> > > > > > > +		if ((val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> > > > > > > +			dev_dbg(dev, "Link enters L1ss after %d  ms\n",
> > > > > > > +					ktime_to_ms(ktime_get() - start));
> > > > > > > +			break;
> > > > > > > +		}
> > > > > > > +
> > > > > > > +		if (ktime_after(ktime_get(), timeout)) {
> > > > > > > +			dev_warn(dev, "Link is not in L1ss\n");
> > > > > > > +			return 0;
> > > > > > > +		}
> > > > > > > +
> > > > > > > +		udelay(1000);
> > > > > > >     	}
> > > > > > > -suspend:
> > > > > > >     	if (pcie->cfg->ops->suspend)
> > > > > > >     		pcie->cfg->ops->suspend(pcie);
> > > > > > > -- 
> > > > > > > 2.7.4
> > > > > > > 

Re: [PATCH v6 2/5] PCI: qcom: Add retry logic for link to be stable in L1ss

[kernel test robot]

Hi Krishna,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on helgaas-pci/next]
[also build test WARNING on next-20220919]
[cannot apply to clk/clk-next linus/master v6.0-rc6]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Krishna-chaitanya-chundru/PCI-qcom-Add-system-suspend-resume-support/20220909-164906
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: arm64-randconfig-r002-20220919 (https://download.01.org/0day-ci/archive/20220920/202209200020.ASFgBZac-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 791a7ae1ba3efd6bca96338e10ffde557ba83920)
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 arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/629a2c707a31ccfdf891d6b580cf3e8c62ab9169
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Krishna-chaitanya-chundru/PCI-qcom-Add-system-suspend-resume-support/20220909-164906
        git checkout 629a2c707a31ccfdf891d6b580cf3e8c62ab9169
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/pci/controller/dwc/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/pci/controller/dwc/pcie-qcom.c:1834:6: warning: format specifies type 'int' but the argument has type 's64' (aka 'long long') [-Wformat]
                                           ktime_to_ms(ktime_get() - start));
                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:158:46: note: expanded from macro 'dev_dbg'
           dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__)
                                               ~~~     ^~~~~~~~~~~
   include/linux/dev_printk.h:129:34: note: expanded from macro 'dev_printk'
                   _dev_printk(level, dev, fmt, ##__VA_ARGS__);            \
                                           ~~~    ^~~~~~~~~~~
   1 warning generated.


vim +1834 drivers/pci/controller/dwc/pcie-qcom.c

  1808	
  1809	static int __maybe_unused qcom_pcie_pm_suspend(struct qcom_pcie *pcie)
  1810	{
  1811		u32 val;
  1812		ktime_t timeout, start;
  1813		struct dw_pcie *pci = pcie->pci;
  1814		struct device *dev = pci->dev;
  1815	
  1816		if (!pcie->cfg->supports_system_suspend)
  1817			return 0;
  1818	
  1819		start = ktime_get();
  1820		/* Wait max 200 ms */
  1821		timeout = ktime_add_ms(start, 200);
  1822	
  1823		while (1) {
  1824	
  1825			if (!dw_pcie_link_up(pci)) {
  1826				dev_warn(dev, "Link is not active\n");
  1827				break;
  1828			}
  1829	
  1830			/* if the link is not in l1ss don't turn off clocks */
  1831			val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
  1832			if ((val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
  1833				dev_dbg(dev, "Link enters L1ss after %d  ms\n",
> 1834						ktime_to_ms(ktime_get() - start));
  1835				break;
  1836			}
  1837	
  1838			if (ktime_after(ktime_get(), timeout)) {
  1839				dev_warn(dev, "Link is not in L1ss\n");
  1840				return 0;
  1841			}
  1842	
  1843			udelay(1000);
  1844		}
  1845	
  1846		if (pcie->cfg->ops->suspend)
  1847			pcie->cfg->ops->suspend(pcie);
  1848	
  1849		pcie->is_suspended = true;
  1850	
  1851		return 0;
  1852	}
  1853	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[PATCH v6 3/5] phy: core: Add support for phy power down & power up

[Krishna chaitanya chundru]

Introducing phy power down/up callbacks for allowing to park the
link-state in L1ss without holding any PCIe resources during
system suspend.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 drivers/phy/phy-core.c  | 30 ++++++++++++++++++++++++++++++
 include/linux/phy/phy.h | 20 ++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index d93ddf1..1b0b757 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -441,6 +441,36 @@ int phy_set_speed(struct phy *phy, int speed)
 }
 EXPORT_SYMBOL_GPL(phy_set_speed);
 
+int phy_power_down(struct phy *phy)
+{
+	int ret;
+
+	if (!phy || !phy->ops->power_down)
+		return 0;
+
+	mutex_lock(&phy->mutex);
+	ret = phy->ops->power_down(phy);
+	mutex_unlock(&phy->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(phy_power_down);
+
+int phy_power_up(struct phy *phy)
+{
+	int ret;
+
+	if (!phy || !phy->ops->power_up)
+		return 0;
+
+	mutex_lock(&phy->mutex);
+	ret = phy->ops->power_up(phy);
+	mutex_unlock(&phy->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(phy_power_up);
+
 int phy_reset(struct phy *phy)
 {
 	int ret;
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index b141375..3a45f4d 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -76,6 +76,8 @@ union phy_configure_opts {
  * @set_mode: set the mode of the phy
  * @set_media: set the media type of the phy (optional)
  * @set_speed: set the speed of the phy (optional)
+ * @power_down: parking the phy in power down state
+ * @power_up: pulling back the phy from power down
  * @reset: resetting the phy
  * @calibrate: calibrate the phy
  * @release: ops to be performed while the consumer relinquishes the PHY
@@ -89,6 +91,8 @@ struct phy_ops {
 	int	(*set_mode)(struct phy *phy, enum phy_mode mode, int submode);
 	int	(*set_media)(struct phy *phy, enum phy_media media);
 	int	(*set_speed)(struct phy *phy, int speed);
+	int	(*power_down)(struct phy *phy);
+	int	(*power_up)(struct phy *phy);
 
 	/**
 	 * @configure:
@@ -226,6 +230,8 @@ int phy_init(struct phy *phy);
 int phy_exit(struct phy *phy);
 int phy_power_on(struct phy *phy);
 int phy_power_off(struct phy *phy);
+int phy_power_down(struct phy *phy);
+int phy_power_up(struct phy *phy);
 int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int submode);
 #define phy_set_mode(phy, mode) \
 	phy_set_mode_ext(phy, mode, 0)
@@ -349,6 +355,20 @@ static inline int phy_power_off(struct phy *phy)
 	return -ENOSYS;
 }
 
+static inline int phy_power_down(struct phy *phy)
+{
+	if (!phy)
+		return 0;
+	return -ENOSYS;
+}
+
+static inline int phy_power_up(struct phy *phy)
+{
+	if (!phy)
+		return 0;
+	return -ENOSYS;
+}
+
 static inline int phy_set_mode_ext(struct phy *phy, enum phy_mode mode,
 				   int submode)
 {
-- 
2.7.4

Re: [PATCH v6 3/5] phy: core: Add support for phy power down & power up

[Dmitry Baryshkov]

On Fri, 9 Sept 2022 at 11:45, Krishna chaitanya chundru
<quic_krichai@quicinc.com> wrote:
>
> Introducing phy power down/up callbacks for allowing to park the
> link-state in L1ss without holding any PCIe resources during
> system suspend.
>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  drivers/phy/phy-core.c  | 30 ++++++++++++++++++++++++++++++
>  include/linux/phy/phy.h | 20 ++++++++++++++++++++
>  2 files changed, 50 insertions(+)
>
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index d93ddf1..1b0b757 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -441,6 +441,36 @@ int phy_set_speed(struct phy *phy, int speed)
>  }
>  EXPORT_SYMBOL_GPL(phy_set_speed);
>
> +int phy_power_down(struct phy *phy)
> +{
> +       int ret;
> +
> +       if (!phy || !phy->ops->power_down)
> +               return 0;
> +
> +       mutex_lock(&phy->mutex);
> +       ret = phy->ops->power_down(phy);
> +       mutex_unlock(&phy->mutex);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_power_down);
> +
> +int phy_power_up(struct phy *phy)
> +{
> +       int ret;
> +
> +       if (!phy || !phy->ops->power_up)
> +               return 0;
> +
> +       mutex_lock(&phy->mutex);
> +       ret = phy->ops->power_up(phy);
> +       mutex_unlock(&phy->mutex);
> +
> +       return ret;
> +}

As it can be seen from the phy_power_off(), the PHY can be a shared
resource, with the power_count counting the number of users that
requested the PHY to be powered up. By introducing suc calls you break
directly into this by allowing a single user to power down the PHY, no
matter how many other users have requested the PHY to stay alive.

> +EXPORT_SYMBOL_GPL(phy_power_up);
> +
>  int phy_reset(struct phy *phy)
>  {
>         int ret;
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index b141375..3a45f4d 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -76,6 +76,8 @@ union phy_configure_opts {
>   * @set_mode: set the mode of the phy
>   * @set_media: set the media type of the phy (optional)
>   * @set_speed: set the speed of the phy (optional)
> + * @power_down: parking the phy in power down state
> + * @power_up: pulling back the phy from power down
>   * @reset: resetting the phy
>   * @calibrate: calibrate the phy
>   * @release: ops to be performed while the consumer relinquishes the PHY
> @@ -89,6 +91,8 @@ struct phy_ops {
>         int     (*set_mode)(struct phy *phy, enum phy_mode mode, int submode);
>         int     (*set_media)(struct phy *phy, enum phy_media media);
>         int     (*set_speed)(struct phy *phy, int speed);
> +       int     (*power_down)(struct phy *phy);
> +       int     (*power_up)(struct phy *phy);
>
>         /**
>          * @configure:
> @@ -226,6 +230,8 @@ int phy_init(struct phy *phy);
>  int phy_exit(struct phy *phy);
>  int phy_power_on(struct phy *phy);
>  int phy_power_off(struct phy *phy);
> +int phy_power_down(struct phy *phy);
> +int phy_power_up(struct phy *phy);
>  int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int submode);
>  #define phy_set_mode(phy, mode) \
>         phy_set_mode_ext(phy, mode, 0)
> @@ -349,6 +355,20 @@ static inline int phy_power_off(struct phy *phy)
>         return -ENOSYS;
>  }
>
> +static inline int phy_power_down(struct phy *phy)
> +{
> +       if (!phy)
> +               return 0;
> +       return -ENOSYS;
> +}
> +
> +static inline int phy_power_up(struct phy *phy)
> +{
> +       if (!phy)
> +               return 0;
> +       return -ENOSYS;
> +}
> +
>  static inline int phy_set_mode_ext(struct phy *phy, enum phy_mode mode,
>                                    int submode)
>  {
> --
> 2.7.4
>


-- 
With best wishes
Dmitry

Re: [PATCH v6 3/5] phy: core: Add support for phy power down & power up

[Krishna Chaitanya Chundru]

On 9/9/2022 2:34 PM, Dmitry Baryshkov wrote:
> On Fri, 9 Sept 2022 at 11:45, Krishna chaitanya chundru
> <quic_krichai@quicinc.com> wrote:
>> Introducing phy power down/up callbacks for allowing to park the
>> link-state in L1ss without holding any PCIe resources during
>> system suspend.
>>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>>   drivers/phy/phy-core.c  | 30 ++++++++++++++++++++++++++++++
>>   include/linux/phy/phy.h | 20 ++++++++++++++++++++
>>   2 files changed, 50 insertions(+)
>>
>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>> index d93ddf1..1b0b757 100644
>> --- a/drivers/phy/phy-core.c
>> +++ b/drivers/phy/phy-core.c
>> @@ -441,6 +441,36 @@ int phy_set_speed(struct phy *phy, int speed)
>>   }
>>   EXPORT_SYMBOL_GPL(phy_set_speed);
>>
>> +int phy_power_down(struct phy *phy)
>> +{
>> +       int ret;
>> +
>> +       if (!phy || !phy->ops->power_down)
>> +               return 0;
>> +
>> +       mutex_lock(&phy->mutex);
>> +       ret = phy->ops->power_down(phy);
>> +       mutex_unlock(&phy->mutex);
>> +
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(phy_power_down);
>> +
>> +int phy_power_up(struct phy *phy)
>> +{
>> +       int ret;
>> +
>> +       if (!phy || !phy->ops->power_up)
>> +               return 0;
>> +
>> +       mutex_lock(&phy->mutex);
>> +       ret = phy->ops->power_up(phy);
>> +       mutex_unlock(&phy->mutex);
>> +
>> +       return ret;
>> +}
> As it can be seen from the phy_power_off(), the PHY can be a shared
> resource, with the power_count counting the number of users that
> requested the PHY to be powered up. By introducing suc calls you break
> directly into this by allowing a single user to power down the PHY, no
> matter how many other users have requested the PHY to stay alive.

can we use same power_count in this function also here and restrict the 
single user to

power down the PHY same like phy_power_off?.

>> +EXPORT_SYMBOL_GPL(phy_power_up);
>> +
>>   int phy_reset(struct phy *phy)
>>   {
>>          int ret;
>> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
>> index b141375..3a45f4d 100644
>> --- a/include/linux/phy/phy.h
>> +++ b/include/linux/phy/phy.h
>> @@ -76,6 +76,8 @@ union phy_configure_opts {
>>    * @set_mode: set the mode of the phy
>>    * @set_media: set the media type of the phy (optional)
>>    * @set_speed: set the speed of the phy (optional)
>> + * @power_down: parking the phy in power down state
>> + * @power_up: pulling back the phy from power down
>>    * @reset: resetting the phy
>>    * @calibrate: calibrate the phy
>>    * @release: ops to be performed while the consumer relinquishes the PHY
>> @@ -89,6 +91,8 @@ struct phy_ops {
>>          int     (*set_mode)(struct phy *phy, enum phy_mode mode, int submode);
>>          int     (*set_media)(struct phy *phy, enum phy_media media);
>>          int     (*set_speed)(struct phy *phy, int speed);
>> +       int     (*power_down)(struct phy *phy);
>> +       int     (*power_up)(struct phy *phy);
>>
>>          /**
>>           * @configure:
>> @@ -226,6 +230,8 @@ int phy_init(struct phy *phy);
>>   int phy_exit(struct phy *phy);
>>   int phy_power_on(struct phy *phy);
>>   int phy_power_off(struct phy *phy);
>> +int phy_power_down(struct phy *phy);
>> +int phy_power_up(struct phy *phy);
>>   int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int submode);
>>   #define phy_set_mode(phy, mode) \
>>          phy_set_mode_ext(phy, mode, 0)
>> @@ -349,6 +355,20 @@ static inline int phy_power_off(struct phy *phy)
>>          return -ENOSYS;
>>   }
>>
>> +static inline int phy_power_down(struct phy *phy)
>> +{
>> +       if (!phy)
>> +               return 0;
>> +       return -ENOSYS;
>> +}
>> +
>> +static inline int phy_power_up(struct phy *phy)
>> +{
>> +       if (!phy)
>> +               return 0;
>> +       return -ENOSYS;
>> +}
>> +
>>   static inline int phy_set_mode_ext(struct phy *phy, enum phy_mode mode,
>>                                     int submode)
>>   {
>> --
>> 2.7.4
>>
>

Re: [PATCH v6 3/5] phy: core: Add support for phy power down & power up

[Dmitry Baryshkov]

On 14/09/2022 17:50, Krishna Chaitanya Chundru wrote:
> 
> On 9/9/2022 2:34 PM, Dmitry Baryshkov wrote:
>> On Fri, 9 Sept 2022 at 11:45, Krishna chaitanya chundru
>> <quic_krichai@quicinc.com> wrote:
>>> Introducing phy power down/up callbacks for allowing to park the
>>> link-state in L1ss without holding any PCIe resources during
>>> system suspend.
>>>
>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>>> ---
>>>   drivers/phy/phy-core.c  | 30 ++++++++++++++++++++++++++++++
>>>   include/linux/phy/phy.h | 20 ++++++++++++++++++++
>>>   2 files changed, 50 insertions(+)
>>>
>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>> index d93ddf1..1b0b757 100644
>>> --- a/drivers/phy/phy-core.c
>>> +++ b/drivers/phy/phy-core.c
>>> @@ -441,6 +441,36 @@ int phy_set_speed(struct phy *phy, int speed)
>>>   }
>>>   EXPORT_SYMBOL_GPL(phy_set_speed);
>>>
>>> +int phy_power_down(struct phy *phy)
>>> +{
>>> +       int ret;
>>> +
>>> +       if (!phy || !phy->ops->power_down)
>>> +               return 0;
>>> +
>>> +       mutex_lock(&phy->mutex);
>>> +       ret = phy->ops->power_down(phy);
>>> +       mutex_unlock(&phy->mutex);
>>> +
>>> +       return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(phy_power_down);
>>> +
>>> +int phy_power_up(struct phy *phy)
>>> +{
>>> +       int ret;
>>> +
>>> +       if (!phy || !phy->ops->power_up)
>>> +               return 0;
>>> +
>>> +       mutex_lock(&phy->mutex);
>>> +       ret = phy->ops->power_up(phy);
>>> +       mutex_unlock(&phy->mutex);
>>> +
>>> +       return ret;
>>> +}
>> As it can be seen from the phy_power_off(), the PHY can be a shared
>> resource, with the power_count counting the number of users that
>> requested the PHY to be powered up. By introducing suc calls you break
>> directly into this by allowing a single user to power down the PHY, no
>> matter how many other users have requested the PHY to stay alive.
> 
> can we use same power_count in this function also here and restrict the 
> single user to
> 
> power down the PHY same like phy_power_off?.

What is the difference between power_off() and power_down()?


-- 
With best wishes
Dmitry

Re: [PATCH v6 3/5] phy: core: Add support for phy power down & power up

[Krishna Chaitanya Chundru]

On 9/19/2022 10:59 PM, Dmitry Baryshkov wrote:
> On 14/09/2022 17:50, Krishna Chaitanya Chundru wrote:
>>
>> On 9/9/2022 2:34 PM, Dmitry Baryshkov wrote:
>>> On Fri, 9 Sept 2022 at 11:45, Krishna chaitanya chundru
>>> <quic_krichai@quicinc.com> wrote:
>>>> Introducing phy power down/up callbacks for allowing to park the
>>>> link-state in L1ss without holding any PCIe resources during
>>>> system suspend.
>>>>
>>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>>>> ---
>>>>   drivers/phy/phy-core.c  | 30 ++++++++++++++++++++++++++++++
>>>>   include/linux/phy/phy.h | 20 ++++++++++++++++++++
>>>>   2 files changed, 50 insertions(+)
>>>>
>>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>>> index d93ddf1..1b0b757 100644
>>>> --- a/drivers/phy/phy-core.c
>>>> +++ b/drivers/phy/phy-core.c
>>>> @@ -441,6 +441,36 @@ int phy_set_speed(struct phy *phy, int speed)
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(phy_set_speed);
>>>>
>>>> +int phy_power_down(struct phy *phy)
>>>> +{
>>>> +       int ret;
>>>> +
>>>> +       if (!phy || !phy->ops->power_down)
>>>> +               return 0;
>>>> +
>>>> +       mutex_lock(&phy->mutex);
>>>> +       ret = phy->ops->power_down(phy);
>>>> +       mutex_unlock(&phy->mutex);
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(phy_power_down);
>>>> +
>>>> +int phy_power_up(struct phy *phy)
>>>> +{
>>>> +       int ret;
>>>> +
>>>> +       if (!phy || !phy->ops->power_up)
>>>> +               return 0;
>>>> +
>>>> +       mutex_lock(&phy->mutex);
>>>> +       ret = phy->ops->power_up(phy);
>>>> +       mutex_unlock(&phy->mutex);
>>>> +
>>>> +       return ret;
>>>> +}
>>> As it can be seen from the phy_power_off(), the PHY can be a shared
>>> resource, with the power_count counting the number of users that
>>> requested the PHY to be powered up. By introducing suc calls you break
>>> directly into this by allowing a single user to power down the PHY, no
>>> matter how many other users have requested the PHY to stay alive.
>>
>> can we use same power_count in this function also here and restrict 
>> the single user to
>>
>> power down the PHY same like phy_power_off?.
>
> What is the difference between power_off() and power_down()?

In power_off  we are turning off PCIe PHY-specific clocks, and also 
resetting the PHY due to this PCIe link
also will go down. To retain, the PCIe link state in l1ss with PHY 
clocks turned off, we need
park PCIe PHY in the power-down state and skip the resets of the PHY so 
that it can maintain the link state in l1ss
with the help of the always-on power domain aka MX).

To support this PHY Power-down state PHY driver has been updated with 
new interface APIs.

Initially we added phy_suspend & phy_resume to phy but as this API's are 
already used by drivers/net/phy/phy_device.c we are getting compilation 
errors.

So we used these power_down & power up.

As power_off & power_down is confusing we will change new api's 
power_down & power_up to phy_pm_suspend & phy_pm_resume in

the next patch series.
>
>

Re: [PATCH v6 3/5] phy: core: Add support for phy power down & power up

[Vinod Koul]

On 09-09-22, 14:14, Krishna chaitanya chundru wrote:
> Introducing phy power down/up callbacks for allowing to park the
> link-state in L1ss without holding any PCIe resources during
> system suspend.

where is the rest of the series, pls cc relevant folks on cover at
least!

> 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  drivers/phy/phy-core.c  | 30 ++++++++++++++++++++++++++++++
>  include/linux/phy/phy.h | 20 ++++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index d93ddf1..1b0b757 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -441,6 +441,36 @@ int phy_set_speed(struct phy *phy, int speed)
>  }
>  EXPORT_SYMBOL_GPL(phy_set_speed);
>  
> +int phy_power_down(struct phy *phy)
> +{
> +	int ret;
> +
> +	if (!phy || !phy->ops->power_down)
> +		return 0;
> +
> +	mutex_lock(&phy->mutex);
> +	ret = phy->ops->power_down(phy);
> +	mutex_unlock(&phy->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_power_down);

I dont think this is a good idea as Dmitry already updated...

> +
> +int phy_power_up(struct phy *phy)
> +{
> +	int ret;
> +
> +	if (!phy || !phy->ops->power_up)
> +		return 0;
> +
> +	mutex_lock(&phy->mutex);
> +	ret = phy->ops->power_up(phy);
> +	mutex_unlock(&phy->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_power_up);
> +
>  int phy_reset(struct phy *phy)
>  {
>  	int ret;
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index b141375..3a45f4d 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -76,6 +76,8 @@ union phy_configure_opts {
>   * @set_mode: set the mode of the phy
>   * @set_media: set the media type of the phy (optional)
>   * @set_speed: set the speed of the phy (optional)
> + * @power_down: parking the phy in power down state
> + * @power_up: pulling back the phy from power down
>   * @reset: resetting the phy
>   * @calibrate: calibrate the phy
>   * @release: ops to be performed while the consumer relinquishes the PHY
> @@ -89,6 +91,8 @@ struct phy_ops {
>  	int	(*set_mode)(struct phy *phy, enum phy_mode mode, int submode);
>  	int	(*set_media)(struct phy *phy, enum phy_media media);
>  	int	(*set_speed)(struct phy *phy, int speed);
> +	int	(*power_down)(struct phy *phy);
> +	int	(*power_up)(struct phy *phy);
>  
>  	/**
>  	 * @configure:
> @@ -226,6 +230,8 @@ int phy_init(struct phy *phy);
>  int phy_exit(struct phy *phy);
>  int phy_power_on(struct phy *phy);
>  int phy_power_off(struct phy *phy);
> +int phy_power_down(struct phy *phy);
> +int phy_power_up(struct phy *phy);
>  int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int submode);
>  #define phy_set_mode(phy, mode) \
>  	phy_set_mode_ext(phy, mode, 0)
> @@ -349,6 +355,20 @@ static inline int phy_power_off(struct phy *phy)
>  	return -ENOSYS;
>  }
>  
> +static inline int phy_power_down(struct phy *phy)
> +{
> +	if (!phy)
> +		return 0;
> +	return -ENOSYS;
> +}
> +
> +static inline int phy_power_up(struct phy *phy)
> +{
> +	if (!phy)
> +		return 0;
> +	return -ENOSYS;
> +}
> +
>  static inline int phy_set_mode_ext(struct phy *phy, enum phy_mode mode,
>  				   int submode)
>  {
> -- 
> 2.7.4

-- 
~Vinod

Re: [PATCH v6 3/5] phy: core: Add support for phy power down & power up

[Bjorn Helgaas]

On Tue, Sep 13, 2022 at 08:28:15PM +0530, Vinod Koul wrote:
> On 09-09-22, 14:14, Krishna chaitanya chundru wrote:
> > Introducing phy power down/up callbacks for allowing to park the
> > link-state in L1ss without holding any PCIe resources during
> > system suspend.
> 
> where is the rest of the series, pls cc relevant folks on cover at
> least!

Would be best to cc relevant folks, but in the meantime, it's easy to
find via lore, e.g., 3/5 has:

  Message-Id: <1662713084-8106-4-git-send-email-quic_krichai@quicinc.com>

so the lore URL is:

  https://lore.kernel.org/r/1662713084-8106-4-git-send-email-quic_krichai@quicinc.com

and the thread overview is at the bottom.

I use this incredibly handy mutt hook that adds lore URLs to emails
directly when viewing them: https://github.com/danrue/lorifier

Bjorn

[PATCH v6 4/5] phy: qcom: Add power down/up callbacks to pcie phy

[Krishna chaitanya chundru]

Add phy power down/up callbacks to pcie phy. Using these callbacks
we can release phy resources like phy specific clocks but continue
maintain pcie link in l1ss state.

This can help in parking pcie link in l1ss state during system
suspend (S3).

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c   |  6 ++--
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 50 ++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 15c2067..1d4b1b0 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1326,7 +1326,8 @@ static int qcom_pcie_resume_2_7_0(struct qcom_pcie *pcie)
 
 	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
 
-	phy_power_on(pcie->phy);
+	/* Bring back PHY from power down */
+	phy_power_up(pcie->phy);
 
 	return ret;
 }
@@ -1335,7 +1336,8 @@ static int qcom_pcie_suspend_2_7_0(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
 
-	phy_power_off(pcie->phy);
+	/* Power down PHY to park the link state in L1ss */
+	phy_power_down(pcie->phy);
 
 	clk_bulk_disable_unprepare(res->num_clks, res->clks);
 	return 0;
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
index 3ddbb8e..c6b3b82 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
@@ -2145,6 +2145,54 @@ static int qcom_qmp_phy_pcie_exit(struct phy *phy)
 	return 0;
 }
 
+static int qcom_qmp_phy_pcie_power_up(struct phy *phy)
+{
+	struct qmp_phy *qphy = phy_get_drvdata(phy);
+	struct qcom_qmp *qmp = qphy->qmp;
+	const struct qmp_phy_cfg *cfg = qphy->cfg;
+	int ret;
+
+	ret = clk_bulk_prepare_enable(cfg->num_clks, qmp->clks);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(qphy->pipe_clk);
+	if (ret)
+		return ret;
+
+	/* Pull out PHY from POWER DOWN state */
+	if (cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL]) {
+		qphy_setbits(qphy->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
+			     cfg->pwrdn_ctrl);
+	} else {
+		qphy_setbits(qphy->pcs, QPHY_V2_PCS_POWER_DOWN_CONTROL,
+				cfg->pwrdn_ctrl);
+	}
+
+	return 0;
+}
+
+static int qcom_qmp_phy_pcie_power_down(struct phy *phy)
+{
+	struct qmp_phy *qphy = phy_get_drvdata(phy);
+	struct qcom_qmp *qmp = qphy->qmp;
+	const struct qmp_phy_cfg *cfg = qphy->cfg;
+
+	clk_disable_unprepare(qphy->pipe_clk);
+	clk_bulk_disable_unprepare(cfg->num_clks, qmp->clks);
+
+	/* Put PHY into POWER DOWN state: active low */
+	if (cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL]) {
+		qphy_clrbits(qphy->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
+			     cfg->pwrdn_ctrl);
+	} else {
+		qphy_clrbits(qphy->pcs, QPHY_V2_PCS_POWER_DOWN_CONTROL,
+				cfg->pwrdn_ctrl);
+	}
+
+	return 0;
+}
+
 static int qcom_qmp_phy_pcie_enable(struct phy *phy)
 {
 	int ret;
@@ -2304,6 +2352,8 @@ static const struct phy_ops qcom_qmp_phy_pcie_ops = {
 	.power_on	= qcom_qmp_phy_pcie_enable,
 	.power_off	= qcom_qmp_phy_pcie_disable,
 	.set_mode	= qcom_qmp_phy_pcie_set_mode,
+	.power_down	= qcom_qmp_phy_pcie_power_down,
+	.power_up	= qcom_qmp_phy_pcie_power_up,
 	.owner		= THIS_MODULE,
 };
 
-- 
2.7.4

[PATCH v6 5/5] clk: qcom: Alwaya on pcie gdsc

[Krishna chaitanya chundru]

Make GDSC always on to ensure controller and its dependent clocks
won't go down during system suspend.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 drivers/clk/qcom/gcc-sc7280.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/gcc-sc7280.c b/drivers/clk/qcom/gcc-sc7280.c
index 7ff64d4..2f781a2 100644
--- a/drivers/clk/qcom/gcc-sc7280.c
+++ b/drivers/clk/qcom/gcc-sc7280.c
@@ -3109,7 +3109,7 @@ static struct gdsc gcc_pcie_1_gdsc = {
 		.name = "gcc_pcie_1_gdsc",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
-	.flags = VOTABLE,
+	.flags = ALWAYS_ON,
 };
 
 static struct gdsc gcc_ufs_phy_gdsc = {
-- 
2.7.4

Re: [PATCH v6 5/5] clk: qcom: Alwaya on pcie gdsc

[Manivannan Sadhasivam]

+ Rajendra

On Fri, Sep 09, 2022 at 02:14:44PM +0530, Krishna chaitanya chundru wrote:
> Make GDSC always on to ensure controller and its dependent clocks
> won't go down during system suspend.
> 

You need to mention the SoC name in subject, otherwise one cannot know for
which platform this patch applies to.

> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  drivers/clk/qcom/gcc-sc7280.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/gcc-sc7280.c b/drivers/clk/qcom/gcc-sc7280.c
> index 7ff64d4..2f781a2 100644
> --- a/drivers/clk/qcom/gcc-sc7280.c
> +++ b/drivers/clk/qcom/gcc-sc7280.c
> @@ -3109,7 +3109,7 @@ static struct gdsc gcc_pcie_1_gdsc = {
>  		.name = "gcc_pcie_1_gdsc",
>  	},
>  	.pwrsts = PWRSTS_OFF_ON,
> -	.flags = VOTABLE,
> +	.flags = ALWAYS_ON,

Rajendra, should we also put PCIe GDSC into retention state as you have done for
USB [1]?

Thanks,
Mani

[1] https://lore.kernel.org/all/20220901101756.28164-2-quic_rjendra@quicinc.com/

>  };
>  
>  static struct gdsc gcc_ufs_phy_gdsc = {
> -- 
> 2.7.4
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v6 5/5] clk: qcom: Alwaya on pcie gdsc

[Rajendra Nayak]

On 9/12/2022 10:34 PM, Manivannan Sadhasivam wrote:
> + Rajendra
> 
> On Fri, Sep 09, 2022 at 02:14:44PM +0530, Krishna chaitanya chundru wrote:
>> Make GDSC always on to ensure controller and its dependent clocks
>> won't go down during system suspend.
>>
> 
> You need to mention the SoC name in subject, otherwise one cannot know for
> which platform this patch applies to.
> 
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>>   drivers/clk/qcom/gcc-sc7280.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/qcom/gcc-sc7280.c b/drivers/clk/qcom/gcc-sc7280.c
>> index 7ff64d4..2f781a2 100644
>> --- a/drivers/clk/qcom/gcc-sc7280.c
>> +++ b/drivers/clk/qcom/gcc-sc7280.c
>> @@ -3109,7 +3109,7 @@ static struct gdsc gcc_pcie_1_gdsc = {
>>   		.name = "gcc_pcie_1_gdsc",
>>   	},
>>   	.pwrsts = PWRSTS_OFF_ON,
>> -	.flags = VOTABLE,
>> +	.flags = ALWAYS_ON,
> 
> Rajendra, should we also put PCIe GDSC into retention state as you have done for
> USB [1]?

Yes, it looks like we should handle this the same way as we did with usb.
Why are we removing the VOTABLE flag anyway?

> 
> Thanks,
> Mani
> 
> [1] https://lore.kernel.org/all/20220901101756.28164-2-quic_rjendra@quicinc.com/
> 
>>   };
>>   
>>   static struct gdsc gcc_ufs_phy_gdsc = {
>> -- 
>> 2.7.4
>>
> 

Re: [PATCH v6 5/5] clk: qcom: Alwaya on pcie gdsc

[Manivannan Sadhasivam]

On Tue, Sep 13, 2022 at 12:12:32PM +0530, Rajendra Nayak wrote:
> 
> On 9/12/2022 10:34 PM, Manivannan Sadhasivam wrote:
> > + Rajendra
> > 
> > On Fri, Sep 09, 2022 at 02:14:44PM +0530, Krishna chaitanya chundru wrote:
> > > Make GDSC always on to ensure controller and its dependent clocks
> > > won't go down during system suspend.
> > > 
> > 
> > You need to mention the SoC name in subject, otherwise one cannot know for
> > which platform this patch applies to.
> > 
> > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > > ---
> > >   drivers/clk/qcom/gcc-sc7280.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clk/qcom/gcc-sc7280.c b/drivers/clk/qcom/gcc-sc7280.c
> > > index 7ff64d4..2f781a2 100644
> > > --- a/drivers/clk/qcom/gcc-sc7280.c
> > > +++ b/drivers/clk/qcom/gcc-sc7280.c
> > > @@ -3109,7 +3109,7 @@ static struct gdsc gcc_pcie_1_gdsc = {
> > >   		.name = "gcc_pcie_1_gdsc",
> > >   	},
> > >   	.pwrsts = PWRSTS_OFF_ON,
> > > -	.flags = VOTABLE,
> > > +	.flags = ALWAYS_ON,
> > 
> > Rajendra, should we also put PCIe GDSC into retention state as you have done for
> > USB [1]?
> 
> Yes, it looks like we should handle this the same way as we did with usb.

Okay, thanks for the confirmation.

> Why are we removing the VOTABLE flag anyway?

Yeah, I don't see a reason for doing that.

Chaitanya, please follow the patch from Rajendra I mentioned above and adapt it
for PCIe GDSC.

Thanks,
Mani

> 
> > 
> > Thanks,
> > Mani
> > 
> > [1] https://lore.kernel.org/all/20220901101756.28164-2-quic_rjendra@quicinc.com/
> > 
> > >   };
> > >   static struct gdsc gcc_ufs_phy_gdsc = {
> > > -- 
> > > 2.7.4
> > > 
> > 

Re: [PATCH v6 5/5] clk: qcom: Alwaya on pcie gdsc

[Krishna Chaitanya Chundru]

On 9/13/2022 10:12 PM, Manivannan Sadhasivam wrote:
> On Tue, Sep 13, 2022 at 12:12:32PM +0530, Rajendra Nayak wrote:
>> On 9/12/2022 10:34 PM, Manivannan Sadhasivam wrote:
>>> + Rajendra
>>>
>>> On Fri, Sep 09, 2022 at 02:14:44PM +0530, Krishna chaitanya chundru wrote:
>>>> Make GDSC always on to ensure controller and its dependent clocks
>>>> won't go down during system suspend.
>>>>
>>> You need to mention the SoC name in subject, otherwise one cannot know for
>>> which platform this patch applies to.
>>>
>>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>>>> ---
>>>>    drivers/clk/qcom/gcc-sc7280.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/clk/qcom/gcc-sc7280.c b/drivers/clk/qcom/gcc-sc7280.c
>>>> index 7ff64d4..2f781a2 100644
>>>> --- a/drivers/clk/qcom/gcc-sc7280.c
>>>> +++ b/drivers/clk/qcom/gcc-sc7280.c
>>>> @@ -3109,7 +3109,7 @@ static struct gdsc gcc_pcie_1_gdsc = {
>>>>    		.name = "gcc_pcie_1_gdsc",
>>>>    	},
>>>>    	.pwrsts = PWRSTS_OFF_ON,
>>>> -	.flags = VOTABLE,
>>>> +	.flags = ALWAYS_ON,
>>> Rajendra, should we also put PCIe GDSC into retention state as you have done for
>>> USB [1]?
>> Yes, it looks like we should handle this the same way as we did with usb.
> Okay, thanks for the confirmation.
>
>> Why are we removing the VOTABLE flag anyway?
> Yeah, I don't see a reason for doing that.
>
> Chaitanya, please follow the patch from Rajendra I mentioned above and adapt it
> for PCIe GDSC.
>
> Thanks,
> Mani
ok I will try to adapt that.
>>> Thanks,
>>> Mani
>>>
>>> [1] https://lore.kernel.org/all/20220901101756.28164-2-quic_rjendra@quicinc.com/
>>>
>>>>    };
>>>>    static struct gdsc gcc_ufs_phy_gdsc = {
>>>> -- 
>>>> 2.7.4
>>>>

Re: [PATCH v6 5/5] clk: qcom: Alwaya on pcie gdsc

[Bjorn Helgaas]

On Mon, Sep 12, 2022 at 10:34:37PM +0530, Manivannan Sadhasivam wrote:
> + Rajendra
> 
> On Fri, Sep 09, 2022 at 02:14:44PM +0530, Krishna chaitanya chundru wrote:
> > Make GDSC always on to ensure controller and its dependent clocks
> > won't go down during system suspend.
> 
> You need to mention the SoC name in subject, otherwise one cannot know for
> which platform this patch applies to.

Also:

s/Alwaya/Always/
s/pcie/PCIe/
s/gdsc/GDSC/ as you did in commit log

I might use "ALWAYS_ON" in the subject to make clear this refers to a
specific flag, not a change in the code logic, e.g.,

  clk: qcom: gcc-sc7280: Mark PCIe GDSC clock ALWAYS_ON

Re: [PATCH v6 5/5] clk: qcom: Alwaya on pcie gdsc

[Krishna Chaitanya Chundru]

On 9/13/2022 10:04 PM, Bjorn Helgaas wrote:
> On Mon, Sep 12, 2022 at 10:34:37PM +0530, Manivannan Sadhasivam wrote:
>> + Rajendra
>>
>> On Fri, Sep 09, 2022 at 02:14:44PM +0530, Krishna chaitanya chundru wrote:
>>> Make GDSC always on to ensure controller and its dependent clocks
>>> won't go down during system suspend.
>> You need to mention the SoC name in subject, otherwise one cannot know for
>> which platform this patch applies to.
> Also:
>
> s/Alwaya/Always/
> s/pcie/PCIe/
> s/gdsc/GDSC/ as you did in commit log
>
> I might use "ALWAYS_ON" in the subject to make clear this refers to a
> specific flag, not a change in the code logic, e.g.,
>
>    clk: qcom: gcc-sc7280: Mark PCIe GDSC clock ALWAYS_ON
ok I will update the subject in next patch.

Re: [PATCH v6 0/5] PCI: qcom: Add system suspend & resume support

[Bjorn Helgaas]

On Fri, Sep 09, 2022 at 02:14:39PM +0530, Krishna chaitanya chundru wrote:
> Add suspend and resume syscore ops.
> 
> When system suspends, and if the link is in L1ss, disable the clocks
> and power down the phy so that system enters into low power state by
> parking link in L1ss to save the maximum power. And when the system
> resumes, enable the clocks back and power on phy if they are disabled
> in the suspend path.
> 
> we are doing this only when link is in l1ss but not in L2/L3 as
> nowhere we are forcing link to L2/L3 by sending PME turn off.
> 
> is_suspended flag indicates if the clocks are disabled in the suspend
> path or not.
> 
> There is access to Ep PCIe space to mask MSI/MSIX after pm suspend ops
> (getting hit by affinity changes while making CPUs offline during suspend,
> this will happen after devices are suspended (all phases of suspend ops)).
> When registered with pm ops there is a crash due to un-clocked access,
> as in the pm suspend op clocks are disabled. So, registering with syscore
> ops which will called after making CPUs offline.
> 
> Make GDSC always on to ensure controller and its dependent clocks
> won't go down during system suspend.
> 
> Krishna chaitanya chundru (5):
>   PCI: qcom: Add system suspend and resume support
>   PCI: qcom: Add retry logic for link to be stable in L1ss
>   phy: core: Add support for phy power down & power up
>   phy: qcom: Add power down/up callbacks to pcie phy
>   clk: qcom: Alwaya on pcie gdsc

This seems fairly ugly because it doesn't fit nicely into the PM
framework.  Why is this a qcom-specific thing?  What about other
DWC-based controllers?

>  drivers/clk/qcom/gcc-sc7280.c            |   2 +-
>  drivers/pci/controller/dwc/pcie-qcom.c   | 156 ++++++++++++++++++++++++++++++-
>  drivers/phy/phy-core.c                   |  30 ++++++
>  drivers/phy/qualcomm/phy-qcom-qmp-pcie.c |  50 ++++++++++
>  include/linux/phy/phy.h                  |  20 ++++
>  5 files changed, 256 insertions(+), 2 deletions(-)
> 
> -- 
> 2.7.4
> 

Re: [PATCH v6 0/5] PCI: qcom: Add system suspend & resume support

[Krishna Chaitanya Chundru]

On 9/10/2022 1:21 AM, Bjorn Helgaas wrote:
> On Fri, Sep 09, 2022 at 02:14:39PM +0530, Krishna chaitanya chundru wrote:
>> Add suspend and resume syscore ops.
>>
>> When system suspends, and if the link is in L1ss, disable the clocks
>> and power down the phy so that system enters into low power state by
>> parking link in L1ss to save the maximum power. And when the system
>> resumes, enable the clocks back and power on phy if they are disabled
>> in the suspend path.
>>
>> we are doing this only when link is in l1ss but not in L2/L3 as
>> nowhere we are forcing link to L2/L3 by sending PME turn off.
>>
>> is_suspended flag indicates if the clocks are disabled in the suspend
>> path or not.
>>
>> There is access to Ep PCIe space to mask MSI/MSIX after pm suspend ops
>> (getting hit by affinity changes while making CPUs offline during suspend,
>> this will happen after devices are suspended (all phases of suspend ops)).
>> When registered with pm ops there is a crash due to un-clocked access,
>> as in the pm suspend op clocks are disabled. So, registering with syscore
>> ops which will called after making CPUs offline.
>>
>> Make GDSC always on to ensure controller and its dependent clocks
>> won't go down during system suspend.
>>
>> Krishna chaitanya chundru (5):
>>    PCI: qcom: Add system suspend and resume support
>>    PCI: qcom: Add retry logic for link to be stable in L1ss
>>    phy: core: Add support for phy power down & power up
>>    phy: qcom: Add power down/up callbacks to pcie phy
>>    clk: qcom: Alwaya on pcie gdsc
> This seems fairly ugly because it doesn't fit nicely into the PM
> framework.  Why is this a qcom-specific thing?  What about other
> DWC-based controllers?
We wanted to allow system S3 state by turning off all PCIe clocks but at 
the same time
retaining NVMe device in D0 state and PCIe link in l1ss state.

Here nothing really specific to DWC as PCIe controller remains intact.

And the Qcom PHY allows this scheme  (that is to retain the link state 
in l1ss
even though all pcie clocks are turned off).

Since clocks are completely managed by qcom platform driver, we are 
trying to manage them
during S3/S0 transitions with PM callbacks.
>>   drivers/clk/qcom/gcc-sc7280.c            |   2 +-
>>   drivers/pci/controller/dwc/pcie-qcom.c   | 156 ++++++++++++++++++++++++++++++-
>>   drivers/phy/phy-core.c                   |  30 ++++++
>>   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c |  50 ++++++++++
>>   include/linux/phy/phy.h                  |  20 ++++
>>   5 files changed, 256 insertions(+), 2 deletions(-)
>>
>> -- 
>> 2.7.4
>>

Re: [PATCH v6 0/5] PCI: qcom: Add system suspend & resume support

[Bjorn Helgaas]

On Mon, Sep 12, 2022 at 09:40:30PM +0530, Krishna Chaitanya Chundru wrote:
> On 9/10/2022 1:21 AM, Bjorn Helgaas wrote:
> > On Fri, Sep 09, 2022 at 02:14:39PM +0530, Krishna chaitanya chundru wrote:
> > > Add suspend and resume syscore ops.
> > > 
> > > When system suspends, and if the link is in L1ss, disable the clocks
> > > and power down the phy so that system enters into low power state by
> > > parking link in L1ss to save the maximum power. And when the system
> > > resumes, enable the clocks back and power on phy if they are disabled
> > > in the suspend path.
> > > 
> > > we are doing this only when link is in l1ss but not in L2/L3 as
> > > nowhere we are forcing link to L2/L3 by sending PME turn off.
> > > 
> > > is_suspended flag indicates if the clocks are disabled in the suspend
> > > path or not.
> > > 
> > > There is access to Ep PCIe space to mask MSI/MSIX after pm suspend ops
> > > (getting hit by affinity changes while making CPUs offline during suspend,
> > > this will happen after devices are suspended (all phases of suspend ops)).
> > > When registered with pm ops there is a crash due to un-clocked access,
> > > as in the pm suspend op clocks are disabled. So, registering with syscore
> > > ops which will called after making CPUs offline.
> > > 
> > > Make GDSC always on to ensure controller and its dependent clocks
> > > won't go down during system suspend.
> > > 
> > > Krishna chaitanya chundru (5):
> > >    PCI: qcom: Add system suspend and resume support
> > >    PCI: qcom: Add retry logic for link to be stable in L1ss
> > >    phy: core: Add support for phy power down & power up
> > >    phy: qcom: Add power down/up callbacks to pcie phy
> > >    clk: qcom: Alwaya on pcie gdsc
> >
> > This seems fairly ugly because it doesn't fit nicely into the PM
> > framework.  Why is this a qcom-specific thing?  What about other
> > DWC-based controllers?
>
> We wanted to allow system S3 state by turning off all PCIe clocks
> but at the same time retaining NVMe device in D0 state and PCIe link
> in l1ss state.
>
> Here nothing really specific to DWC as PCIe controller remains intact.
> 
> And the Qcom PHY allows this scheme  (that is to retain the link
> state in l1ss even though all pcie clocks are turned off).

Is there somewhere in the PCIe spec I can read about how a link with
clocks turned off can remain in L1.1 or L1.2?

> Since clocks are completely managed by qcom platform driver, we are
> trying to manage them during S3/S0 transitions with PM callbacks.

I'm looking at this text in PCIe r6.0, sec 5.4.1:

  Components in the D0 state (i.e., fully active state) normally keep
  their Upstream Link in the active L0 state, as defined in § Section
  5.3.2 . ASPM defines a protocol for components in the D0 state to
  reduce Link power by placing their Links into a low power state and
  instructing the other end of the Link to do likewise. This
  capability allows hardware-autonomous, dynamic Link power reduction
  beyond what is achievable by software-only controlled (i.e., PCI-PM
  software driven) power management.

How does this qcom software management of clocks fit into this scheme?
It seems to me that if you need software to turn clocks off and on,
that is no longer ASPM.

Bjorn

Re: [PATCH v6 0/5] PCI: qcom: Add system suspend & resume support

[Manivannan Sadhasivam]

On Mon, Sep 12, 2022 at 12:08:06PM -0500, Bjorn Helgaas wrote:
> On Mon, Sep 12, 2022 at 09:40:30PM +0530, Krishna Chaitanya Chundru wrote:
> > On 9/10/2022 1:21 AM, Bjorn Helgaas wrote:
> > > On Fri, Sep 09, 2022 at 02:14:39PM +0530, Krishna chaitanya chundru wrote:
> > > > Add suspend and resume syscore ops.
> > > > 
> > > > When system suspends, and if the link is in L1ss, disable the clocks
> > > > and power down the phy so that system enters into low power state by
> > > > parking link in L1ss to save the maximum power. And when the system
> > > > resumes, enable the clocks back and power on phy if they are disabled
> > > > in the suspend path.
> > > > 
> > > > we are doing this only when link is in l1ss but not in L2/L3 as
> > > > nowhere we are forcing link to L2/L3 by sending PME turn off.
> > > > 
> > > > is_suspended flag indicates if the clocks are disabled in the suspend
> > > > path or not.
> > > > 
> > > > There is access to Ep PCIe space to mask MSI/MSIX after pm suspend ops
> > > > (getting hit by affinity changes while making CPUs offline during suspend,
> > > > this will happen after devices are suspended (all phases of suspend ops)).
> > > > When registered with pm ops there is a crash due to un-clocked access,
> > > > as in the pm suspend op clocks are disabled. So, registering with syscore
> > > > ops which will called after making CPUs offline.
> > > > 
> > > > Make GDSC always on to ensure controller and its dependent clocks
> > > > won't go down during system suspend.
> > > > 
> > > > Krishna chaitanya chundru (5):
> > > >    PCI: qcom: Add system suspend and resume support
> > > >    PCI: qcom: Add retry logic for link to be stable in L1ss
> > > >    phy: core: Add support for phy power down & power up
> > > >    phy: qcom: Add power down/up callbacks to pcie phy
> > > >    clk: qcom: Alwaya on pcie gdsc
> > >
> > > This seems fairly ugly because it doesn't fit nicely into the PM
> > > framework.  Why is this a qcom-specific thing?  What about other
> > > DWC-based controllers?
> >
> > We wanted to allow system S3 state by turning off all PCIe clocks
> > but at the same time retaining NVMe device in D0 state and PCIe link
> > in l1ss state.
> >
> > Here nothing really specific to DWC as PCIe controller remains intact.
> > 
> > And the Qcom PHY allows this scheme  (that is to retain the link
> > state in l1ss even though all pcie clocks are turned off).
> 
> Is there somewhere in the PCIe spec I can read about how a link with
> clocks turned off can remain in L1.1 or L1.2?
> 

This part is Qcom specific. On Qcom platforms there are two power domains used,
CX and MX. CX domain is sourcing the PCIe controller and MX is sourcing PCIe
PHY.

Both PCIe and PHY drivers don't control MX domain and only control CX.
So even though this patch is turning off all of the PCIe clocks, that
only helps in powering down the CX domain for achieving the low power
usecase while still keeping MX domain powered on.

So at the end of suspend, the PCIe controller would've turned off but
the link stays in L1SS state due to it being backed by MX.

> > Since clocks are completely managed by qcom platform driver, we are
> > trying to manage them during S3/S0 transitions with PM callbacks.
> 
> I'm looking at this text in PCIe r6.0, sec 5.4.1:
> 
>   Components in the D0 state (i.e., fully active state) normally keep
>   their Upstream Link in the active L0 state, as defined in § Section
>   5.3.2 . ASPM defines a protocol for components in the D0 state to
>   reduce Link power by placing their Links into a low power state and
>   instructing the other end of the Link to do likewise. This
>   capability allows hardware-autonomous, dynamic Link power reduction
>   beyond what is achievable by software-only controlled (i.e., PCI-PM
>   software driven) power management.
> 
> How does this qcom software management of clocks fit into this scheme?
> It seems to me that if you need software to turn clocks off and on,
> that is no longer ASPM.
> 

The PCIe link automatically transitions to L1SS if there is no activity
on the link but still the clocks sourced to the PCIe controller needs to be
turned off. PCIe spec only covers the link specifics and the controller is
platform dependent.

Thanks,
Mani

> Bjorn

Re: [PATCH v6 0/5] PCI: qcom: Add system suspend & resume support

[Manivannan Sadhasivam]

On Fri, Sep 09, 2022 at 02:14:39PM +0530, Krishna chaitanya chundru wrote:
> Add suspend and resume syscore ops.
> 
> When system suspends, and if the link is in L1ss, disable the clocks
> and power down the phy so that system enters into low power state by
> parking link in L1ss to save the maximum power. And when the system
> resumes, enable the clocks back and power on phy if they are disabled
> in the suspend path.
> 

You need to mention that you are only turning off the PCIe controller
clocks and PHY is still powered by a separate domain (MX) so the link
statys intact.

> we are doing this only when link is in l1ss but not in L2/L3 as
> nowhere we are forcing link to L2/L3 by sending PME turn off.
> 
> is_suspended flag indicates if the clocks are disabled in the suspend
> path or not.
> 
> There is access to Ep PCIe space to mask MSI/MSIX after pm suspend ops
> (getting hit by affinity changes while making CPUs offline during suspend,
> this will happen after devices are suspended (all phases of suspend ops)).
> When registered with pm ops there is a crash due to un-clocked access,
> as in the pm suspend op clocks are disabled. So, registering with syscore
> ops which will called after making CPUs offline.
> 
> Make GDSC always on to ensure controller and its dependent clocks
> won't go down during system suspend.
> 

Where is the changelog? You seem to have added PHY and CLK patches to
this series. You need to comment on that.

Thanks,
Mani

> Krishna chaitanya chundru (5):
>   PCI: qcom: Add system suspend and resume support
>   PCI: qcom: Add retry logic for link to be stable in L1ss
>   phy: core: Add support for phy power down & power up
>   phy: qcom: Add power down/up callbacks to pcie phy
>   clk: qcom: Alwaya on pcie gdsc
> 
>  drivers/clk/qcom/gcc-sc7280.c            |   2 +-
>  drivers/pci/controller/dwc/pcie-qcom.c   | 156 ++++++++++++++++++++++++++++++-
>  drivers/phy/phy-core.c                   |  30 ++++++
>  drivers/phy/qualcomm/phy-qcom-qmp-pcie.c |  50 ++++++++++
>  include/linux/phy/phy.h                  |  20 ++++
>  5 files changed, 256 insertions(+), 2 deletions(-)
> 
> -- 
> 2.7.4
> 

Re: [PATCH v6 0/5] PCI: qcom: Add system suspend & resume support

[Krishna Chaitanya Chundru]

On 9/12/2022 11:07 PM, Manivannan Sadhasivam wrote:
> On Fri, Sep 09, 2022 at 02:14:39PM +0530, Krishna chaitanya chundru wrote:
>> Add suspend and resume syscore ops.
>>
>> When system suspends, and if the link is in L1ss, disable the clocks
>> and power down the phy so that system enters into low power state by
>> parking link in L1ss to save the maximum power. And when the system
>> resumes, enable the clocks back and power on phy if they are disabled
>> in the suspend path.
>>
> You need to mention that you are only turning off the PCIe controller
> clocks and PHY is still powered by a separate domain (MX) so the link
> statys intact.
sure I will update the commit in next series.
>> we are doing this only when link is in l1ss but not in L2/L3 as
>> nowhere we are forcing link to L2/L3 by sending PME turn off.
>>
>> is_suspended flag indicates if the clocks are disabled in the suspend
>> path or not.
>>
>> There is access to Ep PCIe space to mask MSI/MSIX after pm suspend ops
>> (getting hit by affinity changes while making CPUs offline during suspend,
>> this will happen after devices are suspended (all phases of suspend ops)).
>> When registered with pm ops there is a crash due to un-clocked access,
>> as in the pm suspend op clocks are disabled. So, registering with syscore
>> ops which will called after making CPUs offline.
>>
>> Make GDSC always on to ensure controller and its dependent clocks
>> won't go down during system suspend.
>>
> Where is the changelog? You seem to have added PHY and CLK patches to
> this series. You need to comment on that.
>
> Thanks,
> Mani
I will update that in next patch.
>> Krishna chaitanya chundru (5):
>>    PCI: qcom: Add system suspend and resume support
>>    PCI: qcom: Add retry logic for link to be stable in L1ss
>>    phy: core: Add support for phy power down & power up
>>    phy: qcom: Add power down/up callbacks to pcie phy
>>    clk: qcom: Alwaya on pcie gdsc
>>
>>   drivers/clk/qcom/gcc-sc7280.c            |   2 +-
>>   drivers/pci/controller/dwc/pcie-qcom.c   | 156 ++++++++++++++++++++++++++++++-
>>   drivers/phy/phy-core.c                   |  30 ++++++
>>   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c |  50 ++++++++++
>>   include/linux/phy/phy.h                  |  20 ++++
>>   5 files changed, 256 insertions(+), 2 deletions(-)
>>
>> -- 
>> 2.7.4
>>