[PATCH] PCI: designware: don't hard-code DBI/ATU offset

[PATCH] PCI: designware: don't hard-code DBI/ATU offset

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

The DWC PCIe core contains various separate register spaces: DBI, DBI2,
ATU, DMA, etc. The relationship between the addresses of these register
spaces is entirely determined by the implementation of the IP block, not
by the IP block design itself. Hence, the DWC driver must not make
assumptions that one register space can be accessed at a fixed offset from
any other register space. To avoid such assumptions, introduce an
explicit/separate register pointer for the ATU register space. In
particular, the current assumption is not valid for NVIDIA's T194 SoC.

The ATU register space is only used on systems that require unrolled ATU
access. This property is detected at run-time for host controllers, and
when this is detected, this patch provides a default value for atu_base
that matches the previous assumption re: register layout. An alternative
would be to update all drivers for HW that requires unrolled access to
explicitly set atu_base. However, it's hard to tell which drivers would
require atu_base to be set. The unrolled property is not detected for
endpoint systems, and so any endpoint driver that requires unrolled access
must explicitly set the iatu_unroll_enabled flag (none do at present), and
so a check is added to require the driver to also set atu_base while at
it.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 .../pci/controller/dwc/pcie-designware-ep.c   |  4 ++++
 .../pci/controller/dwc/pcie-designware-host.c |  3 +++
 drivers/pci/controller/dwc/pcie-designware.c  |  8 ++++----
 drivers/pci/controller/dwc/pcie-designware.h  | 20 +++++++++++++++----
 4 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 1e7b02221eac..880210366e71 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -504,6 +504,10 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 		dev_err(dev, "dbi_base/dbi_base2 is not populated\n");
 		return -EINVAL;
 	}
+	if (pci->iatu_unroll_enabled && !pci->atu_base) {
+		dev_err(dev, "atu_base is not populated\n");
+		return -EINVAL;
+	}
 
 	ret = of_property_read_u32(np, "num-ib-windows", &ep->num_ib_windows);
 	if (ret < 0) {
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 29a05759a294..2ebb7f4768cf 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -699,6 +699,9 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 		dev_dbg(pci->dev, "iATU unroll: %s\n",
 			pci->iatu_unroll_enabled ? "enabled" : "disabled");
 
+		if (pci->iatu_unroll_enabled && !pci->atu_base)
+			pci->atu_base = pci->dbi_base + (0x3 << 20);
+
 		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX0,
 					  PCIE_ATU_TYPE_MEM, pp->mem_base,
 					  pp->mem_bus_addr, pp->mem_size);
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 2153956a0b20..93ef8c31fb39 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -93,7 +93,7 @@ static u32 dw_pcie_readl_ob_unroll(struct dw_pcie *pci, u32 index, u32 reg)
 {
 	u32 offset = PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(index);
 
-	return dw_pcie_readl_dbi(pci, offset + reg);
+	return dw_pcie_readl_atu(pci, offset + reg);
 }
 
 static void dw_pcie_writel_ob_unroll(struct dw_pcie *pci, u32 index, u32 reg,
@@ -101,7 +101,7 @@ static void dw_pcie_writel_ob_unroll(struct dw_pcie *pci, u32 index, u32 reg,
 {
 	u32 offset = PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(index);
 
-	dw_pcie_writel_dbi(pci, offset + reg, val);
+	dw_pcie_writel_atu(pci, offset + reg, val);
 }
 
 static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, int index,
@@ -187,7 +187,7 @@ static u32 dw_pcie_readl_ib_unroll(struct dw_pcie *pci, u32 index, u32 reg)
 {
 	u32 offset = PCIE_GET_ATU_INB_UNR_REG_OFFSET(index);
 
-	return dw_pcie_readl_dbi(pci, offset + reg);
+	return dw_pcie_readl_atu(pci, offset + reg);
 }
 
 static void dw_pcie_writel_ib_unroll(struct dw_pcie *pci, u32 index, u32 reg,
@@ -195,7 +195,7 @@ static void dw_pcie_writel_ib_unroll(struct dw_pcie *pci, u32 index, u32 reg,
 {
 	u32 offset = PCIE_GET_ATU_INB_UNR_REG_OFFSET(index);
 
-	dw_pcie_writel_dbi(pci, offset + reg, val);
+	dw_pcie_writel_atu(pci, offset + reg, val);
 }
 
 static int dw_pcie_prog_inbound_atu_unroll(struct dw_pcie *pci, int index,
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 0989d880ac46..02fb532c5db9 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -93,11 +93,11 @@
 #define PCIE_ATU_UNR_UPPER_TARGET	0x18
 
 /* Register address builder */
-#define PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(region)	\
-			((0x3 << 20) | ((region) << 9))
+#define PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(region) \
+		((region) << 9)
 
-#define PCIE_GET_ATU_INB_UNR_REG_OFFSET(region)				\
-			((0x3 << 20) | ((region) << 9) | (0x1 << 8))
+#define PCIE_GET_ATU_INB_UNR_REG_OFFSET(region) \
+		((region) << 9) | (0x1 << 8)
 
 #define MAX_MSI_IRQS			256
 #define MAX_MSI_IRQS_PER_CTRL		32
@@ -219,6 +219,8 @@ struct dw_pcie {
 	struct device		*dev;
 	void __iomem		*dbi_base;
 	void __iomem		*dbi_base2;
+	/* Used when iatu_unroll_enabled is true */
+	void __iomem		*atu_base;
 	u32			num_viewport;
 	u8			iatu_unroll_enabled;
 	struct pcie_port	pp;
@@ -289,6 +291,16 @@ static inline u32 dw_pcie_readl_dbi2(struct dw_pcie *pci, u32 reg)
 	return __dw_pcie_read_dbi(pci, pci->dbi_base2, reg, 0x4);
 }
 
+static inline void dw_pcie_writel_atu(struct dw_pcie *pci, u32 reg, u32 val)
+{
+	__dw_pcie_write_dbi(pci, pci->atu_base, reg, 0x4, val);
+}
+
+static inline u32 dw_pcie_readl_atu(struct dw_pcie *pci, u32 reg)
+{
+	return __dw_pcie_read_dbi(pci, pci->atu_base, reg, 0x4);
+}
+
 static inline void dw_pcie_dbi_ro_wr_en(struct dw_pcie *pci)
 {
 	u32 reg;
-- 
2.19.1

Re: [PATCH] PCI: designware: don't hard-code DBI/ATU offset

[Gustavo Pimentel]

Hi Stephen,

I'd suggest to change the designware on the patch name by dwc and use another
description for the patch instead of don't hard-code DBI/ATU offset.

On 12/11/2018 22:57, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> The DWC PCIe core contains various separate register spaces: DBI, DBI2,
> ATU, DMA, etc. The relationship between the addresses of these register
> spaces is entirely determined by the implementation of the IP block, not
> by the IP block design itself. Hence, the DWC driver must not make
> assumptions that one register space can be accessed at a fixed offset from
> any other register space. To avoid such assumptions, introduce an
> explicit/separate register pointer for the ATU register space. In
> particular, the current assumption is not valid for NVIDIA's T194 SoC.
> 

If I understood this patch correctly, you basically replace the dbi_base offset
by atu_base offset that still depends of dbi_base offset. I agree the code is
now more readable, but I'm not seeing it by applying this patch what is the
benefit since you are not allowing to change the current atu_base. I though that
was the propose of this patch.

> The ATU register space is only used on systems that require unrolled ATU
> access. This property is detected at run-time for host controllers, and
> when this is detected, this patch provides a default value for atu_base
> that matches the previous assumption re: register layout. An alternative
> would be to update all drivers for HW that requires unrolled access to
> explicitly set atu_base. However, it's hard to tell which drivers would
> require atu_base to be set. The unrolled property is not detected for
> endpoint systems, and so any endpoint driver that requires unrolled access
> must explicitly set the iatu_unroll_enabled flag (none do at present), and
> so a check is added to require the driver to also set atu_base while at
> it.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   |  4 ++++
>  .../pci/controller/dwc/pcie-designware-host.c |  3 +++
>  drivers/pci/controller/dwc/pcie-designware.c  |  8 ++++----
>  drivers/pci/controller/dwc/pcie-designware.h  | 20 +++++++++++++++----
>  4 files changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 1e7b02221eac..880210366e71 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -504,6 +504,10 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  		dev_err(dev, "dbi_base/dbi_base2 is not populated\n");
>  		return -EINVAL;
>  	}
> +	if (pci->iatu_unroll_enabled && !pci->atu_base) {
> +		dev_err(dev, "atu_base is not populated\n");
> +		return -EINVAL;
> +	}
>  
>  	ret = of_property_read_u32(np, "num-ib-windows", &ep->num_ib_windows);
>  	if (ret < 0) {
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 29a05759a294..2ebb7f4768cf 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -699,6 +699,9 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>  		dev_dbg(pci->dev, "iATU unroll: %s\n",
>  			pci->iatu_unroll_enabled ? "enabled" : "disabled");
>  
> +		if (pci->iatu_unroll_enabled && !pci->atu_base)
> +			pci->atu_base = pci->dbi_base + (0x3 << 20);
> +
>  		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX0,
>  					  PCIE_ATU_TYPE_MEM, pp->mem_base,
>  					  pp->mem_bus_addr, pp->mem_size);
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 2153956a0b20..93ef8c31fb39 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -93,7 +93,7 @@ static u32 dw_pcie_readl_ob_unroll(struct dw_pcie *pci, u32 index, u32 reg)
>  {
>  	u32 offset = PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(index);
>  
> -	return dw_pcie_readl_dbi(pci, offset + reg);
> +	return dw_pcie_readl_atu(pci, offset + reg);
>  }
>  
>  static void dw_pcie_writel_ob_unroll(struct dw_pcie *pci, u32 index, u32 reg,
> @@ -101,7 +101,7 @@ static void dw_pcie_writel_ob_unroll(struct dw_pcie *pci, u32 index, u32 reg,
>  {
>  	u32 offset = PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(index);
>  
> -	dw_pcie_writel_dbi(pci, offset + reg, val);
> +	dw_pcie_writel_atu(pci, offset + reg, val);
>  }
>  
>  static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, int index,
> @@ -187,7 +187,7 @@ static u32 dw_pcie_readl_ib_unroll(struct dw_pcie *pci, u32 index, u32 reg)
>  {
>  	u32 offset = PCIE_GET_ATU_INB_UNR_REG_OFFSET(index);
>  
> -	return dw_pcie_readl_dbi(pci, offset + reg);
> +	return dw_pcie_readl_atu(pci, offset + reg);
>  }
>  
>  static void dw_pcie_writel_ib_unroll(struct dw_pcie *pci, u32 index, u32 reg,
> @@ -195,7 +195,7 @@ static void dw_pcie_writel_ib_unroll(struct dw_pcie *pci, u32 index, u32 reg,
>  {
>  	u32 offset = PCIE_GET_ATU_INB_UNR_REG_OFFSET(index);
>  
> -	dw_pcie_writel_dbi(pci, offset + reg, val);
> +	dw_pcie_writel_atu(pci, offset + reg, val);
>  }
>  
>  static int dw_pcie_prog_inbound_atu_unroll(struct dw_pcie *pci, int index,
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 0989d880ac46..02fb532c5db9 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -93,11 +93,11 @@
>  #define PCIE_ATU_UNR_UPPER_TARGET	0x18
>  
>  /* Register address builder */
> -#define PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(region)	\
> -			((0x3 << 20) | ((region) << 9))
> +#define PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(region) \
> +		((region) << 9)

I'd suggest to remove the parenthesis around the region variable, like this:
(region << 9)

>  
> -#define PCIE_GET_ATU_INB_UNR_REG_OFFSET(region)				\
> -			((0x3 << 20) | ((region) << 9) | (0x1 << 8))
> +#define PCIE_GET_ATU_INB_UNR_REG_OFFSET(region) \
> +		((region) << 9) | (0x1 << 8)

I'd suggest to remove the parenthesis around the region variable and to put a
parenthesis in the whole macro computation, like this:
((region << 9) | (0x1 << 8))

Regards,
Gustavo

>  
>  #define MAX_MSI_IRQS			256
>  #define MAX_MSI_IRQS_PER_CTRL		32
> @@ -219,6 +219,8 @@ struct dw_pcie {
>  	struct device		*dev;
>  	void __iomem		*dbi_base;
>  	void __iomem		*dbi_base2;
> +	/* Used when iatu_unroll_enabled is true */
> +	void __iomem		*atu_base;
>  	u32			num_viewport;
>  	u8			iatu_unroll_enabled;
>  	struct pcie_port	pp;
> @@ -289,6 +291,16 @@ static inline u32 dw_pcie_readl_dbi2(struct dw_pcie *pci, u32 reg)
>  	return __dw_pcie_read_dbi(pci, pci->dbi_base2, reg, 0x4);
>  }
>  
> +static inline void dw_pcie_writel_atu(struct dw_pcie *pci, u32 reg, u32 val)
> +{
> +	__dw_pcie_write_dbi(pci, pci->atu_base, reg, 0x4, val);
> +}
> +
> +static inline u32 dw_pcie_readl_atu(struct dw_pcie *pci, u32 reg)
> +{
> +	return __dw_pcie_read_dbi(pci, pci->atu_base, reg, 0x4);
> +}
> +
>  static inline void dw_pcie_dbi_ro_wr_en(struct dw_pcie *pci)
>  {
>  	u32 reg;
> 

Re: [PATCH] PCI: designware: don't hard-code DBI/ATU offset

[Stephen Warren]

On 11/12/18 9:19 PM, Gustavo Pimentel wrote:
> Hi Stephen,
> 
> I'd suggest to change the designware on the patch name by dwc and use another
> description for the patch instead of don't hard-code DBI/ATU offset.

What issue do you see with the patch description? I believe it's
correct/accurate. Perhaps the confusion is due to the question you
raised below; see my answer there.

> On 12/11/2018 22:57, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> The DWC PCIe core contains various separate register spaces: DBI, DBI2,
>> ATU, DMA, etc. The relationship between the addresses of these register
>> spaces is entirely determined by the implementation of the IP block, not
>> by the IP block design itself. Hence, the DWC driver must not make
>> assumptions that one register space can be accessed at a fixed offset from
>> any other register space. To avoid such assumptions, introduce an
>> explicit/separate register pointer for the ATU register space. In
>> particular, the current assumption is not valid for NVIDIA's T194 SoC.
> 
> If I understood this patch correctly, you basically replace the dbi_base offset
> by atu_base offset that still depends of dbi_base offset.

That's not what the patch does.

The patch leaves most DBI accesses still using dbi_base, but updates
just a few accesses to use atu_base. Thus, after the patch, all accesses
use the correct base address for the register being accessed.

There is a default value supplied for atu_base, which does indeed depend
on dbi_base. This maintains backwards compatibility, so that all the
existing drivers don't need to be updated to explicitly set atu_base,
and will continue to use the existing offset between DBI and ATU base.

In the future, we'll send a driver for the NVIDIA Tegra SoC which does
explicitly set atu_base to a non-default value.

> I agree the code is
> now more readable, but I'm not seeing it by applying this patch what is the
> benefit since you are not allowing to change the current atu_base. I though that
> was the propose of this patch.

atu_base may be set by the implementation-specific driver before calling
dw_pcie_ep_init(), or the host controller equivalent.

>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h

>> -#define PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(region)	\
>> -			((0x3 << 20) | ((region) << 9))
>> +#define PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(region) \
>> +		((region) << 9)
> 
> I'd suggest to remove the parenthesis around the region variable, like this:
> (region << 9)

Note that this is a pre-existing issue, and not caused/influenced by
this patch. The existing code is correct though; it's best practice to
always bracket all macro parameters, so that if the expression passed to
the macro contains operators, the macro implementation can guarantee the
correct calculation bracketing.

Re: [PATCH] PCI: designware: don't hard-code DBI/ATU offset

[Gustavo Pimentel]

Hi Stephen,

On 14/11/2018 04:31, Stephen Warren wrote:
> On 11/12/18 9:19 PM, Gustavo Pimentel wrote:
>> Hi Stephen,
>>
>> I'd suggest to change the designware on the patch name by dwc and use another
>> description for the patch instead of don't hard-code DBI/ATU offset.
> 
> What issue do you see with the patch description? I believe it's
> correct/accurate. Perhaps the confusion is due to the question you
> raised below; see my answer there.
> 
>> On 12/11/2018 22:57, Stephen Warren wrote:
>>> From: Stephen Warren <swarren@nvidia.com>
>>>
>>> The DWC PCIe core contains various separate register spaces: DBI, DBI2,
>>> ATU, DMA, etc. The relationship between the addresses of these register
>>> spaces is entirely determined by the implementation of the IP block, not
>>> by the IP block design itself. Hence, the DWC driver must not make
>>> assumptions that one register space can be accessed at a fixed offset from
>>> any other register space. To avoid such assumptions, introduce an
>>> explicit/separate register pointer for the ATU register space. In
>>> particular, the current assumption is not valid for NVIDIA's T194 SoC.
>>
>> If I understood this patch correctly, you basically replace the dbi_base offset
>> by atu_base offset that still depends of dbi_base offset.
> 
> That's not what the patch does.
> 
> The patch leaves most DBI accesses still using dbi_base, but updates
> just a few accesses to use atu_base. Thus, after the patch, all accesses
> use the correct base address for the register being accessed.
> 
> There is a default value supplied for atu_base, which does indeed depend
> on dbi_base. This maintains backwards compatibility, so that all the
> existing drivers don't need to be updated to explicitly set atu_base,
> and will continue to use the existing offset between DBI and ATU base.


I think we're talking about the same thing, but with different terms.

> 
> In the future, we'll send a driver for the NVIDIA Tegra SoC which does
> explicitly set atu_base to a non-default value.

That's what I wanted to know. Because otherwise this patch was just to turn the
code more readable.

> 
>> I agree the code is
>> now more readable, but I'm not seeing it by applying this patch what is the
>> benefit since you are not allowing to change the current atu_base. I though that
>> was the propose of this patch.
> 
> atu_base may be set by the implementation-specific driver before calling
> dw_pcie_ep_init(), or the host controller equivalent.

Agree, as long that maintains the retro-compatibility.

> 
>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> 
>>> -#define PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(region)	\
>>> -			((0x3 << 20) | ((region) << 9))
>>> +#define PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(region) \
>>> +		((region) << 9)
>>
>> I'd suggest to remove the parenthesis around the region variable, like this:
>> (region << 9)
> 
> Note that this is a pre-existing issue, and not caused/influenced by
> this patch. The existing code is correct though; it's best practice to
> always bracket all macro parameters, so that if the expression passed to
> the macro contains operators, the macro implementation can guarantee the
> correct calculation bracketing.
> 

Well seen.

Regards,
Gustavo

Re: [PATCH] PCI: designware: don't hard-code DBI/ATU offset

[Stephen Warren]

On 11/14/18 9:33 PM, Gustavo Pimentel wrote:
> Hi Stephen,
> 
> On 14/11/2018 04:31, Stephen Warren wrote:
>> On 11/12/18 9:19 PM, Gustavo Pimentel wrote:
>>> Hi Stephen,
>>>
>>> I'd suggest to change the designware on the patch name by dwc and use another
>>> description for the patch instead of don't hard-code DBI/ATU offset.
>>
>> What issue do you see with the patch description? I believe it's
>> correct/accurate. Perhaps the confusion is due to the question you
>> raised below; see my answer there.
>>
>>> On 12/11/2018 22:57, Stephen Warren wrote:
>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>
>>>> The DWC PCIe core contains various separate register spaces: DBI, DBI2,
>>>> ATU, DMA, etc. The relationship between the addresses of these register
>>>> spaces is entirely determined by the implementation of the IP block, not
>>>> by the IP block design itself. Hence, the DWC driver must not make
>>>> assumptions that one register space can be accessed at a fixed offset from
>>>> any other register space. To avoid such assumptions, introduce an
>>>> explicit/separate register pointer for the ATU register space. In
>>>> particular, the current assumption is not valid for NVIDIA's T194 SoC.
>>>
>>> If I understood this patch correctly, you basically replace the dbi_base offset
>>> by atu_base offset that still depends of dbi_base offset.
>>
>> That's not what the patch does.
>>
>> The patch leaves most DBI accesses still using dbi_base, but updates
>> just a few accesses to use atu_base. Thus, after the patch, all accesses
>> use the correct base address for the register being accessed.
>>
>> There is a default value supplied for atu_base, which does indeed depend
>> on dbi_base. This maintains backwards compatibility, so that all the
>> existing drivers don't need to be updated to explicitly set atu_base,
>> and will continue to use the existing offset between DBI and ATU base.
> 
> I think we're talking about the same thing, but with different terms.
> 
>> In the future, we'll send a driver for the NVIDIA Tegra SoC which does
>> explicitly set atu_base to a non-default value.
> 
> That's what I wanted to know. Because otherwise this patch was just to turn the
> code more readable.

So it sounds like I've addressed your questions? If so, is the next step
for you to ack the patch and Bjorn to apply it? Thanks!

Re: [PATCH] PCI: designware: don't hard-code DBI/ATU offset

[Kishon Vijay Abraham I]

Hi,

On 15/11/18 11:54 PM, Stephen Warren wrote:
> On 11/14/18 9:33 PM, Gustavo Pimentel wrote:
>> Hi Stephen,
>>
>> On 14/11/2018 04:31, Stephen Warren wrote:
>>> On 11/12/18 9:19 PM, Gustavo Pimentel wrote:
>>>> Hi Stephen,
>>>>
>>>> I'd suggest to change the designware on the patch name by dwc and use another
>>>> description for the patch instead of don't hard-code DBI/ATU offset.
>>>
>>> What issue do you see with the patch description? I believe it's
>>> correct/accurate. Perhaps the confusion is due to the question you
>>> raised below; see my answer there.
>>>
>>>> On 12/11/2018 22:57, Stephen Warren wrote:
>>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>>
>>>>> The DWC PCIe core contains various separate register spaces: DBI, DBI2,
>>>>> ATU, DMA, etc. The relationship between the addresses of these register
>>>>> spaces is entirely determined by the implementation of the IP block, not
>>>>> by the IP block design itself. Hence, the DWC driver must not make
>>>>> assumptions that one register space can be accessed at a fixed offset from
>>>>> any other register space. To avoid such assumptions, introduce an
>>>>> explicit/separate register pointer for the ATU register space. In
>>>>> particular, the current assumption is not valid for NVIDIA's T194 SoC.
>>>>
>>>> If I understood this patch correctly, you basically replace the dbi_base offset
>>>> by atu_base offset that still depends of dbi_base offset.
>>>
>>> That's not what the patch does.
>>>
>>> The patch leaves most DBI accesses still using dbi_base, but updates
>>> just a few accesses to use atu_base. Thus, after the patch, all accesses
>>> use the correct base address for the register being accessed.
>>>
>>> There is a default value supplied for atu_base, which does indeed depend
>>> on dbi_base. This maintains backwards compatibility, so that all the
>>> existing drivers don't need to be updated to explicitly set atu_base,
>>> and will continue to use the existing offset between DBI and ATU base.
>>
>> I think we're talking about the same thing, but with different terms.
>>
>>> In the future, we'll send a driver for the NVIDIA Tegra SoC which does
>>> explicitly set atu_base to a non-default value.
>>
>> That's what I wanted to know. Because otherwise this patch was just to turn the
>> code more readable.
> 
> So it sounds like I've addressed your questions? If so, is the next step
> for you to ack the patch and Bjorn to apply it? Thanks!

I had posted a patch sometime back to fix the same issue of hardcoding 
ATU offset [1]. However that patch also fixed ATU identification logic. 
I can post that as a separate patch if Lorenzo wants to merge this one.

FWIW:
Acked-by: Kishon Vijay Abraham I <kishon@ti.com>

Thanks
Kishon

[1] -> https://lkml.org/lkml/2018/9/21/484
> 

Re: [PATCH] PCI: designware: don't hard-code DBI/ATU offset

[Stephen Warren]

On 11/20/18 2:41 AM, Kishon Vijay Abraham I wrote:
> On 15/11/18 11:54 PM, Stephen Warren wrote:
>> On 11/14/18 9:33 PM, Gustavo Pimentel wrote:
>>> On 14/11/2018 04:31, Stephen Warren wrote:
>>>> On 11/12/18 9:19 PM, Gustavo Pimentel wrote:
>>>>> On 12/11/2018 22:57, Stephen Warren wrote:
>>>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>>>
>>>>>> The DWC PCIe core contains various separate register spaces: DBI, DBI2,
>>>>>> ATU, DMA, etc. The relationship between the addresses of these register
>>>>>> spaces is entirely determined by the implementation of the IP block, not
>>>>>> by the IP block design itself. Hence, the DWC driver must not make
>>>>>> assumptions that one register space can be accessed at a fixed offset from
>>>>>> any other register space. To avoid such assumptions, introduce an
>>>>>> explicit/separate register pointer for the ATU register space. In
>>>>>> particular, the current assumption is not valid for NVIDIA's T194 
>>>>>> SoC.
>>>>>
>>>>> If I understood this patch correctly, you basically replace the dbi_base offset
>>>>> by atu_base offset that still depends of dbi_base offset.
>>>>
>>>> That's not what the patch does.
>>>>
>>>> The patch leaves most DBI accesses still using dbi_base, but updates
>>>> just a few accesses to use atu_base. Thus, after the patch, all accesses
>>>> use the correct base address for the register being accessed.
>>>>
>>>> There is a default value supplied for atu_base, which does indeed depend
>>>> on dbi_base. This maintains backwards compatibility, so that all the
>>>> existing drivers don't need to be updated to explicitly set atu_base,
>>>> and will continue to use the existing offset between DBI and ATU base.
>>>
>>> I think we're talking about the same thing, but with different terms.
>>>
>>>> In the future, we'll send a driver for the NVIDIA Tegra SoC which does
>>>> explicitly set atu_base to a non-default value.
>>>
>>> That's what I wanted to know. Because otherwise this patch was just 
>>> to turn the code more readable.
>>
>> So it sounds like I've addressed your questions? If so, is the next step
>> for you to ack the patch and Bjorn to apply it? Thanks!
> 
> I had posted a patch sometime back to fix the same issue of hardcoding 
> ATU offset [1]. However that patch also fixed ATU identification logic. 
> I can post that as a separate patch if Lorenzo wants to merge this one.
> 
> FWIW:
> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> 
> Thanks
> Kishon
> 
> [1] -> https://lkml.org/lkml/2018/9/21/484

Ah, interesting. I see one problem with that patch on NVIDIA hardware. 
Specifically, it looks like your patch enhances the driver to always set 
iatu_unroll_enabled in dw_pcie_setup() for either RP or EP mode? If so, 
that won't work well for us since the DBI registers can't be accessed at 
that time on our HW. Still, in our particular case, the driver would set 
the version number field and so that register access would actually be 
avoided, so the issue won't actually happen, so maybe the patch is OK... 
I'd love to see at least one of the patches applied.

Re: [PATCH] PCI: designware: don't hard-code DBI/ATU offset

[Gustavo Pimentel]

On 15/11/2018 18:24, Stephen Warren wrote:
> On 11/14/18 9:33 PM, Gustavo Pimentel wrote:
>> Hi Stephen,
>>
>> On 14/11/2018 04:31, Stephen Warren wrote:
>>> On 11/12/18 9:19 PM, Gustavo Pimentel wrote:
>>>> Hi Stephen,
>>>>
>>>> I'd suggest to change the designware on the patch name by dwc and use another
>>>> description for the patch instead of don't hard-code DBI/ATU offset.
>>>
>>> What issue do you see with the patch description? I believe it's
>>> correct/accurate. Perhaps the confusion is due to the question you
>>> raised below; see my answer there.
>>>
>>>> On 12/11/2018 22:57, Stephen Warren wrote:
>>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>>
>>>>> The DWC PCIe core contains various separate register spaces: DBI, DBI2,
>>>>> ATU, DMA, etc. The relationship between the addresses of these register
>>>>> spaces is entirely determined by the implementation of the IP block, not
>>>>> by the IP block design itself. Hence, the DWC driver must not make
>>>>> assumptions that one register space can be accessed at a fixed offset from
>>>>> any other register space. To avoid such assumptions, introduce an
>>>>> explicit/separate register pointer for the ATU register space. In
>>>>> particular, the current assumption is not valid for NVIDIA's T194 SoC.
>>>>
>>>> If I understood this patch correctly, you basically replace the dbi_base offset
>>>> by atu_base offset that still depends of dbi_base offset.
>>>
>>> That's not what the patch does.
>>>
>>> The patch leaves most DBI accesses still using dbi_base, but updates
>>> just a few accesses to use atu_base. Thus, after the patch, all accesses
>>> use the correct base address for the register being accessed.
>>>
>>> There is a default value supplied for atu_base, which does indeed depend
>>> on dbi_base. This maintains backwards compatibility, so that all the
>>> existing drivers don't need to be updated to explicitly set atu_base,
>>> and will continue to use the existing offset between DBI and ATU base.
>>
>> I think we're talking about the same thing, but with different terms.
>>
>>> In the future, we'll send a driver for the NVIDIA Tegra SoC which does
>>> explicitly set atu_base to a non-default value.
>>
>> That's what I wanted to know. Because otherwise this patch was just to turn the
>> code more readable.
> 
> So it sounds like I've addressed your questions? If so, is the next step
> for you to ack the patch and Bjorn to apply it? Thanks!
> 

Please change "designware" by "dwc" on the title (which is the normal tag) as I
suggested and put the first letter of "don't" in capital letter.

With that fix, please re-submit patch version.

Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>

Thanks.

Regards,
Gustavo