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.
And some devices are taking time to settle the link in L1ss, so added a
retry logic in the suspend ops.
Krishna chaitanya chundru (3):
PCI: qcom: Add system PM support
PCI: qcom: Restrict pci transactions after pci suspend
PCI: qcom: Add retry logic for link to be stable in L1ss
drivers/pci/controller/dwc/pcie-designware-host.c | 14 ++-
drivers/pci/controller/dwc/pcie-qcom.c | 117 +++++++++++++++++++++-
2 files changed, 127 insertions(+), 4 deletions(-)
--
2.7.4
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.
Signed-off-by: Krishna chaitanya chundru <[email protected]>
---
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 | 76 ++++++++++++++++++++++++++++++++++
1 file changed, 76 insertions(+)
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index ccf6953..9dd50fc0 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -44,6 +44,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)
@@ -211,6 +214,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 {
@@ -219,6 +224,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 {
@@ -229,6 +235,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)
@@ -1297,6 +1304,29 @@ 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;
@@ -1590,6 +1620,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 */
@@ -1655,6 +1687,7 @@ static const struct qcom_pcie_cfg sm8450_pcie1_cfg = {
static const struct qcom_pcie_cfg sc7280_cfg = {
.ops = &ops_1_9_0,
.has_tbu_clk = true,
+ .supports_system_suspend = true,
};
static const struct qcom_pcie_cfg sc8180x_cfg = {
@@ -1760,6 +1793,48 @@ 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->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 },
@@ -1796,6 +1871,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
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 <[email protected]>
---
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 9979302..2b1e395 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 9dd50fc0..f7dd5dc 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1433,11 +1433,41 @@ static int qcom_pcie_post_init_2_9_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);
}
@@ -1702,6 +1732,8 @@ static const struct qcom_pcie_cfg ipq6018_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
Hi Krishna,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on helgaas-pci/next]
[also build test ERROR on next-20220803]
[cannot apply to linus/master v5.19]
[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-Restrict-pci-transactions-after-pci-suspend/20220803-193033
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: s390-randconfig-r044-20220804 (https://download.01.org/0day-ci/archive/20220804/[email protected]/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 26dd42705c2af0b8f6e5d6cdb32c9bd5ed9524eb)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/9e799ab52db3245911b15a8903977d99554d900d
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Krishna-chaitanya-chundru/PCI-Restrict-pci-transactions-after-pci-suspend/20220803-193033
git checkout 9e799ab52db3245911b15a8903977d99554d900d
# 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=s390 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 errors (new ones prefixed by >>):
In file included from drivers/pci/controller/dwc/pcie-designware-host.c:11:
In file included from include/linux/irqchip/chained_irq.h:10:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
^
In file included from drivers/pci/controller/dwc/pcie-designware-host.c:11:
In file included from include/linux/irqchip/chained_irq.h:10:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
^
In file included from drivers/pci/controller/dwc/pcie-designware-host.c:11:
In file included from include/linux/irqchip/chained_irq.h:10:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
>> drivers/pci/controller/dwc/pcie-designware-host.c:33:24: error: static assertion failed due to requirement '__builtin_types_compatible_p(struct pcie_port, struct dw_pcie_rp) || __builtin_types_compatible_p(struct pcie_port, void)': pointer type mismatch in container_of()
struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
^~~~~~~~~~~~~~~~~~~~~~
drivers/pci/controller/dwc/pcie-designware.h:327:34: note: expanded from macro 'to_dw_pcie_from_pp'
#define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/container_of.h:19:2: note: expanded from macro 'container_of'
static_assert(__same_type(*(ptr), ((type *)0)->member) || \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:77:34: note: expanded from macro 'static_assert'
#define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:78:41: note: expanded from macro '__static_assert'
#define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
^ ~~~~
drivers/pci/controller/dwc/pcie-designware-host.c:44:24: error: static assertion failed due to requirement '__builtin_types_compatible_p(struct pcie_port, struct dw_pcie_rp) || __builtin_types_compatible_p(struct pcie_port, void)': pointer type mismatch in container_of()
struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
^~~~~~~~~~~~~~~~~~~~~~
drivers/pci/controller/dwc/pcie-designware.h:327:34: note: expanded from macro 'to_dw_pcie_from_pp'
#define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/container_of.h:19:2: note: expanded from macro 'container_of'
static_assert(__same_type(*(ptr), ((type *)0)->member) || \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:77:34: note: expanded from macro 'static_assert'
#define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:78:41: note: expanded from macro '__static_assert'
#define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
^ ~~~~
12 warnings and 2 errors generated.
vim +33 drivers/pci/controller/dwc/pcie-designware-host.c
29
30 static void dw_msi_mask_irq(struct irq_data *d)
31 {
32 struct pcie_port *pp = irq_data_get_irq_chip_data(d->parent_data);
> 33 struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
34
35 if (dw_pcie_link_up(pci))
36 pci_msi_mask_irq(d);
37
38 irq_chip_mask_parent(d);
39 }
40
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Quoting Krishna chaitanya chundru (2022-08-03 04:28:53)
> 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.
Why are the platform driver pm suspend calls disabling clks that early?
Can they disable clks in noirq phase, or even later, so that we don't
have to check if the device is clocking in the irq poking functions?
It's best to keep irq operations fast, so that irq control is fast given
that these functions are called from irq flow handlers.
On Wed, Aug 03, 2022 at 04:58:52PM +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.
>
> Signed-off-by: Krishna chaitanya chundru <[email protected]>
> ---
> 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 | 76 ++++++++++++++++++++++++++++++++++
> 1 file changed, 76 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index ccf6953..9dd50fc0 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -44,6 +44,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)
> @@ -211,6 +214,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);
This is really the wrong direction. We don't need the DWC driver
defining per platform ops and then a platform defining sub-platform ops.
IOW, qcom_pcie_ops should go away.
But for now, you don't even need the ops as only 1 version is
implemented. Do you really expect other versions to do something
different than turn off clocks and phys?
> };
>
> struct qcom_pcie_cfg {
> @@ -219,6 +224,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 {
> @@ -229,6 +235,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)
> @@ -1297,6 +1304,29 @@ 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;
> @@ -1590,6 +1620,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 */
> @@ -1655,6 +1687,7 @@ static const struct qcom_pcie_cfg sm8450_pcie1_cfg = {
> static const struct qcom_pcie_cfg sc7280_cfg = {
> .ops = &ops_1_9_0,
> .has_tbu_clk = true,
> + .supports_system_suspend = true,
> };
>
> static const struct qcom_pcie_cfg sc8180x_cfg = {
> @@ -1760,6 +1793,48 @@ 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 anything, this looks like the version specific code.
> +
> + 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->is_suspended)
> + return 0;
Pretty sure the driver core should take care of not calling resume if
not suspended.
> +
> + 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 },
> @@ -1796,6 +1871,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
>
On 8/9/2022 12:42 AM, Stephen Boyd wrote:
> Quoting Krishna chaitanya chundru (2022-08-03 04:28:53)
>> 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.
> Why are the platform driver pm suspend calls disabling clks that early?
> Can they disable clks in noirq phase, or even later, so that we don't
> have to check if the device is clocking in the irq poking functions?
> It's best to keep irq operations fast, so that irq control is fast given
> that these functions are called from irq flow handlers.
We are registering the pcie pm suspend ops as noirq ops only. And this
msix and config
access is coming at the later point of time that is reason we added that
check.
On 8/11/2022 3:20 AM, Rob Herring wrote:
> On Wed, Aug 03, 2022 at 04:58:52PM +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.
>>
>> Signed-off-by: Krishna chaitanya chundru <[email protected]>
>> ---
>> 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 | 76 ++++++++++++++++++++++++++++++++++
>> 1 file changed, 76 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index ccf6953..9dd50fc0 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -44,6 +44,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)
>> @@ -211,6 +214,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);
> This is really the wrong direction. We don't need the DWC driver
> defining per platform ops and then a platform defining sub-platform ops.
> IOW, qcom_pcie_ops should go away.
>
> But for now, you don't even need the ops as only 1 version is
> implemented. Do you really expect other versions to do something
> different than turn off clocks and phys?
We will remove this in next patch.
>
>> };
>>
>> struct qcom_pcie_cfg {
>> @@ -219,6 +224,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 {
>> @@ -229,6 +235,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)
>> @@ -1297,6 +1304,29 @@ 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;
>> @@ -1590,6 +1620,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 */
>> @@ -1655,6 +1687,7 @@ static const struct qcom_pcie_cfg sm8450_pcie1_cfg = {
>> static const struct qcom_pcie_cfg sc7280_cfg = {
>> .ops = &ops_1_9_0,
>> .has_tbu_clk = true,
>> + .supports_system_suspend = true,
>> };
>>
>> static const struct qcom_pcie_cfg sc8180x_cfg = {
>> @@ -1760,6 +1793,48 @@ 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 anything, this looks like the version specific code.
This is common for all qcom specific code. Here we are checking for link
is in l1ss or not
If the link is present in l1ss only we go ahead turn off clocks and phy.
>
>> +
>> + 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->is_suspended)
>> + return 0;
> Pretty sure the driver core should take care of not calling resume if
> not suspended.
We are returning success in suspend call even if we didn't turn off
clocks/phy. so I am using
this flag to know the clocks are off/not.
>
>> +
>> + 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 },
>> @@ -1796,6 +1871,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
>>
On 8/24/2022 8:58 AM, Krishna Chaitanya Chundru wrote:
>
> On 8/11/2022 3:20 AM, Rob Herring wrote:
>> On Wed, Aug 03, 2022 at 04:58:52PM +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.
>>>
>>> Signed-off-by: Krishna chaitanya chundru <[email protected]>
>>> ---
>>> 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 | 76
>>> ++++++++++++++++++++++++++++++++++
>>> 1 file changed, 76 insertions(+)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c
>>> b/drivers/pci/controller/dwc/pcie-qcom.c
>>> index ccf6953..9dd50fc0 100644
>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>> @@ -44,6 +44,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)
>>> @@ -211,6 +214,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);
>> This is really the wrong direction. We don't need the DWC driver
>> defining per platform ops and then a platform defining sub-platform ops.
>> IOW, qcom_pcie_ops should go away.
>>
>> But for now, you don't even need the ops as only 1 version is
>> implemented. Do you really expect other versions to do something
>> different than turn off clocks and phys?
> We will remove this in next patch.
Rob, each version is having a clk structure for example we are using
"struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;"
This we can't keep these in the common function. so for that reason I
added these suspend and resume ops.
>>
>>> };
>>> struct qcom_pcie_cfg {
>>> @@ -219,6 +224,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 {
>>> @@ -229,6 +235,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)
>>> @@ -1297,6 +1304,29 @@ 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;
>>> @@ -1590,6 +1620,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 */
>>> @@ -1655,6 +1687,7 @@ static const struct qcom_pcie_cfg
>>> sm8450_pcie1_cfg = {
>>> static const struct qcom_pcie_cfg sc7280_cfg = {
>>> .ops = &ops_1_9_0,
>>> .has_tbu_clk = true,
>>> + .supports_system_suspend = true,
>>> };
>>> static const struct qcom_pcie_cfg sc8180x_cfg = {
>>> @@ -1760,6 +1793,48 @@ 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 anything, this looks like the version specific code.
>
> This is common for all qcom specific code. Here we are checking for
> link is in l1ss or not
>
> If the link is present in l1ss only we go ahead turn off clocks and phy.
>
>>
>>> +
>>> + 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->is_suspended)
>>> + return 0;
>> Pretty sure the driver core should take care of not calling resume if
>> not suspended.
>
> We are returning success in suspend call even if we didn't turn off
> clocks/phy. so I am using
>
> this flag to know the clocks are off/not.
>
>>
>>> +
>>> + 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 },
>>> @@ -1796,6 +1871,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
>>>
Quoting Krishna Chaitanya Chundru (2022-08-23 20:37:59)
>
> On 8/9/2022 12:42 AM, Stephen Boyd wrote:
> > Quoting Krishna chaitanya chundru (2022-08-03 04:28:53)
> >> 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.
> > Why are the platform driver pm suspend calls disabling clks that early?
> > Can they disable clks in noirq phase, or even later, so that we don't
> > have to check if the device is clocking in the irq poking functions?
> > It's best to keep irq operations fast, so that irq control is fast given
> > that these functions are called from irq flow handlers.
>
> We are registering the pcie pm suspend ops as noirq ops only. And this
> msix and config
>
> access is coming at the later point of time that is reason we added that
> check.
>
What is accessing msix and config? Can you dump_stack() after noirq ops
are called and figure out what is trying to access the bus when it is
powered down?
On Wed, Aug 03, 2022 at 04:58:51PM +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).
If clocks are being turned off while the PCI core is still accessing
the device, I think that means qcom suspend is not implemented
correctly.
> 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.
>
> And some devices are taking time to settle the link in L1ss, so added a
> retry logic in the suspend ops.
>
> Krishna chaitanya chundru (3):
> PCI: qcom: Add system PM support
> PCI: qcom: Restrict pci transactions after pci suspend
> PCI: qcom: Add retry logic for link to be stable in L1ss
>
> drivers/pci/controller/dwc/pcie-designware-host.c | 14 ++-
> drivers/pci/controller/dwc/pcie-qcom.c | 117 +++++++++++++++++++++-
> 2 files changed, 127 insertions(+), 4 deletions(-)
>
> --
> 2.7.4
>
On 8/25/2022 1:59 AM, Bjorn Helgaas wrote:
> On Wed, Aug 03, 2022 at 04:58:51PM +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).
> If clocks are being turned off while the PCI core is still accessing
> the device, I think that means qcom suspend is not implemented
> correctly.
we are registering the suspend and resume ops as NO_IRQ pm ops and in
those suspend ops we are disbaling clks there.
NO_IRQ ops is the last the pm ops that are getting called. But we are
getting pcie access nearly at end of the suspend and near cpu disable.
The pcie access is nothing but the interrupts masks to endpoint to
disable the interrupts.
>> 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.
>>
>> And some devices are taking time to settle the link in L1ss, so added a
>> retry logic in the suspend ops.
>>
>> Krishna chaitanya chundru (3):
>> PCI: qcom: Add system PM support
>> PCI: qcom: Restrict pci transactions after pci suspend
>> PCI: qcom: Add retry logic for link to be stable in L1ss
>>
>> drivers/pci/controller/dwc/pcie-designware-host.c | 14 ++-
>> drivers/pci/controller/dwc/pcie-qcom.c | 117 +++++++++++++++++++++-
>> 2 files changed, 127 insertions(+), 4 deletions(-)
>>
>> --
>> 2.7.4
>>
On 8/24/2022 10:50 PM, Stephen Boyd wrote:
> Quoting Krishna Chaitanya Chundru (2022-08-23 20:37:59)
>> On 8/9/2022 12:42 AM, Stephen Boyd wrote:
>>> Quoting Krishna chaitanya chundru (2022-08-03 04:28:53)
>>>> 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.
>>> Why are the platform driver pm suspend calls disabling clks that early?
>>> Can they disable clks in noirq phase, or even later, so that we don't
>>> have to check if the device is clocking in the irq poking functions?
>>> It's best to keep irq operations fast, so that irq control is fast given
>>> that these functions are called from irq flow handlers.
>> We are registering the pcie pm suspend ops as noirq ops only. And this
>> msix and config
>>
>> access is coming at the later point of time that is reason we added that
>> check.
>>
> What is accessing msix and config? Can you dump_stack() after noirq ops
> are called and figure out what is trying to access the bus when it is
> powered down?
The msix and config space is being accessed to mask interrupts. The
access is coming at the end of the suspend
and near CPU disable. We tried to dump the stack there but the call
stack is not coming as it is near cpu disable.
But we got dump at resume please have look at it
[ 54.946268] Enabling non-boot CPUs ...
[ 54.951182] CPU: 1 PID: 21 Comm: cpuhp/1 Not tainted 5.15.41 #105
43491e4414b1db8a6f59d56b617b520d92a9498e
[ 54.961122] Hardware name: Qualcomm Technologies, Inc. sc7280 IDP
SKU2 platform (DT)
[ 54.969088] Call trace:
[ 54.971612] dump_backtrace+0x0/0x200
[ 54.975399] show_stack+0x20/0x2c
[ 54.978826] dump_stack_lvl+0x6c/0x90
[ 54.982614] dump_stack+0x18/0x38
[ 54.986043] dw_msi_unmask_irq+0x2c/0x58
[ 54.990096] irq_enable+0x58/0x90
[ 54.993522] __irq_startup+0x68/0x94
[ 54.997216] irq_startup+0xf4/0x140
[ 55.000820] irq_affinity_online_cpu+0xc8/0x154
[ 55.005491] cpuhp_invoke_callback+0x19c/0x6e4
[ 55.010077] cpuhp_thread_fun+0x11c/0x188
[ 55.014216] smpboot_thread_fn+0x1ac/0x30c
[ 55.018445] kthread+0x140/0x30c
[ 55.021788] ret_from_fork+0x10/0x20
[ 55.028243] CPU1 is up
So the same stack should be called at the suspend path while disabling CPU.
If there is any other way to remove these calls can you please help us
point that way.
Thanks & Regards,
Krishna Chaitanya
Quoting Krishna Chaitanya Chundru (2022-08-25 06:52:43)
>
> On 8/24/2022 10:50 PM, Stephen Boyd wrote:
> > Quoting Krishna Chaitanya Chundru (2022-08-23 20:37:59)
> >> On 8/9/2022 12:42 AM, Stephen Boyd wrote:
> >>> Quoting Krishna chaitanya chundru (2022-08-03 04:28:53)
> >>>> 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.
> >>> Why are the platform driver pm suspend calls disabling clks that early?
> >>> Can they disable clks in noirq phase, or even later, so that we don't
> >>> have to check if the device is clocking in the irq poking functions?
> >>> It's best to keep irq operations fast, so that irq control is fast given
> >>> that these functions are called from irq flow handlers.
> >> We are registering the pcie pm suspend ops as noirq ops only. And this
> >> msix and config
> >>
> >> access is coming at the later point of time that is reason we added that
> >> check.
> >>
> > What is accessing msix and config? Can you dump_stack() after noirq ops
> > are called and figure out what is trying to access the bus when it is
> > powered down?
>
> The msix and config space is being accessed to mask interrupts. The
> access is coming at the end of the suspend
>
> and near CPU disable. We tried to dump the stack there but the call
> stack is not coming as it is near cpu disable.
That is odd that you can't get a stacktrace.
>
> But we got dump at resume please have look at it
>
> [ 54.946268] Enabling non-boot CPUs ...
> [ 54.951182] CPU: 1 PID: 21 Comm: cpuhp/1 Not tainted 5.15.41 #105
> 43491e4414b1db8a6f59d56b617b520d92a9498e
> [ 54.961122] Hardware name: Qualcomm Technologies, Inc. sc7280 IDP
> SKU2 platform (DT)
> [ 54.969088] Call trace:
> [ 54.971612] dump_backtrace+0x0/0x200
> [ 54.975399] show_stack+0x20/0x2c
> [ 54.978826] dump_stack_lvl+0x6c/0x90
> [ 54.982614] dump_stack+0x18/0x38
> [ 54.986043] dw_msi_unmask_irq+0x2c/0x58
> [ 54.990096] irq_enable+0x58/0x90
> [ 54.993522] __irq_startup+0x68/0x94
> [ 54.997216] irq_startup+0xf4/0x140
> [ 55.000820] irq_affinity_online_cpu+0xc8/0x154
> [ 55.005491] cpuhp_invoke_callback+0x19c/0x6e4
> [ 55.010077] cpuhp_thread_fun+0x11c/0x188
> [ 55.014216] smpboot_thread_fn+0x1ac/0x30c
> [ 55.018445] kthread+0x140/0x30c
> [ 55.021788] ret_from_fork+0x10/0x20
> [ 55.028243] CPU1 is up
>
> So the same stack should be called at the suspend path while disabling CPU.
Sounds like you're getting hit by affinity changes while offlining CPUs
during suspend (see irq_migrate_all_off_this_cpu()). That will happen
after devices are suspended (all phases of suspend ops).
>
> If there is any other way to remove these calls can you please help us
> point that way.
I'm not sure. I believe genirq assumes the irqchips are always
accessible. There is some support to suspend irqchips. See how the
struct irq_chip::irq_suspend() function is called by syscore ops in the
generic irqchip 'irq_gc_syscore_ops' hooks. Maybe you could add a
syscore suspend/resume hook to disable/enable the clks and power to the
PCI controller. syscore ops run after secondary CPUs are hotplugged out
during suspend.
Or maybe setting the IRQCHIP_MASK_ON_SUSPEND flag can be used so that on
irq migration nothing writes the irq hardware because it is already
masked in the hardware earlier. I think the problem is that on resume
we'll restart the irq from the first CPU online event, when you don't
want to do that because it is too early.
I have another question though, which is do MSIs support wakeup? I don't
see how it works if the whole bus is effectively off during suspend. If
wakeup needs to be supported then I suspect the bus can't be powered
down during suspend.
On Fri, Aug 26, 2022 at 03:23:00PM -0500, Stephen Boyd wrote:
> Quoting Krishna Chaitanya Chundru (2022-08-25 06:52:43)
> >
> > On 8/24/2022 10:50 PM, Stephen Boyd wrote:
> > > Quoting Krishna Chaitanya Chundru (2022-08-23 20:37:59)
> > >> On 8/9/2022 12:42 AM, Stephen Boyd wrote:
> > >>> Quoting Krishna chaitanya chundru (2022-08-03 04:28:53)
> > >>>> 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.
> > >>> Why are the platform driver pm suspend calls disabling clks that early?
> > >>> Can they disable clks in noirq phase, or even later, so that we don't
> > >>> have to check if the device is clocking in the irq poking functions?
> > >>> It's best to keep irq operations fast, so that irq control is fast given
> > >>> that these functions are called from irq flow handlers.
> > >> We are registering the pcie pm suspend ops as noirq ops only. And this
> > >> msix and config
> > >>
> > >> access is coming at the later point of time that is reason we added that
> > >> check.
> > >>
> > > What is accessing msix and config? Can you dump_stack() after noirq ops
> > > are called and figure out what is trying to access the bus when it is
> > > powered down?
> >
> > The msix and config space is being accessed to mask interrupts. The
> > access is coming at the end of the suspend
> >
> > and near CPU disable. We tried to dump the stack there but the call
> > stack is not coming as it is near cpu disable.
>
> That is odd that you can't get a stacktrace.
>
> >
> > But we got dump at resume please have look at it
> >
> > [ 54.946268] Enabling non-boot CPUs ...
> > [ 54.951182] CPU: 1 PID: 21 Comm: cpuhp/1 Not tainted 5.15.41 #105
> > 43491e4414b1db8a6f59d56b617b520d92a9498e
> > [ 54.961122] Hardware name: Qualcomm Technologies, Inc. sc7280 IDP
> > SKU2 platform (DT)
> > [ 54.969088] Call trace:
> > [ 54.971612] dump_backtrace+0x0/0x200
> > [ 54.975399] show_stack+0x20/0x2c
> > [ 54.978826] dump_stack_lvl+0x6c/0x90
> > [ 54.982614] dump_stack+0x18/0x38
> > [ 54.986043] dw_msi_unmask_irq+0x2c/0x58
> > [ 54.990096] irq_enable+0x58/0x90
> > [ 54.993522] __irq_startup+0x68/0x94
> > [ 54.997216] irq_startup+0xf4/0x140
> > [ 55.000820] irq_affinity_online_cpu+0xc8/0x154
> > [ 55.005491] cpuhp_invoke_callback+0x19c/0x6e4
> > [ 55.010077] cpuhp_thread_fun+0x11c/0x188
> > [ 55.014216] smpboot_thread_fn+0x1ac/0x30c
> > [ 55.018445] kthread+0x140/0x30c
> > [ 55.021788] ret_from_fork+0x10/0x20
> > [ 55.028243] CPU1 is up
> >
> > So the same stack should be called at the suspend path while disabling CPU.
>
> Sounds like you're getting hit by affinity changes while offlining CPUs
> during suspend (see irq_migrate_all_off_this_cpu()). That will happen
> after devices are suspended (all phases of suspend ops).
The affinity setting should not happen since DWC MSI controller doesn't support
setting IRQ affinity (hierarchial IRQ domain). In the migrate_one_irq()
function, there is a check for the existence of the irq_set_affinity()
callback, but the DWC MSI controller return -EINVAL in the callback. So this
is the reason the migration was still atempted?
A quick check would be to test this suspend/resume with GIC ITS for MSI since
it supports settings IRQ affinity and resides in a separate domain.
Chaitanya, can you try that?
>
> >
> > If there is any other way to remove these calls can you please help us
> > point that way.
>
> I'm not sure. I believe genirq assumes the irqchips are always
> accessible. There is some support to suspend irqchips. See how the
> struct irq_chip::irq_suspend() function is called by syscore ops in the
> generic irqchip 'irq_gc_syscore_ops' hooks. Maybe you could add a
> syscore suspend/resume hook to disable/enable the clks and power to the
> PCI controller. syscore ops run after secondary CPUs are hotplugged out
> during suspend.
>
> Or maybe setting the IRQCHIP_MASK_ON_SUSPEND flag can be used so that on
> irq migration nothing writes the irq hardware because it is already
> masked in the hardware earlier. I think the problem is that on resume
> we'll restart the irq from the first CPU online event, when you don't
> want to do that because it is too early.
>
> I have another question though, which is do MSIs support wakeup? I don't
> see how it works if the whole bus is effectively off during suspend. If
> wakeup needs to be supported then I suspect the bus can't be powered
> down during suspend.
Wake up should be handled by a dedicated side-band GPIO or in-band PME message.
But I still wonder how the link stays in L1/L1ss when the clocks are disabled
and PHY is powered down. Maybe the link or phy is powered by a separate power
domain like MX that keeps the link active?
Thanks,
Mani
--
மணிவண்ணன் சதாசிவம்
On 8/27/2022 10:56 PM, Manivannan Sadhasivam wrote:
> On Fri, Aug 26, 2022 at 03:23:00PM -0500, Stephen Boyd wrote:
>> Quoting Krishna Chaitanya Chundru (2022-08-25 06:52:43)
>>> On 8/24/2022 10:50 PM, Stephen Boyd wrote:
>>>> Quoting Krishna Chaitanya Chundru (2022-08-23 20:37:59)
>>>>> On 8/9/2022 12:42 AM, Stephen Boyd wrote:
>>>>>> Quoting Krishna chaitanya chundru (2022-08-03 04:28:53)
>>>>>>> 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.
>>>>>> Why are the platform driver pm suspend calls disabling clks that early?
>>>>>> Can they disable clks in noirq phase, or even later, so that we don't
>>>>>> have to check if the device is clocking in the irq poking functions?
>>>>>> It's best to keep irq operations fast, so that irq control is fast given
>>>>>> that these functions are called from irq flow handlers.
>>>>> We are registering the pcie pm suspend ops as noirq ops only. And this
>>>>> msix and config
>>>>>
>>>>> access is coming at the later point of time that is reason we added that
>>>>> check.
>>>>>
>>>> What is accessing msix and config? Can you dump_stack() after noirq ops
>>>> are called and figure out what is trying to access the bus when it is
>>>> powered down?
>>> The msix and config space is being accessed to mask interrupts. The
>>> access is coming at the end of the suspend
>>>
>>> and near CPU disable. We tried to dump the stack there but the call
>>> stack is not coming as it is near cpu disable.
>> That is odd that you can't get a stacktrace.
>>
>>> But we got dump at resume please have look at it
>>>
>>> [ 54.946268] Enabling non-boot CPUs ...
>>> [ 54.951182] CPU: 1 PID: 21 Comm: cpuhp/1 Not tainted 5.15.41 #105
>>> 43491e4414b1db8a6f59d56b617b520d92a9498e
>>> [ 54.961122] Hardware name: Qualcomm Technologies, Inc. sc7280 IDP
>>> SKU2 platform (DT)
>>> [ 54.969088] Call trace:
>>> [ 54.971612] dump_backtrace+0x0/0x200
>>> [ 54.975399] show_stack+0x20/0x2c
>>> [ 54.978826] dump_stack_lvl+0x6c/0x90
>>> [ 54.982614] dump_stack+0x18/0x38
>>> [ 54.986043] dw_msi_unmask_irq+0x2c/0x58
>>> [ 54.990096] irq_enable+0x58/0x90
>>> [ 54.993522] __irq_startup+0x68/0x94
>>> [ 54.997216] irq_startup+0xf4/0x140
>>> [ 55.000820] irq_affinity_online_cpu+0xc8/0x154
>>> [ 55.005491] cpuhp_invoke_callback+0x19c/0x6e4
>>> [ 55.010077] cpuhp_thread_fun+0x11c/0x188
>>> [ 55.014216] smpboot_thread_fn+0x1ac/0x30c
>>> [ 55.018445] kthread+0x140/0x30c
>>> [ 55.021788] ret_from_fork+0x10/0x20
>>> [ 55.028243] CPU1 is up
>>>
>>> So the same stack should be called at the suspend path while disabling CPU.
>> Sounds like you're getting hit by affinity changes while offlining CPUs
>> during suspend (see irq_migrate_all_off_this_cpu()). That will happen
>> after devices are suspended (all phases of suspend ops).
> The affinity setting should not happen since DWC MSI controller doesn't support
> setting IRQ affinity (hierarchial IRQ domain). In the migrate_one_irq()
> function, there is a check for the existence of the irq_set_affinity()
> callback, but the DWC MSI controller return -EINVAL in the callback. So this
> is the reason the migration was still atempted?
>
> A quick check would be to test this suspend/resume with GIC ITS for MSI since
> it supports settings IRQ affinity and resides in a separate domain.
> Chaitanya, can you try that?
Hi mani,
I tried with gic its there also I see same behavior.
The only which helps to comment out affinity in the following function.
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 21b3ac2a29d2..042afec1cf9d 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -487,8 +487,9 @@ static int alloc_descs(unsigned int start, unsigned
int cnt, int node,
if (affinity) {
if (affinity->is_managed) {
- flags = IRQD_AFFINITY_MANAGED |
- IRQD_MANAGED_SHUTDOWN;
+// flags = IRQD_AFFINITY_MANAGED |
+// IRQD_MANAGED_SHUTDOWN;
+ flags = 0;//IRQD_AFFINITY_MANAGED |
}
mask = &affinity->mask;
node = cpu_to_node(cpumask_first(mask));
>>> If there is any other way to remove these calls can you please help us
>>> point that way.
>> I'm not sure. I believe genirq assumes the irqchips are always
>> accessible. There is some support to suspend irqchips. See how the
>> struct irq_chip::irq_suspend() function is called by syscore ops in the
>> generic irqchip 'irq_gc_syscore_ops' hooks. Maybe you could add a
>> syscore suspend/resume hook to disable/enable the clks and power to the
>> PCI controller. syscore ops run after secondary CPUs are hotplugged out
>> during suspend.
>>
>> Or maybe setting the IRQCHIP_MASK_ON_SUSPEND flag can be used so that on
>> irq migration nothing writes the irq hardware because it is already
>> masked in the hardware earlier. I think the problem is that on resume
>> we'll restart the irq from the first CPU online event, when you don't
>> want to do that because it is too early.
>>
>> I have another question though, which is do MSIs support wakeup? I don't
>> see how it works if the whole bus is effectively off during suspend. If
>> wakeup needs to be supported then I suspect the bus can't be powered
>> down during suspend.
> Wake up should be handled by a dedicated side-band GPIO or in-band PME message.
>
> But I still wonder how the link stays in L1/L1ss when the clocks are disabled
> and PHY is powered down. Maybe the link or phy is powered by a separate power
> domain like MX that keeps the link active?
We will come back to you on this.
>
> Thanks,
> Mani
>
On Mon, Aug 29, 2022 at 11:01:58PM +0530, Krishna Chaitanya Chundru wrote:
>
> On 8/27/2022 10:56 PM, Manivannan Sadhasivam wrote:
> > On Fri, Aug 26, 2022 at 03:23:00PM -0500, Stephen Boyd wrote:
> > > Quoting Krishna Chaitanya Chundru (2022-08-25 06:52:43)
> > > > On 8/24/2022 10:50 PM, Stephen Boyd wrote:
> > > > > Quoting Krishna Chaitanya Chundru (2022-08-23 20:37:59)
> > > > > > On 8/9/2022 12:42 AM, Stephen Boyd wrote:
> > > > > > > Quoting Krishna chaitanya chundru (2022-08-03 04:28:53)
> > > > > > > > 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.
> > > > > > > Why are the platform driver pm suspend calls disabling clks that early?
> > > > > > > Can they disable clks in noirq phase, or even later, so that we don't
> > > > > > > have to check if the device is clocking in the irq poking functions?
> > > > > > > It's best to keep irq operations fast, so that irq control is fast given
> > > > > > > that these functions are called from irq flow handlers.
> > > > > > We are registering the pcie pm suspend ops as noirq ops only. And this
> > > > > > msix and config
> > > > > >
> > > > > > access is coming at the later point of time that is reason we added that
> > > > > > check.
> > > > > >
> > > > > What is accessing msix and config? Can you dump_stack() after noirq ops
> > > > > are called and figure out what is trying to access the bus when it is
> > > > > powered down?
> > > > The msix and config space is being accessed to mask interrupts. The
> > > > access is coming at the end of the suspend
> > > >
> > > > and near CPU disable. We tried to dump the stack there but the call
> > > > stack is not coming as it is near cpu disable.
> > > That is odd that you can't get a stacktrace.
> > >
> > > > But we got dump at resume please have look at it
> > > >
> > > > [ 54.946268] Enabling non-boot CPUs ...
> > > > [ 54.951182] CPU: 1 PID: 21 Comm: cpuhp/1 Not tainted 5.15.41 #105
> > > > 43491e4414b1db8a6f59d56b617b520d92a9498e
> > > > [ 54.961122] Hardware name: Qualcomm Technologies, Inc. sc7280 IDP
> > > > SKU2 platform (DT)
> > > > [ 54.969088] Call trace:
> > > > [ 54.971612] dump_backtrace+0x0/0x200
> > > > [ 54.975399] show_stack+0x20/0x2c
> > > > [ 54.978826] dump_stack_lvl+0x6c/0x90
> > > > [ 54.982614] dump_stack+0x18/0x38
> > > > [ 54.986043] dw_msi_unmask_irq+0x2c/0x58
> > > > [ 54.990096] irq_enable+0x58/0x90
> > > > [ 54.993522] __irq_startup+0x68/0x94
> > > > [ 54.997216] irq_startup+0xf4/0x140
> > > > [ 55.000820] irq_affinity_online_cpu+0xc8/0x154
> > > > [ 55.005491] cpuhp_invoke_callback+0x19c/0x6e4
> > > > [ 55.010077] cpuhp_thread_fun+0x11c/0x188
> > > > [ 55.014216] smpboot_thread_fn+0x1ac/0x30c
> > > > [ 55.018445] kthread+0x140/0x30c
> > > > [ 55.021788] ret_from_fork+0x10/0x20
> > > > [ 55.028243] CPU1 is up
> > > >
> > > > So the same stack should be called at the suspend path while disabling CPU.
> > > Sounds like you're getting hit by affinity changes while offlining CPUs
> > > during suspend (see irq_migrate_all_off_this_cpu()). That will happen
> > > after devices are suspended (all phases of suspend ops).
> > The affinity setting should not happen since DWC MSI controller doesn't support
> > setting IRQ affinity (hierarchial IRQ domain). In the migrate_one_irq()
> > function, there is a check for the existence of the irq_set_affinity()
> > callback, but the DWC MSI controller return -EINVAL in the callback. So this
> > is the reason the migration was still atempted?
> >
> > A quick check would be to test this suspend/resume with GIC ITS for MSI since
> > it supports settings IRQ affinity and resides in a separate domain.
> > Chaitanya, can you try that?
>
> Hi mani,
>
> I tried with gic its there also I see same behavior.
>
Okay
> The only which helps to comment out affinity in the following function.
>
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index 21b3ac2a29d2..042afec1cf9d 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -487,8 +487,9 @@ static int alloc_descs(unsigned int start, unsigned int
> cnt, int node,
>
>
>
> if (affinity) {
> if (affinity->is_managed) {
> - flags = IRQD_AFFINITY_MANAGED |
> - IRQD_MANAGED_SHUTDOWN;
> +// flags = IRQD_AFFINITY_MANAGED |
> +// IRQD_MANAGED_SHUTDOWN;
> + flags = 0;//IRQD_AFFINITY_MANAGED |
> }
> mask = &affinity->mask;
> node = cpu_to_node(cpumask_first(mask));
>
The only solution I can think of is keeping the clocks related to DBI access
active or switch to another clock source that consumes less power if available
during suspend.
But limiting the DBI access using hacks doesn't look good.
Thanks,
Mani
> > > > If there is any other way to remove these calls can you please help us
> > > > point that way.
> > > I'm not sure. I believe genirq assumes the irqchips are always
> > > accessible. There is some support to suspend irqchips. See how the
> > > struct irq_chip::irq_suspend() function is called by syscore ops in the
> > > generic irqchip 'irq_gc_syscore_ops' hooks. Maybe you could add a
> > > syscore suspend/resume hook to disable/enable the clks and power to the
> > > PCI controller. syscore ops run after secondary CPUs are hotplugged out
> > > during suspend.
> > >
> > > Or maybe setting the IRQCHIP_MASK_ON_SUSPEND flag can be used so that on
> > > irq migration nothing writes the irq hardware because it is already
> > > masked in the hardware earlier. I think the problem is that on resume
> > > we'll restart the irq from the first CPU online event, when you don't
> > > want to do that because it is too early.
> > >
> > > I have another question though, which is do MSIs support wakeup? I don't
> > > see how it works if the whole bus is effectively off during suspend. If
> > > wakeup needs to be supported then I suspect the bus can't be powered
> > > down during suspend.
> > Wake up should be handled by a dedicated side-band GPIO or in-band PME message.
> >
> > But I still wonder how the link stays in L1/L1ss when the clocks are disabled
> > and PHY is powered down. Maybe the link or phy is powered by a separate power
> > domain like MX that keeps the link active?
> We will come back to you on this.
Okay.
Thanks,
Mani
> >
> > Thanks,
> > Mani
> >
>
--
மணிவண்ணன் சதாசிவம்
On 8/30/2022 5:25 PM, Manivannan Sadhasivam wrote:
<SNIP>...
>> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
>> index 21b3ac2a29d2..042afec1cf9d 100644
>> --- a/kernel/irq/irqdesc.c
>> +++ b/kernel/irq/irqdesc.c
>> @@ -487,8 +487,9 @@ static int alloc_descs(unsigned int start, unsigned int
>> cnt, int node,
>>
>>
>>
>> if (affinity) {
>> if (affinity->is_managed) {
>> - flags = IRQD_AFFINITY_MANAGED |
>> - IRQD_MANAGED_SHUTDOWN;
>> +// flags = IRQD_AFFINITY_MANAGED |
>> +// IRQD_MANAGED_SHUTDOWN;
>> + flags = 0;//IRQD_AFFINITY_MANAGED |
>> }
>> mask = &affinity->mask;
>> node = cpu_to_node(cpumask_first(mask));
>>
> The only solution I can think of is keeping the clocks related to DBI access
> active or switch to another clock source that consumes less power if available
> during suspend.
>
> But limiting the DBI access using hacks doesn't look good.
Why not just define "irq_startup and irq_shutdown" callbacks for dw_pcie_msi_irq_chip?
So when the cpu is offlined and irq_shutdown is called for that irqchip in migrate_one_irq(),
you would mask the irq and then disable the clocks. Similarly, on CPU onlining, you would
enable the clocks and unmask the irq. This way XO is still achieved as you are turning off
the clocks before suspend and back on after resume.
Thanks,
Sai