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. :) 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; > >>>>>>>> +}