[PATCH v1 0/2] phy: freescale: imx8m-pcie: fix and cleanup phy

[PATCH v1 0/2] phy: freescale: imx8m-pcie: fix and cleanup phy

[Stefan Eichenberger]

The imx8m-pcie PHY PLL fails to lock under certain conditions when
returning from suspend. This is resolved by asserting the PHY reset when
powering off the PHY during suspend. This ensures that the PHY is
properly reset when powering on again in resume.

Stefan Eichenberger (2):
  phy: freescale: imx8m-pcie: cleanup reset logic
  phy: freescale: imx8m-pcie: assert phy reset and perst in power off

 drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 24 ++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

-- 
2.45.2

[PATCH v1 1/2] phy: freescale: imx8m-pcie: cleanup reset logic

[Stefan Eichenberger]

From: Stefan Eichenberger <stefan.eichenberger@toradex.com>

Remove the switch statement and base perst release on whether it is
found in the device tree. The probe function fails without the reset
property, making it mandatory. Therefore, always release reset
independent of the variant.

This does not change the behavior of the driver but reduces driver
complexity and allows for easier future modifications.

Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
---
 drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
index e98361dcdeadf..00f957a42d9dc 100644
--- a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
+++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
@@ -141,15 +141,11 @@ static int imx8_pcie_phy_power_on(struct phy *phy)
 			   IMX8MM_GPR_PCIE_REF_CLK_PLL);
 	usleep_range(100, 200);
 
-	switch (imx8_phy->drvdata->variant) {
-	case IMX8MP:
+	if (imx8_phy->perst)
 		reset_control_deassert(imx8_phy->perst);
-		fallthrough;
-	case IMX8MM:
-		reset_control_deassert(imx8_phy->reset);
-		usleep_range(200, 500);
-		break;
-	}
+
+	reset_control_deassert(imx8_phy->reset);
+	usleep_range(200, 500);
 
 	/* Do the PHY common block reset */
 	regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
-- 
2.45.2

[PATCH v1 2/2] phy: freescale: imx8m-pcie: assert phy reset and perst in power off

[Stefan Eichenberger]

From: Stefan Eichenberger <stefan.eichenberger@toradex.com>

Ensure the PHY reset and perst is asserted during power-off to
guarantee it is in a reset state upon repeated power-on calls. This
resolves an issue where the PHY may not properly initialize during
subsequent power-on cycles. Power-on will deassert the reset at the
appropriate time after tuning the PHY parameters.

During suspend/resume cycles, we observed that the PHY PLL failed to
lock during resume when the CPU temperature increased from 65C to 75C.
The observed errors were:
  phy phy-32f00000.pcie-phy.3: phy poweron failed --> -110
  imx6q-pcie 33800000.pcie: waiting for PHY ready timeout!
  imx6q-pcie 33800000.pcie: PM: dpm_run_callback(): genpd_resume_noirq+0x0/0x80 returns -110
  imx6q-pcie 33800000.pcie: PM: failed to resume noirq: error -110

This resulted in a complete CPU freeze, which is resolved by ensuring
the PHY is in reset during power-on, thus preventing PHY PLL failures.

Cc: stable@vger.kernel.org
Fixes: 1aa97b002258 ("phy: freescale: pcie: Initialize the imx8 pcie standalone phy driver")
Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
---
 drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
index 00f957a42d9dc..36bef416618de 100644
--- a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
+++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
@@ -158,6 +158,17 @@ static int imx8_pcie_phy_power_on(struct phy *phy)
 	return ret;
 }
 
+static int imx8_pcie_phy_power_off(struct phy *phy)
+{
+	struct imx8_pcie_phy *imx8_phy = phy_get_drvdata(phy);
+
+	reset_control_assert(imx8_phy->reset);
+	if (imx8_phy->perst)
+		reset_control_assert(imx8_phy->perst);
+
+	return 0;
+}
+
 static int imx8_pcie_phy_init(struct phy *phy)
 {
 	struct imx8_pcie_phy *imx8_phy = phy_get_drvdata(phy);
@@ -178,6 +189,7 @@ static const struct phy_ops imx8_pcie_phy_ops = {
 	.init		= imx8_pcie_phy_init,
 	.exit		= imx8_pcie_phy_exit,
 	.power_on	= imx8_pcie_phy_power_on,
+	.power_off	= imx8_pcie_phy_power_off,
 	.owner		= THIS_MODULE,
 };
 
-- 
2.45.2

Re: [PATCH v1 1/2] phy: freescale: imx8m-pcie: cleanup reset logic

[Philipp Zabel]

On Fr, 2025-02-28 at 11:38 +0100, Stefan Eichenberger wrote:
> From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> 
> Remove the switch statement and base perst release on whether it is
> found in the device tree. The probe function fails without the reset
> property, making it mandatory. Therefore, always release reset
> independent of the variant.
> 
> This does not change the behavior of the driver but reduces driver
> complexity and allows for easier future modifications.
> 
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> ---
>  drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> index e98361dcdeadf..00f957a42d9dc 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> @@ -141,15 +141,11 @@ static int imx8_pcie_phy_power_on(struct phy *phy)
>  			   IMX8MM_GPR_PCIE_REF_CLK_PLL);
>  	usleep_range(100, 200);
>  
> -	switch (imx8_phy->drvdata->variant) {
> -	case IMX8MP:
> +	if (imx8_phy->perst)

This check is not necessary, reset_control_deassert(NULL) is a no-op:

>  		reset_control_deassert(imx8_phy->perst);

regards
Philipp

Re: [PATCH v1 2/2] phy: freescale: imx8m-pcie: assert phy reset and perst in power off

[Philipp Zabel]

On Fr, 2025-02-28 at 11:38 +0100, Stefan Eichenberger wrote:
> From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> 
> Ensure the PHY reset and perst is asserted during power-off to
> guarantee it is in a reset state upon repeated power-on calls. This
> resolves an issue where the PHY may not properly initialize during
> subsequent power-on cycles. Power-on will deassert the reset at the
> appropriate time after tuning the PHY parameters.
> 
> During suspend/resume cycles, we observed that the PHY PLL failed to
> lock during resume when the CPU temperature increased from 65C to 75C.
> The observed errors were:
>   phy phy-32f00000.pcie-phy.3: phy poweron failed --> -110
>   imx6q-pcie 33800000.pcie: waiting for PHY ready timeout!
>   imx6q-pcie 33800000.pcie: PM: dpm_run_callback(): genpd_resume_noirq+0x0/0x80 returns -110
>   imx6q-pcie 33800000.pcie: PM: failed to resume noirq: error -110
> 
> This resulted in a complete CPU freeze, which is resolved by ensuring
> the PHY is in reset during power-on, thus preventing PHY PLL failures.
> 
> Cc: stable@vger.kernel.org
> Fixes: 1aa97b002258 ("phy: freescale: pcie: Initialize the imx8 pcie standalone phy driver")
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> ---
>  drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> index 00f957a42d9dc..36bef416618de 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> @@ -158,6 +158,17 @@ static int imx8_pcie_phy_power_on(struct phy *phy)
>  	return ret;
>  }
>  
> +static int imx8_pcie_phy_power_off(struct phy *phy)
> +{
> +	struct imx8_pcie_phy *imx8_phy = phy_get_drvdata(phy);
> +
> +	reset_control_assert(imx8_phy->reset);
> +	if (imx8_phy->perst)

This check is not necessary, reset_control_assert(NULL) is a no-op:

> +		reset_control_assert(imx8_phy->perst);
> +
> +	return 0;
> +}

regards
Philipp

Re: [PATCH v1 1/2] phy: freescale: imx8m-pcie: cleanup reset logic

[Stefan Eichenberger]

Hi Philipp,

On Fri, Feb 28, 2025 at 01:38:11PM +0100, Philipp Zabel wrote:
> On Fr, 2025-02-28 at 11:38 +0100, Stefan Eichenberger wrote:
> > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > 
> > Remove the switch statement and base perst release on whether it is
> > found in the device tree. The probe function fails without the reset
> > property, making it mandatory. Therefore, always release reset
> > independent of the variant.
> > 
> > This does not change the behavior of the driver but reduces driver
> > complexity and allows for easier future modifications.
> > 
> > Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > ---
> >  drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > index e98361dcdeadf..00f957a42d9dc 100644
> > --- a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > +++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > @@ -141,15 +141,11 @@ static int imx8_pcie_phy_power_on(struct phy *phy)
> >  			   IMX8MM_GPR_PCIE_REF_CLK_PLL);
> >  	usleep_range(100, 200);
> >  
> > -	switch (imx8_phy->drvdata->variant) {
> > -	case IMX8MP:
> > +	if (imx8_phy->perst)
> 
> This check is not necessary, reset_control_deassert(NULL) is a no-op:
> 

Thanks for the feedback, I will remove the check in the next version.

Regards,
Stefan