diff mbox series

[v14,14/17] PCI: imx6: Do not hide phy driver callbacks and refine the error handling

Message ID 1656645935-1370-15-git-send-email-hongxing.zhu@nxp.com (mailing list archive)
State Superseded
Headers show
Series PCI: imx6: refine codes and add the error propagation | expand

Commit Message

Hongxing Zhu July 1, 2022, 3:25 a.m. UTC 
- Move the phy_power_on() to host_init from imx6_pcie_clk_enable().
- Move the phy_init() to host_init from imx6_pcie_deassert_core_reset().

Refine the error handling in imx6_pcie_host_init() accordingly.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 34 +++++++++++++++++----------
 1 file changed, 21 insertions(+), 13 deletions(-)

Comments

Lucas Stach July 13, 2022, 8:58 a.m. UTC | #1 
Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> - Move the phy_power_on() to host_init from imx6_pcie_clk_enable().
> - Move the phy_init() to host_init from imx6_pcie_deassert_core_reset().
> 
> Refine the error handling in imx6_pcie_host_init() accordingly.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 34 +++++++++++++++++----------
>  1 file changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 5a06fbca82d6..0b2a5256fb0d 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -639,14 +639,6 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
>  		goto err_ref_clk;
>  	}
>  
> -	switch (imx6_pcie->drvdata->variant) {
> -	case IMX8MM:
> -		if (phy_power_on(imx6_pcie->phy))
> -			dev_err(dev, "unable to power on PHY\n");
> -		break;
> -	default:
> -		break;
> -	}
>  	/* allow the clocks to stabilize */
>  	usleep_range(200, 500);
>  	return 0;
> @@ -723,10 +715,6 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
>  	case IMX8MQ:
>  		reset_control_deassert(imx6_pcie->pciephy_reset);
>  		break;
> -	case IMX8MM:
> -		if (phy_init(imx6_pcie->phy))
> -			dev_err(dev, "waiting for phy ready timeout!\n");
> -		break;
>  	case IMX7D:
>  		reset_control_deassert(imx6_pcie->pciephy_reset);
>  
> @@ -762,6 +750,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
>  		usleep_range(200, 500);
>  		break;
>  	case IMX6Q:		/* Nothing to do */
> +	case IMX8MM:
>  		break;
>  	}
>  
> @@ -913,17 +902,36 @@ static int imx6_pcie_host_init(struct pcie_port *pp)
>  			return ret;
>  		}
>  	}
> +	if (imx6_pcie->phy) {
> +		ret = phy_power_on(imx6_pcie->phy);
> +		if (ret) {
> +			dev_err(dev, "pcie phy power up failed.\n");
> +			goto err_reg_disable;
> +		}
> +	}
>  
>  	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
>  	if (ret < 0) {
>  		dev_err(dev, "pcie deassert core reset failed: %d\n", ret);
> -		goto err_reg_disable;
> +		goto err_phy_off;
>  	}
>  
> +	if (imx6_pcie->phy) {
> +		ret = phy_init(imx6_pcie->phy);
> +		if (ret) {
> +			dev_err(dev, "waiting for phy ready timeout!\n");
> +			goto err_clk_disable;
> +		}
> +	}

Wouldn't it be more logical to put this into imx6_pcie_init_phy()?

Regards,
Lucas

>  	imx6_setup_phy_mpll(imx6_pcie);
>  
>  	return 0;
>  
> +err_clk_disable:
> +	imx6_pcie_clk_disable(imx6_pcie);
> +err_phy_off:
> +	if (imx6_pcie->phy)
> +		phy_power_off(imx6_pcie->phy);
>  err_reg_disable:
>  	if (imx6_pcie->vpcie)
>  		regulator_disable(imx6_pcie->vpcie);
Hongxing Zhu July 13, 2022, 10:57 a.m. UTC | #2 
> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2022年7月13日 16:59
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> festevam@gmail.com; francesco.dolcini@toradex.com
> Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v14 14/17] PCI: imx6: Do not hide phy driver callbacks and
> refine the error handling
> 
> Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> > - Move the phy_power_on() to host_init from imx6_pcie_clk_enable().
> > - Move the phy_init() to host_init from imx6_pcie_deassert_core_reset().
> >
> > Refine the error handling in imx6_pcie_host_init() accordingly.
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 34 +++++++++++++++++----------
> >  1 file changed, 21 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> b/drivers/pci/controller/dwc/pci-imx6.c
> > index 5a06fbca82d6..0b2a5256fb0d 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -639,14 +639,6 @@ static int imx6_pcie_clk_enable(struct imx6_pcie
> *imx6_pcie)
> >  		goto err_ref_clk;
> >  	}
> >
> > -	switch (imx6_pcie->drvdata->variant) {
> > -	case IMX8MM:
> > -		if (phy_power_on(imx6_pcie->phy))
> > -			dev_err(dev, "unable to power on PHY\n");
> > -		break;
> > -	default:
> > -		break;
> > -	}
> >  	/* allow the clocks to stabilize */
> >  	usleep_range(200, 500);
> >  	return 0;
> > @@ -723,10 +715,6 @@ static int imx6_pcie_deassert_core_reset(struct
> imx6_pcie *imx6_pcie)
> >  	case IMX8MQ:
> >  		reset_control_deassert(imx6_pcie->pciephy_reset);
> >  		break;
> > -	case IMX8MM:
> > -		if (phy_init(imx6_pcie->phy))
> > -			dev_err(dev, "waiting for phy ready timeout!\n");
> > -		break;
> >  	case IMX7D:
> >  		reset_control_deassert(imx6_pcie->pciephy_reset);
> >
> > @@ -762,6 +750,7 @@ static int imx6_pcie_deassert_core_reset(struct
> imx6_pcie *imx6_pcie)
> >  		usleep_range(200, 500);
> >  		break;
> >  	case IMX6Q:		/* Nothing to do */
> > +	case IMX8MM:
> >  		break;
> >  	}
> >
> > @@ -913,17 +902,36 @@ static int imx6_pcie_host_init(struct pcie_port
> *pp)
> >  			return ret;
> >  		}
> >  	}
> > +	if (imx6_pcie->phy) {
> > +		ret = phy_power_on(imx6_pcie->phy);
> > +		if (ret) {
> > +			dev_err(dev, "pcie phy power up failed.\n");
> > +			goto err_reg_disable;
> > +		}
> > +	}
> >
> >  	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
> >  	if (ret < 0) {
> >  		dev_err(dev, "pcie deassert core reset failed: %d\n", ret);
> > -		goto err_reg_disable;
> > +		goto err_phy_off;
> >  	}
> >
> > +	if (imx6_pcie->phy) {
> > +		ret = phy_init(imx6_pcie->phy);
> > +		if (ret) {
> > +			dev_err(dev, "waiting for phy ready timeout!\n");
> > +			goto err_clk_disable;
> > +		}
> > +	}
> 
> Wouldn't it be more logical to put this into imx6_pcie_init_phy()?
> 
Before adding i.MX8MM PCIe support, the imx6_pcie_init_phy() only touches the
 GPR registers. PCIe clocks and so on are not required in this case.
But phy_init() used by i.MX8MM PCIe touches not only the GPR registers but
 also the PHY's registers.
The clocks should be on and resets of PHY should be configured properly when
 phy_init() is invoked.
So, phy_init() is placed behind of imx6_pcie_deassert_core_reset() here.

Best Regards
Richard Zhu

> Regards,
> Lucas
> 
> >  	imx6_setup_phy_mpll(imx6_pcie);
> >
> >  	return 0;
> >
> > +err_clk_disable:
> > +	imx6_pcie_clk_disable(imx6_pcie);
> > +err_phy_off:
> > +	if (imx6_pcie->phy)
> > +		phy_power_off(imx6_pcie->phy);
> >  err_reg_disable:
> >  	if (imx6_pcie->vpcie)
> >  		regulator_disable(imx6_pcie->vpcie);
>
Lucas Stach July 13, 2022, 11:17 a.m. UTC | #3 
Am Mittwoch, dem 13.07.2022 um 10:57 +0000 schrieb Hongxing Zhu:
> > -----Original Message-----
> > From: Lucas Stach <l.stach@pengutronix.de>
> > Sent: 2022年7月13日 16:59
> > To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> > robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> > festevam@gmail.com; francesco.dolcini@toradex.com
> > Cc: linux-pci@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org;
> > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > <linux-imx@nxp.com>
> > Subject: Re: [PATCH v14 14/17] PCI: imx6: Do not hide phy driver
> > callbacks and
> > refine the error handling
> > 
> > Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> > > - Move the phy_power_on() to host_init from
> > > imx6_pcie_clk_enable().
> > > - Move the phy_init() to host_init from
> > > imx6_pcie_deassert_core_reset().
> > > 
> > > Refine the error handling in imx6_pcie_host_init() accordingly.
> > > 
> > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > ---
> > >  drivers/pci/controller/dwc/pci-imx6.c | 34 +++++++++++++++++----
> > > ------
> > >  1 file changed, 21 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > > index 5a06fbca82d6..0b2a5256fb0d 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -639,14 +639,6 @@ static int imx6_pcie_clk_enable(struct
> > > imx6_pcie
> > *imx6_pcie)
> > >  		goto err_ref_clk;
> > >  	}
> > > 
> > > -	switch (imx6_pcie->drvdata->variant) {
> > > -	case IMX8MM:
> > > -		if (phy_power_on(imx6_pcie->phy))
> > > -			dev_err(dev, "unable to power on
> > > PHY\n");
> > > -		break;
> > > -	default:
> > > -		break;
> > > -	}
> > >  	/* allow the clocks to stabilize */
> > >  	usleep_range(200, 500);
> > >  	return 0;
> > > @@ -723,10 +715,6 @@ static int
> > > imx6_pcie_deassert_core_reset(struct
> > imx6_pcie *imx6_pcie)
> > >  	case IMX8MQ:
> > >  		reset_control_deassert(imx6_pcie-
> > > >pciephy_reset);
> > >  		break;
> > > -	case IMX8MM:
> > > -		if (phy_init(imx6_pcie->phy))
> > > -			dev_err(dev, "waiting for phy ready
> > > timeout!\n");
> > > -		break;
> > >  	case IMX7D:
> > >  		reset_control_deassert(imx6_pcie-
> > > >pciephy_reset);
> > > 
> > > @@ -762,6 +750,7 @@ static int
> > > imx6_pcie_deassert_core_reset(struct
> > imx6_pcie *imx6_pcie)
> > >  		usleep_range(200, 500);
> > >  		break;
> > >  	case IMX6Q:		/* Nothing to do */
> > > +	case IMX8MM:
> > >  		break;
> > >  	}
> > > 
> > > @@ -913,17 +902,36 @@ static int imx6_pcie_host_init(struct
> > > pcie_port
> > *pp)
> > >  			return ret;
> > >  		}
> > >  	}
> > > +	if (imx6_pcie->phy) {
> > > +		ret = phy_power_on(imx6_pcie->phy);
> > > +		if (ret) {
> > > +			dev_err(dev, "pcie phy power up
> > > failed.\n");
> > > +			goto err_reg_disable;
> > > +		}
> > > +	}
> > > 
> > >  	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
> > >  	if (ret < 0) {
> > >  		dev_err(dev, "pcie deassert core reset failed:
> > > %d\n", ret);
> > > -		goto err_reg_disable;
> > > +		goto err_phy_off;
> > >  	}
> > > 
> > > +	if (imx6_pcie->phy) {
> > > +		ret = phy_init(imx6_pcie->phy);
> > > +		if (ret) {
> > > +			dev_err(dev, "waiting for phy ready
> > > timeout!\n");
> > > +			goto err_clk_disable;
> > > +		}
> > > +	}
> > 
> > Wouldn't it be more logical to put this into imx6_pcie_init_phy()?
> > 
> Before adding i.MX8MM PCIe support, the imx6_pcie_init_phy() only
> touches the
>  GPR registers. PCIe clocks and so on are not required in this case.
> But phy_init() used by i.MX8MM PCIe touches not only the GPR
> registers but
>  also the PHY's registers.
> The clocks should be on and resets of PHY should be configured
> properly when
>  phy_init() is invoked.
> So, phy_init() is placed behind of imx6_pcie_deassert_core_reset()
> here.

The PHY driver should be self-contained enough to not care about the
state of the controller here, no? It should set all the necessary GPRs
and enable clocks as needed on its own. Is this not the case with the
current code?

Also PHY init should be called before PHY power-on, to make things
symmetric with the shutdown paths which do phy_power_off() first, then
phy_exit().

Regards,
Lucas
Hongxing Zhu July 13, 2022, 12:52 p.m. UTC | #4 
> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2022年7月13日 19:17
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> festevam@gmail.com; francesco.dolcini@toradex.com
> Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v14 14/17] PCI: imx6: Do not hide phy driver callbacks and
> refine the error handling
> 
> Am Mittwoch, dem 13.07.2022 um 10:57 +0000 schrieb Hongxing Zhu:
> > > -----Original Message-----
> > > From: Lucas Stach <l.stach@pengutronix.de>
> > > Sent: 2022年7月13日 16:59
> > > To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> > > robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> > > festevam@gmail.com; francesco.dolcini@toradex.com
> > > Cc: linux-pci@vger.kernel.org;
> > > linux-arm-kernel@lists.infradead.org;
> > > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > > <linux-imx@nxp.com>
> > > Subject: Re: [PATCH v14 14/17] PCI: imx6: Do not hide phy driver
> > > callbacks and refine the error handling
> > >
> > > Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> > > > - Move the phy_power_on() to host_init from
> > > > imx6_pcie_clk_enable().
> > > > - Move the phy_init() to host_init from
> > > > imx6_pcie_deassert_core_reset().
> > > >
> > > > Refine the error handling in imx6_pcie_host_init() accordingly.
> > > >
> > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > ---
> > > >  drivers/pci/controller/dwc/pci-imx6.c | 34 +++++++++++++++++----
> > > > ------
> > > >  1 file changed, 21 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > > index 5a06fbca82d6..0b2a5256fb0d 100644
> > > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > > @@ -639,14 +639,6 @@ static int imx6_pcie_clk_enable(struct
> > > > imx6_pcie
> > > *imx6_pcie)
> > > >  		goto err_ref_clk;
> > > >  	}
> > > >
> > > > -	switch (imx6_pcie->drvdata->variant) {
> > > > -	case IMX8MM:
> > > > -		if (phy_power_on(imx6_pcie->phy))
> > > > -			dev_err(dev, "unable to power on
> > > > PHY\n");
> > > > -		break;
> > > > -	default:
> > > > -		break;
> > > > -	}
> > > >  	/* allow the clocks to stabilize */
> > > >  	usleep_range(200, 500);
> > > >  	return 0;
> > > > @@ -723,10 +715,6 @@ static int
> > > > imx6_pcie_deassert_core_reset(struct
> > > imx6_pcie *imx6_pcie)
> > > >  	case IMX8MQ:
> > > >  		reset_control_deassert(imx6_pcie-
> > > > >pciephy_reset);
> > > >  		break;
> > > > -	case IMX8MM:
> > > > -		if (phy_init(imx6_pcie->phy))
> > > > -			dev_err(dev, "waiting for phy ready
> > > > timeout!\n");
> > > > -		break;
> > > >  	case IMX7D:
> > > >  		reset_control_deassert(imx6_pcie-
> > > > >pciephy_reset);
> > > >
> > > > @@ -762,6 +750,7 @@ static int
> > > > imx6_pcie_deassert_core_reset(struct
> > > imx6_pcie *imx6_pcie)
> > > >  		usleep_range(200, 500);
> > > >  		break;
> > > >  	case IMX6Q:		/* Nothing to do */
> > > > +	case IMX8MM:
> > > >  		break;
> > > >  	}
> > > >
> > > > @@ -913,17 +902,36 @@ static int imx6_pcie_host_init(struct
> > > > pcie_port
> > > *pp)
> > > >  			return ret;
> > > >  		}
> > > >  	}
> > > > +	if (imx6_pcie->phy) {
> > > > +		ret = phy_power_on(imx6_pcie->phy);
> > > > +		if (ret) {
> > > > +			dev_err(dev, "pcie phy power up
> > > > failed.\n");
> > > > +			goto err_reg_disable;
> > > > +		}
> > > > +	}
> > > >
> > > >  	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
> > > >  	if (ret < 0) {
> > > >  		dev_err(dev, "pcie deassert core reset failed:
> > > > %d\n", ret);
> > > > -		goto err_reg_disable;
> > > > +		goto err_phy_off;
> > > >  	}
> > > >
> > > > +	if (imx6_pcie->phy) {
> > > > +		ret = phy_init(imx6_pcie->phy);
> > > > +		if (ret) {
> > > > +			dev_err(dev, "waiting for phy ready
> > > > timeout!\n");
> > > > +			goto err_clk_disable;
> > > > +		}
> > > > +	}
> > >
> > > Wouldn't it be more logical to put this into imx6_pcie_init_phy()?
> > >
> > Before adding i.MX8MM PCIe support, the imx6_pcie_init_phy() only
> > touches the
> >  GPR registers. PCIe clocks and so on are not required in this case.
> > But phy_init() used by i.MX8MM PCIe touches not only the GPR registers
> > but
> >  also the PHY's registers.
> > The clocks should be on and resets of PHY should be configured
> > properly when
> >  phy_init() is invoked.
> > So, phy_init() is placed behind of imx6_pcie_deassert_core_reset()
> > here.
> 
> The PHY driver should be self-contained enough to not care about the state of
> the controller here, no? It should set all the necessary GPRs and enable clocks
> as needed on its own. Is this not the case with the current code?
> 
> Also PHY init should be called before PHY power-on, to make things symmetric
> with the shutdown paths which do phy_power_off() first, then phy_exit().
> 
Thanks for your comments.
Yes, agree with that PHY driver should be self-contained enough.
I mis-understood the sequence of phy_init() and phy_power_on() when I add
 the PHY driver. The phy_init() is relied on the phy_power_on() here.
So, I have to place the phy_init() behind phy_power_on() currently.
I think the phy_init() can be placed in imx6_pcie_init_phy() when the order
 issue is fixed.

BTW, Bjorn had noticed me this issue too [1]. Since it's a bug, I would issue
 another standalone bug fix commits to resolve this issue later.
[1] https://patchwork.ozlabs.org/project/linux-pci/patch/1655461874-16908-11-git-send-email-hongxing.zhu@nxp.com/

Best Regards
Richard Zhu

> Regards,
> Lucas
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 5a06fbca82d6..0b2a5256fb0d 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -639,14 +639,6 @@  static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
 		goto err_ref_clk;
 	}
 
-	switch (imx6_pcie->drvdata->variant) {
-	case IMX8MM:
-		if (phy_power_on(imx6_pcie->phy))
-			dev_err(dev, "unable to power on PHY\n");
-		break;
-	default:
-		break;
-	}
 	/* allow the clocks to stabilize */
 	usleep_range(200, 500);
 	return 0;
@@ -723,10 +715,6 @@  static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 	case IMX8MQ:
 		reset_control_deassert(imx6_pcie->pciephy_reset);
 		break;
-	case IMX8MM:
-		if (phy_init(imx6_pcie->phy))
-			dev_err(dev, "waiting for phy ready timeout!\n");
-		break;
 	case IMX7D:
 		reset_control_deassert(imx6_pcie->pciephy_reset);
 
@@ -762,6 +750,7 @@  static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 		usleep_range(200, 500);
 		break;
 	case IMX6Q:		/* Nothing to do */
+	case IMX8MM:
 		break;
 	}
 
@@ -913,17 +902,36 @@  static int imx6_pcie_host_init(struct pcie_port *pp)
 			return ret;
 		}
 	}
+	if (imx6_pcie->phy) {
+		ret = phy_power_on(imx6_pcie->phy);
+		if (ret) {
+			dev_err(dev, "pcie phy power up failed.\n");
+			goto err_reg_disable;
+		}
+	}
 
 	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
 	if (ret < 0) {
 		dev_err(dev, "pcie deassert core reset failed: %d\n", ret);
-		goto err_reg_disable;
+		goto err_phy_off;
 	}
 
+	if (imx6_pcie->phy) {
+		ret = phy_init(imx6_pcie->phy);
+		if (ret) {
+			dev_err(dev, "waiting for phy ready timeout!\n");
+			goto err_clk_disable;
+		}
+	}
 	imx6_setup_phy_mpll(imx6_pcie);
 
 	return 0;
 
+err_clk_disable:
+	imx6_pcie_clk_disable(imx6_pcie);
+err_phy_off:
+	if (imx6_pcie->phy)
+		phy_power_off(imx6_pcie->phy);
 err_reg_disable:
 	if (imx6_pcie->vpcie)
 		regulator_disable(imx6_pcie->vpcie);