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

[Hongxing Zhu]

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> Sent: 2025年4月2日 14:27
> To: Hongxing Zhu <hongxing.zhu@xxxxxxx>
> Cc: Frank Li <frank.li@xxxxxxx>; l.stach@xxxxxxxxxxxxxx; lpieralisi@xxxxxxxxxx;
> kw@xxxxxxxxx; robh@xxxxxxxxxx; bhelgaas@xxxxxxxxxx;
> shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx;
> festevam@xxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxx>
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> 
> 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
> 
> --
> மணிவண்ணன் சதாசிவம்