[PATCH RESEND v4 03/15] PCI: dwc: Convert to using native IP-core versions representation

[PATCH RESEND v4 03/15] PCI: dwc: Convert to using native IP-core versions representation

[Serge Semin]

Since DWC PCIe v4.70a the controller version can be read from the
PORT_LOGIC.PCIE_VERSION_OFF register. Version is represented in the FourCC
format [1]. It's standard versioning approach for the Synopsys DWC
IP-cores. Moreover some of the DWC kernel drivers already make use of it
to fixup version-dependent functionality (See DWC USB3, Stmicro STMMAC or
recent DW SPI driver). In order to preserve the standard version
representation and prevent the data conversion back and forth, we suggest
to preserve the native version representation in the DWC PCIe driver too
in the same way as it has already been done in the rest of the DWC
drivers. IP-core version reading from the CSR will be introduced in the
next commit together with a simple macro-based API to use it.

[1] https://en.wikipedia.org/wiki/FourCC

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 drivers/pci/controller/dwc/pci-keystone.c    | 12 ++++++------
 drivers/pci/controller/dwc/pcie-designware.c |  8 ++++----
 drivers/pci/controller/dwc/pcie-designware.h | 10 +++++++++-
 drivers/pci/controller/dwc/pcie-intel-gw.c   |  4 ++--
 drivers/pci/controller/dwc/pcie-tegra194.c   |  2 +-
 5 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index c3d88aa27dd4..c4ab3d775a18 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -109,7 +109,7 @@ struct ks_pcie_of_data {
 	enum dw_pcie_device_mode mode;
 	const struct dw_pcie_host_ops *host_ops;
 	const struct dw_pcie_ep_ops *ep_ops;
-	unsigned int version;
+	u32 version;
 };
 
 struct keystone_pcie {
@@ -1069,19 +1069,19 @@ static int ks_pcie_am654_set_mode(struct device *dev,
 
 static const struct ks_pcie_of_data ks_pcie_rc_of_data = {
 	.host_ops = &ks_pcie_host_ops,
-	.version = 0x365A,
+	.version = DW_PCIE_VER_365A,
 };
 
 static const struct ks_pcie_of_data ks_pcie_am654_rc_of_data = {
 	.host_ops = &ks_pcie_am654_host_ops,
 	.mode = DW_PCIE_RC_TYPE,
-	.version = 0x490A,
+	.version = DW_PCIE_VER_490A,
 };
 
 static const struct ks_pcie_of_data ks_pcie_am654_ep_of_data = {
 	.ep_ops = &ks_pcie_am654_ep_ops,
 	.mode = DW_PCIE_EP_TYPE,
-	.version = 0x490A,
+	.version = DW_PCIE_VER_490A,
 };
 
 static const struct of_device_id ks_pcie_of_match[] = {
@@ -1114,12 +1114,12 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 	struct device_link **link;
 	struct gpio_desc *gpiod;
 	struct resource *res;
-	unsigned int version;
 	void __iomem *base;
 	u32 num_viewport;
 	struct phy **phy;
 	u32 num_lanes;
 	char name[10];
+	u32 version;
 	int ret;
 	int irq;
 	int i;
@@ -1233,7 +1233,7 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 		goto err_get_sync;
 	}
 
-	if (pci->version >= 0x480A)
+	if (pci->version >= DW_PCIE_VER_480A)
 		ret = ks_pcie_am654_set_mode(dev, mode);
 	else
 		ret = ks_pcie_set_mode(dev);
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index e66d16a86168..f10a7d5d94e8 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -289,7 +289,7 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no,
 	val = type | PCIE_ATU_FUNC_NUM(func_no);
 	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr))
 		val |= PCIE_ATU_INCREASE_REGION_SIZE;
-	if (pci->version == 0x490A)
+	if (pci->version == DW_PCIE_VER_490A)
 		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,
@@ -336,7 +336,7 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
 			   upper_32_bits(cpu_addr));
 	dw_pcie_writel_dbi(pci, PCIE_ATU_LIMIT,
 			   lower_32_bits(limit_addr));
-	if (pci->version >= 0x460A)
+	if (pci->version >= DW_PCIE_VER_460A)
 		dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_LIMIT,
 				   upper_32_bits(limit_addr));
 	dw_pcie_writel_dbi(pci, PCIE_ATU_LOWER_TARGET,
@@ -345,9 +345,9 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
 			   upper_32_bits(pci_addr));
 	val = type | PCIE_ATU_FUNC_NUM(func_no);
 	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr) &&
-	    pci->version >= 0x460A)
+	    pci->version >= DW_PCIE_VER_460A)
 		val |= PCIE_ATU_INCREASE_REGION_SIZE;
-	if (pci->version == 0x490A)
+	if (pci->version == DW_PCIE_VER_490A)
 		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 32df3ebccf19..6b81530fb2ca 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -20,6 +20,14 @@
 #include <linux/pci-epc.h>
 #include <linux/pci-epf.h>
 
+/* DWC PCIe IP-core versions (native support since v4.70a) */
+#define DW_PCIE_VER_365A		0x3336352a
+#define DW_PCIE_VER_460A		0x3436302a
+#define DW_PCIE_VER_470A		0x3437302a
+#define DW_PCIE_VER_480A		0x3438302a
+#define DW_PCIE_VER_490A		0x3439302a
+#define DW_PCIE_VER_520A		0x3532302a
+
 /* Parameters for the waiting for link up routine */
 #define LINK_WAIT_MAX_RETRIES		10
 #define LINK_WAIT_USLEEP_MIN		90000
@@ -270,7 +278,7 @@ struct dw_pcie {
 	struct dw_pcie_rp	pp;
 	struct dw_pcie_ep	ep;
 	const struct dw_pcie_ops *ops;
-	unsigned int		version;
+	u32			version;
 	int			num_lanes;
 	int			link_gen;
 	u8			n_fts[2];
diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c b/drivers/pci/controller/dwc/pcie-intel-gw.c
index 07bc54886d71..371b5aa189d1 100644
--- a/drivers/pci/controller/dwc/pcie-intel-gw.c
+++ b/drivers/pci/controller/dwc/pcie-intel-gw.c
@@ -59,7 +59,7 @@
 #define RESET_INTERVAL_MS		100
 
 struct intel_pcie_soc {
-	unsigned int	pcie_ver;
+	u32	pcie_ver;
 };
 
 struct intel_pcie {
@@ -395,7 +395,7 @@ static const struct dw_pcie_host_ops intel_pcie_dw_ops = {
 };
 
 static const struct intel_pcie_soc pcie_data = {
-	.pcie_ver =		0x520A,
+	.pcie_ver =		DW_PCIE_VER_520A,
 };
 
 static int intel_pcie_probe(struct platform_device *pdev)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index 65135f5c4a4a..f24b30b7454f 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1979,7 +1979,7 @@ static int tegra194_pcie_probe(struct platform_device *pdev)
 	pci->ops = &tegra_dw_pcie_ops;
 	pci->n_fts[0] = N_FTS_VAL;
 	pci->n_fts[1] = FTS_VAL;
-	pci->version = 0x490A;
+	pci->version = DW_PCIE_VER_490A;
 
 	pp = &pci->pp;
 	pp->num_vectors = MAX_MSI_IRQS;
-- 
2.35.1

Re: [PATCH RESEND v4 03/15] PCI: dwc: Convert to using native IP-core versions representation

[Bjorn Helgaas]

On Fri, Jun 24, 2022 at 05:39:35PM +0300, Serge Semin wrote:
> Since DWC PCIe v4.70a the controller version can be read from the
> PORT_LOGIC.PCIE_VERSION_OFF register. Version is represented in the FourCC
> format [1]. It's standard versioning approach for the Synopsys DWC
> IP-cores. Moreover some of the DWC kernel drivers already make use of it
> to fixup version-dependent functionality (See DWC USB3, Stmicro STMMAC or
> recent DW SPI driver).

These references to other drivers might be useful, but without a
function name or file name, I can't easily find them.

> In order to preserve the standard version
> representation and prevent the data conversion back and forth, we suggest
> to preserve the native version representation in the DWC PCIe driver too
> in the same way as it has already been done in the rest of the DWC
> drivers. IP-core version reading from the CSR will be introduced in the
> next commit together with a simple macro-based API to use it.
> 
> [1] https://en.wikipedia.org/wiki/FourCC

Re: [PATCH RESEND v4 03/15] PCI: dwc: Convert to using native IP-core versions representation

[Ben Dooks]

On 28/07/2022 16:24, Bjorn Helgaas wrote:
> On Fri, Jun 24, 2022 at 05:39:35PM +0300, Serge Semin wrote:
>> Since DWC PCIe v4.70a the controller version can be read from the
>> PORT_LOGIC.PCIE_VERSION_OFF register. Version is represented in the FourCC
>> format [1]. It's standard versioning approach for the Synopsys DWC
>> IP-cores. Moreover some of the DWC kernel drivers already make use of it
>> to fixup version-dependent functionality (See DWC USB3, Stmicro STMMAC or
>> recent DW SPI driver).
> 
> These references to other drivers might be useful, but without a
> function name or file name, I can't easily find them.
> 
>> In order to preserve the standard version
>> representation and prevent the data conversion back and forth, we suggest
>> to preserve the native version representation in the DWC PCIe driver too
>> in the same way as it has already been done in the rest of the DWC
>> drivers. IP-core version reading from the CSR will be introduced in the
>> next commit together with a simple macro-based API to use it.
>>
>> [1] https://en.wikipedia.org/wiki/FourCC
> 

I'm currently looking at a OF based dw-apb-timers-pwm driver, so also
would like to follow this.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

Re: [PATCH RESEND v4 03/15] PCI: dwc: Convert to using native IP-core versions representation

[Bjorn Helgaas]

On Thu, Jul 28, 2022 at 04:31:17PM +0100, Ben Dooks wrote:
> On 28/07/2022 16:24, Bjorn Helgaas wrote:
> > On Fri, Jun 24, 2022 at 05:39:35PM +0300, Serge Semin wrote:
> > > Since DWC PCIe v4.70a the controller version can be read from the
> > > PORT_LOGIC.PCIE_VERSION_OFF register. Version is represented in the FourCC
> > > format [1]. It's standard versioning approach for the Synopsys DWC
> > > IP-cores. Moreover some of the DWC kernel drivers already make use of it
> > > to fixup version-dependent functionality (See DWC USB3, Stmicro STMMAC or
> > > recent DW SPI driver).
> > 
> > These references to other drivers might be useful, but without a
> > function name or file name, I can't easily find them.
> > 
> > > In order to preserve the standard version
> > > representation and prevent the data conversion back and forth, we suggest
> > > to preserve the native version representation in the DWC PCIe driver too
> > > in the same way as it has already been done in the rest of the DWC
> > > drivers. IP-core version reading from the CSR will be introduced in the
> > > next commit together with a simple macro-based API to use it.
> > > 
> > > [1] https://en.wikipedia.org/wiki/FourCC
> 
> I'm currently looking at a OF based dw-apb-timers-pwm driver, so also
> would like to follow this.

FWIW, the breadcrumbs I found are:

  dwc3_core_is_valid()
  stmmac_hwif_init()
  dw_spi_hw_init()

Re: [PATCH RESEND v4 03/15] PCI: dwc: Convert to using native IP-core versions representation

[Serge Semin]

On Thu, Jul 28, 2022 at 10:24:18AM -0500, Bjorn Helgaas wrote:
> On Fri, Jun 24, 2022 at 05:39:35PM +0300, Serge Semin wrote:
> > Since DWC PCIe v4.70a the controller version can be read from the
> > PORT_LOGIC.PCIE_VERSION_OFF register. Version is represented in the FourCC
> > format [1]. It's standard versioning approach for the Synopsys DWC
> > IP-cores. Moreover some of the DWC kernel drivers already make use of it
> > to fixup version-dependent functionality (See DWC USB3, Stmicro STMMAC or
> > recent DW SPI driver).
> 

> These references to other drivers might be useful, but without a
> function name or file name, I can't easily find them.

1. DW APB SSI driver, defined in drivers/spi/spi-dw.h:
#define DW_HSSI_102A                    0x3130322a
#define dw_spi_ip_is(_dws, _ip) ...
#define dw_spi_ver_is(_dws, _ip, _ver) ...
#define dw_spi_ver_is_ge(_dws, _ip, _ver) ...

2. DWC USB3 driver, defined in drivers/usb/dwc3/core.h
#define DWC3_IP                 0x5533
#define DWC31_IP                0x3331
#define DWC32_IP                0x3332
#define DWC3_REVISION_173A      0x5533173a
#define DWC3_REVISION_175A      0x5533175a
...
#define DWC31_REVISION_110A     0x3131302a
#define DWC31_REVISION_120A     0x3132302a
...
#define DWC3_IP_IS(_ip) ...
#define DWC3_VER_IS(_ip, _ver) ...
#define DWC3_VER_IS_PRIOR(_ip, _ver) ...
#define DWC3_VER_IS_WITHIN(_ip, _from, _to) ...
#define DWC3_VER_TYPE_IS_WITHIN(_ip, _ver, _from, _to) ...

Regarding the STMMAC (DW MAC/GMAC/xGMAC) driver. I've harried up to
claim it has the native IP-core versioning support. The current kernel
driver doesn't have it. Instead I have it implemented in my local
repo, but it isn't ready to be submitted yet. I need some more time to
finish my DW GMAC/xGMAC work first.

Ideally we could have created a common interface for all the drivers.
I thought about it first when I was initially creating the patchsets.
But now I don't time left for this at all. The review process's taken
way much more time than I had/planned to spend for the PCIe-patches.
So what I can do at this stage is to provide minor fixes if required.

-Sergey

> 
> > In order to preserve the standard version
> > representation and prevent the data conversion back and forth, we suggest
> > to preserve the native version representation in the DWC PCIe driver too
> > in the same way as it has already been done in the rest of the DWC
> > drivers. IP-core version reading from the CSR will be introduced in the
> > next commit together with a simple macro-based API to use it.
> > 
> > [1] https://en.wikipedia.org/wiki/FourCC

Re: [PATCH RESEND v4 03/15] PCI: dwc: Convert to using native IP-core versions representation

[Manivannan Sadhasivam]

On Fri, Jun 24, 2022 at 05:39:35PM +0300, Serge Semin wrote:
> Since DWC PCIe v4.70a the controller version can be read from the
> PORT_LOGIC.PCIE_VERSION_OFF register. Version is represented in the FourCC
> format [1]. It's standard versioning approach for the Synopsys DWC
> IP-cores. Moreover some of the DWC kernel drivers already make use of it
> to fixup version-dependent functionality (See DWC USB3, Stmicro STMMAC or
> recent DW SPI driver). In order to preserve the standard version
> representation and prevent the data conversion back and forth, we suggest
> to preserve the native version representation in the DWC PCIe driver too
> in the same way as it has already been done in the rest of the DWC
> drivers. IP-core version reading from the CSR will be introduced in the
> next commit together with a simple macro-based API to use it.
> 
> [1] https://en.wikipedia.org/wiki/FourCC
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Reviewed-by: Rob Herring <robh@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  drivers/pci/controller/dwc/pci-keystone.c    | 12 ++++++------
>  drivers/pci/controller/dwc/pcie-designware.c |  8 ++++----
>  drivers/pci/controller/dwc/pcie-designware.h | 10 +++++++++-
>  drivers/pci/controller/dwc/pcie-intel-gw.c   |  4 ++--
>  drivers/pci/controller/dwc/pcie-tegra194.c   |  2 +-
>  5 files changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index c3d88aa27dd4..c4ab3d775a18 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -109,7 +109,7 @@ struct ks_pcie_of_data {
>  	enum dw_pcie_device_mode mode;
>  	const struct dw_pcie_host_ops *host_ops;
>  	const struct dw_pcie_ep_ops *ep_ops;
> -	unsigned int version;
> +	u32 version;
>  };
>  
>  struct keystone_pcie {
> @@ -1069,19 +1069,19 @@ static int ks_pcie_am654_set_mode(struct device *dev,
>  
>  static const struct ks_pcie_of_data ks_pcie_rc_of_data = {
>  	.host_ops = &ks_pcie_host_ops,
> -	.version = 0x365A,
> +	.version = DW_PCIE_VER_365A,
>  };
>  
>  static const struct ks_pcie_of_data ks_pcie_am654_rc_of_data = {
>  	.host_ops = &ks_pcie_am654_host_ops,
>  	.mode = DW_PCIE_RC_TYPE,
> -	.version = 0x490A,
> +	.version = DW_PCIE_VER_490A,
>  };
>  
>  static const struct ks_pcie_of_data ks_pcie_am654_ep_of_data = {
>  	.ep_ops = &ks_pcie_am654_ep_ops,
>  	.mode = DW_PCIE_EP_TYPE,
> -	.version = 0x490A,
> +	.version = DW_PCIE_VER_490A,
>  };
>  
>  static const struct of_device_id ks_pcie_of_match[] = {
> @@ -1114,12 +1114,12 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
>  	struct device_link **link;
>  	struct gpio_desc *gpiod;
>  	struct resource *res;
> -	unsigned int version;
>  	void __iomem *base;
>  	u32 num_viewport;
>  	struct phy **phy;
>  	u32 num_lanes;
>  	char name[10];
> +	u32 version;
>  	int ret;
>  	int irq;
>  	int i;
> @@ -1233,7 +1233,7 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
>  		goto err_get_sync;
>  	}
>  
> -	if (pci->version >= 0x480A)
> +	if (pci->version >= DW_PCIE_VER_480A)
>  		ret = ks_pcie_am654_set_mode(dev, mode);
>  	else
>  		ret = ks_pcie_set_mode(dev);
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index e66d16a86168..f10a7d5d94e8 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -289,7 +289,7 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no,
>  	val = type | PCIE_ATU_FUNC_NUM(func_no);
>  	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr))
>  		val |= PCIE_ATU_INCREASE_REGION_SIZE;
> -	if (pci->version == 0x490A)
> +	if (pci->version == DW_PCIE_VER_490A)
>  		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,
> @@ -336,7 +336,7 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
>  			   upper_32_bits(cpu_addr));
>  	dw_pcie_writel_dbi(pci, PCIE_ATU_LIMIT,
>  			   lower_32_bits(limit_addr));
> -	if (pci->version >= 0x460A)
> +	if (pci->version >= DW_PCIE_VER_460A)
>  		dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_LIMIT,
>  				   upper_32_bits(limit_addr));
>  	dw_pcie_writel_dbi(pci, PCIE_ATU_LOWER_TARGET,
> @@ -345,9 +345,9 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
>  			   upper_32_bits(pci_addr));
>  	val = type | PCIE_ATU_FUNC_NUM(func_no);
>  	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr) &&
> -	    pci->version >= 0x460A)
> +	    pci->version >= DW_PCIE_VER_460A)
>  		val |= PCIE_ATU_INCREASE_REGION_SIZE;
> -	if (pci->version == 0x490A)
> +	if (pci->version == DW_PCIE_VER_490A)
>  		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 32df3ebccf19..6b81530fb2ca 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -20,6 +20,14 @@
>  #include <linux/pci-epc.h>
>  #include <linux/pci-epf.h>
>  
> +/* DWC PCIe IP-core versions (native support since v4.70a) */
> +#define DW_PCIE_VER_365A		0x3336352a
> +#define DW_PCIE_VER_460A		0x3436302a
> +#define DW_PCIE_VER_470A		0x3437302a
> +#define DW_PCIE_VER_480A		0x3438302a
> +#define DW_PCIE_VER_490A		0x3439302a
> +#define DW_PCIE_VER_520A		0x3532302a
> +
>  /* Parameters for the waiting for link up routine */
>  #define LINK_WAIT_MAX_RETRIES		10
>  #define LINK_WAIT_USLEEP_MIN		90000
> @@ -270,7 +278,7 @@ struct dw_pcie {
>  	struct dw_pcie_rp	pp;
>  	struct dw_pcie_ep	ep;
>  	const struct dw_pcie_ops *ops;
> -	unsigned int		version;
> +	u32			version;
>  	int			num_lanes;
>  	int			link_gen;
>  	u8			n_fts[2];
> diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c b/drivers/pci/controller/dwc/pcie-intel-gw.c
> index 07bc54886d71..371b5aa189d1 100644
> --- a/drivers/pci/controller/dwc/pcie-intel-gw.c
> +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c
> @@ -59,7 +59,7 @@
>  #define RESET_INTERVAL_MS		100
>  
>  struct intel_pcie_soc {
> -	unsigned int	pcie_ver;
> +	u32	pcie_ver;
>  };
>  
>  struct intel_pcie {
> @@ -395,7 +395,7 @@ static const struct dw_pcie_host_ops intel_pcie_dw_ops = {
>  };
>  
>  static const struct intel_pcie_soc pcie_data = {
> -	.pcie_ver =		0x520A,
> +	.pcie_ver =		DW_PCIE_VER_520A,
>  };
>  
>  static int intel_pcie_probe(struct platform_device *pdev)
> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index 65135f5c4a4a..f24b30b7454f 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> @@ -1979,7 +1979,7 @@ static int tegra194_pcie_probe(struct platform_device *pdev)
>  	pci->ops = &tegra_dw_pcie_ops;
>  	pci->n_fts[0] = N_FTS_VAL;
>  	pci->n_fts[1] = FTS_VAL;
> -	pci->version = 0x490A;
> +	pci->version = DW_PCIE_VER_490A;
>  
>  	pp = &pci->pp;
>  	pp->num_vectors = MAX_MSI_IRQS;
> -- 
> 2.35.1
> 

-- 
மணிவண்ணன் சதாசிவம்

[PATCH RESEND v4 07/15] PCI: tegra194: Drop manual DW PCIe controller version setup

[Serge Semin]

Since the DW PCIe common code now supports the IP-core version
auto-detection there is no point in manually setting the version up for the
controllers newer than v4.70a. Seeing Tegra 194 PCIe Host and EP
controllers are based on the DW PCIe v4.90a IP-core we can freely drop the
dw_pcie.version field initialization.

Suggested-by: Rob Herring <robh@kernel.org>
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

---

Folks, I don't have Tegra 194 PCIe hw instance to test it out. Could you
please make sure this patch doesn't brake anything?

Changelog v3:
- This is a new patch create as a result of the discussion:
  https://lore.kernel.org/linux-pci/20220503214638.1895-6-Sergey.Semin@baikalelectronics.ru/
---
 drivers/pci/controller/dwc/pcie-tegra194.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index f24b30b7454f..e497e6de8d15 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1979,7 +1979,6 @@ static int tegra194_pcie_probe(struct platform_device *pdev)
 	pci->ops = &tegra_dw_pcie_ops;
 	pci->n_fts[0] = N_FTS_VAL;
 	pci->n_fts[1] = FTS_VAL;
-	pci->version = DW_PCIE_VER_490A;
 
 	pp = &pci->pp;
 	pp->num_vectors = MAX_MSI_IRQS;
-- 
2.35.1

Re: [PATCH RESEND v4 07/15] PCI: tegra194: Drop manual DW PCIe controller version setup

[Vidya Sagar]

On 6/24/2022 8:09 PM, Serge Semin wrote:
> External email: Use caution opening links or attachments
> 
> 
> Since the DW PCIe common code now supports the IP-core version
> auto-detection there is no point in manually setting the version up for the
> controllers newer than v4.70a. Seeing Tegra 194 PCIe Host and EP
> controllers are based on the DW PCIe v4.90a IP-core we can freely drop the
> dw_pcie.version field initialization.
> 
> Suggested-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> ---
> 
> Folks, I don't have Tegra 194 PCIe hw instance to test it out. Could you
> please make sure this patch doesn't brake anything?

Hi,
I tried to apply the series 
https://patchwork.kernel.org/project/linux-pci/list/?series=653624 on 
top of linux-next and ran into conflicts. Could you please tell me the 
minimum set of patches to be taken?

Thanks,
Vidya Sagar

> 
> Changelog v3:
> - This is a new patch create as a result of the discussion:
>    https://lore.kernel.org/linux-pci/20220503214638.1895-6-Sergey.Semin@baikalelectronics.ru/
> ---
>   drivers/pci/controller/dwc/pcie-tegra194.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index f24b30b7454f..e497e6de8d15 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> @@ -1979,7 +1979,6 @@ static int tegra194_pcie_probe(struct platform_device *pdev)
>          pci->ops = &tegra_dw_pcie_ops;
>          pci->n_fts[0] = N_FTS_VAL;
>          pci->n_fts[1] = FTS_VAL;
> -       pci->version = DW_PCIE_VER_490A;
> 
>          pp = &pci->pp;
>          pp->num_vectors = MAX_MSI_IRQS;
> --
> 2.35.1
> 

Re: [PATCH RESEND v4 07/15] PCI: tegra194: Drop manual DW PCIe controller version setup

[Serge Semin]

On Mon, Jun 27, 2022 at 01:29:33PM +0530, Vidya Sagar wrote:
> 
> 
> On 6/24/2022 8:09 PM, Serge Semin wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > Since the DW PCIe common code now supports the IP-core version
> > auto-detection there is no point in manually setting the version up for the
> > controllers newer than v4.70a. Seeing Tegra 194 PCIe Host and EP
> > controllers are based on the DW PCIe v4.90a IP-core we can freely drop the
> > dw_pcie.version field initialization.
> > 
> > Suggested-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > 
> > ---
> > 
> > Folks, I don't have Tegra 194 PCIe hw instance to test it out. Could you
> > please make sure this patch doesn't brake anything?
> 

> Hi,
> I tried to apply the series
> https://patchwork.kernel.org/project/linux-pci/list/?series=653624 on top of
> linux-next and ran into conflicts. Could you please tell me the minimum set
> of patches to be taken?

Hi.
As the cover letter says it is supposed to be applied on top of the
next series *:
https://lore.kernel.org/linux-pci/20220624143428.8334-1-Sergey.Semin@baikalelectronics.ru/
* Currently latest patchset version is v5.

Then - the resent series what you are referring to.

-Sergey

> 
> Thanks,
> Vidya Sagar
> 
> > 
> > Changelog v3:
> > - This is a new patch create as a result of the discussion:
> >    https://lore.kernel.org/linux-pci/20220503214638.1895-6-Sergey.Semin@baikalelectronics.ru/
> > ---
> >   drivers/pci/controller/dwc/pcie-tegra194.c | 1 -
> >   1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> > index f24b30b7454f..e497e6de8d15 100644
> > --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> > @@ -1979,7 +1979,6 @@ static int tegra194_pcie_probe(struct platform_device *pdev)
> >          pci->ops = &tegra_dw_pcie_ops;
> >          pci->n_fts[0] = N_FTS_VAL;
> >          pci->n_fts[1] = FTS_VAL;
> > -       pci->version = DW_PCIE_VER_490A;
> > 
> >          pp = &pci->pp;
> >          pp->num_vectors = MAX_MSI_IRQS;
> > --
> > 2.35.1
> > 

Re: [PATCH RESEND v4 07/15] PCI: tegra194: Drop manual DW PCIe controller version setup

[Manivannan Sadhasivam]

On Fri, Jun 24, 2022 at 05:39:39PM +0300, Serge Semin wrote:
> Since the DW PCIe common code now supports the IP-core version
> auto-detection there is no point in manually setting the version up for the
> controllers newer than v4.70a. Seeing Tegra 194 PCIe Host and EP
> controllers are based on the DW PCIe v4.90a IP-core we can freely drop the
> dw_pcie.version field initialization.
> 
> Suggested-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> 
> ---
> 
> Folks, I don't have Tegra 194 PCIe hw instance to test it out. Could you
> please make sure this patch doesn't brake anything?
> 
> Changelog v3:
> - This is a new patch create as a result of the discussion:
>   https://lore.kernel.org/linux-pci/20220503214638.1895-6-Sergey.Semin@baikalelectronics.ru/
> ---
>  drivers/pci/controller/dwc/pcie-tegra194.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index f24b30b7454f..e497e6de8d15 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> @@ -1979,7 +1979,6 @@ static int tegra194_pcie_probe(struct platform_device *pdev)
>  	pci->ops = &tegra_dw_pcie_ops;
>  	pci->n_fts[0] = N_FTS_VAL;
>  	pci->n_fts[1] = FTS_VAL;
> -	pci->version = DW_PCIE_VER_490A;
>  
>  	pp = &pci->pp;
>  	pp->num_vectors = MAX_MSI_IRQS;
> -- 
> 2.35.1
> 

-- 
மணிவண்ணன் சதாசிவம்

[PATCH RESEND v4 11/15] PCI: dwc: Simplify in/outbound iATU setup methods

[Serge Semin]

From maintainability and scalability points of view it has been wrong to
use different iATU inbound and outbound regions accessors for the viewport
and unrolled versions of the iATU CSRs mapping. Seeing the particular iATU
region-wise registers layout is almost fully compatible for different
IP-core versions, there were no much points in splitting the code up that
way since it was possible to implement a common windows setup methods for
both viewport and unrolled iATU CSRs spaces. While what we can observe in
the current driver implementation of these methods, is a lot of code
duplication, which consequently worsen the code readability,
maintainability and scalability. Note the current implementation is a bit
more performant than the one suggested in this commit since it implies
having less MMIO accesses. But the gain just doesn't worth having the
denoted difficulties especially seeing the iATU setup methods are mainly
called on the DW PCIe controller and peripheral devices initialization
stage.

Here we suggest to move the iATU viewport and unrolled CSR access
specifics in the dw_pcie_readl_atu() and dw_pcie_writel_atu() method, and
convert the dw_pcie_prog_outbound_atu() and
dw_pcie_prog_{ep_}inbound_atu() functions to being generic instead of
having a different methods for each viewport and unrolled types of iATU
CSRs mapping. Nothing complex really. First of all the dw_pcie_readl_atu()
and dw_pcie_writel_atu() are converted to accept relative iATU CSRs
address together with the iATU region direction (inbound or outbound) and
region index. If DW PCIe controller doesn't have the unrolled iATU CSRs
space, then the accessors will need to activate a iATU viewport based on
the specified direction and index, otherwise a base address for the
corresponding region CSRs will be calculated by means of the
PCIE_ATU_UNROLL_BASE() macro. The CSRs macro have been modified in
accordance with that logic in the pcie-designware.h header file.

The rest of the changes in this commit just concern converting the iATU
in-/out-bound setup methods and iATU regions detection procedure to be
compatible with the new accessors semantics. As a result we've dropped the
no more required dw_pcie_prog_outbound_atu_unroll(),
dw_pcie_prog_inbound_atu_unroll() and dw_pcie_iatu_detect_regions_unroll()
methods.

Note aside with the denoted code improvements, there is an additional
positive side effect of this change. If at some point an atomic iATU
configs setup procedure is required, it will be possible to be done with
no much effort just by adding the synchronization into the
dw_pcie_readl_atu() and dw_pcie_writel_atu() accessors.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

---

Note the pcie-tegra194-acpi.c driver has been fixed here to have it
utilizing the macros introduced in this patch. Judging by the code
semantics Tegra 194 ACPI has weird iATU mapping. It's clearly unrolled but
the CSRs are accessed over the viewport offsets. This couldn't have been
designed more confusing...

Changelog v2:
- Move the iATU region selection procedure into a helper function (@Rob).
- Simplify the iATU region selection procedure by recalculating the base
  address only if the space is unrolled. The iATU viewport base address
  is saved in the pci->atu_base field from now.

Changelog v3:
- Fix pcie-tegra194-acpi.c driver to using the new macros names.
  (@Manivannan)
---
 drivers/pci/controller/dwc/pcie-designware.c  | 293 ++++++------------
 drivers/pci/controller/dwc/pcie-designware.h  |  48 ++-
 .../pci/controller/dwc/pcie-tegra194-acpi.c   |   7 +-
 3 files changed, 111 insertions(+), 237 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index a90d3f6ce50c..f2aa65d02a6c 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -205,48 +205,64 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val)
 		dev_err(pci->dev, "write DBI address failed\n");
 }
 
-static u32 dw_pcie_readl_atu(struct dw_pcie *pci, u32 reg)
+static inline void __iomem *dw_pcie_select_atu(struct dw_pcie *pci, u32 dir,
+					       u32 index)
 {
+	void __iomem *base = pci->atu_base;
+
+	if (pci->iatu_unroll_enabled)
+		base += PCIE_ATU_UNROLL_BASE(dir, index);
+	else
+		dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, dir | index);
+
+	return base;
+}
+
+static u32 dw_pcie_readl_atu(struct dw_pcie *pci, u32 dir, u32 index, u32 reg)
+{
+	void __iomem *base;
 	int ret;
 	u32 val;
 
+	base = dw_pcie_select_atu(pci, dir, index);
+
 	if (pci->ops && pci->ops->read_dbi)
-		return pci->ops->read_dbi(pci, pci->atu_base, reg, 4);
+		return pci->ops->read_dbi(pci, base, reg, 4);
 
-	ret = dw_pcie_read(pci->atu_base + reg, 4, &val);
+	ret = dw_pcie_read(base + reg, 4, &val);
 	if (ret)
 		dev_err(pci->dev, "Read ATU address failed\n");
 
 	return val;
 }
 
-static void dw_pcie_writel_atu(struct dw_pcie *pci, u32 reg, u32 val)
+static void dw_pcie_writel_atu(struct dw_pcie *pci, u32 dir, u32 index,
+			       u32 reg, u32 val)
 {
+	void __iomem *base;
 	int ret;
 
+	base = dw_pcie_select_atu(pci, dir, index);
+
 	if (pci->ops && pci->ops->write_dbi) {
-		pci->ops->write_dbi(pci, pci->atu_base, reg, 4, val);
+		pci->ops->write_dbi(pci, base, reg, 4, val);
 		return;
 	}
 
-	ret = dw_pcie_write(pci->atu_base + reg, 4, val);
+	ret = dw_pcie_write(base + reg, 4, val);
 	if (ret)
 		dev_err(pci->dev, "Write ATU address failed\n");
 }
 
-static u32 dw_pcie_readl_ob_unroll(struct dw_pcie *pci, u32 index, u32 reg)
+static inline u32 dw_pcie_readl_atu_ob(struct dw_pcie *pci, u32 index, u32 reg)
 {
-	u32 offset = PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(index);
-
-	return dw_pcie_readl_atu(pci, offset + reg);
+	return dw_pcie_readl_atu(pci, PCIE_ATU_REGION_DIR_OB, index, reg);
 }
 
-static void dw_pcie_writel_ob_unroll(struct dw_pcie *pci, u32 index, u32 reg,
-				     u32 val)
+static inline void dw_pcie_writel_atu_ob(struct dw_pcie *pci, u32 index, u32 reg,
+					 u32 val)
 {
-	u32 offset = PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(index);
-
-	dw_pcie_writel_atu(pci, offset + reg, val);
+	dw_pcie_writel_atu(pci, PCIE_ATU_REGION_DIR_OB, index, reg, val);
 }
 
 static inline u32 dw_pcie_enable_ecrc(u32 val)
@@ -290,50 +306,6 @@ static inline u32 dw_pcie_enable_ecrc(u32 val)
 	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,
-					     u64 size)
-{
-	u32 retries, val;
-	u64 limit_addr = cpu_addr + size - 1;
-
-	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_LOWER_BASE,
-				 lower_32_bits(cpu_addr));
-	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_BASE,
-				 upper_32_bits(cpu_addr));
-	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_LOWER_LIMIT,
-				 lower_32_bits(limit_addr));
-	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_LIMIT,
-				 upper_32_bits(limit_addr));
-	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_LOWER_TARGET,
-				 lower_32_bits(pci_addr));
-	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET,
-				 upper_32_bits(pci_addr));
-	val = type | PCIE_ATU_FUNC_NUM(func_no);
-	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr))
-		val |= PCIE_ATU_INCREASE_REGION_SIZE;
-	if (dw_pcie_ver_is(pci, 490A))
-		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);
-
-	/*
-	 * Make sure ATU enable takes effect before any subsequent config
-	 * and I/O accesses.
-	 */
-	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
-		val = dw_pcie_readl_ob_unroll(pci, index,
-					      PCIE_ATU_UNR_REGION_CTRL2);
-		if (val & PCIE_ATU_ENABLE)
-			return;
-
-		mdelay(LINK_WAIT_IATU);
-	}
-	dev_err(pci->dev, "Outbound iATU is not being enabled\n");
-}
-
 static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
 					int index, int type, u64 cpu_addr,
 					u64 pci_addr, u64 size)
@@ -344,49 +316,46 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
 	if (pci->ops && pci->ops->cpu_addr_fixup)
 		cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
 
-	if (pci->iatu_unroll_enabled) {
-		dw_pcie_prog_outbound_atu_unroll(pci, func_no, index, type,
-						 cpu_addr, pci_addr, size);
-		return;
-	}
-
 	limit_addr = cpu_addr + size - 1;
 
-	dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT,
-			   PCIE_ATU_REGION_DIR_OB | index);
-	dw_pcie_writel_dbi(pci, PCIE_ATU_LOWER_BASE,
-			   lower_32_bits(cpu_addr));
-	dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_BASE,
-			   upper_32_bits(cpu_addr));
-	dw_pcie_writel_dbi(pci, PCIE_ATU_LIMIT,
-			   lower_32_bits(limit_addr));
+	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LOWER_BASE,
+			      lower_32_bits(cpu_addr));
+	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_BASE,
+			      upper_32_bits(cpu_addr));
+
+	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LIMIT,
+			      lower_32_bits(limit_addr));
 	if (dw_pcie_ver_is_ge(pci, 460A))
-		dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_LIMIT,
-				   upper_32_bits(limit_addr));
-	dw_pcie_writel_dbi(pci, PCIE_ATU_LOWER_TARGET,
-			   lower_32_bits(pci_addr));
-	dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_TARGET,
-			   upper_32_bits(pci_addr));
+		dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_LIMIT,
+				      upper_32_bits(limit_addr));
+
+	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LOWER_TARGET,
+			      lower_32_bits(pci_addr));
+	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_TARGET,
+			      upper_32_bits(pci_addr));
+
 	val = type | PCIE_ATU_FUNC_NUM(func_no);
 	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr) &&
 	    dw_pcie_ver_is_ge(pci, 460A))
 		val |= PCIE_ATU_INCREASE_REGION_SIZE;
 	if (dw_pcie_ver_is(pci, 490A))
 		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);
+	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_REGION_CTRL1, val);
+
+	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE);
 
 	/*
 	 * Make sure ATU enable takes effect before any subsequent config
 	 * and I/O accesses.
 	 */
 	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
-		val = dw_pcie_readl_dbi(pci, PCIE_ATU_CR2);
+		val = dw_pcie_readl_atu_ob(pci, index, PCIE_ATU_REGION_CTRL2);
 		if (val & PCIE_ATU_ENABLE)
 			return;
 
 		mdelay(LINK_WAIT_IATU);
 	}
+
 	dev_err(pci->dev, "Outbound iATU is not being enabled\n");
 }
 
@@ -405,54 +374,15 @@ void dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
 				    cpu_addr, pci_addr, size);
 }
 
-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_atu(pci, offset + reg);
-}
-
-static void dw_pcie_writel_ib_unroll(struct dw_pcie *pci, u32 index, u32 reg,
-				     u32 val)
+static inline u32 dw_pcie_readl_atu_ib(struct dw_pcie *pci, u32 index, u32 reg)
 {
-	u32 offset = PCIE_GET_ATU_INB_UNR_REG_OFFSET(index);
-
-	dw_pcie_writel_atu(pci, offset + reg, val);
+	return dw_pcie_readl_atu(pci, PCIE_ATU_REGION_DIR_IB, index, reg);
 }
 
-static int dw_pcie_prog_inbound_atu_unroll(struct dw_pcie *pci, u8 func_no,
-					   int index, int type,
-					   u64 cpu_addr, u8 bar)
+static inline void dw_pcie_writel_atu_ib(struct dw_pcie *pci, u32 index, u32 reg,
+					 u32 val)
 {
-	u32 retries, val;
-
-	dw_pcie_writel_ib_unroll(pci, index, PCIE_ATU_UNR_LOWER_TARGET,
-				 lower_32_bits(cpu_addr));
-	dw_pcie_writel_ib_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET,
-				 upper_32_bits(cpu_addr));
-
-	dw_pcie_writel_ib_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, type |
-				 PCIE_ATU_FUNC_NUM(func_no));
-	dw_pcie_writel_ib_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
-				 PCIE_ATU_FUNC_NUM_MATCH_EN |
-				 PCIE_ATU_ENABLE |
-				 PCIE_ATU_BAR_MODE_ENABLE | (bar << 8));
-
-	/*
-	 * Make sure ATU enable takes effect before any subsequent config
-	 * and I/O accesses.
-	 */
-	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
-		val = dw_pcie_readl_ib_unroll(pci, index,
-					      PCIE_ATU_UNR_REGION_CTRL2);
-		if (val & PCIE_ATU_ENABLE)
-			return 0;
-
-		mdelay(LINK_WAIT_IATU);
-	}
-	dev_err(pci->dev, "Inbound iATU is not being enabled\n");
-
-	return -EBUSY;
+	dw_pcie_writel_atu(pci, PCIE_ATU_REGION_DIR_IB, index, reg, val);
 }
 
 int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
@@ -460,51 +390,37 @@ int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
 {
 	u32 retries, val;
 
-	if (pci->iatu_unroll_enabled)
-		return dw_pcie_prog_inbound_atu_unroll(pci, func_no, index, type,
-						       cpu_addr, bar);
+	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_LOWER_TARGET,
+			      lower_32_bits(cpu_addr));
+	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_UPPER_TARGET,
+			      upper_32_bits(cpu_addr));
 
-	dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, PCIE_ATU_REGION_DIR_IB |
-			   index);
-	dw_pcie_writel_dbi(pci, PCIE_ATU_LOWER_TARGET, lower_32_bits(cpu_addr));
-	dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_TARGET, upper_32_bits(cpu_addr));
-
-	dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, type |
-			   PCIE_ATU_FUNC_NUM(func_no));
-	dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, PCIE_ATU_ENABLE |
-			   PCIE_ATU_FUNC_NUM_MATCH_EN |
-			   PCIE_ATU_BAR_MODE_ENABLE | (bar << 8));
+	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_REGION_CTRL1, type |
+			      PCIE_ATU_FUNC_NUM(func_no));
+	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_REGION_CTRL2,
+			      PCIE_ATU_ENABLE | PCIE_ATU_FUNC_NUM_MATCH_EN |
+			      PCIE_ATU_BAR_MODE_ENABLE | (bar << 8));
 
 	/*
 	 * Make sure ATU enable takes effect before any subsequent config
 	 * and I/O accesses.
 	 */
 	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
-		val = dw_pcie_readl_dbi(pci, PCIE_ATU_CR2);
+		val = dw_pcie_readl_atu_ib(pci, index, PCIE_ATU_REGION_CTRL2);
 		if (val & PCIE_ATU_ENABLE)
 			return 0;
 
 		mdelay(LINK_WAIT_IATU);
 	}
+
 	dev_err(pci->dev, "Inbound iATU is not being enabled\n");
 
-	return -EBUSY;
+	return -ETIMEDOUT;
 }
 
 void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index)
 {
-	if (pci->iatu_unroll_enabled) {
-		if (dir == PCIE_ATU_REGION_DIR_IB) {
-			dw_pcie_writel_ib_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
-						 ~(u32)PCIE_ATU_ENABLE);
-		} else {
-			dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
-						 ~(u32)PCIE_ATU_ENABLE);
-		}
-	} else {
-		dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, dir | index);
-		dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, ~(u32)PCIE_ATU_ENABLE);
-	}
+	dw_pcie_writel_atu(pci, dir, index, PCIE_ATU_REGION_CTRL2, 0);
 }
 
 int dw_pcie_wait_for_link(struct dw_pcie *pci)
@@ -606,63 +522,29 @@ static bool dw_pcie_iatu_unroll_enabled(struct dw_pcie *pci)
 	return false;
 }
 
-static void dw_pcie_iatu_detect_regions_unroll(struct dw_pcie *pci)
-{
-	int max_region, i, ob = 0, ib = 0;
-	u32 val;
-
-	max_region = min((int)pci->atu_size / 512, 256);
-
-	for (i = 0; i < max_region; i++) {
-		dw_pcie_writel_ob_unroll(pci, i, PCIE_ATU_UNR_LOWER_TARGET,
-					0x11110000);
-
-		val = dw_pcie_readl_ob_unroll(pci, i, PCIE_ATU_UNR_LOWER_TARGET);
-		if (val == 0x11110000)
-			ob++;
-		else
-			break;
-	}
-
-	for (i = 0; i < max_region; i++) {
-		dw_pcie_writel_ib_unroll(pci, i, PCIE_ATU_UNR_LOWER_TARGET,
-					0x11110000);
-
-		val = dw_pcie_readl_ib_unroll(pci, i, PCIE_ATU_UNR_LOWER_TARGET);
-		if (val == 0x11110000)
-			ib++;
-		else
-			break;
-	}
-	pci->num_ib_windows = ib;
-	pci->num_ob_windows = ob;
-}
-
 static void dw_pcie_iatu_detect_regions(struct dw_pcie *pci)
 {
-	int max_region, i, ob = 0, ib = 0;
+	int max_region, ob, ib;
 	u32 val;
 
-	dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, 0xFF);
-	max_region = dw_pcie_readl_dbi(pci, PCIE_ATU_VIEWPORT) + 1;
+	if (pci->iatu_unroll_enabled) {
+		max_region = min((int)pci->atu_size / 512, 256);
+	} else {
+		dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, 0xFF);
+		max_region = dw_pcie_readl_dbi(pci, PCIE_ATU_VIEWPORT) + 1;
+	}
 
-	for (i = 0; i < max_region; i++) {
-		dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, PCIE_ATU_REGION_DIR_OB | i);
-		dw_pcie_writel_dbi(pci, PCIE_ATU_LOWER_TARGET, 0x11110000);
-		val = dw_pcie_readl_dbi(pci, PCIE_ATU_LOWER_TARGET);
-		if (val == 0x11110000)
-			ob++;
-		else
+	for (ob = 0; ob < max_region; ob++) {
+		dw_pcie_writel_atu_ob(pci, ob, PCIE_ATU_LOWER_TARGET, 0x11110000);
+		val = dw_pcie_readl_atu_ob(pci, ob, PCIE_ATU_LOWER_TARGET);
+		if (val != 0x11110000)
 			break;
 	}
 
-	for (i = 0; i < max_region; i++) {
-		dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, PCIE_ATU_REGION_DIR_IB | i);
-		dw_pcie_writel_dbi(pci, PCIE_ATU_LOWER_TARGET, 0x11110000);
-		val = dw_pcie_readl_dbi(pci, PCIE_ATU_LOWER_TARGET);
-		if (val == 0x11110000)
-			ib++;
-		else
+	for (ib = 0; ib < max_region; ib++) {
+		dw_pcie_writel_atu_ib(pci, ib, PCIE_ATU_LOWER_TARGET, 0x11110000);
+		val = dw_pcie_readl_atu_ib(pci, ib, PCIE_ATU_LOWER_TARGET);
+		if (val != 0x11110000)
 			break;
 	}
 
@@ -690,12 +572,13 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
 		if (!pci->atu_size)
 			/* Pick a minimal default, enough for 8 in and 8 out windows */
 			pci->atu_size = SZ_4K;
-
-		dw_pcie_iatu_detect_regions_unroll(pci);
 	} else {
-		dw_pcie_iatu_detect_regions(pci);
+		pci->atu_base = pci->dbi_base + PCIE_ATU_VIEWPORT_BASE;
+		pci->atu_size = PCIE_ATU_VIEWPORT_SIZE;
 	}
 
+	dw_pcie_iatu_detect_regions(pci);
+
 	dev_info(pci->dev, "iATU unroll: %s\n", pci->iatu_unroll_enabled ?
 		"enabled" : "disabled");
 
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 72d185ff72f3..c18f0db09b31 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -103,10 +103,21 @@
 #define PCIE_VERSION_NUMBER		0x8F8
 #define PCIE_VERSION_TYPE		0x8FC
 
+/*
+ * iATU inbound and outbound windows CSRs. Before the IP-core v4.80a each
+ * iATU region CSRs had been indirectly accessible by means of the dedicated
+ * viewport selector. The iATU/eDMA CSRs space was re-designed in DWC PCIe
+ * v4.80a in a way so the viewport was unrolled into the directly accessible
+ * iATU/eDMA CSRs space.
+ */
 #define PCIE_ATU_VIEWPORT		0x900
 #define PCIE_ATU_REGION_DIR_IB		BIT(31)
 #define PCIE_ATU_REGION_DIR_OB		0
-#define PCIE_ATU_CR1			0x904
+#define PCIE_ATU_VIEWPORT_BASE		0x904
+#define PCIE_ATU_UNROLL_BASE(dir, index) \
+	(((index) << 9) | ((dir == PCIE_ATU_REGION_DIR_IB) ? BIT(8) : 0))
+#define PCIE_ATU_VIEWPORT_SIZE		0x2C
+#define PCIE_ATU_REGION_CTRL1		0x000
 #define PCIE_ATU_INCREASE_REGION_SIZE	BIT(13)
 #define PCIE_ATU_TYPE_MEM		0x0
 #define PCIE_ATU_TYPE_IO		0x2
@@ -114,19 +125,19 @@
 #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_REGION_CTRL2		0x004
 #define PCIE_ATU_ENABLE			BIT(31)
 #define PCIE_ATU_BAR_MODE_ENABLE	BIT(30)
 #define PCIE_ATU_FUNC_NUM_MATCH_EN      BIT(19)
-#define PCIE_ATU_LOWER_BASE		0x90C
-#define PCIE_ATU_UPPER_BASE		0x910
-#define PCIE_ATU_LIMIT			0x914
-#define PCIE_ATU_LOWER_TARGET		0x918
+#define PCIE_ATU_LOWER_BASE		0x008
+#define PCIE_ATU_UPPER_BASE		0x00C
+#define PCIE_ATU_LIMIT			0x010
+#define PCIE_ATU_LOWER_TARGET		0x014
 #define PCIE_ATU_BUS(x)			FIELD_PREP(GENMASK(31, 24), x)
 #define PCIE_ATU_DEV(x)			FIELD_PREP(GENMASK(23, 19), x)
 #define PCIE_ATU_FUNC(x)		FIELD_PREP(GENMASK(18, 16), x)
-#define PCIE_ATU_UPPER_TARGET		0x91C
-#define PCIE_ATU_UPPER_LIMIT		0x924
+#define PCIE_ATU_UPPER_TARGET		0x018
+#define PCIE_ATU_UPPER_LIMIT		0x020
 
 #define PCIE_MISC_CONTROL_1_OFF		0x8BC
 #define PCIE_DBI_RO_WR_EN		BIT(0)
@@ -143,19 +154,6 @@
 
 #define PCIE_PL_CHK_REG_ERR_ADDR			0xB28
 
-/*
- * iATU Unroll-specific register definitions
- * From 4.80 core version the address translation will be made by unroll
- */
-#define PCIE_ATU_UNR_REGION_CTRL1	0x00
-#define PCIE_ATU_UNR_REGION_CTRL2	0x04
-#define PCIE_ATU_UNR_LOWER_BASE		0x08
-#define PCIE_ATU_UNR_UPPER_BASE		0x0C
-#define PCIE_ATU_UNR_LOWER_LIMIT	0x10
-#define PCIE_ATU_UNR_LOWER_TARGET	0x14
-#define PCIE_ATU_UNR_UPPER_TARGET	0x18
-#define PCIE_ATU_UNR_UPPER_LIMIT	0x20
-
 /*
  * The default address offset between dbi_base and atu_base. Root controller
  * drivers are not required to initialize atu_base if the offset matches this
@@ -164,13 +162,6 @@
  */
 #define DEFAULT_DBI_ATU_OFFSET (0x3 << 20)
 
-/* Register address builder */
-#define PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(region) \
-		((region) << 9)
-
-#define PCIE_GET_ATU_INB_UNR_REG_OFFSET(region) \
-		(((region) << 9) | BIT(8))
-
 #define MAX_MSI_IRQS			256
 #define MAX_MSI_IRQS_PER_CTRL		32
 #define MAX_MSI_CTRLS			(MAX_MSI_IRQS / MAX_MSI_IRQS_PER_CTRL)
@@ -277,7 +268,6 @@ 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;
 	size_t			atu_size;
 	u32			num_ib_windows;
diff --git a/drivers/pci/controller/dwc/pcie-tegra194-acpi.c b/drivers/pci/controller/dwc/pcie-tegra194-acpi.c
index c2de6ed4d86f..55f61914a986 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194-acpi.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194-acpi.c
@@ -39,7 +39,8 @@ static int tegra194_acpi_init(struct pci_config_window *cfg)
 static void atu_reg_write(struct tegra194_pcie_ecam *pcie_ecam, int index,
 			  u32 val, u32 reg)
 {
-	u32 offset = PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(index);
+	u32 offset = PCIE_ATU_UNROLL_BASE(PCIE_ATU_REGION_DIR_OB, index) +
+		     PCIE_ATU_VIEWPORT_BASE;
 
 	writel(val, pcie_ecam->iatu_base + offset + reg);
 }
@@ -58,8 +59,8 @@ static void program_outbound_atu(struct tegra194_pcie_ecam *pcie_ecam,
 		      PCIE_ATU_LIMIT);
 	atu_reg_write(pcie_ecam, index, upper_32_bits(pci_addr),
 		      PCIE_ATU_UPPER_TARGET);
-	atu_reg_write(pcie_ecam, index, type, PCIE_ATU_CR1);
-	atu_reg_write(pcie_ecam, index, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
+	atu_reg_write(pcie_ecam, index, type, PCIE_ATU_REGION_CTRL1);
+	atu_reg_write(pcie_ecam, index, PCIE_ATU_ENABLE, PCIE_ATU_REGION_CTRL2);
 }
 
 static void __iomem *tegra194_map_bus(struct pci_bus *bus,
-- 
2.35.1

Re: [PATCH RESEND v4 11/15] PCI: dwc: Simplify in/outbound iATU setup methods

[Manivannan Sadhasivam]

On Fri, Jun 24, 2022 at 05:39:43PM +0300, Serge Semin wrote:
> From maintainability and scalability points of view it has been wrong to
> use different iATU inbound and outbound regions accessors for the viewport
> and unrolled versions of the iATU CSRs mapping. Seeing the particular iATU
> region-wise registers layout is almost fully compatible for different
> IP-core versions, there were no much points in splitting the code up that
> way since it was possible to implement a common windows setup methods for
> both viewport and unrolled iATU CSRs spaces. While what we can observe in
> the current driver implementation of these methods, is a lot of code
> duplication, which consequently worsen the code readability,
> maintainability and scalability. Note the current implementation is a bit
> more performant than the one suggested in this commit since it implies
> having less MMIO accesses. But the gain just doesn't worth having the
> denoted difficulties especially seeing the iATU setup methods are mainly
> called on the DW PCIe controller and peripheral devices initialization
> stage.
> 
> Here we suggest to move the iATU viewport and unrolled CSR access
> specifics in the dw_pcie_readl_atu() and dw_pcie_writel_atu() method, and
> convert the dw_pcie_prog_outbound_atu() and
> dw_pcie_prog_{ep_}inbound_atu() functions to being generic instead of
> having a different methods for each viewport and unrolled types of iATU
> CSRs mapping. Nothing complex really. First of all the dw_pcie_readl_atu()
> and dw_pcie_writel_atu() are converted to accept relative iATU CSRs
> address together with the iATU region direction (inbound or outbound) and
> region index. If DW PCIe controller doesn't have the unrolled iATU CSRs
> space, then the accessors will need to activate a iATU viewport based on
> the specified direction and index, otherwise a base address for the
> corresponding region CSRs will be calculated by means of the
> PCIE_ATU_UNROLL_BASE() macro. The CSRs macro have been modified in
> accordance with that logic in the pcie-designware.h header file.
> 
> The rest of the changes in this commit just concern converting the iATU
> in-/out-bound setup methods and iATU regions detection procedure to be
> compatible with the new accessors semantics. As a result we've dropped the
> no more required dw_pcie_prog_outbound_atu_unroll(),
> dw_pcie_prog_inbound_atu_unroll() and dw_pcie_iatu_detect_regions_unroll()
> methods.
> 
> Note aside with the denoted code improvements, there is an additional
> positive side effect of this change. If at some point an atomic iATU
> configs setup procedure is required, it will be possible to be done with
> no much effort just by adding the synchronization into the
> dw_pcie_readl_atu() and dw_pcie_writel_atu() accessors.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

One nitpick mentioned below. With that fixed,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> 
> ---
> 
> Note the pcie-tegra194-acpi.c driver has been fixed here to have it
> utilizing the macros introduced in this patch. Judging by the code
> semantics Tegra 194 ACPI has weird iATU mapping. It's clearly unrolled but
> the CSRs are accessed over the viewport offsets. This couldn't have been
> designed more confusing...
> 
> Changelog v2:
> - Move the iATU region selection procedure into a helper function (@Rob).
> - Simplify the iATU region selection procedure by recalculating the base
>   address only if the space is unrolled. The iATU viewport base address
>   is saved in the pci->atu_base field from now.
> 
> Changelog v3:
> - Fix pcie-tegra194-acpi.c driver to using the new macros names.
>   (@Manivannan)
> ---
>  drivers/pci/controller/dwc/pcie-designware.c  | 293 ++++++------------
>  drivers/pci/controller/dwc/pcie-designware.h  |  48 ++-
>  .../pci/controller/dwc/pcie-tegra194-acpi.c   |   7 +-
>  3 files changed, 111 insertions(+), 237 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index a90d3f6ce50c..f2aa65d02a6c 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -205,48 +205,64 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val)
>  		dev_err(pci->dev, "write DBI address failed\n");
>  }
>  
> -static u32 dw_pcie_readl_atu(struct dw_pcie *pci, u32 reg)
> +static inline void __iomem *dw_pcie_select_atu(struct dw_pcie *pci, u32 dir,

This could be renamed to "dw_pcie_get_atu_base()" since we are anyway getting
the base address of iATU.

Thanks,
Mani

> +					       u32 index)
>  {
> +	void __iomem *base = pci->atu_base;
> +
> +	if (pci->iatu_unroll_enabled)
> +		base += PCIE_ATU_UNROLL_BASE(dir, index);
> +	else
> +		dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, dir | index);
> +
> +	return base;
> +}
> +
> +static u32 dw_pcie_readl_atu(struct dw_pcie *pci, u32 dir, u32 index, u32 reg)
> +{
> +	void __iomem *base;
>  	int ret;
>  	u32 val;
>  
> +	base = dw_pcie_select_atu(pci, dir, index);
> +
>  	if (pci->ops && pci->ops->read_dbi)
> -		return pci->ops->read_dbi(pci, pci->atu_base, reg, 4);
> +		return pci->ops->read_dbi(pci, base, reg, 4);
>  
> -	ret = dw_pcie_read(pci->atu_base + reg, 4, &val);
> +	ret = dw_pcie_read(base + reg, 4, &val);
>  	if (ret)
>  		dev_err(pci->dev, "Read ATU address failed\n");
>  
>  	return val;
>  }
>  
> -static void dw_pcie_writel_atu(struct dw_pcie *pci, u32 reg, u32 val)
> +static void dw_pcie_writel_atu(struct dw_pcie *pci, u32 dir, u32 index,
> +			       u32 reg, u32 val)
>  {
> +	void __iomem *base;
>  	int ret;
>  
> +	base = dw_pcie_select_atu(pci, dir, index);
> +
>  	if (pci->ops && pci->ops->write_dbi) {
> -		pci->ops->write_dbi(pci, pci->atu_base, reg, 4, val);
> +		pci->ops->write_dbi(pci, base, reg, 4, val);
>  		return;
>  	}
>  
> -	ret = dw_pcie_write(pci->atu_base + reg, 4, val);
> +	ret = dw_pcie_write(base + reg, 4, val);
>  	if (ret)
>  		dev_err(pci->dev, "Write ATU address failed\n");
>  }
>  
> -static u32 dw_pcie_readl_ob_unroll(struct dw_pcie *pci, u32 index, u32 reg)
> +static inline u32 dw_pcie_readl_atu_ob(struct dw_pcie *pci, u32 index, u32 reg)
>  {
> -	u32 offset = PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(index);
> -
> -	return dw_pcie_readl_atu(pci, offset + reg);
> +	return dw_pcie_readl_atu(pci, PCIE_ATU_REGION_DIR_OB, index, reg);
>  }
>  
> -static void dw_pcie_writel_ob_unroll(struct dw_pcie *pci, u32 index, u32 reg,
> -				     u32 val)
> +static inline void dw_pcie_writel_atu_ob(struct dw_pcie *pci, u32 index, u32 reg,
> +					 u32 val)
>  {
> -	u32 offset = PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(index);
> -
> -	dw_pcie_writel_atu(pci, offset + reg, val);
> +	dw_pcie_writel_atu(pci, PCIE_ATU_REGION_DIR_OB, index, reg, val);
>  }
>  
>  static inline u32 dw_pcie_enable_ecrc(u32 val)
> @@ -290,50 +306,6 @@ static inline u32 dw_pcie_enable_ecrc(u32 val)
>  	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,
> -					     u64 size)
> -{
> -	u32 retries, val;
> -	u64 limit_addr = cpu_addr + size - 1;
> -
> -	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_LOWER_BASE,
> -				 lower_32_bits(cpu_addr));
> -	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_BASE,
> -				 upper_32_bits(cpu_addr));
> -	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_LOWER_LIMIT,
> -				 lower_32_bits(limit_addr));
> -	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_LIMIT,
> -				 upper_32_bits(limit_addr));
> -	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_LOWER_TARGET,
> -				 lower_32_bits(pci_addr));
> -	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET,
> -				 upper_32_bits(pci_addr));
> -	val = type | PCIE_ATU_FUNC_NUM(func_no);
> -	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr))
> -		val |= PCIE_ATU_INCREASE_REGION_SIZE;
> -	if (dw_pcie_ver_is(pci, 490A))
> -		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);
> -
> -	/*
> -	 * Make sure ATU enable takes effect before any subsequent config
> -	 * and I/O accesses.
> -	 */
> -	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
> -		val = dw_pcie_readl_ob_unroll(pci, index,
> -					      PCIE_ATU_UNR_REGION_CTRL2);
> -		if (val & PCIE_ATU_ENABLE)
> -			return;
> -
> -		mdelay(LINK_WAIT_IATU);
> -	}
> -	dev_err(pci->dev, "Outbound iATU is not being enabled\n");
> -}
> -
>  static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
>  					int index, int type, u64 cpu_addr,
>  					u64 pci_addr, u64 size)
> @@ -344,49 +316,46 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
>  	if (pci->ops && pci->ops->cpu_addr_fixup)
>  		cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
>  
> -	if (pci->iatu_unroll_enabled) {
> -		dw_pcie_prog_outbound_atu_unroll(pci, func_no, index, type,
> -						 cpu_addr, pci_addr, size);
> -		return;
> -	}
> -
>  	limit_addr = cpu_addr + size - 1;
>  
> -	dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT,
> -			   PCIE_ATU_REGION_DIR_OB | index);
> -	dw_pcie_writel_dbi(pci, PCIE_ATU_LOWER_BASE,
> -			   lower_32_bits(cpu_addr));
> -	dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_BASE,
> -			   upper_32_bits(cpu_addr));
> -	dw_pcie_writel_dbi(pci, PCIE_ATU_LIMIT,
> -			   lower_32_bits(limit_addr));
> +	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LOWER_BASE,
> +			      lower_32_bits(cpu_addr));
> +	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_BASE,
> +			      upper_32_bits(cpu_addr));
> +
> +	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LIMIT,
> +			      lower_32_bits(limit_addr));
>  	if (dw_pcie_ver_is_ge(pci, 460A))
> -		dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_LIMIT,
> -				   upper_32_bits(limit_addr));
> -	dw_pcie_writel_dbi(pci, PCIE_ATU_LOWER_TARGET,
> -			   lower_32_bits(pci_addr));
> -	dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_TARGET,
> -			   upper_32_bits(pci_addr));
> +		dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_LIMIT,
> +				      upper_32_bits(limit_addr));
> +
> +	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LOWER_TARGET,
> +			      lower_32_bits(pci_addr));
> +	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_TARGET,
> +			      upper_32_bits(pci_addr));
> +
>  	val = type | PCIE_ATU_FUNC_NUM(func_no);
>  	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr) &&
>  	    dw_pcie_ver_is_ge(pci, 460A))
>  		val |= PCIE_ATU_INCREASE_REGION_SIZE;
>  	if (dw_pcie_ver_is(pci, 490A))
>  		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);
> +	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_REGION_CTRL1, val);
> +
> +	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE);
>  
>  	/*
>  	 * Make sure ATU enable takes effect before any subsequent config
>  	 * and I/O accesses.
>  	 */
>  	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
> -		val = dw_pcie_readl_dbi(pci, PCIE_ATU_CR2);
> +		val = dw_pcie_readl_atu_ob(pci, index, PCIE_ATU_REGION_CTRL2);
>  		if (val & PCIE_ATU_ENABLE)
>  			return;
>  
>  		mdelay(LINK_WAIT_IATU);
>  	}
> +
>  	dev_err(pci->dev, "Outbound iATU is not being enabled\n");
>  }
>  
> @@ -405,54 +374,15 @@ void dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
>  				    cpu_addr, pci_addr, size);
>  }
>  
> -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_atu(pci, offset + reg);
> -}
> -
> -static void dw_pcie_writel_ib_unroll(struct dw_pcie *pci, u32 index, u32 reg,
> -				     u32 val)
> +static inline u32 dw_pcie_readl_atu_ib(struct dw_pcie *pci, u32 index, u32 reg)
>  {
> -	u32 offset = PCIE_GET_ATU_INB_UNR_REG_OFFSET(index);
> -
> -	dw_pcie_writel_atu(pci, offset + reg, val);
> +	return dw_pcie_readl_atu(pci, PCIE_ATU_REGION_DIR_IB, index, reg);
>  }
>  
> -static int dw_pcie_prog_inbound_atu_unroll(struct dw_pcie *pci, u8 func_no,
> -					   int index, int type,
> -					   u64 cpu_addr, u8 bar)
> +static inline void dw_pcie_writel_atu_ib(struct dw_pcie *pci, u32 index, u32 reg,
> +					 u32 val)
>  {
> -	u32 retries, val;
> -
> -	dw_pcie_writel_ib_unroll(pci, index, PCIE_ATU_UNR_LOWER_TARGET,
> -				 lower_32_bits(cpu_addr));
> -	dw_pcie_writel_ib_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET,
> -				 upper_32_bits(cpu_addr));
> -
> -	dw_pcie_writel_ib_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, type |
> -				 PCIE_ATU_FUNC_NUM(func_no));
> -	dw_pcie_writel_ib_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
> -				 PCIE_ATU_FUNC_NUM_MATCH_EN |
> -				 PCIE_ATU_ENABLE |
> -				 PCIE_ATU_BAR_MODE_ENABLE | (bar << 8));
> -
> -	/*
> -	 * Make sure ATU enable takes effect before any subsequent config
> -	 * and I/O accesses.
> -	 */
> -	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
> -		val = dw_pcie_readl_ib_unroll(pci, index,
> -					      PCIE_ATU_UNR_REGION_CTRL2);
> -		if (val & PCIE_ATU_ENABLE)
> -			return 0;
> -
> -		mdelay(LINK_WAIT_IATU);
> -	}
> -	dev_err(pci->dev, "Inbound iATU is not being enabled\n");
> -
> -	return -EBUSY;
> +	dw_pcie_writel_atu(pci, PCIE_ATU_REGION_DIR_IB, index, reg, val);
>  }
>  
>  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> @@ -460,51 +390,37 @@ int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
>  {
>  	u32 retries, val;
>  
> -	if (pci->iatu_unroll_enabled)
> -		return dw_pcie_prog_inbound_atu_unroll(pci, func_no, index, type,
> -						       cpu_addr, bar);
> +	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_LOWER_TARGET,
> +			      lower_32_bits(cpu_addr));
> +	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_UPPER_TARGET,
> +			      upper_32_bits(cpu_addr));
>  
> -	dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, PCIE_ATU_REGION_DIR_IB |
> -			   index);
> -	dw_pcie_writel_dbi(pci, PCIE_ATU_LOWER_TARGET, lower_32_bits(cpu_addr));
> -	dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_TARGET, upper_32_bits(cpu_addr));
> -
> -	dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, type |
> -			   PCIE_ATU_FUNC_NUM(func_no));
> -	dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, PCIE_ATU_ENABLE |
> -			   PCIE_ATU_FUNC_NUM_MATCH_EN |
> -			   PCIE_ATU_BAR_MODE_ENABLE | (bar << 8));
> +	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_REGION_CTRL1, type |
> +			      PCIE_ATU_FUNC_NUM(func_no));
> +	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_REGION_CTRL2,
> +			      PCIE_ATU_ENABLE | PCIE_ATU_FUNC_NUM_MATCH_EN |
> +			      PCIE_ATU_BAR_MODE_ENABLE | (bar << 8));
>  
>  	/*
>  	 * Make sure ATU enable takes effect before any subsequent config
>  	 * and I/O accesses.
>  	 */
>  	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
> -		val = dw_pcie_readl_dbi(pci, PCIE_ATU_CR2);
> +		val = dw_pcie_readl_atu_ib(pci, index, PCIE_ATU_REGION_CTRL2);
>  		if (val & PCIE_ATU_ENABLE)
>  			return 0;
>  
>  		mdelay(LINK_WAIT_IATU);
>  	}
> +
>  	dev_err(pci->dev, "Inbound iATU is not being enabled\n");
>  
> -	return -EBUSY;
> +	return -ETIMEDOUT;
>  }
>  
>  void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index)
>  {
> -	if (pci->iatu_unroll_enabled) {
> -		if (dir == PCIE_ATU_REGION_DIR_IB) {
> -			dw_pcie_writel_ib_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
> -						 ~(u32)PCIE_ATU_ENABLE);
> -		} else {
> -			dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
> -						 ~(u32)PCIE_ATU_ENABLE);
> -		}
> -	} else {
> -		dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, dir | index);
> -		dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, ~(u32)PCIE_ATU_ENABLE);
> -	}
> +	dw_pcie_writel_atu(pci, dir, index, PCIE_ATU_REGION_CTRL2, 0);
>  }
>  
>  int dw_pcie_wait_for_link(struct dw_pcie *pci)
> @@ -606,63 +522,29 @@ static bool dw_pcie_iatu_unroll_enabled(struct dw_pcie *pci)
>  	return false;
>  }
>  
> -static void dw_pcie_iatu_detect_regions_unroll(struct dw_pcie *pci)
> -{
> -	int max_region, i, ob = 0, ib = 0;
> -	u32 val;
> -
> -	max_region = min((int)pci->atu_size / 512, 256);
> -
> -	for (i = 0; i < max_region; i++) {
> -		dw_pcie_writel_ob_unroll(pci, i, PCIE_ATU_UNR_LOWER_TARGET,
> -					0x11110000);
> -
> -		val = dw_pcie_readl_ob_unroll(pci, i, PCIE_ATU_UNR_LOWER_TARGET);
> -		if (val == 0x11110000)
> -			ob++;
> -		else
> -			break;
> -	}
> -
> -	for (i = 0; i < max_region; i++) {
> -		dw_pcie_writel_ib_unroll(pci, i, PCIE_ATU_UNR_LOWER_TARGET,
> -					0x11110000);
> -
> -		val = dw_pcie_readl_ib_unroll(pci, i, PCIE_ATU_UNR_LOWER_TARGET);
> -		if (val == 0x11110000)
> -			ib++;
> -		else
> -			break;
> -	}
> -	pci->num_ib_windows = ib;
> -	pci->num_ob_windows = ob;
> -}
> -
>  static void dw_pcie_iatu_detect_regions(struct dw_pcie *pci)
>  {
> -	int max_region, i, ob = 0, ib = 0;
> +	int max_region, ob, ib;
>  	u32 val;
>  
> -	dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, 0xFF);
> -	max_region = dw_pcie_readl_dbi(pci, PCIE_ATU_VIEWPORT) + 1;
> +	if (pci->iatu_unroll_enabled) {
> +		max_region = min((int)pci->atu_size / 512, 256);
> +	} else {
> +		dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, 0xFF);
> +		max_region = dw_pcie_readl_dbi(pci, PCIE_ATU_VIEWPORT) + 1;
> +	}
>  
> -	for (i = 0; i < max_region; i++) {
> -		dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, PCIE_ATU_REGION_DIR_OB | i);
> -		dw_pcie_writel_dbi(pci, PCIE_ATU_LOWER_TARGET, 0x11110000);
> -		val = dw_pcie_readl_dbi(pci, PCIE_ATU_LOWER_TARGET);
> -		if (val == 0x11110000)
> -			ob++;
> -		else
> +	for (ob = 0; ob < max_region; ob++) {
> +		dw_pcie_writel_atu_ob(pci, ob, PCIE_ATU_LOWER_TARGET, 0x11110000);
> +		val = dw_pcie_readl_atu_ob(pci, ob, PCIE_ATU_LOWER_TARGET);
> +		if (val != 0x11110000)
>  			break;
>  	}
>  
> -	for (i = 0; i < max_region; i++) {
> -		dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, PCIE_ATU_REGION_DIR_IB | i);
> -		dw_pcie_writel_dbi(pci, PCIE_ATU_LOWER_TARGET, 0x11110000);
> -		val = dw_pcie_readl_dbi(pci, PCIE_ATU_LOWER_TARGET);
> -		if (val == 0x11110000)
> -			ib++;
> -		else
> +	for (ib = 0; ib < max_region; ib++) {
> +		dw_pcie_writel_atu_ib(pci, ib, PCIE_ATU_LOWER_TARGET, 0x11110000);
> +		val = dw_pcie_readl_atu_ib(pci, ib, PCIE_ATU_LOWER_TARGET);
> +		if (val != 0x11110000)
>  			break;
>  	}
>  
> @@ -690,12 +572,13 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
>  		if (!pci->atu_size)
>  			/* Pick a minimal default, enough for 8 in and 8 out windows */
>  			pci->atu_size = SZ_4K;
> -
> -		dw_pcie_iatu_detect_regions_unroll(pci);
>  	} else {
> -		dw_pcie_iatu_detect_regions(pci);
> +		pci->atu_base = pci->dbi_base + PCIE_ATU_VIEWPORT_BASE;
> +		pci->atu_size = PCIE_ATU_VIEWPORT_SIZE;
>  	}
>  
> +	dw_pcie_iatu_detect_regions(pci);
> +
>  	dev_info(pci->dev, "iATU unroll: %s\n", pci->iatu_unroll_enabled ?
>  		"enabled" : "disabled");
>  
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 72d185ff72f3..c18f0db09b31 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -103,10 +103,21 @@
>  #define PCIE_VERSION_NUMBER		0x8F8
>  #define PCIE_VERSION_TYPE		0x8FC
>  
> +/*
> + * iATU inbound and outbound windows CSRs. Before the IP-core v4.80a each
> + * iATU region CSRs had been indirectly accessible by means of the dedicated
> + * viewport selector. The iATU/eDMA CSRs space was re-designed in DWC PCIe
> + * v4.80a in a way so the viewport was unrolled into the directly accessible
> + * iATU/eDMA CSRs space.
> + */
>  #define PCIE_ATU_VIEWPORT		0x900
>  #define PCIE_ATU_REGION_DIR_IB		BIT(31)
>  #define PCIE_ATU_REGION_DIR_OB		0
> -#define PCIE_ATU_CR1			0x904
> +#define PCIE_ATU_VIEWPORT_BASE		0x904
> +#define PCIE_ATU_UNROLL_BASE(dir, index) \
> +	(((index) << 9) | ((dir == PCIE_ATU_REGION_DIR_IB) ? BIT(8) : 0))
> +#define PCIE_ATU_VIEWPORT_SIZE		0x2C
> +#define PCIE_ATU_REGION_CTRL1		0x000
>  #define PCIE_ATU_INCREASE_REGION_SIZE	BIT(13)
>  #define PCIE_ATU_TYPE_MEM		0x0
>  #define PCIE_ATU_TYPE_IO		0x2
> @@ -114,19 +125,19 @@
>  #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_REGION_CTRL2		0x004
>  #define PCIE_ATU_ENABLE			BIT(31)
>  #define PCIE_ATU_BAR_MODE_ENABLE	BIT(30)
>  #define PCIE_ATU_FUNC_NUM_MATCH_EN      BIT(19)
> -#define PCIE_ATU_LOWER_BASE		0x90C
> -#define PCIE_ATU_UPPER_BASE		0x910
> -#define PCIE_ATU_LIMIT			0x914
> -#define PCIE_ATU_LOWER_TARGET		0x918
> +#define PCIE_ATU_LOWER_BASE		0x008
> +#define PCIE_ATU_UPPER_BASE		0x00C
> +#define PCIE_ATU_LIMIT			0x010
> +#define PCIE_ATU_LOWER_TARGET		0x014
>  #define PCIE_ATU_BUS(x)			FIELD_PREP(GENMASK(31, 24), x)
>  #define PCIE_ATU_DEV(x)			FIELD_PREP(GENMASK(23, 19), x)
>  #define PCIE_ATU_FUNC(x)		FIELD_PREP(GENMASK(18, 16), x)
> -#define PCIE_ATU_UPPER_TARGET		0x91C
> -#define PCIE_ATU_UPPER_LIMIT		0x924
> +#define PCIE_ATU_UPPER_TARGET		0x018
> +#define PCIE_ATU_UPPER_LIMIT		0x020
>  
>  #define PCIE_MISC_CONTROL_1_OFF		0x8BC
>  #define PCIE_DBI_RO_WR_EN		BIT(0)
> @@ -143,19 +154,6 @@
>  
>  #define PCIE_PL_CHK_REG_ERR_ADDR			0xB28
>  
> -/*
> - * iATU Unroll-specific register definitions
> - * From 4.80 core version the address translation will be made by unroll
> - */
> -#define PCIE_ATU_UNR_REGION_CTRL1	0x00
> -#define PCIE_ATU_UNR_REGION_CTRL2	0x04
> -#define PCIE_ATU_UNR_LOWER_BASE		0x08
> -#define PCIE_ATU_UNR_UPPER_BASE		0x0C
> -#define PCIE_ATU_UNR_LOWER_LIMIT	0x10
> -#define PCIE_ATU_UNR_LOWER_TARGET	0x14
> -#define PCIE_ATU_UNR_UPPER_TARGET	0x18
> -#define PCIE_ATU_UNR_UPPER_LIMIT	0x20
> -
>  /*
>   * The default address offset between dbi_base and atu_base. Root controller
>   * drivers are not required to initialize atu_base if the offset matches this
> @@ -164,13 +162,6 @@
>   */
>  #define DEFAULT_DBI_ATU_OFFSET (0x3 << 20)
>  
> -/* Register address builder */
> -#define PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(region) \
> -		((region) << 9)
> -
> -#define PCIE_GET_ATU_INB_UNR_REG_OFFSET(region) \
> -		(((region) << 9) | BIT(8))
> -
>  #define MAX_MSI_IRQS			256
>  #define MAX_MSI_IRQS_PER_CTRL		32
>  #define MAX_MSI_CTRLS			(MAX_MSI_IRQS / MAX_MSI_IRQS_PER_CTRL)
> @@ -277,7 +268,6 @@ 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;
>  	size_t			atu_size;
>  	u32			num_ib_windows;
> diff --git a/drivers/pci/controller/dwc/pcie-tegra194-acpi.c b/drivers/pci/controller/dwc/pcie-tegra194-acpi.c
> index c2de6ed4d86f..55f61914a986 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194-acpi.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194-acpi.c
> @@ -39,7 +39,8 @@ static int tegra194_acpi_init(struct pci_config_window *cfg)
>  static void atu_reg_write(struct tegra194_pcie_ecam *pcie_ecam, int index,
>  			  u32 val, u32 reg)
>  {
> -	u32 offset = PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(index);
> +	u32 offset = PCIE_ATU_UNROLL_BASE(PCIE_ATU_REGION_DIR_OB, index) +
> +		     PCIE_ATU_VIEWPORT_BASE;
>  
>  	writel(val, pcie_ecam->iatu_base + offset + reg);
>  }
> @@ -58,8 +59,8 @@ static void program_outbound_atu(struct tegra194_pcie_ecam *pcie_ecam,
>  		      PCIE_ATU_LIMIT);
>  	atu_reg_write(pcie_ecam, index, upper_32_bits(pci_addr),
>  		      PCIE_ATU_UPPER_TARGET);
> -	atu_reg_write(pcie_ecam, index, type, PCIE_ATU_CR1);
> -	atu_reg_write(pcie_ecam, index, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
> +	atu_reg_write(pcie_ecam, index, type, PCIE_ATU_REGION_CTRL1);
> +	atu_reg_write(pcie_ecam, index, PCIE_ATU_ENABLE, PCIE_ATU_REGION_CTRL2);
>  }
>  
>  static void __iomem *tegra194_map_bus(struct pci_bus *bus,
> -- 
> 2.35.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH RESEND v4 11/15] PCI: dwc: Simplify in/outbound iATU setup methods

[Bjorn Helgaas]

On Mon, Aug 01, 2022 at 07:20:57PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Jun 24, 2022 at 05:39:43PM +0300, Serge Semin wrote:
> > From maintainability and scalability points of view it has been wrong to
> > use different iATU inbound and outbound regions accessors for the viewport
> > and unrolled versions of the iATU CSRs mapping. Seeing the particular iATU
> > region-wise registers layout is almost fully compatible for different
> > IP-core versions, there were no much points in splitting the code up that
> > way since it was possible to implement a common windows setup methods for
> > both viewport and unrolled iATU CSRs spaces. While what we can observe in
> > the current driver implementation of these methods, is a lot of code
> > duplication, which consequently worsen the code readability,
> > maintainability and scalability. Note the current implementation is a bit
> > more performant than the one suggested in this commit since it implies
> > having less MMIO accesses. But the gain just doesn't worth having the
> > denoted difficulties especially seeing the iATU setup methods are mainly
> > called on the DW PCIe controller and peripheral devices initialization
> > stage.
> > 
> > Here we suggest to move the iATU viewport and unrolled CSR access
> > specifics in the dw_pcie_readl_atu() and dw_pcie_writel_atu() method, and
> > convert the dw_pcie_prog_outbound_atu() and
> > dw_pcie_prog_{ep_}inbound_atu() functions to being generic instead of
> > having a different methods for each viewport and unrolled types of iATU
> > CSRs mapping. Nothing complex really. First of all the dw_pcie_readl_atu()
> > and dw_pcie_writel_atu() are converted to accept relative iATU CSRs
> > address together with the iATU region direction (inbound or outbound) and
> > region index. If DW PCIe controller doesn't have the unrolled iATU CSRs
> > space, then the accessors will need to activate a iATU viewport based on
> > the specified direction and index, otherwise a base address for the
> > corresponding region CSRs will be calculated by means of the
> > PCIE_ATU_UNROLL_BASE() macro. The CSRs macro have been modified in
> > accordance with that logic in the pcie-designware.h header file.
> > 
> > The rest of the changes in this commit just concern converting the iATU
> > in-/out-bound setup methods and iATU regions detection procedure to be
> > compatible with the new accessors semantics. As a result we've dropped the
> > no more required dw_pcie_prog_outbound_atu_unroll(),
> > dw_pcie_prog_inbound_atu_unroll() and dw_pcie_iatu_detect_regions_unroll()
> > methods.
> > 
> > Note aside with the denoted code improvements, there is an additional
> > positive side effect of this change. If at some point an atomic iATU
> > configs setup procedure is required, it will be possible to be done with
> > no much effort just by adding the synchronization into the
> > dw_pcie_readl_atu() and dw_pcie_writel_atu() accessors.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> One nitpick mentioned below. With that fixed,
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> > +static inline void __iomem *dw_pcie_select_atu(struct dw_pcie *pci, u32 dir,
> 
> This could be renamed to "dw_pcie_get_atu_base()" since we are anyway getting
> the base address of iATU.

I can see it both ways.  It definitely returns a base address, so
"get_atu_base" makes sense.  But it also writes PCIE_ATU_VIEWPORT, and
"select_atu" hints at that side effect while "get_atu_base" does not.

> > +					       u32 index)
> >  {
> > +	void __iomem *base = pci->atu_base;
> > +
> > +	if (pci->iatu_unroll_enabled)
> > +		base += PCIE_ATU_UNROLL_BASE(dir, index);
> > +	else
> > +		dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, dir | index);
> > +
> > +	return base;
> > +}