diff mbox series

[v2] phy: freescale: imx8m-pcie: fix pcie link-up instability

Message ID 20240327071946.8869-1-marcel@ziswiler.com (mailing list archive)
State New, archived
Headers show
Series [v2] phy: freescale: imx8m-pcie: fix pcie link-up instability | expand

Commit Message

Marcel Ziswiler March 27, 2024, 7:19 a.m. UTC
From: Marcel Ziswiler <marcel.ziswiler@toradex.com>

On the i.MX 8M Mini, the AUX_PLL_REFCLK_SEL has to be left at its reset
default of AUX_IN (PLL clock).

Background Information:
In our automated testing setup, we use Delock Mini-PCIe SATA cards [1].
While this setup has proven very stable overall we noticed upstream on
the i.MX 8M Mini fails quite regularly (about 50/50) to bring up the
PCIe link while with NXP's downstream BSP 5.15.71_2.2.2 it always works.
As that old downstream stuff was quite different, I first also tried
NXP's latest downstream BSP 6.1.55_2.2.0 which from a PCIe point of view
is fairly vanilla, however, also there the PCIe link-up was not stable.
Comparing and debugging I noticed that upstream explicitly configures
the AUX_PLL_REFCLK_SEL to I_PLL_REFCLK_FROM_SYSPLL while working
downstream [2] leaving it at reset defaults of AUX_IN (PLL clock).
Unfortunately, the TRM does not mention any further details about this
register (both for the i.MX 8M Mini as well as the Plus).
NXP confirmed their validation codes for the i.MX8MM PCIe doesn't
configure cmn_reg063 (offset: 0x18C).
BTW: On the i.MX 8M Plus we have not seen any issues with PCIe with the
exact same setup which is why I left it unchanged.

[1] https://www.delock.com/produkt/95233/merkmale.html
[2] https://github.com/nxp-imx/linux-imx/blob/lf-5.15.71-2.2.0/drivers/pci/controller/dwc/pci-imx6.c#L1548

Fixes: 1aa97b002258 ("phy: freescale: pcie: Initialize the imx8 pcie standalone phy driver")

Cc: stable@vger.kernel.org # 6.1.x: ca679c49: phy: freescale: imx8m-pcie: Refine i.MX8MM PCIe PHY driver
Cc: stable@vger.kernel.org # 6.1.x
Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
Link: https://lore.kernel.org/all/AS8PR04MB867661386FEA07649771FBE18C362@AS8PR04MB8676.eurprd04.prod.outlook.com

---

Changes in v2:
- Reword the commmit message.
- Meld the background information from the cover letter into the commit
  message as suggested by Fabio. Thanks!
- Document NXP's confirmation from their validation codes and add
  Richard Zhu's reviewed-by. Thanks!

 drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

--
2.44.0

Comments

Vinod Koul April 6, 2024, 7:18 a.m. UTC | #1
On 27-03-24, 08:19, Marcel Ziswiler wrote:
> From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> 
> On the i.MX 8M Mini, the AUX_PLL_REFCLK_SEL has to be left at its reset
> default of AUX_IN (PLL clock).
> 
> Background Information:
> In our automated testing setup, we use Delock Mini-PCIe SATA cards [1].
> While this setup has proven very stable overall we noticed upstream on
> the i.MX 8M Mini fails quite regularly (about 50/50) to bring up the
> PCIe link while with NXP's downstream BSP 5.15.71_2.2.2 it always works.
> As that old downstream stuff was quite different, I first also tried
> NXP's latest downstream BSP 6.1.55_2.2.0 which from a PCIe point of view
> is fairly vanilla, however, also there the PCIe link-up was not stable.
> Comparing and debugging I noticed that upstream explicitly configures
> the AUX_PLL_REFCLK_SEL to I_PLL_REFCLK_FROM_SYSPLL while working
> downstream [2] leaving it at reset defaults of AUX_IN (PLL clock).
> Unfortunately, the TRM does not mention any further details about this
> register (both for the i.MX 8M Mini as well as the Plus).
> NXP confirmed their validation codes for the i.MX8MM PCIe doesn't
> configure cmn_reg063 (offset: 0x18C).
> BTW: On the i.MX 8M Plus we have not seen any issues with PCIe with the
> exact same setup which is why I left it unchanged.

This does not apply on phy/fixes, pls rebase
Vinod Koul April 6, 2024, 9:18 a.m. UTC | #2
On Wed, 27 Mar 2024 08:19:37 +0100, Marcel Ziswiler wrote:
> On the i.MX 8M Mini, the AUX_PLL_REFCLK_SEL has to be left at its reset
> default of AUX_IN (PLL clock).
> 
> Background Information:
> In our automated testing setup, we use Delock Mini-PCIe SATA cards [1].
> While this setup has proven very stable overall we noticed upstream on
> the i.MX 8M Mini fails quite regularly (about 50/50) to bring up the
> PCIe link while with NXP's downstream BSP 5.15.71_2.2.2 it always works.
> As that old downstream stuff was quite different, I first also tried
> NXP's latest downstream BSP 6.1.55_2.2.0 which from a PCIe point of view
> is fairly vanilla, however, also there the PCIe link-up was not stable.
> Comparing and debugging I noticed that upstream explicitly configures
> the AUX_PLL_REFCLK_SEL to I_PLL_REFCLK_FROM_SYSPLL while working
> downstream [2] leaving it at reset defaults of AUX_IN (PLL clock).
> Unfortunately, the TRM does not mention any further details about this
> register (both for the i.MX 8M Mini as well as the Plus).
> NXP confirmed their validation codes for the i.MX8MM PCIe doesn't
> configure cmn_reg063 (offset: 0x18C).
> BTW: On the i.MX 8M Plus we have not seen any issues with PCIe with the
> exact same setup which is why I left it unchanged.
> 
> [...]

Applied, thanks!

[1/1] phy: freescale: imx8m-pcie: fix pcie link-up instability
      commit: 3a161017f1de55cc48be81f6156004c151f32677

Best regards,
Marcel Ziswiler April 7, 2024, 10:23 p.m. UTC | #3
Hi Vinod

On Sat, 2024-04-06 at 14:48 +0530, Vinod Koul wrote:
> 
> On Wed, 27 Mar 2024 08:19:37 +0100, Marcel Ziswiler wrote:
> > On the i.MX 8M Mini, the AUX_PLL_REFCLK_SEL has to be left at its reset
> > default of AUX_IN (PLL clock).
> > 
> > Background Information:
> > In our automated testing setup, we use Delock Mini-PCIe SATA cards [1].
> > While this setup has proven very stable overall we noticed upstream on
> > the i.MX 8M Mini fails quite regularly (about 50/50) to bring up the
> > PCIe link while with NXP's downstream BSP 5.15.71_2.2.2 it always works.
> > As that old downstream stuff was quite different, I first also tried
> > NXP's latest downstream BSP 6.1.55_2.2.0 which from a PCIe point of view
> > is fairly vanilla, however, also there the PCIe link-up was not stable.
> > Comparing and debugging I noticed that upstream explicitly configures
> > the AUX_PLL_REFCLK_SEL to I_PLL_REFCLK_FROM_SYSPLL while working
> > downstream [2] leaving it at reset defaults of AUX_IN (PLL clock).
> > Unfortunately, the TRM does not mention any further details about this
> > register (both for the i.MX 8M Mini as well as the Plus).
> > NXP confirmed their validation codes for the i.MX8MM PCIe doesn't
> > configure cmn_reg063 (offset: 0x18C).
> > BTW: On the i.MX 8M Plus we have not seen any issues with PCIe with the
> > exact same setup which is why I left it unchanged.
> > 
> > [...]
> 
> Applied, thanks!
> 
> [1/1] phy: freescale: imx8m-pcie: fix pcie link-up instability
>       commit: 3a161017f1de55cc48be81f6156004c151f32677

Sorry, but it is slightly confusing whether v1 or v2 now got applied. I believe v1 but then only the commit
messages differ. However, please note that only v2 included information on how to proceed concerning
backporting to stable 6.1.x.

Thanks!

> Best regards,
> --
> -Vinod

Cheers

Marcel
diff mbox series

Patch

diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
index b700f52b7b67..11fcb1867118 100644
--- a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
+++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
@@ -110,8 +110,10 @@  static int imx8_pcie_phy_power_on(struct phy *phy)
 		/* Source clock from SoC internal PLL */
 		writel(ANA_PLL_CLK_OUT_TO_EXT_IO_SEL,
 		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG062);
-		writel(AUX_PLL_REFCLK_SEL_SYS_PLL,
-		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG063);
+		if (imx8_phy->drvdata->variant != IMX8MM) {
+			writel(AUX_PLL_REFCLK_SEL_SYS_PLL,
+			       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG063);
+		}
 		val = ANA_AUX_RX_TX_SEL_TX | ANA_AUX_TX_TERM;
 		writel(val | ANA_AUX_RX_TERM_GND_EN,
 		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG064);