From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 2E2A1C7EE2E for ; Fri, 9 Jun 2023 17:40:12 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:References: List-Owner; bh=tuIGqQoiLL9f4EC4ejPFOBKQosBlu1Pdi/57sCQ4OvQ=; b=AbEaawi010Bbg9 ZVU+6N1UIAaDnLfD/M+iGEnDvl4UuI1DaWd4Gx+LkSkyDULvMEWu+wO2BdNRQyqPn6Lg/UUfB2dFx DlmxUvBbuirz2jx5FfC/1mr3Xqdpbk0qGSh00qg+j6ALegqax65fPDh5nguY1OeO7skw3Peb2qchQ wy+ew9q7ruBta2s+HiuiUrqEitxyG3j3lNTTOAoKaKBA933QrliI/4lcNctYArV+wbvbGal8xw7ur nCLJ/naD7kM/Zpa+Z4yHof79y4Q6G5e1/wTsWHIjGKhP2tnZQsOqyf4CpdgEmzqCh6i4qH4+yJHVI AQg1eouyslcuvkGTdcsA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1q7g5T-00Dkmg-0x; Fri, 09 Jun 2023 17:39:47 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1q7g5Q-00DkmB-2J for linux-arm-kernel@lists.infradead.org; Fri, 09 Jun 2023 17:39:46 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id A043565A6F; Fri, 9 Jun 2023 17:39:43 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 8FC02C433D2; Fri, 9 Jun 2023 17:39:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1686332383; bh=BjBmi5dvVDfL4eANifiLaJ+b0e1ynq0rCl7TNY5H4N0=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=Z8kWRYHlLhrnNe4Jk1duaUAYu2RvNE9vnJfWWl7S84jq3lSfVChTV8WfIhWZXAa4h B5xcBvwP9ZGSnznI8aU4SoG69k5uK92JLJSHujMBLukkKgYPbQ226cX70cYHwhJc5c fjlmJtrZBknwi53/WPaDSS6Ov3npJs0WK9+v5bYwiuXvJvBuayYUuTmAcRNmsxOkUu FGyKGiDt7+abWdUlQmtBSTHdthAgwMUG8ENTr6WN0xeRMYlXSsj+QcuFYlrxw0cyEj HnHRCZmZypsbQ1trGqquC8cdK0XDtCoRbaDTA+dW+/9Y7WZYFNqnBevXv5DaXaO33C FzL4nvCknUHiw== Date: Fri, 9 Jun 2023 12:39:40 -0500 From: Bjorn Helgaas To: Siddharth Vadapalli 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: <20230609173940.GA1252506@bhelgaas> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230607091427.852473-1-s-vadapalli@ti.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230609_103944_855115_E7772412 X-CRM114-Status: GOOD ( 32.58 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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. > > Fixes: 4740b969aaf5 ("PCI: cadence: Retrain Link to work around Gen2 training defect") > Signed-off-by: Siddharth Vadapalli > Reviewed-by: Vignesh Raghavendra > --- > > 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"); > + } else { > + dev_err(dev, "Fatal! Link training incomplete\n"); > + return -ETIMEDOUT; > + } Can I have a brown paper bag, please? I totally blew it here, and I'm sorry. You took my advice by combining this with the existing cdns_pcie_host_wait_for_link(), but I think my advice was poor because (a) now this additional wait is not clearly connected with the erratum, and (b) it affects devices that don't have the erratum. IIUC, this is all part of a workaround for the i2085 erratum. The original workaround, 4740b969aaf5 ("PCI: cadence: Retrain Link to work around Gen2 training defect"), added this: if (!ret && rc->quirk_retrain_flag) ret = cdns_pcie_retrain(pcie); I think the wait for link train to complete should also be in cdns_pcie_retrain() so it's clearly connected with the quirk, which also means we'd only do the wait for devices with the erratum. Which is EXACTLY what your first patch did, and I missed it. I am very sorry. I guess maybe I thought cdns_pcie_retrain() was a general-purpose thing, but in fact it's only used for this quirk. Bjorn _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel