* [PATCH V3 0/2] Add support to configure DWC for ECRC @ 2020-10-29 5:39 Vidya Sagar 2020-10-29 5:39 ` [PATCH V3 1/2] PCI/AER: Add pcie_is_ecrc_enabled() API Vidya Sagar 2020-10-29 5:39 ` [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC Vidya Sagar 0 siblings, 2 replies; 17+ messages in thread From: Vidya Sagar @ 2020-10-29 5:39 UTC (permalink / raw) To: jingoohan1, gustavo.pimentel, lorenzo.pieralisi, bhelgaas, amurray, robh, treding, jonathanh Cc: linux-pci, linux-kernel, kthota, mmaddireddy, vidyas, sagar.tv This series has two patches. Patch-1: Adds a public API to query if the system has ECRC policty turned on. Patch-2: DesignWare core PCIe IP has a TLP Digest (TD) override bit in one of its control registers of ATU. This bit needs to be programmed for proper ECRC functionality. This is currently identified as an issue with DesignWare IP version 4.90a. DWC code queries the PCIe sub-system through the API added in Patch-1 to find out if ECRC is turned on or not and configures ATU accordingly. V3: * Addressed Ethan Zhao's comments for patch-1 V2: * Addressed Jingoo's review comments Vidya Sagar (2): PCI/AER: Add pcie_is_ecrc_enabled() API PCI: dwc: Add support to configure for ECRC drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++-- drivers/pci/controller/dwc/pcie-designware.h | 1 + drivers/pci/pci.h | 2 ++ drivers/pci/pcie/aer.c | 11 +++++++++++ 4 files changed, 20 insertions(+), 2 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH V3 1/2] PCI/AER: Add pcie_is_ecrc_enabled() API 2020-10-29 5:39 [PATCH V3 0/2] Add support to configure DWC for ECRC Vidya Sagar @ 2020-10-29 5:39 ` Vidya Sagar 2020-10-29 5:39 ` [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC Vidya Sagar 1 sibling, 0 replies; 17+ messages in thread From: Vidya Sagar @ 2020-10-29 5:39 UTC (permalink / raw) To: jingoohan1, gustavo.pimentel, lorenzo.pieralisi, bhelgaas, amurray, robh, treding, jonathanh Cc: linux-pci, linux-kernel, kthota, mmaddireddy, vidyas, sagar.tv Adds pcie_is_ecrc_enabled() API to let other sub-systems (like DesignWare) to query if ECRC policy is enabled and perform any configuration required in those respective sub-systems. Signed-off-by: Vidya Sagar <vidyas@nvidia.com> Reviewed-by: Jingoo Han <jingoohan1@gmail.com> --- V3: * Address Ethan Zhao's comments * Added 'Reviewed-by: Jingoo Han <jingoohan1@gmail.com>' V2: * None from V1 drivers/pci/pci.h | 2 ++ drivers/pci/pcie/aer.c | 11 +++++++++++ 2 files changed, 13 insertions(+) diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index fa12f7cbc1a0..325fdbf91dde 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -575,9 +575,11 @@ static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { } #ifdef CONFIG_PCIE_ECRC void pcie_set_ecrc_checking(struct pci_dev *dev); void pcie_ecrc_get_policy(char *str); +bool pcie_is_ecrc_enabled(void); #else static inline void pcie_set_ecrc_checking(struct pci_dev *dev) { } static inline void pcie_ecrc_get_policy(char *str) { } +static inline bool pcie_is_ecrc_enabled(void) { return false; } #endif #ifdef CONFIG_PCIE_PTM diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c index 65dff5f3457a..d0f5a7043aff 100644 --- a/drivers/pci/pcie/aer.c +++ b/drivers/pci/pcie/aer.c @@ -207,6 +207,17 @@ void pcie_ecrc_get_policy(char *str) ecrc_policy = i; } + +/** + * pcie_is_ecrc_enabled - returns if ECRC is enabled in the system or not + * + * Returns true if ECRC policy is enabled and false otherwise + */ +bool pcie_is_ecrc_enabled(void) +{ + return ecrc_policy == ECRC_POLICY_ON; +} +EXPORT_SYMBOL(pcie_is_ecrc_enabled); #endif /* CONFIG_PCIE_ECRC */ #define PCI_EXP_AER_FLAGS (PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \ -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC 2020-10-29 5:39 [PATCH V3 0/2] Add support to configure DWC for ECRC Vidya Sagar 2020-10-29 5:39 ` [PATCH V3 1/2] PCI/AER: Add pcie_is_ecrc_enabled() API Vidya Sagar @ 2020-10-29 5:39 ` Vidya Sagar 2020-10-29 22:03 ` Jingoo Han ` (2 more replies) 1 sibling, 3 replies; 17+ messages in thread From: Vidya Sagar @ 2020-10-29 5:39 UTC (permalink / raw) To: jingoohan1, gustavo.pimentel, lorenzo.pieralisi, bhelgaas, amurray, robh, treding, jonathanh Cc: linux-pci, linux-kernel, kthota, mmaddireddy, vidyas, sagar.tv 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. This patch does the required programming in ATU upon querying the system policy for ECRC. Signed-off-by: Vidya Sagar <vidyas@nvidia.com> Reviewed-by: Jingoo Han <jingoohan1@gmail.com> --- V3: * Added 'Reviewed-by: Jingoo Han <jingoohan1@gmail.com>' V2: * Addressed Jingoo's review comment * Removed saving 'td' bit information in 'dw_pcie' structure drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++-- drivers/pci/controller/dwc/pcie-designware.h | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c index b5e438b70cd5..cbd651b219d2 100644 --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -246,6 +246,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET, upper_32_bits(pci_addr)); val = type | PCIE_ATU_FUNC_NUM(func_no); + if (pci->version == 0x490A) + val = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; val = upper_32_bits(size - 1) ? val | PCIE_ATU_INCREASE_REGION_SIZE : val; dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val); @@ -294,8 +296,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 = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; + 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 e7f441441db2..b01ef407fd52 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -89,6 +89,7 @@ #define PCIE_ATU_TYPE_IO 0x2 #define PCIE_ATU_TYPE_CFG0 0x4 #define PCIE_ATU_TYPE_CFG1 0x5 +#define PCIE_ATU_TD_SHIFT 8 #define PCIE_ATU_FUNC_NUM(pf) ((pf) << 20) #define PCIE_ATU_CR2 0x908 #define PCIE_ATU_ENABLE BIT(31) -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC 2020-10-29 5:39 ` [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC Vidya Sagar @ 2020-10-29 22:03 ` Jingoo Han 2020-10-30 6:45 ` Vidya Sagar 2020-11-02 14:15 ` Rob Herring 2020-11-02 23:02 ` Bjorn Helgaas 2 siblings, 1 reply; 17+ messages in thread From: Jingoo Han @ 2020-10-29 22:03 UTC (permalink / raw) To: Vidya Sagar, gustavo.pimentel, lorenzo.pieralisi, bhelgaas, amurray, robh, treding, jonathanh Cc: linux-pci, linux-kernel, kthota, mmaddireddy, sagar.tv, Han Jingoo On 10/29/20, 1:40 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. This patch does the required programming in ATU upon > querying the system policy for ECRC. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > Reviewed-by: Jingoo Han <jingoohan1@gmail.com> No, it should be Acked-by. I gave you Acked-by, not Reviewed-by. Acked-by: Jingoo Han <jingoohan1@gmail.com> Best regards, Jingoo Han > --- > V3: > * Added 'Reviewed-by: Jingoo Han <jingoohan1@gmail.com>' > > V2: > * Addressed Jingoo's review comment > * Removed saving 'td' bit information in 'dw_pcie' structure > > drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++-- > drivers/pci/controller/dwc/pcie-designware.h | 1 + > 2 files changed, 7 insertions(+), 2 deletions(-) [...] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC 2020-10-29 22:03 ` Jingoo Han @ 2020-10-30 6:45 ` Vidya Sagar 0 siblings, 0 replies; 17+ messages in thread From: Vidya Sagar @ 2020-10-30 6:45 UTC (permalink / raw) To: Jingoo Han, gustavo.pimentel, lorenzo.pieralisi, bhelgaas, amurray, robh, treding, jonathanh Cc: linux-pci, linux-kernel, kthota, mmaddireddy, sagar.tv On 10/30/2020 3:33 AM, Jingoo Han wrote: > External email: Use caution opening links or attachments > > > On 10/29/20, 1:40 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. This patch does the required programming in ATU upon >> querying the system policy for ECRC. >> >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >> Reviewed-by: Jingoo Han <jingoohan1@gmail.com> > > No, it should be Acked-by. I gave you Acked-by, not Reviewed-by. > > Acked-by: Jingoo Han <jingoohan1@gmail.com> Apologies. My bad. I saw the 'Reviewed-by' of the other patch (i.e. PCI/AER: Add pcie_is_ecrc_enabled() API) and put that for this patch as well. I'll update. > > > Best regards, > Jingoo Han > >> --- >> V3: >> * Added 'Reviewed-by: Jingoo Han <jingoohan1@gmail.com>' >> >> V2: >> * Addressed Jingoo's review comment >> * Removed saving 'td' bit information in 'dw_pcie' structure >> >> drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++-- >> drivers/pci/controller/dwc/pcie-designware.h | 1 + >> 2 files changed, 7 insertions(+), 2 deletions(-) > > [...] > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC 2020-10-29 5:39 ` [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC Vidya Sagar 2020-10-29 22:03 ` Jingoo Han @ 2020-11-02 14:15 ` Rob Herring 2020-11-02 14:27 ` Vidya Sagar 2020-11-02 23:02 ` Bjorn Helgaas 2 siblings, 1 reply; 17+ messages in thread From: Rob Herring @ 2020-11-02 14:15 UTC (permalink / raw) To: Vidya Sagar Cc: Jingoo Han, Gustavo Pimentel, Lorenzo Pieralisi, Bjorn Helgaas, Andrew Murray, Thierry Reding, Jon Hunter, PCI, linux-kernel, kthota, Manikanta Maddireddy, sagar.tv On Thu, Oct 29, 2020 at 12:40 AM Vidya Sagar <vidyas@nvidia.com> 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. This patch does the required programming in ATU upon > querying the system policy for ECRC. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > Reviewed-by: Jingoo Han <jingoohan1@gmail.com> > --- > V3: > * Added 'Reviewed-by: Jingoo Han <jingoohan1@gmail.com>' > > V2: > * Addressed Jingoo's review comment > * Removed saving 'td' bit information in 'dw_pcie' structure > > drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++-- > drivers/pci/controller/dwc/pcie-designware.h | 1 + > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > index b5e438b70cd5..cbd651b219d2 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -246,6 +246,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, > dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET, > upper_32_bits(pci_addr)); > val = type | PCIE_ATU_FUNC_NUM(func_no); > + if (pci->version == 0x490A) > + val = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; > val = upper_32_bits(size - 1) ? > val | PCIE_ATU_INCREASE_REGION_SIZE : val; > dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val); > @@ -294,8 +296,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) Is this even possible? Are the non-unroll ATU registers available post 4.80? Rob ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC 2020-11-02 14:15 ` Rob Herring @ 2020-11-02 14:27 ` Vidya Sagar 2020-11-02 15:11 ` Gustavo Pimentel 0 siblings, 1 reply; 17+ messages in thread From: Vidya Sagar @ 2020-11-02 14:27 UTC (permalink / raw) To: Rob Herring Cc: Jingoo Han, Gustavo Pimentel, Lorenzo Pieralisi, Bjorn Helgaas, Andrew Murray, Thierry Reding, Jon Hunter, PCI, linux-kernel, kthota, Manikanta Maddireddy, sagar.tv On 11/2/2020 7:45 PM, Rob Herring wrote: > External email: Use caution opening links or attachments > > > On Thu, Oct 29, 2020 at 12:40 AM Vidya Sagar <vidyas@nvidia.com> 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. This patch does the required programming in ATU upon >> querying the system policy for ECRC. >> >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >> Reviewed-by: Jingoo Han <jingoohan1@gmail.com> >> --- >> V3: >> * Added 'Reviewed-by: Jingoo Han <jingoohan1@gmail.com>' >> >> V2: >> * Addressed Jingoo's review comment >> * Removed saving 'td' bit information in 'dw_pcie' structure >> >> drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++-- >> drivers/pci/controller/dwc/pcie-designware.h | 1 + >> 2 files changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c >> index b5e438b70cd5..cbd651b219d2 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.c >> +++ b/drivers/pci/controller/dwc/pcie-designware.c >> @@ -246,6 +246,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, >> dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET, >> upper_32_bits(pci_addr)); >> val = type | PCIE_ATU_FUNC_NUM(func_no); >> + if (pci->version == 0x490A) >> + val = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; >> val = upper_32_bits(size - 1) ? >> val | PCIE_ATU_INCREASE_REGION_SIZE : val; >> dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val); >> @@ -294,8 +296,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) > > Is this even possible? Are the non-unroll ATU registers available post 4.80? I'm not sure. Gustavo might have information about this. I made this change so that it is taken care off even if they available. > > Rob > ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC 2020-11-02 14:27 ` Vidya Sagar @ 2020-11-02 15:11 ` Gustavo Pimentel 2020-11-02 21:16 ` Rob Herring 0 siblings, 1 reply; 17+ messages in thread From: Gustavo Pimentel @ 2020-11-02 15:11 UTC (permalink / raw) To: Vidya Sagar, Rob Herring Cc: Jingoo Han, Lorenzo Pieralisi, Bjorn Helgaas, Andrew Murray, Thierry Reding, Jon Hunter, PCI, linux-kernel, kthota, Manikanta Maddireddy, sagar.tv On Mon, Nov 2, 2020 at 14:27:9, Vidya Sagar <vidyas@nvidia.com> wrote: > > > On 11/2/2020 7:45 PM, Rob Herring wrote: > > External email: Use caution opening links or attachments > > > > > > On Thu, Oct 29, 2020 at 12:40 AM Vidya Sagar <vidyas@nvidia.com> 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. This patch does the required programming in ATU upon > >> querying the system policy for ECRC. > >> > >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > >> Reviewed-by: Jingoo Han <jingoohan1@gmail.com> > >> --- > >> V3: > >> * Added 'Reviewed-by: Jingoo Han <jingoohan1@gmail.com>' > >> > >> V2: > >> * Addressed Jingoo's review comment > >> * Removed saving 'td' bit information in 'dw_pcie' structure > >> > >> drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++-- > >> drivers/pci/controller/dwc/pcie-designware.h | 1 + > >> 2 files changed, 7 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > >> index b5e438b70cd5..cbd651b219d2 100644 > >> --- a/drivers/pci/controller/dwc/pcie-designware.c > >> +++ b/drivers/pci/controller/dwc/pcie-designware.c > >> @@ -246,6 +246,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, > >> dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET, > >> upper_32_bits(pci_addr)); > >> val = type | PCIE_ATU_FUNC_NUM(func_no); > >> + if (pci->version == 0x490A) > >> + val = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; > >> val = upper_32_bits(size - 1) ? > >> val | PCIE_ATU_INCREASE_REGION_SIZE : val; > >> dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val); > >> @@ -294,8 +296,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) > > > > Is this even possible? Are the non-unroll ATU registers available post 4.80? > I'm not sure. Gustavo might have information about this. I made this > change so that it is taken care off even if they available. The Synopsys DesignWare PCIe IP is highly configurable, therefore is dependable on what the design team has configured for their solution. Although Synopsys doesn't recommend the use of non-unroll ATU, the customers are free to select what they want for their design. -Gustavo > > > > > Rob > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC 2020-11-02 15:11 ` Gustavo Pimentel @ 2020-11-02 21:16 ` Rob Herring 2020-11-02 22:38 ` Gustavo Pimentel 0 siblings, 1 reply; 17+ messages in thread From: Rob Herring @ 2020-11-02 21:16 UTC (permalink / raw) To: Gustavo Pimentel Cc: Vidya Sagar, Jingoo Han, Lorenzo Pieralisi, Bjorn Helgaas, Andrew Murray, Thierry Reding, Jon Hunter, PCI, linux-kernel, kthota, Manikanta Maddireddy, sagar.tv On Mon, Nov 2, 2020 at 9:12 AM Gustavo Pimentel <Gustavo.Pimentel@synopsys.com> wrote: > > On Mon, Nov 2, 2020 at 14:27:9, Vidya Sagar <vidyas@nvidia.com> wrote: > > > > > > > On 11/2/2020 7:45 PM, Rob Herring wrote: > > > External email: Use caution opening links or attachments > > > > > > > > > On Thu, Oct 29, 2020 at 12:40 AM Vidya Sagar <vidyas@nvidia.com> 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. This patch does the required programming in ATU upon > > >> querying the system policy for ECRC. > > >> > > >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > >> Reviewed-by: Jingoo Han <jingoohan1@gmail.com> > > >> --- > > >> V3: > > >> * Added 'Reviewed-by: Jingoo Han <jingoohan1@gmail.com>' > > >> > > >> V2: > > >> * Addressed Jingoo's review comment > > >> * Removed saving 'td' bit information in 'dw_pcie' structure > > >> > > >> drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++-- > > >> drivers/pci/controller/dwc/pcie-designware.h | 1 + > > >> 2 files changed, 7 insertions(+), 2 deletions(-) > > >> > > >> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > >> index b5e438b70cd5..cbd651b219d2 100644 > > >> --- a/drivers/pci/controller/dwc/pcie-designware.c > > >> +++ b/drivers/pci/controller/dwc/pcie-designware.c > > >> @@ -246,6 +246,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, > > >> dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET, > > >> upper_32_bits(pci_addr)); > > >> val = type | PCIE_ATU_FUNC_NUM(func_no); > > >> + if (pci->version == 0x490A) > > >> + val = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; > > >> val = upper_32_bits(size - 1) ? > > >> val | PCIE_ATU_INCREASE_REGION_SIZE : val; > > >> dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val); > > >> @@ -294,8 +296,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) > > > > > > Is this even possible? Are the non-unroll ATU registers available post 4.80? > > I'm not sure. Gustavo might have information about this. I made this > > change so that it is taken care off even if they available. > > The Synopsys DesignWare PCIe IP is highly configurable, therefore is > dependable on what the design team has configured for their solution. > Although Synopsys doesn't recommend the use of non-unroll ATU, the > customers are free to select what they want for their design. Okay, then there's a bug in the driver if the version is set to 0x480A or later and non-unroll is used: if (pci->version >= 0x480A || (!pci->version && dw_pcie_iatu_unroll_enabled(pci))) { Probably can just drop the version checking. The detection should always work. Rob ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC 2020-11-02 21:16 ` Rob Herring @ 2020-11-02 22:38 ` Gustavo Pimentel 2020-11-03 19:48 ` Rob Herring 0 siblings, 1 reply; 17+ messages in thread From: Gustavo Pimentel @ 2020-11-02 22:38 UTC (permalink / raw) To: Rob Herring Cc: Vidya Sagar, Jingoo Han, Lorenzo Pieralisi, Bjorn Helgaas, Andrew Murray, Thierry Reding, Jon Hunter, PCI, linux-kernel, kthota, Manikanta Maddireddy, sagar.tv On Mon, Nov 2, 2020 at 21:16:52, Rob Herring <robh@kernel.org> wrote: > On Mon, Nov 2, 2020 at 9:12 AM Gustavo Pimentel > <Gustavo.Pimentel@synopsys.com> wrote: > > > > On Mon, Nov 2, 2020 at 14:27:9, Vidya Sagar <vidyas@nvidia.com> wrote: > > > > > > > > > > > On 11/2/2020 7:45 PM, Rob Herring wrote: > > > > External email: Use caution opening links or attachments > > > > > > > > > > > > On Thu, Oct 29, 2020 at 12:40 AM Vidya Sagar <vidyas@nvidia.com> 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. This patch does the required programming in ATU upon > > > >> querying the system policy for ECRC. > > > >> > > > >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > >> Reviewed-by: Jingoo Han <jingoohan1@gmail.com> > > > >> --- > > > >> V3: > > > >> * Added 'Reviewed-by: Jingoo Han <jingoohan1@gmail.com>' > > > >> > > > >> V2: > > > >> * Addressed Jingoo's review comment > > > >> * Removed saving 'td' bit information in 'dw_pcie' structure > > > >> > > > >> drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++-- > > > >> drivers/pci/controller/dwc/pcie-designware.h | 1 + > > > >> 2 files changed, 7 insertions(+), 2 deletions(-) > > > >> > > > >> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > > >> index b5e438b70cd5..cbd651b219d2 100644 > > > >> --- a/drivers/pci/controller/dwc/pcie-designware.c > > > >> +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > >> @@ -246,6 +246,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, > > > >> dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET, > > > >> upper_32_bits(pci_addr)); > > > >> val = type | PCIE_ATU_FUNC_NUM(func_no); > > > >> + if (pci->version == 0x490A) > > > >> + val = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; > > > >> val = upper_32_bits(size - 1) ? > > > >> val | PCIE_ATU_INCREASE_REGION_SIZE : val; > > > >> dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val); > > > >> @@ -294,8 +296,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) > > > > > > > > Is this even possible? Are the non-unroll ATU registers available post 4.80? > > > I'm not sure. Gustavo might have information about this. I made this > > > change so that it is taken care off even if they available. > > > > The Synopsys DesignWare PCIe IP is highly configurable, therefore is > > dependable on what the design team has configured for their solution. > > Although Synopsys doesn't recommend the use of non-unroll ATU, the > > customers are free to select what they want for their design. > > Okay, then there's a bug in the driver if the version is set to 0x480A > or later and non-unroll is used: > > if (pci->version >= 0x480A || (!pci->version && > dw_pcie_iatu_unroll_enabled(pci))) { > > Probably can just drop the version checking. The detection should always work. Hi Rob, The "detection" is based on the assumption that the read value on PCIE_ATU_VIEWPORT register is 0xffffffff (which is a hard-coded value by design), if it's true then the iATU is unrolled and the function returns 1, otherwise is non-unrolled returns 0. So like you said it should always work, however, this code behavior was changed by Kishon on the following patch 2aadcb0cd39 ("PCI: dwc: Fix ATU identification for designware version >= 4.80"). His patch makes me believe that on keystone platform the read operation on that register causes some unpredicted behavior leads his platform to crash/abort, that's why he created this alternative version approach to avoid the "detection" algorithm. From what I'm seeing the only drivers that use this "version" approach are the keystone and intel-gw (which probably doesn't need it). To summarize, this is a workaround so that the keystone driver doesn't break independent of the controller IP version. -Gustavo > > Rob ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC 2020-11-02 22:38 ` Gustavo Pimentel @ 2020-11-03 19:48 ` Rob Herring 0 siblings, 0 replies; 17+ messages in thread From: Rob Herring @ 2020-11-03 19:48 UTC (permalink / raw) To: Gustavo Pimentel, Kishon Vijay Abraham I Cc: Vidya Sagar, Jingoo Han, Lorenzo Pieralisi, Bjorn Helgaas, Andrew Murray, Thierry Reding, Jon Hunter, PCI, linux-kernel, kthota, Manikanta Maddireddy, sagar.tv On Mon, Nov 2, 2020 at 4:38 PM Gustavo Pimentel <Gustavo.Pimentel@synopsys.com> wrote: > > On Mon, Nov 2, 2020 at 21:16:52, Rob Herring <robh@kernel.org> wrote: > > > On Mon, Nov 2, 2020 at 9:12 AM Gustavo Pimentel > > <Gustavo.Pimentel@synopsys.com> wrote: > > > > > > On Mon, Nov 2, 2020 at 14:27:9, Vidya Sagar <vidyas@nvidia.com> wrote: > > > > > > > > > > > > > > > On 11/2/2020 7:45 PM, Rob Herring wrote: > > > > > External email: Use caution opening links or attachments > > > > > > > > > > > > > > > On Thu, Oct 29, 2020 at 12:40 AM Vidya Sagar <vidyas@nvidia.com> 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. This patch does the required programming in ATU upon > > > > >> querying the system policy for ECRC. > > > > >> > > > > >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > > >> Reviewed-by: Jingoo Han <jingoohan1@gmail.com> > > > > >> --- > > > > >> V3: > > > > >> * Added 'Reviewed-by: Jingoo Han <jingoohan1@gmail.com>' > > > > >> > > > > >> V2: > > > > >> * Addressed Jingoo's review comment > > > > >> * Removed saving 'td' bit information in 'dw_pcie' structure > > > > >> > > > > >> drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++-- > > > > >> drivers/pci/controller/dwc/pcie-designware.h | 1 + > > > > >> 2 files changed, 7 insertions(+), 2 deletions(-) > > > > >> > > > > >> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > > > >> index b5e438b70cd5..cbd651b219d2 100644 > > > > >> --- a/drivers/pci/controller/dwc/pcie-designware.c > > > > >> +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > > >> @@ -246,6 +246,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, > > > > >> dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET, > > > > >> upper_32_bits(pci_addr)); > > > > >> val = type | PCIE_ATU_FUNC_NUM(func_no); > > > > >> + if (pci->version == 0x490A) > > > > >> + val = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; > > > > >> val = upper_32_bits(size - 1) ? > > > > >> val | PCIE_ATU_INCREASE_REGION_SIZE : val; > > > > >> dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val); > > > > >> @@ -294,8 +296,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) > > > > > > > > > > Is this even possible? Are the non-unroll ATU registers available post 4.80? > > > > I'm not sure. Gustavo might have information about this. I made this > > > > change so that it is taken care off even if they available. > > > > > > The Synopsys DesignWare PCIe IP is highly configurable, therefore is > > > dependable on what the design team has configured for their solution. > > > Although Synopsys doesn't recommend the use of non-unroll ATU, the > > > customers are free to select what they want for their design. Then given the feature is not really tied to the IP version, using version wasn't really a good choice. A better choice would have been a quirk flag that platforms could set. Perhaps called 'iatu_unroll_enabled'... > > Okay, then there's a bug in the driver if the version is set to 0x480A > > or later and non-unroll is used: > > > > if (pci->version >= 0x480A || (!pci->version && > > dw_pcie_iatu_unroll_enabled(pci))) { > > > > Probably can just drop the version checking. The detection should always work. > > Hi Rob, > > The "detection" is based on the assumption that the read value on > PCIE_ATU_VIEWPORT register is 0xffffffff (which is a hard-coded value by > design), if it's true then the iATU is unrolled and the function returns > 1, otherwise is non-unrolled returns 0. So like you said it should always > work, however, this code behavior was changed by Kishon on the following > patch 2aadcb0cd39 ("PCI: dwc: Fix ATU identification for designware > version >= 4.80"). His patch makes me believe that on keystone platform > the read operation on that register causes some unpredicted behavior > leads his platform to crash/abort, that's why he created this alternative > version approach to avoid the "detection" algorithm. Ah, h/w designers and their love for bus aborts... > From what I'm seeing the only drivers that use this "version" approach > are the keystone and intel-gw (which probably doesn't need it). > > To summarize, this is a workaround so that the keystone driver doesn't > break independent of the controller IP version. Keystone is also broken in another way. The dts files claim 16 in and out regions, but the ATU region is 4KB. It would need 8KB for 16 regions as unroll has a stride of 512bytes for each region. Rob ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC 2020-10-29 5:39 ` [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC Vidya Sagar 2020-10-29 22:03 ` Jingoo Han 2020-11-02 14:15 ` Rob Herring @ 2020-11-02 23:02 ` Bjorn Helgaas 2020-11-03 3:27 ` Vidya Sagar 2 siblings, 1 reply; 17+ messages in thread From: Bjorn Helgaas @ 2020-11-02 23:02 UTC (permalink / raw) To: Vidya Sagar Cc: jingoohan1, gustavo.pimentel, lorenzo.pieralisi, bhelgaas, amurray, robh, treding, jonathanh, linux-pci, linux-kernel, kthota, mmaddireddy, sagar.tv On Thu, Oct 29, 2020 at 11:09:59AM +0530, 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. This patch does the required programming in ATU upon > querying the system policy for ECRC. I guess this is a hardware defect, right? How much of a problem would it be if we instead added a "no_ecrc" quirk for this hardware so we never enabled ECRC? IIUC, the current Linux support of ECRC is a single choice at boot-time: by default ECRC is not enabled, but if you boot with "pci=ecrc=on", we turn on ECRC for every device. That seems like the minimal support, but I think the spec allows ECRC to be enabled selectively, on individual devices. I can imagine a sysfs knob that would allow us to enable/disable ECRC per-device at run-time. If we had such a sysfs knob, it would be pretty ugly and maybe impractical to work around this hardware issue. So I'm a little bit hesitant to add functionality that might have to be removed in the future. > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > Reviewed-by: Jingoo Han <jingoohan1@gmail.com> > --- > V3: > * Added 'Reviewed-by: Jingoo Han <jingoohan1@gmail.com>' > > V2: > * Addressed Jingoo's review comment > * Removed saving 'td' bit information in 'dw_pcie' structure > > drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++-- > drivers/pci/controller/dwc/pcie-designware.h | 1 + > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > index b5e438b70cd5..cbd651b219d2 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -246,6 +246,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, > dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET, > upper_32_bits(pci_addr)); > val = type | PCIE_ATU_FUNC_NUM(func_no); > + if (pci->version == 0x490A) > + val = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; > val = upper_32_bits(size - 1) ? > val | PCIE_ATU_INCREASE_REGION_SIZE : val; > dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val); > @@ -294,8 +296,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 = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; > + 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 e7f441441db2..b01ef407fd52 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -89,6 +89,7 @@ > #define PCIE_ATU_TYPE_IO 0x2 > #define PCIE_ATU_TYPE_CFG0 0x4 > #define PCIE_ATU_TYPE_CFG1 0x5 > +#define PCIE_ATU_TD_SHIFT 8 > #define PCIE_ATU_FUNC_NUM(pf) ((pf) << 20) > #define PCIE_ATU_CR2 0x908 > #define PCIE_ATU_ENABLE BIT(31) > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC 2020-11-02 23:02 ` Bjorn Helgaas @ 2020-11-03 3:27 ` Vidya Sagar 2020-11-03 21:07 ` Bjorn Helgaas 0 siblings, 1 reply; 17+ messages in thread From: Vidya Sagar @ 2020-11-03 3:27 UTC (permalink / raw) To: Bjorn Helgaas Cc: jingoohan1, gustavo.pimentel, lorenzo.pieralisi, bhelgaas, amurray, robh, treding, jonathanh, linux-pci, linux-kernel, kthota, mmaddireddy, sagar.tv On 11/3/2020 4:32 AM, Bjorn Helgaas wrote: > External email: Use caution opening links or attachments > > > On Thu, Oct 29, 2020 at 11:09:59AM +0530, 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. This patch does the required programming in ATU upon >> querying the system policy for ECRC. > > I guess this is a hardware defect, right? Yes. This is common across all DWC implementations (version 4.90 precisely) > > How much of a problem would it be if we instead added a "no_ecrc" > quirk for this hardware so we never enabled ECRC? Well, on Tegra for some of the high fidelity use cases, ECRC is required to be turned on and if it can be done safely with these patches, why shouldn't we not enable ECRC at all? > > IIUC, the current Linux support of ECRC is a single choice at > boot-time: by default ECRC is not enabled, but if you boot with > "pci=ecrc=on", we turn on ECRC for every device. > > That seems like the minimal support, but I think the spec allows ECRC > to be enabled selectively, on individual devices. I can imagine a > sysfs knob that would allow us to enable/disable ECRC per-device at > run-time. > > If we had such a sysfs knob, it would be pretty ugly and maybe > impractical to work around this hardware issue. So I'm a little bit > hesitant to add functionality that might have to be removed in the > future. Agree with this. But since it is a boot-time choice at this point, I think we can still go ahead with this approach to have a working ECRC mechanism right? I don't see any sysfs knob for AER controlling at this point. > >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >> Reviewed-by: Jingoo Han <jingoohan1@gmail.com> >> --- >> V3: >> * Added 'Reviewed-by: Jingoo Han <jingoohan1@gmail.com>' >> >> V2: >> * Addressed Jingoo's review comment >> * Removed saving 'td' bit information in 'dw_pcie' structure >> >> drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++-- >> drivers/pci/controller/dwc/pcie-designware.h | 1 + >> 2 files changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c >> index b5e438b70cd5..cbd651b219d2 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.c >> +++ b/drivers/pci/controller/dwc/pcie-designware.c >> @@ -246,6 +246,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, >> dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET, >> upper_32_bits(pci_addr)); >> val = type | PCIE_ATU_FUNC_NUM(func_no); >> + if (pci->version == 0x490A) >> + val = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; >> val = upper_32_bits(size - 1) ? >> val | PCIE_ATU_INCREASE_REGION_SIZE : val; >> dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val); >> @@ -294,8 +296,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 = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; >> + 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 e7f441441db2..b01ef407fd52 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.h >> +++ b/drivers/pci/controller/dwc/pcie-designware.h >> @@ -89,6 +89,7 @@ >> #define PCIE_ATU_TYPE_IO 0x2 >> #define PCIE_ATU_TYPE_CFG0 0x4 >> #define PCIE_ATU_TYPE_CFG1 0x5 >> +#define PCIE_ATU_TD_SHIFT 8 >> #define PCIE_ATU_FUNC_NUM(pf) ((pf) << 20) >> #define PCIE_ATU_CR2 0x908 >> #define PCIE_ATU_ENABLE BIT(31) >> -- >> 2.17.1 >> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC 2020-11-03 3:27 ` Vidya Sagar @ 2020-11-03 21:07 ` Bjorn Helgaas 2020-11-04 11:43 ` Vidya Sagar 0 siblings, 1 reply; 17+ messages in thread From: Bjorn Helgaas @ 2020-11-03 21:07 UTC (permalink / raw) To: Vidya Sagar Cc: jingoohan1, gustavo.pimentel, lorenzo.pieralisi, bhelgaas, amurray, robh, treding, jonathanh, linux-pci, linux-kernel, kthota, mmaddireddy, sagar.tv On Tue, Nov 03, 2020 at 08:57:01AM +0530, Vidya Sagar wrote: > On 11/3/2020 4:32 AM, Bjorn Helgaas wrote: > > On Thu, Oct 29, 2020 at 11:09:59AM +0530, 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. This patch does the required programming in ATU upon > > > querying the system policy for ECRC. > > > > I guess this is a hardware defect, right? > Yes. This is common across all DWC implementations (version 4.90 precisely) > > > How much of a problem would it be if we instead added a "no_ecrc" > > quirk for this hardware so we never enabled ECRC? > Well, on Tegra for some of the high fidelity use cases, ECRC is required to > be turned on and if it can be done safely with these patches, why shouldn't > we not enable ECRC at all? > > > IIUC, the current Linux support of ECRC is a single choice at > > boot-time: by default ECRC is not enabled, but if you boot with > > "pci=ecrc=on", we turn on ECRC for every device. > > > > That seems like the minimal support, but I think the spec allows ECRC > > to be enabled selectively, on individual devices. I can imagine a > > sysfs knob that would allow us to enable/disable ECRC per-device at > > run-time. > > > > If we had such a sysfs knob, it would be pretty ugly and maybe > > impractical to work around this hardware issue. So I'm a little bit > > hesitant to add functionality that might have to be removed in the > > future. > > Agree with this. But since it is a boot-time choice at this point, I think > we can still go ahead with this approach to have a working ECRC mechanism > right? I don't see any sysfs knob for AER controlling at this point. I don't want to do anything that will prevent us from adding per-device ECRC control in the future. My concern is that if we add a run-time sysfs knob in the future, the user experience on this hardware will be poor because there's no convenient path to twiddle the PCIE_ATU_TD bit when the generic code changes the AER Control bit. What is the failure mode in these scenarios: - User boots with defaults, ECRC is disabled. - User enables ECRC via sysfs. - What happens here? ECRC is enabled via AER Control but not via DWC TD bit. I assume ECRC doesn't work. Does it cause PCIe errors (malformed TLP, etc)? Or this one: - User boots with "pci=ecrc=on", ECRC is enabled. - ECRC works fine. - User disables ECRC via sysfs. - What happens here? ECRC is disabled via AER Control, but DWC TD bit thinks it's still enabled. If you enabled ECRC unconditionally on DWC and the sysfs knob had no effect, I'd be OK with that. I'm more worried about what happens when the AER bit and the DWC TD bit are set so they don't match. What is the failure mode there? > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > Reviewed-by: Jingoo Han <jingoohan1@gmail.com> > > > --- > > > V3: > > > * Added 'Reviewed-by: Jingoo Han <jingoohan1@gmail.com>' > > > > > > V2: > > > * Addressed Jingoo's review comment > > > * Removed saving 'td' bit information in 'dw_pcie' structure > > > > > > drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++-- > > > drivers/pci/controller/dwc/pcie-designware.h | 1 + > > > 2 files changed, 7 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > > index b5e438b70cd5..cbd651b219d2 100644 > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > @@ -246,6 +246,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, > > > dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET, > > > upper_32_bits(pci_addr)); > > > val = type | PCIE_ATU_FUNC_NUM(func_no); > > > + if (pci->version == 0x490A) > > > + val = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; > > > val = upper_32_bits(size - 1) ? > > > val | PCIE_ATU_INCREASE_REGION_SIZE : val; > > > dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val); > > > @@ -294,8 +296,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 = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; > > > + 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 e7f441441db2..b01ef407fd52 100644 > > > --- a/drivers/pci/controller/dwc/pcie-designware.h > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h > > > @@ -89,6 +89,7 @@ > > > #define PCIE_ATU_TYPE_IO 0x2 > > > #define PCIE_ATU_TYPE_CFG0 0x4 > > > #define PCIE_ATU_TYPE_CFG1 0x5 > > > +#define PCIE_ATU_TD_SHIFT 8 > > > #define PCIE_ATU_FUNC_NUM(pf) ((pf) << 20) > > > #define PCIE_ATU_CR2 0x908 > > > #define PCIE_ATU_ENABLE BIT(31) > > > -- > > > 2.17.1 > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC 2020-11-03 21:07 ` Bjorn Helgaas @ 2020-11-04 11:43 ` Vidya Sagar 2020-11-04 16:22 ` Bjorn Helgaas 0 siblings, 1 reply; 17+ messages in thread From: Vidya Sagar @ 2020-11-04 11:43 UTC (permalink / raw) To: Bjorn Helgaas Cc: jingoohan1, gustavo.pimentel, lorenzo.pieralisi, bhelgaas, amurray, robh, treding, jonathanh, linux-pci, linux-kernel, kthota, mmaddireddy, sagar.tv On 11/4/2020 2:37 AM, Bjorn Helgaas wrote: > External email: Use caution opening links or attachments > > > On Tue, Nov 03, 2020 at 08:57:01AM +0530, Vidya Sagar wrote: >> On 11/3/2020 4:32 AM, Bjorn Helgaas wrote: >>> On Thu, Oct 29, 2020 at 11:09:59AM +0530, 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. This patch does the required programming in ATU upon >>>> querying the system policy for ECRC. >>> >>> I guess this is a hardware defect, right? >> Yes. This is common across all DWC implementations (version 4.90 precisely) >> >>> How much of a problem would it be if we instead added a "no_ecrc" >>> quirk for this hardware so we never enabled ECRC? >> Well, on Tegra for some of the high fidelity use cases, ECRC is required to >> be turned on and if it can be done safely with these patches, why shouldn't >> we not enable ECRC at all? >> >>> IIUC, the current Linux support of ECRC is a single choice at >>> boot-time: by default ECRC is not enabled, but if you boot with >>> "pci=ecrc=on", we turn on ECRC for every device. >>> >>> That seems like the minimal support, but I think the spec allows ECRC >>> to be enabled selectively, on individual devices. I can imagine a >>> sysfs knob that would allow us to enable/disable ECRC per-device at >>> run-time. >>> >>> If we had such a sysfs knob, it would be pretty ugly and maybe >>> impractical to work around this hardware issue. So I'm a little bit >>> hesitant to add functionality that might have to be removed in the >>> future. >> >> Agree with this. But since it is a boot-time choice at this point, I think >> we can still go ahead with this approach to have a working ECRC mechanism >> right? I don't see any sysfs knob for AER controlling at this point. > > I don't want to do anything that will prevent us from adding > per-device ECRC control in the future. > > My concern is that if we add a run-time sysfs knob in the future, the > user experience on this hardware will be poor because there's no > convenient path to twiddle the PCIE_ATU_TD bit when the generic code > changes the AER Control bit. Agree. Can we add it to the documentation that run time changing of ECRC settings are not supported on this (i.e. Tegra194) platform (or for that matter on any SoC with PCIe based on DesignWare core version 4.90A). By 'not supported', I meant that the ECRC digest part may not work but the normal functionality will continue to work without reporting any AER errors. I tried to emulate the following scenarios on Tegra194 silicon and here are my observations. FWIW, I'm referring to the PCIe spec Rev 5.0 Ver 1.0 (22 May 2019) > > What is the failure mode in these scenarios: > > - User boots with defaults, ECRC is disabled. > - User enables ECRC via sysfs. > - What happens here? ECRC is enabled via AER Control but not via > DWC TD bit. I assume ECRC doesn't work. Does it cause PCIe > errors (malformed TLP, etc)? Since DWC TD bit is not programmed, for all the transactions that go through ATU, TLP Digest won't get generated (although AER registers indicate that ECRC should get generated). As per the spec section "2.7.1 ECRC Rules" --- If a device Function is enabled to generate ECRC, it must calculate and apply ECRC for all TLPs originated by the Function --- So the RP would be violating the PCIe spec, but it doesn't result in any error because the same section has the following rule as well --- If a device Function is enabled to check ECRC, it must do so for all TLPs with ECRC where the device is the ultimate PCI Express Receiver Note that it is still possible for the Function to receive TLPs without ECRC, and these are processed normally - this is not an error --- so, even if the EP has ECRC check enabled, because of the above rule, it just processes those packets without ECRC as normal packets. Basically, whoever is enabling ECRC run time gets cheated as transactions routed through ATU don't really have ECRC digest > > Or this one: > > - User boots with "pci=ecrc=on", ECRC is enabled. > - ECRC works fine. > - User disables ECRC via sysfs. > - What happens here? ECRC is disabled via AER Control, but DWC TD > bit thinks it's still enabled. In this case, the EP doesn't have ECRC check enabled but receives TLPs with ECRC digest. This again won't result in any error because of the section "2.2.3 TLP Digest Rules" which says --- If an intermediate or ultimate PCI Express Receiver of the TLP does not support ECRC checking, the Receiver must ignore the TLP Digest --- So the EP just ignores the TLP Digest and process the TLP normally. Although functionality wise there is no issue here, there could be some impact on the perf because of the extra DWord data. This is again debatable as the perf/data path is typically from EP's DMA engine to host system memory and not config/BAR accesses. > > If you enabled ECRC unconditionally on DWC and the sysfs knob had no > effect, I'd be OK with that. I'm more worried about what happens when > the AER bit and the DWC TD bit are set so they don't match. What is > the failure mode there? Based on the above experiments, we can as well keep the DWC TD bit programmed unconditionally and it won't lead to any errors. As mentioned before, the only downside of it is the extra DWord in each ATU routed TLP which may load the bus (in downstream direction) with no real benefit as such. Please let me know where can we go from here. I can push a different patch to keep DWC TD bit always programmed if that is the best approach to take at this point. > >>>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >>>> Reviewed-by: Jingoo Han <jingoohan1@gmail.com> >>>> --- >>>> V3: >>>> * Added 'Reviewed-by: Jingoo Han <jingoohan1@gmail.com>' >>>> >>>> V2: >>>> * Addressed Jingoo's review comment >>>> * Removed saving 'td' bit information in 'dw_pcie' structure >>>> >>>> drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++-- >>>> drivers/pci/controller/dwc/pcie-designware.h | 1 + >>>> 2 files changed, 7 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c >>>> index b5e438b70cd5..cbd651b219d2 100644 >>>> --- a/drivers/pci/controller/dwc/pcie-designware.c >>>> +++ b/drivers/pci/controller/dwc/pcie-designware.c >>>> @@ -246,6 +246,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, >>>> dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET, >>>> upper_32_bits(pci_addr)); >>>> val = type | PCIE_ATU_FUNC_NUM(func_no); >>>> + if (pci->version == 0x490A) >>>> + val = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; >>>> val = upper_32_bits(size - 1) ? >>>> val | PCIE_ATU_INCREASE_REGION_SIZE : val; >>>> dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val); >>>> @@ -294,8 +296,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 = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; >>>> + 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 e7f441441db2..b01ef407fd52 100644 >>>> --- a/drivers/pci/controller/dwc/pcie-designware.h >>>> +++ b/drivers/pci/controller/dwc/pcie-designware.h >>>> @@ -89,6 +89,7 @@ >>>> #define PCIE_ATU_TYPE_IO 0x2 >>>> #define PCIE_ATU_TYPE_CFG0 0x4 >>>> #define PCIE_ATU_TYPE_CFG1 0x5 >>>> +#define PCIE_ATU_TD_SHIFT 8 >>>> #define PCIE_ATU_FUNC_NUM(pf) ((pf) << 20) >>>> #define PCIE_ATU_CR2 0x908 >>>> #define PCIE_ATU_ENABLE BIT(31) >>>> -- >>>> 2.17.1 >>>> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC 2020-11-04 11:43 ` Vidya Sagar @ 2020-11-04 16:22 ` Bjorn Helgaas 2020-11-04 17:40 ` Vidya Sagar 0 siblings, 1 reply; 17+ messages in thread From: Bjorn Helgaas @ 2020-11-04 16:22 UTC (permalink / raw) To: Vidya Sagar Cc: jingoohan1, gustavo.pimentel, lorenzo.pieralisi, bhelgaas, amurray, robh, treding, jonathanh, linux-pci, linux-kernel, kthota, mmaddireddy, sagar.tv On Wed, Nov 04, 2020 at 05:13:07PM +0530, Vidya Sagar wrote: > On 11/4/2020 2:37 AM, Bjorn Helgaas wrote: > > On Tue, Nov 03, 2020 at 08:57:01AM +0530, Vidya Sagar wrote: > > > On 11/3/2020 4:32 AM, Bjorn Helgaas wrote: > > > > On Thu, Oct 29, 2020 at 11:09:59AM +0530, 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. This patch does the required programming in ATU upon > > > > > querying the system policy for ECRC. > > > > > > > > I guess this is a hardware defect, right? > > > Yes. This is common across all DWC implementations (version 4.90 precisely) > > > > > > > How much of a problem would it be if we instead added a "no_ecrc" > > > > quirk for this hardware so we never enabled ECRC? > > > Well, on Tegra for some of the high fidelity use cases, ECRC is required to > > > be turned on and if it can be done safely with these patches, why shouldn't > > > we not enable ECRC at all? > > > > > > > IIUC, the current Linux support of ECRC is a single choice at > > > > boot-time: by default ECRC is not enabled, but if you boot with > > > > "pci=ecrc=on", we turn on ECRC for every device. > > > > > > > > That seems like the minimal support, but I think the spec allows ECRC > > > > to be enabled selectively, on individual devices. I can imagine a > > > > sysfs knob that would allow us to enable/disable ECRC per-device at > > > > run-time. > > > > > > > > If we had such a sysfs knob, it would be pretty ugly and maybe > > > > impractical to work around this hardware issue. So I'm a little bit > > > > hesitant to add functionality that might have to be removed in the > > > > future. > > > > > > Agree with this. But since it is a boot-time choice at this point, I think > > > we can still go ahead with this approach to have a working ECRC mechanism > > > right? I don't see any sysfs knob for AER controlling at this point. > > > > I don't want to do anything that will prevent us from adding > > per-device ECRC control in the future. > > > > My concern is that if we add a run-time sysfs knob in the future, the > > user experience on this hardware will be poor because there's no > > convenient path to twiddle the PCIE_ATU_TD bit when the generic code > > changes the AER Control bit. > > Agree. > Can we add it to the documentation that run time changing of ECRC settings > are not supported on this (i.e. Tegra194) platform (or for that matter on > any SoC with PCIe based on DesignWare core version 4.90A). By 'not > supported', I meant that the ECRC digest part may not work but the normal > functionality will continue to work without reporting any AER errors. I > tried to emulate the following scenarios on Tegra194 silicon and here are my > observations. > FWIW, I'm referring to the PCIe spec Rev 5.0 Ver 1.0 (22 May 2019) > > > What is the failure mode in these scenarios: > > > > - User boots with defaults, ECRC is disabled. > > - User enables ECRC via sysfs. > > - What happens here? ECRC is enabled via AER Control but not via > > DWC TD bit. I assume ECRC doesn't work. Does it cause PCIe > > errors (malformed TLP, etc)? > > Since DWC TD bit is not programmed, for all the transactions that go through > ATU, TLP Digest won't get generated (although AER registers indicate that > ECRC should get generated). > As per the spec section "2.7.1 ECRC Rules" > > --- > If a device Function is enabled to generate ECRC, it must calculate and > apply ECRC for all TLPs originated by the Function > --- > > So the RP would be violating the PCIe spec, but it doesn't result in any > error because the same section has the following rule as well > > --- > If a device Function is enabled to check ECRC, it must do so for all TLPs > with ECRC where the device is the ultimate PCI Express Receiver > Note that it is still possible for the Function to receive TLPs without > ECRC, and these are processed normally - this is not an error > --- > > so, even if the EP has ECRC check enabled, because of the above rule, it > just processes those packets without ECRC as normal packets. > Basically, whoever is enabling ECRC run time gets cheated as transactions > routed through ATU don't really have ECRC digest > > > Or this one: > > > > - User boots with "pci=ecrc=on", ECRC is enabled. > > - ECRC works fine. > > - User disables ECRC via sysfs. > > - What happens here? ECRC is disabled via AER Control, but DWC TD > > bit thinks it's still enabled. > > In this case, the EP doesn't have ECRC check enabled but receives TLPs with > ECRC digest. This again won't result in any error because of the section > "2.2.3 TLP Digest Rules" which says > > --- > If an intermediate or ultimate PCI Express Receiver of the TLP does not > support ECRC checking, the Receiver must ignore the TLP Digest > --- > > So the EP just ignores the TLP Digest and process the TLP normally. > Although functionality wise there is no issue here, there could be some > impact on the perf because of the extra DWord data. This is again debatable > as the perf/data path is typically from EP's DMA engine to host system > memory and not config/BAR accesses. > > > > If you enabled ECRC unconditionally on DWC and the sysfs knob had no > > effect, I'd be OK with that. I'm more worried about what happens when > > the AER bit and the DWC TD bit are set so they don't match. What is > > the failure mode there? > > Based on the above experiments, we can as well keep the DWC TD bit > programmed unconditionally and it won't lead to any errors. As mentioned > before, the only downside of it is the extra DWord in each ATU routed TLP > which may load the bus (in downstream direction) with no real benefit as > such. IIUC the issue only affects traffic from the Root Port to the Endpoint, and traffic going upstream is unaffected. Here's what I think happens based on the RP and EP settings, please correct me if wrong: 1) RP TD+ ECRC_gen+ generates ECRC EP ECRC_check- ignores ECRC EP ECRC_check+ checks ECRC 2) RP TD+ ECRC_gen- generates ECRC when it shouldn't (defect) EP ECRC_check- ignores ECRC EP ECRC_check+ checks ECRC (may signal errors) 3) RP TD- ECRC_gen+ fails to generate ECRC (defect) EP ECRC_check- ignores ECRC EP ECRC_check+ handles TLPs without ECRC normally, but cannot detect ECRC errors 4) RP TD- ECRC_gen- no ECRC generated (as expected) EP ECRC_check- ignores ECRC EP ECRC_check+ handles TLPs without ECRC normally If my assumptions above are correct, this defect never causes extra errors like Malformed TLP, etc, to be signaled regardless of the AER ECRC_check settings. The only functional problem is that in case 3, the EP *should* be able to check and signal ECRC errors, but it cannot because the RP doesn't generate ECRC. Case 2 is a performance issue because we add the extra dword in every TLP going downstream. That doesn't seem unreasonable since this is just config and BAR accesses. The EP may detect ECRC errors when we don't think the RP is even generating ECRC, but in general we won't enable checking in the EP unless we also enable generation in the RP. So I think setting the DWC TD bit unconditionally seems like a pretty good solution. > Please let me know where can we go from here. > I can push a different patch to keep DWC TD bit always programmed if that is > the best approach to take at this point. > > > > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > > > Reviewed-by: Jingoo Han <jingoohan1@gmail.com> > > > > > --- > > > > > V3: > > > > > * Added 'Reviewed-by: Jingoo Han <jingoohan1@gmail.com>' > > > > > > > > > > V2: > > > > > * Addressed Jingoo's review comment > > > > > * Removed saving 'td' bit information in 'dw_pcie' structure > > > > > > > > > > drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++-- > > > > > drivers/pci/controller/dwc/pcie-designware.h | 1 + > > > > > 2 files changed, 7 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > > > > index b5e438b70cd5..cbd651b219d2 100644 > > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > > > @@ -246,6 +246,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, > > > > > dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET, > > > > > upper_32_bits(pci_addr)); > > > > > val = type | PCIE_ATU_FUNC_NUM(func_no); > > > > > + if (pci->version == 0x490A) > > > > > + val = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; > > > > > val = upper_32_bits(size - 1) ? > > > > > val | PCIE_ATU_INCREASE_REGION_SIZE : val; > > > > > dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val); > > > > > @@ -294,8 +296,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 = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; > > > > > + 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 e7f441441db2..b01ef407fd52 100644 > > > > > --- a/drivers/pci/controller/dwc/pcie-designware.h > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h > > > > > @@ -89,6 +89,7 @@ > > > > > #define PCIE_ATU_TYPE_IO 0x2 > > > > > #define PCIE_ATU_TYPE_CFG0 0x4 > > > > > #define PCIE_ATU_TYPE_CFG1 0x5 > > > > > +#define PCIE_ATU_TD_SHIFT 8 > > > > > #define PCIE_ATU_FUNC_NUM(pf) ((pf) << 20) > > > > > #define PCIE_ATU_CR2 0x908 > > > > > #define PCIE_ATU_ENABLE BIT(31) > > > > > -- > > > > > 2.17.1 > > > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC 2020-11-04 16:22 ` Bjorn Helgaas @ 2020-11-04 17:40 ` Vidya Sagar 0 siblings, 0 replies; 17+ messages in thread From: Vidya Sagar @ 2020-11-04 17:40 UTC (permalink / raw) To: Bjorn Helgaas Cc: jingoohan1, gustavo.pimentel, lorenzo.pieralisi, bhelgaas, amurray, robh, treding, jonathanh, linux-pci, linux-kernel, kthota, mmaddireddy, sagar.tv On 11/4/2020 9:52 PM, Bjorn Helgaas wrote: > External email: Use caution opening links or attachments > > > On Wed, Nov 04, 2020 at 05:13:07PM +0530, Vidya Sagar wrote: >> On 11/4/2020 2:37 AM, Bjorn Helgaas wrote: >>> On Tue, Nov 03, 2020 at 08:57:01AM +0530, Vidya Sagar wrote: >>>> On 11/3/2020 4:32 AM, Bjorn Helgaas wrote: >>>>> On Thu, Oct 29, 2020 at 11:09:59AM +0530, 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. This patch does the required programming in ATU upon >>>>>> querying the system policy for ECRC. >>>>> >>>>> I guess this is a hardware defect, right? >>>> Yes. This is common across all DWC implementations (version 4.90 precisely) >>>> >>>>> How much of a problem would it be if we instead added a "no_ecrc" >>>>> quirk for this hardware so we never enabled ECRC? >>>> Well, on Tegra for some of the high fidelity use cases, ECRC is required to >>>> be turned on and if it can be done safely with these patches, why shouldn't >>>> we not enable ECRC at all? >>>> >>>>> IIUC, the current Linux support of ECRC is a single choice at >>>>> boot-time: by default ECRC is not enabled, but if you boot with >>>>> "pci=ecrc=on", we turn on ECRC for every device. >>>>> >>>>> That seems like the minimal support, but I think the spec allows ECRC >>>>> to be enabled selectively, on individual devices. I can imagine a >>>>> sysfs knob that would allow us to enable/disable ECRC per-device at >>>>> run-time. >>>>> >>>>> If we had such a sysfs knob, it would be pretty ugly and maybe >>>>> impractical to work around this hardware issue. So I'm a little bit >>>>> hesitant to add functionality that might have to be removed in the >>>>> future. >>>> >>>> Agree with this. But since it is a boot-time choice at this point, I think >>>> we can still go ahead with this approach to have a working ECRC mechanism >>>> right? I don't see any sysfs knob for AER controlling at this point. >>> >>> I don't want to do anything that will prevent us from adding >>> per-device ECRC control in the future. >>> >>> My concern is that if we add a run-time sysfs knob in the future, the >>> user experience on this hardware will be poor because there's no >>> convenient path to twiddle the PCIE_ATU_TD bit when the generic code >>> changes the AER Control bit. >> >> Agree. >> Can we add it to the documentation that run time changing of ECRC settings >> are not supported on this (i.e. Tegra194) platform (or for that matter on >> any SoC with PCIe based on DesignWare core version 4.90A). By 'not >> supported', I meant that the ECRC digest part may not work but the normal >> functionality will continue to work without reporting any AER errors. I >> tried to emulate the following scenarios on Tegra194 silicon and here are my >> observations. >> FWIW, I'm referring to the PCIe spec Rev 5.0 Ver 1.0 (22 May 2019) >> >>> What is the failure mode in these scenarios: >>> >>> - User boots with defaults, ECRC is disabled. >>> - User enables ECRC via sysfs. >>> - What happens here? ECRC is enabled via AER Control but not via >>> DWC TD bit. I assume ECRC doesn't work. Does it cause PCIe >>> errors (malformed TLP, etc)? >> >> Since DWC TD bit is not programmed, for all the transactions that go through >> ATU, TLP Digest won't get generated (although AER registers indicate that >> ECRC should get generated). >> As per the spec section "2.7.1 ECRC Rules" >> >> --- >> If a device Function is enabled to generate ECRC, it must calculate and >> apply ECRC for all TLPs originated by the Function >> --- >> >> So the RP would be violating the PCIe spec, but it doesn't result in any >> error because the same section has the following rule as well >> >> --- >> If a device Function is enabled to check ECRC, it must do so for all TLPs >> with ECRC where the device is the ultimate PCI Express Receiver >> Note that it is still possible for the Function to receive TLPs without >> ECRC, and these are processed normally - this is not an error >> --- >> >> so, even if the EP has ECRC check enabled, because of the above rule, it >> just processes those packets without ECRC as normal packets. >> Basically, whoever is enabling ECRC run time gets cheated as transactions >> routed through ATU don't really have ECRC digest >> >>> Or this one: >>> >>> - User boots with "pci=ecrc=on", ECRC is enabled. >>> - ECRC works fine. >>> - User disables ECRC via sysfs. >>> - What happens here? ECRC is disabled via AER Control, but DWC TD >>> bit thinks it's still enabled. >> >> In this case, the EP doesn't have ECRC check enabled but receives TLPs with >> ECRC digest. This again won't result in any error because of the section >> "2.2.3 TLP Digest Rules" which says >> >> --- >> If an intermediate or ultimate PCI Express Receiver of the TLP does not >> support ECRC checking, the Receiver must ignore the TLP Digest >> --- >> >> So the EP just ignores the TLP Digest and process the TLP normally. >> Although functionality wise there is no issue here, there could be some >> impact on the perf because of the extra DWord data. This is again debatable >> as the perf/data path is typically from EP's DMA engine to host system >> memory and not config/BAR accesses. >>> >>> If you enabled ECRC unconditionally on DWC and the sysfs knob had no >>> effect, I'd be OK with that. I'm more worried about what happens when >>> the AER bit and the DWC TD bit are set so they don't match. What is >>> the failure mode there? >> >> Based on the above experiments, we can as well keep the DWC TD bit >> programmed unconditionally and it won't lead to any errors. As mentioned >> before, the only downside of it is the extra DWord in each ATU routed TLP >> which may load the bus (in downstream direction) with no real benefit as >> such. > > IIUC the issue only affects traffic from the Root Port to the > Endpoint, and traffic going upstream is unaffected. Here's what I > think happens based on the RP and EP settings, please correct me if > wrong: > > 1) RP TD+ ECRC_gen+ generates ECRC > EP ECRC_check- ignores ECRC > EP ECRC_check+ checks ECRC > > 2) RP TD+ ECRC_gen- generates ECRC when it shouldn't (defect) > EP ECRC_check- ignores ECRC > EP ECRC_check+ checks ECRC (may signal errors) > > 3) RP TD- ECRC_gen+ fails to generate ECRC (defect) > EP ECRC_check- ignores ECRC > EP ECRC_check+ handles TLPs without ECRC normally, > but cannot detect ECRC errors > > 4) RP TD- ECRC_gen- no ECRC generated (as expected) > EP ECRC_check- ignores ECRC > EP ECRC_check+ handles TLPs without ECRC normally > > If my assumptions above are correct, this defect never causes extra > errors like Malformed TLP, etc, to be signaled regardless of the AER > ECRC_check settings. Yes. > > The only functional problem is that in case 3, the EP *should* be able > to check and signal ECRC errors, but it cannot because the RP doesn't > generate ECRC. Yes. Thats correct > > Case 2 is a performance issue because we add the extra dword in every > TLP going downstream. That doesn't seem unreasonable since this is > just config and BAR accesses. The EP may detect ECRC errors when we > don't think the RP is even generating ECRC, but in general we won't > enable checking in the EP unless we also enable generation in the RP. > > So I think setting the DWC TD bit unconditionally seems like a pretty > good solution. Ok. Sure. I'll push a patch to program DWC TD bit unconditionally then. Thanks for your insights and guidance. > >> Please let me know where can we go from here. >> I can push a different patch to keep DWC TD bit always programmed if that is >> the best approach to take at this point. >>> >>>>>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >>>>>> Reviewed-by: Jingoo Han <jingoohan1@gmail.com> >>>>>> --- >>>>>> V3: >>>>>> * Added 'Reviewed-by: Jingoo Han <jingoohan1@gmail.com>' >>>>>> >>>>>> V2: >>>>>> * Addressed Jingoo's review comment >>>>>> * Removed saving 'td' bit information in 'dw_pcie' structure >>>>>> >>>>>> drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++-- >>>>>> drivers/pci/controller/dwc/pcie-designware.h | 1 + >>>>>> 2 files changed, 7 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c >>>>>> index b5e438b70cd5..cbd651b219d2 100644 >>>>>> --- a/drivers/pci/controller/dwc/pcie-designware.c >>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware.c >>>>>> @@ -246,6 +246,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, >>>>>> dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET, >>>>>> upper_32_bits(pci_addr)); >>>>>> val = type | PCIE_ATU_FUNC_NUM(func_no); >>>>>> + if (pci->version == 0x490A) >>>>>> + val = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; >>>>>> val = upper_32_bits(size - 1) ? >>>>>> val | PCIE_ATU_INCREASE_REGION_SIZE : val; >>>>>> dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val); >>>>>> @@ -294,8 +296,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 = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; >>>>>> + 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 e7f441441db2..b01ef407fd52 100644 >>>>>> --- a/drivers/pci/controller/dwc/pcie-designware.h >>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware.h >>>>>> @@ -89,6 +89,7 @@ >>>>>> #define PCIE_ATU_TYPE_IO 0x2 >>>>>> #define PCIE_ATU_TYPE_CFG0 0x4 >>>>>> #define PCIE_ATU_TYPE_CFG1 0x5 >>>>>> +#define PCIE_ATU_TD_SHIFT 8 >>>>>> #define PCIE_ATU_FUNC_NUM(pf) ((pf) << 20) >>>>>> #define PCIE_ATU_CR2 0x908 >>>>>> #define PCIE_ATU_ENABLE BIT(31) >>>>>> -- >>>>>> 2.17.1 >>>>>> ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-11-04 17:41 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-29 5:39 [PATCH V3 0/2] Add support to configure DWC for ECRC Vidya Sagar 2020-10-29 5:39 ` [PATCH V3 1/2] PCI/AER: Add pcie_is_ecrc_enabled() API Vidya Sagar 2020-10-29 5:39 ` [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC Vidya Sagar 2020-10-29 22:03 ` Jingoo Han 2020-10-30 6:45 ` Vidya Sagar 2020-11-02 14:15 ` Rob Herring 2020-11-02 14:27 ` Vidya Sagar 2020-11-02 15:11 ` Gustavo Pimentel 2020-11-02 21:16 ` Rob Herring 2020-11-02 22:38 ` Gustavo Pimentel 2020-11-03 19:48 ` Rob Herring 2020-11-02 23:02 ` Bjorn Helgaas 2020-11-03 3:27 ` Vidya Sagar 2020-11-03 21:07 ` Bjorn Helgaas 2020-11-04 11:43 ` Vidya Sagar 2020-11-04 16:22 ` Bjorn Helgaas 2020-11-04 17:40 ` Vidya Sagar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).