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

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

[Vidya Sagar]

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>
---
 drivers/pci/controller/dwc/pcie-designware.c | 50 ++++++++++++++++++--
 drivers/pci/controller/dwc/pcie-designware.h |  1 +
 2 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index c2dea8fc97c8..ebdc37a58e94 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -225,6 +225,44 @@ 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 DsignWare 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 +283,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 +332,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

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

[Bjorn Helgaas]

On Tue, Nov 10, 2020 at 12:56:11AM +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.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>

Modulo typos/formatting comments below,

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Thanks for working through this.

> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 50 ++++++++++++++++++--
>  drivers/pci/controller/dwc/pcie-designware.h |  1 +
>  2 files changed, 47 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index c2dea8fc97c8..ebdc37a58e94 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -225,6 +225,44 @@ 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 DsignWare core.

s/DsignWare/DesignWare/

> +	 *    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.

Convention is to separate paragraphs with blank lines, not to indent
the first line.

> +	 * 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",

Wrap to fit in 80 columns.

> +	 *                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 +283,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 +332,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
> 

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

[Vidya Sagar]

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)
+{
+	/*
+	 * 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

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

[Jingoo Han]

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

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

[Vidya Sagar]

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
> 

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

[Bjorn Helgaas]

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

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

[Vidya Sagar]

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

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

[Vidya Sagar]

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

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

[Bjorn Helgaas]

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

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

[Vidya Sagar]

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

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

[Vidya Sagar]

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

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

[Rob Herring]

On Fri, Dec 11, 2020 at 7:58 AM Vidya Sagar <vidyas@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;
> >>>>>>>> +}

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

[Lorenzo Pieralisi]

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

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

[Bjorn Helgaas]

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/

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

[Rob Herring]

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

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

[Rob Herring]

On Mon, Nov 9, 2020 at 1:26 PM 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.
>
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 50 ++++++++++++++++++--
>  drivers/pci/controller/dwc/pcie-designware.h |  1 +
>  2 files changed, 47 insertions(+), 4 deletions(-)

Reviewed-by: Rob Herring <robh@kernel.org>