[PATCH v7 05/10] PCI: imx6: Deassert apps_reset in imx_pcie_deassert_core_reset()

[Hongxing Zhu]

> -----Original Message-----
> From: Tim Harvey <tharvey at gateworks.com>
> Sent: 2025年6月7日 5:04
> To: Hongxing Zhu <hongxing.zhu at nxp.com>
> Cc: l.stach at pengutronix.de; bhelgaas at google.com; lpieralisi at kernel.org;
> kw at linux.com; manivannan.sadhasivam at linaro.org; robh at kernel.org;
> krzk+dt at kernel.org; conor+dt at kernel.org; shawnguo at kernel.org; Frank Li
> <frank.li at nxp.com>; s.hauer at pengutronix.de; festevam at gmail.com;
> imx at lists.linux.dev; kernel at pengutronix.de; linux-pci at vger.kernel.org;
> linux-arm-kernel at lists.infradead.org; devicetree at vger.kernel.org;
> linux-kernel at vger.kernel.org
> Subject: Re: [PATCH v7 05/10] PCI: imx6: Deassert apps_reset in
> imx_pcie_deassert_core_reset()
> 
> On Tue, Nov 26, 2024 at 12:03 AM Richard Zhu <hongxing.zhu at nxp.com>
> wrote:
> >
> > Since the apps_reset is asserted in imx_pcie_assert_core_reset(), it
> > should be deasserted in imx_pcie_deassert_core_reset().
> >
> > Fixes: 9b3fe6796d7c ("PCI: imx6: Add code to support i.MX7D")
> > Signed-off-by: Richard Zhu <hongxing.zhu at nxp.com>
> > Reviewed-by: Manivannan Sadhasivam
> <manivannan.sadhasivam at linaro.org>
> > Reviewed-by: Frank Li <Frank.Li at 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 3538440601a7..413db182ce9f 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -776,6 +776,7 @@ static void imx_pcie_assert_core_reset(struct
> > imx_pcie *imx_pcie)  static int imx_pcie_deassert_core_reset(struct
> > imx_pcie *imx_pcie)  {
> >         reset_control_deassert(imx_pcie->pciephy_reset);
> > +       reset_control_deassert(imx_pcie->apps_reset);
> >
> >         if (imx_pcie->drvdata->core_reset)
> >                 imx_pcie->drvdata->core_reset(imx_pcie, false);
> > --
> > 2.37.1
> >
> >
> 
> Hi Richard,
> 
> I've found that this patch causes a regression on i.MX8MM and i.MX8MP
> boards with hotplug capable bridges:
> i.MX8MM+PI7C9X2G404EV (this switch does not support hotplug) - no issues
> i.MX8MM+PI7C9X2G608GP (hotplug) - fails to reliably enumerate
> downstream devices about 80% of the time ^^^ when this occurs
> PCI_PRIMARY_BUS (0x18) for the root complex
> 0000:00:00.0 reads 0x00000000 instead of 0x00ff0100
> (PCI_SECONDARY_BUS is 0 instead of 1 and PCI_SUBBORDINATE_BUS is 0
> instead of 0xff) i.MX8MP+PI7C9X2G608GP (hotplug) - hangs at
> imx_pcie_ltssm_enable deassert apps_reset
> 
> In both cases here reverting ef61c7d8d032 ("PCI: imx6: Deassert apps_reset
> in imx_pcie_deassert_core_reset()") resolves this.
> 
[Richard Zhu] I'm afraid that the ltssm_en bit assert to 1b'1 in
 imx_pcie_deassert_core_reset() is not correct in your use case.

Actually, the apps_reset isn't a real reset. It's the ltssm_en bit.
From this perspective view, It's inappropriate to toggle the ltssm_en bit in
 imx_pcie_assert/deassert_core_reset() functions.
I consider to move the apps_reset out of _reset_ functions. 
Can you help to test the following changes in you use-case?

--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -776,7 +776,6 @@ static int imx7d_pcie_core_reset(struct imx_pcie *imx_pcie, bool assert)
 static void imx_pcie_assert_core_reset(struct imx_pcie *imx_pcie)
 {
        reset_control_assert(imx_pcie->pciephy_reset);
-       reset_control_assert(imx_pcie->apps_reset);

        if (imx_pcie->drvdata->core_reset)
                imx_pcie->drvdata->core_reset(imx_pcie, true);
@@ -788,7 +787,6 @@ static void imx_pcie_assert_core_reset(struct imx_pcie *imx_pcie)
 static int imx_pcie_deassert_core_reset(struct imx_pcie *imx_pcie)
 {
        reset_control_deassert(imx_pcie->pciephy_reset);
-       reset_control_deassert(imx_pcie->apps_reset);

        if (imx_pcie->drvdata->core_reset)
                imx_pcie->drvdata->core_reset(imx_pcie, false);
@@ -1176,6 +1174,9 @@ static int imx_pcie_host_init(struct dw_pcie_rp *pp)
                }
        }

+       /* Make sure that PCIe LTSSM is cleared */
+       imx_pcie_ltssm_disable(dev);
+
        ret = imx_pcie_deassert_core_reset(imx_pcie);
        if (ret < 0) {
                dev_err(dev, "pcie deassert core reset failed: %d\n", ret);

> I notice the sequence of events here is:
> imx_pcie_assert_core_reset asserts apps_reset (disables LTSSM)
> imx_pcie_deassert_core_reset deasserts apps_reset (enables LTSSM)
> imx_pcie_ltssm_enable deasserts apps_reset (enables LTSSM; this is where it
> hangs on imx8mp)
> 
> Is there perhaps some issue with de-asserting this (enabling LTSSM) when it's
> already in this state?
[Richard Zhu]The apps_reset is updated by src driver by regmap_update_bits().
I can't find the exceptions to update one bit, already has the according value.

Best Regards
Richard Zhu
> 
> In the case where downstream devices do not enumerate some investigation
> points to them not being happy that the link drops so perhaps deasserting
> apps_reset when its already asserted drops the link and restarts it?
> 
> Best Regards,
> 
> Tim