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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Make GDSC always on to ensure controller and its dependent clocks
won't go down during system suspend.
Signed-off-by: Krishna chaitanya chundru <[email protected]>
---
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
On Fri, 9 Sept 2022 at 11:45, Krishna chaitanya chundru
<[email protected]> 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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
>
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
>
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
>> ---
>> 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
>>
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
>>
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 <[email protected]>
>> ---
>> 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
>>
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/[email protected]/#R
> So we are trying this approach.
> >
> > Some more comments inline, for if the current approach moves forward.
> >
> > > Signed-off-by: Krishna chaitanya chundru <[email protected]>
> > > ---
> > > 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
> > >
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
+ 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 <[email protected]>
> ---
> 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/[email protected]/
> };
>
> static struct gdsc gcc_ufs_phy_gdsc = {
> --
> 2.7.4
>
--
மணிவண்ணன் சதாசிவம்
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 <[email protected]>
> > ---
> > 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
> >
--
மணிவண்ணன் சதாசிவம்
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
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
>
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 <[email protected]>
> > > ---
> > > 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
> > >
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 <[email protected]>
>> ---
>> 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/[email protected]/
>
>> };
>>
>> static struct gdsc gcc_ufs_phy_gdsc = {
>> --
>> 2.7.4
>>
>
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 <[email protected]>
>>>> ---
>>>> 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
>>>>
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > > ---
> > > 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/[email protected]/
> >
> > > };
> > > static struct gdsc gcc_ufs_phy_gdsc = {
> > > --
> > > 2.7.4
> > >
> >
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: <[email protected]>
so the lore URL is:
https://lore.kernel.org/r/[email protected]
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
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 <[email protected]>
> > > > > ---
> > > > > 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
> > > > >
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
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.
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 <[email protected]>
>>>> ---
>>>> 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/[email protected]/
>>>
>>>> };
>>>> static struct gdsc gcc_ufs_phy_gdsc = {
>>>> --
>>>> 2.7.4
>>>>
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 <[email protected]>
>>>>>> ---
>>>>>> 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
>>>>>>
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
>>
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 <[email protected]>
> > > > > > > ---
> > > > > > > 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
> > > > > > >
On 9/9/2022 2:34 PM, Dmitry Baryshkov wrote:
> On Fri, 9 Sept 2022 at 11:45, Krishna chaitanya chundru
> <[email protected]> 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 <[email protected]>
>> ---
>> 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
>>
>
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/[email protected]/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 <[email protected]>
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
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
>> <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
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
>>> <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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.
>
>