[PATCH v1 0/1] phy: freescale: imx8m-pcie: facing pcie link-up instability

[PATCH v1 0/1] phy: freescale: imx8m-pcie: facing pcie link-up instability

[Marcel Ziswiler]

From: Marcel Ziswiler <marcel.ziswiler@toradex.com>


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.MX8M 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). Maybe somebody
from NXP could further comment on this?

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


Marcel Ziswiler (1):
  phy: freescale: imx8m-pcie: fix pcie link-up instability

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

--
2.44.0

[PATCH v1 1/1] phy: freescale: imx8m-pcie: fix pcie link-up instability

[Marcel Ziswiler]

From: Marcel Ziswiler <marcel.ziswiler@toradex.com>

Leaving AUX_PLL_REFCLK_SEL at its reset default of AUX_IN (PLL clock)
proves to be more stable on the i.MX 8M Mini.

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

Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
---

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

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);
--
2.44.0

RE: [PATCH v1 1/1] phy: freescale: imx8m-pcie: fix pcie link-up instability

[Hongxing Zhu]

> -----Original Message-----
> From: Marcel Ziswiler <marcel@ziswiler.com>
> Sent: 2024年3月22日 21:07
> To: linux-phy@lists.infradead.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; Lucas Stach <l.stach@pengutronix.de>;
> linux-arm-kernel@lists.infradead.org; kernel@pengutronix.de; Hongxing Zhu
> <hongxing.zhu@nxp.com>; linux-kernel@vger.kernel.org; Marcel Ziswiler
> <marcel.ziswiler@toradex.com>; Fabio Estevam <festevam@gmail.com>; Heiko
> Stuebner <heiko@sntech.de>; Kishon Vijay Abraham I <kishon@kernel.org>;
> Marc Kleine-Budde <mkl@pengutronix.de>; Rob Herring <robh@kernel.org>;
> Sascha Hauer <s.hauer@pengutronix.de>; Shawn Guo <shawnguo@kernel.org>;
> tharvey@gateworks.com; Vinod Koul <vkoul@kernel.org>; Yang Li
> <yang.lee@linux.alibaba.com>; imx@lists.linux.dev
> Subject: [PATCH v1 1/1] phy: freescale: imx8m-pcie: fix pcie link-up instability
> 
> From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> 
> Leaving AUX_PLL_REFCLK_SEL at its reset default of AUX_IN (PLL clock) proves to
> be more stable on the i.MX 8M Mini.
> 
> Fixes: 1aa97b002258 ("phy: freescale: pcie: Initialize the imx8 pcie standalone
> phy driver")
> 
Hi Marcel
I took look back at the validation codes.
i.MX8MM PCIe doesn't configure cmn_reg063 (offset: 0x18C) indeed.

It's my bad to treat i.MX8MM same as i.MX8MP refer to my assumption on the
 literal meaning of these bit definitions.

Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>

Best Regards
Richard Zhu

> Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> ---
> 
>  drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> 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);
> --
> 2.44.0

Re: [PATCH v1 1/1] phy: freescale: imx8m-pcie: fix pcie link-up instability

[Vinod Koul]

On 22-03-24, 14:06, Marcel Ziswiler wrote:
> From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> 
> Leaving AUX_PLL_REFCLK_SEL at its reset default of AUX_IN (PLL clock)
> proves to be more stable on the i.MX 8M Mini.
> 
> Fixes: 1aa97b002258 ("phy: freescale: pcie: Initialize the imx8 pcie standalone phy driver")
> 

Please dont keep an empty line b/w Fixes and s-o-b line...

> Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> ---
> 
>  drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> 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);
> --
> 2.44.0
> 
> 
> -- 
> linux-phy mailing list
> linux-phy@lists.infradead.org
> https://lists.infradead.org/mailman/listinfo/linux-phy

-- 
~Vinod

Re: [PATCH v1 1/1] phy: freescale: imx8m-pcie: fix pcie link-up instability

[Vinod Koul]

On 22-03-24, 14:06, Marcel Ziswiler wrote:
> From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> 
> Leaving AUX_PLL_REFCLK_SEL at its reset default of AUX_IN (PLL clock)
> proves to be more stable on the i.MX 8M Mini.

This doesn't apply on phy/fixes

Please rebase

-- 
~Vinod

Re: [PATCH v1 0/1] phy: freescale: imx8m-pcie: facing pcie link-up instability

[Fabio Estevam]

Hi Marcel,

On Fri, Mar 22, 2024 at 10:07 AM Marcel Ziswiler <marcel@ziswiler.com> wrote:
>
> From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
>
>
> 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.MX8M 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). Maybe somebody
> from NXP could further comment on this?
>
> 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

The detailed information above is important. It should be in the
commit log of the patch IMHO.

Thanks for the fix.

Re: [PATCH v1 0/1] phy: freescale: imx8m-pcie: facing pcie link-up instability

[Vinod Koul]

On Fri, 22 Mar 2024 14:06:31 +0100, Marcel Ziswiler wrote:
> 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.MX8M 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). Maybe somebody
> from NXP could further comment on this?
> 
> [...]

Applied, thanks!

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

Best regards,
-- 
~Vinod

Re: [PATCH v1 0/1] phy: freescale: imx8m-pcie: facing pcie link-up instability

[Marcel Ziswiler]

Hi Vinod

On Sat, 2024-04-06 at 14:48 +0530, Vinod Koul wrote:
> 
> On Fri, 22 Mar 2024 14:06:31 +0100, Marcel Ziswiler wrote:
> > 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.MX8M 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). Maybe somebody
> > from NXP could further comment on this?
> > 
> > [...]
> 
> 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

Re: [PATCH v1 0/1] phy: freescale: imx8m-pcie: facing pcie link-up instability

[Vinod Koul]

On 07-04-24, 22:22, Marcel Ziswiler wrote:
> Hi Vinod
> 
> On Sat, 2024-04-06 at 14:48 +0530, Vinod Koul wrote:
> > 
> > On Fri, 22 Mar 2024 14:06:31 +0100, Marcel Ziswiler wrote:
> > > 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.MX8M 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). Maybe somebody
> > > from NXP could further comment on this?
> > > 
> > > [...]
> > 
> > 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.

V2 was picked, you can check phy tree

Somehow b4 sent email for v1 as well

-- 
~Vinod

Re: [PATCH v1 0/1] phy: freescale: imx8m-pcie: facing pcie link-up instability

[Marcel Ziswiler]

Hi Vinod

On Fri, 2024-04-12 at 16:54 +0530, Vinod Koul wrote:
> On 07-04-24, 22:22, Marcel Ziswiler wrote:
> > Hi Vinod
> > 
> > On Sat, 2024-04-06 at 14:48 +0530, Vinod Koul wrote:
> > > 
> > > On Fri, 22 Mar 2024 14:06:31 +0100, Marcel Ziswiler wrote:
> > > > 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.MX8M 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). Maybe somebody
> > > > from NXP could further comment on this?
> > > > 
> > > > [...]
> > > 
> > > 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.
> 
> V2 was picked, you can check phy tree

Okay, however it uses the commit message of v1 which is rather strange.

> Somehow b4 sent email for v1 as well

Cheers

Marcel