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

[Hongxing Zhu]

> -----Original Message-----
> From: Tim Harvey <tharvey@xxxxxxxxxxxxx>
> Sent: 2025年6月10日 8:24
> To: Hongxing Zhu <hongxing.zhu@xxxxxxx>
> Cc: l.stach@xxxxxxxxxxxxxx; bhelgaas@xxxxxxxxxx; lpieralisi@xxxxxxxxxx;
> kw@xxxxxxxxx; manivannan.sadhasivam@xxxxxxxxxx; robh@xxxxxxxxxx;
> krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; shawnguo@xxxxxxxxxx; Frank Li
> <frank.li@xxxxxxx>; s.hauer@xxxxxxxxxxxxxx; festevam@xxxxxxxxx;
> imx@xxxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v7 05/10] PCI: imx6: Deassert apps_reset in
> imx_pcie_deassert_core_reset()
>
> On Mon, Jun 9, 2025 at 1:03 AM Hongxing Zhu <hongxing.zhu@xxxxxxx>
> wrote:
> >
> > > -----Original Message-----
> > > From: Tim Harvey <tharvey@xxxxxxxxxxxxx>
> > > Sent: 2025年6月7日 5:04
> > > To: Hongxing Zhu <hongxing.zhu@xxxxxxx>
> > > Cc: l.stach@xxxxxxxxxxxxxx; bhelgaas@xxxxxxxxxx;
> > > lpieralisi@xxxxxxxxxx; kw@xxxxxxxxx;
> > > manivannan.sadhasivam@xxxxxxxxxx; robh@xxxxxxxxxx;
> > > krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; shawnguo@xxxxxxxxxx; Frank
> > > krzk+Li
> > > <frank.li@xxxxxxx>; s.hauer@xxxxxxxxxxxxxx; festevam@xxxxxxxxx;
> > > imx@xxxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx;
> > > linux-pci@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> > > devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > > 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@xxxxxxx>
> > > 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@xxxxxxx>
> > > > Reviewed-by: Manivannan Sadhasivam
> > > <manivannan.sadhasivam@xxxxxxxxxx>
> > > > Reviewed-by: Frank Li <Frank.Li@xxxxxxx>
> > > > ---
> > > >  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.
> >
>
> Hi Richard,
>
> Thanks for your quick response. Do you mean not correct for newer IP core in
> i.MX8MM/i.MX8MP or in the case of a bridge?
[Richard Zhu] What's I mean is that the ltssm_en should be asserted in
 imx_pcie_star_link().
>
> > 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);
> >
>
> Yes this resolves the regression of failing to reliably enumerate downstream
> devices. I think that should be submitted with a fixes tag.
[Richard Zhu] Thanks. That's right.

Best Regards
Richard
>
> The i.MX8MP+PI7C9X2G608GP switch hanging issue was hardware related...
> i was sadly testing on an old board with a defect. I did previously have a hang
> issue there discussed previously here [1] but it was resolved with commit
> 9c03e30e3ade ("PCI: imx6: Skip link up workaround for newer platforms").
>
> How much testing is done with i.MX8M{M,P} board with a switch? I feel like
> I'm the only one with these SoC's and a switch and I need to get better at
> monitoring patches to the IMX6 PCI controller driver and testing these
> scenarios.
>
> Best Regards,
>
> Tim
> [1]
> https://www.s/
> pinics.net%2Flists%2Flinux-pci%2Fmsg142764.html&data=05%7C02%7Chong
> xing.zhu%40nxp.com%7C2fd64741e96e47efc71208dda7b522f6%7C686ea1d
> 3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638851118601391191%7CUnk
> nown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwM
> CIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C
> &sdata=01lfNA%2BfX8dgBStQ6Q7QhcsAU8gVWD0I4kJjJ6SnLck%3D&reserve
> d=0
>
>
>
>
> > > 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