[PATCH v1 0/2] PCI: Restrict pci transactions after pci suspend

[PATCH v1 0/2] PCI: Restrict pci transactions after pci suspend

[Krishna chaitanya chundru]

If the endpoint device state is D0 and irq's are not freed, then
kernel try to mask interrupts by writing in to the vector
table (for MSIX interrupts) and config space (for MSI's).

These transactions are initiated after clocks are getting disabled
as part of PM suspend call. Due to it, these transactions are
resulting in un-clocked access and eventual to crashes.

So added a logic in qcom driver to restrict the unclocked access.
And updated the logic to check the link state before masking
or unmasking the interrupts.

Krishna chaitanya chundru (2):
  PCI: qcom: Add system PM support
  PCI: qcom: Restrict pci transactions after pci suspend

 drivers/pci/controller/dwc/pcie-designware-host.c |  12 ++-
 drivers/pci/controller/dwc/pcie-qcom.c            | 115 +++++++++++++++++++++-
 2 files changed, 123 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH v1 1/2] PCI: qcom: Add system PM support

[Krishna chaitanya chundru]

Add suspend and resume pm callbacks.

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

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

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 6ab9089..b3029ca 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -41,6 +41,9 @@
 #define L23_CLK_RMV_DIS				BIT(2)
 #define L1_CLK_RMV_DIS				BIT(1)
 
+#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)
@@ -190,6 +193,8 @@ 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 (*enable_clks)(struct qcom_pcie *pcie);
+	int (*disable_clks)(struct qcom_pcie *pcie);
 };
 
 struct qcom_pcie_cfg {
@@ -199,6 +204,8 @@ struct qcom_pcie_cfg {
 	unsigned int has_ddrss_sf_tbu_clk:1;
 	unsigned int has_aggre0_clk:1;
 	unsigned int has_aggre1_clk:1;
+	unsigned int support_pm_ops:1;
+	unsigned int is_suspended:1;
 };
 
 struct qcom_pcie {
@@ -1308,6 +1315,23 @@ static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
 	clk_disable_unprepare(res->pipe_clk);
 }
 
+static int qcom_pcie_enable_clks_2_7_0(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
+
+	return clk_bulk_prepare_enable(res->num_clks, res->clks);
+}
+
+static int qcom_pcie_disable_clks_2_7_0(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
+
+	clk_bulk_disable_unprepare(res->num_clks, res->clks);
+
+	return 0;
+}
+
+
 static int qcom_pcie_link_up(struct dw_pcie *pci)
 {
 	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
@@ -1485,6 +1509,8 @@ static const struct qcom_pcie_ops ops_2_7_0 = {
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
 	.post_init = qcom_pcie_post_init_2_7_0,
 	.post_deinit = qcom_pcie_post_deinit_2_7_0,
+	.enable_clks = qcom_pcie_enable_clks_2_7_0,
+	.disable_clks = qcom_pcie_disable_clks_2_7_0,
 };
 
 /* Qcom IP rev.: 1.9.0 */
@@ -1548,6 +1574,7 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
 	.ops = &ops_1_9_0,
 	.has_tbu_clk = true,
 	.pipe_clk_need_muxing = true,
+	.support_pm_ops = true,
 };
 
 static const struct dw_pcie_ops dw_pcie_ops = {
@@ -1591,6 +1618,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 
 	pcie->cfg = pcie_cfg;
 
+	pcie->cfg->is_suspended = false;
+
 	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
 	if (IS_ERR(pcie->reset)) {
 		ret = PTR_ERR(pcie->reset);
@@ -1645,6 +1674,56 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static int __maybe_unused qcom_pcie_pm_suspend(struct device *dev)
+{
+	struct qcom_pcie *pcie = dev_get_drvdata(dev);
+
+	if (!pcie->cfg->support_pm_ops)
+		return 0;
+
+	/* 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_err(dev, "Link is not in L1ss\n");
+		return 0;
+	}
+
+	if (pcie->cfg->ops->disable_clks)
+		pcie->cfg->ops->disable_clks(pcie);
+
+	if (pcie->cfg->ops->post_deinit)
+		pcie->cfg->ops->post_deinit(pcie);
+
+	pcie->cfg->is_suspended = true;
+
+	return 0;
+}
+
+static int __maybe_unused qcom_pcie_pm_resume(struct device *dev)
+{
+	struct qcom_pcie *pcie = dev_get_drvdata(dev);
+
+	if (!pcie->cfg->support_pm_ops)
+		return 0;
+
+	if (!pcie->cfg->is_suspended)
+		return 0;
+
+	if (pcie->cfg->ops->enable_clks)
+		pcie->ops->enable_clks(pcie);
+
+	if (pcie->cfg->ops->post_init)
+		pcie->ops->post_init(pcie);
+
+	pcie->cfg->is_suspended = false;
+
+	return 0;
+}
+
+static const struct dev_pm_ops qcom_pcie_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend, qcom_pcie_pm_resume)
+};
+
 static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
 	{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
@@ -1679,6 +1758,7 @@ static struct platform_driver qcom_pcie_driver = {
 	.probe = qcom_pcie_probe,
 	.driver = {
 		.name = "qcom-pcie",
+		.pm = &qcom_pcie_pm_ops,
 		.suppress_bind_attrs = true,
 		.of_match_table = qcom_pcie_match,
 	},
-- 
2.7.4

[PATCH v1 2/2] PCI: qcom: Restrict pci transactions after pci suspend

[Krishna chaitanya chundru]

If the endpoint device state is D0 and irq's are not freed, then
kernel try to mask interrupts by writing in to the vector
table (for MSIX interrupts) and config space (for MSI's).

These transactions are initiated after clocks are getting disabled
as part of PM suspend call. Due to it, these transactions are
resulting in un-clocked access and eventual to crashes.

So added a logic in qcom driver to restrict the unclocked access.
And updated the logic to check the link state before masking
or unmasking the interrupts.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 12 ++++++--
 drivers/pci/controller/dwc/pcie-qcom.c            | 35 +++++++++++++++++++++--
 2 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 2fa86f3..52ed865 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -29,13 +29,21 @@ static void dw_msi_ack_irq(struct irq_data *d)
 
 static void dw_msi_mask_irq(struct irq_data *d)
 {
-	pci_msi_mask_irq(d);
+	struct pcie_port *pp = irq_data_get_irq_chip_data(d->parent_data);
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+	if (dw_pcie_link_up(pci))
+		pci_msi_mask_irq(d);
 	irq_chip_mask_parent(d);
 }
 
 static void dw_msi_unmask_irq(struct irq_data *d)
 {
-	pci_msi_unmask_irq(d);
+	struct pcie_port *pp = irq_data_get_irq_chip_data(d->parent_data);
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+	if (dw_pcie_link_up(pci))
+		pci_msi_unmask_irq(d);
 	irq_chip_unmask_parent(d);
 }
 
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index b3029ca..af05fa7 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1331,12 +1331,41 @@ static int qcom_pcie_disable_clks_2_7_0(struct qcom_pcie *pcie)
 	return 0;
 }
 
+static u32 qcom_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
+				u32 reg, size_t size)
+{
+	struct qcom_pcie *pcie = to_qcom_pcie(pci);
+	u32 val;
+
+	if (pcie->cfg->is_suspended)
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+
+	dw_pcie_read(base + reg, size, &val);
+	return val;
+}
+
+static void qcom_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
+					u32 reg, size_t size, u32 val)
+{
+	struct qcom_pcie *pcie = to_qcom_pcie(pci);
+
+	if (pcie->cfg->is_suspended)
+		return;
+
+	dw_pcie_write(base + reg, size, val);
+}
 
 static int qcom_pcie_link_up(struct dw_pcie *pci)
 {
-	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
-	u16 val = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
+	struct qcom_pcie *pcie = to_qcom_pcie(pci);
+	u16 offset;
+	u16 val;
+
+	if (pcie->cfg->is_suspended)
+		return false;
 
+	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	val = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
 	return !!(val & PCI_EXP_LNKSTA_DLLLA);
 }
 
@@ -1580,6 +1609,8 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
 static const struct dw_pcie_ops dw_pcie_ops = {
 	.link_up = qcom_pcie_link_up,
 	.start_link = qcom_pcie_start_link,
+	.read_dbi = qcom_pcie_read_dbi,
+	.write_dbi = qcom_pcie_write_dbi,
 };
 
 static int qcom_pcie_probe(struct platform_device *pdev)
-- 
2.7.4

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

[Matthias Kaehlcke]

On Fri, Jun 24, 2022 at 12:58:01PM +0530, Krishna chaitanya chundru wrote:
> Add suspend and resume pm callbacks.
> 
> When system suspends, and if the link is in L1ss, disable the clocks
> so that system enters into low power state to save the maximum power.
> And when the system resumes, enable the clocks back if they are
> disabled in the suspend path.
> 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>

On which tree is this series based on?

With v5.19-rc3 I get:

  CC      drivers/pci/controller/dwc/pcie-qcom.o
/mnt/host/source/src/third_party/kernel/v5.15/drivers/pci/controller/dwc/pcie-qcom.c:1658:26: error: cannot assign to non-static data member 'cfg' with const-qualified type 'const struct qcom_pcie_cfg *'
        pcie->cfg->is_suspended = false;
        ~~~~~~~~~~~~~~~~~~~~~~~ ^
/mnt/host/source/src/third_party/kernel/v5.15/drivers/pci/controller/dwc/pcie-qcom.c:218:30: note: non-static data member 'cfg' declared const here
        const struct qcom_pcie_cfg *cfg;
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
/mnt/host/source/src/third_party/kernel/v5.15/drivers/pci/controller/dwc/pcie-qcom.c:1721:2: error: use of undeclared identifier 'val'
        val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
        ^
/mnt/host/source/src/third_party/kernel/v5.15/drivers/pci/controller/dwc/pcie-qcom.c:1722:8: error: use of undeclared identifier 'val'
        if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
              ^
/mnt/host/source/src/third_party/kernel/v5.15/drivers/pci/controller/dwc/pcie-qcom.c:1733:26: error: cannot assign to non-static data member 'cfg' with const-qualified type 'const struct qcom_pcie_cfg *'
        pcie->cfg->is_suspended = true;
        ~~~~~~~~~~~~~~~~~~~~~~~ ^
/mnt/host/source/src/third_party/kernel/v5.15/drivers/pci/controller/dwc/pcie-qcom.c:218:30: note: non-static data member 'cfg' declared const here
        const struct qcom_pcie_cfg *cfg;
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
/mnt/host/source/src/third_party/kernel/v5.15/drivers/pci/controller/dwc/pcie-qcom.c:1749:9: error: no member named 'ops' in 'struct qcom_pcie'
                pcie->ops->enable_clks(pcie);
                ~~~~  ^
/mnt/host/source/src/third_party/kernel/v5.15/drivers/pci/controller/dwc/pcie-qcom.c:1752:9: error: no member named 'ops' in 'struct qcom_pcie'
                pcie->ops->post_init(pcie);
                ~~~~  ^
/mnt/host/source/src/third_party/kernel/v5.15/drivers/pci/controller/dwc/pcie-qcom.c:1754:26: error: cannot assign to non-static data member 'cfg' with const-qualified type 'const struct qcom_pcie_cfg *'
        pcie->cfg->is_suspended = false;
        ~~~~~~~~~~~~~~~~~~~~~~~ ^
/mnt/host/source/src/third_party/kernel/v5.15/drivers/pci/controller/dwc/pcie-qcom.c:218:30: note: non-static data member 'cfg' declared const here
        const struct qcom_pcie_cfg *cfg;
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
7 errors generated.

[PATCH v2 0/2] PCI: Restrict pci transactions after pci suspend

[Krishna chaitanya chundru]

If the endpoint device state is D0 and irq's are not freed, then
kernel try to mask interrupts by writing in to the vector
table (for MSIX interrupts) and config space (for MSI's).

These transactions are initiated after clocks are getting disabled
as part of PM suspend call. Due to it, these transactions are
resulting in un-clocked access and eventual to crashes.

So added a logic in qcom driver to restrict the unclocked access.
And updated the logic to check the link state before masking
or unmasking the interrupts.

Krishna chaitanya chundru (2):
  PCI: qcom: Add system PM support
  PCI: qcom: Restrict pci transactions after pci suspend

 drivers/pci/controller/dwc/pcie-designware-host.c |  14 ++-
 drivers/pci/controller/dwc/pcie-qcom.c            | 116 +++++++++++++++++++++-
 2 files changed, 126 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH v2 1/2] PCI: qcom: Add system PM support

[Krishna chaitanya chundru]

Add suspend and resume pm callbacks.

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

Changes since v1:
	- Fixed compilation errors.

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

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 6ab9089..8e9ef37 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -41,6 +41,9 @@
 #define L23_CLK_RMV_DIS				BIT(2)
 #define L1_CLK_RMV_DIS				BIT(1)
 
+#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)
@@ -190,6 +193,8 @@ 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 (*enable_clks)(struct qcom_pcie *pcie);
+	int (*disable_clks)(struct qcom_pcie *pcie);
 };
 
 struct qcom_pcie_cfg {
@@ -199,6 +204,7 @@ struct qcom_pcie_cfg {
 	unsigned int has_ddrss_sf_tbu_clk:1;
 	unsigned int has_aggre0_clk:1;
 	unsigned int has_aggre1_clk:1;
+	unsigned int support_pm_ops:1;
 };
 
 struct qcom_pcie {
@@ -209,6 +215,7 @@ 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)
@@ -1308,6 +1315,23 @@ static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
 	clk_disable_unprepare(res->pipe_clk);
 }
 
+static int qcom_pcie_enable_clks_2_7_0(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
+
+	return clk_bulk_prepare_enable(res->num_clks, res->clks);
+}
+
+static int qcom_pcie_disable_clks_2_7_0(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
+
+	clk_bulk_disable_unprepare(res->num_clks, res->clks);
+
+	return 0;
+}
+
+
 static int qcom_pcie_link_up(struct dw_pcie *pci)
 {
 	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
@@ -1485,6 +1509,8 @@ static const struct qcom_pcie_ops ops_2_7_0 = {
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
 	.post_init = qcom_pcie_post_init_2_7_0,
 	.post_deinit = qcom_pcie_post_deinit_2_7_0,
+	.enable_clks = qcom_pcie_enable_clks_2_7_0,
+	.disable_clks = qcom_pcie_disable_clks_2_7_0,
 };
 
 /* Qcom IP rev.: 1.9.0 */
@@ -1548,6 +1574,7 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
 	.ops = &ops_1_9_0,
 	.has_tbu_clk = true,
 	.pipe_clk_need_muxing = true,
+	.support_pm_ops = true,
 };
 
 static const struct dw_pcie_ops dw_pcie_ops = {
@@ -1591,6 +1618,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 
 	pcie->cfg = pcie_cfg;
 
+	pcie->is_suspended = false;
+
 	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
 	if (IS_ERR(pcie->reset)) {
 		ret = PTR_ERR(pcie->reset);
@@ -1645,6 +1674,57 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static int __maybe_unused qcom_pcie_pm_suspend(struct device *dev)
+{
+	struct qcom_pcie *pcie = dev_get_drvdata(dev);
+	u32 val;
+
+	if (!pcie->cfg->support_pm_ops)
+		return 0;
+
+	/* 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_err(dev, "Link is not in L1ss\n");
+		return 0;
+	}
+
+	if (pcie->cfg->ops->disable_clks)
+		pcie->cfg->ops->disable_clks(pcie);
+
+	if (pcie->cfg->ops->post_deinit)
+		pcie->cfg->ops->post_deinit(pcie);
+
+	pcie->is_suspended = true;
+
+	return 0;
+}
+
+static int __maybe_unused qcom_pcie_pm_resume(struct device *dev)
+{
+	struct qcom_pcie *pcie = dev_get_drvdata(dev);
+
+	if (!pcie->cfg->support_pm_ops)
+		return 0;
+
+	if (!pcie->is_suspended)
+		return 0;
+
+	if (pcie->cfg->ops->enable_clks)
+		pcie->cfg->ops->enable_clks(pcie);
+
+	if (pcie->cfg->ops->post_init)
+		pcie->cfg->ops->post_init(pcie);
+
+	pcie->is_suspended = false;
+
+	return 0;
+}
+
+static const struct dev_pm_ops qcom_pcie_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend, qcom_pcie_pm_resume)
+};
+
 static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
 	{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
@@ -1679,6 +1759,7 @@ static struct platform_driver qcom_pcie_driver = {
 	.probe = qcom_pcie_probe,
 	.driver = {
 		.name = "qcom-pcie",
+		.pm = &qcom_pcie_pm_ops,
 		.suppress_bind_attrs = true,
 		.of_match_table = qcom_pcie_match,
 	},
-- 
2.7.4

[PATCH v2 2/2] PCI: qcom: Restrict pci transactions after pci suspend

[Krishna chaitanya chundru]

If the endpoint device state is D0 and irq's are not freed, then
kernel try to mask interrupts by writing in to the vector
table (for MSIX interrupts) and config space (for MSI's).

These transactions are initiated after clocks are getting disabled
as part of PM suspend call. Due to it, these transactions are
resulting in un-clocked access and eventual to crashes.

So added a logic in qcom driver to restrict the unclocked access.
And updated the logic to check the link state before masking
or unmasking the interrupts.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 14 +++++++--
 drivers/pci/controller/dwc/pcie-qcom.c            | 35 +++++++++++++++++++++--
 2 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 2fa86f3..2a46b40 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -29,13 +29,23 @@ static void dw_msi_ack_irq(struct irq_data *d)
 
 static void dw_msi_mask_irq(struct irq_data *d)
 {
-	pci_msi_mask_irq(d);
+	struct pcie_port *pp = irq_data_get_irq_chip_data(d->parent_data);
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+	if (dw_pcie_link_up(pci))
+		pci_msi_mask_irq(d);
+
 	irq_chip_mask_parent(d);
 }
 
 static void dw_msi_unmask_irq(struct irq_data *d)
 {
-	pci_msi_unmask_irq(d);
+	struct pcie_port *pp = irq_data_get_irq_chip_data(d->parent_data);
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+	if (dw_pcie_link_up(pci))
+		pci_msi_unmask_irq(d);
+
 	irq_chip_unmask_parent(d);
 }
 
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 8e9ef37..227bc24a 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1331,12 +1331,41 @@ static int qcom_pcie_disable_clks_2_7_0(struct qcom_pcie *pcie)
 	return 0;
 }
 
+static u32 qcom_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
+				u32 reg, size_t size)
+{
+	struct qcom_pcie *pcie = to_qcom_pcie(pci);
+	u32 val;
+
+	if (pcie->is_suspended)
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+
+	dw_pcie_read(base + reg, size, &val);
+	return val;
+}
+
+static void qcom_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
+					u32 reg, size_t size, u32 val)
+{
+	struct qcom_pcie *pcie = to_qcom_pcie(pci);
+
+	if (pcie->is_suspended)
+		return;
+
+	dw_pcie_write(base + reg, size, val);
+}
 
 static int qcom_pcie_link_up(struct dw_pcie *pci)
 {
-	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
-	u16 val = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
+	struct qcom_pcie *pcie = to_qcom_pcie(pci);
+	u16 offset;
+	u16 val;
+
+	if (pcie->is_suspended)
+		return false;
 
+	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	val = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
 	return !!(val & PCI_EXP_LNKSTA_DLLLA);
 }
 
@@ -1580,6 +1609,8 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
 static const struct dw_pcie_ops dw_pcie_ops = {
 	.link_up = qcom_pcie_link_up,
 	.start_link = qcom_pcie_start_link,
+	.read_dbi = qcom_pcie_read_dbi,
+	.write_dbi = qcom_pcie_write_dbi,
 };
 
 static int qcom_pcie_probe(struct platform_device *pdev)
-- 
2.7.4

Re: [PATCH v2 1/2] PCI: qcom: Add system PM support

[Matthias Kaehlcke]

On Wed, Jun 29, 2022 at 03:03:33PM +0530, Krishna chaitanya chundru wrote:
> Add suspend and resume pm callbacks.
> 
> When system suspends, and if the link is in L1ss, disable the clocks
> so that system enters into low power state to save the maximum power.
> And when the system resumes, enable the clocks back if they are
> disabled in the suspend path.
> 
> Changes since v1:
> 	- Fixed compilation errors.
> 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 81 ++++++++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 6ab9089..8e9ef37 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -41,6 +41,9 @@
>  #define L23_CLK_RMV_DIS				BIT(2)
>  #define L1_CLK_RMV_DIS				BIT(1)
>  
> +#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)
> @@ -190,6 +193,8 @@ 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 (*enable_clks)(struct qcom_pcie *pcie);
> +	int (*disable_clks)(struct qcom_pcie *pcie);
>  };
>  
>  struct qcom_pcie_cfg {
> @@ -199,6 +204,7 @@ struct qcom_pcie_cfg {
>  	unsigned int has_ddrss_sf_tbu_clk:1;
>  	unsigned int has_aggre0_clk:1;
>  	unsigned int has_aggre1_clk:1;
> +	unsigned int support_pm_ops:1;

nit: s/support_pm_ops/supports_system_suspend/ ?

It's not really the ops that are supported, but system suspend. 'supports'
(with an 's' at the end) to specify a characteristic of the
driver/controller (similar to the 'has_<something>'s above), rather than
an imperative of what the driver should do.

In any case, it's just a nit :)

>  };
>  
>  struct qcom_pcie {
> @@ -209,6 +215,7 @@ 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)
> @@ -1308,6 +1315,23 @@ static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
>  	clk_disable_unprepare(res->pipe_clk);
>  }
>  
> +static int qcom_pcie_enable_clks_2_7_0(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> +
> +	return clk_bulk_prepare_enable(res->num_clks, res->clks);
> +}
> +
> +static int qcom_pcie_disable_clks_2_7_0(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> +
> +	clk_bulk_disable_unprepare(res->num_clks, res->clks);
> +
> +	return 0;
> +}
> +
> +

nit: delete one empty line

>  static int qcom_pcie_link_up(struct dw_pcie *pci)
>  {
>  	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> @@ -1485,6 +1509,8 @@ static const struct qcom_pcie_ops ops_2_7_0 = {
>  	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
>  	.post_init = qcom_pcie_post_init_2_7_0,
>  	.post_deinit = qcom_pcie_post_deinit_2_7_0,
> +	.enable_clks = qcom_pcie_enable_clks_2_7_0,
> +	.disable_clks = qcom_pcie_disable_clks_2_7_0,
>  };
>  
>  /* Qcom IP rev.: 1.9.0 */
> @@ -1548,6 +1574,7 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
>  	.ops = &ops_1_9_0,
>  	.has_tbu_clk = true,
>  	.pipe_clk_need_muxing = true,
> +	.support_pm_ops = true,
>  };
>  
>  static const struct dw_pcie_ops dw_pcie_ops = {
> @@ -1591,6 +1618,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  
>  	pcie->cfg = pcie_cfg;
>  
> +	pcie->is_suspended = false;

not strictly necessary because of kzalloc, but does no harm either.

> +
>  	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
>  	if (IS_ERR(pcie->reset)) {
>  		ret = PTR_ERR(pcie->reset);
> @@ -1645,6 +1674,57 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> +static int __maybe_unused qcom_pcie_pm_suspend(struct device *dev)
> +{
> +	struct qcom_pcie *pcie = dev_get_drvdata(dev);
> +	u32 val;
> +
> +	if (!pcie->cfg->support_pm_ops)
> +		return 0;
> +
> +	/* 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_err(dev, "Link is not in L1ss\n");

If this is an error, should the function return an error? Otherwise maybe it
should be a warning.

> +		return 0;
> +	}
> +
> +	if (pcie->cfg->ops->disable_clks)
> +		pcie->cfg->ops->disable_clks(pcie);
> +
> +	if (pcie->cfg->ops->post_deinit)
> +		pcie->cfg->ops->post_deinit(pcie);
> +
> +	pcie->is_suspended = true;
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused qcom_pcie_pm_resume(struct device *dev)
> +{
> +	struct qcom_pcie *pcie = dev_get_drvdata(dev);
> +
> +	if (!pcie->cfg->support_pm_ops)
> +		return 0;
> +
> +	if (!pcie->is_suspended)
> +		return 0;
> +
> +	if (pcie->cfg->ops->enable_clks)
> +		pcie->cfg->ops->enable_clks(pcie);
> +
> +	if (pcie->cfg->ops->post_init)
> +		pcie->cfg->ops->post_init(pcie);
> +
> +	pcie->is_suspended = false;
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops qcom_pcie_pm_ops = {
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend, qcom_pcie_pm_resume)
> +};
> +
>  static const struct of_device_id qcom_pcie_match[] = {
>  	{ .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
>  	{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
> @@ -1679,6 +1759,7 @@ static struct platform_driver qcom_pcie_driver = {
>  	.probe = qcom_pcie_probe,
>  	.driver = {
>  		.name = "qcom-pcie",
> +		.pm = &qcom_pcie_pm_ops,
>  		.suppress_bind_attrs = true,
>  		.of_match_table = qcom_pcie_match,
>  	},
> -- 
> 2.7.4
> 

Re: [PATCH v2 1/2] PCI: qcom: Add system PM support

[Manivannan Sadhasivam]

On Wed, Jun 29, 2022 at 03:03:33PM +0530, Krishna chaitanya chundru wrote:
> Add suspend and resume pm callbacks.
> 
> When system suspends, and if the link is in L1ss, disable the clocks
> so that system enters into low power state to save the maximum power.
> And when the system resumes, enable the clocks back if they are
> disabled in the suspend path.
> 

Why only during L1ss and not L2/L3?

> Changes since v1:
> 	- Fixed compilation errors.
> 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 81 ++++++++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 6ab9089..8e9ef37 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -41,6 +41,9 @@
>  #define L23_CLK_RMV_DIS				BIT(2)
>  #define L1_CLK_RMV_DIS				BIT(1)
>  
> +#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)
> @@ -190,6 +193,8 @@ 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 (*enable_clks)(struct qcom_pcie *pcie);
> +	int (*disable_clks)(struct qcom_pcie *pcie);

I think these could vary between platforms. Like some other platform may try to
disable regulators etc... So use names such as suspend and resume.

>  };
>  
>  struct qcom_pcie_cfg {
> @@ -199,6 +204,7 @@ struct qcom_pcie_cfg {
>  	unsigned int has_ddrss_sf_tbu_clk:1;
>  	unsigned int has_aggre0_clk:1;
>  	unsigned int has_aggre1_clk:1;
> +	unsigned int support_pm_ops:1;
>  };
>  
>  struct qcom_pcie {
> @@ -209,6 +215,7 @@ struct qcom_pcie {
>  	struct phy *phy;
>  	struct gpio_desc *reset;
>  	const struct qcom_pcie_cfg *cfg;
> +	unsigned int is_suspended:1;

Why do you need this flag? Is suspend going to happen multiple times in
an out-of-order manner?

>  };
>  
>  #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
> @@ -1308,6 +1315,23 @@ static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
>  	clk_disable_unprepare(res->pipe_clk);
>  }
>  

[...]

> +static const struct dev_pm_ops qcom_pcie_pm_ops = {
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend, qcom_pcie_pm_resume)

Use the new macro: NOIRQ_SYSTEM_SLEEP_PM_OPS

> +};
> +
>  static const struct of_device_id qcom_pcie_match[] = {
>  	{ .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
>  	{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
> @@ -1679,6 +1759,7 @@ static struct platform_driver qcom_pcie_driver = {
>  	.probe = qcom_pcie_probe,
>  	.driver = {
>  		.name = "qcom-pcie",
> +		.pm = &qcom_pcie_pm_ops,

There will be warnings when CONFIG_PM_SLEEP is not set. So use below,

		.pm = pm_sleep_ptr(&qcom_pcie_pm_ops),

Thanks,
Mani

>  		.suppress_bind_attrs = true,
>  		.of_match_table = qcom_pcie_match,
>  	},
> -- 
> 2.7.4
> 

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

Re: [PATCH v2 1/2] PCI: qcom: Add system PM support

[Krishna Chaitanya Chundru]

On 6/30/2022 10:04 AM, Manivannan Sadhasivam wrote:
> On Wed, Jun 29, 2022 at 03:03:33PM +0530, Krishna chaitanya chundru wrote:
>> Add suspend and resume pm callbacks.
>>
>> When system suspends, and if the link is in L1ss, disable the clocks
>> so that system enters into low power state to save the maximum power.
>> And when the system resumes, enable the clocks back if they are
>> disabled in the suspend path.
>>
> Why only during L1ss and not L2/L3?

with aspm the link will automatically go to L1ss. for L2/L3 entry we 
need to explicitly send

PME turn off which we are not doing now. So we are checking only for L1ss.

>
>> Changes since v1:
>> 	- Fixed compilation errors.
>>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-qcom.c | 81 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 81 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 6ab9089..8e9ef37 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -41,6 +41,9 @@
>>   #define L23_CLK_RMV_DIS				BIT(2)
>>   #define L1_CLK_RMV_DIS				BIT(1)
>>   
>> +#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)
>> @@ -190,6 +193,8 @@ 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 (*enable_clks)(struct qcom_pcie *pcie);
>> +	int (*disable_clks)(struct qcom_pcie *pcie);
> I think these could vary between platforms. Like some other platform may try to
> disable regulators etc... So use names such as suspend and resume.
Sure will change in the next patch.
>>   };
>>   
>>   struct qcom_pcie_cfg {
>> @@ -199,6 +204,7 @@ struct qcom_pcie_cfg {
>>   	unsigned int has_ddrss_sf_tbu_clk:1;
>>   	unsigned int has_aggre0_clk:1;
>>   	unsigned int has_aggre1_clk:1;
>> +	unsigned int support_pm_ops:1;
>>   };
>>   
>>   struct qcom_pcie {
>> @@ -209,6 +215,7 @@ struct qcom_pcie {
>>   	struct phy *phy;
>>   	struct gpio_desc *reset;
>>   	const struct qcom_pcie_cfg *cfg;
>> +	unsigned int is_suspended:1;
> Why do you need this flag? Is suspend going to happen multiple times in
> an out-of-order manner?

We are using this flag in the resume function to check whether we 
suspended and disabled

the clocks in the suspend path. And we want to use this flag to control 
the access to dbi etc

after suspend.

>>   };
>>   
>>   #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
>> @@ -1308,6 +1315,23 @@ static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
>>   	clk_disable_unprepare(res->pipe_clk);
>>   }
>>   
> [...]
>
>> +static const struct dev_pm_ops qcom_pcie_pm_ops = {
>> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend, qcom_pcie_pm_resume)
> Use the new macro: NOIRQ_SYSTEM_SLEEP_PM_OPS
Will update in the next patch.
>> +};
>> +
>>   static const struct of_device_id qcom_pcie_match[] = {
>>   	{ .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
>>   	{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
>> @@ -1679,6 +1759,7 @@ static struct platform_driver qcom_pcie_driver = {
>>   	.probe = qcom_pcie_probe,
>>   	.driver = {
>>   		.name = "qcom-pcie",
>> +		.pm = &qcom_pcie_pm_ops,
> There will be warnings when CONFIG_PM_SLEEP is not set. So use below,
will update in the next patch.
>
> 		.pm = pm_sleep_ptr(&qcom_pcie_pm_ops),
>
> Thanks,
> Mani
>
>>   		.suppress_bind_attrs = true,
>>   		.of_match_table = qcom_pcie_match,
>>   	},
>> -- 
>> 2.7.4
>>

[PATCH v3 0/2] PCI: Restrict pci transactions after pci suspend

[Krishna chaitanya chundru]

If the endpoint device state is D0 and irq's are not freed, then
kernel try to mask interrupts in system suspend path by writing
in to the vector table (for MSIX interrupts) and config space (for MSI's).

These transactions are initiated in the pm suspend after pcie clocks got
disabled as part of platform driver pm  suspend call. Due to it, these
transactions are resulting in un-clocked access and eventually to crashes.

So added a logic in qcom driver to restrict these unclocked access.
And updated the logic to check the link state before masking
or unmasking the interrupts.

Krishna chaitanya chundru (2):
  PCI: qcom: Add system PM support
  PCI: qcom: Restrict pci transactions after pci suspend

 drivers/pci/controller/dwc/pcie-designware-host.c |  14 ++-
 drivers/pci/controller/dwc/pcie-qcom.c            | 114 +++++++++++++++++++++-
 2 files changed, 124 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH v3 1/2] PCI: qcom: Add system PM support

[Krishna chaitanya chundru]

Add suspend and resume pm callbacks.

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

Changes since v2:
	- Replaced he enable, disable clks ops with suspend and resume
	- Renamed support_pm_opsi flag  with supports_system_suspend.
Changes since v1:
	- Fixed compilation errors.

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

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 6ab9089..d7ede0c 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -41,6 +41,9 @@
 #define L23_CLK_RMV_DIS				BIT(2)
 #define L1_CLK_RMV_DIS				BIT(1)
 
+#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)
@@ -190,6 +193,8 @@ 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 {
@@ -199,6 +204,7 @@ struct qcom_pcie_cfg {
 	unsigned int has_ddrss_sf_tbu_clk:1;
 	unsigned int has_aggre0_clk:1;
 	unsigned int has_aggre1_clk:1;
+	unsigned int supports_system_suspend:1;
 };
 
 struct qcom_pcie {
@@ -209,6 +215,7 @@ 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)
@@ -1308,6 +1315,26 @@ static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
 	clk_disable_unprepare(res->pipe_clk);
 }
 
+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;
+
+	clk_prepare_enable(res->pipe_clk);
+
+	return clk_bulk_prepare_enable(res->num_clks, res->clks);
+}
+
+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;
+
+	clk_bulk_disable_unprepare(res->num_clks, res->clks);
+
+	clk_disable_unprepare(res->pipe_clk);
+
+	return 0;
+}
+
 static int qcom_pcie_link_up(struct dw_pcie *pci)
 {
 	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
@@ -1496,6 +1523,8 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
 	.post_init = qcom_pcie_post_init_2_7_0,
 	.post_deinit = qcom_pcie_post_deinit_2_7_0,
 	.config_sid = qcom_pcie_config_sid_sm8250,
+	.suspend = qcom_pcie_suspend_2_7_0,
+	.resume = qcom_pcie_resume_2_7_0,
 };
 
 static const struct qcom_pcie_cfg apq8084_cfg = {
@@ -1548,6 +1577,7 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
 	.ops = &ops_1_9_0,
 	.has_tbu_clk = true,
 	.pipe_clk_need_muxing = true,
+	.supports_system_suspend = true,
 };
 
 static const struct dw_pcie_ops dw_pcie_ops = {
@@ -1591,6 +1621,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 
 	pcie->cfg = pcie_cfg;
 
+	pcie->is_suspended = false;
+
 	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
 	if (IS_ERR(pcie->reset)) {
 		ret = PTR_ERR(pcie->reset);
@@ -1645,6 +1677,51 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static int __maybe_unused qcom_pcie_pm_suspend(struct device *dev)
+{
+	struct qcom_pcie *pcie = dev_get_drvdata(dev);
+	u32 val;
+
+	if (!pcie->cfg->supports_system_suspend)
+		return 0;
+
+	/* 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;
+	}
+
+	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 device *dev)
+{
+	struct qcom_pcie *pcie = dev_get_drvdata(dev);
+
+	if (!pcie->cfg->supports_system_suspend)
+		return 0;
+
+	if (!pcie->is_suspended)
+		return 0;
+
+	if (pcie->cfg->ops->resume)
+		pcie->cfg->ops->resume(pcie);
+
+	pcie->is_suspended = false;
+
+	return 0;
+}
+
+static const struct dev_pm_ops qcom_pcie_pm_ops = {
+	NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend, qcom_pcie_pm_resume)
+};
+
 static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
 	{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
@@ -1679,6 +1756,7 @@ static struct platform_driver qcom_pcie_driver = {
 	.probe = qcom_pcie_probe,
 	.driver = {
 		.name = "qcom-pcie",
+		.pm = pm_sleep_ptr(&qcom_pcie_pm_ops),
 		.suppress_bind_attrs = true,
 		.of_match_table = qcom_pcie_match,
 	},
-- 
2.7.4

[PATCH v3 2/2] PCI: qcom: Restrict pci transactions after pci suspend

[Krishna chaitanya chundru]

If the endpoint device state is D0 and irq's are not freed, then
kernel try to mask interrupts in system suspend path by writing
in to the vector table (for MSIX interrupts) and config space (for MSI's).

These transactions are initiated in the pm suspend after pcie clocks got
disabled as part of platform driver pm  suspend call. Due to it, these
transactions are resulting in un-clocked access and eventually to crashes.

So added a logic in qcom driver to restrict these unclocked access.
And updated the logic to check the link state before masking
or unmasking the interrupts.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 14 +++++++--
 drivers/pci/controller/dwc/pcie-qcom.c            | 36 +++++++++++++++++++++--
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 2fa86f3..2a46b40 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -29,13 +29,23 @@ static void dw_msi_ack_irq(struct irq_data *d)
 
 static void dw_msi_mask_irq(struct irq_data *d)
 {
-	pci_msi_mask_irq(d);
+	struct pcie_port *pp = irq_data_get_irq_chip_data(d->parent_data);
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+	if (dw_pcie_link_up(pci))
+		pci_msi_mask_irq(d);
+
 	irq_chip_mask_parent(d);
 }
 
 static void dw_msi_unmask_irq(struct irq_data *d)
 {
-	pci_msi_unmask_irq(d);
+	struct pcie_port *pp = irq_data_get_irq_chip_data(d->parent_data);
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+	if (dw_pcie_link_up(pci))
+		pci_msi_unmask_irq(d);
+
 	irq_chip_unmask_parent(d);
 }
 
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index d7ede0c..f0e9079 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1335,11 +1335,41 @@ static int qcom_pcie_suspend_2_7_0(struct qcom_pcie *pcie)
 	return 0;
 }
 
+static u32 qcom_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
+				u32 reg, size_t size)
+{
+	struct qcom_pcie *pcie = to_qcom_pcie(pci);
+	u32 val;
+
+	if (pcie->is_suspended)
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+
+	dw_pcie_read(base + reg, size, &val);
+	return val;
+}
+
+static void qcom_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
+				u32 reg, size_t size, u32 val)
+{
+	struct qcom_pcie *pcie = to_qcom_pcie(pci);
+
+	if (pcie->is_suspended)
+		return;
+
+	dw_pcie_write(base + reg, size, val);
+}
+
 static int qcom_pcie_link_up(struct dw_pcie *pci)
 {
-	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
-	u16 val = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
+	struct qcom_pcie *pcie = to_qcom_pcie(pci);
+	u16 offset;
+	u16 val;
+
+	if (pcie->is_suspended)
+		return false;
 
+	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	val = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
 	return !!(val & PCI_EXP_LNKSTA_DLLLA);
 }
 
@@ -1583,6 +1613,8 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
 static const struct dw_pcie_ops dw_pcie_ops = {
 	.link_up = qcom_pcie_link_up,
 	.start_link = qcom_pcie_start_link,
+	.read_dbi = qcom_pcie_read_dbi,
+	.write_dbi = qcom_pcie_write_dbi,
 };
 
 static int qcom_pcie_probe(struct platform_device *pdev)
-- 
2.7.4

Re: [PATCH v2 1/2] PCI: qcom: Add system PM support

[Manivannan Sadhasivam]

On Thu, Jun 30, 2022 at 03:29:33PM +0530, Krishna Chaitanya Chundru wrote:
> 
> On 6/30/2022 10:04 AM, Manivannan Sadhasivam wrote:
> > On Wed, Jun 29, 2022 at 03:03:33PM +0530, Krishna chaitanya chundru wrote:
> > > Add suspend and resume pm callbacks.
> > > 
> > > When system suspends, and if the link is in L1ss, disable the clocks
> > > so that system enters into low power state to save the maximum power.
> > > And when the system resumes, enable the clocks back if they are
> > > disabled in the suspend path.
> > > 
> > Why only during L1ss and not L2/L3?
> 
> with aspm the link will automatically go to L1ss. for L2/L3 entry we need to
> explicitly send
> 
> PME turn off which we are not doing now. So we are checking only for L1ss.
> 

Okay, please mention this in the commit message.

> > 
> > > Changes since v1:
> > > 	- Fixed compilation errors.
> > > 
> > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > > ---
> > >   drivers/pci/controller/dwc/pcie-qcom.c | 81 ++++++++++++++++++++++++++++++++++
> > >   1 file changed, 81 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > index 6ab9089..8e9ef37 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > @@ -41,6 +41,9 @@
> > >   #define L23_CLK_RMV_DIS				BIT(2)
> > >   #define L1_CLK_RMV_DIS				BIT(1)
> > > +#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)
> > > @@ -190,6 +193,8 @@ 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 (*enable_clks)(struct qcom_pcie *pcie);
> > > +	int (*disable_clks)(struct qcom_pcie *pcie);
> > I think these could vary between platforms. Like some other platform may try to
> > disable regulators etc... So use names such as suspend and resume.
> Sure will change in the next patch.
> > >   };
> > >   struct qcom_pcie_cfg {
> > > @@ -199,6 +204,7 @@ struct qcom_pcie_cfg {
> > >   	unsigned int has_ddrss_sf_tbu_clk:1;
> > >   	unsigned int has_aggre0_clk:1;
> > >   	unsigned int has_aggre1_clk:1;
> > > +	unsigned int support_pm_ops:1;
> > >   };
> > >   struct qcom_pcie {
> > > @@ -209,6 +215,7 @@ struct qcom_pcie {
> > >   	struct phy *phy;
> > >   	struct gpio_desc *reset;
> > >   	const struct qcom_pcie_cfg *cfg;
> > > +	unsigned int is_suspended:1;
> > Why do you need this flag? Is suspend going to happen multiple times in
> > an out-of-order manner?
> 
> We are using this flag in the resume function to check whether we suspended
> and disabled
> 
> the clocks in the suspend path. And we want to use this flag to control the
> access to dbi etc
> 
> after suspend.
> 

Okay. The DBI access patch is not included in this series, so you should
mention it in the commit message.

Thanks,
Mani

> > >   };
> > >   #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
> > > @@ -1308,6 +1315,23 @@ static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
> > >   	clk_disable_unprepare(res->pipe_clk);
> > >   }
> > [...]
> > 
> > > +static const struct dev_pm_ops qcom_pcie_pm_ops = {
> > > +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend, qcom_pcie_pm_resume)
> > Use the new macro: NOIRQ_SYSTEM_SLEEP_PM_OPS
> Will update in the next patch.
> > > +};
> > > +
> > >   static const struct of_device_id qcom_pcie_match[] = {
> > >   	{ .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
> > >   	{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
> > > @@ -1679,6 +1759,7 @@ static struct platform_driver qcom_pcie_driver = {
> > >   	.probe = qcom_pcie_probe,
> > >   	.driver = {
> > >   		.name = "qcom-pcie",
> > > +		.pm = &qcom_pcie_pm_ops,
> > There will be warnings when CONFIG_PM_SLEEP is not set. So use below,
> will update in the next patch.
> > 
> > 		.pm = pm_sleep_ptr(&qcom_pcie_pm_ops),
> > 
> > Thanks,
> > Mani
> > 
> > >   		.suppress_bind_attrs = true,
> > >   		.of_match_table = qcom_pcie_match,
> > >   	},
> > > -- 
> > > 2.7.4
> > > 

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

Re: [PATCH v2 1/2] PCI: qcom: Add system PM support

[Manivannan Sadhasivam]

On Mon, Jul 04, 2022 at 10:14:06AM +0530, Manivannan Sadhasivam wrote:
> On Thu, Jun 30, 2022 at 03:29:33PM +0530, Krishna Chaitanya Chundru wrote:
> > 
> > On 6/30/2022 10:04 AM, Manivannan Sadhasivam wrote:
> > > On Wed, Jun 29, 2022 at 03:03:33PM +0530, Krishna chaitanya chundru wrote:
> > > > Add suspend and resume pm callbacks.
> > > > 
> > > > When system suspends, and if the link is in L1ss, disable the clocks
> > > > so that system enters into low power state to save the maximum power.
> > > > And when the system resumes, enable the clocks back if they are
> > > > disabled in the suspend path.
> > > > 
> > > Why only during L1ss and not L2/L3?
> > 
> > with aspm the link will automatically go to L1ss. for L2/L3 entry we need to
> > explicitly send
> > 
> > PME turn off which we are not doing now. So we are checking only for L1ss.
> > 
> 
> Okay, please mention this in the commit message.
> 
> > > 
> > > > Changes since v1:
> > > > 	- Fixed compilation errors.
> > > > 
> > > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > > > ---
> > > >   drivers/pci/controller/dwc/pcie-qcom.c | 81 ++++++++++++++++++++++++++++++++++
> > > >   1 file changed, 81 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > index 6ab9089..8e9ef37 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > @@ -41,6 +41,9 @@
> > > >   #define L23_CLK_RMV_DIS				BIT(2)
> > > >   #define L1_CLK_RMV_DIS				BIT(1)
> > > > +#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)
> > > > @@ -190,6 +193,8 @@ 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 (*enable_clks)(struct qcom_pcie *pcie);
> > > > +	int (*disable_clks)(struct qcom_pcie *pcie);
> > > I think these could vary between platforms. Like some other platform may try to
> > > disable regulators etc... So use names such as suspend and resume.
> > Sure will change in the next patch.
> > > >   };
> > > >   struct qcom_pcie_cfg {
> > > > @@ -199,6 +204,7 @@ struct qcom_pcie_cfg {
> > > >   	unsigned int has_ddrss_sf_tbu_clk:1;
> > > >   	unsigned int has_aggre0_clk:1;
> > > >   	unsigned int has_aggre1_clk:1;
> > > > +	unsigned int support_pm_ops:1;
> > > >   };
> > > >   struct qcom_pcie {
> > > > @@ -209,6 +215,7 @@ struct qcom_pcie {
> > > >   	struct phy *phy;
> > > >   	struct gpio_desc *reset;
> > > >   	const struct qcom_pcie_cfg *cfg;
> > > > +	unsigned int is_suspended:1;
> > > Why do you need this flag? Is suspend going to happen multiple times in
> > > an out-of-order manner?
> > 
> > We are using this flag in the resume function to check whether we suspended
> > and disabled
> > 
> > the clocks in the suspend path. And we want to use this flag to control the
> > access to dbi etc
> > 
> > after suspend.
> > 
> 
> Okay. The DBI access patch is not included in this series, so you should
> mention it in the commit message.
> 

Ah, it is the patch 2/2. Still you can mention the reasoning.

Thanks,
Mani

> Thanks,
> Mani
> 
> > > >   };
> > > >   #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
> > > > @@ -1308,6 +1315,23 @@ static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
> > > >   	clk_disable_unprepare(res->pipe_clk);
> > > >   }
> > > [...]
> > > 
> > > > +static const struct dev_pm_ops qcom_pcie_pm_ops = {
> > > > +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend, qcom_pcie_pm_resume)
> > > Use the new macro: NOIRQ_SYSTEM_SLEEP_PM_OPS
> > Will update in the next patch.
> > > > +};
> > > > +
> > > >   static const struct of_device_id qcom_pcie_match[] = {
> > > >   	{ .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
> > > >   	{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
> > > > @@ -1679,6 +1759,7 @@ static struct platform_driver qcom_pcie_driver = {
> > > >   	.probe = qcom_pcie_probe,
> > > >   	.driver = {
> > > >   		.name = "qcom-pcie",
> > > > +		.pm = &qcom_pcie_pm_ops,
> > > There will be warnings when CONFIG_PM_SLEEP is not set. So use below,
> > will update in the next patch.
> > > 
> > > 		.pm = pm_sleep_ptr(&qcom_pcie_pm_ops),
> > > 
> > > Thanks,
> > > Mani
> > > 
> > > >   		.suppress_bind_attrs = true,
> > > >   		.of_match_table = qcom_pcie_match,
> > > >   	},
> > > > -- 
> > > > 2.7.4
> > > > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

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

Re: [PATCH v2 2/2] PCI: qcom: Restrict pci transactions after pci suspend

[Manivannan Sadhasivam]

On Wed, Jun 29, 2022 at 03:03:34PM +0530, Krishna chaitanya chundru wrote:
> If the endpoint device state is D0 and irq's are not freed, then
> kernel try to mask interrupts by writing in to the vector
> table (for MSIX interrupts) and config space (for MSI's).
> 
> These transactions are initiated after clocks are getting disabled
> as part of PM suspend call. Due to it, these transactions are
> resulting in un-clocked access and eventual to crashes.
> 
> So added a logic in qcom driver to restrict the unclocked access.
> And updated the logic to check the link state before masking
> or unmasking the interrupts.
> 

No other PCI driver is doing the DBI access restriction. So this makes me feel
that the fix is somewhere else.

I'll dig into it and come back.

Thanks,
Mani

> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 14 +++++++--
>  drivers/pci/controller/dwc/pcie-qcom.c            | 35 +++++++++++++++++++++--
>  2 files changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 2fa86f3..2a46b40 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -29,13 +29,23 @@ static void dw_msi_ack_irq(struct irq_data *d)
>  
>  static void dw_msi_mask_irq(struct irq_data *d)
>  {
> -	pci_msi_mask_irq(d);
> +	struct pcie_port *pp = irq_data_get_irq_chip_data(d->parent_data);
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
> +	if (dw_pcie_link_up(pci))
> +		pci_msi_mask_irq(d);
> +
>  	irq_chip_mask_parent(d);
>  }
>  
>  static void dw_msi_unmask_irq(struct irq_data *d)
>  {
> -	pci_msi_unmask_irq(d);
> +	struct pcie_port *pp = irq_data_get_irq_chip_data(d->parent_data);
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
> +	if (dw_pcie_link_up(pci))
> +		pci_msi_unmask_irq(d);
> +
>  	irq_chip_unmask_parent(d);
>  }
>  
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 8e9ef37..227bc24a 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1331,12 +1331,41 @@ static int qcom_pcie_disable_clks_2_7_0(struct qcom_pcie *pcie)
>  	return 0;
>  }
>  
> +static u32 qcom_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
> +				u32 reg, size_t size)
> +{
> +	struct qcom_pcie *pcie = to_qcom_pcie(pci);
> +	u32 val;
> +
> +	if (pcie->is_suspended)
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +
> +	dw_pcie_read(base + reg, size, &val);
> +	return val;
> +}
> +
> +static void qcom_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
> +					u32 reg, size_t size, u32 val)
> +{
> +	struct qcom_pcie *pcie = to_qcom_pcie(pci);
> +
> +	if (pcie->is_suspended)
> +		return;
> +
> +	dw_pcie_write(base + reg, size, val);
> +}
>  
>  static int qcom_pcie_link_up(struct dw_pcie *pci)
>  {
> -	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> -	u16 val = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
> +	struct qcom_pcie *pcie = to_qcom_pcie(pci);
> +	u16 offset;
> +	u16 val;
> +
> +	if (pcie->is_suspended)
> +		return false;
>  
> +	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	val = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
>  	return !!(val & PCI_EXP_LNKSTA_DLLLA);
>  }
>  
> @@ -1580,6 +1609,8 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
>  static const struct dw_pcie_ops dw_pcie_ops = {
>  	.link_up = qcom_pcie_link_up,
>  	.start_link = qcom_pcie_start_link,
> +	.read_dbi = qcom_pcie_read_dbi,
> +	.write_dbi = qcom_pcie_write_dbi,
>  };
>  
>  static int qcom_pcie_probe(struct platform_device *pdev)
> -- 
> 2.7.4
> 

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

[PATCH v4 0/2] PCI: Restrict pci transactions after pci suspend

[Krishna chaitanya chundru]

If the endpoint device state is D0 and irq's are not freed, then
kernel try to mask interrupts in system suspend path by writing
in to the vector table (for MSIX interrupts) and config space (for MSI's).

These transactions are initiated in the pm suspend after pcie clocks got
disabled as part of platform driver pm  suspend call. Due to it, these
transactions are resulting in un-clocked access and eventually to crashes.

So added a logic in qcom driver to restrict these unclocked access.
And updated the logic to check the link state before masking
or unmasking the interrupts.


Krishna chaitanya chundru (2):
  PCI: qcom: Add system PM support
  PCI: qcom: Restrict pci transactions after pci suspend

 drivers/pci/controller/dwc/pcie-designware-host.c |  14 ++-
 drivers/pci/controller/dwc/pcie-qcom.c            | 121 +++++++++++++++++++++-
 2 files changed, 131 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH v4 1/2] PCI: qcom: Add system PM support

[Krishna chaitanya chundru]

Add suspend and resume pm callbacks.

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
no where 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. And this flag is being used to restrict the access to
config space, dbi etc when clock are turned-off.

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.

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

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 6ab9089..0a9d1ee 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -41,6 +41,9 @@
 #define L23_CLK_RMV_DIS				BIT(2)
 #define L1_CLK_RMV_DIS				BIT(1)
 
+#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)
@@ -190,6 +193,8 @@ 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 {
@@ -199,6 +204,7 @@ struct qcom_pcie_cfg {
 	unsigned int has_ddrss_sf_tbu_clk:1;
 	unsigned int has_aggre0_clk:1;
 	unsigned int has_aggre1_clk:1;
+	unsigned int supports_system_suspend:1;
 };
 
 struct qcom_pcie {
@@ -209,6 +215,7 @@ 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)
@@ -1308,6 +1315,33 @@ static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
 	clk_disable_unprepare(res->pipe_clk);
 }
 
+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;
+
+	clk_prepare_enable(res->pipe_clk);
+
+	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);
+
+	clk_disable_unprepare(res->pipe_clk);
+
+	return 0;
+}
+
 static int qcom_pcie_link_up(struct dw_pcie *pci)
 {
 	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
@@ -1496,6 +1530,8 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
 	.post_init = qcom_pcie_post_init_2_7_0,
 	.post_deinit = qcom_pcie_post_deinit_2_7_0,
 	.config_sid = qcom_pcie_config_sid_sm8250,
+	.suspend = qcom_pcie_suspend_2_7_0,
+	.resume = qcom_pcie_resume_2_7_0,
 };
 
 static const struct qcom_pcie_cfg apq8084_cfg = {
@@ -1548,6 +1584,7 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
 	.ops = &ops_1_9_0,
 	.has_tbu_clk = true,
 	.pipe_clk_need_muxing = true,
+	.supports_system_suspend = true,
 };
 
 static const struct dw_pcie_ops dw_pcie_ops = {
@@ -1591,6 +1628,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 
 	pcie->cfg = pcie_cfg;
 
+	pcie->is_suspended = false;
+
 	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
 	if (IS_ERR(pcie->reset)) {
 		ret = PTR_ERR(pcie->reset);
@@ -1645,6 +1684,51 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static int __maybe_unused qcom_pcie_pm_suspend(struct device *dev)
+{
+	struct qcom_pcie *pcie = dev_get_drvdata(dev);
+	u32 val;
+
+	if (!pcie->cfg->supports_system_suspend)
+		return 0;
+
+	/* 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;
+	}
+
+	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 device *dev)
+{
+	struct qcom_pcie *pcie = dev_get_drvdata(dev);
+
+	if (!pcie->cfg->supports_system_suspend)
+		return 0;
+
+	if (!pcie->is_suspended)
+		return 0;
+
+	if (pcie->cfg->ops->resume)
+		pcie->cfg->ops->resume(pcie);
+
+	pcie->is_suspended = false;
+
+	return 0;
+}
+
+static const struct dev_pm_ops qcom_pcie_pm_ops = {
+	NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend, qcom_pcie_pm_resume)
+};
+
 static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
 	{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
@@ -1679,6 +1763,7 @@ static struct platform_driver qcom_pcie_driver = {
 	.probe = qcom_pcie_probe,
 	.driver = {
 		.name = "qcom-pcie",
+		.pm = pm_sleep_ptr(&qcom_pcie_pm_ops),
 		.suppress_bind_attrs = true,
 		.of_match_table = qcom_pcie_match,
 	},
-- 
2.7.4

[PATCH v4 2/2] PCI: qcom: Restrict pci transactions after pci suspend

[Krishna chaitanya chundru]

If the endpoint device state is D0 and irq's are not freed, then
kernel try to mask interrupts in system suspend path by writing
in to the vector table (for MSIX interrupts) and config space (for MSI's).

These transactions are initiated in the pm suspend after pcie clocks got
disabled as part of platform driver pm  suspend call. Due to it, these
transactions are resulting in un-clocked access and eventually to crashes.

So added a logic in qcom driver to restrict these unclocked access.
And updated the logic to check the link state before masking
or unmasking the interrupts.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 14 +++++++--
 drivers/pci/controller/dwc/pcie-qcom.c            | 36 +++++++++++++++++++++--
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 2fa86f3..2a46b40 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -29,13 +29,23 @@ static void dw_msi_ack_irq(struct irq_data *d)
 
 static void dw_msi_mask_irq(struct irq_data *d)
 {
-	pci_msi_mask_irq(d);
+	struct pcie_port *pp = irq_data_get_irq_chip_data(d->parent_data);
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+	if (dw_pcie_link_up(pci))
+		pci_msi_mask_irq(d);
+
 	irq_chip_mask_parent(d);
 }
 
 static void dw_msi_unmask_irq(struct irq_data *d)
 {
-	pci_msi_unmask_irq(d);
+	struct pcie_port *pp = irq_data_get_irq_chip_data(d->parent_data);
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+	if (dw_pcie_link_up(pci))
+		pci_msi_unmask_irq(d);
+
 	irq_chip_unmask_parent(d);
 }
 
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 0a9d1ee..78bc463 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1342,11 +1342,41 @@ static int qcom_pcie_suspend_2_7_0(struct qcom_pcie *pcie)
 	return 0;
 }
 
+static u32 qcom_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
+				u32 reg, size_t size)
+{
+	struct qcom_pcie *pcie = to_qcom_pcie(pci);
+	u32 val;
+
+	if (pcie->is_suspended)
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+
+	dw_pcie_read(base + reg, size, &val);
+	return val;
+}
+
+static void qcom_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
+				u32 reg, size_t size, u32 val)
+{
+	struct qcom_pcie *pcie = to_qcom_pcie(pci);
+
+	if (pcie->is_suspended)
+		return;
+
+	dw_pcie_write(base + reg, size, val);
+}
+
 static int qcom_pcie_link_up(struct dw_pcie *pci)
 {
-	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
-	u16 val = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
+	struct qcom_pcie *pcie = to_qcom_pcie(pci);
+	u16 offset;
+	u16 val;
+
+	if (pcie->is_suspended)
+		return false;
 
+	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	val = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
 	return !!(val & PCI_EXP_LNKSTA_DLLLA);
 }
 
@@ -1590,6 +1620,8 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
 static const struct dw_pcie_ops dw_pcie_ops = {
 	.link_up = qcom_pcie_link_up,
 	.start_link = qcom_pcie_start_link,
+	.read_dbi = qcom_pcie_read_dbi,
+	.write_dbi = qcom_pcie_write_dbi,
 };
 
 static int qcom_pcie_probe(struct platform_device *pdev)
-- 
2.7.4

Re: [PATCH v4 0/2] PCI: Restrict pci transactions after pci suspend

[Dmitry Baryshkov]

On 06/07/2022 17:40, Krishna chaitanya chundru wrote:
> If the endpoint device state is D0 and irq's are not freed, then
> kernel try to mask interrupts in system suspend path by writing
> in to the vector table (for MSIX interrupts) and config space (for MSI's).
> 
> These transactions are initiated in the pm suspend after pcie clocks got
> disabled as part of platform driver pm  suspend call. Due to it, these
> transactions are resulting in un-clocked access and eventually to crashes.
> 
> So added a logic in qcom driver to restrict these unclocked access.
> And updated the logic to check the link state before masking
> or unmasking the interrupts.

Please do not send new versions as replies to previous ones. This breaks 
threading for the reviewers.

> Krishna chaitanya chundru (2):
>    PCI: qcom: Add system PM support
>    PCI: qcom: Restrict pci transactions after pci suspend
> 
>   drivers/pci/controller/dwc/pcie-designware-host.c |  14 ++-
>   drivers/pci/controller/dwc/pcie-qcom.c            | 121 +++++++++++++++++++++-
>   2 files changed, 131 insertions(+), 4 deletions(-)


-- 
With best wishes
Dmitry

Re: [PATCH v4 1/2] PCI: qcom: Add system PM support

[Matthias Kaehlcke]

On Wed, Jul 06, 2022 at 08:10:24PM +0530, Krishna chaitanya chundru wrote:
> Add suspend and resume pm callbacks.
> 
> 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
> no where 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. And this flag is being used to restrict the access to
> config space, dbi etc when clock are turned-off.
> 
> 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.
> 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 85 ++++++++++++++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 6ab9089..0a9d1ee 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -41,6 +41,9 @@
>  #define L23_CLK_RMV_DIS				BIT(2)
>  #define L1_CLK_RMV_DIS				BIT(1)
>  
> +#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)
> @@ -190,6 +193,8 @@ 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 {
> @@ -199,6 +204,7 @@ struct qcom_pcie_cfg {
>  	unsigned int has_ddrss_sf_tbu_clk:1;
>  	unsigned int has_aggre0_clk:1;
>  	unsigned int has_aggre1_clk:1;
> +	unsigned int supports_system_suspend:1;
>  };
>  
>  struct qcom_pcie {
> @@ -209,6 +215,7 @@ 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)
> @@ -1308,6 +1315,33 @@ static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
>  	clk_disable_unprepare(res->pipe_clk);
>  }
>  
> +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;
> +
> +	clk_prepare_enable(res->pipe_clk);
> +
> +	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);
> +
> +	clk_disable_unprepare(res->pipe_clk);
> +
> +	return 0;
> +}
> +
>  static int qcom_pcie_link_up(struct dw_pcie *pci)
>  {
>  	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> @@ -1496,6 +1530,8 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
>  	.post_init = qcom_pcie_post_init_2_7_0,
>  	.post_deinit = qcom_pcie_post_deinit_2_7_0,
>  	.config_sid = qcom_pcie_config_sid_sm8250,
> +	.suspend = qcom_pcie_suspend_2_7_0,
> +	.resume = qcom_pcie_resume_2_7_0,
>  };
>  
>  static const struct qcom_pcie_cfg apq8084_cfg = {
> @@ -1548,6 +1584,7 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
>  	.ops = &ops_1_9_0,
>  	.has_tbu_clk = true,
>  	.pipe_clk_need_muxing = true,
> +	.supports_system_suspend = true,
>  };
>  
>  static const struct dw_pcie_ops dw_pcie_ops = {
> @@ -1591,6 +1628,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  
>  	pcie->cfg = pcie_cfg;
>  
> +	pcie->is_suspended = false;
> +
>  	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
>  	if (IS_ERR(pcie->reset)) {
>  		ret = PTR_ERR(pcie->reset);
> @@ -1645,6 +1684,51 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> +static int __maybe_unused qcom_pcie_pm_suspend(struct device *dev)
> +{
> +	struct qcom_pcie *pcie = dev_get_drvdata(dev);
> +	u32 val;
> +
> +	if (!pcie->cfg->supports_system_suspend)
> +		return 0;
> +
> +	/* 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;
> +	}
> +
> +	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 device *dev)
> +{
> +	struct qcom_pcie *pcie = dev_get_drvdata(dev);
> +
> +	if (!pcie->cfg->supports_system_suspend)
> +		return 0;

The above check can be omitted, it is implied by the next one.
'is_suspended' can only be true when system suspend is supported.

> +
> +	if (!pcie->is_suspended)
> +		return 0;
> +
> +	if (pcie->cfg->ops->resume)
> +		pcie->cfg->ops->resume(pcie);
> +
> +	pcie->is_suspended = false;
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops qcom_pcie_pm_ops = {
> +	NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend, qcom_pcie_pm_resume)
> +};
> +
>  static const struct of_device_id qcom_pcie_match[] = {
>  	{ .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
>  	{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
> @@ -1679,6 +1763,7 @@ static struct platform_driver qcom_pcie_driver = {
>  	.probe = qcom_pcie_probe,
>  	.driver = {
>  		.name = "qcom-pcie",
> +		.pm = pm_sleep_ptr(&qcom_pcie_pm_ops),
>  		.suppress_bind_attrs = true,
>  		.of_match_table = qcom_pcie_match,
>  	},
> -- 
> 2.7.4
> 

Re: [PATCH v4 2/2] PCI: qcom: Restrict pci transactions after pci suspend

[Matthias Kaehlcke]

On Wed, Jul 06, 2022 at 08:10:25PM +0530, Krishna chaitanya chundru wrote:
> If the endpoint device state is D0 and irq's are not freed, then
> kernel try to mask interrupts in system suspend path by writing
> in to the vector table (for MSIX interrupts) and config space (for MSI's).
> 
> These transactions are initiated in the pm suspend after pcie clocks got
> disabled as part of platform driver pm  suspend call. Due to it, these
> transactions are resulting in un-clocked access and eventually to crashes.
> 
> So added a logic in qcom driver to restrict these unclocked access.
> And updated the logic to check the link state before masking
> or unmasking the interrupts.
> 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 14 +++++++--
>  drivers/pci/controller/dwc/pcie-qcom.c            | 36 +++++++++++++++++++++--
>  2 files changed, 46 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 2fa86f3..2a46b40 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -29,13 +29,23 @@ static void dw_msi_ack_irq(struct irq_data *d)
>  
>  static void dw_msi_mask_irq(struct irq_data *d)
>  {
> -	pci_msi_mask_irq(d);
> +	struct pcie_port *pp = irq_data_get_irq_chip_data(d->parent_data);
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
> +	if (dw_pcie_link_up(pci))
> +		pci_msi_mask_irq(d);
> +
>  	irq_chip_mask_parent(d);
>  }
>  
>  static void dw_msi_unmask_irq(struct irq_data *d)
>  {
> -	pci_msi_unmask_irq(d);
> +	struct pcie_port *pp = irq_data_get_irq_chip_data(d->parent_data);
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
> +	if (dw_pcie_link_up(pci))
> +		pci_msi_unmask_irq(d);
> +
>  	irq_chip_unmask_parent(d);
>  }
>  
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 0a9d1ee..78bc463 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1342,11 +1342,41 @@ static int qcom_pcie_suspend_2_7_0(struct qcom_pcie *pcie)
>  	return 0;
>  }
>  
> +static u32 qcom_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
> +				u32 reg, size_t size)
> +{
> +	struct qcom_pcie *pcie = to_qcom_pcie(pci);
> +	u32 val;
> +
> +	if (pcie->is_suspended)
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +
> +	dw_pcie_read(base + reg, size, &val);
> +	return val;
> +}
> +
> +static void qcom_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
> +				u32 reg, size_t size, u32 val)
> +{
> +	struct qcom_pcie *pcie = to_qcom_pcie(pci);
> +
> +	if (pcie->is_suspended)
> +		return;
> +
> +	dw_pcie_write(base + reg, size, val);
> +}
> +
>  static int qcom_pcie_link_up(struct dw_pcie *pci)
>  {
> -	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> -	u16 val = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
> +	struct qcom_pcie *pcie = to_qcom_pcie(pci);
> +	u16 offset;
> +	u16 val;
> +
> +	if (pcie->is_suspended)
> +		return false;
>  
> +	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	val = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
>  	return !!(val & PCI_EXP_LNKSTA_DLLLA);
>  }
>  
> @@ -1590,6 +1620,8 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
>  static const struct dw_pcie_ops dw_pcie_ops = {
>  	.link_up = qcom_pcie_link_up,
>  	.start_link = qcom_pcie_start_link,
> +	.read_dbi = qcom_pcie_read_dbi,
> +	.write_dbi = qcom_pcie_write_dbi,
>  };
>  
>  static int qcom_pcie_probe(struct platform_device *pdev)

This patch fixes an issue that is introduced by the previous patch of the
series, i.e. the first patch can not be applied by itself without breaking
things. This is generally avoided as it complicates bisecting.

This patch should be before 'PCI: qcom: Add system PM support' in the series
(which obviously requires shuffling where the 'is_suspended' flag is added).
With that the series can be applied partially without introducing unclocked
reads or writes.

Re: [PATCH v4 1/2] PCI: qcom: Add system PM support

[Krishna Chaitanya Chundru]

On 7/12/2022 3:23 AM, Matthias Kaehlcke wrote:
> On Wed, Jul 06, 2022 at 08:10:24PM +0530, Krishna chaitanya chundru wrote:
>> Add suspend and resume pm callbacks.
>>
>> 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
>> no where 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. And this flag is being used to restrict the access to
>> config space, dbi etc when clock are turned-off.
>>
>> 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.
>>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-qcom.c | 85 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 85 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 6ab9089..0a9d1ee 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -41,6 +41,9 @@
>>   #define L23_CLK_RMV_DIS				BIT(2)
>>   #define L1_CLK_RMV_DIS				BIT(1)
>>   
>> +#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)
>> @@ -190,6 +193,8 @@ 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 {
>> @@ -199,6 +204,7 @@ struct qcom_pcie_cfg {
>>   	unsigned int has_ddrss_sf_tbu_clk:1;
>>   	unsigned int has_aggre0_clk:1;
>>   	unsigned int has_aggre1_clk:1;
>> +	unsigned int supports_system_suspend:1;
>>   };
>>   
>>   struct qcom_pcie {
>> @@ -209,6 +215,7 @@ 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)
>> @@ -1308,6 +1315,33 @@ static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
>>   	clk_disable_unprepare(res->pipe_clk);
>>   }
>>   
>> +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;
>> +
>> +	clk_prepare_enable(res->pipe_clk);
>> +
>> +	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);
>> +
>> +	clk_disable_unprepare(res->pipe_clk);
>> +
>> +	return 0;
>> +}
>> +
>>   static int qcom_pcie_link_up(struct dw_pcie *pci)
>>   {
>>   	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>> @@ -1496,6 +1530,8 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
>>   	.post_init = qcom_pcie_post_init_2_7_0,
>>   	.post_deinit = qcom_pcie_post_deinit_2_7_0,
>>   	.config_sid = qcom_pcie_config_sid_sm8250,
>> +	.suspend = qcom_pcie_suspend_2_7_0,
>> +	.resume = qcom_pcie_resume_2_7_0,
>>   };
>>   
>>   static const struct qcom_pcie_cfg apq8084_cfg = {
>> @@ -1548,6 +1584,7 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
>>   	.ops = &ops_1_9_0,
>>   	.has_tbu_clk = true,
>>   	.pipe_clk_need_muxing = true,
>> +	.supports_system_suspend = true,
>>   };
>>   
>>   static const struct dw_pcie_ops dw_pcie_ops = {
>> @@ -1591,6 +1628,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>   
>>   	pcie->cfg = pcie_cfg;
>>   
>> +	pcie->is_suspended = false;
>> +
>>   	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
>>   	if (IS_ERR(pcie->reset)) {
>>   		ret = PTR_ERR(pcie->reset);
>> @@ -1645,6 +1684,51 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>   	return ret;
>>   }
>>   
>> +static int __maybe_unused qcom_pcie_pm_suspend(struct device *dev)
>> +{
>> +	struct qcom_pcie *pcie = dev_get_drvdata(dev);
>> +	u32 val;
>> +
>> +	if (!pcie->cfg->supports_system_suspend)
>> +		return 0;
>> +
>> +	/* 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;
>> +	}
>> +
>> +	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 device *dev)
>> +{
>> +	struct qcom_pcie *pcie = dev_get_drvdata(dev);
>> +
>> +	if (!pcie->cfg->supports_system_suspend)
>> +		return 0;
> The above check can be omitted, it is implied by the next one.
> 'is_suspended' can only be true when system suspend is supported.
Sure will remove in the next patch.
>
>> +
>> +	if (!pcie->is_suspended)
>> +		return 0;
>> +
>> +	if (pcie->cfg->ops->resume)
>> +		pcie->cfg->ops->resume(pcie);
>> +
>> +	pcie->is_suspended = false;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct dev_pm_ops qcom_pcie_pm_ops = {
>> +	NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend, qcom_pcie_pm_resume)
>> +};
>> +
>>   static const struct of_device_id qcom_pcie_match[] = {
>>   	{ .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
>>   	{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
>> @@ -1679,6 +1763,7 @@ static struct platform_driver qcom_pcie_driver = {
>>   	.probe = qcom_pcie_probe,
>>   	.driver = {
>>   		.name = "qcom-pcie",
>> +		.pm = pm_sleep_ptr(&qcom_pcie_pm_ops),
>>   		.suppress_bind_attrs = true,
>>   		.of_match_table = qcom_pcie_match,
>>   	},
>> -- 
>> 2.7.4
>>

Re: [PATCH v4 1/2] PCI: qcom: Add system PM support

[Johan Hovold]

On Wed, Jul 06, 2022 at 08:10:24PM +0530, Krishna chaitanya chundru wrote:
> Add suspend and resume pm callbacks.
> 
> 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
> no where 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. And this flag is being used to restrict the access to
> config space, dbi etc when clock are turned-off.
 
> 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.

Changelogs typically go below the --- line below so that they don't end
up in the git logs.
 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 85 ++++++++++++++++++++++++++++++++++
>  1 file changed, 85 insertions(+)

> +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;
> +
> +	clk_prepare_enable(res->pipe_clk);

Note that pipe clock management has now been removed from this driver.

Please consider rebasing on this branch:

	https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/ctrl/qcom

> +
> +	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
> +
> +	phy_power_on(pcie->phy);
> +
> +	return ret;
> +}

Johan

Re: [PATCH v4 2/2] PCI: qcom: Restrict pci transactions after pci suspend

[Johan Hovold]

On Wed, Jul 06, 2022 at 08:10:25PM +0530, Krishna chaitanya chundru wrote:
> If the endpoint device state is D0 and irq's are not freed, then
> kernel try to mask interrupts in system suspend path by writing
> in to the vector table (for MSIX interrupts) and config space (for MSI's).
> 
> These transactions are initiated in the pm suspend after pcie clocks got
> disabled as part of platform driver pm  suspend call. Due to it, these
> transactions are resulting in un-clocked access and eventually to crashes.
> 
> So added a logic in qcom driver to restrict these unclocked access.
> And updated the logic to check the link state before masking
> or unmasking the interrupts.
> 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 14 +++++++--
>  drivers/pci/controller/dwc/pcie-qcom.c            | 36 +++++++++++++++++++++--
>  2 files changed, 46 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 2fa86f3..2a46b40 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -29,13 +29,23 @@ static void dw_msi_ack_irq(struct irq_data *d)
>  
>  static void dw_msi_mask_irq(struct irq_data *d)
>  {
> -	pci_msi_mask_irq(d);
> +	struct pcie_port *pp = irq_data_get_irq_chip_data(d->parent_data);
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);

When rebasing you'll notice that struct pcie_port has now been renamed:

	60b3c27fb9b9 ("PCI: dwc: Rename struct pcie_port to dw_pcie_rp")

Johan