On Fri, Dec 11, 2020 at 08:49:16AM -0600, Rob Herring wrote: > On Fri, Dec 11, 2020 at 7:58 AM Vidya Sagar <vidyas@xxxxxxxxxx> wrote: > > > > Hi Lorenzo, > > Apologies to bug you, but wondering if you have any further comments on > > this patch that I need to take care of? > > You can check the status of your patches in Patchwork: > > https://patchwork.kernel.org/project/linux-pci/patch/20201111121145.7015-1-vidyas@xxxxxxxxxx/ > > If it's in 'New' state and delegated to Lorenzo or Bjorn, it's in > their queue. You can shorten the queue by reviewing stuff in front of > you. :) Yes that's right. There are a couple of patches pending ahead, if this one can be rebased against my pci/dwc branch and resent I can apply it. Thanks, Lorenzo > > Rob > > > > > Thanks, > > Vidya Sagar > > > > On 12/3/2020 5:40 PM, Vidya Sagar wrote: > > > > > > > > > On 11/25/2020 2:32 AM, Bjorn Helgaas wrote: > > >> External email: Use caution opening links or attachments > > >> > > >> > > >> 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. > > > Thanks Bjorn. > > > > > > Hi Lorenzo, > > > Please let me know if you have any comments for this patch. > > > > > > Thanks, > > > Vidya Sagar > > > > > >> > > >>> 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 <vidyas@xxxxxxxxxx> > > >>>>>>>> Acked-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> > > >>>>>>>> --- > > >>>>>>>> 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; > > >>>>>>>> +}
