> -----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
>
> --
> மணிவண்ணன் சதாசிவம்
