[PATCH] PCI: imx6: Fix link initialisation when the phy is ref clk provider

[PATCH] PCI: imx6: Fix link initialisation when the phy is ref clk provider

[Sascha Hauer]

When the phy is the reference clock provider then it must be initialised
and powered on before the reset on the client is deasserted, otherwise
the link will never come up. The order was changed in cf236e0c0d59.
Restore the correct order to make the driver work again on boards where
the phy provides the reference clock.

Fixes: cf236e0c0d59 ("PCI: imx6: Do not hide PHY driver callbacks and refine the error handling")
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/pci/controller/dwc/pci-imx6.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index b5f0de455a7bd..211eb55d6d34b 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -942,12 +942,6 @@ static int imx6_pcie_host_init(struct dw_pcie_rp *pp)
 		}
 	}
 
-	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
-	if (ret < 0) {
-		dev_err(dev, "pcie deassert core reset failed: %d\n", ret);
-		goto err_phy_off;
-	}
-
 	if (imx6_pcie->phy) {
 		ret = phy_power_on(imx6_pcie->phy);
 		if (ret) {
@@ -955,6 +949,13 @@ static int imx6_pcie_host_init(struct dw_pcie_rp *pp)
 			goto err_phy_off;
 		}
 	}
+
+	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
+	if (ret < 0) {
+		dev_err(dev, "pcie deassert core reset failed: %d\n", ret);
+		goto err_phy_off;
+	}
+
 	imx6_setup_phy_mpll(imx6_pcie);
 
 	return 0;
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] PCI: imx6: Fix link initialisation when the phy is ref clk provider

[Hongxing Zhu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 2723 bytes --]


> -----Original Message-----
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Sent: 2022Äê10ÔÂ12ÈÕ 21:27
> To: linux-pci@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org; Hongxing Zhu
> <hongxing.zhu@nxp.com>; Lorenzo Pieralisi <lpieralisi@kernel.org>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; dl-linux-imx
> <linux-imx@nxp.com>; Rob Herring <robh@kernel.org>; Krzysztof Wilczy¨½ski
> <kw@linux.com>; Sascha Hauer <s.hauer@pengutronix.de>
> Subject: [PATCH] PCI: imx6: Fix link initialisation when the phy is ref clk
> provider
> 
> When the phy is the reference clock provider then it must be initialised and
> powered on before the reset on the client is deasserted, otherwise the link will
> never come up. The order was changed in cf236e0c0d59.
> Restore the correct order to make the driver work again on boards where the
> phy provides the reference clock.
> 
> Fixes: cf236e0c0d59 ("PCI: imx6: Do not hide PHY driver callbacks and refine
> the error handling")
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Hi Sascha:
Thanks for your patch.
initialisation /s/ initialization
Initialised /s/ initialized

As I remember that the sequence of the initialization is not changed in
 cf236e0c0d59 ("PCI: imx6: Do not hide PHY driver callbacks and refine the error handling")
That commit just refactors the codes, and doesn't change the init-sequence.
Anyway, I had tested your patch on i.MX8MM and i.MX8MP EVK boards.
Both of these two boards works fine when the NVME device is inserted.

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

Best Regards
Richard Zhu
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> b/drivers/pci/controller/dwc/pci-imx6.c
> index b5f0de455a7bd..211eb55d6d34b 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -942,12 +942,6 @@ static int imx6_pcie_host_init(struct dw_pcie_rp
> *pp)
>  		}
>  	}
> 
> -	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
> -	if (ret < 0) {
> -		dev_err(dev, "pcie deassert core reset failed: %d\n", ret);
> -		goto err_phy_off;
> -	}
> -
>  	if (imx6_pcie->phy) {
>  		ret = phy_power_on(imx6_pcie->phy);
>  		if (ret) {
> @@ -955,6 +949,13 @@ static int imx6_pcie_host_init(struct dw_pcie_rp
> *pp)
>  			goto err_phy_off;
>  		}
>  	}
> +
> +	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
> +	if (ret < 0) {
> +		dev_err(dev, "pcie deassert core reset failed: %d\n", ret);
> +		goto err_phy_off;
> +	}
> +
>  	imx6_setup_phy_mpll(imx6_pcie);
> 
>  	return 0;
> --
> 2.30.2


[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: imx6: Fix link initialisation when the phy is ref clk provider

[Bjorn Helgaas]

Would you mind rewording the subject so it says something more
specific about what the patch does?  E.g.,

  PCI: imx6: Initialize PHY before deasserting core reset

It may be that this ordering is only *required* when the PHY is the
ref clk provider, but the patch doesn't test for that; it *always*
initializes the PHY first.

On Wed, Oct 12, 2022 at 03:26:34PM +0200, Sascha Hauer wrote:
> When the phy is the reference clock provider then it must be initialised
> and powered on before the reset on the client is deasserted, otherwise
> the link will never come up. The order was changed in cf236e0c0d59.
> Restore the correct order to make the driver work again on boards where
> the phy provides the reference clock.

s/phy/PHY/ several places above

> Fixes: cf236e0c0d59 ("PCI: imx6: Do not hide PHY driver callbacks and refine the error handling")
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index b5f0de455a7bd..211eb55d6d34b 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -942,12 +942,6 @@ static int imx6_pcie_host_init(struct dw_pcie_rp *pp)
>  		}
>  	}
>  
> -	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
> -	if (ret < 0) {
> -		dev_err(dev, "pcie deassert core reset failed: %d\n", ret);
> -		goto err_phy_off;
> -	}
> -
>  	if (imx6_pcie->phy) {
>  		ret = phy_power_on(imx6_pcie->phy);
>  		if (ret) {
> @@ -955,6 +949,13 @@ static int imx6_pcie_host_init(struct dw_pcie_rp *pp)
>  			goto err_phy_off;
>  		}
>  	}
> +
> +	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
> +	if (ret < 0) {
> +		dev_err(dev, "pcie deassert core reset failed: %d\n", ret);
> +		goto err_phy_off;
> +	}
> +
>  	imx6_setup_phy_mpll(imx6_pcie);
>  
>  	return 0;
> -- 
> 2.30.2
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: imx6: Fix link initialisation when the phy is ref clk provider

[Sascha Hauer]

On Thu, Oct 13, 2022 at 09:04:50AM -0500, Bjorn Helgaas wrote:
> Would you mind rewording the subject so it says something more
> specific about what the patch does?  E.g.,
> 
>   PCI: imx6: Initialize PHY before deasserting core reset

Ok.

> 
> It may be that this ordering is only *required* when the PHY is the
> ref clk provider, but the patch doesn't test for that; it *always*
> initializes the PHY first.

Yes, it is only required when the PHY is the ref clk provider.
It shouldn't hurt though when the ref clk is generated elsewhere.

The PCI host driver doesn't know about the ref clk provider, that
knowledge is limited to the phy driver only.

Sascha

> 
> On Wed, Oct 12, 2022 at 03:26:34PM +0200, Sascha Hauer wrote:
> > When the phy is the reference clock provider then it must be initialised
> > and powered on before the reset on the client is deasserted, otherwise
> > the link will never come up. The order was changed in cf236e0c0d59.
> > Restore the correct order to make the driver work again on boards where
> > the phy provides the reference clock.
> 
> s/phy/PHY/ several places above
> 
> > Fixes: cf236e0c0d59 ("PCI: imx6: Do not hide PHY driver callbacks and refine the error handling")
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index b5f0de455a7bd..211eb55d6d34b 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -942,12 +942,6 @@ static int imx6_pcie_host_init(struct dw_pcie_rp *pp)
> >  		}
> >  	}
> >  
> > -	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
> > -	if (ret < 0) {
> > -		dev_err(dev, "pcie deassert core reset failed: %d\n", ret);
> > -		goto err_phy_off;
> > -	}
> > -
> >  	if (imx6_pcie->phy) {
> >  		ret = phy_power_on(imx6_pcie->phy);
> >  		if (ret) {
> > @@ -955,6 +949,13 @@ static int imx6_pcie_host_init(struct dw_pcie_rp *pp)
> >  			goto err_phy_off;
> >  		}
> >  	}
> > +
> > +	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
> > +	if (ret < 0) {
> > +		dev_err(dev, "pcie deassert core reset failed: %d\n", ret);
> > +		goto err_phy_off;
> > +	}
> > +
> >  	imx6_setup_phy_mpll(imx6_pcie);
> >  
> >  	return 0;
> > -- 
> > 2.30.2
> > 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel