Re: [PATCH V2] PCI: dwc: Add support to configure for ECRC
From: Rob Herring
Date: Fri Dec 11 2020 - 10:01:55 EST
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;
> >>>>>>>> +}