mbox series

[00/16] PCI: dwc: Add dma-ranges/YAML-schema/Baikal-T1 support

Message ID 20220324013734.18234-1-Sergey.Semin@baikalelectronics.ru
Headers show
Series PCI: dwc: Add dma-ranges/YAML-schema/Baikal-T1 support | expand

Message

Serge Semin March 24, 2022, 1:37 a.m. UTC
This patchset is a third one in the series created in the framework of
my Baikal-T1 PCIe/eDMA-related work:

[1: In-progress] clk: Baikal-T1 DDR/PCIe resets and some xGMAC fixes
Link: --submitted--
[2: In-progress] PCI: dwc: Various fixes and cleanups
Link: --submitted--
[3: In-progress] PCI: dwc: Add dma-ranges/YAML-schema/Baikal-T1 support
Link: --you are looking at it--
[4: Stalling]    dmaengine: dw-edma: Add RP/EP local DMA controllers support
Link: --being submitted afterwards--

Note it is very recommended to merge the patchsets in the same order as
they are listed in the set above in order to have them applied smoothly.
Nothing prevents them from being reviewed synchronously though.

This series is about adding new features to the DW PCIe Host/End-point
driver. First of all we start from splitting up the DT-bindings into
common properties schema and generic DW PCIe bindings definition. It's
done to support the generic DW PCIe Host/End-point available platforms
with adding a common YAML-schema to be reused by the platform-specific DW
PCIe bindings. @Rob could you please take a look at that patch? I've got a
problem with dt_bindings_check-ing the schema which is likely caused by
the dt-schema parser misbehaviour. After that we suggest to add a more
verbose link-up log message. Really printing link generation and width
would be much more informative than just "link up". Then a series of
IP-core version-related patches go, like using a native FourCC version
representation, adding the IP-core auto-detection, adding better
structured IP-core version/type interface. After that the
platform-specific host de-initialization method is introduced. A series of
iATU optimizations, cleanups and new features goes afterwards. In
particular we suggest to drop some redundant enumerations, add iATU
regions size detection procedure and then use the regions parameters to
verify the requested by the platform iATU ranges/dma-ranges settings.
After that the dma-ranges property support is added for the DW PCIe Host
controllers. Then a structured set of the DW PCIe RP/EP specific clocks
and resets names/IDs is introduced so to be re-used by the generic and new
platforms. Note it is fully coherent with the DW PCIe controller manuals
(see the patch log for details). Also note the patch doesn't affect the
already available DW PCIe platform-specific code since it would be too
risky for my to do the corresponding conversion, but the maintainers are
welcome to do that. Finally at the series closure we introduce the
Baikal-T1 PCIe interface support, which uses all the recently added
features including the set of the generic clocks and resets names.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Rob Herring <robh@kernel.org>
Cc: "Krzysztof WilczyƄski" <kw@linux.com>
Cc: Frank Li <Frank.Li@nxp.com>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-pci@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Serge Semin (16):
  dt-bindings: PCI: dwc: Define generic and native DT bindings
  dt-bindings: PCI: dwc: Add Baikal-T1 PCIe Root Port bindings
  PCI: dwc: Add more verbose link-up message
  PCI: dwc: Convert to using native IP-core versions representation
  PCI: dwc: Add IP-core version detection procedure
  PCI: dwc: Introduce Synopsys IP-core versions/types interface
  PCI: dwc: Add host de-initialization callback
  PCI: dwc: Drop inbound iATU types enumeration - dw_pcie_as_type
  PCI: dwc: Simplify in/outbound iATU setup methods
  PCI: dwc: Drop iATU regions enumeration - dw_pcie_region_type
  PCI: dwc: Add iATU regions size detection procedure
  PCI: dwc: Verify in/out regions against iATU constraints
  PCI: dwc: Check iATU in/outbound ranges setup methods status
  PCI: dwc: Introduce dma-ranges property support for RC-host
  PCI: dwc: Introduce generic platform clocks and resets sets
  PCI: dwc: Add Baikal-T1 PCIe controller support

 .../bindings/pci/baikal,bt1-pcie.yaml         | 148 ++++
 .../bindings/pci/fsl,imx6q-pcie.yaml          |   5 +-
 .../bindings/pci/hisilicon,kirin-pcie.yaml    |   4 +-
 .../bindings/pci/sifive,fu740-pcie.yaml       |   4 +-
 .../bindings/pci/snps,dw-pcie-common.yaml     | 298 ++++++++
 .../bindings/pci/snps,dw-pcie-ep.yaml         | 143 ++--
 .../devicetree/bindings/pci/snps,dw-pcie.yaml | 189 ++++--
 .../bindings/pci/toshiba,visconti-pcie.yaml   |   2 +-
 drivers/pci/controller/dwc/Kconfig            |   9 +
 drivers/pci/controller/dwc/Makefile           |   1 +
 drivers/pci/controller/dwc/pci-keystone.c     |  12 +-
 drivers/pci/controller/dwc/pcie-bt1.c         | 638 ++++++++++++++++++
 .../pci/controller/dwc/pcie-designware-ep.c   |  36 +-
 .../pci/controller/dwc/pcie-designware-host.c | 198 ++++--
 drivers/pci/controller/dwc/pcie-designware.c  | 465 ++++++-------
 drivers/pci/controller/dwc/pcie-designware.h  | 200 ++++--
 drivers/pci/controller/dwc/pcie-intel-gw.c    |  10 +-
 drivers/pci/controller/dwc/pcie-tegra194.c    |   2 +-
 18 files changed, 1857 insertions(+), 507 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/baikal,bt1-pcie.yaml
 create mode 100644 Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
 create mode 100644 drivers/pci/controller/dwc/pcie-bt1.c

Comments

Serge Semin April 17, 2022, 3:13 p.m. UTC | #1
On Tue, Mar 29, 2022 at 10:08:53AM -0500, Rob Herring wrote:
> On Thu, Mar 24, 2022 at 04:37:23AM +0300, Serge Semin wrote:
> > Since DWC PCIe v4.70a the controller version and version type can be read
> > from the PORT_LOGIC.PCIE_VERSION_OFF and PORT_LOGIC.PCIE_VERSION_TYPE_OFF
> > registers respectively. Seeing the generic code has got version-dependent
> > parts let's use these registers to find out the controller version.  The
> > detection procedure is executed for both RC and EP modes right after the
> > platform-specific initialization. We can't do that earlier since the
> > glue-drivers can perform the DBI-related setups there including the bus
> > reference clocks activation, without which the CSRs just can't be read.
> > 
> > Note the CSRs content is zero on the older DWC PCIe controller. In that
> > case we have no choice but to rely on the platform setup.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > ---
> >  .../pci/controller/dwc/pcie-designware-ep.c   |  2 ++
> >  .../pci/controller/dwc/pcie-designware-host.c |  2 ++
> >  drivers/pci/controller/dwc/pcie-designware.c  | 24 +++++++++++++++++++
> >  drivers/pci/controller/dwc/pcie-designware.h  |  6 +++++
> >  4 files changed, 34 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index 7c9315fffe24..3b981d13cca9 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -645,6 +645,8 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
> >  	u32 reg;
> >  	int i;
> >  
> > +	dw_pcie_version_detect(pci);
> > +
> >  	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) &
> >  		   PCI_HEADER_TYPE_MASK;
> >  	if (hdr_type != PCI_HEADER_TYPE_NORMAL) {
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index 8364ea234e88..8f0d473ff770 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -398,6 +398,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >  		}
> >  	}
> >  
> > +	dw_pcie_version_detect(pci);
> > +
> >  	dw_pcie_iatu_detect(pci);
> >  
> >  	dw_pcie_setup_rc(pp);
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index c21373c6cb51..49c494d82042 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -16,6 +16,30 @@
> >  #include "../../pci.h"
> >  #include "pcie-designware.h"
> >  
> > +void dw_pcie_version_detect(struct dw_pcie *pci)
> > +{
> > +	u32 ver;
> > +
> > +	/* The content of the CSR is zero on DWC PCIe older than v4.70a */
> > +	ver = dw_pcie_readl_dbi(pci, PCIE_VERSION_NUMBER);
> > +	if (!ver)
> > +		return;
> > +
> > +	if (pci->version && pci->version != ver)
> > +		dev_warn(pci->dev, "Versions don't match (%08x != %08x)\n",
> > +			 pci->version, ver);
> 

> Trust the h/w is correct until we have a known case where it isn't. Just 
> read the h/w reg if pci->version is zero.
> 
> I would suspect as-is this will give some warnings. No doubt there is 
> some platform with multiple revs of Si that didn't change their version 
> in the driver. Or the author just guessed on the version that picked the 
> right paths in the driver.

I'm not sure I fully get it. Could you elaborate what you suggest
here?  Do you suggest for me to drop the warnings and rewrite the
pci->{version, type} fields with the values read from the registers if
these fields were set with zeros?

-Sergey

> 
> Rob
Serge Semin April 17, 2022, 5:52 p.m. UTC | #2
On Tue, Mar 29, 2022 at 10:31:18AM -0500, Rob Herring wrote:
> On Thu, Mar 24, 2022 at 04:37:28AM +0300, Serge Semin wrote:
> > There is no point in having the dw_pcie_region_type enumeration for almost
> > the same reasons as it was stated for dw_pcie_as_type. First of all it's
> > redundant since the driver already has a set of macro declared which
> > describe the possible inbound and outbound iATU regions. Having an
> > addition abstraction just needlessly complicates the code. Secondly
> > checking the region index passed to the dw_pcie_disable_atu() method for
> > validity is pointless since the erroneous situation will be just
> > ignored in the current code implementation. So to speak let's drop the
> > redundant dw_pcie_region_type enumeration replacing it with the direct
> > iATU direction macro usage.
> > 
> > While at it we suggest to convert the dw_pcie_disable_atu() method to
> > being more consistent with the dw_pcie_readl_atu{_ib}() and
> > dw_pcie_readl_atu{_ob}() functions by having the direction parameter
> > specified ahead of the region index. Thus the code will be a little bit
> > more pleasant to read.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-ep.c  |  4 ++--
> >  .../pci/controller/dwc/pcie-designware-host.c    |  2 +-
> >  drivers/pci/controller/dwc/pcie-designware.c     | 16 +---------------
> >  drivers/pci/controller/dwc/pcie-designware.h     |  9 +--------
> >  4 files changed, 5 insertions(+), 26 deletions(-)
> 

> This answers my question. I would have expected this to come before the 
> previous patch, but if it's easier to do it this way it's fine.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>

I thought about placing this patch before the previous one, but it
turned a bit easier for me to split the changes in the reverse order.
It has made this patch a bit smaller and more coherent. But seeing you
weren't happy with too many changes in the previous patch I'll do as
you suggest and change the patches order. Since the patch content will
be changed I won't add your reviewed-by tag there on v2. So please
consider re-reviewing it one more time.

-Sergey
Serge Semin April 17, 2022, 8:08 p.m. UTC | #3
On Tue, Mar 29, 2022 at 10:28:56AM -0500, Rob Herring wrote:
> On Thu, Mar 24, 2022 at 04:37:27AM +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>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware.c | 301 ++++++-------------
> >  drivers/pci/controller/dwc/pcie-designware.h |  50 ++-
> >  2 files changed, 112 insertions(+), 239 deletions(-)
> 

> Nice diffstat. I didn't really like how this was implemented either.

Yeah, the change has turned to be a bit bulky. I wouldn't really
bother with it if I didn't need to add the "dma-ranges" support.

> 
> I'm guessing you tested the unrolled case only? IIRC, QEMU has the older 
> interface. I can also throw this at kernel-ci if needed. There's a few 
> platforms with DWC that get tested.

In fact I tested the series on DW PCIe Host v4.60a with the legacy
iATU/eDMA registers mapping (viewport-based). So testing on the modern
controllers with the unrolled iATU/eDMA region would be very welcome.

> 
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index b983128584ff..f1aa6e2e85fe 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -205,48 +205,67 @@ 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 u32 dw_pcie_readl_atu(struct dw_pcie *pci, u32 dir, u32 region, u32 reg)
> >  {
> > +	void __iomem *base;
> >  	int ret;
> >  	u32 val;
> >  
> > +	if (pci->iatu_unroll_enabled) {
> > +		base = pci->atu_base;
> > +		reg = reg + PCIE_ATU_UNROLL_BASE(dir, region);
> > +	} else {
> > +		base = pci->dbi_base;
> > +		reg = reg + PCIE_ATU_VIEWPORT_BASE;
> > +
> > +		dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, dir | region);
> > +	}
> > +
> >  	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 region,
> > +			       u32 reg, u32 val)
> >  {
> > +	void __iomem *base;
> >  	int ret;
> >  
> > +	if (pci->iatu_unroll_enabled) {
> > +		base = pci->atu_base;
> > +		reg = reg + PCIE_ATU_UNROLL_BASE(dir, region);
> > +	} else {
> > +		base = pci->dbi_base;
> > +		reg = reg + PCIE_ATU_VIEWPORT_BASE;
> > +
> > +		dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, dir | region);
> > +	}
> 

> You have this same sequence twice. Make it a helper function.

Ok. I'll move this code into a helper function - dw_pcie_select_atu().

> 
> > +
> >  	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 region, 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, region, 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 region, 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, region, reg, val);
> >  }
> >  
> >  static inline u32 dw_pcie_enable_ecrc(u32 val)
> > @@ -290,50 +309,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 +319,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_OUTBOUND | 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 +377,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 region, 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, region, 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 region, 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, region, reg, val);
> >  }
> >  
> >  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > @@ -460,65 +393,51 @@ 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_dbi(pci, PCIE_ATU_VIEWPORT, PCIE_ATU_REGION_INBOUND |
> > -			   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_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_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, int index,
> >  			 enum dw_pcie_region_type type)
> >  {
> > -	int region;
> > +	u32 dir;
> >  
> >  	switch (type) {
> >  	case DW_PCIE_REGION_INBOUND:
> > -		region = PCIE_ATU_REGION_INBOUND;
> > +		dir = PCIE_ATU_REGION_DIR_IB;
> 

> Is this the same double definition with the enum?

Absolutely right. I'll drop it in the next patch of this series. I'll
also move that patch to being applied before this one so to prevent
this question from araising and to simplify this patch a bit.

> 
> >  		break;
> >  	case DW_PCIE_REGION_OUTBOUND:
> > -		region = PCIE_ATU_REGION_OUTBOUND;
> > +		dir = PCIE_ATU_REGION_DIR_OB;
> >  		break;
> >  	default:
> >  		return;
> >  	}
> >  
> > -	if (pci->iatu_unroll_enabled) {
> > -		if (region == PCIE_ATU_REGION_INBOUND) {
> > -			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, region | 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)
> > @@ -622,63 +541,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_OUTBOUND | 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_INBOUND | 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;
> >  	}
> >  
> > @@ -707,12 +592,10 @@ 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);
> >  	}
> >  
> > +	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 449c5ad92edc..6adf0c957c3b 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -103,10 +103,20 @@
> >  #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.
> 
> IIRC, I think it is configurable in later versions. There was some 
> discussion when I did the detection.

There are two internal IP-core parameters which indicate whether the
unrolled eDMA/iATU CSRs mapping is enabled:
CC_UNROLL_EN = (CC_DMA_ENABLE || CX_INTERNAL_ATU_ENABLE) && !AHB_POPULATED
and
CC_UNROLL_ENABLE = ((CC_UNROLL_EN ==0) ? 0: 1)
Normally these parameters shouldn't be manually changed since they
aren't advertised in the list of the configurable IP-core parameters.
But there is a note in the databook (v4.90a, v5.20a, v5.40a) which
suggests to contact the Synopsys support if it's required to enable
the legacy mapping on the later IP-core versions. So basically you are
right. It is configurable, but not in the normal circumstances.
Practically I don't think there is any modern DW PCIe controller
implemented with the legacy eDMA/iATU CSRs mapping since the modern
CPUs/SoC-Interconnects mainly have enough unused IO space for
the unrolled access.

-Sergey

> 
> > + */
> >  #define PCIE_ATU_VIEWPORT		0x900
> > -#define PCIE_ATU_REGION_INBOUND		BIT(31)
> > -#define PCIE_ATU_REGION_OUTBOUND	0
> > -#define PCIE_ATU_CR1			0x904
> > +#define PCIE_ATU_REGION_DIR_IB		BIT(31)
> > +#define PCIE_ATU_REGION_DIR_OB		0
> > +#define PCIE_ATU_VIEWPORT_BASE		0x904
> > +#define PCIE_ATU_UNROLL_BASE(dir, region) \
> > +	(((region) << 9) | ((dir == PCIE_ATU_REGION_DIR_IB) ? BIT(8) : 0))
> > +#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 +124,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 +153,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 +161,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)
> > -- 
> > 2.35.1
> >