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 X-Spam-Level: X-Spam-Status: No, score=-16.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3EB4BC2D0E4 for ; Tue, 24 Nov 2020 21:02:32 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D22412083E for ; Tue, 24 Nov 2020 21:02:31 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="c6o9EoPq" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729246AbgKXVCb (ORCPT ); Tue, 24 Nov 2020 16:02:31 -0500 Received: from mail.kernel.org ([198.145.29.99]:35280 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729237AbgKXVCb (ORCPT ); Tue, 24 Nov 2020 16:02:31 -0500 Received: from localhost (129.sub-72-107-112.myvzw.com [72.107.112.129]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 9706A206E0; Tue, 24 Nov 2020 21:02:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1606251749; bh=Vv6gJogUgiKM0cCB+Vjosc+BGKDTw/FrAd25jTAaBq4=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=c6o9EoPqlr8xKPTXMLa0X2gzGR2WMvhH01H7jmbjyq+c7EGNPSURQ+rcOgs5Cbg2x W4YalLf8/k1lNpYGrfzrzOIzxUaLyjd8wgo+IeQ1yxlO5tK5E1ZQhk7C6O3Odm8KLL bLY/uSObwrYGlYl3CAyWpFSOqfd2YnzymUz89x9I= Date: Tue, 24 Nov 2020 15:02:28 -0600 From: Bjorn Helgaas To: Vidya Sagar Cc: Jingoo Han , "gustavo.pimentel@synopsys.com" , "lorenzo.pieralisi@arm.com" , "bhelgaas@google.com" , "amurray@thegoodpenguin.co.uk" , "robh@kernel.org" , "treding@nvidia.com" , "jonathanh@nvidia.com" , "linux-pci@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "kthota@nvidia.com" , "mmaddireddy@nvidia.com" , "sagar.tv@gmail.com" Subject: Re: [PATCH V2] PCI: dwc: Add support to configure for ECRC Message-ID: <20201124210228.GA589610@bjorn-Precision-5520> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <40a89fcd-7f8f-fd68-2a01-4008be345c32@nvidia.com> Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org On Tue, Nov 24, 2020 at 03:50:01PM +0530, Vidya Sagar wrote: > Hi Bjorn, > Please let me know if this patch needs any further modifications I'm fine with it, but of course Lorenzo will take care of it. > On 11/12/2020 10:32 PM, Vidya Sagar wrote: > > External email: Use caution opening links or attachments > > > > > > On 11/12/2020 3:59 AM, Bjorn Helgaas wrote: > > > External email: Use caution opening links or attachments > > > > > > > > > On Wed, Nov 11, 2020 at 10:21:46PM +0530, Vidya Sagar wrote: > > > > > > > > > > > > On 11/11/2020 9:57 PM, Jingoo Han wrote: > > > > > External email: Use caution opening links or attachments > > > > > > > > > > > > > > > On 11/11/20, 7:12 AM, Vidya Sagar wrote: > > > > > > > > > > > > DesignWare core has a TLP digest (TD) override bit in > > > > > > one of the control > > > > > > registers of ATU. This bit also needs to be programmed for proper ECRC > > > > > > functionality. This is currently identified as an issue > > > > > > with DesignWare > > > > > > IP version 4.90a. > > > > > > > > > > > > Signed-off-by: Vidya Sagar > > > > > > Acked-by: Bjorn Helgaas > > > > > > --- > > > > > > V2: > > > > > > * Addressed Bjorn's comments > > > > > > > > > > > >    drivers/pci/controller/dwc/pcie-designware.c | 52 > > > > > > ++++++++++++++++++-- > > > > > >    drivers/pci/controller/dwc/pcie-designware.h |  1 + > > > > > >    2 files changed, 49 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git > > > > > > a/drivers/pci/controller/dwc/pcie-designware.c > > > > > > b/drivers/pci/controller/dwc/pcie-designware.c > > > > > > index c2dea8fc97c8..ec0d13ab6bad 100644 > > > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > > > > @@ -225,6 +225,46 @@ static void > > > > > > dw_pcie_writel_ob_unroll(struct dw_pcie *pci, u32 index, > > > > > > u32 reg, > > > > > >         dw_pcie_writel_atu(pci, offset + reg, val); > > > > > >    } > > > > > > > > > > > > +static inline u32 dw_pcie_enable_ecrc(u32 val) > > > > > > > > > > What is the reason to use inline here? > > > > > > > > Actually, I wanted to move the programming part inside the > > > > respective APIs > > > > but then I wanted to give some details as well in comments so to avoid > > > > duplication, I came up with this function. But, I'm making it inline for > > > > better code optimization by compiler. > > > > > > I don't really care either way, but I'd be surprised if the compiler > > > didn't inline this all by itself even without the explicit "inline". > > I just checked it and you are right that compiler is indeed inlining it > > without explicitly mentioning 'inline'. > > I hope it is ok to leave it that way. > > > > > > > > > > > +{ > > > > > > +     /* > > > > > > +      * DesignWare core version 4.90A has this strange design issue > > > > > > +      * where the 'TD' bit in the Control register-1 of > > > > > > the ATU outbound > > > > > > +      * region acts like an override for the ECRC > > > > > > setting i.e. the presence > > > > > > +      * of TLP Digest(ECRC) in the outgoing TLPs is > > > > > > solely determined by > > > > > > +      * this bit. This is contrary to the PCIe spec > > > > > > which says that the > > > > > > +      * enablement of the ECRC is solely determined by > > > > > > the AER registers. > > > > > > +      * > > > > > > +      * Because of this, even when the ECRC is enabled through AER > > > > > > +      * registers, the transactions going through ATU > > > > > > won't have TLP Digest > > > > > > +      * as there is no way the AER sub-system could > > > > > > program the TD bit which > > > > > > +      * is specific to DesignWare core. > > > > > > +      * > > > > > > +      * The best way to handle this scenario is to program the TD bit > > > > > > +      * always. It affects only the traffic from root > > > > > > port to downstream > > > > > > +      * devices. > > > > > > +      * > > > > > > +      * At this point, > > > > > > +      * When ECRC is enabled in AER registers, > > > > > > everything works normally > > > > > > +      * When ECRC is NOT enabled in AER registers, then, > > > > > > +      * on Root Port:- TLP Digest (DWord size) gets > > > > > > appended to each packet > > > > > > +      *                even through it is not required. > > > > > > Since downstream > > > > > > +      *                TLPs are mostly for > > > > > > configuration accesses and BAR > > > > > > +      *                accesses, they are not in > > > > > > critical path and won't > > > > > > +      *                have much negative effect on the performance. > > > > > > +      * on End Point:- TLP Digest is received for > > > > > > some/all the packets coming > > > > > > +      *                from the root port. TLP Digest > > > > > > is ignored because, > > > > > > +      *                as per the PCIe Spec r5.0 v1.0 section 2.2.3 > > > > > > +      *                "TLP Digest Rules", when an > > > > > > endpoint receives TLP > > > > > > +      *                Digest when its ECRC check > > > > > > functionality is disabled > > > > > > +      *                in AER registers, received TLP > > > > > > Digest is just ignored. > > > > > > +      * Since there is no issue or error reported > > > > > > either side, best way to > > > > > > +      * handle the scenario is to program TD bit by default. > > > > > > +      */ > > > > > > + > > > > > > +     return val | PCIE_ATU_TD; > > > > > > +}