[PATCH v1 0/4] Add quirks to proceed PME handshake in DWC PM

[PATCH v1 0/4] Add quirks to proceed PME handshake in DWC PM

[Richard Zhu]

Some platforms are broken in PME_TURN_OFF broadcast and ACK handshake.
Add the quirks to support these platforms when generic DWC
suspend/resume callbacks are used.

[PATCH v1 1/4] PCI: dwc: Add quirk to fix hang issue in L2 poll of
[PATCH v1 2/4] PCI: dwc: Add quirk to fix hang issue when send out
[PATCH v1 3/4] PCI: imx6: Fix i.MX6QP PCIe hang issue in L2 poll of
[PATCH v1 4/4] PCI: imx6: Add quirks to fix i.MX7D PCIe hang in

drivers/pci/controller/dwc/pci-imx6.c             |  4 ++++
drivers/pci/controller/dwc/pcie-designware-host.c | 38 +++++++++++++++++++++++---------------
drivers/pci/controller/dwc/pcie-designware.h      |  5 +++++
3 files changed, 32 insertions(+), 15 deletions(-)

[PATCH v1 1/4] PCI: dwc: Add quirk to fix hang issue in L2 poll of suspend

[Richard Zhu]

i.MX6QP PCIe is hang in L2 poll during suspend when one endpoint device is
connected, for example the Intel e1000e network card.

Refer to Figure5-1 Link Power Management State Flow Diagram of PCI
Express Base Spec Rev6.0. L0 can be transferred to LDn directly.

It's harmless to let dw_pcie_suspend_noirq() proceed suspend after the
PME_Turn_Off is sent out, whatever the ltssm state is in L2 or L3 on
some PME_Turn_Off handshake broken platforms.

To fix this issue, add one quirk to remove the L2 entry poll and let
dw_pcie_suspend_noirq() proceed directly after PME_Turn_Off is sent out.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 .../pci/controller/dwc/pcie-designware-host.c | 22 ++++++++++---------
 drivers/pci/controller/dwc/pcie-designware.h  |  4 ++++
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index ecc33f6789e3..0817df5b8a59 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -947,7 +947,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
 {
 	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
 	u32 val;
-	int ret;
+	int ret = 0;
 
 	/*
 	 * If L1SS is supported, then do not put the link into L2 as some
@@ -964,15 +964,17 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
 			return ret;
 	}
 
-	ret = read_poll_timeout(dw_pcie_get_ltssm, val,
-				val == DW_PCIE_LTSSM_L2_IDLE ||
-				val <= DW_PCIE_LTSSM_DETECT_WAIT,
-				PCIE_PME_TO_L2_TIMEOUT_US/10,
-				PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
-	if (ret) {
-		/* Only log message when LTSSM isn't in DETECT or POLL */
-		dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n", val);
-		return ret;
+	if (!dwc_check_quirk(pci, QUIRK_NOL2POLL_IN_PM)) {
+		ret = read_poll_timeout(dw_pcie_get_ltssm, val,
+					val == DW_PCIE_LTSSM_L2_IDLE ||
+					val <= DW_PCIE_LTSSM_DETECT_WAIT,
+					PCIE_PME_TO_L2_TIMEOUT_US/10,
+					PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
+		if (ret) {
+			/* Only log message when LTSSM isn't in DETECT or POLL */
+			dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n", val);
+			return ret;
+		}
 	}
 
 	/*
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 56aafdbcdaca..05fe654d7761 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -282,6 +282,9 @@
 /* Default eDMA LLP memory size */
 #define DMA_LLP_MEM_SIZE		PAGE_SIZE
 
+#define QUIRK_NOL2POLL_IN_PM		BIT(0)
+#define dwc_check_quirk(pci, val)	(pci->quirk_flag & val)
+
 struct dw_pcie;
 struct dw_pcie_rp;
 struct dw_pcie_ep;
@@ -491,6 +494,7 @@ struct dw_pcie {
 	const struct dw_pcie_ops *ops;
 	u32			version;
 	u32			type;
+	u32			quirk_flag;
 	unsigned long		caps;
 	int			num_lanes;
 	int			max_link_speed;
-- 
2.37.1

[PATCH v1 2/4] PCI: dwc: Add quirk to fix hang issue when send out PME_Turn_Off Msg TLP

[Richard Zhu]

When no endpoint is connected on i.MX7D PCIe, chip would be freeze when do
the dummy write in dw_pcie_pme_turn_off() to issue a PME_Turn_Off Msg TLP.

Add one quirk to issue the PME_Turn_Off only when link is up to avoid
this problem on the PME_Turn_Off handshake broken platform, for example
the i.MX7D.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 .../pci/controller/dwc/pcie-designware-host.c    | 16 +++++++++++-----
 drivers/pci/controller/dwc/pcie-designware.h     |  1 +
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 0817df5b8a59..a62bf7e0ade8 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -956,12 +956,18 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
 	if (dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKCTL) & PCI_EXP_LNKCTL_ASPM_L1)
 		return 0;
 
-	if (pci->pp.ops->pme_turn_off) {
-		pci->pp.ops->pme_turn_off(&pci->pp);
+	if (dw_pcie_get_ltssm(pci) <= DW_PCIE_LTSSM_DETECT_WAIT &&
+	    dwc_check_quirk(pci, QUIRK_NOLINK_NOPME)) {
+		/* Don't send the PME_TURN_OFF when link is down. */
+		;
 	} else {
-		ret = dw_pcie_pme_turn_off(pci);
-		if (ret)
-			return ret;
+		if (pci->pp.ops->pme_turn_off) {
+			pci->pp.ops->pme_turn_off(&pci->pp);
+		} else {
+			ret = dw_pcie_pme_turn_off(pci);
+			if (ret)
+				return ret;
+		}
 	}
 
 	if (!dwc_check_quirk(pci, QUIRK_NOL2POLL_IN_PM)) {
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 05fe654d7761..d752af660e95 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -283,6 +283,7 @@
 #define DMA_LLP_MEM_SIZE		PAGE_SIZE
 
 #define QUIRK_NOL2POLL_IN_PM		BIT(0)
+#define QUIRK_NOLINK_NOPME		BIT(1)
 #define dwc_check_quirk(pci, val)	(pci->quirk_flag & val)
 
 struct dw_pcie;
-- 
2.37.1

[PATCH v1 3/4] PCI: imx6: Fix i.MX6QP PCIe hang issue in L2 poll of suspend

[Richard Zhu]

During suspend/resume, i.MX6QP PCIe is hang in L2 poll when one endpoint
device is connected, for example the Intel e1000e network card.

Refer to Figure5-1 Link Power Management State Flow Diagram of PCI
Express Base Spec Rev6.0. L0 can be transferred to LDn directly.

It's harmless to let dw_pcie_suspend_noirq() proceed suspend after the
PME_Turn_Off is sent out, whateve the ltssm state is in L2 or L3 on some
PME_Turn_Off handshake broken platforms.

To fix this issue, add one quirk to remove the L2 entry poll and let
dw_pcie_suspend_noirq() proceed directly after PME_Turn_Off is sent out.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 5f267dd261b5..aade80010cbe 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -116,6 +116,7 @@ struct imx_pcie_drvdata {
 	enum imx_pcie_variants variant;
 	enum dw_pcie_device_mode mode;
 	u32 flags;
+	u32 quirk;
 	int dbi_length;
 	const char *gpr;
 	const u32 ltssm_off;
@@ -1614,6 +1615,7 @@ static int imx_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	pci->quirk_flag = imx_pcie->drvdata->quirk;
 	pci->use_parent_dt_ranges = true;
 	if (imx_pcie->drvdata->mode == DW_PCIE_EP_TYPE) {
 		ret = imx_add_pcie_ep(imx_pcie, pdev);
@@ -1692,6 +1694,7 @@ static const struct imx_pcie_drvdata drvdata[] = {
 		.enable_ref_clk = imx6q_pcie_enable_ref_clk,
 		.core_reset = imx6qp_pcie_core_reset,
 		.ops = &imx_pcie_host_ops,
+		.quirk = QUIRK_NOL2POLL_IN_PM,
 	},
 	[IMX7D] = {
 		.variant = IMX7D,
-- 
2.37.1

[PATCH v1 4/4] PCI: imx6: Add quirks to fix i.MX7D PCIe hang in suspend

[Richard Zhu]

When no endpoint is connected on i.MX7D PCIe, chip would be freeze when do
the dummy write in dw_pcie_pme_turn_off() to issue a PME_Turn_Off Msg TLP.

And, i.MX7D has the hang problem in the L2 state poll after PME_Turn_Off
is sent out when one endpoint device is connected.

Add quirk flags to issue the PME_Turn_Off only when link is up and don't
do the L2 state poll to avoid problems on i.MX7D platform.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index aade80010cbe..779b6c7a501d 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1706,6 +1706,7 @@ static const struct imx_pcie_drvdata drvdata[] = {
 		.mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
 		.enable_ref_clk = imx7d_pcie_enable_ref_clk,
 		.core_reset = imx7d_pcie_core_reset,
+		.quirk = QUIRK_NOLINK_NOPME | QUIRK_NOL2POLL_IN_PM,
 	},
 	[IMX8MQ] = {
 		.variant = IMX8MQ,
-- 
2.37.1

Re: [PATCH v1 1/4] PCI: dwc: Add quirk to fix hang issue in L2 poll of suspend

[Bjorn Helgaas]

On Tue, Apr 08, 2025 at 02:52:18PM +0800, Richard Zhu wrote:
> i.MX6QP PCIe is hang in L2 poll during suspend when one endpoint device is
> connected, for example the Intel e1000e network card.
> 
> Refer to Figure5-1 Link Power Management State Flow Diagram of PCI
> Express Base Spec Rev6.0. L0 can be transferred to LDn directly.

Please include the section number.  Section numbers are easy to find
because they're in the spec PDF contents, but figures are not.  E.g.,
"PCIe r6.0, sec 5.2, fig 5-1"

> It's harmless to let dw_pcie_suspend_noirq() proceed suspend after the
> PME_Turn_Off is sent out, whatever the ltssm state is in L2 or L3 on
> some PME_Turn_Off handshake broken platforms.

Maybe we don't need to poll for these LTSSM states on *any* platform,
and we could just remove the poll and timeout completely?

If not, we need to explain why it is safe to skip the poll on some
platforms.  "Skipping the poll avoids a hang" is not a sufficient
explanation.

s/ltssm/LTSSM/

> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -947,7 +947,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
>  {
>  	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>  	u32 val;
> -	int ret;
> +	int ret = 0;
>  
>  	/*
>  	 * If L1SS is supported, then do not put the link into L2 as some
> @@ -964,15 +964,17 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
>  			return ret;
>  	}
>  
> -	ret = read_poll_timeout(dw_pcie_get_ltssm, val,
> -				val == DW_PCIE_LTSSM_L2_IDLE ||
> -				val <= DW_PCIE_LTSSM_DETECT_WAIT,
> -				PCIE_PME_TO_L2_TIMEOUT_US/10,
> -				PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
> -	if (ret) {
> -		/* Only log message when LTSSM isn't in DETECT or POLL */
> -		dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n", val);
> -		return ret;
> +	if (!dwc_check_quirk(pci, QUIRK_NOL2POLL_IN_PM)) {
> +		ret = read_poll_timeout(dw_pcie_get_ltssm, val,
> +					val == DW_PCIE_LTSSM_L2_IDLE ||
> +					val <= DW_PCIE_LTSSM_DETECT_WAIT,
> +					PCIE_PME_TO_L2_TIMEOUT_US/10,
> +					PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
> +		if (ret) {
> +			/* Only log message when LTSSM isn't in DETECT or POLL */
> +			dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n", val);
> +			return ret;
> +		}
>  	}
>  
>  	/*
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 56aafdbcdaca..05fe654d7761 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -282,6 +282,9 @@
>  /* Default eDMA LLP memory size */
>  #define DMA_LLP_MEM_SIZE		PAGE_SIZE
>  
> +#define QUIRK_NOL2POLL_IN_PM		BIT(0)
> +#define dwc_check_quirk(pci, val)	(pci->quirk_flag & val)

Maybe just my personal preference, but I don't like things named
"check" because that just means "look at"; it doesn't give any hint
about how to interpret the result of looking at it.

>  struct dw_pcie;
>  struct dw_pcie_rp;
>  struct dw_pcie_ep;
> @@ -491,6 +494,7 @@ struct dw_pcie {
>  	const struct dw_pcie_ops *ops;
>  	u32			version;
>  	u32			type;
> +	u32			quirk_flag;
>  	unsigned long		caps;
>  	int			num_lanes;
>  	int			max_link_speed;
> -- 
> 2.37.1
> 

RE: [PATCH v1 1/4] PCI: dwc: Add quirk to fix hang issue in L2 poll of suspend

[Hongxing Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: 2025年4月8日 22:59
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: jingoohan1@gmail.com; Frank Li <frank.li@nxp.com>;
> l.stach@pengutronix.de; lpieralisi@kernel.org; kw@linux.com;
> manivannan.sadhasivam@linaro.org; robh@kernel.org;
> bhelgaas@google.com; shawnguo@kernel.org; s.hauer@pengutronix.de;
> kernel@pengutronix.de; festevam@gmail.com; linux-pci@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; imx@lists.linux.dev;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v1 1/4] PCI: dwc: Add quirk to fix hang issue in L2 poll of
> suspend
> 
> On Tue, Apr 08, 2025 at 02:52:18PM +0800, Richard Zhu wrote:
> > i.MX6QP PCIe is hang in L2 poll during suspend when one endpoint
> > device is connected, for example the Intel e1000e network card.
> >
> > Refer to Figure5-1 Link Power Management State Flow Diagram of PCI
> > Express Base Spec Rev6.0. L0 can be transferred to LDn directly.
> 
> Please include the section number.  Section numbers are easy to find
> because they're in the spec PDF contents, but figures are not.  E.g., "PCIe
> r6.0, sec 5.2, fig 5-1"
> 
Okay, would add them later.

> > It's harmless to let dw_pcie_suspend_noirq() proceed suspend after the
> > PME_Turn_Off is sent out, whatever the ltssm state is in L2 or L3 on
> > some PME_Turn_Off handshake broken platforms.
> 
> Maybe we don't need to poll for these LTSSM states on *any* platform, and
> we could just remove the poll and timeout completely?
> 
Yes, I used to suggest remove the L2 poll and timeout in the following
 discussion.
https://lkml.org/lkml/2024/11/18/200
Hi Krishna:
Is it feasible to eliminate the L2 poll and timeout in this context?

> If not, we need to explain why it is safe to skip the poll on some platforms.
> "Skipping the poll avoids a hang" is not a sufficient explanation.
> 
> s/ltssm/LTSSM/
Okay.
> 
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -947,7 +947,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)  {
> >  	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> >  	u32 val;
> > -	int ret;
> > +	int ret = 0;
> >
> >  	/*
> >  	 * If L1SS is supported, then do not put the link into L2 as some @@
> > -964,15 +964,17 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
> >  			return ret;
> >  	}
> >
> > -	ret = read_poll_timeout(dw_pcie_get_ltssm, val,
> > -				val == DW_PCIE_LTSSM_L2_IDLE ||
> > -				val <= DW_PCIE_LTSSM_DETECT_WAIT,
> > -				PCIE_PME_TO_L2_TIMEOUT_US/10,
> > -				PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
> > -	if (ret) {
> > -		/* Only log message when LTSSM isn't in DETECT or POLL */
> > -		dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n",
> val);
> > -		return ret;
> > +	if (!dwc_check_quirk(pci, QUIRK_NOL2POLL_IN_PM)) {
> > +		ret = read_poll_timeout(dw_pcie_get_ltssm, val,
> > +					val == DW_PCIE_LTSSM_L2_IDLE ||
> > +					val <= DW_PCIE_LTSSM_DETECT_WAIT,
> > +					PCIE_PME_TO_L2_TIMEOUT_US/10,
> > +					PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
> > +		if (ret) {
> > +			/* Only log message when LTSSM isn't in DETECT or POLL */
> > +			dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM:
> 0x%x\n", val);
> > +			return ret;
> > +		}
> >  	}
> >
> >  	/*
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h
> > b/drivers/pci/controller/dwc/pcie-designware.h
> > index 56aafdbcdaca..05fe654d7761 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -282,6 +282,9 @@
> >  /* Default eDMA LLP memory size */
> >  #define DMA_LLP_MEM_SIZE		PAGE_SIZE
> >
> > +#define QUIRK_NOL2POLL_IN_PM		BIT(0)
> > +#define dwc_check_quirk(pci, val)	(pci->quirk_flag & val)
> 
> Maybe just my personal preference, but I don't like things named "check"
> because that just means "look at"; it doesn't give any hint about how to
> interpret the result of looking at it.
> 
How about dwc_match_quirk(pci, val) (pci->quirk_flag & val)?

Best Regards
Richard Zhu
> >  struct dw_pcie;
> >  struct dw_pcie_rp;
> >  struct dw_pcie_ep;
> > @@ -491,6 +494,7 @@ struct dw_pcie {
> >  	const struct dw_pcie_ops *ops;
> >  	u32			version;
> >  	u32			type;
> > +	u32			quirk_flag;
> >  	unsigned long		caps;
> >  	int			num_lanes;
> >  	int			max_link_speed;
> > --
> > 2.37.1
> >

Re: [PATCH v1 1/4] PCI: dwc: Add quirk to fix hang issue in L2 poll of suspend

[manivannan.sadhasivam]

On Wed, Apr 09, 2025 at 02:31:22AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@kernel.org>
> > Sent: 2025年4月8日 22:59
> > To: Hongxing Zhu <hongxing.zhu@nxp.com>
> > Cc: jingoohan1@gmail.com; Frank Li <frank.li@nxp.com>;
> > l.stach@pengutronix.de; lpieralisi@kernel.org; kw@linux.com;
> > manivannan.sadhasivam@linaro.org; robh@kernel.org;
> > bhelgaas@google.com; shawnguo@kernel.org; s.hauer@pengutronix.de;
> > kernel@pengutronix.de; festevam@gmail.com; linux-pci@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org; imx@lists.linux.dev;
> > linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v1 1/4] PCI: dwc: Add quirk to fix hang issue in L2 poll of
> > suspend
> > 
> > On Tue, Apr 08, 2025 at 02:52:18PM +0800, Richard Zhu wrote:
> > > i.MX6QP PCIe is hang in L2 poll during suspend when one endpoint
> > > device is connected, for example the Intel e1000e network card.
> > >
> > > Refer to Figure5-1 Link Power Management State Flow Diagram of PCI
> > > Express Base Spec Rev6.0. L0 can be transferred to LDn directly.
> > 
> > Please include the section number.  Section numbers are easy to find
> > because they're in the spec PDF contents, but figures are not.  E.g., "PCIe
> > r6.0, sec 5.2, fig 5-1"
> > 
> Okay, would add them later.
> 
> > > It's harmless to let dw_pcie_suspend_noirq() proceed suspend after the
> > > PME_Turn_Off is sent out, whatever the ltssm state is in L2 or L3 on
> > > some PME_Turn_Off handshake broken platforms.
> > 
> > Maybe we don't need to poll for these LTSSM states on *any* platform, and
> > we could just remove the poll and timeout completely?
> > 
> Yes, I used to suggest remove the L2 poll and timeout in the following
>  discussion.
> https://lkml.org/lkml/2024/11/18/200
> Hi Krishna:
> Is it feasible to eliminate the L2 poll and timeout in this context?
> 

Spec r6.0, sec 5.2 mandates L2/L3 Ready state:

"L2/L3 Ready transition protocol support is required."

Also in many places, it suggests waiting for L2/L3 Ready state before powering
down the device. So I don't think we should just remove the poll for all
platforms.

> > If not, we need to explain why it is safe to skip the poll on some platforms.
> > "Skipping the poll avoids a hang" is not a sufficient explanation.
> > 

So the issue is that the device doesn't transition to L2/L3 Ready state and the
host platform just 'hangs'? Do we know why the hang happens?

> > s/ltssm/LTSSM/
> Okay.
> > 
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -947,7 +947,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)  {
> > >  	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > >  	u32 val;
> > > -	int ret;
> > > +	int ret = 0;
> > >
> > >  	/*
> > >  	 * If L1SS is supported, then do not put the link into L2 as some @@
> > > -964,15 +964,17 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
> > >  			return ret;
> > >  	}
> > >
> > > -	ret = read_poll_timeout(dw_pcie_get_ltssm, val,
> > > -				val == DW_PCIE_LTSSM_L2_IDLE ||
> > > -				val <= DW_PCIE_LTSSM_DETECT_WAIT,
> > > -				PCIE_PME_TO_L2_TIMEOUT_US/10,
> > > -				PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
> > > -	if (ret) {
> > > -		/* Only log message when LTSSM isn't in DETECT or POLL */
> > > -		dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n",
> > val);
> > > -		return ret;
> > > +	if (!dwc_check_quirk(pci, QUIRK_NOL2POLL_IN_PM)) {
> > > +		ret = read_poll_timeout(dw_pcie_get_ltssm, val,
> > > +					val == DW_PCIE_LTSSM_L2_IDLE ||
> > > +					val <= DW_PCIE_LTSSM_DETECT_WAIT,
> > > +					PCIE_PME_TO_L2_TIMEOUT_US/10,
> > > +					PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
> > > +		if (ret) {
> > > +			/* Only log message when LTSSM isn't in DETECT or POLL */
> > > +			dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM:
> > 0x%x\n", val);
> > > +			return ret;
> > > +		}
> > >  	}
> > >
> > >  	/*
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h
> > > b/drivers/pci/controller/dwc/pcie-designware.h
> > > index 56aafdbcdaca..05fe654d7761 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > @@ -282,6 +282,9 @@
> > >  /* Default eDMA LLP memory size */
> > >  #define DMA_LLP_MEM_SIZE		PAGE_SIZE
> > >
> > > +#define QUIRK_NOL2POLL_IN_PM		BIT(0)
> > > +#define dwc_check_quirk(pci, val)	(pci->quirk_flag & val)
> > 
> > Maybe just my personal preference, but I don't like things named "check"
> > because that just means "look at"; it doesn't give any hint about how to
> > interpret the result of looking at it.
> > 
> How about dwc_match_quirk(pci, val) (pci->quirk_flag & val)?
> 

Maybe just dwc_quirk()?

- Mani

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

Re: [PATCH v1 2/4] PCI: dwc: Add quirk to fix hang issue when send out PME_Turn_Off Msg TLP

[Manivannan Sadhasivam]

On Tue, Apr 08, 2025 at 02:52:19PM +0800, Richard Zhu wrote:
> When no endpoint is connected on i.MX7D PCIe, chip would be freeze when do
> the dummy write in dw_pcie_pme_turn_off() to issue a PME_Turn_Off Msg TLP.
> 
> Add one quirk to issue the PME_Turn_Off only when link is up to avoid
> this problem on the PME_Turn_Off handshake broken platform, for example
> the i.MX7D.
> 

Why do we need to send PME_Turn_Off if there is no endpoint connected? We should
skip PME_Turn_Off and L2/L3 Ready transitions in that case.

- Mani

> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  .../pci/controller/dwc/pcie-designware-host.c    | 16 +++++++++++-----
>  drivers/pci/controller/dwc/pcie-designware.h     |  1 +
>  2 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 0817df5b8a59..a62bf7e0ade8 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -956,12 +956,18 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
>  	if (dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKCTL) & PCI_EXP_LNKCTL_ASPM_L1)
>  		return 0;
>  
> -	if (pci->pp.ops->pme_turn_off) {
> -		pci->pp.ops->pme_turn_off(&pci->pp);
> +	if (dw_pcie_get_ltssm(pci) <= DW_PCIE_LTSSM_DETECT_WAIT &&
> +	    dwc_check_quirk(pci, QUIRK_NOLINK_NOPME)) {
> +		/* Don't send the PME_TURN_OFF when link is down. */
> +		;
>  	} else {
> -		ret = dw_pcie_pme_turn_off(pci);
> -		if (ret)
> -			return ret;
> +		if (pci->pp.ops->pme_turn_off) {
> +			pci->pp.ops->pme_turn_off(&pci->pp);
> +		} else {
> +			ret = dw_pcie_pme_turn_off(pci);
> +			if (ret)
> +				return ret;
> +		}
>  	}
>  
>  	if (!dwc_check_quirk(pci, QUIRK_NOL2POLL_IN_PM)) {
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 05fe654d7761..d752af660e95 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -283,6 +283,7 @@
>  #define DMA_LLP_MEM_SIZE		PAGE_SIZE
>  
>  #define QUIRK_NOL2POLL_IN_PM		BIT(0)
> +#define QUIRK_NOLINK_NOPME		BIT(1)
>  #define dwc_check_quirk(pci, val)	(pci->quirk_flag & val)
>  
>  struct dw_pcie;
> -- 
> 2.37.1
> 

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

RE: [PATCH v1 1/4] PCI: dwc: Add quirk to fix hang issue in L2 poll of suspend

[Hongxing Zhu]

> -----Original Message-----
> From: manivannan.sadhasivam@linaro.org
> <manivannan.sadhasivam@linaro.org>
> Sent: 2025年6月1日 1:47
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: Bjorn Helgaas <helgaas@kernel.org>; Krishna Chaitanya Chundru
> <quic_krichai@quicinc.com>; jingoohan1@gmail.com; Frank Li
> <frank.li@nxp.com>; l.stach@pengutronix.de; lpieralisi@kernel.org;
> kw@linux.com; robh@kernel.org; bhelgaas@google.com;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; linux-pci@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; imx@lists.linux.dev;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v1 1/4] PCI: dwc: Add quirk to fix hang issue in L2 poll of
> suspend
> 
> On Wed, Apr 09, 2025 at 02:31:22AM +0000, Hongxing Zhu wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas <helgaas@kernel.org>
> > > Sent: 2025年4月8日 22:59
> > > To: Hongxing Zhu <hongxing.zhu@nxp.com>
> > > Cc: jingoohan1@gmail.com; Frank Li <frank.li@nxp.com>;
> > > l.stach@pengutronix.de; lpieralisi@kernel.org; kw@linux.com;
> > > manivannan.sadhasivam@linaro.org; robh@kernel.org;
> > > bhelgaas@google.com; shawnguo@kernel.org; s.hauer@pengutronix.de;
> > > kernel@pengutronix.de; festevam@gmail.com;
> > > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > > imx@lists.linux.dev; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH v1 1/4] PCI: dwc: Add quirk to fix hang issue in
> > > L2 poll of suspend
> > >
> > > On Tue, Apr 08, 2025 at 02:52:18PM +0800, Richard Zhu wrote:
> > > > i.MX6QP PCIe is hang in L2 poll during suspend when one endpoint
> > > > device is connected, for example the Intel e1000e network card.
> > > >
> > > > Refer to Figure5-1 Link Power Management State Flow Diagram of PCI
> > > > Express Base Spec Rev6.0. L0 can be transferred to LDn directly.
> > >
> > > Please include the section number.  Section numbers are easy to find
> > > because they're in the spec PDF contents, but figures are not.
> > > E.g., "PCIe r6.0, sec 5.2, fig 5-1"
> > >
> > Okay, would add them later.
> >
> > > > It's harmless to let dw_pcie_suspend_noirq() proceed suspend after
> > > > the PME_Turn_Off is sent out, whatever the ltssm state is in L2 or
> > > > L3 on some PME_Turn_Off handshake broken platforms.
> > >
> > > Maybe we don't need to poll for these LTSSM states on *any*
> > > platform, and we could just remove the poll and timeout completely?
> > >
> > Yes, I used to suggest remove the L2 poll and timeout in the following
> > discussion.
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml
> > .org%2Flkml%2F2024%2F11%2F18%2F200&data=05%7C02%7Chongxing.zhu
> %40nxp.c
> >
> om%7C59dcbf2632b340e748bb08dda06b34e1%7C686ea1d3bc2b4c6fa92cd99c
> 5c3016
> >
> 35%7C0%7C0%7C638843104499682928%7CUnknown%7CTWFpbGZsb3d8eyJFb
> XB0eU1hcG
> >
> kiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoy
> >
> fQ%3D%3D%7C0%7C%7C%7C&sdata=eEXSd14o6XYHC0ww06NaFH2L7Pr%2FgT
> P2Dxr77xd5
> > Zp0%3D&reserved=0
> > Hi Krishna:
> > Is it feasible to eliminate the L2 poll and timeout in this context?
> >
> 
> Spec r6.0, sec 5.2 mandates L2/L3 Ready state:
> 
> "L2/L3 Ready transition protocol support is required."
> 
> Also in many places, it suggests waiting for L2/L3 Ready state before powering
> down the device. So I don't think we should just remove the poll for all
> platforms.
> 
Hi Mani:
Thanks for your review. 
For some L2 POLL broken platforms (e.x i.MX6QP PCIe). Can we just wait
 for a enough long time here without the L2/L3 state polling?
Because that a recommended 10ms max wait refer to PCIe r6.0, sec 5.3.3.2.1
 PME Synchronization.

> > > If not, we need to explain why it is safe to skip the poll on some platforms.
> > > "Skipping the poll avoids a hang" is not a sufficient explanation.
> > >
> 
> So the issue is that the device doesn't transition to L2/L3 Ready state and the
> host platform just 'hangs'? Do we know why the hang happens?
> 
I can’t find the exact root cause of hand after I looked through the
 trace log captured by the protocol analyzer. Without the L2 poll by SW,
 the PM_Enter_L23 DLLP(R<-) and PM_Request_Ack  DLLP (R->) can be
 captured after PME_Turn_Off is kicked off, and PME_TO_Ack is received.

Since the designer of the i.MX6Q PCIe can't be contacted anymore. I don't
konw what's going on when do the L2 POLL. I suspect that the LTSSM_STATE 
registers are not accessible anymore after the PM_Enter_L23/PM_Request_Ack
 handshake on i.MX6Q PCIe.

Best Regards
Richard Zhu
> > > s/ltssm/LTSSM/
> > Okay.
> > >
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > @@ -947,7 +947,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
> {
> > > >  	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > > >  	u32 val;
> > > > -	int ret;
> > > > +	int ret = 0;
> > > >
> > > >  	/*
> > > >  	 * If L1SS is supported, then do not put the link into L2 as
> > > > some @@
> > > > -964,15 +964,17 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
> > > >  			return ret;
> > > >  	}
> > > >
> > > > -	ret = read_poll_timeout(dw_pcie_get_ltssm, val,
> > > > -				val == DW_PCIE_LTSSM_L2_IDLE ||
> > > > -				val <= DW_PCIE_LTSSM_DETECT_WAIT,
> > > > -				PCIE_PME_TO_L2_TIMEOUT_US/10,
> > > > -				PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
> > > > -	if (ret) {
> > > > -		/* Only log message when LTSSM isn't in DETECT or POLL */
> > > > -		dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM:
> 0x%x\n",
> > > val);
> > > > -		return ret;
> > > > +	if (!dwc_check_quirk(pci, QUIRK_NOL2POLL_IN_PM)) {
> > > > +		ret = read_poll_timeout(dw_pcie_get_ltssm, val,
> > > > +					val == DW_PCIE_LTSSM_L2_IDLE ||
> > > > +					val <= DW_PCIE_LTSSM_DETECT_WAIT,
> > > > +					PCIE_PME_TO_L2_TIMEOUT_US/10,
> > > > +					PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
> > > > +		if (ret) {
> > > > +			/* Only log message when LTSSM isn't in DETECT or POLL */
> > > > +			dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM:
> > > 0x%x\n", val);
> > > > +			return ret;
> > > > +		}
> > > >  	}
> > > >
> > > >  	/*
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h
> > > > b/drivers/pci/controller/dwc/pcie-designware.h
> > > > index 56aafdbcdaca..05fe654d7761 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > > @@ -282,6 +282,9 @@
> > > >  /* Default eDMA LLP memory size */
> > > >  #define DMA_LLP_MEM_SIZE		PAGE_SIZE
> > > >
> > > > +#define QUIRK_NOL2POLL_IN_PM		BIT(0)
> > > > +#define dwc_check_quirk(pci, val)	(pci->quirk_flag & val)
> > >
> > > Maybe just my personal preference, but I don't like things named "check"
> > > because that just means "look at"; it doesn't give any hint about
> > > how to interpret the result of looking at it.
> > >
> > How about dwc_match_quirk(pci, val) (pci->quirk_flag & val)?
> >
> 
> Maybe just dwc_quirk()?
> 
> - Mani
> 
> --
> மணிவண்ணன் சதாசிவம்