[PATCH v2 0/5] PCI: Keystone: Misc fixes for TI's AM65x PCIe

[PATCH v2 0/5] PCI: Keystone: Misc fixes for TI's AM65x PCIe

[Kishon Vijay Abraham I]

Patch series includes miscellaneous fixes for TI's AM65x SoC
"PCI: keystone: Add workaround for Errata #i2037 (AM65x SR 1.0)"  has
already been sent before [1]

The other patch is to prevent PCIEPORTBUS driver to write to
MSI-X table (which is not mapped) leading to ~10sec delay
due to msix_mask_all().

v1 if the patch series is @ [2]

Changes from v1:
1) Added two patches to fix 'dtbs_check'; a DT binding documentation
update and a driver update.
2) Remove falling back to smaller DMA mask as suggested by Christoph.

[1] -> https://lore.kernel.org/r/20210325090026.8843-7-kishon@ti.com
[2] -> https://lore.kernel.org/r/20211110073343.12396-1-kishon@ti.com

Kishon Vijay Abraham I (5):
  dt-bindings: PCI: ti,am65: Fix
    "ti,syscon-pcie-id"/"ti,syscon-pcie-mode" to take argument
  PCI: keystone: Use phandle argument from
    "ti,syscon-pcie-id"/"ti,syscon-pcie-mode"
  PCI: keystone: Add workaround for Errata #i2037 (AM65x SR 1.0)
  PCI: keystone: Add quirk to mark AM654 RC BAR flag as IORESOURCE_UNSET
  PCI: keystone: Set DMA mask and coherent DMA mask

 .../bindings/pci/ti,am65-pci-ep.yaml          |  8 +-
 .../bindings/pci/ti,am65-pci-host.yaml        | 16 +++-
 drivers/pci/controller/dwc/pci-keystone.c     | 82 ++++++++++++++++++-
 3 files changed, 96 insertions(+), 10 deletions(-)

-- 
2.17.1

[PATCH v2 1/5] dt-bindings: PCI: ti,am65: Fix "ti,syscon-pcie-id"/"ti,syscon-pcie-mode" to take argument

[Kishon Vijay Abraham I]

Fix binding documentation of "ti,syscon-pcie-id" and "ti,syscon-pcie-mode"
to take phandle with argument. The argument is the register offset within
"syscon" used to configure PCIe controller. Similar change for j721e is
discussed in [1]

[1] -> http://lore.kernel.org/r/CAL_JsqKiUcO76bo1GoepWM1TusJWoty_BRy2hFSgtEVMqtrvvQ@mail.gmail.com

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 .../devicetree/bindings/pci/ti,am65-pci-ep.yaml  |  8 ++++++--
 .../bindings/pci/ti,am65-pci-host.yaml           | 16 ++++++++++++----
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-ep.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-ep.yaml
index 78c217d362a7..98d933b792e7 100644
--- a/Documentation/devicetree/bindings/pci/ti,am65-pci-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-ep.yaml
@@ -32,8 +32,12 @@ properties:
     maxItems: 1
 
   ti,syscon-pcie-mode:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - items:
+          - description: Phandle to the SYSCON entry
+          - description: pcie_ctrl register offset within SYSCON
     description: Phandle to the SYSCON entry required for configuring PCIe in RC or EP mode.
-    $ref: /schemas/types.yaml#/definitions/phandle
 
   interrupts:
     minItems: 1
@@ -65,7 +69,7 @@ examples:
                <0x5506000 0x1000>;
         reg-names = "app", "dbics", "addr_space", "atu";
         power-domains = <&k3_pds 120 TI_SCI_PD_EXCLUSIVE>;
-        ti,syscon-pcie-mode = <&pcie0_mode>;
+        ti,syscon-pcie-mode = <&scm_conf 0x4060>;
         num-ib-windows = <16>;
         num-ob-windows = <16>;
         max-link-speed = <2>;
diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
index 834dc1c1743c..f909e262f593 100644
--- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
+++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
@@ -33,12 +33,20 @@ properties:
     maxItems: 1
 
   ti,syscon-pcie-id:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - items:
+          - description: Phandle to the SYSCON entry
+          - description: pcie_device_id register offset within SYSCON
     description: Phandle to the SYSCON entry required for getting PCIe device/vendor ID
-    $ref: /schemas/types.yaml#/definitions/phandle
 
   ti,syscon-pcie-mode:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - items:
+          - description: Phandle to the SYSCON entry
+          - description: pcie_ctrl register offset within SYSCON
     description: Phandle to the SYSCON entry required for configuring PCIe in RC or EP mode.
-    $ref: /schemas/types.yaml#/definitions/phandle
 
   msi-map: true
 
@@ -84,8 +92,8 @@ examples:
         #size-cells = <2>;
         ranges = <0x81000000 0 0          0x10020000 0 0x00010000>,
                  <0x82000000 0 0x10030000 0x10030000 0 0x07FD0000>;
-        ti,syscon-pcie-id = <&pcie_devid>;
-        ti,syscon-pcie-mode = <&pcie0_mode>;
+        ti,syscon-pcie-id = <&scm_conf 0x0210>;
+        ti,syscon-pcie-mode = <&scm_conf 0x4060>;
         bus-range = <0x0 0xff>;
         num-viewport = <16>;
         max-link-speed = <2>;
-- 
2.17.1

[PATCH v2 2/5] PCI: keystone: Use phandle argument from "ti,syscon-pcie-id"/"ti,syscon-pcie-mode"

[Kishon Vijay Abraham I]

Get "syscon" pcie_mode and pcie_id offset from the argument of
"ti,syscon-pcie-id" and "ti,syscon-pcie-mode" phandle respectively.
Previously a subnode to "syscon" node was added which has the
exact memory mapped address of pcie_mode and pcie_id but now the
offset of pcie_mode and pcie_id within "syscon" is now being passed
as argument to "ti,syscon-pcie-id" and "ti,syscon-pcie-mode" phandle.

If the offset is not provided in "ti,syscon-pcie-id"/"ti,syscon-pcie-mode",
the full memory mapped address of pcie_ctrl is used in order to maintain
old DT compatibility.

Similar change for J721E is as discussed in [1]

[1] -> http://lore.kernel.org/r/CAL_JsqKiUcO76bo1GoepWM1TusJWoty_BRy2hFSgtEVMqtrvvQ@mail.gmail.com

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 27 ++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 865258d8c53c..13f03a97714c 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -775,12 +775,19 @@ static int __init ks_pcie_init_id(struct keystone_pcie *ks_pcie)
 	struct dw_pcie *pci = ks_pcie->pci;
 	struct device *dev = pci->dev;
 	struct device_node *np = dev->of_node;
+	struct of_phandle_args args;
+	unsigned int offset = 0;
 
 	devctrl_regs = syscon_regmap_lookup_by_phandle(np, "ti,syscon-pcie-id");
 	if (IS_ERR(devctrl_regs))
 		return PTR_ERR(devctrl_regs);
 
-	ret = regmap_read(devctrl_regs, 0, &id);
+	/* Do not error out to maintain old DT compatibility */
+	ret = of_parse_phandle_with_fixed_args(np, "ti,syscon-pcie-id", 1, 0, &args);
+	if (!ret)
+		offset = args.args[0];
+
+	ret = regmap_read(devctrl_regs, offset, &id);
 	if (ret)
 		return ret;
 
@@ -989,6 +996,8 @@ static int ks_pcie_enable_phy(struct keystone_pcie *ks_pcie)
 static int ks_pcie_set_mode(struct device *dev)
 {
 	struct device_node *np = dev->of_node;
+	struct of_phandle_args args;
+	unsigned int offset = 0;
 	struct regmap *syscon;
 	u32 val;
 	u32 mask;
@@ -998,10 +1007,15 @@ static int ks_pcie_set_mode(struct device *dev)
 	if (IS_ERR(syscon))
 		return 0;
 
+	/* Do not error out to maintain old DT compatibility */
+	ret = of_parse_phandle_with_fixed_args(np, "ti,syscon-pcie-mode", 1, 0, &args);
+	if (!ret)
+		offset = args.args[0];
+
 	mask = KS_PCIE_DEV_TYPE_MASK | KS_PCIE_SYSCLOCKOUTEN;
 	val = KS_PCIE_DEV_TYPE(RC) | KS_PCIE_SYSCLOCKOUTEN;
 
-	ret = regmap_update_bits(syscon, 0, mask, val);
+	ret = regmap_update_bits(syscon, offset, mask, val);
 	if (ret) {
 		dev_err(dev, "failed to set pcie mode\n");
 		return ret;
@@ -1014,6 +1028,8 @@ static int ks_pcie_am654_set_mode(struct device *dev,
 				  enum dw_pcie_device_mode mode)
 {
 	struct device_node *np = dev->of_node;
+	struct of_phandle_args args;
+	unsigned int offset = 0;
 	struct regmap *syscon;
 	u32 val;
 	u32 mask;
@@ -1023,6 +1039,11 @@ static int ks_pcie_am654_set_mode(struct device *dev,
 	if (IS_ERR(syscon))
 		return 0;
 
+	/* Do not error out to maintain old DT compatibility */
+	ret = of_parse_phandle_with_fixed_args(np, "ti,syscon-pcie-mode", 1, 0, &args);
+	if (!ret)
+		offset = args.args[0];
+
 	mask = AM654_PCIE_DEV_TYPE_MASK;
 
 	switch (mode) {
@@ -1037,7 +1058,7 @@ static int ks_pcie_am654_set_mode(struct device *dev,
 		return -EINVAL;
 	}
 
-	ret = regmap_update_bits(syscon, 0, mask, val);
+	ret = regmap_update_bits(syscon, offset, mask, val);
 	if (ret) {
 		dev_err(dev, "failed to set pcie mode\n");
 		return ret;
-- 
2.17.1

[PATCH v2 3/5] PCI: keystone: Add workaround for Errata #i2037 (AM65x SR 1.0)

[Kishon Vijay Abraham I]

Errata #i2037 in AM65x/DRA80xM Processors Silicon Revision 1.0
(SPRZ452D–July 2018–Revised December 2019 [1]) mentions when an
inbound PCIe TLP spans more than two internal AXI 128-byte bursts,
the bus may corrupt the packet payload and the corrupt data may
cause associated applications or the processor to hang.

The workaround for Errata #i2037 is to limit the maximum read
request size and maximum payload size to 128 bytes. Add workaround
for Errata #i2037 here. The errata and workaround is applicable
only to AM65x SR 1.0 and later versions of the silicon will have
this fixed.

[1] -> https://www.ti.com/lit/er/sprz452f/sprz452f.pdf

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 42 +++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 13f03a97714c..52d20fe17ee9 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -35,6 +35,11 @@
 #define PCIE_DEVICEID_SHIFT	16
 
 /* Application registers */
+#define PID				0x000
+#define RTL				GENMASK(15, 11)
+#define RTL_SHIFT			11
+#define AM6_PCI_PG1_RTL_VER		0x15
+
 #define CMD_STATUS			0x004
 #define LTSSM_EN_VAL		        BIT(0)
 #define OB_XLAT_EN_VAL		        BIT(1)
@@ -105,6 +110,8 @@
 
 #define to_keystone_pcie(x)		dev_get_drvdata((x)->dev)
 
+#define PCI_DEVICE_ID_TI_AM654X		0xb00c
+
 struct ks_pcie_of_data {
 	enum dw_pcie_device_mode mode;
 	const struct dw_pcie_host_ops *host_ops;
@@ -528,7 +535,11 @@ static int ks_pcie_start_link(struct dw_pcie *pci)
 static void ks_pcie_quirk(struct pci_dev *dev)
 {
 	struct pci_bus *bus = dev->bus;
+	struct keystone_pcie *ks_pcie;
+	struct device *bridge_dev;
 	struct pci_dev *bridge;
+	u32 val;
+
 	static const struct pci_device_id rc_pci_devids[] = {
 		{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCIE_RC_K2HK),
 		 .class = PCI_CLASS_BRIDGE_PCI << 8, .class_mask = ~0, },
@@ -540,6 +551,11 @@ static void ks_pcie_quirk(struct pci_dev *dev)
 		 .class = PCI_CLASS_BRIDGE_PCI << 8, .class_mask = ~0, },
 		{ 0, },
 	};
+	static const struct pci_device_id am6_pci_devids[] = {
+		{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_AM654X),
+		 .class = PCI_CLASS_BRIDGE_PCI << 8, .class_mask = ~0, },
+		{ 0, },
+	};
 
 	if (pci_is_root_bus(bus))
 		bridge = dev;
@@ -565,6 +581,32 @@ static void ks_pcie_quirk(struct pci_dev *dev)
 			pcie_set_readrq(dev, 256);
 		}
 	}
+
+	/*
+	 * Memory transactions fail with PCI controller in AM654 PG1.0
+	 * when MRRS is set to more than 128 bytes. Force the MRRS to
+	 * 128 bytes in all downstream devices.
+	 */
+	if (pci_match_id(am6_pci_devids, bridge)) {
+		bridge_dev = pci_get_host_bridge_device(dev);
+		if (!bridge_dev && !bridge_dev->parent)
+			return;
+
+		ks_pcie = dev_get_drvdata(bridge_dev->parent);
+		if (!ks_pcie)
+			return;
+
+		val = ks_pcie_app_readl(ks_pcie, PID);
+		val &= RTL;
+		val >>= RTL_SHIFT;
+		if (val != AM6_PCI_PG1_RTL_VER)
+			return;
+
+		if (pcie_get_readrq(dev) > 128) {
+			dev_info(&dev->dev, "limiting MRRS to 128 bytes\n");
+			pcie_set_readrq(dev, 128);
+		}
+	}
 }
 DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, ks_pcie_quirk);
 
-- 
2.17.1

[PATCH v2 4/5] PCI: keystone: Add quirk to mark AM654 RC BAR flag as IORESOURCE_UNSET

[Kishon Vijay Abraham I]

AM654 RootComplex has a hard coded 64 bit BAR of size 1MB and also has
both MSI and MSI-X capability in it's config space. If PCIEPORTBUS is
enabled, it tries to configure MSI-X and msix_mask_all() adds about 10
Second boot up delay when it tries to write to undefined location.

Add quirk to mark AM654 RC BAR flag as IORESOURCE_UNSET so that
msix_map_region() returns NULL for Root Complex and avoid un-desirable
writes to MSI-X table.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 52d20fe17ee9..73e6626a0d8f 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -557,8 +557,14 @@ static void ks_pcie_quirk(struct pci_dev *dev)
 		{ 0, },
 	};
 
-	if (pci_is_root_bus(bus))
+	if (pci_is_root_bus(bus)) {
 		bridge = dev;
+		if (pci_match_id(am6_pci_devids, bridge)) {
+			struct resource *r = &dev->resource[0];
+
+			r->flags |= IORESOURCE_UNSET;
+		}
+	}
 
 	/* look for the host bridge */
 	while (!pci_is_root_bus(bus)) {
-- 
2.17.1

[PATCH v2 5/5] PCI: keystone: Set DMA mask and coherent DMA mask

[Kishon Vijay Abraham I]

Set DMA mask and coherent DMA mask such to indicate the device
can address the entire address space (32-bit in the case of
K2G and 48-bit in the case of AM654).

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 73e6626a0d8f..80dbab267f7b 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -1224,6 +1224,11 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(48))) {
+		dev_err(dev, "Cannot set DMA mask\n");
+		return -EINVAL;
+	}
+
 	ret = of_property_read_u32(np, "num-lanes", &num_lanes);
 	if (ret)
 		num_lanes = 1;
-- 
2.17.1

Re: [PATCH v2 1/5] dt-bindings: PCI: ti,am65: Fix "ti,syscon-pcie-id"/"ti,syscon-pcie-mode" to take argument

[Rob Herring]

On Fri, 26 Nov 2021 14:01:15 +0530, Kishon Vijay Abraham I wrote:
> Fix binding documentation of "ti,syscon-pcie-id" and "ti,syscon-pcie-mode"
> to take phandle with argument. The argument is the register offset within
> "syscon" used to configure PCIe controller. Similar change for j721e is
> discussed in [1]
> 
> [1] -> http://lore.kernel.org/r/CAL_JsqKiUcO76bo1GoepWM1TusJWoty_BRy2hFSgtEVMqtrvvQ@mail.gmail.com
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  .../devicetree/bindings/pci/ti,am65-pci-ep.yaml  |  8 ++++++--
>  .../bindings/pci/ti,am65-pci-host.yaml           | 16 ++++++++++++----
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 

Running 'make dtbs_check' with the schema in this patch gives the
following warnings. Consider if they are expected or the schema is
incorrect. These may not be new warnings.

Note that it is not yet a requirement to have 0 warnings for dtbs_check.
This will change in the future.

Full log is available here: https://patchwork.ozlabs.org/patch/1559994


pcie@21020000: compatible: Additional items are not allowed ('snps,dw-pcie' was unexpected)
	arch/arm/boot/dts/keystone-k2e-evm.dt.yaml

pcie@21020000: compatible: ['ti,keystone-pcie', 'snps,dw-pcie'] is too long
	arch/arm/boot/dts/keystone-k2e-evm.dt.yaml

pcie@21020000: reg: [[553783296, 8192], [553779200, 4096], [39977256, 4]] is too short
	arch/arm/boot/dts/keystone-k2e-evm.dt.yaml

pcie@21800000: compatible: Additional items are not allowed ('snps,dw-pcie' was unexpected)
	arch/arm/boot/dts/keystone-k2e-evm.dt.yaml
	arch/arm/boot/dts/keystone-k2hk-evm.dt.yaml
	arch/arm/boot/dts/keystone-k2l-evm.dt.yaml

pcie@21800000: compatible: ['ti,keystone-pcie', 'snps,dw-pcie'] is too long
	arch/arm/boot/dts/keystone-k2e-evm.dt.yaml
	arch/arm/boot/dts/keystone-k2hk-evm.dt.yaml
	arch/arm/boot/dts/keystone-k2l-evm.dt.yaml

pcie@21800000: reg: [[562040832, 8192], [562036736, 4096], [39977256, 4]] is too short
	arch/arm/boot/dts/keystone-k2e-evm.dt.yaml
	arch/arm/boot/dts/keystone-k2hk-evm.dt.yaml
	arch/arm/boot/dts/keystone-k2l-evm.dt.yaml

pcie@5500000: ti,syscon-pcie-id:0: [52] is too short
	arch/arm64/boot/dts/ti/k3-am654-base-board.dt.yaml

pcie@5500000: ti,syscon-pcie-id:0: [60] is too short
	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dt.yaml
	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dt.yaml

pcie@5500000: ti,syscon-pcie-id:0: [61] is too short
	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dt.yaml
	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-pg2.dt.yaml

pcie@5500000: ti,syscon-pcie-mode:0: [53] is too short
	arch/arm64/boot/dts/ti/k3-am654-base-board.dt.yaml

pcie@5500000: ti,syscon-pcie-mode:0: [61] is too short
	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dt.yaml
	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dt.yaml

pcie@5500000: ti,syscon-pcie-mode:0: [62] is too short
	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dt.yaml
	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-pg2.dt.yaml

pcie@5600000: ti,syscon-pcie-id:0: [52] is too short
	arch/arm64/boot/dts/ti/k3-am654-base-board.dt.yaml

pcie@5600000: ti,syscon-pcie-id:0: [60] is too short
	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dt.yaml
	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dt.yaml

pcie@5600000: ti,syscon-pcie-id:0: [61] is too short
	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dt.yaml
	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-pg2.dt.yaml

pcie@5600000: ti,syscon-pcie-mode:0: [55] is too short
	arch/arm64/boot/dts/ti/k3-am654-base-board.dt.yaml

pcie@5600000: ti,syscon-pcie-mode:0: [63] is too short
	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dt.yaml
	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dt.yaml

pcie@5600000: ti,syscon-pcie-mode:0: [64] is too short
	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dt.yaml
	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-pg2.dt.yaml

pcie-ep@5500000: ti,syscon-pcie-mode:0: [53] is too short
	arch/arm64/boot/dts/ti/k3-am654-base-board.dt.yaml

pcie-ep@5500000: ti,syscon-pcie-mode:0: [61] is too short
	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dt.yaml
	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dt.yaml

pcie-ep@5500000: ti,syscon-pcie-mode:0: [62] is too short
	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dt.yaml
	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-pg2.dt.yaml

pcie-ep@5600000: ti,syscon-pcie-mode:0: [55] is too short
	arch/arm64/boot/dts/ti/k3-am654-base-board.dt.yaml

pcie-ep@5600000: ti,syscon-pcie-mode:0: [63] is too short
	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dt.yaml
	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dt.yaml

pcie-ep@5600000: ti,syscon-pcie-mode:0: [64] is too short
	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dt.yaml
	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-pg2.dt.yaml

Re: [PATCH v2 1/5] dt-bindings: PCI: ti,am65: Fix "ti,syscon-pcie-id"/"ti,syscon-pcie-mode" to take argument

[Kishon Vijay Abraham I]

Hi Rob,

On 28/11/21 4:43 am, Rob Herring wrote:
> On Fri, 26 Nov 2021 14:01:15 +0530, Kishon Vijay Abraham I wrote:
>> Fix binding documentation of "ti,syscon-pcie-id" and "ti,syscon-pcie-mode"
>> to take phandle with argument. The argument is the register offset within
>> "syscon" used to configure PCIe controller. Similar change for j721e is
>> discussed in [1]
>>
>> [1] -> http://lore.kernel.org/r/CAL_JsqKiUcO76bo1GoepWM1TusJWoty_BRy2hFSgtEVMqtrvvQ@mail.gmail.com
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  .../devicetree/bindings/pci/ti,am65-pci-ep.yaml  |  8 ++++++--
>>  .../bindings/pci/ti,am65-pci-host.yaml           | 16 ++++++++++++----
>>  2 files changed, 18 insertions(+), 6 deletions(-)
>>
> 
> Running 'make dtbs_check' with the schema in this patch gives the
> following warnings. Consider if they are expected or the schema is
> incorrect. These may not be new warnings.
> 
> Note that it is not yet a requirement to have 0 warnings for dtbs_check.
> This will change in the future.

Once this series is merged, I'll send an update to the device tree files.
Without the corresponding driver changes, update to DT will break functionality.

Thanks,
Kishon
> 
> Full log is available here: https://patchwork.ozlabs.org/patch/1559994
> 
> 
> pcie@21020000: compatible: Additional items are not allowed ('snps,dw-pcie' was unexpected)
> 	arch/arm/boot/dts/keystone-k2e-evm.dt.yaml
> 
> pcie@21020000: compatible: ['ti,keystone-pcie', 'snps,dw-pcie'] is too long
> 	arch/arm/boot/dts/keystone-k2e-evm.dt.yaml
> 
> pcie@21020000: reg: [[553783296, 8192], [553779200, 4096], [39977256, 4]] is too short
> 	arch/arm/boot/dts/keystone-k2e-evm.dt.yaml
> 
> pcie@21800000: compatible: Additional items are not allowed ('snps,dw-pcie' was unexpected)
> 	arch/arm/boot/dts/keystone-k2e-evm.dt.yaml
> 	arch/arm/boot/dts/keystone-k2hk-evm.dt.yaml
> 	arch/arm/boot/dts/keystone-k2l-evm.dt.yaml
> 
> pcie@21800000: compatible: ['ti,keystone-pcie', 'snps,dw-pcie'] is too long
> 	arch/arm/boot/dts/keystone-k2e-evm.dt.yaml
> 	arch/arm/boot/dts/keystone-k2hk-evm.dt.yaml
> 	arch/arm/boot/dts/keystone-k2l-evm.dt.yaml
> 
> pcie@21800000: reg: [[562040832, 8192], [562036736, 4096], [39977256, 4]] is too short
> 	arch/arm/boot/dts/keystone-k2e-evm.dt.yaml
> 	arch/arm/boot/dts/keystone-k2hk-evm.dt.yaml
> 	arch/arm/boot/dts/keystone-k2l-evm.dt.yaml
> 
> pcie@5500000: ti,syscon-pcie-id:0: [52] is too short
> 	arch/arm64/boot/dts/ti/k3-am654-base-board.dt.yaml
> 
> pcie@5500000: ti,syscon-pcie-id:0: [60] is too short
> 	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dt.yaml
> 	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dt.yaml
> 
> pcie@5500000: ti,syscon-pcie-id:0: [61] is too short
> 	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dt.yaml
> 	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-pg2.dt.yaml
> 
> pcie@5500000: ti,syscon-pcie-mode:0: [53] is too short
> 	arch/arm64/boot/dts/ti/k3-am654-base-board.dt.yaml
> 
> pcie@5500000: ti,syscon-pcie-mode:0: [61] is too short
> 	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dt.yaml
> 	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dt.yaml
> 
> pcie@5500000: ti,syscon-pcie-mode:0: [62] is too short
> 	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dt.yaml
> 	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-pg2.dt.yaml
> 
> pcie@5600000: ti,syscon-pcie-id:0: [52] is too short
> 	arch/arm64/boot/dts/ti/k3-am654-base-board.dt.yaml
> 
> pcie@5600000: ti,syscon-pcie-id:0: [60] is too short
> 	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dt.yaml
> 	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dt.yaml
> 
> pcie@5600000: ti,syscon-pcie-id:0: [61] is too short
> 	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dt.yaml
> 	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-pg2.dt.yaml
> 
> pcie@5600000: ti,syscon-pcie-mode:0: [55] is too short
> 	arch/arm64/boot/dts/ti/k3-am654-base-board.dt.yaml
> 
> pcie@5600000: ti,syscon-pcie-mode:0: [63] is too short
> 	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dt.yaml
> 	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dt.yaml
> 
> pcie@5600000: ti,syscon-pcie-mode:0: [64] is too short
> 	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dt.yaml
> 	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-pg2.dt.yaml
> 
> pcie-ep@5500000: ti,syscon-pcie-mode:0: [53] is too short
> 	arch/arm64/boot/dts/ti/k3-am654-base-board.dt.yaml
> 
> pcie-ep@5500000: ti,syscon-pcie-mode:0: [61] is too short
> 	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dt.yaml
> 	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dt.yaml
> 
> pcie-ep@5500000: ti,syscon-pcie-mode:0: [62] is too short
> 	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dt.yaml
> 	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-pg2.dt.yaml
> 
> pcie-ep@5600000: ti,syscon-pcie-mode:0: [55] is too short
> 	arch/arm64/boot/dts/ti/k3-am654-base-board.dt.yaml
> 
> pcie-ep@5600000: ti,syscon-pcie-mode:0: [63] is too short
> 	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dt.yaml
> 	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dt.yaml
> 
> pcie-ep@5600000: ti,syscon-pcie-mode:0: [64] is too short
> 	arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dt.yaml
> 	arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-pg2.dt.yaml
> 

Re: [PATCH v2 1/5] dt-bindings: PCI: ti,am65: Fix "ti,syscon-pcie-id"/"ti,syscon-pcie-mode" to take argument

[Rob Herring]

On Fri, 26 Nov 2021 14:01:15 +0530, Kishon Vijay Abraham I wrote:
> Fix binding documentation of "ti,syscon-pcie-id" and "ti,syscon-pcie-mode"
> to take phandle with argument. The argument is the register offset within
> "syscon" used to configure PCIe controller. Similar change for j721e is
> discussed in [1]
> 
> [1] -> http://lore.kernel.org/r/CAL_JsqKiUcO76bo1GoepWM1TusJWoty_BRy2hFSgtEVMqtrvvQ@mail.gmail.com
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  .../devicetree/bindings/pci/ti,am65-pci-ep.yaml  |  8 ++++++--
>  .../bindings/pci/ti,am65-pci-host.yaml           | 16 ++++++++++++----
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 0/5] PCI: Keystone: Misc fixes for TI's AM65x PCIe

[Kishon Vijay Abraham I]

Hi Lorenzo,

On 26/11/21 2:01 pm, Kishon Vijay Abraham I wrote:
> Patch series includes miscellaneous fixes for TI's AM65x SoC
> "PCI: keystone: Add workaround for Errata #i2037 (AM65x SR 1.0)"  has
> already been sent before [1]
> 
> The other patch is to prevent PCIEPORTBUS driver to write to
> MSI-X table (which is not mapped) leading to ~10sec delay
> due to msix_mask_all().
> 
> v1 if the patch series is @ [2]
> 
> Changes from v1:
> 1) Added two patches to fix 'dtbs_check'; a DT binding documentation
> update and a driver update.
> 2) Remove falling back to smaller DMA mask as suggested by Christoph.
> 
> [1] -> https://lore.kernel.org/r/20210325090026.8843-7-kishon@ti.com
> [2] -> https://lore.kernel.org/r/20211110073343.12396-1-kishon@ti.com
> 
> Kishon Vijay Abraham I (5):
>   dt-bindings: PCI: ti,am65: Fix
>     "ti,syscon-pcie-id"/"ti,syscon-pcie-mode" to take argument
>   PCI: keystone: Use phandle argument from
>     "ti,syscon-pcie-id"/"ti,syscon-pcie-mode"
>   PCI: keystone: Add workaround for Errata #i2037 (AM65x SR 1.0)
>   PCI: keystone: Add quirk to mark AM654 RC BAR flag as IORESOURCE_UNSET
>   PCI: keystone: Set DMA mask and coherent DMA mask
> 
>  .../bindings/pci/ti,am65-pci-ep.yaml          |  8 +-
>  .../bindings/pci/ti,am65-pci-host.yaml        | 16 +++-
>  drivers/pci/controller/dwc/pci-keystone.c     | 82 ++++++++++++++++++-
>  3 files changed, 96 insertions(+), 10 deletions(-)

Can this be merged?

Regards,
Kishon

Re: [PATCH v2 4/5] PCI: keystone: Add quirk to mark AM654 RC BAR flag as IORESOURCE_UNSET

[Lorenzo Pieralisi]

On Fri, Nov 26, 2021 at 02:01:18PM +0530, Kishon Vijay Abraham I wrote:
> AM654 RootComplex has a hard coded 64 bit BAR of size 1MB and also has
> both MSI and MSI-X capability in it's config space. If PCIEPORTBUS is
> enabled, it tries to configure MSI-X and msix_mask_all() adds about 10
> Second boot up delay when it tries to write to undefined location.
> 
> Add quirk to mark AM654 RC BAR flag as IORESOURCE_UNSET so that
> msix_map_region() returns NULL for Root Complex and avoid un-desirable
> writes to MSI-X table.

I don't think this is the right fix (it is not even a fix, just a
plaster to workaround an issue).

What do you mean by "writing to an undefined location" ?

What does "a hard coded BAR" mean ?

What happens if we _rightly_ write into it (ie to size it) ?

Lorenzo

> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/dwc/pci-keystone.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index 52d20fe17ee9..73e6626a0d8f 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -557,8 +557,14 @@ static void ks_pcie_quirk(struct pci_dev *dev)
>  		{ 0, },
>  	};
>  
> -	if (pci_is_root_bus(bus))
> +	if (pci_is_root_bus(bus)) {
>  		bridge = dev;
> +		if (pci_match_id(am6_pci_devids, bridge)) {
> +			struct resource *r = &dev->resource[0];
> +
> +			r->flags |= IORESOURCE_UNSET;
> +		}
> +	}
>  
>  	/* look for the host bridge */
>  	while (!pci_is_root_bus(bus)) {
> -- 
> 2.17.1
> 

Re: [PATCH v2 4/5] PCI: keystone: Add quirk to mark AM654 RC BAR flag as IORESOURCE_UNSET

[Rob Herring]

On Fri, Nov 26, 2021 at 2:31 AM Kishon Vijay Abraham I <kishon@ti.com> wrote:
>
> AM654 RootComplex has a hard coded 64 bit BAR of size 1MB and also has
> both MSI and MSI-X capability in it's config space. If PCIEPORTBUS is
> enabled, it tries to configure MSI-X and msix_mask_all() adds about 10
> Second boot up delay when it tries to write to undefined location.
>
> Add quirk to mark AM654 RC BAR flag as IORESOURCE_UNSET so that
> msix_map_region() returns NULL for Root Complex and avoid un-desirable
> writes to MSI-X table.
>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/dwc/pci-keystone.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index 52d20fe17ee9..73e6626a0d8f 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -557,8 +557,14 @@ static void ks_pcie_quirk(struct pci_dev *dev)
>                 { 0, },
>         };
>
> -       if (pci_is_root_bus(bus))
> +       if (pci_is_root_bus(bus)) {

The existing quirk has to be called for every device. But this quirk
applies to just the RC, so can't you add another quirk and use the
existing quirk infrastructure matching mechanism?

>                 bridge = dev;
> +               if (pci_match_id(am6_pci_devids, bridge)) {
> +                       struct resource *r = &dev->resource[0];
> +
> +                       r->flags |= IORESOURCE_UNSET;
> +               }
> +       }
>
>         /* look for the host bridge */
>         while (!pci_is_root_bus(bus)) {
> --
> 2.17.1
>

Re: (subset) [PATCH v2 0/5] PCI: Keystone: Misc fixes for TI's AM65x PCIe

[Lorenzo Pieralisi]

On Fri, 26 Nov 2021 14:01:14 +0530, Kishon Vijay Abraham I wrote:
> Patch series includes miscellaneous fixes for TI's AM65x SoC
> "PCI: keystone: Add workaround for Errata #i2037 (AM65x SR 1.0)"  has
> already been sent before [1]
> 
> The other patch is to prevent PCIEPORTBUS driver to write to
> MSI-X table (which is not mapped) leading to ~10sec delay
> due to msix_mask_all().
> 
> [...]

Applied (patches 1 and 2) to pci/keystone, thanks!

[1/5] dt-bindings: PCI: ti,am65: Fix "ti,syscon-pcie-id"/"ti,syscon-pcie-mode" to take argument
      https://git.kernel.org/lpieralisi/pci/c/d91e775e66
[2/5] PCI: keystone: Use phandle argument from "ti,syscon-pcie-id"/"ti,syscon-pcie-mode"
      https://git.kernel.org/lpieralisi/pci/c/7dcf07ac88

Thanks,
Lorenzo

Re: [PATCH v2 4/5] PCI: keystone: Add quirk to mark AM654 RC BAR flag as IORESOURCE_UNSET

[Kishon Vijay Abraham I]

Hi Lorenzo,

On 04/01/22 9:27 pm, Lorenzo Pieralisi wrote:
> On Fri, Nov 26, 2021 at 02:01:18PM +0530, Kishon Vijay Abraham I wrote:
>> AM654 RootComplex has a hard coded 64 bit BAR of size 1MB and also has
>> both MSI and MSI-X capability in it's config space. If PCIEPORTBUS is
>> enabled, it tries to configure MSI-X and msix_mask_all() adds about 10
>> Second boot up delay when it tries to write to undefined location.
>>
>> Add quirk to mark AM654 RC BAR flag as IORESOURCE_UNSET so that
>> msix_map_region() returns NULL for Root Complex and avoid un-desirable
>> writes to MSI-X table.
> 
> I don't think this is the right fix (it is not even a fix, just a
> plaster to workaround an issue).
> 
> What do you mean by "writing to an undefined location" ?
> 
> What does "a hard coded BAR" mean ?
> 
> What happens if we _rightly_ write into it (ie to size it) ?

There are two parts w.r.t setting the BAR; one is during the configuration and
the other is during the enumeration.
i) During the configuration, the size of the BAR is configured and the inbound
ATU is configured to map the BAR to a physical memory.
ii) During the enumeration, the size of the BAR is obtained and an address is
allocated and programmed in the BAR.

In the case of RC, for (i) above, the BAR size is configured as '0'
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-designware-host.c#n556
and the inbound ATU is not programmed at all.

However, in the case of AM654, the HW configures BAR0 for a fixed size of 1MB
(irrespective of what SW programmed in [i]). While this was done more for a
endpoint usecase, since the same IP is configured for both RC mode and EP mode,
the fixed BAR size is seen with RC mode as well. AM654 also has MSI-X capability
for RC mode (the IP should have been ideally configured to have MSI-X capability
for EP mode). This results in PCIEPORTBUS doing some undesired access in
msix_mask_all().

Here I configure IORESOURCE_UNSET so that memory is not allocated for RC BAR.

Thank You,
Kishon


> 
> Lorenzo
> 
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/pci/controller/dwc/pci-keystone.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
>> index 52d20fe17ee9..73e6626a0d8f 100644
>> --- a/drivers/pci/controller/dwc/pci-keystone.c
>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
>> @@ -557,8 +557,14 @@ static void ks_pcie_quirk(struct pci_dev *dev)
>>  		{ 0, },
>>  	};
>>  
>> -	if (pci_is_root_bus(bus))
>> +	if (pci_is_root_bus(bus)) {
>>  		bridge = dev;
>> +		if (pci_match_id(am6_pci_devids, bridge)) {
>> +			struct resource *r = &dev->resource[0];
>> +
>> +			r->flags |= IORESOURCE_UNSET;
>> +		}
>> +	}
>>  
>>  	/* look for the host bridge */
>>  	while (!pci_is_root_bus(bus)) {
>> -- 
>> 2.17.1
>>

Re: [PATCH v2 4/5] PCI: keystone: Add quirk to mark AM654 RC BAR flag as IORESOURCE_UNSET

[Kishon Vijay Abraham I]

Hi Lorenzo,

On 11/01/22 11:53 am, Kishon Vijay Abraham I wrote:
> Hi Lorenzo,
> 
> On 04/01/22 9:27 pm, Lorenzo Pieralisi wrote:
>> On Fri, Nov 26, 2021 at 02:01:18PM +0530, Kishon Vijay Abraham I wrote:
>>> AM654 RootComplex has a hard coded 64 bit BAR of size 1MB and also has
>>> both MSI and MSI-X capability in it's config space. If PCIEPORTBUS is
>>> enabled, it tries to configure MSI-X and msix_mask_all() adds about 10
>>> Second boot up delay when it tries to write to undefined location.
>>>
>>> Add quirk to mark AM654 RC BAR flag as IORESOURCE_UNSET so that
>>> msix_map_region() returns NULL for Root Complex and avoid un-desirable
>>> writes to MSI-X table.
>>
>> I don't think this is the right fix (it is not even a fix, just a
>> plaster to workaround an issue).
>>
>> What do you mean by "writing to an undefined location" ?
>>
>> What does "a hard coded BAR" mean ?
>>
>> What happens if we _rightly_ write into it (ie to size it) ?
> 
> There are two parts w.r.t setting the BAR; one is during the configuration and
> the other is during the enumeration.
> i) During the configuration, the size of the BAR is configured and the inbound
> ATU is configured to map the BAR to a physical memory.
> ii) During the enumeration, the size of the BAR is obtained and an address is
> allocated and programmed in the BAR.
> 
> In the case of RC, for (i) above, the BAR size is configured as '0'
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-designware-host.c#n556
> and the inbound ATU is not programmed at all.
> 
> However, in the case of AM654, the HW configures BAR0 for a fixed size of 1MB
> (irrespective of what SW programmed in [i]). While this was done more for a
> endpoint usecase, since the same IP is configured for both RC mode and EP mode,
> the fixed BAR size is seen with RC mode as well. AM654 also has MSI-X capability
> for RC mode (the IP should have been ideally configured to have MSI-X capability
> for EP mode). This results in PCIEPORTBUS doing some undesired access in
> msix_mask_all().
> 
> Here I configure IORESOURCE_UNSET so that memory is not allocated for RC BAR.

Do you need further clarifications on this?

Thank You,
Kishon

> 
> Thank You,
> Kishon
> 
> 
>>
>> Lorenzo
>>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> ---
>>>  drivers/pci/controller/dwc/pci-keystone.c | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
>>> index 52d20fe17ee9..73e6626a0d8f 100644
>>> --- a/drivers/pci/controller/dwc/pci-keystone.c
>>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
>>> @@ -557,8 +557,14 @@ static void ks_pcie_quirk(struct pci_dev *dev)
>>>  		{ 0, },
>>>  	};
>>>  
>>> -	if (pci_is_root_bus(bus))
>>> +	if (pci_is_root_bus(bus)) {
>>>  		bridge = dev;
>>> +		if (pci_match_id(am6_pci_devids, bridge)) {
>>> +			struct resource *r = &dev->resource[0];
>>> +
>>> +			r->flags |= IORESOURCE_UNSET;
>>> +		}
>>> +	}
>>>  
>>>  	/* look for the host bridge */
>>>  	while (!pci_is_root_bus(bus)) {
>>> -- 
>>> 2.17.1
>>>

Re: [PATCH v2 4/5] PCI: keystone: Add quirk to mark AM654 RC BAR flag as IORESOURCE_UNSET

[Lorenzo Pieralisi]

On Fri, Feb 04, 2022 at 08:38:46PM +0530, Kishon Vijay Abraham I wrote:
> Hi Lorenzo,
> 
> On 11/01/22 11:53 am, Kishon Vijay Abraham I wrote:
> > Hi Lorenzo,
> > 
> > On 04/01/22 9:27 pm, Lorenzo Pieralisi wrote:
> >> On Fri, Nov 26, 2021 at 02:01:18PM +0530, Kishon Vijay Abraham I wrote:
> >>> AM654 RootComplex has a hard coded 64 bit BAR of size 1MB and also has
> >>> both MSI and MSI-X capability in it's config space. If PCIEPORTBUS is
> >>> enabled, it tries to configure MSI-X and msix_mask_all() adds about 10
> >>> Second boot up delay when it tries to write to undefined location.
> >>>
> >>> Add quirk to mark AM654 RC BAR flag as IORESOURCE_UNSET so that
> >>> msix_map_region() returns NULL for Root Complex and avoid un-desirable
> >>> writes to MSI-X table.
> >>
> >> I don't think this is the right fix (it is not even a fix, just a
> >> plaster to workaround an issue).
> >>
> >> What do you mean by "writing to an undefined location" ?
> >>
> >> What does "a hard coded BAR" mean ?
> >>
> >> What happens if we _rightly_ write into it (ie to size it) ?
> > 
> > There are two parts w.r.t setting the BAR; one is during the configuration and
> > the other is during the enumeration.
> > i) During the configuration, the size of the BAR is configured and the inbound
> > ATU is configured to map the BAR to a physical memory.
> > ii) During the enumeration, the size of the BAR is obtained and an address is
> > allocated and programmed in the BAR.
> > 
> > In the case of RC, for (i) above, the BAR size is configured as '0'
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-designware-host.c#n556
> > and the inbound ATU is not programmed at all.
> > 
> > However, in the case of AM654, the HW configures BAR0 for a fixed size of 1MB
> > (irrespective of what SW programmed in [i]). While this was done more for a
> > endpoint usecase, since the same IP is configured for both RC mode and EP mode,
> > the fixed BAR size is seen with RC mode as well. AM654 also has MSI-X capability
> > for RC mode (the IP should have been ideally configured to have MSI-X capability
> > for EP mode). This results in PCIEPORTBUS doing some undesired access in
> > msix_mask_all().
> > 
> > Here I configure IORESOURCE_UNSET so that memory is not allocated for RC BAR.
> 
> Do you need further clarifications on this?

There are two things here:

1) As Rob mentioned, you can write it as a quirk applying only to the
   bridge _only_
2) What you want is that the BAR should not be visible to the OS since
   it is not an actual resource. What I am questioning is whether your
   way of doing that complies with how this is done in the kernel for
   other devices/bridges. I need Bjorn's input on this since he knows
   better (especially wrt IORESOURCE_UNSET usage). I don't want to add
   any other IORESOURCE_UNSET usage that deviates from what's expected
   from it

Lorenzo

> > 
> >>
> >> Lorenzo
> >>
> >>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >>> ---
> >>>  drivers/pci/controller/dwc/pci-keystone.c | 8 +++++++-
> >>>  1 file changed, 7 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> >>> index 52d20fe17ee9..73e6626a0d8f 100644
> >>> --- a/drivers/pci/controller/dwc/pci-keystone.c
> >>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> >>> @@ -557,8 +557,14 @@ static void ks_pcie_quirk(struct pci_dev *dev)
> >>>  		{ 0, },
> >>>  	};
> >>>  
> >>> -	if (pci_is_root_bus(bus))
> >>> +	if (pci_is_root_bus(bus)) {
> >>>  		bridge = dev;
> >>> +		if (pci_match_id(am6_pci_devids, bridge)) {
> >>> +			struct resource *r = &dev->resource[0];
> >>> +
> >>> +			r->flags |= IORESOURCE_UNSET;
> >>> +		}
> >>> +	}
> >>>  
> >>>  	/* look for the host bridge */
> >>>  	while (!pci_is_root_bus(bus)) {
> >>> -- 
> >>> 2.17.1
> >>>

Re: [PATCH v2 4/5] PCI: keystone: Add quirk to mark AM654 RC BAR flag as IORESOURCE_UNSET

[Bjorn Helgaas]

On Tue, Feb 08, 2022 at 11:53:44AM +0000, Lorenzo Pieralisi wrote:
> On Fri, Feb 04, 2022 at 08:38:46PM +0530, Kishon Vijay Abraham I wrote:
> > On 11/01/22 11:53 am, Kishon Vijay Abraham I wrote:
> > > On 04/01/22 9:27 pm, Lorenzo Pieralisi wrote:
> > >> On Fri, Nov 26, 2021 at 02:01:18PM +0530, Kishon Vijay Abraham I wrote:
> > >>> AM654 RootComplex has a hard coded 64 bit BAR of size 1MB and
> > >>> also has both MSI and MSI-X capability in it's config space.
> > >>> If PCIEPORTBUS is enabled, it tries to configure MSI-X and
> > >>> msix_mask_all() adds about 10 Second boot up delay when it
> > >>> tries to write to undefined location.

s/AM654 RootComplex/The AM654 Root Complex/

But Root Complexes are not normally materialized as PCI devices with
their own bus/device/function address, config space, BARs, etc.
Sounds like this might really be a Root *Port*, not a Root Complex?

s/it's config/its config/
s/10 Second/10 second/

> > >>> Add quirk to mark AM654 RC BAR flag as IORESOURCE_UNSET so
> > >>> that msix_map_region() returns NULL for Root Complex and avoid
> > >>> un-desirable writes to MSI-X table.
> > >>
> > >> I don't think this is the right fix (it is not even a fix, just
> > >> a plaster to workaround an issue).
> > >>
> > >> What do you mean by "writing to an undefined location" ?
> > >>
> > >> What does "a hard coded BAR" mean ?
> > >>
> > >> What happens if we _rightly_ write into it (ie to size it) ?
> > > 
> > > There are two parts w.r.t setting the BAR; one is during the
> > > configuration and the other is during the enumeration.
> > > i) During the configuration, the size of the BAR is configured
> > >    and the inbound ATU is configured to map the BAR to a
> > >    physical memory.
> > > ii) During the enumeration, the size of the BAR is obtained and
> > >     an address is allocated and programmed in the BAR.
> > > 
> > > In the case of RC, for (i) above, the BAR size is configured as '0'
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-designware-host.c#n556
> > > and the inbound ATU is not programmed at all.
> > > 
> > > However, in the case of AM654, the HW configures BAR0 for a
> > > fixed size of 1MB (irrespective of what SW programmed in [i]).
> > > While this was done more for a endpoint usecase, since the same
> > > IP is configured for both RC mode and EP mode, the fixed BAR
> > > size is seen with RC mode as well.

This doesn't seem to quite answer Lorenzo's question.  On AM654, does
__pci_read_base() discover the 1MB size correctly for both RC and EP
mode?

What value should BAR0 contain in RC mode?  Does the device respond at
that address?  Do we still need to ensure that 1MB address space is
not assigned to any other device?

> > > AM654 also has MSI-X capability for RC mode (the IP should have
> > > been ideally configured to have MSI-X capability for EP mode).
> > > This results in PCIEPORTBUS doing some undesired access in
> > > msix_mask_all().

We have several quirks that set dev->no_msi.  Maybe that would be a
better way to prevent use of MSI-X (it would also prevent use of MSI;
not sure whether that needs to be avoided as well)?

> > > Here I configure IORESOURCE_UNSET so that memory is not
> > > allocated for RC BAR.

I guess this implies that this device (RP?  I don't think RCs have
BARs) never responds to PCI address space described by BAR0?

> > Do you need further clarifications on this?
> 
> There are two things here:
> 
> 1) As Rob mentioned, you can write it as a quirk applying only to
>    the bridge _only_
> 2) What you want is that the BAR should not be visible to the OS
>    since it is not an actual resource. What I am questioning is
>    whether your way of doing that complies with how this is done in
>    the kernel for other devices/bridges. I need Bjorn's input on
>    this since he knows better (especially wrt IORESOURCE_UNSET
>    usage). I don't want to add any other IORESOURCE_UNSET usage that
>    deviates from what's expected from it

If BAR0 should not exist at all as far as the OS is concerned, I think
you should just set r->flags = 0.  But that assumes the device never
uses the value in the BAR, and it never responds using whatever
address is there, even when PCI_COMMAND_MEMORY is set.

> > >>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> > >>> ---
> > >>>  drivers/pci/controller/dwc/pci-keystone.c | 8 +++++++-
> > >>>  1 file changed, 7 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> > >>> index 52d20fe17ee9..73e6626a0d8f 100644
> > >>> --- a/drivers/pci/controller/dwc/pci-keystone.c
> > >>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> > >>> @@ -557,8 +557,14 @@ static void ks_pcie_quirk(struct pci_dev *dev)
> > >>>  		{ 0, },
> > >>>  	};
> > >>>  
> > >>> -	if (pci_is_root_bus(bus))
> > >>> +	if (pci_is_root_bus(bus)) {
> > >>>  		bridge = dev;
> > >>> +		if (pci_match_id(am6_pci_devids, bridge)) {
> > >>> +			struct resource *r = &dev->resource[0];
> > >>> +
> > >>> +			r->flags |= IORESOURCE_UNSET;
> > >>> +		}
> > >>> +	}
> > >>>  
> > >>>  	/* look for the host bridge */
> > >>>  	while (!pci_is_root_bus(bus)) {
> > >>> -- 
> > >>> 2.17.1
> > >>>

Re: [PATCH v2 5/5] PCI: keystone: Set DMA mask and coherent DMA mask

[Christian Gmeiner]

Hi all.

Am So., 28. Nov. 2021 um 18:12 Uhr schrieb Kishon Vijay Abraham I
<kishon@ti.com>:
>
> Set DMA mask and coherent DMA mask such to indicate the device
> can address the entire address space (32-bit in the case of
> K2G and 48-bit in the case of AM654).
>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/dwc/pci-keystone.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index 73e6626a0d8f..80dbab267f7b 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -1224,6 +1224,11 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
>                 return ret;
>         }
>
> +       if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(48))) {
> +               dev_err(dev, "Cannot set DMA mask\n");
> +               return -EINVAL;
> +       }
> +
>         ret = of_property_read_u32(np, "num-lanes", &num_lanes);
>         if (ret)
>                 num_lanes = 1;
> --
> 2.17.1
>

Can this single patch (from the whole series) be merged?

-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info/privacypolicy