On Fri, Dec 11, 2020 at 08:49:16AM -0600, Rob Herring wrote:
> On Fri, Dec 11, 2020 at 7:58 AM Vidya Sagar <vid...@nvidia.com> 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-vid...@nvidia.com/
>
> 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 <vid...@nvidia.com>
> > >>>>>>>> Acked-by: Bjorn Helgaas <bhelg...@google.com>
> > >>>>>>>> ---
> > >>>>>>>> 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;
> > >>>>>>>> +}
