diff mbox series

[v3,1/6] PCI: imx6: Start link directly when workaround is not required

Message ID 20250328030213.1650990-2-hongxing.zhu@nxp.com (mailing list archive)
State New
Delegated to: Krzysztof Wilczyński
Headers show
Series Add some enhancements for i.MX95 PCIe | expand

Commit Message

Hongxing Zhu March 28, 2025, 3:02 a.m. UTC 
The current link setup procedure is one workaround to detect the device
behind PCIe switches on some i.MX6 platforms.

To describe more accurately, change the flag name from
IMX_PCIE_FLAG_IMX_SPEED_CHANGE to IMX_PCIE_FLAG_SPEED_CHANGE_WORKAROUND.

Start PCIe link directly when this flag is not set on i.MX7 or later
platforms to simple and speed up link training.

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

Comments

Frank Li March 28, 2025, 2:47 p.m. UTC | #1 
On Fri, Mar 28, 2025 at 11:02:08AM +0800, Richard Zhu wrote:
> The current link setup procedure is one workaround to detect the device
> behind PCIe switches on some i.MX6 platforms.
>
> To describe more accurately, change the flag name from
> IMX_PCIE_FLAG_IMX_SPEED_CHANGE to IMX_PCIE_FLAG_SPEED_CHANGE_WORKAROUND.
>
> Start PCIe link directly when this flag is not set on i.MX7 or later
> platforms to simple and speed up link training.
>
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/pci/controller/dwc/pci-imx6.c | 34 +++++++++++----------------
>  1 file changed, 14 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index c1f7904e3600..57aa777231ae 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -91,7 +91,7 @@ enum imx_pcie_variants {
>  };
>
>  #define IMX_PCIE_FLAG_IMX_PHY			BIT(0)
> -#define IMX_PCIE_FLAG_IMX_SPEED_CHANGE		BIT(1)
> +#define IMX_PCIE_FLAG_SPEED_CHANGE_WORKAROUND	BIT(1)
>  #define IMX_PCIE_FLAG_SUPPORTS_SUSPEND		BIT(2)
>  #define IMX_PCIE_FLAG_HAS_PHYDRV		BIT(3)
>  #define IMX_PCIE_FLAG_HAS_APP_RESET		BIT(4)
> @@ -860,6 +860,12 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
>  	u32 tmp;
>  	int ret;
>
> +	if (!(imx_pcie->drvdata->flags &
> +	    IMX_PCIE_FLAG_SPEED_CHANGE_WORKAROUND)) {
> +		imx_pcie_ltssm_enable(dev);
> +		return 0;
> +	}
> +
>  	/*
>  	 * Force Gen1 operation when starting the link.  In case the link is
>  	 * started in Gen2 mode, there is a possibility the devices on the
> @@ -896,22 +902,10 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
>  		dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, tmp);
>  		dw_pcie_dbi_ro_wr_dis(pci);
>
> -		if (imx_pcie->drvdata->flags &
> -		    IMX_PCIE_FLAG_IMX_SPEED_CHANGE) {
> -
> -			/*
> -			 * On i.MX7, DIRECT_SPEED_CHANGE behaves differently
> -			 * from i.MX6 family when no link speed transition
> -			 * occurs and we go Gen1 -> yep, Gen1. The difference
> -			 * is that, in such case, it will not be cleared by HW
> -			 * which will cause the following code to report false
> -			 * failure.
> -			 */
> -			ret = imx_pcie_wait_for_speed_change(imx_pcie);
> -			if (ret) {
> -				dev_err(dev, "Failed to bring link up!\n");
> -				goto err_reset_phy;
> -			}
> +		ret = imx_pcie_wait_for_speed_change(imx_pcie);
> +		if (ret) {
> +			dev_err(dev, "Failed to bring link up!\n");
> +			goto err_reset_phy;
>  		}
>
>  		/* Make sure link training is finished as well! */
> @@ -1665,7 +1659,7 @@ static const struct imx_pcie_drvdata drvdata[] = {
>  	[IMX6Q] = {
>  		.variant = IMX6Q,
>  		.flags = IMX_PCIE_FLAG_IMX_PHY |
> -			 IMX_PCIE_FLAG_IMX_SPEED_CHANGE |
> +			 IMX_PCIE_FLAG_SPEED_CHANGE_WORKAROUND |
>  			 IMX_PCIE_FLAG_BROKEN_SUSPEND |
>  			 IMX_PCIE_FLAG_SUPPORTS_SUSPEND,
>  		.dbi_length = 0x200,
> @@ -1681,7 +1675,7 @@ static const struct imx_pcie_drvdata drvdata[] = {
>  	[IMX6SX] = {
>  		.variant = IMX6SX,
>  		.flags = IMX_PCIE_FLAG_IMX_PHY |
> -			 IMX_PCIE_FLAG_IMX_SPEED_CHANGE |
> +			 IMX_PCIE_FLAG_SPEED_CHANGE_WORKAROUND |
>  			 IMX_PCIE_FLAG_SUPPORTS_SUSPEND,
>  		.gpr = "fsl,imx6q-iomuxc-gpr",
>  		.ltssm_off = IOMUXC_GPR12,
> @@ -1696,7 +1690,7 @@ static const struct imx_pcie_drvdata drvdata[] = {
>  	[IMX6QP] = {
>  		.variant = IMX6QP,
>  		.flags = IMX_PCIE_FLAG_IMX_PHY |
> -			 IMX_PCIE_FLAG_IMX_SPEED_CHANGE |
> +			 IMX_PCIE_FLAG_SPEED_CHANGE_WORKAROUND |
>  			 IMX_PCIE_FLAG_SUPPORTS_SUSPEND,
>  		.dbi_length = 0x200,
>  		.gpr = "fsl,imx6q-iomuxc-gpr",
> --
> 2.37.1
>
Manivannan Sadhasivam April 2, 2025, 6:27 a.m. UTC | #2 
On Fri, Mar 28, 2025 at 11:02:08AM +0800, Richard Zhu wrote:
> The current link setup procedure is one workaround to detect the device
> behind PCIe switches on some i.MX6 platforms.
> 
> To describe more accurately, change the flag name from
> IMX_PCIE_FLAG_IMX_SPEED_CHANGE to IMX_PCIE_FLAG_SPEED_CHANGE_WORKAROUND.
> 
> Start PCIe link directly when this flag is not set on i.MX7 or later
> platforms to simple and speed up link training.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

One observation below (not related to this change).

> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 34 +++++++++++----------------
>  1 file changed, 14 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index c1f7904e3600..57aa777231ae 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -91,7 +91,7 @@ enum imx_pcie_variants {
>  };
>  
>  #define IMX_PCIE_FLAG_IMX_PHY			BIT(0)
> -#define IMX_PCIE_FLAG_IMX_SPEED_CHANGE		BIT(1)
> +#define IMX_PCIE_FLAG_SPEED_CHANGE_WORKAROUND	BIT(1)
>  #define IMX_PCIE_FLAG_SUPPORTS_SUSPEND		BIT(2)
>  #define IMX_PCIE_FLAG_HAS_PHYDRV		BIT(3)
>  #define IMX_PCIE_FLAG_HAS_APP_RESET		BIT(4)
> @@ -860,6 +860,12 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
>  	u32 tmp;
>  	int ret;
>  
> +	if (!(imx_pcie->drvdata->flags &
> +	    IMX_PCIE_FLAG_SPEED_CHANGE_WORKAROUND)) {
> +		imx_pcie_ltssm_enable(dev);
> +		return 0;

While looking into the code, I realized that we could skip waiting for link
during the workaround in atleast one instance:

```
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 5f267dd261b5..9dbfbd9de2da 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -875,11 +875,11 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
        /* Start LTSSM. */
        imx_pcie_ltssm_enable(dev);
 
-       ret = dw_pcie_wait_for_link(pci);
-       if (ret)
-               goto err_reset_phy;
-
        if (pci->max_link_speed > 1) {
+               ret = dw_pcie_wait_for_link(pci);
+               if (ret)
+                       goto err_reset_phy;
+
                /* Allow faster modes after the link is up */
                dw_pcie_dbi_ro_wr_en(pci);
                tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
@@ -913,17 +913,10 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
                                goto err_reset_phy;
                        }
                }
-
-               /* Make sure link training is finished as well! */
-               ret = dw_pcie_wait_for_link(pci);
-               if (ret)
-                       goto err_reset_phy;
        } else {
                dev_info(dev, "Link: Only Gen1 is enabled\n");
        }
 
-       tmp = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA);
-       dev_info(dev, "Link up, Gen%i\n", tmp & PCI_EXP_LNKSTA_CLS);
        return 0;
 
 err_reset_phy:
```

dw_pcie_wait_for_link() in DWC core should serve the purpose.

- Mani
Hongxing Zhu April 2, 2025, 7:39 a.m. UTC | #3 
> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: 2025年4月2日 14:27
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: Frank Li <frank.li@nxp.com>; l.stach@pengutronix.de; lpieralisi@kernel.org;
> kw@linux.com; robh@kernel.org; bhelgaas@google.com;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; linux-pci@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; imx@lists.linux.dev;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 1/6] PCI: imx6: Start link directly when workaround is not
> required
> 
> On Fri, Mar 28, 2025 at 11:02:08AM +0800, Richard Zhu wrote:
> > The current link setup procedure is one workaround to detect the
> > device behind PCIe switches on some i.MX6 platforms.
> >
> > To describe more accurately, change the flag name from
> > IMX_PCIE_FLAG_IMX_SPEED_CHANGE to
> IMX_PCIE_FLAG_SPEED_CHANGE_WORKAROUND.
> >
> > Start PCIe link directly when this flag is not set on i.MX7 or later
> > platforms to simple and speed up link training.
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> One observation below (not related to this change).
> 
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 34
> > +++++++++++----------------
> >  1 file changed, 14 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index c1f7904e3600..57aa777231ae 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -91,7 +91,7 @@ enum imx_pcie_variants {  };
> >
> >  #define IMX_PCIE_FLAG_IMX_PHY			BIT(0)
> > -#define IMX_PCIE_FLAG_IMX_SPEED_CHANGE		BIT(1)
> > +#define IMX_PCIE_FLAG_SPEED_CHANGE_WORKAROUND	BIT(1)
> >  #define IMX_PCIE_FLAG_SUPPORTS_SUSPEND		BIT(2)
> >  #define IMX_PCIE_FLAG_HAS_PHYDRV		BIT(3)
> >  #define IMX_PCIE_FLAG_HAS_APP_RESET		BIT(4)
> > @@ -860,6 +860,12 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
> >  	u32 tmp;
> >  	int ret;
> >
> > +	if (!(imx_pcie->drvdata->flags &
> > +	    IMX_PCIE_FLAG_SPEED_CHANGE_WORKAROUND)) {
> > +		imx_pcie_ltssm_enable(dev);
> > +		return 0;
> 
> While looking into the code, I realized that we could skip waiting for link during
> the workaround in atleast one instance:
> 
> ```
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> b/drivers/pci/controller/dwc/pci-imx6.c
> index 5f267dd261b5..9dbfbd9de2da 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -875,11 +875,11 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
>         /* Start LTSSM. */
>         imx_pcie_ltssm_enable(dev);
> 
> -       ret = dw_pcie_wait_for_link(pci);
> -       if (ret)
> -               goto err_reset_phy;
> -
>         if (pci->max_link_speed > 1) {
> +               ret = dw_pcie_wait_for_link(pci);
> +               if (ret)
> +                       goto err_reset_phy;
> +
>                 /* Allow faster modes after the link is up */
>                 dw_pcie_dbi_ro_wr_en(pci);
>                 tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> @@ -913,17 +913,10 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
>                                 goto err_reset_phy;
>                         }
>                 }
> -
> -               /* Make sure link training is finished as well! */
> -               ret = dw_pcie_wait_for_link(pci);
> -               if (ret)
> -                       goto err_reset_phy;
>         } else {
>                 dev_info(dev, "Link: Only Gen1 is enabled\n");
>         }
> 
> -       tmp = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA);
> -       dev_info(dev, "Link up, Gen%i\n", tmp & PCI_EXP_LNKSTA_CLS);
>         return 0;
> 
>  err_reset_phy:
> ```
> 
> dw_pcie_wait_for_link() in DWC core should serve the purpose.
Good suggestion. Thanks.
I can add another patch to optimize the workaround procedure without
 function changes.

Best Regards
Richard Zhu
> 
> - Mani
> 
> --
> மணிவண்ணன் சதாசிவம்
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index c1f7904e3600..57aa777231ae 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -91,7 +91,7 @@  enum imx_pcie_variants {
 };
 
 #define IMX_PCIE_FLAG_IMX_PHY			BIT(0)
-#define IMX_PCIE_FLAG_IMX_SPEED_CHANGE		BIT(1)
+#define IMX_PCIE_FLAG_SPEED_CHANGE_WORKAROUND	BIT(1)
 #define IMX_PCIE_FLAG_SUPPORTS_SUSPEND		BIT(2)
 #define IMX_PCIE_FLAG_HAS_PHYDRV		BIT(3)
 #define IMX_PCIE_FLAG_HAS_APP_RESET		BIT(4)
@@ -860,6 +860,12 @@  static int imx_pcie_start_link(struct dw_pcie *pci)
 	u32 tmp;
 	int ret;
 
+	if (!(imx_pcie->drvdata->flags &
+	    IMX_PCIE_FLAG_SPEED_CHANGE_WORKAROUND)) {
+		imx_pcie_ltssm_enable(dev);
+		return 0;
+	}
+
 	/*
 	 * Force Gen1 operation when starting the link.  In case the link is
 	 * started in Gen2 mode, there is a possibility the devices on the
@@ -896,22 +902,10 @@  static int imx_pcie_start_link(struct dw_pcie *pci)
 		dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, tmp);
 		dw_pcie_dbi_ro_wr_dis(pci);
 
-		if (imx_pcie->drvdata->flags &
-		    IMX_PCIE_FLAG_IMX_SPEED_CHANGE) {
-
-			/*
-			 * On i.MX7, DIRECT_SPEED_CHANGE behaves differently
-			 * from i.MX6 family when no link speed transition
-			 * occurs and we go Gen1 -> yep, Gen1. The difference
-			 * is that, in such case, it will not be cleared by HW
-			 * which will cause the following code to report false
-			 * failure.
-			 */
-			ret = imx_pcie_wait_for_speed_change(imx_pcie);
-			if (ret) {
-				dev_err(dev, "Failed to bring link up!\n");
-				goto err_reset_phy;
-			}
+		ret = imx_pcie_wait_for_speed_change(imx_pcie);
+		if (ret) {
+			dev_err(dev, "Failed to bring link up!\n");
+			goto err_reset_phy;
 		}
 
 		/* Make sure link training is finished as well! */
@@ -1665,7 +1659,7 @@  static const struct imx_pcie_drvdata drvdata[] = {
 	[IMX6Q] = {
 		.variant = IMX6Q,
 		.flags = IMX_PCIE_FLAG_IMX_PHY |
-			 IMX_PCIE_FLAG_IMX_SPEED_CHANGE |
+			 IMX_PCIE_FLAG_SPEED_CHANGE_WORKAROUND |
 			 IMX_PCIE_FLAG_BROKEN_SUSPEND |
 			 IMX_PCIE_FLAG_SUPPORTS_SUSPEND,
 		.dbi_length = 0x200,
@@ -1681,7 +1675,7 @@  static const struct imx_pcie_drvdata drvdata[] = {
 	[IMX6SX] = {
 		.variant = IMX6SX,
 		.flags = IMX_PCIE_FLAG_IMX_PHY |
-			 IMX_PCIE_FLAG_IMX_SPEED_CHANGE |
+			 IMX_PCIE_FLAG_SPEED_CHANGE_WORKAROUND |
 			 IMX_PCIE_FLAG_SUPPORTS_SUSPEND,
 		.gpr = "fsl,imx6q-iomuxc-gpr",
 		.ltssm_off = IOMUXC_GPR12,
@@ -1696,7 +1690,7 @@  static const struct imx_pcie_drvdata drvdata[] = {
 	[IMX6QP] = {
 		.variant = IMX6QP,
 		.flags = IMX_PCIE_FLAG_IMX_PHY |
-			 IMX_PCIE_FLAG_IMX_SPEED_CHANGE |
+			 IMX_PCIE_FLAG_SPEED_CHANGE_WORKAROUND |
 			 IMX_PCIE_FLAG_SUPPORTS_SUSPEND,
 		.dbi_length = 0x200,
 		.gpr = "fsl,imx6q-iomuxc-gpr",