Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 8 Jun 2023 21:12:06 +0530
From:   Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To:     Siddharth Vadapalli <s-vadapalli@ti.com>
Cc:     tjoseph@cadence.com, lpieralisi@kernel.org, robh@kernel.org,
        kw@linux.com, bhelgaas@google.com, nadeem@cadence.com,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, vigneshr@ti.com, srk@ti.com,
        nm@ti.com
Subject: Re: [PATCH v3] PCI: cadence: Fix Gen2 Link Retraining process
Message-ID: <20230608154206.GI5672@thinkpad>
References: <20230607091427.852473-1-s-vadapalli@ti.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230607091427.852473-1-s-vadapalli@ti.com>
Precedence: bulk

On Wed, Jun 07, 2023 at 02:44:27PM +0530, Siddharth Vadapalli wrote:
> The Link Retraining process is initiated to account for the Gen2 defect in
> the Cadence PCIe controller in J721E SoC. The errata corresponding to this
> is i2085, documented at:
> https://www.ti.com/lit/er/sprz455c/sprz455c.pdf
> 
> The existing workaround implemented for the errata waits for the Data Link
> initialization to complete and assumes that the link retraining process
> at the Physical Layer has completed. However, it is possible that the
> Physical Layer training might be ongoing as indicated by the
> PCI_EXP_LNKSTA_LT bit in the PCI_EXP_LNKSTA register.
> 
> Fix the existing workaround, to ensure that the Physical Layer training
> has also completed, in addition to the Data Link initialization.
> 

cdns_pcie_host_wait_for_link() function is called even for the non-quirky cases
as well, so does this patch. But if your patch is only targeting the link
retraining case, you should move the logic to cdns_pcie_retrain().


> Fixes: 4740b969aaf5 ("PCI: cadence: Retrain Link to work around Gen2 training defect")
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> Reviewed-by: Vignesh Raghavendra <vigneshr@ti.com>
> ---
> 
> Hello,
> 
> This patch is based on linux-next tagged next-20230606.
> 
> v2:
> https://lore.kernel.org/r/20230315070800.1615527-1-s-vadapalli@ti.com/
> Changes since v2:
> - Merge the cdns_pcie_host_training_complete() function with the
>   cdns_pcie_host_wait_for_link() function, as suggested by Bjorn
>   for the v2 patch.
> - Add dev_err() to notify when Link Training fails, since this is a
>   fatal error and proceeding from this point will almost always crash
>   the kernel.
> 
> v1:
> https://lore.kernel.org/r/20230102075656.260333-1-s-vadapalli@ti.com/
> Changes since v1:
> - Collect Reviewed-by tag from Vignesh Raghavendra.
> - Rebase on next-20230315.
> 
> Regards,
> Siddharth.
> 
>  .../controller/cadence/pcie-cadence-host.c    | 20 +++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index 940c7dd701d6..70a5f581ff4f 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -12,6 +12,8 @@
>  
>  #include "pcie-cadence.h"
>  
> +#define LINK_RETRAIN_TIMEOUT HZ
> +
>  static u64 bar_max_size[] = {
>  	[RP_BAR0] = _ULL(128 * SZ_2G),
>  	[RP_BAR1] = SZ_2G,
> @@ -80,8 +82,26 @@ static struct pci_ops cdns_pcie_host_ops = {
>  static int cdns_pcie_host_wait_for_link(struct cdns_pcie *pcie)
>  {
>  	struct device *dev = pcie->dev;
> +	unsigned long end_jiffies;
> +	u16 link_status;
>  	int retries;
>  
> +	/* Wait for link training to complete */
> +	end_jiffies = jiffies + LINK_RETRAIN_TIMEOUT;
> +	do {
> +		link_status = cdns_pcie_rp_readw(pcie, CDNS_PCIE_RP_CAP_OFFSET + PCI_EXP_LNKSTA);
> +		if (!(link_status & PCI_EXP_LNKSTA_LT))
> +			break;
> +		usleep_range(0, 1000);
> +	} while (time_before(jiffies, end_jiffies));
> +
> +	if (!(link_status & PCI_EXP_LNKSTA_LT)) {
> +		dev_info(dev, "Link training complete\n");

This info is not needed.

> +	} else {
> +		dev_err(dev, "Fatal! Link training incomplete\n");

This could be, "Link retraining incomplete".

- Mani

> +		return -ETIMEDOUT;
> +	}
> +
>  	/* Check if the link is up or not */
>  	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
>  		if (cdns_pcie_link_up(pcie)) {
> -- 
> 2.25.1
> 

-- 
மணிவண்ணன் சதாசிவம்