On Tue, Oct 08, 2024 at 08:25:32AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > Sent: 2024年10月3日 14:04
> > To: Hongxing Zhu <hongxing.zhu@xxxxxxx>
> > Cc: jingoohan1@xxxxxxxxx; kwilczynski@xxxxxxxxxx; bhelgaas@xxxxxxxxxx;
> > lpieralisi@xxxxxxxxxx; Frank Li <frank.li@xxxxxxx>; robh@xxxxxxxxxx;
> > linux-pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH v1 1/2] PCI: dwc: Fix resume failure if no EP is connected on
> > some platforms
> >
> > On Wed, Sep 25, 2024 at 01:48:36PM +0800, Richard Zhu wrote:
> > > The dw_pcie_suspend_noirq() function currently returns success
> > > directly if no endpoint (EP) device is connected. However, on some
> > > platforms, power loss occurs during suspend, causing dw_resume() to do
> > nothing in this case.
> > > This results in a system halt because the DWC controller is not
> > > initialized after power-on during resume.
> > >
> > > Change call to deinit() in suspend and init() at resume regardless of
> >
> > s/Change call to/Call
> >
> > > whether there are EP device connections or not. It is not harmful to
> > > perform deinit() and init() again for the no power-off case, and it
> > > keeps the code simple and consistent in logic.
> > >
> > > Fixes: 4774faf854f5 ("PCI: dwc: Implement generic suspend/resume
> > > functionality")
> > > Signed-off-by: Richard Zhu <hongxing.zhu@xxxxxxx>
> > > Reviewed-by: Frank Li <Frank.Li@xxxxxxx>
> > > ---
> > > .../pci/controller/dwc/pcie-designware-host.c | 30
> > > +++++++++----------
> > > 1 file changed, 15 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > index a0822d5371bc..cb8c3c2bcc79 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -933,23 +933,23 @@ 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;
> > >
> >
> > There is one more condition above. It checks whether the link is in L1ss state or
> > not and if it is, the just returns 0. Going by your case, if the power goes off during
> > suspend, then it will be an issue, right?
> >
> Hi Manivannan:
> Thanks for your comments.
> Yes, you're right. It's a problem that power is off in suspend when link
> is in L1ss.
> How about to issue another patch to fix this problem?
> Since this commit is verified to fix the resume failure when no EP is
> connected. I'm not sure I can combine them together or not.
>
Fine with me.
- Mani
> Best Regards
> Richard Zhu
> > > - if (dw_pcie_get_ltssm(pci) <= DW_PCIE_LTSSM_DETECT_ACT)
> > > - return 0;
> > > -
> > > - if (pci->pp.ops->pme_turn_off)
> > > - pci->pp.ops->pme_turn_off(&pci->pp);
> > > - else
> > > - ret = dw_pcie_pme_turn_off(pci);
> > > + if (dw_pcie_get_ltssm(pci) > DW_PCIE_LTSSM_DETECT_ACT) {
> > > + /* Only send out PME_TURN_OFF when PCIE link is up */
> >
> > Move this comment above the 'if' condition.
> >
> > - Mani
> >
> > > + 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 (ret)
> > > + return ret;
> > >
> > > - ret = read_poll_timeout(dw_pcie_get_ltssm, val, val ==
> > DW_PCIE_LTSSM_L2_IDLE,
> > > - PCIE_PME_TO_L2_TIMEOUT_US/10,
> > > - PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
> > > - if (ret) {
> > > - dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n",
> > val);
> > > - return ret;
> > > + ret = read_poll_timeout(dw_pcie_get_ltssm, val, val ==
> > DW_PCIE_LTSSM_L2_IDLE,
> > > + PCIE_PME_TO_L2_TIMEOUT_US/10,
> > > + PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
> > > + if (ret) {
> > > + dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n",
> > val);
> > > + return ret;
> > > + }
> > > }
> > >
> > > if (pci->pp.ops->deinit)
> > > --
> > > 2.37.1
> > >
> >
> > --
> > மணிவண்ணன் சதாசிவம்
--
மணிவண்ணன் சதாசிவம்
