[PATCH V2] PCI: dwc: Don't hard-code DBI/ATU offset

[PATCH V2] PCI: dwc: 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>
Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
v2:
* Modified patch subject
* Added missing outer brackets to PCIE_GET_ATU_INB_UNR_REG_OFFSET macro
---
 .../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..e895f37b1dee 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 V2] PCI: dwc: Don't hard-code DBI/ATU offset

[Vidya Sagar]

On 11/20/2018 11:27 PM, 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.
>
> 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>
> Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
> v2:
> * Modified patch subject
> * Added missing outer brackets to PCIE_GET_ATU_INB_UNR_REG_OFFSET macro
> ---
>   .../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");

I see that dw_pcie_setup_rc() API has code where 
pci->iatu_unroll_enabled gets set based on reading a viewport register.

IMO, we need a check like following to avoid pci->iatu_unroll_enabled 
getting modified (if already set)

if (!pci->iatu_unroll_enabled)
     pci->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pci);

>   
> +		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..e895f37b1dee 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;
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [PATCH V2] PCI: dwc: Don't hard-code DBI/ATU offset

[Stephen Warren]

On 11/25/18 10:51 PM, Vidya Sagar wrote:
> 
> On 11/20/2018 11:27 PM, 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.
>>
>> 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>
>> Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> ---
>> v2:
>> * Modified patch subject
>> * Added missing outer brackets to PCIE_GET_ATU_INB_UNR_REG_OFFSET macro
>> ---
>>   .../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");
> 
> I see that dw_pcie_setup_rc() API has code where 
> pci->iatu_unroll_enabled gets set based on reading a viewport register.
> 
> IMO, we need a check like following to avoid pci->iatu_unroll_enabled 
> getting modified (if already set)
> 
> if (!pci->iatu_unroll_enabled)
>      pci->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pci);

That's only true if you believe that dw_pcie_iatu_unroll_enabled() will 
give the wrong answer in some circumstances. It's working OK right now, 
so I'm not sure why that function would start having problems?

Re: [PATCH V2] PCI: dwc: Don't hard-code DBI/ATU offset

[Vidya Sagar]

On 11/26/2018 10:24 PM, Stephen Warren wrote:
> On 11/25/18 10:51 PM, Vidya Sagar wrote:
>>
>> On 11/20/2018 11:27 PM, 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.
>>>
>>> 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>
>>> Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>> ---
>>> v2:
>>> * Modified patch subject
>>> * Added missing outer brackets to PCIE_GET_ATU_INB_UNR_REG_OFFSET macro
>>> ---
>>>   .../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");
>>
>> I see that dw_pcie_setup_rc() API has code where 
>> pci->iatu_unroll_enabled gets set based on reading a viewport register.
>>
>> IMO, we need a check like following to avoid pci->iatu_unroll_enabled 
>> getting modified (if already set)
>>
>> if (!pci->iatu_unroll_enabled)
>>      pci->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pci);
>
> That's only true if you believe that dw_pcie_iatu_unroll_enabled() 
> will give the wrong answer in some circumstances. It's working OK 
> right now, so I'm not sure why that function would start having problems?
Since dw_pcie_iatu_unroll_enabled() is reading PCIE_ATU_VIEWPORT 
register and in case of Nvidia's DWC based PCIe implementation, this 
register is not listed. In case, if it is returning '0', then, 
pci->iatu_unroll_enabled which was set by implementation specific driver 
gets overwritten with this value. Though it is working fine now, I'm not 
sure if we can take this for granted that it would always work.

Re: [PATCH V2] PCI: dwc: Don't hard-code DBI/ATU offset

[Stephen Warren]

On 11/26/18 8:52 PM, Vidya Sagar wrote:
> 
> On 11/26/2018 10:24 PM, Stephen Warren wrote:
>> On 11/25/18 10:51 PM, Vidya Sagar wrote:
>>>
>>> On 11/20/2018 11:27 PM, 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.
>>>>
>>>> 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>
>>>> Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>>> ---
>>>> v2:
>>>> * Modified patch subject
>>>> * Added missing outer brackets to PCIE_GET_ATU_INB_UNR_REG_OFFSET macro
>>>> ---
>>>>   .../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");
>>>
>>> I see that dw_pcie_setup_rc() API has code where 
>>> pci->iatu_unroll_enabled gets set based on reading a viewport register.
>>>
>>> IMO, we need a check like following to avoid pci->iatu_unroll_enabled 
>>> getting modified (if already set)
>>>
>>> if (!pci->iatu_unroll_enabled)
>>>      pci->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pci);
>>
>> That's only true if you believe that dw_pcie_iatu_unroll_enabled() 
>> will give the wrong answer in some circumstances. It's working OK 
>> right now, so I'm not sure why that function would start having problems?
 >
> Since dw_pcie_iatu_unroll_enabled() is reading PCIE_ATU_VIEWPORT 
> register and in case of Nvidia's DWC based PCIe implementation, this 
> register is not listed. In case, if it is returning '0', then, 
> pci->iatu_unroll_enabled which was set by implementation specific driver 
> gets overwritten with this value. Though it is working fine now, I'm not 
> sure if we can take this for granted that it would always work.

For current hardware, I think we can assume this works, because it does 
now. The HW behaviour isn't going to suddenly change for a specific chip 
that's already designed. The behaviour seems to be a property of the 
core DWC HW since the driver already relies on it for multiple different 
vendor uses of the core DWC HW. If for some reason it turns out that 
this doesn't work for a future HW design, we can always fix it up then.

I don't think this patch affects this issue at all though. If you want 
to modify the DWC core and all DWC HW-specific drivers so that the 
HW-specific drivers always set iatu_unroll_enabled when required, rather 
than auto-detecting the value, that seems fine, but I think you should 
send a separate patch for that.

Re: [PATCH V2] PCI: dwc: Don't hard-code DBI/ATU offset

[Vidya Sagar]

On 27/11/18 11:41 PM, Stephen Warren wrote:
> On 11/26/18 8:52 PM, Vidya Sagar wrote:
>>
>> On 11/26/2018 10:24 PM, Stephen Warren wrote:
>>> On 11/25/18 10:51 PM, Vidya Sagar wrote:
>>>>
>>>> On 11/20/2018 11:27 PM, 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.
>>>>>
>>>>> 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>
>>>>> Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>>>> ---
>>>>> v2:
>>>>> * Modified patch subject
>>>>> * Added missing outer brackets to PCIE_GET_ATU_INB_UNR_REG_OFFSET 
>>>>> macro
>>>>> ---
>>>>>   .../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");
>>>>
>>>> I see that dw_pcie_setup_rc() API has code where 
>>>> pci->iatu_unroll_enabled gets set based on reading a viewport 
>>>> register.
>>>>
>>>> IMO, we need a check like following to avoid 
>>>> pci->iatu_unroll_enabled getting modified (if already set)
>>>>
>>>> if (!pci->iatu_unroll_enabled)
>>>>      pci->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pci);
>>>
>>> That's only true if you believe that dw_pcie_iatu_unroll_enabled() 
>>> will give the wrong answer in some circumstances. It's working OK 
>>> right now, so I'm not sure why that function would start having 
>>> problems?
> >
>> Since dw_pcie_iatu_unroll_enabled() is reading PCIE_ATU_VIEWPORT 
>> register and in case of Nvidia's DWC based PCIe implementation, this 
>> register is not listed. In case, if it is returning '0', then, 
>> pci->iatu_unroll_enabled which was set by implementation specific 
>> driver gets overwritten with this value. Though it is working fine 
>> now, I'm not sure if we can take this for granted that it would 
>> always work.
>
> For current hardware, I think we can assume this works, because it 
> does now. The HW behaviour isn't going to suddenly change for a 
> specific chip that's already designed. The behaviour seems to be a 
> property of the core DWC HW since the driver already relies on it for 
> multiple different vendor uses of the core DWC HW. If for some reason 
> it turns out that this doesn't work for a future HW design, we can 
> always fix it up then.
>
> I don't think this patch affects this issue at all though. If you want 
> to modify the DWC core and all DWC HW-specific drivers so that the 
> HW-specific drivers always set iatu_unroll_enabled when required, 
> rather than auto-detecting the value, that seems fine, but I think you 
> should send a separate patch for that.

Fine. I'm convinced with the explanation.

Acked-by: Vidya Sagar <vidyas@nvidia.com>

Re: [PATCH V2] PCI: dwc: Don't hard-code DBI/ATU offset

[Lorenzo Pieralisi]

On Tue, Nov 20, 2018 at 10:57:50AM -0700, 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.
> 
> 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>
> Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
> v2:
> * Modified patch subject
> * Added missing outer brackets to PCIE_GET_ATU_INB_UNR_REG_OFFSET macro
> ---
>  .../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);

Hi Stephen,

I think the patch is sound, minor nit, do you think that making

(0x3 << 20)

a preprocessor macro (and a comment on the default initialization above)
can help clarify the code ?

eg DEFAULT_ATU_BASE_OFFSET

or something like that.

Please let me know, I can merge the patch as-is, it is just to improve
readability.

Thanks,
Lorenzo

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