| Message ID | 20201111121145.7015-1-vidyas@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V2] PCI: dwc: Add support to configure for ECRC | expand |
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@nvidia.com> > Acked-by: Bjorn Helgaas <bhelgaas@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? Best regards, Jingoo Han > +{ > + /* > + * 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; > +} > + > static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, > int index, int type, > u64 cpu_addr, u64 pci_addr, > @@ -245,8 +285,10 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, > lower_32_bits(pci_addr)); > dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET, > upper_32_bits(pci_addr)); > - dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, > - type | PCIE_ATU_FUNC_NUM(func_no)); > + val = type | PCIE_ATU_FUNC_NUM(func_no); > + if (pci->version == 0x490A) > + val = dw_pcie_enable_ecrc(val); > + dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val); > dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2, > PCIE_ATU_ENABLE); > > @@ -292,8 +334,10 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no, > lower_32_bits(pci_addr)); > dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_TARGET, > upper_32_bits(pci_addr)); > - dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, type | > - PCIE_ATU_FUNC_NUM(func_no)); > + val = type | PCIE_ATU_FUNC_NUM(func_no); > + if (pci->version == 0x490A) > + val = dw_pcie_enable_ecrc(val); > + dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, val); > dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, PCIE_ATU_ENABLE); > > /* > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 9d2f511f13fa..285c0ae364ae 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -88,6 +88,7 @@ > #define PCIE_ATU_TYPE_IO 0x2 > #define PCIE_ATU_TYPE_CFG0 0x4 > #define PCIE_ATU_TYPE_CFG1 0x5 > +#define PCIE_ATU_TD BIT(8) > #define PCIE_ATU_FUNC_NUM(pf) ((pf) << 20) > #define PCIE_ATU_CR2 0x908 > #define PCIE_ATU_ENABLE BIT(31) > -- > 2.17.1
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@nvidia.com> >> Acked-by: Bjorn Helgaas <bhelgaas@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. > > > Best regards, > Jingoo Han > >> +{ >> + /* >> + * 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; >> +} >> + >> static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, >> int index, int type, >> u64 cpu_addr, u64 pci_addr, >> @@ -245,8 +285,10 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, >> lower_32_bits(pci_addr)); >> dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET, >> upper_32_bits(pci_addr)); >> - dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, >> - type | PCIE_ATU_FUNC_NUM(func_no)); >> + val = type | PCIE_ATU_FUNC_NUM(func_no); >> + if (pci->version == 0x490A) >> + val = dw_pcie_enable_ecrc(val); >> + dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val); >> dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2, >> PCIE_ATU_ENABLE); >> >> @@ -292,8 +334,10 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no, >> lower_32_bits(pci_addr)); >> dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_TARGET, >> upper_32_bits(pci_addr)); >> - dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, type | >> - PCIE_ATU_FUNC_NUM(func_no)); >> + val = type | PCIE_ATU_FUNC_NUM(func_no); >> + if (pci->version == 0x490A) >> + val = dw_pcie_enable_ecrc(val); >> + dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, val); >> dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, PCIE_ATU_ENABLE); >> >> /* >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h >> index 9d2f511f13fa..285c0ae364ae 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.h >> +++ b/drivers/pci/controller/dwc/pcie-designware.h >> @@ -88,6 +88,7 @@ >> #define PCIE_ATU_TYPE_IO 0x2 >> #define PCIE_ATU_TYPE_CFG0 0x4 >> #define PCIE_ATU_TYPE_CFG1 0x5 >> +#define PCIE_ATU_TD BIT(8) >> #define PCIE_ATU_FUNC_NUM(pf) ((pf) << 20) >> #define PCIE_ATU_CR2 0x908 >> #define PCIE_ATU_ENABLE BIT(31) >> -- >> 2.17.1 >
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@nvidia.com> > > > Acked-by: Bjorn Helgaas <bhelgaas@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". > > > +{ > > > + /* > > > + * 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; > > > +}
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@nvidia.com> >>>> Acked-by: Bjorn Helgaas <bhelgaas@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; >>>> +}
Hi Bjorn, Please let me know if this patch needs any further modifications 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@nvidia.com> >>>>> Acked-by: Bjorn Helgaas <bhelgaas@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; >>>>> +}
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. > 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@nvidia.com> > > > > > > Acked-by: Bjorn Helgaas <bhelgaas@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; > > > > > > +}
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@nvidia.com> >>>>>>> Acked-by: Bjorn Helgaas <bhelgaas@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; >>>>>>> +}
Hi Lorenzo, Apologies to bug you, but wondering if you have any further comments on this patch that I need to take care of? 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@nvidia.com> >>>>>>>> Acked-by: Bjorn Helgaas <bhelgaas@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; >>>>>>>> +}
On Fri, Dec 11, 2020 at 7:58 AM Vidya Sagar <vidyas@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-vidyas@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. :) 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@nvidia.com> > >>>>>>>> Acked-by: Bjorn Helgaas <bhelgaas@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; > >>>>>>>> +}
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@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-vidyas@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 <vidyas@nvidia.com> > > >>>>>>>> Acked-by: Bjorn Helgaas <bhelgaas@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; > > >>>>>>>> +}
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@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-vidyas@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. :) Thanks for pointing this out, this is exactly right. I *love* it when people help review things. Obviously it saves me time, and often it raises questions I would have missed. Even "trivial" things like spelling, grammar, whitespace, subject line formats, etc. help because they are distractions to me and I will comment on them or spend time fixing them myself. That doesn't mean "repost immediately after somebody points out a typo"; it means "collect feedback for a week or so, fix everything, *then* repost." This is an old but still relevant list of some of my OCD tendencies that take no special expertise to review for: https://lore.kernel.org/linux-pci/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com/
On Fri, Dec 11, 2020 at 10:54 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > 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@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-vidyas@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. :) > > Thanks for pointing this out, this is exactly right. I *love* it when > people help review things. Obviously it saves me time, and often it > raises questions I would have missed. I can't take credit, this was actually someone's strategy for getting their patches reviewed I heard about one time. > Even "trivial" things like spelling, grammar, whitespace, subject line > formats, etc. help because they are distractions to me and I will > comment on them or spend time fixing them myself. That doesn't mean > "repost immediately after somebody points out a typo"; it means > "collect feedback for a week or so, fix everything, *then* repost." > > This is an old but still relevant list of some of my OCD tendencies > that take no special expertise to review for: > https://lore.kernel.org/linux-pci/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com/ It's been on my wish list for checkpatch.pl do something like: "Commit subject should begin $(git log --oneline $files | cut -d':' -f-3 | sort | uniq -c | sort -nr | head -1)". Or maybe a git commit template to fill in subject prefix and Cc list. The hard part is of course all the corner cases such as touching multiple directories/files. Rob
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) +{ + /* + * 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; +} + static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, int index, int type, u64 cpu_addr, u64 pci_addr, @@ -245,8 +285,10 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, lower_32_bits(pci_addr)); dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET, upper_32_bits(pci_addr)); - dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, - type | PCIE_ATU_FUNC_NUM(func_no)); + val = type | PCIE_ATU_FUNC_NUM(func_no); + if (pci->version == 0x490A) + val = dw_pcie_enable_ecrc(val); + dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val); dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2, PCIE_ATU_ENABLE); @@ -292,8 +334,10 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no, lower_32_bits(pci_addr)); dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_TARGET, upper_32_bits(pci_addr)); - dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, type | - PCIE_ATU_FUNC_NUM(func_no)); + val = type | PCIE_ATU_FUNC_NUM(func_no); + if (pci->version == 0x490A) + val = dw_pcie_enable_ecrc(val); + dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, val); dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, PCIE_ATU_ENABLE); /* diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index 9d2f511f13fa..285c0ae364ae 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -88,6 +88,7 @@ #define PCIE_ATU_TYPE_IO 0x2 #define PCIE_ATU_TYPE_CFG0 0x4 #define PCIE_ATU_TYPE_CFG1 0x5 +#define PCIE_ATU_TD BIT(8) #define PCIE_ATU_FUNC_NUM(pf) ((pf) << 20) #define PCIE_ATU_CR2 0x908 #define PCIE_ATU_ENABLE BIT(31)