Re: [PATCH V2] PCI: dwc: Add support to configure for ECRC

[Rob Herring]

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;
> >>>>>>>> +}