* [PATCH v4 0/2] Bug fixes when dwc generic suspend/resume callbacks are used
@ 2024-12-10 8:15 Richard Zhu
2024-12-10 8:15 ` [PATCH v4 1/2] PCI: dwc: Always stop link in the dw_pcie_suspend_noirq Richard Zhu
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Richard Zhu @ 2024-12-10 8:15 UTC (permalink / raw)
To: dlemoal, jingoohan1, bhelgaas, lpieralisi, kw,
manivannan.sadhasivam, robh, frank.li, quic_krichai
Cc: imx, kernel, linux-pci, linux-kernel
Some bug fixes when DWC generic suspend/resume callbacks are used.
Drop the first patch of v3 patch-set, since the use case of this patch had been
covered by #3 commit "PCI: dwc: Clean up some unnecessary codes in dw_pcie_suspend_noirq()".
To be simple, re-format the codes, drop the first patch of v3 patch-set, and
only keep last two patches of v3 in v4 or later
Here is the discussion [1] and final solution [2] of the codes clean up commit.
[1] https://patchwork.kernel.org/project/linux-pci/patch/1721628913-1449-1-git-send-email-hongxing.zhu@nxp.com/
[2] https://patchwork.kernel.org/project/linux-pci/patch/20241126073909.4058733-1-hongxing.zhu@nxp.com/
v4 changes:
Drop the first patch("PCI: dwc: Fix resume failure if no EP is connected on some platforms")
in v3, since it's use-case had been covered by #3 patch of v3.
Add one Fixes tag into "PCI: dwc: Clean up some unnecessary codes in dw_pcie_suspend_noirq()".
Refer to Damien's comments, let ret test go inside the "else" and remove the
initialization of ret to 0 declaration. Thanks.
v3 changes:
Regarding the discussion listed above[2].
Resend the patch-set after adding one more codes clean up patch together.
v2 changes:
Thanks for Manivannan's review.
- Refine the subject of second patch and add
"Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>"
- In first patch, update the commit message and move one comment into
proper place.
BTW, Manivannan found another potential issue that suspend is entry but
the link is in L1SS stat in v1 review. This is a new story. And it's better
to be verified and fixed by another commit later.
v2: https://patchwork.kernel.org/project/linux-pci/cover/1728539269-1861-1-git-send-email-hongxing.zhu@nxp.com/
[PATCH v4 1/2] PCI: dwc: Always stop link in the
[PATCH v4 2/2] PCI: dwc: Clean up some unnecessary codes in
drivers/pci/controller/dwc/pcie-designware-host.c | 28 ++++++++++++++++++----------
drivers/pci/controller/dwc/pcie-designware.h | 1 +
2 files changed, 19 insertions(+), 10 deletions(-)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v4 1/2] PCI: dwc: Always stop link in the dw_pcie_suspend_noirq
2024-12-10 8:15 [PATCH v4 0/2] Bug fixes when dwc generic suspend/resume callbacks are used Richard Zhu
@ 2024-12-10 8:15 ` Richard Zhu
2025-01-14 18:15 ` Bjorn Helgaas
2024-12-10 8:15 ` [PATCH v4 2/2] PCI: dwc: Clean up some unnecessary codes in dw_pcie_suspend_noirq() Richard Zhu
2025-01-15 11:33 ` [PATCH v4 0/2] Bug fixes when dwc generic suspend/resume callbacks are used Krzysztof Wilczyński
2 siblings, 1 reply; 9+ messages in thread
From: Richard Zhu @ 2024-12-10 8:15 UTC (permalink / raw)
To: dlemoal, jingoohan1, bhelgaas, lpieralisi, kw,
manivannan.sadhasivam, robh, frank.li, quic_krichai
Cc: imx, kernel, linux-pci, linux-kernel, Richard Zhu
On i.MX8QM, PCIe link can't be re-established again in
dw_pcie_resume_noirq(), if the LTSSM_EN bit is not cleared properly in
dw_pcie_suspend_noirq().
Add dw_pcie_stop_link() into dw_pcie_suspend_noirq() to fix this issue and
keep symmetric in suspend/resume function since there is
dw_pcie_start_link() in dw_pcie_resume_noirq().
Fixes: 4774faf854f5 ("PCI: dwc: Implement generic suspend/resume functionality")
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
drivers/pci/controller/dwc/pcie-designware-host.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index f882b11fd7b94..f56cb7b9e6f99 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -1001,6 +1001,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
return ret;
}
+ dw_pcie_stop_link(pci);
if (pci->pp.ops->deinit)
pci->pp.ops->deinit(&pci->pp);
--
2.37.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v4 2/2] PCI: dwc: Clean up some unnecessary codes in dw_pcie_suspend_noirq()
2024-12-10 8:15 [PATCH v4 0/2] Bug fixes when dwc generic suspend/resume callbacks are used Richard Zhu
2024-12-10 8:15 ` [PATCH v4 1/2] PCI: dwc: Always stop link in the dw_pcie_suspend_noirq Richard Zhu
@ 2024-12-10 8:15 ` Richard Zhu
2025-01-15 11:33 ` [PATCH v4 0/2] Bug fixes when dwc generic suspend/resume callbacks are used Krzysztof Wilczyński
2 siblings, 0 replies; 9+ messages in thread
From: Richard Zhu @ 2024-12-10 8:15 UTC (permalink / raw)
To: dlemoal, jingoohan1, bhelgaas, lpieralisi, kw,
manivannan.sadhasivam, robh, frank.li, quic_krichai
Cc: imx, kernel, linux-pci, linux-kernel, Richard Zhu, Frank Li
Before sending PME_TURN_OFF, don't test the LTSSM state. Since it's safe
to send PME_TURN_OFF message regardless of whether the link is up or
down. So, there would be no need to test the LTSSM state before sending
PME_TURN_OFF message.
Only print the message when ltssm_stat is not in DETECT and POLL.
In the other words, there isn't an error message when no endpoint is
connected at all.
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
.../pci/controller/dwc/pcie-designware-host.c | 27 ++++++++++++-------
drivers/pci/controller/dwc/pcie-designware.h | 1 +
2 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index f56cb7b9e6f99..bdb9c1ad6cdd5 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -973,7 +973,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 = 0;
+ int ret;
/*
* If L1SS is supported, then do not put the link into L2 as some
@@ -982,25 +982,32 @@ 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 (dw_pcie_get_ltssm(pci) <= DW_PCIE_LTSSM_DETECT_ACT)
- return 0;
-
- if (pci->pp.ops->pme_turn_off)
+ if (pci->pp.ops->pme_turn_off) {
pci->pp.ops->pme_turn_off(&pci->pp);
- else
+ } else {
ret = dw_pcie_pme_turn_off(pci);
+ if (ret)
+ return ret;
+ }
- if (ret)
- return ret;
-
- ret = read_poll_timeout(dw_pcie_get_ltssm, val, val == DW_PCIE_LTSSM_L2_IDLE,
+ 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 dump message when ltssm_stat isn't in DETECT and POLL */
dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n", val);
return ret;
}
+ /*
+ * Refer to r6.0, sec 5.3.3.2.1, software should wait at least
+ * 100ns after L2/L3 Ready before turning off refclock and
+ * main power. It's harmless too when no endpoint connected.
+ */
+ udelay(1);
+
dw_pcie_stop_link(pci);
if (pci->pp.ops->deinit)
pci->pp.ops->deinit(&pci->pp);
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 5c14ed2cb91ed..7efcb4af66da3 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -330,6 +330,7 @@ enum dw_pcie_ltssm {
/* Need to align with PCIE_PORT_DEBUG0 bits 0:5 */
DW_PCIE_LTSSM_DETECT_QUIET = 0x0,
DW_PCIE_LTSSM_DETECT_ACT = 0x1,
+ DW_PCIE_LTSSM_DETECT_WAIT = 0x6,
DW_PCIE_LTSSM_L0 = 0x11,
DW_PCIE_LTSSM_L2_IDLE = 0x15,
--
2.37.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] PCI: dwc: Always stop link in the dw_pcie_suspend_noirq
2024-12-10 8:15 ` [PATCH v4 1/2] PCI: dwc: Always stop link in the dw_pcie_suspend_noirq Richard Zhu
@ 2025-01-14 18:15 ` Bjorn Helgaas
2025-01-14 19:32 ` Frank Li
0 siblings, 1 reply; 9+ messages in thread
From: Bjorn Helgaas @ 2025-01-14 18:15 UTC (permalink / raw)
To: Richard Zhu
Cc: dlemoal, jingoohan1, bhelgaas, lpieralisi, kw,
manivannan.sadhasivam, robh, frank.li, quic_krichai, imx, kernel,
linux-pci, linux-kernel
On Tue, Dec 10, 2024 at 04:15:56PM +0800, Richard Zhu wrote:
> On i.MX8QM, PCIe link can't be re-established again in
> dw_pcie_resume_noirq(), if the LTSSM_EN bit is not cleared properly in
> dw_pcie_suspend_noirq().
>
> Add dw_pcie_stop_link() into dw_pcie_suspend_noirq() to fix this issue and
> keep symmetric in suspend/resume function since there is
> dw_pcie_start_link() in dw_pcie_resume_noirq().
>
> Fixes: 4774faf854f5 ("PCI: dwc: Implement generic suspend/resume functionality")
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
> drivers/pci/controller/dwc/pcie-designware-host.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index f882b11fd7b94..f56cb7b9e6f99 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -1001,6 +1001,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
> return ret;
> }
>
> + dw_pcie_stop_link(pci);
We should try to avoid changes to the generic DWC path just to
accommodate one controller. Since other DWC-based controllers
apparently don't need dw_pcie_stop_link() here, this seems like it
might be the wrong place for this change.
If doing dw_pcie_stop_link() here is really helpful for all DWC
controllers, this would be fine, but the commit log should then explain
why it helps everybody, not why one particular controller benefits.
If it's only needed for i.MX8QM, we do already have a
controller-specific hook (.deinit()) just below; maybe that could be
used?
> if (pci->pp.ops->deinit)
> pci->pp.ops->deinit(&pci->pp);
>
> --
> 2.37.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] PCI: dwc: Always stop link in the dw_pcie_suspend_noirq
2025-01-14 18:15 ` Bjorn Helgaas
@ 2025-01-14 19:32 ` Frank Li
2025-01-14 20:26 ` Bjorn Helgaas
0 siblings, 1 reply; 9+ messages in thread
From: Frank Li @ 2025-01-14 19:32 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Richard Zhu, dlemoal, jingoohan1, bhelgaas, lpieralisi, kw,
manivannan.sadhasivam, robh, quic_krichai, imx, kernel, linux-pci,
linux-kernel
On Tue, Jan 14, 2025 at 12:15:18PM -0600, Bjorn Helgaas wrote:
> On Tue, Dec 10, 2024 at 04:15:56PM +0800, Richard Zhu wrote:
> > On i.MX8QM, PCIe link can't be re-established again in
> > dw_pcie_resume_noirq(), if the LTSSM_EN bit is not cleared properly in
> > dw_pcie_suspend_noirq().
> >
> > Add dw_pcie_stop_link() into dw_pcie_suspend_noirq() to fix this issue and
> > keep symmetric in suspend/resume function since there is
> > dw_pcie_start_link() in dw_pcie_resume_noirq().
> >
> > Fixes: 4774faf854f5 ("PCI: dwc: Implement generic suspend/resume functionality")
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> > drivers/pci/controller/dwc/pcie-designware-host.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index f882b11fd7b94..f56cb7b9e6f99 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -1001,6 +1001,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
> > return ret;
> > }
> >
> > + dw_pcie_stop_link(pci);
>
> We should try to avoid changes to the generic DWC path just to
> accommodate one controller. Since other DWC-based controllers
> apparently don't need dw_pcie_stop_link() here, this seems like it
> might be the wrong place for this change.
>
> If doing dw_pcie_stop_link() here is really helpful for all DWC
> controllers, this would be fine, but the commit log should then explain
> why it helps everybody, not why one particular controller benefits.
It should be for all dwc controllers although find such problem at i.MX8QM
platfrom. It should keep symmetric between suspend/resume function.
So far only layerscape and i.MX platform use these common functions. Other
dwc platform still have not switched to this common function yet.
Frank
>
> If it's only needed for i.MX8QM, we do already have a
> controller-specific hook (.deinit()) just below; maybe that could be
> used?
>
> > if (pci->pp.ops->deinit)
> > pci->pp.ops->deinit(&pci->pp);
> >
> > --
> > 2.37.1
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] PCI: dwc: Always stop link in the dw_pcie_suspend_noirq
2025-01-14 19:32 ` Frank Li
@ 2025-01-14 20:26 ` Bjorn Helgaas
2025-01-14 20:43 ` Bjorn Helgaas
2025-01-14 20:51 ` Frank Li
0 siblings, 2 replies; 9+ messages in thread
From: Bjorn Helgaas @ 2025-01-14 20:26 UTC (permalink / raw)
To: Frank Li
Cc: Richard Zhu, dlemoal, jingoohan1, bhelgaas, lpieralisi, kw,
manivannan.sadhasivam, robh, quic_krichai, imx, kernel, linux-pci,
linux-kernel
On Tue, Jan 14, 2025 at 02:32:38PM -0500, Frank Li wrote:
> On Tue, Jan 14, 2025 at 12:15:18PM -0600, Bjorn Helgaas wrote:
> > On Tue, Dec 10, 2024 at 04:15:56PM +0800, Richard Zhu wrote:
> > > On i.MX8QM, PCIe link can't be re-established again in
> > > dw_pcie_resume_noirq(), if the LTSSM_EN bit is not cleared properly in
> > > dw_pcie_suspend_noirq().
> > >
> > > Add dw_pcie_stop_link() into dw_pcie_suspend_noirq() to fix this issue and
> > > keep symmetric in suspend/resume function since there is
> > > dw_pcie_start_link() in dw_pcie_resume_noirq().
> > >
> > > Fixes: 4774faf854f5 ("PCI: dwc: Implement generic suspend/resume functionality")
> > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > > drivers/pci/controller/dwc/pcie-designware-host.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > index f882b11fd7b94..f56cb7b9e6f99 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -1001,6 +1001,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
> > > return ret;
> > > }
> > >
> > > + dw_pcie_stop_link(pci);
> >
> > We should try to avoid changes to the generic DWC path just to
> > accommodate one controller. Since other DWC-based controllers
> > apparently don't need dw_pcie_stop_link() here, this seems like it
> > might be the wrong place for this change.
> >
> > If doing dw_pcie_stop_link() here is really helpful for all DWC
> > controllers, this would be fine, but the commit log should then explain
> > why it helps everybody, not why one particular controller benefits.
>
> It should be for all dwc controllers although find such problem at i.MX8QM
> platfrom. It should keep symmetric between suspend/resume function.
>
> So far only layerscape and i.MX platform use these common functions. Other
> dwc platform still have not switched to this common function yet.
I see that layerscape uses dw_pcie_suspend_noirq():
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pci-layerscape.c?id=v6.13-rc7#n379
But I don't see where imx6 does:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pci-imx6.c?id=v6.13-rc7#n1236
We don't currently have anything queued that touches pci-imx6.c; am I
missing a patch that converts pci-imx6.c to use
dw_pcie_suspend_noirq()?
This doesn't feel urgent yet since the commit log talks about i.MX8QM,
but I can't make a connection between i.MX8QM and this patch.
Bjorn
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] PCI: dwc: Always stop link in the dw_pcie_suspend_noirq
2025-01-14 20:26 ` Bjorn Helgaas
@ 2025-01-14 20:43 ` Bjorn Helgaas
2025-01-14 20:51 ` Frank Li
1 sibling, 0 replies; 9+ messages in thread
From: Bjorn Helgaas @ 2025-01-14 20:43 UTC (permalink / raw)
To: Frank Li
Cc: Richard Zhu, dlemoal, jingoohan1, bhelgaas, lpieralisi, kw,
manivannan.sadhasivam, robh, quic_krichai, imx, kernel, linux-pci,
linux-kernel
On Tue, Jan 14, 2025 at 02:26:25PM -0600, Bjorn Helgaas wrote:
> On Tue, Jan 14, 2025 at 02:32:38PM -0500, Frank Li wrote:
> > On Tue, Jan 14, 2025 at 12:15:18PM -0600, Bjorn Helgaas wrote:
> > > On Tue, Dec 10, 2024 at 04:15:56PM +0800, Richard Zhu wrote:
> > > > On i.MX8QM, PCIe link can't be re-established again in
> > > > dw_pcie_resume_noirq(), if the LTSSM_EN bit is not cleared properly in
> > > > dw_pcie_suspend_noirq().
> > > >
> > > > Add dw_pcie_stop_link() into dw_pcie_suspend_noirq() to fix this issue and
> > > > keep symmetric in suspend/resume function since there is
> > > > dw_pcie_start_link() in dw_pcie_resume_noirq().
> > > >
> > > > Fixes: 4774faf854f5 ("PCI: dwc: Implement generic suspend/resume functionality")
> > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > ---
> > > > drivers/pci/controller/dwc/pcie-designware-host.c | 1 +
> > > > 1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > index f882b11fd7b94..f56cb7b9e6f99 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > @@ -1001,6 +1001,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
> > > > return ret;
> > > > }
> > > >
> > > > + dw_pcie_stop_link(pci);
> > >
> > > We should try to avoid changes to the generic DWC path just to
> > > accommodate one controller. Since other DWC-based controllers
> > > apparently don't need dw_pcie_stop_link() here, this seems like it
> > > might be the wrong place for this change.
> > >
> > > If doing dw_pcie_stop_link() here is really helpful for all DWC
> > > controllers, this would be fine, but the commit log should then explain
> > > why it helps everybody, not why one particular controller benefits.
> >
> > It should be for all dwc controllers although find such problem at i.MX8QM
> > platfrom. It should keep symmetric between suspend/resume function.
> >
> > So far only layerscape and i.MX platform use these common functions. Other
> > dwc platform still have not switched to this common function yet.
>
> I see that layerscape uses dw_pcie_suspend_noirq():
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pci-layerscape.c?id=v6.13-rc7#n379
>
> But I don't see where imx6 does:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pci-imx6.c?id=v6.13-rc7#n1236
>
> We don't currently have anything queued that touches pci-imx6.c; am I
> missing a patch that converts pci-imx6.c to use
> dw_pcie_suspend_noirq()?
I guess it's this series:
https://lore.kernel.org/all/20241126075702.4099164-1-hongxing.zhu@nxp.com/
where "[PATCH v7 08/10] PCI: imx6: Use dwc common suspend resume
method" does this conversion.
I see some review of v6
(https://lore.kernel.org/r/20241101070610.1267391-1-hongxing.zhu@nxp.com),
but no comments for v7, although Mani has already reviewed six of the
ten.
> This doesn't feel urgent yet since the commit log talks about i.MX8QM,
> but I can't make a connection between i.MX8QM and this patch.
>
> Bjorn
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] PCI: dwc: Always stop link in the dw_pcie_suspend_noirq
2025-01-14 20:26 ` Bjorn Helgaas
2025-01-14 20:43 ` Bjorn Helgaas
@ 2025-01-14 20:51 ` Frank Li
1 sibling, 0 replies; 9+ messages in thread
From: Frank Li @ 2025-01-14 20:51 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Richard Zhu, dlemoal, jingoohan1, bhelgaas, lpieralisi, kw,
manivannan.sadhasivam, robh, quic_krichai, imx, kernel, linux-pci,
linux-kernel
On Tue, Jan 14, 2025 at 02:26:22PM -0600, Bjorn Helgaas wrote:
> On Tue, Jan 14, 2025 at 02:32:38PM -0500, Frank Li wrote:
> > On Tue, Jan 14, 2025 at 12:15:18PM -0600, Bjorn Helgaas wrote:
> > > On Tue, Dec 10, 2024 at 04:15:56PM +0800, Richard Zhu wrote:
> > > > On i.MX8QM, PCIe link can't be re-established again in
> > > > dw_pcie_resume_noirq(), if the LTSSM_EN bit is not cleared properly in
> > > > dw_pcie_suspend_noirq().
> > > >
> > > > Add dw_pcie_stop_link() into dw_pcie_suspend_noirq() to fix this issue and
> > > > keep symmetric in suspend/resume function since there is
> > > > dw_pcie_start_link() in dw_pcie_resume_noirq().
> > > >
> > > > Fixes: 4774faf854f5 ("PCI: dwc: Implement generic suspend/resume functionality")
> > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > ---
> > > > drivers/pci/controller/dwc/pcie-designware-host.c | 1 +
> > > > 1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > index f882b11fd7b94..f56cb7b9e6f99 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > @@ -1001,6 +1001,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
> > > > return ret;
> > > > }
> > > >
> > > > + dw_pcie_stop_link(pci);
> > >
> > > We should try to avoid changes to the generic DWC path just to
> > > accommodate one controller. Since other DWC-based controllers
> > > apparently don't need dw_pcie_stop_link() here, this seems like it
> > > might be the wrong place for this change.
> > >
> > > If doing dw_pcie_stop_link() here is really helpful for all DWC
> > > controllers, this would be fine, but the commit log should then explain
> > > why it helps everybody, not why one particular controller benefits.
> >
> > It should be for all dwc controllers although find such problem at i.MX8QM
> > platfrom. It should keep symmetric between suspend/resume function.
> >
> > So far only layerscape and i.MX platform use these common functions. Other
> > dwc platform still have not switched to this common function yet.
>
> I see that layerscape uses dw_pcie_suspend_noirq():
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pci-layerscape.c?id=v6.13-rc7#n379
>
> But I don't see where imx6 does:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pci-imx6.c?id=v6.13-rc7#n1236
>
> We don't currently have anything queued that touches pci-imx6.c; am I
> missing a patch that converts pci-imx6.c to use
> dw_pcie_suspend_noirq()?
Oh! It is here,
https://lore.kernel.org/imx/20241126075702.4099164-9-hongxing.zhu@nxp.com/
I am not sure why still not merged yet. (Nov 26).
Can you help check "elimiated pci_fixup_addr() callback patches". I already
ping serial times. no one reply me. I think it is more important, so other
person can start clean up cpu_fixup_addr().
https://lore.kernel.org/imx/Z1i9uEGvsVACsF2r@lizhi-Precision-Tower-5810/
Frank
>
> This doesn't feel urgent yet since the commit log talks about i.MX8QM,
> but I can't make a connection between i.MX8QM and this patch.
>
> Bjorn
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 0/2] Bug fixes when dwc generic suspend/resume callbacks are used
2024-12-10 8:15 [PATCH v4 0/2] Bug fixes when dwc generic suspend/resume callbacks are used Richard Zhu
2024-12-10 8:15 ` [PATCH v4 1/2] PCI: dwc: Always stop link in the dw_pcie_suspend_noirq Richard Zhu
2024-12-10 8:15 ` [PATCH v4 2/2] PCI: dwc: Clean up some unnecessary codes in dw_pcie_suspend_noirq() Richard Zhu
@ 2025-01-15 11:33 ` Krzysztof Wilczyński
2 siblings, 0 replies; 9+ messages in thread
From: Krzysztof Wilczyński @ 2025-01-15 11:33 UTC (permalink / raw)
To: Richard Zhu
Cc: dlemoal, jingoohan1, bhelgaas, lpieralisi, manivannan.sadhasivam,
robh, frank.li, quic_krichai, imx, kernel, linux-pci,
linux-kernel
Hello,
> Some bug fixes when DWC generic suspend/resume callbacks are used.
> Drop the first patch of v3 patch-set, since the use case of this patch had been
> covered by #3 commit "PCI: dwc: Clean up some unnecessary codes in dw_pcie_suspend_noirq()".
> To be simple, re-format the codes, drop the first patch of v3 patch-set, and
> only keep last two patches of v3 in v4 or later
>
> Here is the discussion [1] and final solution [2] of the codes clean up commit.
> [1] https://patchwork.kernel.org/project/linux-pci/patch/1721628913-1449-1-git-send-email-hongxing.zhu@nxp.com/
> [2] https://patchwork.kernel.org/project/linux-pci/patch/20241126073909.4058733-1-hongxing.zhu@nxp.com/
>
> v4 changes:
> Drop the first patch("PCI: dwc: Fix resume failure if no EP is connected on some platforms")
> in v3, since it's use-case had been covered by #3 patch of v3.
> Add one Fixes tag into "PCI: dwc: Clean up some unnecessary codes in dw_pcie_suspend_noirq()".
> Refer to Damien's comments, let ret test go inside the "else" and remove the
> initialization of ret to 0 declaration. Thanks.
Applied to controller/dwc for v6.14, thank you!
Krzysztof
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-01-15 11:34 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-10 8:15 [PATCH v4 0/2] Bug fixes when dwc generic suspend/resume callbacks are used Richard Zhu
2024-12-10 8:15 ` [PATCH v4 1/2] PCI: dwc: Always stop link in the dw_pcie_suspend_noirq Richard Zhu
2025-01-14 18:15 ` Bjorn Helgaas
2025-01-14 19:32 ` Frank Li
2025-01-14 20:26 ` Bjorn Helgaas
2025-01-14 20:43 ` Bjorn Helgaas
2025-01-14 20:51 ` Frank Li
2024-12-10 8:15 ` [PATCH v4 2/2] PCI: dwc: Clean up some unnecessary codes in dw_pcie_suspend_noirq() Richard Zhu
2025-01-15 11:33 ` [PATCH v4 0/2] Bug fixes when dwc generic suspend/resume callbacks are used Krzysztof Wilczyński
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox