[PATCH 00/12] PCI: dwc: Various fixes and cleanups

[PATCH 00/12] PCI: dwc: Various fixes and cleanups

[Serge Semin]

This patchset is a second 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: --you are looking at it--
[3: Stalling]    PCI: dwc: Add dma-ranges/YAML-schema/Baikal-T1 support
Link: --being submitted afterwards--
[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 placed in the list above in order to prevent possible merge/build
conflicts. Nothing prevents them from being reviewed synchronously though.

As it can be easily inferred from the patchset title, this series is about
the DW PCIe Root Port/End-point driver fixes and the code cleanups, where
fixes come before the cleanup patches. The patchset starts with adding the
stop_link() platform-specific method invocation in case of the PCIe host
probe procedure errors. It has been missing in the cleanup-on-error path
of the DW PCIe Host initialization method. After that there is a patch
which fixes the host own cfg-space accessors for the case of the
platform-specific DBI implementation. Third the unrolled CSRs layout is
added to the iATU disable procedure. Fourth the disable iATU procedure is
fixed to be called only for the internal ATU as being specific for the
internal ATU implementation. Last but no least the outbound iATU extended
region setup procedure is fixed to have the INCREASE_REGION_SIZE flag set
based on the limit-address - not the region size one.

Afterwards there is a series of cleanups. It concerns the changes like
adding braces to the multi-line if-else constructions, trailing new-lines
to the print format-string, dropping unnecessary version checking, and
various code simplifications and optimizations.

New features like adding two-level DT bindings abstraction, adding better
structured IP-core version interface, adding iATU regions size detection
and the PCIe regions verification procedure, adding dma-ranges support,
introducing a set of generic platform clocks and resets and finally adding
Baikal-T1 PCIe interface support will be submitted in the next part of the
series.

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: linux-pci@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Serge Semin (12):
  PCI: dwc: Stop link in the host init error and de-initialization
  PCI: dwc: Don't use generic IO-ops for DBI-space access
  PCI: dwc: Add unroll iATU space support to the regions disable method
  PCI: dwc: Disable outbound windows for controllers with iATU
  PCI: dwc: Set INCREASE_REGION_SIZE flag based on limit address
  PCI: dwc: Add braces to the multi-line if-else statements
  PCI: dwc: Add trailing new-line literals to the log messages
  PCI: dwc: Discard IP-core version checking on unrolled iATU detection
  PCI: dwc: Convert Link-up status method to using dw_pcie_readl_dbi()
  PCI: dwc-plat: Simplify the probe method return value handling
  PCI: dwc-plat: Discard unused regmap pointer
  PCI: dwc-plat: Drop dw_plat_pcie_of_match forward declaration

 .../pci/controller/dwc/pcie-designware-ep.c   |  4 +-
 .../pci/controller/dwc/pcie-designware-host.c | 66 +++++++++++++++----
 .../pci/controller/dwc/pcie-designware-plat.c | 13 ++--
 drivers/pci/controller/dwc/pcie-designware.c  | 48 +++++++++-----
 4 files changed, 93 insertions(+), 38 deletions(-)

-- 
2.35.1

[PATCH 01/12] PCI: dwc: Stop link in the host init error and de-initialization

[Serge Semin]

It's logically correct to undo everything what was done in case of an
error is discovered or in the corresponding cleanup counterpart. Otherwise
the host controller will be left in an undetermined state. Seeing the link
is set up in the Host-initialization method it will be right to
de-activate it there in the cleanup-on-error block and stop the link in
the antagonistic routine - dw_pcie_host_deinit(). The link de-activation
is a platform-specific thing and is supposed to be implemented in the
framework of the dw_pcie_ops.stop_link() operation.

Fixes: 886a9c134755 ("PCI: dwc: Move link handling into common code")
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 .../pci/controller/dwc/pcie-designware-host.c    | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index f4755f3a03be..a03619a30c20 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -414,8 +414,14 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	bridge->sysdata = pp;
 
 	ret = pci_host_probe(bridge);
-	if (!ret)
-		return 0;
+	if (ret)
+		goto err_stop_link;
+
+	return 0;
+
+err_stop_link:
+	if (pci->ops && pci->ops->stop_link)
+		pci->ops->stop_link(pci);
 
 err_free_msi:
 	if (pp->has_msi_ctrl)
@@ -426,8 +432,14 @@ EXPORT_SYMBOL_GPL(dw_pcie_host_init);
 
 void dw_pcie_host_deinit(struct pcie_port *pp)
 {
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
 	pci_stop_root_bus(pp->bridge->bus);
 	pci_remove_root_bus(pp->bridge->bus);
+
+	if (pci->ops && pci->ops->stop_link)
+		pci->ops->stop_link(pci);
+
 	if (pp->has_msi_ctrl)
 		dw_pcie_free_msi(pp);
 }
-- 
2.35.1

[PATCH 02/12] PCI: dwc: Don't use generic IO-ops for DBI-space access

[Serge Semin]

Commit c2b0c098fbd1 ("PCI: dwc: Use generic config accessors") replaced
the locally defined DW PCIe host controller config-space accessors with
the generic methods pci_generic_config_read() and
pci_generic_config_write(). It was intended that the corresponding
bus-mapping callback returned a correct virtual address of the passed PCI
config-space register. The problem of the proposed solution was that it
didn't take into account the way the host config-space is accessed on the
DW PCIe. Depending on the DW PCIe IP-core synthesize parameters different
interfaces can be used to access the host and peripheral config/memory
spaces. The former one can be accessed via the DBI interface, while the
later ones is reached via the AHB/AXI application bus. In case if the DW
PCIe controller is configured to have a dedicated DBI interface, the way
it is mapped into the IO-memory turns to be platform-specific. For such
setups the DWC PCIe driver provides a set of the callbacks
dw_pcie_ops.{read_dbi,write_dbi} so the platforms glue-drivers would be
able to take into account the DBI bus IO peculiarities. Since
commit c2b0c098fbd1 ("PCI: dwc: Use generic config accessors") these
methods haven't been utilized during the generic host initialization
performed by the PCIe subsystem code.

I don't really know how come there have been no problems spotted for the
Histb/Exynos/Kirin PCIe controllers so far, but in our case with dword
aligned IO requirement the generic config-space accessors can't be
utilized for the host config-space. Thus in order to make sure the host
config-space is properly accessed via the DBI bus let's get back the
dw_pcie_rd_own_conf() and dw_pcie_wr_own_conf() methods. They are going to
be just wrappers around the already defined
dw_pcie_read_dbi()/dw_pcie_write_dbi() functions with proper arguments
conversion. These methods perform the platform-specific config-space IO if
the DBI accessors are specified, otherwise they call normal MMIO
operations.

Fixes: c2b0c098fbd1 ("PCI: dwc: Use generic config accessors")
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 .../pci/controller/dwc/pcie-designware-host.c | 34 +++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index a03619a30c20..f89e6552139b 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -528,10 +528,40 @@ void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, unsigned int devfn,
 }
 EXPORT_SYMBOL_GPL(dw_pcie_own_conf_map_bus);
 
+static int dw_pcie_rd_own_conf(struct pci_bus *bus, unsigned int devfn,
+			       int where, int size, u32 *val)
+{
+	struct pcie_port *pp = bus->sysdata;
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+	if (PCI_SLOT(devfn) > 0) {
+		*val = ~0U;
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	}
+
+	*val = dw_pcie_read_dbi(pci, where, size);
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int dw_pcie_wr_own_conf(struct pci_bus *bus, unsigned int devfn,
+			       int where, int size, u32 val)
+{
+	struct pcie_port *pp = bus->sysdata;
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+	if (PCI_SLOT(devfn) > 0)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	dw_pcie_write_dbi(pci, where, size, val);
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
 static struct pci_ops dw_pcie_ops = {
 	.map_bus = dw_pcie_own_conf_map_bus,
-	.read = pci_generic_config_read,
-	.write = pci_generic_config_write,
+	.read = dw_pcie_rd_own_conf,
+	.write = dw_pcie_wr_own_conf,
 };
 
 void dw_pcie_setup_rc(struct pcie_port *pp)
-- 
2.35.1

[PATCH 03/12] PCI: dwc: Add unroll iATU space support to the regions disable method

[Serge Semin]

The dw_pcie_disable_atu() method was introduced in the commit f8aed6ec624f
("PCI: dwc: designware: Add EP mode support"). Since then it hasn't
changed at all.  For all that time the method has supported the viewport
version of the iATU CSRs only. Basically it works for the DW PCIe IP-cores
older than v4.80a since the newer controllers are equipped with the
unrolled iATU/eDMA space. It means the methods using it like
pci_epc_ops.clear_bar and pci_epc_ops.unmap_addr callbacks just don't work
correctly for the DW PCIe controllers with unrolled iATU CSRs. The same
concerns the dw_pcie_setup_rc() method, which disables the outbound iATU
entries before re-initializing them.

So in order to fix the problems denoted above let's convert the
dw_pcie_disable_atu() method to disabling the iATU inbound and outbound
regions in the unrolled iATU CSRs in case the DW PCIe controller has been
synthesized with the ones support. The former semantics will be remained
for the controller having iATU mapped over the viewport.

Fixes: f8aed6ec624f ("PCI: dwc: designware: Add EP mode support")
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/pci/controller/dwc/pcie-designware.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index d92c8a25094f..7dc8c360a0d4 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -504,8 +504,18 @@ void dw_pcie_disable_atu(struct dw_pcie *pci, int index,
 		return;
 	}
 
-	dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, region | index);
-	dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, ~(u32)PCIE_ATU_ENABLE);
+	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);
+	}
 }
 
 int dw_pcie_wait_for_link(struct dw_pcie *pci)
-- 
2.35.1

[PATCH 04/12] PCI: dwc: Disable outbound windows for controllers with iATU

[Serge Semin]

In accordance with the dw_pcie_setup_rc() method semantics and judging by
what the comment added in commit dd193929d91e ("PCI: designware: Explain
why we don't program ATU for some platforms") states there are DWC
PCIe-available platforms like Keystone (pci-keystone.c) or Amazon's
Annapurna Labs (pcie-al.c) which don't have the DW PCIe internal ATU
enabled and use it's own address translation approach implemented. In
these cases at the very least there is no point in touching the DW PCIe
iATU CSRs. Moreover depending on the vendor-specific address translation
implementation it might be even erroneous. So let's move the iATU windows
disabling procedure to being under the corresponding conditional statement
clause thus performing that procedure only if the iATU is expected to be
available on the platform.

Fixes: 458ad06c4cdd ("PCI: dwc: Ensure all outbound ATU windows are reset")
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index f89e6552139b..a048d88e0c30 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -566,7 +566,6 @@ static struct pci_ops dw_pcie_ops = {
 
 void dw_pcie_setup_rc(struct pcie_port *pp)
 {
-	int i;
 	u32 val, ctrl, num_ctrls;
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 
@@ -618,19 +617,22 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 		PCI_COMMAND_MASTER | PCI_COMMAND_SERR;
 	dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
 
-	/* Ensure all outbound windows are disabled so there are multiple matches */
-	for (i = 0; i < pci->num_ob_windows; i++)
-		dw_pcie_disable_atu(pci, i, DW_PCIE_REGION_OUTBOUND);
-
 	/*
 	 * If the platform provides its own child bus config accesses, it means
 	 * the platform uses its own address translation component rather than
 	 * ATU, so we should not program the ATU here.
 	 */
 	if (pp->bridge->child_ops == &dw_child_pcie_ops) {
-		int atu_idx = 0;
+		int i, atu_idx = 0;
 		struct resource_entry *entry;
 
+		/*
+		 * Ensure all outbound windows are disabled so there are
+		 * multiple matches
+		 */
+		for (i = 0; i < pci->num_ob_windows; i++)
+			dw_pcie_disable_atu(pci, i, DW_PCIE_REGION_OUTBOUND);
+
 		/* Get last memory resource entry */
 		resource_list_for_each_entry(entry, &pp->bridge->windows) {
 			if (resource_type(entry->res) != IORESOURCE_MEM)
-- 
2.35.1

[PATCH 05/12] PCI: dwc: Set INCREASE_REGION_SIZE flag based on limit address

[Serge Semin]

It was wrong to use the region size parameter in order to determine
whether the INCREASE_REGION_SIZE flag needs to be set for the outbound
iATU entry because in general there are cases when combining a region base
address and size together produces the out of bounds upper range limit
while upper_32_bits(size) still returns zero. So having a region size
within the permitted values doesn't mean the region limit address will fit
to the corresponding CSR. Here is the way iATU calculates the in- and
outbound untranslated regions if the INCREASE_REGION_SIZE flag is cleared
[1]:

  Start address:                     End address:
63              31              0   63              31              0
+---------------+---------------+   +---------------+---------------+
|               |         |  0s |   |               |         |  Fs |
+---------------+---------------+   +---------------+---------------+
   upper base   |   lower base       !upper! base   | limit address
     address          address           address

So the region start address is determined by the iATU lower and upper base
address registers, while the region upper boundary is calculated based on
the 32-bits limit address register and the upper part of the base address.
In accordance with that logic for instance the range
0xf0000000 @ 0x20000000 does have the size smaller than 4GB, but the
actual limit address turns to be invalid forming the untranslated address
map as [0xf0000000; 0x0000FFFF], which isn't what the original range was.
In order to fix that we need to check whether the size being added to the
lower part of the base address causes the 4GB range overflow. If it does
then we need to set the INCREASE_REGION_SIZE flag thus activating the
extended limit address by means of an additional iATU CSR (upper limit
address register) [2]:

  Start address:                     End address:
63              31              0   63      x       31              0
+---------------+---------------+   +---------------+---------------+
|               |         |  0s |   |       |       |         |  Fs |
+---------------+---------------+   +---------------+---------------+
  upper base   |   lower base         upper | upper | limit address
    address          address          base  | limit |
                                     address|address|

Otherwise there is enough room in the 32-bits wide limit address register,
and the flag can be left unset.

Note the case when the size-based flag setting approach is correct implies
requiring to have the size-aligned base addresses only. But that
restriction isn't relevant to the PCIe ranges accepted by the kernel.
There is also no point in implementing it either seeing the problem can be
easily fixed by checking the whole limit address instead of the region
size.

[1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port,
    v5.40a, March 2019, fig.3-36, p.175
[2] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port,
    v5.40a, March 2019, fig.3-37, p.176

Fixes: 5b4cf0f65324 ("PCI: dwc: Add upper limit address for outbound iATU")
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/pci/controller/dwc/pcie-designware.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 7dc8c360a0d4..d737af058903 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -287,8 +287,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no,
 	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);
-	val = upper_32_bits(size - 1) ?
-		val | PCIE_ATU_INCREASE_REGION_SIZE : val;
+	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr))
+		val |= PCIE_ATU_INCREASE_REGION_SIZE;
 	if (pci->version == 0x490A)
 		val = dw_pcie_enable_ecrc(val);
 	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val);
@@ -315,6 +315,7 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
 					u64 pci_addr, u64 size)
 {
 	u32 retries, val;
+	u64 limit_addr;
 
 	if (pci->ops && pci->ops->cpu_addr_fixup)
 		cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
@@ -325,6 +326,8 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
 		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,
@@ -332,17 +335,18 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
 	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(cpu_addr + size - 1));
+			   lower_32_bits(limit_addr));
 	if (pci->version >= 0x460A)
 		dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_LIMIT,
-				   upper_32_bits(cpu_addr + size - 1));
+				   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));
 	val = type | PCIE_ATU_FUNC_NUM(func_no);
-	val = ((upper_32_bits(size - 1)) && (pci->version >= 0x460A)) ?
-		val | PCIE_ATU_INCREASE_REGION_SIZE : val;
+	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr) &&
+	    pci->version >= 0x460A)
+		val |= PCIE_ATU_INCREASE_REGION_SIZE;
 	if (pci->version == 0x490A)
 		val = dw_pcie_enable_ecrc(val);
 	dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, val);
-- 
2.35.1

[PATCH 06/12] PCI: dwc: Add braces to the multi-line if-else statements

[Serge Semin]

In accordance with [1] if there is at least one multi-line if-else
clause in the statement, then each clause will need to be surrounded by
the braces. The driver code violates that coding style rule in a few
places. Let's fix it.

[1] Documentation/process/coding-style.rst

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/pci/controller/dwc/pcie-designware-ep.c | 4 ++--
 drivers/pci/controller/dwc/pcie-designware.c    | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 0eda8236c125..7c9315fffe24 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -699,9 +699,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 
 	if (!pci->dbi_base2) {
 		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi2");
-		if (!res)
+		if (!res) {
 			pci->dbi_base2 = pci->dbi_base + SZ_4K;
-		else {
+		} else {
 			pci->dbi_base2 = devm_pci_remap_cfg_resource(dev, res);
 			if (IS_ERR(pci->dbi_base2))
 				return PTR_ERR(pci->dbi_base2);
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index d737af058903..9f4d2b44612b 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -699,8 +699,9 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
 			pci->atu_size = SZ_4K;
 
 		dw_pcie_iatu_detect_regions_unroll(pci);
-	} else
+	} else {
 		dw_pcie_iatu_detect_regions(pci);
+	}
 
 	dev_info(pci->dev, "iATU unroll: %s\n", pci->iatu_unroll_enabled ?
 		"enabled" : "disabled");
-- 
2.35.1

[PATCH 07/12] PCI: dwc: Add trailing new-line literals to the log messages

[Serge Semin]

There are two places in the DW PCIe controller driver, which omit the
trailing newlines in the log info and warn messages. Not having them there
will cause leaving the messages in the printk ring buffer until a next
'\n'-terminated message is printed (it will consequently cause the buffer
flush). As it isn't what the corresponding code implies let's add the
new-line characters in the messages.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 2 +-
 drivers/pci/controller/dwc/pcie-designware.c      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index a048d88e0c30..8364ea234e88 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -657,7 +657,7 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 		}
 
 		if (pci->num_ob_windows <= atu_idx)
-			dev_warn(pci->dev, "Resources exceed number of ATU entries (%d)",
+			dev_warn(pci->dev, "Resources exceed number of ATU entries (%d)\n",
 				 pci->num_ob_windows);
 	}
 
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 9f4d2b44612b..3bd1cfd12148 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -706,7 +706,7 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
 	dev_info(pci->dev, "iATU unroll: %s\n", pci->iatu_unroll_enabled ?
 		"enabled" : "disabled");
 
-	dev_info(pci->dev, "Detected iATU regions: %u outbound, %u inbound",
+	dev_info(pci->dev, "Detected iATU regions: %u outbound, %u inbound\n",
 		 pci->num_ob_windows, pci->num_ib_windows);
 }
 
-- 
2.35.1

[PATCH 08/12] PCI: dwc: Discard IP-core version checking on unrolled iATU detection

[Serge Semin]

It's pretty much pointless. Even though unrolled version of the internal
ATU has been indeed available since DWC PCIe v4.80a IP-core, there is no
guarantee it was enabled during the IP-core configuration (Synopsys
suggests to contact the Solvnet support for guidance of how to do that for
the newer IP-cores). So the only reliable way to find out the unrolled
iATU feature availability is indeed to check the iATU viewport register
content. In accordance with the reference manual [1] if the register
doesn't exist (unrolled iATU is enabled) it's content is fixed with
0xff-s, otherwise it will contain some zeros. So we can freely drop the
IP-core version checking in this matter then and use the
dw_pcie_iatu_unroll_enabled() method only to detect whether iATU/eDMA
space is unrolled.

[1] DesignWare Cores, PCI Express Controller, Register Desciptions,
v.4.90a, December 2016, p.855

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/pci/controller/dwc/pcie-designware.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 3bd1cfd12148..e3d2c11e6998 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -600,15 +600,15 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen)
 
 }
 
-static u8 dw_pcie_iatu_unroll_enabled(struct dw_pcie *pci)
+static bool dw_pcie_iatu_unroll_enabled(struct dw_pcie *pci)
 {
 	u32 val;
 
 	val = dw_pcie_readl_dbi(pci, PCIE_ATU_VIEWPORT);
 	if (val == 0xffffffff)
-		return 1;
+		return true;
 
-	return 0;
+	return false;
 }
 
 static void dw_pcie_iatu_detect_regions_unroll(struct dw_pcie *pci)
@@ -680,9 +680,8 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
 	struct device *dev = pci->dev;
 	struct platform_device *pdev = to_platform_device(dev);
 
-	if (pci->version >= 0x480A || (!pci->version &&
-				       dw_pcie_iatu_unroll_enabled(pci))) {
-		pci->iatu_unroll_enabled = true;
+	pci->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pci);
+	if (pci->iatu_unroll_enabled) {
 		if (!pci->atu_base) {
 			struct resource *res =
 				platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu");
-- 
2.35.1

[PATCH 09/12] PCI: dwc: Convert Link-up status method to using dw_pcie_readl_dbi()

[Serge Semin]

While the rest of the generic DWC PCIe code is using the dedicated IO-mem
accessors, the dw_pcie_link_up() method for some unobvious reason directly
calls readl() to get PortLogic.DEBUG1 register content. Since the way the
dbi-bus is accessed can be platform-specific let's replace the direct dbi
memory space read procedure with the readl-wrapper invocation. Thus we'll
have a slightly more generic dw_pcie_link_up() method.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/pci/controller/dwc/pcie-designware.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index e3d2c11e6998..6e81264fdfb4 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -548,7 +548,7 @@ int dw_pcie_link_up(struct dw_pcie *pci)
 	if (pci->ops && pci->ops->link_up)
 		return pci->ops->link_up(pci);
 
-	val = readl(pci->dbi_base + PCIE_PORT_DEBUG1);
+	val = dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1);
 	return ((val & PCIE_PORT_DEBUG1_LINK_UP) &&
 		(!(val & PCIE_PORT_DEBUG1_LINK_IN_TRAINING)));
 }
-- 
2.35.1

[PATCH 10/12] PCI: dwc-plat: Simplify the probe method return value handling

[Serge Semin]

The whole switch-case-logic implemented in the DWC PCIe RC/EP probe
procedure doesn't seem well thought through. First of all the ret variable
is unused in the EP-case and is only partly involved in the RC-case of the
switch-case statement, which unnecessary complicates the code. Secondly
the probe method will return zero if an unknown mode is detected. That is
improbable situation since the OF-device data is initialized only with
valid modes, but such code is still wrong at least from maintainability
point of view. So let's convert the switch-case part of the probe function
to being more coherent. We suggest to use the local ret variable to
preserve the status of the case-clauses and return its value from the
probe procedure after the work is done.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/pci/controller/dwc/pcie-designware-plat.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-plat.c b/drivers/pci/controller/dwc/pcie-designware-plat.c
index 0c5de87d3cc6..fea785096261 100644
--- a/drivers/pci/controller/dwc/pcie-designware-plat.c
+++ b/drivers/pci/controller/dwc/pcie-designware-plat.c
@@ -153,20 +153,21 @@ static int dw_plat_pcie_probe(struct platform_device *pdev)
 			return -ENODEV;
 
 		ret = dw_plat_add_pcie_port(dw_plat_pcie, pdev);
-		if (ret < 0)
-			return ret;
 		break;
 	case DW_PCIE_EP_TYPE:
 		if (!IS_ENABLED(CONFIG_PCIE_DW_PLAT_EP))
 			return -ENODEV;
 
 		pci->ep.ops = &pcie_ep_ops;
-		return dw_pcie_ep_init(&pci->ep);
+		ret = dw_pcie_ep_init(&pci->ep);
+		break;
 	default:
 		dev_err(dev, "INVALID device type %d\n", dw_plat_pcie->mode);
+		ret = -EINVAL;
+		break;
 	}
 
-	return 0;
+	return ret;
 }
 
 static const struct dw_plat_pcie_of_data dw_plat_pcie_rc_of_data = {
-- 
2.35.1

[PATCH 11/12] PCI: dwc-plat: Discard unused regmap pointer

[Serge Semin]

The regmap pointer was added into the dw_plat_pcie structure in
commit 1d906b22076e ("PCI: dwc: Add support for EP mode"), but it hasn't
been utilized neither in the code submitted in the denoted so far nor in
the platform driver evolving afterwards. Drop it then for good.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/pci/controller/dwc/pcie-designware-plat.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-plat.c b/drivers/pci/controller/dwc/pcie-designware-plat.c
index fea785096261..99cf2ac5b0ba 100644
--- a/drivers/pci/controller/dwc/pcie-designware-plat.c
+++ b/drivers/pci/controller/dwc/pcie-designware-plat.c
@@ -17,13 +17,11 @@
 #include <linux/platform_device.h>
 #include <linux/resource.h>
 #include <linux/types.h>
-#include <linux/regmap.h>
 
 #include "pcie-designware.h"
 
 struct dw_plat_pcie {
 	struct dw_pcie			*pci;
-	struct regmap			*regmap;
 	enum dw_pcie_device_mode	mode;
 };
 
-- 
2.35.1

[PATCH 12/12] PCI: dwc-plat: Drop dw_plat_pcie_of_match forward declaration

[Serge Semin]

The denoted forward declaration used to be required to get the OF-device
ID structure by calling the of_match_device() method. The later method
invocation has been replaced with the of_device_get_match_data() call in
the commit 5c204204cf24 ("PCI: designware-plat: Prefer
of_device_get_match_data()"). Thus the forward declaration of the
OF-compatible device strings no longer needed. Drop it for good.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/pci/controller/dwc/pcie-designware-plat.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-plat.c b/drivers/pci/controller/dwc/pcie-designware-plat.c
index 99cf2ac5b0ba..e606c5d5f06f 100644
--- a/drivers/pci/controller/dwc/pcie-designware-plat.c
+++ b/drivers/pci/controller/dwc/pcie-designware-plat.c
@@ -29,8 +29,6 @@ struct dw_plat_pcie_of_data {
 	enum dw_pcie_device_mode	mode;
 };
 
-static const struct of_device_id dw_plat_pcie_of_match[];
-
 static const struct dw_pcie_host_ops dw_plat_pcie_host_ops = {
 };
 
-- 
2.35.1

Re: [PATCH 01/12] PCI: dwc: Stop link in the host init error and de-initialization

[Manivannan Sadhasivam]

On Thu, Mar 24, 2022 at 04:25:12AM +0300, Serge Semin wrote:
> It's logically correct to undo everything what was done in case of an
> error is discovered or in the corresponding cleanup counterpart. Otherwise
> the host controller will be left in an undetermined state. Seeing the link
> is set up in the Host-initialization method it will be right to
> de-activate it there in the cleanup-on-error block and stop the link in
> the antagonistic routine - dw_pcie_host_deinit(). The link de-activation
> is a platform-specific thing and is supposed to be implemented in the
> framework of the dw_pcie_ops.stop_link() operation.
> 
> Fixes: 886a9c134755 ("PCI: dwc: Move link handling into common code")
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

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

Thanks,
Mani

> ---
>  .../pci/controller/dwc/pcie-designware-host.c    | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index f4755f3a03be..a03619a30c20 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -414,8 +414,14 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  	bridge->sysdata = pp;
>  
>  	ret = pci_host_probe(bridge);
> -	if (!ret)
> -		return 0;
> +	if (ret)
> +		goto err_stop_link;
> +
> +	return 0;
> +
> +err_stop_link:
> +	if (pci->ops && pci->ops->stop_link)
> +		pci->ops->stop_link(pci);
>  
>  err_free_msi:
>  	if (pp->has_msi_ctrl)
> @@ -426,8 +432,14 @@ EXPORT_SYMBOL_GPL(dw_pcie_host_init);
>  
>  void dw_pcie_host_deinit(struct pcie_port *pp)
>  {
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
>  	pci_stop_root_bus(pp->bridge->bus);
>  	pci_remove_root_bus(pp->bridge->bus);
> +
> +	if (pci->ops && pci->ops->stop_link)
> +		pci->ops->stop_link(pci);
> +
>  	if (pp->has_msi_ctrl)
>  		dw_pcie_free_msi(pp);
>  }
> -- 
> 2.35.1
> 

Re: [PATCH 02/12] PCI: dwc: Don't use generic IO-ops for DBI-space access

[Manivannan Sadhasivam]

On Thu, Mar 24, 2022 at 04:25:13AM +0300, Serge Semin wrote:
> Commit c2b0c098fbd1 ("PCI: dwc: Use generic config accessors") replaced
> the locally defined DW PCIe host controller config-space accessors with
> the generic methods pci_generic_config_read() and
> pci_generic_config_write(). It was intended that the corresponding
> bus-mapping callback returned a correct virtual address of the passed PCI
> config-space register. The problem of the proposed solution was that it
> didn't take into account the way the host config-space is accessed on the
> DW PCIe. Depending on the DW PCIe IP-core synthesize parameters different
> interfaces can be used to access the host and peripheral config/memory
> spaces. The former one can be accessed via the DBI interface, while the
> later ones is reached via the AHB/AXI application bus. In case if the DW
> PCIe controller is configured to have a dedicated DBI interface, the way
> it is mapped into the IO-memory turns to be platform-specific. For such
> setups the DWC PCIe driver provides a set of the callbacks
> dw_pcie_ops.{read_dbi,write_dbi} so the platforms glue-drivers would be
> able to take into account the DBI bus IO peculiarities. Since
> commit c2b0c098fbd1 ("PCI: dwc: Use generic config accessors") these
> methods haven't been utilized during the generic host initialization
> performed by the PCIe subsystem code.
> 
> I don't really know how come there have been no problems spotted for the
> Histb/Exynos/Kirin PCIe controllers so far, but in our case with dword
> aligned IO requirement the generic config-space accessors can't be
> utilized for the host config-space. Thus in order to make sure the host
> config-space is properly accessed via the DBI bus let's get back the
> dw_pcie_rd_own_conf() and dw_pcie_wr_own_conf() methods. They are going to
> be just wrappers around the already defined
> dw_pcie_read_dbi()/dw_pcie_write_dbi() functions with proper arguments
> conversion. These methods perform the platform-specific config-space IO if
> the DBI accessors are specified, otherwise they call normal MMIO
> operations.
> 

Why can't you override the "pci_ops" in your host driver's "host_init"?

Thanks,
Mani

> Fixes: c2b0c098fbd1 ("PCI: dwc: Use generic config accessors")
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> ---
>  .../pci/controller/dwc/pcie-designware-host.c | 34 +++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index a03619a30c20..f89e6552139b 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -528,10 +528,40 @@ void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, unsigned int devfn,
>  }
>  EXPORT_SYMBOL_GPL(dw_pcie_own_conf_map_bus);
>  
> +static int dw_pcie_rd_own_conf(struct pci_bus *bus, unsigned int devfn,
> +			       int where, int size, u32 *val)
> +{
> +	struct pcie_port *pp = bus->sysdata;
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
> +	if (PCI_SLOT(devfn) > 0) {
> +		*val = ~0U;
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +	}
> +
> +	*val = dw_pcie_read_dbi(pci, where, size);
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int dw_pcie_wr_own_conf(struct pci_bus *bus, unsigned int devfn,
> +			       int where, int size, u32 val)
> +{
> +	struct pcie_port *pp = bus->sysdata;
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
> +	if (PCI_SLOT(devfn) > 0)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	dw_pcie_write_dbi(pci, where, size, val);
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
>  static struct pci_ops dw_pcie_ops = {
>  	.map_bus = dw_pcie_own_conf_map_bus,
> -	.read = pci_generic_config_read,
> -	.write = pci_generic_config_write,
> +	.read = dw_pcie_rd_own_conf,
> +	.write = dw_pcie_wr_own_conf,
>  };
>  
>  void dw_pcie_setup_rc(struct pcie_port *pp)
> -- 
> 2.35.1
> 

Re: [PATCH 03/12] PCI: dwc: Add unroll iATU space support to the regions disable method

[Manivannan Sadhasivam]

On Thu, Mar 24, 2022 at 04:25:14AM +0300, Serge Semin wrote:
> The dw_pcie_disable_atu() method was introduced in the commit f8aed6ec624f
> ("PCI: dwc: designware: Add EP mode support"). Since then it hasn't
> changed at all.  For all that time the method has supported the viewport
> version of the iATU CSRs only. Basically it works for the DW PCIe IP-cores
> older than v4.80a since the newer controllers are equipped with the
> unrolled iATU/eDMA space. It means the methods using it like
> pci_epc_ops.clear_bar and pci_epc_ops.unmap_addr callbacks just don't work
> correctly for the DW PCIe controllers with unrolled iATU CSRs. The same
> concerns the dw_pcie_setup_rc() method, which disables the outbound iATU
> entries before re-initializing them.
> 
> So in order to fix the problems denoted above let's convert the
> dw_pcie_disable_atu() method to disabling the iATU inbound and outbound
> regions in the unrolled iATU CSRs in case the DW PCIe controller has been
> synthesized with the ones support. The former semantics will be remained
> for the controller having iATU mapped over the viewport.
> 
> Fixes: f8aed6ec624f ("PCI: dwc: designware: Add EP mode support")
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

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

Thanks,
Mani

> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index d92c8a25094f..7dc8c360a0d4 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -504,8 +504,18 @@ void dw_pcie_disable_atu(struct dw_pcie *pci, int index,
>  		return;
>  	}
>  
> -	dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, region | index);
> -	dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, ~(u32)PCIE_ATU_ENABLE);
> +	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);
> +	}
>  }
>  
>  int dw_pcie_wait_for_link(struct dw_pcie *pci)
> -- 
> 2.35.1
> 

Re: [PATCH 04/12] PCI: dwc: Disable outbound windows for controllers with iATU

[Manivannan Sadhasivam]

On Thu, Mar 24, 2022 at 04:25:15AM +0300, Serge Semin wrote:
> In accordance with the dw_pcie_setup_rc() method semantics and judging by
> what the comment added in commit dd193929d91e ("PCI: designware: Explain
> why we don't program ATU for some platforms") states there are DWC
> PCIe-available platforms like Keystone (pci-keystone.c) or Amazon's
> Annapurna Labs (pcie-al.c) which don't have the DW PCIe internal ATU
> enabled and use it's own address translation approach implemented. In
> these cases at the very least there is no point in touching the DW PCIe
> iATU CSRs. Moreover depending on the vendor-specific address translation
> implementation it might be even erroneous. So let's move the iATU windows
> disabling procedure to being under the corresponding conditional statement
> clause thus performing that procedure only if the iATU is expected to be
> available on the platform.
> 
> Fixes: 458ad06c4cdd ("PCI: dwc: Ensure all outbound ATU windows are reset")
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

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

Thanks,
Mani

> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index f89e6552139b..a048d88e0c30 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -566,7 +566,6 @@ static struct pci_ops dw_pcie_ops = {
>  
>  void dw_pcie_setup_rc(struct pcie_port *pp)
>  {
> -	int i;
>  	u32 val, ctrl, num_ctrls;
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  
> @@ -618,19 +617,22 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>  		PCI_COMMAND_MASTER | PCI_COMMAND_SERR;
>  	dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
>  
> -	/* Ensure all outbound windows are disabled so there are multiple matches */
> -	for (i = 0; i < pci->num_ob_windows; i++)
> -		dw_pcie_disable_atu(pci, i, DW_PCIE_REGION_OUTBOUND);
> -
>  	/*
>  	 * If the platform provides its own child bus config accesses, it means
>  	 * the platform uses its own address translation component rather than
>  	 * ATU, so we should not program the ATU here.
>  	 */
>  	if (pp->bridge->child_ops == &dw_child_pcie_ops) {
> -		int atu_idx = 0;
> +		int i, atu_idx = 0;
>  		struct resource_entry *entry;
>  
> +		/*
> +		 * Ensure all outbound windows are disabled so there are
> +		 * multiple matches
> +		 */
> +		for (i = 0; i < pci->num_ob_windows; i++)
> +			dw_pcie_disable_atu(pci, i, DW_PCIE_REGION_OUTBOUND);
> +
>  		/* Get last memory resource entry */
>  		resource_list_for_each_entry(entry, &pp->bridge->windows) {
>  			if (resource_type(entry->res) != IORESOURCE_MEM)
> -- 
> 2.35.1
> 

Re: [PATCH 05/12] PCI: dwc: Set INCREASE_REGION_SIZE flag based on limit address

[Manivannan Sadhasivam]

On Thu, Mar 24, 2022 at 04:25:16AM +0300, Serge Semin wrote:
> It was wrong to use the region size parameter in order to determine
> whether the INCREASE_REGION_SIZE flag needs to be set for the outbound
> iATU entry because in general there are cases when combining a region base
> address and size together produces the out of bounds upper range limit
> while upper_32_bits(size) still returns zero. So having a region size
> within the permitted values doesn't mean the region limit address will fit
> to the corresponding CSR. Here is the way iATU calculates the in- and
> outbound untranslated regions if the INCREASE_REGION_SIZE flag is cleared
> [1]:
> 
>   Start address:                     End address:
> 63              31              0   63              31              0
> +---------------+---------------+   +---------------+---------------+
> |               |         |  0s |   |               |         |  Fs |
> +---------------+---------------+   +---------------+---------------+
>    upper base   |   lower base       !upper! base   | limit address
>      address          address           address
> 
> So the region start address is determined by the iATU lower and upper base
> address registers, while the region upper boundary is calculated based on
> the 32-bits limit address register and the upper part of the base address.
> In accordance with that logic for instance the range
> 0xf0000000 @ 0x20000000 does have the size smaller than 4GB, but the
> actual limit address turns to be invalid forming the untranslated address
> map as [0xf0000000; 0x0000FFFF], which isn't what the original range was.

I find the example confusing:

If the start address is 0x0-0xf0000000 and size is 0x20000000. Then the end
address without the INCREASE_REGION_SIZE is going to be:

0x0-0x1000FFFF and this is wrong.

If the INCREASE_REGION_SIZE is set, then the end address will be:

0x1-0x1000FFFF and this is correct.

> In order to fix that we need to check whether the size being added to the
> lower part of the base address causes the 4GB range overflow. If it does
> then we need to set the INCREASE_REGION_SIZE flag thus activating the
> extended limit address by means of an additional iATU CSR (upper limit
> address register) [2]:
> 
>   Start address:                     End address:
> 63              31              0   63      x       31              0
> +---------------+---------------+   +---------------+---------------+
> |               |         |  0s |   |       |       |         |  Fs |
> +---------------+---------------+   +---------------+---------------+
>   upper base   |   lower base         upper | upper | limit address
>     address          address          base  | limit |
>                                      address|address|
> 
> Otherwise there is enough room in the 32-bits wide limit address register,
> and the flag can be left unset.
> 
> Note the case when the size-based flag setting approach is correct implies
> requiring to have the size-aligned base addresses only. But that
> restriction isn't relevant to the PCIe ranges accepted by the kernel.
> There is also no point in implementing it either seeing the problem can be
> easily fixed by checking the whole limit address instead of the region
> size.
> 
> [1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port,
>     v5.40a, March 2019, fig.3-36, p.175
> [2] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port,
>     v5.40a, March 2019, fig.3-37, p.176
> 
> Fixes: 5b4cf0f65324 ("PCI: dwc: Add upper limit address for outbound iATU")
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

With the example fixed,

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

Thanks,
Mani

> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 7dc8c360a0d4..d737af058903 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -287,8 +287,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no,
>  	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);
> -	val = upper_32_bits(size - 1) ?
> -		val | PCIE_ATU_INCREASE_REGION_SIZE : val;
> +	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr))
> +		val |= PCIE_ATU_INCREASE_REGION_SIZE;
>  	if (pci->version == 0x490A)
>  		val = dw_pcie_enable_ecrc(val);
>  	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val);
> @@ -315,6 +315,7 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
>  					u64 pci_addr, u64 size)
>  {
>  	u32 retries, val;
> +	u64 limit_addr;
>  
>  	if (pci->ops && pci->ops->cpu_addr_fixup)
>  		cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
> @@ -325,6 +326,8 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
>  		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,
> @@ -332,17 +335,18 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
>  	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(cpu_addr + size - 1));
> +			   lower_32_bits(limit_addr));
>  	if (pci->version >= 0x460A)
>  		dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_LIMIT,
> -				   upper_32_bits(cpu_addr + size - 1));
> +				   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));
>  	val = type | PCIE_ATU_FUNC_NUM(func_no);
> -	val = ((upper_32_bits(size - 1)) && (pci->version >= 0x460A)) ?
> -		val | PCIE_ATU_INCREASE_REGION_SIZE : val;
> +	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr) &&
> +	    pci->version >= 0x460A)
> +		val |= PCIE_ATU_INCREASE_REGION_SIZE;
>  	if (pci->version == 0x490A)
>  		val = dw_pcie_enable_ecrc(val);
>  	dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, val);
> -- 
> 2.35.1
> 

Re: [PATCH 06/12] PCI: dwc: Add braces to the multi-line if-else statements

[Manivannan Sadhasivam]

On Thu, Mar 24, 2022 at 04:25:17AM +0300, Serge Semin wrote:
> In accordance with [1] if there is at least one multi-line if-else
> clause in the statement, then each clause will need to be surrounded by
> the braces. The driver code violates that coding style rule in a few
> places. Let's fix it.
> 
> [1] Documentation/process/coding-style.rst
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

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

Thanks,
Mani

> ---
>  drivers/pci/controller/dwc/pcie-designware-ep.c | 4 ++--
>  drivers/pci/controller/dwc/pcie-designware.c    | 3 ++-
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 0eda8236c125..7c9315fffe24 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -699,9 +699,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  
>  	if (!pci->dbi_base2) {
>  		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi2");
> -		if (!res)
> +		if (!res) {
>  			pci->dbi_base2 = pci->dbi_base + SZ_4K;
> -		else {
> +		} else {
>  			pci->dbi_base2 = devm_pci_remap_cfg_resource(dev, res);
>  			if (IS_ERR(pci->dbi_base2))
>  				return PTR_ERR(pci->dbi_base2);
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index d737af058903..9f4d2b44612b 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -699,8 +699,9 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
>  			pci->atu_size = SZ_4K;
>  
>  		dw_pcie_iatu_detect_regions_unroll(pci);
> -	} else
> +	} else {
>  		dw_pcie_iatu_detect_regions(pci);
> +	}
>  
>  	dev_info(pci->dev, "iATU unroll: %s\n", pci->iatu_unroll_enabled ?
>  		"enabled" : "disabled");
> -- 
> 2.35.1
> 

Re: [PATCH 07/12] PCI: dwc: Add trailing new-line literals to the log messages

[Manivannan Sadhasivam]

On Thu, Mar 24, 2022 at 04:25:18AM +0300, Serge Semin wrote:
> There are two places in the DW PCIe controller driver, which omit the
> trailing newlines in the log info and warn messages. Not having them there
> will cause leaving the messages in the printk ring buffer until a next
> '\n'-terminated message is printed (it will consequently cause the buffer
> flush). As it isn't what the corresponding code implies let's add the
> new-line characters in the messages.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

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

Thanks,
Mani

> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 2 +-
>  drivers/pci/controller/dwc/pcie-designware.c      | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index a048d88e0c30..8364ea234e88 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -657,7 +657,7 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>  		}
>  
>  		if (pci->num_ob_windows <= atu_idx)
> -			dev_warn(pci->dev, "Resources exceed number of ATU entries (%d)",
> +			dev_warn(pci->dev, "Resources exceed number of ATU entries (%d)\n",
>  				 pci->num_ob_windows);
>  	}
>  
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 9f4d2b44612b..3bd1cfd12148 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -706,7 +706,7 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
>  	dev_info(pci->dev, "iATU unroll: %s\n", pci->iatu_unroll_enabled ?
>  		"enabled" : "disabled");
>  
> -	dev_info(pci->dev, "Detected iATU regions: %u outbound, %u inbound",
> +	dev_info(pci->dev, "Detected iATU regions: %u outbound, %u inbound\n",
>  		 pci->num_ob_windows, pci->num_ib_windows);
>  }
>  
> -- 
> 2.35.1
> 

Re: [PATCH 08/12] PCI: dwc: Discard IP-core version checking on unrolled iATU detection

[Manivannan Sadhasivam]

On Thu, Mar 24, 2022 at 04:25:19AM +0300, Serge Semin wrote:
> It's pretty much pointless. Even though unrolled version of the internal
> ATU has been indeed available since DWC PCIe v4.80a IP-core, there is no
> guarantee it was enabled during the IP-core configuration (Synopsys
> suggests to contact the Solvnet support for guidance of how to do that for
> the newer IP-cores). So the only reliable way to find out the unrolled
> iATU feature availability is indeed to check the iATU viewport register
> content. In accordance with the reference manual [1] if the register
> doesn't exist (unrolled iATU is enabled) it's content is fixed with
> 0xff-s, otherwise it will contain some zeros. So we can freely drop the
> IP-core version checking in this matter then and use the
> dw_pcie_iatu_unroll_enabled() method only to detect whether iATU/eDMA
> space is unrolled.
> 
> [1] DesignWare Cores, PCI Express Controller, Register Desciptions,
> v.4.90a, December 2016, p.855
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

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

Thanks,
Mani

> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 3bd1cfd12148..e3d2c11e6998 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -600,15 +600,15 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen)
>  
>  }
>  
> -static u8 dw_pcie_iatu_unroll_enabled(struct dw_pcie *pci)
> +static bool dw_pcie_iatu_unroll_enabled(struct dw_pcie *pci)
>  {
>  	u32 val;
>  
>  	val = dw_pcie_readl_dbi(pci, PCIE_ATU_VIEWPORT);
>  	if (val == 0xffffffff)
> -		return 1;
> +		return true;
>  
> -	return 0;
> +	return false;
>  }
>  
>  static void dw_pcie_iatu_detect_regions_unroll(struct dw_pcie *pci)
> @@ -680,9 +680,8 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
>  	struct device *dev = pci->dev;
>  	struct platform_device *pdev = to_platform_device(dev);
>  
> -	if (pci->version >= 0x480A || (!pci->version &&
> -				       dw_pcie_iatu_unroll_enabled(pci))) {
> -		pci->iatu_unroll_enabled = true;
> +	pci->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pci);
> +	if (pci->iatu_unroll_enabled) {
>  		if (!pci->atu_base) {
>  			struct resource *res =
>  				platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu");
> -- 
> 2.35.1
> 

Re: [PATCH 09/12] PCI: dwc: Convert Link-up status method to using dw_pcie_readl_dbi()

[Manivannan Sadhasivam]

On Thu, Mar 24, 2022 at 04:25:20AM +0300, Serge Semin wrote:
> While the rest of the generic DWC PCIe code is using the dedicated IO-mem
> accessors, the dw_pcie_link_up() method for some unobvious reason directly
> calls readl() to get PortLogic.DEBUG1 register content. Since the way the
> dbi-bus is accessed can be platform-specific let's replace the direct dbi
> memory space read procedure with the readl-wrapper invocation. Thus we'll
> have a slightly more generic dw_pcie_link_up() method.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

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

Thanks,
Mani

> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index e3d2c11e6998..6e81264fdfb4 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -548,7 +548,7 @@ int dw_pcie_link_up(struct dw_pcie *pci)
>  	if (pci->ops && pci->ops->link_up)
>  		return pci->ops->link_up(pci);
>  
> -	val = readl(pci->dbi_base + PCIE_PORT_DEBUG1);
> +	val = dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1);
>  	return ((val & PCIE_PORT_DEBUG1_LINK_UP) &&
>  		(!(val & PCIE_PORT_DEBUG1_LINK_IN_TRAINING)));
>  }
> -- 
> 2.35.1
> 

Re: [PATCH 10/12] PCI: dwc-plat: Simplify the probe method return value handling

[Manivannan Sadhasivam]

On Thu, Mar 24, 2022 at 04:25:21AM +0300, Serge Semin wrote:
> The whole switch-case-logic implemented in the DWC PCIe RC/EP probe
> procedure doesn't seem well thought through. First of all the ret variable
> is unused in the EP-case and is only partly involved in the RC-case of the
> switch-case statement, which unnecessary complicates the code. Secondly
> the probe method will return zero if an unknown mode is detected. That is
> improbable situation since the OF-device data is initialized only with
> valid modes, but such code is still wrong at least from maintainability
> point of view. So let's convert the switch-case part of the probe function
> to being more coherent. We suggest to use the local ret variable to
> preserve the status of the case-clauses and return its value from the
> probe procedure after the work is done.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

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

Thanks,
Mani

> ---
>  drivers/pci/controller/dwc/pcie-designware-plat.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-plat.c b/drivers/pci/controller/dwc/pcie-designware-plat.c
> index 0c5de87d3cc6..fea785096261 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-plat.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-plat.c
> @@ -153,20 +153,21 @@ static int dw_plat_pcie_probe(struct platform_device *pdev)
>  			return -ENODEV;
>  
>  		ret = dw_plat_add_pcie_port(dw_plat_pcie, pdev);
> -		if (ret < 0)
> -			return ret;
>  		break;
>  	case DW_PCIE_EP_TYPE:
>  		if (!IS_ENABLED(CONFIG_PCIE_DW_PLAT_EP))
>  			return -ENODEV;
>  
>  		pci->ep.ops = &pcie_ep_ops;
> -		return dw_pcie_ep_init(&pci->ep);
> +		ret = dw_pcie_ep_init(&pci->ep);
> +		break;
>  	default:
>  		dev_err(dev, "INVALID device type %d\n", dw_plat_pcie->mode);
> +		ret = -EINVAL;
> +		break;
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static const struct dw_plat_pcie_of_data dw_plat_pcie_rc_of_data = {
> -- 
> 2.35.1
> 

Re: [PATCH 11/12] PCI: dwc-plat: Discard unused regmap pointer

[Manivannan Sadhasivam]

On Thu, Mar 24, 2022 at 04:25:22AM +0300, Serge Semin wrote:
> The regmap pointer was added into the dw_plat_pcie structure in
> commit 1d906b22076e ("PCI: dwc: Add support for EP mode"), but it hasn't
> been utilized neither in the code submitted in the denoted so far nor in
> the platform driver evolving afterwards. Drop it then for good.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

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

Thanks,
Mani

> ---
>  drivers/pci/controller/dwc/pcie-designware-plat.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-plat.c b/drivers/pci/controller/dwc/pcie-designware-plat.c
> index fea785096261..99cf2ac5b0ba 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-plat.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-plat.c
> @@ -17,13 +17,11 @@
>  #include <linux/platform_device.h>
>  #include <linux/resource.h>
>  #include <linux/types.h>
> -#include <linux/regmap.h>
>  
>  #include "pcie-designware.h"
>  
>  struct dw_plat_pcie {
>  	struct dw_pcie			*pci;
> -	struct regmap			*regmap;
>  	enum dw_pcie_device_mode	mode;
>  };
>  
> -- 
> 2.35.1
> 

Re: [PATCH 12/12] PCI: dwc-plat: Drop dw_plat_pcie_of_match forward declaration

[Manivannan Sadhasivam]

On Thu, Mar 24, 2022 at 04:25:23AM +0300, Serge Semin wrote:
> The denoted forward declaration used to be required to get the OF-device
> ID structure by calling the of_match_device() method. The later method
> invocation has been replaced with the of_device_get_match_data() call in
> the commit 5c204204cf24 ("PCI: designware-plat: Prefer
> of_device_get_match_data()"). Thus the forward declaration of the
> OF-compatible device strings no longer needed. Drop it for good.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

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

Thanks,
Mani

> ---
>  drivers/pci/controller/dwc/pcie-designware-plat.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-plat.c b/drivers/pci/controller/dwc/pcie-designware-plat.c
> index 99cf2ac5b0ba..e606c5d5f06f 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-plat.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-plat.c
> @@ -29,8 +29,6 @@ struct dw_plat_pcie_of_data {
>  	enum dw_pcie_device_mode	mode;
>  };
>  
> -static const struct of_device_id dw_plat_pcie_of_match[];
> -
>  static const struct dw_pcie_host_ops dw_plat_pcie_host_ops = {
>  };
>  
> -- 
> 2.35.1
> 

Re: [PATCH 03/12] PCI: dwc: Add unroll iATU space support to the regions disable method

[kernel test robot]

Hi Serge,

I love your patch! Perhaps something to improve:

[auto build test WARNING on helgaas-pci/next]
[also build test WARNING on v5.17 next-20220328]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Serge-Semin/PCI-dwc-Various-fixes-and-cleanups/20220328-231112
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: arm64-buildonly-randconfig-r004-20220327 (https://download.01.org/0day-ci/archive/20220329/202203290533.uObikFCd-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 0f6d9501cf49ce02937099350d08f20c4af86f3d)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/fb5c4386b8af12124d1e7b48bd7f538c12d85100
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Serge-Semin/PCI-dwc-Various-fixes-and-cleanups/20220328-231112
        git checkout fb5c4386b8af12124d1e7b48bd7f538c12d85100
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/pci/controller/dwc/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/pci/controller/dwc/pcie-designware.c:508:14: warning: result of comparison of constant 2147483648 with expression of type 'int' is always false [-Wtautological-constant-out-of-range-compare]
                   if (region == PCIE_ATU_REGION_INBOUND) {
                       ~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~
   1 warning generated.


vim +/int +508 drivers/pci/controller/dwc/pcie-designware.c

   490	
   491	void dw_pcie_disable_atu(struct dw_pcie *pci, int index,
   492				 enum dw_pcie_region_type type)
   493	{
   494		int region;
   495	
   496		switch (type) {
   497		case DW_PCIE_REGION_INBOUND:
   498			region = PCIE_ATU_REGION_INBOUND;
   499			break;
   500		case DW_PCIE_REGION_OUTBOUND:
   501			region = PCIE_ATU_REGION_OUTBOUND;
   502			break;
   503		default:
   504			return;
   505		}
   506	
   507		if (pci->iatu_unroll_enabled) {
 > 508			if (region == PCIE_ATU_REGION_INBOUND) {
   509				dw_pcie_writel_ib_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
   510							 ~(u32)PCIE_ATU_ENABLE);
   511			} else {
   512				dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
   513							 ~(u32)PCIE_ATU_ENABLE);
   514			}
   515		} else {
   516			dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, region | index);
   517			dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, ~(u32)PCIE_ATU_ENABLE);
   518		}
   519	}
   520	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 03/12] PCI: dwc: Add unroll iATU space support to the regions disable method

[kernel test robot]

Hi Serge,

I love your patch! Perhaps something to improve:

[auto build test WARNING on helgaas-pci/next]
[also build test WARNING on v5.17 next-20220328]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Serge-Semin/PCI-dwc-Various-fixes-and-cleanups/20220328-231112
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: x86_64-randconfig-a013-20220328 (https://download.01.org/0day-ci/archive/20220329/202203290527.iS9iplnr-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 0f6d9501cf49ce02937099350d08f20c4af86f3d)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/fb5c4386b8af12124d1e7b48bd7f538c12d85100
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Serge-Semin/PCI-dwc-Various-fixes-and-cleanups/20220328-231112
        git checkout fb5c4386b8af12124d1e7b48bd7f538c12d85100
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/pci/controller/dwc/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/pci/controller/dwc/pcie-designware.c:508:14: warning: result of comparison of constant 2147483648 with expression of type 'int' is always false [-Wtautological-constant-out-of-range-compare]
                   if (region == PCIE_ATU_REGION_INBOUND) {
                       ~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~
   1 warning generated.


vim +/int +508 drivers/pci/controller/dwc/pcie-designware.c

   490	
   491	void dw_pcie_disable_atu(struct dw_pcie *pci, int index,
   492				 enum dw_pcie_region_type type)
   493	{
   494		int region;
   495	
   496		switch (type) {
   497		case DW_PCIE_REGION_INBOUND:
   498			region = PCIE_ATU_REGION_INBOUND;
   499			break;
   500		case DW_PCIE_REGION_OUTBOUND:
   501			region = PCIE_ATU_REGION_OUTBOUND;
   502			break;
   503		default:
   504			return;
   505		}
   506	
   507		if (pci->iatu_unroll_enabled) {
 > 508			if (region == PCIE_ATU_REGION_INBOUND) {
   509				dw_pcie_writel_ib_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
   510							 ~(u32)PCIE_ATU_ENABLE);
   511			} else {
   512				dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
   513							 ~(u32)PCIE_ATU_ENABLE);
   514			}
   515		} else {
   516			dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, region | index);
   517			dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, ~(u32)PCIE_ATU_ENABLE);
   518		}
   519	}
   520	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 02/12] PCI: dwc: Don't use generic IO-ops for DBI-space access

[Serge Semin]

On Thu, Mar 24, 2022 at 01:57:46PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Mar 24, 2022 at 04:25:13AM +0300, Serge Semin wrote:
> > Commit c2b0c098fbd1 ("PCI: dwc: Use generic config accessors") replaced
> > the locally defined DW PCIe host controller config-space accessors with
> > the generic methods pci_generic_config_read() and
> > pci_generic_config_write(). It was intended that the corresponding
> > bus-mapping callback returned a correct virtual address of the passed PCI
> > config-space register. The problem of the proposed solution was that it
> > didn't take into account the way the host config-space is accessed on the
> > DW PCIe. Depending on the DW PCIe IP-core synthesize parameters different
> > interfaces can be used to access the host and peripheral config/memory
> > spaces. The former one can be accessed via the DBI interface, while the
> > later ones is reached via the AHB/AXI application bus. In case if the DW
> > PCIe controller is configured to have a dedicated DBI interface, the way
> > it is mapped into the IO-memory turns to be platform-specific. For such
> > setups the DWC PCIe driver provides a set of the callbacks
> > dw_pcie_ops.{read_dbi,write_dbi} so the platforms glue-drivers would be
> > able to take into account the DBI bus IO peculiarities. Since
> > commit c2b0c098fbd1 ("PCI: dwc: Use generic config accessors") these
> > methods haven't been utilized during the generic host initialization
> > performed by the PCIe subsystem code.
> > 
> > I don't really know how come there have been no problems spotted for the
> > Histb/Exynos/Kirin PCIe controllers so far, but in our case with dword
> > aligned IO requirement the generic config-space accessors can't be
> > utilized for the host config-space. Thus in order to make sure the host
> > config-space is properly accessed via the DBI bus let's get back the
> > dw_pcie_rd_own_conf() and dw_pcie_wr_own_conf() methods. They are going to
> > be just wrappers around the already defined
> > dw_pcie_read_dbi()/dw_pcie_write_dbi() functions with proper arguments
> > conversion. These methods perform the platform-specific config-space IO if
> > the DBI accessors are specified, otherwise they call normal MMIO
> > operations.
> > 
> 

> Why can't you override the "pci_ops" in your host driver's "host_init"?

I have already defined the platform-specific DBI operations. These
operations are supposed to be used to access the DBI MMIO including
the own config space. Not doing so is very wrong and will cause
problems not only on my platforms, but on all platforms with specific
DBI implementation (such that requiring some more actions than
standard {read|write}{b,w,l} ops). So the commit c2b0c098fbd1 ("PCI:
dwc: Use generic config accessors") shouldn't have introduced the
generic IOs usage in this case and it needs to be fixed.

-Sergey

> 
> Thanks,
> Mani
> 
> > Fixes: c2b0c098fbd1 ("PCI: dwc: Use generic config accessors")
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > ---
> >  .../pci/controller/dwc/pcie-designware-host.c | 34 +++++++++++++++++--
> >  1 file changed, 32 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index a03619a30c20..f89e6552139b 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -528,10 +528,40 @@ void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, unsigned int devfn,
> >  }
> >  EXPORT_SYMBOL_GPL(dw_pcie_own_conf_map_bus);
> >  
> > +static int dw_pcie_rd_own_conf(struct pci_bus *bus, unsigned int devfn,
> > +			       int where, int size, u32 *val)
> > +{
> > +	struct pcie_port *pp = bus->sysdata;
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +
> > +	if (PCI_SLOT(devfn) > 0) {
> > +		*val = ~0U;
> > +		return PCIBIOS_DEVICE_NOT_FOUND;
> > +	}
> > +
> > +	*val = dw_pcie_read_dbi(pci, where, size);
> > +
> > +	return PCIBIOS_SUCCESSFUL;
> > +}
> > +
> > +static int dw_pcie_wr_own_conf(struct pci_bus *bus, unsigned int devfn,
> > +			       int where, int size, u32 val)
> > +{
> > +	struct pcie_port *pp = bus->sysdata;
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +
> > +	if (PCI_SLOT(devfn) > 0)
> > +		return PCIBIOS_DEVICE_NOT_FOUND;
> > +
> > +	dw_pcie_write_dbi(pci, where, size, val);
> > +
> > +	return PCIBIOS_SUCCESSFUL;
> > +}
> > +
> >  static struct pci_ops dw_pcie_ops = {
> >  	.map_bus = dw_pcie_own_conf_map_bus,
> > -	.read = pci_generic_config_read,
> > -	.write = pci_generic_config_write,
> > +	.read = dw_pcie_rd_own_conf,
> > +	.write = dw_pcie_wr_own_conf,
> >  };
> >  
> >  void dw_pcie_setup_rc(struct pcie_port *pp)
> > -- 
> > 2.35.1
> > 

Re: [PATCH 05/12] PCI: dwc: Set INCREASE_REGION_SIZE flag based on limit address

[Serge Semin]

On Thu, Mar 24, 2022 at 05:35:40PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Mar 24, 2022 at 04:25:16AM +0300, Serge Semin wrote:
> > It was wrong to use the region size parameter in order to determine
> > whether the INCREASE_REGION_SIZE flag needs to be set for the outbound
> > iATU entry because in general there are cases when combining a region base
> > address and size together produces the out of bounds upper range limit
> > while upper_32_bits(size) still returns zero. So having a region size
> > within the permitted values doesn't mean the region limit address will fit
> > to the corresponding CSR. Here is the way iATU calculates the in- and
> > outbound untranslated regions if the INCREASE_REGION_SIZE flag is cleared
> > [1]:
> > 
> >   Start address:                     End address:
> > 63              31              0   63              31              0
> > +---------------+---------------+   +---------------+---------------+
> > |               |         |  0s |   |               |         |  Fs |
> > +---------------+---------------+   +---------------+---------------+
> >    upper base   |   lower base       !upper! base   | limit address
> >      address          address           address
> > 
> > So the region start address is determined by the iATU lower and upper base
> > address registers, while the region upper boundary is calculated based on
> > the 32-bits limit address register and the upper part of the base address.
> > In accordance with that logic for instance the range
> > 0xf0000000 @ 0x20000000 does have the size smaller than 4GB, but the
> > actual limit address turns to be invalid forming the untranslated address
> > map as [0xf0000000; 0x0000FFFF], which isn't what the original range was.
> 

> I find the example confusing:
> 
> If the start address is 0x0-0xf0000000 and size is 0x20000000. Then the end
> address without the INCREASE_REGION_SIZE is going to be:
> 
> 0x0-0x1000FFFF and this is wrong.
> 
> If the INCREASE_REGION_SIZE is set, then the end address will be:
> 
> 0x1-0x1000FFFF and this is correct.

Not only confusing but indeed being wrong in the end address part.
Thanks for noticing that. I'll fix it in v2.

> 
> > In order to fix that we need to check whether the size being added to the
> > lower part of the base address causes the 4GB range overflow. If it does
> > then we need to set the INCREASE_REGION_SIZE flag thus activating the
> > extended limit address by means of an additional iATU CSR (upper limit
> > address register) [2]:
> > 
> >   Start address:                     End address:
> > 63              31              0   63      x       31              0
> > +---------------+---------------+   +---------------+---------------+
> > |               |         |  0s |   |       |       |         |  Fs |
> > +---------------+---------------+   +---------------+---------------+
> >   upper base   |   lower base         upper | upper | limit address
> >     address          address          base  | limit |
> >                                      address|address|
> > 
> > Otherwise there is enough room in the 32-bits wide limit address register,
> > and the flag can be left unset.
> > 
> > Note the case when the size-based flag setting approach is correct implies
> > requiring to have the size-aligned base addresses only. But that
> > restriction isn't relevant to the PCIe ranges accepted by the kernel.
> > There is also no point in implementing it either seeing the problem can be
> > easily fixed by checking the whole limit address instead of the region
> > size.
> > 
> > [1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port,
> >     v5.40a, March 2019, fig.3-36, p.175
> > [2] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port,
> >     v5.40a, March 2019, fig.3-37, p.176
> > 
> > Fixes: 5b4cf0f65324 ("PCI: dwc: Add upper limit address for outbound iATU")
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 

> With the example fixed,
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Ok. Thanks.

-Sergey

> 
> Thanks,
> Mani
> 
> > ---
> >  drivers/pci/controller/dwc/pcie-designware.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 7dc8c360a0d4..d737af058903 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -287,8 +287,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no,
> >  	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);
> > -	val = upper_32_bits(size - 1) ?
> > -		val | PCIE_ATU_INCREASE_REGION_SIZE : val;
> > +	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr))
> > +		val |= PCIE_ATU_INCREASE_REGION_SIZE;
> >  	if (pci->version == 0x490A)
> >  		val = dw_pcie_enable_ecrc(val);
> >  	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val);
> > @@ -315,6 +315,7 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
> >  					u64 pci_addr, u64 size)
> >  {
> >  	u32 retries, val;
> > +	u64 limit_addr;
> >  
> >  	if (pci->ops && pci->ops->cpu_addr_fixup)
> >  		cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
> > @@ -325,6 +326,8 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
> >  		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,
> > @@ -332,17 +335,18 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
> >  	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(cpu_addr + size - 1));
> > +			   lower_32_bits(limit_addr));
> >  	if (pci->version >= 0x460A)
> >  		dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_LIMIT,
> > -				   upper_32_bits(cpu_addr + size - 1));
> > +				   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));
> >  	val = type | PCIE_ATU_FUNC_NUM(func_no);
> > -	val = ((upper_32_bits(size - 1)) && (pci->version >= 0x460A)) ?
> > -		val | PCIE_ATU_INCREASE_REGION_SIZE : val;
> > +	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr) &&
> > +	    pci->version >= 0x460A)
> > +		val |= PCIE_ATU_INCREASE_REGION_SIZE;
> >  	if (pci->version == 0x490A)
> >  		val = dw_pcie_enable_ecrc(val);
> >  	dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, val);
> > -- 
> > 2.35.1
> >