[PATCH v3 0/4] PCI: cadence: Deprecate inbound/outbound specific bindings

[PATCH v3 0/4] PCI: cadence: Deprecate inbound/outbound specific bindings

[Kishon Vijay Abraham I]

This series is a result of comments given by Rob Herring @ [1].
Patch series changes the DT bindings and makes the corresponding driver
changes.

Changes from v2:
1) Changed the order of patches (no solid reason. Just save some
rebasing effort for me)
2) Added Acked-by Tom and Rob except for the dma-ranges patch
3) Re-worked dma-ranges patch for it do decode multiple dma-ranges
   and configure BAR0, BAR1 and NO_BAR instead of just NO_BAR [2].

Changes from v1:
1) Added Reviewed-by: Rob Herring <robh@kernel.org> for dt-binding patch
2) Fixed nitpick comments from Bjorn Helgaas
3) Added a patch to read 32-bit Vendor ID/Device ID property from DT

[1] -> http://lore.kernel.org/r/20200219202700.GA21908@bogus
[2] -> http://lore.kernel.org/r/eb1ffcb3-264f-5174-1f25-b5b2d3269840@ti.com

Kishon Vijay Abraham I (4):
  dt-bindings: PCI: cadence: Deprecate inbound/outbound specific
    bindings
  PCI: cadence: Remove "cdns,max-outbound-regions" DT property
  PCI: cadence: Fix to read 32-bit Vendor ID/Device ID property from DT
  PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits"
    property

 .../bindings/pci/cdns,cdns-pcie-ep.yaml       |   2 +-
 .../bindings/pci/cdns,cdns-pcie-host.yaml     |   3 +-
 .../devicetree/bindings/pci/cdns-pcie-ep.yaml |  25 +++
 .../bindings/pci/cdns-pcie-host.yaml          |  10 ++
 .../devicetree/bindings/pci/cdns-pcie.yaml    |   8 -
 .../controller/cadence/pcie-cadence-host.c    | 151 +++++++++++++++---
 drivers/pci/controller/cadence/pcie-cadence.h |  23 ++-
 7 files changed, 182 insertions(+), 40 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/cdns-pcie-ep.yaml

-- 
2.17.1

[PATCH v3 1/4] dt-bindings: PCI: cadence: Deprecate inbound/outbound specific bindings

[Kishon Vijay Abraham I]

Deprecate cdns,max-outbound-regions and cdns,no-bar-match-nbits for
host mode as both these could be derived from "ranges" and "dma-ranges"
property. "cdns,max-outbound-regions" property would still be required
for EP mode.

Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Tom Joseph <tjoseph@cadence.com>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 .../bindings/pci/cdns,cdns-pcie-ep.yaml       |  2 +-
 .../bindings/pci/cdns,cdns-pcie-host.yaml     |  3 +--
 .../devicetree/bindings/pci/cdns-pcie-ep.yaml | 25 +++++++++++++++++++
 .../bindings/pci/cdns-pcie-host.yaml          | 10 ++++++++
 .../devicetree/bindings/pci/cdns-pcie.yaml    |  8 ------
 5 files changed, 37 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/cdns-pcie-ep.yaml

diff --git a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.yaml
index 2996f8d4777c..50ce5d79d2c7 100644
--- a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.yaml
@@ -10,7 +10,7 @@ maintainers:
   - Tom Joseph <tjoseph@cadence.com>
 
 allOf:
-  - $ref: "cdns-pcie.yaml#"
+  - $ref: "cdns-pcie-ep.yaml#"
   - $ref: "pci-ep.yaml#"
 
 properties:
diff --git a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
index cabbe46ff578..84a8f095d031 100644
--- a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
+++ b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
@@ -45,8 +45,6 @@ examples:
             #size-cells = <2>;
             bus-range = <0x0 0xff>;
             linux,pci-domain = <0>;
-            cdns,max-outbound-regions = <16>;
-            cdns,no-bar-match-nbits = <32>;
             vendor-id = <0x17cd>;
             device-id = <0x0200>;
 
@@ -57,6 +55,7 @@ examples:
 
             ranges = <0x02000000 0x0 0x42000000  0x0 0x42000000  0x0 0x1000000>,
                      <0x01000000 0x0 0x43000000  0x0 0x43000000  0x0 0x0010000>;
+            dma-ranges = <0x02000000 0x0 0x0 0x0 0x0 0x1 0x00000000>;
 
             #interrupt-cells = <0x1>;
 
diff --git a/Documentation/devicetree/bindings/pci/cdns-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/cdns-pcie-ep.yaml
new file mode 100644
index 000000000000..6150a7a7bdbf
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/cdns-pcie-ep.yaml
@@ -0,0 +1,25 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/pci/cdns-pcie-ep.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Cadence PCIe Device
+
+maintainers:
+  - Tom Joseph <tjoseph@cadence.com>
+
+allOf:
+  - $ref: "cdns-pcie.yaml#"
+
+properties:
+  cdns,max-outbound-regions:
+    description: maximum number of outbound regions
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 32
+    default: 32
+
+required:
+  - cdns,max-outbound-regions
diff --git a/Documentation/devicetree/bindings/pci/cdns-pcie-host.yaml b/Documentation/devicetree/bindings/pci/cdns-pcie-host.yaml
index ab6e43b636ec..3d64f85aeb39 100644
--- a/Documentation/devicetree/bindings/pci/cdns-pcie-host.yaml
+++ b/Documentation/devicetree/bindings/pci/cdns-pcie-host.yaml
@@ -14,6 +14,15 @@ allOf:
   - $ref: "cdns-pcie.yaml#"
 
 properties:
+  cdns,max-outbound-regions:
+    description: maximum number of outbound regions
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 32
+    default: 32
+    deprecated: true
+
   cdns,no-bar-match-nbits:
     description:
       Set into the no BAR match register to configure the number of least
@@ -23,5 +32,6 @@ properties:
     minimum: 0
     maximum: 64
     default: 32
+    deprecated: true
 
   msi-parent: true
diff --git a/Documentation/devicetree/bindings/pci/cdns-pcie.yaml b/Documentation/devicetree/bindings/pci/cdns-pcie.yaml
index 6887ccc339cc..02553d5e6c51 100644
--- a/Documentation/devicetree/bindings/pci/cdns-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/cdns-pcie.yaml
@@ -10,14 +10,6 @@ maintainers:
   - Tom Joseph <tjoseph@cadence.com>
 
 properties:
-  cdns,max-outbound-regions:
-    description: maximum number of outbound regions
-    allOf:
-      - $ref: /schemas/types.yaml#/definitions/uint32
-    minimum: 1
-    maximum: 32
-    default: 32
-
   phys:
     description:
       One per lane if more than one in the list. If only one PHY listed it must
-- 
2.17.1

[PATCH v3 2/4] PCI: cadence: Remove "cdns,max-outbound-regions" DT property

[Kishon Vijay Abraham I]

"cdns,max-outbound-regions" device tree property provides the
maximum number of outbound regions supported by the Host PCIe
controller. However the outbound regions are configured based
on what is populated in the "ranges" DT property.

Avoid using two properties for configuring outbound regions and
use only "ranges" property instead.

Acked-by: Rob Herring <robh@kernel.org>
Acked-by: Tom Joseph <tjoseph@cadence.com>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/cadence/pcie-cadence-host.c | 6 ------
 drivers/pci/controller/cadence/pcie-cadence.h      | 2 --
 2 files changed, 8 deletions(-)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index 597b61e6cc4b..079692aa4da1 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -146,9 +146,6 @@ static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc)
 	for_each_of_pci_range(&parser, &range) {
 		bool is_io;
 
-		if (r >= rc->max_regions)
-			break;
-
 		if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_MEM)
 			is_io = false;
 		else if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_IO)
@@ -242,9 +239,6 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 	pcie = &rc->pcie;
 	pcie->is_rc = true;
 
-	rc->max_regions = 32;
-	of_property_read_u32(np, "cdns,max-outbound-regions", &rc->max_regions);
-
 	rc->no_bar_nbits = 32;
 	of_property_read_u32(np, "cdns,no-bar-match-nbits", &rc->no_bar_nbits);
 
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index ecb159dba6f6..cf1afd85c2f5 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -266,7 +266,6 @@ struct cdns_pcie {
  * @bus_range: first/last buses behind the PCIe host controller
  * @cfg_base: IO mapped window to access the PCI configuration space of a
  *            single function at a time
- * @max_regions: maximum number of regions supported by the hardware
  * @no_bar_nbits: Number of bits to keep for inbound (PCIe -> CPU) address
  *                translation (nbits sets into the "no BAR match" register)
  * @vendor_id: PCI vendor ID
@@ -277,7 +276,6 @@ struct cdns_pcie_rc {
 	struct resource		*cfg_res;
 	struct resource		*bus_range;
 	void __iomem		*cfg_base;
-	u32			max_regions;
 	u32			no_bar_nbits;
 	u16			vendor_id;
 	u16			device_id;
-- 
2.17.1

[PATCH v3 3/4] PCI: cadence: Fix to read 32-bit Vendor ID/Device ID property from DT

[Kishon Vijay Abraham I]

The PCI Bus Binding specification (IEEE Std 1275-1994 Revision 2.1 [1])
defines both Vendor ID and Device ID to be 32-bits. Fix
pcie-cadence-host.c driver to read 32-bit Vendor ID and Device ID
properties from device tree.

[1] -> https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf

Acked-by: Rob Herring <robh@kernel.org>
Acked-by: Tom Joseph <tjoseph@cadence.com>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/cadence/pcie-cadence-host.c | 4 ++--
 drivers/pci/controller/cadence/pcie-cadence.h      | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index 079692aa4da1..6ecebb79057a 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -243,10 +243,10 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 	of_property_read_u32(np, "cdns,no-bar-match-nbits", &rc->no_bar_nbits);
 
 	rc->vendor_id = 0xffff;
-	of_property_read_u16(np, "vendor-id", &rc->vendor_id);
+	of_property_read_u32(np, "vendor-id", &rc->vendor_id);
 
 	rc->device_id = 0xffff;
-	of_property_read_u16(np, "device-id", &rc->device_id);
+	of_property_read_u32(np, "device-id", &rc->device_id);
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "reg");
 	pcie->reg_base = devm_ioremap_resource(dev, res);
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index cf1afd85c2f5..f349f5828a58 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -277,8 +277,8 @@ struct cdns_pcie_rc {
 	struct resource		*bus_range;
 	void __iomem		*cfg_base;
 	u32			no_bar_nbits;
-	u16			vendor_id;
-	u16			device_id;
+	u32			vendor_id;
+	u32			device_id;
 };
 
 /**
-- 
2.17.1

[PATCH v3 4/4] PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits" property

[Kishon Vijay Abraham I]

Cadence PCIe core driver (host mode) uses "cdns,no-bar-match-nbits"
property to configure the number of bits passed through from PCIe
address to internal address in Inbound Address Translation register.
This only used the NO MATCH BAR.

However standard PCI dt-binding already defines "dma-ranges" to
describe the address ranges accessible by PCIe controller. Add support
in Cadence PCIe host driver to parse dma-ranges and configure the
inbound regions for BAR0, BAR1 and NO MATCH BAR. Cadence IP specifies
maximum size for BAR0 as 256GB, maximum size for BAR1 as 2 GB, so if
the dma-ranges specifies a size larger than the maximum allowed, the
driver will split and configure the BARs.

Legacy device tree binding compatibility is maintained by retaining
support for "cdns,no-bar-match-nbits".

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 .../controller/cadence/pcie-cadence-host.c    | 141 ++++++++++++++++--
 drivers/pci/controller/cadence/pcie-cadence.h |  17 ++-
 2 files changed, 141 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index 6ecebb79057a..2485ecd8434d 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -11,6 +11,12 @@
 
 #include "pcie-cadence.h"
 
+static u64 cdns_rp_bar_max_size[] = {
+	[RP_BAR0] = _ULL(128 * SZ_2G),
+	[RP_BAR1] = SZ_2G,
+	[RP_NO_BAR] = SZ_64T,
+};
+
 void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
 			       int where)
 {
@@ -106,6 +112,117 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
 	return 0;
 }
 
+static void cdns_pcie_host_bar_ib_config(struct cdns_pcie_rc *rc,
+					 enum cdns_pcie_rp_bar bar,
+					 u64 cpu_addr, u32 aperture)
+{
+	struct cdns_pcie *pcie = &rc->pcie;
+	u32 addr0, addr1;
+
+	addr0 = CDNS_PCIE_AT_IB_RP_BAR_ADDR0_NBITS(aperture) |
+		(lower_32_bits(cpu_addr) & GENMASK(31, 8));
+	addr1 = upper_32_bits(cpu_addr);
+	cdns_pcie_writel(pcie, CDNS_PCIE_AT_IB_RP_BAR_ADDR0(bar), addr0);
+	cdns_pcie_writel(pcie, CDNS_PCIE_AT_IB_RP_BAR_ADDR1(bar), addr1);
+}
+
+static int cdns_pcie_host_bar_config(struct cdns_pcie_rc *rc,
+				     struct resource_entry *entry,
+				     enum cdns_pcie_rp_bar *index)
+{
+	u64 cpu_addr, pci_addr, size, winsize;
+	struct cdns_pcie *pcie = &rc->pcie;
+	struct device *dev = pcie->dev;
+	enum cdns_pcie_rp_bar bar;
+	unsigned long flags;
+	u32 aperture;
+	u32 value;
+
+	cpu_addr = entry->res->start;
+	flags = entry->res->flags;
+	pci_addr = entry->res->start - entry->offset;
+	size = resource_size(entry->res);
+	bar = *index;
+
+	if (entry->offset) {
+		dev_err(dev, "Cannot map PCI addr: %llx to CPU addr: %llx\n",
+			pci_addr, cpu_addr);
+		return -EINVAL;
+	}
+
+	value = cdns_pcie_readl(pcie, CDNS_PCIE_LM_RC_BAR_CFG);
+	while (size > 0) {
+		if (bar > RP_NO_BAR) {
+			dev_err(dev, "Failed to map inbound regions!\n");
+			return -EINVAL;
+		}
+
+		winsize = size;
+		if (size > cdns_rp_bar_max_size[bar])
+			winsize = cdns_rp_bar_max_size[bar];
+
+		aperture = ilog2(winsize);
+
+		cdns_pcie_host_bar_ib_config(rc, bar, cpu_addr, aperture);
+
+		if (bar == RP_NO_BAR)
+			break;
+
+		if (winsize + cpu_addr >= SZ_4G) {
+			if (!(flags & IORESOURCE_PREFETCH))
+				value |= LM_RC_BAR_CFG_CTRL_MEM_64BITS(bar);
+			value |= LM_RC_BAR_CFG_CTRL_PREF_MEM_64BITS(bar);
+		} else {
+			if (!(flags & IORESOURCE_PREFETCH))
+				value |= LM_RC_BAR_CFG_CTRL_MEM_32BITS(bar);
+			value |= LM_RC_BAR_CFG_CTRL_PREF_MEM_32BITS(bar);
+		}
+
+		value |= LM_RC_BAR_CFG_APERTURE(bar, aperture);
+
+		size -= winsize;
+		cpu_addr += winsize;
+		bar++;
+	}
+	cdns_pcie_writel(pcie, CDNS_PCIE_LM_RC_BAR_CFG, value);
+	*index = bar;
+
+	return 0;
+}
+
+static int cdns_pcie_host_map_dma_ranges(struct cdns_pcie_rc *rc)
+{
+	enum cdns_pcie_rp_bar bar = RP_BAR0;
+	struct cdns_pcie *pcie = &rc->pcie;
+	struct device *dev = pcie->dev;
+	struct device_node *np = dev->of_node;
+	struct pci_host_bridge *bridge;
+	struct resource_entry *entry;
+	u32 no_bar_nbits = 32;
+	int err;
+
+	bridge = pci_host_bridge_from_priv(rc);
+	if (!bridge)
+		return -ENOMEM;
+
+	if (list_empty(&bridge->dma_ranges)) {
+		of_property_read_u32(np, "cdns,no-bar-match-nbits",
+				     &no_bar_nbits);
+		cdns_pcie_host_bar_ib_config(rc, RP_NO_BAR, 0x0, no_bar_nbits);
+		return 0;
+	}
+
+	resource_list_for_each_entry(entry, &bridge->dma_ranges) {
+		err = cdns_pcie_host_bar_config(rc, entry, &bar);
+		if (err) {
+			dev_err(dev, "Fail to configure IB using dma-ranges\n");
+			return err;
+		}
+	}
+
+	return 0;
+}
+
 static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc)
 {
 	struct cdns_pcie *pcie = &rc->pcie;
@@ -160,16 +277,9 @@ static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc)
 		r++;
 	}
 
-	/*
-	 * Set Root Port no BAR match Inbound Translation registers:
-	 * needed for MSI and DMA.
-	 * Root Port BAR0 and BAR1 are disabled, hence no need to set their
-	 * inbound translation registers.
-	 */
-	addr0 = CDNS_PCIE_AT_IB_RP_BAR_ADDR0_NBITS(rc->no_bar_nbits);
-	addr1 = 0;
-	cdns_pcie_writel(pcie, CDNS_PCIE_AT_IB_RP_BAR_ADDR0(RP_NO_BAR), addr0);
-	cdns_pcie_writel(pcie, CDNS_PCIE_AT_IB_RP_BAR_ADDR1(RP_NO_BAR), addr1);
+	err = cdns_pcie_host_map_dma_ranges(rc);
+	if (err)
+		return err;
 
 	return 0;
 }
@@ -179,10 +289,16 @@ static int cdns_pcie_host_init(struct device *dev,
 			       struct cdns_pcie_rc *rc)
 {
 	struct resource *bus_range = NULL;
+	struct pci_host_bridge *bridge;
 	int err;
 
+	bridge = pci_host_bridge_from_priv(rc);
+	if (!bridge)
+		return -ENOMEM;
+
 	/* Parse our PCI ranges and request their resources */
-	err = pci_parse_request_of_pci_ranges(dev, resources, NULL, &bus_range);
+	err = pci_parse_request_of_pci_ranges(dev, resources,
+					      &bridge->dma_ranges, &bus_range);
 	if (err)
 		return err;
 
@@ -239,9 +355,6 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 	pcie = &rc->pcie;
 	pcie->is_rc = true;
 
-	rc->no_bar_nbits = 32;
-	of_property_read_u32(np, "cdns,no-bar-match-nbits", &rc->no_bar_nbits);
-
 	rc->vendor_id = 0xffff;
 	of_property_read_u32(np, "vendor-id", &rc->vendor_id);
 
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index f349f5828a58..336237a7025c 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -92,6 +92,20 @@
 #define  CDNS_PCIE_LM_BAR_CFG_CTRL_MEM_64BITS		0x6
 #define  CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_64BITS	0x7
 
+#define LM_RC_BAR_CFG_CTRL_DISABLED(bar)		\
+		(CDNS_PCIE_LM_BAR_CFG_CTRL_DISABLED << (((bar) * 8) + 6))
+#define LM_RC_BAR_CFG_CTRL_IO_32BITS(bar)		\
+		(CDNS_PCIE_LM_BAR_CFG_CTRL_IO_32BITS << (((bar) * 8) + 6))
+#define LM_RC_BAR_CFG_CTRL_MEM_32BITS(bar)		\
+		(CDNS_PCIE_LM_BAR_CFG_CTRL_MEM_32BITS << (((bar) * 8) + 6))
+#define LM_RC_BAR_CFG_CTRL_PREF_MEM_32BITS(bar)	\
+	(CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_32BITS << (((bar) * 8) + 6))
+#define LM_RC_BAR_CFG_CTRL_MEM_64BITS(bar)		\
+		(CDNS_PCIE_LM_BAR_CFG_CTRL_MEM_64BITS << (((bar) * 8) + 6))
+#define LM_RC_BAR_CFG_CTRL_PREF_MEM_64BITS(bar)	\
+	(CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_64BITS << (((bar) * 8) + 6))
+#define LM_RC_BAR_CFG_APERTURE(bar, aperture)		\
+					(((aperture) - 2) << ((bar) * 8))
 
 /*
  * Endpoint Function Registers (PCI configuration space for endpoint functions)
@@ -266,8 +280,6 @@ struct cdns_pcie {
  * @bus_range: first/last buses behind the PCIe host controller
  * @cfg_base: IO mapped window to access the PCI configuration space of a
  *            single function at a time
- * @no_bar_nbits: Number of bits to keep for inbound (PCIe -> CPU) address
- *                translation (nbits sets into the "no BAR match" register)
  * @vendor_id: PCI vendor ID
  * @device_id: PCI device ID
  */
@@ -276,7 +288,6 @@ struct cdns_pcie_rc {
 	struct resource		*cfg_res;
 	struct resource		*bus_range;
 	void __iomem		*cfg_base;
-	u32			no_bar_nbits;
 	u32			vendor_id;
 	u32			device_id;
 };
-- 
2.17.1

Re: [PATCH v3 4/4] PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits" property

[Kishon Vijay Abraham I]

Hi RobH, Robin,

On 5/8/2020 6:36 PM, Kishon Vijay Abraham I wrote:
> Cadence PCIe core driver (host mode) uses "cdns,no-bar-match-nbits"
> property to configure the number of bits passed through from PCIe
> address to internal address in Inbound Address Translation register.
> This only used the NO MATCH BAR.
> 
> However standard PCI dt-binding already defines "dma-ranges" to
> describe the address ranges accessible by PCIe controller. Add support
> in Cadence PCIe host driver to parse dma-ranges and configure the
> inbound regions for BAR0, BAR1 and NO MATCH BAR. Cadence IP specifies
> maximum size for BAR0 as 256GB, maximum size for BAR1 as 2 GB, so if
> the dma-ranges specifies a size larger than the maximum allowed, the
> driver will split and configure the BARs.
> 
> Legacy device tree binding compatibility is maintained by retaining
> support for "cdns,no-bar-match-nbits".

Do you have any comments for this patch?

Tom,

where you able to test this in Cadence platform?

Thanks
Kishon

> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  .../controller/cadence/pcie-cadence-host.c    | 141 ++++++++++++++++--
>  drivers/pci/controller/cadence/pcie-cadence.h |  17 ++-
>  2 files changed, 141 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index 6ecebb79057a..2485ecd8434d 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -11,6 +11,12 @@
>  
>  #include "pcie-cadence.h"
>  
> +static u64 cdns_rp_bar_max_size[] = {
> +	[RP_BAR0] = _ULL(128 * SZ_2G),
> +	[RP_BAR1] = SZ_2G,
> +	[RP_NO_BAR] = SZ_64T,
> +};
> +
>  void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
>  			       int where)
>  {
> @@ -106,6 +112,117 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
>  	return 0;
>  }
>  
> +static void cdns_pcie_host_bar_ib_config(struct cdns_pcie_rc *rc,
> +					 enum cdns_pcie_rp_bar bar,
> +					 u64 cpu_addr, u32 aperture)
> +{
> +	struct cdns_pcie *pcie = &rc->pcie;
> +	u32 addr0, addr1;
> +
> +	addr0 = CDNS_PCIE_AT_IB_RP_BAR_ADDR0_NBITS(aperture) |
> +		(lower_32_bits(cpu_addr) & GENMASK(31, 8));
> +	addr1 = upper_32_bits(cpu_addr);
> +	cdns_pcie_writel(pcie, CDNS_PCIE_AT_IB_RP_BAR_ADDR0(bar), addr0);
> +	cdns_pcie_writel(pcie, CDNS_PCIE_AT_IB_RP_BAR_ADDR1(bar), addr1);
> +}
> +
> +static int cdns_pcie_host_bar_config(struct cdns_pcie_rc *rc,
> +				     struct resource_entry *entry,
> +				     enum cdns_pcie_rp_bar *index)
> +{
> +	u64 cpu_addr, pci_addr, size, winsize;
> +	struct cdns_pcie *pcie = &rc->pcie;
> +	struct device *dev = pcie->dev;
> +	enum cdns_pcie_rp_bar bar;
> +	unsigned long flags;
> +	u32 aperture;
> +	u32 value;
> +
> +	cpu_addr = entry->res->start;
> +	flags = entry->res->flags;
> +	pci_addr = entry->res->start - entry->offset;
> +	size = resource_size(entry->res);
> +	bar = *index;
> +
> +	if (entry->offset) {
> +		dev_err(dev, "Cannot map PCI addr: %llx to CPU addr: %llx\n",
> +			pci_addr, cpu_addr);
> +		return -EINVAL;
> +	}
> +
> +	value = cdns_pcie_readl(pcie, CDNS_PCIE_LM_RC_BAR_CFG);
> +	while (size > 0) {
> +		if (bar > RP_NO_BAR) {
> +			dev_err(dev, "Failed to map inbound regions!\n");
> +			return -EINVAL;
> +		}
> +
> +		winsize = size;
> +		if (size > cdns_rp_bar_max_size[bar])
> +			winsize = cdns_rp_bar_max_size[bar];
> +
> +		aperture = ilog2(winsize);
> +
> +		cdns_pcie_host_bar_ib_config(rc, bar, cpu_addr, aperture);
> +
> +		if (bar == RP_NO_BAR)
> +			break;
> +
> +		if (winsize + cpu_addr >= SZ_4G) {
> +			if (!(flags & IORESOURCE_PREFETCH))
> +				value |= LM_RC_BAR_CFG_CTRL_MEM_64BITS(bar);
> +			value |= LM_RC_BAR_CFG_CTRL_PREF_MEM_64BITS(bar);
> +		} else {
> +			if (!(flags & IORESOURCE_PREFETCH))
> +				value |= LM_RC_BAR_CFG_CTRL_MEM_32BITS(bar);
> +			value |= LM_RC_BAR_CFG_CTRL_PREF_MEM_32BITS(bar);
> +		}
> +
> +		value |= LM_RC_BAR_CFG_APERTURE(bar, aperture);
> +
> +		size -= winsize;
> +		cpu_addr += winsize;
> +		bar++;
> +	}
> +	cdns_pcie_writel(pcie, CDNS_PCIE_LM_RC_BAR_CFG, value);
> +	*index = bar;
> +
> +	return 0;
> +}
> +
> +static int cdns_pcie_host_map_dma_ranges(struct cdns_pcie_rc *rc)
> +{
> +	enum cdns_pcie_rp_bar bar = RP_BAR0;
> +	struct cdns_pcie *pcie = &rc->pcie;
> +	struct device *dev = pcie->dev;
> +	struct device_node *np = dev->of_node;
> +	struct pci_host_bridge *bridge;
> +	struct resource_entry *entry;
> +	u32 no_bar_nbits = 32;
> +	int err;
> +
> +	bridge = pci_host_bridge_from_priv(rc);
> +	if (!bridge)
> +		return -ENOMEM;
> +
> +	if (list_empty(&bridge->dma_ranges)) {
> +		of_property_read_u32(np, "cdns,no-bar-match-nbits",
> +				     &no_bar_nbits);
> +		cdns_pcie_host_bar_ib_config(rc, RP_NO_BAR, 0x0, no_bar_nbits);
> +		return 0;
> +	}
> +
> +	resource_list_for_each_entry(entry, &bridge->dma_ranges) {
> +		err = cdns_pcie_host_bar_config(rc, entry, &bar);
> +		if (err) {
> +			dev_err(dev, "Fail to configure IB using dma-ranges\n");
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc)
>  {
>  	struct cdns_pcie *pcie = &rc->pcie;
> @@ -160,16 +277,9 @@ static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc)
>  		r++;
>  	}
>  
> -	/*
> -	 * Set Root Port no BAR match Inbound Translation registers:
> -	 * needed for MSI and DMA.
> -	 * Root Port BAR0 and BAR1 are disabled, hence no need to set their
> -	 * inbound translation registers.
> -	 */
> -	addr0 = CDNS_PCIE_AT_IB_RP_BAR_ADDR0_NBITS(rc->no_bar_nbits);
> -	addr1 = 0;
> -	cdns_pcie_writel(pcie, CDNS_PCIE_AT_IB_RP_BAR_ADDR0(RP_NO_BAR), addr0);
> -	cdns_pcie_writel(pcie, CDNS_PCIE_AT_IB_RP_BAR_ADDR1(RP_NO_BAR), addr1);
> +	err = cdns_pcie_host_map_dma_ranges(rc);
> +	if (err)
> +		return err;
>  
>  	return 0;
>  }
> @@ -179,10 +289,16 @@ static int cdns_pcie_host_init(struct device *dev,
>  			       struct cdns_pcie_rc *rc)
>  {
>  	struct resource *bus_range = NULL;
> +	struct pci_host_bridge *bridge;
>  	int err;
>  
> +	bridge = pci_host_bridge_from_priv(rc);
> +	if (!bridge)
> +		return -ENOMEM;
> +
>  	/* Parse our PCI ranges and request their resources */
> -	err = pci_parse_request_of_pci_ranges(dev, resources, NULL, &bus_range);
> +	err = pci_parse_request_of_pci_ranges(dev, resources,
> +					      &bridge->dma_ranges, &bus_range);
>  	if (err)
>  		return err;
>  
> @@ -239,9 +355,6 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>  	pcie = &rc->pcie;
>  	pcie->is_rc = true;
>  
> -	rc->no_bar_nbits = 32;
> -	of_property_read_u32(np, "cdns,no-bar-match-nbits", &rc->no_bar_nbits);
> -
>  	rc->vendor_id = 0xffff;
>  	of_property_read_u32(np, "vendor-id", &rc->vendor_id);
>  
> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
> index f349f5828a58..336237a7025c 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.h
> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> @@ -92,6 +92,20 @@
>  #define  CDNS_PCIE_LM_BAR_CFG_CTRL_MEM_64BITS		0x6
>  #define  CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_64BITS	0x7
>  
> +#define LM_RC_BAR_CFG_CTRL_DISABLED(bar)		\
> +		(CDNS_PCIE_LM_BAR_CFG_CTRL_DISABLED << (((bar) * 8) + 6))
> +#define LM_RC_BAR_CFG_CTRL_IO_32BITS(bar)		\
> +		(CDNS_PCIE_LM_BAR_CFG_CTRL_IO_32BITS << (((bar) * 8) + 6))
> +#define LM_RC_BAR_CFG_CTRL_MEM_32BITS(bar)		\
> +		(CDNS_PCIE_LM_BAR_CFG_CTRL_MEM_32BITS << (((bar) * 8) + 6))
> +#define LM_RC_BAR_CFG_CTRL_PREF_MEM_32BITS(bar)	\
> +	(CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_32BITS << (((bar) * 8) + 6))
> +#define LM_RC_BAR_CFG_CTRL_MEM_64BITS(bar)		\
> +		(CDNS_PCIE_LM_BAR_CFG_CTRL_MEM_64BITS << (((bar) * 8) + 6))
> +#define LM_RC_BAR_CFG_CTRL_PREF_MEM_64BITS(bar)	\
> +	(CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_64BITS << (((bar) * 8) + 6))
> +#define LM_RC_BAR_CFG_APERTURE(bar, aperture)		\
> +					(((aperture) - 2) << ((bar) * 8))
>  
>  /*
>   * Endpoint Function Registers (PCI configuration space for endpoint functions)
> @@ -266,8 +280,6 @@ struct cdns_pcie {
>   * @bus_range: first/last buses behind the PCIe host controller
>   * @cfg_base: IO mapped window to access the PCI configuration space of a
>   *            single function at a time
> - * @no_bar_nbits: Number of bits to keep for inbound (PCIe -> CPU) address
> - *                translation (nbits sets into the "no BAR match" register)
>   * @vendor_id: PCI vendor ID
>   * @device_id: PCI device ID
>   */
> @@ -276,7 +288,6 @@ struct cdns_pcie_rc {
>  	struct resource		*cfg_res;
>  	struct resource		*bus_range;
>  	void __iomem		*cfg_base;
> -	u32			no_bar_nbits;
>  	u32			vendor_id;
>  	u32			device_id;
>  };
> 

Re: [PATCH v3 4/4] PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits" property

[Rob Herring]

On Fri, May 8, 2020 at 7:07 AM Kishon Vijay Abraham I <kishon@ti.com> wrote:
>
> Cadence PCIe core driver (host mode) uses "cdns,no-bar-match-nbits"
> property to configure the number of bits passed through from PCIe
> address to internal address in Inbound Address Translation register.
> This only used the NO MATCH BAR.
>
> However standard PCI dt-binding already defines "dma-ranges" to
> describe the address ranges accessible by PCIe controller. Add support
> in Cadence PCIe host driver to parse dma-ranges and configure the
> inbound regions for BAR0, BAR1 and NO MATCH BAR. Cadence IP specifies
> maximum size for BAR0 as 256GB, maximum size for BAR1 as 2 GB, so if
> the dma-ranges specifies a size larger than the maximum allowed, the
> driver will split and configure the BARs.

Would be useful to know what your dma-ranges contains now.


> Legacy device tree binding compatibility is maintained by retaining
> support for "cdns,no-bar-match-nbits".
>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  .../controller/cadence/pcie-cadence-host.c    | 141 ++++++++++++++++--
>  drivers/pci/controller/cadence/pcie-cadence.h |  17 ++-
>  2 files changed, 141 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index 6ecebb79057a..2485ecd8434d 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -11,6 +11,12 @@
>
>  #include "pcie-cadence.h"
>
> +static u64 cdns_rp_bar_max_size[] = {
> +       [RP_BAR0] = _ULL(128 * SZ_2G),
> +       [RP_BAR1] = SZ_2G,
> +       [RP_NO_BAR] = SZ_64T,
> +};
> +
>  void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
>                                int where)
>  {
> @@ -106,6 +112,117 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
>         return 0;
>  }
>
> +static void cdns_pcie_host_bar_ib_config(struct cdns_pcie_rc *rc,
> +                                        enum cdns_pcie_rp_bar bar,
> +                                        u64 cpu_addr, u32 aperture)
> +{
> +       struct cdns_pcie *pcie = &rc->pcie;
> +       u32 addr0, addr1;
> +
> +       addr0 = CDNS_PCIE_AT_IB_RP_BAR_ADDR0_NBITS(aperture) |
> +               (lower_32_bits(cpu_addr) & GENMASK(31, 8));
> +       addr1 = upper_32_bits(cpu_addr);
> +       cdns_pcie_writel(pcie, CDNS_PCIE_AT_IB_RP_BAR_ADDR0(bar), addr0);
> +       cdns_pcie_writel(pcie, CDNS_PCIE_AT_IB_RP_BAR_ADDR1(bar), addr1);
> +}
> +
> +static int cdns_pcie_host_bar_config(struct cdns_pcie_rc *rc,
> +                                    struct resource_entry *entry,
> +                                    enum cdns_pcie_rp_bar *index)
> +{
> +       u64 cpu_addr, pci_addr, size, winsize;
> +       struct cdns_pcie *pcie = &rc->pcie;
> +       struct device *dev = pcie->dev;
> +       enum cdns_pcie_rp_bar bar;
> +       unsigned long flags;
> +       u32 aperture;
> +       u32 value;
> +
> +       cpu_addr = entry->res->start;
> +       flags = entry->res->flags;
> +       pci_addr = entry->res->start - entry->offset;
> +       size = resource_size(entry->res);
> +       bar = *index;
> +
> +       if (entry->offset) {
> +               dev_err(dev, "Cannot map PCI addr: %llx to CPU addr: %llx\n",
> +                       pci_addr, cpu_addr);

Would be a bit more clear to say PCI addr must equal CPU addr.

> +               return -EINVAL;
> +       }
> +
> +       value = cdns_pcie_readl(pcie, CDNS_PCIE_LM_RC_BAR_CFG);
> +       while (size > 0) {
> +               if (bar > RP_NO_BAR) {
> +                       dev_err(dev, "Failed to map inbound regions!\n");
> +                       return -EINVAL;
> +               }
> +
> +               winsize = size;
> +               if (size > cdns_rp_bar_max_size[bar])
> +                       winsize = cdns_rp_bar_max_size[bar];
> +
> +               aperture = ilog2(winsize);
> +
> +               cdns_pcie_host_bar_ib_config(rc, bar, cpu_addr, aperture);
> +
> +               if (bar == RP_NO_BAR)
> +                       break;
> +
> +               if (winsize + cpu_addr >= SZ_4G) {
> +                       if (!(flags & IORESOURCE_PREFETCH))
> +                               value |= LM_RC_BAR_CFG_CTRL_MEM_64BITS(bar);
> +                       value |= LM_RC_BAR_CFG_CTRL_PREF_MEM_64BITS(bar);
> +               } else {
> +                       if (!(flags & IORESOURCE_PREFETCH))
> +                               value |= LM_RC_BAR_CFG_CTRL_MEM_32BITS(bar);
> +                       value |= LM_RC_BAR_CFG_CTRL_PREF_MEM_32BITS(bar);
> +               }
> +
> +               value |= LM_RC_BAR_CFG_APERTURE(bar, aperture);
> +
> +               size -= winsize;
> +               cpu_addr += winsize;
> +               bar++;
> +       }
> +       cdns_pcie_writel(pcie, CDNS_PCIE_LM_RC_BAR_CFG, value);
> +       *index = bar;
> +
> +       return 0;
> +}
> +
> +static int cdns_pcie_host_map_dma_ranges(struct cdns_pcie_rc *rc)
> +{
> +       enum cdns_pcie_rp_bar bar = RP_BAR0;
> +       struct cdns_pcie *pcie = &rc->pcie;
> +       struct device *dev = pcie->dev;
> +       struct device_node *np = dev->of_node;
> +       struct pci_host_bridge *bridge;
> +       struct resource_entry *entry;
> +       u32 no_bar_nbits = 32;
> +       int err;
> +
> +       bridge = pci_host_bridge_from_priv(rc);
> +       if (!bridge)
> +               return -ENOMEM;
> +
> +       if (list_empty(&bridge->dma_ranges)) {
> +               of_property_read_u32(np, "cdns,no-bar-match-nbits",
> +                                    &no_bar_nbits);
> +               cdns_pcie_host_bar_ib_config(rc, RP_NO_BAR, 0x0, no_bar_nbits);
> +               return 0;
> +       }
> +
> +       resource_list_for_each_entry(entry, &bridge->dma_ranges) {
> +               err = cdns_pcie_host_bar_config(rc, entry, &bar);

Seems like this should have some better logic to pick which BAR to
use. Something like find the biggest region and then find the smallest
BAR that it fits in. Then get the next biggest...

> +               if (err) {
> +                       dev_err(dev, "Fail to configure IB using dma-ranges\n");
> +                       return err;
> +               }
> +       }
> +
> +       return 0;
> +}
> +

Re: [PATCH v3 4/4] PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits" property

[Kishon Vijay Abraham I]

Hi Rob,

On 5/19/2020 10:41 PM, Rob Herring wrote:
> On Fri, May 8, 2020 at 7:07 AM Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>
>> Cadence PCIe core driver (host mode) uses "cdns,no-bar-match-nbits"
>> property to configure the number of bits passed through from PCIe
>> address to internal address in Inbound Address Translation register.
>> This only used the NO MATCH BAR.
>>
>> However standard PCI dt-binding already defines "dma-ranges" to
>> describe the address ranges accessible by PCIe controller. Add support
>> in Cadence PCIe host driver to parse dma-ranges and configure the
>> inbound regions for BAR0, BAR1 and NO MATCH BAR. Cadence IP specifies
>> maximum size for BAR0 as 256GB, maximum size for BAR1 as 2 GB, so if
>> the dma-ranges specifies a size larger than the maximum allowed, the
>> driver will split and configure the BARs.
> 
> Would be useful to know what your dma-ranges contains now.
> 
> 
>> Legacy device tree binding compatibility is maintained by retaining
>> support for "cdns,no-bar-match-nbits".
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  .../controller/cadence/pcie-cadence-host.c    | 141 ++++++++++++++++--
>>  drivers/pci/controller/cadence/pcie-cadence.h |  17 ++-
>>  2 files changed, 141 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> index 6ecebb79057a..2485ecd8434d 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> @@ -11,6 +11,12 @@
>>
>>  #include "pcie-cadence.h"
>>
>> +static u64 cdns_rp_bar_max_size[] = {
>> +       [RP_BAR0] = _ULL(128 * SZ_2G),
>> +       [RP_BAR1] = SZ_2G,
>> +       [RP_NO_BAR] = SZ_64T,
>> +};
>> +
>>  void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
>>                                int where)
>>  {
>> @@ -106,6 +112,117 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
>>         return 0;
>>  }
>>
>> +static void cdns_pcie_host_bar_ib_config(struct cdns_pcie_rc *rc,
>> +                                        enum cdns_pcie_rp_bar bar,
>> +                                        u64 cpu_addr, u32 aperture)
>> +{
>> +       struct cdns_pcie *pcie = &rc->pcie;
>> +       u32 addr0, addr1;
>> +
>> +       addr0 = CDNS_PCIE_AT_IB_RP_BAR_ADDR0_NBITS(aperture) |
>> +               (lower_32_bits(cpu_addr) & GENMASK(31, 8));
>> +       addr1 = upper_32_bits(cpu_addr);
>> +       cdns_pcie_writel(pcie, CDNS_PCIE_AT_IB_RP_BAR_ADDR0(bar), addr0);
>> +       cdns_pcie_writel(pcie, CDNS_PCIE_AT_IB_RP_BAR_ADDR1(bar), addr1);
>> +}
>> +
>> +static int cdns_pcie_host_bar_config(struct cdns_pcie_rc *rc,
>> +                                    struct resource_entry *entry,
>> +                                    enum cdns_pcie_rp_bar *index)
>> +{
>> +       u64 cpu_addr, pci_addr, size, winsize;
>> +       struct cdns_pcie *pcie = &rc->pcie;
>> +       struct device *dev = pcie->dev;
>> +       enum cdns_pcie_rp_bar bar;
>> +       unsigned long flags;
>> +       u32 aperture;
>> +       u32 value;
>> +
>> +       cpu_addr = entry->res->start;
>> +       flags = entry->res->flags;
>> +       pci_addr = entry->res->start - entry->offset;
>> +       size = resource_size(entry->res);
>> +       bar = *index;
>> +
>> +       if (entry->offset) {
>> +               dev_err(dev, "Cannot map PCI addr: %llx to CPU addr: %llx\n",
>> +                       pci_addr, cpu_addr);
> 
> Would be a bit more clear to say PCI addr must equal CPU addr.
> 
>> +               return -EINVAL;
>> +       }
>> +
>> +       value = cdns_pcie_readl(pcie, CDNS_PCIE_LM_RC_BAR_CFG);
>> +       while (size > 0) {
>> +               if (bar > RP_NO_BAR) {
>> +                       dev_err(dev, "Failed to map inbound regions!\n");
>> +                       return -EINVAL;
>> +               }
>> +
>> +               winsize = size;
>> +               if (size > cdns_rp_bar_max_size[bar])
>> +                       winsize = cdns_rp_bar_max_size[bar];
>> +
>> +               aperture = ilog2(winsize);
>> +
>> +               cdns_pcie_host_bar_ib_config(rc, bar, cpu_addr, aperture);
>> +
>> +               if (bar == RP_NO_BAR)
>> +                       break;
>> +
>> +               if (winsize + cpu_addr >= SZ_4G) {
>> +                       if (!(flags & IORESOURCE_PREFETCH))
>> +                               value |= LM_RC_BAR_CFG_CTRL_MEM_64BITS(bar);
>> +                       value |= LM_RC_BAR_CFG_CTRL_PREF_MEM_64BITS(bar);
>> +               } else {
>> +                       if (!(flags & IORESOURCE_PREFETCH))
>> +                               value |= LM_RC_BAR_CFG_CTRL_MEM_32BITS(bar);
>> +                       value |= LM_RC_BAR_CFG_CTRL_PREF_MEM_32BITS(bar);
>> +               }
>> +
>> +               value |= LM_RC_BAR_CFG_APERTURE(bar, aperture);
>> +
>> +               size -= winsize;
>> +               cpu_addr += winsize;
>> +               bar++;
>> +       }
>> +       cdns_pcie_writel(pcie, CDNS_PCIE_LM_RC_BAR_CFG, value);
>> +       *index = bar;
>> +
>> +       return 0;
>> +}
>> +
>> +static int cdns_pcie_host_map_dma_ranges(struct cdns_pcie_rc *rc)
>> +{
>> +       enum cdns_pcie_rp_bar bar = RP_BAR0;
>> +       struct cdns_pcie *pcie = &rc->pcie;
>> +       struct device *dev = pcie->dev;
>> +       struct device_node *np = dev->of_node;
>> +       struct pci_host_bridge *bridge;
>> +       struct resource_entry *entry;
>> +       u32 no_bar_nbits = 32;
>> +       int err;
>> +
>> +       bridge = pci_host_bridge_from_priv(rc);
>> +       if (!bridge)
>> +               return -ENOMEM;
>> +
>> +       if (list_empty(&bridge->dma_ranges)) {
>> +               of_property_read_u32(np, "cdns,no-bar-match-nbits",
>> +                                    &no_bar_nbits);
>> +               cdns_pcie_host_bar_ib_config(rc, RP_NO_BAR, 0x0, no_bar_nbits);
>> +               return 0;
>> +       }
>> +
>> +       resource_list_for_each_entry(entry, &bridge->dma_ranges) {
>> +               err = cdns_pcie_host_bar_config(rc, entry, &bar);
> 
> Seems like this should have some better logic to pick which BAR to
> use. Something like find the biggest region and then find the smallest
> BAR that it fits in. Then get the next biggest...

Okay, I'll change this something like for each region, find the smallest BAR
that it fits in and if there is no BAR big enough to hold the region, split the
region to see if can be fitted using multiple BARs. I don't see the purpose of
finding the biggest region first since at all times we'll only use the smallest
BAR to fit.

Thanks
Kishon

Re: [PATCH v3 4/4] PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits" property

[Kishon Vijay Abraham I]

Rob,

On 5/21/2020 9:00 AM, Kishon Vijay Abraham I wrote:
> Hi Rob,
> 
> On 5/19/2020 10:41 PM, Rob Herring wrote:
>> On Fri, May 8, 2020 at 7:07 AM Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>>
>>> Cadence PCIe core driver (host mode) uses "cdns,no-bar-match-nbits"
>>> property to configure the number of bits passed through from PCIe
>>> address to internal address in Inbound Address Translation register.
>>> This only used the NO MATCH BAR.
>>>
>>> However standard PCI dt-binding already defines "dma-ranges" to
>>> describe the address ranges accessible by PCIe controller. Add support
>>> in Cadence PCIe host driver to parse dma-ranges and configure the
>>> inbound regions for BAR0, BAR1 and NO MATCH BAR. Cadence IP specifies
>>> maximum size for BAR0 as 256GB, maximum size for BAR1 as 2 GB, so if
>>> the dma-ranges specifies a size larger than the maximum allowed, the
>>> driver will split and configure the BARs.
>>
>> Would be useful to know what your dma-ranges contains now.
>>
>>
>>> Legacy device tree binding compatibility is maintained by retaining
>>> support for "cdns,no-bar-match-nbits".
>>>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> ---
>>>  .../controller/cadence/pcie-cadence-host.c    | 141 ++++++++++++++++--
>>>  drivers/pci/controller/cadence/pcie-cadence.h |  17 ++-
>>>  2 files changed, 141 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>> index 6ecebb79057a..2485ecd8434d 100644
>>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>> @@ -11,6 +11,12 @@
>>>
>>>  #include "pcie-cadence.h"
>>>
>>> +static u64 cdns_rp_bar_max_size[] = {
>>> +       [RP_BAR0] = _ULL(128 * SZ_2G),
>>> +       [RP_BAR1] = SZ_2G,
>>> +       [RP_NO_BAR] = SZ_64T,
>>> +};
>>> +
>>>  void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
>>>                                int where)
>>>  {
>>> @@ -106,6 +112,117 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
>>>         return 0;
>>>  }
>>>
>>> +static void cdns_pcie_host_bar_ib_config(struct cdns_pcie_rc *rc,
>>> +                                        enum cdns_pcie_rp_bar bar,
>>> +                                        u64 cpu_addr, u32 aperture)
>>> +{
>>> +       struct cdns_pcie *pcie = &rc->pcie;
>>> +       u32 addr0, addr1;
>>> +
>>> +       addr0 = CDNS_PCIE_AT_IB_RP_BAR_ADDR0_NBITS(aperture) |
>>> +               (lower_32_bits(cpu_addr) & GENMASK(31, 8));
>>> +       addr1 = upper_32_bits(cpu_addr);
>>> +       cdns_pcie_writel(pcie, CDNS_PCIE_AT_IB_RP_BAR_ADDR0(bar), addr0);
>>> +       cdns_pcie_writel(pcie, CDNS_PCIE_AT_IB_RP_BAR_ADDR1(bar), addr1);
>>> +}
>>> +
>>> +static int cdns_pcie_host_bar_config(struct cdns_pcie_rc *rc,
>>> +                                    struct resource_entry *entry,
>>> +                                    enum cdns_pcie_rp_bar *index)
>>> +{
>>> +       u64 cpu_addr, pci_addr, size, winsize;
>>> +       struct cdns_pcie *pcie = &rc->pcie;
>>> +       struct device *dev = pcie->dev;
>>> +       enum cdns_pcie_rp_bar bar;
>>> +       unsigned long flags;
>>> +       u32 aperture;
>>> +       u32 value;
>>> +
>>> +       cpu_addr = entry->res->start;
>>> +       flags = entry->res->flags;
>>> +       pci_addr = entry->res->start - entry->offset;
>>> +       size = resource_size(entry->res);
>>> +       bar = *index;
>>> +
>>> +       if (entry->offset) {
>>> +               dev_err(dev, "Cannot map PCI addr: %llx to CPU addr: %llx\n",
>>> +                       pci_addr, cpu_addr);
>>
>> Would be a bit more clear to say PCI addr must equal CPU addr.
>>
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       value = cdns_pcie_readl(pcie, CDNS_PCIE_LM_RC_BAR_CFG);
>>> +       while (size > 0) {
>>> +               if (bar > RP_NO_BAR) {
>>> +                       dev_err(dev, "Failed to map inbound regions!\n");
>>> +                       return -EINVAL;
>>> +               }
>>> +
>>> +               winsize = size;
>>> +               if (size > cdns_rp_bar_max_size[bar])
>>> +                       winsize = cdns_rp_bar_max_size[bar];
>>> +
>>> +               aperture = ilog2(winsize);
>>> +
>>> +               cdns_pcie_host_bar_ib_config(rc, bar, cpu_addr, aperture);
>>> +
>>> +               if (bar == RP_NO_BAR)
>>> +                       break;
>>> +
>>> +               if (winsize + cpu_addr >= SZ_4G) {
>>> +                       if (!(flags & IORESOURCE_PREFETCH))
>>> +                               value |= LM_RC_BAR_CFG_CTRL_MEM_64BITS(bar);
>>> +                       value |= LM_RC_BAR_CFG_CTRL_PREF_MEM_64BITS(bar);
>>> +               } else {
>>> +                       if (!(flags & IORESOURCE_PREFETCH))
>>> +                               value |= LM_RC_BAR_CFG_CTRL_MEM_32BITS(bar);
>>> +                       value |= LM_RC_BAR_CFG_CTRL_PREF_MEM_32BITS(bar);
>>> +               }
>>> +
>>> +               value |= LM_RC_BAR_CFG_APERTURE(bar, aperture);
>>> +
>>> +               size -= winsize;
>>> +               cpu_addr += winsize;
>>> +               bar++;
>>> +       }
>>> +       cdns_pcie_writel(pcie, CDNS_PCIE_LM_RC_BAR_CFG, value);
>>> +       *index = bar;
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int cdns_pcie_host_map_dma_ranges(struct cdns_pcie_rc *rc)
>>> +{
>>> +       enum cdns_pcie_rp_bar bar = RP_BAR0;
>>> +       struct cdns_pcie *pcie = &rc->pcie;
>>> +       struct device *dev = pcie->dev;
>>> +       struct device_node *np = dev->of_node;
>>> +       struct pci_host_bridge *bridge;
>>> +       struct resource_entry *entry;
>>> +       u32 no_bar_nbits = 32;
>>> +       int err;
>>> +
>>> +       bridge = pci_host_bridge_from_priv(rc);
>>> +       if (!bridge)
>>> +               return -ENOMEM;
>>> +
>>> +       if (list_empty(&bridge->dma_ranges)) {
>>> +               of_property_read_u32(np, "cdns,no-bar-match-nbits",
>>> +                                    &no_bar_nbits);
>>> +               cdns_pcie_host_bar_ib_config(rc, RP_NO_BAR, 0x0, no_bar_nbits);
>>> +               return 0;
>>> +       }
>>> +
>>> +       resource_list_for_each_entry(entry, &bridge->dma_ranges) {
>>> +               err = cdns_pcie_host_bar_config(rc, entry, &bar);
>>
>> Seems like this should have some better logic to pick which BAR to
>> use. Something like find the biggest region and then find the smallest
>> BAR that it fits in. Then get the next biggest...
> 
> Okay, I'll change this something like for each region, find the smallest BAR
> that it fits in and if there is no BAR big enough to hold the region, split the
> region to see if can be fitted using multiple BARs. I don't see the purpose of
> finding the biggest region first since at all times we'll only use the smallest
> BAR to fit.

Nevermind, I realized finding the biggest region is useful. I have sent a patch
adding support for that.

Thanks
Kishon

Re: [PATCH v3 0/4] PCI: cadence: Deprecate inbound/outbound specific bindings

[Lorenzo Pieralisi]

On Fri, May 08, 2020 at 06:36:42PM +0530, Kishon Vijay Abraham I wrote:
> This series is a result of comments given by Rob Herring @ [1].
> Patch series changes the DT bindings and makes the corresponding driver
> changes.
> 
> Changes from v2:
> 1) Changed the order of patches (no solid reason. Just save some
> rebasing effort for me)
> 2) Added Acked-by Tom and Rob except for the dma-ranges patch
> 3) Re-worked dma-ranges patch for it do decode multiple dma-ranges
>    and configure BAR0, BAR1 and NO_BAR instead of just NO_BAR [2].
> 
> Changes from v1:
> 1) Added Reviewed-by: Rob Herring <robh@kernel.org> for dt-binding patch
> 2) Fixed nitpick comments from Bjorn Helgaas
> 3) Added a patch to read 32-bit Vendor ID/Device ID property from DT
> 
> [1] -> http://lore.kernel.org/r/20200219202700.GA21908@bogus
> [2] -> http://lore.kernel.org/r/eb1ffcb3-264f-5174-1f25-b5b2d3269840@ti.com
> 
> Kishon Vijay Abraham I (4):
>   dt-bindings: PCI: cadence: Deprecate inbound/outbound specific
>     bindings
>   PCI: cadence: Remove "cdns,max-outbound-regions" DT property
>   PCI: cadence: Fix to read 32-bit Vendor ID/Device ID property from DT
>   PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits"
>     property
> 
>  .../bindings/pci/cdns,cdns-pcie-ep.yaml       |   2 +-
>  .../bindings/pci/cdns,cdns-pcie-host.yaml     |   3 +-
>  .../devicetree/bindings/pci/cdns-pcie-ep.yaml |  25 +++
>  .../bindings/pci/cdns-pcie-host.yaml          |  10 ++
>  .../devicetree/bindings/pci/cdns-pcie.yaml    |   8 -
>  .../controller/cadence/pcie-cadence-host.c    | 151 +++++++++++++++---
>  drivers/pci/controller/cadence/pcie-cadence.h |  23 ++-
>  7 files changed, 182 insertions(+), 40 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/pci/cdns-pcie-ep.yaml

I have applied patches 1-3 to pci/cadence (that I think are
self-contained), waiting for the dma-ranges discussion to wrap up on
patch 4.

Lorenzo